<feature>[keyprovider]: Add key provider canonical events#4214
Open
ZStack-Robot wants to merge 1 commit into
Open
<feature>[keyprovider]: Add key provider canonical events#4214ZStack-Robot wants to merge 1 commit into
ZStack-Robot wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 3 hours, 14 minutes, and 40 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough该 PR 引入关键提供方连接失败的规范事件基础结构，并在 LUKS 密钥物化和 DEK 处理的多个代码路径中集成事件触发，以报告密钥提供方不可用或连接异常情况。 ChangesKey Provider Connection Failure Event Reporting
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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 (1)
storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.java (1)
271-293:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
materializeDek调用缺少异常事件上报此方法中第 284 行调用
materializeDek(volumeUuid, kpUuid)未包裹在 try-catch 中，因此当materializeDek失败时不会触发HOST_KEY_PROVIDER_CONNECTION_ABNORMAL事件。这与
prepareLuksSecretMaterialFileOnHost（第 147-160 行）的处理方式不一致，后者对materializeDek进行了异常捕获和事件上报。🔧 建议修复
public String prepareLuksEnvelopeDekOnHost(String hostUuid, String volumeUuid) { if (StringUtils.isBlank(hostUuid) || StringUtils.isBlank(volumeUuid)) { throw new OperationFailureException(operr( "prepare LUKS envelope DEK requires non-blank hostUuid and volumeUuid")); } String kpUuid = volumeEncryptedResourceKeyBackend.findKeyProviderUuidByVolume(volumeUuid); if (StringUtils.isBlank(kpUuid)) { throw new OperationFailureException(operr( "volume[uuid:%s] requires LUKS secret material but has no key provider binding", volumeUuid)); } - EncryptedResourceKeyManager.ResourceKeyResult keyResult = materializeDek(volumeUuid, kpUuid); - String dekBase64 = keyResult.getDekBase64(); - if (StringUtils.isBlank(dekBase64)) { - throw new OperationFailureException(operr( - "encrypted volume[uuid:%s]: key manager returned empty DEK for LUKS envelope", - volumeUuid)); + String dekBase64; + try { + EncryptedResourceKeyManager.ResourceKeyResult keyResult = materializeDek(volumeUuid, kpUuid); + dekBase64 = keyResult.getDekBase64(); + if (StringUtils.isBlank(dekBase64)) { + throw new OperationFailureException(operr( + "encrypted volume[uuid:%s]: key manager returned empty DEK for LUKS envelope", + volumeUuid)); + } + } catch (OperationFailureException e) { + fireHostKeyProviderConnectionAbnormal(hostUuid, kpUuid, failureReason(e)); + throw e; } - return prepareLuksEnvelopeDekOnHost(hostUuid, volumeUuid, dekBase64); + return prepareLuksEnvelopeDekOnHost(hostUuid, volumeUuid, kpUuid, dekBase64); }🤖 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 `@storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.java` around lines 271 - 293, The call to materializeDek(volumeUuid, kpUuid) inside prepareLuksEnvelopeDekOnHost(String hostUuid, String volumeUuid) must be wrapped in a try-catch that mirrors prepareLuksSecretMaterialFileOnHost: catch the exception from materializeDek, publish/emit the HOST_KEY_PROVIDER_CONNECTION_ABNORMAL event (including hostUuid and kpUuid/volumeUuid context) so the failure is reported, and then rethrow or convert the exception to the existing OperationFailureException path; ensure the catch targets the same exception types and uses the same event payload/utility used by prepareLuksSecretMaterialFileOnHost to keep behavior consistent.
🧹 Nitpick comments (2)
header/src/main/java/org/zstack/header/keyprovider/KeyProviderCanonicalEvents.java (2)
12-67: 💤 Low value建议抽取公共基类以消除重复代码
KeyProviderServiceUnavailableData与KeyProviderServiceRecoveredData的字段完全相同（keyProviderUuid、keyProviderName、keyProviderType、statusCode、statusMessage、defaultProvider），可以抽取一个公共基类（如KeyProviderServiceStatusData）来减少重复。Also applies to: 70-125
🤖 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 `@header/src/main/java/org/zstack/header/keyprovider/KeyProviderCanonicalEvents.java` around lines 12 - 67, Extract a new base class KeyProviderServiceStatusData containing the shared fields (keyProviderUuid, keyProviderName, keyProviderType, statusCode, statusMessage, defaultProvider) with their getters/setters, then make KeyProviderServiceUnavailableData and KeyProviderServiceRecoveredData extend KeyProviderServiceStatusData and remove the duplicated fields and accessor methods from those subclasses; ensure the new base class and subclasses keep the original visibility and any annotations so serialization/usage (e.g., in KeyProviderServiceUnavailableData and KeyProviderServiceRecoveredData) continues to work.
128-135: 💤 Low value字段类型语义建议
daysLeft字段使用String类型，但语义上应为数值类型（如Integer或Long），便于事件消费端进行数值比较和计算。同样，defaultProvider字段语义上更适合使用Boolean类型。🤖 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 `@header/src/main/java/org/zstack/header/keyprovider/KeyProviderCanonicalEvents.java` around lines 128 - 135, The fields in KeyProviderCertificateExpiringData have wrong types: change daysLeft from String to an appropriate numeric type (e.g., Integer or Long) and change defaultProvider from String to Boolean; update the class KeyProviderCertificateExpiringData by renaming types for fields daysLeft and defaultProvider, adjust their getters/setters and any constructors/builders that reference them, and update JSON (de)serialization or mapping annotations and any callers/consumers to handle numeric/boolean values instead of strings to preserve compatibility.
🤖 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
`@storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.java`:
- Around line 177-179: The method in VolumeEncryptedSecretHelper mistakenly
redeclares a local variable String dekBase64 which shadows the method parameter
and causes the passed-in dekBase64 to be ignored; remove the local declaration
and change the logic so it uses the method parameter when non-null/non-empty and
only calls materializeDek() when the parameter is null/empty (or otherwise
absent), updating the code paths that reference dekBase64 accordingly (look for
the method containing the materializeDek() call and the local "String
dekBase64;" declaration).
- Around line 163-193: The method prepareLuksEnvelopeDekOnHost(hostUuid,
resourceUuid, dekBase64) incorrectly references volumeUuid, redeclares
dekBase64, and calls materializeDek() (ignoring the passed dek); fix it to
validate non-blank hostUuid/resourceUuid/dekBase64, use resourceUuid when
calling
volumeEncryptedResourceKeyBackend.findKeyProviderUuidByVolume(resourceUuid),
throw an OperationFailureException mentioning resourceUuid if kpUuid is blank,
do NOT redeclare or overwrite dekBase64 or call
materializeDek()/fireHostKeyProviderConnectionAbnormal, and simply return
prepareLuksEnvelopeDekOnHost(hostUuid, resourceUuid, kpUuid, dekBase64).
---
Outside diff comments:
In
`@storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.java`:
- Around line 271-293: The call to materializeDek(volumeUuid, kpUuid) inside
prepareLuksEnvelopeDekOnHost(String hostUuid, String volumeUuid) must be wrapped
in a try-catch that mirrors prepareLuksSecretMaterialFileOnHost: catch the
exception from materializeDek, publish/emit the
HOST_KEY_PROVIDER_CONNECTION_ABNORMAL event (including hostUuid and
kpUuid/volumeUuid context) so the failure is reported, and then rethrow or
convert the exception to the existing OperationFailureException path; ensure the
catch targets the same exception types and uses the same event payload/utility
used by prepareLuksSecretMaterialFileOnHost to keep behavior consistent.
---
Nitpick comments:
In
`@header/src/main/java/org/zstack/header/keyprovider/KeyProviderCanonicalEvents.java`:
- Around line 12-67: Extract a new base class KeyProviderServiceStatusData
containing the shared fields (keyProviderUuid, keyProviderName, keyProviderType,
statusCode, statusMessage, defaultProvider) with their getters/setters, then
make KeyProviderServiceUnavailableData and KeyProviderServiceRecoveredData
extend KeyProviderServiceStatusData and remove the duplicated fields and
accessor methods from those subclasses; ensure the new base class and subclasses
keep the original visibility and any annotations so serialization/usage (e.g.,
in KeyProviderServiceUnavailableData and KeyProviderServiceRecoveredData)
continues to work.
- Around line 128-135: The fields in KeyProviderCertificateExpiringData have
wrong types: change daysLeft from String to an appropriate numeric type (e.g.,
Integer or Long) and change defaultProvider from String to Boolean; update the
class KeyProviderCertificateExpiringData by renaming types for fields daysLeft
and defaultProvider, adjust their getters/setters and any constructors/builders
that reference them, and update JSON (de)serialization or mapping annotations
and any callers/consumers to handle numeric/boolean values instead of strings to
preserve compatibility.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 20b2e24f-4d2b-434a-9d1a-5e5103f1880a
📒 Files selected for processing (2)
header/src/main/java/org/zstack/header/keyprovider/KeyProviderCanonicalEvents.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.java
Comment on lines
163
to
193
| public String prepareLuksEnvelopeDekOnHost(String hostUuid, String resourceUuid, String dekBase64) { | ||
| if (StringUtils.isBlank(hostUuid) || StringUtils.isBlank(resourceUuid) || | ||
| StringUtils.isBlank(dekBase64)) { | ||
| throw new OperationFailureException(operr( | ||
| "encrypted volume[uuid:%s]: key manager returned empty DEK for LUKS secret material file", | ||
|
|
||
| "prepare LUKS envelope DEK requires non-blank hostUuid and volumeUuid")); | ||
| } | ||
|
|
||
| String kpUuid = volumeEncryptedResourceKeyBackend.findKeyProviderUuidByVolume(volumeUuid); | ||
| if (StringUtils.isBlank(kpUuid)) { | ||
| throw new OperationFailureException(operr( | ||
| "volume[uuid:%s] requires LUKS secret material but has no key provider binding", | ||
| volumeUuid)); | ||
| } | ||
|
|
||
| return ensureLuksSecretFileOnHost(hostUuid, volumeUuid, dekBase64); | ||
| String dekBase64; | ||
| try { | ||
| EncryptedResourceKeyManager.ResourceKeyResult keyResult = materializeDek(volumeUuid, kpUuid); | ||
| dekBase64 = keyResult.getDekBase64(); | ||
| if (StringUtils.isBlank(dekBase64)) { | ||
| throw new OperationFailureException(operr( | ||
| "encrypted volume[uuid:%s]: key manager returned empty DEK for LUKS envelope", | ||
| volumeUuid)); | ||
| } | ||
| } catch (OperationFailureException e) { | ||
| fireHostKeyProviderConnectionAbnormal(hostUuid, kpUuid, failureReason(e)); | ||
| throw e; | ||
| } | ||
|
|
||
| return prepareLuksEnvelopeDekOnHost(hostUuid, volumeUuid, kpUuid, dekBase64); | ||
| } |
There was a problem hiding this comment.
方法 prepareLuksEnvelopeDekOnHost(hostUuid, resourceUuid, dekBase64) 实现与签名不匹配
该方法存在根本性问题：方法签名表明它接收 dekBase64 参数并直接使用，但方法体却：
- 引用了未定义的变量
volumeUuid（应使用参数resourceUuid） - 重新声明了
String dekBase64局部变量，遮蔽了同名参数 - 调用
materializeDek()重新获取 DEK，完全忽略了传入的dekBase64参数
这表明该方法是从其他重载方法复制而来但未正确修改。需要明确该方法的设计意图后重新实现。
🤖 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
`@storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.java`
around lines 163 - 193, The method prepareLuksEnvelopeDekOnHost(hostUuid,
resourceUuid, dekBase64) incorrectly references volumeUuid, redeclares
dekBase64, and calls materializeDek() (ignoring the passed dek); fix it to
validate non-blank hostUuid/resourceUuid/dekBase64, use resourceUuid when
calling
volumeEncryptedResourceKeyBackend.findKeyProviderUuidByVolume(resourceUuid),
throw an OperationFailureException mentioning resourceUuid if kpUuid is blank,
do NOT redeclare or overwrite dekBase64 or call
materializeDek()/fireHostKeyProviderConnectionAbnormal, and simply return
prepareLuksEnvelopeDekOnHost(hostUuid, resourceUuid, kpUuid, dekBase64).
Comment on lines
+177
to
+179
|
|
||
| return ensureLuksSecretFileOnHost(hostUuid, volumeUuid, dekBase64); | ||
| String dekBase64; | ||
| try { |
There was a problem hiding this comment.
严重：参数 dekBase64 被局部变量遮蔽
第 163 行方法参数已声明 dekBase64，但第 178 行又重新声明了同名局部变量 String dekBase64;，遮蔽了方法参数。这导致传入的 dekBase64 参数完全被忽略，而是重新调用 materializeDek() 获取新的 DEK。
这表明该方法的实现逻辑与方法签名的设计意图不符。如果方法已接收 dekBase64 参数，则不应再次调用 materializeDek()。
🤖 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
`@storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.java`
around lines 177 - 179, The method in VolumeEncryptedSecretHelper mistakenly
redeclares a local variable String dekBase64 which shadows the method parameter
and causes the passed-in dekBase64 to be ignored; remove the local declaration
and change the logic so it uses the method parameter when non-null/non-empty and
only calls materializeDek() when the parameter is null/empty (or otherwise
absent), updating the code paths that reference dekBase64 accordingly (look for
the method containing the materializeDek() call and the local "String
dekBase64;" declaration).
MatheMatrix
force-pushed
the
sync/zstackio/codex/keyprovider-events-alarms@@2
branch
from
400d23a to
69f1a50
Compare
Collaborator
Author
|
Comment on Comment from 刘志建: 这个文件都已经回退了，不存在该问题 |
Collaborator
|
Comment from yaohua.wu: Review: MR !10128 — ZSV-237Background (preserved across rounds)
P1 — High
P3 — Low
Coverage
Verdict: REVISION_REQUIRED本 MR 代码无缺陷，但存在两项需作者确认的完整性问题：(1) 描述承诺的事件发射逻辑缺失（仅剩定义，且当前无调用方），需确认是否后续 MR 落地并相应修正描述；(2) Jira Resolves 引用串号，需修正。两项均不阻塞编译，但建议合入前澄清。 🤖 Robot Reviewer |
MatheMatrix
force-pushed
the
sync/zstackio/codex/keyprovider-events-alarms@@2
branch
from
69f1a50 to
30b7917
Compare
Add key provider canonical events and emit host/key-provider abnormal events when encrypted volume secret preparation, envelope generation, or libvirt secret operations fail. Resolves: ZSV-237 Change-Id: I77716165756971736379216977626b6f6f65656b
MatheMatrix
force-pushed
the
sync/zstackio/codex/keyprovider-events-alarms@@2
branch
from
30b7917 to
8915cd4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add key provider canonical events and emit host/key-provider abnormal
events when encrypted volume secret preparation, envelope generation, or
libvirt secret operations fail.
Resolves: ZSV-237
Change-Id: I77716165756971736379216977626b6f6f65656b
sync from gitlab !10128