From ebac8bc08885d6862f75b1133752b639dcf54b1c Mon Sep 17 00:00:00 2001 From: Adam Ruka Date: Tue, 8 Jun 2021 00:22:22 -0700 Subject: [PATCH 1/2] fix(core): property overrides fail for references (#15018) If a property override added an `IResolvable` as the value, it would not be resolved, but instead treated as a CloudFormation object when rendering the resource's properties, which would lead to some weird behavior (the properties of the `IResolvable` would be merged into the template). This was noticed by a customer using CFN-Include. As a solution, resolve the overrides before merging them with the properties of the resource. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../properties-not-in-cfn-spec.json | 5 +++ packages/@aws-cdk/core/lib/cfn-resource.ts | 11 +++++- packages/@aws-cdk/core/lib/private/resolve.ts | 17 +++++++-- packages/@aws-cdk/core/lib/token.ts | 8 +++++ packages/@aws-cdk/core/test/resource.test.ts | 35 +++++++++++++++++-- 5 files changed, 71 insertions(+), 5 deletions(-) diff --git a/packages/@aws-cdk/cloudformation-include/test/test-templates/properties-not-in-cfn-spec.json b/packages/@aws-cdk/cloudformation-include/test/test-templates/properties-not-in-cfn-spec.json index eec4fc6f608c7..383b85eb5b1a0 100644 --- a/packages/@aws-cdk/cloudformation-include/test/test-templates/properties-not-in-cfn-spec.json +++ b/packages/@aws-cdk/cloudformation-include/test/test-templates/properties-not-in-cfn-spec.json @@ -42,6 +42,11 @@ "Type": "Api", "PropertyNotInCfnSchema": "unmodeled property in map" } + }, + "ParentPropertyNotInCfnSchema": { + "ChildPropertyNotInCfnSchema": { + "Ref": "Bucket" + } } } } diff --git a/packages/@aws-cdk/core/lib/cfn-resource.ts b/packages/@aws-cdk/core/lib/cfn-resource.ts index 9f9ee669374ed..d6be50b9149fb 100644 --- a/packages/@aws-cdk/core/lib/cfn-resource.ts +++ b/packages/@aws-cdk/core/lib/cfn-resource.ts @@ -7,9 +7,11 @@ import { CfnCreationPolicy, CfnDeletionPolicy, CfnUpdatePolicy } from './cfn-res import { Construct, IConstruct, Node } from 'constructs'; import { addDependency } from './deps'; import { CfnReference } from './private/cfn-reference'; +import { CLOUDFORMATION_TOKEN_RESOLVER } from './private/cloudformation-lang'; import { Reference } from './reference'; import { RemovalPolicy, RemovalPolicyOptions } from './removal-policy'; import { TagManager } from './tag-manager'; +import { Tokenization } from './token'; import { capitalizePropertyNames, ignoreEmpty, PostResolveToken } from './util'; export interface CfnResourceProps { @@ -326,7 +328,14 @@ export class CfnResource extends CfnRefElement { const hasDefined = Object.values(renderedProps).find(v => v !== undefined); resourceDef.Properties = hasDefined !== undefined ? renderedProps : undefined; } - return deepMerge(resourceDef, this.rawOverrides); + const resolvedRawOverrides = Tokenization.resolve(this.rawOverrides, { + scope: this, + resolver: CLOUDFORMATION_TOKEN_RESOLVER, + // we need to preserve the empty elements here, + // as that's how removing overrides are represented as + removeEmpty: false, + }); + return deepMerge(resourceDef, resolvedRawOverrides); }), }, }; diff --git a/packages/@aws-cdk/core/lib/private/resolve.ts b/packages/@aws-cdk/core/lib/private/resolve.ts index 5f9620ecb759c..f560a852ffd4d 100644 --- a/packages/@aws-cdk/core/lib/private/resolve.ts +++ b/packages/@aws-cdk/core/lib/private/resolve.ts @@ -84,6 +84,13 @@ export interface IResolveOptions { * @default false */ allowIntrinsicKeys?: boolean; + + /** + * Whether to remove undefined elements from arrays and objects when resolving. + * + * @default true + */ + removeEmpty?: boolean; } /** @@ -120,6 +127,9 @@ export function resolve(obj: any, options: IResolveOptions): any { throw new Error('Unable to resolve object tree with circular reference. Path: ' + pathName); } + // whether to leave the empty elements when resolving - false by default + const leaveEmpty = options.removeEmpty === false; + // // undefined // @@ -188,7 +198,7 @@ export function resolve(obj: any, options: IResolveOptions): any { const arr = obj .map((x, i) => makeContext(`${i}`)[0].resolve(x)) - .filter(x => typeof(x) !== 'undefined'); + .filter(x => leaveEmpty || typeof(x) !== 'undefined'); return arr; } @@ -221,6 +231,9 @@ export function resolve(obj: any, options: IResolveOptions): any { // skip undefined if (typeof(value) === 'undefined') { + if (leaveEmpty) { + result[key] = undefined; + } continue; } @@ -326,4 +339,4 @@ function tagResolvedValue(value: any, typeHint: ResolutionTypeHint): any { export function resolvedTypeHint(value: any): ResolutionTypeHint | undefined { if (typeof value !== 'object' || value == null) { return undefined; } return value[RESOLUTION_TYPEHINT_SYM]; -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/core/lib/token.ts b/packages/@aws-cdk/core/lib/token.ts index e17227d0f8ef5..9b87a0792fa2b 100644 --- a/packages/@aws-cdk/core/lib/token.ts +++ b/packages/@aws-cdk/core/lib/token.ts @@ -191,6 +191,7 @@ export class Tokenization { scope: options.scope, resolver: options.resolver, preparing: (options.preparing ?? false), + removeEmpty: options.removeEmpty, }); } @@ -265,6 +266,13 @@ export interface ResolveOptions { * @default false */ readonly preparing?: boolean; + + /** + * Whether to remove undefined elements from arrays and objects when resolving. + * + * @default true + */ + readonly removeEmpty?: boolean; } /** diff --git a/packages/@aws-cdk/core/test/resource.test.ts b/packages/@aws-cdk/core/test/resource.test.ts index aa8fbe74575fc..4fbe41692756d 100644 --- a/packages/@aws-cdk/core/test/resource.test.ts +++ b/packages/@aws-cdk/core/test/resource.test.ts @@ -420,6 +420,37 @@ nodeunitShim({ test.done(); }, + 'addPropertyOverride() allows assigning an attribute of a different resource'(test: Test) { + // GIVEN + const stack = new Stack(); + const r1 = new CfnResource(stack, 'MyResource1', { type: 'AWS::Resource::Type' }); + const r2 = new CfnResource(stack, 'MyResource2', { type: 'AWS::Resource::Type' }); + + // WHEN + r2.addPropertyOverride('A', { + B: r1.getAtt('Arn'), + }); + + // THEN + test.deepEqual(toCloudFormation(stack), { + Resources: { + MyResource1: { + Type: 'AWS::Resource::Type', + }, + MyResource2: { + Type: 'AWS::Resource::Type', + Properties: { + A: { + B: { 'Fn::GetAtt': ['MyResource1', 'Arn'] }, + }, + }, + }, + }, + }); + + test.done(); + }, + 'addOverride(p, null) will assign an "null" value'(test: Test) { // GIVEN const stack = new Stack(); @@ -513,7 +544,7 @@ nodeunitShim({ test.done(); }, - 'addDeletionOverride(p) and addPropertyDeletionOverride(pp) are sugar `undefined`'(test: Test) { + 'addDeletionOverride(p) and addPropertyDeletionOverride(pp) are sugar for `undefined`'(test: Test) { // GIVEN const stack = new Stack(); @@ -904,4 +935,4 @@ class CustomizableResource extends CfnResource { /** * Because Resource is abstract */ -class TestResource extends Resource {} \ No newline at end of file +class TestResource extends Resource {} From 7c98fb2fdaf4256dd9f3c484303b92ce3e00db7c Mon Sep 17 00:00:00 2001 From: Otavio Macedo Date: Tue, 8 Jun 2021 15:55:09 +0100 Subject: [PATCH 2/2] chore(pipelines): added troubleshooting sub-section for the "S3 error: Access Denied" case (#14986) Related to #14944 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/@aws-cdk/pipelines/README.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/packages/@aws-cdk/pipelines/README.md b/packages/@aws-cdk/pipelines/README.md index a99dba1630f3c..d59446e8b8b7e 100644 --- a/packages/@aws-cdk/pipelines/README.md +++ b/packages/@aws-cdk/pipelines/README.md @@ -815,6 +815,26 @@ After turning on `privilegedMode: true`, you will need to do a one-time manual c pipeline to get it going again (as with a broken 'synth' the pipeline will not be able to self update to the right state). +### S3 error: Access Denied + +Some constructs, such as EKS clusters, generate nested stacks. When CloudFormation tries +to deploy those stacks, it may fail with this error: + +```console +S3 error: Access Denied For more information check http://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html +``` + +This happens because the pipeline is not self-mutating and, as a consequence, the `FileAssetX` +build projects get out-of-sync with the generated templates. To fix this, make sure the +`selfMutating` property is set to `true`: + +```typescript +const pipeline = new CdkPipeline(this, 'MyPipeline', { + selfMutating: true, + ... +}); +``` + ## Current Limitations Limitations that we are aware of and will address: