rfc: resource pinning

[MarkMcCulloh]

#901

Rendered Version

By submitting this pull request, I confirm that my contribution is made under the terms of the Wing Cloud Contribution License.

[monadabot]

Thanks for opening this pull request! 🎉
Please consult the contributing guidelines for details on how to contribute to this project.
If you need any assistence, don't hesitate to ping the relevant owner over Discord.

Topic	Owner
Wing SDK and utility APIs	@chriscbr
Wing Console	@ainvoner, @skyrpex, @polamoros
JSON, structs, primitives and collections	@hasanaburayyan
Platforms and plugins	@hasanaburayyan
Frontend resources (website, react, etc)	@tsuf239
Language design	@eladb
VSCode extension and language server	@markmcculloh
Compiler architecture, inflights, lifting	@yoav-steinberg
Wing Testing Framework	@tsuf239
Wing CLI	@markmcculloh
Build system, dev environment, releases	@markmcculloh
Library Ecosystem	@chriscbr
Documentation	@hasanaburayyan
SDK test suite	@tsuf239
Examples	@skorfmann
Wing Playground	@eladcon

[eladb]

Is there a requirement to be able to control the policy? In CFN and TF there's a distinction between "disallow removal" and "leave orphan".

[MarkMcCulloh]

I'm not sure what this means in the context of the logical tree.

In the context of CFN/TF, I considered platform-level handling out of scope for this RFC. Presumably this is the sort of metadata that can be added to the pinfile next to.

[eladb]

Yes, sounds like the right direction to put this in the pinfile as additional attributes.

Might want to make sure the schema of this file allows it.

[MarkMcCulloh]

In CFN and TF there's a distinction between "disallow removal" and "leave orphan".

Does TF have "leave orphan"? I mean you can technically remove a tf resource from the json and the statefile at the same time but terraform itself doesn't have a any way to declare that intention AFAIK. hashicorp/terraform#27035

If TF doesn't have it, I think it's okay leaving out for now. We can add it to the pinfile but it would likely not be implemented in the first version.

[eladb]

a few minor notes

[eladb]

Suggested change

	[-pin] /root/CoolBucket
	[unpinned] /root/CoolBucket

[MarkMcCulloh]

I like the simple verb, it's shorter (IMO unpinned is kinda an icky looking word) and makes it clear that the CLI just did an action for you. Not a deal breaker for me though.

rewrite

[eladb]

This still feels wrong... I am sorry :-(

The new terraform resource name (aws_s3_bucket.MyBucket_AD8CE4AC) is not really something that I care about or understand as a user and not something that I need to carry forward with me.

Wing code "speaks" in logical node path terms. These are the IDs that users understand (and oftentimes assign themselves through as X).

I am sorry but I still think that this simply needs to be a map between logical node paths and terraform resource names.

Happy to talk more about this because it feels like something is misaligned in requirements/use case :-)

I also don't think we need the attributes of the resource here. This will be huge duplication with the Terraform output and I don't see why it is needed.

This is what I think this should look like:

// main.w.pin.json
{
  "version": "1",
  "pinned": {
    "tf-aws": {
      "/MyBucket/Bucket": {
        "terraform_name": "aws_s3_bucket.CoolBucket",
        "type": "@cdktf/provider-aws.s3bucket.S3Bucket"
      }
    }
  }
}

[MarkMcCulloh]

The problem with logical->physical is that 1 logical can have multiple physical, no?
I'm probably mistaken, so I'll change that

I also don't think we need the attributes of the resource here.

To prevent attributes from changing to avoid resource replacement. I can remove this if it's not needed.

terraform_name

I think it's useful for the schema of this pinfile to be standard across platforms (other than the attribute keys). Especially since it's not difficult to do. If it's not useful I can remove it.

[eladb]

The problem with logical->physical is that 1 logical can have multiple physical, no?

I think this should refer to the logical path of the leaf node.

I think it's useful for the schema of this pinfile to be standard across platforms

OK, although I am not sure this is something we need to abstract. This name is target specific. For example, in Terraform, it always has the TF type as a prefix, in CFN there are some characters that are not allowed, in K8S it has a strict length limit. In other targets it might not even be a string (god forbid)...

So from a design perspective, it actually makes more sense to give it a concrete name like terraform_name or terraform_id instead of trying to generalize.

[MarkMcCulloh]

I think this should refer to the logical path of the leaf node.

Yeah that's what I meant. I think platforms can technically emit more than one physical thing given a single logical leaf.

[eladb]

Eventually, in CDKTF/AWSCDK/CDK8S there's a 1:1 mapping between L1s and resources in the target configuration. That's the "leaf node" that we should pin.

[MarkMcCulloh]

Eventually, in CDKTF/AWSCDK/CDK8S there's a 1:1 mapping between L1s and resources in the target configuration

I understand that's how these CDKs work, but it is technically possible for a single construct to spit out multiple things. Or I could even make my own type of CDK and not consider that restriction either. I thought it'd be useful for this pinfile to basically skip any possible abstractions and go straight to the actual output both for the source and destination of the mapping.

That being said, I'll change it to logical -> physical because the 1:many is obviously the 0.01% case

btw, are you okay with storing the attributes in the pinfile as well or should that be addressed?

[eladb]

btw, are you okay with storing the attributes in the pinfile as well or should that be addressed?

I would recommend starting with the minimal amount of surface for this and add later, so I'd feel more comfortable to remove it for now, but if you feel strongly about it, then I am okay :-)