Skip to content

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

Merged
merged 10 commits into from
Jul 16, 2024
Merged

APIGW NG implement AWS integration #11196

merged 10 commits into from
Jul 16, 2024

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Jul 12, 2024

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 and action. 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 the X-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 the action 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

  • implement a generalized way to support AWS integration
    • support the path type which is very simple
    • for the action 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.
  • validate existing tests with AWS integrations (DynamoDB, SQS, SFn, Kinesis, EventBridge, Lambda, SSM and S3)
    • moved the SFn test into its own file
  • change how we mapped default values for Content-Type and Accept for integration request (not passthrough but provided by AWS)
  • implement rendering of URI, still need to implement more tests in the future
  • skip needs_fixing tests for the new provider, as they often don't setup APIGW correctly
  • last fix (last in list): fix the access to the REST API to be case insensitive when fetching the stage variables

Testing

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:

FAILED tests/aws/scenario/note_taking/test_note_taking.py::TestNoteTakingScenario::test_notes_rest_api
FAILED tests/aws/services/stepfunctions/v2/services/test_apigetway_task_service.py::TestTaskApiGateway::test_invoke_base
FAILED tests/aws/services/stepfunctions/v2/services/test_apigetway_task_service.py::TestTaskApiGateway::test_invoke_with_body_post[None]
FAILED tests/aws/services/stepfunctions/v2/services/test_apigetway_task_service.py::TestTaskApiGateway::test_invoke_with_body_post[]
FAILED tests/aws/services/stepfunctions/v2/services/test_apigetway_task_service.py::TestTaskApiGateway::test_invoke_with_body_post[HelloWorld]
FAILED tests/aws/services/stepfunctions/v2/services/test_apigetway_task_service.py::TestTaskApiGateway::test_invoke_with_body_post[request_body3]
FAILED tests/aws/services/stepfunctions/v2/services/test_apigetway_task_service.py::TestTaskApiGateway::test_invoke_with_query_parameters
FAILED tests/aws/test_multiregion.py::TestMultiRegion::test_multi_region_api_gateway

@bentsku bentsku added aws:apigateway Amazon API Gateway semver: patch Non-breaking changes which can be included in patch releases labels Jul 12, 2024
@bentsku bentsku self-assigned this Jul 12, 2024
@bentsku bentsku marked this pull request as draft July 12, 2024 20:53
Copy link

github-actions bot commented Jul 12, 2024

S3 Image Test Results (AMD64 / ARM64)

  2 files    2 suites   3m 20s ⏱️
404 tests 352 ✅  52 💤 0 ❌
808 runs  704 ✅ 104 💤 0 ❌

Results for commit 2df63de.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jul 12, 2024

LocalStack Community integration with Pro

  2 files  ±    0    2 suites  ±0   24m 7s ⏱️ - 1h 11m 9s
663 tests  - 2 517  617 ✅  - 2 162  46 💤  - 355  0 ❌ ±0 
665 runs   - 2 517  617 ✅  - 2 162  48 💤  - 355  0 ❌ ±0 

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.
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…
tests.aws.services.apigateway.test_apigateway_ssm ‑ test_get_parameter_query_protocol
tests.aws.services.apigateway.test_apigateway_stepfunctions.TestApigatewayStepfunctions ‑ test_apigateway_with_step_function_integration[DeleteStateMachine]
tests.aws.services.apigateway.test_apigateway_stepfunctions.TestApigatewayStepfunctions ‑ test_apigateway_with_step_function_integration[StartExecution]
This pull request removes 356 skipped tests and adds 1 skipped test. Note that renamed tests count towards both.
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input4-FAILED]
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_deployed_infra_state
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_populate_data
tests.aws.scenario.mythical_mysfits.test_mythical_misfits.TestMythicalMisfitsScenario ‑ test_user_clicks_are_stored
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_api_exceptions
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_create_exceptions
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_create_invalid_desiredstate
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_double_create_with_client_token
tests.aws.services.cloudcontrol.test_cloudcontrol_api.TestCloudControlResourceApi ‑ test_lifecycle
…
tests.aws.services.apigateway.test_apigateway_ssm ‑ test_get_parameter_query_protocol

♻️ This comment has been updated with latest results.

@bentsku bentsku mentioned this pull request Jul 13, 2024
1 task
@bentsku bentsku removed the request for review from thrau July 13, 2024 00:50
@bentsku bentsku added this to the 3.6 milestone Jul 13, 2024
@bentsku bentsku marked this pull request as ready for review July 13, 2024 02:14
Copy link
Contributor

@cloutierMat cloutierMat left a 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!

Comment on lines +46 to +47
if not uri:
return uri
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Is this possible?

Copy link
Contributor Author

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

Comment on lines +127 to +129
@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")
Copy link
Contributor

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)
Copy link
Contributor

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!

Copy link
Contributor Author

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 😬

@bentsku bentsku merged commit a24bcaf into master Jul 16, 2024
32 checks passed
@bentsku bentsku deleted the apigw-ng-aws-integration branch July 16, 2024 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:apigateway Amazon API Gateway semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants