fix(cli): deployment stops on AccessDenied looking up bootstrap stack #26925
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
bug
rix0rrr
had a problem deploying
to
test-pipeline
GitHub Actions
Failure
rix0rrr
changed the title
aws-cdk-automation
previously requested changes
Aug 29, 2023
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
aws-cdk-automation
added
the
pr/needs-cli-test-run
rix0rrr
added
the
pr-linter/cli-integ-tested
aws-cdk-automation
removed
the
pr/needs-cli-test-run
rix0rrr
added
pr-linter/exempt-integ-test
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
aws-cdk-automation
added
the
pr/needs-maintainer-review
mrgrain
temporarily deployed
to
test-pipeline
GitHub Actions
Inactive
|
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
mrgrain
approved these changes
Sep 6, 2023
aws-cdk-automation
removed
the
pr/needs-maintainer-review
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
rix0rrr
had a problem deploying
to
test-pipeline
GitHub Actions
Failure
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
mikewrighton
pushed a commit
that referenced
this pull request
Sep 14, 2023
…#26925) The CLI always looks up the default bootstrap stack, for backwards compatibility reasons: in case the attributes introduced by the V2 `DefaultStackSynthesizer` that tell it what SSM parameter to use and what bucket to write assets to are not present, it needs to fall back to the default bootstrap stack found in CloudFormation. The code happily survives a `StackNotFound` error, but is not prepared to deal with an `AccessDenied` error, that a customer in #26588 had configured their AWS account for. The essence of the fix here is to catch all errors when looking up the toolkit stack, because they only become relevant if any of the properties of the toolkit stack are ever accessed. The customer also made the point that the lookup didn't even need to happen in the first place, because all information was already there. This is fair, and the organization of the code in this area has been a thorn in my side for a while now. There is some code that doesn't need to be on `ToolkitInfo` (which is the ancient name for the Bootstrap Stack), but is there for legacy reasons. This PR introduces a refactor, where we introduce a new class `EnvironmentResources`, that manages interacting with the bootstrap resources in a particular environment. We can now pass `EnvironmentResources` everywhere we used to pass `ToolkitInfo`, and the actual lookup of the Bootstrap Stack is only triggered if the need arises (which hopefully should be never). Closes #26588. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The CLI always looks up the default bootstrap stack, for backwards compatibility reasons: in case the attributes introduced by the V2
DefaultStackSynthesizerthat tell it what SSM parameter to use and what bucket to write assets to are not present, it needs to fall back to the default bootstrap stack found in CloudFormation.The code happily survives a
StackNotFounderror, but is not prepared to deal with anAccessDeniederror, that a customer in #26588 had configured their AWS account for.The essence of the fix here is to catch all errors when looking up the toolkit stack, because they only become relevant if any of the properties of the toolkit stack are ever accessed.
The customer also made the point that the lookup didn't even need to happen in the first place, because all information was already there. This is fair, and the organization of the code in this area has been a thorn in my side for a while now. There is some code that doesn't need to be on
ToolkitInfo(which is the ancient name for the Bootstrap Stack), but is there for legacy reasons.This PR introduces a refactor, where we introduce a new class
EnvironmentResources, that manages interacting with the bootstrap resources in a particular environment. We can now passEnvironmentResourceseverywhere we used to passToolkitInfo, and the actual lookup of the Bootstrap Stack is only triggered if the need arises (which hopefully should be never).Closes #26588.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license