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

feat(aws-cdk): add CDK app version negotiation #988

Merged
merged 5 commits into from
Oct 25, 2018

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Oct 22, 2018

Tag the CDK app output with a version, so that the Toolkit can
compare the sent and expected versions and complain if there's
a mismatch.

Fixes #891.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

Tag the CDK app output with a version, so that the Toolkit can
compare the sent and expected versions and complain if there's
a mismatch.

Fixes #891.
@rix0rrr rix0rrr requested review from RomainMuller and eladb October 22, 2018 17:16
*
* - The versions are not compared in a semver way, they are used as
* opaque ordered tokens.
* - The version needs to be set to the NEXT releasable version when it's
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmmmnmnmmm this smells like something that one might forget to do, since it happens in a different place than the other version bumps. I don't know that this is a real concern / reason not to, but I wanted to have it up there.

*
* Note the following:
*
* - The versions are not compared in a semver way, they are used as
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not, though? Your verification code uses semver already and I reckon a protocol-breaking change should derive in a version bump... So you could totally semver that.

@@ -39,6 +39,7 @@
"@types/request": "^2.47.1",
"@types/uuid": "^3.4.3",
"@types/yargs": "^8.0.3",
"@types/semver": "^5.5.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You add a dependency on @types/semver but not on semver... That make sit look fishy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And semver is used

@@ -39,6 +39,7 @@
"@types/request": "^2.47.1",
"@types/uuid": "^3.4.3",
"@types/yargs": "^8.0.3",
"@types/semver": "^5.5.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And semver is used

* updated (as the current verison in package.json has already been released!)
* - The request does not have versioning yet, only the response.
*/
export const PROTO_RESPONSE_VERSION = '0.14.0';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's either use separate versioning or use the version of the cx-api module as the version. I don't think manually aligning those makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That only works if we move to "bump-when-changed" versioning of libraries, which we're not doing yet.

If we use the version of cx-api today, every CDK release will require a new toolkit version. Seems disruptive to user experience.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So let's just define a separate version line for the interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So do you mean switch to 1, 2, 3, ... versioning for the interface?

How do you propose to map this version number to an actionable and helpful instruction for the user? As in:

CDK Toolkit >= 0.16.0 required to interact with this program.

Where do we get the 0.16.0? Do we do this:

const INTERFACE_VERSION = 3;

const VERSION_MAPS = {
  1: '0.12.0',
  2: '0.13.2',
  3: '0.16.0',
};

And then go CDK Toolkit >= ${VERSIONS_MAP[INTERFACE_VERSION]} required to interact with this program?

Or you do just want to say:

Interface version 3 used. You must use a newer version of CDK toolkit to interact with this program.

We can have 2 versions in the protocol:

interface_version
software_version

So that if the interface_version doesn't match, we can say you must upgrade to >= ${software_version} (which might actually be incorrect because the protocol might have been bumped in 0.15.0 so that will suffice but since the current framework version is 0.16.0 it will say that).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would still use semantic versioning though, not just "1,2,3"
I like the VERSION_MAPS approach. Basically you indicate when was the interface version introduced.

Copy link
Contributor Author

@rix0rrr rix0rrr Oct 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why no version map?

A problem of the VERSION_MAP is that we have to use the map from the "newest" side, because that's the only one that will know what interface version corresponds to what software version.

So if the toolkit has this data:

const CURRENT_PROTOCOL_VERSION = 2;

const VERSION_MAP = {
  1: '0.14.0',
  2: '0.15.0',
};

And it receives protocol_version=3, it still won't know what version to recommend to the user to upgrade. So the wire protocol must include this information as well, as follows:

{
   protocol_version: 3,
   software_version: '0.16.0',
}

So that the toolkit can provide the appropriate error message. Then the newer toolkit has the following information internally:

const CURRENT_PROTOCOL_VERSION = 3;

const VERSION_MAP = {
  1: '0.14.0',
  2: '0.15.0',
  3: '0.16.0',
};

const response = {
  protocol_version: CURRENT_PROTOCOL_VERSION,
  software_version: VERSION_MAP[CURRENT_PROTOCOL_VERSION],
};

It will only ever read the CURRENT_PROTOCOL_VERSION from the version map, never any other, so we could also simplify it to:

const CURRENT_PROTOCOL_VERSION = 3;
const PROTOCOL_SOFTWARE_VERSION = '0.16.0';

And send both of those on every response.

The only thing that's different between this code and the PR is that in the PR those 2 versions fields (which always bump versions together) have been collapsed into one.

Because: why have two fields if one of them conveys strictly more information than the other one?

But why not semantic versioning?

I don't think semantic versioning makes sense in this case.

With semantic versioning a provider can express to a consumer the notion: "this interface can do at least what you expect, and potentially more."

It is used in situations where a provider advertises a version number beforehand, and a consumer can compare this this number to their required version number, and then decide whether or not to proceed with the request. That is, the consumer makes the continue/stop decision based on the advertised information by the provider.

In this case though, who is the provider, and who is the consumer? I would argue that the toolkit is the provider that renders services (such as reading context variables, packaging assets, deploying resources) to the CDK app. The CDK app is the consumer, making a request to the toolkit to "please perform some actions."

In that sense, there is no way in which the app can bump their version number in a way that an older toolkit that doesn't know about the changes can still service the request. The app might be requesting new features that the toolkit does not know how to deliver. The best thing we can do at this point is say to the user something like:

This app is requesting some features that this toolkit might not be able to provide. 
Your deployment might or might not work. If it doesn't, please upgrade the toolkit and try again.

That's ludicrous, and way too hand-wavy for users to be able to deal with in practice. The answer at this point is to fail and tell the user to upgrade, because whatever they're trying to do, it probably won't work.

The only way in which the CDK app can semantically bump their version number is if they're requiring fewer features from the toolkit (because they'd be narrowing their postconditions). This might be usefully used on the path to feature deprecation, but that seems like an exceedingly small use case. In practice, if the app wants fewer features from the toolkit, it would stop emitting the fields that lead to those features being activated, without changing the version number.

The version number in the protocol is much more like a protocol version number in version negotation. By the time the request gets to the provider: they have full information, they know whether they recognize the request version or not. There are two cases:

  • The request version number is the same as ours: easy, we service the request.
  • The request version number is too new: the previous case we talked about; we can give a message about how some features (which ones?) might or might not work, but it's safer to just tell to upgrade.
  • The request version number is an old one: since we have full information, we can decide on whether we can service the request (because we know what was intended by this request) or tell the user to upgrade their framework.

But I really want semantic versioning!

We can reverse it: the toolkit can advertise their feature set in a semver way via an environment variable and the app can look at that version and decide whether to proceed (continuing if the advertised version number is a semantic superset of the app's version number). We could even make it emit older output in case we want to maintain backwards compatibility.

But for some reason, it feels much more natural to me, and safer, to maintain these kinds of checks in the toolkit.

@rix0rrr rix0rrr self-assigned this Oct 23, 2018
@rix0rrr rix0rrr merged commit db4e718 into master Oct 25, 2018
@rix0rrr rix0rrr deleted the huijbers/cxapi-version branch October 25, 2018 10:41
rix0rrr pushed a commit that referenced this pull request Oct 26, 2018
__IMPORTANT NOTE__: when upgrading to this version of the CDK framework, you must also upgrade
your installation the CDK Toolkit to the matching version:

```shell
$ npm i -g aws-cdk
$ cdk --version
0.14.0 (build ...)
```

Bug Fixes
=========

* remove CloudFormation property renames ([#973](#973)) ([3f86603](3f86603)), closes [#852](#852)
* **aws-ec2:** fix retention of all egress traffic rule ([#998](#998)) ([b9d5b43](b9d5b43)), closes [#987](#987)
* **aws-s3-deployment:** avoid deletion during update using physical ids ([#1006](#1006)) ([bca99c6](bca99c6)), closes [#981](#981) [#981](#981)
* **cloudformation-diff:** ignore changes to DependsOn ([#1005](#1005)) ([3605f9c](3605f9c)), closes [#274](#274)
* **cloudformation-diff:** track replacements ([#1003](#1003)) ([a83ac5f](a83ac5f)), closes [#1001](#1001)
* **docs:** fix EC2 readme for "natgatway" configuration ([#994](#994)) ([0b1e7cc](0b1e7cc))
* **docs:** updates to contribution guide ([#997](#997)) ([b42e742](b42e742))
* **iam:** Merge multiple principals correctly ([#983](#983)) ([3fc5c8c](3fc5c8c)), closes [#924](#924) [#916](#916) [#958](#958)

Features
=========

* add construct library for Application AutoScaling ([#933](#933)) ([7861c6f](7861c6f)), closes [#856](#856) [#861](#861) [#640](#640) [#644](#644)
* add HostedZone context provider ([#823](#823)) ([1626c37](1626c37))
* **assert:** haveResource lists failing properties ([#1016](#1016)) ([7f6f3fd](7f6f3fd))
* **aws-cdk:** add CDK app version negotiation ([#988](#988)) ([db4e718](db4e718)), closes [#891](#891)
* **aws-codebuild:** Introduce a CodePipeline test Action. ([#873](#873)) ([770f9aa](770f9aa))
* **aws-sqs:** Add grantXxx() methods ([#1004](#1004)) ([8c90350](8c90350))
* **core:** Pre-concatenate Fn::Join ([#967](#967)) ([33c32a8](33c32a8)), closes [#916](#916) [#958](#958)

BREAKING CHANGES
=========

* DynamoDB AutoScaling: Instead of `addReadAutoScaling()`, call `autoScaleReadCapacity()`, and similar for write scaling.
* CloudFormation resource usage: If you use L1s, you may need to change some `XxxName` properties back into `Name`. These will match the CloudFormation property names.
* You must use the matching `aws-cdk` toolkit when upgrading to this version, or context providers will cease to work. All existing cached context values in `cdk.json` will be invalidated and refreshed.
@rix0rrr rix0rrr mentioned this pull request Oct 26, 2018
rix0rrr added a commit that referenced this pull request Oct 26, 2018
__IMPORTANT NOTE__: when upgrading to this version of the CDK framework, you must also upgrade
your installation the CDK Toolkit to the matching version:

```shell
$ npm i -g aws-cdk
$ cdk --version
0.14.0 (build ...)
```

Bug Fixes
=========

* remove CloudFormation property renames ([#973](#973)) ([3f86603](3f86603)), closes [#852](#852)
* **aws-ec2:** fix retention of all egress traffic rule ([#998](#998)) ([b9d5b43](b9d5b43)), closes [#987](#987)
* **aws-s3-deployment:** avoid deletion during update using physical ids ([#1006](#1006)) ([bca99c6](bca99c6)), closes [#981](#981) [#981](#981)
* **cloudformation-diff:** ignore changes to DependsOn ([#1005](#1005)) ([3605f9c](3605f9c)), closes [#274](#274)
* **cloudformation-diff:** track replacements ([#1003](#1003)) ([a83ac5f](a83ac5f)), closes [#1001](#1001)
* **docs:** fix EC2 readme for "natgatway" configuration ([#994](#994)) ([0b1e7cc](0b1e7cc))
* **docs:** updates to contribution guide ([#997](#997)) ([b42e742](b42e742))
* **iam:** Merge multiple principals correctly ([#983](#983)) ([3fc5c8c](3fc5c8c)), closes [#924](#924) [#916](#916) [#958](#958)

Features
=========

* add construct library for Application AutoScaling ([#933](#933)) ([7861c6f](7861c6f)), closes [#856](#856) [#861](#861) [#640](#640) [#644](#644)
* add HostedZone context provider ([#823](#823)) ([1626c37](1626c37))
* **assert:** haveResource lists failing properties ([#1016](#1016)) ([7f6f3fd](7f6f3fd))
* **aws-cdk:** add CDK app version negotiation ([#988](#988)) ([db4e718](db4e718)), closes [#891](#891)
* **aws-codebuild:** Introduce a CodePipeline test Action. ([#873](#873)) ([770f9aa](770f9aa))
* **aws-sqs:** Add grantXxx() methods ([#1004](#1004)) ([8c90350](8c90350))
* **core:** Pre-concatenate Fn::Join ([#967](#967)) ([33c32a8](33c32a8)), closes [#916](#916) [#958](#958)

BREAKING CHANGES
=========

* DynamoDB AutoScaling: Instead of `addReadAutoScaling()`, call `autoScaleReadCapacity()`, and similar for write scaling.
* CloudFormation resource usage: If you use L1s, you may need to change some `XxxName` properties back into `Name`. These will match the CloudFormation property names.
* You must use the matching `aws-cdk` toolkit when upgrading to this version, or context providers will cease to work. All existing cached context values in `cdk.json` will be invalidated and refreshed.
jonparker pushed a commit to jonparker/aws-cdk that referenced this pull request Oct 29, 2018
__IMPORTANT NOTE__: when upgrading to this version of the CDK framework, you must also upgrade
your installation the CDK Toolkit to the matching version:

```shell
$ npm i -g aws-cdk
$ cdk --version
0.14.0 (build ...)
```

Bug Fixes
=========

* remove CloudFormation property renames ([aws#973](aws#973)) ([3f86603](aws@3f86603)), closes [aws#852](aws#852)
* **aws-ec2:** fix retention of all egress traffic rule ([aws#998](aws#998)) ([b9d5b43](aws@b9d5b43)), closes [aws#987](aws#987)
* **aws-s3-deployment:** avoid deletion during update using physical ids ([aws#1006](aws#1006)) ([bca99c6](aws@bca99c6)), closes [aws#981](aws#981) [aws#981](aws#981)
* **cloudformation-diff:** ignore changes to DependsOn ([aws#1005](aws#1005)) ([3605f9c](aws@3605f9c)), closes [aws#274](aws#274)
* **cloudformation-diff:** track replacements ([aws#1003](aws#1003)) ([a83ac5f](aws@a83ac5f)), closes [aws#1001](aws#1001)
* **docs:** fix EC2 readme for "natgatway" configuration ([aws#994](aws#994)) ([0b1e7cc](aws@0b1e7cc))
* **docs:** updates to contribution guide ([aws#997](aws#997)) ([b42e742](aws@b42e742))
* **iam:** Merge multiple principals correctly ([aws#983](aws#983)) ([3fc5c8c](aws@3fc5c8c)), closes [aws#924](aws#924) [aws#916](aws#916) [aws#958](aws#958)

Features
=========

* add construct library for Application AutoScaling ([aws#933](aws#933)) ([7861c6f](aws@7861c6f)), closes [aws#856](aws#856) [aws#861](aws#861) [aws#640](aws#640) [aws#644](aws#644)
* add HostedZone context provider ([aws#823](aws#823)) ([1626c37](aws@1626c37))
* **assert:** haveResource lists failing properties ([aws#1016](aws#1016)) ([7f6f3fd](aws@7f6f3fd))
* **aws-cdk:** add CDK app version negotiation ([aws#988](aws#988)) ([db4e718](aws@db4e718)), closes [aws#891](aws#891)
* **aws-codebuild:** Introduce a CodePipeline test Action. ([aws#873](aws#873)) ([770f9aa](aws@770f9aa))
* **aws-sqs:** Add grantXxx() methods ([aws#1004](aws#1004)) ([8c90350](aws@8c90350))
* **core:** Pre-concatenate Fn::Join ([aws#967](aws#967)) ([33c32a8](aws@33c32a8)), closes [aws#916](aws#916) [aws#958](aws#958)

BREAKING CHANGES
=========

* DynamoDB AutoScaling: Instead of `addReadAutoScaling()`, call `autoScaleReadCapacity()`, and similar for write scaling.
* CloudFormation resource usage: If you use L1s, you may need to change some `XxxName` properties back into `Name`. These will match the CloudFormation property names.
* You must use the matching `aws-cdk` toolkit when upgrading to this version, or context providers will cease to work. All existing cached context values in `cdk.json` will be invalidated and refreshed.
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add protocol version to App output
4 participants