From dd22e8fc29f1fc33d391d1bb9ae93963bfd82563 Mon Sep 17 00:00:00 2001 From: Benura Abeywardena <43112139+BLasan@users.noreply.github.com> Date: Fri, 26 Mar 2021 09:24:16 +0530 Subject: [PATCH 1/3] fix(rds): fail with a descriptive error if Cluster's instance count is a deploy-time value (#13765) Added a condition to check whether the `instanceCount` is a token or not. If it's not a token then an exception will be thrown. Fixes #13558 ---- *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-rds/lib/cluster.ts | 3 ++ .../@aws-cdk/aws-rds/test/cluster.test.ts | 50 ++++++------------- 2 files changed, 17 insertions(+), 36 deletions(-) diff --git a/packages/@aws-cdk/aws-rds/lib/cluster.ts b/packages/@aws-cdk/aws-rds/lib/cluster.ts index a3f06555afdb9..4e4e86f6a4d7f 100644 --- a/packages/@aws-cdk/aws-rds/lib/cluster.ts +++ b/packages/@aws-cdk/aws-rds/lib/cluster.ts @@ -651,6 +651,9 @@ interface InstanceConfig { */ function createInstances(cluster: DatabaseClusterNew, props: DatabaseClusterBaseProps, subnetGroup: ISubnetGroup): InstanceConfig { const instanceCount = props.instances != null ? props.instances : 2; + if (Token.isUnresolved(instanceCount)) { + throw new Error('The number of instances an RDS Cluster consists of cannot be provided as a deploy-time only value!'); + } if (instanceCount < 1) { throw new Error('At least one instance is required'); } diff --git a/packages/@aws-cdk/aws-rds/test/cluster.test.ts b/packages/@aws-cdk/aws-rds/test/cluster.test.ts index 74f538be5d998..de7c5900de836 100644 --- a/packages/@aws-cdk/aws-rds/test/cluster.test.ts +++ b/packages/@aws-cdk/aws-rds/test/cluster.test.ts @@ -50,8 +50,22 @@ describe('cluster', () => { DeletionPolicy: 'Delete', UpdateReplacePolicy: 'Delete', }, ResourcePart.CompleteDefinition); + }); + test('validates that the number of instances is not a deploy-time value', () => { + const stack = testStack(); + const vpc = new ec2.Vpc(stack, 'VPC'); + const parameter = new cdk.CfnParameter(stack, 'Param', { type: 'Number' }); + expect(() => { + new DatabaseCluster(stack, 'Database', { + instances: parameter.valueAsNumber, + engine: DatabaseClusterEngine.AURORA, + instanceProps: { + vpc, + }, + }); + }).toThrow('The number of instances an RDS Cluster consists of cannot be provided as a deploy-time only value!'); }); test('can create a cluster with a single instance', () => { @@ -81,8 +95,6 @@ describe('cluster', () => { MasterUserPassword: 'tooshort', VpcSecurityGroupIds: [{ 'Fn::GetAtt': ['DatabaseSecurityGroup5C91FDCB', 'GroupId'] }], }); - - }); test('can create a cluster with imported vpc and security group', () => { @@ -116,8 +128,6 @@ describe('cluster', () => { MasterUserPassword: 'tooshort', VpcSecurityGroupIds: ['SecurityGroupId12345'], }); - - }); test('cluster with parameter group', () => { @@ -150,8 +160,6 @@ describe('cluster', () => { expect(stack).toHaveResource('AWS::RDS::DBCluster', { DBClusterParameterGroupName: { Ref: 'ParamsA8366201' }, }); - - }); test("sets the retention policy of the SubnetGroup to 'Retain' if the Cluster is created with 'Retain'", () => { @@ -172,8 +180,6 @@ describe('cluster', () => { DeletionPolicy: 'Retain', UpdateReplacePolicy: 'Retain', }, ResourcePart.CompleteDefinition); - - }); test('creates a secret when master credentials are not specified', () => { @@ -230,8 +236,6 @@ describe('cluster', () => { SecretStringTemplate: '{"username":"admin"}', }, }); - - }); test('create an encrypted cluster with custom KMS key', () => { @@ -261,8 +265,6 @@ describe('cluster', () => { ], }, }); - - }); test('cluster with instance parameter group', () => { @@ -294,8 +296,6 @@ describe('cluster', () => { Ref: 'ParameterGroup5E32DECB', }, }); - - }); describe('performance insights', () => { @@ -323,8 +323,6 @@ describe('cluster', () => { PerformanceInsightsRetentionPeriod: 731, PerformanceInsightsKMSKeyId: { 'Fn::GetAtt': ['Key961B73FD', 'Arn'] }, }); - - }); test('setting performance insights fields enables performance insights', () => { @@ -348,8 +346,6 @@ describe('cluster', () => { EnablePerformanceInsights: true, PerformanceInsightsRetentionPeriod: 731, }); - - }); test('throws if performance insights fields are set but performance insights is disabled', () => { @@ -370,8 +366,6 @@ describe('cluster', () => { }, }); }).toThrow(/`enablePerformanceInsights` disabled, but `performanceInsightRetention` or `performanceInsightEncryptionKey` was set/); - - }); }); @@ -392,8 +386,6 @@ describe('cluster', () => { expect(stack).toHaveResource('AWS::RDS::DBInstance', { AutoMinorVersionUpgrade: false, }); - - }); test('cluster with allow upgrade of major version', () => { @@ -413,8 +405,6 @@ describe('cluster', () => { expect(stack).toHaveResourceLike('AWS::RDS::DBInstance', { AllowMajorVersionUpgrade: true, }); - - }); test('cluster with disallow remove backups', () => { @@ -434,8 +424,6 @@ describe('cluster', () => { expect(stack).toHaveResourceLike('AWS::RDS::DBInstance', { DeleteAutomatedBackups: false, }); - - }); test('create a cluster using a specific version of MySQL', () => { @@ -462,8 +450,6 @@ describe('cluster', () => { Engine: 'aurora-mysql', EngineVersion: '5.7.mysql_aurora.2.04.4', }); - - }); test('create a cluster using a specific version of Postgresql', () => { @@ -513,8 +499,6 @@ describe('cluster', () => { // THEN expect(stack.resolve(cluster.clusterEndpoint)).not.toEqual(stack.resolve(cluster.clusterReadEndpoint)); - - }); test('imported cluster with imported security group honors allowAllOutbound', () => { @@ -540,8 +524,6 @@ describe('cluster', () => { expect(stack).toHaveResource('AWS::EC2::SecurityGroupEgress', { GroupId: 'sg-123456789', }); - - }); test('can import a cluster with minimal attributes', () => { @@ -567,8 +549,6 @@ describe('cluster', () => { expect(() => cluster.clusterReadEndpoint).toThrow(/Cannot access `clusterReadEndpoint` of an imported cluster/); expect(() => cluster.instanceIdentifiers).toThrow(/Cannot access `instanceIdentifiers` of an imported cluster/); expect(() => cluster.instanceEndpoints).toThrow(/Cannot access `instanceEndpoints` of an imported cluster/); - - }); test('imported cluster can access properties if attributes are provided', () => { @@ -590,8 +570,6 @@ describe('cluster', () => { expect(cluster.clusterReadEndpoint.socketAddress).toEqual('reader-address:3306'); expect(cluster.instanceIdentifiers).toEqual(['identifier']); expect(cluster.instanceEndpoints.map(endpoint => endpoint.socketAddress)).toEqual(['instance-addr:3306']); - - }); test('cluster supports metrics', () => { From 9cceb3f855b1ece2effe60b5a8b84f2986c270c4 Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Fri, 26 Mar 2021 13:14:35 +0000 Subject: [PATCH 2/3] chore(core): remove all references to BundlingDockerImage in the public API (#13814) A previous commit - ad01099d - deprecated BundlingDockerImage in favour of DockerImage. However, there are still uses of BundlingDockerImage that remain. Since bundling is still experimental, swap all uses of BundlingDockerImage and replace with DockerImage. Motivation No non-deprecated public API can reference a deprecated type as part of CDKv2. BREAKING CHANGE: The type of the `image` property in `BundlingOptions` is changed from `BundlingDockerImage` to `DockerImage`. * **core:** The return type of the `DockerImage.fromBuild()` API is changed from `BundlingDockerImage` to `DockerImage`. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/@aws-cdk/core/lib/bundling.ts | 58 ++++++++++---------- packages/@aws-cdk/core/test/bundling.test.ts | 4 +- 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/packages/@aws-cdk/core/lib/bundling.ts b/packages/@aws-cdk/core/lib/bundling.ts index a9567fe6e464e..5b4a579caf8f1 100644 --- a/packages/@aws-cdk/core/lib/bundling.ts +++ b/packages/@aws-cdk/core/lib/bundling.ts @@ -12,7 +12,7 @@ export interface BundlingOptions { /** * The Docker image where the command will run. */ - readonly image: BundlingDockerImage; + readonly image: DockerImage; /** * The entrypoint to run in the Docker container. @@ -158,33 +158,7 @@ export class BundlingDockerImage { * @deprecated use DockerImage.fromBuild() */ public static fromAsset(path: string, options: DockerBuildOptions = {}) { - const buildArgs = options.buildArgs || {}; - - if (options.file && isAbsolute(options.file)) { - throw new Error(`"file" must be relative to the docker build directory. Got ${options.file}`); - } - - // Image tag derived from path and build options - const input = JSON.stringify({ path, ...options }); - const tagHash = crypto.createHash('sha256').update(input).digest('hex'); - const tag = `cdk-${tagHash}`; - - const dockerArgs: string[] = [ - 'build', '-t', tag, - ...(options.file ? ['-f', join(path, options.file)] : []), - ...flatten(Object.entries(buildArgs).map(([k, v]) => ['--build-arg', `${k}=${v}`])), - path, - ]; - - dockerExec(dockerArgs); - - // Fingerprints the directory containing the Dockerfile we're building and - // differentiates the fingerprint based on build arguments. We do this so - // we can provide a stable image hash. Otherwise, the image ID will be - // different every time the Docker layer cache is cleared, due primarily to - // timestamps. - const hash = FileSystem.fingerprint(path, { extraHash: JSON.stringify(options) }); - return new BundlingDockerImage(tag, hash); + DockerImage.fromBuild(path, options) as BundlingDockerImage; } /** @param image The Docker image */ @@ -276,7 +250,33 @@ export class DockerImage extends BundlingDockerImage { * @param options Docker build options */ public static fromBuild(path: string, options: DockerBuildOptions = {}) { - return BundlingDockerImage.fromAsset(path, options); + const buildArgs = options.buildArgs || {}; + + if (options.file && isAbsolute(options.file)) { + throw new Error(`"file" must be relative to the docker build directory. Got ${options.file}`); + } + + // Image tag derived from path and build options + const input = JSON.stringify({ path, ...options }); + const tagHash = crypto.createHash('sha256').update(input).digest('hex'); + const tag = `cdk-${tagHash}`; + + const dockerArgs: string[] = [ + 'build', '-t', tag, + ...(options.file ? ['-f', join(path, options.file)] : []), + ...flatten(Object.entries(buildArgs).map(([k, v]) => ['--build-arg', `${k}=${v}`])), + path, + ]; + + dockerExec(dockerArgs); + + // Fingerprints the directory containing the Dockerfile we're building and + // differentiates the fingerprint based on build arguments. We do this so + // we can provide a stable image hash. Otherwise, the image ID will be + // different every time the Docker layer cache is cleared, due primarily to + // timestamps. + const hash = FileSystem.fingerprint(path, { extraHash: JSON.stringify(options) }); + return new DockerImage(tag, hash); } /** diff --git a/packages/@aws-cdk/core/test/bundling.test.ts b/packages/@aws-cdk/core/test/bundling.test.ts index 99548030c011a..1c909142b3127 100644 --- a/packages/@aws-cdk/core/test/bundling.test.ts +++ b/packages/@aws-cdk/core/test/bundling.test.ts @@ -60,7 +60,7 @@ nodeunitShim({ const fingerprintStub = sinon.stub(FileSystem, 'fingerprint'); fingerprintStub.callsFake(() => imageHash); - const image = BundlingDockerImage.fromAsset('docker-path', { + const image = DockerImage.fromBuild('docker-path', { buildArgs: { TEST_ARG: 'cdk-test', }, @@ -139,7 +139,7 @@ nodeunitShim({ const fingerprintStub = sinon.stub(FileSystem, 'fingerprint'); fingerprintStub.callsFake(() => imageHash); - const image = BundlingDockerImage.fromAsset('docker-path'); + const image = DockerImage.fromBuild('docker-path'); const tagHash = crypto.createHash('sha256').update(JSON.stringify({ path: 'docker-path', From 88f2c5ac302145f0b56462943a72e2b133c70bdf Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Fri, 26 Mar 2021 14:29:00 +0000 Subject: [PATCH 3/3] chore(aws-cdk-lib): strip out deprecated symbols from public API (#13815) The jsii build the 'aws-cdk-lib' module will now run with the 'strip deprecated' flag enabled. This ensures that the public API of this module will contain no deprecated symbols. This is enabled by a new option configured in the module's `package.json` and recognized by `cdk-build`. Previous commits - ca391b59 and a872e672 - have removed the majority of deprecated symbols from public APIs. A few remain that are removed as part of this change. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../@aws-cdk/aws-lambda/lib/log-retention.ts | 2 -- packages/@aws-cdk/aws-lambda/lib/runtime.ts | 9 ++++++++- packages/@aws-cdk/core/lib/construct-compat.ts | 8 +++++++- packages/@aws-cdk/core/lib/stack.ts | 18 +++++++++++++----- packages/aws-cdk-lib/package.json | 3 ++- tools/cdk-build-tools/lib/compile.ts | 2 +- tools/cdk-build-tools/lib/package-info.ts | 14 ++++++++++++-- 7 files changed, 43 insertions(+), 13 deletions(-) diff --git a/packages/@aws-cdk/aws-lambda/lib/log-retention.ts b/packages/@aws-cdk/aws-lambda/lib/log-retention.ts index e499764f96e20..3fe6db5e81880 100644 --- a/packages/@aws-cdk/aws-lambda/lib/log-retention.ts +++ b/packages/@aws-cdk/aws-lambda/lib/log-retention.ts @@ -3,8 +3,6 @@ import { Construct } from 'constructs'; /** * Retry options for all AWS API calls. - * - * @deprecated use `LogRetentionRetryOptions` from '@aws-cdk/aws-logs' instead */ export interface LogRetentionRetryOptions extends logs.LogRetentionRetryOptions { } diff --git a/packages/@aws-cdk/aws-lambda/lib/runtime.ts b/packages/@aws-cdk/aws-lambda/lib/runtime.ts index bb24fbba9eeae..e7e1451b79125 100644 --- a/packages/@aws-cdk/aws-lambda/lib/runtime.ts +++ b/packages/@aws-cdk/aws-lambda/lib/runtime.ts @@ -209,16 +209,23 @@ export class Runtime { public readonly family?: RuntimeFamily; /** - * The bundling Docker image for this runtime. + * DEPRECATED + * @deprecated use `bundlingImage` */ public readonly bundlingDockerImage: BundlingDockerImage; + /** + * The bundling Docker image for this runtime. + */ + public readonly bundlingImage: DockerImage; + constructor(name: string, family?: RuntimeFamily, props: LambdaRuntimeProps = { }) { this.name = name; this.supportsInlineCode = !!props.supportsInlineCode; this.family = family; const imageName = props.bundlingDockerImage ?? `amazon/aws-sam-cli-build-image-${name}`; this.bundlingDockerImage = DockerImage.fromRegistry(imageName); + this.bundlingImage = this.bundlingDockerImage; this.supportsCodeGuruProfiling = props.supportsCodeGuruProfiling ?? false; Runtime.ALL.push(this); diff --git a/packages/@aws-cdk/core/lib/construct-compat.ts b/packages/@aws-cdk/core/lib/construct-compat.ts index ac7873a837dde..09a6fbd929496 100644 --- a/packages/@aws-cdk/core/lib/construct-compat.ts +++ b/packages/@aws-cdk/core/lib/construct-compat.ts @@ -412,11 +412,17 @@ export class ConstructNode { return this._actualNode.tryGetContext(key); } + /** + * DEPRECATED + * @deprecated use `metadataEntry` + */ + public get metadata() { return this._actualNode.metadata as cxapi.MetadataEntry[]; } + /** * An immutable array of metadata objects associated with this construct. * This can be used, for example, to implement support for deprecation notices, source mapping, etc. */ - public get metadata() { return this._actualNode.metadata as cxapi.MetadataEntry[]; } + public get metadataEntry() { return this._actualNode.metadata; } /** * Adds a metadata entry to this construct. diff --git a/packages/@aws-cdk/core/lib/stack.ts b/packages/@aws-cdk/core/lib/stack.ts index 2284ddedc203f..bd6f77c0da2f9 100644 --- a/packages/@aws-cdk/core/lib/stack.ts +++ b/packages/@aws-cdk/core/lib/stack.ts @@ -424,6 +424,17 @@ export class Stack extends CoreConstruct implements ITaggable { return CloudFormationLang.toJSON(obj, space).toString(); } + /** + * DEPRECATED + * @deprecated use `reportMissingContextKey()` + */ + public reportMissingContext(report: cxapi.MissingContext) { + if (!Object.values(cxschema.ContextProvider).includes(report.provider as cxschema.ContextProvider)) { + throw new Error(`Unknown context provider requested in: ${JSON.stringify(report)}`); + } + this.reportMissingContextKey(report as cxschema.MissingContext); + } + /** * Indicate that a context key was expected * @@ -432,11 +443,8 @@ export class Stack extends CoreConstruct implements ITaggable { * * @param report The set of parameters needed to obtain the context */ - public reportMissingContext(report: cxapi.MissingContext) { - if (!Object.values(cxschema.ContextProvider).includes(report.provider as cxschema.ContextProvider)) { - throw new Error(`Unknown context provider requested in: ${JSON.stringify(report)}`); - } - this._missingContext.push(report as cxschema.MissingContext); + public reportMissingContextKey(report: cxschema.MissingContext) { + this._missingContext.push(report); } /** diff --git a/packages/aws-cdk-lib/package.json b/packages/aws-cdk-lib/package.json index bc1a4b73f08c2..5bd2c80c966f9 100644 --- a/packages/aws-cdk-lib/package.json +++ b/packages/aws-cdk-lib/package.json @@ -33,7 +33,8 @@ "cdk-build": { "eslint": { "disable": true - } + }, + "stripDeprecated": true }, "pkglint": { "exclude": [ diff --git a/tools/cdk-build-tools/lib/compile.ts b/tools/cdk-build-tools/lib/compile.ts index fe63d0c8f346e..173fea93685b5 100644 --- a/tools/cdk-build-tools/lib/compile.ts +++ b/tools/cdk-build-tools/lib/compile.ts @@ -7,7 +7,7 @@ import { Timers } from './timer'; */ export async function compileCurrentPackage(options: CDKBuildOptions, timers: Timers, compilers: CompilerOverrides = {}): Promise { const env = options.env; - await shell(packageCompiler(compilers), { timers, env }); + await shell(packageCompiler(compilers, options), { timers, env }); // Find files in bin/ that look like they should be executable, and make them so. const scripts = currentPackageJson().bin || {}; diff --git a/tools/cdk-build-tools/lib/package-info.ts b/tools/cdk-build-tools/lib/package-info.ts index 9e12b86b80580..b9d5b7614eb30 100644 --- a/tools/cdk-build-tools/lib/package-info.ts +++ b/tools/cdk-build-tools/lib/package-info.ts @@ -81,9 +81,13 @@ export interface CompilerOverrides { /** * Return the compiler for this package (either tsc or jsii) */ -export function packageCompiler(compilers: CompilerOverrides): string[] { +export function packageCompiler(compilers: CompilerOverrides, options?: CDKBuildOptions): string[] { if (isJsii()) { - return [compilers.jsii || require.resolve('jsii/bin/jsii'), '--silence-warnings=reserved-word']; + const args = ['--silence-warnings=reserved-word']; + if (options?.stripDeprecated) { + args.push('--strip-deprecated'); + } + return [compilers.jsii || require.resolve('jsii/bin/jsii'), ...args]; } else { return [compilers.tsc || require.resolve('typescript/bin/tsc'), '--build']; } @@ -148,6 +152,12 @@ export interface CDKBuildOptions { * Environment variables to be passed to 'cdk-build' and all of its child processes. */ env?: NodeJS.ProcessEnv; + + /** + * Whether deprecated symbols should be stripped from the jsii assembly and typescript declaration files. + * @see https://aws.github.io/jsii/user-guides/lib-author/toolchain/jsii/#-strip-deprecated + */ + stripDeprecated?: boolean; } /**