wy471x commented on PR #6456:
URL: https://github.com/apache/shenyu/pull/6456#issuecomment-5254765183
> 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]
