feat(apigateway): define Resources on imported RestApi #8270

nija-at · 2020-05-29T09:47:49Z

The apigateway CDK construct library creates a large number of
CloudFormation resources.
To define a single Method that is backed with a Lambda function and
authorizer with an Authorizer, ~8 CloudFormation resources are created.
This quickly grows and a reasonable sized RestApi is bound to hit the
200 resource limit on their stack.

Re-organizing the CDK app into multiple Stacks that share the same
instance of RestApi will not resolve this problem, since the CDK
still makes an executive decision to keep the Methods and Resources in
the same stack as the RestApi construct. (see #7391)

This change introduces a new import style API fromRestApiAttributes()
that returns an instance of IRestApi that allows for new Resources to
be defined across stacks.

As a nice side effect, this change also adds the ability to define
Resources on SpecRestApi in addition to what has already been defined in
the OpenAPI spec.

closes #1477
closes #7391
fixes #8347

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

The apigateway CDK construct library creates a large number of CloudFormation resources. To define a single Method that is backed with a Lambda function and authorizer with an Authorizer, ~8 CloudFormation resources are created. This quickly grows and a reasonable sized RestApi is bound to hit the 200 resource limit on their stack. Re-organizing the CDK app into multiple `Stack`s that share the same instance of `RestApi` will not resolve this problem, since the CDK still makes an executive decision to keep the Methods and Resources in the same stack as the `RestApi` construct. (see #7391) This change introduces a new import style API `fromRestApiAttributes()` that returns an instance of `IRestApi` that allows for new Resources to be defined across stacks. As a nice side effect, this change also adds the ability to define Resources on SpecRestApi in addition to what has already been defined in the OpenAPI spec. closes #1477

aws-cdk-automation · 2020-05-29T10:09:43Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: ad39968
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-05-29T11:36:25Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: fc760a9
Result: SUCCEEDED
Build Logs (available for 30 days)

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

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

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

eladb · 2020-06-09T05:20:44Z

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

  }
 }

 export class Resource extends ResourceBase {
  public readonly parentResource?: IResource;
-  public readonly restApi: RestApi;
+  public readonly api: MutableRestApi;


Is it possible to just expose IRestApi here and this way avoid exposing MutableRestApi as a public api?

This might require type checks in the code but it will keep the public api cleaner and with less surface.

I've taken another stab at trying to erase this type from the public API. However, it requires some methods to be promoted into IRestApi. Let me know what you think here.

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

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

Feedback addressed

aws-cdk-automation · 2020-06-09T16:21:50Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: fb8a765
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-06-09T16:24:04Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: ce878c0
Result: FAILED
Build Logs (available for 30 days)

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

eladb · 2020-06-10T19:40:58Z

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

+   * Checks if the given object is an instance of RestApiBase.
+   */
+  public static isRestApiBase(x: any): x is IRestApi {
+    return x !== null && typeof(x) === 'object' && RESTAPI_SYMBOL in x;


I believe this symbol is no longer required now that we use peer dependencies. You can just use instanceof

I don't believe this method is needed.

instanceof still does not work in some cases. See 1423c53.

symbol based matching seems more more robust.

cc @RomainMuller who has thoughts on why this should not be instanceof.

It is enough to have ONE package in your dependency closure incorrectly modeling their dependencies for the instanceof idiom to fail. This is a surprisingly easy way to break people's code.

Additionally, the npm de-duplication logic is strictly best-effort, so it is possible that for some reason, npm decides to install multiple copies of some library, and then boom, the instanceof idiom breaks.

In my opinion, instanceof can only be safely used in JavaScript if the tested instance has been created in the same file that performs the instanceof check (guaranteeing the imported types come from the same location).

Aside from the Romain's points, the error message to a customer when they don't use peer dependencies is not very informative. In some cases these are not failures either - they just result in the wrong template or state.

Using the current symbol based approach just works in all these cases.

eladb · 2020-06-10T19:42:35Z

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

+    if (RestApiBase.isRestApiBase(props.resource.api)) {
+      (props.resource.api as any)._attachMethod(this);
+    }


This should work (the static method is redundant and you don't need that as any):

Suggested change

if (RestApiBase.isRestApiBase(props.resource.api)) {

(props.resource.api as any)._attachMethod(this);

}

if (props.resource.api instanceof RestApiBase) {

props.resource.api._attachMethod(this);

}

The cast would not be needed if isRestApiBase returned x is RestApiBase instead of x is IRestApi.

aws-cdk-automation · 2020-06-11T12:30:25Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 9f7ee9b
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-06-11T12:38:03Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: ae3b59f
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-06-12T09:17:30Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: f7dfa3f
Result: FAILED
Build Logs (available for 30 days)

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

mergify · 2020-06-12T10:42:31Z

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

aws-cdk-automation · 2020-06-12T10:42:52Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: bce41e4
Result: SUCCEEDED
Build Logs (available for 30 days)

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

tristanHessell · 2020-06-16T04:25:17Z

@nija-at Is there an expected time-frame for an npm release containing these changes?

nija-at · 2020-06-16T09:44:41Z

It should be available this week.

zubairzahoor · 2020-06-19T16:18:39Z

If an authorizer is created by passing the reference of main restApiId, can that authorizer be used to authenticate the API Endpoints that are defined across stacks?

zubairzahoor · 2020-06-22T13:47:46Z

It works but only with MockIntegration. When I try to use LambdaIntegration(lambdaFn) the following error is thrown:
RestApi is not available on Resource not connected to an instance of RestApi. Use api instead
What am I missing here? @nija-at

nija-at · 2020-06-22T14:52:21Z

Please open a separate issue to report bugs and guidance queries.

### Description of changes Fixes the readme sections order to match the original order when `Breaking up Methods and Resources across Stacks` section was originally added in this PR #8270. The current order does not make sense, this section does not related to the Step function backed rest API ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

nija-at requested review from eladb and a team May 29, 2020 09:47

nija-at self-assigned this May 29, 2020

mergify bot added the contribution/core This is a PR that came from AWS. label May 29, 2020

remove experimental tag

fc760a9

eladb reviewed Jun 2, 2020

View reviewed changes

packages/@aws-cdk/aws-apigateway/lib/resource.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-apigateway/lib/restapi.ts Outdated Show resolved Hide resolved

nija-at requested review from a team and eladb June 4, 2020 17:11

eladb previously requested changes Jun 9, 2020

View reviewed changes

Niranjan Jayakar added 3 commits June 9, 2020 16:56

so far so good

e19e6bb

split restapibase into separate file

b6b8bfe

PR feedback

fb8a765

nija-at requested review from eladb and a team June 9, 2020 16:11

slight adjustment

ce878c0

nija-at commented Jun 9, 2020

View reviewed changes

packages/@aws-cdk/aws-apigateway/lib/private/restapi-base.ts Outdated Show resolved Hide resolved

eladb suggested changes Jun 10, 2020

View reviewed changes

nija-at requested a review from eladb June 11, 2020 09:15

Niranjan Jayakar added 2 commits June 11, 2020 13:19

mark isRestApiBase as @internal

9f7ee9b

update a bit more of the README

ae3b59f

nija-at mentioned this pull request Jun 11, 2020

defaultMethodOptions in SpecRestApi is confusing #8347

Closed

eladb approved these changes Jun 11, 2020

View reviewed changes

unfortunately RestApiBase needs to be exported

f7dfa3f

Merge branch 'master' into apigateway-configureImported

bce41e4

mergify bot merged commit 21a1de3 into master Jun 12, 2020

mergify bot deleted the nija-at/apigateway-configureImported branch June 12, 2020 10:42

crawfobw mentioned this pull request Aug 12, 2021

(aws_apigateway): Requests route to proxy for resources defined by RestApi.from_rest_api_attributes #16000

Closed

gracelu0 mentioned this pull request Apr 19, 2024

fix(events-targets): ApiGateway events target should accept IRestApi #29397

Merged

1 task

moelasmar mentioned this pull request Dec 23, 2024

chore(apigateway): fix the readme sections order. #32650

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(apigateway): define Resources on imported RestApi #8270

feat(apigateway): define Resources on imported RestApi #8270

nija-at commented May 29, 2020 •

edited

Loading

aws-cdk-automation commented May 29, 2020

aws-cdk-automation commented May 29, 2020

eladb Jun 9, 2020

nija-at Jun 9, 2020

aws-cdk-automation commented Jun 9, 2020

aws-cdk-automation commented Jun 9, 2020

eladb Jun 10, 2020

eladb Jun 10, 2020

nija-at Jun 11, 2020 •

edited

Loading

nija-at Jun 11, 2020

RomainMuller Jun 11, 2020

nija-at Jun 11, 2020 •

edited

Loading

eladb Jun 10, 2020

RomainMuller Jun 11, 2020

aws-cdk-automation commented Jun 11, 2020

aws-cdk-automation commented Jun 11, 2020

aws-cdk-automation commented Jun 12, 2020

mergify bot commented Jun 12, 2020

aws-cdk-automation commented Jun 12, 2020

tristanHessell commented Jun 16, 2020

nija-at commented Jun 16, 2020

zubairzahoor commented Jun 19, 2020

zubairzahoor commented Jun 22, 2020

nija-at commented Jun 22, 2020

feat(apigateway): define Resources on imported RestApi #8270

feat(apigateway): define Resources on imported RestApi #8270

Conversation

nija-at commented May 29, 2020 • edited Loading

aws-cdk-automation commented May 29, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 29, 2020

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jun 9, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 9, 2020

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at Jun 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jun 11, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 11, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Jun 12, 2020

AWS CodeBuild CI Report

mergify bot commented Jun 12, 2020

aws-cdk-automation commented Jun 12, 2020

AWS CodeBuild CI Report

tristanHessell commented Jun 16, 2020

nija-at commented Jun 16, 2020

zubairzahoor commented Jun 19, 2020

zubairzahoor commented Jun 22, 2020

nija-at commented Jun 22, 2020

nija-at commented May 29, 2020 •

edited

Loading

nija-at Jun 11, 2020 •

edited

Loading

nija-at Jun 11, 2020 •

edited

Loading