fix(ecs): firelens configFileValue is unnecessarily required by PettitWesley · Pull Request #20636 · aws/aws-cdk

PettitWesley · 2022-06-06T21:21:12Z

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.

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?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- 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

gitpod-io · 2022-06-06T21:21:24Z

kaizencc

Hi @PettitWesley, thanks for contributing this! In addition to the PR comments, please add a PR description and add unit tests to validate your changes.

kaizencc · 2022-06-10T21:06:54Z

packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts


  /**
   * Custom configuration file, S3 ARN or a file path
+   * @default - configFileValue is user defined and optional and there is no default


Suggested change

* @default - configFileValue is user defined and optional and there is no default

*

* @default - no config file value

packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts

kaizencc · 2022-06-10T21:23:10Z

packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts

  if (!firelensConfig.options) {
    return { type: firelensConfig.type };
+  } else if (firelensConfig.options.configFileValue === undefined) {
+    // config file options must be set together and are optional


can you be more specific in this comment. i think i understand -- if configFileValue isn't set, then configFileType doesn't need to be either. But it took some time to get there. Also, do you have a link to why this is the case, or anything like that?

It looks like we may need to improve the docs to make it more clear, but basically, its inherent in the meaning of the fields that they have to be used together. config-file-type exists to describe the value of the config-file-value.

kaizencc · 2022-06-10T21:24:42Z

packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts

        '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!,


Suggested change

'config-file-value': firelensConfig.options.configFileValue!,

'config-file-value': firelensConfig.options.configFileValue,

can you double check this? I think typescript should know that configFileValue is defined because of the else if condition above this.

kaizencc · 2022-06-10T21:29:30Z

packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts

+      if (options.configFileValue != undefined) {
+        this.firelensConfig = {
+          type: props.firelensConfig.type,
+          options: {
+            enableECSLogMetadata,
+            configFileType,
+            configFileValue: options.configFileValue,
+          },
+        };
+      } else {
+        this.firelensConfig = {
+          type: props.firelensConfig.type,
+          options: {
+            enableECSLogMetadata: enableECSLogMetadata,
+          },
+        };
      }


Suggested change

if (options.configFileValue != undefined) {

this.firelensConfig = {

type: props.firelensConfig.type,

options: {

enableECSLogMetadata,

configFileType,

configFileValue: options.configFileValue,

},

};

} else {

this.firelensConfig = {

type: props.firelensConfig.type,

options: {

enableECSLogMetadata: enableECSLogMetadata,

},

};

}

this.firelensConfig = {

type: props.firelensConfig.type,

options: {

enableECSLogMetadata,

...(options.configFileValue ? {

configFileType,

configFileValue: options.configFileValue,

} : {},

),

}

my editor strongly suggests this code is not valid

kaizencc · 2022-06-10T21:29:57Z

packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts

+        };
      }
+
+      if (options.configFileValue != undefined) {


Suggested change

if (options.configFileValue != undefined) {

if (options.configFileValue !== undefined) {

kaizencc · 2022-06-10T21:30:25Z

packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts

+            actions: [
+              's3:GetBucketLocation',
+            ],
+            resources: [(options.configFileValue || '').split('/')[0]],


Suggested change

resources: [(options.configFileValue || '').split('/')[0]],

resources: [(options.configFileValue ?? '').split('/')[0]],

kaizencc · 2022-06-10T21:31:37Z

packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts

      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 || ''))


this is getting ugly, can we pull this logic into a isS3FileType() method?

kaizencc · 2022-06-10T21:34:40Z

packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts

-          resources: [options.configFileValue.split('/')[0]],
-        }));
+
+      if (options.configFileValue != undefined) {


instead of using options.configFileValue as the "source of truth" for all the logic that governs config files, let's create a variable configFile: boolean so that it's more clear that configFileValue and configFileType are under the same umbrella.

In it's current state it's quite confusing.

Pull request has been modified.

kaizencc

Thanks for adding the test, @PettitWesley! A few more comments elsewhere in the PR

kaizencc · 2022-07-19T18:48:25Z

packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts

+      if (hasConfig) {
+        this.firelensConfig = {
+          type: props.firelensConfig.type,
+          options: {
+            enableECSLogMetadata,
+            configFileType,
+            configFileValue: options.configFileValue,
+          },
+        };
+      } else {
+        this.firelensConfig = {
+          type: props.firelensConfig.type,
+          options: {
+            enableECSLogMetadata: enableECSLogMetadata,
+          },
+        };


Suggested change

if (hasConfig) {

this.firelensConfig = {

type: props.firelensConfig.type,

options: {

enableECSLogMetadata,

configFileType,

configFileValue: options.configFileValue,

},

};

} else {

this.firelensConfig = {

type: props.firelensConfig.type,

options: {

enableECSLogMetadata: enableECSLogMetadata,

},

};

this.firelensConfig = {

type: props.firelensConfig.type,

options: {

enableECSLogMetadata,

...(hasConfig ? {

configFileType,

configFileValue: options.configFileValue,

} : {}),

},

};

This pattern is used elsewhere in the CDK and is cleaner. It might need to be tweaked because I did not compile it.

It does seem to compile thanks

packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts

kaizencc · 2022-07-19T18:52:56Z

packages/@aws-cdk/aws-ecs/test/firelens-log-driver.test.ts

+    // test added for: https://github.com/aws/aws-for-fluent-bit/issues/352
+    test('firelens config-file-* options are fully optional', () => {


Suggested change

// test added for: https://github.com/aws/aws-for-fluent-bit/issues/352

test('firelens config-file-* options are fully optional', () => {

test('firelens config options are fully optional', () => {

packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts

kaizencc · 2022-07-19T18:59:59Z

packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts

    super(scope, id, props);
    const options = props.firelensConfig.options;
    if (options) {
+      const hasConfig = (options.configFileValue !== undefined);


the synth-time check mentioned in the other comment should go here.

Pull request has been modified.

Signed-off-by: Wesley Pettit <wppttt@amazon.com>

packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts

packages/@aws-cdk/aws-ecs/test/firelens-log-driver.test.ts

packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts

mergify · 2022-07-26T18:33:49Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-07-26T19:09:36Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 9889d8d
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-07-26T19:09:56Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

`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*

kaizencc · 2022-08-22T14:56:56Z

@Mergifyio backport v1-main

mergify · 2022-08-22T14:57:23Z

backport v1-main

✅ Backports have been created

Details

#21710 fix(ecs): firelens configFileValue is unnecessarily required (backport #20636) has been created for branch v1-main

`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* (cherry picked from commit b79b2e4)

…#20636) (#21710) This is an automatic backport of pull request #20636 done by [Mergify](https://mergify.com). --- <details> <summary>Mergify commands and options</summary> <br /> 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 <destination>` will backport this PR on `<destination>` 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 </details>

github-actions bot added the p2 label Jun 6, 2022

aws-cdk-automation requested a review from a team June 6, 2022 21:21

PettitWesley force-pushed the config-file-value-optional branch 2 times, most recently from c30efe6 to 0b8d047 Compare June 10, 2022 17:22

kaizencc changed the title ~~fix: firelens configFileValue is optional~~ fix(ecs): firelens configFileValue is optional Jun 10, 2022

kaizencc previously requested changes Jun 10, 2022

View reviewed changes

madeline-k added p1 and removed p2 labels Jun 13, 2022

PettitWesley force-pushed the config-file-value-optional branch from 0b8d047 to 7f58d4a Compare June 14, 2022 20:06

PettitWesley force-pushed the config-file-value-optional branch 5 times, most recently from 00a56d5 to 2526e29 Compare June 16, 2022 23:16

PettitWesley requested a review from kaizencc June 20, 2022 21:48

PettitWesley force-pushed the config-file-value-optional branch from 2526e29 to 3128025 Compare June 21, 2022 14:56

kaizencc changed the title ~~fix(ecs): firelens configFileValue is optional~~ fix(ecs): firelens configFileValue is unnecessarily required Jun 27, 2022

PettitWesley force-pushed the config-file-value-optional branch 4 times, most recently from d6c80d7 to 2778733 Compare July 19, 2022 17:31

kaizencc previously requested changes Jul 19, 2022

View reviewed changes

PettitWesley force-pushed the config-file-value-optional branch from 2778733 to 56c0168 Compare July 20, 2022 20:33

PettitWesley force-pushed the config-file-value-optional branch 4 times, most recently from 1df88f9 to 64bd224 Compare July 25, 2022 20:49

PettitWesley force-pushed the config-file-value-optional branch 2 times, most recently from 6179dfe to 01a5599 Compare July 25, 2022 23:04

fix: firelens configFileValue is optional

d966806

Signed-off-by: Wesley Pettit <wppttt@amazon.com>

PettitWesley force-pushed the config-file-value-optional branch from 01a5599 to d966806 Compare July 26, 2022 00:10

kaizencc reviewed Jul 26, 2022

View reviewed changes

Apply suggestions from code review

5216510

kaizencc approved these changes Jul 26, 2022

View reviewed changes

Merge branch 'main' into config-file-value-optional

9889d8d

mergify bot merged commit b79b2e4 into aws:main Jul 26, 2022

PettitWesley mentioned this pull request Aug 15, 2022

(aws-ecs): configFileValue should be optional in FirelensOptions #16594

Closed

mergify bot mentioned this pull request Aug 22, 2022

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

Merged

HBobertz mentioned this pull request Nov 10, 2022

v1 main #22869

Closed

4 tasks

This was referenced Mar 31, 2023

chore: revert cfn spec for v1 #24891

Merged

chore: revert previous cfn spec reverts #24897

Closed

	* @default - configFileValue is user defined and optional and there is no default
	*
	* @default - no config file value

	'config-file-value': firelensConfig.options.configFileValue!,
	'config-file-value': firelensConfig.options.configFileValue,

	if (options.configFileValue != undefined) {
	if (options.configFileValue !== undefined) {

	resources: [(options.configFileValue \|\| '').split('/')[0]],
	resources: [(options.configFileValue ?? '').split('/')[0]],

		// test added for: https://github.com/aws/aws-for-fluent-bit/issues/352
		test('firelens config-file-* options are fully optional', () => {

	// test added for: https://github.com/aws/aws-for-fluent-bit/issues/352
	test('firelens config-file-* options are fully optional', () => {
	test('firelens config options are fully optional', () => {

Conversation

PettitWesley commented Jun 6, 2022 • edited by kaizencc Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Adding new Unconventional Dependencies:

New Features

Uh oh!

gitpod-io bot commented Jun 6, 2022

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaizencc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mergify bot commented Jul 26, 2022

Uh oh!

aws-cdk-automation commented Jul 26, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jul 26, 2022

Uh oh!

kaizencc commented Aug 22, 2022

Uh oh!

mergify bot commented Aug 22, 2022

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PettitWesley commented Jun 6, 2022 •

edited by kaizencc

Loading