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 (backport #20636) #21710

Merged
merged 2 commits into from
Aug 22, 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
3 changes: 3 additions & 0 deletions allowed-breaking-changes.txt
Original file line number Diff line number Diff line change
Expand Up @@ -139,3 +139,6 @@ removed:@aws-cdk/aws-lambda-event-sources.SelfManagedKafkaEventSourceProps.onFai

# removed kubernetes version from EKS
removed:@aws-cdk/aws-eks.KubernetesVersion.V1_22

#configFileValue is unnecessarily required; not a breaking change
weakened:@aws-cdk/aws-ecs.FirelensOptions
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',
},
},
}),
],
});
});
});
});