refactor: gate access to environment SDK behind new class#31904
refactor: gate access to environment SDK behind new class#31904mergify[bot] merged 11 commits intomainfrom
Conversation
Previously there were methods on the `Deployments` class that made it possible to directly get an SDK from the `SdkProvider` for a particular environment. Calling these methods made it possible to get an SDK without thinking of assuming roles to go into a different account. This PR introduces a new class, `EnvironmentAccess`, with a couple of public methods that are the only ones allowed to obtain SDKs with credentials. It has the methods: - accessStackForStackOperations(stack) - accessStackForLookup(stack) - accessStackForReading(stack) These will always respect the role information on the stack. Ideally there would have been similar methods for assets as well, but the `cdk-assets` library is entirely handling asset roles itself, and it's not in the scope of this PR to change that. That keeps on using a plain `SdkProvider`. Hotswap deployments will also just use CLI credentials and not assume role, so that also keeps on using an `SdkProvider`. All other uses have moved to `EnvironmentAccess`.
aws-cdk-automation
left a comment
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.
mrgrain
left a comment
There was a problem hiding this comment.
Looks great. My main issue is with the now seemingly required call of env.replacePlaceholders() by the caller. That is equally non obvious as the problem we were are trying to fix.
| }); | ||
| } | ||
|
|
||
| export type StringWithoutPlaceholders = Branded<string, 'NoPlaceholders'>; No newline at end of file |
There was a problem hiding this comment.
| export type StringWithoutPlaceholders = Branded<string, 'NoPlaceholders'>; | |
| export type StringWithoutPlaceholders = Branded<string, 'NoPlaceholders'>; | |
| return value as A; | ||
| } | ||
|
|
||
| type TypeUnderlyingBrand<A> = A extends Branded<infer T, any> ? T : never; No newline at end of file |
There was a problem hiding this comment.
| type TypeUnderlyingBrand<A> = A extends Branded<infer T, any> ? T : never; | |
| type TypeUnderlyingBrand<A> = A extends Branded<infer T, any> ? T : never; | |
| * @default - Role specified on stack, otherwise current | ||
| */ | ||
| readonly roleArn?: string; | ||
| readonly roleArn?: StringWithoutPlaceholders; |
There was a problem hiding this comment.
Maybe add a note how to convert a string to StringWithoutPlaceholders.
|
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter 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). |
|
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). |
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). |
|
Comments on closed issues and PRs are hard for our team to see. |
Previously there were methods on the
Deploymentsclass that made it possible to directly get an SDK from theSdkProviderfor a particular environment. Calling these methods made it possible to get an SDK without thinking of assuming roles to go into a different account.This PR introduces a new class,
EnvironmentAccess, with a couple of public methods that are the only ones allowed to obtain SDKs with credentials. It has the methods:These will always respect the role information on the stack.
Ideally there would have been similar methods for assets as well, but the
cdk-assetslibrary is entirely handling asset roles itself, and it's not in the scope of this PR to change that. That keeps on using a plainSdkProvider. Hotswap deployments will also just use CLI credentials and not assume role, so that also keeps on using anSdkProvider.All other uses have moved to
EnvironmentAccess.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license