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

Protection and tools for managing stateful resources and avoiding deletion #901

Open
Chriscbr opened this issue Dec 14, 2022 · 7 comments
Open
Assignees
Labels
✨ enhancement New feature or request needs-discussion Further discussion is needed prior to impl 🎨 sdk SDK

Comments

@Chriscbr
Copy link
Contributor

Chriscbr commented Dec 14, 2022

RFC (Rev 1.)

Here's a proposal for a simple mechanism to prevent damage caused by logical name mapping changes.

If a resource is marked as stateful, it's Terraform resource name will be determined by looking up its path in a map stored in a <entrypoint>.w.lock file, next to the entrypoint file, which will be committed to the repository.

If an entry for this resource cannot be found in the lockfile, the compilation will fail and instruct the user to explicitly add an entry and provide a resource name.

Additionally, compilation will also fail if the file includes an entry that doesn't map to a stateful resource in the app.

This will ensure that:

  1. Stateful resources are explicitly named.
  2. If a stateful resource is moved within the app (e.g. it's path is changed), then users will easily be able to map the new path to the same stateful resource.
  3. If a stateful resource is accidentally removed, users will have to acknowledge this by explicitly removing the resource entry from the lockfile.

We can offer some CLI commands to edit the lockfile, but that's not P1.

Here's an example.

Say I write this Wing called hello.main.w:

bring cloud;

new cloud.Bucket();

Now, I compile:

$ wing compile -t tf-aws hello.main.w
ERROR: stateful resource "/root/Bucket" doesn't have a name in the lockfile.
Please edit `hello.main.w.lock`.
$ cat hello.main.w.lock
/root/Bucket: <NEW>
$ edit hello.main.w.lock
/root/Bucket: my_s3_bucket
$ wing compile -t tf-aws hello.main.w
$ # cool!

Now, let's say we refactored the app and we will move the same bucket into a class:

bring cloud;

class MyStore {
  new() {
    new cloud.Bucket();
  }
}

new MyStore();
$ wing compile -t tf-aws hello.main.w
ERROR: stateful resource "/root/MyStore/Bucket" doesn't have a name in the lockfile.
ERROR: stateful resource "/root/Bucket" with the name "my_s3_bucket" was deleted.
Please edit `hello.main.w.lock`.
$ cat hello.main.w.lock
/root/Bucket: my_s3_bucket <DELETED>
/root/MyStore/Bucket: <NEW>
$ edit hello.main.w.lock
/root/MyStore/Bucket: my_s3_bucket
$ wing compile -t tf-aws hello.main.w

At some point we can be smarter and offer a nice DX:

It seems like the stateful resource "my_s3_bucket" was moved
from "/root/Bucket" to "/root/MyStore/Bucket".
Type YES to accept: YES_

We can also offer some CLI commands to update:

$ wing state new /root/Bucket my_s3_bucket
$ wing state mv /root/Bucket /root/MyStore/Bucket
$ wing state rm /root/Bucket
$ # etc

Original Feature Spec

Each Wing resource has a path that represents its unique address within the resource tree. This path is used to produce a deterministic Terraform identifier for each resource (which is what Terraform uses in its state file to map to the physical resource).

When refactoring code and resources are moved around, their Terraform identifier could change. In certain cases, especially for stateful resources with important data, this could be hazardous.

Wing resources can be explicitly marked as "stateful" or "stateless". If a resource is marked as "stateful", Wing will protect it from being accidentally deleted during deployment, and will offer a way to associated the

We provide some mechanism for you to relocate the terraform identifier of a resource in order to link the old identifier to the newly placed resource.

Use Cases

In some cases, users may want their Buckets to be emptied when their app is destroyed, while in other cases users may want their Buckets to be retained if their app is destroyed. (And in further cases, users may want to configure this on a per-Bucket basis).

The same general pattern applies to any stateful resource, including Counter etc.

Implementation Notes

Sadly, and surprisingly, Terraform's prevent_destory attribute cannot be used to implement this feature:

Since this argument must be present in configuration for the protection to apply, note that this setting does not prevent the remote object from being destroyed if the resource block were removed from configuration entirely: in that case, the prevent_destroy setting is removed along with it, and so Terraform will allow the destroy operation to succeed.

This means that if the resource is completely removed from the configuration, it will be destroyed in the next apply.

There is a heated conversation in this issue.

Alternatives to consider:

  1. Use resource-specific capabilities. It seems like some providers offer explicit deletion protection (e.g sql_database_instance).
  2. Add a capability into Wing that will assist with tracking resource identifiers throughout iterations. Something like taking and storing snapshot during compilation that can be used during synthesis to identify/handle these cases.

Intuitively it feels like this is something Wing should handle regardless of the provisioning system, so I believe it's worth trying to find the right mechanism.

@Chriscbr Chriscbr added ✨ enhancement New feature or request 🎨 sdk SDK labels Dec 14, 2022
@eladb eladb changed the title Deletion policies Stateful resources and deletion policies Jan 25, 2023
@eladb eladb changed the title Stateful resources and deletion policies Protection and tools for managing stateful resources and avoiding deletion Jan 25, 2023
@eladb
Copy link
Contributor

eladb commented Jan 25, 2023

@ekeren @staycoolcall911 I suspect this is something we will need to consider as P1 for beta given the fact that Terraform doesn't offer support for this (so this can't even be solved using plugins).

@eladb
Copy link
Contributor

eladb commented Jan 25, 2023

I've bumped to P1 for now and we can discuss scope and priority for beta.

@Chriscbr
Copy link
Contributor Author

See also #1054

@ekeren
Copy link
Collaborator

ekeren commented Feb 20, 2023

This is p1 for RFC

@eladb eladb removed their assignment Apr 11, 2023
@staycoolcall911 staycoolcall911 added the needs-discussion Further discussion is needed prior to impl label Oct 4, 2023
@ekeren
Copy link
Collaborator

ekeren commented Dec 25, 2023

Did you consider making ID the key and not the value?

bring cloud;

new cloud.Bucket();
$ wing compile -t tf-aws hello.main.w
ERROR: stateful resource "/root/Bucket" doesn't have a name in the lockfile.
Please edit `hello.main.w.lock`.
$ cat hello.main.w.lock
<NEW>: /root/Bucket
$ edit hello.main.w.lock
my_s3_bucket: /root/Bucket
$ wing compile -t tf-aws hello.main.w
$ # cool!
bring cloud;

class MyStore {
  new() {
    new cloud.Bucket();
  }
}

new MyStore();
$ wing compile -t tf-aws hello.main.w
ERROR: stateful resource "/root/MyStore/Bucket" doesn't have a name in the lockfile.
ERROR: stateful resource "/root/Bucket" with the name "my_s3_bucket" was deleted.
Please edit `hello.main.w.lock`.
$ cat hello.main.w.lock
<NOT_FOUND> my_s3_bucket: /root/Bucket 
<NEW>: /root/MyStore/Bucket: 
$ edit hello.main.w.lock
my_s3_bucket: /root/MyStore/Bucket
$ wing compile -t tf-aws hello.main.w

My thinking that reading the diff file from left to right will be easier(starting with the key as the constant)

@skorfmann
Copy link
Contributor

some thoughts:

  • looks like this is inferred from the resource? Could I opt to do this manually?
  • this might lead to cases where this will still destroy the resource, e.g. when constructed attribute values (e.g. a bucket name based on some other input) are forcing a re-create of the resource.
  • there should be a way to ignore this via a global setting
  • perhaps a way to isolate stateful resources in dedicated tf configs / stacks would be useful as well. Smaller changesets, faster and generally best practice anyway. See also Terraform Stacks (Terraform BSL only). But generally speaking, something like CDKTF / AWS CDK Stacks.

@eladb
Copy link
Contributor

eladb commented Dec 25, 2023

looks like this is inferred from the resource? Could I opt to do this manually?

Not sure I understand... what's the use case? Are you referring to custom terraform resources?

this might lead to cases where this will still destroy the resource, e.g. when constructed attribute values (e.g. a bucket name based on some other input) are forcing a re-create of the resource.

Yes, good point. This is meant to prevent resource destruction caused by identity mapping. We should also address replacement protection, which I think terraform is very lousy about... I'll spin off another issue to discuss and focus this issue.

there should be a way to ignore this via a global setting

Yes, CLI should have wing compile --no-lockfile or something like this.

perhaps a way to isolate stateful resources in dedicated tf configs / stacks would be useful as well. Smaller changesets, faster and generally best practice anyway. See also Terraform Stacks (Terraform BSL only). But generally speaking, something like CDKTF / AWS CDK Stacks.

Yes. I am wondering if this is something we can implement as a platform provider, I'll open an issue to track.

eladb added a commit that referenced this issue Feb 8, 2024
Use only the class name instead of the fully qualified name as the default preflight object ID because the namespace (e.g. `foo` in `foo.MyClass`)
is a local name determined by the `bring` statement.

Fixes #5564

BREAKING CHANGE: caution (and apologies)! This change is going to cause a replacement of any resource that didn't have an explicit identity.
We are working to make it [safer and easier](#901) to control IDs of resources (especially ones with state).
@Chriscbr Chriscbr added this to the Winglang Stable Release milestone Feb 29, 2024
mergify bot pushed a commit that referenced this issue Mar 12, 2024
…5658)

Use only the class name instead of the fully qualified name as the default preflight object ID because the namespace (e.g. `foo` in `foo.MyClass`) is a local name determined by the `bring` statement.

Fixes #5564

BREAKING CHANGE: Caution ⚠️ (and apologies 🙏)! This change is going to cause a replacement of any resource that didn't have an explicit identity. We are working to make it [safer and easier](#901) to control IDs of resources (especially ones with state), but for now, if you wish to avoid the replacement, you'll need to add `as "cloud.Xxx"` to explicitly use the old naming. Don't hesitate to [ping us](https://t.winglang.io/slack) and we will help out with migration as needed.

## Checklist

- [x] Title matches [Winglang's style guide](https://www.winglang.io/contributing/start-here/pull_requests#how-are-pull-request-titles-formatted)
- [x] Description explains motivation and solution
- [x] Tests added (always)
- [x] Docs updated (only required for features)
- [x] Added `pr/e2e-full` label if this feature requires end-to-end testing

*By submitting this pull request, I confirm that my contribution is made under the terms of the [Wing Cloud Contribution License](https://github.com/winglang/wing/blob/main/CONTRIBUTION_LICENSE.md)*.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or request needs-discussion Further discussion is needed prior to impl 🎨 sdk SDK
Projects
Status: 🤝 Backlog - handoff to owners
Development

No branches or pull requests

6 participants