Skip to content

fix(firehose): preserve S3 record bytes and match AWS object names - #2715

Merged
vieiralucas merged 5 commits into
faiscadev:mainfrom
cavanaug:fix/firehose-s3-output
Oct 16, 2026
Merged

vieiralucas merged 5 commits into
faiscadev:mainfrom
cavanaug:fix/firehose-s3-output

Conversation

@cavanaug

@cavanaug cavanaug commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Part 1 of 4 of the Firehose S3 delivery series proposed in #2708. This PR stands alone against main; GZIP, Snappy, and cross-call size/time buffering follow as separate increments.

  • Preserve decoded record bytes exactly instead of inserting newlines into every delivery. Valid records within a PutRecordBatch are concatenated byte-for-byte.
  • Generate S3 keys from the verbatim static prefix, UTC date hierarchy, stream/version/delivery-time suffix, and hyphenated UUID. Honor FileExtension overrides and validate their model constraints on create/update and CloudFormation provisioning.
  • Implement the built-in AppendDelimiterToRecord processor: when enabled, append a newline after every record before writing, including the final record and empty records. Other processors retain their existing configuration-only behavior in this increment.
  • Use the stored object's MD5 as its ETag and verify delivered-object metadata survives persistent-mode restart.
  • Have SES supply its own documented trailing newline when publishing Firehose event records, preserving producer-owned framing after Firehose stops injecting delimiters.
  • Update service/parity documentation to describe this increment's actual behavior and limitations.

Compatibility

Consumers that relied on automatic newline insertion must supply delimiters themselves or enable AppendDelimiterToRecord. Output remains immediate and uncompressed in this PR: buffering hints and compression settings do not acquire their later behavior here.

The SES delimiter hunk is also present in #2712's broader SES publishing fix; it can be deduplicated when rebasing whichever PR lands second. This PR does not depend on #2712, #2713, or #2714.

Test plan

  • Independent detailed code review; follow-up fixes include FileExtension validation, CloudFormation handling, delimiter processing, and MD5 ETags.
  • cargo clippy --workspace --all-targets -- -D warnings.
  • cargo fmt --all --check.
  • Upstream CI-equivalent workspace test command below: passed. It includes the Firehose, SES, and CloudFormation unit regressions.
  • cargo test -p fakecloud-conformance --lib — 94 passed.
  • cargo build --bin fakecloud followed by cargo test -p fakecloud-e2e --test firehose --test firehose_persistence — 12 + 3 passed, using official SDK clients.
  • cargo test -p fakecloud-conformance --test firehose --test ses — 6 + 50 passed.
  • Documentation counts and git diff --check.

The SDK coverage checks exact binary payloads and producer delimiters, the optional newline processor and its updates, prefix/slash behavior, UTC key/version/UUID structure, valid/invalid extension overrides, ETags, and restart durability.

Local verification limitation

cargo test --workspace \
  --exclude fakecloud-e2e --exclude fakecloud-conformance \
  --exclude fakecloud-tfacc --exclude fakecloud-parity \
  -- --skip data_plane::tests::vpc_link_proxy_resolves_target_nlb_and_attempts_connection

This matches upstream's CI workspace test selection, with one local-only API Gateway exclusion: unused localhost ports time out under this WSL network configuration, yielding 504 rather than the fixture's expected immediate-refusal 502. The exact failure was reproduced on untouched upstream 041ddc563. No tests are disabled in this PR, and conformance baselines are unchanged.

The previously attempted unrestricted workspace run also encountered unavailable Podman, an RDS readiness timeout, and the tool timeout during Terraform provider acceptance tests. Those environments are covered by separate upstream workflows; this PR's affected SDK/conformance tests above passed.


Summary by cubic

Aligns Firehose S3 delivery with AWS: delivered objects now contain exact record bytes instead of auto-inserted newlines, and object keys follow AWS's UTC hierarchy with stream/version/time suffix and UUID naming, with validated FileExtension overrides. Adds the built-in AppendDelimiterToRecord processor (writes \n after every record, including the last and empty ones), MD5-based ETags, and CloudFormation storage of FileExtension and ProcessingConfiguration so the delimiter processor works for stack-created streams. PutRecordBatch now accepts empty records.

Compatibility

Consumers relying on automatic newline insertion must supply their own delimiters or enable AppendDelimiterToRecord. Output remains immediate and uncompressed; buffering and compression behavior is unchanged in this increment.

Written for commit 45df858. Summary will update on new commits.

View guided diff

# Conflicts:
#	crates/fakecloud-firehose/src/service.rs
#	crates/fakecloud-ses/src/fanout.rs
- Store ProcessingConfiguration from CloudFormation
  AWS::KinesisFirehose::DeliveryStream S3 destinations (normalizing a
  string Enabled to a boolean), so AppendDelimiterToRecord works for
  stack-created streams too.
- Accept empty records in PutRecordBatch, matching PutRecord and the
  model's min-length-0 Data blob.
- Drop the duplicated SES newline test now covered by the merged SES
  publishing fix, and update the Kinesis-source admission tests for
  verbatim record bytes.
- Firehose service docs: use colons instead of em-dashes and describe
  CloudFormation ProcessingConfiguration support.
# Conflicts:
#	crates/fakecloud-cloudformation/src/resource_provisioner/firehose.rs
@vieiralucas
vieiralucas merged commit f4f41e5 into faiscadev:main Oct 16, 2026
162 checks passed
@vieiralucas

Copy link
Copy Markdown
Member

Thanks a lot for this, @cavanaug. Keeping record bytes verbatim, matching the real S3 object key layout, and using the body MD5 as the ETag closes a real fidelity gap for anyone reading Firehose output back out of S3. I pushed a few follow-ups before merging: CloudFormation AWS::KinesisFirehose::DeliveryStream resources now store the same ProcessingConfiguration (so AppendDelimiterToRecord works from templates too, with string Enabled values normalized), PutRecordBatch accepts empty records the same way PutRecord does, the SES event newline is no longer added twice now that #2712 landed, the existing tests were updated for verbatim bytes, and the docs dashes were tidied. Merged. Really appreciate the contribution, and thanks for the whole Firehose series.

@cavanaug

Copy link
Copy Markdown
Contributor Author

I’ll work on the 3 follow ons previously described.

I’ll also be working on supporting the thruput thresholds as well. Not so much for testing at scale but for scaling down the thresholds to test application response when thresholds are reached.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants