cmek split 7/8: schemastore encrypted persistence#4575
Conversation
Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
ti-chi-bot
Bot
added
the
do-not-merge/release-note-label-needed
|
Warning Review limit reached
Next review available in: 27 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 Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds optional encryption support for schema snapshots and persisted DDL events, including masked-key compatibility and decryption during loading. Persistent storage now propagates the encryption manager, while DDL resolution preserves progress and retries failed persistence. ChangesSchema Store Encryption Integration
Estimated code review effort: 4 (Complex) | ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
ti-chi-bot
Bot
added
the
size/XL
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request integrates encryption into the schema store's persistence layer, ensuring that all schema metadata and DDL events stored on disk are encrypted. It introduces a new encryption manager to handle the cryptographic operations, making the stored data more secure. The changes also improve the robustness of DDL job processing by implementing retry mechanisms for persistence failures and ensuring that schema state updates are contingent on successful encryption. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 adds encryption support to the schemastore's persistence layer. The changes introduce encryption and decryption for various schema-related data stored on disk. The overall approach is good, with the addition of an EncryptionManager and new tests to cover the encryption logic. However, there are several places where log.Fatal is used for error handling upon encryption/decryption failures. This is a critical issue as it can cause the entire service to terminate unexpectedly. My review focuses on replacing these log.Fatal calls with proper error returns. Additionally, I've pointed out the use of context.Background() and suggested passing a context.Context for better control and traceability.
| if err != nil { | ||
| log.Fatal("decrypt db info failed", zap.Error(err)) | ||
| } |
There was a problem hiding this comment.
Using log.Fatal can abruptly terminate the application, making error handling and recovery difficult. It's better to return the error to the caller. Additionally, it's a good practice to pass a context.Context to this function instead of using context.Background() to allow for cancellation and tracing.
| if err != nil { | |
| log.Fatal("decrypt db info failed", zap.Error(err)) | |
| } | |
| if err != nil { | |
| return nil, errors.Wrap(err, "decrypt db info failed") | |
| } |
| if err != nil { | ||
| log.Fatal("decrypt ddl job failed when loading ddl history", | ||
| zap.Uint32("keyspaceID", keyspaceID), | ||
| zap.Binary("ddlKey", append([]byte(nil), snapIter.Key()...)), | ||
| zap.Error(err)) | ||
| } |
There was a problem hiding this comment.
Using log.Fatal is risky as it terminates the process. Please return an error instead to allow for graceful error handling. Also, consider passing a context.Context to this function and using it for the decryption call instead of context.Background().
if err != nil {
return nil, nil, errors.Wrap(err, "decrypt ddl job failed when loading ddl history")
}| if err != nil { | ||
| log.Fatal("decrypt ddl event failed", | ||
| zap.Uint64("version", version), | ||
| zap.Uint32("keyspaceID", keyspaceID), | ||
| zap.Error(err)) | ||
| } |
There was a problem hiding this comment.
Using log.Fatal can abruptly terminate the application. It's better to return an error. This would require changing the function signature of readPersistedDDLEventWithEncryption to return an error (e.g., (PersistedDDLEvent, error)), and updating its callers to handle the error. Also, context.Background() should be replaced by a passed-in context for better traceability and cancellation.
| if err != nil { | ||
| log.Fatal("encrypt schema info failed", zap.Error(err)) | ||
| } |
There was a problem hiding this comment.
Using log.Fatal can abruptly terminate the application. It's better to return an error. This would require changing the function signature of addSchemaInfoToBatchWithEncryption to return an error. Also, context.Background() should be replaced by a passed-in context.
| if err != nil { | ||
| log.Fatal("encrypt table info entry failed", zap.Error(err)) | ||
| } |
There was a problem hiding this comment.
Using log.Fatal can abruptly terminate the application. It's better to return an error. This would require changing the function signature of addTableInfoToBatchWithEncryption to return an error. Also, context.Background() should be replaced by a passed-in context.
| if err != nil { | ||
| log.Fatal("decrypt ddl job failed when loading all physical tables", | ||
| zap.Uint32("keyspaceID", keyspaceID), | ||
| zap.Binary("ddlKey", append([]byte(nil), snapIter.Key()...)), | ||
| zap.Error(err)) | ||
| } |
There was a problem hiding this comment.
Using log.Fatal can abruptly terminate the application. Since this function already returns an error, please return the decryption error instead of calling log.Fatal. Also, consider passing a context.Context to this function and using it for the decryption call instead of context.Background().
if err != nil {
return nil, errors.Wrap(err, "decrypt ddl job failed when loading all physical tables")
}|
|
||
| // Encrypt if encryption is enabled | ||
| if encMgr != nil { | ||
| encryptedValue, err := encMgr.EncryptData(context.Background(), keyspaceID, ddlValue) |
There was a problem hiding this comment.
To enable request cancellation and tracing, it's better to pass a context.Context to this function and use it here instead of context.Background(). This would require updating the function signature and its callers.
ti-chi-bot
Bot
added
release-note-none
tenfyzhong
force-pushed
the
split/pr3955-06-eventstore
branch
2 times, most recently
from
c9c0621 to
9aafb69
Compare
ti-chi-bot
Bot
added
size/XXL
Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
size/XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
logservice/schemastore/persist_storage.go (1)
270-278:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftEncryption wiring is partial: snapshot restore path is still plaintext.
This call-site only wires encryption for DDL history.
initializeFromDiskstill loads DB/table snapshots through non-encrypted readers, so CMEK coverage is incomplete for schema snapshot persistence/restore.🤖 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 `@logservice/schemastore/persist_storage.go` around lines 270 - 278, initializeFromDisk currently restores DB/table snapshots with plaintext readers while only loadAndApplyDDLHistory is passed p.encryptionManager and p.keyspaceID; update the snapshot restore path to use the same encryption wiring. Specifically, modify initializeFromDisk (the code paths that load database and table snapshots) to accept and pass p.encryptionManager and p.keyspaceID into the snapshot reader/loader invocations (the same pattern used when calling loadAndApplyDDLHistory) so that DB and table snapshot reads use encrypted readers consistent with CMEK handling.logservice/schemastore/disk_format.go (1)
834-844:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse the encryption-aware DB snapshot loader in physical-table rebuild.
loadAllPhysicalTablesAtTsreceivesencMgr/keyspaceIDbut still calls the plaintext loader (loadDatabasesInKVSnap). If snapshot DB entries are encrypted, this path will fail during unmarshal.Suggested fix
- databaseMap, err := loadDatabasesInKVSnap(storageSnap, gcTs) + databaseMap, err := loadDatabasesInKVSnapWithEncryption(storageSnap, gcTs, encMgr, keyspaceID)🤖 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 `@logservice/schemastore/disk_format.go` around lines 834 - 844, loadAllPhysicalTablesAtTs currently calls the plaintext loader loadDatabasesInKVSnap even though it receives encMgr and keyspaceID; replace that call with the encryption-aware snapshot loader used elsewhere (the variant that accepts an EncryptionManager and keyspaceID) and pass storageSnap, gcTs, encMgr, keyspaceID so encrypted DB entries are decrypted before unmarshalling; ensure the call uses the same return types and propagate/handle the error as before in loadAllPhysicalTablesAtTs.
🤖 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 `@go.mod`:
- Line 3: The root module's Go version declared as the `go` directive in go.mod
is too low (1.25.5) compared to submodules requiring 1.25.8; update the `go`
directive in the root go.mod to `1.25.8` (or a higher compatible version) so it
matches the stricter requirement used by submodules (e.g., tools/check,
tools/workload, tests/integration_tests/debezium01) to prevent GOTOOLCHAIN=local
CI failures.
In `@logservice/schemastore/disk_format.go`:
- Line 197: The DecryptData calls use context.Background(), which ignores caller
cancellations/timeouts; update those calls (encMgr.DecryptData(...)) to accept
and use a context propagated from the calling stack (e.g., replace
context.Background() with the function's ctx parameter) or, if the caller has no
context, create a derived context with a sensible timeout (context.WithTimeout)
that is canceled after use. Thread the ctx through the surrounding functions in
disk_format.go so all calls to encMgr.DecryptData (and any symmetric
EncryptData/EncryptData calls) use the propagated ctx (or a short derived ctx)
instead of context.Background() to ensure encryption I/O respects cancellation
and deadlines.
---
Outside diff comments:
In `@logservice/schemastore/disk_format.go`:
- Around line 834-844: loadAllPhysicalTablesAtTs currently calls the plaintext
loader loadDatabasesInKVSnap even though it receives encMgr and keyspaceID;
replace that call with the encryption-aware snapshot loader used elsewhere (the
variant that accepts an EncryptionManager and keyspaceID) and pass storageSnap,
gcTs, encMgr, keyspaceID so encrypted DB entries are decrypted before
unmarshalling; ensure the call uses the same return types and propagate/handle
the error as before in loadAllPhysicalTablesAtTs.
In `@logservice/schemastore/persist_storage.go`:
- Around line 270-278: initializeFromDisk currently restores DB/table snapshots
with plaintext readers while only loadAndApplyDDLHistory is passed
p.encryptionManager and p.keyspaceID; update the snapshot restore path to use
the same encryption wiring. Specifically, modify initializeFromDisk (the code
paths that load database and table snapshots) to accept and pass
p.encryptionManager and p.keyspaceID into the snapshot reader/loader invocations
(the same pattern used when calling loadAndApplyDDLHistory) so that DB and table
snapshot reads use encrypted readers consistent with CMEK handling.
🪄 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: ad8e19d8-57d8-4caa-a009-03256ce1200a
📒 Files selected for processing (5)
go.modlogservice/schemastore/disk_format.gologservice/schemastore/persist_storage.gologservice/schemastore/schema_store.gologservice/schemastore/schema_store_test.go
|
|
||
| // Decrypt if encryption is enabled | ||
| if encMgr != nil { | ||
| decryptedValue, err := encMgr.DecryptData(context.Background(), keyspaceID, value) |
There was a problem hiding this comment.
Avoid context.Background() for encryption operations in load/write paths.
Encryption calls currently ignore caller cancellation and deadlines. A slow/stalled encryption backend can block startup/read/update flows without bounded timeout.
Also applies to: 371-371, 526-526, 576-576, 610-610, 659-659
🤖 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 `@logservice/schemastore/disk_format.go` at line 197, The DecryptData calls use
context.Background(), which ignores caller cancellations/timeouts; update those
calls (encMgr.DecryptData(...)) to accept and use a context propagated from the
calling stack (e.g., replace context.Background() with the function's ctx
parameter) or, if the caller has no context, create a derived context with a
sensible timeout (context.WithTimeout) that is canceled after use. Thread the
ctx through the surrounding functions in disk_format.go so all calls to
encMgr.DecryptData (and any symmetric EncryptData/EncryptData calls) use the
propagated ctx (or a short derived ctx) instead of context.Background() to
ensure encryption I/O respects cancellation and deadlines.
Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
|
/test legacy-safepoint |
Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
Append an 8-byte mask to persisted schema and DDL keys, using bit 0 to record whether the value passed through the encryption layer. Read the mask instead of inferring encryption from manager availability, while retaining compatibility with legacy keys. Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
ti-chi-bot
Bot
removed
the
size/XL
ti-chi-bot
Bot
added
the
size/XXL
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
logservice/schemastore/disk_format.go (1)
336-337: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
table_info_entryuses snake_case, not lowerCamelCase.Appears in the newly-introduced
loadTablesInKVSnapWithEncryption,loadFullTablesInKVSnapWithEncryption, andreadTableInfoInKVSnapWithEncryption. Rename totableInfoEntryfor consistency.As per coding guidelines, "Variables should use lowerCamelCase naming (e.g.,
flushInterval, notflush_interval)."Also applies to: 411-412, 564-574
🤖 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 `@logservice/schemastore/disk_format.go` around lines 336 - 337, Rename the local variable table_info_entry to tableInfoEntry in loadTablesInKVSnapWithEncryption, loadFullTablesInKVSnapWithEncryption, and readTableInfoInKVSnapWithEncryption, updating every reference consistently while preserving behavior.Source: Coding guidelines
🤖 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 `@logservice/schemastore/persist_storage.go`:
- Around line 263-266: Update the fatal error handling around
loadDatabasesInKVSnapWithEncryption to pass the captured err to log.Fatal via
zap.Error(err), preserving the existing message while including the underlying
encrypted-loading failure details.
---
Nitpick comments:
In `@logservice/schemastore/disk_format.go`:
- Around line 336-337: Rename the local variable table_info_entry to
tableInfoEntry in loadTablesInKVSnapWithEncryption,
loadFullTablesInKVSnapWithEncryption, and readTableInfoInKVSnapWithEncryption,
updating every reference consistently while preserving behavior.
🪄 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: a29fc24b-9c84-4f78-a5fe-76aa3331a176
📒 Files selected for processing (5)
logservice/schemastore/disk_format.gologservice/schemastore/disk_format_test.gologservice/schemastore/persist_storage.gologservice/schemastore/schema_store.gologservice/schemastore/schema_store_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- logservice/schemastore/schema_store.go
| if p.databaseMap, err = loadDatabasesInKVSnapWithEncryption( | ||
| storageSnap, p.gcTs, p.encryptionManager, p.keyspaceID); err != nil { | ||
| log.Fatal("load database info from disk failed") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Include zap.Error(err) in log.Fatal for diagnosability.
Line 265 calls log.Fatal("load database info from disk failed") without the error, unlike line 240 which does include zap.Error(err). With encrypted loading, decryption failures are a new error mode — losing the error details makes root-cause debugging much harder on a fatal crash.
🔧 Proposed fix
log.Fatal("load database info from disk failed")
+ log.Fatal("load database info from disk failed", zap.Error(err))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if p.databaseMap, err = loadDatabasesInKVSnapWithEncryption( | |
| storageSnap, p.gcTs, p.encryptionManager, p.keyspaceID); err != nil { | |
| log.Fatal("load database info from disk failed") | |
| } | |
| if p.databaseMap, err = loadDatabasesInKVSnapWithEncryption( | |
| storageSnap, p.gcTs, p.encryptionManager, p.keyspaceID); err != nil { | |
| log.Fatal("load database info from disk failed", zap.Error(err)) | |
| } |
🤖 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 `@logservice/schemastore/persist_storage.go` around lines 263 - 266, Update the
fatal error handling around loadDatabasesInKVSnapWithEncryption to pass the
captured err to log.Fatal via zap.Error(err), preserving the existing message
while including the underlying encrypted-loading failure details.
Signed-off-by: tenfyzhong <tenfy@tenfy.cn>
|
@tenfyzhong: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
What problem does this PR solve?
Issue Number: ref #3943
This is split PR 7/8 from #3955.
What is changed and how it works?
This PR adds CMEK support to SchemaStore persistence and DDL loading:
Check List
Tests
Questions
Will it cause performance regression or break compatibility?
When CMEK is enabled, decryption cost is expected on load/read paths. Compatibility remains for non-encrypted data.
Do you need to update user documentation, design documentation or monitoring documentation?
No.
Release note
Summary by CodeRabbit
New Features
Bug Fixes
Tests