Support for matching existing Terraform names

[jsteinich]

This provides ways to better support mapping from existing Terraform stacks to cdktf. All elements (resources, data sources, etc) declared as direct children to the TerraformStack will use their construct id (what's past in the constructor) as their Terraform name. This is directly inspired from AWS CDK logical ids.

The naming restrictions have also been loosened to allow '_' and '-' in logical ids (names). It might make sense to loosen farther since I believe Terraform has even fewer restrictions, but this at least allows common naming patterns.

Since both of those changes are breaking (Terraform resources had different local ids / local names), this also introduces the concept of feature flags. This is accomplished through construct runtime context, though support is limited at this time.

Since construct ids need to be unique and Terraform local names don't completely need to be, this also adds overrideLogicalId method for complete control over local names. Also, allocateLogicalId is a new overridable method for controlling logical id generation for an entire stack.

[skorfmann]

Yes! That's a good idea 👍

[skorfmann]

That's gonna be a breaking change though - not a big issue, but we should think about way to deal with that.

[cmclaughlin]

Not sure if I fully understand this PR, but so far I've been depending on the stack name to uniquely identify resources in the synthesized output.

When running the CLI, I pass in two environment variables which I use in all stack names:

• ENV - dev, qa, prod, etc
• REGION - us-east-1, us-west-2, etc

I was just working on a stack named rds-api (an RDS cluster that our API servers use). So in our dev environment running in us-east-1 the stack name becomes rds-api-dev-us-east-1.

I thought this naming convention was a good idea so I could write my stacks once and synth/plan/apply for many environments in a few regions. If there are any best practices or if this is a bad design on my part, perhaps we should add some docs for recommendations.

So far we only have a few cdktf stacks, so breaking changes are not a big deal.
I haven't tried importing any of our older raw HCL stacks, but anything to make that easier does sound great.

[cmclaughlin]

Just took another look at the code... I think I really like it. I'd keep using my ENV vars to separate my environments in my remote S3 state, but which is probably enough.

[jsteinich]

That's gonna be a breaking change though - not a big issue, but we should think about way to deal with that.

That's a good point. A couple ideas:

• Add an option to maintain existing behavior
• Add in some of the rename capability that CDK has (https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_core.Stack.html#rename-wbr-logical-wbr-idoldid-newid)

I'll push forward with fixing the integration tests and checking that the CLI handles the format.

[skorfmann]

• Add an option to maintain existing behavior

How about something along the lines of this https://docs.aws.amazon.com/cdk/latest/guide/featureflags.html ?

[skorfmann]

@cmclaughlin I understand you correctly, that you drive multiple stacks from the same Terraform CDK codebase based on injected envs? That sounds pretty cool - would love to see an example of this :)

Not sure if I fully understand this PR, but so far I've been depending on the stack name to uniquely identify resources in the synthesized output.

Yes, the stack name would be omitted from the resource name.

[skorfmann]

For reference, some input regarding resource names by @josiahdecker #248 (comment)

[jsteinich]

• Add an option to maintain existing behavior

How about something along the lines of this https://docs.aws.amazon.com/cdk/latest/guide/featureflags.html ?

I started porting this over from CDK, but discovered loading in context isn't actually something supported at the moment. I could port that over as well, or I could just do something simpler for now. @skorfmann any thoughts?

[skorfmann]

I started porting this over from CDK, but discovered loading in context isn't actually something supported at the moment. I could port that over as well, or I could just do something simpler for now.

Any idea already how a simpler approach would look like?

[jsteinich]

Any idea already how a simpler approach would look like?

I haven't really thought about it. Was just thinking reading in one value might be easier, but that might not be true.

I could bypass the file altogether and just set programmatically when the app is created. I don't overly like it, but would be a quick way.

[skorfmann]

I could bypass the file altogether and just set programmatically when the app is created. I don't overly like it, but would be a quick way.

You mean as part of the template?

[skorfmann]

I'm not sure if making it part of the template would be less work. Even if the logic would be living in the CLI itself, the template would still have to do search and replace, which becomes more and more complex as well.

What I was thinking of the other day: Perhaps we could change the templates to do actual code generation, rather than search and replace. This might be an opportunity to think about this. But again - I doubt this would be less effort compared to driving feature flags via cdktf.json.

Generally speaking, I'm pretty sure feature flags should be global to the application and not based on individual stacks.

Maybe this topic should be an issue / PR on its own?

[jsteinich]

I'm not sure if making it part of the template would be less work

I think it would be very similar to something I've already done getting it into cdktf.json, so shouldn't be too hard.

Perhaps we could change the templates to do actual code generation

I think overtime that may become necessary. Definitely impacts #339

Maybe this topic should be an issue / PR on its own?

I'll work on creating some new issues (and PRs) when I get some free time.

[jsteinich]

I've updated the description with some changes made to make this more general purpose and support feature flags.

The python integration tests are currently failing, but if I change pipenv run... to cdktf synth then they will succeed, The reason for this is because cdktf.json is only loaded by the cli, so setting only apply when run that way. I'm pretty sure this matches AWS CDK behavior, but we could change it. (I think the typescript tests that do similar work because of the way environment variables are transferred).

[jsteinich]

@skorfmann any thoughts regarding #329 (comment) or anything else with this PR? This is one I'd really like to get in before long.

[skorfmann]

Have only skimmed through this, looks good though. To sum up my understanding of what this does:

• Introduces Feature Flags which can be provided via cdktf.json or CDKTF_CONTEXT_JSON
• Existing apps won't be affected from the introduced changes
• New apps will add the feature flags EXCLUDE_STACK_ID_FROM_LOGICAL_IDS and ALLOW_SEP_CHARS_IN_LOGICAL_IDS to cdktf.json on init
• This will have the following effects
  • Stack name will be omitted from resource names
  • - and _ won't be removed from resources names
  • No unique id will be added at the end of root level resource names
• overrideLogicalId can be used to set a fixed resource name

Is that correct, did I miss anything?

Gonna give it a try locally.

[jsteinich]

Is that correct, did I miss anything?

Feature flags can only be provided in cdktf.json. The environment variable is just used to transport the data between the cli and running the app.

There's also allocateLogicalId, which is a new overridable method for controlling logical id generation for an entire stack.

@jsteinich have you seen this comment btw? #248 (comment)

That's why I added overrideLogicalId at this time. Ultimately the resource type is part of the synthesized json, it's just constructs that require uniqueness on the ids.
We could could change the code generation / some fixed types to prepend to the construct id, but it seems a bit intrusive to me.

@skorfmann did you see my question about either changing integration tests or when cdktf.json is loaded?

[skorfmann]

The python integration tests are currently failing, but if I change pipenv run... to cdktf synth then they will succeed, The reason for this is because cdktf.json is only loaded by the cli, so setting only apply when run that way. I'm pretty sure this matches AWS CDK behavior, but we could change it. (I think the typescript tests that do similar work because of the way environment variables are transferred).

yes, I think it's ok to assume that synth happens trough the CLI. If we'll see bug reports around this, we still could implement some check to produce a warning if the CLI isn't involved in synth.

@jsteinich have you seen this comment btw? #248 (comment)

That's why I added overrideLogicalId at this time. Ultimately the resource type is part of the synthesized json, it's just constructs that require uniqueness on the ids.
We could could change the code generation / some fixed types to prepend to the construct id, but it seems a bit intrusive to me.

Yeah, I was thinking about the same. Probably not worthwhile doing 👍

[skorfmann]

Cool, looks good. I think in general it's good to go, right? @jsteinich

[jsteinich]

Cool, looks good. I think in general it's good to go, right? @jsteinich

Yeah, just accepted your change. Assuming tests pass (they did locally), then I think everything is ready.

[david92rl]

This was a blocker for us. Well done 🎉
Do you have any plans to release a new version including this change soon?
Also, is there any documentation explaining the usage of overrideLogicalId and allocateLogicalId

Thanks a lot

[jsteinich]

Do you have any plans to release a new version including this change soon?

If you're using ts/js it should already be available using the @next version. I'm not sure when the next stable version will be released.

Also, is there any documentation explaining the usage of overrideLogicalId and allocateLogicalId

overrideLogicalId should result in the resource having exactly that terraform name.
The CDK documentation for allocateLogicalId pretty much matches how it is used here.

[david92rl]

Thank you @jsteinich
We're using Python, but it's fine. I can use the built version of master until it gets released 👍

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.