Skip to content

Bug: APIGatewayRestResolver fails to route when "/" appears at end of route name #1552

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

Closed
walmsles opened this issue Sep 29, 2022 · 8 comments · Fixed by #1609
Closed

Bug: APIGatewayRestResolver fails to route when "/" appears at end of route name #1552

walmsles opened this issue Sep 29, 2022 · 8 comments · Fixed by #1609
Assignees
Labels
feature-request feature request v2

Comments

@walmsles
Copy link
Contributor

walmsles commented Sep 29, 2022

Expected Behaviour

Given I am using APIGatewayRestResolver and I define a route as follows:

from aws_lambda_powertools.event_handler import APIGatewayRestResolver

app = APIGatewayRestResolver()

@app.get("/hello")
def hello_world():
    return {"message": "Hello World"}

I expect the following 2 GET request URLs:

  1. https://myapigwid.execute-api.ap-southeast-2.amazonaws.com/Prod/hello/
  2. https://myapigwid.execute-api.ap-southeast-2.amazonaws.com/Prod/hello

to return the same response:

{"message":"Hello World"}

Current Behaviour

Given I am using APIGatewayRestResolver and I define a route as follows:

@app.get("/hello")
def hello_world():
    return {"message": "Hello World"}

When I use the following URL:

  1. https://myapigwid.execute-api.ap-southeast-2.amazonaws.com/Prod/hello/

AWS Lambda Powertools returns a 404 not found response:

{"statusCode":404,"message":"Not found"}

Code snippet

from aws_lambda_powertools.event_handler import APIGatewayRestResolver
from aws_lambda_powertools import Logger

app = APIGatewayRestResolver()
logger = Logger()

@app.get("/hello")
def hello_world():
    return {"message": "Hello World"}

@logger.inject_lambda_context(log_event=True)
def lambda_handler(event, context):
    logger.info("Executing app.resolve")
    return app.resolve(event, context)

Possible Solution

Route resolution uses the path attribute of the AWS API Gateway Lambda event so that the 2 URLs (one with trailing "/") are no longer the same even though the API Gateway Service treats the routes correctly and executes the expected Lambda function.

I would suggest that during route resolution trailing "/" characters be removed and not considered in route resolution.

Unsure if this would be considered a breaking change and whether any customers are using this behaviour - I would not think this behaviour would be in use y customers but am unsure 😬

Steps to Reproduce

Create and deploy the SAM CLI Hello World function then replace the app.py with my code snippet above which has replaced it with AWS Lambda powertools resolver once deployed try loading the API in browser with and without trailing "/" character

AWS Lambda Powertools for Python version

latest

AWS Lambda function runtime

3.9

Packaging format used

PyPi

Debugging logs

START RequestId: a6a6f7b6-84db-4e1b-bfa6-59af38e9927d Version: $LATEST  2022/09/29/[$LATEST]c9ddb616b59546349b99ceaf1ef78e36

2022-09-29T17:49:38.102+10:00   {"level":"INFO","location":"decorate:352","message":{"resource":"/hello","path":"/hello","httpMethod":"GET",....} 2022/09/29/[$LATEST]c9ddb616b59546349b99ceaf1ef78e36

2022-09-29T17:49:38.102+10:00   {"level":"INFO","location":"lambda_handler:26","message":"Executing app.resolve","timestamp":"2022-09-29 07:49:38,102+0000","service":"service_undefined","cold_start":false,"function_name":"sam-app-HelloWorldFunction-nF3rtaBXzZ7S","function_memory_size":"128","function_arn":"arn:aws:lambda:ap-southeast-2:308836149415:function:sam-app-HelloWorldFunction-nF3rtaBXzZ7S","function_request_id":"a6a6f7b6-84db-4e1b-bfa6-59af38e9927d","xray_trace_id":"1-63354e12-521e23257116158e4057dc1e"}    2022/09/29/[$LATEST]c9ddb616b59546349b99ceaf1ef78e36

2022-09-29T17:49:38.103+10:00   END RequestId: a6a6f7b6-84db-4e1b-bfa6-59af38e9927d 2022/09/29/[$LATEST]c9ddb616b59546349b99ceaf1ef78e36

2022-09-29T17:49:38.103+10:00   REPORT RequestId: a6a6f7b6-84db-4e1b-bfa6-59af38e9927d Duration: 1.88 ms Billed Duration: 2 ms Memory Size: 128 MB Max Memory Used: 57 MB   2022/09/29/[$LATEST]c9ddb616b59546349b99ceaf1ef78e36

2022-09-29T17:49:41.754+10:00   START RequestId: e119c41b-45a7-464b-8931-cde0f6b2b51e Version: $LATEST  2022/09/29/[$LATEST]c9ddb616b59546349b99ceaf1ef78e36

2022-09-29T17:49:41.755+10:00   {"level":"INFO","location":"decorate:352","message":{"resource":"/hello","path":"/hello/","httpMethod":"GET",....} 2022/09/29/[$LATEST]c9ddb616b59546349b99ceaf1ef78e36

2022-09-29T17:49:41.755+10:00   {"level":"INFO","location":"lambda_handler:26","message":"Executing app.resolve","timestamp":"2022-09-29 07:49:41,755+0000","service":"service_undefined","cold_start":false,"function_name":"sam-app-HelloWorldFunction-nF3rtaBXzZ7S","function_memory_size":"128","function_arn":"arn:aws:lambda:ap-southeast-2:308836149415:function:sam-app-HelloWorldFunction-nF3rtaBXzZ7S","function_request_id":"e119c41b-45a7-464b-8931-cde0f6b2b51e","xray_trace_id":"1-63354e15-4cad301429daa2311eaa530a"}    2022/09/29/[$LATEST]c9ddb616b59546349b99ceaf1ef78e36

2022-09-29T17:49:41.756+10:00   END RequestId: e119c41b-45a7-464b-8931-cde0f6b2b51e 2022/09/29/[$LATEST]c9ddb616b59546349b99ceaf1ef78e36

2022-09-29T17:49:41.756+10:00   REPORT RequestId: e119c41b-45a7-464b-8931-cde0f6b2b51e Duration: 1.68 ms Billed Duration: 2 ms Memory Size: 128 MB Max Memory Used
@walmsles walmsles added bug Something isn't working triage Pending triage from maintainers labels Sep 29, 2022
@heitorlessa heitorlessa removed the triage Pending triage from maintainers label Sep 29, 2022
@heitorlessa
Copy link
Contributor

Hey @walmsles, thanks for flagging this.

API Gateway actually doesn't resolve trailing slash (unless that changed). It does work when you have a greedy path - /{proxy+}.

Would you like to send a fix so we can support both? Tests should be able to catch any breaking change

@walmsles
Copy link
Contributor Author

hmmm ... In my test project Lambda was executed for both trailing slash and not trailing slash.

Let me clarify the behaviour - its been working this way from memory for a while now

Will look at testing across the various resolvers as well.
Let me confirm the behaviours everywhere and come back - happy to put a pull-request together for this one.

@heitorlessa heitorlessa added feature-request feature request area/event_handlers and removed bug Something isn't working labels Sep 29, 2022
@heitorlessa
Copy link
Contributor

No at all. I changed the labels accordingly.

The reason that API GW (ALB too IIRC) shouldn't work is because they are technically two distinct URIs - although we both know they are pointing to the same resource ("hello").

From memory, this is as old as Apache HTTPD (prolly even before). The closest authoritative source I can find is RFC 3986 on Path.

@walmsles
Copy link
Contributor Author

Have read the RFC which is not exactly clear but looking at how URLs work from Apache HTTPD days - it is correct that /hello !== /hello/ so this is definitely not a bug.

I did discover that both API Gateway REST and HTTPV2 route both /hello and /hello/ to the Lambda regardless of configuration of:

  • Path: /hello
  • Path: /hello/
  • Path: /{proxy+}

of course /{proxy+} will cause Lambda to be invoked always as expected and the router result is correct.

Curiously RestAPI (V1) with a path defined as "/hello/" is deployed with a path of "/hello" and when you hit "/hello" and "/hello/" are all routed to the Lambda.

For this reason, I wonder if the API gateway Resolvers and only API gateway Resolvers should ignore the trailing "/" when resolving routes?

@walmsles
Copy link
Contributor Author

Tested further on HTTP Api - unable to register a definite route ending in trailing slash is ignored by the API gateway, and the route is simply not registered - unsure if this is documented behaviour, but that's how ap-southeast-2 is behaving today 🤷

Function URL has no routing applied to it, so the current behaviour makes sense.

@heitorlessa wanted to get your thoughts on this one before I go off and do anything. My main thinking is APIGateway resolvers should route "hello" and "hello/" the same since if I did not use the router at all and just a lambda handler with an event - this is the outcome I get, so consistency of using Lambda Powertools or not should be a destination this project should head towards.

@heitorlessa
Copy link
Contributor

Apologies for the delay, handling a sensitive issue in the last 24 hours. Back now.

RestAPI (v1) route both "/hello/" and "/hello to the same Lambda, so it does HTTP API.

This is surprising. I vividly remember REST API failing with authorization token when the route had a trailing slash.

about consistency

I agree with you, we should match the experience on API Gateway. We should also have an E2E test to be sure this won't bite us in the future. It could be easier to test with ALB too via E2E as we already have one.

Feel free to shout if you need any help with the E2E test. It should be easy to add a new function, but the details are here if you ever need: https://github.com/awslabs/aws-lambda-powertools-python/blob/develop/MAINTAINERS.md#run-end-to-end-tests

Thanks a lot for digging on this!

@heitorlessa
Copy link
Contributor

Closing as we're wrap to launch V2

@github-actions
Copy link
Contributor

⚠️COMMENT VISIBILITY WARNING⚠️

This issue is now closed. Please be mindful that future comments are hard for our team to see.

If you need more assistance, please either tag a team member or open a new issue that references this one.

If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request feature request v2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants