Duansg commented on PR #4281:
URL: https://github.com/apache/hertzbeat/pull/4281#issuecomment-5413256201
The parameter-definition fix in `fcbdf3f2d3` looks right — app defines are
always loaded, scrape defines are appended, and `checkJobFields` covers both. I
also verified the runtime story end-to-end: one-off detect jobs go through
`WheelTimerTask.initJobMetrics` and `AesUtil.aesDecode` falls back to the
legacy default key on `BadPaddingException`, so retained legacy ciphertext
still decrypts. However, this head introduces two new blocking regressions.
> 1. Detect from the edit page now fails for every monitor with a stored
password
`MonitorController.java:102` calls `validate(monitorDto, null)`, but the
mask-restore branch requires `Boolean.TRUE.equals(isModify)` and otherwise
throws "The credential mask cannot be used as a new value." The edit page's
`onDetect` (monitor-edit.component.ts) submits the form params verbatim — and
the form is populated from `GET /monitor/{id}`, which now returns `******`. So
opening any existing mysql/ssh/redis/ollama monitor and clicking availability
detect returns an error unless the user re-types the password. This is not
limited to the two migrated fields; it regresses detect for every
password-bearing app (it worked before because the API returned ciphertext,
which the validator accepted).
Suggestion: allow the mask-restore (including the destination-binding check)
on the detect path when the DTO carries an existing monitor id, while keeping
the rejection for `isModify == FALSE` (import / new monitors).
> 2. Import masks the exported ciphertext in memory, so any export
containing a password param fails to import
The export side now preserves ciphertext, but the import side undoes it:
`convert(ExportMonitorDTO)` at `AbstractImExportServiceImpl.java:169` calls the
single-arg `monitorDto.setParams(list)`, which defaults to
`maskCredentials=true`. The imported type-2 ciphertext is replaced with
`******` before `validateImportBatch` runs `validate(dto, false)` — which then
rejects the mask and fails the whole batch. JSON, Excel, and YAML all share
this convert. This is the same round-trip scenario from the first review; the
export half is fixed but this default cancels it on import.
The new `testExportConfigPreservesEncryptedCredentialForImportRoundTrip`
only asserts on the export stream with `monitorService` fully mocked, which is
why the import path wasn't caught. Suggestion: use `monitorDto.setParams(list,
false)` in the import-side convert and add a real round-trip test through
`convert(ExportMonitorDTO)` with a type-2 param.
Non-blocking
- Concurrent first boot of two managers sharing a database: the second
`INSERT` of the migration marker hits a primary-key conflict and aborts
startup; consider treating duplicate-key as already-migrated.
- `CREDENTIAL_DESTINATION_FIELDS` is keyed by bare field name; a future
template reusing `apiKey` would silently inherit ollama's host/port/ssl
binding. Consider keying by app/scrape or documenting the assumption.
- Destination binding covers only the three listed fields; other apps'
passwords can still be restored via the mask after a host change. Not a
regression, but worth stating the boundary in the PR description or a follow-up
issue.
--
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]
