Skip to content

[Store] Externalize S3 client config via environment variables#2649

Open
mzygQAQ wants to merge 4 commits into
kvcache-ai:mainfrom
mzygQAQ:feature/s3-use-https-env-var
Open

[Store] Externalize S3 client config via environment variables#2649
mzygQAQ wants to merge 4 commits into
kvcache-ai:mainfrom
mzygQAQ:feature/s3-use-https-env-var

Conversation

@mzygQAQ

@mzygQAQ mzygQAQ commented Jun 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

Externalize three S3 client knobs as environment variables so they can be tuned at runtime without rebuilding. Defaults preserve current behavior — existing users see
no change.

New environment variables

Env var Values Default Purpose
MOONCAKE_AWS_S3_USE_HTTPS true / false true Switch between HTTPS and HTTP scheme
MOONCAKE_AWS_S3_REQUEST_CHECKSUM when_supported / when_required when_supported PUT request checksum behavior
MOONCAKE_AWS_S3_RESPONSE_CHECKSUM when_supported / when_required when_supported GET response checksum validation

Why

  • MOONCAKE_AWS_S3_USE_HTTPS — the original code hardcoded HTTPS, breaking deployments to plaintext MinIO/Ceph.
  • The two checksum vars — AWS SDK 1.11+ added flexible checksum trailers that fail PUT against MinIO and other S3-compatible backends. Setting when_required restores
    pre-1.11 behavior.

Backward compatibility

All three defaults match the previous behavior:

  • scheme: HTTPS (was hardcoded)
  • checksums: WHEN_SUPPORTED (AWS SDK default, was implicit)

Users on AWS-proper S3 see no change. Users on MinIO/Ceph can now opt into compatible behavior by setting the two checksum vars to when_required.

Module

  • Transfer Engine (mooncake-transfer-engine)
  • Mooncake Store (mooncake-store)
  • Mooncake EP (mooncake-ep)
  • Mooncake PG (mooncake-pg)
  • Integration (mooncake-integration)
  • P2P Store (mooncake-p2p-store)
  • Python Wheel (mooncake-wheel)
  • Common (mooncake-common)
  • Mooncake RL (mooncake-rl)
  • CI/CD
  • Docs
  • Other

Type of Change

  • Bug fix
  • New feature
  • Refactor
  • Breaking change
  • Documentation update
  • Performance improvement
  • Other

How Has This Been Tested?

Test commands:

# Example: bash scripts/run_ci_test.sh

Test results:

  • Unit tests pass
  • Integration tests pass (if applicable)
  • Manual testing done (describe below)

Checklist

  • I have performed a self-review of my own code
  • I have formatted my code using ./scripts/code_format.sh
  • I have run pre-commit run --all-files and all hooks pass
  • I have updated the documentation (if applicable)
  • I have added tests to prove my changes are effective
  • For changes >500 LOC: I have filed an RFC issue

AI Assistance Disclosure

  • No AI tools were used
  • AI tools were used (specify below)

mzygQAQ and others added 3 commits June 27, 2026 15:01
@mzygQAQ @claude
[Store] Externalize S3 client config via environment variables
b729cc1 
Add four new env vars to control S3Helper behavior at runtime:

- MOONCAKE_AWS_S3_USE_HTTPS (bool, default true)
  Switch between HTTPS and HTTP scheme.

- MOONCAKE_AWS_LOG_LEVEL (off/fatal/error/warn/info/debug/trace,
  default error)
  Control AWS SDK log verbosity.

- MOONCAKE_AWS_S3_REQUEST_CHECKSUM (when_supported/when_required,
  default when_supported)
  When set to when_required, avoids the new checksum trailer
  behavior that breaks compatibility with MinIO and some other
  S3-compatible backends.

- MOONCAKE_AWS_S3_RESPONSE_CHECKSUM (when_supported/when_required,
  default when_supported)
  Same purpose as above for response validation.

All defaults preserve backward compatibility with existing behavior.
connection_info_ log line updated to include the new values for
easier debugging.

Co-Authored-By: Claude <noreply@anthropic.com>
@mzygQAQ @claude
[Store] Drop MOONCAKE_AWS_LOG_LEVEL and trim connection_info_
ca43c7e 
- Remove MOONCAKE_AWS_LOG_LEVEL env var, ParseLogLevel function, and
  the corresponding <aws/core/utils/logging/LogLevel.h> include. The
  underlying use case (changing AWS SDK log verbosity at runtime) is
  rarely needed in practice and adds an extra knob to maintain.

- Remove logLevel / requestChecksum / responseChecksum fields from
  the connection_info_ log line to reduce startup log noise.

Co-Authored-By: Claude <noreply@anthropic.com>
@mzygQAQ @claude
[Store] Apply clang-format to s3_helper.cpp
a173d78 
Wrap LOG(WARNING) stream chains and the config.scheme ternary to
match the project's column-width style. Add trailing newline.

Co-Authored-By: Claude <noreply@anthropic.com>
gemini-code-assist[bot]
gemini-code-assist Bot reviewed Jun 27, 2026
View reviewed changes

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request introduces configuration options for AWS S3 client settings, allowing users to configure HTTPS usage and request/response checksum calculations via environment variables. Feedback was provided regarding the checksum parsing logic: the current implementation incorrectly logs a warning when the valid value "when_supported" is supplied. It is recommended to explicitly handle "when_supported" and make the parsing case-insensitive to improve robustness.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread mooncake-store/src/utils/s3_helper.cpp Outdated
@mzygQAQ mzygQAQ force-pushed the feature/s3-use-https-env-var branch 3 times, most recently from 7b63722 to b8531d9 Compare June 27, 2026 07:21
@mzygQAQ @claude
[Store] Fix false 'invalid value' warning for when_supported checksum
491951f 
ParseRequestChecksum / ParseResponseChecksum logged a warning
whenever value != "when_required" and value was non-empty. This
incorrectly flagged the valid value "when_supported" as invalid.

- Add an explicit "when_supported" branch before the warning.
- Lowercase the input via std::transform + std::tolower for
  case-insensitive comparison (matching the convention already
  used in real_client.cpp / ssd_register_client.cpp).
- Collapse the two near-identical functions into a single
  ParseChecksumMode<T>() template — both AWS SDK enum classes
  share the WHEN_REQUIRED / WHEN_SUPPORTED enumerator names.

Co-Authored-By: Claude <noreply@anthropic.com>
@mzygQAQ mzygQAQ force-pushed the feature/s3-use-https-env-var branch from b8531d9 to 491951f Compare June 27, 2026 07:26
@codecov-commenter

codecov-commenter commented Jun 27, 2026

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

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

Reviewers

@ykwd ykwd Awaiting requested review from ykwd ykwd is a code owner
@stmatengss stmatengss Awaiting requested review from stmatengss stmatengss is a code owner
@XucSh XucSh Awaiting requested review from XucSh XucSh is a code owner
@YiXR YiXR Awaiting requested review from YiXR YiXR is a code owner
1 more reviewer
@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

run-ci Store

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mzygQAQ @codecov-commenter