5.5.22 unified-hardware QA bug batch: 85182/85184/85190/85350#4034
Open
MatheMatrix wants to merge 9 commits into
Open
5.5.22 unified-hardware QA bug batch: 85182/85184/85190/85350#4034MatheMatrix wants to merge 9 commits into
MatheMatrix wants to merge 9 commits into
Conversation
Scanner/PowerTracker static overrides return ShellResult instead of pre-classified Status, so prod PowerStatusParser and isAuthFailure classifier run on the mocked stdout/stderr. Drop TestGatewayPxeProvisionProvider + spring XML + the two PhysicalServerOpsCase provision cases. OSS contract assertions jobResult.contains(server/network uuid) moved to premium happy path; STANDALONE_PXE reserved-stub case removed. Resolves: ZSTAC-84191 Change-Id: I6fcf4a29c55553e20ff5517540cbf563e753b543
PRD §2.5.1 requires PhysicalServerVO.serialNumber / manufacturer / model to be backfilled from Connect-stage signals. Confluence pageId=208903964 QA#2 captured the gap: path-2 AddKVMHost leaves these three columns null. InitPhysicalServerCapacityFlow runs at chain head (positions 2-5/10) per ADR-012 fail-loud ordering, before saveGeneralHostHardwareFacts writes SYSTEM_SERIAL_NUMBER / SYSTEM_MANUFACTURER / SYSTEM_PRODUCT_NAME at position 7+. Reading SystemTag there returns null. Hook the chain tail call-after-add-host-extension instead (HostAddExtensionPoint.afterAddHost) — fires once tags are written. Null-only update preserves any user-supplied value from path-1 CreatePhysicalServer. KVM only. BM2 / Container chassis have no top-level identity columns; their backfill ships once the FRU / nodeInfo discovery adapters land. PhysicalServerKvmBackfillCase covers tier-3 auto-create, pre-resolved serverUuid, and idempotent preservation of user-supplied manufacturer. Change-Id: Iba0069ef7fcbd5b747750796e1bdedb7960ac6c9
@ExceptionSafe handles uncaught throwables uniformly via AspectJ; local try-catch in afterAddHost only added noise and would have silently swallowed real bugs. Plain logic, completion.success() at the end. PRD §2.5.1 backfill behavior unchanged. Change-Id: I371208a31119e6781c8125b489746bcd72479557
Walkthrough此PR通过数据库schema更新、加密框架依赖隔离、硬件字段精简、API验证增强和测试基础设施改进，重构了物理服务器硬件信息模型。新增KVM主机身份回填机制和电源检测测试注入重构，完善了系统的数据完整性和测试可覆盖性。 ChangesPhysical Server Hardware Refactoring
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (3 passed)
✨ 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: 3
🤖 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 `@header/src/main/java/org/zstack/header/core/encrypt/PasswordConverter.java`:
- Around line 39-40: In PasswordConverter, do not treat encryptFacade == null as
a permissive passthrough; update both convertToEntityAttribute and
convertToDatabaseColumn so they only return the raw attribute/value when
encryptFacade.isEncryptionDisabled() is true, and if encryptFacade is null then
throw an explicit runtime exception (e.g., IllegalStateException) or otherwise
fail-fast (with a clear error message) to block writing cleartext passwords;
reference the class PasswordConverter and its methods
convertToEntityAttribute/convertToDatabaseColumn and the
encryptFacade/isEncryptionDisabled symbols when making this change.
In
`@plugin/physicalServer/src/main/java/org/zstack/server/PhysicalServerApiInterceptor.java`:
- Around line 84-95: Trim message string fields before performing
required/uniqueness checks: in PhysicalServerApiInterceptor, obtain a trimmed
value from msg.getSerialNumber() (e.g., call trim() if not null) and use that
trimmed value for the empty-check and the Q query (zoneUuid/serialNumber
uniqueness) and in the ApiMessageInterceptionException message; apply the same
trim-before-validate change to the other block referenced (the similar
validation around msg.getSerialNumber() at the later check). Ensure you never
rely on raw msg.getSerialNumber() for isEmpty() or the Q.New(...) eq(...)
comparison—always use the normalized trimmed string.
In
`@test/src/test/groovy/org/zstack/test/integration/server/PhysicalServerKvmBackfillCase.groovy`:
- Around line 20-33: Replace all Chinese text in the test file’s comments and
assertion messages with clear, correct English; specifically update the
class-level comment block describing QA gap and data flow and any
assertion/expectation messages within the test methods
testTier3AutoCreatedPsBackfilled, testPreResolvedPsBackfilled, and
testUserSuppliedFieldNotOverwritten so they are fully in English with no
spelling errors, preserving the original meaning (e.g., describe backfill
behavior, fields serialNumber/manufacturer/model, and that user-supplied
manufacturer is not overwritten).
🪄 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: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: d4203345-4101-4f7d-8010-27dd942de2b6
⛔ Files ignored due to path filters (3)
conf/springConfigXml/PhysicalServerManager.xmlis excluded by!**/*.xmlconf/springConfigXml/encrypt.xmlis excluded by!**/*.xmltest/src/test/resources/springConfigXml/PhysicalServerTestProviders.xmlis excluded by!**/*.xml
📒 Files selected for processing (28)
conf/db/upgrade/V5.5.18__schema.sqlcore/src/main/java/org/zstack/core/convert/SpecialDataConverter.javacore/src/main/java/org/zstack/core/encrypt/EncryptFacade.javacore/src/main/java/org/zstack/core/encrypt/EncryptFacadeImpl.javaheader/src/main/java/org/zstack/header/core/encrypt/EncryptFacade.javaheader/src/main/java/org/zstack/header/core/encrypt/PasswordConverter.javaheader/src/main/java/org/zstack/header/server/PhysicalServerAO.javaheader/src/main/java/org/zstack/header/server/PhysicalServerHardwareDiscoveryExtensionPoint.javaheader/src/main/java/org/zstack/header/server/PhysicalServerHardwareInfoVO.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/CephMonAO.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHostVO.javaplugin/kvm/src/main/java/org/zstack/kvm/server/KvmHardwareDiscoveryAdapter.javaplugin/physicalServer/src/main/java/org/zstack/server/PhysicalServerApiInterceptor.javaplugin/physicalServer/src/main/java/org/zstack/server/PhysicalServerKvmIdentityBackfillExtension.javaplugin/physicalServer/src/main/java/org/zstack/server/PhysicalServerManagerImpl.javaplugin/physicalServer/src/main/java/org/zstack/server/PhysicalServerPowerTracker.javaplugin/physicalServer/src/main/java/org/zstack/server/PhysicalServerScanner.javaplugin/physicalServer/src/main/java/org/zstack/server/hardware/PhysicalServerHardwareService.javaplugin/physicalServer/src/main/java/org/zstack/server/hardware/UnifiedHardwareInfo.javaplugin/physicalServer/src/test/java/org/zstack/server/hardware/UnifiedHardwareInfoMergeTest.javaplugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorageVO.javatest/src/test/groovy/org/zstack/test/integration/kvm/KvmTest.groovytest/src/test/groovy/org/zstack/test/integration/kvm/host/HostPasswordEncryptCase.groovytest/src/test/groovy/org/zstack/test/integration/server/PhysicalServerInterceptorErrorsCase.groovytest/src/test/groovy/org/zstack/test/integration/server/PhysicalServerKvmBackfillCase.groovytest/src/test/groovy/org/zstack/test/integration/server/PhysicalServerOobPasswordEncryptCase.groovytest/src/test/groovy/org/zstack/test/integration/server/PhysicalServerOpsCase.groovytest/src/test/java/org/zstack/test/TestGatewayPxeProvisionProvider.java
💤 Files with no reviewable changes (8)
- test/src/test/java/org/zstack/test/TestGatewayPxeProvisionProvider.java
- plugin/physicalServer/src/main/java/org/zstack/server/hardware/PhysicalServerHardwareService.java
- header/src/main/java/org/zstack/header/server/PhysicalServerHardwareDiscoveryExtensionPoint.java
- conf/db/upgrade/V5.5.18__schema.sql
- core/src/main/java/org/zstack/core/encrypt/EncryptFacade.java
- test/src/test/groovy/org/zstack/test/integration/kvm/KvmTest.groovy
- plugin/physicalServer/src/main/java/org/zstack/server/hardware/UnifiedHardwareInfo.java
- header/src/main/java/org/zstack/header/server/PhysicalServerHardwareInfoVO.java
Comment on lines
+39
to
40
| if (encryptFacade == null || encryptFacade.isEncryptionDisabled()) { | ||
| return attribute; |
There was a problem hiding this comment.
encryptFacade == null 不应降级为明文透传
Line 39 和 Line 50 把 encryptFacade == null 视为“直接返回原值”，会在注入异常/初始化异常时静默写入明文密码，属于 fail-open。建议仅在 isEncryptionDisabled() 时透传，encryptFacade == null 应显式失败（或至少告警并阻断写入）。
🔧 建议修复
`@Override`
public String convertToDatabaseColumn(String attribute) {
- if (encryptFacade == null || encryptFacade.isEncryptionDisabled()) {
+ if (encryptFacade == null) {
+ throw new IllegalStateException("EncryptFacade is not initialized");
+ }
+ if (encryptFacade.isEncryptionDisabled()) {
return attribute;
}
if (StringUtils.isEmpty(attribute)) {
return attribute;
}
return encryptFacade.encrypt(attribute);
}
`@Override`
public String convertToEntityAttribute(String dbData) {
- if (encryptFacade == null || encryptFacade.isEncryptionDisabled()) {
+ if (encryptFacade == null) {
+ throw new IllegalStateException("EncryptFacade is not initialized");
+ }
+ if (encryptFacade.isEncryptionDisabled()) {
return dbData;
}
if (StringUtils.isEmpty(dbData)) {
return dbData ;
}
return encryptFacade.decrypt(dbData);
}Also applies to: 50-51
🤖 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/core/encrypt/PasswordConverter.java`
around lines 39 - 40, In PasswordConverter, do not treat encryptFacade == null
as a permissive passthrough; update both convertToEntityAttribute and
convertToDatabaseColumn so they only return the raw attribute/value when
encryptFacade.isEncryptionDisabled() is true, and if encryptFacade is null then
throw an explicit runtime exception (e.g., IllegalStateException) or otherwise
fail-fast (with a clear error message) to block writing cleartext passwords;
reference the class PasswordConverter and its methods
convertToEntityAttribute/convertToDatabaseColumn and the
encryptFacade/isEncryptionDisabled symbols when making this change.
Comment on lines
+84
to
+95
| if (msg.getSerialNumber() != null && !msg.getSerialNumber().isEmpty() && msg.getZoneUuid() != null) { | ||
| boolean dup = Q.New(PhysicalServerVO.class) | ||
| .eq(PhysicalServerAO_.zoneUuid, msg.getZoneUuid()) | ||
| .eq(PhysicalServerAO_.serialNumber, msg.getSerialNumber()) | ||
| .isExists(); | ||
| if (dup) { | ||
| throw new ApiMessageInterceptionException(argerr( | ||
| "ORG_ZSTACK_PHYSICAL_SERVER_SERIAL_NUMBER_DUPLICATE", | ||
| "PhysicalServer with serialNumber[%s] already exists in Zone[uuid:%s]; serialNumber must be unique per zone", | ||
| msg.getSerialNumber(), msg.getZoneUuid())); | ||
| } | ||
| } |
There was a problem hiding this comment.
先对消息字段做 trim，再做必填/唯一性校验。
这里直接用 isEmpty()，会把仅空白字符当作有效值，导致 DHCP 必填校验可被空格绕过，serialNumber 去重也会基于未规整值执行。
🔧 建议修改
+ private static boolean isBlank(String value) {
+ return value == null || value.trim().isEmpty();
+ }
+
private void validate(APICreatePhysicalServerMsg msg) {
@@
- if (msg.getSerialNumber() != null && !msg.getSerialNumber().isEmpty() && msg.getZoneUuid() != null) {
+ String serialNumber = msg.getSerialNumber() == null ? null : msg.getSerialNumber().trim();
+ if (!isBlank(serialNumber) && msg.getZoneUuid() != null) {
boolean dup = Q.New(PhysicalServerVO.class)
.eq(PhysicalServerAO_.zoneUuid, msg.getZoneUuid())
- .eq(PhysicalServerAO_.serialNumber, msg.getSerialNumber())
+ .eq(PhysicalServerAO_.serialNumber, serialNumber)
.isExists();
if (dup) {
throw new ApiMessageInterceptionException(argerr(
"ORG_ZSTACK_PHYSICAL_SERVER_SERIAL_NUMBER_DUPLICATE",
"PhysicalServer with serialNumber[%s] already exists in Zone[uuid:%s]; serialNumber must be unique per zone",
- msg.getSerialNumber(), msg.getZoneUuid()));
+ serialNumber, msg.getZoneUuid()));
}
}
}
@@
- if ("GATEWAY_PXE".equals(msg.getType())) {
+ String type = msg.getType() == null ? null : msg.getType().trim();
+ if ("GATEWAY_PXE".equals(type)) {
java.util.List<String> missing = new java.util.ArrayList<>();
- if (msg.getDhcpInterface() == null || msg.getDhcpInterface().isEmpty()) {
+ if (isBlank(msg.getDhcpInterface())) {
missing.add("dhcpInterface");
}
- if (msg.getDhcpRangeStartIp() == null || msg.getDhcpRangeStartIp().isEmpty()) {
+ if (isBlank(msg.getDhcpRangeStartIp())) {
missing.add("dhcpRangeStartIp");
}
- if (msg.getDhcpRangeEndIp() == null || msg.getDhcpRangeEndIp().isEmpty()) {
+ if (isBlank(msg.getDhcpRangeEndIp())) {
missing.add("dhcpRangeEndIp");
}
- if (msg.getDhcpRangeNetmask() == null || msg.getDhcpRangeNetmask().isEmpty()) {
+ if (isBlank(msg.getDhcpRangeNetmask())) {
missing.add("dhcpRangeNetmask");
}As per coding guidelines **/*Interceptor.java: 注意检查来自 Message 的参数是否做过 trim，用户可能在浏览器上复制粘贴的数据带有空格、换行符等。
Also applies to: 122-135
🤖 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
`@plugin/physicalServer/src/main/java/org/zstack/server/PhysicalServerApiInterceptor.java`
around lines 84 - 95, Trim message string fields before performing
required/uniqueness checks: in PhysicalServerApiInterceptor, obtain a trimmed
value from msg.getSerialNumber() (e.g., call trim() if not null) and use that
trimmed value for the empty-check and the Q query (zoneUuid/serialNumber
uniqueness) and in the ApiMessageInterceptionException message; apply the same
trim-before-validate change to the other block referenced (the similar
validation around msg.getSerialNumber() at the later check). Ensure you never
rely on raw msg.getSerialNumber() for isEmpty() or the Q.New(...) eq(...)
comparison—always use the normalized trimmed string.
Comment on lines
+20
to
+33
| * QA gap (Confluence pageId=208903964 #2) — 真实物理机 path-2 添加后 PhysicalServerVO | ||
| * 的 serialNumber / manufacturer / model 一直为 null，违反 PRD §2.5.1 明文要求： | ||
| * "这些字段由 InitPhysicalServerCapacityFlow（或 AutoAssociateFlow）从 Connect 阶段 | ||
| * 已获取的基础信息回填 —— 不依赖 HardwareDiscoveryQueue"。 | ||
| * | ||
| * 数据通路：KVM agent (simulator override) -> HostFactResponse.systemSerialNumber/ | ||
| * SystemManufacturer/SystemProductName -> saveGeneralHostHardwareFacts() 写 HostSystemTag | ||
| * -> InitPhysicalServerCapacityFlow 读 tag null-only 回填 PS. | ||
| * | ||
| * 覆盖： | ||
| * testTier3AutoCreatedPsBackfilled — AutoAssociate 新建 PS（三字段全 null）→ 全部回填 | ||
| * testPreResolvedPsBackfilled — caller 提供 serverUuid，预建 PS（三字段全 null）→ 全部回填 | ||
| * testUserSuppliedFieldNotOverwritten — 预建 PS 手填 manufacturer → backfill 不动它 | ||
| */ |
There was a problem hiding this comment.
将中文注释和断言文案统一改为英文。
当前这几处包含中文（注释/断言消息），与仓库的语言规范冲突，建议统一替换为无拼写错误的英文。
✏️ 示例
- "QA-2 失败: PS.serialNumber expected=${FAKE_SN}, got=${ps.serialNumber}"
+ "QA-2 failed: PS.serialNumber expected=${FAKE_SN}, got=${ps.serialNumber}"As per coding guidelines **/*.*: 代码里不应当有中文，包括报错、注释等都应当使用正确的、无拼写错误的英文来写。
Also applies to: 103-107, 152-156, 196-197
🤖 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
`@test/src/test/groovy/org/zstack/test/integration/server/PhysicalServerKvmBackfillCase.groovy`
around lines 20 - 33, Replace all Chinese text in the test file’s comments and
assertion messages with clear, correct English; specifically update the
class-level comment block describing QA gap and data flow and any
assertion/expectation messages within the test methods
testTier3AutoCreatedPsBackfilled, testPreResolvedPsBackfilled, and
testUserSuppliedFieldNotOverwritten so they are fully in English with no
spelling errors, preserving the original meaning (e.g., describe backfill
behavior, fields serialNumber/manufacturer/model, and that user-supplied
manufacturer is not overwritten).
MatheMatrix
force-pushed
the
sync/jin.ma/cordon-container@@2
branch
from
a3bdf52 to
94b8d12
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@docs/brainstorms/2026-05-12-followups.md`:
- Around line 27-33: The markdown table beginning with "3 处 prod 早返：" is missing
blank lines immediately before and after it, causing MD058; fix by inserting a
blank line above the table header and a blank line after the table end (the
closing |---|---|---|---|---| row block) so the table is separated from
surrounding text; update the same section in
docs/brainstorms/2026-05-12-followups.md where the table appears to ensure
there's an empty line before the line "3 处 prod 早返：" and another empty line
after the final table row.
In `@docs/brainstorms/archives/2026-05-11-case-fix-log.md`:
- Around line 74-77: The fenced code block that starts with
"org.codehaus.groovy.runtime.powerassert.PowerAssertionError:" is missing a
language tag and triggers MD040; update that fenced block to include a language
identifier (e.g., change ``` to ```text) so the block becomes ```text ... ```;
locate the block containing "assert 4 == createConfigCmds.size()" and add the
language marker to fix the lint error.
- Around line 3-10: Remove the empty blank line inside the blockquote that
starts with "**2026-05-12 人工复核结论（最权威，新 agent 扫描请以此为准）**：" (the quoted paragraph
containing "BM2NicCase 4→5 + line 474 revert ✅ 已人工验证可信"); simply delete the
stray blank line so there are no blank lines inside the blockquote (resolving
MD028 / no-blanks-blockquote).
In
`@test/src/test/groovy/org/zstack/test/integration/server/PhysicalServerOobPasswordEncryptCase.groovy`:
- Around line 20-22: 注释中的包路径与实际代码迁移不一致（注释写了 header.core.encrypt，但类实际在
org.zstack.header.encrypt），请把注释中涉及 PasswordConverter 和 EncryptFacade 的包路径从
header.core.encrypt 修改为 org.zstack.header.encrypt，并同时检查并更新提到的 PhysicalServerAO
的描述以保持一致；确保注释中所有包名引用与代码中类型所在包（org.zstack.header.encrypt）完全匹配。
🪄 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: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: d7490b22-3412-46d8-84ac-2573604d1981
⛔ Files ignored due to path filters (1)
conf/springConfigXml/encrypt.xmlis excluded by!**/*.xml
📒 Files selected for processing (25)
conf/db/upgrade/V5.5.18__schema.sqlcore/src/main/java/org/zstack/core/convert/SpecialDataConverter.javacore/src/main/java/org/zstack/core/encrypt/EncryptFacade.javacore/src/main/java/org/zstack/core/encrypt/EncryptFacadeImpl.javadocs/brainstorms/2026-05-12-followups.mddocs/brainstorms/archives/2026-05-11-assertion-changelog.mddocs/brainstorms/archives/2026-05-11-case-fix-log.mddocs/brainstorms/next-session.mddocs/review/v5518-ut-on-bypass-audit.mddocs/todolist.mdheader/src/main/java/org/zstack/header/encrypt/EncryptFacade.javaheader/src/main/java/org/zstack/header/encrypt/PasswordConverter.javaheader/src/main/java/org/zstack/header/server/PhysicalServerAO.javaheader/src/main/java/org/zstack/header/server/PhysicalServerHardwareDiscoveryExtensionPoint.javaheader/src/main/java/org/zstack/header/server/PhysicalServerHardwareInfoVO.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/CephMonAO.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHostVO.javaplugin/physicalServer/src/main/java/org/zstack/server/PhysicalServerApiInterceptor.javaplugin/physicalServer/src/main/java/org/zstack/server/hardware/PhysicalServerHardwareService.javaplugin/physicalServer/src/main/java/org/zstack/server/hardware/UnifiedHardwareInfo.javaplugin/physicalServer/src/test/java/org/zstack/server/hardware/UnifiedHardwareInfoMergeTest.javaplugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorageVO.javatest/src/test/groovy/org/zstack/test/integration/kvm/host/HostPasswordEncryptCase.groovytest/src/test/groovy/org/zstack/test/integration/server/PhysicalServerInterceptorErrorsCase.groovytest/src/test/groovy/org/zstack/test/integration/server/PhysicalServerOobPasswordEncryptCase.groovy
💤 Files with no reviewable changes (5)
- core/src/main/java/org/zstack/core/encrypt/EncryptFacade.java
- header/src/main/java/org/zstack/header/server/PhysicalServerHardwareDiscoveryExtensionPoint.java
- conf/db/upgrade/V5.5.18__schema.sql
- plugin/physicalServer/src/main/java/org/zstack/server/hardware/PhysicalServerHardwareService.java
- header/src/main/java/org/zstack/header/server/PhysicalServerHardwareInfoVO.java
✅ Files skipped from review due to trivial changes (5)
- docs/brainstorms/next-session.md
- plugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorageVO.java
- test/src/test/groovy/org/zstack/test/integration/kvm/host/HostPasswordEncryptCase.groovy
- docs/brainstorms/archives/2026-05-11-assertion-changelog.md
- header/src/main/java/org/zstack/header/server/PhysicalServerAO.java
Comment on lines
+3
to
+10
| > **作用**：复盘 2026-05-10 / 2026-05-11 两天对本 MR 所有 case 的改动，留作 review + | ||
| > 审计资料。**重点标注哪些 case 的 assertion 数值/逻辑被改了，但没在代码里留 inline | ||
| > 注释或 Jira/MR 引用**——这是后续防止「修 case 只压症状不留痕迹」的关键线索。 | ||
| > | ||
| > 本文档**只读** worktree、**不动** tracked 文件。 | ||
|
|
||
| > **2026-05-12 人工复核结论（最权威，新 agent 扫描请以此为准）**： | ||
| > 1. **BM2NicCase 4→5 + line 474 revert ✅ 已人工验证可信**：MR prod 改动让 retryInSecs 真的多发 1 次 CREATE/PREPARE，5 是 prod 真值；line 474 由 premium `d453e70ba7 <fix>[premium]: PSC cleanup + BM2NicCase revert` 还原原 `assert 0L == ...ClusterRefVO.count()`，对应 V5.5.18 schema 删 orphan FK DROP 已 land。§2.1 / §5 BM2 nic 违规清单与 §7 Action #1-#2-#3 **已 closed，无需再追**。 |
There was a problem hiding this comment.
移除 blockquote 内空白行，避免 MD028 告警。
Line 8 在引用块中出现空白行，markdownlint 会报 no-blanks-blockquote。
建议修改
> 注释或 Jira/MR 引用**——这是后续防止「修 case 只压症状不留痕迹」的关键线索。
->
> 本文档**只读** worktree、**不动** tracked 文件。📝 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.
Suggested change
| > **作用**：复盘 2026-05-10 / 2026-05-11 两天对本 MR 所有 case 的改动，留作 review + | |
| > 审计资料。**重点标注哪些 case 的 assertion 数值/逻辑被改了，但没在代码里留 inline | |
| > 注释或 Jira/MR 引用**——这是后续防止「修 case 只压症状不留痕迹」的关键线索。 | |
| > | |
| > 本文档**只读** worktree、**不动** tracked 文件。 | |
| > **2026-05-12 人工复核结论（最权威，新 agent 扫描请以此为准）**： | |
| > 1. **BM2NicCase 4→5 + line 474 revert ✅ 已人工验证可信**：MR prod 改动让 retryInSecs 真的多发 1 次 CREATE/PREPARE，5 是 prod 真值；line 474 由 premium `d453e70ba7 <fix>[premium]: PSC cleanup + BM2NicCase revert` 还原原 `assert 0L == ...ClusterRefVO.count()`，对应 V5.5.18 schema 删 orphan FK DROP 已 land。§2.1 / §5 BM2 nic 违规清单与 §7 Action #1-#2-#3 **已 closed，无需再追**。 | |
| > **作用**：复盘 2026-05-10 / 2026-05-11 两天对本 MR 所有 case 的改动，留作 review + | |
| > 审计资料。**重点标注哪些 case 的 assertion 数值/逻辑被改了，但没在代码里留 inline | |
| > 注释或 Jira/MR 引用**——这是后续防止「修 case 只压症状不留痕迹」的关键线索。 | |
| > 本文档**只读** worktree、**不动** tracked 文件。 | |
| > **2026-05-12 人工复核结论（最权威，新 agent 扫描请以此为准）**： | |
| > 1. **BM2NicCase 4→5 + line 474 revert ✅ 已人工验证可信**：MR prod 改动让 retryInSecs 真的多发 1 次 CREATE/PREPARE，5 是 prod 真值；line 474 由 premium `d453e70ba7 <fix>[premium]: PSC cleanup + BM2NicCase revert` 还原原 `assert 0L == ...ClusterRefVO.count()`，对应 V5.5.18 schema 删 orphan FK DROP 已 land。§2.1 / §5 BM2 nic 违规清单与 §7 Action `#1-`#2-#3 **已 closed，无需再追**。 |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 8-8: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 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/brainstorms/archives/2026-05-11-case-fix-log.md` around lines 3 - 10,
Remove the empty blank line inside the blockquote that starts with "**2026-05-12
人工复核结论（最权威，新 agent 扫描请以此为准）**：" (the quoted paragraph containing "BM2NicCase 4→5
+ line 474 revert ✅ 已人工验证可信"); simply delete the stray blank line so there are
no blank lines inside the blockquote (resolving MD028 / no-blanks-blockquote).
Comment on lines
+74
to
+77
| ``` | ||
| org.codehaus.groovy.runtime.powerassert.PowerAssertionError: | ||
| assert 4 == createConfigCmds.size() | ||
| ``` |
There was a problem hiding this comment.
为代码块补充语言标识，避免 MD040。
Line 74 的 fenced code block 缺少语言类型。
建议修改
-```
+```text
org.codehaus.groovy.runtime.powerassert.PowerAssertionError:
assert 4 == createConfigCmds.size()</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 74-74: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
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/brainstorms/archives/2026-05-11-case-fix-log.md around lines 74 - 77,
The fenced code block that starts with
"org.codehaus.groovy.runtime.powerassert.PowerAssertionError:" is missing a
language tag and triggers MD040; update that fenced block to include a language
identifier (e.g., change totext) so the block becomes text ... ;
locate the block containing "assert 4 == createConfigCmds.size()" and add the
language marker to fix the lint error.
</details>
<!-- fingerprinting:phantom:triton:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+20
to
+22
| * <p>The fix relocates {@code PasswordConverter} and {@code EncryptFacade} to | ||
| * {@code header.core.encrypt} so {@link org.zstack.header.server.PhysicalServerAO} | ||
| * can wire {@code @Convert(PasswordConverter.class)} directly — same mechanism |
There was a problem hiding this comment.
注释中的包路径与实际迁移结果不一致
Line 21 写的是 header.core.encrypt，但当前迁移后的类型位于 org.zstack.header.encrypt。建议同步修正，避免后续排障按注释误定位。
🔧 建议修复
- * {`@code` header.core.encrypt} so {`@link` org.zstack.header.server.PhysicalServerAO}
+ * {`@code` header.encrypt} so {`@link` org.zstack.header.server.PhysicalServerAO}📝 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.
Suggested change
| * <p>The fix relocates {@code PasswordConverter} and {@code EncryptFacade} to | |
| * {@code header.core.encrypt} so {@link org.zstack.header.server.PhysicalServerAO} | |
| * can wire {@code @Convert(PasswordConverter.class)} directly — same mechanism | |
| * <p>The fix relocates {`@code` PasswordConverter} and {`@code` EncryptFacade} to | |
| * {`@code` header.encrypt} so {`@link` org.zstack.header.server.PhysicalServerAO} | |
| * can wire {`@code` `@Convert`(PasswordConverter.class)} directly — same mechanism |
🤖 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
`@test/src/test/groovy/org/zstack/test/integration/server/PhysicalServerOobPasswordEncryptCase.groovy`
around lines 20 - 22, 注释中的包路径与实际代码迁移不一致（注释写了 header.core.encrypt，但类实际在
org.zstack.header.encrypt），请把注释中涉及 PasswordConverter 和 EncryptFacade 的包路径从
header.core.encrypt 修改为 org.zstack.header.encrypt，并同时检查并更新提到的 PhysicalServerAO
的描述以保持一致；确保注释中所有包名引用与代码中类型所在包（org.zstack.header.encrypt）完全匹配。
Per QA review (Confluence pageId=208903964): cpuSockets, cpuCores, memoryModuleCount, diskCount, nicCount, gpuCount on PhysicalServerHardwareInfoVO have no consistent owner across the three discovery adapters and were not actually surfaced to operators. Dropping them removes a class of stale-data foot-guns. Touched layers, all in lockstep: - V5.5.18 Flyway schema (still pre-release per ADR-007). - PhysicalServerHardwareInfoVO entity fields + accessors. - HardwareInfoCarrier SPI setters (interface contract). - UnifiedHardwareInfo DTO fields + accessors. - PhysicalServerHardwareService getHardware / mergeNonNull / applyNonNull blocks. - KvmHardwareDiscoveryAdapter (HostCapacityVO write-throughs). - Bm2IpmiFruDiscoveryAdapter (offering CPU count + chassis Nic / Disk / Gpu COUNT() probes — kept totalDiskBytes sum). - ContainerNodeInfoDiscoveryAdapter (K8s capacity.cpu / 1000 cores write-through). Keeps cpuModel / cpuArchitecture / totalMemoryBytes / totalDiskBytes / healthStatus + system identity fields, which the adapters do populate reliably. HostCapacityVO.cpuSockets stays — that's the allocator side, not the discovery summary. Change-Id: I5285dbb857340ac7aa71c11b8c50674fa7365099
Fallout from ad45ca4 removing 6 count fields from UnifiedHardwareInfo / PhysicalServerHardwareInfoVO. Adapt the merge tests to use totalMemoryBytes as the numeric-zero canary and drop deleted setters/getters. 9 cases pass. Change-Id: Ib83f0f565cde561f4729fbdc8acba274a33b8a7d
PhysicalServerVO lives in header which cannot depend on core.convert.PasswordConverter, so the standard JPA AttributeConverter pattern used by KVMHostVO is unavailable. The @EncryptColumn marker on oobPassword only registers the column for integrity checking — its javadoc explicitly notes the field is not encrypted. Funnel every write site through PhysicalServerOobPasswordCrypto.encryptForStorage and every plaintext read site through decryptFromStorage so the DB column never holds plaintext: - CreatePhysicalServer / UpdatePhysicalServer / scanner. - probePowerStatus tempfile + IpmiPowerExecutor caller. - ProvisionService forward to PhysicalServerProvisionTarget and AttachPhysicalServerRole forward to CreateRoleEntityContext. IT verifies raw column != plaintext, decrypt round-trips, update path re-encrypts, and disabled-encryption mode is a pass-through (matches PasswordConverter contract). Resolves: ZSTAC-85182 Change-Id: If506f4bcfefc0957173ba5990e396f098787114d
Relocate PasswordConverter and EncryptFacade interface from core to header.core.encrypt so header-resident VOs (notably PhysicalServerAO.oobPassword for ZSTAC-85182) can apply @convert(PasswordConverter.class) directly, mirroring the KVMHostVO.password mechanism. EncryptFacade picks up isEncryptionDisabled() so the converter no longer needs to import EncryptGlobalConfig. EncryptFacadeImpl in core overrides the new method. Mechanical FQN updates across existing import sites (KVMHostVO, CephMonAO, SftpBackupStorageVO, HostPasswordEncryptCase, SpecialDataConverter, encrypt.xml bean class). Resolves: ZSTAC-85182 Change-Id: Ied6ba0daecabda732e68335a8f333e7e40ca4789
Now that PasswordConverter lives in header, annotate PhysicalServerAO.oobPassword with @convert(PasswordConverter.class). Hibernate handles encrypt-on-write and decrypt-on-read transparently — same mechanism KVMHostVO.password uses. Removes the application-layer PhysicalServerOobPasswordCrypto helper introduced as a stopgap: managers and read sites now use the plain getter/setter, simpler and on par with the rest of the platform. IT PhysicalServerOobPasswordEncryptCase now mirrors HostPasswordEncryptCase: asserts the @convert field is registered in EncryptEntityMetadataVO, that the toggle flips to Encrypted, and that the converter is a pass- through when encryption is globally disabled. Resolves: ZSTAC-85182 Change-Id: I083434f27cacc9ab4b03a3b1f2ae13ae9b5aa0b5
PhysicalServerApiInterceptor now surfaces named errors instead of bubbling SYS.1000 constraint violations or SYS.1006 with just a uuid as details. - ZSTAC-85184 CreatePhysicalServer: pre-check zone+serialNumber uniqueness; explicit message names the offending serialNumber and Zone. - ZSTAC-85190 AttachPhysicalServerRole: pre-check (serverUuid, roleType); explicit message names the conflicting role and suggests detach-first. - ZSTAC-85350 CreateProvisionNetwork GATEWAY_PXE: reject when DHCP wiring (dhcpInterface + dhcpRangeStartIp/EndIp/Netmask) is missing; lists the missing fields in one consolidated error. All three errors are emitted via 3-arg argerr so globalErrorCode + formatted details land in the response payload. IT PhysicalServerInterceptorErrorsCase exercises all three error paths and asserts the offending field appears in the message. Resolves: ZSTAC-85184 Change-Id: Ia5779b95fad27c3e2c826014d0116557089eff5a
MatheMatrix
force-pushed
the
sync/jin.ma/cordon-container@@2
branch
from
94b8d12 to
e424b7e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
test/src/test/groovy/org/zstack/test/integration/server/PhysicalServerOobPasswordEncryptCase.groovy (1)
20-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win注释中的包路径与实际迁移结果不一致
这里写成了
header.core.encrypt，但当前PasswordConverter与EncryptFacade位于org.zstack.header.convert。建议同步修正注释，避免误导定位。建议修复
- * <p>The fix relocates {`@code` PasswordConverter} and {`@code` EncryptFacade} to - * {`@code` header.core.encrypt} so {`@link` org.zstack.header.server.PhysicalServerAO} + * <p>The fix relocates {`@code` PasswordConverter} and {`@code` EncryptFacade} to + * {`@code` org.zstack.header.convert} so {`@link` org.zstack.header.server.PhysicalServerAO}🤖 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 `@test/src/test/groovy/org/zstack/test/integration/server/PhysicalServerOobPasswordEncryptCase.groovy` around lines 20 - 22, 注释中写的包路径 header.core.encrypt 与实际类所在包不一致；把注释中提到的 PasswordConverter 和 EncryptFacade 的包路径改为实际迁移后的包 org.zstack.header.convert（或直接移除错误的包名），并同时确认注释中提到的 PhysicalServerAO 的 `@Convert`(PasswordConverter.class) 描述与当前代码匹配，以避免误导定位。header/src/main/java/org/zstack/header/convert/PasswordConverter.java (1)
39-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
encryptFacade == null不应走明文透传路径当前逻辑在注入异常或初始化异常时会静默降级为明文处理，存在 fail-open 风险。建议
encryptFacade == null时直接 fail-fast，仅在isEncryptionDisabled()为 true 时透传。建议修复
`@Override` public String convertToDatabaseColumn(String attribute) { - if (encryptFacade == null || encryptFacade.isEncryptionDisabled()) { + if (encryptFacade == null) { + throw new IllegalStateException("EncryptFacade is not initialized"); + } + if (encryptFacade.isEncryptionDisabled()) { return attribute; } if (StringUtils.isEmpty(attribute)) { return attribute; } return encryptFacade.encrypt(attribute); } `@Override` public String convertToEntityAttribute(String dbData) { - if (encryptFacade == null || encryptFacade.isEncryptionDisabled()) { + if (encryptFacade == null) { + throw new IllegalStateException("EncryptFacade is not initialized"); + } + if (encryptFacade.isEncryptionDisabled()) { return dbData; } if (StringUtils.isEmpty(dbData)) { return dbData ; } return encryptFacade.decrypt(dbData); }Also applies to: 50-51
🤖 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/convert/PasswordConverter.java` around lines 39 - 40, The current null-check allows encryptFacade == null to fall through to plaintext; update PasswordConverter so that when encryptFacade == null it fails fast (throw an IllegalStateException or similar) instead of returning attribute, and only allow plaintext passthrough when encryptFacade != null && encryptFacade.isEncryptionDisabled() is true; apply the same change to the other similar null/check block (the second occurrence around lines 50-51) so both code paths use encryptFacade != null && encryptFacade.isEncryptionDisabled() for passthrough and explicitly error when encryptFacade is null.plugin/physicalServer/src/main/java/org/zstack/server/PhysicalServerApiInterceptor.java (2)
84-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick win先对 serialNumber 参数执行 trim，再进行必填和唯一性校验。
此问题在之前的审查中已被指出但尚未修复。直接使用
msg.getSerialNumber()进行isEmpty()判断和数据库查询会导致：
- 仅包含空白字符的输入被当作有效值
- 唯一性校验基于未规整的值执行，可能产生重复记录
🔧 建议修改
private void validate(APICreatePhysicalServerMsg msg) { if (!NetworkUtils.isIpv4Address(msg.getManagementIp())) { throw new ApiMessageInterceptionException(argerr("invalid managementIp[%s]", msg.getManagementIp())); } // Validate poolUuid belongs to same zone if (msg.getPoolUuid() != null && msg.getZoneUuid() != null) { ServerPoolVO pool = dbf.findByUuid(msg.getPoolUuid(), ServerPoolVO.class); if (pool != null && !pool.getZoneUuid().equals(msg.getZoneUuid())) { throw new ApiMessageInterceptionException(argerr( "ServerPool[uuid:%s] belongs to Zone[uuid:%s], but PhysicalServer specifies Zone[uuid:%s]", msg.getPoolUuid(), pool.getZoneUuid(), msg.getZoneUuid() )); } } // ZSTAC-85184: pre-check serialNumber uniqueness within zone so callers see a // readable error instead of the SYS.1000 ConstraintViolationException bubbling // from the DB. The DB unique key remains as a belt-and-braces guard for races. + String serialNumber = msg.getSerialNumber() == null ? null : msg.getSerialNumber().trim(); - if (msg.getSerialNumber() != null && !msg.getSerialNumber().isEmpty() && msg.getZoneUuid() != null) { + if (serialNumber != null && !serialNumber.isEmpty() && msg.getZoneUuid() != null) { boolean dup = Q.New(PhysicalServerVO.class) .eq(PhysicalServerAO_.zoneUuid, msg.getZoneUuid()) - .eq(PhysicalServerAO_.serialNumber, msg.getSerialNumber()) + .eq(PhysicalServerAO_.serialNumber, serialNumber) .isExists(); if (dup) { throw new ApiMessageInterceptionException(argerr( "ORG_ZSTACK_PHYSICAL_SERVER_SERIAL_NUMBER_DUPLICATE", "PhysicalServer with serialNumber[%s] already exists in Zone[uuid:%s]; serialNumber must be unique per zone", - msg.getSerialNumber(), msg.getZoneUuid())); + serialNumber, msg.getZoneUuid())); } } }As per coding guidelines
**/*Interceptor.java: 注意检查来自 Message 的参数是否做过 trim，用户可能在浏览器上复制粘贴的数据带有空格、换行符等。🤖 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 `@plugin/physicalServer/src/main/java/org/zstack/server/PhysicalServerApiInterceptor.java` around lines 84 - 95, Trim the serialNumber from the Message before doing required/uniqueness checks in PhysicalServerApiInterceptor: retrieve msg.getSerialNumber(), call trim() into a local variable (e.g., trimmedSerial), then use trimmedSerial (not msg.getSerialNumber()) for the null/empty check and in the Q.New(PhysicalServerVO.class) .eq(PhysicalServerAO_.serialNumber, ...) existence query and in the ApiMessageInterceptionException message; ensure you treat a trimmed empty string as missing and enforce uniqueness per zone using the trimmed value.
122-142:⚠️ Potential issue | 🟠 Major | ⚡ Quick win先对 type 和 DHCP 相关字段执行 trim，再进行类型判断和必填校验。
此问题在之前的审查中已被指出但尚未修复。直接使用
msg.getType()、msg.getDhcpInterface()等进行判断会导致：
- type 为
" GATEWAY_PXE "时无法匹配校验逻辑- DHCP 字段仅包含空白字符时绕过必填校验
- 错误消息中可能包含不可见的空白字符
🔧 建议修改
+ private static boolean isBlank(String value) { + return value == null || value.trim().isEmpty(); + } + private void validate(APICreateProvisionNetworkMsg msg) { if (msg.getDhcpRangeStartIp() != null && !NetworkUtils.isIpv4Address(msg.getDhcpRangeStartIp())) { throw new ApiMessageInterceptionException(argerr("invalid dhcpRangeStartIp[%s]", msg.getDhcpRangeStartIp())); } if (msg.getDhcpRangeEndIp() != null && !NetworkUtils.isIpv4Address(msg.getDhcpRangeEndIp())) { throw new ApiMessageInterceptionException(argerr("invalid dhcpRangeEndIp[%s]", msg.getDhcpRangeEndIp())); } if (msg.getDhcpRangeNetmask() != null && !NetworkUtils.isIpv4Address(msg.getDhcpRangeNetmask())) { throw new ApiMessageInterceptionException(argerr("invalid dhcpRangeNetmask[%s]", msg.getDhcpRangeNetmask())); } if (msg.getDhcpRangeGateway() != null && !NetworkUtils.isIpv4Address(msg.getDhcpRangeGateway())) { throw new ApiMessageInterceptionException(argerr("invalid dhcpRangeGateway[%s]", msg.getDhcpRangeGateway())); } // ZSTAC-85350: GATEWAY_PXE requires DHCP wiring to be usable as a PXE network. // Pre-fix the API would persist a half-built ProvisionNetwork with Enabled state // that would later fail at provisioning time. Surface the missing fields up-front // with one consolidated error so the caller knows exactly what to supply. + String type = msg.getType() == null ? null : msg.getType().trim(); - if ("GATEWAY_PXE".equals(msg.getType())) { + if ("GATEWAY_PXE".equals(type)) { java.util.List<String> missing = new java.util.ArrayList<>(); - if (msg.getDhcpInterface() == null || msg.getDhcpInterface().isEmpty()) { + if (isBlank(msg.getDhcpInterface())) { missing.add("dhcpInterface"); } - if (msg.getDhcpRangeStartIp() == null || msg.getDhcpRangeStartIp().isEmpty()) { + if (isBlank(msg.getDhcpRangeStartIp())) { missing.add("dhcpRangeStartIp"); } - if (msg.getDhcpRangeEndIp() == null || msg.getDhcpRangeEndIp().isEmpty()) { + if (isBlank(msg.getDhcpRangeEndIp())) { missing.add("dhcpRangeEndIp"); } - if (msg.getDhcpRangeNetmask() == null || msg.getDhcpRangeNetmask().isEmpty()) { + if (isBlank(msg.getDhcpRangeNetmask())) { missing.add("dhcpRangeNetmask"); } if (!missing.isEmpty()) { throw new ApiMessageInterceptionException(argerr( "ORG_ZSTACK_PROVISION_NETWORK_DHCP_MISSING", "ProvisionNetwork[type:GATEWAY_PXE] requires DHCP wiring; missing field(s): %s", String.join(", ", missing))); } } }As per coding guidelines
**/*Interceptor.java: 注意检查来自 Message 的参数是否做过 trim，用户可能在浏览器上复制粘贴的数据带有空格、换行符等。🤖 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 `@plugin/physicalServer/src/main/java/org/zstack/server/PhysicalServerApiInterceptor.java` around lines 122 - 142, In PhysicalServerApiInterceptor, sanitize inputs by trimming msg.getType() and all DHCP-related fields before using them: obtain trimmedType = safeTrim(msg.getType()) and trimmedDhcpInterface/trimmedDhcpRangeStartIp/trimmedDhcpRangeEndIp/trimmedDhcpRangeNetmask from msg, use trimmedType for the "GATEWAY_PXE" equality check and use the trimmed DHCP values when checking emptiness and when building the missing list; throw the same ApiMessageInterceptionException/argerr with String.join on the trimmed missing names so error messages do not include invisible whitespace.
🧹 Nitpick comments (2)
plugin/physicalServer/src/main/java/org/zstack/server/hardware/PhysicalServerHardwareService.java (2)
194-194: ⚡ Quick win将辅助方法可见性收紧为
private。Line 194 位于“private helpers”区块，当前包级可见性会扩大类外可调用面，建议收紧封装边界。
💡 建议修改
- void persistHardwareInfo(String serverUuid, UnifiedHardwareInfo info) { + private void persistHardwareInfo(String serverUuid, UnifiedHardwareInfo info) {🤖 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 `@plugin/physicalServer/src/main/java/org/zstack/server/hardware/PhysicalServerHardwareService.java` at line 194, The helper method persistHardwareInfo(String serverUuid, UnifiedHardwareInfo info) currently has package-private visibility but is declared among private helpers; change its visibility to private to tighten encapsulation by updating the method signature of persistHardwareInfo to be private so it cannot be called from outside the class.
77-82: ⚡ Quick win建议把
mergeNonNull改为void并同步更新注释契约。Line 77-80 已不再使用返回值，保留
boolean返回会误导后续维护者（尤其当前注释仍提到discoverSource语义）。💡 建议修改
- boolean mergeNonNull(UnifiedHardwareInfo target, UnifiedHardwareInfo source) { + void mergeNonNull(UnifiedHardwareInfo target, UnifiedHardwareInfo source) { if (source == null) { - return false; + return; } - boolean changed = false; if (target.getManufacturer() == null && source.getManufacturer() != null) { target.setManufacturer(source.getManufacturer()); - changed = true; } @@ if (target.getTotalMemoryBytes() == null && source.getTotalMemoryBytes() != null) { target.setTotalMemoryBytes(source.getTotalMemoryBytes()); - changed = true; } - return changed; }🤖 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 `@plugin/physicalServer/src/main/java/org/zstack/server/hardware/PhysicalServerHardwareService.java` around lines 77 - 82, The method mergeNonNull currently returns a boolean but its return value is no longer used (calls like mergeNonNull(merged, runExt(...)) ignore the result); change mergeNonNull's signature to return void and update its implementation accordingly, then update all call sites (e.g. the sequence with runExt(SOURCE_IPMI_FRU, server), runExt(SOURCE_KVM_AGENT, server), runExt(SOURCE_K8S_NODEINFO, server) and subsequent persistHardwareInfo(serverUuid, merged)) to treat it as a void helper; also update the method's Javadoc/comments (and any mention of discoverSource semantics) to reflect the new void contract so the behavior and intent are clear to future maintainers.
🤖 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
`@plugin/physicalServer/src/main/java/org/zstack/server/PhysicalServerApiInterceptor.java`:
- Around line 48-59: The validate(APIAttachPhysicalServerRoleMsg msg) method
uses msg.getServerUuid() and msg.getRoleType() directly; trim both values (e.g.,
assign String serverUuid = msg.getServerUuid().trim(); String roleType =
msg.getRoleType().trim()) before using them in the
Q.New(PhysicalServerRoleVO.class) query (PhysicalServerRoleVO_.serverUuid,
PhysicalServerRoleVO_.roleType) and when building the
ApiMessageInterceptionException via argerr so the existence check and the error
message use the normalized (trimmed) values.
---
Duplicate comments:
In `@header/src/main/java/org/zstack/header/convert/PasswordConverter.java`:
- Around line 39-40: The current null-check allows encryptFacade == null to fall
through to plaintext; update PasswordConverter so that when encryptFacade ==
null it fails fast (throw an IllegalStateException or similar) instead of
returning attribute, and only allow plaintext passthrough when encryptFacade !=
null && encryptFacade.isEncryptionDisabled() is true; apply the same change to
the other similar null/check block (the second occurrence around lines 50-51) so
both code paths use encryptFacade != null &&
encryptFacade.isEncryptionDisabled() for passthrough and explicitly error when
encryptFacade is null.
In
`@plugin/physicalServer/src/main/java/org/zstack/server/PhysicalServerApiInterceptor.java`:
- Around line 84-95: Trim the serialNumber from the Message before doing
required/uniqueness checks in PhysicalServerApiInterceptor: retrieve
msg.getSerialNumber(), call trim() into a local variable (e.g., trimmedSerial),
then use trimmedSerial (not msg.getSerialNumber()) for the null/empty check and
in the Q.New(PhysicalServerVO.class) .eq(PhysicalServerAO_.serialNumber, ...)
existence query and in the ApiMessageInterceptionException message; ensure you
treat a trimmed empty string as missing and enforce uniqueness per zone using
the trimmed value.
- Around line 122-142: In PhysicalServerApiInterceptor, sanitize inputs by
trimming msg.getType() and all DHCP-related fields before using them: obtain
trimmedType = safeTrim(msg.getType()) and
trimmedDhcpInterface/trimmedDhcpRangeStartIp/trimmedDhcpRangeEndIp/trimmedDhcpRangeNetmask
from msg, use trimmedType for the "GATEWAY_PXE" equality check and use the
trimmed DHCP values when checking emptiness and when building the missing list;
throw the same ApiMessageInterceptionException/argerr with String.join on the
trimmed missing names so error messages do not include invisible whitespace.
In
`@test/src/test/groovy/org/zstack/test/integration/server/PhysicalServerOobPasswordEncryptCase.groovy`:
- Around line 20-22: 注释中写的包路径 header.core.encrypt 与实际类所在包不一致；把注释中提到的
PasswordConverter 和 EncryptFacade 的包路径改为实际迁移后的包
org.zstack.header.convert（或直接移除错误的包名），并同时确认注释中提到的 PhysicalServerAO 的
`@Convert`(PasswordConverter.class) 描述与当前代码匹配，以避免误导定位。
---
Nitpick comments:
In
`@plugin/physicalServer/src/main/java/org/zstack/server/hardware/PhysicalServerHardwareService.java`:
- Line 194: The helper method persistHardwareInfo(String serverUuid,
UnifiedHardwareInfo info) currently has package-private visibility but is
declared among private helpers; change its visibility to private to tighten
encapsulation by updating the method signature of persistHardwareInfo to be
private so it cannot be called from outside the class.
- Around line 77-82: The method mergeNonNull currently returns a boolean but its
return value is no longer used (calls like mergeNonNull(merged, runExt(...))
ignore the result); change mergeNonNull's signature to return void and update
its implementation accordingly, then update all call sites (e.g. the sequence
with runExt(SOURCE_IPMI_FRU, server), runExt(SOURCE_KVM_AGENT, server),
runExt(SOURCE_K8S_NODEINFO, server) and subsequent
persistHardwareInfo(serverUuid, merged)) to treat it as a void helper; also
update the method's Javadoc/comments (and any mention of discoverSource
semantics) to reflect the new void contract so the behavior and intent are clear
to future maintainers.
🪄 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: Path: http://open.zstack.ai:20001/code-reviews/zstack-cloud.yaml (via .coderabbit.yaml)
Review profile: CHILL
Plan: Pro
Run ID: f19b4733-8c63-4290-ab5b-5e953f38f997
⛔ Files ignored due to path filters (1)
conf/springConfigXml/encrypt.xmlis excluded by!**/*.xml
📒 Files selected for processing (25)
conf/db/upgrade/V5.5.18__schema.sqlcore/src/main/java/org/zstack/core/convert/SpecialDataConverter.javacore/src/main/java/org/zstack/core/encrypt/EncryptFacade.javacore/src/main/java/org/zstack/core/encrypt/EncryptFacadeImpl.javadocs/brainstorms/2026-05-12-followups.mddocs/brainstorms/archives/2026-05-11-assertion-changelog.mddocs/brainstorms/archives/2026-05-11-case-fix-log.mddocs/brainstorms/next-session.mddocs/review/v5518-ut-on-bypass-audit.mddocs/todolist.mdheader/src/main/java/org/zstack/header/convert/EncryptFacade.javaheader/src/main/java/org/zstack/header/convert/PasswordConverter.javaheader/src/main/java/org/zstack/header/server/PhysicalServerAO.javaheader/src/main/java/org/zstack/header/server/PhysicalServerHardwareDiscoveryExtensionPoint.javaheader/src/main/java/org/zstack/header/server/PhysicalServerHardwareInfoVO.javaplugin/ceph/src/main/java/org/zstack/storage/ceph/CephMonAO.javaplugin/kvm/src/main/java/org/zstack/kvm/KVMHostVO.javaplugin/physicalServer/src/main/java/org/zstack/server/PhysicalServerApiInterceptor.javaplugin/physicalServer/src/main/java/org/zstack/server/hardware/PhysicalServerHardwareService.javaplugin/physicalServer/src/main/java/org/zstack/server/hardware/UnifiedHardwareInfo.javaplugin/physicalServer/src/test/java/org/zstack/server/hardware/UnifiedHardwareInfoMergeTest.javaplugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorageVO.javatest/src/test/groovy/org/zstack/test/integration/kvm/host/HostPasswordEncryptCase.groovytest/src/test/groovy/org/zstack/test/integration/server/PhysicalServerInterceptorErrorsCase.groovytest/src/test/groovy/org/zstack/test/integration/server/PhysicalServerOobPasswordEncryptCase.groovy
💤 Files with no reviewable changes (4)
- core/src/main/java/org/zstack/core/encrypt/EncryptFacade.java
- header/src/main/java/org/zstack/header/server/PhysicalServerHardwareDiscoveryExtensionPoint.java
- conf/db/upgrade/V5.5.18__schema.sql
- header/src/main/java/org/zstack/header/server/PhysicalServerHardwareInfoVO.java
✅ Files skipped from review due to trivial changes (6)
- plugin/sftpBackupStorage/src/main/java/org/zstack/storage/backup/sftp/SftpBackupStorageVO.java
- test/src/test/groovy/org/zstack/test/integration/kvm/host/HostPasswordEncryptCase.groovy
- core/src/main/java/org/zstack/core/convert/SpecialDataConverter.java
- docs/brainstorms/next-session.md
- docs/brainstorms/archives/2026-05-11-assertion-changelog.md
- plugin/physicalServer/src/test/java/org/zstack/server/hardware/UnifiedHardwareInfoMergeTest.java
Comment on lines
+48
to
+59
| private void validate(APIAttachPhysicalServerRoleMsg msg) { | ||
| boolean exists = Q.New(PhysicalServerRoleVO.class) | ||
| .eq(PhysicalServerRoleVO_.serverUuid, msg.getServerUuid()) | ||
| .eq(PhysicalServerRoleVO_.roleType, msg.getRoleType()) | ||
| .isExists(); | ||
| if (exists) { | ||
| throw new ApiMessageInterceptionException(argerr( | ||
| "ORG_ZSTACK_PHYSICAL_SERVER_ROLE_DUPLICATE", | ||
| "PhysicalServer[uuid:%s] already has role[type:%s] attached; detach first or pick a different roleType", | ||
| msg.getServerUuid(), msg.getRoleType())); | ||
| } | ||
| } |
There was a problem hiding this comment.
对 roleType 和 serverUuid 参数执行 trim 后再进行查询和错误消息拼接。
用户在浏览器中复制粘贴时可能携带空格或换行符，直接使用 msg.getRoleType() 和 msg.getServerUuid() 会导致重复性检查失效或错误消息中包含不可见字符。
🔧 建议修改
private void validate(APIAttachPhysicalServerRoleMsg msg) {
+ String serverUuid = msg.getServerUuid() == null ? null : msg.getServerUuid().trim();
+ String roleType = msg.getRoleType() == null ? null : msg.getRoleType().trim();
+
boolean exists = Q.New(PhysicalServerRoleVO.class)
- .eq(PhysicalServerRoleVO_.serverUuid, msg.getServerUuid())
- .eq(PhysicalServerRoleVO_.roleType, msg.getRoleType())
+ .eq(PhysicalServerRoleVO_.serverUuid, serverUuid)
+ .eq(PhysicalServerRoleVO_.roleType, roleType)
.isExists();
if (exists) {
throw new ApiMessageInterceptionException(argerr(
"ORG_ZSTACK_PHYSICAL_SERVER_ROLE_DUPLICATE",
"PhysicalServer[uuid:%s] already has role[type:%s] attached; detach first or pick a different roleType",
- msg.getServerUuid(), msg.getRoleType()));
+ serverUuid, roleType));
}
}As per coding guidelines **/*Interceptor.java: 注意检查来自 Message 的参数是否做过 trim，用户可能在浏览器上复制粘贴的数据带有空格、换行符等。
🤖 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
`@plugin/physicalServer/src/main/java/org/zstack/server/PhysicalServerApiInterceptor.java`
around lines 48 - 59, The validate(APIAttachPhysicalServerRoleMsg msg) method
uses msg.getServerUuid() and msg.getRoleType() directly; trim both values (e.g.,
assign String serverUuid = msg.getServerUuid().trim(); String roleType =
msg.getRoleType().trim()) before using them in the
Q.New(PhysicalServerRoleVO.class) query (PhysicalServerRoleVO_.serverUuid,
PhysicalServerRoleVO_.roleType) and when building the
ApiMessageInterceptionException via argerr so the existence check and the error
message use the normalized (trimmed) values.
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.
Scope
Fixes 4 of the 5 jiras under epic ZSTAC-85170 (5.5.22-Cloud-硬件统一管理-Buglist). The 5th (ZSTAC-85334) lives in premium baremetal2 — see companion MR on premium repo (same source branch).
Commits in this MR
ab995e3edcPasswordConverter+EncryptFacadeinterface fromcoretoheader.core.encryptso header-resident VOs can use@Convert(PasswordConverter.class)(the same mechanism KVMHostVO uses).EncryptFacade.isEncryptionDisabled()absorbs the previousEncryptGlobalConfig.value()check so the converter has zero core deps. ~30 import-rename sites across kvm/ceph/sftp/test.d15368f77fPhysicalServerAO.oobPasswordwith@Convert(PasswordConverter.class)and drop the application-layerPhysicalServerOobPasswordCryptostop-gap. Hibernate handles encrypt-on-write / decrypt-on-read transparently. IT case rewritten to mirrorHostPasswordEncryptCase(state assertion viaEncryptEntityMetadataVO).a3bdf52fcdPhysicalServerApiInterceptor: pre-check zone+serialNumber uniqueness; pre-check (serverUuid, roleType) uniqueness; rejectGATEWAY_PXEwith missing DHCP wiring. All use 3-argargerr(globalErrorCode, fmt, args...)so the user-facingdetailsshows a fully formatted sentence instead of just the offending uuid.Integration tests
PhysicalServerOobPasswordEncryptCase(test/) — 3 sub-cases PASSEDPhysicalServerInterceptorErrorsCase(test/) — 3 sub-cases PASSEDE2E (MN 172.24.226.204)
Hot-deployed all touched jars + xmls; verified on real MN:
E2E-Final-Refactor-Pwd!→ DBoobPassword= 64-byte ciphertextRoPHbrg24uuy54aCjuInm30ZbOtwg21o...(encryption=LocalEncryption); decrypt round-trip matches.details: PhysicalServer with serialNumber[TC-PS-01] already exists in Zone[uuid:...]; serialNumber must be unique per zone.details: PhysicalServer[uuid:...] already has role[type:KVM_HOST] attached; detach first or pick a different roleType.details: ProvisionNetwork[type:GATEWAY_PXE] requires DHCP wiring; missing field(s): dhcpInterface, dhcpRangeStartIp, dhcpRangeEndIp, dhcpRangeNetmask.MN rolled back to baseline (test PSes deleted, encrypt config restored to None).
Companion MR
Premium side (ZSTAC-85334 + the cross-repo refactor sweep) is on the matching
cordon-container@@2branch inzstackio/premium.Resolves: ZSTAC-85182, ZSTAC-85184, ZSTAC-85190, ZSTAC-85350
sync from gitlab !9930