Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

defaultMethodOptions in SpecRestApi is confusing #8347

Closed
john-tipper opened this issue Jun 3, 2020 · 9 comments · Fixed by #8270 or #9099
Closed

defaultMethodOptions in SpecRestApi is confusing #8347

john-tipper opened this issue Jun 3, 2020 · 9 comments · Fixed by #8270 or #9099
Assignees
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway bug This issue is a bug. p2

Comments

@john-tipper
Copy link

The documentation states about defaultIntegration:

An integration to use as a default for all methods created within this API unless an integration is specified.

Yet when I define an API with no integration defined within the OpenAPI spec, the default integration is ignored and an error occurs when deploying.

Reproduction Steps

SpecRestApi openapiRestApi = SpecRestApi.Builder.create(this, "MyApi")
						.restApiName("MyApi")
						.apiDefinition(ApiDefinition.fromAsset("openapi.yaml"))
						.defaultIntegration(LambdaIntegration.Builder.create(myLambda)
									.proxy(false)
									.build())
						.deploy(true)
						.build();

where myLambda is a lambda defined earlier in the CDK.

The OpenApi spec has a single method defined (the header and model are not shown):

paths:
  /items:
    get:
      summary: List all items.
      responses:
        '200':
          description: OK
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/ItemList'

Error Log

The following error appears in the CloudFormation events when deploying:

No integration defined for method (Service: AmazonApiGateway; Status Code: 400; Error Code: BadRequestException; Request ID: 56113150-1460-4ed2-93b9-a12618864582)

Environment

  • CLI Version : 1.42.0
  • Framework Version: 1.42.1
  • Node.js Version: v12.16.0
  • OS : Windows
  • Language (Version): Java

Other


This is 🐛 Bug Report

@john-tipper john-tipper added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 3, 2020
@john-tipper
Copy link
Author

This could simply be me not reading the documentation correctly, but on first pass, I think the documentation suggests that I shouldn't need to define anything with x-amazon-apigateway-integration inside the OpenAPI spec if there is a default integration specified within the SpecRestApi.

If I need to specify a lambda within a x-amazon-apigateway-integration stanza in my OpenAPI spec then I don't see how to do this if the lambda is also defined as part of the CDK stack as it seems like a chick-and-egg problem.

@SomayaB SomayaB added the @aws-cdk/aws-apigateway Related to Amazon API Gateway label Jun 5, 2020
@isaacliu
Copy link

isaacliu commented Jun 7, 2020

Observed same problem today with CDK 1.42.0 (build 3b64241).

The 'defaultIntegration' was ignored when create new 'SpecRestApi'. Specified AwsIntegration was not picked up and result error ('No integration defined for method') when deploy and failed.

@nija-at nija-at added p1 and removed needs-triage This issue or PR still needs to be triaged. labels Jun 10, 2020
@nija-at
Copy link
Contributor

nija-at commented Jun 11, 2020

This will be fixed with #8270.

However, note that, the defaultIntegration option and others similar will only apply to Methods and Resources defined in the CDK. They will not apply to the Resources and Methods in the OpenAPI spec.
As suggested by @john-tipper, you will need to configure these in the spec directly using the APIGateway extensions.

@mergify mergify bot closed this as completed in #8270 Jun 12, 2020
mergify bot pushed a commit that referenced this issue Jun 12, 2020
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
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*
@mmuller88
Copy link
Contributor

Mhh it doesn't look like that the chick-and-egg problem is solved. I can't see that the code handles that somehow. I worked around that with deploying two times and doing a json merge to have the x-amazon-apigateway-integration into the spec. I described it in the Workaround section here: https://martinmueller.dev/cdk-swagger-eng/ but yeah is kind of hard to explain.

@gregorypierce
Copy link

I'm still seeing this as well, just in Typescript.

I am creating a LambdaFunction and a LambdaIntegration for that function so that they can be the default handler. If I give that toa SpecRestApi, it just ignores it and throws an exception:

No integration defined for method (Service: AmazonApiGateway; Status Code: 400; Error Code: BadRequestException; Request ID: fcf92d52-dab4-4c7e-9b0c-598f77ed5ba3)

@nija-at
Copy link
Contributor

nija-at commented Jun 30, 2020

Re-opening to investigate.

@nija-at nija-at reopened this Jun 30, 2020
@nija-at nija-at removed the p1 label Jun 30, 2020
@nija-at
Copy link
Contributor

nija-at commented Jun 30, 2020

Re-reading some of the comments here.

The CDK does not open and read the OpenAPI definition files. This means that any value provided under defaultIntegration or any other property in the CDK application will only apply to Methods and Resources in the CDK app. We are not able to (and currently do not) apply these to the Methods and Resources defined in the OpenAPI definitions.

Perhaps having a defaultIntegration property on SpecRestApi is creating this confusion. Would it be less ambiguous if this property was removed entirely and users had to explicitly define the integration for each API?

If I've misunderstood and this is not what you're trying to do, could you please clarify and provide code examples for reproduction?

@nija-at nija-at added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jun 30, 2020
@john-tipper
Copy link
Author

john-tipper commented Jun 30, 2020

This means that any value provided under defaultIntegration or any other property in the CDK application will only apply to Methods and Resources in the CDK app. We are not able to (and currently do not) apply these to the Methods and Resources defined in the OpenAPI definitions.

@nija-at Actually, I think it might just be fine to call this out clearly in the documentation, as I had interpreted the name defaultIntegration to mean exactly that, i.e. if I do nothing then it is the integration used, come what may, whereas it is only ever used if the API is defined using CDK Methods and Resources.

@mmuller88 I got around the chicken and the egg problem by templating the API spec, rather than having to do two deployments and I used Mustache. So, for a CDK defined API in Java, I added this to the dependencies:

implementation 'com.github.spullara.mustache.java:compiler:0.9.6'

Then, within the API spec, I defined things like:

x-amazon-apigateway-integration:
uri: "{{restapi-lambda}}"
passthroughBehavior: "when_no_match"
httpMethod: "POST"
type: "aws_proxy"
credentials: "{{restapi-role}}"

Finally, within the CDK definition, I did this:

Map<String, Object> variables = new HashMap<>();
variables.put("restapi-lambda", String.format(
    "arn:aws:apigateway:%s:lambda:path/2015-03-31/functions/%s/invocations",
    props.getEnv()
       .getRegion(),
    myRestHandler.getFunctionArn()));
variables.put("restapi-role", apiGatewayRole.getRoleArn());

Writer writer = new StringWriter();
MustacheFactory mf = new DefaultMustacheFactory();

Object openapiSpecAsObject;
try (Reader reader = new FileReader(new File("openapi.yaml"))) {
  Mustache mustache = mf.compile(reader, "OAS");
  mustache.execute(writer, variables);
  writer.flush();

  ObjectMapper yamlMapper = new ObjectMapper(new YAMLFactory());

  openapiSpecAsObject = yamlMapper.readValue(writer.toString(), Object.class);

}

SpecRestApi openapiRestApi = SpecRestApi.Builder.create(this, "MyRestApi")
                        .restApiName("MyApi")
                        .apiDefinition(ApiDefinition.fromInline(openapiSpecAsObject))
                        .deploy(true)
                        .deployOptions(StageOptions.builder()
                                       .stageName("prod")
                                       .build())
                        .build();

I found some bizarre dependency issues that sometimes cropped up where the deployment seemed to happen before the Lambda permissions had been created and then the ApiGateway would not be able to invoke the functions, even though the permissions had been set. I'd have to re-deploy the API in the console in order to have the permissions set correctly, so I can only assume that the ApiGateway, when it creates a deployment, takes a copy of the Lambda permissions as they exist at that moment of deployment, and any subsequent changes have no effect. To get around this, I needed to do the following:

CfnPermission restPermission = CfnPermission.Builder.create(this, "GrantPermissionToApiGateway")
									.action("lambda:InvokeFunction")
									.principal("apigateway.amazonaws.com")
									.sourceArn(String.format(
											"arn:aws:execute-api:%s:%s:*",
											props.getEnv()
												 .getRegion(),
											props.getEnv()
												 .getAccount()))
									.functionName(myRestHandler.getFunctionName())
									.build();


openapiRestApi.getNode()
	  .addDependency(apiGatewayRole);

openapiRestApi.getNode()
	  .addDependency(restPermission);

openapiRestApi.getDeploymentStage()
	  .getNode()
	  .addDependency(restPermission);

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jul 1, 2020
@gregorypierce
Copy link

gregorypierce commented Jul 2, 2020 via email

@nija-at nija-at changed the title SpecRestApi defaultIntegration appears not to work defaultMethodOptions in SpecRestApi has inconsistent behaviour - consider its removal Jul 15, 2020
@nija-at nija-at changed the title defaultMethodOptions in SpecRestApi has inconsistent behaviour - consider its removal defaultMethodOptions in SpecRestApi is confusing - consider its removal Jul 15, 2020
nija-at pushed a commit that referenced this issue Jul 16, 2020
SpecRestApiProps contained `defaultMethodOptions`,
`defaultCorsPreflightOptions` and `defaultIntegration` as properties.
However, these defaults are only applied to Methods and Resources
defined via addMethod() and addResource() APIs. These do *not* apply to
Resources and Methods defined in the OpenAPI spec.

Users have complained that this is unclear[1][2]. This PR removes these
options from the constructor properties of `SpecRestApi`. Users can
still specify these when adding new Resources and new Methods.

These options don't make much sense to be specified on a `SpecRestApi`
since they cannot be applied to the Resources and Methods in the OpenAPI
spec. This will match up with the functionality available.

closes #8347

[1]: #8347 (comment)
[2]: #8347 (comment)

BREAKING CHANGE: `defaultMethodOptions`, `defaultCorsPreflightOptions`
and `defaultIntegration` have been removed from `SpecRestApiProps`.
@nija-at nija-at added the p2 label Jul 16, 2020
@nija-at nija-at changed the title defaultMethodOptions in SpecRestApi is confusing - consider its removal defaultMethodOptions in SpecRestApi is confusing Jul 16, 2020
@mergify mergify bot closed this as completed in #9099 Jul 16, 2020
mergify bot pushed a commit that referenced this issue Jul 16, 2020
SpecRestApiProps contained `defaultMethodOptions`,
`defaultCorsPreflightOptions` and `defaultIntegration` as properties.
However, these defaults are only applied to Methods and Resources
defined via addMethod() and addResource() APIs. These do *not* apply to
Resources and Methods defined in the OpenAPI spec.

Users have complained that this is unclear [1] [2]. This PR removes
these options from the constructor properties of `SpecRestApi`. Users
can still specify these when adding new Resources and new Methods.

These options don't make much sense to be specified on a `SpecRestApi`
since they cannot be applied to the Resources and Methods in the OpenAPI
spec. This will match up with the functionality available.

closes #8347

[1]: #8347 (comment)
[2]: #8347 (comment)

BREAKING CHANGE: `defaultMethodOptions`, `defaultCorsPreflightOptions`
and `defaultIntegration` have been removed from `SpecRestApiProps`.
These can be specifed directly in the OpenAPI spec or via `addMethod()`
 and `addResource()` APIs.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
curtiseppel pushed a commit to curtiseppel/aws-cdk that referenced this issue Aug 11, 2020
)

SpecRestApiProps contained `defaultMethodOptions`,
`defaultCorsPreflightOptions` and `defaultIntegration` as properties.
However, these defaults are only applied to Methods and Resources
defined via addMethod() and addResource() APIs. These do *not* apply to
Resources and Methods defined in the OpenAPI spec.

Users have complained that this is unclear [1] [2]. This PR removes
these options from the constructor properties of `SpecRestApi`. Users
can still specify these when adding new Resources and new Methods.

These options don't make much sense to be specified on a `SpecRestApi`
since they cannot be applied to the Resources and Methods in the OpenAPI
spec. This will match up with the functionality available.

closes aws#8347

[1]: aws#8347 (comment)
[2]: aws#8347 (comment)

BREAKING CHANGE: `defaultMethodOptions`, `defaultCorsPreflightOptions`
and `defaultIntegration` have been removed from `SpecRestApiProps`.
These can be specifed directly in the OpenAPI spec or via `addMethod()`
 and `addResource()` APIs.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway bug This issue is a bug. p2
Projects
None yet
6 participants