<feature>[storage]: support encrypted scsi lun#4309
Open
ZStack-Robot wants to merge 1 commit into
Open
Conversation
Expose encrypted SCSI LUN schema and SDK fields. Test: covered by premium EncryptScsiLunVmLifecycleCase. Change-Id: Ib25cca2a8805d8f96d7edd9f602e4a3218d493bb
Walkthrough为 SCSI LUN 新增 LUKS 加密支持：数据库 schema 在 ChangesSCSI LUN LUKS 加密支持
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 docstrings
🧪 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
🤖 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 `@conf/db/zsv/V5.1.0__schema.sql`:
- Around line 8-9: The migration is adding NOT NULL columns (encrypted) directly
to LunVO and ScsiLunVmInstanceRefVO tables without explicitly processing
historical data first. According to repository guidelines, you must first add
the columns as nullable, then use a stored procedure or explicit UPDATE
statement to process and populate historical data for existing rows, and finally
alter the columns to be NOT NULL. Break the current migration into three
distinct steps: (1) add the encrypted column as nullable with a default value to
LunVO and ScsiLunVmInstanceRefVO, (2) create and execute a stored procedure or
UPDATE statement to handle historical data backfill, and (3) alter both columns
to NOT NULL after the data is processed.
In
`@storage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.java`:
- Around line 351-370: The method defineSecretFromBindingForResource
materializes the DEK by calling materializeResourceDek before validating all
required parameters, creating side effects in the key system if parameters like
secretPurpose, usageInstance, or resourceType are invalid. Add parameter
validation checks for secretPurpose, usageInstance, and resourceType (using
StringUtils.isBlank or appropriate validation) before the materializeResourceDek
call to ensure parameters are valid and prevent wasted materialization with
subsequent failures.
🪄 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: e18c7682-ad7a-4e66-9da3-ced365d23630
📒 Files selected for processing (5)
conf/db/zsv/V5.1.0__schema.sqlsdk/src/main/java/org/zstack/sdk/AttachScsiLunToVmInstanceAction.javasdk/src/main/java/org/zstack/sdk/LunInventory.javasdk/src/main/java/org/zstack/sdk/ScsiLunVmInstanceRefInventory.javastorage/src/main/java/org/zstack/storage/encrypt/VolumeEncryptedSecretHelper.java
Comment on lines
+8
to
+9
| ALTER TABLE `zstack`.`LunVO` ADD COLUMN `encrypted` tinyint(1) NOT NULL DEFAULT 0; | ||
| ALTER TABLE `zstack`.`ScsiLunVmInstanceRefVO` ADD COLUMN `encrypted` tinyint(1) NOT NULL DEFAULT 0; |
There was a problem hiding this comment.
NOT NULL 列缺少显式历史数据处理步骤（升级风险）
Line 8 和 Line 9 直接新增 NOT NULL 列，但当前迁移未体现“先处理历史数据再收紧约束”的步骤。按仓库 SQL 规范，这里需要通过存储过程或函数（或等价回填步骤）显式处理历史数据，再改为 NOT NULL。
🔧 建议迁移方式（示例）
-ALTER TABLE `zstack`.`LunVO` ADD COLUMN `encrypted` tinyint(1) NOT NULL DEFAULT 0;
-ALTER TABLE `zstack`.`ScsiLunVmInstanceRefVO` ADD COLUMN `encrypted` tinyint(1) NOT NULL DEFAULT 0;
+ALTER TABLE `zstack`.`LunVO` ADD COLUMN `encrypted` tinyint(1) DEFAULT NULL;
+ALTER TABLE `zstack`.`ScsiLunVmInstanceRefVO` ADD COLUMN `encrypted` tinyint(1) DEFAULT NULL;
+
+-- 使用存储过程/函数（或等价批处理）回填历史数据
+UPDATE `zstack`.`LunVO` SET `encrypted` = 0 WHERE `encrypted` IS NULL;
+UPDATE `zstack`.`ScsiLunVmInstanceRefVO` SET `encrypted` = 0 WHERE `encrypted` IS NULL;
+
+ALTER TABLE `zstack`.`LunVO` MODIFY COLUMN `encrypted` tinyint(1) NOT NULL DEFAULT 0;
+ALTER TABLE `zstack`.`ScsiLunVmInstanceRefVO` MODIFY COLUMN `encrypted` tinyint(1) NOT NULL DEFAULT 0;As per coding guidelines: "When NOT NULL exists, must use stored procedure or other functions to process historical data, this is very very important".
🤖 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 `@conf/db/zsv/V5.1.0__schema.sql` around lines 8 - 9, The migration is adding
NOT NULL columns (encrypted) directly to LunVO and ScsiLunVmInstanceRefVO tables
without explicitly processing historical data first. According to repository
guidelines, you must first add the columns as nullable, then use a stored
procedure or explicit UPDATE statement to process and populate historical data
for existing rows, and finally alter the columns to be NOT NULL. Break the
current migration into three distinct steps: (1) add the encrypted column as
nullable with a default value to LunVO and ScsiLunVmInstanceRefVO, (2) create
and execute a stored procedure or UPDATE statement to handle historical data
backfill, and (3) alter both columns to NOT NULL after the data is processed.
Source: Coding guidelines
Comment on lines
+351
to
370
| public String defineSecretFromBindingForResource(String hostUuid, String vmUuid, String resourceUuid, | ||
| String resourceType, String kpUuid, String secretPurpose, | ||
| String usageInstance, String secretUuid) { | ||
| if (StringUtils.isBlank(kpUuid)) { | ||
| throw new OperationFailureException(operr( | ||
| "encrypted volume[uuid:%s] has no key provider binding; cannot define libvirt secret on host[uuid:%s]", | ||
| volUuid, hostUuid)); | ||
| "encrypted resource[type:%s, uuid:%s] has no key provider binding; cannot define libvirt secret on host[uuid:%s]", | ||
| resourceType, resourceUuid, hostUuid)); | ||
| } | ||
| EncryptedResourceKeyManager.ResourceKeyResult keyResult = materializeDek(volUuid, kpUuid); | ||
| EncryptedResourceKeyManager.ResourceKeyResult keyResult = materializeResourceDek(resourceUuid, resourceType, | ||
| kpUuid, "define-" + secretPurpose + "-secret"); | ||
| String dekBase64 = keyResult.getDekBase64(); | ||
| if (StringUtils.isBlank(dekBase64)) { | ||
| throw new OperationFailureException(operr( | ||
| "encrypted volume[uuid:%s]: key manager returned empty DEK for libvirt secret", | ||
| volUuid)); | ||
| "encrypted resource[type:%s, uuid:%s]: key manager returned empty DEK for libvirt secret", | ||
| resourceType, resourceUuid)); | ||
| } | ||
| return defineLibvirtSecretOnHost(hostUuid, vmUuid, volUuid, dekBase64, keyResult.getKeyVersion(), secretUuid); | ||
| String description = String.format("LUKS DEK for %s %s", resourceType, resourceUuid); | ||
| return defineLibvirtSecretForResourceOnHost(hostUuid, vmUuid, resourceUuid, dekBase64, | ||
| keyResult.getKeyVersion(), secretUuid, secretPurpose, usageInstance, description); | ||
| } |
There was a problem hiding this comment.
在参数合法性未确认前就物化 DEK，存在副作用顺序问题
Line 359 在校验 secretPurpose/usageInstance/resourceType 之前调用了 materializeResourceDek。如果这些参数无效，下游（Line 368-369）才失败，会留下“已物化但不可用”的密钥副作用，增加密钥系统噪音并放大排障复杂度。
🔧 建议修复
public String defineSecretFromBindingForResource(String hostUuid, String vmUuid, String resourceUuid,
String resourceType, String kpUuid, String secretPurpose,
String usageInstance, String secretUuid) {
+ if (StringUtils.isBlank(hostUuid) || StringUtils.isBlank(resourceUuid)
+ || StringUtils.isBlank(resourceType)
+ || StringUtils.isBlank(secretPurpose)
+ || StringUtils.isBlank(usageInstance)) {
+ throw new OperationFailureException(operr(
+ "defineSecretFromBindingForResource requires non-blank hostUuid, resourceUuid, resourceType, secretPurpose and usageInstance"));
+ }
if (StringUtils.isBlank(kpUuid)) {
throw new OperationFailureException(operr(
"encrypted resource[type:%s, uuid:%s] has no key provider binding; cannot define libvirt secret on host[uuid:%s]",
resourceType, resourceUuid, hostUuid));
}🤖 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 351 - 370, The method defineSecretFromBindingForResource
materializes the DEK by calling materializeResourceDek before validating all
required parameters, creating side effects in the key system if parameters like
secretPurpose, usageInstance, or resourceType are invalid. Add parameter
validation checks for secretPurpose, usageInstance, and resourceType (using
StringUtils.isBlank or appropriate validation) before the materializeResourceDek
call to ensure parameters are valid and prevent wasted materialization with
subsequent failures.
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
1 participant
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.
Summary: add shared encrypted resource secret helpers and SCSI LUN encrypted schema/SDK fields. Testing: covered by premium EncryptScsiLunVmLifecycleCase.
sync from gitlab !10268