ECS private registry support design #1737

allisaurus · 2019-02-12T00:52:34Z

Pull Request Checklist

Testing
- N/A, design doc
Docs
- Documents feature design
Title and Description
- "feat" prefix
- addresses ECS: Support private registry authentication #1698
Sensitive Modules (requires 2 PR approvers)
- N/A

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

design/aws-ecs-priv-registry-support.md

rix0rrr · 2019-02-12T10:35:44Z

design/aws-ecs-priv-registry-support.md

+
+There's also no 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.
+
+Therefore, to support the use of private registries we would need to add credentials to the `DockerHubImage` type (as optional), and add a new `PrivateImage` type which could house credentials (required) and the private registry URI.


I have a tingling spidey sense that says we keep those 2 things in the same class, since it feels to me that they don't differ that much.

Their behavior seems like it would be much the same, only differing in the presence of a 'registry' field (which could even be encoded into the imageName, though I'm not sure that it should).

The EcrImage is notably different in that it also sets up IAM permissions, neither DockerHubImage nor PrivateImage would do that.

rix0rrr · 2019-02-12T10:39:40Z

design/aws-ecs-priv-registry-support.md

+
+```ts
+export interface IRepositoryCreds {
+    readonly credentialsParameter: string


How do private registry credentials work? Do they have to be a SecretsManager secret with predefined JSON entries (probably username and password?)

Right now, yes. (Though in the future maybe it would support different cred sources? e.g., SSM Param store.)

I think for simplicity's sake I'd still prefer to just take a secretsmanager.SecretString for now. Let's not overdesign now for a feature that may never come.

In case it's ambiguous above: ECS expects this field to contain an AWS Secrets Manager ARN or secret name (if within the same region as the stack), not the secret value itself.

rix0rrr · 2019-02-12T10:45:46Z

design/aws-ecs-priv-registry-support.md

+      // sets credentialsParameter from secret.secretArn 
+    }
+
+    public static withCredentialsParameter(param: string) {


Are these literal credentials, or the name of a secret?

If it's the latter, there's no need to make this distinction here. There is a secretsmanager.SecretString which can be constructed from a Secret object (a secret that gets deployed in the same CDK application) or constructed by hand from a secret that's been prepopulated into the AWS account. In that case, I think the whole RepositoryCreds objects can be replaced with a SecretString object.

With permissions and everything I agree that the existing API surface there is clunky, but I'm all in favor of simplifying, making the existing objects richer if we can, so that we need fewer classes. I don't want to go into too much design work here in case I'm completely misunderstanding the problem, so I'll shut up now, but get back to me if you want to discuss more.

The thought here was this is an escape hatch if someone has the ARN they want to set but don't want to construct a separate Secret construct (or maybe in the future if it supports diff kinds of cred resources, this would still work w/o needing to be updated to accommodate a resource type).

Another diff is that fromSecret would add "read" access to the Secret to the execution role, while this one would not. But maybe adding it now is jumping the gun?

Re: using SecretString : I steered away from it based on the package REAMDE warning that using it may lead to the creds surfacing in plain text, but maybe I misunderstood when exactly that could happen.

This is an unfortunate case of the docs being too informative and thereby confusing.

They are talking about 2 different SecretStrings, btw. The docs are talking about the
SecretString attribute to create a Secret, but the SecretString I'm talking about is for refencing an existing (or created) secret.

What the docs are trying to tell you is that creating a Secret via CloudFormation only allows it to be populated by a randomly generated password, because doing otherwise would mean putting the literal secret value you want to put into into the Secret somewhere in your CloudFormation template... defeating the purpose of it being a secret. A randomly generated secret can still be useful for setting the admin login of an RDS instance, where you need some secret but you don't really care what the value is.

For our purposes, people are going to have to manually prepopulate a secret into their account, which we can then read using the SecretString class. It's not documented in that README, which is a shame, but it's documented here: https://integ-docs-aws.amazon.com/CDK/latest/userguide/how_to_get_ext_values.html#passing_secrets_manager

What the docs are trying to tell you is that creating a Secret via CloudFormation only allows it to be populated by a randomly generated password

But you can also import an existing secret (example in README) into a Secret construct, which is the use case I was considering most common.

For our purposes, people are going to have to manually prepopulate a secret into their account

Do you mean into AWS Secrets Manager, or the 3rd party account that houses the registry? If the former, what makes SecretString more appropriate here?

To clarify: ECS is expecting the actual ARN of the credential secret here, not the secret value itself. (Apologies if a previous response was misleading.)

I was expecting that.

I mean users populate their own Secret, then import into CDK using a name or ARN.

How the credentials are fed into the actual registry is out of scope I think :) (unless it's an ECR registry, in which case it could be an autogenerated Secret)

Per offline discussion: agreed that a Secret construct is appropriate here.

rix0rrr · 2019-02-12T10:48:53Z

design/aws-ecs-priv-registry-support.md

+```ts
+export class DockerHubImage implements IContainerImage {
+  // add repositoryCredentials to constructor
+  constructor(public readonly imageName: string, public readonly repositoryCredentials: RepositoryCreds) {


I think we're settling on keyword arguments as much as we can for "complex configuration" things, even if it's only for one parameter.

So that would mean defining something like:

export interface DockerHubImageProps { credentials: RepositoryCreds; } // Take as second argument // To be used as: taskDefinition.AddContainer('myPrivateContainer', { image: ecs.ContainerImage.fromDockerHub('userx/test', { credentials: ecs.RepositoryCreds.fromSecret(secret) }) });

rix0rrr · 2019-02-12T10:50:18Z

design/aws-ecs-priv-registry-support.md

+
+For non-DockerHub privately hosted images, we can add a new subclass:
+```ts
+export class PrivateImage implements IContainerImage {


This confirms my suspicion that PrivateImage looks a whole lot like DockerHubImage :)

rix0rrr · 2019-02-12T10:51:00Z

design/aws-ecs-priv-registry-support.md

+  }
+
+  public bind(_containerDefinition: ContainerDefinition): void {
+    // Nothing to do


Nothing to do is no longer true, right? We should be calling this.repositoryCredentials.bind() in here if I'm understanding correctly?

You're right; I have bind() defined on RepositoryCreds but I suppose it needs to be actually called at some point :P

rix0rrr · 2019-02-12T10:52:00Z

Oh oops. Didn't mean to approve yet :)

allisaurus · 2019-02-20T03:29:27Z

@rix0rrr @SoManyHs Updated doc to reflect feedback. Please let me know if you have any additional comments (including new name for "DockerHubImage"). Thanks!

rix0rrr

I think this all makes perfect sense.

Let's get to implementing :)

design/aws-ecs-priv-registry-support.md

rix0rrr · 2019-02-20T10:12:31Z

design/aws-ecs-priv-registry-support.md


 ```ts
 export interface IRepositoryCreds {
-    readonly credentialsParameter: string
+    readonly secret: secretsManager.Secret;
 }

 export class RepositoryCreds {


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

rix0rrr · 2019-02-20T10:14:49Z

design/aws-ecs-priv-registry-support.md


-The `DockerHubImage` construct, which currently takes a string for image name, can be extended to take in an (optional) "repositoryCredentials" field of type `IRepositoryCreds`:
+The `DockerHubImage` construct will be renamed to `WebHostedImage`, and augmented to take in optional "credentials" via keyword props:


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.

rix0rrr · 2019-02-20T10:15:20Z

design/aws-ecs-priv-registry-support.md

-  }
+// define props
+export interface WebHostedImageProps {
+    credentials: RepositoryCreds;


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

rix0rrr · 2019-02-20T10:15:37Z

design/aws-ecs-priv-registry-support.md

-export class PrivateImage implements IContainerImage {
-    constructor(public readonly image: string, public readonly repositoryCredentials: RepositoryCreds) {
+  // add credentials to constructor
+  constructor(imageName: string, props: WebHostedImageProps) {


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

allisaurus · 2019-03-08T01:22:13Z

@rix0rrr updated per feedback re: abstracting render func & limiting credendials property access.

rix0rrr

Approved modulo build failures

rix0rrr · 2019-03-08T11:25:03Z

packages/@aws-cdk/aws-ecs/lib/images/asset-image.ts

@@ -24,6 +26,10 @@ export class AssetImage extends ContainerImage {
    this.asset.repository.grantPull(containerDefinition.taskDefinition.obtainExecutionRole());
  }

+  public renderRepositoryCredentials(): undefined {


jsii should give you an error here because you changed the return type of this function while overriding it. C# does not allow narrowing the return type, so we have to disallow that everywhere as well.

You need to declare the return type as CfnTaskDefinition.RepositoryCredentialsProperty | undefined, even though we could give a narrower type.

rix0rrr · 2019-03-08T11:25:47Z

packages/@aws-cdk/aws-ecs/lib/images/ecr.ts

@@ -18,4 +19,8 @@ export class EcrImage extends ContainerImage {
  public bind(containerDefinition: ContainerDefinition): void {
    this.repository.grantPull(containerDefinition.taskDefinition.obtainExecutionRole());
  }
+
+  public renderRepositoryCredentials(): undefined {


eladb

See some renames...

eladb · 2019-03-13T06:55:48Z

packages/@aws-cdk/aws-ecs/README.md

@@ -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.fromInternet("amazon/amazon-ecs-sample"),


How about we call this fromRegistry ("registry" is the official term in the dockerverse)

ECR is also a registry but we don't want to include that. But maybe that ambiguity is okay.

I think it is okay, especially given we will have both “fromEcr” and “fromRegistry” next to each other which disambiguates

eladb · 2019-03-13T06:56:29Z

packages/@aws-cdk/aws-ecs/lib/container-image.ts

-  public static fromDockerHub(name: string) {
-    return new DockerHubImage(name);
+  public static fromInternet(name: string, props: InternetHostedImageProps = {}) {
+    return new InternetHostedImage(name, props);


RegistryImage

eladb · 2019-03-13T06:57:00Z

packages/@aws-cdk/aws-ecs/lib/container-image.ts

+/**
+ * Repository Credential resources
+ */
+export interface RepositoryCreds {


I am wondering why we need this interface which adds another unneeded indirection instead of flat:

ContainerImage.fromRegistry('name', { credentialsSecret: secret });

That is in fact how it works. This interface is not used for input, it's for output.

eladb · 2019-03-13T06:57:34Z

packages/@aws-cdk/aws-ecs/lib/container-image.ts

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


Usually we call these methods toRepositoryCredentialsJson (JSON indicates it's low-level)

rix0rrr · 2019-03-13T09:23:50Z

packages/@aws-cdk/aws-ecs/lib/container-image.ts

+  /**
+   * Optional credentials for a private image registry
+   */
+  protected abstract readonly credentials?: RepositoryCreds;


But you know what... this propery can probably be removed, no? I don't think it's used anywhere?

(replying for context) You're right, it's a holdover from when we were checking it directly from ContainerDefinition to make a decision re: calling toRepositoryCredentialsJson, but once we moved to using multiple return types this check & field became unnecessary.

…type

allisaurus requested a review from a team as a code owner February 12, 2019 00:52

SoManyHs self-requested a review February 12, 2019 00:54

SoManyHs added the @aws-cdk/aws-ecs Related to Amazon Elastic Container label Feb 12, 2019

rix0rrr approved these changes Feb 12, 2019

View reviewed changes

rix0rrr self-assigned this Feb 18, 2019

allisaurus force-pushed the ecs-private-registry branch from f8f430c to 1b461da Compare February 20, 2019 03:27

rix0rrr reviewed Feb 20, 2019

View reviewed changes

allisaurus mentioned this pull request Mar 1, 2019

feat(ecs): Add RepositoryCredentials support allisaurus/aws-cdk#1

Closed

4 tasks

allisaurus force-pushed the ecs-private-registry branch from 1b461da to 10d4c1c Compare March 4, 2019 21:03

allisaurus and others added 6 commits March 7, 2019 17:07

feat(ecs): proposed design for ECS private registry auth

08980f5

feat(ecs): Add RepositoryCredentials support [WIP]

9410746

Rename DockerHubImage to InternetHostedImage

f2f94a8

render RepositoryCredentials only if populated

d7674ec

Update comments/docs

6ede80e

Abstract renderRepositoryCredentials() func

7877598

allisaurus force-pushed the ecs-private-registry branch from 78f47a1 to 7877598 Compare March 8, 2019 01:15

rix0rrr approved these changes Mar 8, 2019

View reviewed changes

allisaurus and others added 3 commits March 8, 2019 08:57

Update return type for renderRepositoryCreds() in ecr and asset images

f2ed804

Merge branch 'master' into ecs-private-registry

7c2d578

Remove 'fromDockerHub' from decdk ecs example

7c61d80

eladb reviewed Mar 13, 2019

View reviewed changes

rix0rrr reviewed Mar 13, 2019

View reviewed changes

rix0rrr and others added 3 commits March 13, 2019 15:48

Apply suggested renames, remove unused declaration and corresponding …

3c969fd

…type

Merge branch 'master' into ecs-private-registry

aee4606

Rename 'fromRepository' to 'fromRegistry'

c6f63a4

rix0rrr merged commit 11ed691 into aws:master Mar 13, 2019

snyk-bot mentioned this pull request Dec 12, 2019

[Snyk] Upgrade @aws-cdk/cdk from 0.22.0 to 0.35.0 MechanicalRock/scdk#3

Closed

This was referenced Dec 12, 2019

[Snyk] Upgrade @aws-cdk/cdk from 0.24.1 to 0.35.0 MechanicalRock/cdk-constructs#8

Open

[Snyk] Upgrade @aws-cdk/cdk from 0.24.1 to 0.35.0 MechanicalRock/account-reaper#6

Open


		There's also no 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.

		Therefore, to support the use of private registries we would need to add credentials to the `DockerHubImage` type (as optional), and add a new `PrivateImage` type which could house credentials (required) and the private registry URI.


		The `DockerHubImage` construct, which currently takes a string for image name, can be extended to take in an (optional) "repositoryCredentials" field of type `IRepositoryCreds`:
		The `DockerHubImage` construct will be renamed to `WebHostedImage`, and augmented to take in optional "credentials" via keyword props:

ECS private registry support design #1737

ECS private registry support design #1737

Conversation

allisaurus commented Feb 12, 2019

Pull Request Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Feb 12, 2019

allisaurus commented Feb 20, 2019

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisaurus commented Mar 8, 2019

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisaurus Mar 13, 2019 • edited Loading

Choose a reason for hiding this comment

allisaurus Mar 13, 2019 •

edited

Loading