DanielLeens commented on PR #11613: URL: https://github.com/apache/seatunnel/pull/11613#issuecomment-5701066643
Thanks for the independent cross-check, and for confirming the two real fixes (log-leak / mTLS status accuracy) I flagged in my last round. Going through your six points against source (current head `a947704b`, unchanged since my last review): 1. **F3 (AuthZ on the two new endpoints)** — not a gap. Both `httpServiceStatusHolder` and `updateLocalMemberTagsHandler` are registered on the same `ServletContextHandler` instance (`JettyService.java`, the `context.addServlet(...)` block that also wires every pre-existing endpoint), and `BasicAuthFilter` is attached to that same context at `"/*"` earlier in `createContext()` when basic auth is enabled. mTLS is enforced at the Jetty `Server`'s connector/SSL layer, which is shared across the whole context too, not per-servlet. So both new endpoints inherit the same auth chain as every existing one — I verified this in my 2026-08-19 round and it's unchanged. 2. **F2 (RestApiIT/E2E for the two new endpoints)** — fair, non-blocking observation. Coverage today is unit-level (`UpdateTagsServiceTest`, `HttpServiceStatusServletTest`), and I don't see a dedicated `RestApiIT`/E2E case exercising either endpoint over real HTTP. I wouldn't block merge on it given the unit coverage is solid and both endpoints are read-only or narrowly scoped, but it's a reasonable follow-up ask. 3. **F4 (tag editor behind LB / local-only UUID validation)** — this is by design, not a bug: `validateTargetMember()` intentionally requires the uuid to equal the node the request lands on, so the editor only ever mutates the member actually serving the UI. Worth a doc callout for the "Web UI behind a load balancer fronting multiple masters" case if it isn't already explicit, but not a functional defect. 4. **F6 (Restore Latest State on a still-RUNNING source job / cleaned-up checkpoint state)** — you're right, this is genuinely open. I checked `JobInfoService.validateCheckpointRestoreRequest()` end to end: it only rejects a blank `restoreSourceJobId`, there's no check on the source job's current status or on whether the referenced checkpoint/savepoint still exists on disk. The UI's `canRestore()` in `checkpoints.tsx` similarly only checks that pipeline data exists in the overview response, not job status. It's not a new regression from this PR's hardening commit, and I wouldn't treat it as a merge blocker, but it's worth either a guard or an explicit documented limitation — good catch. 5. **F1/F5/F7/F8 (docs completeness)** — agreed these are worth doing; non-blocking polish items, same category I'd have raised as "recommended, non-blocking" rather than a blocker. None of this changes my merge recommendation from the last round: still no source-level blocker, the two open items I called out (Build check currently red on pre-existing/tracked flakes, and Draft status) remain the only things standing between this head and merge. F2/F4-doc/F6/docs above are good non-blocking follow-ups for @danielnadean to fold in when convenient, not new blockers I'm raising. -- 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]
