From d3861ee5ba12cd56c94137558114b0d78db6d7ab Mon Sep 17 00:00:00 2001 From: Neta Nir Date: Mon, 10 Aug 2020 03:18:38 -0700 Subject: [PATCH 1/2] chore(core): document how to override availability zones (#9504) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/@aws-cdk/aws-ec2/README.md | 18 ++++++++++++++++++ packages/@aws-cdk/core/lib/stack.ts | 4 +++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-ec2/README.md b/packages/@aws-cdk/aws-ec2/README.md index 991a2e39c8316..668943296ac4d 100644 --- a/packages/@aws-cdk/aws-ec2/README.md +++ b/packages/@aws-cdk/aws-ec2/README.md @@ -90,6 +90,24 @@ itself to 2 Availability Zones. Therefore, to get the VPC to spread over 3 or more availability zones, you must specify the environment where the stack will be deployed. +You can gain full control over the availability zones selection strategy by overriding the Stack's [`get availabilityZones()`](https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/core/lib/stack.ts) method: + +```ts +class MyStack extends Stack { + + get availabilityZones(): string[] { + return ['us-west-2a', 'us-west-2b']; + } + + constructor(scope: Construct, id: string, props?: StackProps) { + super(scope, id, props); + ... + } +} +``` + +Note that overriding the `get availabilityZones()` method will override the default behavior for all constructs defined within the Stack. + ### Choosing subnets for resources When creating resources that create Elastic Network Interfaces (such as diff --git a/packages/@aws-cdk/core/lib/stack.ts b/packages/@aws-cdk/core/lib/stack.ts index f913377b54a20..53433d0823fbe 100644 --- a/packages/@aws-cdk/core/lib/stack.ts +++ b/packages/@aws-cdk/core/lib/stack.ts @@ -571,7 +571,7 @@ export class Stack extends Construct implements ITaggable { } /** - * Returnst the list of AZs that are availability in the AWS environment + * Returns the list of AZs that are available in the AWS environment * (account/region) associated with this stack. * * If the stack is environment-agnostic (either account and/or region are @@ -582,6 +582,8 @@ export class Stack extends Construct implements ITaggable { * If they are not available in the context, returns a set of dummy values and * reports them as missing, and let the CLI resolve them by calling EC2 * `DescribeAvailabilityZones` on the target environment. + * + * To specify a different strategy for selecting availability zones override this method. */ public get availabilityZones(): string[] { // if account/region are tokens, we can't obtain AZs through the context From 74e839189b2e9b028e6b9944884bf8fe73de2429 Mon Sep 17 00:00:00 2001 From: Daniel Neilson <53624638+ddneilson@users.noreply.github.com> Date: Mon, 10 Aug 2020 05:40:36 -0500 Subject: [PATCH 2/2] fix(ec2): Volume grants have an overly complicated API (#9115) I designed & wrote the code for the L2 Volume construct. My design for the `grantAttachVolumeByResourceTag()` and `grantDetachVolumeByResourceTag()` is flawed. Those functions have three design requirements: 1) To allow an instance to attach a Volume to itself; 2) To allow an instance to attach multiple, different, volumes; and 3) To allow the same volume to be attached to different instances via separate grants without clobbering other grants. Since the implementation mechanism for this is a `ResourceTag` condition on the policy, the later two requirements mean that we must have both a unique tag key and a unique tag value for each separate permission grant (i.e. separate call to the volume's grant function). The original design had the tag key being derived from only the volume, the tag value from only the instance(s), and allowed for overriding the tag key via a parameter to allow for the same volume to attach to multiple instances over separate grants. In hindsight, we should be deriving both the resource tag and value from the combination of the instance(s) and the volume's unique properties. This completely eliminates the need for the tag key override. The current design results in code like: ```ts // To be able to mount the *same* volume to multiple instances we must provide a tag suffix to the permission grant // that is unique to this particular combination of volume + mount target. function hashUniqueIds(resources: IConstruct[]): string { const md5 = crypto.createHash('md5'); resources.forEach(res => md5.update(res.node.uniqueId)); return md5.digest('hex'); } this.props.blockVolume.grantAttachVolumeByResourceTag(target.grantPrincipal, [target], hashUniqueIds([target, this.props.blockVolume])); ``` It is much more desirable for this to be simply: ```ts this.props.blockVolume.grantAttachVolumeByResourceTag(target.grantPrincipal, [target]); ``` Resolves: #9114 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/@aws-cdk/aws-ec2/lib/volume.ts | 21 ++++-------- packages/@aws-cdk/aws-ec2/test/volume.test.ts | 34 +++++++++---------- 2 files changed, 22 insertions(+), 33 deletions(-) diff --git a/packages/@aws-cdk/aws-ec2/lib/volume.ts b/packages/@aws-cdk/aws-ec2/lib/volume.ts index d379ae6622b1f..e33c32a9d63b6 100644 --- a/packages/@aws-cdk/aws-ec2/lib/volume.ts +++ b/packages/@aws-cdk/aws-ec2/lib/volume.ts @@ -290,14 +290,10 @@ export interface IVolume extends IResource { * given the ability to AttachVolume if both the Volume and the destination Instance have that * tag applied to them. * - * If you need to call this method multiple times on different sets of constructs, then provide a - * unique `tagKeySuffix` for each call; failure to do so will result in an inability to attach this - * volume to some of the grants because it will overwrite the tag. - * * @param grantee the principal being granted permission. * @param constructs The list of constructs that will have the generated resource tag applied to them. * @param tagKeySuffix A suffix to use on the generated Tag key in place of the generated hash value. - * Defaults to a hash calculated from this volume. + * Defaults to a hash calculated from this volume and list of constructs. (DEPRECATED) */ grantAttachVolumeByResourceTag(grantee: IGrantable, constructs: Construct[], tagKeySuffix?: string): Grant; @@ -324,7 +320,7 @@ export interface IVolume extends IResource { * @param grantee the principal being granted permission. * @param constructs The list of constructs that will have the generated resource tag applied to them. * @param tagKeySuffix A suffix to use on the generated Tag key in place of the generated hash value. - * Defaults to a hash calculated from this volume. + * Defaults to a hash calculated from this volume and list of constructs. (DEPRECATED) */ grantDetachVolumeByResourceTag(grantee: IGrantable, constructs: Construct[], tagKeySuffix?: string): Grant; } @@ -497,8 +493,8 @@ abstract class VolumeBase extends Resource implements IVolume { } public grantAttachVolumeByResourceTag(grantee: IGrantable, constructs: Construct[], tagKeySuffix?: string): Grant { - const tagKey = `VolumeGrantAttach-${tagKeySuffix ?? this.stringHash(this.node.uniqueId)}`; - const tagValue = this.calculateResourceTagValue(constructs); + const tagValue = this.calculateResourceTagValue([this, ...constructs]); + const tagKey = `VolumeGrantAttach-${tagKeySuffix ?? tagValue.slice(0,10).toUpperCase()}`; const grantCondition: { [key: string]: string } = {}; grantCondition[`ec2:ResourceTag/${tagKey}`] = tagValue; @@ -526,8 +522,8 @@ abstract class VolumeBase extends Resource implements IVolume { } public grantDetachVolumeByResourceTag(grantee: IGrantable, constructs: Construct[], tagKeySuffix?: string): Grant { - const tagKey = `VolumeGrantDetach-${tagKeySuffix ?? this.stringHash(this.node.uniqueId)}`; - const tagValue = this.calculateResourceTagValue(constructs); + const tagValue = this.calculateResourceTagValue([this, ...constructs]); + const tagKey = `VolumeGrantDetach-${tagKeySuffix ?? tagValue.slice(0,10).toUpperCase()}`; const grantCondition: { [key: string]: string } = {}; grantCondition[`ec2:ResourceTag/${tagKey}`] = tagValue; @@ -558,11 +554,6 @@ abstract class VolumeBase extends Resource implements IVolume { return resourceArns; } - private stringHash(value: string): string { - const md5 = crypto.createHash('md5').update(value).digest('hex'); - return md5.slice(0, 8).toUpperCase(); - } - private calculateResourceTagValue(constructs: Construct[]): string { const md5 = crypto.createHash('md5'); constructs.forEach(construct => md5.update(construct.node.uniqueId)); diff --git a/packages/@aws-cdk/aws-ec2/test/volume.test.ts b/packages/@aws-cdk/aws-ec2/test/volume.test.ts index 0600d201755a7..05ae59f8d7a0f 100644 --- a/packages/@aws-cdk/aws-ec2/test/volume.test.ts +++ b/packages/@aws-cdk/aws-ec2/test/volume.test.ts @@ -739,7 +739,7 @@ nodeunitShim({ ], Condition: { 'ForAnyValue:StringEquals': { - 'ec2:ResourceTag/VolumeGrantAttach-BD7A9717': 'd9a17c1c9e8ef6866e4dbeef41c741b2', + 'ec2:ResourceTag/VolumeGrantAttach-B2376B2BDA': 'b2376b2bda65cb40f83c290dd844c4aa', }, }, }], @@ -748,8 +748,8 @@ nodeunitShim({ cdkExpect(stack).to(haveResourceLike('AWS::EC2::Volume', { Tags: [ { - Key: 'VolumeGrantAttach-BD7A9717', - Value: 'd9a17c1c9e8ef6866e4dbeef41c741b2', + Key: 'VolumeGrantAttach-B2376B2BDA', + Value: 'b2376b2bda65cb40f83c290dd844c4aa', }, ], }, ResourcePart.Properties)); @@ -757,8 +757,8 @@ nodeunitShim({ Tags: [ {}, { - Key: 'VolumeGrantAttach-BD7A9717', - Value: 'd9a17c1c9e8ef6866e4dbeef41c741b2', + Key: 'VolumeGrantAttach-B2376B2BDA', + Value: 'b2376b2bda65cb40f83c290dd844c4aa', }, ], }, ResourcePart.Properties)); @@ -816,7 +816,7 @@ nodeunitShim({ ], Condition: { 'ForAnyValue:StringEquals': { - 'ec2:ResourceTag/VolumeGrantAttach-TestSuffix': 'd9a17c1c9e8ef6866e4dbeef41c741b2', + 'ec2:ResourceTag/VolumeGrantAttach-TestSuffix': 'b2376b2bda65cb40f83c290dd844c4aa', }, }, }], @@ -826,7 +826,7 @@ nodeunitShim({ Tags: [ { Key: 'VolumeGrantAttach-TestSuffix', - Value: 'd9a17c1c9e8ef6866e4dbeef41c741b2', + Value: 'b2376b2bda65cb40f83c290dd844c4aa', }, ], }, ResourcePart.Properties)); @@ -835,11 +835,10 @@ nodeunitShim({ {}, { Key: 'VolumeGrantAttach-TestSuffix', - Value: 'd9a17c1c9e8ef6866e4dbeef41c741b2', + Value: 'b2376b2bda65cb40f83c290dd844c4aa', }, ], }, ResourcePart.Properties)); - test.done(); }, @@ -1051,7 +1050,7 @@ nodeunitShim({ ], Condition: { 'ForAnyValue:StringEquals': { - 'ec2:ResourceTag/VolumeGrantDetach-BD7A9717': 'd9a17c1c9e8ef6866e4dbeef41c741b2', + 'ec2:ResourceTag/VolumeGrantDetach-B2376B2BDA': 'b2376b2bda65cb40f83c290dd844c4aa', }, }, }], @@ -1060,8 +1059,8 @@ nodeunitShim({ cdkExpect(stack).to(haveResourceLike('AWS::EC2::Volume', { Tags: [ { - Key: 'VolumeGrantDetach-BD7A9717', - Value: 'd9a17c1c9e8ef6866e4dbeef41c741b2', + Key: 'VolumeGrantDetach-B2376B2BDA', + Value: 'b2376b2bda65cb40f83c290dd844c4aa', }, ], }, ResourcePart.Properties)); @@ -1069,8 +1068,8 @@ nodeunitShim({ Tags: [ {}, { - Key: 'VolumeGrantDetach-BD7A9717', - Value: 'd9a17c1c9e8ef6866e4dbeef41c741b2', + Key: 'VolumeGrantDetach-B2376B2BDA', + Value: 'b2376b2bda65cb40f83c290dd844c4aa', }, ], }, ResourcePart.Properties)); @@ -1128,7 +1127,7 @@ nodeunitShim({ ], Condition: { 'ForAnyValue:StringEquals': { - 'ec2:ResourceTag/VolumeGrantDetach-TestSuffix': 'd9a17c1c9e8ef6866e4dbeef41c741b2', + 'ec2:ResourceTag/VolumeGrantDetach-TestSuffix': 'b2376b2bda65cb40f83c290dd844c4aa', }, }, }], @@ -1138,7 +1137,7 @@ nodeunitShim({ Tags: [ { Key: 'VolumeGrantDetach-TestSuffix', - Value: 'd9a17c1c9e8ef6866e4dbeef41c741b2', + Value: 'b2376b2bda65cb40f83c290dd844c4aa', }, ], }, ResourcePart.Properties)); @@ -1147,11 +1146,10 @@ nodeunitShim({ {}, { Key: 'VolumeGrantDetach-TestSuffix', - Value: 'd9a17c1c9e8ef6866e4dbeef41c741b2', + Value: 'b2376b2bda65cb40f83c290dd844c4aa', }, ], }, ResourcePart.Properties)); - test.done(); },