-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(imagebuilder-alpha): add support for Component Construct #36006
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
2b1ee8a to
b9d17a0
Compare
ozelalisen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments as first part of the review, I am not fully completed
| changeDescription: props.changeDescription, | ||
| description: props.description, | ||
| platform: props.platform, | ||
| kmsKeyId: props.kmsKey?.keyArn, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see any configuration for kmsKey, should we not configure permissions for grantEncryptDecrypt for the key to be able to use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I can add a method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please add an integ test regarding this, since this is only way we can test it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah i think i misunderstood. So, IB APIs are able to use the key so long as the caller (deployer of the CFN stack or the role associated with the stack) have permissions to do so. The only other principal that would need access is the instance profile role in the Infrastructure Configuration. We can grant the role permissions on the key automatically if you suggest (whenever we create an image or pipeline in the Image/ImagePipeline construct), or just add a grantDecrypt method on the component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding integ test - the key is actually only used during image builds. Given the construct does not exist yet for Image/ImagePipeline, we probably cannot test that the key permission grant works in an image build until we have those constructs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes sense to add grantDecrypt method on constructor when key exists.
Sounds good, we can add integ tests when those constructs created
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should go with the first approach where you grant permissions on the role within Image/Image Pipeline, not with exposing grantDecrypt on this interface, as it has basically no usage rather than exposing a public method, that kmsKey already does. Let's remove this grantMethod in this class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah got it - I had misunderstood. we can do that then
ozelalisen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Publishing second round of comments
packages/@aws-cdk/aws-imagebuilder-alpha/test/integ.all-parameters.component.ts
Outdated
Show resolved
Hide resolved
| changeDescription: props.changeDescription, | ||
| description: props.description, | ||
| platform: props.platform, | ||
| kmsKeyId: props.kmsKey?.keyArn, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please add an integ test regarding this, since this is only way we can test it
| const [componentNameFromArn, componentVersionFromArn] = (() => { | ||
| if (cdk.Token.isUnresolved(componentNameVersion)) { | ||
| const componentNameVersionSplit = cdk.Fn.split('/', componentNameVersion); | ||
| return [cdk.Fn.select(0, componentNameVersionSplit), cdk.Fn.select(1, componentNameVersionSplit)]; | ||
| } | ||
|
|
||
| const componentNameVersionSplit = componentNameVersion.split('/'); | ||
| return [componentNameVersionSplit[0], componentNameVersionSplit[1]]; | ||
| })(); | ||
|
|
||
| return [componentNameFromArn, componentVersionFromArn]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This overcomplicates the token validation with redundant checking. Fn.split and Fn.select already handle tokens internally. So you could basically simplify this as:
const componentNameVersionSplit = componentNameVersion.split('/');
return [componentNameVersionSplit[0], componentNameVersionSplit[1]];There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about this, I recommended wrong method, this should be with FN functions since they can be tokens:
const componentNameVersionSplit = cdk.Fn.split('/', componentNameVersion);
return [
cdk.Fn.select(0, componentNameVersionSplit),
cdk.Fn.select(1, componentNameVersionSplit),
];There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean here is that you do not need that second split on implementation, as Fn.split checks internally if componentNameVersion is token, so you do not need to provide second part where you use native typescript split function, using only Fn.select and Fn.split is sufficient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see - didn't know Fn worked like that! Updated
|
Just reminder for next PRs that title should include alpha suffix to be appeared in the CHANGELOG correctly |
b9d17a0 to
5667d1a
Compare
Pull request has been modified.
| const [componentNameFromArn, componentVersionFromArn] = (() => { | ||
| if (cdk.Token.isUnresolved(componentNameVersion)) { | ||
| const componentNameVersionSplit = cdk.Fn.split('/', componentNameVersion); | ||
| return [cdk.Fn.select(0, componentNameVersionSplit), cdk.Fn.select(1, componentNameVersionSplit)]; | ||
| } | ||
|
|
||
| const componentNameVersionSplit = componentNameVersion.split('/'); | ||
| return [componentNameVersionSplit[0], componentNameVersionSplit[1]]; | ||
| })(); | ||
|
|
||
| return [componentNameFromArn, componentVersionFromArn]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about this, I recommended wrong method, this should be with FN functions since they can be tokens:
const componentNameVersionSplit = cdk.Fn.split('/', componentNameVersion);
return [
cdk.Fn.select(0, componentNameVersionSplit),
cdk.Fn.select(1, componentNameVersionSplit),
];Pull request has been modified.
33fb075 to
0d7a7d4
Compare
c913c97 to
ac32fa8
Compare
Issue
aws/aws-cdk-rfcs#789
Reason for this change
This change adds a new alpha module for EC2 Image Builder L2 Constructs (
@aws-cdk/aws-imagebuilder-alpha), as outlined in aws/aws-cdk-rfcs#789. This PR specifically implements theComponentconstruct.Description of changes
This change implements the
Componentconstruct, which is a higher-level construct ofCfnComponent.Example
Describe any new or updated permissions being added
N/A - new L2 construct in alpha module
Description of how you validated changes
Validated with unit tests and integration tests. Manually verified generated CFN templates as well.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license