snapshot: harden object offload validation#1133
Conversation
|
Current head: Evidence:
Validation:
Please review the immutable object commit protocol, manifest trust boundary, conditional S3 semantics, download filesystem safety, and offline restore flow. @codex review |
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughマニフェストのConfState検証を拡張し、S3保存に条件付きリトライとマルチパート処理を追加した。SSE/KMS情報をObjectInfoへ反映し、暗号化設定の検証をCLI、保存、取得処理へ統合した。 Changesスナップショットオフロード検証と保存
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SnapshotOffloadCLI
participant S3Store
participant S3ObjectClient
participant S3
SnapshotOffloadCLI->>S3Store: S3暗号化設定を検証
S3Store->>S3ObjectClient: 条件付きPutObjectまたはマルチパート保存
S3ObjectClient->>S3: オブジェクトを書き込む
S3->>S3ObjectClient: オブジェクト情報と暗号化メタデータを返す
S3ObjectClient->>S3Store: 保存結果を返す
S3Store->>S3Store: SSE/KMSとチェックサムを検証
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the elastickv-physical-snapshot command-line utility and its supporting snapshotoffload package to enable publishing, downloading, and restoring Raft/FSM snapshots using S3-compatible object storage. Feedback on the changes highlights a critical issue in the membership validation logic that incorrectly rejects valid joint consensus states where a node ID appears in both voters and outgoing voters. Additionally, the reviewer recommended using deferred Close() calls in download.go and publisher.go to prevent resource leaks during panics and to ensure that errors during cleanup are properly logged or propagated.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/design/2026_07_19_proposed_physical_snapshot_object_offload.md (1)
143-154: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winファイル名とヘッダーを
partialに揃えてください
M0/M1 は完了済みで、M2/M3 は未完了です。docs/design/README.mdの定義ではこの段階は*_partial_*.mdに該当するため、2026_07_19_partial_physical_snapshot_object_offload.mdへgit mvし、Status: Partialに更新してください。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design/2026_07_19_proposed_physical_snapshot_object_offload.md` around lines 143 - 154, Rename the design document with git mv to 2026_07_19_partial_physical_snapshot_object_offload.md and update its header status to Partial, reflecting completed M0/M1 and pending M2/M3. Update every reference, including docs/design/README.md references if applicable, and verify no references to the old proposed filename remain.Source: Coding guidelines
🧹 Nitpick comments (1)
cmd/elastickv-physical-snapshot/operations.go (1)
97-105: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win必須フィールド検証ロジックが3箇所で重複し、mapのイテレーション順序が非決定的
parsePublishFlags、validateDownloadConfig、parseRestoreFlagsで同じ「map[string]stringを作ってrequiredを呼ぶ」パターンが重複しています。Goの map イテレーション順序は非決定的なため、複数の必須フラグが同時に欠落した場合、実行ごとに異なるエラーメッセージが返る可能性があります。順序付きの
[]struct{name, value string}を使う共通ヘルパーに抽出することで、重複を解消し、エラーメッセージを決定的にできます。♻️ 修正案(例)
+func requireAll(fields ...[2]string) error { + for _, f := range fields { + if err := required(f[0], strings.TrimSpace(f[1])); err != nil { + return err + } + } + return nil +}呼び出し側:
- for name, value := range map[string]string{ - "data-dir": cfg.dataDir, - "cluster-id": cfg.clusterID, - "prefix": cfg.prefix, - } { - if err := required(name, strings.TrimSpace(value)); err != nil { - return nil, err - } - } + if err := requireAll( + [2]string{"data-dir", cfg.dataDir}, + [2]string{"cluster-id", cfg.clusterID}, + [2]string{"prefix", cfg.prefix}, + ); err != nil { + return nil, err + }Also applies to: 170-178, 273-282
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/elastickv-physical-snapshot/operations.go` around lines 97 - 105, Extract the duplicated required-field validation from parsePublishFlags, validateDownloadConfig, and parseRestoreFlags into a shared helper accepting an ordered []struct{name, value string}. Preserve each caller’s existing field order and required validation behavior so the first missing field and resulting error are deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/elastickv-physical-snapshot/main.go`:
- Around line 63-68: Update the required function to create its validation error
with the file’s existing github.com/cockroachdb/errors package instead of
fmt.Errorf, preserving the current message and required-value behavior.
---
Outside diff comments:
In `@docs/design/2026_07_19_proposed_physical_snapshot_object_offload.md`:
- Around line 143-154: Rename the design document with git mv to
2026_07_19_partial_physical_snapshot_object_offload.md and update its header
status to Partial, reflecting completed M0/M1 and pending M2/M3. Update every
reference, including docs/design/README.md references if applicable, and verify
no references to the old proposed filename remain.
---
Nitpick comments:
In `@cmd/elastickv-physical-snapshot/operations.go`:
- Around line 97-105: Extract the duplicated required-field validation from
parsePublishFlags, validateDownloadConfig, and parseRestoreFlags into a shared
helper accepting an ordered []struct{name, value string}. Preserve each caller’s
existing field order and required validation behavior so the first missing field
and resulting error are deterministic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 31fd3fc0-015b-4441-8d13-770c2e174df2
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
cmd/elastickv-physical-snapshot/config.gocmd/elastickv-physical-snapshot/main.gocmd/elastickv-physical-snapshot/main_test.gocmd/elastickv-physical-snapshot/operations.godocs/design/2026_07_19_proposed_physical_snapshot_object_offload.mdgo.modinternal/snapshotoffload/download.gointernal/snapshotoffload/manifest.gointernal/snapshotoffload/manifest_test.gointernal/snapshotoffload/publisher.gointernal/snapshotoffload/publisher_test.gointernal/snapshotoffload/s3_store.gointernal/snapshotoffload/s3_store_test.gointernal/snapshotoffload/store.go
![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: 14056d6226
ℹ️ 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".
|
Current head: Addressed all nine root findings from the prior head:
Additional design status work:
Semantic caller audit:
Validation:
The full @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce379ceff5
ℹ️ 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".
| return errors.Wrapf(ErrRemoteIntegrity, "key=%s kind=%q expected=%q", key, metadataKind, kind) | ||
| case encryption != "" && info.Encryption != encryption: | ||
| return errors.Wrapf(ErrRemoteIntegrity, "key=%s encryption=%q expected=%q", key, info.Encryption, encryption) | ||
| case encryption == EncryptionKMS && strings.TrimSpace(info.KMSKeyID) != strings.TrimSpace(kmsKeyID): |
There was a problem hiding this comment.
When operators pass a KMS alias or bare key id to --kms-key-id (values S3 accepts for SSE-KMS uploads), S3 can report the canonical key identifier/ARN on HEAD/GET; this exact string comparison then rejects the object after a successful publish, and later downloads fail with ErrRemoteIntegrity, even though the requested key was used. Either require a canonical ARN/key ID at validation time or resolve aliases before comparing.
Useful? React with 👍 / 👎.
| s3DefaultMultipartPart = int64(64 * 1024 * 1024) | ||
| s3MaxMultipartPart = int64(5 * 1024 * 1024 * 1024) | ||
| s3MaxMultipartParts = int64(10_000) | ||
| s3MaxObjectBytes = int64(5 * 1024 * 1024 * 1024 * 1024) |
There was a problem hiding this comment.
For physical snapshots above 5 TiB, multipartPartSize returns ErrObjectInvalid before trying multipart upload, but the current S3 multipart limits allow substantially larger objects with up to 10,000 parts (AWS docs). Since the code already scales part size up to the 5 GiB part limit, this hard cap unnecessarily blocks valid large snapshot uploads; raise the object limit to match S3 or make it backend-configurable.
Useful? React with 👍 / 👎.
| } | ||
| if prior, ok := seen[id]; ok { | ||
| jointVoter := prior == "voters" && names[i] == "voters_outgoing" | ||
| if !jointVoter && prior != "learners_next" && names[i] != "learners_next" { |
There was a problem hiding this comment.
When a manifest has learners_next containing an incoming voter/learner, this condition exempts the duplicate solely because one side is learners_next, so voters:[1], learners_next:[1] passes DecodeManifest as a validated snapshot witness. In etcd joint consensus, LearnersNext is only for outgoing voters being demoted, so accepting these impossible ConfStates makes corrupted or hand-written manifests look valid instead of failing closed.
Useful? React with 👍 / 👎.
| if _, ok := learners[peers[i].ID]; ok { | ||
| peers[i].Suffrage = etcd.SuffrageLearner | ||
| } else { | ||
| peers[i].Suffrage = etcd.SuffrageVoter | ||
| } |
There was a problem hiding this comment.
If --learners names every peer, this loop marks the whole restore membership as learners and returns success, so runRestore proceeds to fetch the snapshot from S3 and only later fails inside PreparePhysicalSnapshotRestore with “at least one voter is required”. For large physical snapshots this turns a locally detectable flag error into an unnecessary full download; count voters here and fail before contacting the object store.
Useful? React with 👍 / 👎.
ce379ce to
b9cdf7e
Compare
bootjp
changed the title
|
Current head: Rebased onto current Addressed current safety gaps:
Validation:
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/snapshotoffload/manifest.go`:
- Around line 129-146: Update validateManifestConfState to reject duplicate IDs
within each individual role list, while preserving the existing zero-ID and
cross-role validation. Track IDs separately for each role during iteration and
return ErrInvalidOptions with the role name when a duplicate is found.
In `@internal/snapshotoffload/s3_store.go`:
- Around line 217-306: Update createMultipartUpload and uploadParts to honor
s.disableChecksumHeaders consistently with the single-PUT path: omit
ChecksumAlgorithm and ChecksumSHA256 fields when disabled, including
CompletedPart.ChecksumSHA256, while retaining SHA-256 checksum calculation and
headers when enabled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0faa4fd3-65a5-43b4-a7a6-5486cf9b2cf8
📒 Files selected for processing (7)
cmd/elastickv-snapshot-offload/main.gocmd/elastickv-snapshot-offload/main_test.gointernal/snapshotoffload/manifest.gointernal/snapshotoffload/offload_test.gointernal/snapshotoffload/s3_store.gointernal/snapshotoffload/s3_store_test.gointernal/snapshotoffload/store.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9cdf7e09d
ℹ️ 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".
| input := &s3.CreateMultipartUploadInput{ | ||
| Bucket: aws.String(s.bucket), | ||
| Key: aws.String(key), | ||
| ChecksumAlgorithm: types.ChecksumAlgorithmSha256, |
There was a problem hiding this comment.
When --s3-disable-checksum-headers is set and a payload exceeds the single-PUT limit, this multipart path still sends checksum headers because CreateMultipartUpload always sets ChecksumAlgorithm here and UploadPart also unconditionally sends SHA256 checksums. That preserves the failure mode the flag is meant to avoid for S3-compatible stores without checksum-header support, so large snapshots can fail even though the user explicitly disabled those headers.
Useful? React with 👍 / 👎.
| sum := sha256.Sum256(part) | ||
| checksum := base64.StdEncoding.EncodeToString(sum[:]) |
There was a problem hiding this comment.
For multipart uploads, this only hashes each part for S3 and never compares an accumulated full-object hash with opts.SHA256 before completing the upload. If the source file changes or is corrupted between spooling and this upload path, S3 accepts the same-length parts and the later HEAD check trusts the pre-written metadata SHA, so publishing a >5 GiB snapshot can succeed while storing bytes that restore will later reject.
Useful? React with 👍 / 👎.
| s3DefaultMultipartPart = int64(64 * 1024 * 1024) | ||
| s3MaxMultipartPart = int64(5 * 1024 * 1024 * 1024) | ||
| s3MaxMultipartParts = int64(10_000) | ||
| s3MaxObjectBytes = int64(5 * 1024 * 1024 * 1024 * 1024) |
There was a problem hiding this comment.
This new hard cap rejects any snapshot payload above 5 TiB before upload, but current S3 multipart limits allow up to 10,000 parts of 5 GiB each, i.e. 48.8 TiB (AWS docs). In deployments with very large persisted FSM snapshots, offload now returns ErrInvalidOptions for valid S3 objects instead of choosing a larger part size and proceeding.
Useful? React with 👍 / 👎.
| ChecksumAlgorithm: types.ChecksumAlgorithmSha256, | ||
| ChecksumSHA256: aws.String(checksum), |
There was a problem hiding this comment.
For real AWS S3 multipart uploads, sending per-part SHA256 checksums here makes S3 expose a composite SHA256 checksum on GetObject when the SDK enables checksum mode. s3ChecksumSHA expects a plain full-object base64 digest, so restoring a >5 GiB object uploaded by this path can fail while parsing the returned checksum before the payload body is verified.
Useful? React with 👍 / 👎.
|
Current head: Addressed the current snapshot offload review findings:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdf55743f0
ℹ️ 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".
| remaining := totalBytes | ||
| for partNumber := int32(1); remaining > 0; partNumber++ { | ||
| partBytes := min(remaining, partSize) | ||
| part := make([]byte, int(partBytes)) |
There was a problem hiding this comment.
For snapshots above about 640 GiB, multipartPartSize grows partBytes beyond the 64 MiB default to stay under 10,000 parts; at the newly accepted S3 limit it reaches 5 GiB. This allocation loads each part fully into RAM before every UploadPart, so a snapshot that passes validation can OOM the offload process before upload completes; stream/hash a section of the spooled file instead of allocating partBytes.
Useful? React with 👍 / 👎.
| if metadataSHA != "" { | ||
| return metadataSHA, nil |
There was a problem hiding this comment.
When an existing S3 object has both our SHA metadata and a real full-object S3 checksum, this early return trusts the user-controlled metadata and never checks whether the S3 checksum disagrees. In that case verifyExistingStoreObject can reuse an overwritten/corrupt payload without downloading it, publish a manifest for the expected SHA, and leave restore to fail later; only composite multipart checksum values should be ignored, not every checksum whenever metadata is present.
Useful? React with 👍 / 👎.
|
@codex review Addressed the latest snapshot offload review findings in 7cea249:
Caller audit:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cea2491f0
ℹ️ 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".
| s3DefaultMultipartPart = int64(64 * 1024 * 1024) | ||
| s3MaxMultipartPart = int64(5 * 1024 * 1024 * 1024) | ||
| s3MaxMultipartParts = int64(10_000) | ||
| s3MaxObjectBytes = s3MaxMultipartPart * s3MaxMultipartParts |
There was a problem hiding this comment.
For snapshots larger than S3's 5 TiB object limit but below this computed 50 TiB ceiling, multipartPartSize accepts the input and the store can upload thousands of parts before CompleteMultipartUpload fails because the final object is too large. That turns an unsupported snapshot size into a very expensive late failure; cap this constant at the service object limit instead of deriving it from max part size times max part count.
Useful? React with 👍 / 👎.
|
Evidence for current HEAD 7db43c8: Addressed the latest snapshot offload review findings:
Validation:
Resolved the remaining current checksum-mode review thread. Current-head CI is running after the push. @codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Validation
go test ./internal/snapshotoffload ./cmd/elastickv-snapshot-offload -count=1go test ./internal/snapshotoffload ./internal/raftengine/etcd ./cmd/elastickv-snapshot-offload -count=1go test ./... -run '^$'golangci-lint run ./internal/snapshotoffload ./cmd/elastickv-snapshot-offload --timeout=5mgolangci-lint --config=.golangci.yaml run --fixAuthor: bootjp
Summary by CodeRabbit