Support aws ecs readonly_root_filesystem

[NickLavrov]

Summary & Motivation

When unset, readonlyRootFilesystem is false on ECS tasks, which then violates this Security Hub control

How I Tested These Changes

Not sure how to test ECS launching. I added an assertion to the existing pytest that this defaults to None, and added a new pytest that verifies a setting of true carries through to the task definition.

Changelog

Add optional boolean readonly_root_filesystem to container_context.

[gibsondan]

@NickLavrov thanks for sending this out! Do you think its plausible that somebody would want to vary the value of this for different code locations within their deployment? Or would it always be a single setting that you put on the run launcher / agent in the dagster.yaml? Asking because if you don't need to vary it per code location, it doesn't need to go on EcsContainerContext

[NickLavrov]

@gibsondan Personally I want this value to be true for all ecs tasks, per the AWS Security Hub control. I am not sure where someone would want this to be false, but I also don't really have that many use-cases so far with how we use Dagster, so I think leaving it configurable like this makes sense. I leave it defaulting to None to match existing behavior, also.

I'm not sure how to implement that other scenario, also. I just followed the pattern of repository_credentials more or less.

I also note all the CI tests are failing due to a docker pull error unrelated to my changes.

[gibsondan]

nice, that's my intuition too. I think if that's the case we can simplify this a bit - removing it from the 'per-location configuration piece' and only referencing it in the run launcher instead of EcsContainerContext. That should be a strict subset of what you have now.

We'll get the CI fixed, we changed the image recently and just need to get it applied here too. Thanks for flagging that.

[gibsondan]

once this is just on the run launcher and not on EcsContainerContext, this can be self.readonly_root_filesystem instead

[graphite-app]

The comment readonlyRootFilesystem is set to correctly contains a grammatical error. Consider revising to readonlyRootFilesystem is set correctly.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

[gibsondan]

configuration that just goes on the run launcher config and not in the per-location config goes here instead:

dagster/python_modules/libraries/dagster-aws/dagster_aws/ecs/launcher.py

Lines 392 to 399 in ff96fcb

	"task_definition_prefix": Field(
	StringSource,
	is_required=False,
	default_value="run",
	description=(
	"A prefix that is applied to all task definitions created by the EcsRunLauncher. Defaults to 'run'."
	),
	),

- so you would just copy paste this same configuration to be there instead of in SHARED_ECS_SCHEMA.

[NickLavrov]

I commented out all instances of readonly_root_filesystem in container_context.py, moved that def to launcher.py, and changed the pytest to use:

    with instance_cm(
        {
            "task_definition": {
                "task_role_arn": "fake-task-role",
                "execution_role_arn": "fake-execution-role",
            },
            "readonly_root_filesystem": readonly_root_filesystem,
        }

Is this approach correct in general (i.e. is that the right way to have this test run?)? I think I will have to uncomment some instances of readonly_root_filesystem in container_context.py.

[gibsondan]

that looks right to me!

[mlarose]

Not sure how to test ECS launching. I added an assertion to the existing pytest that this defaults to None, and added a new pytest that verifies a setting of true carries through to the task definition.

Hi @NickLavrov , thanks for this. I once explored implementing this for Dagster+ and I encountered pitfalls that I am not sure you'll also experience in a OSS context. There are, depending on logging setup or other configuration, files that may be written to the volume, not to mention what the actual user code does. To get the full value out of this change, you may have other hurdles to jump through, such as defining and mounting writable volumes to receive these files.

That being said, if that bit works correctly, then the rest can be incremental change.