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.

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

fix(cloudformation-include): drops unknown policy attributes #32321

Merged
merged 10 commits into from
Dec 2, 2024
1 change: 0 additions & 1 deletion packages/aws-cdk-lib/assertions/lib/matcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,6 @@ export class MatchResult {
*/
public toHumanStrings(): string[] {
const failures = new Array<MatchFailure>();
debugger;
recurse(this, []);

return failures.map(r => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"Resources": {
"ASG": {
"Type": "AWS::AutoScaling::AutoScalingGroup",
"Properties": {
"MinSize": "1",
"MaxSize": "5"
},
"CreationPolicy": {
"NonExistentResourceAttribute": "Bucket1"
},
"UpdatePolicy": {
"NonExistentResourceAttribute": "Bucket1"
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -707,6 +707,13 @@ describe('CDK Include', () => {
);
});

test('preserves unknown policy attributes', () => {
const cfnTemplate = includeTestTemplate(stack, 'non-existent-policy-attribute.json');
Template.fromStack(stack).templateMatches(
loadTestFileToJsObject('non-existent-policy-attribute.json'),
);
});

test("correctly handles referencing the ingested template's resources across Stacks", () => {
// for cross-stack sharing to work, we need an App
const app = new core.App();
Expand Down
182 changes: 119 additions & 63 deletions packages/aws-cdk-lib/core/lib/helpers-internal/cfn-parse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -356,14 +356,23 @@ export class CfnParser {
const cfnOptions = resource.cfnOptions;
this.stack = Stack.of(resource);

cfnOptions.creationPolicy = this.parseCreationPolicy(resourceAttributes.CreationPolicy, logicalId);
cfnOptions.updatePolicy = this.parseUpdatePolicy(resourceAttributes.UpdatePolicy, logicalId);
const creationPolicy = this.parseCreationPolicy(resourceAttributes.CreationPolicy, logicalId);
const updatePolicy = this.parseUpdatePolicy(resourceAttributes.UpdatePolicy, logicalId);
cfnOptions.creationPolicy = creationPolicy.value;
cfnOptions.updatePolicy = updatePolicy.value;
cfnOptions.deletionPolicy = this.parseDeletionPolicy(resourceAttributes.DeletionPolicy);
cfnOptions.updateReplacePolicy = this.parseDeletionPolicy(resourceAttributes.UpdateReplacePolicy);
cfnOptions.version = this.parseValue(resourceAttributes.Version);
cfnOptions.description = this.parseValue(resourceAttributes.Description);
cfnOptions.metadata = this.parseValue(resourceAttributes.Metadata);

for (const [key, value] of Object.entries(creationPolicy.extraProperties)) {
resource.addOverride(`CreationPolicy.${key}`, value);
}
for (const [key, value] of Object.entries(updatePolicy.extraProperties)) {
resource.addOverride(`UpdatePolicy.${key}`, value);
}

// handle Condition
if (resourceAttributes.Condition) {
const condition = this.finder.findCondition(resourceAttributes.Condition);
Expand All @@ -386,98 +395,93 @@ export class CfnParser {
}
}

private parseCreationPolicy(policy: any, logicalId: string): CfnCreationPolicy | undefined {
if (typeof policy !== 'object') { return undefined; }
private parseCreationPolicy(policy: any, logicalId: string): FromCloudFormationResult<CfnCreationPolicy | undefined> {
if (typeof policy !== 'object') { return new FromCloudFormationResult(undefined); }
this.throwIfIsIntrinsic(policy, logicalId);
const self = this;

// change simple JS values to their CDK equivalents
policy = this.parseValue(policy);

return undefinedIfAllValuesAreEmpty({
autoScalingCreationPolicy: parseAutoScalingCreationPolicy(policy.AutoScalingCreationPolicy),
resourceSignal: parseResourceSignal(policy.ResourceSignal),
});
const creaPol = new ObjectParser<CfnCreationPolicy>(this.parseValue(policy));
creaPol.parse('autoScalingCreationPolicy', 'AutoScalingCreationPolicy', parseAutoScalingCreationPolicy);
creaPol.parse('resourceSignal', 'ResourceSignal', parseResourceSignal);
return creaPol.toResult();

function parseAutoScalingCreationPolicy(p: any): CfnResourceAutoScalingCreationPolicy | undefined {
function parseAutoScalingCreationPolicy(p: any): FromCloudFormationResult<CfnResourceAutoScalingCreationPolicy | undefined> {
self.throwIfIsIntrinsic(p, logicalId);
if (typeof p !== 'object') { return undefined; }
if (typeof p !== 'object') { return new FromCloudFormationResult(undefined); }

return undefinedIfAllValuesAreEmpty({
minSuccessfulInstancesPercent: FromCloudFormation.getNumber(p.MinSuccessfulInstancesPercent).value,
});
const autoPol = new ObjectParser<CfnResourceAutoScalingCreationPolicy>(p);
autoPol.parse('minSuccessfulInstancesPercent', 'MinSuccessfulInstancesPercent', FromCloudFormation.getNumber);
return autoPol.toResult();
}

function parseResourceSignal(p: any): CfnResourceSignal | undefined {
if (typeof p !== 'object') { return undefined; }
function parseResourceSignal(p: any): FromCloudFormationResult<CfnResourceSignal | undefined> {
if (typeof p !== 'object') { return new FromCloudFormationResult(undefined); }
self.throwIfIsIntrinsic(p, logicalId);

return undefinedIfAllValuesAreEmpty({
count: FromCloudFormation.getNumber(p.Count).value,
timeout: FromCloudFormation.getString(p.Timeout).value,
});
const sig = new ObjectParser<CfnResourceSignal>(p);
sig.parse('count', 'Count', FromCloudFormation.getNumber);
sig.parse('timeout', 'Tiemout', FromCloudFormation.getString);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sig.parse('timeout', 'Tiemout', FromCloudFormation.getString);
sig.parse('timeout', 'Timeout', FromCloudFormation.getString);

Perhaps we could have special method, built on top of parse, that converts the cases automatically, to reduce the risk of typos like this one.

parseConvertingCase('timeout', FromCloudFormation.getString);

return sig.toResult();
}
}

private parseUpdatePolicy(policy: any, logicalId: string): CfnUpdatePolicy | undefined {
if (typeof policy !== 'object') { return undefined; }
private parseUpdatePolicy(policy: any, logicalId: string): FromCloudFormationResult<CfnUpdatePolicy | undefined> {
if (typeof policy !== 'object') { return new FromCloudFormationResult(undefined); }
this.throwIfIsIntrinsic(policy, logicalId);
const self = this;

// change simple JS values to their CDK equivalents
policy = this.parseValue(policy);

return undefinedIfAllValuesAreEmpty({
autoScalingReplacingUpdate: parseAutoScalingReplacingUpdate(policy.AutoScalingReplacingUpdate),
autoScalingRollingUpdate: parseAutoScalingRollingUpdate(policy.AutoScalingRollingUpdate),
autoScalingScheduledAction: parseAutoScalingScheduledAction(policy.AutoScalingScheduledAction),
codeDeployLambdaAliasUpdate: parseCodeDeployLambdaAliasUpdate(policy.CodeDeployLambdaAliasUpdate),
enableVersionUpgrade: FromCloudFormation.getBoolean(policy.EnableVersionUpgrade).value,
useOnlineResharding: FromCloudFormation.getBoolean(policy.UseOnlineResharding).value,
});

function parseAutoScalingReplacingUpdate(p: any): CfnAutoScalingReplacingUpdate | undefined {
if (typeof p !== 'object') { return undefined; }
const uppol = new ObjectParser<CfnUpdatePolicy>(this.parseValue(policy));
uppol.parse('autoScalingReplacingUpdate', 'AutoScalingReplacingUpdate', parseAutoScalingReplacingUpdate);
uppol.parse('autoScalingRollingUpdate', 'AutoScalingRollingUpdate', parseAutoScalingRollingUpdate);
uppol.parse('autoScalingScheduledAction', 'AutoScalingScheduledAction', parseAutoScalingScheduledAction);
uppol.parse('codeDeployLambdaAliasUpdate', 'CodeDeployLambdaAliasUpdate', parseCodeDeployLambdaAliasUpdate);
uppol.parse('enableVersionUpgrade', 'EnableVersionUpgrade', (x) => FromCloudFormation.getBoolean(x) as any);
uppol.parse('useOnlineResharding', 'UseOnlineResharding', (x) => FromCloudFormation.getBoolean(x) as any);
return uppol.toResult();

function parseAutoScalingReplacingUpdate(p: any): FromCloudFormationResult<CfnAutoScalingReplacingUpdate | undefined> {
if (typeof p !== 'object') { return new FromCloudFormationResult(undefined); }
self.throwIfIsIntrinsic(p, logicalId);

return undefinedIfAllValuesAreEmpty({
willReplace: p.WillReplace,
});
const repUp = new ObjectParser<CfnAutoScalingReplacingUpdate>(p);
repUp.parse('willReplace', 'WillReplace', (x) => x);
return repUp.toResult();
}

function parseAutoScalingRollingUpdate(p: any): CfnAutoScalingRollingUpdate | undefined {
if (typeof p !== 'object') { return undefined; }
function parseAutoScalingRollingUpdate(p: any): FromCloudFormationResult<CfnAutoScalingRollingUpdate | undefined> {
if (typeof p !== 'object') { return new FromCloudFormationResult(undefined); }
self.throwIfIsIntrinsic(p, logicalId);

return undefinedIfAllValuesAreEmpty({
maxBatchSize: FromCloudFormation.getNumber(p.MaxBatchSize).value,
minInstancesInService: FromCloudFormation.getNumber(p.MinInstancesInService).value,
minSuccessfulInstancesPercent: FromCloudFormation.getNumber(p.MinSuccessfulInstancesPercent).value,
pauseTime: FromCloudFormation.getString(p.PauseTime).value,
suspendProcesses: FromCloudFormation.getStringArray(p.SuspendProcesses).value,
waitOnResourceSignals: FromCloudFormation.getBoolean(p.WaitOnResourceSignals).value,
});
const rollUp = new ObjectParser<CfnAutoScalingRollingUpdate>(p);
rollUp.parse('maxBatchSize', 'MaxBatchSize', FromCloudFormation.getNumber);
rollUp.parse('minInstancesInService', 'MinInstancesInService', FromCloudFormation.getNumber);
rollUp.parse('minSuccessfulInstancesPercent', 'MinSuccessfulInstancesPercent', FromCloudFormation.getNumber);
rollUp.parse('pauseTime', 'PauseTime', FromCloudFormation.getString);
rollUp.parse('suspendProcesses', 'SuspendProcesses', FromCloudFormation.getStringArray);
rollUp.parse('waitOnResourceSignals', 'WaitOnResourceSignals', FromCloudFormation.getBoolean);
return rollUp.toResult();
}

function parseCodeDeployLambdaAliasUpdate(p: any): CfnCodeDeployLambdaAliasUpdate | undefined {
function parseCodeDeployLambdaAliasUpdate(p: any): FromCloudFormationResult<CfnCodeDeployLambdaAliasUpdate | undefined> {
if (typeof p !== 'object') { return new FromCloudFormationResult(undefined); }
self.throwIfIsIntrinsic(p, logicalId);
if (typeof p !== 'object') { return undefined; }

return {
beforeAllowTrafficHook: FromCloudFormation.getString(p.BeforeAllowTrafficHook).value,
afterAllowTrafficHook: FromCloudFormation.getString(p.AfterAllowTrafficHook).value,
applicationName: FromCloudFormation.getString(p.ApplicationName).value,
deploymentGroupName: FromCloudFormation.getString(p.DeploymentGroupName).value,
};
const cdUp = new ObjectParser<CfnCodeDeployLambdaAliasUpdate>(p);
cdUp.parse('beforeAllowTrafficHook', 'BeforeAllowTrafficHook', FromCloudFormation.getString);
cdUp.parse('afterAllowTrafficHook', 'AfterAllowTrafficHook', FromCloudFormation.getString);
cdUp.parse('applicationName', 'ApplicationName', FromCloudFormation.getString);
cdUp.parse('deploymentGroupName', 'DeploymentGroupName', FromCloudFormation.getString);
return cdUp.toResult();
}

function parseAutoScalingScheduledAction(p: any): CfnAutoScalingScheduledAction | undefined {
function parseAutoScalingScheduledAction(p: any): FromCloudFormationResult<CfnAutoScalingScheduledAction | undefined> {
if (typeof p !== 'object') { return new FromCloudFormationResult(undefined); }
self.throwIfIsIntrinsic(p, logicalId);
if (typeof p !== 'object') { return undefined; }

return undefinedIfAllValuesAreEmpty({
ignoreUnmodifiedGroupSizeProperties: FromCloudFormation.getBoolean(p.IgnoreUnmodifiedGroupSizeProperties).value,
});
const schedUp = new ObjectParser<CfnAutoScalingScheduledAction>(p);
schedUp.parse('ignoreUnmodifiedGroupSizeProperties', 'IgnoreUnmodifiedGroupSizeProperties', FromCloudFormation.getBoolean);
return schedUp.toResult();
}
}

Expand Down Expand Up @@ -853,3 +857,55 @@ export class CfnParser {
return this.options.parameters || {};
}
}

class ObjectParser<T extends object> {
private readonly parsed: Record<string, unknown> = {};
private readonly unparsed: Record<string, unknown> = {};

constructor(input: Record<string, unknown>) {
this.unparsed = { ...input };
}

// eslint-disable-next-line max-len
public parse<K extends keyof T>(targetKey: K, key: string, parser: (x: any) => T[K] | FromCloudFormationResult<T[K] | IResolvable>) {
Copy link
Contributor

@mrgrain mrgrain Nov 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public parse<K extends keyof T>(targetKey: K, key: string, parser: (x: any) => T[K] | FromCloudFormationResult<T[K] | IResolvable>) {
public parse<K extends Extract<keyof T, string>>(targetKey: K, key: Capitalize<K>, parser: (x: any) => T[K] | FromCloudFormationResult<T[K] | IResolvable>) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I agree with @otaviomacedo A simple firstCharUpper would be even better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't because we can't assume this is true in general. But I guess this is on a small enough domain that it's always true here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know about Capitalize btw. That's neat.

if (!(key in this.unparsed)) {
return;
}

const value = parser(this.unparsed[key]);
delete this.unparsed[key];

if (value instanceof FromCloudFormationResult) {
for (const [k, v] of Object.entries(value.extraProperties ?? {})) {
this.unparsed[`${key}.${k}`] = v;
}
this.parsed[targetKey as any] = value.value;
} else {
this.parsed[targetKey as any] = value;
}
}

public toResult(): FromCloudFormationResult<T | undefined> {
const ret = new FromCloudFormationResult(undefinedIfAllValuesAreEmpty(this.parsed as any));
for (const [k, v] of Object.entries(this.unparsedKeys)) {
ret.extraProperties[k] = v;
}
return ret;
}

private get unparsedKeys(): Record<string, unknown> {
const unparsed = { ...this.unparsed };

for (const [k, v] of Object.entries(this.unparsed)) {
if (v instanceof FromCloudFormationResult) {
for (const [k2, v2] of Object.entries(v.extraProperties ?? {})) {
unparsed[`${k}.${k2}`] = v2;
}
} else {
unparsed[k] = v;
}
}

return unparsed;
}
}
2 changes: 1 addition & 1 deletion packages/aws-cdk-lib/core/lib/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,6 @@ export function findLastCommonElement<T>(path1: T[], path2: T[]): T | undefined
return path1[i - 1];
}

export function undefinedIfAllValuesAreEmpty(object: object): object | undefined {
export function undefinedIfAllValuesAreEmpty<A extends object>(object: A): A | undefined {
return Object.values(object).some(v => v !== undefined) ? object : undefined;
}
Loading