fix: support IPv6 address parsing in upstream URL handling

[wy471x]

Replace raw split(":") with IPv6-aware host:port parsing using bracket
notation detection and java.net.URI to avoid breaking IPv6 addresses
like [2001:db8::1]:8080 in health checks, discovery, and API doc loading.

Make sure that:

• You have read the contribution guidelines.
• You submit test cases (unit or integration tests) that back your changes.
• Your local test passed ./mvnw clean install -Dmaven.javadoc.skip=true.

Summary of All Changes

Problem

Four files parsed upstream URLs/host:port strings by splitting on :. This broke
IPv6 addresses like [2001:db8::1]:8080 — internal colons caused host truncation,
wrong port parsing, or NumberFormatException.

Files Changed

Core Logic — shenyu-common

IpUtils.java — New parseHostPort() method that correctly handles:

• IPv4 with/without port (192.168.1.1:8080, 192.168.1.1)
• IPv6 bracket notation with/without port ([::1]:9090, [2001:db8::1])
• IPv6 zone IDs ([fe80::1%eth0]:8080) — stripped before validation to avoid platform-dependent UnknownHostException
• Hostnames with/without port (example.com:8080, example.com)
• Null/blank input → IllegalArgumentException
• Non-numeric port → IllegalArgumentException
• Port out of range (0 or >65535) → IllegalArgumentException
• Trailing junk after ] → IllegalArgumentException
• Empty port defaults to "80"

UpstreamCheckUtils.java — Adapted to use the new parseHostPort().

Wrapper & Callers — shenyu-admin

CommonUpstreamUtils.java — Two new utility methods:

• normalizeUrl(url) — Normalizes upstream URLs by adding default port 80 when missing, and wrapping bare IPv6 addresses in brackets
• parseHostPort(url) — Delegates to IpUtils.parseHostPort()

DiscoveryTransfer.java — Uses normalizeUrl() when converting discovery upstream DTOs, plus null/blank guards on URL and properties.

DiscoveryUpstreamServiceImpl.java — Transaction atomicity (@transactional), host:port fallback logic, normalization on both create and update paths.

LoadServiceDocEntryImpl.java — Adapted to use new parseHostPort().

UpstreamCheckService.java — Adapted to use new parseHostPort().

DiscoveryDataChangedEventSyncListener.java — Null guard on discoveryUpstreams list.

Notes

Scope: shenyu-admin and shenyu-common only.
Follow-up: Gateway-side consumers (grpc, tars) to be handled in subsequent work.

close #6442

[Copilot]

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Adds IPv6-safe upstream URL parsing to avoid breaking bracketed IPv6 host/port strings (e.g., [2001:db8::1]:8080) across health checks, discovery mapping, and service-doc loading.

Changes:

• Introduces a shared parseHostPort utility (IPv6 bracket notation support) and wires it into call sites that previously used split(":").
• Updates upstream URL checking to rely on java.net.URI for http(s)://... inputs and the new host/port parser for bare host:port inputs.
• Adds tests covering IPv6 parsing and IPv6 URL formatting/bracketing.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
shenyu-common/src/test/java/org/apache/shenyu/common/utils/UpstreamCheckUtilsTest.java	Adds regression tests ensuring IPv6 inputs don’t throw during URL checking.
shenyu-common/src/main/java/org/apache/shenyu/common/utils/UpstreamCheckUtils.java	Uses URI for http(s) URLs and delegates bare host:port parsing to IpUtils.parseHostPort.
shenyu-common/src/main/java/org/apache/shenyu/common/utils/IpUtils.java	Adds parseHostPort with IPv6 bracket notation handling and default port behavior.
shenyu-admin/src/test/java/org/apache/shenyu/admin/utils/CommonUpstreamUtilsTest.java	Adds tests for parsing host/port (IPv4/IPv6/hostname) and IPv6 bracketing in buildUrl.
shenyu-admin/src/main/java/org/apache/shenyu/admin/utils/CommonUpstreamUtils.java	Brackets IPv6 in buildUrl and delegates parseHostPort to IpUtils.
shenyu-admin/src/main/java/org/apache/shenyu/admin/transfer/DiscoveryTransfer.java	Replaces split(":") parsing with parseHostPort and simplifies upstream mapping.
shenyu-admin/src/main/java/org/apache/shenyu/admin/service/manager/impl/LoadServiceDocEntryImpl.java	Replaces split(":") parsing with parseHostPort when building UpstreamInstance.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Aias00]

I found a compatibility issue in the IPv6 URL handling.

This PR changes the canonical serialized IPv6 upstream URL from the old bare form (host:port) to bracketed form ([host]:port) in CommonUpstreamUtils.buildUrl(...). However, discovery persistence, dedupe, status updates, and deletes still use exact upstream_url string matching.

That means an existing persisted IPv6 upstream written by older code, for example 2001:db8::1:8080, will not match the new [2001:db8::1]:8080 value after upgrade. nativeCreateOrUpdate(...) can insert a duplicate instead of updating the old row, and status/delete paths can leave the old row stale because the SQL and in-memory comparisons still key on exact upstream_url.

There are a few parser edge cases worth covering at the same time:

• Bare IPv6 without brackets, e.g. 2001:db8::1, is split on the last colon and becomes host 2001:db8: + port 1.
• Already bracketed host input passed to buildUrl, e.g. [2001:db8::1], becomes [[2001:db8::1]]:8080.
• Bracketed malformed input like [2001:db8::1 throws a low-level StringIndexOutOfBoundsException, while [2001:db8::1]junk is silently accepted as port 80.

Could we add an upgrade-safe canonicalization strategy here, either by migrating old upstream_url values to the new bracketed form or by making lookup/update/delete/dedupe tolerant of both legacy and canonical forms until data is normalized? Tests for legacy bare IPv6 rows plus the malformed parser cases would catch the risky paths.

[wy471x]

I found a compatibility issue in the IPv6 URL handling.

This PR changes the canonical serialized IPv6 upstream URL from the old bare form (host:port) to bracketed form ([host]:port) in CommonUpstreamUtils.buildUrl(...). However, discovery persistence, dedupe, status updates, and deletes still use exact upstream_url string matching.

That means an existing persisted IPv6 upstream written by older code, for example 2001:db8::1:8080, will not match the new [2001:db8::1]:8080 value after upgrade. nativeCreateOrUpdate(...) can insert a duplicate instead of updating the old row, and status/delete paths can leave the old row stale because the SQL and in-memory comparisons still key on exact upstream_url.

There are a few parser edge cases worth covering at the same time:

• Bare IPv6 without brackets, e.g. 2001:db8::1, is split on the last colon and becomes host 2001:db8: + port 1.
• Already bracketed host input passed to buildUrl, e.g. [2001:db8::1], becomes [[2001:db8::1]]:8080.
• Bracketed malformed input like [2001:db8::1 throws a low-level StringIndexOutOfBoundsException, while [2001:db8::1]junk is silently accepted as port 80.

Could we add an upgrade-safe canonicalization strategy here, either by migrating old upstream_url values to the new bracketed form or by making lookup/update/delete/dedupe tolerant of both legacy and canonical forms until data is normalized? Tests for legacy bare IPv6 rows plus the malformed parser cases would catch the risky paths.

@Aias00 Hi, I have fixed the issues you mentioned. Please take a look when you have time.

[Aias00]

Reviewed #6456 — IPv6 address parsing in upstream URL handling.

Thanks for this — the IPv6-aware parsing in IpUtils.parseHostPort and the URI-based checkUrl rewrite look correct (verified URI.getHost() returns the bracketed IPv6 form and InetSocketAddress accepts it). But there's a blocking issue plus a few consistency gaps to address before merge.

Blocker — CI is red (NPE regression in nativeCreateOrUpdate)

DiscoveryUpstreamServiceImpl.nativeCreateOrUpdate now calls CommonUpstreamUtils.normalizeUrl(...) unconditionally, and normalizeUrl → IpUtils.parseHostPort does upstreamUrl.startsWith("[") with no null guard. The existing DiscoveryUpstreamServiceTest.testNativeCreate builds a DTO with no URL set, so the PR now throws:

java.lang.NullPointerException: Cannot invoke "String.startsWith(String)" because "upstreamUrl" is null
  at DiscoveryUpstreamServiceTest.testNativeCreate(DiscoveryUpstreamServiceTest.java:205)
  at DiscoveryUpstreamServiceTest.testNativeCreateOrUpdate(DiscoveryUpstreamServiceTest.java:127)

This is the actual cause of the build (17, windows-latest) FAILURE (ci workflow); the ubuntu matrix was cancelled by fail-fast. mergeStateStatus is BLOCKED. Beyond the test, it's a runtime regression: master inserted the row (with null url) without throwing, the PR now 500s. The new DiscoveryUpstreamServiceImplTest always sets a URL via buildDTO, so it masks the regression; the pre-existing DiscoveryUpstreamServiceTest was not updated.

Fix options: null/blank-guard IpUtils.parseHostPort (or normalizeUrl), and add the same exception guard to the service path that you already added to the listener's handleAdded/Updated/Deleted. Then fix testNativeCreate to set a URL (or assert the new typed exception).

Should-fix

1. Listener swallows exceptions inside @Transactional. handleUpdated/handleDeleted catch Exception, log, and continue, but onChange is @Transactional(rollbackFor=Exception.class) — so partial batches now commit instead of rolling back (master propagated → rollback). Either rethrow after logging to keep atomicity, or drop the @Transactional and document per-item best-effort; don't swallow-and-commit silently.

2. Service-layer write paths lack the host:port fallback. deleteBySelectorIdAndUrl and changeStatusBySelectorIdAndUrl only exact-match the normalized URL; the listener additionally falls back to matchByHostAndPort for old-format records. So old example.com (no port) or unbracketed 2001:db8::1:8080 rows can't be deleted/status-toggled via the register/REST path until migrated, while the event path can. Reuse matchByHostAndPort in both service methods (delete/update by id) so all write paths match.

3. mapToCommonUpstream is now throwable in unguarded streams. Replacing url.split(":")[0] with parseHostPort(url)[0] means malformed bracketed input ([broken, [], [localhost]) throws IllegalArgumentException. Callers in UpstreamCheckService (lines 482-484, 509) map with no try/catch — one bad row breaks the whole DIVIDE stream. Skip-and-log per item, or return null on parse failure and filter.

Nits

• normalizeUrl does Integer.parseInt(parts[1]) but parseHostPort doesn't validate the port token is numeric (example.com:abc → uncaught NumberFormatException in the service path). Validate the port in parseHostPort or guard callers.
• IPv6 coverage stops at admin/common. ShenyuResolverHelper (grpc) and PrxInfoUtil (tars) still split(":") and will NumberFormatException on [ipv6]:port. Pre-existing (unbracketed form was already broken), but please scope the PR description to admin/common, or open a follow-up for the gateway-side consumers.

Tests: the new IpUtilsTest / CommonUpstreamUtilsTest cases are good and cover IPv4/IPv6/hostname/bracket edge cases. Please also add round-trip normalizeUrl tests for hostname-without-port and IPv6-with-zone. And re-run CI with fail-fast: false so the ubuntu matrix actually executes.

[Aias00]

Follow-up after the new commits (d05fec8 + f0a26bf). Thanks — the substantive issues are addressed. Per-finding:

• #1 blocker (null-url NPE in nativeCreateOrUpdate + CI red): fixed. IpUtils.parseHostPort and CommonUpstreamUtils.normalizeUrl now throw IllegalArgumentException("Invalid upstream URL, null or blank") on blank/null, and testNativeCreate now sets a URL. The originally-red k8s-examples-http is passing again. Verified in commit f0a26bf.
• #2 listener swallowed exceptions inside @Transactional: fixed. handleUpdated/handleDeleted now throw e after logging (commit d05fec8), so onChange rolls back on failure — atomicity restored.
• #3 service write paths lacked host:port fallback: fixed. deleteBySelectorIdAndUrl and changeStatusBySelectorIdAndUrl now check effect == 0 and fall back to matchByHostAndPort + delete/update by id, mirroring the listener path (d05fec8).
• #4 mapToCommonUpstream throwable in unguarded streams: fixed. mapToCommonUpstream and mapToDiscoveryUpstreamData now wrap parseHostPort in try/catch, return null + log warn on IllegalArgumentException, and all three stream call sites (updateSelectorHandler, fetchUpstreamData, onDiscoveryUpstreamUpdated) .filter(Objects::nonNull). mapToDiscoveryUpstreamData's null-url path now returns null instead of throwing (d05fec8).
• #5 non-numeric port NumberFormatException: partially fixed. Blank/null is now a typed IllegalArgumentException. A non-numeric port (example.com:abc) still makes normalizeUrl's Integer.parseInt(parts[1]) throw NumberFormatException. In the transfer/stream paths that's now harmless (NFE is a subclass of IAE, caught and skipped), but the direct normalizeUrl call in nativeCreateOrUpdate still surfaces an untyped NFE → 500. Minor — consider validating the port token in parseHostPort (throw typed IAE) so the service path fails cleanly too. Not a blocker.
• #6 IPv6 coverage stops at admin/common (ShenyuResolverHelper/PrxInfoUtil still split(":")): not addressed. Out of scope for this PR as flagged; please open a follow-up for the grpc/tars gateway-side consumers, or note in the PR description that this PR scopes to admin/common.

One thing to look at before merge: the build matrix is currently red on all OS jobs (windows + ubuntu 17/19/20/21), but it's not a code failure — the log shows ##[error]The operation was canceled. ~7 min in, with every surefire suite passing (Tests run: X, Failures: 0, Errors: 0) and no compile errors. It looks like a job timeout/cancellation, not a regression from these commits. Please re-run CI; if it reproduces, the build workflow's timeout-minutes may need bumping (the reactor is large and shared runners are slow). Once build is green I'm happy to approve — no code blockers remain.

[Aias00]

Correction to my previous comment — I was wrong that the build red was a timeout. I dug into the per-job conclusions of run 30756841941 and the real failure is on the Windows job; the four ubuntu jobs were cancelled by fail-fast (the build matrix uses the default fail-fast: true), which is why their logs end with The operation was canceled. mid-build. So the red is real test failures in this PR's own code, not an infra timeout. Please ignore my earlier "re-run and approve" line — re-running will fail the same way until these are fixed.

The Windows job (build (17, windows-latest)) ends with BUILD FAILURE — Tests run: 1474, Failures: 0, Errors: 3. The three errors:

1. DiscoveryUpstreamServiceTest.testNativeCreateOrUpdate:128 -> testNativeUpdate:200 — java.lang.IllegalArgumentException: Invalid upstream URL, null or blank.
   Your f0a26bf blank-guard in parseHostPort/normalizeUrl throws on a null/blank URL, and you fixed testNativeCreate to set 192.168.1.1:8080 — but testNativeUpdate (also called by testNativeCreateOrUpdate at line 128) still builds a DTO with no URL set, so the new guard throws. Fix: set a URL in testNativeUpdate too (the blank-guard is the right behavior; the test was relying on the old null-tolerant path).

2. CommonUpstreamUtilsTest.testNormalizeUrlIpv6WithZoneIdAndPortRoundTrip:506 and
   testNormalizeUrlIpv6WithZoneIdWithoutPortRoundTrip:515 — java.lang.IllegalArgumentException: Invalid IPv6 address: fe80::1%lo0, Caused by: java.net.UnknownHostException: no such interface lo0.
   These new tests use fe80::1%lo0, where lo0 is the macOS loopback interface name. Linux and Windows CI runners have no lo0 interface, so the zone-id resolution fails and the test errors. So these tests are platform-dependent and break on every non-macOS runner (they'd fail on Linux too — windows just happened to reach the module first).

This points at a product gap, not just a test bug: parseHostPort resolves the IPv6 zone id to a real network interface, so on a Linux/Windows gateway an upstream URL containing a link-local IPv6 with a zone id (e.g. fe80::1%eth0) will fail to parse at runtime. For an IPv6-support PR, link-local + zone ids being unsupported on the production platform is a real correctness gap — please either parse the zone id without resolving it to an interface (treat it as an opaque scope id), or explicitly document that link-local zone-id upstreams are unsupported and skip these tests on CI rather than hardcoding a macOS interface name.

So: the blocker stands (CI red, 3 real failures). Keeping changes-requested. Once testNativeUpdate sets a URL and the zone-id case is either implemented without interface resolution or the tests are made platform-agnostic, build should go green.

[wy471x]

Follow-up after the new commits (d05fec8 + f0a26bf). Thanks — the substantive issues are addressed. Per-finding:

• Soul Admin Service #1 blocker (null-url NPE in nativeCreateOrUpdate + CI red): fixed. IpUtils.parseHostPort and CommonUpstreamUtils.normalizeUrl now throw IllegalArgumentException("Invalid upstream URL, null or blank") on blank/null, and testNativeCreate now sets a URL. The originally-red k8s-examples-http is passing again. Verified in commit f0a26bf.
• Dev jiangxiaofeng #2 listener swallowed exceptions inside @Transactional: fixed. handleUpdated/handleDeleted now throw e after logging (commit d05fec8), so onChange rolls back on failure — atomicity restored.
• Dev #3 service write paths lacked host:port fallback: fixed. deleteBySelectorIdAndUrl and changeStatusBySelectorIdAndUrl now check effect == 0 and fall back to matchByHostAndPort + delete/update by id, mirroring the listener path (d05fec8).
• Soul Check Style #4 mapToCommonUpstream throwable in unguarded streams: fixed. mapToCommonUpstream and mapToDiscoveryUpstreamData now wrap parseHostPort in try/catch, return null + log warn on IllegalArgumentException, and all three stream call sites (updateSelectorHandler, fetchUpstreamData, onDiscoveryUpstreamUpdated) .filter(Objects::nonNull). mapToDiscoveryUpstreamData's null-url path now returns null instead of throwing (d05fec8).
• Soul Admin Interface #5 non-numeric port NumberFormatException: partially fixed. Blank/null is now a typed IllegalArgumentException. A non-numeric port (example.com:abc) still makes normalizeUrl's Integer.parseInt(parts[1]) throw NumberFormatException. In the transfer/stream paths that's now harmless (NFE is a subclass of IAE, caught and skipped), but the direct normalizeUrl call in nativeCreateOrUpdate still surfaces an untyped NFE → 500. Minor — consider validating the port token in parseHostPort (throw typed IAE) so the service path fails cleanly too. Not a blocker.
• Dev jiangxiaofeng #6 IPv6 coverage stops at admin/common (ShenyuResolverHelper/PrxInfoUtil still split(":")): not addressed. Out of scope for this PR as flagged; please open a follow-up for the grpc/tars gateway-side consumers, or note in the PR description that this PR scopes to admin/common.

One thing to look at before merge: the build matrix is currently red on all OS jobs (windows + ubuntu 17/19/20/21), but it's not a code failure — the log shows ##[error]The operation was canceled. ~7 min in, with every surefire suite passing (Tests run: X, Failures: 0, Errors: 0) and no compile errors. It looks like a job timeout/cancellation, not a regression from these commits. Please re-run CI; if it reproduces, the build workflow's timeout-minutes may need bumping (the reactor is large and shared runners are slow). Once build is green I'm happy to approve — no code blockers remain.

Hi,I have fixed the issues you mentioned. Please take a look when you have time.

[Aias00]

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.