Skip to content

Commit

Permalink
fix(ecs): firelens configFileValue is unnecessarily required (aws#20636)
Browse files Browse the repository at this point in the history
`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*
  • Loading branch information
PettitWesley authored and mrgrain committed Jul 28, 2022
1 parent ae13dc6 commit 9d12f55
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 19 deletions.
65 changes: 46 additions & 19 deletions packages/@aws-cdk/aws-ecs/lib/firelens-log-router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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' }),
Expand Down
33 changes: 33 additions & 0 deletions packages/@aws-cdk/aws-ecs/test/firelens-log-driver.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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' }),
Expand Down Expand Up @@ -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',
},
},
}),
],
});
});
});
});

0 comments on commit 9d12f55

Please sign in to comment.