fix(ecs): firelens configFileValue is unnecessarily required (#20636)

PettitWesley · web-flow · commit b79b2e4702c7 · 2022-07-26T19:09:53.000Z
`FirelensOptions` should have only optional properties, but `configFileValue` was previously marked as required. This caused some confusion and incorrect configuration like `configFileValue = ''` as seen here: aws/aws-for-fluent-bit#352. This fix marks `configFileValue` as optional, and makes sure that `configFileValue` and `configFileType` are set together, or not at all. See [docs](https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ecs-taskdefinition-firelensconfiguration.html#cfn-ecs-taskdefinition-firelensconfiguration-options). Signed-off-by: Wesley Pettit <wppttt@amazon.com> Needed to fix: aws/aws-for-fluent-bit#352 ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
diff --git 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
@@ -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
@@ -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',
+              },
+            },
+          }),
+        ],
+      });
+    });
   });
 });
