feat(ecs): Add RepositoryCredentials support #1
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,28 @@ | ||
| import ecr = require('@aws-cdk/aws-ecr'); | ||
| import secretsmanager = require('@aws-cdk/aws-secretsmanager'); | ||
| import cdk = require('@aws-cdk/cdk'); | ||
| import { ContainerDefinition } from './container-definition'; | ||
| import { CfnTaskDefinition } from './ecs.generated'; | ||
|
|
||
| /** | ||
| * Repository Credential resources | ||
| */ | ||
| export interface RepositoryCreds { | ||
|
There was a problem hiding this comment. What are your thoughts for this type? Future proofing? Because it's not strictly necessary today, right? There was a problem hiding this comment. Correct, today we could potentially add a secret to ContainerImage directly but I want to minimize the need for rewiring if other resources or additional fields become valid in the future. Do you foresee issues with this approach? |
||
| /** | ||
| * The secret that contains credentials for the image repository | ||
| */ | ||
| readonly secret: secretsmanager.ISecret; | ||
| } | ||
|
|
||
| /** | ||
| * Constructs for types of container images | ||
| */ | ||
| export abstract class ContainerImage { | ||
| /** | ||
| * Reference an image on DockerHub or another online registry | ||
| */ | ||
| public static fromInternet(name: string, props: InternetHostedImageProps = {}) { | ||
| return new InternetHostedImage(name, props); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -33,12 +44,26 @@ export abstract class ContainerImage { | |
| */ | ||
| public abstract readonly imageName: string; | ||
|
|
||
| /** | ||
| * Optional credentials for a private image registry | ||
| */ | ||
| public abstract readonly credentials?: RepositoryCreds; | ||
|
There was a problem hiding this comment. Does this need to be publicly accessible? Seems like There was a problem hiding this comment.
|
||
|
|
||
| /** | ||
| * Called when the image is used by a ContainerDefinition | ||
| */ | ||
| public abstract bind(containerDefinition: ContainerDefinition): void; | ||
|
|
||
| /** | ||
| * Render the Repository credentials to the CloudFormation object | ||
| */ | ||
| public renderRepositoryCredentials(): CfnTaskDefinition.RepositoryCredentialsProperty { | ||
| return { | ||
| credentialsParameter: this.credentials ? this.credentials.secret.secretArn : '' | ||
|
There was a problem hiding this comment. To avoid breaking all existing integration tests, maybe return undefined for this whole object if we don't have credentials? public renderRepositoryCredentials(): CfnTaskDefinition.RepositoryCredentialsProperty | undefined {
if (!this.credentials) { return undefined; }
return {
credentialsParameter: this.credentials.secret.secretArn
} ;
}There was a problem hiding this comment. And in fact, this method could be There was a problem hiding this comment. Well the others would need to do a base implementation of There was a problem hiding this comment.
Ah, the conditional here can probably be removed b/c this func should only be called if
I tried returning There was a problem hiding this comment.
It's hard to see the change in the code I pasted because it scrolls off the side of the comment box, but you should have written the return type as: CfnTaskDefinition.RepositoryCredentialsProperty | undefinedThat would have allowed you to return There was a problem hiding this comment. Ah, I totally missed that, thanks for pointing it out! In that case I'll update it per the above. |
||
| }; | ||
| } | ||
| } | ||
|
|
||
| import { AssetImage, AssetImageProps } from './images/asset-image'; | ||
| import { EcrImage } from './images/ecr'; | ||
| import { InternetHostedImage, InternetHostedImageProps } from './images/internet-hosted'; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import secretsmanager = require('@aws-cdk/aws-secretsmanager'); | ||
| import { ContainerDefinition } from "../container-definition"; | ||
| import { ContainerImage, RepositoryCreds } from "../container-image"; | ||
|
|
||
| export interface InternetHostedImageProps { | ||
| /** | ||
| * Optional secret that houses credentials for the image registry | ||
| */ | ||
| credentials?: secretsmanager.ISecret; | ||
| } | ||
|
|
||
| /** | ||
| * A container image hosted on DockerHub or another online registry | ||
| */ | ||
| export class InternetHostedImage extends ContainerImage { | ||
| public readonly imageName: string; | ||
| public readonly credentials?: RepositoryCreds; | ||
|
|
||
| constructor(imageName: string, props: InternetHostedImageProps = {}) { | ||
| super(); | ||
| this.imageName = imageName; | ||
|
|
||
| if (props.credentials !== undefined) { | ||
| this.credentials = { secret: props.credentials }; | ||
| } | ||
| } | ||
|
|
||
| public bind(containerDefinition: ContainerDefinition): void { | ||
| if (this.credentials !== undefined) { | ||
| this.credentials.secret.grantRead(containerDefinition.taskDefinition.obtainExecutionRole()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't write it like this, this isn't copy/pasteable user code. Just add a new line below to do something like:
ecs.ContainerImage.fromInternet(imageName, { credentials: mySecret }): use a private image that needs credentials.