feat(ecs): support secret environment variables #2994
Conversation
Add a union class to treat environment variable values whether they are given as clear text, from a SSM parameter or a secret. Closes aws#1478 BREAKING CHANGE: `environment` in `ecs.ContainerDefinition` now takes an object whose values are of `ecs.EnvironmentValue` type.
| @@ -65,7 +65,7 @@ export interface LoadBalancedServiceBaseProps { | |||
| * | |||
| * @default - No environment variables. | |||
| */ | |||
| readonly environment?: { [key: string]: string }; | |||
| readonly environment?: { [key: string]: ecs.EnvironmentValue }; | |||
There was a problem hiding this comment.
I feel like we might want to use some token magic here.
FWIW, we should have Tokenized representations of secret values already. Can we not use those in some way?
There was a problem hiding this comment.
For the Secrets property, CF expects the ARN of the secret/ssm param, the container will pull that value at startup and set it as env var avoiding passing secrets in clear text. Secret env vars are retrieved at runtime (always up to date), this is not the case with simple env vars (fixed at deploy time)
The class EnvironmentValue will feed either environment or secrets based on the type. This gives a better user experience (higher level of abstraction) than having to specify environment and/or secrets manually (both will become env vars in the running container at the end).
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ecs-taskdefinition-containerdefinitions.html#cfn-ecs-taskdefinition-containerdefinition-environment
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ecs-taskdefinition-secret.html
The support for the Secrets property was added in the latest release of CF (https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/ReleaseHistory.html).
https://docs.aws.amazon.com/AmazonECS/latest/developerguide/specifying-sensitive-data.html
|
|
|
@eladb build is failing due to breaking changes... what do you suggest here? I think it's nice to have everything expressed as |
|
@eladb @rix0rrr this is breaking. Two solutions here:
The CF support for secret env vars was a highly requested feature (aws/containers-roadmap#97) and should be supported with cdk... |
|
I think the |
| return new Secret({ secret }); | ||
| } | ||
|
|
||
| constructor(public readonly props: SecretProps) {} |
There was a problem hiding this comment.
This is a bit unnecessarily convoluted I think. Why not just accept an “arn” string here? This will remove the need for SecretProps and the special casing in renderContainerDefintion
There was a problem hiding this comment.
How do you suggest granting read (secret.grantRead() or parameter.grantRead()) to the task execution role in renderContainerDefinition? With a .fromArn() there? (there's no way to import from ARN currently in SSM, works for AWS Secrets)
There was a problem hiding this comment.
It is also possible to do taskDefintion.obtainExecutionRole().addToPolicy(...) but I find that duplicating SSM/Secrets read IAM permissions is not a elegant solution...
There was a problem hiding this comment.
Something like that (polymorphism ;-)):
abstract class Secret {
public static fromSecretsManager(secret: ISecret): Secret {
return {
arn: secret.secretArn,
grantRead: grantee => secret.grantRead(grantee)
};
}
public abstract readonly arn: string;
public abstract grantRead(grantee: IGrantee): Grant;
}
packages/@aws-cdk/aws-ecs-patterns/lib/base/queue-processing-service-base.ts
Show resolved
Hide resolved
| return new Secret({ secret }); | ||
| } | ||
|
|
||
| constructor(public readonly props: SecretProps) {} |
There was a problem hiding this comment.
Something like that (polymorphism ;-)):
abstract class Secret {
public static fromSecretsManager(secret: ISecret): Secret {
return {
arn: secret.secretArn,
grantRead: grantee => secret.grantRead(grantee)
};
}
public abstract readonly arn: string;
public abstract grantRead(grantee: IGrantee): Grant;
}
| @@ -29,6 +29,13 @@ export interface QueueProcessingServiceBaseProps extends BaseProps { | |||
| */ | |||
| readonly enableLogging?: boolean; | |||
|
|
|||
| /** | |||
| * The environment variables to pass to the container. | |||
| * | |||
There was a problem hiding this comment.
Is this really the default or will it always be passed?
There was a problem hiding this comment.
This part is not mine, it seems that it's always passed, I can update the doc.
| @@ -1,3 +1,5 @@ | |||
| export * from './base/props'; | |||
There was a problem hiding this comment.
Do we really need this exported? The base interface e doesn’t need to be public
There was a problem hiding this comment.
Can you do this with jsii?
There was a problem hiding this comment.
OK, need to add @internal for this to work.
There was a problem hiding this comment.
What happens if you just not export this file here?
There was a problem hiding this comment.
error TS9999: JSII: Unable to resolve referenced type '@aws-cdk/aws-ecs-patterns.BaseProps'. Type may be @internal or unexported
There was a problem hiding this comment.
ahhh... apologies for the hassle... Let's revert this base struct... On second thought, it does not really make sense for these to be shared between the patterns...
|
Is this ok? Can this be merged? |
Add support for runtime secrets in containers by adding a union class to treat secret environment
variable values whether they are pulled from a SSM parameter or a AWS Secrets secret.
https://docs.aws.amazon.com/AmazonECS/latest/developerguide/specifying-sensitive-data.html
Closes #1478
Pull Request Checklist
designfolderBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.