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

Interchangeability of Tokens and Literals #168

Closed
rix0rrr opened this issue Jun 22, 2018 · 17 comments
Closed

Interchangeability of Tokens and Literals #168

rix0rrr opened this issue Jun 22, 2018 · 17 comments
Labels
needs-discussion This issue/PR requires more discussion with community.

Comments

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 22, 2018

This is definitely a design challenge we haven't solved yet, and one of the only remaining abstraction leaks we need to resolve.

At the "L1" CloudFormation resource layer (the XxxResource constructs), we are currently using TypeScript unions to allow specifying either concrete values or tokens for every primitive list property.

This works for L1, however, it's sub-optimal for a few reasons:

  1. In some languages, which do not support unions, we get a sub-par experience. For example, in .NET, all L1 props are Object (which we might want to change to "setXxx" methods just for the type-safety). In Java, we have overloads for setters, and getters return Object. It's not elegant or nice, or idiomatic, and we don't want to leak this ugliness to L2...
  2. Theoretically, it requires that every property in every construct we or anyone else will ever write will use unions to support these fringe use cases. Furthermore, when you interact with a prop that uses a union type, it means you will need to be type-sensitive every time you do anything with the value. This will "litter" any construct code with union types and type-checks everywhere (which are very hard in JavaScript). That's not an acceptable developer experience (remember, we are building a framework, not just a library).

Let's try to classify the use cases where people need to interact with tokens:

Parametric CloudFormation templates

When using the CDK "natively", this is rarely needed since the CDK is designed to synthesize concrete CloudFormation templates for every environment the app is deployed. There are better tools in programming languages for defining abstractions, such as functions and classes. This approach also promotes deterministic deployments and healthier change governance (since configuration is checked-into code).

However, we are aware that parametric CloudFormation templates are something that people need, even as they transition to a more CDK-native code base.

Work with resource attribute values

This is actually a more long-term concern as it's not going to go away when people go "full CDK".

There will always be a use case for using a resource attribute value that only resolves at deploy-time as an input for another value. For example, say I want to include the generated name of the bucket in a string. Ideally, I want to write it like this:

const message = `Your file has been uploaded to the bucket ${myBucket.bucketName}`;

This will currently not work. Since bucketName is a Token (actually it's a BucketName which extends Token, but those are just aliases), you cannot use them in string substitutions. Alternatively, you will need to do something like this:

const message = Fn.concat("Your file has been uploaded to the bucket", myBucket.bucketName);

This is not that bad, but it's an abstraction leak. It's not the natural thing a user will do. Moreover, in some cases, bucketName can be resolved to a concrete value during synthesis (e.g. when we explicitly specify the bucket name).

Proposed solution

As of this writing, the CloudFormation spec had 171 resource attributes, out of 157 are strings (~92%) (see analysis).

Let's try to optimize for the common case, where tokens resolve to strings.

We can represent tokens as string variable expansions. Then, when we synthesize a template, if a property value includes the string "${id}", it will be substituted with { "Ref": "id" }, and "${id.attr}" will be substituted with { "Fn::GetAt": [ "id", "attr" ] }. The substitution can use Fn::Join in case there are string before and/or after the expansion (i.e. "hello ${foo}" => { "Fn::Join": [ "", [ "hello", { "Ref": "foo" } ] ] }.

To apply this safely (without breaking the cases where attributes/parameters are not strings), what we can do is:

  1. Represent all string resource attribute as string that return ${id.attr} instead of an opaque token. Can we do that in a way that doesn't lose the semi-strong-typing we have today for attributes? (we currently generate an attribute type class for each attribute that extends Token). Maybe we can just override token.toString for those types and have token.toString for non-string types throw an exception?
  2. All resource.refs can probably just return a string ${id}
  3. We can add a parameter.stringValue property which will return a string ${param-id}. We can also create a subclass StringParameter whose .value is a string.

As @rix0rrr suggested let's also use this as a tracking issue to see how often the issues of people wanting to pass literals where Tokens are declared, or vice versa (passing CFN intrinsics where the declared type is a literal string) crop up.

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jun 22, 2018

While using LambdaS3Code, key is a string but wants to { Fn::Sub } a (deploy-time) parameter.

#166

@rix0rrr rix0rrr added management/tracking Issues that track a subject or multiple issues design labels Jun 22, 2018
@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jul 6, 2018

Support request over email:

Use Parameter for bucketName.

@eladb
Copy link
Contributor

eladb commented Jul 8, 2018

A little data analysis of attribute types in CloudFormation (based on the spec snapshot from July 8, 2018):

  • 171 attributes
  • 158 primitive types (157 'String', 1 'Json')
  • 13 list types, all with 'String' element types

Non-string attributes:

AWS::CloudFormation::WaitCondition.Data { PrimitiveType: 'Json' }
AWS::DirectoryService::MicrosoftAD.DnsIpAddresses { PrimitiveItemType: 'String', Type: 'List' }
AWS::ElasticLoadBalancingV2::LoadBalancer.SecurityGroups { PrimitiveItemType: 'String', Type: 'List' }
AWS::EC2::NetworkInterface.SecondaryPrivateIpAddresses { PrimitiveItemType: 'String', Type: 'List' }
AWS::ElastiCache::ReplicationGroup.ReadEndPoint.Addresses.List { PrimitiveItemType: 'String', Type: 'List' }
AWS::ElastiCache::ReplicationGroup.ReadEndPoint.Ports.List { PrimitiveItemType: 'String', Type: 'List' }
AWS::Route53::HostedZone.NameServers { PrimitiveItemType: 'String', Type: 'List' }
AWS::EC2::Subnet.Ipv6CidrBlocks { PrimitiveItemType: 'String', Type: 'List' }
AWS::EC2::VPC.CidrBlockAssociations { PrimitiveItemType: 'String', Type: 'List' }
AWS::EC2::VPC.Ipv6CidrBlocks { PrimitiveItemType: 'String', Type: 'List' }
AWS::DirectoryService::SimpleAD.DnsIpAddresses { PrimitiveItemType: 'String', Type: 'List' }
AWS::ElasticLoadBalancingV2::TargetGroup.LoadBalancerArns { PrimitiveItemType: 'String', Type: 'List' }
AWS::DMS::ReplicationInstance.ReplicationInstancePublicIpAddresses { PrimitiveItemType: 'String', Type: 'List' }
AWS::DMS::ReplicationInstance.ReplicationInstancePrivateIpAddresses { PrimitiveItemType: 'String', Type: 'List' }

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jul 9, 2018

Interesting. I would have thought that Port, EvaluationPeriods, Threshold et. al. would have been numbers.

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jul 9, 2018

Also, I feel like the piece about automatic string interpolation is interesting but maybe hijacking the thread a bit.

@eladb
Copy link
Contributor

eladb commented Jul 9, 2018

@rix0rrr the piece about string manipulation is an attempt to provide a solution. Do you think we should create a separate issue?

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jul 9, 2018

Oh, is your solution to just accept string everywhere and let people pass token.toString()?

@eladb
Copy link
Contributor

eladb commented Jul 9, 2018

The solution is to find a way to represent string attributes (or parameters) as string and not as Tokens. Then, they can naturally be used everywhere. I was thinking out loud about whether we can preserve the strong-typing of attribute types by using .toString() but I agree that this is still not a good experience to have to .toString() every time you assign.

I am comfortable with all these attributes (or string parameters) to just be string. The strong typing (QueueArn) is not critical in my mind and doesn't provide tons of value, especially given those are practically untyped inside /and/ we have a better way to type resources (QueueRef).

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jul 9, 2018

So let me reiterate to see if I understand correctly:

  • Based on the premise that the logical return type of most attributes and and intrinsics is string;
  • we're going to make all attributes (for example, queue.queueArn and Fn.concat(bucket.bucketName, '-is-a-great-bucket') return string literals with magic markers in them;
    • For example [invented syntax]:
    • "{{{ATTRIBUTE:Queue12345.QueueArn}}}"
    • "{{{FN:CONCAT({{{ATTRIBUTE:BucketABCD.BucketName}}}, '-is-a-great-bucket'}}}";
  • which we'll then parse out and revert back to CloudFormation intrinsics in resolve().

Is that right?

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jul 9, 2018

For my money, I'd rather keep this in the type system, so my vote would be either:

  • Union types, bucketName: string | StringToken, which jsii languages are free to implement using builder overloads withBucketName(string) and withBucketName(StringToken) and/or explicit union types Union<string, StringToken>.
  • Generics, bucketName: Value<string>, which Java clients are still free to hide behind overloads and C# clients can implicitly lift string -> Value<string>.

@eladb
Copy link
Contributor

eladb commented Jul 9, 2018

So let me reiterate to see if I understand correctly

Almost, but not exactly. I am thinking that we only need to support Ref and Fn::GetAtt as expansions, in which case we can use ${logical} and ${logical.attr} as the values returned from:

  • Resource string attributes (~92% of them)
  • String parameters (i.e. we can just add parameter.stringValue that returns ${logical}.
  • String ImportValue (similar - importValue.stringValue).

Unions/Generics

I don't think either union types nor generics are good enough in terms of overall developer experience (see issue description for some details). They may be type-system pure, but they will cause a lot of headache across the board, because these patterns will "infect" every piece of CDK code (every property in the world will need to use this). The cognitive load and amount of boilerplate is not acceptable in my mind.

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jul 9, 2018

Preamble note: I'm aware I'm probably coming off as needlessly argumentative, and in the way of progress. This is not my intention, I'm just wary of jumping into solutions that seem to solve the immediate complaint without solving the larger issue.


I am thinking that we only need to support Ref and Fn::GetAtt as expansions

I'm not fond of ignoring other intrinsics. What's the solution for when people want to use Fn::Join, Fn::Concat or Fn::Sub? In fact, you already wrote this in your own example code:

const message = Fn.concat("Your file has been uploaded to the bucket", myBucket.bucketName);

What's the correct type of message, which could be passed into anywhere a string would be expected in the CDK model? By the current proposed argument it must be string.

Let's say we don't support intrinsics, what are consumers to do then? Cast the difference away with as any? If it's acceptable in that case, then it's just as acceptable in the normal case, and we don't have to do anything.

in which case we can use ${logical} and ${logical.attr} as the values returned from

How do we determine the difference between strings that have ${xxx} tokens that we generated, vs the ones that were already in there? As in, how do we determine the difference between:

new Messages(this, 'Messages', {
    message1: parameter.stringValue,
    message2: '${pleaseDont.replaceMe}',
});

If the counterargument to this is, 'that is unlikely to happen', I don't find that a particularly strong argument.


I don't think either union types nor generics are good enough in terms of overall developer experience (see issue description for some details)

I'm not super convinced by the counteraguments. My counter-counter arguments:

  1. In some languages, which do not support unions, we get a sub-par experience. For example, in .NET, all L1 props are Object. In Java, we have overloads for setters, and getters return Object.

This may be true at the moment, but it's not a general truth that the generated Java code HAS TO be this way. As mentioned, we could model it properly as a Union<string, StringToken>, potentially with overloaded setters so the user doesn't even have to notice when creating a props object. What I'm saying is, the following can work fine and consumers won't even know what's going on:

new Messages(this, "Messages", MessagesProps.builder()
  .withMessage1(parameter)
  .withMessage2("some string")
  .build());

They WILL have to notice when inspecting the values, but I will argue that is a fringe case, even for construct authors. Most values are passed down directly into the CloudFormation template without any code even looking at them. If you want me to, I can substantiate that with numbers, but my intuition says this is true (for all properties except enums). If we only have Token support for strings, I dare say the user experience is just about the same, and we will have solid foundations for allowing deploy-time computed values everywhere.

  1. Theoretically, it requires that every property in every construct we or anyone else will ever write will use unions to support these fringe use cases.

If the concern is writing an elaborate type:

interface MessageProps {
    message1: string | StringToken;
}

That does not have to be true with type aliases. We can totally define a type named StringValue, and you just tell people to declare properties of type StringValue.

If the concern is inspecting the type, then yes, but as mentioned before I think that is a fringe case:

This will "litter" any construct code with union types and type-checks everywhere

Again, I don't think the mentioned "littering" will occur. If we limit to strings, I'm guessing we'll see a 95%+ blind pass-through rate.


This argues for a StringValue type in addition to regular types. If we go generics we can achieve the following user experience:

interface FooProps {
    lookAtThis: string;

    dontLookAtThis: string;
}

class Foo extends Construct {
    constructor(parent: Construct, name: string, props: Props<FooProps>) {
        super(parent, name);

        if (readProperty(props.lookAtThis) === "value") {    
        }

        // If calling readProperty repeatedly is too ugly
        const values = readProperties(props);
        if (values.lookAtThis === "value") {
            // Will throw if not a literal value
        }
    } 
}

// Still just used as
new Foo(this, 'Foo', {
   lookAtThis: 'hello',
   dontLookAtThis: parameter
});

The types in the interface are the same, and the constructor has 2 additional things it needs to do, wrap the type and unwrap the value.

Yes, this is some overhead. Does it constitute "too much" overhead? Given that we already have some overhead in writing a construct, with the inheritance and the "super" call, for example? It's two small statements that are easily trained and corrected for. Most importantly, the call site is still clean.

This has the advantage over the previous solution that it works for all types, and doesn't require introduction of something like StringValue. The type mechanism is still the same (unions) and the author story is nicer in accessing the properties since no case switching has to be done on every property read.

@eladb
Copy link
Contributor

eladb commented Jul 9, 2018

Sorry, but I still think this is too much overhead and too much cognitive given this has to be done in every construct ever created past, present and future, especially given this is to address use cases that are going to be less and less common. Our bar for introducing idioms that are so wide-spread should be extremely high. The parent, id, props idiom is probably the only one we have (and in some languages we might be able to get rid).

I think I might have not explained myself well. I don't think we should completely get rid of tokens, all I am saying is that if we only make string attributes (and parameters/imports) expressible as strings, the experience will be natural. If people still want to use complex intrinsic functions, they should be able to, and I think that the current best pragmatic solution is to just use as any in these cases.

We should continue to look for use cases though, because I am struggling to find examples.

The Fn.concat example above was an example of how we don't want people to write code. They should be able to just use natural string concatenation, not tokenized concatenation.

I think the only remaining question is what are the use cases people have for using something other than just string concatenation (i.e. splitting a parameter value?).

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Jul 9, 2018

Yes, split-and-select. Although I would expect the use of that to be rare as well. Join maybe. But even those seem more designed to work at a lower level than we expect people to work at, to get information in and out of CloudFormation parameter strings.

Though if we made Token.stringValue(), we can just have it generate an unique identifier and register itself in a global map which we can find back during resolve(), and any intrinsic could be supported that way.


If we're going the "all attributes are strings" way, there's an idiom I expect people are going to write and then be confused:

const bucket = new Bucket(...);

if (bucket.bucketName == "my-bucket-1234QZY") {
  // Why is this never true? I see the bucket name like that in the console!
}

So better find good terminology to cut this off at the pass. We can't allow bucket.bucketName to be a string itself. So it should be something like bucket.bucketName.asProperty() ?

@eladb
Copy link
Contributor

eladb commented Jul 10, 2018

@rix0rrr, I like the idea of allowing any token to be "stringified" by allocating an global ID which is resolved during synthesis. It preserves the semi-strong-typing of attributes (BucketName) and allows any intrinsic function to be used within strings as needed. Obviously people will need to know what they are doing, but I guess that's okay given they choose to go down the intrinsic function path.

Regarding an explicit method, I wonder if .toString() is the right name. The benefit is that in contexts, it will implicitly be called. For example, in JavaScript: The bucket ${bucket.bucketName} is awesome will work without needing to invoke .toString() explicitly.

@eladb
Copy link
Contributor

eladb commented Jul 19, 2018

Duplicate: #24

rix0rrr pushed a commit that referenced this issue Aug 7, 2018
Tokens (such as resource attributes) can now be implicitly converted
into strings. Parameters and attributes can now be combined into larger
strings using the native string facilities of the host language.

The process will be reversed by the resolve() function during synthesis,
which will split the string up again into string literals and
CloudFormation intrinsics.

The same transformation is applied during JSON.stringify(), so that
Tokens in a complex JSON structure are preserved.

Fixes #24 and #168.
rix0rrr added a commit that referenced this issue Aug 15, 2018
Tokens (such as resource attributes) can now be implicitly converted
into strings. This allows using any late-bound CloudFormation value
where strings can be used in the host language, and the native string
facilities of the host language can be used as well to further process
strings.

The process is reversed by the resolve() function during synthesis,
which will split the string up again into string literals and
CloudFormation intrinsics (combined by `{Fn::Join}`).

Introduces `CloudFormationJSON.stringify` to produce a similar result,
but used for JSON-encoding of complex objects that may contain Tokens.
This is used in JSON-encoded policy statements, CloudFormation actions,
state machine definitions, and more.

Additional changes:
- Add a VSCode launch config (with a helper script) to directly debug a
  unit test from within the IDE. This allows setting breakpoints.
- SecretParameter no longer duckily implements Token; this will not work
  in other jsii languages anyway.
- Nested `FnConcat` structures will be flattened in the CloudFormation
  output.

Fixes #24 and #168.
@rix0rrr
Copy link
Contributor Author

rix0rrr commented Sep 17, 2018

Superseded by #695. Closing.

@rix0rrr rix0rrr closed this as completed Sep 17, 2018
eladb pushed a commit that referenced this issue Sep 20, 2018
__NOTICE__: This release includes a framework-wide [__breaking
change__](#712) which changes the type
of all the string resource attributes across the framework. Instead of using
strong-types that extend `cdk.Token` (such as `QueueArn`, `TopicName`, etc), we
now represent all these attributes as normal `string`s, and codify the tokens
into the string (using the feature introduced in [#168](#168)).

Furthermore, the `cdk.Arn` type has been removed. In order to format/parse ARNs,
use the static methods on `cdk.ArnUtils`.

See motivation and discussion in [#695](#695).

* **cfn2ts:** use stringified tokens for resource attributes instead of strong types ([#712](#712)) ([6508f78](6508f78)), closes [#518](#518) [#695](#695) [#744](#744)
* **aws-dynamodb:** Attribute type for keys, changes the signature of the `addPartitionKey` and `addSortKey` methods to be consistent across the board. ([#720](#720)) ([e6cc189](e6cc189))
* **aws-codebuild:** fix typo "priviledged" -> "privileged

* **assets:** cab't use multiple assets in the same stack ([#725](#725)) ([bba2e5b](bba2e5b)), closes [#706](#706)
* **aws-codebuild:** typo in BuildEnvironment "priviledged" -> "privileged     ([#734](#734)) ([72fec36](72fec36))
* **aws-ecr:** fix addToResourcePolicy ([#737](#737)) ([eadbda5](eadbda5))
* **aws-events:** ruleName can now be specified ([#726](#726)) ([a7bc5ee](a7bc5ee)), closes [#708](#708)
* **aws-lambda:** jsii use no long requires 'sourceAccount' ([#728](#728)) ([9e7d311](9e7d311)), closes [#714](#714)
* **aws-s3:** remove `policy` argument ([#730](#730)) ([a79190c](a79190c)), closes [#672](#672)
* **cdk:** "cdk init" java template is broken ([#732](#732)) ([281c083](281c083)), closes [#711](#711) [aws/jsii#233](aws/jsii#233)

* **aws-apigateway:** new API Gateway Construct Library ([#665](#665)) ([b0f3857](b0f3857))
* **aws-cdk:** detect presence of EC2 credentials ([#724](#724)) ([8e8c295](8e8c295)), closes [#702](#702) [#130](#130)
* **aws-codepipeline:** make the Stage insertion API in CodePipeline more flexible ([#460](#460)) ([d182818](d182818))
* **aws-codepipeline:** new "Pipeline#addStage" convenience method ([#647](#647)) ([25c9fa0](25c9fa0))
* **aws-rds:** add support for parameter groups ([#729](#729)) ([2541508](2541508)), closes [#719](#719)
* **docs:** add documentation for CDK toolkit plugings ([#733](#733)) ([965b918](965b918))
* **dependencies:** upgrade to [jsii 0.7.6](https://github.com/awslabs/jsii/releases/tag/v0.7.6)
eladb pushed a commit that referenced this issue Sep 20, 2018
* v0.9.2

__NOTICE__: This release includes a framework-wide [__breaking
change__](#712) which changes the type
of all the string resource attributes across the framework. Instead of using
strong-types that extend `cdk.Token` (such as `QueueArn`, `TopicName`, etc), we
now represent all these attributes as normal `string`s, and codify the tokens
into the string (using the feature introduced in [#168](#168)).

Furthermore, the `cdk.Arn` type has been removed. In order to format/parse ARNs,
use the static methods on `cdk.ArnUtils`.

See motivation and discussion in [#695](#695).

* **cfn2ts:** use stringified tokens for resource attributes instead of strong types ([#712](#712)) ([6508f78](6508f78)), closes [#518](#518) [#695](#695) [#744](#744)
* **aws-dynamodb:** Attribute type for keys, changes the signature of the `addPartitionKey` and `addSortKey` methods to be consistent across the board. ([#720](#720)) ([e6cc189](e6cc189))
* **aws-codebuild:** fix typo "priviledged" -> "privileged

* **assets:** cab't use multiple assets in the same stack ([#725](#725)) ([bba2e5b](bba2e5b)), closes [#706](#706)
* **aws-codebuild:** typo in BuildEnvironment "priviledged" -> "privileged     ([#734](#734)) ([72fec36](72fec36))
* **aws-ecr:** fix addToResourcePolicy ([#737](#737)) ([eadbda5](eadbda5))
* **aws-events:** ruleName can now be specified ([#726](#726)) ([a7bc5ee](a7bc5ee)), closes [#708](#708)
* **aws-lambda:** jsii use no long requires 'sourceAccount' ([#728](#728)) ([9e7d311](9e7d311)), closes [#714](#714)
* **aws-s3:** remove `policy` argument ([#730](#730)) ([a79190c](a79190c)), closes [#672](#672)
* **cdk:** "cdk init" java template is broken ([#732](#732)) ([281c083](281c083)), closes [#711](#711) [aws/jsii#233](aws/jsii#233)

* **aws-apigateway:** new API Gateway Construct Library ([#665](#665)) ([b0f3857](b0f3857))
* **aws-cdk:** detect presence of EC2 credentials ([#724](#724)) ([8e8c295](8e8c295)), closes [#702](#702) [#130](#130)
* **aws-codepipeline:** make the Stage insertion API in CodePipeline more flexible ([#460](#460)) ([d182818](d182818))
* **aws-codepipeline:** new "Pipeline#addStage" convenience method ([#647](#647)) ([25c9fa0](25c9fa0))
* **aws-rds:** add support for parameter groups ([#729](#729)) ([2541508](2541508)), closes [#719](#719)
* **docs:** add documentation for CDK toolkit plugings ([#733](#733)) ([965b918](965b918))
* **dependencies:** upgrade to [jsii 0.7.6](https://github.com/awslabs/jsii/releases/tag/v0.7.6)
@srchase srchase added needs-design and removed design management/tracking Issues that track a subject or multiple issues labels Jan 3, 2019
@srchase srchase added the needs-discussion This issue/PR requires more discussion with community. label Jan 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-discussion This issue/PR requires more discussion with community.
Projects
None yet
Development

No branches or pull requests

3 participants