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

feat: ability to configure log driver and log options #255

Merged
merged 4 commits into from
Sep 6, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
19 changes: 19 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,25 @@ input of the second:
cluster: my-cluster
```

Use the following approach to configure your log driver if needed:

```yaml
- name: Render Amazon ECS task definition
id: render-web-container
uses: aws-actions/amazon-ecs-render-task-definition@v1
with:
task-definition: task-definition.json
container-name: web
image: amazon/amazon-ecs-sample:latest
log-configuration-log-driver: awslogs
log-configuration-log-options: |
awslogs-create-group=true
awslogs-group=/ecs/web
awslogs-region=us-east-1
awslogs-stream-prefix=ecs

```

See [action.yml](action.yml) for the full documentation for this action's inputs and outputs.

## License Summary
Expand Down
6 changes: 6 additions & 0 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ inputs:
environment-variables:
description: 'Variables to add to the container. Each variable is of the form KEY=value, you can specify multiple variables with multi-line YAML strings.'
required: false
log-configuration-log-driver:
description: "Create/Override logDriver inside logConfiguration"
required: false
log-configuration-options:
description: "Create/Override options inside logConfiguration. Each variable is of the form key=value, you can specify multiple variables with multi-line YAML strings."
required: false
outputs:
task-definition:
description: 'The path to the rendered task definition file'
Expand Down
32 changes: 29 additions & 3 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ async function run() {

const environmentVariables = core.getInput('environment-variables', { required: false });

const logConfigurationLogDriver = core.getInput("log-configuration-log-driver", { required: false });
const logConfigurationOptions = core.getInput("log-configuration-options", { required: false });

// Parse the task definition
const taskDefPath = path.isAbsolute(taskDefinitionFile) ?
taskDefinitionFile :
Expand All @@ -25,7 +28,7 @@ async function run() {
if (!Array.isArray(taskDefContents.containerDefinitions)) {
throw new Error('Invalid task definition format: containerDefinitions section is not present or is not an array');
}
const containerDef = taskDefContents.containerDefinitions.find(function(element) {
const containerDef = taskDefContents.containerDefinitions.find(function (element) {
return element.name == containerName;
});
if (!containerDef) {
Expand All @@ -50,7 +53,7 @@ async function run() {
const separatorIdx = trimmedLine.indexOf("=");
// If there's nowhere to split
if (separatorIdx === -1) {
throw new Error(`Cannot parse the environment variable '${trimmedLine}'. Environment variable pairs must be of the form NAME=value.`);
throw new Error(`Cannot parse the environment variable '${trimmedLine}'. Environment variable pairs must be of the form NAME=value.`);
}
// Build object
const variable = {
Expand All @@ -70,6 +73,29 @@ async function run() {
})
}

if (logConfigurationLogDriver) {
if (!containerDef.logConfiguration) { containerDef.logConfiguration = {} }
const validDrivers = ["json-file", "syslog", "journald", "gelf", "fluentd", "awslogs", "splunk", "awsfirelens"];
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently for the ECS task, these are the log configurations that are supported.

For tasks on AWS Fargate, the supported log drivers are awslogs, splunk, and awsfirelens
For tasks hosted on Amazon EC2 instances, the supported log drivers are awslogs, fluentd, gelf, json-file, journald, logentries, syslog, splunk, and awsfirelens.

Are we assuming these drivers for ECS Tasks based on Ec2 instance? if this is the case do we need to add logentries as well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

my current scenario is for fargate but yes, let's add logentries!

if (!validDrivers.find(driver => driver === logConfigurationLogDriver)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be simplified?

if (!validDrivers.includes(logConfigurationLogDriver)){
        throw new Error(`'${logConfigurationLogDriver}' is invalid logConfigurationLogDriver. valid options are ${validDrivers}. More details: https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_LogConfiguration.html`)
      }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i could remove the link to the documentation or the valid options list. what do you think?

Copy link
Contributor

@KollaAdithya KollaAdithya Aug 1, 2023

Choose a reason for hiding this comment

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

what i mean by this comment is that

instead of using array function in L79. if (!validDrivers.find(driver => driver === logConfigurationLogDriver))
can you just use if (!validDrivers.includes(logConfigurationLogDriver))?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahhh right away!

throw new Error(`'${logConfigurationLogDriver}' is invalid logConfigurationLogDriver. valid options are ${validDrivers}. More details: https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_LogConfiguration.html`)
}
containerDef.logConfiguration.logDriver = logConfigurationLogDriver
}

if (logConfigurationOptions) {
if (!containerDef.logConfiguration) { containerDef.logConfiguration = {} }
if (!containerDef.logConfiguration.options) { containerDef.logConfiguration.options = {} }
logConfigurationOptions.split("\n").forEach(function (option) {
option = option.trim();
if (option && option.length) { // not a blank line
if (option.indexOf("=") == -1) {
throw new Error(`Can't parse logConfiguration option ${option}. Must be in key=value format, one per line`);
}
const [key, value] = option.split("=");
containerDef.logConfiguration.options[key] = value
}
})
}

// Write out a new task definition file
var updatedTaskDefFile = tmp.fileSync({
Expand All @@ -92,5 +118,5 @@ module.exports = run;

/* istanbul ignore next */
if (require.main === module) {
run();
run();
}
65 changes: 63 additions & 2 deletions index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ describe('Render task definition', () => {
postfix: '.json',
keep: true,
discardDescriptor: true
});
});
expect(fs.writeFileSync).toHaveBeenNthCalledWith(1, 'new-task-def-file-name',
JSON.stringify({
family: 'task-def-family',
Expand Down Expand Up @@ -129,7 +129,7 @@ describe('Render task definition', () => {
postfix: '.json',
keep: true,
discardDescriptor: true
});
});
expect(fs.writeFileSync).toHaveBeenNthCalledWith(1, 'new-task-def-file-name',
JSON.stringify({
family: 'task-def-family',
Expand All @@ -150,6 +150,67 @@ describe('Render task definition', () => {
expect(core.setOutput).toHaveBeenNthCalledWith(1, 'task-definition', 'new-task-def-file-name');
});

test('renders logConfiguration on the task definition', async () => {
core.getInput = jest
.fn()
.mockReturnValueOnce('task-definition.json')
.mockReturnValueOnce('web')
.mockReturnValueOnce('nginx:latest')
.mockReturnValueOnce('FOO=bar\nHELLO=world')
.mockReturnValueOnce('awslogs')
.mockReturnValueOnce(`awslogs-create-group=true\nawslogs-group=/ecs/web\nawslogs-region=us-east-1\nawslogs-stream-prefix=ecs`);

await run()

expect(tmp.fileSync).toHaveBeenNthCalledWith(1, {
tmpdir: '/home/runner/work/_temp',
prefix: 'task-definition-',
postfix: '.json',
keep: true,
discardDescriptor: true
});


expect(fs.writeFileSync).toHaveBeenNthCalledWith(1, 'new-task-def-file-name',
JSON.stringify({
family: 'task-def-family',
containerDefinitions: [
{
name: "web",
image: "nginx:latest",
environment: [
{
name: "FOO",
value: "bar"
},
{
name: "DONT-TOUCH",
value: "me"
},
{
name: "HELLO",
value: "world"
}
],
logConfiguration: {
logDriver: "awslogs",
options: {
"awslogs-create-group": "true",
"awslogs-group": "/ecs/web",
"awslogs-region": "us-east-1",
"awslogs-stream-prefix": "ecs"
}
}
},
{
name: "sidecar",
image: "hello"
}
]
}, null, 2)
);
});

test('error returned for missing task definition file', async () => {
fs.existsSync.mockReturnValue(false);
core.getInput = jest
Expand Down