-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(eks): add removal policy support for EKS cluster construct #35560
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
Conversation
63c3f76
to
9a7715e
Compare
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 review is outdated)
68f0857
to
c4a5575
Compare
5b73c19
to
58e7399
Compare
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
05ddd5a
to
b9a6607
Compare
3cbc600
to
065fa26
Compare
* This affects the EKS cluster itself, associated IAM roles, node groups, security groups, VPC | ||
* and any other CloudFormation resources managed by this construct. | ||
* | ||
* @default - Resources will follow their default removal policies |
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.
Shouldn't the default value be RETAIN
here?
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 default is delete in CFN, i can update that.
67aab2c
to
f86be7a
Compare
readonly remotePodNetworks?: RemotePodNetwork[]; | ||
|
||
/** | ||
* The removal policy applied to all CloudFormation resources created by this construct |
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.
removal policy applied to all CloudFormation resources created by this construct
Is this intended? When removalPolicy
is set to DESTROY
, it could affect shared resources like VPC, security groups, etc. that might be getting used elsewhere?
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.
It should not affect the VPC passed by the user (if any), only if the construct created its own default vpc. I will add a unit test to verify that. RemovalPolicies.of(this).apply
affects resources created under the construct, not the ones passed to 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.
RemovalPolicies.of(this).apply affects resources created under the construct
But can these resources created by the construct be shared or referenced by other resources outside this construct?
For example, if this construct creates security groups or IAM roles that are later referenced by other services, applying DESTROY removal policy on such resources could cause issues.
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.
It is not expected that customers reuse the default resources created the construct on another service, instead they should have provided a role to the construct. Anyway the default is delete, and this is a known default by CFN. If they pass RETAIN, and the role/vpc are retained then it is also expected because the cluster cannot function without them. If a resource is referenced somewhere else in the stack, you cannot even delete it because that will fail the deployment and in CDK the typechecking/synth would also fail.
f86be7a
to
72ff301
Compare
72ff301
to
6f5aded
Compare
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). |
This pull request has been removed from the queue for the following reason: The pull request can't be updated. You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. |
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). |
@Mergifyio update |
❌ Mergify doesn't have permission to updateFor security reasons, Mergify can't update this pull request. Try updating locally. |
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). |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #25544
Reason for this change
Added the
removalPolicy
prop to the EKS cluster. This will apply the removal policy to all resources created by the cluster including node groups, roles, vpc and security groups. This also includes the custom resource that created the construct. Currently this is possible withRemovalPolicies.of(cluster).apply
but this is not a user friendly API, this PR just abstracts that with aremovalPolicy
prop which is the expected behavior when using L2s.Description of changes
Added
removalPolicy
property to theClusterProps
interface in the EKS library that allows users to specify a removal policy for all CloudFormation resources created by the EKS cluster construct.readonly removalPolicy?: RemovalPolicy
toClusterProps
interfaceinteg.eks-cluster-retain.ts
to verify removal policy functionality withRemovalPolicy.DESTROY
. If it deploys with destroy, it will also deploy with retain. The reason we don't write a specific retain integ test is that it will orphan resources in the account for anyone who deploys the integ test.The removal policy affects the EKS cluster itself, associated IAM roles, node groups, security groups, VPC resources, and any other CloudFormation resources managed by this construct.
Describe any new or updated permissions being added
No new IAM permissions are required for this change.
Description of how you validated changes
integ.eks-cluster-retain.ts
that creates an EKS cluster withRemovalPolicy.RETAIN
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license