From 5176be908a24621d108303946324b522940b8d58 Mon Sep 17 00:00:00 2001 From: david-doyle-as24 <78368860+david-doyle-as24@users.noreply.github.com> Date: Thu, 7 Oct 2021 20:41:54 +0200 Subject: [PATCH 1/2] fixing bug --- .../aws-ecs/lib/firelens-log-router.ts | 31 ++++++++++++------- .../aws-ecs/test/container-definition.test.ts | 2 +- .../test/ec2/ec2-task-definition.test.ts | 2 +- .../integ.firelens-cloudwatch.expected.json | 5 ++- .../aws-ecs/test/firelens-log-driver.test.ts | 14 +++++++-- 5 files changed, 37 insertions(+), 17 deletions(-) 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..b73267bcd883e 100644 --- a/packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts +++ b/packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts @@ -60,8 +60,9 @@ export interface FirelensOptions { /** * Custom configuration file, S3 ARN or a file path + * @default - No default file value */ - readonly configFileValue: string; + readonly configFileValue?: string; } /** @@ -78,7 +79,7 @@ export interface FirelensConfig { /** * Firelens options - * @default - no additional options + * @default - EnableECSLogMetadata option is set to true */ readonly options?: FirelensOptions; } @@ -116,7 +117,7 @@ function renderFirelensConfig(firelensConfig: FirelensConfig): CfnTaskDefinition options: { 'enable-ecs-log-metadata': firelensConfig.options.enableECSLogMetadata ? 'true' : 'false', 'config-file-type': firelensConfig.options.configFileType!, - 'config-file-value': firelensConfig.options.configFileValue, + 'config-file-value': firelensConfig.options.configFileValue!, }, }; } @@ -200,37 +201,43 @@ export class FirelensLogRouter extends ContainerDefinition { constructor(scope: Construct, id: string, props: FirelensLogRouterProps) { super(scope, id, props); const options = props.firelensConfig.options; - if (options) { - 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)) + const enableECSLogMetadata = options?.enableECSLogMetadata || options?.enableECSLogMetadata === undefined; + if (options?.configFileValue) { + const configFileType = (options?.configFileType === undefined || options?.configFileType === FirelensConfigFileType.S3) && + (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, + configFileValue: options?.configFileValue, }, }; - // grant s3 access permissions if (configFileType === FirelensConfigFileType.S3) { props.taskDefinition.addToExecutionRolePolicy(new iam.PolicyStatement({ actions: [ 's3:GetObject', ], - resources: [options.configFileValue], + resources: [options?.configFileValue], })); props.taskDefinition.addToExecutionRolePolicy(new iam.PolicyStatement({ actions: [ 's3:GetBucketLocation', ], - resources: [options.configFileValue.split('/')[0]], + resources: [options?.configFileValue.split('/')[0]], })); } + } else if (options?.configFileType && !options?.configFileValue) { + throw new Error('Firelens Config Error: Config file value cannot be undefined when config file type is declared.'); } else { - this.firelensConfig = props.firelensConfig; + this.firelensConfig = { + type: props.firelensConfig.type, + options: { + enableECSLogMetadata, + }, + }; } } diff --git a/packages/@aws-cdk/aws-ecs/test/container-definition.test.ts b/packages/@aws-cdk/aws-ecs/test/container-definition.test.ts index 524551aba1286..a5153b82d331a 100644 --- a/packages/@aws-cdk/aws-ecs/test/container-definition.test.ts +++ b/packages/@aws-cdk/aws-ecs/test/container-definition.test.ts @@ -5,9 +5,9 @@ import * as ecr_assets from '@aws-cdk/aws-ecr-assets'; import * as s3 from '@aws-cdk/aws-s3'; import * as secretsmanager from '@aws-cdk/aws-secretsmanager'; import * as ssm from '@aws-cdk/aws-ssm'; +import { testFutureBehavior } from '@aws-cdk/cdk-build-tools/lib/feature-flag'; import * as cdk from '@aws-cdk/core'; import * as cxapi from '@aws-cdk/cx-api'; -import { testFutureBehavior } from '@aws-cdk/cdk-build-tools/lib/feature-flag'; import * as ecs from '../lib'; describe('container definition', () => { diff --git a/packages/@aws-cdk/aws-ecs/test/ec2/ec2-task-definition.test.ts b/packages/@aws-cdk/aws-ecs/test/ec2/ec2-task-definition.test.ts index 4f80d48118bc6..966473e5cc6cb 100644 --- a/packages/@aws-cdk/aws-ecs/test/ec2/ec2-task-definition.test.ts +++ b/packages/@aws-cdk/aws-ecs/test/ec2/ec2-task-definition.test.ts @@ -5,9 +5,9 @@ import { Repository } from '@aws-cdk/aws-ecr'; import * as iam from '@aws-cdk/aws-iam'; import * as secretsmanager from '@aws-cdk/aws-secretsmanager'; import * as ssm from '@aws-cdk/aws-ssm'; +import { testFutureBehavior } from '@aws-cdk/cdk-build-tools/lib/feature-flag'; import * as cdk from '@aws-cdk/core'; import * as cxapi from '@aws-cdk/cx-api'; -import { testFutureBehavior } from '@aws-cdk/cdk-build-tools/lib/feature-flag'; import * as ecs from '../../lib'; describe('ec2 task definition', () => { diff --git a/packages/@aws-cdk/aws-ecs/test/fargate/integ.firelens-cloudwatch.expected.json b/packages/@aws-cdk/aws-ecs/test/fargate/integ.firelens-cloudwatch.expected.json index a10c635e498d6..fb627b0328ac2 100644 --- a/packages/@aws-cdk/aws-ecs/test/fargate/integ.firelens-cloudwatch.expected.json +++ b/packages/@aws-cdk/aws-ecs/test/fargate/integ.firelens-cloudwatch.expected.json @@ -431,7 +431,10 @@ { "Essential": true, "FirelensConfiguration": { - "Type": "fluentbit" + "Type": "fluentbit", + "Options": { + "enable-ecs-log-metadata": "true" + } }, "Image": { "Ref": "SsmParameterValueawsserviceawsforfluentbitlatestC96584B6F00A464EAD1953AFF4B05118Parameter" 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 6ab96b05df364..e4ee7c06ccf9f 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 @@ -35,6 +35,9 @@ describe('firelens log driver', () => { Essential: true, FirelensConfiguration: { Type: 'fluentbit', + Options: { + 'enable-ecs-log-metadata': 'true', + }, }, }, ], @@ -211,6 +214,9 @@ describe('firelens log driver', () => { Essential: true, FirelensConfiguration: { Type: 'fluentbit', + Options: { + 'enable-ecs-log-metadata': 'true', + }, }, }, ], @@ -250,6 +256,9 @@ describe('firelens log driver', () => { Essential: true, FirelensConfiguration: { Type: 'fluentbit', + Options: { + 'enable-ecs-log-metadata': 'true', + }, }, }, ], @@ -279,6 +288,9 @@ describe('firelens log driver', () => { Name: 'log_router', FirelensConfiguration: { Type: 'fluentd', + Options: { + 'enable-ecs-log-metadata': 'true', + }, }, }, ], @@ -359,8 +371,6 @@ describe('firelens log driver', () => { }, ], }); - - }); }); }); From ff2045d768eb6ad85af1755c9edf2be11e6c104c Mon Sep 17 00:00:00 2001 From: david-doyle-as24 <78368860+david-doyle-as24@users.noreply.github.com> Date: Fri, 8 Oct 2021 10:45:50 +0200 Subject: [PATCH 2/2] adding test --- .../ecs-service-extensions/test/firelens.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/@aws-cdk-containers/ecs-service-extensions/test/firelens.test.ts b/packages/@aws-cdk-containers/ecs-service-extensions/test/firelens.test.ts index 2f30e6139e33f..ebdfac8e876d8 100644 --- a/packages/@aws-cdk-containers/ecs-service-extensions/test/firelens.test.ts +++ b/packages/@aws-cdk-containers/ecs-service-extensions/test/firelens.test.ts @@ -78,6 +78,9 @@ describe('firelens', () => { Essential: true, FirelensConfiguration: { Type: 'fluentbit', + Options: { + 'enable-ecs-log-metadata': 'true', + }, }, Image: { Ref: 'SsmParameterValueawsserviceawsforfluentbitlatestC96584B6F00A464EAD1953AFF4B05118Parameter',