CNTRLPLANE-3820: add cleanleaked tool for AWS leaked resource cleanup - #8964
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@jparrill: This pull request references CNTRLPLANE-3820 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds the Sequence Diagram(s)sequenceDiagram
participant CLI as cleanleaked CLI
participant Scanner
participant AWS as AWS APIs
participant Prow as CorrelateWithProw
participant Deleter
CLI->>Scanner: Scan(ctx)
Scanner->>AWS: Inspect VPCs, OIDC, S3, and EC2
AWS-->>Scanner: Resource activity and inventory
Scanner-->>CLI: InfraSet verdicts
CLI->>Prow: CorrelateWithProw(sets)
Prow->>AWS: Inspect Route53 zones and TXT records
AWS-->>Prow: Namespace and zone metadata
Prow-->>CLI: Prow metadata
alt delete requested
CLI->>Deleter: DeleteAll(leaked, mode)
Deleter->>AWS: Delete zones, IAM resources, and VPC dependencies
AWS-->>Deleter: Deletion results
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 7❌ Failed checks (7 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
area/ai
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Triage Skill Output ExampleThe Output: |
|
@jparrill: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
716d5da to
2172c30
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
contrib/aws-leaked-resources/tags.go (1)
57-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead/misleading format entry in
parseExpirationDate.
"2006-01-02T15:04+00:00"is not a valid Go time layout for a numeric offset — the stdlib only recognizes the"-0700"/"-07:00"family (which is sign-agnostic and already matches+hh:mminputs) or the"Z..."family. Since"+00:00"isn't a special reference token, it's treated as literal text, so this format only ever matches inputs containing the literal substring"+00:00". In practice, the preceding format ("2006-01-02T15:04-07:00") already parses any±hh:mmoffset, so this entry is unreachable/dead — confirmed byTestParseExpirationDate's "short ISO with offset" case succeeding purely via the prior format. Consider removing this entry to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/aws-leaked-resources/tags.go` around lines 57 - 73, The parseExpirationDate helper includes a misleading dead layout entry that can never match numeric offsets as intended. Remove the redundant "2006-01-02T15:04+00:00" format from the formats list in parseExpirationDate, since the preceding "2006-01-02T15:04-07:00" already handles ±hh:mm offsets and this extra entry only adds confusion.contrib/aws-leaked-resources/scan.go (2)
398-444: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDuplicate AWS lookups for VPC creation time between
resolveVPCCreationTimeandinventorySubResources.Both functions independently call
DescribeVpcEndpointsandDescribeNetworkInterfacesand run the same "earliest timestamp wins" logic to backfillinfra.VPCs[i].CreatedAt. For any VPC lacking acreation_datetag that ultimately gets classifiedLEAKED, these two API calls are issued twice. Consider extracting a shared helper (e.g.resolveCreatedAtFromEndpoint/resolveCreatedAtFromENI) that both call sites reuse, or haveinventorySubResourcesreuse the already-fetched endpoint/ENI results fromresolveVPCCreationTimeinstead of re-querying.Also applies to: 498-511
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/aws-leaked-resources/scan.go` around lines 398 - 444, The VPC creation-time backfill logic is duplicated between resolveVPCCreationTime and inventorySubResources, causing repeated DescribeVpcEndpoints and DescribeNetworkInterfaces calls for the same VPC. Refactor the shared “earliest timestamp wins” logic into a helper that both resolveVPCCreationTime and inventorySubResources reuse, or pass the already-fetched endpoint/ENI data through so CreatedAt is resolved once. Keep the behavior centered around infra.VPCs[i].CreatedAt and the existing EC2 lookup paths.
567-651: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAWS API errors are silently swallowed in the inventory functions.
inventorySubResources,inventoryHostedZones, andinventoryIAMRolesall ignore errors fromDescribe*/List*calls without logging (e.g.inventoryHostedZonesreturns immediately on the firstListHostedZoneserror, leavingHostedZonesempty for that infra set). Contrast this withcheckEC2Instances(lines 381-384), which logs a WARN and reports an explicit Uncertain reason on failure. For a tool whose output drives real deletion decisions, silently under-reporting inventory on transient AWS errors (throttling, permissions) could mask resources that should have been flagged, without any signal to the operator. Consider applying the same WARN-and-note pattern here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/aws-leaked-resources/scan.go` around lines 567 - 651, AWS API failures in the inventory helpers are currently swallowed, causing partial or empty results without any operator signal. Update inventorySubResources, inventoryHostedZones, and inventoryIAMRoles to handle Describe*/List* errors like checkEC2Instances does: log a WARN with the AWS call context and error, and record an uncertain/incomplete inventory reason on the InfraSet instead of just returning. Use the existing helper symbols inventoryHostedZones, inventoryIAMRoles, and inventorySubResources to add the same failure-reporting pattern consistently.contrib/aws-leaked-resources/clients.go (1)
13-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffLarge, producer-side interfaces conflict with the stated Go guidelines.
EC2API(~20 methods),IAMAPI(~11 methods), and friends are defined here as producer-side interfaces, whereas the repo guidelines call for small (1–2 method), consumer-defined interfaces. This is a common/pragmatic pattern for wrapping AWS SDK clients for mocking, so I'm not blocking on it, but be aware it's a guideline deviation if scrutinized later. As per coding guidelines, "Keep interfaces small and focused (generally 1–2 methods)" and "Define interfaces on the consumer side, not the producer side."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/aws-leaked-resources/clients.go` around lines 13 - 72, The AWS client interfaces in EC2API, IAMAPI, Route53API, ELBv2API, and S3API are large producer-side mocks and should be refactored to follow the repo’s consumer-defined, small-interface guideline. Split the methods into narrower interfaces at the call sites that need them, and move the interface definitions next to the consumers that use them instead of keeping one broad wrapper in clients.go. Preserve the same method signatures, but only expose the minimal methods each consumer actually requires.Source: Coding guidelines
contrib/aws-leaked-resources/prow.go (1)
45-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefix ordering is fragile but currently correct.
InferTestTyperelies onknownTestPrefixesbeing ordered so more-specific prefixes (karpenter-upgrade-control-plane) precede shorter ones that are also prefixes (karpenter). It works today, but a future addition placed in the wrong position would silently misclassify test types with no compiler/test signal.Consider adding a short comment above
knownTestPrefixesdocumenting the "longer/more-specific prefix must come first" invariant, or sorting by descending length at init to make it order-independent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/aws-leaked-resources/prow.go` around lines 45 - 64, InferTestType depends on knownTestPrefixes being ordered from more-specific to less-specific values, so add a clear comment or similar guidance near that symbol documenting the “longer prefix first” invariant. If you want to remove the ordering fragility entirely, update the initialization of knownTestPrefixes so it is sorted by descending length before InferTestType iterates over it, keeping the existing matching behavior in isHexInfraID unchanged.contrib/aws-leaked-resources/prow_test.go (1)
1-173: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo test coverage for
LookupNamespaceFromServiceZone,FindServiceCIZoneID, orCorrelateWithProw.These are the only functions in
prow.gothat talk to Route53 and contain pagination logic — and, per the companion comment onprow.go(lines 80-111), the pagination handling there is currently missing. A table-driven test using a mockRoute53APIthat returns a truncated first page (IsTruncated: true) followed by a second page containing the matching TXT record would have caught that gap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/aws-leaked-resources/prow_test.go` around lines 1 - 173, Add table-driven coverage in prow_test.go for LookupNamespaceFromServiceZone, FindServiceCIZoneID, and CorrelateWithProw using a mocked Route53API. Make the mock return a truncated first page (IsTruncated true) and a second page with the matching TXT record so the pagination path in prow.go is exercised. Assert the namespace/zone ID/prow correlation are found only after the second page, and keep the tests centered around the Route53API and the three function names so they’re easy to locate.contrib/aws-leaked-resources/report.go (1)
12-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
PrintTablemixes sorting, header rendering, and detailed sub-resource dumping in one function.The function handles sort ordering, the summary row, and then four different detail-block loops (VPCs, hosted zones, OIDC providers, IAM roles) for leaked/uncertain sets. Extracting the "print details for a leaked/uncertain set" block (lines 41-70) into a helper (e.g.
printInfraDetails(w, s)) would makePrintTableeasier to scan and test independently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/aws-leaked-resources/report.go` around lines 12 - 72, The PrintTable function is doing too much by combining table sorting, summary-row rendering, and the leaked/uncertain detail output in one block. Extract the detail-printing section for VerdictLeaked and VerdictUncertain into a dedicated helper such as printInfraDetails(w, s), and have PrintTable call it after writing the summary row; keep the existing VPC, HostedZones, OIDCProviders, and IAMRoles loops inside that helper so the main function is easier to scan and test.contrib/aws-leaked-resources/main.go (1)
116-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate flag registration between
scanCmdanddeleteCmd.
--region,--min-age,--target,--output, and--output-dirare registered identically on both subcommands. Since they bind to the same sharedcfgstruct, these could be declared once asrootCmd.PersistentFlags()instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/aws-leaked-resources/main.go` around lines 116 - 130, The flags registered in both scanCmd and deleteCmd are duplicated even though they all bind to the shared cfg struct. Move the common options like region, min-age, target, output, and output-dir to rootCmd.PersistentFlags() in main.go so both subcommands inherit them, and keep only command-specific flags on scanCmd and deleteCmd.contrib/aws-leaked-resources/delete.go (1)
587-754: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
printDetailedInventoryduplicates the resource-listing logic already implemented in the dedicateddelete*helpers.This function re-implements its own Describe+iterate loops for ELBs, VPCEs, NATs, IGWs, subnets, RTBs, SGs, and ENIs, instead of reusing
deleteLoadBalancers,deleteVPCEndpoints, etc. (in list-only mode). Keeping two independent implementations of "what belongs to this VPC" risks the two falling out of sync (e.g., a future filter added to one path but not the other).Extract small
list*(ctx, vpcID)helpers used by both the deletion cascade and this inventory printer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/aws-leaked-resources/delete.go` around lines 587 - 754, printDetailedInventory currently duplicates the “what belongs to this VPC” listing logic that already exists in the delete* flow, so the two paths can drift. Refactor the resource discovery in Deleter by extracting shared list helpers (for example listLoadBalancers, listVPCEndpoints, listNatGateways, listInternetGateways, listSubnets, listRouteTables, listSecurityGroups, and listNetworkInterfaces) and have both printDetailedInventory and the existing deleteLoadBalancers/deleteVPCEndpoints/etc. reuse them. Keep the VPC-scoped filtering and formatting centralized in those shared helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/triage-leaked-infra/SKILL.md:
- Around line 89-97: The shell examples in the triage guide are broken by inline
comments after a pipe, so they cannot be copied and run verbatim. Update the
command examples in the skill documentation so the AWS CLI snippets stand alone
as valid shell, and move explanatory notes about filtering or cross-checking
outside the command text; check the affected snippet blocks around the
describe-vpc-endpoint-service-configurations and describe-addresses examples for
consistency.
In `@contrib/aws-leaked-resources/delete.go`:
- Around line 108-120: DeleteAll currently reports deletion counts but always
returns nil, so failures in failedVPCs and failedZones never reach
runDelete/main and the CLI can exit successfully despite leaks. Update DeleteAll
to surface an error when any VPC or Zone deletions fail, using the existing
failure counters and the deletion summary logic near the end of the function, so
runDelete and main() can propagate a non-zero exit in CI-safe delete mode.
Ensure the success path still returns nil only when all tracked deletions
complete without failures.
- Around line 227-287: The regional list calls in deleteLoadBalancers and
deleteVPCEndpointServices only fetch the first page, so leaked resources can be
missed when there are multiple pages. Update these helpers to page through
DescribeLoadBalancers and DescribeVpcEndpointServiceConfigurations once per
cleanup run, collect all results, and then filter locally by VpcId or cluster
tag before calling retryDelete. Use the existing deleteLoadBalancers and
deleteVPCEndpointServices methods as the main touchpoints.
In `@contrib/aws-leaked-resources/prow.go`:
- Around line 151-178: CorrelateWithProw currently skips VerdictUncertain sets
even though PrintTable in report.go renders the TestType/Namespace/ProwLink
details for both leaked and uncertain infra. Update CorrelateWithProw to enrich
both VerdictLeaked and VerdictUncertain entries by changing the verdict filter
near the start of the loop, while keeping the existing ProwJobURL and
LookupNamespaceFromServiceZone behavior unchanged. Use the CorrelateWithProw and
PrintTable symbols to verify the correlation now feeds the report path for
uncertain sets too.
- Around line 80-111: LookupNamespaceFromServiceZone only inspects the first
ListResourceRecordSets response, so it can miss matching TXT records in larger
hosted zones. Update the Route53 scan logic in LookupNamespaceFromServiceZone to
iterate through all pages using route53.NewListResourceRecordSetsPaginator (or
equivalent manual pagination with
StartRecordName/StartRecordType/StartRecordIdentifier until IsTruncated is
false), while preserving the existing TXT filtering and extractNamespace
matching.
In `@contrib/aws-leaked-resources/report.go`:
- Around line 74-92: PrintSummary is missing the VerdictUntagged bucket from the
summary breakdown. Update PrintSummary to include an Untagged line using
counts[VerdictUntagged] and countVPCs(sets, VerdictUntagged), alongside the
existing Protected, Active, Too young, Uncertain, and Leaked entries. Keep the
output aligned with the other verdict rows so the totals remain consistent with
len(sets) and the VPC counts.
In `@contrib/aws-leaked-resources/scan.go`:
- Around line 274-293: The age gate in checkAge currently treats an undetermined
VPC age as a pass by returning empty verdicts when oldestVPC(vpcs) cannot infer
a creation time. Update checkAge to fail safe by returning VerdictUncertain with
a clear reason whenever oldestVPC is zero/unknown, matching the existing
uncertain handling used in checkEC2Instances and other ambiguous checks. Add or
update tests around checkAge/oldestVPC to cover VPC sets with no creation_date,
VPCE, or ENI attach time.
---
Nitpick comments:
In `@contrib/aws-leaked-resources/clients.go`:
- Around line 13-72: The AWS client interfaces in EC2API, IAMAPI, Route53API,
ELBv2API, and S3API are large producer-side mocks and should be refactored to
follow the repo’s consumer-defined, small-interface guideline. Split the methods
into narrower interfaces at the call sites that need them, and move the
interface definitions next to the consumers that use them instead of keeping one
broad wrapper in clients.go. Preserve the same method signatures, but only
expose the minimal methods each consumer actually requires.
In `@contrib/aws-leaked-resources/delete.go`:
- Around line 587-754: printDetailedInventory currently duplicates the “what
belongs to this VPC” listing logic that already exists in the delete* flow, so
the two paths can drift. Refactor the resource discovery in Deleter by
extracting shared list helpers (for example listLoadBalancers, listVPCEndpoints,
listNatGateways, listInternetGateways, listSubnets, listRouteTables,
listSecurityGroups, and listNetworkInterfaces) and have both
printDetailedInventory and the existing
deleteLoadBalancers/deleteVPCEndpoints/etc. reuse them. Keep the VPC-scoped
filtering and formatting centralized in those shared helpers.
In `@contrib/aws-leaked-resources/main.go`:
- Around line 116-130: The flags registered in both scanCmd and deleteCmd are
duplicated even though they all bind to the shared cfg struct. Move the common
options like region, min-age, target, output, and output-dir to
rootCmd.PersistentFlags() in main.go so both subcommands inherit them, and keep
only command-specific flags on scanCmd and deleteCmd.
In `@contrib/aws-leaked-resources/prow_test.go`:
- Around line 1-173: Add table-driven coverage in prow_test.go for
LookupNamespaceFromServiceZone, FindServiceCIZoneID, and CorrelateWithProw using
a mocked Route53API. Make the mock return a truncated first page (IsTruncated
true) and a second page with the matching TXT record so the pagination path in
prow.go is exercised. Assert the namespace/zone ID/prow correlation are found
only after the second page, and keep the tests centered around the Route53API
and the three function names so they’re easy to locate.
In `@contrib/aws-leaked-resources/prow.go`:
- Around line 45-64: InferTestType depends on knownTestPrefixes being ordered
from more-specific to less-specific values, so add a clear comment or similar
guidance near that symbol documenting the “longer prefix first” invariant. If
you want to remove the ordering fragility entirely, update the initialization of
knownTestPrefixes so it is sorted by descending length before InferTestType
iterates over it, keeping the existing matching behavior in isHexInfraID
unchanged.
In `@contrib/aws-leaked-resources/report.go`:
- Around line 12-72: The PrintTable function is doing too much by combining
table sorting, summary-row rendering, and the leaked/uncertain detail output in
one block. Extract the detail-printing section for VerdictLeaked and
VerdictUncertain into a dedicated helper such as printInfraDetails(w, s), and
have PrintTable call it after writing the summary row; keep the existing VPC,
HostedZones, OIDCProviders, and IAMRoles loops inside that helper so the main
function is easier to scan and test.
In `@contrib/aws-leaked-resources/scan.go`:
- Around line 398-444: The VPC creation-time backfill logic is duplicated
between resolveVPCCreationTime and inventorySubResources, causing repeated
DescribeVpcEndpoints and DescribeNetworkInterfaces calls for the same VPC.
Refactor the shared “earliest timestamp wins” logic into a helper that both
resolveVPCCreationTime and inventorySubResources reuse, or pass the
already-fetched endpoint/ENI data through so CreatedAt is resolved once. Keep
the behavior centered around infra.VPCs[i].CreatedAt and the existing EC2 lookup
paths.
- Around line 567-651: AWS API failures in the inventory helpers are currently
swallowed, causing partial or empty results without any operator signal. Update
inventorySubResources, inventoryHostedZones, and inventoryIAMRoles to handle
Describe*/List* errors like checkEC2Instances does: log a WARN with the AWS call
context and error, and record an uncertain/incomplete inventory reason on the
InfraSet instead of just returning. Use the existing helper symbols
inventoryHostedZones, inventoryIAMRoles, and inventorySubResources to add the
same failure-reporting pattern consistently.
In `@contrib/aws-leaked-resources/tags.go`:
- Around line 57-73: The parseExpirationDate helper includes a misleading dead
layout entry that can never match numeric offsets as intended. Remove the
redundant "2006-01-02T15:04+00:00" format from the formats list in
parseExpirationDate, since the preceding "2006-01-02T15:04-07:00" already
handles ±hh:mm offsets and this extra entry only adds confusion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 10d6af41-62b0-4df5-a271-a3aa5ef6ac90
📒 Files selected for processing (15)
.claude/skills/triage-leaked-infra/SKILL.mdcontrib/aws-leaked-resources/.gitignorecontrib/aws-leaked-resources/Makefilecontrib/aws-leaked-resources/README.mdcontrib/aws-leaked-resources/clients.gocontrib/aws-leaked-resources/delete.gocontrib/aws-leaked-resources/delete_test.gocontrib/aws-leaked-resources/main.gocontrib/aws-leaked-resources/prow.gocontrib/aws-leaked-resources/prow_test.gocontrib/aws-leaked-resources/report.gocontrib/aws-leaked-resources/scan.gocontrib/aws-leaked-resources/scan_test.gocontrib/aws-leaked-resources/tags.gocontrib/aws-leaked-resources/types.go
2172c30 to
2649352
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.claude/skills/triage-leaked-infra/SKILL.md (1)
89-97: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBroken shell snippets still present despite prior "addressed" note.
Lines 89 and 96 still pipe into a trailing
# comment, which makes the pipeline truncate at the comment and produces invalid/incomplete shell (the AWS CLI output is piped into nothing runnable). This matches a previously raised concern that was marked as addressed, but the current file content shows the same issue unresolved.🔧 Proposed fix
-aws ec2 describe-vpc-endpoint-service-configurations --region us-east-1 --output json | # filter by kubernetes.io/cluster/<infraID> tag +aws ec2 describe-vpc-endpoint-service-configurations --region us-east-1 --output json +# filter by kubernetes.io/cluster/<infraID> tag @@ -aws ec2 describe-addresses --region us-east-1 --filters "Name=domain,Values=vpc" --output text | # cross-check with NAT gateway EIPs +aws ec2 describe-addresses --region us-east-1 --filters "Name=domain,Values=vpc" --output text +# cross-check with NAT gateway EIPsAs per path instructions,
.claudefiles are high risk and should be reviewed with a security and malware lens; although this is a documentation-only issue, it should be fixed so the snippets remain copy/paste-safe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/triage-leaked-infra/SKILL.md around lines 89 - 97, The shell snippets in the triage guide still contain trailing inline comments after a pipe, which breaks the command flow and leaves incomplete copy/pasteable AWS CLI examples. Update the affected snippets in SKILL.md so the comment is moved onto its own line or removed, preserving valid pipelines in the examples around the VPC endpoint, NAT gateway, and address checks. Keep the commands in the same section and verify the snippets remain runnable as written.Source: Path instructions
🧹 Nitpick comments (1)
.claude/skills/triage-leaked-infra/SKILL.md (1)
36-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded username allowlist will go stale.
The protected-user list is a hardcoded set of individual usernames embedded directly in the skill doc. As team membership changes, this list requires manual doc edits to stay accurate, and it's easy to forget. Consider sourcing this from a shared, more discoverable location (e.g., a small config file or a well-known Slack/JIRA group) referenced by the skill instead of inlining names in markdown.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/triage-leaked-infra/SKILL.md around lines 36 - 38, The protected-user check in the triage skill is using a hardcoded username allowlist that will drift over time. Update the skill guidance where it currently enumerates usernames to reference a shared source of truth instead, such as a small config file or an approved group identifier, and make the infraID check in the skill point to that external list. Keep the existing FAIL behavior for matches, but ensure the usernames themselves are no longer embedded directly in the markdown.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.claude/skills/triage-leaked-infra/SKILL.md:
- Around line 89-97: The shell snippets in the triage guide still contain
trailing inline comments after a pipe, which breaks the command flow and leaves
incomplete copy/pasteable AWS CLI examples. Update the affected snippets in
SKILL.md so the comment is moved onto its own line or removed, preserving valid
pipelines in the examples around the VPC endpoint, NAT gateway, and address
checks. Keep the commands in the same section and verify the snippets remain
runnable as written.
---
Nitpick comments:
In @.claude/skills/triage-leaked-infra/SKILL.md:
- Around line 36-38: The protected-user check in the triage skill is using a
hardcoded username allowlist that will drift over time. Update the skill
guidance where it currently enumerates usernames to reference a shared source of
truth instead, such as a small config file or an approved group identifier, and
make the infraID check in the skill point to that external list. Keep the
existing FAIL behavior for matches, but ensure the usernames themselves are no
longer embedded directly in the markdown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 83375943-372c-4040-8e02-8beeb707e0f3
📒 Files selected for processing (15)
.claude/skills/triage-leaked-infra/SKILL.mdcontrib/aws-leaked-resources/.gitignorecontrib/aws-leaked-resources/Makefilecontrib/aws-leaked-resources/README.mdcontrib/aws-leaked-resources/clients.gocontrib/aws-leaked-resources/delete.gocontrib/aws-leaked-resources/delete_test.gocontrib/aws-leaked-resources/main.gocontrib/aws-leaked-resources/prow.gocontrib/aws-leaked-resources/prow_test.gocontrib/aws-leaked-resources/report.gocontrib/aws-leaked-resources/scan.gocontrib/aws-leaked-resources/scan_test.gocontrib/aws-leaked-resources/tags.gocontrib/aws-leaked-resources/types.go
✅ Files skipped from review due to trivial changes (2)
- contrib/aws-leaked-resources/.gitignore
- contrib/aws-leaked-resources/README.md
🚧 Files skipped from review as they are similar to previous changes (11)
- contrib/aws-leaked-resources/prow_test.go
- contrib/aws-leaked-resources/report.go
- contrib/aws-leaked-resources/clients.go
- contrib/aws-leaked-resources/prow.go
- contrib/aws-leaked-resources/tags.go
- contrib/aws-leaked-resources/types.go
- contrib/aws-leaked-resources/main.go
- contrib/aws-leaked-resources/scan.go
- contrib/aws-leaked-resources/scan_test.go
- contrib/aws-leaked-resources/delete.go
- contrib/aws-leaked-resources/delete_test.go
|
/uncc |
|
/cc @sdminonne |
| return | ||
| } | ||
|
|
||
| out, err := d.ELBv2.DescribeLoadBalancers(ctx, &elbv2.DescribeLoadBalancersInput{}) |
There was a problem hiding this comment.
Hmm, if I'm understanding correctly we may need to add a paginator here
There was a problem hiding this comment.
Added — both deleteLoadBalancers and inventorySubResources now paginate through all pages. Also added a per-scan cache in Scanner so the scan path fetches LBs once per run.
There was a problem hiding this comment.
Yeah. Thanks it paginates only for VPCs right? All the other subresources are not. Guess is fine in practice or at least for resources we handle at the moment but it may change in the future (if we decide to handle different resources in other ways).
| } | ||
|
|
||
| if s.ELBv2 != nil { | ||
| if out, err := s.ELBv2.DescribeLoadBalancers(ctx, &elbv2.DescribeLoadBalancersInput{}); err == nil { |
There was a problem hiding this comment.
Fixed — paginated DescribeVpcEndpointServiceConfigurations here and in delete.go.
| // Input: "heritage=external-dns,...,external-dns/resource=route/e2e-clusters-7c84g-node-pool-78vcg/kube-apiserver" | ||
| // Output: "e2e-clusters-7c84g-node-pool-78vcg" | ||
| func extractNamespace(txtValue string) string { | ||
| re := regexp.MustCompile(`resource=route/([^/]+)/`) |
There was a problem hiding this comment.
Can we put in a var this regexp? I know it won't be executed many times though :)
There was a problem hiding this comment.
Done — moved to a package-level var namespaceFromRouteRe = regexp.MustCompile(...).
| } else { | ||
| fmt.Fprintf(os.Stderr, " LEAKED: %s — inventorying sub-resources...\n", infraID) | ||
| } | ||
| s.inventorySubResources(ctx, &infraSet) |
There was a problem hiding this comment.
This is called for one infraID, right?
There was a problem hiding this comment.
Yes, inventorySubResources is called once per InfraSet (= one infraID). It iterates over the VPCs in that set and counts their sub-resources.
| VerdictUncertain Verdict = "UNCERTAIN" | ||
| ) | ||
|
|
||
| type InfraSet struct { |
There was a problem hiding this comment.
To be honest the InfraSet name is a little bit misleading, since it can contain only one infraID but it's `nit
There was a problem hiding this comment.
Acknowledged — InfraSet groups all AWS resources (VPCs, hosted zones, OIDC providers, IAM roles) that share the same infraID. Think of it as the set of infrastructure belonging to one cluster deployment. The name is accurate since it's a set of resources, not a set of infraIDs. That said, if it causes confusion during reviews I'm open to renaming in a follow-up.
There was a problem hiding this comment.
InfraInfo? InfraData? It may come from my past but a Set is an ensemble of things. Anyway. A follow-up is good too.
| } | ||
|
|
||
| if s.ELBv2 != nil { | ||
| if out, err := s.ELBv2.DescribeLoadBalancers(ctx, &elbv2.DescribeLoadBalancersInput{}); err == nil { |
There was a problem hiding this comment.
Here we describe all account LB, right so for each potentially leaked infraID we goes over all the LBs... May we cache it somhow?
There was a problem hiding this comment.
Good catch — fixed. Added a per-scan LB cache to Scanner that fetches all LBs once (with pagination) and filters locally per VPC. No more N×DescribeLoadBalancers calls.
| natEIPs := d.collectNATGatewayEIPs(ctx, vpcID) | ||
| if natDeleted := d.deleteNATGateways(ctx, vpcID); natDeleted > 0 { | ||
| fmt.Fprintf(os.Stderr, " Waiting for NAT gateways to release ENIs...\n") | ||
| if err := sleepWithContext(ctx, 15*time.Second); err != nil { |
There was a problem hiding this comment.
Not sure 15secs. couldn't we implement retry wit exponential backoff? I won't block on this since I'm not totally sure neither. Just a question
There was a problem hiding this comment.
Good question. The 15s sleep was a grace period for NAT gateways to release their ENIs (async AWS behavior), not a retry. That said, a blind sleep is fragile. I've replaced it with a polling loop: check every 5s whether NATs are still in deleting state, up to 12 retries (60s max). Fixed interval, no exponential backoff — this isn't a k8s controller and these AWS calls are cheap and on-demand.
There was a problem hiding this comment.
SGTM it was just to think about this and sharing rationale. TY!
| RoleName: aws.String(roleName), | ||
| }); err == nil { | ||
| for _, prof := range profOut.InstanceProfiles { | ||
| _, _ = d.IAM.RemoveRoleFromInstanceProfile(ctx, &iam.RemoveRoleFromInstanceProfileInput{ |
There was a problem hiding this comment.
OK but we should delete the instance profile too, in case of no error we should invoke DeleteInstanceProfile.
There was a problem hiding this comment.
Good catch — added DeleteInstanceProfile call right after RemoveRoleFromInstanceProfile so the orphaned profile doesn't linger.
2649352 to
4f1efd0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8964 +/- ##
==========================================
+ Coverage 43.37% 44.11% +0.73%
==========================================
Files 771 772 +1
Lines 95718 96227 +509
==========================================
+ Hits 41520 42449 +929
+ Misses 51313 50832 -481
- Partials 2885 2946 +61 see 52 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
4f1efd0 to
ee69d33
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
contrib/aws-leaked-resources/scan.go (2)
74-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
--limitselects a nondeterministic subset of leaked sets.
Scanranges overgrouped(a map), so iteration order is random. WithConfig.Limit > 0, the firstLimitleaked sets encountered are inventoried and the rest are dropped (Line 103-105), meaning repeated runs report/act on different infra sets. For a repeatable cleanup tool, consider iterating over sorted infra IDs so--limitis deterministic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/aws-leaked-resources/scan.go` around lines 74 - 105, Make the iteration that drives Scan’s limit handling deterministic by extracting the infra IDs from grouped, sorting them, and iterating in that sorted order instead of ranging over the map directly. Preserve the existing per-set processing and --limit behavior while ensuring the same infra IDs are selected on every run.
630-662: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache account-wide IAM role and hosted-zone listings like ELBs. Both functions run once per leaked infra set (Lines 147-148), and each paginates the entire account (
ListRoles/ListHostedZones) from scratch every time. With many leaked sets this is O(leaked × total-roles) and O(leaked × total-zones) API calls — the exact N× fan-out that was already fixed for load balancers vialbCache/fetchAllLoadBalancers. Fetch each collection once and filter locally per infra set.
contrib/aws-leaked-resources/scan.go#L630-L662: add arolesCache(fetched once, paginated) toScannerand filter by suffix/infraID locally instead of callingListRolesper infra set.contrib/aws-leaked-resources/scan.go#L578-L611: add azonesCache(fetched once, paginated) toScannerand runZoneMatchesInfralocally instead of callingListHostedZonesper infra set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/aws-leaked-resources/scan.go` around lines 630 - 662, Update contrib/aws-leaked-resources/scan.go:630-662 around Scanner.inventoryIAMRoles to add a Scanner-level rolesCache, fetch and paginate ListRoles once, then filter cached roles by suffix and infraID locally for each infra set instead of calling ListRoles per set. Also update contrib/aws-leaked-resources/scan.go:578-611 around the hosted-zone inventory function to add and reuse a zonesCache populated once through paginated ListHostedZones calls, applying ZoneMatchesInfra locally for each infra set.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/triage-leaked-infra/SKILL.md:
- Around line 17-19: Update the identifier extraction flow to fully validate
user-provided VPC IDs and infraIDs against their required formats before
constructing AWS CLI commands. Store validated values in shell variables and
quote those variables in every command interpolation, including both
VPC-to-infraID and infraID-to-VPC lookups.
- Around line 61-65: Update the OIDC liveness check instructions for the aws
s3api head-object commands so only an explicit 404 is classified as PASS;
classify 200 as FAIL and all other errors, including AccessDenied, throttling,
wrong-region, or transport failures, as UNKNOWN. Ensure UNKNOWN blocks deletion
rather than being treated as evidence that the object is absent.
In `@contrib/aws-leaked-resources/Makefile`:
- Around line 7-10: Define one root-relative BINDIR for the aws-leaked-resources
artifact directory, then update the build and clean targets to create, write to,
echo, and remove that same directory. Replace the separate ./bin creation and
hard-coded $(REPO_ROOT)/contrib/aws-leaked-resources/bin references while
preserving the existing binary name.
---
Nitpick comments:
In `@contrib/aws-leaked-resources/scan.go`:
- Around line 74-105: Make the iteration that drives Scan’s limit handling
deterministic by extracting the infra IDs from grouped, sorting them, and
iterating in that sorted order instead of ranging over the map directly.
Preserve the existing per-set processing and --limit behavior while ensuring the
same infra IDs are selected on every run.
- Around line 630-662: Update contrib/aws-leaked-resources/scan.go:630-662
around Scanner.inventoryIAMRoles to add a Scanner-level rolesCache, fetch and
paginate ListRoles once, then filter cached roles by suffix and infraID locally
for each infra set instead of calling ListRoles per set. Also update
contrib/aws-leaked-resources/scan.go:578-611 around the hosted-zone inventory
function to add and reuse a zonesCache populated once through paginated
ListHostedZones calls, applying ZoneMatchesInfra locally for each infra set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 8094bf45-ae9e-4139-90f5-1ea285ffeeb7
📒 Files selected for processing (15)
.claude/skills/triage-leaked-infra/SKILL.mdcontrib/aws-leaked-resources/.gitignorecontrib/aws-leaked-resources/Makefilecontrib/aws-leaked-resources/README.mdcontrib/aws-leaked-resources/clients.gocontrib/aws-leaked-resources/delete.gocontrib/aws-leaked-resources/delete_test.gocontrib/aws-leaked-resources/main.gocontrib/aws-leaked-resources/prow.gocontrib/aws-leaked-resources/prow_test.gocontrib/aws-leaked-resources/report.gocontrib/aws-leaked-resources/scan.gocontrib/aws-leaked-resources/scan_test.gocontrib/aws-leaked-resources/tags.gocontrib/aws-leaked-resources/types.go
🚧 Files skipped from review as they are similar to previous changes (10)
- contrib/aws-leaked-resources/.gitignore
- contrib/aws-leaked-resources/types.go
- contrib/aws-leaked-resources/clients.go
- contrib/aws-leaked-resources/report.go
- contrib/aws-leaked-resources/main.go
- contrib/aws-leaked-resources/prow.go
- contrib/aws-leaked-resources/delete.go
- contrib/aws-leaked-resources/scan_test.go
- contrib/aws-leaked-resources/delete_test.go
- contrib/aws-leaked-resources/README.md
| Extract the **infraID** and **VPC ID** from the input. If only one is given, derive the other: | ||
| - VPC ID → `aws ec2 describe-vpcs --vpc-ids <VPC> --query 'Vpcs[0].Tags'` → extract infraID from `kubernetes.io/cluster/<infraID>` tag or `Name` tag (strip `-vpc` suffix) | ||
| - infraID → `aws ec2 describe-vpcs --filters "Name=tag:Name,Values=<infraID>-vpc"` → get VPC ID |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate identifiers before interpolating them into shell commands.
Lines 17–19 accept user-provided values that are then copied into AWS CLI snippets. A crafted value can inject shell syntax and execute commands under the operator’s credentials. Require full-format allow-list validation for VPC IDs and infraIDs, then use quoted shell variables in every command.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/triage-leaked-infra/SKILL.md around lines 17 - 19, Update the
identifier extraction flow to fully validate user-provided VPC IDs and infraIDs
against their required formats before constructing AWS CLI commands. Store
validated values in shell variables and quote those variables in every command
interpolation, including both VPC-to-infraID and infraID-to-VPC lookups.
Source: Path instructions
| aws s3api head-object --bucket hypershift-ci-oidc --key "<infraID>/.well-known/openid-configuration" --region us-east-1 | ||
| aws s3api head-object --bucket hypershift-ci-2-oidc --key "<infraID>/.well-known/openid-configuration" --region us-east-1 | ||
| aws s3api head-object --bucket hypershift-ci-3-oidc --key "<infraID>/.well-known/openid-configuration" --region us-east-1 | ||
| ``` | ||
| If any returns 200 → **FAIL** (cluster still active). If all return 404/error → **PASS**. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
# Compare a known existing OIDC key and a known-absent key. Both commands must
# have their AWS error code captured; a non-zero exit alone is not "not found".
aws s3api head-object --bucket hypershift-ci-oidc \
--key "<known-existing-infraID>/.well-known/openid-configuration" \
--region us-east-1
aws s3api head-object --bucket hypershift-ci-oidc \
--key "__known-absent-key__" \
--region us-east-1Repository: openshift/hypershift
Length of output: 236
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect the target file around the cited lines.
file=".claude/skills/triage-leaked-infra/SKILL.md"
wc -l "$file"
sed -n '45,85p' "$file"Repository: openshift/hypershift
Length of output: 2793
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
file=".claude/skills/triage-leaked-infra/SKILL.md"
nl -ba "$file" | sed -n '58,68p'Repository: openshift/hypershift
Length of output: 198
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('.claude/skills/triage-leaked-infra/SKILL.md')
lines = p.read_text().splitlines()
for i in range(58, 69):
if 1 <= i <= len(lines):
print(f"{i}: {lines[i-1]}")
PYRepository: openshift/hypershift
Length of output: 725
Treat only 404 as PASS in the OIDC liveness check. head-object failures like AccessDenied, throttling, wrong-region, or transport errors can mask an active cluster; classify them as UNKNOWN and block deletion unless the bucket returns a real 404.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/triage-leaked-infra/SKILL.md around lines 61 - 65, Update the
OIDC liveness check instructions for the aws s3api head-object commands so only
an explicit 404 is classified as PASS; classify 200 as FAIL and all other
errors, including AccessDenied, throttling, wrong-region, or transport failures,
as UNKNOWN. Ensure UNKNOWN blocks deletion rather than being treated as evidence
that the object is absent.
Source: Path instructions
| build: | ||
| @mkdir -p bin | ||
| cd $(REPO_ROOT) && go build -o $(PKG)/bin/$(BINARY) $(PKG) | ||
| @echo "Built: bin/$(BINARY)" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use one absolute artifact directory for build and clean.
Lines 8–10 create ./bin but build to $(REPO_ROOT)/contrib/aws-leaked-resources/bin/cleanleaked; from the repository root, that parent directory does not exist, so make build fails. Line 19 also removes the wrong directory. Define a single root-relative BINDIR and use it consistently.
Proposed fix
BINARY := cleanleaked
REPO_ROOT := $(shell git rev-parse --show-toplevel)
PKG := ./contrib/aws-leaked-resources
+BINDIR := $(REPO_ROOT)/bin
build:
- `@mkdir` -p bin
- cd $(REPO_ROOT) && go build -o $(PKG)/bin/$(BINARY) $(PKG)
- `@echo` "Built: bin/$(BINARY)"
+ `@mkdir` -p $(BINDIR)
+ cd $(REPO_ROOT) && go build -o $(BINDIR)/$(BINARY) $(PKG)
+ `@echo` "Built: $(BINDIR)/$(BINARY)"
clean:
- rm -rf bin
+ rm -rf $(BINDIR)Also applies to: 18-19
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contrib/aws-leaked-resources/Makefile` around lines 7 - 10, Define one
root-relative BINDIR for the aws-leaked-resources artifact directory, then
update the build and clean targets to create, write to, echo, and remove that
same directory. Replace the separate ./bin creation and hard-coded
$(REPO_ROOT)/contrib/aws-leaked-resources/bin references while preserving the
existing binary name.
ee69d33 to
0ec5cc6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
.claude/skills/triage-leaked-infra/SKILL.md (3)
17-19:⚠️ Potential issue | 🟠 MajorValidate identifiers before interpolating them into shell commands.
User-provided VPC IDs and infraIDs are inserted into AWS CLI snippets without strict format validation or shell-safe variables. Require allow-list validation, assign validated values to variables, and quote every interpolation.
As per path instructions,
.claudefiles are high risk and require security-focused review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/triage-leaked-infra/SKILL.md around lines 17 - 19, Validate the extracted VPC ID and infraID against strict allow-lists before using them in AWS CLI commands, then assign the validated values to shell variables. Update the VPC lookup and infraID lookup commands to quote every variable interpolation and avoid embedding unvalidated input directly in command strings.Source: Path instructions
61-65:⚠️ Potential issue | 🟠 MajorFail closed on OIDC liveness errors.
Only an explicit 404 should be a PASS; 200 must be a FAIL, and AccessDenied, throttling, wrong-region, or transport errors must be UNKNOWN and block deletion. Treating every error as absence can hide an active cluster.
As per path instructions,
.claudefiles are high risk and require security-focused review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/triage-leaked-infra/SKILL.md around lines 61 - 65, Update the OIDC liveness checks in the triage procedure so only an explicit 404 is treated as PASS and a 200 as FAIL; classify AccessDenied, throttling, wrong-region, and transport errors as UNKNOWN and block deletion. Revise the documented command-result handling around the three aws s3api head-object checks to fail closed rather than treating all errors as absence.Source: Path instructions
89-96:⚠️ Potential issue | 🟡 MinorKeep the shell examples executable.
The pipelines on Line 89 and Line 96 end at inline comments, so these commands cannot be copied and run verbatim. Move the explanatory text outside the commands or implement the intended filters.
As per path instructions,
.claudefiles are high risk and require security-focused review.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/skills/triage-leaked-infra/SKILL.md around lines 89 - 96, Update the AWS CLI examples in the triage-leaked-infra instructions so every command remains executable when copied verbatim. Remove the inline comments after the pipelines on the endpoint-service and address commands, placing their explanatory text outside the commands or replacing it with valid filtering logic; preserve the intended Kubernetes tag filtering and NAT gateway EIP cross-check guidance.Source: Path instructions
contrib/aws-leaked-resources/Makefile (1)
7-10:⚠️ Potential issue | 🟠 MajorUse one artifact directory for build and clean.
The build creates
./binbut writes to$(PKG)/bin/cleanleaked, whose parent is not created from the repository root.make buildtherefore fails, andcleanremoves the wrong directory. Define oneBINDIRand use it consistently.Also applies to: 18-19
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contrib/aws-leaked-resources/Makefile` around lines 7 - 10, Update the Makefile build and clean targets to define a single BINDIR artifact directory, create that directory before building, and write the binary there. Replace both hardcoded or inconsistent bin paths in build and clean with BINDIR so they operate on the same artifact location.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/triage-leaked-infra/SKILL.md:
- Line 126: Update the fenced report example in the triage-leaked-infra skill
documentation to specify the markdown language identifier, changing the opening
fence to use markdown while preserving the example content.
- Around line 149-157: Update the verdict rules to prevent any UNKNOWN
deletion-gating check among checks 1–8 from producing SAFE TO DELETE. Restrict
the existing CI-pattern/age exception to cases where all required safety checks
PASS, or explicitly identify only non-gating informational checks that may
remain UNKNOWN; preserve DO NOT DELETE for any FAIL and human review for
unresolved UNKNOWN states.
---
Duplicate comments:
In @.claude/skills/triage-leaked-infra/SKILL.md:
- Around line 17-19: Validate the extracted VPC ID and infraID against strict
allow-lists before using them in AWS CLI commands, then assign the validated
values to shell variables. Update the VPC lookup and infraID lookup commands to
quote every variable interpolation and avoid embedding unvalidated input
directly in command strings.
- Around line 61-65: Update the OIDC liveness checks in the triage procedure so
only an explicit 404 is treated as PASS and a 200 as FAIL; classify
AccessDenied, throttling, wrong-region, and transport errors as UNKNOWN and
block deletion. Revise the documented command-result handling around the three
aws s3api head-object checks to fail closed rather than treating all errors as
absence.
- Around line 89-96: Update the AWS CLI examples in the triage-leaked-infra
instructions so every command remains executable when copied verbatim. Remove
the inline comments after the pipelines on the endpoint-service and address
commands, placing their explanatory text outside the commands or replacing it
with valid filtering logic; preserve the intended Kubernetes tag filtering and
NAT gateway EIP cross-check guidance.
In `@contrib/aws-leaked-resources/Makefile`:
- Around line 7-10: Update the Makefile build and clean targets to define a
single BINDIR artifact directory, create that directory before building, and
write the binary there. Replace both hardcoded or inconsistent bin paths in
build and clean with BINDIR so they operate on the same artifact location.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4ee96c95-9db3-44f1-81b7-d5776667e6de
📒 Files selected for processing (15)
.claude/skills/triage-leaked-infra/SKILL.mdcontrib/aws-leaked-resources/.gitignorecontrib/aws-leaked-resources/Makefilecontrib/aws-leaked-resources/README.mdcontrib/aws-leaked-resources/clients.gocontrib/aws-leaked-resources/delete.gocontrib/aws-leaked-resources/delete_test.gocontrib/aws-leaked-resources/main.gocontrib/aws-leaked-resources/prow.gocontrib/aws-leaked-resources/prow_test.gocontrib/aws-leaked-resources/report.gocontrib/aws-leaked-resources/scan.gocontrib/aws-leaked-resources/scan_test.gocontrib/aws-leaked-resources/tags.gocontrib/aws-leaked-resources/types.go
🚧 Files skipped from review as they are similar to previous changes (13)
- contrib/aws-leaked-resources/.gitignore
- contrib/aws-leaked-resources/clients.go
- contrib/aws-leaked-resources/report.go
- contrib/aws-leaked-resources/README.md
- contrib/aws-leaked-resources/types.go
- contrib/aws-leaked-resources/prow.go
- contrib/aws-leaked-resources/delete.go
- contrib/aws-leaked-resources/main.go
- contrib/aws-leaked-resources/tags.go
- contrib/aws-leaked-resources/prow_test.go
- contrib/aws-leaked-resources/scan_test.go
- contrib/aws-leaked-resources/delete_test.go
- contrib/aws-leaked-resources/scan.go
|
|
||
| Present the report in this format: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the fenced report example.
The code fence starting on Line 126 has no language identifier and triggers markdownlint MD040; mark it as ```markdown.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 126-126: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/triage-leaked-infra/SKILL.md at line 126, Update the fenced
report example in the triage-leaked-infra skill documentation to specify the
markdown language identifier, changing the opening fence to use markdown while
preserving the example content.
Source: Linters/SAST tools
| ## Verdict rules | ||
|
|
||
| These are non-negotiable: | ||
|
|
||
| - Any **FAIL** → verdict is **DO NOT DELETE** (explain which check failed and why) | ||
| - All checks **PASS** and CI pattern matches → **SAFE TO DELETE** | ||
| - All checks **PASS** but CI pattern is **UNKNOWN** → **UNCERTAIN** (could be a developer cluster) | ||
| - If CI pattern is **PASS** and age/expiration is **PASS** (older than 24h or expired), remaining UNKNOWN checks do not block → **SAFE TO DELETE** (a confirmed CI resource older than 24h with no OIDC, no instances, and no protection tags is leaked) | ||
| - If CI pattern is **UNKNOWN** and ANY check is **UNKNOWN** → **UNCERTAIN — REQUIRES HUMAN DECISION** |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not let UNKNOWN safety checks produce SAFE TO DELETE.
Line 156 permits deletion when checks remain UNKNOWN. A failed protection-tag, OIDC, or instance query could therefore be mistaken for an absent resource. Require PASS for all deletion-gating checks (1-8), or explicitly limit this exception to informational checks only.
As per path instructions, .claude files are high risk and require security-focused review.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/triage-leaked-infra/SKILL.md around lines 149 - 157, Update
the verdict rules to prevent any UNKNOWN deletion-gating check among checks 1–8
from producing SAFE TO DELETE. Restrict the existing CI-pattern/age exception to
cases where all required safety checks PASS, or explicitly identify only
non-gating informational checks that may remain UNKNOWN; preserve DO NOT DELETE
for any FAIL and human review for unresolved UNKNOWN states.
Source: Path instructions
Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe PR adds 2,586 new source lines across 8 Go files and 2,325 test lines across 3 test files. While the test-to-source ratio is healthy (89.9%), the tests concentrate on certain files while leaving others completely untested:
The aggregate result: 706 lines hit / 1,729 executable lines = 40.83%, which is 2.54 percentage points below the 43.37% project-wide target that codecov enforces as a patch requirement. The gap is approximately 44 additional lines needing test coverage to cross the threshold. The two zero-coverage files ( Recommendations
Evidence
|
|
/lgtm |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
0ec5cc6 to
7942e1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/triage-leaked-infra/SKILL.md:
- Around line 96-97: Update the Elastic IP inventory command in the triage
workflow to scope results to the target VPC by filtering through its NAT gateway
or network-interface associations, rather than listing all account-wide
VPC-domain addresses. Preserve the existing cross-check with NAT gateway EIPs
and report only allocations associated with the target VPC.
- Line 3: Restrict the SKILL.md description trigger to AWS VPC IDs and
HyperShift infraIDs, removing the broad “any AWS resource” wording. Keep
activation for cleanleaked output, VPC safety checks, and LEAKED or UNCERTAIN
infra verdicts, without implying support for IAM roles, hosted zones, or
unrelated resources.
- Around line 18-19: Update the VPC discovery commands for both the VPC ID →
infraID and infraID → VPC ID flows to explicitly use AWS region us-east-1, and
add the same region option to the protection-tag query used by the safety
checks. Ensure every related AWS CLI command in this discovery and protection
flow is pinned to us-east-1 rather than relying on the operator’s default
region.
In `@contrib/aws-leaked-resources/delete.go`:
- Around line 47-50: Update the deletion flow around the infra-set loop and
final failure-counter check to return ctx.Err() when cancellation occurs, rather
than breaking and returning nil. Check ctx.Err() immediately before the final
failure evaluation, while preserving the existing cancellation message and
failure handling for non-canceled runs.
- Around line 77-94: Update the cleanup logic surrounding deleteOIDCProvider and
deleteIAMRole to track failed deletion counts, incrementing the appropriate
counter whenever either operation returns an error. Include those OIDC-provider
and IAM-role failures in the final result returned by the enclosing cleanup
method, while preserving the existing logging and success counters.
- Around line 395-403: Update collectNATGatewayEIPs and the related cleanup
paths at the referenced symbols to stop discarding discovery, detach, revoke,
release, and IAM deletion errors. Propagate or aggregate each failure through
deleteVPC and deleteIAMRole, wrapping returned errors with operation-specific
context while preserving successful cleanup behavior and ensuring EIP and
instance-profile failures reach the caller.
- Around line 594-627: Update deleteHostedZone to avoid accumulating all record
deletions into one ChangeResourceRecordSets request. Build and submit quota-safe
batches while iterating records, keeping each batch within both 1,000
ResourceRecord elements and 32,000 total value characters, and reuse the
existing retryDelete behavior for each batch. Preserve skipping NS and SOA
records and ensure the final partial batch is deleted.
- Around line 126-130: Update retryDelete to accept the caller’s context and
pass that ctx to retryDeleteWithCtx instead of creating context.Background(),
preserving cancellation during retry delays and attempts.
- Around line 296-300: Update the retryDelete callbacks in DeleteVpcEndpoints
and DeleteVpcEndpointServiceConfigurations to inspect each EC2 response’s
Unsuccessful entries and return an error containing the failed resource IDs when
any exist, even if the request error is nil. Preserve transport-error handling
and allow retryDelete to retry these partial failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a5dff954-99d6-4b89-8712-733f6057c1cf
📒 Files selected for processing (16)
.claude/skills/triage-leaked-infra/SKILL.mdcodecov.ymlcontrib/aws-leaked-resources/.gitignorecontrib/aws-leaked-resources/Makefilecontrib/aws-leaked-resources/README.mdcontrib/aws-leaked-resources/clients.gocontrib/aws-leaked-resources/delete.gocontrib/aws-leaked-resources/delete_test.gocontrib/aws-leaked-resources/main.gocontrib/aws-leaked-resources/prow.gocontrib/aws-leaked-resources/prow_test.gocontrib/aws-leaked-resources/report.gocontrib/aws-leaked-resources/scan.gocontrib/aws-leaked-resources/scan_test.gocontrib/aws-leaked-resources/tags.gocontrib/aws-leaked-resources/types.go
🚧 Files skipped from review as they are similar to previous changes (11)
- contrib/aws-leaked-resources/clients.go
- contrib/aws-leaked-resources/prow.go
- contrib/aws-leaked-resources/prow_test.go
- contrib/aws-leaked-resources/README.md
- contrib/aws-leaked-resources/tags.go
- contrib/aws-leaked-resources/types.go
- contrib/aws-leaked-resources/main.go
- contrib/aws-leaked-resources/delete_test.go
- contrib/aws-leaked-resources/report.go
- contrib/aws-leaked-resources/scan.go
- contrib/aws-leaked-resources/scan_test.go
| - VPC ID → `aws ec2 describe-vpcs --vpc-ids <VPC> --query 'Vpcs[0].Tags'` → extract infraID from `kubernetes.io/cluster/<infraID>` tag or `Name` tag (strip `-vpc` suffix) | ||
| - infraID → `aws ec2 describe-vpcs --filters "Name=tag:Name,Values=<infraID>-vpc"` → get VPC ID |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pin VPC discovery and protection checks to us-east-1.
These commands omit --region us-east-1 despite the instruction on Line 23. If the operator’s AWS CLI default is another region, discovery can fail or select a different VPC before safety checks run. Add the explicit region to both derivation commands and the protection-tag query.
Also applies to: 26-28
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/triage-leaked-infra/SKILL.md around lines 18 - 19, Update the
VPC discovery commands for both the VPC ID → infraID and infraID → VPC ID flows
to explicitly use AWS region us-east-1, and add the same region option to the
protection-tag query used by the safety checks. Ensure every related AWS CLI
command in this discovery and protection flow is pinned to us-east-1 rather than
relying on the operator’s default region.
| aws ec2 describe-addresses --region us-east-1 --filters "Name=domain,Values=vpc" --output text | # cross-check with NAT gateway EIPs | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Scope Elastic IP inventory to the target VPC.
describe-addresses with only domain=vpc returns account-wide EIPs, so unrelated allocations can be attributed to <VPC>. Filter through the target VPC’s NAT gateway or network-interface associations before reporting them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/skills/triage-leaked-infra/SKILL.md around lines 96 - 97, Update the
Elastic IP inventory command in the triage workflow to scope results to the
target VPC by filtering through its NAT gateway or network-interface
associations, rather than listing all account-wide VPC-domain addresses.
Preserve the existing cross-check with NAT gateway EIPs and report only
allocations associated with the target VPC.
| if err := ctx.Err(); err != nil { | ||
| fmt.Fprintf(os.Stderr, "\nCanceled.\n") | ||
| break | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return cancellation instead of reporting success.
Cancellation between infra sets breaks the loop with zero failures, then returns nil. Check ctx.Err() before the final failure-counter check.
Proposed fix
+ if err := ctx.Err(); err != nil {
+ return fmt.Errorf("deletion canceled: %w", err)
+ }
if failedVPCs > 0 || failedZones > 0 {As per path instructions, “context.Context for cancellation and timeouts.”
Also applies to: 119-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contrib/aws-leaked-resources/delete.go` around lines 47 - 50, Update the
deletion flow around the infra-set loop and final failure-counter check to
return ctx.Err() when cancellation occurs, rather than breaking and returning
nil. Check ctx.Err() immediately before the final failure evaluation, while
preserving the existing cancellation message and failure handling for
non-canceled runs.
Source: Path instructions
| for _, oidcARN := range infra.OIDCProviders { | ||
| if err := d.deleteOIDCProvider(ctx, oidcARN); err != nil { | ||
| fmt.Fprintf(os.Stderr, " FAILED OIDC provider %s: %v\n", oidcARN, err) | ||
| } else { | ||
| fmt.Fprintf(os.Stderr, " Deleted OIDC provider: %s\n", oidcARN) | ||
| deletedOIDC++ | ||
| } | ||
| } | ||
|
|
||
| for _, roleName := range infra.IAMRoles { | ||
| if err := ctx.Err(); err != nil { | ||
| break | ||
| } | ||
| if err := d.deleteIAMRole(ctx, roleName); err != nil { | ||
| fmt.Fprintf(os.Stderr, " FAILED IAM role %s: %v\n", roleName, err) | ||
| } else { | ||
| fmt.Fprintf(os.Stderr, " Deleted IAM role: %s\n", roleName) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Include OIDC and IAM failures in the final result.
Failed OIDC-provider and IAM-role deletions are logged but never affect the returned error. Track their failure counts so automation cannot report successful cleanup while these resources remain.
Also applies to: 108-120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contrib/aws-leaked-resources/delete.go` around lines 77 - 94, Update the
cleanup logic surrounding deleteOIDCProvider and deleteIAMRole to track failed
deletion counts, incrementing the appropriate counter whenever either operation
returns an error. Include those OIDC-provider and IAM-role failures in the final
result returned by the enclosing cleanup method, while preserving the existing
logging and success counters.
| if len(serviceIDs) > 0 { | ||
| if err := retryDelete(fmt.Sprintf("VPCE services (%d)", len(serviceIDs)), func() error { | ||
| _, err := d.EC2.DeleteVpcEndpointServiceConfigurations(ctx, &ec2.DeleteVpcEndpointServiceConfigurationsInput{ServiceIds: serviceIDs}) | ||
| return err | ||
| }); err != nil { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 'DeleteVpcEndpoints|DeleteVpcEndpointServiceConfigurations|Unsuccessful' contrib/aws-leaked-resourcesRepository: openshift/hypershift
Length of output: 2058
🌐 Web query:
AWS EC2 DeleteVpcEndpoints Unsuccessful response field DeleteVpcEndpointServiceConfigurations Unsuccessful API reference
💡 Result:
Both the AWS DeleteVpcEndpoints and DeleteVpcEndpointServiceConfigurations API operations include an Unsuccessful response field [1][2]. The Unsuccessful field is a list containing information about resources that were not successfully deleted during the batch request [2][3]. Each entry in this list typically includes a ResourceId, identifying the specific resource that failed to process, and an Error structure containing a Code and Message detailing why the operation failed for that specific item [2]. In AWS SDKs and CLI tools, this field allows developers to handle partial failures [4][5][2]. For example, in many SDKs, you can use methods such as hasUnsuccessful to determine if the service returned any failure information in the response, allowing you to distinguish between an empty list (successful batch) and a situation where the service returned no data for this field [6][7]. If a command is partially successful or unsuccessful, the Unsuccessful list is populated; if the entire operation is successful, the returned list is empty [4][3].
Citations:
- 1: https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_DeleteVpcEndpoints.html
- 2: https://docs.aws.amazon.com/boto3/latest/reference/services/ec2/client/delete_vpc_endpoint_service_configurations.html
- 3: https://awscli.amazonaws.com/v2/documentation/api/2.9.6/reference/ec2/delete-vpc-endpoints.html
- 4: https://docs.aws.amazon.com/cli/latest/reference/ec2/delete-vpc-endpoints.html
- 5: https://docs.aws.amazon.com/ec2/latest/devguide/example_ec2_DeleteVpcEndpoints_section.html
- 6: https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/services/ec2/model/DeleteVpcEndpointsResponse.html
- 7: https://sdk.amazonaws.com/java/api/2.41.1/software/amazon/awssdk/services/ec2/model/DeleteVpcEndpointServiceConfigurationsResponse.html
Treat partial EC2 batch failures as errors.
DeleteVpcEndpoints and DeleteVpcEndpointServiceConfigurations can return per-item failures in Unsuccessful even when the request error is nil, so returning only the transport error treats partial deletions as success. Inspect the response and retry or surface the failed IDs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contrib/aws-leaked-resources/delete.go` around lines 296 - 300, Update the
retryDelete callbacks in DeleteVpcEndpoints and
DeleteVpcEndpointServiceConfigurations to inspect each EC2 response’s
Unsuccessful entries and return an error containing the failed resource IDs when
any exist, even if the request error is nil. Preserve transport-error handling
and allow retryDelete to retry these partial failures.
| func (d *Deleter) collectNATGatewayEIPs(ctx context.Context, vpcID string) []string { | ||
| out, err := d.EC2.DescribeNatGateways(ctx, &ec2.DescribeNatGatewaysInput{ | ||
| Filter: []ec2types.Filter{ | ||
| {Name: aws.String("vpc-id"), Values: []string{vpcID}}, | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Propagate dependency-cleanup errors instead of discarding them.
These paths suppress discovery, detach, revoke, release, and IAM deletion errors. Most critically, a failed EIP lookup/release can leave a billable allocation after the VPC disappears, and a failed instance-profile deletion can leave an orphan after the role is successfully deleted.
Return or aggregate these errors through deleteVPC and deleteIAMRole rather than only logging them.
As per coding guidelines, “Always check errors — don’t ignore them” and “Wrap errors with context when rethrowing.”
Also applies to: 419-431, 450-455, 488-491, 516-521, 568-575, 819-857
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contrib/aws-leaked-resources/delete.go` around lines 395 - 403, Update
collectNATGatewayEIPs and the related cleanup paths at the referenced symbols to
stop discarding discovery, detach, revoke, release, and IAM deletion errors.
Propagate or aggregate each failure through deleteVPC and deleteIAMRole,
wrapping returned errors with operation-specific context while preserving
successful cleanup behavior and ensuring EIP and instance-profile failures reach
the caller.
Source: Coding guidelines
| func (d *Deleter) deleteHostedZone(ctx context.Context, zoneID, zoneName string) error { | ||
| var changes []route53types.Change | ||
| input := &route53.ListResourceRecordSetsInput{ | ||
| HostedZoneId: aws.String(zoneID), | ||
| } | ||
| for { | ||
| rrOut, err := d.Route53.ListResourceRecordSets(ctx, input) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for _, rr := range rrOut.ResourceRecordSets { | ||
| if rr.Type == route53types.RRTypeNs || rr.Type == route53types.RRTypeSoa { | ||
| continue | ||
| } | ||
| changes = append(changes, route53types.Change{ | ||
| Action: route53types.ChangeActionDelete, ResourceRecordSet: &rr, | ||
| }) | ||
| } | ||
|
|
||
| if !rrOut.IsTruncated { | ||
| break | ||
| } | ||
| input.StartRecordName = rrOut.NextRecordName | ||
| input.StartRecordType = rrOut.NextRecordType | ||
| input.StartRecordIdentifier = rrOut.NextRecordIdentifier | ||
| } | ||
|
|
||
| if len(changes) > 0 { | ||
| if err := retryDelete("records in "+zoneName, func() error { | ||
| _, err := d.Route53.ChangeResourceRecordSets(ctx, &route53.ChangeResourceRecordSetsInput{ | ||
| HostedZoneId: aws.String(zoneID), | ||
| ChangeBatch: &route53types.ChangeBatch{Changes: changes}, | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 'ChangeResourceRecordSets|ResourceRecordSets|1000|32000' contrib/aws-leaked-resourcesRepository: openshift/hypershift
Length of output: 2058
🌐 Web query:
AWS Route53 ChangeResourceRecordSets 1000 ResourceRecord elements 32000 characters limit
💡 Result:
When using the AWS Route53 ChangeResourceRecordSets API, your request is subject to two specific constraints regarding the size and number of resource records [1]: 1. Maximum ResourceRecord Elements: A single ChangeResourceRecordSets request cannot contain more than 1,000 ResourceRecord elements in total [1]. This limit includes all alias records present in the request [1]. 2. Maximum Character Count: The total length of all Value elements combined within a single request must not exceed 32,000 characters [1]. Additionally, it is important to note that for individual DNS records, the Value string itself cannot exceed 4,000 characters [2][3]. If your request exceeds these thresholds, the API will reject the operation; you should structure your updates to remain within these limits by batching your changes accordingly [1].
Citations:
- 1: https://docs.aws.amazon.com/Route53/latest/DeveloperGuide/DNSLimitations.html
- 2: https://docs.aws.amazon.com/boto3/latest/reference/services/route53/client/change_resource_record_sets.html
- 3: https://docs.aws.amazon.com/botocore/latest/reference/services/route53/client/change_resource_record_sets.html
Split Route53 deletions into quota-safe batches. A single ChangeResourceRecordSets request is capped at 1,000 ResourceRecord elements and 32,000 total value characters, so accumulating an entire hosted zone here can fail for large or multi-value zones. Batch by both limits before deleting the zone.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@contrib/aws-leaked-resources/delete.go` around lines 594 - 627, Update
deleteHostedZone to avoid accumulating all record deletions into one
ChangeResourceRecordSets request. Build and submit quota-safe batches while
iterating records, keeping each batch within both 1,000 ResourceRecord elements
and 32,000 total value characters, and reuse the existing retryDelete behavior
for each batch. Preserve skipping NS and SOA records and ensure the final
partial batch is deleted.
|
/acknowledge-critical-fixes-only |
csrwng
left a comment
csrwng
left a comment
There was a problem hiding this comment.
Nice tool — safety gates and cascade ordering look solid. A few nits below, nothing blocking.
| // retryDelete executes fn up to maxRetries times with retryDelay between attempts. | ||
| // Respects context cancellation between retries so Ctrl+C works immediately. | ||
| func retryDelete(desc string, fn func() error) error { | ||
| return retryDeleteWithCtx(context.Background(), desc, fn) |
There was a problem hiding this comment.
nit: retryDelete wraps retryDeleteWithCtx with context.Background(), so the retry sleep (line 143) won't wake up on Ctrl+C — it blocks for the full 2s delay. The actual AWS API calls inside fn() do use the real ctx from their closure, so cancellation eventually propagates, but the gap contradicts the stated "Ctrl+C interrupts immediately during retries" behavior.
All callers in deleteVPC and its sub-methods (deleteLoadBalancers, deleteNATGateways, etc.) use retryDelete instead of retryDeleteWithCtx with the proper context. Consider removing the retryDelete wrapper and passing ctx through everywhere.
| return retryDeleteWithCtx(context.Background(), desc, fn) | |
| func retryDelete(ctx context.Context, desc string, fn func() error) error { | |
| return retryDeleteWithCtx(ctx, desc, fn) | |
| } |
There was a problem hiding this comment.
Done — removed the retryDelete wrapper entirely. The function now takes ctx directly, and all 18 callers updated. Ctrl+C interrupts retry sleeps immediately.
| return "", "" | ||
| } | ||
|
|
||
| func (s *Scanner) s3DocExists(ctx context.Context, bucket, _, infraID string) bool { |
There was a problem hiding this comment.
nit: the region parameter is named _ and unused — the S3 client is already initialized with the config region. Dead parameter.
There was a problem hiding this comment.
Done — removed the unused region parameter from s3DocExists and updated both callers.
| const ( | ||
| VerdictProtected Verdict = "PROTECTED" | ||
| VerdictTooYoung Verdict = "TOO_YOUNG" | ||
| VerdictUntagged Verdict = "UNTAGGED" |
There was a problem hiding this comment.
nit: VerdictUntagged is defined here but no code path in scan.go ever assigns it. It's referenced in report sorting/summary output, but never produced. Dead code — either remove it or add the gate that produces it.
There was a problem hiding this comment.
Done — removed VerdictUntagged from types.go, the sort order in report.go, and the summary line in PrintSummary.
|
/acknowledge-critical-fixes-only |
Add a standalone Go tool under contrib/aws-leaked-resources/ that scans for and optionally deletes leaked AWS infrastructure from CI e2e test runs. The tool identifies VPCs, hosted zones, OIDC providers, and IAM roles left behind by failed or interrupted HyperShift CI jobs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
7942e1d to
40eb21c
Compare
|
@jparrill: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/lgtm |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
/verified by @jparrill The AWS Cleanup (the right one) was done via this utility. |
openshift-ci-robot
added
the
verified
|
@jparrill: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/label acknowledge-critical-fixes-only |
openshift-ci
Bot
added
the
acknowledge-critical-fixes-only
5 participants
Summary
contrib/aws-leaked-resources/cleanleaked) that scans AWS VPCs, classifies them through safety gates, and deletes leaked CI resources with full cascade supporttriage-leaked-infra) for interactive assessment of individual infra setsContext
On 2026-07-06, ad-hoc bash cleanup scripts accidentally deleted NAT gateways belonging to ci-2/ci-3 management clusters due to a bash
IFSfield-parsing bug, causing a multi-hour CI outage. This tool addresses the root causes with testable classification logic, protection tags, and safety invariants verified by unit tests.Classification Gates
do-not-deletetag, protected VPC name, protected usernameexpirationDatetag or VPCE/ENI timestamp < min-ageFeatures
--confirm(CI-safe),--interactive(per infra set with full inventory)--targetfor single infra set operations,--limitfor batch control,--dry-runCleanup Results (2026-07-08)
Test plan
go build ./contrib/aws-leaked-resources/compilesgo test ./contrib/aws-leaked-resources/...— all unit tests passgo vet ./contrib/aws-leaked-resources/...— no issuesdo-not-delete=truetag always results in PROTECTED verdict--dry-runverified against real AWS — no resources modified--interactivemode shows full resource inventory before promptingFixes CNTRLPLANE-3820
🤖 Generated with Claude Code
Summary by CodeRabbit
cleanleakedworkflow to scan and safely delete leaked AWS HyperShift CI infrastructure with verdict-based gating and dry-run/confirmation modes.cleanleakeddocumentation covering scan/delete modes, flags, and safety constraints.