-
Notifications
You must be signed in to change notification settings - Fork 332
Add a jpms opener for HostNameResolver cache #11095
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.
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
Open
amarziali
wants to merge
2
commits into
master
Choose a base branch
from
andrea.marziali/jps-hostname
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9 changes: 9 additions & 0 deletions
9
...src/main/java/datadog/trace/bootstrap/instrumentation/java/net/JpmsInetAddressHelper.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| package datadog.trace.bootstrap.instrumentation.java.net; | ||
|
|
||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
|
|
||
| public class JpmsInetAddressHelper { | ||
| public static final AtomicBoolean OPENED = new AtomicBoolean(false); | ||
|
|
||
| private JpmsInetAddressHelper() {} | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
33 changes: 33 additions & 0 deletions
33
...rc/main/java/datadog/trace/instrumentation/httpclient/JpmsInetAddressInstrumentation.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| package datadog.trace.instrumentation.httpclient; | ||
|
|
||
| import static net.bytebuddy.matcher.ElementMatchers.isConstructor; | ||
|
|
||
| import com.google.auto.service.AutoService; | ||
| import datadog.environment.JavaVirtualMachine; | ||
| import datadog.trace.agent.tooling.Instrumenter; | ||
| import datadog.trace.agent.tooling.InstrumenterModule; | ||
|
|
||
| @AutoService(InstrumenterModule.class) | ||
| public class JpmsInetAddressInstrumentation extends InstrumenterModule.Tracing | ||
| implements Instrumenter.HasMethodAdvice, Instrumenter.ForSingleType { | ||
|
|
||
| public JpmsInetAddressInstrumentation() { | ||
| super("java-net"); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isEnabled() { | ||
| return super.isEnabled() && JavaVirtualMachine.isJavaVersionAtLeast(9); | ||
| } | ||
|
|
||
| @Override | ||
| public String instrumentedType() { | ||
| return "java.net.InetAddress"; | ||
| } | ||
|
|
||
| @Override | ||
| public void methodAdvice(MethodTransformer transformer) { | ||
| // it does not work with typeInitializer() | ||
| transformer.applyAdvice(isConstructor(), packageName + ".JpmsInetAddressClearanceAdvice"); | ||
| } | ||
| } |
21 changes: 21 additions & 0 deletions
21
.../main/java11/datadog/trace/instrumentation/httpclient/JpmsInetAddressClearanceAdvice.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| package datadog.trace.instrumentation.httpclient; | ||
|
|
||
| import datadog.trace.bootstrap.instrumentation.java.net.HostNameResolver; | ||
| import datadog.trace.bootstrap.instrumentation.java.net.JpmsInetAddressHelper; | ||
| import java.net.InetAddress; | ||
| import net.bytebuddy.asm.Advice; | ||
|
|
||
| public class JpmsInetAddressClearanceAdvice { | ||
| @Advice.OnMethodExit(suppress = Throwable.class) | ||
| public static void openOnReturn() { | ||
| if (JpmsInetAddressHelper.OPENED.compareAndSet(false, true)) { | ||
| // This call needs imperatively to be done from the same module we're adding opens to, | ||
| // because the JDK checks that the caller belongs to the same module. | ||
| // The code of this advice is inlined into the constructor of InetAddress (java.base), | ||
| // so it will work. Moving the same call to a helper class won't. | ||
| InetAddress.class.getModule().addOpens("java.net", HostNameResolver.class.getModule()); | ||
| // Now that java.net is open for deep reflection, initialize the HostNameResolver handles | ||
| HostNameResolver.tryInitialize(); | ||
| } | ||
| } | ||
| } |
40 changes: 40 additions & 0 deletions
40
.../groovy/datadog/trace/instrumentation/httpclient/JpmsInetAddressDisabledForkedTest.groovy
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| package datadog.trace.instrumentation.httpclient | ||
|
|
||
| import datadog.trace.agent.test.InstrumentationSpecification | ||
| import datadog.trace.bootstrap.instrumentation.java.net.HostNameResolver | ||
|
|
||
| class JpmsInetAddressDisabledForkedTest extends InstrumentationSpecification { | ||
|
|
||
| @Override | ||
| protected void configurePreAgent() { | ||
| super.configurePreAgent() | ||
| // Disable the JPMS instrumentation so java.net is NOT opened for deep reflection. | ||
| // HostNameResolver will be unable to bypass the IP→hostname cache and will fall back | ||
| // to the cache keyed by IP address. | ||
| injectSysConfig("dd.trace.java-net.enabled", "false") | ||
| } | ||
|
|
||
| /** | ||
| * Verifies the fallback behaviour when the JPMS instrumentation is disabled: | ||
| * HostNameResolver cannot reflectively read the pre-set hostname from InetAddress and | ||
| * falls back to a cache keyed by IP address. As a result, once a hostname has been | ||
| * cached for a given IP, every subsequent lookup for that IP returns the first cached | ||
| * value, even when the InetAddress object carries a different hostname. | ||
| * | ||
| * This is the broken behaviour that the JPMS instrumentation is designed to fix. | ||
| */ | ||
| def "without JPMS instrumentation, IP cache causes stale hostname to be returned"() { | ||
| given: | ||
| def ip = [192, 0, 2, 2] as byte[] // different subnet from the enabled-test to avoid cross-test cache pollution | ||
| def addr1 = InetAddress.getByAddress("service1.example.com", ip) | ||
| // Prime the IP→hostname cache with service1's hostname | ||
| HostNameResolver.hostName(addr1, "192.0.2.2") | ||
|
|
||
| when: "a second service with the same IP but a different hostname is resolved" | ||
| def addr2 = InetAddress.getByAddress("service2.example.com", ip) | ||
| def result = HostNameResolver.hostName(addr2, "192.0.2.2") | ||
|
|
||
| then: "the stale cached hostname of service1 is returned instead of service2's" | ||
| result == "service1.example.com" | ||
| } | ||
| } |
33 changes: 33 additions & 0 deletions
33
...src/test/groovy/datadog/trace/instrumentation/httpclient/JpmsInetAddressForkedTest.groovy
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| package datadog.trace.instrumentation.httpclient | ||
|
|
||
| import datadog.trace.agent.test.InstrumentationSpecification | ||
| import datadog.trace.bootstrap.instrumentation.java.net.HostNameResolver | ||
|
|
||
| class JpmsInetAddressForkedTest extends InstrumentationSpecification { | ||
|
|
||
| /** | ||
| * Verifies that the JPMS instrumentation opens java.base/java.net so that | ||
| * HostNameResolver can bypass its IP→hostname cache and return the correct | ||
| * peer.hostname even when multiple services share a single IP address | ||
| * (e.g. services behind a reverse proxy). | ||
| * | ||
| * Without the fix, HostNameResolver cannot reflectively access | ||
| * InetAddress$InetAddressHolder on Java 9+ and falls back to a cache keyed | ||
| * by IP, causing the first service's hostname to be returned for all | ||
| * subsequent services on the same IP. | ||
| */ | ||
| def "instrumentation opens java.net so hostname is resolved correctly when IP is shared"() { | ||
| given: | ||
| def ip = [192, 0, 2, 1] as byte[] // TEST-NET, will never appear in real DNS cache | ||
| def addr1 = InetAddress.getByAddress("service1.example.com", ip) | ||
| // Warm the IP→hostname cache with service1's hostname | ||
| HostNameResolver.hostName(addr1, "192.0.2.1") | ||
|
|
||
| when: "a second service with the same IP but different hostname is resolved" | ||
| def addr2 = InetAddress.getByAddress("service2.example.com", ip) | ||
| def result = HostNameResolver.hostName(addr2, "192.0.2.1") | ||
|
|
||
| then: "the hostname of addr2 is returned, not the cached hostname of addr1" | ||
| result == "service2.example.com" | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this get called continually if there's a genuine issue creating the handle? If so can we avoid repeated attempts from this code path? (I'd like to avoid a flood of exceptions being created in such a case)
Say maybe
InetAddressHoldergets renamed, or a different vendor implements it another way...