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

1Pager: stateful resources #398

Closed
wants to merge 8 commits into from
Closed

Conversation

iliapolo
Copy link
Contributor

@iliapolo iliapolo commented Jan 6, 2022

This is a request for initial feedback about a possible RFC for modeling stateful resources in the CDK core framework.

Rendered Version


By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache-2.0 license


To retain stateful resources, authors must remember to configure those policies
with the `Retain` value. Having to remember this makes it also easy to forget.
This means that stateful resources might be shipped with the incorrect policy.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of this has been implemented in aws/aws-cdk#12920

Copy link
Contributor Author

@iliapolo iliapolo Jan 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking a look.

implementation of the `autoDeleteObjects` property. Then, when the resource was
removed due to the toggle, we would identify this as a removal of a stateful resource, and block it.

### Curating a list of stateful resources is a maintenance burden - can we do it differently?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modifying the file should be straightforward and infrequent. So I think it's only a burden to the extent that people can forget to update it when creating a new stateful resource. Is there any mechanism to remind developers to check this (like a lint rule)?

@iliapolo
Copy link
Contributor Author

iliapolo commented Jan 6, 2022

@rix0rrr Has just informed my about Stack Policies so i'm going to do some research into this and see how it affects my proposal.


### Policy Change

CDK applications are often comprised out of many third-party resources.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand what this paragraph is trying to say.

Do you wish to continue (y/n)?
```

Blocking these types of changes will keep CloudFormation templates in a safe state.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CloudFormation Stack Policies will be able to prevent updates. We would mark certain resources as "protected" in the CDK application.

Of course, we will need a way to indicate that you are okay with making certain changes.

  • The simplest solution is to stop marking resources as "protected" in the template, and do a regular deployment.
    • In fact, in practice it would take 2 deployments: (1) switch off the protection (2) remove the resource.
  • Slightly more advanced would be a CLI confirmation prompt, but then the CLI needs to be able to look into and parse and modify stack policies. Also it needs extra effort in a CI/CD pipeline.

constructor(scope: Construct, id: string, props: CfnResourceProps) {
super(scope, id);
this.stateful = props.stateful;
this.replacedIf = props.replacedIf;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think replacedIf is probably unnecessary when using stack policies.


constructor(scope: cdk.Construct, id: string, props: CfnBucketProps = {}) {
super(scope, id, {
stateful: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go the "stack policies" route, we need a way to turn off the protection, in a way that is user controllable (when they finally do want to get rid of a stateful resource).

Maybe it's a sort of removalPolicy though:

  • RemovalPolicy.DESTROY
  • RemovalPolicy.RETAIN
  • RemovalPolicy.PROTECTED

When you want to get rid of it, you switch to RemovalPolicy.DESTROY first, then remove it.


The experience described here lays under the following assumptions:

1. The correct policy for stateful resources is always `Retain`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this be more appropriate? Presumably there are cases users want a stateful resource without "Retain", such as buckets holding cached data that can be easily re-generated, etc. -- otherwise, we shouldn't expose it in the L2 at all.

Suggested change
1. The correct policy for stateful resources is always `Retain`.
1. The correct default policy for stateful resources is always `Retain`.

Comment on lines +103 to +107
> **Side note**: Explicitly differentiating stateful resources from stateless ones is a
> common practice in infrastructure modeling. For example, Kubernetes
> uses the [`StatefulSet`](https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/)
> resource for this.
> And terraform has the [`prevent_destroy`](https://www.terraform.io/language/meta-arguments/lifecycle#prevent_destroy) property to mark stateful resources.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useful context 👍

to occur on stateful resources in the stack. This closes the loop and should
allow for the [customer experience](#desired-customer-experience) we described.

## Q & A
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might not be enough room to include this since it's not a full RFC, but I think it could be helpful to add a short section on what alternatives were considered to addressing the customer pain and why they were not chosen over the suggested solution. These could be alternatives to the proposed CLI experience, or alternatives to the proposed API (like adding a IStateful interface instead), etc.

Comment on lines +213 to +215
### Why do we need the API change given we have the curated file?

Its true that the existence of the file allows us to codegen marked L1 resources
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarify what "the curated file" refers to, since this is a bit confusing if you read the document out of order

@mrgrain mrgrain closed this Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants