-
Notifications
You must be signed in to change notification settings - Fork 403
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
feat(event-handler): prefixes to strip for custom mappings #579
Conversation
@walmsles this might help solve the issue you raised. |
Codecov Report
@@ Coverage Diff @@
## develop #579 +/- ##
========================================
Coverage 99.95% 99.95%
========================================
Files 114 114
Lines 4581 4594 +13
Branches 249 253 +4
========================================
+ Hits 4579 4592 +13
Partials 2 2
Continue to review full report at Codecov.
|
Hi @michaelbrewer it kind of does in a limited way but would think there should be a no code/config option of doing this to use APIGW fully. API GW allows me to create a lambda that can be mapped to multiple domains/mappings and I should be free to do that to mount an API with resolver to multiple domains with potentially different mapping paths without needing to consider environment setup as well. Does the solution allow for multiple mappings? It seems singular. Let me add some more comments/data to the original request to help more. I feel a design change on the actual router implementation would be simpler, less code and more flexible (although might feel like a rewrite so needs careful consideration) |
Ok interesting so you have the same "status" lambda for many different api gateways / mappings and it needs to automagically find this "status" route? With some test events maybe there is a way, but I would not to result in strange breaking behavior. |
@walmsles also note, this would have to work for Api Gateway V1, V2 and ALB lambdas. |
@michaelbrewer understand - not familiar with all the event models for these other API components so maybe there is no nice way - Have added a sample test event to the original request for discussion :-) |
@michaelbrewer fair on support for ALL event formats. What you have proposed is useful - the use case of same lambda to many mappings is an unlikely use case but APIGW does allow/support that. For my exact use case this week - it was the need to having my ApiGatewayResolver configured differently to my APIGW config which felt weird and wrong :-) The prefix idea will assist and resolve this particular use case so happy if that's the outcome. I don't see where I would actually normally use the same APIGW mapped to many custom domains - the use case exists and is where my Software engineering brain went thinking about how APIGW works in this case - but in reality is probably a weird edge case :-) |
@walmsles - i will run some test cases on a deployed lambda to see if there is a method to determine the internally mapped lambda vs the custom mapped lambda |
I'd like us to think how we could solve this on our side as a fallback mechanism without having to ask customers to change their code - whether they use custom domain mappings or not. |
@heitorlessa - any ideas would be great, based on what we get back from the aws events. |
UPDATE: Option 3 won't work as REST API v1 also includes the stage name (see new comment) I totally missed option 3, since that seems to be always true in REST API v1 payload: Since we know upfront whether customers are using it for REST API v1, ALB, or HTTP API, what's the downside of using Replaced def _resolve(self) -> ResponseBuilder:
"""Resolves the response or return the not found response"""
method = self.current_event.http_method.upper()
path = self._extract_path() # NEW
for route in self._routes:
if method != route.method:
continue
match: Optional[re.Match] = route.rule.match(path)
if match:
logger.debug("Found a registered route. Calling function")
return self._call_route(route, match.groupdict()) # pass fn args
logger.debug(f"No match found for path {path} and method {method}")
return self._not_found(method)
# NEW
def _extract_path(self) -> str:
"""Extracts correct path regardless of custom domain mapping setting"""
resolver = self._proxy_type
if self._proxy_type == ProxyEventType.APIGatewayProxyEvent:
return self.current_event.request_context.path |
It sounded too good to be true on only relying on This problem gets further complicated when you have nested mapping paths of variable lengths. This alone will be difficult to keep track in the environment variable as you might have countless of these, if you follow this edge case of having multiple APIs with different mappings pointing to the same Lambda fn. The only solution I can think of is:
Both leak the abstraction but I can't really think of anything else that would work for all scenarios without change one's code, as recommending customers to move to HTTP API isn't a good solution (feature parity!). what do you reckon @walmsles @michaelbrewer? If you agree, we have logic that can be reused to accept both explicit and env var mechanisms. Single custom domain mapping path - {
"path": "/v1/payment",
"resource": "/payment",
"requestContext": {
"resource": "/payment",
"path": "/v1/payment",
"httpMethod": "GET",
"requestContext": {
"resourceId": "j9knhf",
"resourcePath": "/payment",
"httpMethod": "GET",
"path": "/v1/payment",
"stage": "default",
"domainPrefix": "api",
"domainName": "api.serverlessa.dev",
},
}
} Nested custom domain mapping path - Custom domain mapping - proxy resource
{
"path": "/v1/nested/payment/123456789-afekl-13456/",
"resource": "/payment/{invoice+}",
"requestContext": {
"resource": "/payment/{invoice+}",
"path": "/v1/nested/payment/123456789-afekl-13456/",
"httpMethod": "GET",
"requestContext": {
"resourceId": "8em8dt",
"resourcePath": "/payment/{invoice+}",
"httpMethod": "GET",
"path": "/v1/nested/payment/123456789-afekl-13456/",
"stage": "default",
"domainPrefix": "api",
"domainName": "api.serverlessa.dev",
}
}
} |
@michaelbrewer lemme know what types of events you want me to generate to help with tests. I can also help write these too. These events above are actual events coming from API GW hence why my surprise to Your initial solution on |
@heitorlessa - i am sure with more test cases we can come up with a cleaner solutions. Test cases wise: Rest APIThis is the problematic one when mapping the routes against the path, as custom mappings or a direct mapping via Route 53 gives different outcomes.
|
Http API (V1)About the same as the Rest API (although, i think are some differences). So similar test cases as above
HTTP API (V2)Does not seem to need any changes, but a confirmation would help. Application Load BalancerTheir are not custom mapping considerations, so this should just be a regular event. |
I'll set them up tomorrow and dump them here |
@heitorlessa and @michaelbrewer Apologies for being quiet for a while. I think the solution above is workable, I feel it is an edge case (multiple API mappings) but awesome that you have arrived at a solution - let me know if I can assist. |
Dropping most of the events here except Route53 (need to set that up). One issue I noticed on HTTP API too is that the I created the same API in all three versions (REST, HTTP v1, HTTP v2 payload) using REST APIDirect call using stage{
"resource": "/payment",
"path": "/payment/",
"httpMethod": "GET",
"pathParameters": null,
"requestContext": {
"resourcePath": "/payment",
"path": "/default/payment/",
"stage": "default",
"domainPrefix": "qeo6tqt75i",
"domainName": "qeo6tqt75i.execute-api.eu-west-1.amazonaws.com",
}
} Single custom mapping{
"path": "/v1/payment",
"resource": "/payment",
"httpMethod": "GET",
"pathParameters": null,
"requestContext": {
"resourcePath": "/payment",
"path": "/v1/payment",
"stage": "default",
"domainPrefix": "api",
"domainName": "api.serverlessa.dev",
},
} Nested custom mapping{
"path": "/v1/nested/payment/123456789-afekl-13456/",
"resource": "/payment/{invoice+}",
"httpMethod": "GET",
"pathParameters": {
"invoice": "123456789-afekl-13456"
},
"requestContext": {
"resourcePath": "/payment/{invoice+}",
"path": "/v1/nested/payment/123456789-afekl-13456/",
"stage": "default",
"domainName": "api.serverlessa.dev",
}
} Route 53{
"path": "/v1/payment",
"resource": "/payment",
"httpMethod": "GET",
"pathParameters": null,
"headers": {
"User-Agent": "Amazon-Route53-Health-Check-Service (ref b83f7d76-9e42-43fb-be1d-67619818499a; report http://amzn.to/1vsZADi)"
},
"requestContext": {
"resourcePath": "/payment",
"path": "/v1/payment",
"stage": "default",
"domainName": "api.serverlessa.dev"
}
} HTTP API v1Direct call using stage{
"resource": "/payment",
"path": "/default/payment",
"httpMethod": "GET",
"pathParameters": null,
"requestContext": {
"resourcePath": "/payment",
"path": "/default/payment",
"stage": "default",
"domainPrefix": "yx3odp7ks2",
"domainName": "yx3odp7ks2.execute-api.eu-west-1.amazonaws.com"
}
} Single custom mapping{
"resource": "/payment/{invoice+}",
"path": "/v2/payment/",
"httpMethod": "GET",
"pathParameters": null,
"requestContext": {
"resourcePath": "/payment/{invoice+}",
"path": "/default/payment/",
"stage": "default",
"domainName": "api.serverlessa.dev",
"domainPrefix": "api"
}
} Nested custom mapping{
"resource": "/payment/{invoice+}",
"path": "/v2/payment/123456789-afekl-13456",
"httpMethod": "GET",
"pathParameters": {
"invoice": "123456789-afekl-13456"
},
"requestContext": {
"resourcePath": "/payment/{invoice+}",
"path": "/default/payment/123456789-afekl-13456",
"stage": "default",
"domainName": "api.serverlessa.dev",
"domainPrefix": "api"
}
} Route53{
"resource": "/payment",
"path": "/v2/payment",
"httpMethod": "GET",
"pathParameters": null,
"headers": {
"User-Agent": "Amazon-Route53-Health-Check-Service (ref b83f7d76-9e42-43fb-be1d-67619818499a; report http://amzn.to/1vsZADi)",
},
"queryStringParameters": null,
"multiValueQueryStringParameters": null,
"requestContext": {
"resourcePath": "/payment",
"path": "/default/payment",
"stage": "default",
"domainName": "api.serverlessa.dev",
"httpMethod": "GET"
}
} HTTP API v2Direct call using stage{
"routeKey": "ANY /payment",
"rawPath": "/default/payment",
"requestContext": {
"domainName": "swgeupeic9.execute-api.eu-west-1.amazonaws.com",
"http": {
"method": "GET",
"path": "/default/payment"
},
"routeKey": "ANY /payment",
"stage": "default"
}
} Single custom mapping{
"routeKey": "ANY /payment",
"rawPath": "/default/payment",
"requestContext": {
"domainName": "api.serverlessa.dev",
"http": {
"method": "GET",
"path": "/default/payment"
},
"routeKey": "ANY /payment",
"stage": "default"
}
} Nested custom mapping{
"routeKey": "ANY /payment/{invoice+}",
"rawPath": "/default/payment/123456789-afekl-13456",
"pathParameters": {
"invoice": "123456789-afekl-13456"
},
"requestContext": {
"domainName": "api.serverlessa.dev",
"http": {
"method": "GET",
"path": "/default/payment/123456789-afekl-13456"
},
"routeKey": "ANY /payment/{invoice+}",
"stage": "default"
}
} Route53{
"routeKey": "ANY /payment",
"rawPath": "/default/payment",
"headers": {
"user-agent": "Amazon-Route53-Health-Check-Service (ref b83f7d76-9e42-43fb-be1d-67619818499a; report http://amzn.to/1vsZADi)"
},
"requestContext": {
"domainName": "api.serverlessa.dev",
"http": {
"method": "GET",
"path": "/default/payment"
},
"routeKey": "ANY /payment",
"stage": "default"
}
} |
Just added Route 53 event to complete the list of scenarios @michaelbrewer lemme know if there's any other data missing |
@heitorlessa - ok i have added support for using a list of prefixes. also it will auto add the last ie: path |
Great, makes sense. Could you update the PR description using a list of strings instead of the env var? We also don't know the env var values based on the description |
LGTM - @walmsles are you happy with it? Or do we must support environment variable too? If we do need an environment variable, we would need the values to be as comma-separated values: " If not, then we can merge it and move to a few last improvements and prepare a release |
@heitorlessa I think no env var support is good - means AppConfig or Parameters can be used to dynamically control the prefix configuration which IMO is cleaner and what I would look to do to achieve the outcome from this feature. Working out complex Env Var values on LAMBDA is hard! |
Amazing - merging it then ;) Thanks for the help flagging it @walmsles, and obviously a huge help as always to @michaelbrewer |
Love your work @heitorlessa @michaelbrewer - thankyou! |
Issue #, if available: aws-powertools/powertools-lambda#34
Description of changes:
Changes:
prefix
to be stripped from the event path, default isNone
which does not modify the pathserializer
Example usage:
strip_prefixes
"/unique/v1" and "/foo/v1"/status
will then match on/unique/v1/status
(and/status
)strip_prefixes
is None, and therefore does not touch the event path/
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.