feat(apigateway): Add LambdaIntegrationOptions to LambdaRestApi by laurelmay · Pull Request #17065 · aws/aws-cdk

laurelmay · 2021-10-20T01:33:03Z

This adds an integrationOptions field to the LambdaRestApiProps and ensures that proxy cannot be set differently from the base proxy flag (but discourages setting it at all). This will allow setting the cacheKeyParameters, allowTestInvoke, and vpcLink configuration fields without having to fall back to using a regular RestApi.

Some of this is inspired by #12004 and its review comments, which was closed earlier this year due to inactivity.

Resolves: #3269

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

gitpod-io · 2021-10-20T01:33:06Z

laurelmay · 2022-03-31T04:19:11Z

Turns out apparently API Gateway doesn't allow the proxy key as a CacheKeyParameter. The same behavior can be seen in the console. The PR has been updated to primarily focus on timeout instead of proxy. It doesn't seem to be a limitation of this design since the same behavior can be replicated using LambdaIntegration and RestApi separately (and in the console). But for completeness on the change here, the relevant event details from CloudTrail are:

Event Information


    "eventSource": "apigateway.amazonaws.com",
    "eventName": "UpdateIntegration",
    "awsRegion": "us-east-1",
    "sourceIPAddress": "AWS Internal",
    "userAgent": "AWS Internal",
    "errorCode": "BadRequestException",
    "errorMessage": "Invalid cache key parameter specified",
    "requestParameters": {
        "updateIntegrationInput": {
            "patchOperations": [
                {
                    "path": "/cacheKeyParameters/method.request.path.proxy",
                    "op": "add"
                }
            ]
        },
        "restApiId": "hwnay7i82b",
        "resourceId": "7xtf1y",
        "httpMethod": "ANY",
        "template": false
    },

comcalvi

Looks great! Just one minor question.

comcalvi · 2022-04-05T20:25:40Z

packages/@aws-cdk/aws-apigateway/rosetta/default.ts-fixture

 // Fixture with packages imported, but nothing else
 import { Construct } from 'constructs';
-import { Stack } from '@aws-cdk/core';
+import { Duration, Stack } from '@aws-cdk/core';


Why did we add the Duration import here?

It seems to be necessary based on the changes I also made to README.md. I got a build failure without it since jsii-rosetta couldn't find Duration (if I interpreted the error message correctly). I guess it might be possible to choose another possible integration example but that might still require another import.

comcalvi

This is looking pretty good, but there's a few small issues I'd like to see resolved. Please fix the merge conflicts as well, they're preventing our usual automation from running.

packages/@aws-cdk/aws-apigateway/lib/lambda-api.ts

comcalvi · 2022-04-07T20:04:25Z

packages/@aws-cdk/aws-apigateway/test/integ.lambda-api.ts

+}
+
+const app = new App();
+new LambdaApiIntegrationOptionsStack(app);


Please add a newline to the end of this file.

comcalvi · 2022-04-07T20:06:47Z

packages/@aws-cdk/aws-apigateway/test/lambda-api.test.ts

+      proxy: false,
+      integrationOptions: {},
+    })).not.toThrow();
+


could we remove this blank line?

comcalvi · 2022-04-07T20:11:26Z

packages/@aws-cdk/aws-apigateway/README.md

+Note that `proxy: false` may not be specified in the `integrationOptions`
+and must be specified directly in `props`.


why is this only proxy: false and not proxy?

comcalvi · 2022-04-07T20:15:09Z

packages/@aws-cdk/aws-apigateway/lib/lambda-api.ts

+   *
+   * @default - see defaults defined in {@link LambdaIntegrationOptions}.
+   */
+  readonly integrationOptions?: LambdaIntegrationOptions;


We should explain the interaction between integrationOptions.proxy and props.proxy here.

This is probably no longer necessary

comcalvi · 2022-04-07T20:16:13Z

packages/@aws-cdk/aws-apigateway/lib/lambda-api.ts

    }
+    // Forbid setting `integrationOptions.proxy = false` unless `proxy = false`
+    if (props.integrationOptions?.proxy !== undefined && props.integrationOptions?.proxy !== (props?.proxy ?? true)) {
+      throw new Error('Cannot specify "props.integrationOptions.proxy". Instead use "props.proxy".');


could we change this message to reflect the precise behavior of this interaction?

comcalvi · 2022-04-07T20:20:00Z

packages/@aws-cdk/aws-apigateway/lib/lambda-api.ts

      throw new Error('Cannot specify "defaultIntegration" since Lambda integration is automatically defined');
    }
+    // Forbid setting `integrationOptions.proxy = false` unless `proxy = false`
+    if (props.integrationOptions?.proxy !== undefined && props.integrationOptions?.proxy !== (props?.proxy ?? true)) {


We reference props.integrationOptions several times. Could we extract this to a local variable, integrationOptions?

comcalvi · 2022-04-07T20:43:13Z

packages/@aws-cdk/aws-apigateway/lib/lambda-api.ts

+    if (props.options?.defaultIntegration || props.defaultIntegration) {
      throw new Error('Cannot specify "defaultIntegration" since Lambda integration is automatically defined');
    }
+    // Forbid setting `integrationOptions.proxy = false` unless `proxy = false`


The comment on the line above should be more specific about this behavior, as well as the reasoning behind why this is desirable. Ideally this would be written with positive terms, instead of the currently used negative. I don't understand the desired interaction between integrationOptions.proxy and proxy.

comcalvi · 2022-04-07T20:55:37Z

packages/@aws-cdk/aws-apigateway/test/lambda-api.test.ts

+    expect(() => new apigw.LambdaRestApi(stack, 'lambda-rest-api2', {
+      handler,
+      proxy: false,
+      integrationOptions: {
+        proxy: false,
+      },
+    })).not.toThrow();


How does this work? The readme change has this:

Note that `proxy: false` may not be specified in the `integrationOptions` and must be specified directly in `props`.

same goes everywhere else with proxy: false in these tests.

comcalvi · 2022-04-07T21:43:53Z

packages/@aws-cdk/aws-apigateway/lib/lambda-api.ts

      throw new Error('Cannot specify "defaultIntegration" since Lambda integration is automatically defined');
    }
+    // Forbid setting `integrationOptions.proxy = false` unless `proxy = false`
+    if (props.integrationOptions?.proxy !== undefined && props.integrationOptions?.proxy !== (props?.proxy ?? true)) {


Please simplify this if as well, it's very complicated right now.

I think it should be broken apart like this:

if (props.integrationOptions?.proxy !== undefined) { if (props.integrationOptions.proxy !== (props?.proxy ?? true)) { ... // ... } }

and I think the props?.proxy ?? true should be extracted into a variable with a name that describes what that part of the expression represents.

Pull request has been modified.

laurelmay · 2022-04-08T01:17:55Z

@comcalvi Thanks for the review feedback! As I was working on implementing changes based on the feedback, I realized that the proxy attributes of LambdaRestApiProps and LambdaIntegrationOptions have different meanings. LambdaRestApiProps.proxy is "should this create a {proxy+} route" and LambdaIntegrationOptions.proxy is "should this create an API Gateway Lambda Proxy integration" which I misunderstood in the initial implementation.

There should be no reason to forbid LambdaIntegrationOptions.proxy, because per https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-set-up-simple-proxy.html,

Greedy path variables, ANY methods, and proxy integration types are independent features, although they are commonly used together. You can configure a specific HTTP method on a greedy resource or apply non-proxy integration types to a proxy resource.

I think forbidding anything around props.integrationOptions.proxy was based around my understanding of #12004 but realizing the different meaning of the two proxy flags and the capabilities of API Gateway, I think removing any prohibitions on the value(s) of either is best since one if for {proxy+} paths and the other is for AWS_PROXY-type integrations (and it's possible to support one, both, or neither).

mergify · 2022-06-15T00:04:36Z

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).

mergify · 2022-06-15T00:06:51Z

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).

Pull request has been modified.

mergify · 2022-06-22T08:11:17Z

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 · 2022-06-22T08:49:39Z

AWS CodeBuild CI Report

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

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

mergify · 2022-06-22T08:49:58Z

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).

…17065) This adds an `integrationOptions` field to the `LambdaRestApiProps` and ensures that `proxy` cannot be set differently from the base `proxy` flag (but discourages setting it at all). This will allow setting the `cacheKeyParameters`, `allowTestInvoke`, and `vpcLink` configuration fields without having to fall back to using a regular `RestApi`. Some of this is inspired by aws#12004 and its review comments, which was closed earlier this year due to inactivity. Resolves: aws#3269 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added the @aws-cdk/aws-apigateway Related to Amazon API Gateway label Oct 20, 2021

github-actions bot assigned nija-at Oct 20, 2021

laurelmay mentioned this pull request Oct 27, 2021

ApiGateway LambdaRestApi doesn't allow overriding default integration properties #3269

Closed

5 tasks

nija-at added effort/small Small work item – less than a day of effort p2 labels Nov 5, 2021

nija-at removed their assignment Nov 5, 2021

laurelmay force-pushed the lambdarestapi-lambdaintegrationoptions branch from 643796e to 87e3a45 Compare November 8, 2021 20:51

laurelmay force-pushed the lambdarestapi-lambdaintegrationoptions branch 3 times, most recently from a6e655c to c7f8a77 Compare December 1, 2021 22:19

rix0rrr added feature-request A feature should be added or improved. p2 and removed p2 @aws-cdk/aws-apigateway Related to Amazon API Gateway effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. labels Mar 4, 2022

laurelmay force-pushed the lambdarestapi-lambdaintegrationoptions branch 2 times, most recently from 2714a44 to 1ff28a7 Compare March 31, 2022 03:45

laurelmay force-pushed the lambdarestapi-lambdaintegrationoptions branch from 1ff28a7 to 31c3a42 Compare March 31, 2022 04:33

comcalvi reviewed Apr 5, 2022

View reviewed changes

comcalvi self-assigned this Apr 5, 2022

comcalvi previously requested changes Apr 7, 2022

View reviewed changes

comcalvi reviewed Apr 7, 2022

View reviewed changes

laurelmay force-pushed the lambdarestapi-lambdaintegrationoptions branch from 31c3a42 to a561d12 Compare April 8, 2022 01:16

laurelmay force-pushed the lambdarestapi-lambdaintegrationoptions branch from a561d12 to a751a41 Compare April 8, 2022 01:22

laurelmay added 3 commits May 5, 2022 22:49

feat(apigateway): Add LambdaIntegrationOptions to LambdaRestApi

2b609e3

Refactor based on updated proxy attribute understanding

67c56fd

Update to use new integration tests

9b07f25

laurelmay force-pushed the lambdarestapi-lambdaintegrationoptions branch from a751a41 to 9b07f25 Compare May 6, 2022 03:09

comcalvi previously approved these changes Jun 15, 2022

View reviewed changes

comcalvi added the pr/do-not-merge This PR should not be merged at this time. label Jun 15, 2022

comcalvi changed the base branch from v1-main to main June 15, 2022 00:04

comcalvi removed the pr/do-not-merge This PR should not be merged at this time. label Jun 15, 2022

Merge branch 'main' into lambdarestapi-lambdaintegrationoptions

37fa5b2

comcalvi approved these changes Jun 22, 2022

View reviewed changes

Merge branch 'main' into lambdarestapi-lambdaintegrationoptions

31b7e70

mergify bot merged commit b117469 into aws:main Jun 22, 2022

		Note that `proxy: false` may not be specified in the `integrationOptions`
		and must be specified directly in `props`.

Conversation

laurelmay commented Oct 20, 2021

Uh oh!

gitpod-io bot commented Oct 20, 2021

Uh oh!

laurelmay commented Mar 31, 2022

Uh oh!

comcalvi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comcalvi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comcalvi Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laurelmay commented Apr 8, 2022

Uh oh!

mergify bot commented Jun 15, 2022

Uh oh!

mergify bot commented Jun 15, 2022

Uh oh!

mergify bot commented Jun 22, 2022

Uh oh!

aws-cdk-automation commented Jun 22, 2022

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jun 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

comcalvi Apr 7, 2022 •

edited

Loading