dask / dask-cloudprovider

Cloud provider cluster managers for Dask. Supports AWS, Google Cloud Azure and more...
https://cloudprovider.dask.org
BSD 3-Clause "New" or "Revised" License
130 stars 107 forks source link

Allow specification of secrets in scheduler and worker container definitions for ECSCluster / FargateCluster #332

Open JonathanLifschutz opened 2 years ago

JonathanLifschutz commented 2 years ago

AWS allows you to specify 'secrets' in container definitions. This avoids having to call secrets manager directly in the application code.

The secrets key-value pair would need to be added to this block below (and again for the worker) based on init parameters.

containerDefinitions=[
    {
        "name": "dask-scheduler",
        "image": self.image,
        "cpu": self._scheduler_cpu,
        "memory": self._scheduler_mem,
        "memoryReservation": self._scheduler_mem,
        "essential": True,
        "command": [
            "dask-scheduler",
            "--idle-timeout",
            self._scheduler_timeout,
        ]
        + (
            list()
            if not self._scheduler_extra_args
            else self._scheduler_extra_args
        ),
        "ulimits": [
            {
                "name": "nofile",
                "softLimit": 65535,
                "hardLimit": 65535,
            },
        ],
        "logConfiguration": {
            "logDriver": "awslogs",
            "options": {
                "awslogs-region": ecs.meta.region_name,
                "awslogs-group": self.cloudwatch_logs_group,
                "awslogs-stream-prefix": self._cloudwatch_logs_stream_prefix,
                "awslogs-create-group": "true",
            },
        },
        "mountPoints": self._mount_points
        if self._mount_points and self._mount_volumes_on_scheduler
        else [],
    }
],

I envisage reusing the same pattern used by _scheduler_extra_args:

"secrets": []
    + (
        list()
        if not self._scheduler_extra_secrets
        else self._scheduler_extra_secrets
    )

N.B. there are other key-value pairs that it could be useful to specify, such as 'environment' for defining environment variables inline. These could be added in the same way, or better yet, in a more generic way that allows you to specify arbitrary containerDefinitions kwargs.

I'd be happy to provide a PR for either approach.

jacobtomlinson commented 2 years ago

A generic way to add extra keys to the container definition sounds great. Thanks for the offer to raise a PR, I look forward to reviewing it.

grochmal commented 1 month ago

Two years later, but i'm keen on taking over this. The improvement to use "secrets" is good and i already hacked something for myself that works in my local env.

@JonathanLifschutz is my CTO so i know the original requirements as to where this issue came from quite well :) .

The improvement to add "environment" in a similar way is not needed as that is already handled by containerOverrides here.

I may struggle to add tests (any suggestion on tests other then the import tests are welcome). I think i'll try to port some ec2 tests to ecs, but let's see how it goes.

jacobtomlinson commented 3 weeks ago

@grochmal sounds good. Feel free to ping me for review.