DanielLeens commented on PR #10900:
URL: https://github.com/apache/seatunnel/pull/10900#issuecomment-5203580723
Thanks @SEZ9 for the thorough pass, and thanks again to the contributors for
staying with this one. I re-checked the same head (`7d2cd7b3ff32...`, no new
commit landed since my last check on this PR) and traced each finding against
the current `connector-syslog` source directly. Going through them one by one:
**Issue 1 (unauthenticated `0.0.0.0` listener) — agreed, High, and I'd like
to reinforce it with an internal precedent.** `SyslogSourceOptions.HOST` does
default to `"0.0.0.0"` with no TLS/allowlist option
(`SyslogSourceOptions.java:34-39`), and the docs actively recommend that
default. What tips this from "nice to have" to "should block" for me is that we
already have a sibling listener-style source in the same module tree,
`connector-edge-socket`, which also binds `0.0.0.0` by default but requires a
`token` option (`Required: Yes`) and a `__AUTH__:<token>` handshake before
accepting any data (`docs/en/connectors/source/EdgeSocket.md`, "Authentication"
section). So the project already has an established pattern for "listener
source with no built-in transport security must still authenticate senders,"
and this PR doesn't follow it. I'd ask for at minimum an optional
shared-secret/allowlist mechanism (mirroring EdgeSocket) before merge, or a
hard default of binding to loopback wi
th an explicit opt-in + prominent security warning to expose it more broadly.
**Issue 2 (no checkpoint coupling / delivery semantics) — agreed on the core
point, but one correction on the parallelism sub-claim.** I confirmed there's
no `snapshotState`/`restoreState` override in `SyslogSourceReader`, so it falls
back to the no-op defaults in `AbstractSingleSplitReader`, and rows sitting in
the OS socket buffer (not yet read into `pendingRows`) at the moment of a task
failure are indeed lost with no redelivery — that's a real at-most-once gap
worth documenting explicitly, agreed.
However, the "parallelism is unsupported... source must enforce
parallelism=1" part isn't an open gap — it's already enforced upstream.
`AbstractSingleSplitSource.createReader(SourceReader.Context)` (the shared base
class `SyslogSource` extends) does:
```java
checkArgument(
readerContext.getIndexOfSubtask() == 0,
"A single split source allows only one single reader to be created.
Please make sure source parallelism = 1");
```
So any subtask index other than 0 throws before
`SyslogSource.createReader(SingleSplitReaderContext)` (where the `ServerSocket`
bind happens) is ever reached — multiple subtasks can't race to bind the port,
they fail fast instead. This matches the docs' feature matrix, which already
lists `parallelism` as unchecked. I'd keep the checkpoint/at-most-once
documentation ask, but drop the "must enforce parallelism=1" action item since
it's already covered by the framework.
**Issue 3 (no connection/message-size/timeout limits) — agreed, Medium,
verified.** `connectionExecutor` in `SyslogSourceReader.startAcceptLoop()` is
an unbounded `Executors.newCachedThreadPool()` with no cap on concurrent client
sockets, and `processConnection()` reads via `BufferedReader.readLine()`, whose
internal buffer grows unbounded while it waits for a line terminator — a client
that never sends `\n` can grow that buffer indefinitely. Both are real DoS/OOM
vectors for a socket that's reachable from any peer that can route to it,
especially combined with Issue 1's default exposure.
**Issue 4 (no port range validation) — agreed, verified.** `SyslogConfig`
copies `config.get(SyslogSourceOptions.PORT)` straight through with no bounds
check, so an out-of-range port only surfaces later as an opaque `SYSLOG-01`
bind failure. Cheap fix, worth doing in this pass since the config class is
already being touched.
**Issue 5 (changelog placeholder + missing trailing newlines) — agreed, Low,
verified with two extra data points.** `commit/7e29472` in
`docs/en/connectors/changelog/connector-syslog.md` isn't a valid object in the
repo (checked locally), and all four new/changed doc files end without a
trailing newline. For comparison, other recently-added connectors (e.g.
`connector-fluss`) ship an *empty* changelog table pre-merge rather than a
placeholder hash, so leaving the row empty and letting it get filled in
post-merge would match existing convention.
**Net:** I'm aligned with SEZ9's conclusion — Issue 1 is the one true
blocker here (now with a concrete pattern to follow from
`connector-edge-socket`), Issues 3-5 are worth fixing in the same pass since
they touch the same small config/docs surface, and Issue 2's documentation ask
stands minus the parallelism sub-point. Once Issue 1 has an auth/allowlist
story (even a minimal shared-secret option), I don't see a remaining
source-level blocker from my side.
--
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]
