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.

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

Merged
merged 3 commits into from
Jul 26, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
kaizencc marked this conversation as resolved.
Show resolved Hide resolved
return {
type: firelensConfig.type,
options: {
'enable-ecs-log-metadata': firelensConfig.options.enableECSLogMetadata ? 'true' : 'false',
kaizencc marked this conversation as resolved.
Show resolved Hide resolved
},
};
} 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);
Copy link
Contributor

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.

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 || ''))
Copy link
Contributor

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?

? 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',
},
},
}),
],
});
});
});
});