Skip to content

fix: guardrails for secrets marked for deletion#2

Merged
gusfcarvalho merged 2 commits into
mainfrom
gc-fix-marked-for-deletion-guardrails
Jun 1, 2026
Merged

fix: guardrails for secrets marked for deletion#2
gusfcarvalho merged 2 commits into
mainfrom
gc-fix-marked-for-deletion-guardrails

Conversation

@gusfcarvalho

@gusfcarvalho gusfcarvalho commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling of AWS Secrets Manager secrets marked for deletion to avoid failed API calls and unnecessary follow-up operations on pending-deletion resources.
  • Documentation

    • Updated docs to explain how the collector records deletion metadata and treats secrets that are pending deletion.
  • Tests

    • Enhanced test coverage for deleted-secret scenarios with stronger assertions ensuring no forbidden follow-up calls occur and versions are represented as empty when applicable.

Signed-off-by: Gustavo Carvalho <gustavo.carvalho@container-solutions.com>
Copilot AI review requested due to automatic review settings June 1, 2026 11:41
@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 61c7a55c-98a1-4f47-966e-aad7c5749c35

📥 Commits

Reviewing files that changed from the base of the PR and between e5353fe and 3947059.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (1)
  • go.mod

📝 Walkthrough

Walkthrough

This change makes the collector return safe defaults for Secrets Manager secrets with a DeletedDate—skipping GetResourcePolicy and ListSecretVersionIds—and updates module dependencies in go.mod. Tests and README coverage text were adjusted to reflect the deletion-path behavior.

Changes

Deleted Secret Handling Optimization

Layer / File(s) Summary
Deletion-handling implementation and test validation
collector.go, collector_test.go
collectSecret initializes resource_policy/resource_policy_present, empty versions, and deprecated_version_count before checking describe.DeletedDate; if deleted, it returns immediately without calling GetResourcePolicy or listVersions. Tests assert deleted secrets skip those fetches, show resource_policy_present: false, and expose versions as an empty slice.
Coverage documentation
README.md
Coverage section documents that pending-deletion secrets are recorded with DescribeSecret metadata and any matched CloudTrail deletion context but skip per-secret subresource API calls that AWS rejects.

Go module dependency updates

Layer / File(s) Summary
go.mod dependency edits
go.mod
Bumped github.com/compliance-framework/agent from v0.7.0 to v0.7.1 and added/adjusted indirect dependency entries (github.com/decred/dcrd/dcrec/secp256k1/v4, github.com/goccy/go-json, github.com/lestrrat-go/dsig-secp256k1, github.com/segmentio/asm).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A secret slated for silence, I mind,
I keep its date and the trail it left behind.
No futile calls that AWS will deny,
Just tidy defaults and a test that’s dry.
Hop, stamp, and nod — the collector’s kind.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: guardrails for secrets marked for deletion' accurately describes the main change: adding safety checks to prevent expensive API calls on secrets pending deletion.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds guardrails to handle AWS Secrets Manager secrets that are marked for deletion by avoiding subresource API calls that AWS rejects for pending-deletion secrets, while still emitting useful DescribeSecret metadata and any matched CloudTrail deletion context.

Changes:

  • Skip per-secret subresource calls (GetResourcePolicy, ListSecretVersionIds) when DescribeSecret.DeletedDate is set, while still emitting consistent default config fields.
  • Add a regression test ensuring no policy/version calls occur for pending-deletion secrets and that default config values are present.
  • Document the pending-deletion behavior in the README coverage section.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
README.md Documents that pending-deletion secrets skip policy/version subresource calls while still capturing deletion metadata/context.
collector.go Initializes default resource_policy* and versions fields, then returns early for pending-deletion secrets to avoid rejected AWS calls.
collector_test.go Adds assertions verifying pending-deletion secrets do not trigger policy/version calls and still emit expected defaults.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Signed-off-by: Gustavo Carvalho <gustavo.carvalho@container-solutions.com>
@gusfcarvalho gusfcarvalho merged commit f72db7a into main Jun 1, 2026
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants