From 1e7c690f5ec404d7c620dc54692999fee67b3eaf Mon Sep 17 00:00:00 2001 From: Leonardo Gama <51037424+Leo10Gama@users.noreply.github.com> Date: Thu, 8 Aug 2024 16:39:18 -0700 Subject: [PATCH] fix(ec2): prevent deduplication of init command args (#30821) ### Issue # (if applicable) Closes #26221 ### Reason for this change Previously, using `ec2.InitCommand.argvCommand()` would remove some duplicate strings in the input array. This produces an incorrect command in the template, leading to unexpected behaviour. ### Description of changes An additional line was added to the `deepMerge` function that is called in the `InitConfig.bindForType()` method, which checks the key of the input array, preventing it from becoming a set (removing duplicates) if it is a list of commands. ### Description of how you validated changes A unit test was added to generate an `AWS::CloudFormation::Init` resource identical to the one reproduced in the issue. The test was run and failed before the changes were made, and following the changes in `cfn-init.ts`, the test passed. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- ...c2-multiple-instances-in-stack.assets.json | 4 +- ...-multiple-instances-in-stack.template.json | 25 +++++++- .../manifest.json | 7 ++- .../tree.json | 2 +- .../test/integ.instance-init-multiple.ts | 6 ++ .../manifest.json | 14 ++--- packages/aws-cdk-lib/aws-ec2/lib/cfn-init.ts | 15 +++-- .../aws-cdk-lib/aws-ec2/test/cfn-init.test.ts | 62 +++++++++++++++++++ 8 files changed, 115 insertions(+), 20 deletions(-) diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.js.snapshot/integ-ec2-multiple-instances-in-stack.assets.json b/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.js.snapshot/integ-ec2-multiple-instances-in-stack.assets.json index 37139758282cd..efd673ae6169d 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.js.snapshot/integ-ec2-multiple-instances-in-stack.assets.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.js.snapshot/integ-ec2-multiple-instances-in-stack.assets.json @@ -40,7 +40,7 @@ } } }, - "55adc2a4d264e77d2c794df6cb13dd26ee0a9986d5b00a9bfe3cd48b6e0b2dfe": { + "db1c5d9623e5e22db4e511f52daf89f93fb1cbb8f325df40d03d5c50413e63a3": { "source": { "path": "integ-ec2-multiple-instances-in-stack.template.json", "packaging": "file" @@ -48,7 +48,7 @@ "destinations": { "current_account-current_region": { "bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}", - "objectKey": "55adc2a4d264e77d2c794df6cb13dd26ee0a9986d5b00a9bfe3cd48b6e0b2dfe.json", + "objectKey": "db1c5d9623e5e22db4e511f52daf89f93fb1cbb8f325df40d03d5c50413e63a3.json", "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}" } } diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.js.snapshot/integ-ec2-multiple-instances-in-stack.template.json b/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.js.snapshot/integ-ec2-multiple-instances-in-stack.template.json index 787b41606da14..e09958092be8c 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.js.snapshot/integ-ec2-multiple-instances-in-stack.template.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.js.snapshot/integ-ec2-multiple-instances-in-stack.template.json @@ -906,7 +906,7 @@ "Fn::Join": [ "", [ - "#!/bin/bash\n# fingerprint: 8787022e9944cbeb\n(\n set +e\n /opt/aws/bin/cfn-init -v --region ", + "#!/bin/bash\n# fingerprint: 370d9b2dcf8bf44b\n(\n set +e\n /opt/aws/bin/cfn-init -v --region ", { "Ref": "AWS::Region" }, @@ -955,6 +955,29 @@ "owner": "root", "group": "root" } + }, + "commands": { + "000": { + "command": [ + "useradd", + "-u", + "1001", + "-g", + "1001", + "eguser" + ] + }, + "001": { + "command": [ + "useradd", + "-a", + "-u", + "1001", + "-g", + "1001", + "eguser" + ] + } } } }, diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.js.snapshot/manifest.json b/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.js.snapshot/manifest.json index 9d805bac6514b..bd244eaf3c6a4 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.js.snapshot/manifest.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.js.snapshot/manifest.json @@ -18,7 +18,7 @@ "validateOnSynth": false, "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}", "cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}", - "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/55adc2a4d264e77d2c794df6cb13dd26ee0a9986d5b00a9bfe3cd48b6e0b2dfe.json", + "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/db1c5d9623e5e22db4e511f52daf89f93fb1cbb8f325df40d03d5c50413e63a3.json", "requiresBootstrapStackVersion": 6, "bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version", "additionalDependencies": [ @@ -259,7 +259,10 @@ "/integ-ec2-multiple-instances-in-stack/SecondInstance/Resource": [ { "type": "aws:cdk:logicalId", - "data": "SecondInstance4834A636" + "data": "SecondInstance4834A636", + "trace": [ + "!!DESTRUCTIVE_CHANGES: MAY_REPLACE" + ] } ], "/integ-ec2-multiple-instances-in-stack/BootstrapVersion": [ diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.js.snapshot/tree.json b/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.js.snapshot/tree.json index e556e0419844d..11c69d2c333ad 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.js.snapshot/tree.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.js.snapshot/tree.json @@ -1265,7 +1265,7 @@ "Fn::Join": [ "", [ - "#!/bin/bash\n# fingerprint: 8787022e9944cbeb\n(\n set +e\n /opt/aws/bin/cfn-init -v --region ", + "#!/bin/bash\n# fingerprint: 370d9b2dcf8bf44b\n(\n set +e\n /opt/aws/bin/cfn-init -v --region ", { "Ref": "AWS::Region" }, diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.ts b/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.ts index 87a8ab87c74e6..de01a25b4715f 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.ts +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init-multiple.ts @@ -55,6 +55,12 @@ class TestStack extends cdk.Stack { '/target/path/config.json', path.join(tmpDir, 'testConfigFile2'), ), + ec2.InitCommand.argvCommand([ + 'useradd', '-u', '1001', '-g', '1001', 'eguser', + ]), + ec2.InitCommand.argvCommand([ + 'useradd', '-a', '-u', '1001', '-g', '1001', 'eguser', + ]), ]), }, }), diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init.js.snapshot/manifest.json b/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init.js.snapshot/manifest.json index b0e1d19683224..e49e7eba966a3 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init.js.snapshot/manifest.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-ec2/test/integ.instance-init.js.snapshot/manifest.json @@ -199,7 +199,10 @@ "/integ-init/Instance2/Resource": [ { "type": "aws:cdk:logicalId", - "data": "Instance255F3526574cbd507dfce8b71" + "data": "Instance255F3526574cbd507dfce8b71", + "trace": [ + "!!DESTRUCTIVE_CHANGES: WILL_DESTROY" + ] } ], "/integ-init/SsmParameterValue:--aws--service--ami-amazon-linux-latest--amzn-ami-hvm-x86_64-gp2:C96584B6-F00A-464E-AD19-53AFF4B05118.Parameter": [ @@ -219,15 +222,6 @@ "type": "aws:cdk:logicalId", "data": "CheckBootstrapVersion" } - ], - "Instance255F35265a0c5f577d761edb0": [ - { - "type": "aws:cdk:logicalId", - "data": "Instance255F35265a0c5f577d761edb0", - "trace": [ - "!!DESTRUCTIVE_CHANGES: WILL_DESTROY" - ] - } ] }, "displayName": "integ-init" diff --git a/packages/aws-cdk-lib/aws-ec2/lib/cfn-init.ts b/packages/aws-cdk-lib/aws-ec2/lib/cfn-init.ts index bf055b4b27a41..e97992612b233 100644 --- a/packages/aws-cdk-lib/aws-ec2/lib/cfn-init.ts +++ b/packages/aws-cdk-lib/aws-ec2/lib/cfn-init.ts @@ -311,10 +311,17 @@ function deepMerge(target?: Record, src?: Record) { if (target[key] && !Array.isArray(target[key])) { throw new Error(`Trying to merge array [${value}] into a non-array '${target[key]}'`); } - target[key] = Array.from(new Set([ - ...target[key] ?? [], - ...value, - ])); + if (key === 'command') { // don't deduplicate command arguments + target[key] = new Array( + ...target[key] ?? [], + ...value, + ); + } else { + target[key] = Array.from(new Set([ + ...target[key] ?? [], + ...value, + ])); + } continue; } if (typeof value === 'object' && value) { diff --git a/packages/aws-cdk-lib/aws-ec2/test/cfn-init.test.ts b/packages/aws-cdk-lib/aws-ec2/test/cfn-init.test.ts index bf6823a598988..0a7f5f3a7181b 100644 --- a/packages/aws-cdk-lib/aws-ec2/test/cfn-init.test.ts +++ b/packages/aws-cdk-lib/aws-ec2/test/cfn-init.test.ts @@ -136,6 +136,68 @@ test('empty configs are not rendered', () => { }); }); +test('duplicate config arguments not deduplicated', () => { + //GIVEN + const config = new ec2.InitConfig([ + ec2.InitCommand.argvCommand([ + 'useradd', '-u', '1001', '-g', '1001', 'eguser', + ]), + ec2.InitCommand.argvCommand([ + 'useradd', '-a', '-u', '1001', '-g', '1001', 'eguser', + ]), + ]); + + // WHEN + const init = ec2.CloudFormationInit.fromConfigSets({ + configSets: { default: ['config'] }, + configs: { config }, + }); + init.attach(resource, linuxOptions()); + + // THEN + expectMetadataLike({ + 'AWS::CloudFormation::Init': { + configSets: { + default: ['config'], + }, + config: { + commands: { + '000': { + command: ['useradd', '-u', '1001', '-g', '1001', 'eguser'], + }, + '001': { + command: ['useradd', '-a', '-u', '1001', '-g', '1001', 'eguser'], + }, + }, + }, + }, + }); +}); + +test('deepMerge properly deduplicates non-command arguments', () => { + // WHEN + const config = new ec2.InitConfig([ + ec2.InitSource.fromUrl('/tmp/blinky', 'https://amazon.com/blinky.zip'), + ec2.InitSource.fromUrl('/tmp/blinky', 'https://amazon.com/blinky.zip'), + ec2.InitSource.fromUrl('/tmp/pinky', 'https://amazon.com/pinky.zip'), + ec2.InitSource.fromUrl('/tmp/pinky', 'https://amazon.com/pinky.zip'), + ec2.InitSource.fromUrl('/tmp/inky', 'https://amazon.com/inky.zip'), + ec2.InitSource.fromUrl('/tmp/clyde', 'https://amazon.com/blinky.zip'), + ec2.InitSource.fromUrl('/tmp/clyde', 'https://amazon.com/blinky.zip'), + ec2.InitSource.fromUrl('/tmp/clyde', 'https://amazon.com/blinky.zip'), + ]); + + // THEN + expect(config._bind(stack, linuxOptions()).config).toEqual(expect.objectContaining({ + sources: { + '/tmp/blinky': 'https://amazon.com/blinky.zip', + '/tmp/pinky': 'https://amazon.com/pinky.zip', + '/tmp/inky': 'https://amazon.com/inky.zip', + '/tmp/clyde': 'https://amazon.com/blinky.zip', + }, + })); +}); + describe('userdata', () => { let simpleInit: ec2.CloudFormationInit; beforeEach(() => {