-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
fix(ecs): firelens configFileValue is unnecessarily required #20636
Conversation
c30efe6
to
0b8d047
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -60,8 +60,9 @@ export interface FirelensOptions { | |||
|
|||
/** | |||
* Custom configuration file, S3 ARN or a file path | |||
* @default - configFileValue is user defined and optional and there is no default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @default - configFileValue is user defined and optional and there is no default | |
* | |
* @default - no config file value |
@@ -109,14 +110,22 @@ 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 must be set together and are optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
} else { | ||
// firelensConfig.options.configFileType has been filled with s3 or file type in constructor. | ||
return { | ||
type: firelensConfig.type, | ||
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!, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'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.
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, | ||
}, | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, | |
} : {}, | |
), | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my editor strongly suggests this code is not valid
} | ||
|
||
if (options.configFileValue != undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (options.configFileValue != undefined) { | |
if (options.configFileValue !== undefined) { |
actions: [ | ||
's3:GetBucketLocation', | ||
], | ||
resources: [(options.configFileValue || '').split('/')[0]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resources: [(options.configFileValue || '').split('/')[0]], | |
resources: [(options.configFileValue ?? '').split('/')[0]], |
@@ -203,32 +212,45 @@ export class FirelensLogRouter extends ContainerDefinition { | |||
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)) | |||
(cdk.Token.isUnresolved(options.configFileValue) || /arn:aws[a-zA-Z-]*:s3:::.+/.test(options.configFileValue || '')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is getting ugly, can we pull this logic into a isS3FileType()
method?
resources: [options.configFileValue.split('/')[0]], | ||
})); | ||
|
||
if (options.configFileValue != undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
0b8d047
to
7f58d4a
Compare
00a56d5
to
2526e29
Compare
2526e29
to
3128025
Compare
d6c80d7
to
2778733
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the test, @PettitWesley! A few more comments elsewhere in the PR
if (hasConfig) { | ||
this.firelensConfig = { | ||
type: props.firelensConfig.type, | ||
options: { | ||
enableECSLogMetadata, | ||
configFileType, | ||
configFileValue: options.configFileValue, | ||
}, | ||
}; | ||
} else { | ||
this.firelensConfig = { | ||
type: props.firelensConfig.type, | ||
options: { | ||
enableECSLogMetadata: enableECSLogMetadata, | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does seem to compile thanks
// test added for: https://github.com/aws/aws-for-fluent-bit/issues/352 | ||
test('firelens config-file-* options are fully optional', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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', () => { |
@@ -201,34 +213,48 @@ export class FirelensLogRouter extends ContainerDefinition { | |||
super(scope, id, props); | |||
const options = props.firelensConfig.options; | |||
if (options) { | |||
const hasConfig = (options.configFileValue !== undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the synth-time check mentioned in the other comment should go here.
2778733
to
56c0168
Compare
1df88f9
to
64bd224
Compare
6179dfe
to
01a5599
Compare
Signed-off-by: Wesley Pettit <wppttt@amazon.com>
01a5599
to
d966806
Compare
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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*
`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*
`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*
@Mergifyio backport v1-main |
✅ Backports have been created
|
`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>
`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*
FirelensOptions
should have only optional properties, butconfigFileValue
was previously marked as required. This caused some confusion and incorrect configuration likeconfigFileValue = ''
as seen here: aws/aws-for-fluent-bit#352. This fix marksconfigFileValue
as optional, and makes sure thatconfigFileValue
andconfigFileType
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:
Adding new Unconventional Dependencies:
New Features
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