compose-x / ecs_composex

Manage, Configure and Deploy your services and AWS services and applications from your docker-compose definitions
https://docs.compose-x.io
Mozilla Public License 2.0
165 stars 17 forks source link

[BUG] ECS ContainerDefinition has broken Environment entries for Secrets #707

Closed thorfi closed 10 months ago

thorfi commented 10 months ago

ECS Task Definition has Environment variable and Secrets variable set.

CloudFormation crashes on the task sub-stack with:

Resource handler returned message: "Invalid request provided: Create TaskDefinition: The secret name must be unique and not shared with any new or existing environment variables set on the container, such as 'DB_PASSWORD'. 

To Reproduce Steps to reproduce the behavior:

  1. pip install
  2. cli
  3. Docker compose file

Note: fred/barney is the secretsmanager secret id for a JSON secret

services: 
  postgres:
    ...
    environment:
      - POSTGRES_DB=foo
      - POSTGRES_USER=quux
    secrets:
      - POSTGRES_PASSWORD

...

secrets:
  POSTGRES_PASSWORD:
    file: fred/barney/POSTGRES_PASSWORD
    x-secrets: # ecscomposex
      LinksTo: &ecx-secrets-links-to
        - EcsExecutionRole
        - EcsTaskRole
      Name: fred/barney
      VarName: POSTGRES_PASSWORD
      JsonKeys:
        - SecretKey: POSTGRES_PASSWORD
  1. See error

Generated CloudFormation Sub Stack postgres.yaml

The Environment: list has entries for the Secrets: entries which should not be there. They are strangely also different to the Secrets: entries.

Mappings:
  secrets:
    POSTGRESPASSWORD:
      Name: fred/barney
...      
Resources:
...
  EcsTaskDefinition:
    Properties:
      ContainerDefinitions:
        - Command:
            Ref: AWS::NoValue
          Cpu:
            Ref: AWS::NoValue
          DependsOn:
            Ref: AWS::NoValue
          DockerLabels:
            container_name: postgres
            ecs_task_family:
              Ref: MicroServiceName
          EntryPoint:
            Ref: AWS::NoValue
          Environment:
            - Name: POSTGRES_PASSWORD
              Value:
                Fn::Sub:
                  - arn:${AWS::Partition}:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:${SecretName}
                  - SecretName:
                      Fn::FindInMap:
                        - secrets
                        - POSTGRESPASSWORD
                        - Name
            - Name: POSTGRES_DB
              Value: foo
            - Fn::If:
                - UseExternalLaunchType
                - Name: AWS_DEFAULT_REGION
                  Value:
                    Ref: AWS::Region
                - Ref: AWS::NoValue
          Secrets:
            - Name: POSTGRES_PASSWORD
              ValueFrom:
                Fn::Sub:
                  - 'arn:${AWS::Partition}:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:${SecretName}:POSTGRES_PASSWORD::'
                  - SecretName:
                      Fn::FindInMap:
                        - secrets
                        - POSTGRESPASSWORD
                        - Name

Expected behavior The ECS TaskDefinition should be created without Environment entries for the Secrets entries.

Logs N/A

Desktop (please complete the following information):

thorfi commented 10 months ago

Hi @JohnPreston - I don't have a fix for this one, but if you are busy and can point me at the right files where the Environment substitution might be happening, I can sort it.

thorfi commented 10 months ago

@JohnPreston Hm, I sent a PR for a bit of a dirty hack - feel free to reject in favour of pointing out a better way to fix :-)

JohnPreston commented 10 months ago

@JohnPreston Hm, I sent a PR for a bit of a dirty hack - feel free to reject in favour of pointing out a better way to fix :-)

Thanks for this again. The issue comes from your secret name POSTGRES_PASSWORD being the same as the env var that you want. To avoid that, from your compose sample, you need only to set VarName: POSTGRES_PASSWORD_ARN and that will change the key of the environment variable

          Environment:
            - Name: POSTGRES_DB
              Value: foo
            - Name: POSTGRES_PASSWORD_ARN
              Value:
                Fn::Sub:
                  - arn:${AWS::Partition}:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:${SecretName}
                  - SecretName:
                      Fn::FindInMap:
                        - secrets
                        - POSTGRESPASSWORD
                        - Name
          Secrets:
            - Name: POSTGRES_PASSWORD
              ValueFrom:
                Fn::Sub:
                  - 'arn:${AWS::Partition}:secretsmanager:${AWS::Region}:${AWS::AccountId}:secret:${SecretName}:POSTGRES_PASSWORD::'
                  - SecretName:
                      Fn::FindInMap:
                        - secrets
                        - POSTGRESPASSWORD
                        - Name
JohnPreston commented 10 months ago

I have long been thinking of automatically updating the value for VarName if already found in the secrets What do you think?

thorfi commented 10 months ago

@JohnPreston Hm, I sent a PR for a bit of a dirty hack - feel free to reject in favour of pointing out a better way to fix :-)

Thanks for this again. The issue comes from your secret name POSTGRES_PASSWORD being the same as the env var that you want. To avoid that, from your compose sample, you need only to set VarName: POSTGRES_PASSWORD_ARN and that will change the key of the environment variable

I don't have POSTGRES_PASSWORD environment set in my docker-compose.yml file at all - it's a duplicate being generated somehow during the ecs-composex render...

thorfi commented 10 months ago

I have long been thinking of automatically updating the value for VarName if already found in the secrets What do you think?

As in, if there is an environment entry that's duplicated by a secrets entry, rename the Environment? I think it's probably a good idea, along with generating a warning message.

thorfi commented 10 months ago

@JohnPreston OK, Environment renaming fix pushed

JohnPreston commented 10 months ago

Thanks @thorfi The POSTGRES_PASSWORD is not defined in your env vars, but it is the name of your secret, which itself becomes an env var, hence why the VarName overrides that.