Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Expose DNS error codes through the UnknownHostException #13721

Merged
merged 2 commits into from
Jan 12, 2024
Merged

Expose DNS error codes through the UnknownHostException #13721

merged 2 commits into from
Jan 12, 2024

Conversation

tkountis
Copy link
Contributor

@tkountis tkountis commented Dec 8, 2023 •
edited

Motivation:

UnknownHostException are covering a broader range of discovery failures, where the API consumer has no visibility of the underlying reason.

Modification:

Expose initial-cause for UnknownHostExceptions to enrich the result.

Result:

The consumer now can decide whether the exception is due to failures or due an authoritative NXDOMAIN.

@tkountis tkountis marked this pull request as ready for review December 9, 2023 00:39
@tkountis tkountis changed the title [WIP] Expose DNS error code through the UnknownHostException when not NX Expose DNS error codes through the UnknownHostException Dec 9, 2023
@tkountis tkountis mentioned this pull request Dec 12, 2023
Merged
@normanmaurer
Copy link
Member

@tkountis I did retry this build a few times now and it always fails during dns tests… Can you have a look if this might break something ?

@normanmaurer normanmaurer added this to the 4.1.105.Final milestone Jan 12, 2024
@normanmaurer normanmaurer merged commit 47e1f43 into netty:4.1 Jan 12, 2024
15 checks passed
@normanmaurer
Copy link
Member

@tkountis thanks a lot!

normanmaurer pushed a commit that referenced this pull request Jan 12, 2024
@tkountis @normanmaurer
Expose DNS error codes through the UnknownHostException (#13721)
ae253d7 
Motivation:

UnknownHostException are covering a broader range of discovery failures,
where the API consumer has no visibility of the underlying reason.

Modification:

Expose initial-cause for UnknownHostExceptions to enrich the result.

Result:

The consumer now can decide whether the exception is due to failures or
due an authoritative NXDOMAIN.
franz1981 pushed a commit to franz1981/netty that referenced this pull request Feb 9, 2024
@tkountis @franz1981
Expose DNS error codes through the UnknownHostException (netty#13721)
a83c155 
Motivation:

UnknownHostException are covering a broader range of discovery failures,
where the API consumer has no visibility of the underlying reason.

Modification:

Expose initial-cause for UnknownHostExceptions to enrich the result. 

Result:

The consumer now can decide whether the exception is due to failures or
due an authoritative NXDOMAIN.
@tkountis tkountis mentioned this pull request Feb 17, 2024
Merged
normanmaurer added a commit that referenced this pull request Feb 21, 2024
@tkountis @normanmaurer
DNS resolver final CNAME lookup disabled (#13850)
8e41c9d 
Follow up of #13721

Motivation:

The previous effort introduced a root-cause for NXDOMAIN and SERVFAIL.
That was only for the authoritative part. The non-authoritative was
slightly different because it allowed a CNAME lookup regardless, for
legacy reasons. In this effort, I take another stab at it, by making the
additional CNAME lookup an opt-in feature. Justification included in the
patch.

Modification:

Last resort CNAME lookup while doing address resolutions, is now an
opt-in feature by default. The non-authoritative flow also includes the
root cause as part of the UnknownHostException.

---------

Co-authored-by: Norman Maurer <norman_maurer@apple.com>
normanmaurer added a commit that referenced this pull request Feb 21, 2024
@tkountis @normanmaurer
DNS resolver final CNAME lookup disabled (#13850)
822b42a 
Follow up of #13721

Motivation:

The previous effort introduced a root-cause for NXDOMAIN and SERVFAIL.
That was only for the authoritative part. The non-authoritative was
slightly different because it allowed a CNAME lookup regardless, for
legacy reasons. In this effort, I take another stab at it, by making the
additional CNAME lookup an opt-in feature. Justification included in the
patch.

Modification:

Last resort CNAME lookup while doing address resolutions, is now an
opt-in feature by default. The non-authoritative flow also includes the
root cause as part of the UnknownHostException.

---------

Co-authored-by: Norman Maurer <norman_maurer@apple.com>
@jrhee17 jrhee17 mentioned this pull request Apr 9, 2024
Merged
jrhee17 added a commit to line/armeria that referenced this pull request Apr 11, 2024
@jrhee17
Upgrade dependencies for 1.28.0 (#5584)
9bc1571 
**Dependencies**

- Brave 6.0.0 -> 6.0.2
- Brotli4j 1.15.0 -> 1.16.0
- java-control-plane 1.0.42 -> 1.0.44
- Dropwizard 2.1.10 -> 2.1.12
- Dropwizard Metrics 4.2.24 -> 4.2.25
- Eureka 2.0.1 -> 2.0.2
- fastUtil 8.5.12 -> 8.5.13
- gRPC-Java 1.61.0 -> 1.63.0
- Guava 33.0.0-jre -> 33.1.0-jre
- Jackson 2.16.1 -> 2.17.0
- JCTools 4.0.2 -> 4.0.3
- Jetty10 10.0.19 -> 10.0.20
- Jetty11 11.0.19 -> 11.0.20
- Jetty12 12.0.5 -> 12.0.8
- Jetty9.4 9.4.52.v20230823 -> 9.4.54.v20240208
- Kotlin 1.9.22 -> 1.9.23
- kotlinx.coroutines 1.9.22 -> 1.9.23
- Kubernetes Client 6.10.0 -> 6.11.0
- Micrometer 1.12.2 -> 1.12.4
- Micrometer Tracing 1.2.2 -> 1.2.4
- Netty 4.1.106.Final -> 4.1.108.Final
- Netty io_uring 0.0.24.Final -> 0.0.25.Final
- protobuf-jackson 2.2.0 -> 2.5.0
- Reactor 3.6.2 -> 3.6.4
- RESTEasy 5.0.7.Final -> 5.0.9.Final
- Retrofit2 2.9.0 -> 2.11.0
- Sangria 4.0.2 -> 4.1.0
- Scala2.12 2.12.18 -> 2.12.19
- Scala2.13 2.13.12 -> 2.13.13
- Scala3 3.3.0 -> 3.4.1
- Spring6 6.1.3 -> 6.1.5
- Spring-Boot3 3.2.2 -> 3.2.4
- Tomcat8 8.5.98 -> 8.5.100
- Tomcat8 9.0.85 -> 9.0.87
- Tomcat8 10.1.18 -> 10.1.20
- Build
  - Apache HttpClient 5.3 -> 5.3.1
  - asm 9.6 -> 9.7
  - AssertJ 3.25.2 -> 3.25.3
  - Awaitility 4.2.1 -> 4.2.1
  - dagger 2.50 -> 2.51.1
  - dgs 8.2.2 -> 8.5.3
  - Error Prone 2.24.1 -> 2.26.1
  - GAX Java 2.40.0 -> 2.46.1
  - Gradle 8.5 -> 8.7
  - J2ObjC 2.8 -> 3.0.0
  - Java-WebSocket 1.5.5 -> 1.5.6
  - JUnit5 5.10.1 -> 5.10.2
  - Kafka 3.6.1 -> 3.7.0
  - krotoDC 1.0.6 -> 1.1.1
  - Gradle Nexus Publish Plugin 7.0.1 -> 7.0.2
  - SLF4J2 2.0.11 -> 2.0.12
  - Testcontainers 1.19.3 -> 1.19.7
  - Zookeeper 3.9.1 -> 3.9.2

**Additional Modifications**
- `closeAndReleaseStagingRepository` has been renamed to
`closeAndReleaseStagingRepositories`
  - gradle-nexus/publish-plugin#236
- `DnsErrorCauseException` has been introduced in Netty
  - netty/netty#13721
  - `DnsUtil#isDnsQueryTimedOut` has been updated to reflect this
- `RefreshingAddressResolver` now uses `DnsUtil#isDnsQueryTimedOut` to
determine cachability for consistency
- Note that users should sync versions between netty and armeria for
this release. Otherwise, a compliation error may occur.
- `krotodc` now requires the `protobuf-java-util` dependency
  - mscheong01/krotoDC#25
- Sangria has a breaking change regarding `DeprecationTracker`
  - sangria-graphql/sangria#1064
tkountis added a commit to apple/servicetalk that referenced this pull request Apr 11, 2024
@tkountis
Handle DNS SERVFAIL differently than NXDOMAIN (#2776)
e605c87 
Motivation:
An UnknownHostException can have multiple root causes, each with a different application logic.
By far the most prominent is due to an NXDOMAIN from the nameserver, meaning that the domain in the query doesn't not exist. Other reasons could be a SERVFAIL or timeouts during resolutions. Due to insufficient information in the error itself, the dns-client has no obvious way of distinctly handling the various causes.

Modification:
Netty will expose that information as an init-cause. ST can leverage the enrichment to handle the different scenarios.

Results:
Better behavior.
To allow NXDOMAIN errors to invalidate DNS state, set the property `io.servicetalk.dns.discovery.nxdomain.invalidation` to true.

Depends on netty/netty#13721
Depends on netty/netty#13850
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bryce-anderson bryce-anderson bryce-anderson approved these changes

@Scottmitch Scottmitch Scottmitch approved these changes

@normanmaurer normanmaurer Awaiting requested review from normanmaurer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.1.105.Final
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tkountis @normanmaurer @bryce-anderson @Scottmitch