fix(lambda): support S3 code packages for CreateFunction and UpdateFunctionCode (#34) - #51
fix(lambda): support S3 code packages for CreateFunction and UpdateFunctionCode (#34)#51tyrchen wants to merge 2 commits into
Conversation
…nctionCode (#34) CreateFunction/UpdateFunctionCode accepted Code.S3Bucket/S3Key but never downloaded the object, so the first Invoke failed with InvalidCode("missing code root") and GetFunction reported a fake S3 location. Standard AWS SDK clients (e.g. Java v2) that deploy from S3 were silently broken. What landed: - New S3CodeFetcher trait + S3CodeFetchError in rustack-lambda-core::code (async-trait; object-safe dyn dispatch); the default UnavailableS3CodeFetcher rejects S3 packages with a clear error when no S3 backend is wired. - RustackLambda gains a code_fetcher field + with_code_fetcher builder; process_code takes a CodeSource bundling the mutually-exclusive code inputs and stores fetched bytes via the same store_zip_code path as inline ZipFile (same checksum, size, extraction, executor behavior). - Fail-fast validation at create/update time: ZipFile/S3Bucket/ImageUri mutual exclusion, S3Key required with S3Bucket, empty and oversize S3 fields rejected, AWS-compatible NoSuchBucket/NoSuchKey/NoSuchVersion error messages (InvalidParameterValueException, HTTP 400). - apps/rustack::lambda_s3_bridge implements S3CodeFetcher over the in-process RustackS3 provider (bucket + version resolution mirroring handle_get_object, including delete-marker rejection and "null" unversioned ids); main.rs wires it when S3 is enabled, else the unavailable fetcher. No core->core dependency. - Spec: specs/ruststack-lambda-s3-code-design.md (design + testing plan + exit criteria); stale "does not fetch from S3" notes updated in ruststack-lambda-design.md and ruststack-lambda-executor-design.md. Verified: - 13 new lambda-core unit tests, 7 bridge unit tests, 2 SDK integration tests (test_lambda.rs), 1 native-executor S3-code round-trip test (test_lambda_invoke.rs). - End-to-end: zip uploaded to rustack S3, function created from S3 code, invoked -> 200 with echo payload; missing bucket -> 400 InvalidParameterValueException. - Gates: cargo build, cargo test (workspace), cargo +nightly fmt --check, cargo clippy -D warnings (1.95 pinned + 1.97 on changed crates), RUSTDOCFLAGS="-D warnings" cargo doc. Note: the pre-existing working-tree deletion of rust-toolchain.toml (not part of this change) was left unstaged; .DS_Store likewise. Closes #34
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c90cfdbf9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let zip_bytes = self | ||
| .code_fetcher | ||
| .fetch_code(bucket, key, source.s3_object_version) | ||
| .await | ||
| .map_err(map_s3_code_fetch_error)?; |
There was a problem hiding this comment.
When an S3 object exceeds MAX_ZIP_SIZE or is a compressed archive that expands dramatically, this call materializes the entire object and then passes it to store_zip_code, which writes and extracts it before CreateFunction checks the compressed size; UpdateFunctionCode never performs that check at all. Consequently, an untrusted S3 package can consume unbounded memory/disk, and oversized updates are accepted successfully. Check the fetched length before storage and enforce an extracted-byte limit while unpacking.
AGENTS.md reference: AGENTS.md:L125-L125
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0359966: check_code_size now runs inside process_code for both the inline ZipFile and S3 branches — before any store_zip_code write/extraction — so oversized packages fail fast on both CreateFunction and UpdateFunctionCode (which previously had no size check at all). Additionally, extract_zip now caps extracted bytes at 250 MB (MAX_EXTRACTED_SIZE, mirroring the AWS unzipped limit): each entry's declared size is pre-checked against the remaining budget and actual copied bytes are bounded via Read::take, rejecting zip bombs with InvalidZipFile. New tests: oversize-on-create, oversize-on-update, extraction budget accept/reject.
| .read_object(bucket, key, &storage_version_id, None) | ||
| .await |
There was a problem hiding this comment.
When an S3-backed package has spilled to disk and that I/O stalls, this unbounded await keeps the CreateFunction or UpdateFunctionCode request open indefinitely because the gateway has no enclosing request timeout. Wrap the read in a configured tokio::time::timeout and map expiry to a service error.
AGENTS.md reference: AGENTS.md:L122-L122
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0359966: the read_object await in LambdaS3CodeFetcher::fetch_code is now wrapped in tokio::time::timeout(DEFAULT_READ_TIMEOUT) (30s const in the bridge), mapping expiry to S3CodeFetchError::Internal with a descriptive message → InvalidParameterValueException at the provider boundary.
| if let Some(version) = source.s3_object_version { | ||
| validate_s3_field("Code.S3ObjectVersion", version, MAX_S3_VERSION_LEN)?; |
There was a problem hiding this comment.
When S3ObjectVersion is supplied alongside ZipFile or ImageUri but no S3Bucket/S3Key, validation only checks its length and the request succeeds while silently ignoring the version. Treat the version as part of the S3 source and require both bucket and key, just as the existing checks do for either location field.
AGENTS.md reference: AGENTS.md:L99-L99
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0359966: validate_code_source now rejects S3ObjectVersion unless both S3Bucket and S3Key are present ("Code.S3ObjectVersion requires Code.S3Bucket and Code.S3Key"), instead of silently ignoring the version next to ZipFile/ImageUri. Covered by test_should_reject_s3_version_without_location.
Addresses the Codex review on PR #51: - Enforce the 50 MB zipped package limit BEFORE any storage or extraction (check_code_size in process_code, applied to both inline ZipFile and S3 code paths, so UpdateFunctionCode is covered too — previously it had no size check at all and oversized CreateFunction packages were written and extracted before being rejected). - Cap extracted bytes at 250 MB during zip extraction (store_zip_code / extract_zip) to defeat zip-bomb expansion; declared entry size is pre-checked against the remaining budget and actual copied bytes are bounded. - Bound the S3 object read with a 30s tokio::time::timeout in the bridge (DEFAULT_READ_TIMEOUT) so a stalled disk I/O cannot hold CreateFunction / UpdateFunctionCode open indefinitely; expiry maps to a service error. - Reject S3ObjectVersion without a full S3 location (bucket + key), instead of silently ignoring it next to ZipFile/ImageUri. - New tests: oversize S3 code on create and update, version without location, extraction budget accept/reject, delete-marker pinning. Spec updated to match (size check, extraction cap, timeout, version rule). Verified: workspace tests green (135 lambda-core + 51 app), clippy -D warnings on 1.95 (pinned) and 1.97 for changed crates, RUSTDOCFLAGS="-D warnings" cargo doc, and the native S3-code round-trip integration test. Closes #34
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03599661f0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut extracted_total: u64 = 0; | ||
| for i in 0..archive.len() { |
There was a problem hiding this comment.
A package can remain below both byte limits while containing hundreds of thousands of empty files or directories, and this loop creates every entry without decrementing extracted_total; an S3-backed deployment can therefore exhaust inodes or occupy a blocking worker for a long time despite the new ZIP-bomb guard. Reject archives above a bounded entry count before creating their paths.
AGENTS.md reference: AGENTS.md:L103-L103
Useful? React with 👍 / 👎.
| let (code_sha256, code_size, zip_bytes, code_path, image_uri) = | ||
| self.process_code(&name, "$LATEST", code_source).await?; |
There was a problem hiding this comment.
When UpdateFunctionCode names a nonexistent function, this call downloads and extracts the S3 package before self.store.update discovers that the record is absent. Each distinct nonexistent name therefore leaves an orphaned code directory containing up to 250 MB of extracted data, so repeated requests referencing the same compressed object can exhaust disk; resolve the record before processing code or clean up on the not-found path.
Useful? React with 👍 / 👎.
| let extract_result = tokio::task::spawn_blocking(move || { | ||
| extract_zip(&bytes_owned, &extract_to, MAX_EXTRACTED_SIZE) | ||
| }) |
There was a problem hiding this comment.
When an update archive exceeds MAX_EXTRACTED_SIZE, this newly fallible extraction runs only after store_zip_code has overwritten code.zip, removed the existing $LATEST/extracted directory, and begun writing the replacement. Returning the size error leaves the function record pointing at its old deployment while the on-disk code is partial, so the rejected update can break subsequent cold invocations; extract into a temporary directory and atomically replace the live package only after validation succeeds.
Useful? React with 👍 / 👎.
| let (code_sha256, code_size, zip_bytes, code_path, image_uri) = | ||
| self.process_code(&name, "$LATEST", code_source).await?; |
There was a problem hiding this comment.
When the target function was created with PackageType: Image, an S3 update reaches this call and subsequently stores ZIP bytes/path while clearing image_uri, but never changes or validates record.latest.package_type. The resulting record still selects the Image execution path with no image URI, so later invocations fail; require S3/Zip sources for Zip functions and ImageUri for Image functions before processing the update.
AGENTS.md reference: AGENTS.md:L107-L107
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes #34 —
CreateFunction/UpdateFunctionCodeacceptedCode.S3Bucket/S3Keybut never downloaded the object, so the firstInvokefailed withInvalidCode("missing code root")andGetFunctionreported a fake S3code_location. Standard AWS SDK clients (e.g. AWS SDK for Java v2) that deploy from S3 were silently broken.This PR makes S3 code packages fully functional: the object is downloaded at create/update time and stored through the exact same path as inline
ZipFile(store_zip_code), so checksum, size, extraction, and executor behavior are identical. Invalid S3 references now fail fast at creation time with AWS-compatibleInvalidParameterValueExceptionmessages instead of failing later at invoke.Design
Follows the repo's established cross-service bridge pattern (SNS→SQS
SqsPublisher, EventBridge→SQSTargetDelivery): a trait in the consuming core crate, an implementation in the server binary, no core→core dependency.rustack-lambda-core::code— newS3CodeFetchertrait (async-trait, object-safe) +S3CodeFetchError(thiserror) +UnavailableS3CodeFetcherdefault that produces a clear "S3 service is not enabled" error when no S3 backend is wired.RustackLambdagainscode_fetcher+with_code_fetcher;process_codetakes aCodeSourcebundling the mutually-exclusive code inputs. Fail-fast validation: ZipFile/S3Bucket/ImageUri mutual exclusion, S3Key required with S3Bucket, empty/oversize S3 fields rejected (byte caps per AGENTS.md).apps/rustack/src/lambda_s3_bridge.rsimplementsS3CodeFetcherover the in-processRustackS3provider. Bucket + version resolution mirrorshandle_get_object: latest non-delete-marker viaget, explicit versions viaget_version, delete-marker rejection,"null"unversioned ids, parking_lot guard dropped before any.await.main.rsbuilds the S3 provider before Lambda and selects the bridge when S3 is enabled, else the unavailable fetcher (feature-flag safe).Spec
specs/ruststack-lambda-s3-code-design.md — design, error model, testing plan, exit criteria. Stale "does not fetch from S3" notes in
ruststack-lambda-design.md/ruststack-lambda-executor-design.mdupdated.Verification
"null"version)test_lambda.rs— create with S3 code asserts populated code metadata; missing bucket assertsInvalidParameterValueException)test_lambda_invoke.rs— real bootstrap zip uploaded to S3, function created from S3 code, invoke → 200 echo)CreateFunctionwith S3 code →Invoke→{"echo":{"hello":"from-s3"}}HTTP 200; missing bucket → HTTP 400NoSuchBucketmessagecargo build/cargo test(workspace) /cargo +nightly fmt --check/cargo clippy -- -D warnings(1.95 pinned + current on changed crates) /RUSTDOCFLAGS="-D warnings" cargo doc— all green.Notes
rust-toolchain.toml(not part of this change) was left unstaged.