wy471x commented on PR #6456:
URL: https://github.com/apache/shenyu/pull/6456#issuecomment-5254849432
> 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.
Thank you for the detailed code review and constructive suggestions for
improvements.
Issues Fixed
1. Non-bracketed IPv6 silently misparsed (IpUtils.java)
- parseHostPort used lastIndexOf(':') for non-bracketed input. An
unbracketed IPv6 like ::1 produced garbage host : with port 1; 2001:db8::1:8080
coincidentally worked only when
the last segment was the port.
- Fix: After extracting host via lastColon, check host.indexOf(':') >= 0 —
if the host portion still contains colons, it's an unbracketed IPv6. Throw
IllegalArgumentException with
message: "IPv6 addresses must be bracketed, e.g. [::1]:8080".
2. matchByHostAndPort caused O(N×K) DB queries (CommonUpstreamUtils.java,
DiscoveryDataChangedEventSyncListener.java)
- Called per-upstream inside the handleAdded/Updated/Deleted loops, each
call doing mapper.selectByDiscoveryHandlerId(...) — a full-list query. For K
upstreams, that's K DB
round-trips.
- Fix: Added an overload matchByHostAndPort(List<DiscoveryUpstreamDO>,
String) that takes a pre-loaded in-memory list. The sync listener loads the
list lazily via a holder array —
only on the first iteration that actually needs the fallback match, and
reuses it for subsequent iterations. When all upstreams match exactly by URL,
zero extra queries are made.
3. One malformed URL aborted the entire sync batch
(DiscoveryDataChangedEventSyncListener.java)
- handleAdded only caught DuplicateKeyException, so
IllegalArgumentException from normalizeUrl propagated and killed the loop.
handleUpdated/Deleted caught Exception but re-threw,
also aborting.
- Fix: handleAdded now catches general Exception (logging and continuing).
handleUpdated and handleDeleted removed the throw e — log the error and
continue to the next upstream.
--
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]
