Skip to content
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

(core): add setter for termination protection #14463

Closed
rittneje opened this issue Apr 30, 2021 · 10 comments · Fixed by #26992
Closed

(core): add setter for termination protection #14463

rittneje opened this issue Apr 30, 2021 · 10 comments · Fixed by #26992
Labels
@aws-cdk/core Related to core CDK functionality feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md p2

Comments

@rittneje
Copy link

Please add a setter for the termination protection flag to the Stack class. https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_core.Stack.html#terminationprotection

Use Case

We would like to be able to automatically enable termination protection to all of our stacks via an Aspect. However, since there is no setter for this property (it can only be set in the constructor), there's no way for that to work.


This is a 🚀 Feature Request

@rittneje rittneje added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Apr 30, 2021
@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label Apr 30, 2021
@rix0rrr rix0rrr added good first issue Related to contributions. See CONTRIBUTING.md p2 labels May 10, 2021
@rix0rrr rix0rrr removed their assignment May 10, 2021
@jashgala
Copy link

jashgala commented May 10, 2021

Not sure if this will solve your problem, but have you considered creating an aspect that loops over your stacks and sets termination protection over them?

e.g.

export class DisableTerminationProtectionAspect implements IAspect {
  visit(construct: IConstruct): void {
    if (Stack.isStack(construct)) {
      (<any>construct).terminationProtection = false;
    }
  }
}

And then add the aspect to all your stacks which you are defining:

for (const stack of listOfStacks) {
      Aspects.of(stack).add(new DisableTerminationProtectionAspect());
}

@rittneje
Copy link
Author

@jashgala That’s what we want to do, but it won’t work right now because the terminationProtection field is read-only, hence this request.

@jashgala
Copy link

@rittneje

I tried testing this behavior locally and here's a easy to re-produce example:
(Tested with AWS CDK version 1.102.0 build a75d52f - currently latest on NPM)

import * as cdk from '@aws-cdk/core';
import * as s3 from '@aws-cdk/aws-s3';
import { Aspects, IAspect, IConstruct, Stack } from '@aws-cdk/core';

export class DisableTerminationProtectionAspect implements IAspect {
  visit(construct: IConstruct): void {
    if (Stack.isStack(construct)) {
      (<any>construct).terminationProtection = false;
    }
  }
}

export class CdkPlaygroundStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);
    const bucket = new s3.Bucket(this, 'Bucket');
  }
}

const app = new cdk.App();

const stack1 =new CdkPlaygroundStack(app, 'CdkPlaygroundStack1', {
  env: {
    account: process.env.ACCOUNT_ID,
    region: process.env.REGION,
  },
  terminationProtection: true
});


const stack2 = new CdkPlaygroundStack(app, 'CdkPlaygroundStack2', {
  env: {
    account: process.env.ACCOUNT_ID,
    region: process.env.REGION,
  },
  terminationProtection: true
});


Aspects.of(stack1).add(new DisableTerminationProtectionAspect());
Aspects.of(stack2).add(new DisableTerminationProtectionAspect());

When I deployed the above without the last two lines, the stacks deployed with termination protection enabled. However, after adding the last two lines and re-deploying, the stacks have their termination protection disabled.

Somehow the aspect is working fine without needing a separate setter.

@rittneje
Copy link
Author

Then it sounds like there is a bug in CDK allowing you to set purportedly read-only fields.

For example, the Java documentation does not have a setter. https://docs.aws.amazon.com/cdk/api/latest/java/index.html?software/amazon/awscdk/core/Stack.html

Neither does the C# documentation. https://docs.aws.amazon.com/cdk/api/latest/dotnet/api/Amazon.CDK.Stack.html#Amazon_CDK_Stack_TerminationProtection

Even the TypeScript code claims it is read-only.

/**
* Whether to enable termination protection for this stack.
*
* @default false
*/
readonly terminationProtection?: boolean;

cjihrig added a commit to cjihrig/aws-cdk that referenced this issue May 16, 2021
This commit adds a getter and setter for Stack termination
protection. This allows termination protection to be configured
after the Stack has been constructed.

fixes aws#14463
@NGL321 NGL321 removed the needs-triage This issue or PR still needs to be triaged. label Jul 24, 2021
@rittneje
Copy link
Author

ping @rix0rrr

@github-actions
Copy link

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Sep 22, 2022
@rittneje
Copy link
Author

Do not close.

@github-actions github-actions bot removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Sep 22, 2022
@comcalvi comcalvi changed the title (core.Stack): add setter for termination protection (core): add setter for termination protection Jan 25, 2023
@msambol
Copy link
Contributor

msambol commented Aug 30, 2023

@rittneje Can I take this?

@rittneje
Copy link
Author

Go for it.

@mergify mergify bot closed this as completed in #26992 Sep 19, 2023
mergify bot pushed a commit that referenced this issue Sep 19, 2023
Closes #14463.
Closes #21304.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

HBobertz pushed a commit that referenced this issue Sep 19, 2023
Closes #14463.
Closes #21304.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/core Related to core CDK functionality feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md p2
Projects
None yet
5 participants