RFC 617: CloudFront S3 Origin Access Control L2

[gracelu0]

This is a request for comments about CloudFront Origin Access Control L2 for S3 origins. See #617 for
additional details.

APIs are signed off by @comcalvi .

---

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

[ahammond]

Nice!!!

[TheRealAmazonKendra]

Removing the final commenting period because we're not quite there yet.

[TheRealAmazonKendra]

So, this took me a very long time and I left quite a lot of comments. Given that I want to say that I think this is a very good start and this this is a quite complex set of functionality to navigate given the limitations and pre-existing functionality that needs to be worked around.

[TheRealAmazonKendra]

This isn't quite what this RFC proposes. According to the CloudFormation OriginAccessControl documentation, there are four types. While it is not necessarily invalid to scope this change down to just S3, the design must be extensible to all four types. My feedback below will be based off that requirement.

[TheRealAmazonKendra]

Also, while I won't ask for a full design on the other three types, I'd like a section briefly discussing the expected user contract for them. No implementation details or anything further is needed on those.

[TheRealAmazonKendra]

I'm not actually sure that we have implementations for Origin for MediaPackage or MediaStore but they might(?) fall under one of the other origin types. I'm kind of unclear on that because the origin types we have implemented don't actually align with what I see listed on the Origin property page for the Distribution resource. Can we get clarification from the CloudFront team on that?

[comcalvi]

Can we update the changelog to reflect that this is only for the S3 origin type?

[TheRealAmazonKendra]

TBH I glazed over everything between my last comment and this one because of the comment that I didn't think the custom resource was the way to go here.

[comcalvi]

Can we update the changelog to reflect that this is only for the S3 origin type?

Pull request has been modified.

[TheRealAmazonKendra]

I see that you took the feedback here from myself and Calvin but I think we pushed it a little too far in the opposite direction of where you were starting from.

S3BucketOriginWithOAI, S3BucketOriginWithOAC, and S3BucketOriginPublic classes in addition to S3StaticWebsiteOrigin seems a bit overkill and I think we can simplify this a bit more.

Nearly no notes to SigningBehavior, SigningProtocol, or Signing, but the rest feel like we could be abstracting more away from the user and simplifying the contract a bit.

What I'm thinking is something more along the lines of separating out the two types of bucket origins for better clarity (which you totally did) but utilizing static function to minimize nesting of props. Something like:

/**
 * Props for adding OAC
 */
export interface S3OriginAccessControlProps extends cloudfront.OriginProps  {
  /**
   * @default - SIGV4_ALWAYS
   */
  signing?: Signing 
}

export abstract class S3BucketOrigin {
  /**
   * Static function to create the OAC with a little bit of an example of how this 
   * could potentially be implemented.
   */
  public static withOriginAccessControl(bucket: IBucket, props?: S3OriginAccessControlProps): cloudfront.IOrigin {
    return new class implements cloudfront.IOrigin {
      bind(scope: Construct, options: cloudfront.OriginBindOptions): cloudfront.OriginBindConfig {
        // Create the OAC here and return the config
      }
    }();
  }
  /**
   * We hope people won't use this but we can't remove the functionality altogether
   */
  public static withOriginAccessIdentity(bucket: IBucket, identity: cloudfront.IOriginAccessIdentity): cloudfront.IOrigin {
    // Samesies again
  }
  /**
   * Unideal, but still technically an option. Make them have to declare it manually
   */
  public static withBucketDefaults(bucket: IBucket, props: cloudfront.OriginProps): cloudfront.IOrigin {
    // Samesies as above
  }

  constructor() { }
}

/**
 * For namespace reservation only
 */
export interface S3StaticWebsiteOriginProps extends cloudfront.OriginProps {}

export class S3StaticWebsiteOrigin implements cloudfront.IOrigin {
  constructor (bucket: IBucket, props?: S3StaticWebsiteOriginProps) { }

  bind(scope: Construct, options: cloudfront.OriginBindOptions): cloudfront.OriginBindConfig {
    // Use this to pass the scope so that the user doesn't have to provide it
  }
}

So, in practice, this looks something like:

const bucket = new Bucket(this, 'bucket');

new cloudfront.Distribution(this, 'newDist', {
  defaultBehavior: {
    origin: S3BucketOrigin.withOriginAccessControl(bucket),
  },
});

new cloudfront.Distribution(this, 'newDist', {
  defaultBehavior: {
    origin: S3BucketOrigin.withOriginAccessControl(bucket, { signing: Signing.SIGV4_NO_OVERRIDE }),
  },
});

new cloudfront.Distribution(this, 'newDist', {
  defaultBehavior: {
    origin: S3BucketOrigin.withOriginAccessIdentity(bucket, cloudfront.OriginAccessIdentity.fromOriginAccessIdentityId(this, 'OAIResource', 'OAI')),
  }
});

This comment covers my overall feedback so I'll avoid adding comments to this extent in every location where the proposed contract is different.

[TheRealAmazonKendra]

There will be a little more to it than that but the specifics of migration don't necessarily need to be covered here.

[gracelu0]

Could you elaborate on what is missing as part of migration?

Pull request has been modified.

Pull request has been modified.

[gracelu0]

Hi all! This RFC has been approved by the API Bar Raiser and will remain open for a week for any final comments. If no further comments we will merge in a week. Thank you!

[gracelu0]

No new comments, merging the RFC now as the final comment period has passed.