chore(cloudfront): small refactoring of the Origin API

packages/@aws-cdk/aws-cloudfront-origins/lib/s3-origin.ts

@@ -26,9 +26,8 @@ export interface S3OriginProps {
  *
  * @experimental
  */
-export class S3Origin extends cloudfront.Origin {
-
-  private readonly origin: cloudfront.Origin;
+export class S3Origin implements cloudfront.IOrigin {
+  private readonly origin: cloudfront.IOrigin;
 
   constructor(bucket: s3.IBucket, props: S3OriginProps = {}) {
     let proxyOrigin;
@@ -43,22 +42,10 @@ export class S3Origin extends cloudfront.Origin {
         ...props,
       });
     }
-
-    super(proxyOrigin.domainName);
-
     this.origin = proxyOrigin;
   }
 
-  public get id() {
-    return this.origin.id;
-  }
-
-  public bind(scope: cdk.Construct, options: cloudfront.OriginBindOptions) {
-    this.origin.bind(scope, options);
-  }
-
-  public renderOrigin() {
-    return this.origin.renderOrigin();
+  public bind(scope: cdk.Construct, options: cloudfront.OriginBindOptions): cloudfront.OriginBindConfig {
+    return this.origin.bind(scope, options);
   }
-
 }

packages/@aws-cdk/aws-cloudfront-origins/test/http-origin.test.ts

@@ -15,9 +15,9 @@ beforeEach(() => {
 
 test('Renders minimal example with just a domain name', () => {
   const origin = new HttpOrigin('www.example.com');
-  origin.bind(stack, { originIndex: 0 });
+  const originBindConfig = origin.bind(stack, { originId: 'StackOrigin029E19582' });
 
-  expect(origin.renderOrigin()).toEqual({
+  expect(originBindConfig.originProperty).toEqual({
     id: 'StackOrigin029E19582',
     domainName: 'www.example.com',
     customOriginConfig: {
@@ -32,9 +32,9 @@ test('Can customize properties of the origin', () => {
     readTimeout: Duration.seconds(10),
     protocolPolicy: cloudfront.OriginProtocolPolicy.MATCH_VIEWER,
   });
-  origin.bind(stack, { originIndex: 0 });
+  const originBindConfig = origin.bind(stack, { originId: 'StackOrigin029E19582' });
 
-  expect(origin.renderOrigin()).toEqual({
+  expect(originBindConfig.originProperty).toEqual({
     id: 'StackOrigin029E19582',
     domainName: 'www.example.com',
     originCustomHeaders: [{

packages/@aws-cdk/aws-cloudfront-origins/test/integ.s3-origin.expected.json

@@ -23,7 +23,7 @@
               "Principal": {
                 "CanonicalUser": {
                   "Fn::GetAtt": [
-                    "DistributionS3Origin115FD918D",
+                    "DistributionOrigin1S3Origin5F5C0696",
                     "S3CanonicalUserId"
                   ]
                 }
@@ -56,7 +56,7 @@
         }
       }
     },
-    "DistributionS3Origin115FD918D": {
+    "DistributionOrigin1S3Origin5F5C0696": {
       "Type": "AWS::CloudFront::CloudFrontOriginAccessIdentity",
       "Properties": {
         "CloudFrontOriginAccessIdentityConfig": {
@@ -92,7 +92,7 @@
                     [
                       "origin-access-identity/cloudfront/",
                       {
-                        "Ref": "DistributionS3Origin115FD918D"
+                        "Ref": "DistributionOrigin1S3Origin5F5C0696"
                       }
                     ]
                   ]
@@ -104,4 +104,4 @@
       }
     }
   }
-}
+}

packages/@aws-cdk/aws-cloudfront-origins/test/load-balancer-origin.test.ts

@@ -21,9 +21,9 @@ test('Renders minimal example with just a load balancer', () => {
   });
 
   const origin = new LoadBalancerV2Origin(loadBalancer);
-  origin.bind(stack, { originIndex: 0 });
+  const originBindConfig = origin.bind(stack, { originId: 'StackOrigin029E19582' });
 
-  expect(origin.renderOrigin()).toEqual({
+  expect(originBindConfig.originProperty).toEqual({
     id: 'StackOrigin029E19582',
     domainName: loadBalancer.loadBalancerDnsName,
     customOriginConfig: {
@@ -43,9 +43,9 @@ test('Can customize properties of the origin', () => {
     connectionTimeout: Duration.seconds(5),
     protocolPolicy: cloudfront.OriginProtocolPolicy.MATCH_VIEWER,
   });
-  origin.bind(stack, { originIndex: 0 });
+  const originBindConfig = origin.bind(stack, { originId: 'StackOrigin029E19582' });
 
-  expect(origin.renderOrigin()).toEqual({
+  expect(originBindConfig.originProperty).toEqual({
     id: 'StackOrigin029E19582',
     domainName: loadBalancer.loadBalancerDnsName,
     connectionAttempts: 3,

packages/@aws-cdk/aws-cloudfront-origins/test/s3-origin.test.ts

@@ -17,9 +17,9 @@ test('With non-website bucket, renders all required properties, including S3Orig
   const bucket = new s3.Bucket(stack, 'Bucket');
 
   const origin = new S3Origin(bucket);
-  origin.bind(stack, { originIndex: 0 });
+  const originBindConfig = origin.bind(stack, { originId: 'StackOrigin029E19582' });
 
-  expect(origin.renderOrigin()).toEqual({
+  expect(originBindConfig.originProperty).toEqual({
     id: 'StackOrigin029E19582',
     domainName: bucket.bucketRegionalDomainName,
     s3OriginConfig: {
@@ -34,9 +34,9 @@ test('With website bucket, renders all required properties, including custom ori
   });
 
   const origin = new S3Origin(bucket);
-  origin.bind(stack, { originIndex: 0 });
+  const originBindConfig = origin.bind(stack, { originId: 'StackOrigin029E19582' });
 
-  expect(origin.renderOrigin()).toEqual({
+  expect(originBindConfig.originProperty).toEqual({
     id: 'StackOrigin029E19582',
     domainName: bucket.bucketWebsiteDomainName,
     customOriginConfig: {
@@ -51,14 +51,14 @@ test('Respects props passed down to underlying origin', () => {
   });
 
   const origin = new S3Origin(bucket, { originPath: '/website' });
-  origin.bind(stack, { originIndex: 0 });
+  const originBindConfig = origin.bind(stack, { originId: 'StackOrigin029E19582' });
 
-  expect(origin.renderOrigin()).toEqual({
+  expect(originBindConfig.originProperty).toEqual({
     id: 'StackOrigin029E19582',
     domainName: bucket.bucketWebsiteDomainName,
     originPath: '/website',
     customOriginConfig: {
       originProtocolPolicy: 'http-only',
     },
   });
-});
+});

packages/@aws-cdk/aws-cloudfront/lib/distribution.ts

@@ -2,7 +2,7 @@ import * as acm from '@aws-cdk/aws-certificatemanager';
 import * as lambda from '@aws-cdk/aws-lambda';
 import { Construct, IResource, Lazy, Resource, Stack, Token, Duration } from '@aws-cdk/core';
 import { CfnDistribution } from './cloudfront.generated';
-import { Origin } from './origin';
+import { IOrigin, OriginBindConfig, OriginBindOptions } from './origin';
 import { CacheBehavior } from './private/cache-behavior';
 
 /**
@@ -53,6 +53,10 @@ export interface DistributionAttributes {
   readonly distributionId: string;
 }
 
+interface BoundOrigin extends OriginBindOptions, OriginBindConfig {
+  readonly origin: IOrigin;
+}
+
 /**
  * Properties for a Distribution
  *
@@ -127,7 +131,7 @@ export class Distribution extends Resource implements IDistribution {
 
   private readonly defaultBehavior: CacheBehavior;
   private readonly additionalBehaviors: CacheBehavior[] = [];
-  private readonly origins: Set<Origin> = new Set<Origin>();
+  private readonly boundOrigins: BoundOrigin[] = [];
 
   private readonly errorResponses: ErrorResponse[];
   private readonly certificate?: acm.ICertificate;
@@ -142,8 +146,8 @@ export class Distribution extends Resource implements IDistribution {
       }
     }
 
-    this.defaultBehavior = new CacheBehavior({ pathPattern: '*', ...props.defaultBehavior });
-    this.addOrigin(this.defaultBehavior.origin);
+    const originId = this.addOrigin(props.defaultBehavior.origin);
+    this.defaultBehavior = new CacheBehavior(originId, { pathPattern: '*', ...props.defaultBehavior });
     if (props.additionalBehaviors) {
       Object.entries(props.additionalBehaviors).forEach(([pathPattern, behaviorOptions]) => {
         this.addBehavior(pathPattern, behaviorOptions.origin, behaviorOptions);
@@ -172,26 +176,38 @@ export class Distribution extends Resource implements IDistribution {
    * Adds a new behavior to this distribution for the given pathPattern.
    *
    * @param pathPattern the path pattern (e.g., 'images/*') that specifies which requests to apply the behavior to.
+   * @param origin the origin to use for this behavior
    * @param behaviorOptions the options for the behavior at this path.
    */
-  public addBehavior(pathPattern: string, origin: Origin, behaviorOptions: AddBehaviorOptions = {}) {
+  public addBehavior(pathPattern: string, origin: IOrigin, behaviorOptions: AddBehaviorOptions = {}) {
     if (pathPattern === '*') {
       throw new Error('Only the default behavior can have a path pattern of \'*\'');
     }
-    this.additionalBehaviors.push(new CacheBehavior({ pathPattern, origin, ...behaviorOptions }));
-    this.addOrigin(origin);
+    const originId = this.addOrigin(origin);
+    this.additionalBehaviors.push(new CacheBehavior(originId, { pathPattern, ...behaviorOptions }));
   }
 
-  private addOrigin(origin: Origin) {
-    if (!this.origins.has(origin)) {
-      this.origins.add(origin);
-      origin.bind(this, { originIndex: this.origins.size });
+  private addOrigin(origin: IOrigin): string {
+    const existingOrigin = this.boundOrigins.find(boundOrigin => boundOrigin.origin === origin);
+    if (existingOrigin) {
+      return existingOrigin.originId;
+    } else {
+      const originIndex = this.boundOrigins.length + 1;
+      const scope = new Construct(this, `Origin${originIndex}`);
+      const originId = scope.node.uniqueId;
+      const originBindConfig = origin.bind(scope, { originId });
+      this.boundOrigins.push({ origin, originId, ...originBindConfig });
+      return originId;
     }
   }
 
   private renderOrigins(): CfnDistribution.OriginProperty[] {
     const renderedOrigins: CfnDistribution.OriginProperty[] = [];
-    this.origins.forEach(origin => renderedOrigins.push(origin.renderOrigin()));
+    this.boundOrigins.forEach(boundOrigin => {
+      if (boundOrigin.originProperty) {
+        renderedOrigins.push(boundOrigin.originProperty);
+      }
+    });
     return renderedOrigins;
   }
 
@@ -229,7 +245,6 @@ export class Distribution extends Resource implements IDistribution {
       minimumProtocolVersion: SecurityPolicyProtocol.TLS_V1_2_2018,
     };
   }
-
 }
 
 /**
@@ -443,5 +458,5 @@ export interface BehaviorOptions extends AddBehaviorOptions {
   /**
    * The origin that you want CloudFront to route requests to when they match this behavior.
    */
-  readonly origin: Origin;
+  readonly origin: IOrigin;
 }

packages/@aws-cdk/aws-cloudfront/lib/origin.ts

@@ -4,6 +4,28 @@ import { CfnDistribution } from './cloudfront.generated';
 import { OriginProtocolPolicy } from './distribution';
 import { OriginAccessIdentity } from './origin_access_identity';
 
+/** The struct returned from {@link IOrigin.bind}. */
+export interface OriginBindConfig {
+  /**
+   * The CloudFormation OriginProperty configuration for this Origin.
+   *
+   * @default - nothing is returned
+   */
+  readonly originProperty?: CfnDistribution.OriginProperty;
+}
+
+/**
+ * Represents the concept of a CloudFront Origin.
+ * You provide one or more origins when creating a Distribution.
+ */
+export interface IOrigin {
+  /**
+   * The method called when a given Origin is added
+   * (for the first time) to a Distribution.
+   */
+  bind(scope: Construct, options: OriginBindOptions): OriginBindConfig;
+}
+
 /**
  * Properties to define an Origin.
  *
@@ -48,9 +70,10 @@ export interface OriginProps {
  */
 export interface OriginBindOptions {
   /**
-   * The positional index of this origin within the distribution. Used for ensuring unique IDs.
+   * The identifier of this Origin,
+   * as assigned by the Distribution this Origin has been used added to.
    */
-  readonly originIndex: number;
+  readonly originId: string;
 }
 
 /**
@@ -59,13 +82,8 @@ export interface OriginBindOptions {
  *
  * @experimental
  */
-export abstract class Origin {
-
-  /**
-   * The domain name of the origin.
-   */
-  public readonly domainName: string;
-
+export abstract class Origin implements IOrigin {
+  private readonly domainName: string;
   private readonly originPath?: string;
   private readonly connectionTimeout?: Duration;
   private readonly connectionAttempts?: number;
@@ -97,22 +115,17 @@ export abstract class Origin {
   /**
    * Binds the origin to the associated Distribution. Can be used to grant permissions, create dependent resources, etc.
    */
-  public bind(scope: Construct, options: OriginBindOptions): void {
-    this.originId = new Construct(scope, `Origin${options.originIndex}`).node.uniqueId;
-  }
+  public bind(_scope: Construct, options: OriginBindOptions): OriginBindConfig {
+    this.originId = options.originId;
 
-  /**
-   * Creates and returns the CloudFormation representation of this origin.
-   */
-  public renderOrigin(): CfnDistribution.OriginProperty {
     const s3OriginConfig = this.renderS3OriginConfig();
     const customOriginConfig = this.renderCustomOriginConfig();
 
     if (!s3OriginConfig && !customOriginConfig) {
       throw new Error('Subclass must override and provide either s3OriginConfig or customOriginConfig');
     }
 
-    return {
+    return { originProperty: {
       domainName: this.domainName,
       id: this.id,
       originPath: this.originPath,
@@ -121,7 +134,7 @@ export abstract class Origin {
       originCustomHeaders: this.renderCustomHeaders(),
       s3OriginConfig,
       customOriginConfig,
-    };
+    }};
   }
 
   // Overridden by sub-classes to provide S3 origin config.
@@ -153,7 +166,6 @@ export abstract class Origin {
     if (path.endsWith('/')) { path = path.substr(0, path.length - 1); }
     return path;
   }
-
 }
 
 /**
@@ -184,12 +196,12 @@ export class S3Origin extends Origin {
     this.bucket = props.bucket;
   }
 
-  public bind(scope: Construct, options: OriginBindOptions) {
-    super.bind(scope, options);
+  public bind(scope: Construct, options: OriginBindOptions): OriginBindConfig {
     if (!this.originAccessIdentity) {
-      this.originAccessIdentity = new OriginAccessIdentity(scope, `S3Origin${options.originIndex}`);
+      this.originAccessIdentity = new OriginAccessIdentity(scope, 'S3Origin');
       this.bucket.grantRead(this.originAccessIdentity);
     }
+    return super.bind(scope, options);
   }
 
   protected renderS3OriginConfig(): CfnDistribution.S3OriginConfigProperty | undefined {
@@ -275,4 +287,4 @@ function validateIntInRangeOrUndefined(name: string, min: number, max: number, v
     const seconds = isDuration ? ' seconds' : '';
     throw new Error(`${name}: Must be an int between ${min} and ${max}${seconds} (inclusive); received ${value}.`);
   }
-}
+}