Address comments

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

@@ -14,46 +14,40 @@ The [`ecs.ContainerImage`](https://github.com/awslabs/aws-cdk/blob/master/packag
 
 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 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.
+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.
 
-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.
+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. 
 
-We should also have a unified way to define repository credentials which can be passed to the constructors of both image types.  
+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 additions to support private registry authentication:
+Given the above, we should make the following changes to support private registry authentication:
 
 1. Define `RepositoryCredentials` interface & class, add to `IContainerImage` 
-2. Update `DockerHubImage` construct to optionally accept and set `RepositoryCreds` 
-3. Add new `PrivateImage` class which implements `IContainerImage` and requires `RepositoryCreds`
+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 with a "credentialsParamater" string and a new `RepositoryCreds` class which satisfies it using specific constructs (e.g., "fromSecret").
+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 {
-    readonly credentialsParameter: string
+    readonly secret: secretsManager.Secret;
 }
 
 export class RepositoryCreds {
-    public readonly credentialsParameter: string;
-    private readonly secret: secretsManager.Secret;
+  public readonly secret: secretsManager.Secret;
 
-    public static fromSecret(secret: secretsManager.Secret) {
-      // sets credentialsParameter from secret.secretArn 
-    }
-
-    public static withCredentialsParameter(param: string) {
-      // sets credentialsParameter as provided string
-    }
+  public static fromSecret(secret: secretsManager.Secret) {
+    this.secret = secret;
+  }
 
-    public bind(containerDefinition: ContainerDefinition): void {
-      if (this.secret !== null) {
-        this.secret.grantRead(containerDefinition.taskDefinition.obtainExecutionRole());
-      }
+  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());
   }
 }
 ```
@@ -75,56 +69,39 @@ export interface IContainerImage {
 ```
 
 
-## Part 2: Extend `DockerHubImage` class to include optional creds
+# Part 2: Rename `DockerHubImage` class to `WebHostedImage`, add optional creds
 
-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:
 ```ts
-export class DockerHubImage implements IContainerImage {
-  // add repositoryCredentials to constructor
-  constructor(public readonly imageName: string, public readonly repositoryCredentials: RepositoryCreds) {
-  }
-
-  public bind(_containerDefinition: ContainerDefinition): void {
-    // Nothing to do
-  }
+// define props
+export interface WebHostedImageProps {
+    credentials: RepositoryCreds;
 }
-```
-
-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.fromDockerHub('userx/test', ecs.RepositoryCreds.fromSecret(secret));
-});
+// "DockerHubImage" class --> "WebHostedImage"
+export class WebHostedImage implements IContainerImage {
+  public readonly imageName: string;
+  public readonly credentials: IRepositoryCreds;
 
-```
-
-
-## Part 3: Add `fromPrivateRepository` method on `ContainerImage`, return new `PrivateImage` subclass
-
-For non-DockerHub privately hosted images, we can add a new subclass:
-```ts
-export class PrivateImage implements IContainerImage {
-    constructor(public readonly image: string, public readonly repositoryCredentials: RepositoryCreds) {
+  // add credentials to constructor
+  constructor(imageName: string, props: WebHostedImageProps) {
+    this.imageName = imageName
+    this.credentials = props.credentials
   }
 
   public bind(_containerDefinition: ContainerDefinition): void {
-    // Nothing to do
+    // bind repositoryCredentials to ContainerDefinition
+    this.repositoryCredentials.bind();
   }
 }
 ```
 
-This will be returned by a new API on `ContainerImage`:
+We will also update the API on `ContainerImage` to match:
 ```ts
 export class ContainerImage {
   //...
-  public static fromPrivateRepository(imageUri: string, creds: RepositoryCreds) {
-    return new PrivateImage(imageUri, creds);
+  public static fromInternet(imageName: string, props: WebHostedImageProps) {
+    return new WebHostedImage(imageName, props);
   }
   // ...
 
@@ -140,7 +117,9 @@ const secret = secretsManager.Secret.import(stack, 'myRepoSecret', {
 })
 
 taskDefinition.AddContainer('myPrivateContainer', {
-  image: ecs.ContainerImage.fromPrivateRepository('myrepo.net/test:latest', ecs.RepositoryCreds.fromSecret(secret));
+  image: ecs.ContainerImage.fromInternet('userx/test', {
+      credentials: ecs.RepositoryCreds.fromSecret(secret)
+  });
 });
 
 ```