From e2c48dacbf5f8c09c7c143b043ba2622987e42d9 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Mon, 22 Aug 2022 18:51:48 +0000 Subject: [PATCH] fix(ecs): firelens configFileValue is unnecessarily required (backport #20636) (#21710) This is an automatic backport of pull request #20636 done by [Mergify](https://mergify.com). ---
Mergify commands and options
More conditions and actions can be found in the [documentation](https://docs.mergify.com/). You can also trigger Mergify actions by commenting on this pull request: - `@Mergifyio refresh` will re-evaluate the rules - `@Mergifyio rebase` will rebase this PR on its base branch - `@Mergifyio update` will merge the base branch into this PR - `@Mergifyio backport ` will backport this PR on `` branch Additionally, on Mergify [dashboard](https://dashboard.mergify.com/) you can: - look at your merge queues - generate the Mergify configuration with the config editor. Finally, you can contact us on https://mergify.com
--- allowed-breaking-changes.txt | 3 + .../aws-ecs/lib/firelens-log-router.ts | 65 +++++++++++++------ .../test/ec2/integ.firelens-s3-config.ts | 1 + .../aws-ecs/test/firelens-log-driver.test.ts | 33 ++++++++++ 4 files changed, 83 insertions(+), 19 deletions(-) diff --git a/allowed-breaking-changes.txt b/allowed-breaking-changes.txt index 371f4711ee727..f6900c074f41c 100644 --- a/allowed-breaking-changes.txt +++ b/allowed-breaking-changes.txt @@ -139,3 +139,6 @@ removed:@aws-cdk/aws-lambda-event-sources.SelfManagedKafkaEventSourceProps.onFai # removed kubernetes version from EKS removed:@aws-cdk/aws-eks.KubernetesVersion.V1_22 + +#configFileValue is unnecessarily required; not a breaking change +weakened:@aws-cdk/aws-ecs.FirelensOptions \ No newline at end of file diff --git a/packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts b/packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts index 0d77fca0eb0e2..76f6452095bb1 100644 --- a/packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts +++ b/packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts @@ -53,15 +53,22 @@ export interface FirelensOptions { readonly enableECSLogMetadata?: boolean; /** - * Custom configuration file, s3 or file + * Custom configuration file, s3 or file. + * Both configFileType and configFileValue must be used together + * to define a custom configuration source. + * * @default - determined by checking configFileValue with S3 ARN. */ readonly configFileType?: FirelensConfigFileType; /** * Custom configuration file, S3 ARN or a file path + * Both configFileType and configFileValue must be used together + * to define a custom configuration source. + * + * @default - no config file value */ - readonly configFileValue: string; + readonly configFileValue?: string; } /** @@ -109,6 +116,16 @@ export interface FirelensLogRouterDefinitionOptions extends ContainerDefinitionO function renderFirelensConfig(firelensConfig: FirelensConfig): CfnTaskDefinition.FirelensConfigurationProperty { if (!firelensConfig.options) { return { type: firelensConfig.type }; + } else if (firelensConfig.options.configFileValue === undefined) { + // config file options work as a pair together to define a custom config source + // a custom config source is optional, + // and thus the `config-file-x` keys should be set together or not at all + return { + type: firelensConfig.type, + options: { + 'enable-ecs-log-metadata': firelensConfig.options.enableECSLogMetadata ? 'true' : 'false', + }, + }; } else { // firelensConfig.options.configFileType has been filled with s3 or file type in constructor. return { @@ -201,33 +218,43 @@ export class FirelensLogRouter extends ContainerDefinition { super(scope, id, props); const options = props.firelensConfig.options; if (options) { + if ((options.configFileValue && options.configFileType === undefined) || (options.configFileValue === undefined && options.configFileType)) { + throw new Error('configFileValue and configFileType must be set together to define a custom config source'); + } + + const hasConfig = (options.configFileValue !== undefined); const enableECSLogMetadata = options.enableECSLogMetadata || options.enableECSLogMetadata === undefined; const configFileType = (options.configFileType === undefined || options.configFileType === FirelensConfigFileType.S3) && - (cdk.Token.isUnresolved(options.configFileValue) || /arn:aws[a-zA-Z-]*:s3:::.+/.test(options.configFileValue)) + (cdk.Token.isUnresolved(options.configFileValue) || /arn:aws[a-zA-Z-]*:s3:::.+/.test(options.configFileValue || '')) ? FirelensConfigFileType.S3 : FirelensConfigFileType.FILE; + this.firelensConfig = { type: props.firelensConfig.type, options: { enableECSLogMetadata, - configFileType, - configFileValue: options.configFileValue, + ...(hasConfig ? { + configFileType, + configFileValue: options.configFileValue, + } : {}), }, }; - // grant s3 access permissions - if (configFileType === FirelensConfigFileType.S3) { - props.taskDefinition.addToExecutionRolePolicy(new iam.PolicyStatement({ - actions: [ - 's3:GetObject', - ], - resources: [options.configFileValue], - })); - props.taskDefinition.addToExecutionRolePolicy(new iam.PolicyStatement({ - actions: [ - 's3:GetBucketLocation', - ], - resources: [options.configFileValue.split('/')[0]], - })); + if (hasConfig) { + // grant s3 access permissions + if (configFileType === FirelensConfigFileType.S3) { + props.taskDefinition.addToExecutionRolePolicy(new iam.PolicyStatement({ + actions: [ + 's3:GetObject', + ], + resources: [(options.configFileValue ?? '')], + })); + props.taskDefinition.addToExecutionRolePolicy(new iam.PolicyStatement({ + actions: [ + 's3:GetBucketLocation', + ], + resources: [(options.configFileValue ?? '').split('/')[0]], + })); + } } } else { this.firelensConfig = props.firelensConfig; diff --git a/packages/@aws-cdk/aws-ecs/test/ec2/integ.firelens-s3-config.ts b/packages/@aws-cdk/aws-ecs/test/ec2/integ.firelens-s3-config.ts index 20085edeb5228..3ac9d46ba6d6e 100644 --- a/packages/@aws-cdk/aws-ecs/test/ec2/integ.firelens-s3-config.ts +++ b/packages/@aws-cdk/aws-ecs/test/ec2/integ.firelens-s3-config.ts @@ -28,6 +28,7 @@ taskDefinition.addFirelensLogRouter('log_router', { options: { enableECSLogMetadata: false, configFileValue: `${asset.bucket.bucketArn}/${asset.s3ObjectKey}`, + configFileType: ecs.FirelensConfigFileType.S3, }, }, logging: new ecs.AwsLogDriver({ streamPrefix: 'firelens' }), diff --git a/packages/@aws-cdk/aws-ecs/test/firelens-log-driver.test.ts b/packages/@aws-cdk/aws-ecs/test/firelens-log-driver.test.ts index 547382ab8d3de..62aa0b8e7bd5b 100644 --- a/packages/@aws-cdk/aws-ecs/test/firelens-log-driver.test.ts +++ b/packages/@aws-cdk/aws-ecs/test/firelens-log-driver.test.ts @@ -288,6 +288,7 @@ describe('firelens log driver', () => { options: { enableECSLogMetadata: false, configFileValue: 'arn:aws:s3:::mybucket/fluent.conf', + configFileType: ecs.FirelensConfigFileType.S3, }, }, logging: new ecs.AwsLogDriver({ streamPrefix: 'firelens' }), @@ -349,5 +350,37 @@ describe('firelens log driver', () => { ], }); }); + + test('firelens config options are fully optional', () => { + // GIVEN + td.addFirelensLogRouter('log_router', { + image: ecs.obtainDefaultFluentBitECRImage(td, undefined, '2.1.0'), + firelensConfig: { + type: ecs.FirelensLogRouterType.FLUENTBIT, + options: { + enableECSLogMetadata: false, + }, + }, + logging: new ecs.AwsLogDriver({ streamPrefix: 'firelens' }), + memoryReservationMiB: 50, + }); + + // THEN + Template.fromStack(stack).hasResourceProperties('AWS::ECS::TaskDefinition', { + ContainerDefinitions: [ + Match.objectLike({ + Essential: true, + MemoryReservation: 50, + Name: 'log_router', + FirelensConfiguration: { + Type: 'fluentbit', + Options: { + 'enable-ecs-log-metadata': 'false', + }, + }, + }), + ], + }); + }); }); });