Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

2.25.0+ has a bug when config-file-value is empty string or with broken @INCLUDE statement #352

Open
PettitWesley opened this issue May 14, 2022 · 2 comments · Fixed by aws/aws-cdk#20636
Open

2.25.0+ has a bug when config-file-value is empty string or with broken @INCLUDE statement #352

PettitWesley opened this issue May 14, 2022 · 2 comments · Fixed by aws/aws-cdk#20636

Comments

@PettitWesley
Copy link
Contributor

PettitWesley commented May 14, 2022 •
edited
Loading

Tracking this upstream issue (full issues description from Fluent Bit angle ->): fluent/fluent-bit#5454

We saw this with FireLens where a customer had this AWS CDK config:


      firelensConfig: {
        type: FirelensLogRouterType.FLUENTBIT,
        options: {
          enableECSLogMetadata: false,
          configFileValue: "",
        },
      },

That field should not be left empty. This field is not required, its optional according to CDK docs: https://docs.aws.amazon.com/cdk/api/v1/docs/@aws-cdk_aws-ecs.FirelensOptions.html
PettitWesley added a commit to PettitWesley/aws-cdk that referenced this issue May 17, 2022
@PettitWesley
fix: FireLens configFileValue should be optional
5278719 
aws/aws-for-fluent-bit#352

Signed-off-by: Wesley Pettit <wppttt@amazon.com>
@PettitWesley
Copy link
Contributor Author

Actually I read the CDK docs wrong and it is marked as non optional right now, so fixing that: aws/aws-cdk#20381

PettitWesley added a commit to PettitWesley/aws-cdk that referenced this issue May 18, 2022
@PettitWesley
fix: firelens configFileValue is optional
aafc101 
aws/aws-for-fluent-bit#352

Signed-off-by: Wesley Pettit <wppttt@amazon.com>
PettitWesley added a commit to PettitWesley/aws-cdk that referenced this issue May 18, 2022
@PettitWesley
fix: firelens configFileValue is optional
39d588a 
aws/aws-for-fluent-bit#352

Signed-off-by: Wesley Pettit <wppttt@amazon.com>
@zhonghui12
Copy link
Contributor

Have put up the CR internally to fix the validation part in FireLens

PettitWesley added a commit to PettitWesley/aws-cdk that referenced this issue May 25, 2022
@PettitWesley
fix: firelens configFileValue is optional
a22bfc2 
aws/aws-for-fluent-bit#352

Signed-off-by: Wesley Pettit <wppttt@amazon.com>
PettitWesley added a commit to PettitWesley/aws-cdk that referenced this issue May 25, 2022
@PettitWesley
fix: firelens configFileValue is optional
4ef742e 
aws/aws-for-fluent-bit#352

Signed-off-by: Wesley Pettit <wppttt@amazon.com>
PettitWesley added a commit to PettitWesley/aws-cdk that referenced this issue May 25, 2022
@PettitWesley
fix: firelens configFileValue is optional
5a64818 
aws/aws-for-fluent-bit#352

Signed-off-by: Wesley Pettit <wppttt@amazon.com>
PettitWesley added a commit to PettitWesley/aws-cdk that referenced this issue May 25, 2022
@PettitWesley
fix: firelens configFileValue is optional
c821559 
aws/aws-for-fluent-bit#352

Signed-off-by: Wesley Pettit <wppttt@amazon.com>
PettitWesley added a commit to PettitWesley/aws-cdk that referenced this issue Jun 1, 2022
@PettitWesley
fix: firelens configFileValue is optional
adad238 
aws/aws-for-fluent-bit#352

Signed-off-by: Wesley Pettit <wppttt@amazon.com>
PettitWesley added a commit to PettitWesley/aws-cdk that referenced this issue Jun 2, 2022
@PettitWesley
fix: firelens configFileValue is optional
f524b65 
aws/aws-for-fluent-bit#352

Signed-off-by: Wesley Pettit <wppttt@amazon.com>
PettitWesley added a commit to PettitWesley/aws-cdk that referenced this issue Jun 2, 2022
@PettitWesley
fix: firelens configFileValue is optional
3d5b50b 
aws/aws-for-fluent-bit#352

Signed-off-by: Wesley Pettit <wppttt@amazon.com>
@PettitWesley PettitWesley mentioned this issue Jun 6, 2022
Merged
4 tasks
mergify bot pushed a commit to aws/aws-cdk that referenced this issue Jul 26, 2022
@PettitWesley
fix(ecs): firelens configFileValue is unnecessarily required (#20636)
b79b2e4 
`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*
mrgrain pushed a commit to mrgrain/aws-cdk that referenced this issue Jul 27, 2022
@PettitWesley @mrgrain
fix(ecs): firelens configFileValue is unnecessarily required (aws#20636)
21e1d54 
`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*
mrgrain pushed a commit to mrgrain/aws-cdk that referenced this issue Jul 28, 2022
@PettitWesley @mrgrain
fix(ecs): firelens configFileValue is unnecessarily required (aws#20636)
9d12f55 
`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*
mrgrain pushed a commit to mrgrain/aws-cdk that referenced this issue Jul 28, 2022
@PettitWesley @mrgrain
fix(ecs): firelens configFileValue is unnecessarily required (aws#20636)
7a8a954 
`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*
Closed
@PettitWesley PettitWesley changed the title 2.25.0 has a bug when config-file-value is empty string or with broken @INCLUDE statement 2.25.0+ has a bug when config-file-value is empty string or with broken @INCLUDE statement Aug 15, 2022
mergify bot pushed a commit to aws/aws-cdk that referenced this issue Aug 22, 2022
@PettitWesley @mergify
fix(ecs): firelens configFileValue is unnecessarily required (#20636)
42c7178 
`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)
josephedward pushed a commit to josephedward/aws-cdk that referenced this issue Aug 30, 2022
@PettitWesley @josephedward
fix(ecs): firelens configFileValue is unnecessarily required (aws#20636)
1fc60ff 
`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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(ecs): firelens configFileValue is unnecessarily required PettitWesley/aws-cdk
2 participants
@PettitWesley @zhonghui12