aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.62k stars 3.91k forks source link

aws_apigateway: RestApi add_proxy will create 2 methods for each add_method call #22288

Open klang opened 2 years ago

klang commented 2 years ago

Describe the bug

After creating a proxy resource and manually calling add_method for each method needed, the methods were doubled and present both under the proxy and the root resources.

add_proxy

Expected Behavior

I expected each add_method call made on the resource added to result in one and only one method, like this:

add_resource

Current Behavior

Each call of add_method results in an additional method being added under the root resource.

add_proxy

Reproduction Steps

in apigateway/library_api_stack.py:

from aws_cdk import (Stack,aws_apigateway as api)
from constructs import Construct

class LibraryApiStack(Stack):
    def __init__(self, scope: Construct, construct_id: str, **kwargs) -> None:
        super().__init__(scope, construct_id, **kwargs)
        library = api.RestApi(self, "library")
        other = library.root.add_proxy(any_method=False)
        other.add_method('GET')
        other.add_method('POST')

and in app.py

#!/usr/bin/env python3
import aws_cdk as cdk
from apigateway.library_api_stack import LibraryApiStack

app = cdk.App()
LibraryApiStack(app, "Library")
app.synth()

Possible Solution

simply specifying the add_proxy intended functionality with add_resource (with the right path_part specified) will do the right thing

in apigateway/library_api_stack.py:

from aws_cdk import (Stack,aws_apigateway as api)
from constructs import Construct

class LibraryApiStack(Stack):
    def __init__(self, scope: Construct, construct_id: str, **kwargs) -> None:
        super().__init__(scope, construct_id, **kwargs)
        library = api.RestApi(self, "library")
        other = library.root.add_resource("{proxy+}")
        other.add_method('GET')
        other.add_method('POST')

and in app.py

#!/usr/bin/env python3
import aws_cdk as cdk
from apigateway.library_api_stack import LibraryApiStack

app = cdk.App()
LibraryApiStack(app, "Library")
app.synth()

Additional Information/Context

No response

CDK CLI Version

2.40.0 (build 56ba2ab)

Framework Version

No response

Node.js Version

v18.8.0

OS

macOS Monterey Version 12.5

Language

Python

Language Version

Python 3.9.10

Other information

May be related to https://github.com/aws/aws-cdk/issues/22139

The following is just added to help other users, when they find out that add_proxy doesn't do what they expect it to do.

Adding a proxy via the console, will configure method.request.path.proxy as well. It is quite difficult to find information on how to do that, in CDK.

other.add_method(
        http_method="GET",
        integration=api.HttpIntegration(
            http_method="GET",
            url="http://endpoint/{proxy}',
            options=api.IntegrationOptions(
                request_parameters={"integration.request.path.proxy":"method.request.path.proxy"})),
        request_parameters={"method.request.path.proxy": True})
peterwoodworth commented 1 year ago

This is a deliberate choice to ensure that the root paths are proxied as well: https://github.com/aws/aws-cdk/blob/6e4b4eab65dacea52af7d955575887bbafdff1fc/packages/%40aws-cdk/aws-apigateway/lib/resource.ts#L537-L547

Is this breaking some use case of yours? We could potentially disable this with a prop if not every user is going to want this

klang commented 1 year ago

This is a deliberate choice to ensure that the root paths are proxied as well:

https://github.com/aws/aws-cdk/blob/6e4b4eab65dacea52af7d955575887bbafdff1fc/packages/%40aws-cdk/aws-apigateway/lib/resource.ts#L537-L547

Is this breaking some use case of yours? We could potentially disable this with a prop if not every user is going to want this

In my case, this wouldn't break anything. This would actually give the correct outcome as addMethod wouldn't add anything if a deliberate and conscious decision to include a method on the parent level had already been made.

L2 constructs can be "opinionated", but in most cases they do the right thing. Maybe I was just confused that addProxy took it upon itself to adjust something that I'd already thought about :-)

peterwoodworth commented 1 year ago

Great to hear! maybe we should document this a bit better 🙂

wahab-io commented 1 year ago

Adding a proxy via the console, will configure method.request.path.proxy as well. It is quite difficult to find information on how to do that, in CDK.

other.add_method(
       http_method="GET",
       integration=api.HttpIntegration(
           http_method="GET",
           url="http://endpoint/{proxy}',
           options=api.IntegrationOptions(
               request_parameters={"integration.request.path.proxy":"method.request.path.proxy"})),
       request_parameters={"method.request.path.proxy": True})

Couldn't agree more to the above. @klang this saved a day for me. We should definitely make the docs better. I was doing something similar but with VPC Private Link Integration and after several retries and reading the docs back and forth was able to accomplish

nlb = elbv2.NetworkLoadBalancer(self, "NLB", vpc=vpc)
  vpc_link = apigw.VpcLink(self, "VpcLink", targets=[nlb])

  proxy = "{proxy}"
  integration = apigw.Integration(
      type=apigw.IntegrationType.HTTP_PROXY,
      integration_http_method="ANY",
      options=apigw.IntegrationOptions(
          connection_type=apigw.ConnectionType.VPC_LINK,
          vpc_link=vpc_link,
          request_parameters={
              "integration.request.path.proxy": "method.request.path.proxy"
          },
      ),
      uri=f"http://{nlb.load_balancer_dns_name}/{proxy}",
  )

  api = apigw.RestApi(self, "RestApi")
  api.root.add_proxy(
      any_method=True,
      default_integration=integration,
      default_method_options=apigw.MethodOptions(
          request_parameters={"method.request.path.proxy": True}
      ),
  )
peterwoodworth commented 1 year ago

@wahab-io if you have any suggestions on how to improve this in the docs, feel free to submit a PR 🙂 Else, I'm planning on going through and implementing lots of docs issues within the next month or two and will probably get to this whenever that happens.