-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(eks): add deletionProtection property to Cluster construct #36474
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?
feat(eks): add deletionProtection property to Cluster construct #36474
Conversation
|
|
||||||||||||||||||
|
|
||||||||||||||||||||||
…at/add-deletionProtection-to-ClusterOptions
badmintoncryer
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.
Thank you for your contribution!! I've added some comments.
| test.each([ | ||
| [true, true], | ||
| [false, false], | ||
| ])('deletionProtection(%s) should work', (input, expected) => { | ||
| // GIVEN | ||
| const { stack } = testFixture(); | ||
|
|
||
| // WHEN | ||
| new eks.Cluster(stack, 'Cluster', { | ||
| version: CLUSTER_VERSION, | ||
| deletionProtection: input, | ||
| kubectlLayer: new KubectlV31Layer(stack, 'KubectlLayer'), | ||
| }); | ||
|
|
||
| // THEN | ||
| Template.fromStack(stack).hasResourceProperties('Custom::AWSCDK-EKS-Cluster', { | ||
| Config: { | ||
| deletionProtection: expected, | ||
| }, | ||
| }); | ||
| }); |
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.
You can describe test conditions more simply.
| test.each([ | |
| [true, true], | |
| [false, false], | |
| ])('deletionProtection(%s) should work', (input, expected) => { | |
| // GIVEN | |
| const { stack } = testFixture(); | |
| // WHEN | |
| new eks.Cluster(stack, 'Cluster', { | |
| version: CLUSTER_VERSION, | |
| deletionProtection: input, | |
| kubectlLayer: new KubectlV31Layer(stack, 'KubectlLayer'), | |
| }); | |
| // THEN | |
| Template.fromStack(stack).hasResourceProperties('Custom::AWSCDK-EKS-Cluster', { | |
| Config: { | |
| deletionProtection: expected, | |
| }, | |
| }); | |
| }); | |
| test.each([ | |
| true, false | |
| ])('deletionProtection(%s) should work', (deletionProtection) => { | |
| // GIVEN | |
| const { stack } = testFixture(); | |
| // WHEN | |
| new eks.Cluster(stack, 'Cluster', { | |
| version: CLUSTER_VERSION, | |
| deletionProtection, | |
| kubectlLayer: new KubectlV31Layer(stack, 'KubectlLayer'), | |
| }); | |
| // THEN | |
| Template.fromStack(stack).hasResourceProperties('Custom::AWSCDK-EKS-Cluster', { | |
| Config: { | |
| deletionProtection, | |
| }, | |
| }); | |
| }); |
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've simplified the test descriptions as you suggested.
| // THEN | ||
| Template.fromStack(stack).hasResourceProperties('Custom::AWSCDK-EKS-Cluster', { | ||
| Config: Match.not(Match.objectLike({ | ||
| deletionProtection: Match.anyValue(), |
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.
Is it better to use Match.absent()?
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.
You're absolutely right, updated it to use Match.absent().
|
|
||
| new integ.IntegTest(app, 'aws-cdk-eks-cluster-deletion-protection-integ', { | ||
| testCases: [stack], | ||
| diffAssets: false, |
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.
Because false is the default setting, it may be better to remove this line.
| diffAssets: false, |
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.
The integ-test file I referenced was using diffAssets: false,
but as you mentioned it's the default setting, so I removed it.
I'll be more careful about checking default values next time.
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.
Have you forgotten to push the modification?
| cdkCommandOptions: { | ||
| deploy: { | ||
| args: { | ||
| rollback: true, | ||
| }, | ||
| }, | ||
| }, |
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.
Is this condition needed? I think rollback: true is default setting for cdk deploy.
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.
same as above
| }, | ||
| }); | ||
|
|
||
| app.synth(); |
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 line is not needed.
| app.synth(); |
| * When true, deletion protection is enabled and the cluster cannot be deleted until protection is disabled. | ||
| * When false, the cluster can be deleted normally. This setting only applies to clusters in an active state. | ||
| * | ||
| * @default - none |
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.
Could you please describe the default value or behavior that happens if this property is not supplied?
| * @default - none | |
| // default value | |
| * @default false | |
| // default behaviour | |
| * @default - deletion protection is disabled |
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 fixed comment to use
@default - deletion protection is disabled instead of @default - none
| * The current deletion protection setting for the cluster. | ||
| * When true, deletion protection is enabled and the cluster cannot be deleted until protection is disabled. | ||
| * When false, the cluster can be deleted normally. This setting only applies to clusters in an active state. | ||
| * |
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.
Could you please add @see section?
| * | |
| * | |
| * @see https://docs.aws.amazon.com/eks/latest/userguide/deletion-protection.html | |
| * |
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 added 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.
Could you please add the link to the documentation?
https://docs.aws.amazon.com/eks/latest/userguide/deletion-protection.html
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 added it.
…at/add-deletionProtection-to-ClusterOptions
|
Could you please resolve build error? aws-cdk-lib: /codebuild/output/src3139386751/src/actions-runner/_work/aws-cdk/aws-cdk/packages/aws-cdk-lib/aws-eks/lib/cluster.ts
aws-cdk-lib: 740:5 error Trailing spaces not allowed no-trailing-spaces
aws-cdk-lib: 742:5 error Trailing spaces not allowed no-trailing-spaces
aws-cdk-lib: /codebuild/output/src3139386751/src/actions-runner/_work/aws-cdk/aws-cdk/packages/aws-cdk-lib/aws-eks/test/cluster.test.ts
aws-cdk-lib: 3968:16 error Missing trailing comma @stylistic/comma-dangle |
Sorry for being late, I resoleved those linting errors. |
|
@matoom-nomu Thanks!
It may be not essential. Security guardian CI is introduced experimentally and I think it has no effect to review process. Could you please address this comment? I'll approve this PR after that. Have a happy new year!! |
badmintoncryer
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.
Thanks!
Thanks for the review, have a happy new year too! And it seems there is a build error due to the year change....
Shall i make the Issue ticket about this ? |
|
@matoom-nomu This is annual problem. How about submit PR like this? |
Issue # (if applicable)
Closes #36460
Reason for this change
AWS EKS supports deletion protection to prevent accidental cluster deletion, as documented in the CloudFormation reference.
Description of changes
deletionProtectionoptional property toClusterPropsinterfaceDescribe any new or updated permissions being added
None
Description of how you validated changes
Added unit/integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license