feat(cloudfront-origins): allow custom originPath for apigateway.RestApi constructs #24023

oieduardorabelo · 2023-02-06T02:38:20Z

Current behavior

the apigateway.RestApi splits the restApi.url and uses the stage name as originPath / "Origin path - optional" option in the CloudFront Origin by default

Issue / Limitation

when using multiple Behaviors in my CloudFront Distribution:
- S3 as the default behavior
- Regional API Gateway as additional behavior and CloudFront Origin

My API Gateway receives an extra /<api-gateway-stage>/ in its event path and doesn't trigger the correct lambda integration.

The below CDK example reproduces the behavior mentioned above:

import * as cdk from "aws-cdk-lib";

const meLambda = new cdk.aws_lambda_nodejs.NodejsFunction(
  this,
  "meLambda",
  {
    entry: "handlers/me.ts",
  }
);

const s3_bucket = new cdk.aws_s3.Bucket(this, "somebucket");

const api = new cdk.aws_apigateway.RestApi(this, "somerestapi", {
      endpointConfiguration: {
        types: [apigw.EndpointType.REGIONAL],
      },
      defaultCorsPreflightOptions: {
        allowOrigins: ["*"],
      },
});
api.root
  .addResource("me")
  .addMethod("GET", new apigw.LambdaIntegration(meLambda));

const apiOrigin = new cdk.aws_cloudfront_origins.RestApiOrigin(api);

const cdn = new cdk.aws_cloudfront.Distribution(this, "websitecdn", {
    defaultBehavior: {
      origin: new cdk.aws_cloudfront_origins.S3Origin(s3_bucket),
      allowedMethods: cdk.aws_cloudfront.AllowedMethods.ALLOW_GET_HEAD_OPTIONS,
      viewerProtocolPolicy: cdk.aws_cloudfront.ViewerProtocolPolicy.REDIRECT_TO_HTTPS,
    },
    additionalBehaviors: {
      "prod/*": {  // <<<---- The "prod/" is added in the CloudFront Origin Path, NOT DESIRED IS THIS CASE
        origin: apiOrigin,
        allowedMethods: cdk.aws_cloudfront.AllowedMethods.ALLOW_ALL,
        cachePolicy: cdk.aws_cloudfront.CachePolicy.CACHING_DISABLED,
        viewerProtocolPolicy: cdk.aws_cloudfront.ViewerProtocolPolicy.HTTPS_ONLY,
      },
    },
});

The above will have the following URLs:

https://<api-gateway-url>/<api-gateway-stage>
https://<cloudfront-distribution-url>/some-image.png => Default Behavior, S3 Bucket
https://<cloudfront-distribution-url>/<api-gateway-stage> => API Gateway forward request

Because the CloudFront Origin created by RestApiOrigin appends the <api-gateway-stage> in its CloudFront Origin "Origin Path - optional" option, when using https://<cloudfront-distribution-url>/<api-gateway-stage>/me the API Gateway receives /<api-gateway-stage>/<api-gateway-stage>/me instead of /<api-gateway-stage>/me

Removing the "Origin Path - optional" value, fixes the problem.

Workaround

To fix the problem described above, I need to use a custom cdk.aws_cloudfront_origins.HttpOrigin construct

Replace the apiOrigin construct in the example above by:

const apiOrigin = new origins.HttpOrigin(
  `${api.restApiId}.execute-api.${cdk.Aws.REGION}.${cdk.Aws.URL_SUFFIX}`,
  {
    originSslProtocols: [cloudfront.OriginSslPolicy.TLS_V1_2],
    protocolPolicy: cloudfront.OriginProtocolPolicy.HTTPS_ONLY,
  }
);

Solution

This PR allows the customization of apigateway.RestApi by extending its props from cloudfront.OriginProps
Allowing the consumer to pass a props.originPath to the RestApiOrigin class

gitpod-io · 2023-02-06T02:38:28Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

TheRealAmazonKendra

Thank you for your contribution! Overall this looks good. I just have a question inline. Additionally, please add an integ test to cover this change. I'm concerned about the potential testing gap given that your change wasn't picked up by a test. Not your fault, but I like to take every opportunity to improve our test coverage.

If, for any reason, running tests isn't feasible for you, please let me know and I can assist after you write the test case.

TheRealAmazonKendra · 2023-02-10T05:24:54Z

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

@@ -6,7 +6,7 @@ import { validateSecondsInRangeOrUndefined } from './private/utils';
 /**
 * Properties for an Origin for an API Gateway REST API.
 */
-export interface RestApiOriginProps extends cloudfront.OriginOptions {
+export interface RestApiOriginProps extends cloudfront.OriginProps {


What is in OriginProps that isn't in OriginOptions and vice versa? I'm concerned this might break other languages even if it's fine in TypeScript.

OriginProps extends OriginOptions and declares the originPath.

export interface OriginOptions { readonly connectionTimeout?: Duration; readonly connectionAttempts?: number; readonly customHeaders?: Record<string, string>; readonly originShieldRegion?: string; readonly originShieldEnabled?: boolean; readonly originId?: string; } export interface OriginProps extends OriginOptions { readonly originPath?: string; }

OriginProps is used in other packages. Where OriginOptions is internal? (not sure)

since the only new property is optional, this should be non-breaking

oieduardorabelo · 2023-02-10T11:02:57Z

@TheRealAmazonKendra is the integ test case correct? I followed the instructions at INTEGRATION_TESTS.md and based it on aws-cloudfront-origins/test/integ.rest-api-origin.ts

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

Pull request has been modified.

Naumel · 2023-02-27T12:39:05Z

🔄

@aws-cdk/aws-cloudfront-origins: To re-run failed tests run: integ-runner --update-on-failed
@aws-cdk/aws-cloudfront-origins:     at main (/codebuild/output/src297086622/src/github.com/aws/aws-cdk/packages/@aws-cdk/integ-runner/lib/cli.js:164:19)
@aws-cdk/aws-cloudfront-origins: Error: integ-runner exited with error code 1
@aws-cdk/aws-cloudfront-origins: Tests failed. Total time (15.1s) | /codebuild/output/src297086622/src/github.com/aws/aws-cdk/node_modules/jest/bin/jest.js (11.9s) | integ-runner (3.2s)

oieduardorabelo · 2023-02-27T22:45:47Z

@Naumel 🟢

comcalvi

lgtm, leaving dnm so @TheRealAmazonKendra can take a look

comcalvi · 2023-03-01T22:19:33Z

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

@@ -6,7 +6,7 @@ import { validateSecondsInRangeOrUndefined } from './private/utils';
 /**
 * Properties for an Origin for an API Gateway REST API.
 */
-export interface RestApiOriginProps extends cloudfront.OriginOptions {
+export interface RestApiOriginProps extends cloudfront.OriginProps {


since the only new property is optional, this should be non-breaking

aws-cdk-automation · 2023-03-23T00:08:04Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

…Api constructs

…igin path

…igin path when using origins.RestApiOrigin

oieduardorabelo · 2023-03-23T00:55:47Z

PR not abandoned

Pull request has been modified.

Naumel · 2023-03-23T11:42:40Z

PR not abandoned

👍
Someone from the team has already reviewed and labeled the PR. I'll ping to check if they're available for a follow-up.

mergify · 2023-03-23T15:49:30Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-03-23T16:33:02Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 8cc235a
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-03-23T16:33:23Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…Api constructs (aws#24023) ### Current behavior - the `apigateway.RestApi` splits the `restApi.url` and uses the stage name as `originPath` / "Origin path - optional" option in the CloudFront Origin by default ### Issue / Limitation - when using multiple Behaviors in my CloudFront Distribution: - S3 as the default behavior - Regional API Gateway as additional behavior and CloudFront Origin My API Gateway receives an extra `/<api-gateway-stage>/` in its event path and doesn't trigger the correct lambda integration. The below CDK example reproduces the behavior mentioned above: ```typescript import * as cdk from "aws-cdk-lib"; const meLambda = new cdk.aws_lambda_nodejs.NodejsFunction( this, "meLambda", { entry: "handlers/me.ts", } ); const s3_bucket = new cdk.aws_s3.Bucket(this, "somebucket"); const api = new cdk.aws_apigateway.RestApi(this, "somerestapi", { endpointConfiguration: { types: [apigw.EndpointType.REGIONAL], }, defaultCorsPreflightOptions: { allowOrigins: ["*"], }, }); api.root .addResource("me") .addMethod("GET", new apigw.LambdaIntegration(meLambda)); const apiOrigin = new cdk.aws_cloudfront_origins.RestApiOrigin(api); const cdn = new cdk.aws_cloudfront.Distribution(this, "websitecdn", { defaultBehavior: { origin: new cdk.aws_cloudfront_origins.S3Origin(s3_bucket), allowedMethods: cdk.aws_cloudfront.AllowedMethods.ALLOW_GET_HEAD_OPTIONS, viewerProtocolPolicy: cdk.aws_cloudfront.ViewerProtocolPolicy.REDIRECT_TO_HTTPS, }, additionalBehaviors: { "prod/*": { // <<<---- The "prod/" is added in the CloudFront Origin Path, NOT DESIRED IS THIS CASE origin: apiOrigin, allowedMethods: cdk.aws_cloudfront.AllowedMethods.ALLOW_ALL, cachePolicy: cdk.aws_cloudfront.CachePolicy.CACHING_DISABLED, viewerProtocolPolicy: cdk.aws_cloudfront.ViewerProtocolPolicy.HTTPS_ONLY, }, }, }); ``` The above will have the following URLs: - `https://<api-gateway-url>/<api-gateway-stage>` - `https://<cloudfront-distribution-url>/some-image.png` => Default Behavior, S3 Bucket - `https://<cloudfront-distribution-url>/<api-gateway-stage>` => API Gateway forward request Because the CloudFront Origin created by `RestApiOrigin` appends the `<api-gateway-stage>` in its CloudFront Origin "Origin Path - optional" option, when using `https://<cloudfront-distribution-url>/<api-gateway-stage>/me` the API Gateway receives `/<api-gateway-stage>/<api-gateway-stage>/me` instead of `/<api-gateway-stage>/me` Removing the "Origin Path - optional" value, fixes the problem. ![00](https://user-images.githubusercontent.com/829902/216874002-06338400-9d88-4c98-b393-da56cc63be02.png) ![01](https://user-images.githubusercontent.com/829902/216874042-643ef152-b2ad-465e-bdda-8a98c68c6dcd.png) ### Workaround - To fix the problem described above, I need to use a custom `cdk.aws_cloudfront_origins.HttpOrigin` construct Replace the `apiOrigin` construct in the example above by: ```typescript const apiOrigin = new origins.HttpOrigin( `${api.restApiId}.execute-api.${cdk.Aws.REGION}.${cdk.Aws.URL_SUFFIX}`, { originSslProtocols: [cloudfront.OriginSslPolicy.TLS_V1_2], protocolPolicy: cloudfront.OriginProtocolPolicy.HTTPS_ONLY, } ); ``` ### Solution - This PR allows the customization of `apigateway.RestApi` by extending its props from `cloudfront.OriginProps` - Allowing the consumer to pass a `props.originPath` to the `RestApiOrigin` class

github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Feb 6, 2023

aws-cdk-automation requested a review from a team February 6, 2023 02:38

aws-cdk-automation previously requested changes Feb 6, 2023

View reviewed changes

TheRealAmazonKendra previously requested changes Feb 10, 2023

View reviewed changes

oieduardorabelo force-pushed the main branch from 97fed5b to 1b5eedc Compare February 10, 2023 10:34

oieduardorabelo requested review from TheRealAmazonKendra and aws-cdk-automation and removed request for TheRealAmazonKendra February 10, 2023 11:04

oieduardorabelo requested review from TheRealAmazonKendra and removed request for aws-cdk-automation February 10, 2023 11:11

oieduardorabelo force-pushed the main branch from 58e9396 to dea87bf Compare February 11, 2023 00:09

oieduardorabelo force-pushed the main branch 2 times, most recently from a7e4593 to 0101b88 Compare February 27, 2023 20:45

comcalvi previously approved these changes Mar 1, 2023

View reviewed changes

comcalvi added the pr/do-not-merge This PR should not be merged at this time. label Mar 1, 2023

oieduardorabelo added 4 commits March 23, 2023 13:55

feat(cloudfront-origins): allow custom originPath for apigateway.Rest…

6409cb1

…Api constructs

feat(cloudfront-origins): add intg test for rest api origin custom or…

9cf55b2

…igin path

docs(cloudfront-origins): add instructions in readme to change the or…

ce22e80

…igin path when using origins.RestApiOrigin

fix(cloudfront-origins): run integ tests with update

a3bc774

oieduardorabelo force-pushed the main branch from 0101b88 to a3bc774 Compare March 23, 2023 00:55

comcalvi removed the pr/do-not-merge This PR should not be merged at this time. label Mar 23, 2023

comcalvi approved these changes Mar 23, 2023

View reviewed changes

Merge branch 'main' into main

8cc235a

mergify bot merged commit bc3db02 into aws:main Mar 23, 2023

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cloudfront-origins): allow custom originPath for apigateway.RestApi constructs #24023

feat(cloudfront-origins): allow custom originPath for apigateway.RestApi constructs #24023

oieduardorabelo commented Feb 6, 2023 •

edited

Loading

gitpod-io bot commented Feb 6, 2023

aws-cdk-automation left a comment

TheRealAmazonKendra left a comment

TheRealAmazonKendra Feb 10, 2023

oieduardorabelo Feb 10, 2023

comcalvi Mar 1, 2023

oieduardorabelo commented Feb 10, 2023

Naumel commented Feb 27, 2023

oieduardorabelo commented Feb 27, 2023

comcalvi left a comment

comcalvi Mar 1, 2023

aws-cdk-automation commented Mar 23, 2023

oieduardorabelo commented Mar 23, 2023

Naumel commented Mar 23, 2023 •

edited

Loading

mergify bot commented Mar 23, 2023

aws-cdk-automation commented Mar 23, 2023

mergify bot commented Mar 23, 2023

feat(cloudfront-origins): allow custom originPath for apigateway.RestApi constructs #24023

feat(cloudfront-origins): allow custom originPath for apigateway.RestApi constructs #24023

Conversation

oieduardorabelo commented Feb 6, 2023 • edited Loading

Current behavior

Issue / Limitation

Workaround

Solution

gitpod-io bot commented Feb 6, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra Feb 10, 2023

Choose a reason for hiding this comment

oieduardorabelo Feb 10, 2023

Choose a reason for hiding this comment

comcalvi Mar 1, 2023

Choose a reason for hiding this comment

oieduardorabelo commented Feb 10, 2023

Naumel commented Feb 27, 2023

oieduardorabelo commented Feb 27, 2023

comcalvi left a comment

Choose a reason for hiding this comment

comcalvi Mar 1, 2023

Choose a reason for hiding this comment

aws-cdk-automation commented Mar 23, 2023

oieduardorabelo commented Mar 23, 2023

Naumel commented Mar 23, 2023 • edited Loading

mergify bot commented Mar 23, 2023

aws-cdk-automation commented Mar 23, 2023

AWS CodeBuild CI Report

mergify bot commented Mar 23, 2023

oieduardorabelo commented Feb 6, 2023 •

edited

Loading

Naumel commented Mar 23, 2023 •

edited

Loading