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

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

Closed
thorfi opened this issue Nov 13, 2023 · 8 comments · Fixed by #710
Closed

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

thorfi opened this issue Nov 13, 2023 · 8 comments · Fixed by #710
Assignees
Labels
bug Something isn't working

Comments

@thorfi
Copy link
Contributor

thorfi commented Nov 13, 2023


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 thorfi added the bug Something isn't working label Nov 13, 2023
@thorfi
Copy link
Contributor Author

thorfi commented Nov 13, 2023

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
Copy link
Contributor Author

thorfi commented Nov 13, 2023

@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 :-)

thorfi added a commit to thorfi/ecs_composex that referenced this issue Nov 13, 2023
@JohnPreston
Copy link
Member

@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
Copy link
Member

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

@thorfi
Copy link
Contributor Author

thorfi commented Nov 13, 2023

@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
Copy link
Contributor Author

thorfi commented Nov 13, 2023

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 added a commit to thorfi/ecs_composex that referenced this issue Nov 14, 2023
thorfi added a commit to thorfi/ecs_composex that referenced this issue Nov 14, 2023
@thorfi
Copy link
Contributor Author

thorfi commented Nov 14, 2023

@JohnPreston OK, Environment renaming fix pushed

@JohnPreston
Copy link
Member

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.

@JohnPreston JohnPreston linked a pull request Nov 14, 2023 that will close this issue
thorfi added a commit to thorfi/ecs_composex that referenced this issue Nov 20, 2023
thorfi added a commit to thorfi/ecs_composex that referenced this issue Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants