-
Notifications
You must be signed in to change notification settings - Fork 6
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: mount a volume and enforce readonlyRootFilesystem in ecs-task #2544
Conversation
… volume to ecs task containers
🦋 Changeset detectedLatest commit: 9fea171 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
7460a88
to
9fea171
Compare
}); | ||
|
||
containerDefinition.addMountPoints({ | ||
sourceVolume: `${app}-volume`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work only because the string matches name
in taskDefinition.addVolume
above? If so, I think we could make this more explicit via:
import type { Volume } from 'aws-cdk-lib/aws-ecs';
const volume: Volume = {
name: `${app}-volume`,
};
taskDefinition.addVolume(volume);
containerDefinition.addMountPoints({
sourceVolume: volume.name,
containerPath: volumeMountPath,
readOnly: false
});
/** | ||
* If your container needs to write to disk whilst running, you will need to mount a non-root volume to use. Setting | ||
* volumeMountPath will ensure a volume is mounted at that location, making use of Faragate ephemeral storage (set by | ||
* 'storage' param) | ||
* | ||
*/ | ||
volumeMountPath?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative might be to expose the created taskDefinition
and containerDefinition
as properties from GuEcsTask
:
export class GuEcsTask extends Construct {
public readonly task: TaskDefinition;
public readonly container: ContainerDefinition;
constructor() {
super();
const taskDefinition = new TaskDefinition(...);
const containerDefinition = new ContainerDefinition(...);
this.task = taskDefinition;
this.container = containerDefinition;
}
}
This approach avoids adding a new optional prop1 and increases flexibility as we can now freely call any function available on TaskDefinition
and ContainerDefinition
from our own project's infrastructure definition:
const { task, container } = new GuEcsTask(...);
task.addVolume({
name: 'asd'
});
task.someOtherUsefulFunction(...);
Footnotes
-
It also means this repository has a smaller surface area (less maintenance). ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh that's a much better approach!
"@guardian/cdk": major | ||
--- | ||
|
||
Enforce readonlyRootFilesystem in ecs-task. Support mounting a volume in ecs-task. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we more descriptive here and offer an example implementation?
Closing in favour of #2545 |
What does this change?
I'm trying to resolve [ECS.5] ECS containers should be limited to read-only access to root filesystems
for a number of ECS tasks in the investigations account. To do this we need to switch those tasks over to writing to a non-root volume.
Given that this is best practice, in this PR I am enforcing readonlyRootFilesystem. Of the 3 services that use this pattern, coverdrop already has readonlyRootFilesystem set to true. I'll update lurch and transcription service once this feature is out.
This PR adds an extra parameter to ecs-task
volumeMountPath
which, when set, will mount a volume for use by the container. I opted to just support a single volume rather than making this an array or similar to keep the task simple - of the 3 services which use this pattern, all of them will be fine with just the one volume.How to test
I tested this change using one of the lurch CODE tasks definitions, and verified that it was able to run succesfully (writing to disk along the way) without any problems