Accept RFC 1123 hostnames in EndpointUtil.validateEndpoint - #8746
Accept RFC 1123 hostnames in EndpointUtil.validateEndpoint#8746kalayciburak wants to merge 1 commit into
Conversation
URI.getHost() returns null for some valid DNS names (JDK-8188305), such as a label that starts with a digit. Fall back to URL.getHost() so those endpoints are accepted, matching OtlpConfigUtil. Fixes open-telemetry#8745 Signed-off-by: Burak KALAYCI <kalayciburak1996@gmail.com>
|
|
Pull request dashboard statusWaiting on the author · refreshed 2026-09-08 22:52 UTC Respond to 1 review item (e.g. link a commit, explain why not, ask a follow-up):
Status above doesn't look right?
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69f1520108
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "Invalid endpoint, must start with http:// or https://: " + uri); | ||
| } | ||
| if (uri.getHost() == null) { | ||
| if (!hasHost(uri, endpoint)) { |
There was a problem hiding this comment.
When this fallback is taken, including for the three new test cases, the method still returns the original URI, whose getHost() is null and whose getPort() is -1. Supported consumers cannot use that representation: JdkHttpSender.java:198 passes it to HttpRequest.Builder.uri, which throws IllegalArgumentException: unsupported URI, while UpstreamGrpcSenderProvider.java:71 passes the null host and invalid port to ManagedChannelBuilder.forAddress. Consequently, these newly accepted endpoints still fail whenever the JDK HTTP or managed-channel gRPC sender is selected; the parsed host and port must be preserved or those consumers must use a parser that supports these names.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified, with nuance.
For "http://otlp.1234-k8s-namespace:4318", URI.getHost() returns null and URI.getPort() returns -1 (the authority is unparsed, so port information is also lost).
The default OkHttp sender is fine. OkHttpHttpSender constructs its request URL via HttpUrl.get(endpoint), which re-parses from uri.toString() using OkHttp's own URL parser. That parser handles RFC 1123 hostnames correctly and never calls uri.getHost(). Fixing validateEndpoint to accept these URIs is sufficient for the common case.
The JDK sender fails at runtime. JdkHttpSender passes the URI directly to HttpRequest.newBuilder().uri(endpoint), which internally calls uri.getHost() and throws:
java.lang.IllegalArgumentException: unsupported URI http://otlp.1234-k8s-namespace:4318
Similarly, UpstreamGrpcSenderProvider calls endpoint.getHost() and endpoint.getPort() directly, passing null and -1 to ManagedChannelBuilder.forAddress.
Fixing JdkHttpSender and UpstreamGrpcSenderProvider for these hostnames is non-trivial: there is no standard way to construct a java.net.URI whose getHost() returns an RFC 1123 hostname that the JDK parser rejects (URI is final and all its constructors run the same RFC 2396 validation). That is a separate problem from what this PR addresses. This PR is sufficient to unblock users of the default OkHttp sender.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (57.14%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #8746 +/- ##
============================================
- Coverage 91.29% 91.28% -0.01%
- Complexity 10498 10501 +3
============================================
Files 1006 1006
Lines 28338 28344 +6
Branches 3581 3582 +1
============================================
+ Hits 25870 25873 +3
- Misses 1675 1677 +2
- Partials 793 794 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @kalayciburak — just a friendly reminder that this pull request is waiting on you. The dashboard status comment has the open items and is kept current.
|
| /** | ||
| * {@link URI#getHost()} follows RFC 2396 and returns {@code null} for some valid DNS names | ||
| * (JDK-8188305), for example a label that starts with a digit. {@link URL#getHost()} accepts | ||
| * those names, matching {@code OtlpConfigUtil.validateEndpoint}. |
There was a problem hiding this comment.
Should amend this works for OkHttp sender but not for the others. For this reason, this does fix #8745. We'll want to amend the PR description accordingly, and keep that issue open for tracking.
There was a problem hiding this comment.
updated the PR body. OkHttp only, #8745 stays open for the JDK/gRPC cases.
2 participants
Description
EndpointUtil.validateEndpointrejects hostnames thatjava.net.URI#getHost()cannot parse even when they are valid DNS names. That is JDK-8188305: RFC 2396 host parsing returnsnullfor labels that start with a digit, which is common for Kubernetes namespaces such asotlp.1234-k8s-namespace.The existing
uri.getHost() == nullcheck was added in #8489 to reject truly host-less URIs (http:localhost:4317,https:/foo). That check is still needed, but it is too strict whengetHost()isnulland a host is present.This falls back to
URL.getHost(), the same parser used byOtlpConfigUtil.validateEndpoint. Host-less URIs still fail; RFC 1123 names thatURI.getHost()misses are accepted.This unblocks the default OkHttp sender (
OkHttpHttpSenderre-parses viaHttpUrl.get). It does not makeJdkHttpSenderorUpstreamGrpcSenderProviderwork: those still callURI.getHost()/URI.getPort(), which staynull/-1for these names. There is no standard way to build ajava.net.URIwhosegetHost()returns an RFC 1123 hostname the JDK parser rejects.Related to #8745. Leaving that issue open to track the JDK HTTP and managed-channel gRPC senders.
Testing done
./gradlew :exporters:common:test --tests io.opentelemetry.exporter.internal.EndpointUtilTest- 12 tests, 0 failedURI.getHost() == nullbefore the change./gradlew :exporters:common:check- passed (tests, checkstyle, spotless, japicmp, animalsniffer)