Aias00 commented on PR #6456:
URL: https://github.com/apache/shenyu/pull/6456#issuecomment-5193437020
The core bracket-IPv6 path is correct, and the layered defense (skip
malformed URLs in `DiscoveryTransfer.mapToCommonUpstream` + `Objects::nonNull`
filters in `UpstreamCheckService`) is good. A few things worth addressing:
**Non-bracketed IPv6 is silently misparsed.** `IpUtils.parseHostPort` falls
back to `lastIndexOf(':')` for non-bracketed input. That works for `host:port`
and IPv4, but for a non-bracketed IPv6 like `::1` (no port) it produces
`host=":"`, `port="1"`, and `2001:db8::1:8080` produces `host="2001:db8::1"`,
`port="8080"` (only coincidentally right when the last segment is the port).
There's no "more than one colon and not bracketed → reject" guard, so a user
who forgets brackets gets a silently broken upstream (garbage host) instead of
a clear error. Since `normalizeUrl` then feeds that garbage host into
`buildUrl`, you get `[:]`-style nonsense stored. Suggest: if a non-bracketed
URL has >1 colon, throw "IPv6 addresses must be bracketed, e.g. [::1]:8080" (or
parse it as IPv6+port). file: `IpUtils.parseHostPort`.
**`matchByHostAndPort` is O(N×K) DB load.** It's called per-upstream inside
the `handleAdded`/`handleUpdated`/`handleDeleted` loops, and each call does
`mapper.selectByDiscoveryHandlerId(...)` (full list for the handler). For a
sync batch of K upstreams that's K full-list queries. Fine for small handlers,
but it's a perf cliff for large deployments / bulk re-registration. Suggest
loading the handler's existing upstreams once per batch and passing the
in-memory list into the matcher.
**One malformed URL aborts the whole sync batch.** In `handleAdded`,
`normalizeUrl` throws `IllegalArgumentException` but the only catch is
`DuplicateKeyException`, so it propagates and aborts the ADDED loop. In
`handleUpdated`/`handleDeleted` the `catch (Exception e) { LOG.error(...);
throw e; }` logs then rethrows, also aborting the batch. For a discovery sync
listener, one bad URL shouldn't kill the entire batch. Suggest catching
`IllegalArgumentException` per-iteration, logging, and continuing.
Minor: `validateIPv6Address` calls `InetAddress.getByName` for bracketed
input — fine for IPv6 literals (no DNS), but a bracketed non-IPv6-with-colons
string triggers a DNS lookup before throwing. Slow rejection of invalid input,
not a correctness issue.
Overall the direction is right; the three items above are the ones I'd want
addressed before merge.
--
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]
