feat(eks): add deletionProtection property to Cluster construct#36474
feat(eks): add deletionProtection property to Cluster construct#36474matoom-nomu wants to merge 12 commits intoaws:mainfrom
Conversation
|
|
||||||||||||||
|
|
||||||||||||||||||
…at/add-deletionProtection-to-ClusterOptions
badmintoncryer
left a comment
There was a problem hiding this comment.
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.
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.
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.
Is it better to use Match.absent()?
There was a problem hiding this comment.
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.
Because false is the default setting, it may be better to remove this line.
| diffAssets: false, |
There was a problem hiding this comment.
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.
Have you forgotten to push the modification?
| cdkCommandOptions: { | ||
| deploy: { | ||
| args: { | ||
| rollback: true, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Is this condition needed? I think rollback: true is default setting for cdk deploy.
| }, | ||
| }); | ||
|
|
||
| app.synth(); |
There was a problem hiding this comment.
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.
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.
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.
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.
Could you please add the link to the documentation?
https://docs.aws.amazon.com/eks/latest/userguide/deletion-protection.html
…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!! |
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? |
…at/add-deletionProtection-to-ClusterOptions
…://github.com/matoom-nomu/aws-cdk into feat/add-deletionProtection-to-ClusterOptions
438ba33 to
b34aff4
Compare
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