Uh oh!
There was an error while loading. Please reload this page.
fix(integ-tests): resolve remaining API Gateway auth failures - #3972
Conversation
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Code Review Results
Reviewed: bf2378c..8bfe4d7
Files: 7
Comments: 2
Comments on lines outside the diff:
[integration/resources/templates/combination/api_with_binary_media_types_with_definition_body_openapi.yaml:28][GENERAL] This change does not match the approach described in the PR. The description states that for this template IAM auth is valid there — so the auth is kept and the request is signed instead, with this diff:
- response = self.do_get_request_with_logging(url, headers)+ response = self.do_get_request_with_sigv4(url, headers)But the actual change removes the Auth block from this template, and integration/combination/test_api_settings.py is not part of the diff at all — verify_binary_media_request (line 161) still calls the unsigned do_get_request_with_logging. The test will pass, but only because the endpoint is now unauthenticated, which reverts the #3963 intent for a template where IAM auth demonstrably works (it uses DefinitionBody, so the transform accepts DefaultAuthorizer: AWS_IAM).
Either restore the auth and sign the request, or update the PR description to state that auth is being dropped here too. The signing helper already exists in integration/helpers/base_test.py:
defverify_binary_media_request(self, url, expected_status_code):
headers= {"accept": "image/png"}
response=self.do_get_request_with_sigv4(url, headers)[integration/resources/templates/combination/api_with_binary_media_types_with_definition_body_openapi.yaml:29][BUG] Re-raising an unresolved issue from the previous review (not dismissed by the author): the same unsigned-request-against-IAM-auth failure still exists in integration/single/test_basic_function.py, so the "resolves the two remaining auth failures" claim is incomplete.
test_function_with_http_api_events (line 70) runs against single/function_with_http_api_events and single/function_alias_with_http_api_events. Both templates carry IAM auth:
MyHttpApi:
Type: AWS::Serverless::HttpApiProperties:
Auth:
EnableIamAuthorizer: trueDefaultAuthorizer: AWS_IAMThe test then calls self._verify_get_request(endpoint, self.FUNCTION_OUTPUT) (line 75), which delegates to verify_get_request_response(url, 200) — the unsigned path (do_get_request_with_logging, base_test.py:537). API Gateway will return 403, so the assertion fails the same way test_binary_media_types_with_definition_body_openapi did. The @pytest.mark.flaky(reruns=5) decorator and the tenacity retry will not help, since the failure is deterministic.
Sibling tests already use the signed helper (test_function_with_http_api.py, test_function_with_implicit_http_api.py call verify_get_request_response_sigv4), so the consistent fix is:
defverifyget_request(self, url, expected_text):
response=self.verify_get_request_response_sigv4(url, 200)
self.assertEqual(response.text, expected_text)Note that _verify_get_request is also used at line 199 for a Lambda function URL test, so if that URL is not IAM-authenticated, sign only the HTTP API call site rather than changing the shared helper.
There was a problem hiding this comment.
Code Review Results
Reviewed: bf2378c..fe03cfb
Files: 9
Comments: 1
Comments on lines outside the diff:
[integration/resources/templates/single/function_with_http_api_events.yaml:3][GENERAL] This file and function_alias_with_http_api_events.yaml drop EnableIamAuthorizer/DefaultAuthorizer: AWS_IAM, but neither is covered by the two issues in the PR description (Issue 1 lists six WebSocket templates, Issue 2 lists the binary-media template). These two templates fail for the third reason raised in the previous review: test_function_with_http_api_events calls self._verify_get_request(endpoint, ...) (integration/single/test_basic_function.py:75), which goes through verify_get_request_response → do_get_request_with_logging, i.e. an unsigned request against an IAM-authorized API.
For that failure mode the codebase already has an established fix that keeps the auth coverage: verify_get_request_response_sigv4, used by test_function_with_http_api and test_function_with_implicit_http_api for templates that retain EnableIamAuthorizer (integration/combination/test_function_with_http_api.py:21-32, integration/combination/test_function_with_implicit_http_api.py:15-17). The description's rationale for preferring removal — "avoids changing shared test helpers" — does not apply here: _verify_get_request is a private helper in test_basic_function.py with only two call sites, and the second one (line 199) is inside a test permanently disabled by @skipIf(True, ...), so signing this request touches nothing else.
As written, the explicit-HttpApi and AutoPublishAlias+HttpApi-event paths lose their IAM authorizer coverage while sibling HttpApi templates keep it, leaving the suite inconsistent about which HttpApi templates are authorized.
Suggested alternative — keep the auth in both templates and sign the request:
deftest_function_with_http_api_events(self, file_name):
self.create_and_verify_stack(file_name)
endpoint=self.get_api_v2_endpoint("MyHttpApi")
response=self.verify_get_request_response_sigv4(endpoint, 200)
self.assertEqual(response.text, self.FUNCTION_OUTPUT)If dropping the auth is the deliberate choice, please state the reason for these two templates in the description as you did for Issues 1 and 2, so the coverage loss is a recorded decision rather than an unexplained diff.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Summary
Follow-up to #3963 and #3967. Resolves the remaining integration test failures introduced when IAM auth was added to the test templates.
Auth is only removed where the SAM transform makes it impossible. Where auth is valid, it is kept and the test request is signed instead.
Issue 1 — WebSocket auth requires a
$connectrouteSix templates configure only a
$defaultroute, so the transform failed:These templates intentionally cover minimal WebSocket configurations, so adding a
$connectroute would change what they test. Auth is removed instead:websocket_api_basic.yamlwebsocket_api_basic_config.yamlwebsocket_api_custom_domains_regional.yamlwebsocket_api_multiple_api.yaml(bothApi1andApi2)websocket_api_route_settings.yamlwebsocket_api_stage_config.yamlDedicated IAM auth coverage for WebSocket APIs remains in
test_websocket_api_with_auth.py, and every template that keepsAuthType: AWS_IAMhas a$connectroute.Issue 2 — unsigned requests against authenticated APIs
Two tests issue unsigned requests against APIs that gained IAM auth, returning
403where200was expected:The affected templates use an inline
DefinitionBody(REST) orAWS::Serverless::HttpApi, so IAM auth is valid for them. The auth is kept and the requests are signed, matching the existing pattern intest_function_with_http_api.pyandtest_function_with_implicit_http_api.py:test_binary_media_types_with_definition_body_openapi—combination/api_with_binary_media_types_with_definition_body_openapi.yamldef verify_binary_media_request(self, url, expected_status_code): headers = {"accept": "image/png"} - response = self.do_get_request_with_logging(url, headers)+ response = self.do_get_request_with_sigv4(url, headers)test_function_with_http_api_events—single/function_with_http_api_events.yamlandsingle/function_alias_with_http_api_events.yamlendpoint = self.get_api_v2_endpoint("MyHttpApi") - self._verify_get_request(endpoint, self.FUNCTION_OUTPUT)+ response = self.verify_get_request_response_sigv4(endpoint, 200)+ self.assertEqual(response.text, self.FUNCTION_OUTPUT)Signing is applied at the call site rather than inside the shared
_verify_get_requesthelper, since that helper is also used by a Lambda function URL test.Testing
Verified in account 830899278857.
us-west-2 — each affected test confirmed:
test_binary_media_types_with_definition_body_openapitest_function_with_http_api_events(both parameterized cases)test_websocket_api_basictest_websocket_api_basic_configtest_websocket_multi_apitest_websocket_api_route_settingstest_websocket_api_stage_configFull
test_api_settings.pyalso run: 9 passed, 1 xpassed.For the binary-media failure, all three configurations were tested to confirm the diagnosis:
AWS_IAMdevelop)AWS_IAMus-east-1 — the 4 custom-domain tests pass (
CustomDomainis only enabled there):test_custom_http_api_domains_regional+..._ownership_verificationtest_custom_rest_api_domains_edge(+regional,regional_ownership_verification)test_websocket_custom_api_domains_regionalruff checkpasses on both modified test files. The tworuff formatwarnings on them also occur on a cleandevelop, so they are left untouched to avoid unrelated reformatting.