Skip to content

Project sidecar writer registry#1058

Open
bootjp wants to merge 10 commits into
mainfrom
design/encryption-stage7-projection
Open

Project sidecar writer registry#1058
bootjp wants to merge 10 commits into
mainfrom
design/encryption-stage7-projection

Conversation

@bootjp

@bootjp bootjp commented Jul 10, 2026

Copy link
Copy Markdown
Owner

Summary

  • project WriterRegistryForCaller from the writer registry in GetSidecarState and ResyncSidecar
  • wire the writer registry into each shard's EncryptionAdmin server
  • mark Stage 7 writer-registry projection shipped in the encryption design docs

Validation

  • git diff --check
  • go test ./adapter -run 'TestEncryptionAdmin_(GetSidecarState|ResyncSidecar)' -count=1 -timeout=240s
  • go test ./adapter -run TestEncryptionAdmin -count=1 -timeout=300s
  • go test ./cmd/elastickv-admin -run 'TestEncryption(Status|Endpoint|Sidecar)' -count=1 -timeout=180s
  • go test ./store ./internal/encryption . -count=1 -timeout=180s
  • golangci-lint run ./adapter ./store ./internal/encryption . --timeout=5m

Author: bootjp

Summary by CodeRabbit

  • 新機能

    • サイドカー状態取得・再同期時に、呼び出し元ごとの Writer Registry 情報を返すようになりました。
    • DEK に対応するレジストリ情報がない場合は、誤ったゼロ値を返さず除外します。
    • 不正なノード ID の衝突を検出し、エラーとして扱います。
    • Writer Registry 未設定時も、空のマップを返すようになりました。
  • ドキュメント

    • 部分暗号化およびサイドカーのレジストリ投影機能の実装状況を更新しました。

bootjp commented Jul 10, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@coderabbitai

coderabbitai Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@bootjp, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 51 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 50c2421f-c9a3-4165-8522-e8e7480fd506

📥 Commits

Reviewing files that changed from the base of the PR and between 101ca68 and a39527d.

⛔ Files ignored due to path filters (1)
  • proto/encryption_admin.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (36)
  • adapter/encryption_admin.go
  • adapter/encryption_admin_test.go
  • docs/design/2026_04_29_partial_data_at_rest_encryption.md
  • docs/design/2026_05_29_implemented_7c_confchange_time_registration.md
  • docs/design/2026_07_11_implemented_7d_sidecar_registry_projection.md
  • docs/design/2026_07_18_implemented_9a_encryption_compression.md
  • go.mod
  • internal/admin/capability_fanout.go
  • internal/admin/capability_fanout_test.go
  • internal/encryption/envelope.go
  • internal/encryption/envelope_test.go
  • internal/encryption/errors.go
  • internal/encryption/raft_envelope.go
  • internal/encryption/raft_envelope_test.go
  • internal/raftadmin/interceptor.go
  • internal/raftadmin/server.go
  • internal/raftadmin/server_test.go
  • main.go
  • main_encryption_admin.go
  • main_encryption_admin_test.go
  • main_encryption_confchange.go
  • main_encryption_confchange_test.go
  • main_encryption_fanout.go
  • main_encryption_fanout_test.go
  • main_encryption_write_wiring.go
  • main_encryption_write_wiring_test.go
  • multiraft_runtime.go
  • proto/encryption_admin.proto
  • store/encryption_compression_test.go
  • store/encryption_glue.go
  • store/lsm_store.go
  • store/lsm_store_env_test.go
  • store/lsm_store_sync_mode_benchmark_test.go
  • store/lsm_store_test.go
  • store/snapshot_pebble_sst.go
  • store/snapshot_pebble_sst_test.go
📝 Walkthrough

Walkthrough

EncryptionAdminServer が writer registry を受け取り、GetSidecarStateResyncSidecar で caller 別の WriterRegistryForCaller を返すようになりました。ランタイム配線、テスト、Stage 7 の設計文書も更新されています。

Changes

Writer registry projection

Layer / File(s) Summary
投影契約と実装
adapter/encryption_admin.go
WriterRegistryStore を保持・設定し、サイドカーの DEK と caller node ID に対応する registry 行から LastSeenLocalEpoch を投影する。欠損行は省略し、無効な caller ID、取得・デコード失敗、node collision はエラーにする。
ランタイム配線
multiraft_runtime.go, main.go
raftGroupRuntime に writer registry を保持し、EncryptionAdminServer 登録時に WithEncryptionAdminWriterRegistry で渡す。
投影動作の検証
adapter/encryption_admin_test.go
ローカル caller と Resync caller の投影、欠損 DEK の省略、未配線時の非 nil 空 map、node collision の拒否を検証する。
Stage 7 実装記録
docs/design/...
Stage 7 の出荷状況と sidecar registry projection の実装内容を更新する。

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Sidecar
  participant EncryptionAdminServer
  participant WriterRegistryStore
  Sidecar->>EncryptionAdminServer: GetSidecarState または ResyncSidecar を呼び出す
  EncryptionAdminServer->>WriterRegistryStore: DEK と caller node ID で registry 行を取得する
  WriterRegistryStore-->>EncryptionAdminServer: registry 行を返す
  EncryptionAdminServer-->>Sidecar: WriterRegistryForCaller を返す
Loading

Possibly related PRs

  • bootjp/elastickv#754: 同じ EncryptionAdmin RPC の空 map 応答を実装しており、本変更が実データ投影へ拡張している。
  • bootjp/elastickv#760: EncryptionAdminServer の登録経路を追加しており、本変更が registry オプションを同経路へ渡している。
  • bootjp/elastickv#765: 本変更が利用する WriterRegistryStoreGetRegistryRow の実装を追加している。
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed サイドカーの writer registry 投影と配線という主要変更を概ね表しており、タイトルとして十分に関連しています。
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@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 implements the remaining Stage 7 §5.5 recovery surface for WriterRegistryForCaller by wiring the WriterRegistryStore into EncryptionAdminServer. Both GetSidecarState and ResyncSidecar now project the writer registry for the caller, returning the decoded LastSeenLocalEpoch for each DEK present in the sidecar. The review feedback correctly points out that returning codes.InvalidArgument when fullNodeID == 0 is misleading for GetSidecarState since it uses the local node's ID, and recommends returning codes.FailedPrecondition or codes.Internal instead.

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 adapter/encryption_admin.go
@bootjp

bootjp commented Jul 10, 2026

Copy link
Copy Markdown
Owner Author

Addressed latest-head review finding:

  • GetSidecarState now reports missing local fullNodeID as a server-side Internal error when the writer registry is wired
  • ResyncSidecar keeps returning InvalidArgument for a missing caller-supplied caller_full_node_id

Caller audit:

  • production GetSidecarState caller is cmd/elastickv-admin/encryption.go; Internal bubbles up as a non-zero CLI error instead of being treated as soft unavailable
  • no production ResyncSidecar caller was found

Validation:

  • go test ./adapter -run 'TestEncryptionAdmin_(GetSidecarState|ResyncSidecar)' -count=1 -timeout=240s\n- go test ./adapter -run TestEncryptionAdmin -count=1 -timeout=300s\n- go test ./cmd/elastickv-admin -run 'TestEncryption(Status|Endpoint|Sidecar)' -count=1 -timeout=180s\n- golangci-lint run ./adapter ./store ./internal/encryption . --timeout=5m\n- git diff --check\n- git verify-commit HEAD\n\n@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c8c99a0a8c

ℹ️ 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".

Comment thread main.go Outdated
@bootjp

bootjp commented Jul 10, 2026

Copy link
Copy Markdown
Owner Author

Addressed latest-head P2 review finding:

  • EncryptionAdmin sidecar projection now uses the default group writer registry for every shard listener, matching the startup/runtime registration authority.
  • Added coverage that non-default runtimes do not supply their per-shard registry to sidecar projection.

Validation:

  • go test ./adapter -run TestEncryptionAdmin -count=1 -timeout=300s
  • go test ./cmd/elastickv-admin -run 'TestEncryption(Status|Endpoint|Sidecar)' -count=1 -timeout=180s\n- go test . -run 'Test(WriterRegistryForEncryptionAdmin|EncryptionAdmin)' -count=1 -timeout=180s\n- go test ./store ./internal/encryption . -count=1 -timeout=180s\n- git diff --check\n- golangci-lint run ./adapter ./store ./internal/encryption . --timeout=5m\n- git verify-commit HEAD\n\n@codex review

@bootjp

bootjp commented Jul 11, 2026

Copy link
Copy Markdown
Owner Author

Current head is green and the latest sidecar projection findings have been addressed with tests. @codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: cbc93a7bd7

ℹ️ 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".

@bootjp

bootjp commented Jul 11, 2026

Copy link
Copy Markdown
Owner Author

Current head already handles the remaining sidecar projection thread:

  • GetSidecarState passes codes.Internal for missing local fullNodeID, treating it as server configuration state.
  • ResyncSidecar still passes codes.InvalidArgument for missing caller-supplied caller_full_node_id.

Verification:

  • go test ./adapter -run 'TestEncryptionAdmin_GetSidecarState_(ProjectsWriterRegistryForLocalNode|RejectsMissingLocalNodeIDAsInternal)|TestEncryptionAdmin_ResyncSidecar_(ProjectsWriterRegistryForCaller|RejectsMissingCallerNodeIDAsInvalidArgument)' -count=1 -timeout=120s

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Breezy!

Reviewed commit: cbc93a7bd7

ℹ️ 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".

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b3acb8331c

ℹ️ 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".

Comment thread main.go Outdated
Comment thread store/encryption_glue.go
Comment thread store/encryption_glue.go
@bootjp
bootjp force-pushed the design/encryption-stage7-projection branch from b3acb83 to 39142ba Compare July 19, 2026 11:59
@bootjp

bootjp commented Jul 19, 2026

Copy link
Copy Markdown
Owner Author

Resolved the latest-head findings in 39142ba090d5e6ab26fe87a2cf9bf41bd5449e57:

  • recovery projection now uses the default-group LeaderView and applied index on every shard listener
  • compressed storage writes use envelope V2, while V1 remains uncompressed-only, so old readers fail closed
  • visibility-only scans authenticate without Snappy decompression

Caller audit:

  • DecodeEnvelope has two production paths: storage accepts V1/V2 by version/flag contract; Raft additionally requires V1/flag=0
  • plaintext-returning storage callers keep decryptForKey; the two visibility-only callers use authenticateForKey
  • only ResyncSidecar uses the default-group recovery LeaderView; all five mutating RPC callers retain the per-shard requireLeader path

Verification:

  • focused tests: root, adapter, internal/encryption, and store passed
  • focused race tests: root, adapter, internal/encryption, and store passed
  • golangci-lint --config=.golangci.yaml run --fix: 0 issues
  • golangci-lint run . ./adapter ./internal/encryption ./store --timeout=10m --allow-parallel-runners: 0 issues
  • go mod tidy -diff and git diff --check: clean

History/author audit:

  • rebuilt the PR on current main and removed the GitHub-authored merge commit b3acb833
  • git range-diff cd583545..b3acb833 origin/main..20f222eb maps all six source commits in order; four are patch-identical, the two context-only differences are the current-main server struct context and current-main dependency versions
  • the only post-rebuild intentional delta is 39142ba0, containing the three review fixes above
  • GitHub compare reports all seven PR-visible commits as verified=true, with author and committer both bootjp <contact@bootjp.me>
  • the branch is now MERGEABLE; current CI is the remaining gate

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Hooray!

Reviewed commit: 39142ba090

ℹ️ 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".

@bootjp
bootjp force-pushed the design/encryption-stage7-projection branch from 39142ba to 04bfde5 Compare July 19, 2026 14:01
@bootjp

bootjp commented Jul 19, 2026

Copy link
Copy Markdown
Owner Author

Rebased the signed stack onto current main 910a97e0 after #1121/#1127/#1130 advanced the base. git range-diff 915bc777..backup/pr1058-39142ba0-v2 origin/main..04bfde58 maps the seven prior commits in order: six are patch-identical; the compression commit differs only where current main moved restore/open logic into reopenStoreDB. The intentional integration delta 04bfde58 threads the encrypted-store NoCompression policy through the new SST snapshot export/ingest callers and adds an encrypted SST snapshot round-trip test. Focused store/root/adapter/provider tests passed with an isolated Go cache, targeted lint reports 0 issues, and the commit hook reports 0 issues. All eight PR-visible commits have author and committer bootjp contact@bootjp.me with valid signatures. @codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 04bfde58f0

ℹ️ 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".

Comment thread store/snapshot_pebble_sst.go
Comment thread store/encryption_glue.go
Comment thread main.go Outdated
@bootjp
bootjp force-pushed the design/encryption-stage7-projection branch from 04bfde5 to e34b4de Compare July 19, 2026 15:25
@bootjp

bootjp commented Jul 19, 2026

Copy link
Copy Markdown
Owner Author

Current head: e34b4de

Resolved review findings:

  • P1: EncryptionAdmin mutators are exposed only with default-group authority; non-default listeners are read-only.
  • P1: V2 compressed writes remain off until every voter/learner in every group advertises storage_envelope_v2_capable. Older members that omit the field fail closed. Joining members are probed before registry mutation while encryption is active.
  • P2: encrypted SST snapshot export sets sstable.NoCompression on generated SST writers, not only on the read-only checkpoint DB.

Semantic caller audit:

  • encryptForKey production callers are PutAt, ExpireAt, and mutation apply; all share the same store-local V2 activation gate.
  • MembershipChangeInterceptor.PreAddMember production callers are AddVoter and AddLearner; both pass the candidate address and fail before registry proposal when capability is absent.
  • EncryptionAdmin registration is centralized in startRaftServers; only the default group receives proposer/barrier mutator options.

Validation:

  • make gen
  • focused tests for adapter, root wiring, capability fanout, raft admin, compression gate, and encrypted SST snapshot round-trip
  • go test ./store -count=1 -timeout=10m
  • golangci-lint run ./store ./internal/admin ./internal/raftadmin ./adapter . --timeout=8m --allow-parallel-runners: 0 issues
  • git diff --check
  • all 9 PR-visible commits are bootjp contact@bootjp.me, signed, and GitHub reports verified=true

History audit:

  • 5 of the prior 8 patches are patch-identical after rebuild.
  • The other 3 retain only required current-main integrations for S3 observer arguments, the process-wide shared Pebble cache, and the current SST snapshot API.
  • e34b4de is the review-fix commit.

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e34b4de8a6

ℹ️ 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".

Comment thread main_encryption_fanout.go Outdated
Comment thread main_encryption_confchange.go
Comment thread main_encryption_confchange.go
@bootjp

bootjp commented Jul 19, 2026

Copy link
Copy Markdown
Owner Author

Current head: a39527d

Resolved latest-head findings:

  • P1: the sticky V2 write latch cannot run before ActiveStorageKeyID confirms bootstrap; pre-bootstrap AddVoter/AddLearner also probe candidates, closing bootstrap/join ordering races.
  • P2: each join-time capability RPC has an internal 5-second deadline.
  • P2: CapabilityReport.full_node_id must equal the ID derived from the requested Raft ID before any registry proposal.

Semantic caller audit:

  • MembershipChangeInterceptor.PreAddMember has exactly two production callers: AddVoter and AddLearner.
  • Both callers pass the requested Raft ID and address through the same fail-closed capability path.
  • The probe runs before the active-DEK registry check; pre-bootstrap calls validate capability and then skip registry work.
  • After bootstrap, storage and optional raft registry registration retain their prior behavior.

Validation:

  • go test . -count=1 -timeout=8m
  • focused tests for pre-bootstrap monitor behavior, pre-bootstrap membership probe, probe timeout, node-ID mismatch, and default-group mutator authority
  • go test ./internal/raftadmin ./internal/admin ./adapter ./store with the encryption/capability/SST test selection
  • golangci-lint run . ./internal/raftadmin ./internal/admin ./adapter ./store --timeout=8m --allow-parallel-runners: 0 issues
  • git diff --check
  • signed bootjp contact@bootjp.me; GitHub verification is valid

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a39527d37d

ℹ️ 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".

Comment on lines +98 to +99
if err := e.capabilityProbe(ctx, address, newNodeFullID); err != nil {
return errors.Wrap(err, "encryption: refuse member before V2 capability confirmation")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Skip V2 probing when encryption is not configured

When the cluster runs without encryption/sidecar configuration, run() still constructs this interceptor because encWiring.cache is defaulted and the default group store exists, so AddVoter/AddLearner now reach this unconditional probe before the previous ActiveStorageKeyID no-op path. A target node with no sidecar reports full_node_id=0/encryption_capable=false, causing membership changes in non-encrypted deployments to fail even though the interceptor should be inert there; gate interceptor construction or this probe on encryption being configured rather than just on the cache existing.

Useful? React with 👍 / 👎.

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.

1 participant