terraform-cdk icon indicating copy to clipboard operation
terraform-cdk copied to clipboard

New token is generated each time a property is accessed

Open Chriscbr opened this issue 3 years ago • 6 comments

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

cdktf & Language Versions

node: 18.12.1 npm library "@cdktf/provider-aws": 12.0.1 npm library "cdktf": 0.15.2

Affected Resource(s)

Debug Output

Expected Behavior

In other CDK frameworks like AWS CDK, I've noticed that trying to access a property that has a late-bound value will return the same token twice. For example:

import * as s3 from "aws-cdk-lib/aws-s3";

const bucket = new s3.Bucket(this, "CdkTestBucket");
console.log(bucket.bucketArn);
console.log(bucket.bucketArn);

produces:

${Token[TOKEN.237]}
${Token[TOKEN.237]}

But I've noticed in CDKTF, doing something similar usually gets me a different token each time:

import { S3Bucket } from "@cdktf/provider-aws/lib/s3-bucket";
const bucket = new S3Bucket(this, "CdkTestBucket");
console.log(bucket.bucket);
console.log(bucket.bucket);

produces:

${TfToken[TOKEN.0]}
${TfToken[TOKEN.1]}

I'm wondering if there is a technical reason for this, as sometimes it can be useful to tell if two tokens are the same (even if they aren't resolved yet).

Actual Behavior

Steps to Reproduce

Important Factoids

References

  • #0000

Chriscbr avatar Feb 15 '23 19:02 Chriscbr

Hi @Chriscbr 👋

the only difference I could spot between CDKTF and AWS CDK constructs was that our properties sometimes have a different value for setting vs. getting it – and as getters and setters must adhere to the same type, we use a putXy method instead of a setter. I couldn't find such a case for AWS CDK (Cfn) constructs Here's an example of such a case in CDKTF: cdktf-provider-aws/src/s3-bucket/index.ts Interestingly enough, the instance that is returned there, is cached in _expiration 😄 but apparently that is not the case for tokens we return (Line 2766).

While we already store the input value, we don't do so for the token – but I'd be open to caching the token we'll return as well (e.g. in private _expirationReference or similar). We'd need to check whether this introduces problems, but I don't expect there to be any (besides maybe some performance implications; but we could also lazy-load them in that case).

However, one potential thing blocking this, could be how we resolve cross stack references, although it seems that creating a Reference will only lookup the source stack of the construct that is going to be referenced which would be the same each time the reference is accessed.

I vaguely remember that we used to have a bug where storing a reference in a variable and using it in two difference stacks caused cross stack references to fail, but I think that one was fixed and the Reference#resolve method does not look like its having a side-effect anymore that might cause this. Maybe @DanielMSchmidt remembers more about this.

So tl;dr would be: We currently don't return the same Token but we probably could start doing that 😄

ansgarm avatar Feb 17 '23 11:02 ansgarm

Cool, thanks for the prompt answer @ansgarm 😄 I don't think this issue is a blocker for my use case (in the end comparing token identities is probably not the safest thing to do in the first place), but it's handy to learn a bit more about how tokens work!

Chriscbr avatar Feb 17 '23 15:02 Chriscbr

I vaguely remember that we used to have a bug where storing a reference in a variable and using it in two difference stacks caused cross stack references to fail, but I think that one was fixed and the Reference#resolve method does not look like its having a side-effect anymore that might cause this. Maybe @DanielMSchmidt remembers more about this

I definitely recall noticing the different tokens when debugging an issue with cross stack references. I think at the time I experimented with making them the same and ran into some issues. Definitely possible that those have been fixed now though.

jsteinich avatar Feb 17 '23 22:02 jsteinich

Following up on the thread here, the same behavior is observed in 0.20.5.

danielenricocahall avatar Apr 08 '24 11:04 danielenricocahall

What would be the best solutions for this implementation-wise? Would it be to store a cached token directly on the class (with a private field, inside the provider-generated files)? Or are there any other ways to go about this, like maybe caching the results of this.interpolationForAttribute(x) (can we safely assume this is a pure function)? Maybe that would avoid the need to update any code generators. TBH I'm not as familiar with the token system / how cross-stack references plays into this.

Chriscbr avatar Jul 31 '24 23:07 Chriscbr

There's already internal caching of token instances at the lowest level: https://github.com/hashicorp/terraform-cdk/blob/050ce8423b8e025857fe1a3fa2a1ceeebc3a6f2a/packages/cdktf/lib/tokens/private/token-map.ts#L231

The problem is that what's passed in is quite often a new object for every get call.

Caching the result of interpolationForAttribute certainly seems like a simple solution that has the potential to work. I'm not sure if this even works correctly now, but since the function does check mutable state, it is currently possible to return different results at different points in time.

My memory's definitely a bit fuzzy, but I believe previous issues with caching and cross-stack references were the result of losing information after the token is resolved the first time. I'm pretty sure there are tests in place that would catch a regression though.

jsteinich avatar Sep 05 '24 01:09 jsteinich