Duansg commented on PR #4293:
URL: https://github.com/apache/hertzbeat/pull/4293#issuecomment-5243198376
The compatibility migration described in the body is not in the diff —
existing SFTP monitors will stop collecting on upgrade
The Summary and "Upgrade behavior" both promise a one-time migration setting
insecureSkipVerify=true on legacy SFTP monitors, and Validation lists
SftpHostKeyCompatibilityMigrationTest. The current head contains only 7 files
(FtpCollectImpl, FtpProtocol and their tests, app-ftp.yml, two ftp.md docs) —
no migration class, no SQL, no such test. Searching the diff for
migration|compatibility|CommandLineRunner|Flyway|.sql returns nothing. It looks
like the migration was dropped in a rebase and the description didn't follow.
Failure chain: a legacy monitor's params contain neither field →
CollectUtil.replaceSmilingPlaceholder takes the else { value = null; break; }
path, so both arrive as null → in preCheck, parseBoolean(ssl) is true and
parseBoolean(null) is false, entering the new branch where Assert.hasText(null)
throws → MetricsCollect:201-213 sets Code.FAIL → basic is the priority: 0
availability metric, so the monitor goes unavailable. Plain FTP is unaffected.
Please pick one and make the description match:
- Restore the migration. Note the earlier revision's concern still applies:
a CommandLineRunner with no version or completion marker re-runs on every
startup, so a monitor later created via import, the API, or direct SQL without
a host-key policy gets silently flagged insecureSkipVerify=true — reversing the
fail-closed guarantee this PR is built on. A Flyway migration or a persisted
completion marker avoids that.
- Accept fail-closed. Better posture, and it removes the hole above — but
Summary, "Upgrade behavior" and Validation all need rewriting, and the release
notes must state that after upgrade every SFTP monitor stops collecting until
an operator supplies a fingerprint verified through a trusted channel, or ticks
the temporary skip option.
Validation should also be re-run against the actual head —
SftpHostKeyCompatibilityMigrationTest doesn't exist there, so "15 tests passed"
isn't reproducible.
Minor: depend: ssl on the two new params won't take effect. depend is only
re-evaluated for radio fields (boolean dispatches to onParamBooleanChanged,
which never touches depend), and ssl is boolean — so both fields will show
unconditionally. Every existing depend: in the repo targets a radio field.
The verifier itself looks right: explicit BuiltinDigests.sha256, enforced
SHA256:base64 format, multiple fingerprints for rotation, MessageDigest.isEqual
comparison.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
