From 90282693999efdc43330b9526b9d7f4cd0fa5736 Mon Sep 17 00:00:00 2001 From: Alban Esc Date: Tue, 9 Feb 2021 03:08:59 -0800 Subject: [PATCH 1/3] fix(ec2): Subnet cidr missing for Vpc.from_lookup() (#12878) When importing a VPC using `Vpc.fromLookup()`, the subnets are properly looked up and saved into the `cdk.context.json`. However, the Subnets object within the imported VPC don't have the CIDR attached and trying to access the subnet CIDR within the code triggers the following error: > You cannot reference an imported Subnet's IPv4 CIDR if it was not supplied. Add the ipv4CidrBlock when importing using Subnet.fromSubnetAttributes(). This PR fixes the issue. closes #11821 ---- *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/vpc.ts | 1 + .../aws-ec2/test/vpc.from-lookup.test.ts | 41 +++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/packages/@aws-cdk/aws-ec2/lib/vpc.ts b/packages/@aws-cdk/aws-ec2/lib/vpc.ts index 4e3f90780db4c..2a4e524fc5150 100644 --- a/packages/@aws-cdk/aws-ec2/lib/vpc.ts +++ b/packages/@aws-cdk/aws-ec2/lib/vpc.ts @@ -1886,6 +1886,7 @@ class LookedUpVpc extends VpcBase { availabilityZone: vpcSubnet.availabilityZone, subnetId: vpcSubnet.subnetId, routeTableId: vpcSubnet.routeTableId, + ipv4CidrBlock: vpcSubnet.cidr, })); } return ret; diff --git a/packages/@aws-cdk/aws-ec2/test/vpc.from-lookup.test.ts b/packages/@aws-cdk/aws-ec2/test/vpc.from-lookup.test.ts index 5555dc3fa9ed7..12ed7d05329d5 100644 --- a/packages/@aws-cdk/aws-ec2/test/vpc.from-lookup.test.ts +++ b/packages/@aws-cdk/aws-ec2/test/vpc.from-lookup.test.ts @@ -210,6 +210,47 @@ nodeunitShim({ test.done(); }, + 'subnets in imported VPC has all expected attributes'(test: Test) { + const previous = mockVpcContextProviderWith(test, { + vpcId: 'vpc-1234', + subnetGroups: [ + { + name: 'Public', + type: cxapi.VpcSubnetGroupType.PUBLIC, + subnets: [ + { + subnetId: 'pub-sub-in-us-east-1a', + availabilityZone: 'us-east-1a', + routeTableId: 'rt-123', + cidr: '10.100.0.0/24', + }, + ], + }, + ], + }, options => { + test.deepEqual(options.filter, { + isDefault: 'true', + }); + + test.equal(options.subnetGroupNameTag, undefined); + }); + + const stack = new Stack(); + const vpc = Vpc.fromLookup(stack, 'Vpc', { + isDefault: true, + }); + + let subnet = vpc.publicSubnets[0]; + + test.equal(subnet.availabilityZone, 'us-east-1a'); + test.equal(subnet.subnetId, 'pub-sub-in-us-east-1a'); + test.equal(subnet.routeTable.routeTableId, 'rt-123'); + test.equal(subnet.ipv4CidrBlock, '10.100.0.0/24'); + + + restoreContextProvider(previous); + test.done(); + }, }, }); From 2b917eceb598b3365123781445df7e2bd8a80b74 Mon Sep 17 00:00:00 2001 From: Nick Lynch Date: Tue, 9 Feb 2021 11:47:31 +0000 Subject: [PATCH 2/3] fix(kms): cross-environment usage fails when trustAccountIdentities is set (#12925) When the `trustAccountIdentities` flag is set (either directly, or set by default via the `@aws-cdk/aws-kms:defaultKeyPolicies` feature flag), any grant statements are always added to the principal OR resource, defaulting to the principal if possible. In cross-environment usage, this means that the principal IAM policy is updated, but the key policy is never updated. However, cross-environment usage always requires the key policy to be updated. Attempting to use the key in a cross-environment usage with `trustAccountIdentities` enabled initially presents itself as a cross-environment reference issue, but even if the stack was successfully deployed, the delegated account would not have access to use/admin the key. This change updates the logic to make use of the existing cross-environment logic whenever cross-environment usage is detected, regardless of the value of `trustAccountIdentities`. This has the impact of fixing the cross-env references and ensuring the key policy is properly updated. fixes #12921, fixes #12741 ---- *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-kms/lib/key.ts | 2 +- packages/@aws-cdk/aws-kms/test/key.test.ts | 97 ++++++++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-kms/lib/key.ts b/packages/@aws-cdk/aws-kms/lib/key.ts index d8de804e61a12..4e18d228fe38e 100644 --- a/packages/@aws-cdk/aws-kms/lib/key.ts +++ b/packages/@aws-cdk/aws-kms/lib/key.ts @@ -156,7 +156,7 @@ abstract class KeyBase extends Resource implements IKey { resourceArns: [this.keyArn], resourceSelfArns: crossEnvironment ? undefined : ['*'], }; - if (this.trustAccountIdentities) { + if (this.trustAccountIdentities && !crossEnvironment) { return iam.Grant.addToPrincipalOrResource(grantOptions); } else { return iam.Grant.addToPrincipalAndResource({ diff --git a/packages/@aws-cdk/aws-kms/test/key.test.ts b/packages/@aws-cdk/aws-kms/test/key.test.ts index 42461faece6f4..b2d37496d00ec 100644 --- a/packages/@aws-cdk/aws-kms/test/key.test.ts +++ b/packages/@aws-cdk/aws-kms/test/key.test.ts @@ -245,6 +245,103 @@ describe('key policies', () => { }); }); + testFutureBehavior('grant for a principal in a different region', flags, cdk.App, (app) => { + const principalStack = new cdk.Stack(app, 'PrincipalStack', { env: { region: 'testregion1' } }); + const principal = new iam.Role(principalStack, 'Role', { + assumedBy: new iam.AnyPrincipal(), + roleName: 'MyRolePhysicalName', + }); + + const keyStack = new cdk.Stack(app, 'KeyStack', { env: { region: 'testregion2' } }); + const key = new kms.Key(keyStack, 'Key'); + + key.grantEncrypt(principal); + + expect(keyStack).toHaveResourceLike('AWS::KMS::Key', { + KeyPolicy: { + Statement: arrayWith( + { + Action: [ + 'kms:Encrypt', + 'kms:ReEncrypt*', + 'kms:GenerateDataKey*', + ], + Effect: 'Allow', + Principal: { AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::', { Ref: 'AWS::AccountId' }, ':role/MyRolePhysicalName']] } }, + Resource: '*', + }, + ), + Version: '2012-10-17', + }, + }); + expect(principalStack).toHaveResourceLike('AWS::IAM::Policy', { + PolicyDocument: { + Statement: [ + { + Action: [ + 'kms:Encrypt', + 'kms:ReEncrypt*', + 'kms:GenerateDataKey*', + ], + Effect: 'Allow', + Resource: '*', + }, + ], + Version: '2012-10-17', + }, + }); + }); + + testFutureBehavior('grant for a principal in a different account', flags, cdk.App, (app) => { + const principalStack = new cdk.Stack(app, 'PrincipalStack', { env: { account: '0123456789012' } }); + const principal = new iam.Role(principalStack, 'Role', { + assumedBy: new iam.AnyPrincipal(), + roleName: 'MyRolePhysicalName', + }); + + const keyStack = new cdk.Stack(app, 'KeyStack', { env: { account: '111111111111' } }); + const key = new kms.Key(keyStack, 'Key'); + + key.grantEncrypt(principal); + + expect(keyStack).toHaveResourceLike('AWS::KMS::Key', { + KeyPolicy: { + Statement: [ + { + // Default policy, unmodified + }, + { + Action: [ + 'kms:Encrypt', + 'kms:ReEncrypt*', + 'kms:GenerateDataKey*', + ], + Effect: 'Allow', + Principal: { AWS: { 'Fn::Join': ['', ['arn:', { Ref: 'AWS::Partition' }, ':iam::0123456789012:role/MyRolePhysicalName']] } }, + Resource: '*', + }, + ], + Version: '2012-10-17', + }, + }); + expect(principalStack).toHaveResourceLike('AWS::IAM::Policy', { + PolicyDocument: { + Statement: [ + { + Action: [ + 'kms:Encrypt', + 'kms:ReEncrypt*', + 'kms:GenerateDataKey*', + ], + Effect: 'Allow', + Resource: '*', + }, + ], + Version: '2012-10-17', + }, + }); + }); + testFutureBehavior('additional key admins can be specified (with imported/immutable principal)', flags, cdk.App, (app) => { const stack = new cdk.Stack(app); const adminRole = iam.Role.fromRoleArn(stack, 'Admin', 'arn:aws:iam::123456789012:role/TrustedAdmin'); From 6597a09310fbc13d43389eca91b0e4b26f8ca680 Mon Sep 17 00:00:00 2001 From: kirintw Date: Tue, 9 Feb 2021 20:27:02 +0800 Subject: [PATCH 3/3] feat(core): configure bundling docker entrypoint (#12660) Allow customizing the docker entrypoint for the bundling image. Note that the option `entrypoint` is a `string[]` which match up with nearly every container runtime. The problem is that `docker run` 's `--entrypoint` only accepts a string. If the entrypoint we specify is `["/bin/sh", "-c"]`, the final command should be like: `docker run --entrypoint /bin/sh some-img -c some commands`. I have do some experiments to prove this: ```sh $ docker run -it --rm --name test --entrypoint /bin/sh -c alpine ls invalid argument "alpine" for "-c, --cpu-shares" flag: strconv.ParseInt: parsing "alpine": invalid syntax See 'docker run --help'. $docker run -it --rm --name test --entrypoint "/bin/sh -c" alpine ls docker: Error response from daemon: OCI runtime create failed: container_linux.go:349: starting container process caused "exec: \"/bin/sh -c\": stat /bin/sh -c: no such file or directory": unknown. $docker run -it --rm --name test --entrypoint /bin/sh alpine -c ls bin etc lib mnt proc run srv tmp var dev home media opt root sbin sys usr ``` One more thing: if one specify custom entrypoint but no custom commands, the default cmd for the image will not be presented. I do think it's a expected behavior for container runtime through. References: * [kubernetes - Define a Command and Arguments for a Container](https://kubernetes.io/docs/tasks/inject-data-application/define-command-argument-container/#notes) close #11984 ---- *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-s3-assets/README.md | 1 + packages/@aws-cdk/core/lib/bundling.ts | 31 +++++++++++++++- packages/@aws-cdk/core/test/bundling.test.ts | 38 ++++++++++++++++++++ 3 files changed, 69 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-s3-assets/README.md b/packages/@aws-cdk/aws-s3-assets/README.md index 3d508070d6a50..aab4c46d9c44d 100644 --- a/packages/@aws-cdk/aws-s3-assets/README.md +++ b/packages/@aws-cdk/aws-s3-assets/README.md @@ -115,6 +115,7 @@ new assets.Asset(this, 'BundledAsset', { }, // Docker bundling fallback image: BundlingDockerImage.fromRegistry('alpine'), + entrypoint: ['/bin/sh', '-c'], command: ['bundle'], }, }); diff --git a/packages/@aws-cdk/core/lib/bundling.ts b/packages/@aws-cdk/core/lib/bundling.ts index c9a9c07e77f34..b1247fd913ea0 100644 --- a/packages/@aws-cdk/core/lib/bundling.ts +++ b/packages/@aws-cdk/core/lib/bundling.ts @@ -13,6 +13,17 @@ export interface BundlingOptions { */ readonly image: BundlingDockerImage; + /** + * The entrypoint to run in the Docker container. + * + * @example ['/bin/sh', '-c'] + * + * @see https://docs.docker.com/engine/reference/builder/#entrypoint + * + * @default - run the entrypoint defined in the image + */ + readonly entrypoint?: string[]; + /** * The command to run in the Docker container. * @@ -152,7 +163,15 @@ export class BundlingDockerImage { public run(options: DockerRunOptions = {}) { const volumes = options.volumes || []; const environment = options.environment || {}; - const command = options.command || []; + const entrypoint = options.entrypoint?.[0] || null; + const command = [ + ...options.entrypoint?.[1] + ? [...options.entrypoint.slice(1)] + : [], + ...options.command + ? [...options.command] + : [], + ]; const dockerArgs: string[] = [ 'run', '--rm', @@ -164,6 +183,9 @@ export class BundlingDockerImage { ...options.workingDirectory ? ['-w', options.workingDirectory] : [], + ...entrypoint + ? ['--entrypoint', entrypoint] + : [], this.image, ...command, ]; @@ -238,6 +260,13 @@ export enum DockerVolumeConsistency { * Docker run options */ export interface DockerRunOptions { + /** + * The entrypoint to run in the container. + * + * @default - run the entrypoint defined in the image + */ + readonly entrypoint?: string[]; + /** * The command to run in the container. * diff --git a/packages/@aws-cdk/core/test/bundling.test.ts b/packages/@aws-cdk/core/test/bundling.test.ts index e2b0d6b43b98b..258860d65585c 100644 --- a/packages/@aws-cdk/core/test/bundling.test.ts +++ b/packages/@aws-cdk/core/test/bundling.test.ts @@ -171,6 +171,44 @@ nodeunitShim({ test.done(); }, + 'custom entrypoint is passed through to docker exec'(test: Test) { + const spawnSyncStub = sinon.stub(child_process, 'spawnSync').returns({ + status: 0, + stderr: Buffer.from('stderr'), + stdout: Buffer.from('stdout'), + pid: 123, + output: ['stdout', 'stderr'], + signal: null, + }); + + const image = BundlingDockerImage.fromRegistry('alpine'); + image.run({ + entrypoint: ['/cool/entrypoint', '--cool-entrypoint-arg'], + command: ['cool', 'command'], + environment: { + VAR1: 'value1', + VAR2: 'value2', + }, + volumes: [{ hostPath: '/host-path', containerPath: '/container-path' }], + workingDirectory: '/working-directory', + user: 'user:group', + }); + + test.ok(spawnSyncStub.calledWith('docker', [ + 'run', '--rm', + '-u', 'user:group', + '-v', '/host-path:/container-path:delegated', + '--env', 'VAR1=value1', + '--env', 'VAR2=value2', + '-w', '/working-directory', + '--entrypoint', '/cool/entrypoint', + 'alpine', + '--cool-entrypoint-arg', + 'cool', 'command', + ], { stdio: ['ignore', process.stderr, 'inherit'] })); + test.done(); + }, + 'cp utility copies from an image'(test: Test) { // GIVEN const containerId = '1234567890abcdef1234567890abcdef';