DanielLeens commented on PR #11613:
URL: https://github.com/apache/seatunnel/pull/11613#issuecomment-5379366298
# What Problem Does This PR Solve?
- **User pain point**: the Zeta Web UI was mostly a read-only inspection
console — operators could see jobs, metrics and checkpoints, but had to fall
back to the CLI/REST directly for job submission,
stopping/savepointing/cancelling a running job, restoring from a checkpoint,
looking up a connector's option rules, checking whether
HTTP/HTTPS/basic-auth/mTLS are actually enabled on a node, or updating a
worker's tags.
- **Fix approach**: this PR wires all of that into the UI — job submission
(text or uploaded config, with an optional savepoint-restore via
`restoreJobId`), running-job control (stop / savepoint-stop / force-cancel), a
checkpoint detail/history view with a "Restore Latest State" shortcut, a
connector option-rule lookup page, a new read-only `GET /http-service/status`
page, and a local-worker tag editor — plus a safety check added to the existing
`update-tags` endpoint.
- **One-sentence summary**: turns the Zeta Web UI from a read-only console
into a genuine operations surface, on top of already-existing REST job-control
endpoints.
I did a fresh, independent verification pass rather than reusing the
extensive review history already on this PR: I pulled every backend file
changed by this PR straight from the GitHub Contents/Files API at the current
head (`37ca28b6e4139365ae7e9a45acbc6b10e3e446b8`) and re-derived the
security-relevant conclusions below (auth coverage, sensitive-field leakage,
backward compatibility of `update-tags`) directly from that source, plus a
fresh live check of CI.
# 1. Code Change Review
## 1.1 Core Logic Analysis
This PR's backend footprint is small and additive relative to its UI
footprint: `JettyService.java` (+6), `RestConstant.java` (+1),
`BaseService.java` (+15/-1), `UpdateTagsService.java` (+71/-9), and one new
file, `HttpServiceStatusServlet.java` (140 lines). Everything else is
`seatunnel-engine-ui` (Vue/TSX) plus `docs/en|zh`.
I read `HttpServiceStatusServlet.java` in full and checked every field it
returns against what it must never leak:
```java
JsonObject status = new JsonObject()
.add("httpEnabled", httpConfig.isEnabled())
.add("httpsEnabled", httpConfig.isEnableHttps())
.add("configuredHttpPort", httpConfig.getPort())
.add("configuredHttpsPort", httpConfig.getHttpsPort())
.add("httpPort", findConnectorPort(false, httpConfig.getPort()))
.add("httpsPort", findConnectorPort(true, httpConfig.getHttpsPort()))
.add("contextPath", defaultContextPath(httpConfig.getContextPath()))
.add("dynamicPortEnabled", httpConfig.isEnableDynamicPort())
.add("portRange", httpConfig.getPortRange())
.add("basicAuthEnabled", httpConfig.isEnableBasicAuth())
.add("mutualTlsEnabled", hasText(httpConfig.getTrustStorePath()) &&
hasText(httpConfig.getTrustStorePassword()));
```
Confirmed field by field: no `username`, `password`, `keyStorePath`,
`keyStorePassword`, `trustStorePath`, or `trustStorePassword` value is ever put
into the response — `mutualTlsEnabled` only exposes a derived boolean of
whether those two fields are non-blank, never their contents. This is a
genuinely well-designed "safe status" endpoint.
I then checked the auth-coverage claim directly in `JettyService.java`
rather than taking it on faith:
```java
FilterHolder exceptionFilterHolder = new FilterHolder(new
ExceptionHandlingFilter());
context.addFilter(exceptionFilterHolder, "/*",
EnumSet.of(DispatcherType.REQUEST));
...
FilterHolder basicAuthFilterHolder = new FilterHolder(new
BasicAuthFilter(httpConfig));
context.addFilter(basicAuthFilterHolder, "/*",
EnumSet.of(DispatcherType.REQUEST));
...
context.addServlet(new ServletHolder(new
HttpServiceStatusServlet(nodeEngine, seaTunnelConfig, server)), ...)
```
Both filters are registered on the same Jetty `context` with a `"/*"` path
spec, and `HttpServiceStatusServlet` is registered on that same context. So
when basic auth is enabled cluster-wide, this new endpoint requires it exactly
like every other endpoint; when disabled, it's exactly as open as every other
endpoint today. **No new unauthenticated attack surface.**
`UpdateTagsService.java` — I read the full 116-line file. The new
`validateTargetMember` check:
```java
Object uuid = params.get("uuid");
if (uuid != null &&
!localMember.getUuid().toString().equals(uuid.toString())) {
throw new IllegalArgumentException(...);
}
```
is a real, net-positive scoping fix: it rejects a request whose explicit
`uuid` doesn't match the node actually serving it, preventing an operator from
silently mutating the wrong worker's tags via a stale UI selection.
`extractTagParams` still falls back to treating the whole body (minus `uuid`)
as the legacy flat tag map when no `tags` object is present, so the historical
request shape keeps working.
**New finding this round, not raised before**: `extractTagParams` (lines
87-98) now does:
```java
Object tags = params.get("tags");
if (tags instanceof Map) { return (Map<String, Object>) tags; }
if (tags != null) { throw new IllegalArgumentException("The tags field must
be an object."); }
Map<String, Object> legacyTags = new HashMap<>(params);
legacyTags.remove("uuid");
return legacyTags;
```
Before this PR, a legacy-format request with a top-level key literally named
`tags` whose value is a non-object (e.g. `{"tags": "some-value"}`, intending to
set a tag *named* `tags`) would have been silently accepted as a flat tag map
entry. After this PR, that exact request now throws
`IllegalArgumentException("The tags field must be an object.")` instead. This
is a narrow, low-probability compatibility edge case — a caller would need to
have been using `tags` as a literal tag *key* under the old flat-map contract —
but it is a real behavior change for that one input shape, not just an additive
safety check, and it isn't mentioned in the PR's compatibility notes. I'm
keeping this Low severity since the field name collision is unlikely in
practice, but it should be called out explicitly.
## 1.2 Compatibility Impact
**Mostly compatible, with one narrow caveat (see Issue 2 below).**
`REST_URL_HTTP_SERVICE_STATUS = "/http-service/status"` is a brand-new,
additive endpoint. `BaseService`'s monitoring-info JSON gains new fields
additively (I confirmed no existing field is renamed or removed in the
`BaseService.java` diff). `update-tags`'s legacy flat-map body still works for
every input except the one edge case above. No SeaTunnel `Option`, default
value, checkpoint/savepoint format, or job-state serialization is touched.
## 1.3 Performance / Side-Effect Analysis
`HttpServiceStatusServlet` is read-only: it iterates
`server.getConnectors()` (a small, bounded, in-memory list) and does no
locking, no allocation beyond the JSON response, no I/O. No new concurrency,
retry, or resource-release concerns introduced by the backend changes in this
PR.
## 1.4 Error Handling and Logging
No error-handling regression found in the backend files I read. The one
carryover item from the review history (`JettyService.java` Spotless/Code-style
failure) is confirmed **fixed and still green** at the current head — see the
live CI section below for the up-to-date evidence.
Formal issues, sorted by severity:
**Issue 1 (Medium, carried over from the prior round, re-verified live): the
`seatunnel-ui` CI job that actually exercises `jobs.spec.ts` still does not run
on this branch's CI configuration.**
- Location: `.github/workflows/backend.yml`'s `seatunnel-ui` job on this
branch (104+ commits behind current `dev`, predating PR #11515's more granular
UI-path routing).
- I re-checked this live rather than trusting the previous round's claim: on
the fork's current run for this exact head (`danielnadean/seatunnel` run
`32327632157`), `Run / Build SeaTunnel UI` is still `skipped`. So the last
commit's test-only fix to `jobs.spec.ts` (see below) has still never actually
been executed by CI on this branch.
- Best improvement: rebase onto current `dev` to pick up #11515's routing,
or have the author run `npm run test:unit` locally and report the result.
- Severity: Medium (verification-confidence gap; my own reading of the diff,
described below, is that the fix is correct). Raised by another reviewer: No
(carried over from Daniel's own prior round, now re-verified independently).
**Issue 2 (Low, new this round): `update-tags` legacy flat-map compatibility
has one narrow edge case**
- Location: `UpdateTagsService.java:87-98` (`extractTagParams`).
- A legacy request using a top-level key literally named `tags` with a
non-object value now throws `IllegalArgumentException` instead of being
silently accepted as before. Worth a one-line mention in the PR
description/docs since it's a real (if unlikely) compatibility edge case, not
purely additive.
- Raised by another reviewer: No.
**Issue 3 (Low, informational, live-verified): current CI is red, but the
failure is unrelated to this PR's diff.**
- I checked this myself rather than trusting the `FAILURE` status label at
face value. The apache-side `Build` pointer check for this head is `failure`,
backed by fork run `32327632157`. Of 82 check runs in that fork run, exactly
one failed: `Run / unit-test (8, ubuntu-latest)`. I pulled that job's full log
and the actual failure is:
```
[ERROR] MessageDelayedEventLimiterTest.testAcquire:48 expected: <true> but
was: <false>
... in module connector-cdc-base
(seatunnel-connectors-v2/connector-cdc/connector-cdc-base)
```
This PR touches only `seatunnel-engine-server`, `seatunnel-engine-ui`, and
docs — it does not touch `connector-cdc-base` or anything CDC-related at all. A
rate-limiter `testAcquire` boundary assertion failing after a 10-second wait
window is a classic CI-load timing flake, not something this diff could have
caused. `Run / Code style` on the same run is green (confirms Issue 1's earlier
Spotless blocker is genuinely still fixed). This is not a blocker; it needs a
rerun of the failed job (or a rebase, given how far behind `dev` this branch
is), not any code change here.
- Raised by another reviewer: No.
# 2. Code Quality Assessment
## 2.1 Coding Standards
Consistent with the rest of the module; ASF headers present; new Java
classes (`HttpServiceStatusServlet`) carry class- and method-level Javadoc,
including an explicit "must not include passwords/keystore paths" contract
comment that I confirmed the implementation actually honors.
## 2.2 Test Coverage and Test Stability
I checked the last commit specifically, since it's the only change since the
prior full review round: it modifies only
`seatunnel-engine-ui/src/tests/jobs.spec.ts` (+9/-3), replacing a zero-arg
`onPositiveClick()` invocation with a helper that passes a synthetic
`MouseEvent`, to satisfy `NPopconfirm`'s typed prop signature under stricter
type-checking. The three assertions on `stopJobSpy`'s call arguments are
unchanged — this reads as a real, narrowly-scoped type-check fix, not a
behavior or coverage change. However, per Issue 1, this fix has never actually
been executed by CI on this branch (the `seatunnel-ui` job is skipped), so
"fixed" here is my own static read of the diff, not a CI-confirmed result.
The other UI test files added by this PR (`checkpoints.spec.ts`,
`operations.spec.ts`, `managers.spec.ts` additions, `detail.spec.ts` additions)
are mock-based with `flushPromises()`, no fixed sleeps, no real network
dependency, no shared mutable global state, no order-sensitive or
floating-point assertions.
**Stability rating: Stable** for what's written; the open item is
verification confidence (Issue 1), not flakiness.
## 2.3 Documentation Updates
`docs/en/engines/zeta/rest-api-v2.md` and `web-ui.md`, plus their `zh`
counterparts, are all updated with matching line counts (+55/-2 and +28/-9
respectively in both languages) — no en/zh drift in the file list. I did not do
a full line-by-line bilingual diff of prose content, but the structural
symmetry (identical +/- counts per language pair) is a good sign of parity.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
**Precise fix, well-scoped.** This PR wires UI controls on top of
already-existing, already-shipped job-control REST endpoints (`stopJob`,
savepoint-stop, cancel) rather than reimplementing any state-machine or
coordinator logic — I confirmed `BaseService.handleStopJob()` and the
coordinator-side cancel/savepoint/stop methods are not touched by this diff at
all. The two backend changes it does make (a read-only status servlet, and a
safety check on an existing mutation endpoint) are minimal and consistent with
the existing filter/auth architecture, not a parallel one.
## 3.2 Maintainability
Small, focused new classes; the `validateTargetMember` addition is a clear,
single-purpose guard. No connector/plugin/SPI registration is touched, so the
full "registration matrix" check (factory/mapper/dist packaging) doesn't apply
to this PR.
## 3.3 Extensibility
The option-rule lookup page and status page both follow existing
servlet/service patterns, so future endpoints in the same family should be easy
to add consistently.
## 3.4 Historical-Version Compatibility
No REST contract, config option, or serialized/checkpoint state is broken.
The one real (if narrow) behavior change is Issue 2 above; everything else is
additive.
# 4. Issue Summary
| # | Issue | Location | Severity | Raised by another reviewer |
| --- | --- | --- | --- | --- |
| 1 | `seatunnel-ui` CI job still skipped for this head; the last commit's
test fix has never actually run in CI | `.github/workflows/backend.yml`,
`jobs.spec.ts` | Medium | No (carried over from Daniel's own prior round,
re-verified live this round) |
| 2 | `update-tags` legacy flat-map body now rejects a literal `tags` key
with a non-object value (narrow compat edge case) |
`UpdateTagsService.java:87-98` | Low | No |
| 3 | Current CI failure is a pre-existing, unrelated flaky test
(`connector-cdc-base`), not caused by this diff | live CI, fork run
`32327632157` | Low, informational | No |
# 5. Merge Recommendation
### Conclusion: Ready to merge after fixes
**1. Blockers — must be fixed before merge:**
- Issue 1 — get the `seatunnel-ui` job to actually execute against this
exact head (rebase onto current `dev` to pick up #11515's routing, or have the
author report a local `npm run test:unit` run) before treating the last
commit's test fix as CI-confirmed rather than statically-read.
**2. Recommended fixes — non-blocking:**
- Issue 2 — document (or adjust) the narrow `tags`-key-collision edge case
in `extractTagParams`.
- Issue 3 — rerun the failed `unit-test (8, ubuntu-latest)` job (or rely on
the rebase in Issue 1 picking up a healthier base); no code change needed on
this PR's side.
- Non-blocking test/doc suggestion carried from earlier rounds: a small
backend test locking in the "`/http-service/status` never returns a sensitive
field" contract would be a nice belt-and-suspenders addition, though I verified
that contract holds today by reading the servlet in full.
**Overall assessment.** I independently re-verified the two things that
matter most for a PR that adds job-control and a new status endpoint to the Web
UI: (1) the new endpoint cannot leak credentials or bypass the existing auth
filter chain — confirmed field-by-field and via the actual filter registration
code, not by trusting the description; and (2) the hardening added to
`update-tags` is a real, backward-compatible-in-the-common-case safety
improvement, with one narrow edge case now flagged as Issue 2 that wasn't
called out in earlier rounds. Live CI is currently red, but I traced the actual
failure to an unrelated pre-existing flaky test in `connector-cdc-base` that
this diff cannot have caused — the real gate is still Issue 1 (get the UI test
job to actually run against this head). This is my own PR, and GitHub blocks
self-approval on this account, so I'm posting this as a plain comment; a
maintainer with approval rights is the remaining gate once Issue 1 is resolved.
--
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]
