fix(aws-apigateway): make LambdaRestApi proxy by default #963

rix0rrr · 2018-10-18T14:41:40Z

The LambdaRestApi construct now takes a 'proxyAll' argument to enable
forwarding all requests to the given Lambda Function.

BREAKING CHANGE: specifying a path no longer works. If you used to
provide a '/', now supply true. Otherwise, you will have to construct
more complex resource paths yourself.

Fixes #959.

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

The LambdaRestApi construct now takes a 'proxyAll' argument to enable forwarding all requests to the given Lambda Function. BREAKING CHANGE: specifying a path no longer works. If you used to provide a '/', now supply true. Otherwise, you will have to construct more complex resource paths yourself. Fixes #959.

eladb · 2018-10-18T16:19:46Z

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

   * `addResource` and `addMethod` (or `addProxy`).
   *
-   * @default undefined
+   * @default false


We want the default to be true

eladb · 2018-10-18T16:20:22Z

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

-      curr.addProxy();
+    if (props.proxyAll) {
+      this.root.addProxy();
+      this.root.addMethod('ANY');


I am wondering if this should always be done in addProxy...

eladb · 2018-10-18T16:20:58Z

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

-    // THEN
+    // THEN -- can't customize further
+    test.throws(() => {
+      api.root.addResource('cant-touch-this');


eladb

Please update README and move implementation of root.ANY to ProxyResource.

eladb · 2018-10-19T08:20:41Z

packages/@aws-cdk/aws-apigateway/lib/restapi.ts

@@ -214,6 +214,8 @@ export class RestApi extends RestApiRef implements cdk.IDependable {
        return new Method(this, httpMethod, { resource: this.root, httpMethod, integration, options });
      },
      addProxy: (options?: ResourceOptions) => {
+        // Must have ANY method on the root when doing this
+        this.root.addMethod('ANY');


Would have been nicer if you put this logic in ProxyResource in case someone uses it directly. Ideally we want the addXxx APIs to behave exactly like using the APIGW constructs directly. You should be able to identify that "parent" is "root" by going back to the API object

eladb

Also, I think this function is no longer needed.

eladb · 2018-10-19T08:41:25Z

packages/@aws-cdk/aws-apigateway/lib/resource.ts

@@ -179,6 +179,11 @@ export class ProxyResource extends Resource {
      defaultMethodOptions: props.defaultMethodOptions,
    });

+    // Add ANY method to parent but only if it's the root


Didn't we say we want this behavior in an override of addMethod, so it will apply to methods added manually as well?

rix0rrr · 2018-10-19T08:46:30Z

https://github.com/awslabs/aws-cdk/blob/master/packages/@aws-cdk/aws-apigateway/lib/stage.ts#L217

eladb

Looks good!

eladb · 2018-10-19T09:29:37Z

@rix0rrr merge?

### Highlights - __A new construct library for AWS Step Functions__ ([docs](https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-stepfunctions/README.md)). The library provides rich APIs for modeling state machines by exposing a programmatic interface for [Amazon State Language](https://docs.aws.amazon.com/step-functions/latest/dg/concepts-amazon-states-language.html). - __A new construct library for Amazon S3 bucket deployments__ ([docs](https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-s3-deployment/README.md)). You can use now automatically populate an S3 Bucket from a .zip file or a local directory. This is a building block for end-to-end support for static websites in the AWS CDK. ### Bug Fixes * **aws-apigateway:** make LambdaRestApi proxy by default ([#963](#963)) ([a5f5e2c](a5f5e2c)), closes [#959](#959) * **aws-cdk:** Allow use of assumed roles behind a proxy ([#898](#898)) ([f2b1048](f2b1048)) * **aws-cdk:** Auto-delete stacks that failed creating before new attempt ([#917](#917)) ([2af8309](2af8309)) * **aws-cloudfront:** expose distributionId ([#938](#938)) ([f58d98c](f58d98c)) * **aws-dynamodb:** don't emit empty array properties ([#909](#909)) ([841975a](841975a)) * **docs:** use ..code to display file structure in "writing constructs" ([#935](#935)) ([b743362](b743362)) ### Features * **assets:** isZipArchive indicates if this is a zip asset ([#944](#944)) ([65190f9](65190f9)) * **aws-cdk:** deploy supports CloudFormation Role ([#940](#940)) ([393be6f](393be6f)), closes [#735](#735) * **aws-cloudformation:** allow specifying custom resource type ([#943](#943)) ([9de3a84](9de3a84)) * **aws-cloudformation:** correctly handle the templateConfiguration property in the CreateUpdateStack Pipeline Action. ([#923](#923)) ([d251a46](d251a46)) * **aws-cloudfront:** add support for "webAclId" ([#969](#969)) ([3ec9d76](3ec9d76)) * **aws-codedeploy:** add auto rollback configuration to server Deployment Group. ([#925](#925)) ([7ee91cf](7ee91cf)) * **aws-codedeploy:** add instance tag filter support for server Deployment Groups. ([#824](#824)) ([e6e8c51](e6e8c51)) * **aws-codedeploy:** add support for setting CloudWatch alarms on a server Deployment Group. ([#926](#926)) ([27b26b1](27b26b1)) * add support for Step Functions ([#827](#827)) ([81b533c](81b533c)) * **aws-lambda:** add grantInvoke() method ([#962](#962)) ([1ee8135](1ee8135)), closes [#961](#961) * **aws-lambda:** improvements to the code and runtime APIs ([#945](#945)) ([36f29b6](36f29b6)), closes [#902](#902) [#188](#188) [#947](#947) [#947](#947) [#664](#664) * **aws-logs:** extractMetric() returns Metric object ([#939](#939)) ([5558fff](5558fff)), closes [#850](#850) * **aws-s3:** initial support for website hosting ([#946](#946)) ([2d3661c](2d3661c)) * **aws-s3-deployment:** bucket deployments ([#971](#971)) ([84d6876](84d6876)), closes [#952](#952) [#953](#953) [#954](#954) * **docs:** added link to CloudFormation concepts ([#934](#934)) ([666bbba](666bbba)) ### BREAKING CHANGES * **aws-apigateway:** specifying a path no longer works. If you used to provide a '/', remove it. Otherwise, you will have to supply `proxy: false` and construct more complex resource paths yourself. * **aws-lambda:** The construct `lambda.InlineJavaScriptLambda` is no longer supported. Use `lambda.Code.inline` instead; `lambda.Runtime.NodeJS43Edge` runtime is removed. CloudFront docs [stipulate](https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/lambda-requirements-limits.html#lambda-requirements-lambda-function-configuration) that you should use node6.10 or node8.10. It is always possible to use any value by instantiating a `lambda.Runtime` object.

rix0rrr requested a review from eladb October 18, 2018 14:41

eladb suggested changes Oct 18, 2018

View reviewed changes

Make proxy default to true, implicitly add ANY to root

4566544

eladb reviewed Oct 19, 2018

View reviewed changes

Rico Huijbers added 2 commits October 19, 2018 10:27

Update README, move addMethod() logic into the proxy class

751cf28

Move parent-proxying behavior to addMethod()

dd839da

eladb reviewed Oct 19, 2018

View reviewed changes

rix0rrr changed the title ~~fix(aws-apigateway): change 'proxyPath' to 'proxyAll'~~ fix(aws-apigateway): make LambdaRestApi proxy by default Oct 19, 2018

eladb approved these changes Oct 19, 2018

View reviewed changes

rix0rrr merged commit a5f5e2c into master Oct 19, 2018

rix0rrr deleted the huijbers/apigateway-default branch October 19, 2018 09:42

eladb mentioned this pull request Oct 19, 2018

v0.13.0 #975

Merged

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(aws-apigateway): make LambdaRestApi proxy by default #963

fix(aws-apigateway): make LambdaRestApi proxy by default #963

rix0rrr commented Oct 18, 2018

eladb Oct 18, 2018

eladb Oct 18, 2018

eladb Oct 18, 2018

eladb left a comment

eladb Oct 19, 2018

eladb left a comment

eladb Oct 19, 2018

rix0rrr commented Oct 19, 2018

eladb left a comment

eladb commented Oct 19, 2018

fix(aws-apigateway): make LambdaRestApi proxy by default #963

fix(aws-apigateway): make LambdaRestApi proxy by default #963

Conversation

rix0rrr commented Oct 18, 2018

eladb Oct 18, 2018

Choose a reason for hiding this comment

eladb Oct 18, 2018

Choose a reason for hiding this comment

eladb Oct 18, 2018

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

eladb Oct 19, 2018

Choose a reason for hiding this comment

eladb left a comment

Choose a reason for hiding this comment

eladb Oct 19, 2018

Choose a reason for hiding this comment

rix0rrr commented Oct 19, 2018

eladb left a comment

Choose a reason for hiding this comment

eladb commented Oct 19, 2018