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

ECS private registry support design #1737

Merged
merged 12 commits into from
Mar 13, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
125 changes: 125 additions & 0 deletions design/aws-ecs-priv-registry-support.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
# AWS ECS - Support for Private Registry Authentication
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved

To address issue [#1698](https://github.com/awslabs/aws-cdk/issues/1698), the ECS construct library should provide a way for customers to specify [`repositoryCredentials`](https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_ContainerDefinition.html#ECS-Type-ContainerDefinition-repositoryCredentials) on their container.

Minimally, this would mean adding a new string field on `ContainerDefinition`, however this doesn't provide any added value in terms of logical grouping or resource creation. We can instead modify the existing ECS CDK construct [`ContainerImage`](https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-ecs/lib/container-image.ts) so that repository credentials are specified along with the image they're meant to access.
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved

## General approach

The [`ecs.ContainerImage`](https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-ecs/lib/container-image.ts) class already includes constructs for 3 types of images:

* DockerHubImage
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
* EcrImage
* AssetImage

DockerHub images are assumed public, however DockerHub also provides private repositories and there's currently no way to specify credentials for them in the CDK.

There's also no explicit way to specify images hosted outside of DockerHub, AWS, or your local machine. Customers hosting their own registries or using another registry host, like Quay.io or JFrog Artifactory, would need to be able to specify both the image URI and the registry credentials in order to pull their images down for ECS tasks.

Fundamentally, specifying images hosted in DockerHub or elsewhere works the same, because when passed an image URI vs. a plain (or namespaced) image name + tag, the Docker daemon does the right thing and tries to pull the image from the specified registery.

Therefore, we should rename the existing `DockerHubImage` type be more generic and add the ability to optionally specify credentials.


## Code changes

Given the above, we should make the following changes to support private registry authentication:

1. Define `RepositoryCredentials` interface & class, add to `IContainerImage`
2. Rename `DockerHubImage` construct to be more generic, optionally accept and set `RepositoryCreds`


# Part 1: How to define registry credentials

For extensibility, we can define a new `IRepositoryCreds` interface to house the AWS Secrets Manager secret with the creds and a new `RepositoryCreds` class which satisfies it using specific constructs (e.g., "fromSecret").

```ts
export interface IRepositoryCreds {
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
readonly secret: secretsManager.Secret;
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
}

export class RepositoryCreds {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would get rid of this class and just call the secret grant directly in ContainerImage.

public readonly secret: secretsManager.Secret;

public static fromSecret(secret: secretsManager.Secret) {
this.secret = secret;
}

public bind(containerDefinition: ContainerDefinition): void {
// grant the execution role read access so the secret can be read prior to image pull
this.secret.grantRead(containerDefinition.taskDefinition.obtainExecutionRole());
}
}
```

The `IContainerImage` interface will be updated to include an optional `repositoryCredentials` property:
```ts
export interface IContainerImage {
// ...
readonly imageName: string;

/**
* NEW: The credentials required to access the image
*/
readonly repositoryCredentials?: IRepositoryCreds;

// ...
bind(containerDefinition: ContainerDefinition): void;
}
```


# Part 2: Rename `DockerHubImage` class to `WebHostedImage`, add optional creds

The `DockerHubImage` construct will be renamed to `WebHostedImage`, and augmented to take in optional "credentials" via keyword props:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind this rename at all, but being the pedant that I am, is "Web" the right term to use? It doesn't really have anything to do with a linked network of HTML pages... InternetHostedImage would be a more correct term.

Not saying better, just more correct.

```ts
// define props
export interface WebHostedImageProps {
credentials: RepositoryCreds;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they default to being optional (with a ?)

}

// "DockerHubImage" class --> "WebHostedImage"
export class WebHostedImage implements IContainerImage {
public readonly imageName: string;
public readonly credentials: IRepositoryCreds;

// add credentials to constructor
constructor(imageName: string, props: WebHostedImageProps) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since all properties will be optional, default to props: WebHostedImageProps = {}.

this.imageName = imageName
this.credentials = props.credentials
}

public bind(_containerDefinition: ContainerDefinition): void {
// bind repositoryCredentials to ContainerDefinition
this.repositoryCredentials.bind();
}
}
```

We will also update the API on `ContainerImage` to match:
```ts
export class ContainerImage {
//...
public static fromInternet(imageName: string, props: WebHostedImageProps) {
return new WebHostedImage(imageName, props);
}
// ...

}
```

Example use:
```ts
const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'TaskDef');

const secret = secretsManager.Secret.import(stack, 'myRepoSecret', {
secretArn: 'arn:aws:secretsmanager:.....'
})

taskDefinition.AddContainer('myPrivateContainer', {
image: ecs.ContainerImage.fromInternet('userx/test', {
credentials: ecs.RepositoryCreds.fromSecret(secret)
});
});

```
10 changes: 5 additions & 5 deletions packages/@aws-cdk/aws-ecs/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ cluster.addDefaultAutoScalingGroupCapacity('Capacity', {
const ecsService = new ecs.LoadBalancedEc2Service(this, 'Service', {
cluster,
memoryLimitMiB: 512,
image: ecs.ContainerImage.fromDockerHub("amazon/amazon-ecs-sample"),
image: ecs.ContainerImage.fromRegistry("amazon/amazon-ecs-sample"),
});
```

Expand Down Expand Up @@ -134,7 +134,7 @@ To add containers to a task definition, call `addContainer()`:
```ts
const container = fargateTaskDefinition.addContainer("WebContainer", {
// Use an image from DockerHub
image: ecs.ContainerImage.fromDockerHub("amazon/amazon-ecs-sample"),
image: ecs.ContainerImage.fromRegistry("amazon/amazon-ecs-sample"),
// ... other options here ...
});
```
Expand All @@ -148,7 +148,7 @@ const ec2TaskDefinition = new ecs.Ec2TaskDefinition(this, 'TaskDef', {

const container = ec2TaskDefinition.addContainer("WebContainer", {
// Use an image from DockerHub
image: ecs.ContainerImage.fromDockerHub("amazon/amazon-ecs-sample"),
image: ecs.ContainerImage.fromRegistry("amazon/amazon-ecs-sample"),
memoryLimitMiB: 1024
// ... other options here ...
});
Expand Down Expand Up @@ -183,8 +183,8 @@ const taskDefinition = new ecs.TaskDefinition(this, 'TaskDef', {
Images supply the software that runs inside the container. Images can be
obtained from either DockerHub or from ECR repositories, or built directly from a local Dockerfile.

* `ecs.ContainerImage.fromDockerHub(imageName)`: use a publicly available image from
DockerHub.
* `ecs.ContainerImage.fromRegistry(imageName)`: use a public image.
* `ecs.ContainerImage.fromRegistry(imageName, { credentials: mySecret })`: use a private image that requires credentials.
* `ecs.ContainerImage.fromEcrRepository(repo, tag)`: use the given ECR repository as the image
to start. If no tag is provided, "latest" is assumed.
* `ecs.ContainerImage.fromAsset(this, 'Image', { directory: './image' })`: build and upload an
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-ecs/lib/container-definition.ts
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ export class ContainerDefinition extends cdk.Construct {
portMappings: this.portMappings.map(renderPortMapping),
privileged: this.props.privileged,
readonlyRootFilesystem: this.props.readonlyRootFilesystem,
repositoryCredentials: undefined, // FIXME
repositoryCredentials: this.props.image.toRepositoryCredentialsJson(),
ulimits: this.ulimits.map(renderUlimit),
user: this.props.user,
volumesFrom: this.volumesFrom.map(renderVolumeFrom),
Expand Down
15 changes: 10 additions & 5 deletions packages/@aws-cdk/aws-ecs/lib/container-image.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
import ecr = require('@aws-cdk/aws-ecr');
import cdk = require('@aws-cdk/cdk');

import { ContainerDefinition } from './container-definition';
import { CfnTaskDefinition } from './ecs.generated';

/**
* Constructs for types of container images
*/
export abstract class ContainerImage {
/**
* Reference an image on DockerHub
* Reference an image on DockerHub or another online registry
*/
public static fromDockerHub(name: string) {
return new DockerHubImage(name);
public static fromRegistry(name: string, props: RepositoryImageProps = {}) {
return new RepositoryImage(name, props);
}

/**
Expand All @@ -37,8 +37,13 @@ export abstract class ContainerImage {
* Called when the image is used by a ContainerDefinition
*/
public abstract bind(containerDefinition: ContainerDefinition): void;

/**
* Render the Repository credentials to the CloudFormation object
*/
public abstract toRepositoryCredentialsJson(): CfnTaskDefinition.RepositoryCredentialsProperty | undefined;
}

import { AssetImage, AssetImageProps } from './images/asset-image';
import { DockerHubImage } from './images/dockerhub';
import { EcrImage } from './images/ecr';
import { RepositoryImage, RepositoryImageProps } from './images/repository';
6 changes: 6 additions & 0 deletions packages/@aws-cdk/aws-ecs/lib/images/asset-image.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { DockerImageAsset } from '@aws-cdk/assets-docker';
import cdk = require('@aws-cdk/cdk');
import { ContainerDefinition } from '../container-definition';
import { ContainerImage } from '../container-image';
import { CfnTaskDefinition } from '../ecs.generated';

export interface AssetImageProps {
/**
Expand All @@ -15,6 +16,7 @@ export interface AssetImageProps {
*/
export class AssetImage extends ContainerImage {
private readonly asset: DockerImageAsset;

constructor(scope: cdk.Construct, id: string, props: AssetImageProps) {
super();
this.asset = new DockerImageAsset(scope, id, { directory: props.directory });
Expand All @@ -24,6 +26,10 @@ export class AssetImage extends ContainerImage {
this.asset.repository.grantPull(containerDefinition.taskDefinition.obtainExecutionRole());
}

public toRepositoryCredentialsJson(): CfnTaskDefinition.RepositoryCredentialsProperty | undefined {
return undefined;
}

public get imageName() {
return this.asset.imageUri;
}
Expand Down
15 changes: 0 additions & 15 deletions packages/@aws-cdk/aws-ecs/lib/images/dockerhub.ts

This file was deleted.

5 changes: 5 additions & 0 deletions packages/@aws-cdk/aws-ecs/lib/images/ecr.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import ecr = require('@aws-cdk/aws-ecr');
import { ContainerDefinition } from '../container-definition';
import { ContainerImage } from '../container-image';
import { CfnTaskDefinition } from '../ecs.generated';

/**
* An image from an ECR repository
Expand All @@ -18,4 +19,8 @@ export class EcrImage extends ContainerImage {
public bind(containerDefinition: ContainerDefinition): void {
this.repository.grantPull(containerDefinition.taskDefinition.obtainExecutionRole());
}

public toRepositoryCredentialsJson(): CfnTaskDefinition.RepositoryCredentialsProperty | undefined {
return undefined;
}
}
39 changes: 39 additions & 0 deletions packages/@aws-cdk/aws-ecs/lib/images/repository.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import secretsmanager = require('@aws-cdk/aws-secretsmanager');
import { ContainerDefinition } from "../container-definition";
import { ContainerImage } from "../container-image";
import { CfnTaskDefinition } from '../ecs.generated';

export interface RepositoryImageProps {
/**
* Optional secret that houses credentials for the image registry
*/
credentials?: secretsmanager.ISecret;
}

/**
* A container image hosted on DockerHub or another online registry
*/
export class RepositoryImage extends ContainerImage {
public readonly imageName: string;

private credentialsSecret?: secretsmanager.ISecret;

constructor(imageName: string, props: RepositoryImageProps = {}) {
super();
this.imageName = imageName;
this.credentialsSecret = props.credentials;
}

public bind(containerDefinition: ContainerDefinition): void {
if (this.credentialsSecret) {
this.credentialsSecret.grantRead(containerDefinition.taskDefinition.obtainExecutionRole());
}
}

public toRepositoryCredentialsJson(): CfnTaskDefinition.RepositoryCredentialsProperty | undefined {
if (!this.credentialsSecret) { return undefined; }
return {
credentialsParameter: this.credentialsSecret.secretArn
};
}
}
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-ecs/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ export * from './load-balanced-ecs-service';
export * from './load-balanced-fargate-service-applet';

export * from './images/asset-image';
export * from './images/dockerhub';
export * from './images/repository';
export * from './images/ecr';

export * from './log-drivers/aws-log-driver';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ export class LoadBalancedFargateServiceApplet extends cdk.Stack {
memoryMiB: props.memoryMiB,
publicLoadBalancer: props.publicLoadBalancer,
publicTasks: props.publicTasks,
image: ContainerImage.fromDockerHub(props.image),
image: ContainerImage.fromRegistry(props.image),
desiredCount: props.desiredCount,
environment: props.environment,
certificate,
Expand Down
2 changes: 2 additions & 0 deletions packages/@aws-cdk/aws-ecs/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
"@aws-cdk/aws-logs": "^0.25.3",
"@aws-cdk/aws-route53": "^0.25.3",
"@aws-cdk/aws-sns": "^0.25.3",
"@aws-cdk/aws-secretsmanager": "^0.25.3",
"@aws-cdk/cdk": "^0.25.3",
"@aws-cdk/cx-api": "^0.25.3"
},
Expand All @@ -97,6 +98,7 @@
"@aws-cdk/aws-iam": "^0.25.3",
"@aws-cdk/aws-logs": "^0.25.3",
"@aws-cdk/aws-route53": "^0.25.3",
"@aws-cdk/aws-secretsmanager": "^0.25.3",
"@aws-cdk/cdk": "^0.25.3"
},
"engines": {
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-awsvpc-nw.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'TaskDef', {
});

const container = taskDefinition.addContainer('web', {
image: ecs.ContainerImage.fromDockerHub("amazon/amazon-ecs-sample"),
image: ecs.ContainerImage.fromRegistry("amazon/amazon-ecs-sample"),
memoryLimitMiB: 256,
});

Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-ecs/test/ec2/integ.lb-bridge-nw.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'TaskDef', {
});

const container = taskDefinition.addContainer('web', {
image: ecs.ContainerImage.fromDockerHub("amazon/amazon-ecs-sample"),
image: ecs.ContainerImage.fromRegistry("amazon/amazon-ecs-sample"),
memoryLimitMiB: 256,
});
container.addPortMappings({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export = {

const taskDefinition = new ecs.Ec2TaskDefinition(stack, 'TaskDef');
taskDefinition.addContainer('TheContainer', {
image: ecs.ContainerImage.fromDockerHub('henk'),
image: ecs.ContainerImage.fromRegistry('henk'),
memoryLimitMiB: 256
});

Expand Down
Loading