Repository navigation
feat: enable VPC Lattice support - #829
nanookclaw wants to merge 6 commits into
Conversation
![aws-sam-tooling-bot[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
| "apigw_http", | ||
| "apigw_rest", | ||
| "alb", | ||
| "vpc_lattice", |
There was a problem hiding this comment.
[BUG] Adding another HTTP event variant alongside pass_through changes event classification, and nothing in this PR verifies that non-HTTP events still behave as before.
pass_through is the fallback variant of lambda_http's event enum, and it is the adapter's only mechanism for non-HTTP triggers. In src/lib.rs:
if matches!(request_context, RequestContext::PassThrough) && parts.method == Method::POST {
path = self.pass_through_path.as_str();
}Every SQS, SNS, S3, DynamoDB, EventBridge, and Bedrock Agent payload reaches the app only because it deserializes into PassThrough (documented in docs/guide/src/features/non-http-events.md, exercised by examples/sqs-expressjs and examples/bedrock-agent-fastapi). Enabling one more variant necessarily shrinks the set of payloads that reach that fallback. VPC Lattice payloads are shaped as loosely typed method / raw_path / headers / query_string_parameters / body / is_base64_encoded fields, so if those fields deserialize with defaults, an unrelated event JSON can match the Lattice variant instead of falling through. The failure is silent: the event would be forwarded as a GET to / with an empty body rather than POSTed to AWS_LWA_PASS_THROUGH_PATH, so a pass-through handler would simply stop receiving messages.
Two things are worth adding before merge:
- A regression test asserting a non-HTTP payload still routes to the pass-through path. The current harness cannot express this —
tests/integ_tests/common/mod.rsonly builds ALB events:
pub enum LambdaEventType {
#[default]
ALB,
// TODO: Add other event types
}- A test covering the new path itself: a VPC Lattice event producing the expected request path, query string, and
x-amzn-request-contextheader. There is currently no coverage that the newly enabled variant works end to end throughfetch_response, which derives the path fromraw_http_path()and serializes the context into a header.
This matters more than usual here because the PR description notes cargo check --locked could not complete in the author's environment, so neither compilation nor the existing test suite has been run against the change.
|
Added focused regression coverage in
Verification passed: |
There was a problem hiding this comment.
Code Review Results
Reviewed: 34d3a29..fe18556
Files: 3
Comments: 1
Comments on lines outside the diff:
[Cargo.toml:31] [GENERAL] Adding vpc_lattice inserts another variant into lambda_http's event enum, which is matched untagged (variants are tried in declaration order, with pass_through as the fallback). The new tests cover the two ends of that ordering — a Bedrock Agent payload still resolving to RequestContext::PassThrough, and a VPC Lattice V2 payload resolving to RequestContext::VpcLattice — but nothing covers the variants in between.
That gap matters because no existing test deserializes a real event payload at all. LambdaEventBuilder::build() in tests/integ_tests/common/mod.rs returns LambdaRequest::Alb(...) directly, and the unit tests in src/lib.rs construct LambdaRequest::Alb / LambdaRequest::ApiGatewayV2 directly as well. Both bypass serde entirely, so if a VPC Lattice V2 payload shape overlaps an ALB or API Gateway payload shape (both carry path/headers/queryStringParameters/body/requestContext), the misclassification would produce a wrong x-amzn-request-context header and a wrong forwarded path with the entire suite still green. The e2e tests that would catch it are #[ignore]d and require a deployed stack.
Since the new tests already establish the pattern, the check is cheap to add:
#[test]
fn test_http_events_still_classify_after_enabling_vpc_lattice() {
let alb = lambda_http::request::from_str(&alb_event_json()).expect("Failed to deserialize ALB event");
assert!(matches!(alb.request_context(), RequestContext::Alb(_)));
let apigw = lambda_http::request::from_str(&apigw_v2_event_json()).expect("Failed to deserialize APIGW event");
assert!(matches!(apigw.request_context(), RequestContext::ApiGatewayV2(_)));
}This is also worth confirming before merge because the PR description notes that cargo check --locked could not complete in this environment, so neither the new tests nor the feature combination have actually been compiled or run.
Note: Cargo.lock was reviewed only for consistency with the manifest change (version and checksum bumps for aws_lambda_events, lambda_http, lambda_runtime, lambda_runtime_api_client); as a generated lock file it is otherwise skipped.
|
Added commit
|
| "apigw_http", | ||
| "apigw_rest", | ||
| "alb", | ||
| "vpc_lattice", |
There was a problem hiding this comment.
[GENERAL] Enabling vpc_lattice makes VPC Lattice a supported trigger, but no user-facing documentation in this repo reflects that, and the supported payload format is left implicit.
Concretely stale/incomplete after this change:
README.md— the features list still reads "Supports Amazon API Gateway Rest API and Http API endpoints, Lambda Function URLs, and Application Load Balancer".docs/guide/src/features/request-context.md— describesx-amzn-request-contextpurely as "API Gateway request context"; with this change the header can now carry a VPC Lattice context (serviceNetworkArn,serviceArn,targetGroupArn,identity), which is exactly the metadata an app behind Lattice would read for authorization.
The payload-format point matters behaviorally, not just editorially: the PR description and the test fixture ("version": "2.0") target the VPC Lattice V2 event structure. A Lattice target group configured with the other payload format would not match that variant and would instead fall back to the pass-through path in src/lib.rs:
if matches!(request_context, RequestContext::PassThrough) && parts.method == Method::POST {
path = self.pass_through_path.as_str();
}That means a misconfigured target group silently POSTs the raw event to AWS_LWA_PASS_THROUGH_PATH (default /events) instead of the app's real route — a failure mode that is very hard to diagnose without a documented requirement. Please state which payload format(s) are supported and note the target-group configuration requirement.
| .expect("Failed to create adapter"); | ||
| let mut request = lambda_http::request::from_str(&event).expect("Failed to deserialize event"); | ||
|
|
||
| assert!(matches!(request.request_context(), RequestContext::PassThrough)); |
There was a problem hiding this comment.
[GENERAL] The regression guard for the classification change covers a single non-HTTP payload shape, which leaves most of the documented pass-through surface unguarded.
This PR inserts a new variant into lambda_http's untagged event enum, where pass_through is the fallback and variants are tried in declaration order. test_non_http_event_routes_to_configured_pass_through_path proves a Bedrock Agent payload still resolves to RequestContext::PassThrough, but docs/guide/src/features/non-http-events.md claims support for "SQS, SNS, S3, DynamoDB, Kinesis, Kafka, EventBridge, and Bedrock Agents". A records-style payload ({"Records": [...]}) has a completely different shape from the Bedrock payload, so it exercises a different matching path and is the more representative case for the adapter's non-HTTP triggers — and the repo already ships a fixture at examples/sqs-expressjs/events/sqs.json to model it after.
Similarly, test_http_event_request_context_classification asserts ALB and API Gateway V2 but omits API Gateway REST (V1), which is one of the adapter's headline supported triggers and is equally subject to variant-ordering changes.
Suggested additions, following the pattern already established in the new test:
let sqs_event = json!({
"Records": [{
"messageId": "059f36b4-87a3-44ab-83d2-661975830a7d",
"receiptHandle": "AQEBwJnKyrHigUMZj6rYigCgxlaS3SLy0a",
"body": "Test message.",
"eventSource": "aws:sqs",
"awsRegion": "us-east-1"
}]
})
.to_string();
let sqs_request = lambda_http::request::from_str(&sqs_event).expect("Failed to deserialize SQS event");
assert!(matches!(sqs_request.request_context(), RequestContext::PassThrough));Without these, a future variant reordering or event-struct loosening in lambda_http could silently reroute non-HTTP triggers away from AWS_LWA_PASS_THROUGH_PATH and the suite would still pass.
Cargo.lock was reviewed as a lock file only (version/checksum bumps for aws_lambda_events, lambda_http, lambda_runtime, lambda_runtime_api_client); no findings. I did not evaluate whether the vpc_lattice feature or the pinned versions resolve correctly, since the crate sources are not available in this workspace and the PR notes cargo check --locked could not complete — worth confirming in CI before merge, given the 244 lines of new test code have not been compiled.
|
Added commit
Verification passed: |
|
@nanookclaw Are you going to fix the failing tests? |
| let sqs_request = lambda_http::request::from_str(sqs_event).expect("Failed to deserialize SQS event"); | ||
| assert!(matches!(sqs_request.request_context(), RequestContext::PassThrough)); | ||
|
|
||
| let api_gateway_v1_event = json!({ |
There was a problem hiding this comment.
[GENERAL] These hand-written minimal payloads are a fragile way to pin down classification, and they are the most likely source of the failures @bnusunny is asking about.
LambdaRequest is an untagged serde enum: variants are tried in declaration order and a variant is rejected when any field it requires is missing. That means an incomplete fixture does not produce a deserialization error — it silently falls through to the next variant and ultimately to PassThrough. So the failure mode here is an assertion mismatch (or a wrong-but-passing assertion), with no indication of which field caused it.
The API Gateway V1 fixture omits fields that a real REST API payload always carries, notably requestTimeEpoch and identity inside requestContext, both of which aws_lambda_events' ApiGatewayProxyRequestContext deserializes without a #[serde(default)] fallback. The same concern applies to the trimmed ALB and API Gateway V2 objects, and to pass_through_bedrock_agent_event().
The repo already ships complete, realistic payloads for exactly these shapes, and this test already uses that approach for SQS:
let api_gateway_v1_event = include_str!("../../examples/fastapi/events/event.json");
let bedrock_event = include_str!("../../examples/bedrock-agent-fastapi/events/s3_object.json");Reusing the fixtures makes the test assert against payloads AWS actually sends, and keeps it from breaking whenever an optional/required field distinction changes upstream. For the shapes with no fixture in the repo (ALB, VPC Lattice V2), copy the full documented sample event rather than a subset.
|
|
||
| See the [API Gateway docs](https://docs.aws.amazon.com/apigateway/latest/developerguide/set-up-lambda-proxy-integrations.html#api-gateway-simple-proxy-for-lambda-input-format) for the full request context schema. | ||
|
|
||
| For VPC Lattice, the adapter supports the V2 payload format and requires target-group configuration. |
There was a problem hiding this comment.
[GENERAL] "requires target-group configuration" does not tell the user what to configure, and the misconfiguration it is warning about fails silently rather than loudly.
VPC Lattice Lambda target groups have a Lambda event structure version setting (V1 or V2). If a user leaves it at V1 while this PR only wires up the V2 payload, the V1 event matches no HTTP variant of lambda_http's event enum, so src/lib.rs classifies it as RequestContext::PassThrough and rewrites the request:
if matches!(request_context, RequestContext::PassThrough) && parts.method == Method::POST {
path = self.pass_through_path.as_str();
}Every VPC Lattice request would then arrive at the app as a POST /events carrying the raw event JSON instead of the requested method and path — no error, no log, just wrong routing that is hard to trace back to a target group setting.
Please state the required setting explicitly in both this page and the matching README bullet (README.md:16), e.g. "Register the Lambda function in a VPC Lattice target group with the Lambda event structure version set to V2; V1 payloads are not recognized as HTTP requests and are forwarded to the pass-through path (AWS_LWA_PASS_THROUGH_PATH, default /events)." If lambda_http's vpc_lattice feature does in fact also deserialize V1 payloads, then the "supports the V2 payload format" wording is the part that needs correcting instead.
|
Yes—pushed Local verification on the reviewed tree: |
Accept the request-context fields that the adapter must preserve while allowing additional context fields and JSON formatting differences.
444ba89 to
145a3d3
Compare
|
Rebased onto the current The update preserves the VPC Lattice change and the focused request-context coverage. Local verification passed for |
|
|
||
| See the [API Gateway docs](https://docs.aws.amazon.com/apigateway/latest/developerguide/set-up-lambda-proxy-integrations.html#api-gateway-simple-proxy-for-lambda-input-format) for the full request context schema. | ||
|
|
||
| For VPC Lattice, the adapter supports the V2 payload format and requires target-group configuration. |
There was a problem hiding this comment.
[GENERAL] "requires target-group configuration" does not tell the reader what to configure, and the misconfiguration it is warning about fails silently rather than loudly.
VPC Lattice Lambda target groups have a Lambda event structure version setting (V1 or V2). This PR wires up only the V2 payload. A V1 payload has a different shape — snake_case keys and no requestContext:
{
"raw_path": "/health",
"method": "POST",
"headers": { "header-key": "header-value" },
"query_string_parameters": { "mode": "fast" },
"body": "...",
"is_base64_encoded": false
}It therefore matches no HTTP variant of lambda_http's untagged event enum and lands on the pass_through fallback. Following that through src/lib.rs:
if matches!(request_context, RequestContext::PassThrough) && parts.method == Method::POST {
path = self.pass_through_path.as_str();
}the request path is replaced with AWS_LWA_PASS_THROUGH_PATH (default /events) and the raw event JSON is POSTed there. The user's actual route never sees the request, nothing logs a warning, and the symptom looks like a routing bug in their application rather than a target-group setting.
Worth stating explicitly: set the target group's Lambda event structure version to V2, and say what happens with V1 (treated as a non-HTTP event and forwarded to the pass-through path). The same vagueness is in the new README.md bullet ("with target-group configuration").
Two other trigger lists in the guide still omit VPC Lattice and are now stale: docs/guide/src/introduction.md:11 and docs/guide/src/reference/architecture.md:11-13.
| } | ||
|
|
||
| #[test] | ||
| fn test_http_event_request_context_classification() { |
There was a problem hiding this comment.
[GENERAL] The classification guard misses the payload shapes most at risk from inserting a variant into an untagged enum.
Context that raises the stakes: every other integration test builds its event as a typed value (LambdaRequest::Alb in tests/integ_tests/common/mod.rs), which bypasses serde's untagged variant selection entirely. So this test plus the two new event-string tests are the only coverage for the one behavioral thing this PR changes — which payload resolves to which RequestContext.
The ALB fixture here covers only the multi-value-headers-disabled shape:
"headers": {"host": "example.com"},An ALB target group with multi-value headers enabled sends multiValueHeaders / multiValueQueryStringParameters and omits headers / queryStringParameters — i.e. header and query maps whose values are arrays, which is precisely the shape VPC Lattice V2 uses ("headers": {"accept": ["*/*"]}, as in vpc_lattice_v2_event()). That is the shape a newly inserted variant is most likely to capture, and no fixture in this test — or anywhere in the repo, I checked examples/*/events/*.json — exercises it. The same gap applies to API Gateway REST with multi-value headers.
Two cheap improvements:
- Add an ALB case with
multiValueHeaders/multiValueQueryStringParametersand no single-value maps, assertingRequestContext::Alb(_). - Use the realistic REST payload the repo already ships instead of the 4-key object, the same way the SQS case is loaded:
let api_gateway_v1_event = include_str!("../../examples/fastapi/events/event.json");
let api_gateway_v1_request =
lambda_http::request::from_str(api_gateway_v1_event).expect("Failed to deserialize API Gateway V1 event");
assert!(matches!(
api_gateway_v1_request.request_context(),
RequestContext::ApiGatewayV1(_)
));That also removes a maintenance trap: a minimal fixture pins the variant only for the fields it happens to include, so it can keep passing while a real event with a conflicting field type classifies differently.
|
Follow-up for the two latest review threads is pushed as
Validation passed: |
| } | ||
|
|
||
| #[test] | ||
| fn test_http_event_request_context_classification() { |
There was a problem hiding this comment.
[GENERAL] The PR documents VPC Lattice V1 behavior in four places but never tests it, and V1 is the one classification outcome the new variant is most likely to change.
The diff adds this contract to README.md, docs/guide/src/introduction.md, docs/guide/src/reference/architecture.md, and docs/guide/src/features/request-context.md:
V1 events are treated as non-HTTP events and passed through to
AWS_LWA_PASS_THROUGH_PATH
test_http_event_request_context_classification covers SQS, API Gateway V1/V2, and ALB, and the new test_vpc_lattice_v2_event_routes_with_path_query_and_context covers V2 — but nothing covers V1. That is the gap that matters most here, because a V1 payload is the shape closest to the variant this PR inserts: it carries method, headers, query_string_parameters, body, and is_base64_encoded, differing from V2 mainly in snake_case keys, raw_path instead of path, and the absence of requestContext. In lambda_http's untagged enum a variant is selected when every field it requires is satisfiable, so whether a V1 event lands on PassThrough or partially matches the new VpcLattice variant depends on which V2 fields are optional — not something the documented promise should rest on.
The consequence if it does not land on PassThrough: src/lib.rs only rewrites the path to pass_through_path for a RequestContext::PassThrough event, so a V1 payload classified as an HTTP event would be forwarded as a request derived from fields it does not have, instead of being POSTed as a raw payload to /events — i.e. exactly the behavior the docs rule out, and silently.
The existing test is the natural place for it:
let vpc_lattice_v1_event = json!({
"raw_path": "/health",
"method": "GET",
"headers": {"accept": "*/*"},
"query_string_parameters": {"state": "prod"},
"body": VPC_LATTICE_BODY,
"is_base64_encoded": false
})
.to_string();
let vpc_lattice_v1_request =
lambda_http::request::from_str(&vpc_lattice_v1_event).expect("Failed to deserialize VPC Lattice V1 event");
assert!(matches!(
vpc_lattice_v1_request.request_context(),
RequestContext::PassThrough
));|
|
||
| #[test] | ||
| fn test_http_event_request_context_classification() { | ||
| let sqs_event = include_str!("../../examples/sqs-expressjs/events/sqs.json"); |
There was a problem hiding this comment.
[GENERAL] Pulling test fixtures out of examples/ puts them outside the trigger paths of the workflows that run these tests, so edits to them get no CI signal.
The two new include_str! calls make the integration test target depend on files under examples/:
let sqs_event = include_str!("../../examples/sqs-expressjs/events/sqs.json");
// ...
let api_gateway_v1_event = include_str!("../../examples/fastapi/events/event.json");Both .github/workflows/pr.yaml and .github/workflows/merge.yaml declare:
paths-ignore:
- "docs/**"
- "examples/**"So a PR that renames, moves, or edits either JSON file runs neither cargo clippy nor cargo nextest run. A rename breaks compilation of the whole integration test target, and an edit can silently change what the assertions mean (e.g. dropping httpMethod from the fastapi event would make the ApiGatewayV1 assertion fail). Because the merge workflow ignores the same paths, the breakage does not surface when it lands on main either — it appears on the next unrelated PR, attributed to whoever opened it.
The example event files also exist to be passed to sam local invoke, so they are maintained for a different purpose and under different constraints than a test fixture pinning serde variant selection.
Copying the payloads under tests/ (e.g. tests/integ_tests/fixtures/) keeps them real recorded payloads while putting them back under CI's trigger paths and decoupling them from example maintenance.
This branch has not been deployed
2 participants
Enable VPC Lattice event support in the adapter by enabling
lambda_http's existingvpc_latticefeature and updating the direct dependency from 1.1.1 to 1.2.0. The existing API Gateway, ALB, pass-through, tracing, and Tokio concurrency features are preserved.Cargo.lockis refreshed for the required Lambda crates.This exposes the VPC Lattice V2 event support already implemented by
lambda_http.Validation:
cargo fmt --all -- --checkcargo metadata --locked --offline --format-version 1 --no-depsgit diff --checkcargo check --lockedcould not reach compilation because this environment could not resolvestatic.crates.iowhile downloading the newly lockedlambda_runtimesource.Closes #789