fix: warn before removing shared BPA exclusion - #104
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dae97ac6d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| resources=$(aws cloudformation describe-stack-resources \ | ||
| --stack-name "$stack_name" --region "$SCAN_REGION" \ | ||
| --query "StackResources[?ResourceType=='AWS::EC2::VPCBlockPublicAccessExclusion'].[LogicalResourceId,PhysicalResourceId]" \ |
There was a problem hiding this comment.
For deployments produced from this commit, this query always returns empty because a repo-wide search shows that neither deploy/cloudformation/template.yaml nor the installer defines AWS::EC2::VPCBlockPublicAccessExclusion, VpcBpaExclusion, ExistingVpcBpaExclusion, or the advertised CreateVpcBpaExclusion parameter; those identifiers exist only in this new uninstaller code and its fake test. Consequently vpc_bpa_lifecycle never returns owned:*, so the destructive flow still emits no warning unless users happen to have deployed an external/unmerged template. Include the prerequisite template change in this commit's ancestry or detect the actual resource representation used by shipped deployments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This issue is explicitly a follow-up to PR #102: the BPA template resources are intentionally not on main yet. The uninstaller does not need that template in this commit's tree to detect the deployed representation.
vpc_bpa_lifecycle calls cloudformation describe-stack-resources against the live target stack and filters by the runtime resource type AWS::EC2::VPCBlockPublicAccessExclusion; it then distinguishes the deployed VpcBpaExclusion and ExistingVpcBpaExclusion logical resources. Before PR #102 is deployed, the live stack has no such resource and the helper returns none, so the warning is inert. After PR #102 is deployed, the same read-only query returns the owned or retained resource and the warning path is active. The regression test stubs those exact describe-stack-resources responses, including the retained case.
Copying PR #102's template into this follow-up would duplicate the parent PR's change and create two competing sources of truth. I recommend keeping this as a follow-up against the live resource representation introduced by #102.
dae97ac to
8effed4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8effed46f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| --query 'Reservations[].Instances[?State.Name!=`terminated`].[InstanceId, Tags[?Key==`loki:watermark`].Value|[0]]' \ | ||
| --output text 2>/dev/null || echo "") |
There was a problem hiding this comment.
When ec2:DescribeInstances is denied or the API request fails, this fallback converts the error into an empty result, so vpc_has_other_lowkey_deployment reports that the VPC is unshared and suppresses the safety warning. An operator can still have permission to call CloudFormation DeleteStack—especially when the stack has an execution role—so the subsequent deletion can succeed and remove the stack-owned BPA exclusion while another deployment remains. Surface the failed inspection or fail closed instead of treating it as evidence that no other deployment exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 86c5c34f0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| stacks=$(aws cloudformation list-stacks \ | ||
| --stack-status-filter CREATE_COMPLETE UPDATE_COMPLETE \ | ||
| --region "$SCAN_REGION" \ | ||
| --query 'StackSummaries[].StackName' --output text 2>/dev/null || echo "") |
There was a problem hiding this comment.
When list-stacks is throttled or transiently fails during confirmation, this fallback converts the error into an empty stack list, so vpc_bpa_lifecycle reports no owned exclusion and suppresses the safety warning. try_delete_cfn_stack repeats the lookup after confirmation and may then succeed, deleting the owning stack and its BPA exclusion while another deployment still uses the VPC. Propagate an indeterminate status and warn or fail closed, as the EC2 inspection path now does.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1112010397
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| while IFS=$'\t' read -r _ watermark; do | ||
| [[ -z "$watermark" || "$watermark" == "None" ]] && continue | ||
| [[ "$watermark" != "$selected_watermark" ]] && return 0 |
There was a problem hiding this comment.
When two stacks use the same LokiWatermark—for example, direct CloudFormation deployments that both retain the template default lowkey—the second instance is ignored here even if it belongs to a different stack. The function then returns 1, so warn_shared_vpc_bpa suppresses the warning before deleting the original VPC-owning stack and its BPA exclusion, potentially cutting off the remaining deployment. Identify the selected deployment by stack or instance identity rather than assuming watermarks are unique.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db6c093204
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| local stack_name stack_vpc resources | ||
| local stacks | ||
| if ! stacks=$(aws cloudformation list-stacks \ | ||
| --stack-status-filter CREATE_COMPLETE UPDATE_COMPLETE \ |
There was a problem hiding this comment.
When the VPC-owning stack is in the stable and common UPDATE_ROLLBACK_COMPLETE state, this filter omits it, so vpc_bpa_lifecycle emits none and suppresses the shared-VPC warning even though the stack still owns its VPC and BPA exclusion. The same restricted filter in try_delete_cfn_stack then misses the stack and permits resource-by-resource cleanup, so another deployment in that VPC can be disrupted without the warning this change adds. The AWS CLI reference specifies that this option lists only stacks with the supplied status codes; include all live/stable statuses such as UPDATE_ROLLBACK_COMPLETE, or inspect stacks without this narrow filter.
Useful? React with 👍 / 👎.
Warn during destructive review when a stack-owned VPC BPA exclusion may be removed while another managed deployment remains in the VPC. Retained reused-VPC exclusions and unshared VPCs stay silent. Crew: Bode (Kiro Crew Issue Radar)
Crew: Bode (Kiro Crew Issue Radar)
Crew: Bode (Kiro Crew Issue Radar)
Crew: Bode (Kiro Crew Issue Radar)
Crew: Bode (Kiro Crew Issue Radar)
e935a16 to
8f4f23f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f4f23fda9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ! stack_vpc=$(aws cloudformation describe-stack-resources \ | ||
| --stack-name "$stack_name" --region "$SCAN_REGION" \ | ||
| --query "StackResources[?ResourceType=='AWS::EC2::VPC'].PhysicalResourceId" \ | ||
| --output text 2>/dev/null || echo "") | ||
| --output text 2>/dev/null); then | ||
| warn "Could not inspect CloudFormation stack ${stack_name} for VPC ${vpc_id}; refusing to remove it." | ||
| return 2 |
There was a problem hiding this comment.
When an account contains an unrelated live stack that the caller cannot describe, this branch returns 2 before reaching the selected deployment, so remove_deployment aborts even when the caller has full access to the target stack. The AWS CLI documentation notes that omitting --stack-status-filter returns all stacks (with the JMESPath expression here only removing deleting/deleted ones), making any unrelated permission error or transient failure block every CloudFormation uninstall; narrow the candidates using the selected VPC's CloudFormation tags or otherwise distinguish inaccessible unrelated stacks. See the list-stacks option documentation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 64a8dc8. The uninstaller now resolves the selected VPC’s aws:cloudformation:stack-name tag and queries only that stack; it no longer enumerates unrelated live stacks. Tag or selected-stack inspection failures remain fail-closed. Added an inaccessible-unrelated-stack regression: the pre-fix implementation fails it, while the current suite passes 16/16.
Crew: Bode (Kiro Crew Issue Radar)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64a8dc8b24
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| warn "Could not verify whether VPC ${vpc_id} has a stack-owned BPA exclusion." | ||
| warn "Treating the VPC as potentially shared; review the deployment before continuing." | ||
| fi | ||
| return 0 |
There was a problem hiding this comment.
When describe-stack-resources fails but describe-tags and delete-stack remain permitted—for example, with a restricted operator and a CloudFormation execution role—this returns success after only a generic warning, and try_delete_cfn_stack subsequently deletes the resolved stack without rechecking its BPA resources. A shared VPC can therefore lose its stack-owned exclusion despite the intended fail-closed behavior. Fresh evidence beyond the earlier finding is that the new direct-tag lookup in try_delete_cfn_stack no longer performs the lifecycle API call whose failure must block deletion; retain this indeterminate state and refuse deletion.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b6b1337. try_delete_cfn_stack now rechecks the selected stack’s VPC resource after resolving its stack-name tag and returns status 2 before delete-stack if that inspection fails or does not match the target VPC. The new regression fails against 64a8dc8 and the current focused suite passes 18/18.
Crew: Bode (Kiro Crew Issue Radar)
1 participant
Summary
Root cause
The uninstaller's confirmation plan did not account for the accepted PR #102 lifecycle limitation: a new-VPC stack can own an exclusion that is deleted with the stack while another deployment still uses the VPC. The warning is read-only and runs before the final confirmation prompt.
Verification
bash tests/test-uninstall-bpa.sh— 5/5 passed.mainfails withwarn_shared_vpc_bpa: command not found.bash -n install.sh uninstall.sh tests/test-uninstall-bpa.sh— passed.shellcheck --severity=error install.sh uninstall.sh tests/test-uninstall-bpa.sh— passed.git diff --check— passed.The remote branch rule reported merge commit
094f2099cfc3fc6fd25401eb8733742125a494f1, which is inherited frommain; this branch adds no merge commits.Fixes #103