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

fix(ecs): configFileValue on FirelensOptions should be an optional property #16856

Closed
wants to merge 4 commits into from
Closed

fix(ecs): configFileValue on FirelensOptions should be an optional property #16856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5176be9
fixing bug
david-doyle-as24 Oct 7, 2021
6d45aab
Merge branch 'aws:master' into master
david-doyle-as24 Oct 7, 2021
ff2045d
adding test
david-doyle-as24 Oct 8, 2021
b1945a5
Merge branch 'master' into master
upparekh Nov 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/@aws-cdk-containers/ecs-service-extensions/test/firelens.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ describe('firelens', () => {
Essential: true,
FirelensConfiguration: {
Type: 'fluentbit',
Options: {
'enable-ecs-log-metadata': 'true',
},
},
Image: {
Ref: 'SsmParameterValueawsserviceawsforfluentbitlatestC96584B6F00A464EAD1953AFF4B05118Parameter',
Expand Down
31 changes: 19 additions & 12 deletions packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand All @@ -78,7 +79,7 @@ export interface FirelensConfig {

/**
* Firelens options
* @default - no additional options
* @default - EnableECSLogMetadata option is set to true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: @default - ECS log metadata enabled

*/
readonly options?: FirelensOptions;
}
Expand Down Expand Up @@ -116,7 +117,7 @@ function renderFirelensConfig(firelensConfig: FirelensConfig): CfnTaskDefinition
options: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can also remove the if-else condition here to always return the firelens config in the else part (as the enable-ecs-log-metadata in the options will always be present now).

'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!,
},
};
}
Expand Down Expand Up @@ -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.');
Comment on lines +232 to +233
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add a unit test to assert this error being thrown?

} else {
this.firelensConfig = props.firelensConfig;
this.firelensConfig = {
type: props.firelensConfig.type,
options: {
enableECSLogMetadata,
},
};
}
}

Expand Down
5 changes: 4 additions & 1 deletion packages/@aws-cdk/aws-ecs/test/fargate/integ.firelens-cloudwatch.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,10 @@
{
"Essential": true,
"FirelensConfiguration": {
"Type": "fluentbit"
"Type": "fluentbit",
"Options": {
"enable-ecs-log-metadata": "true"
}
},
"Image": {
"Ref": "SsmParameterValueawsserviceawsforfluentbitlatestC96584B6F00A464EAD1953AFF4B05118Parameter"
Expand Down
14 changes: 12 additions & 2 deletions packages/@aws-cdk/aws-ecs/test/firelens-log-driver.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ describe('firelens log driver', () => {
Essential: true,
FirelensConfiguration: {
Type: 'fluentbit',
Options: {
'enable-ecs-log-metadata': 'true',
},
},
},
],
Expand Down Expand Up @@ -211,6 +214,9 @@ describe('firelens log driver', () => {
Essential: true,
FirelensConfiguration: {
Type: 'fluentbit',
Options: {
'enable-ecs-log-metadata': 'true',
},
},
},
],
Expand Down Expand Up @@ -250,6 +256,9 @@ describe('firelens log driver', () => {
Essential: true,
FirelensConfiguration: {
Type: 'fluentbit',
Options: {
'enable-ecs-log-metadata': 'true',
},
},
},
],
Expand Down Expand Up @@ -279,6 +288,9 @@ describe('firelens log driver', () => {
Name: 'log_router',
FirelensConfiguration: {
Type: 'fluentd',
Options: {
'enable-ecs-log-metadata': 'true',
},
},
},
],
Expand Down Expand Up @@ -359,8 +371,6 @@ describe('firelens log driver', () => {
},
],
});


});
});
});