-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
APIGW NG implement AWS integration #11196
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
Conversation
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 3m 20s ⏱️ Results for commit 2df63de. ♻️ This comment has been updated with latest results. |
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 24m 7s ⏱️ - 1h 11m 9s Results for commit f0e2d3b. ± Comparison against base commit ab9613a. This pull request removes 2520 and adds 3 tests. Note that renamed tests count towards both.
This pull request removes 356 skipped tests and adds 1 skipped test. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impressive amount and quality of changes!
It is really awesome that we can have, for now, some clear path for AWS integrations. It will be good to explore how the other services behave!
if not uri: | ||
return uri |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Is this possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, for MOCK
implementation 😅 I got bitten by it, and it was easier to manage in one point
@lru_cache(maxsize=64) | ||
def get_target_prefix_for_service(service_name: str) -> str | None: | ||
return get_service_catalog().get(service_name).metadata.get("targetPrefix") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 Nice
response = retry(invoke_api, sleep=2, retries=10, url=invocation_url, is_valid_xml=is_valid_xml) | ||
response = retry(invoke_api, sleep=2, retries=10, url=invocation_url, validate_xml=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😕 How did that work before, was it just referencing a function, so it would evaluate as True
? Nice catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly have no idea how it worked 😅 I think it was overriding and kinda working somehow, but it's nicer like this 😬
Motivation
This PR implements the last missing integration:
AWS
. This integration allows API Gateway to interact with other AWS services, by constructing mostly manually a request following the service specs (APIGW will take of the signing and endpoint mostly, and in some case it helps directing the request to the right operation).There are 2 kind of AWS integrations:
path
andaction
. The documentation is very sparse on those.Path allows you to override the path where the request is sent, this can be used for services like S3 for example.
action
allows you to target an AWS service action directly, and you might or might not need to give very specific data like theX-Amz-Target
header to your request.Before, we would specify a sub type of integration for each service, but after looking deeply into it with @cloutierMat, we realized it is only one integration in the end, with maybe a few differences between services but that are generalizable between services.
If things get out of control with specific behavior for the
action
type, we could register a "before send" and "after response" callback to fix the request/response before sending it for specific services. It's worth to say that there is literally zero documentation about theaction
type and which/how services are supported.Which means we're now support way more services out of the box when using
path
URI! 🚀By running all the AWS integrations test (which now all pass ✅), I've spotted a few bugs that have been fixed in the PR as well.
As a side note, this PR put into light the fact that AWS still supports the Query Protocol for the SSM service, and that API Gateway still sends request with that protocol by default to SSM... even though the botocore specs have always been the JSON specs. Fun 😬 \cc @alexrashed you might like that one...
Changes
AWS
integrationpath
type which is very simpleaction
type, it is a bit more complicated and will still need a bit of effort to support new services, since it seems they fall into different categories.Content-Type
andAccept
for integration request (not passthrough but provided by AWS)needs_fixing
tests for the new provider, as they often don't setup APIGW correctlyTesting
Now with the new provider enabled, all APIGW tests only are passing locally! We will soon enable a new pipeline for the APIGW provider only.
Still 8 failing tests in the pipeline, fixed with #11198: