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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Oct 25, 2018
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e443a23
feat(aws-cdk): add CDK app version negotiation
Oct 22, 2018
14b4376
Remove trailing import
Oct 23, 2018
7406207
Assert that parsing succeeded
Oct 23, 2018
0e006a5
Update test failure
Oct 24, 2018
fbdeec3
Add explicit dependency on used 'semver' package
Oct 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/@aws-cdk/cdk/lib/app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export class App extends Root {
}

const result: cxapi.SynthesizeResponse = {
version: cxapi.PROTO_RESPONSE_VERSION,
stacks: this.synthesizeStacks(Object.keys(this.stacks)),
runtime: this.collectRuntimeInformation()
};
Expand Down
4 changes: 3 additions & 1 deletion packages/@aws-cdk/cdk/test/test.app.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,9 @@ export = {
response.stacks.forEach(s => delete s.metadata);
delete response.runtime;

test.deepEqual(response, { stacks:
test.deepEqual(response, {
version: '0.14.0',
stacks:
[ { name: 'stack1',
environment:
{ name: '12345/us-east-1',
Expand Down
22 changes: 22 additions & 0 deletions packages/@aws-cdk/cx-api/lib/cxapi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,24 @@

import { Environment } from './environment';

/**
* Bump this to the library version if and only if the CX protocol changes.
*
* We could also have used 1, 2, 3, ... here to indicate protocol versions, but
* those then still need to be mapped to software versions to be useful. So we
* might as well use the software version as protocol version and immediately
* generate a useful error message from this.
*
* 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.

* 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.

* 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 •
edited
Loading

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.


export const OUTFILE_NAME = 'cdk.out';
export const OUTDIR_ENV = 'CDK_OUTDIR';
export const CONTEXT_ENV = 'CDK_CONTEXT_JSON';
Expand All @@ -21,6 +39,10 @@ export interface MissingContext {
}

export interface SynthesizeResponse {
/**
* Protocol version
*/
version: string;
stacks: SynthesizedStack[];
runtime?: AppRuntime;
}
Expand Down
33 changes: 32 additions & 1 deletion packages/aws-cdk/bin/cdk.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import YAML = require('js-yaml');
import minimatch = require('minimatch');
import os = require('os');
import path = require('path');
import semver = require('semver');
import util = require('util');
import yargs = require('yargs');
import cdkUtil = require('../lib/util');
Expand Down Expand Up @@ -469,7 +470,7 @@ async function initCommandLine() {

const response = await fs.readJson(outfile);
debug(response);
return response;
return versionCheckResponse(response);
} finally {
debug('Removing outdir', outdir);
await fs.remove(outdir);
Expand Down Expand Up @@ -508,7 +509,37 @@ async function initCommandLine() {
});
}
}
}

/**
* Look at the type of response we get and upgrade it to the latest expected version
*/
function versionCheckResponse(response: cxapi.SynthesizeResponse): cxapi.SynthesizeResponse {
if (!response.version) {
// tslint:disable-next-line:max-line-length
throw new Error(`CDK Framework >= ${cxapi.PROTO_RESPONSE_VERSION} is required in order to interact with this version of the Toolkit.`);
}

const frameworkVersion = semver.coerce(response.version);
const toolkitVersion = semver.coerce(cxapi.PROTO_RESPONSE_VERSION);

// Should not happen, but I don't trust this library 100% either, so let's check for it to be safe
if (!frameworkVersion || !toolkitVersion) { throw new Error('SemVer library could not parse versions'); }

if (semver.gt(frameworkVersion, toolkitVersion)) {
throw new Error(`CDK Toolkit >= ${response.version} is required in order to interact with this program.`);
}

if (semver.lt(frameworkVersion, toolkitVersion)) {
// Toolkit protocol is newer than the framework version, and we KNOW the
// version. This is a scenario in which we could potentially do some
// upgrading of the response in the future.
//
// For now though, we simply reject old responses.
throw new Error(`CDK Framework >= ${cxapi.PROTO_RESPONSE_VERSION} is required in order to interact with this version of the Toolkit.`);
}

return response;
}

/**
Expand Down
1 change: 1 addition & 0 deletions packages/aws-cdk/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

"cdk-build-tools": "^0.13.0",
"mockery": "^2.1.0",
"pkglint": "^0.13.0"
Expand Down