chore(cloudfront): Removed duplicate origins in aws-cloudfront module (…

…aws#9326)

Prior to this change, there were both HttpOrigin and S3Origin classes in both
the aws-cloudfront and aws-cloudfront-origins module. The behaviors of the
S3Origin classes were also slightly different.

This change removes the duplication by removing the aws-cloudfront versions of
the origins.

----

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

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

@@ -1,21 +1,82 @@
 import * as cloudfront from '@aws-cdk/aws-cloudfront';
+import * as cdk from '@aws-cdk/core';
 
 /**
- * Properties for an Origin backed by any HTTP server.
+ * Properties for an Origin backed by an S3 website-configured bucket, load balancer, or custom HTTP server.
  *
  * @experimental
  */
-export interface HttpOriginProps extends cloudfront.HttpOriginProps { }
+export interface HttpOriginProps extends cloudfront.OriginProps {
+  /**
+   * Specifies the protocol (HTTP or HTTPS) that CloudFront uses to connect to the origin.
+   *
+   * @default OriginProtocolPolicy.HTTPS_ONLY
+   */
+  readonly protocolPolicy?: cloudfront.OriginProtocolPolicy;
+
+  /**
+   * The HTTP port that CloudFront uses to connect to the origin.
+   *
+   * @default 80
+   */
+  readonly httpPort?: number;
+
+  /**
+   * The HTTPS port that CloudFront uses to connect to the origin.
+   *
+   * @default 443
+   */
+  readonly httpsPort?: number;
+
+  /**
+   * Specifies how long, in seconds, CloudFront waits for a response from the origin, also known as the origin response timeout.
+   * The valid range is from 1 to 60 seconds, inclusive.
+   *
+   * @default Duration.seconds(30)
+   */
+  readonly readTimeout?: cdk.Duration;
+
+  /**
+   * Specifies how long, in seconds, CloudFront persists its connection to the origin.
+   * The valid range is from 1 to 60 seconds, inclusive.
+   *
+   * @default Duration.seconds(5)
+   */
+  readonly keepaliveTimeout?: cdk.Duration;
+}
 
 /**
- * An Origin for an HTTP server.
+ * An Origin for an HTTP server or S3 bucket configured for website hosting.
  *
  * @experimental
  */
-export class HttpOrigin extends cloudfront.HttpOrigin {
+export class HttpOrigin extends cloudfront.OriginBase {
+
+  constructor(domainName: string, private readonly props: HttpOriginProps = {}) {
+    super(domainName, props);
 
-  constructor(domainName: string, props: HttpOriginProps = {}) {
-    super(domainName, { ...props });
+    validateSecondsInRangeOrUndefined('readTimeout', 1, 60, props.readTimeout);
+    validateSecondsInRangeOrUndefined('keepaliveTimeout', 1, 60, props.keepaliveTimeout);
   }
 
+  protected renderCustomOriginConfig(): cloudfront.CfnDistribution.CustomOriginConfigProperty | undefined {
+    return {
+      originProtocolPolicy: this.props.protocolPolicy ?? cloudfront.OriginProtocolPolicy.HTTPS_ONLY,
+      httpPort: this.props.httpPort,
+      httpsPort: this.props.httpsPort,
+      originReadTimeout: this.props.readTimeout?.toSeconds(),
+      originKeepaliveTimeout: this.props.keepaliveTimeout?.toSeconds(),
+    };
+  }
+}
+
+/**
+ * Throws an error if a duration is defined and not an integer number of seconds within a range.
+ */
+function validateSecondsInRangeOrUndefined(name: string, min: number, max: number, duration?: cdk.Duration) {
+  if (duration === undefined) { return; }
+  const value = duration.toSeconds();
+  if (!Number.isInteger(value) || value < min || value > max) {
+    throw new Error(`${name}: Must be an int between ${min} and ${max} seconds (inclusive); received ${value}.`);
+  }
 }

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

@@ -1,19 +1,19 @@
-import * as cloudfront from '@aws-cdk/aws-cloudfront';
 import * as elbv2 from '@aws-cdk/aws-elasticloadbalancingv2';
+import { HttpOrigin, HttpOriginProps } from './http-origin';
 
 /**
  * Properties for an Origin backed by a v2 load balancer.
  *
  * @experimental
  */
-export interface LoadBalancerV2OriginProps extends cloudfront.HttpOriginProps { }
+export interface LoadBalancerV2OriginProps extends HttpOriginProps { }
 
 /**
  * An Origin for a v2 load balancer.
  *
  * @experimental
  */
-export class LoadBalancerV2Origin extends cloudfront.HttpOrigin {
+export class LoadBalancerV2Origin extends HttpOrigin {
 
   constructor(loadBalancer: elbv2.ILoadBalancerV2, props: LoadBalancerV2OriginProps = {}) {
     super(loadBalancer.loadBalancerDnsName, { ...props });

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

@@ -1,6 +1,7 @@
 import * as cloudfront from '@aws-cdk/aws-cloudfront';
 import * as s3 from '@aws-cdk/aws-s3';
 import * as cdk from '@aws-cdk/core';
+import { HttpOrigin } from './http-origin';
 
 /**
  * Properties to use to customize an S3 Origin.
@@ -30,22 +31,41 @@ export class S3Origin implements cloudfront.IOrigin {
   private readonly origin: cloudfront.IOrigin;
 
   constructor(bucket: s3.IBucket, props: S3OriginProps = {}) {
-    let proxyOrigin;
-    if (bucket.isWebsite) {
-      proxyOrigin = new cloudfront.HttpOrigin(bucket.bucketWebsiteDomainName, {
+    this.origin = bucket.isWebsite ?
+      new HttpOrigin(bucket.bucketWebsiteDomainName, {
         protocolPolicy: cloudfront.OriginProtocolPolicy.HTTP_ONLY, // S3 only supports HTTP for website buckets
         ...props,
-      });
-    } else {
-      proxyOrigin = new cloudfront.S3Origin({
-        bucket,
-        ...props,
-      });
-    }
-    this.origin = proxyOrigin;
+      }) :
+      new S3BucketOrigin(bucket, props);
   }
 
   public bind(scope: cdk.Construct, options: cloudfront.OriginBindOptions): cloudfront.OriginBindConfig {
     return this.origin.bind(scope, options);
   }
+
+}
+
+/**
+ * An Origin specific to a S3 bucket (not configured for website hosting).
+ *
+ * Contains additional logic around bucket permissions and origin access identities.
+ */
+class S3BucketOrigin extends cloudfront.OriginBase {
+  private originAccessIdentity!: cloudfront.OriginAccessIdentity;
+
+  constructor(private readonly bucket: s3.IBucket, props: S3OriginProps) {
+    super(bucket.bucketRegionalDomainName, props);
+  }
+
+  public bind(scope: cdk.Construct, options: cloudfront.OriginBindOptions): cloudfront.OriginBindConfig {
+    if (!this.originAccessIdentity) {
+      this.originAccessIdentity = new cloudfront.OriginAccessIdentity(scope, 'S3Origin');
+      this.bucket.grantRead(this.originAccessIdentity);
+    }
+    return super.bind(scope, options);
+  }
+
+  protected renderS3OriginConfig(): cloudfront.CfnDistribution.S3OriginConfigProperty | undefined {
+    return { originAccessIdentity: `origin-access-identity/cloudfront/${this.originAccessIdentity.originAccessIdentityName}` };
+  }
 }

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

@@ -8,7 +8,7 @@ let stack: Stack;
 
 beforeEach(() => {
   app = new App();
-  stack = new Stack(app, 'Stack', {
+  new Stack(app, 'Stack', {
     env: { account: '1234', region: 'testregion' },
   });
 });
@@ -26,24 +26,64 @@ test('Renders minimal example with just a domain name', () => {
   });
 });
 
-test('Can customize properties of the origin', () => {
+test('renders an example with all available props', () => {
   const origin = new HttpOrigin('www.example.com', {
+    originPath: '/app',
+    connectionTimeout: Duration.seconds(5),
+    connectionAttempts: 2,
     customHeaders: { AUTH: 'NONE' },
-    readTimeout: Duration.seconds(10),
     protocolPolicy: cloudfront.OriginProtocolPolicy.MATCH_VIEWER,
+    httpPort: 8080,
+    httpsPort: 8443,
+    readTimeout: Duration.seconds(45),
+    keepaliveTimeout: Duration.seconds(3),
   });
   const originBindConfig = origin.bind(stack, { originId: 'StackOrigin029E19582' });
 
   expect(originBindConfig.originProperty).toEqual({
     id: 'StackOrigin029E19582',
     domainName: 'www.example.com',
+    originPath: '/app',
+    connectionTimeout: 5,
+    connectionAttempts: 2,
     originCustomHeaders: [{
       headerName: 'AUTH',
       headerValue: 'NONE',
     }],
     customOriginConfig: {
       originProtocolPolicy: 'match-viewer',
-      originReadTimeout: 10,
+      httpPort: 8080,
+      httpsPort: 8443,
+      originReadTimeout: 45,
+      originKeepaliveTimeout: 3,
     },
   });
 });
+
+test.each([
+  Duration.seconds(0),
+  Duration.seconds(0.5),
+  Duration.seconds(60.5),
+  Duration.seconds(61),
+  Duration.minutes(5),
+])('validates readTimeout is an integer between 1 and 60 seconds', (readTimeout) => {
+  expect(() => {
+    new HttpOrigin('www.example.com', {
+      readTimeout,
+    });
+  }).toThrow(`readTimeout: Must be an int between 1 and 60 seconds (inclusive); received ${readTimeout.toSeconds()}.`);
+});
+
+test.each([
+  Duration.seconds(0),
+  Duration.seconds(0.5),
+  Duration.seconds(60.5),
+  Duration.seconds(61),
+  Duration.minutes(5),
+])('validates keepaliveTimeout is an integer between 1 and 60 seconds', (keepaliveTimeout) => {
+  expect(() => {
+    new HttpOrigin('www.example.com', {
+      keepaliveTimeout,
+    });
+  }).toThrow(`keepaliveTimeout: Must be an int between 1 and 60 seconds (inclusive); received ${keepaliveTimeout.toSeconds()}.`);
+});

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

@@ -1,4 +1,5 @@
 import '@aws-cdk/assert/jest';
+import * as cloudfront from '@aws-cdk/aws-cloudfront';
 import * as s3 from '@aws-cdk/aws-s3';
 import { App, Stack } from '@aws-cdk/core';
 import { S3Origin } from '../lib';
@@ -13,52 +14,94 @@ beforeEach(() => {
   });
 });
 
-test('With non-website bucket, renders all required properties, including S3Origin config', () => {
-  const bucket = new s3.Bucket(stack, 'Bucket');
+describe('With bucket', () => {
+  test('renders minimal example', () => {
+    const bucket = new s3.Bucket(stack, 'Bucket');
 
-  const origin = new S3Origin(bucket);
-  const originBindConfig = origin.bind(stack, { originId: 'StackOrigin029E19582' });
+    const origin = new S3Origin(bucket);
+    const originBindConfig = origin.bind(stack, { originId: 'StackOrigin029E19582' });
 
-  expect(originBindConfig.originProperty).toEqual({
-    id: 'StackOrigin029E19582',
-    domainName: bucket.bucketRegionalDomainName,
-    s3OriginConfig: {
-      originAccessIdentity: 'origin-access-identity/cloudfront/${Token[TOKEN.69]}',
-    },
+    expect(originBindConfig.originProperty).toEqual({
+      id: 'StackOrigin029E19582',
+      domainName: bucket.bucketRegionalDomainName,
+      s3OriginConfig: {
+        originAccessIdentity: 'origin-access-identity/cloudfront/${Token[TOKEN.69]}',
+      },
+    });
   });
-});
 
-test('With website bucket, renders all required properties, including custom origin config', () => {
-  const bucket = new s3.Bucket(stack, 'Bucket', {
-    websiteIndexDocument: 'index.html',
+  test('can customize properties', () => {
+    const bucket = new s3.Bucket(stack, 'Bucket');
+
+    const origin = new S3Origin(bucket, { originPath: '/assets' });
+    const originBindConfig = origin.bind(stack, { originId: 'StackOrigin029E19582' });
+
+    expect(originBindConfig.originProperty).toEqual({
+      id: 'StackOrigin029E19582',
+      domainName: bucket.bucketRegionalDomainName,
+      originPath: '/assets',
+      s3OriginConfig: {
+        originAccessIdentity: 'origin-access-identity/cloudfront/${Token[TOKEN.89]}',
+      },
+    });
   });
 
-  const origin = new S3Origin(bucket);
-  const originBindConfig = origin.bind(stack, { originId: 'StackOrigin029E19582' });
+  test('creates an OriginAccessIdentity and grants read permissions on the bucket', () => {
+    const bucket = new s3.Bucket(stack, 'Bucket');
+
+    const origin = new S3Origin(bucket);
+    new cloudfront.Distribution(stack, 'Dist', { defaultBehavior: { origin } });
 
-  expect(originBindConfig.originProperty).toEqual({
-    id: 'StackOrigin029E19582',
-    domainName: bucket.bucketWebsiteDomainName,
-    customOriginConfig: {
-      originProtocolPolicy: 'http-only',
-    },
+    expect(stack).toHaveResourceLike('AWS::CloudFront::CloudFrontOriginAccessIdentity', {
+      CloudFrontOriginAccessIdentityConfig: {
+        Comment: 'Allows CloudFront to reach the bucket',
+      },
+    });
+    expect(stack).toHaveResourceLike('AWS::S3::BucketPolicy', {
+      PolicyDocument: {
+        Statement: [{
+          Principal: {
+            CanonicalUser: { 'Fn::GetAtt': [ 'DistOrigin1S3Origin87D64058', 'S3CanonicalUserId' ] },
+          },
+        }],
+      },
+    });
   });
 });
 
-test('Respects props passed down to underlying origin', () => {
-  const bucket = new s3.Bucket(stack, 'Bucket', {
-    websiteIndexDocument: 'index.html',
+describe('With website-configured bucket', () => {
+  test('renders all required properties, including custom origin config', () => {
+    const bucket = new s3.Bucket(stack, 'Bucket', {
+      websiteIndexDocument: 'index.html',
+    });
+
+    const origin = new S3Origin(bucket);
+    const originBindConfig = origin.bind(stack, { originId: 'StackOrigin029E19582' });
+
+    expect(originBindConfig.originProperty).toEqual({
+      id: 'StackOrigin029E19582',
+      domainName: bucket.bucketWebsiteDomainName,
+      customOriginConfig: {
+        originProtocolPolicy: 'http-only',
+      },
+    });
   });
 
-  const origin = new S3Origin(bucket, { originPath: '/website' });
-  const originBindConfig = origin.bind(stack, { originId: 'StackOrigin029E19582' });
+  test('can customize properties', () => {
+    const bucket = new s3.Bucket(stack, 'Bucket', {
+      websiteIndexDocument: 'index.html',
+    });
+
+    const origin = new S3Origin(bucket, { originPath: '/assets' });
+    const originBindConfig = origin.bind(stack, { originId: 'StackOrigin029E19582' });
 
-  expect(originBindConfig.originProperty).toEqual({
-    id: 'StackOrigin029E19582',
-    domainName: bucket.bucketWebsiteDomainName,
-    originPath: '/website',
-    customOriginConfig: {
-      originProtocolPolicy: 'http-only',
-    },
+    expect(originBindConfig.originProperty).toEqual({
+      id: 'StackOrigin029E19582',
+      domainName: bucket.bucketWebsiteDomainName,
+      originPath: '/assets',
+      customOriginConfig: {
+        originProtocolPolicy: 'http-only',
+      },
+    });
   });
 });