Skip to content

Commit 0964835

Browse files
mccullsmcculls
committed
Avoid merging in supplementary host/port settings when dealing with unix: URLs
1 parent 4b5cad9 commit 0964835

File tree

6 files changed

+83
-27
lines changed

6 files changed

+83
-27
lines changed

‎communication/src/main/java/datadog/communication/ddagent/SharedCommunicationObjects.java‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ public void createRemaining(Config config) {
2929
monitoring = Monitoring.DISABLED;
3030
}
3131
if (agentUrl == null) {
32-
agentUrl = HttpUrl.parse(config.getAgentUrl());
32+
agentUrl = parseAgentUrl(config);
3333
if (agentUrl == null) {
3434
throw new IllegalArgumentException("Bad agent URL: " + config.getAgentUrl());
3535
}
@@ -43,6 +43,15 @@ public void createRemaining(Config config) {
4343
}
4444
}
4545

46+
private static HttpUrl parseAgentUrl(Config config) {
47+
String agentUrl = config.getAgentUrl();
48+
if (agentUrl.startsWith("unix:")) {
49+
// provide placeholder agent URL, in practice we'll be tunnelling over UDS
50+
agentUrl = "http://" + config.getAgentHost() + ":" + config.getAgentPort();
51+
}
52+
return HttpUrl.parse(agentUrl);
53+
}
54+
4655
private static long getHttpClientTimeout(Config config) {
4756
if (!config.isCiVisibilityEnabled()) {
4857
return TimeUnit.SECONDS.toMillis(config.getAgentTimeout());

‎dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerAgent.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,9 @@ public static synchronized void run(
142142
private static String getDiagnosticEndpoint(
143143
Config config, DDAgentFeaturesDiscovery ddAgentFeaturesDiscovery) {
144144
if (ddAgentFeaturesDiscovery.supportsDebuggerDiagnostics()) {
145-
return config.getAgentUrl() + "/" + DDAgentFeaturesDiscovery.DEBUGGER_DIAGNOSTICS_ENDPOINT;
145+
return ddAgentFeaturesDiscovery
146+
.buildUrl(DDAgentFeaturesDiscovery.DEBUGGER_DIAGNOSTICS_ENDPOINT)
147+
.toString();
146148
}
147149
return config.getFinalDebuggerSnapshotUrl();
148150
}

‎dd-trace-core/src/main/java/datadog/trace/common/metrics/ConflatingMetricsAggregator.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ public ConflatingMetricsAggregator(
7171
sharedCommunicationObjects.featuresDiscovery(config),
7272
new OkHttpSink(
7373
sharedCommunicationObjects.okHttpClient,
74-
config.getAgentUrl(),
74+
sharedCommunicationObjects.agentUrl.toString(),
7575
V6_METRICS_ENDPOINT,
7676
config.isTracerMetricsBufferingEnabled(),
7777
false,

‎dd-trace-core/src/test/groovy/datadog/trace/common/metrics/MetricsAggregatorFactoryTest.groovy‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,24 +3,29 @@ package datadog.trace.common.metrics
33
import datadog.communication.ddagent.SharedCommunicationObjects
44
import datadog.trace.api.Config
55
import datadog.trace.test.util.DDSpecification
6+
import okhttp3.HttpUrl
67

78
class MetricsAggregatorFactoryTest extends DDSpecification {
89

910
def "when metrics disabled no-op aggregator created"() {
1011
setup:
1112
Config config = Mock(Config)
1213
config.isTracerMetricsEnabled() >> false
14+
def sco = Mock(SharedCommunicationObjects)
15+
sco.agentUrl = HttpUrl.parse("http://localhost:8126")
1316
expect:
14-
def aggregator = MetricsAggregatorFactory.createMetricsAggregator(config, Mock(SharedCommunicationObjects))
17+
def aggregator = MetricsAggregatorFactory.createMetricsAggregator(config, sco)
1518
assert aggregator instanceof NoOpMetricsAggregator
1619
}
1720

1821
def "when metrics enabled conflating aggregator created"() {
1922
setup:
2023
Config config = Spy(Config.get())
2124
config.isTracerMetricsEnabled() >> true
25+
def sco = Mock(SharedCommunicationObjects)
26+
sco.agentUrl = HttpUrl.parse("http://localhost:8126")
2227
expect:
23-
def aggregator = MetricsAggregatorFactory.createMetricsAggregator(config, Mock(SharedCommunicationObjects))
28+
def aggregator = MetricsAggregatorFactory.createMetricsAggregator(config, sco)
2429
assert aggregator instanceof ConflatingMetricsAggregator
2530
}
2631
}

‎internal-api/src/main/java/datadog/trace/api/Config.java‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1090,14 +1090,17 @@ private Config(final ConfigProvider configProvider, final InstrumenterConfig ins
10901090
}
10911091
}
10921092

1093-
if (agentHostFromEnvironment == null) {
1094-
agentHostFromEnvironment = configProvider.getString(AGENT_HOST);
1095-
rebuildAgentUrl = true;
1096-
}
1097-
1098-
if (agentPortFromEnvironment < 0) {
1099-
agentPortFromEnvironment = configProvider.getInteger(TRACE_AGENT_PORT, -1, AGENT_PORT_LEGACY);
1100-
rebuildAgentUrl = true;
1093+
// avoid merging in supplementary host/port settings when dealing with unix: URLs
1094+
if (unixSocketFromEnvironment == null) {
1095+
if (agentHostFromEnvironment == null) {
1096+
agentHostFromEnvironment = configProvider.getString(AGENT_HOST);
1097+
rebuildAgentUrl = true;
1098+
}
1099+
if (agentPortFromEnvironment < 0) {
1100+
agentPortFromEnvironment =
1101+
configProvider.getInteger(TRACE_AGENT_PORT, -1, AGENT_PORT_LEGACY);
1102+
rebuildAgentUrl = true;
1103+
}
11011104
}
11021105

11031106
if (agentHostFromEnvironment == null) {

‎internal-api/src/test/groovy/datadog/trace/api/ConfigTest.groovy‎

Lines changed: 51 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2036,8 +2036,45 @@ class ConfigTest extends DDSpecification {
20362036
!config.perfMetricsEnabled
20372037
}
20382038

2039-
def "trace_agent_url overrides either host and port or unix domain"() {
2039+
def "trace_agent_url overrides default host and port or unix domain"() {
20402040
setup:
2041+
if (configuredUrl != null) {
2042+
System.setProperty(PREFIX + TRACE_AGENT_URL, configuredUrl)
2043+
} else {
2044+
System.clearProperty(PREFIX + TRACE_AGENT_URL)
2045+
}
2046+
2047+
when:
2048+
def config = new Config()
2049+
2050+
then:
2051+
config.agentUrl == expectedUrl
2052+
config.agentHost == expectedHost
2053+
config.agentPort == expectedPort
2054+
config.agentUnixDomainSocket == expectedUnixDomainSocket
2055+
2056+
where:
2057+
// spotless:off
2058+
configuredUrl | expectedUrl | expectedHost | expectedPort | expectedUnixDomainSocket
2059+
null | "http://localhost:8126" | "localhost" | 8126 | null
2060+
"" | "http://localhost:8126" | "localhost" | 8126 | null
2061+
"http://localhost:1234" | "http://localhost:1234" | "localhost" | 1234 | null
2062+
"http://somehost" | "http://somehost:8126" | "somehost" | 8126 | null
2063+
"http://somehost:80" | "http://somehost:80" | "somehost" | 80 | null
2064+
"https://somehost:8143" | "https://somehost:8143" | "somehost" | 8143 | null
2065+
"unix:///another/socket/path" | "unix:///another/socket/path" | "localhost" | 8126 | "/another/socket/path"
2066+
"unix:///another%2Fsocket%2Fpath" | "unix:///another%2Fsocket%2Fpath" | "localhost" | 8126 | "/another/socket/path"
2067+
"http:" | "http://localhost:8126" | "localhost" | 8126 | null
2068+
"unix:" | "http://localhost:8126" | "localhost" | 8126 | null
2069+
"1234" | "http://localhost:8126" | "localhost" | 8126 | null
2070+
":1234" | "http://localhost:8126" | "localhost" | 8126 | null
2071+
// spotless:on
2072+
}
2073+
2074+
def "trace_agent_url overrides configured host and port or unix domain"() {
2075+
setup:
2076+
System.setProperty(PREFIX + AGENT_HOST, "test-host")
2077+
System.setProperty(PREFIX + TRACE_AGENT_PORT, "8888")
20412078
System.setProperty(PREFIX + AGENT_UNIX_DOMAIN_SOCKET, "/path/to/socket")
20422079
if (configuredUrl != null) {
20432080
System.setProperty(PREFIX + TRACE_AGENT_URL, configuredUrl)
@@ -2056,19 +2093,19 @@ class ConfigTest extends DDSpecification {
20562093

20572094
where:
20582095
// spotless:off
2059-
configuredUrl | expectedUrl | expectedHost | expectedPort | expectedUnixDomainSocket
2060-
null | "http://localhost:8126" | "localhost" | 8126 | "/path/to/socket"
2061-
"" | "http://localhost:8126" | "localhost" | 8126 | "/path/to/socket"
2062-
"http://localhost:1234" | "http://localhost:1234" | "localhost" | 1234 | "/path/to/socket"
2063-
"http://somehost" | "http://somehost:8126" | "somehost" | 8126 | "/path/to/socket"
2064-
"http://somehost:80" | "http://somehost:80" | "somehost" | 80 | "/path/to/socket"
2065-
"https://somehost:8143" | "https://somehost:8143" | "somehost" | 8143 | "/path/to/socket"
2066-
"unix:///another/socket/path" | "http://localhost:8126" | "localhost" | 8126 | "/another/socket/path"
2067-
"unix:///another%2Fsocket%2Fpath" | "http://localhost:8126" | "localhost" | 8126 | "/another/socket/path"
2068-
"http:" | "http://localhost:8126" | "localhost" | 8126 | "/path/to/socket"
2069-
"unix:" | "http://localhost:8126" | "localhost" | 8126 | "/path/to/socket"
2070-
"1234" | "http://localhost:8126" | "localhost" | 8126 | "/path/to/socket"
2071-
":1234" | "http://localhost:8126" | "localhost" | 8126 | "/path/to/socket"
2096+
configuredUrl | expectedUrl | expectedHost | expectedPort | expectedUnixDomainSocket
2097+
null | "http://test-host:8888" | "test-host" | 8888 | "/path/to/socket"
2098+
"" | "http://test-host:8888" | "test-host" | 8888 | "/path/to/socket"
2099+
"http://localhost:1234" | "http://localhost:1234" | "localhost" | 1234 | "/path/to/socket"
2100+
"http://somehost" | "http://somehost:8888" | "somehost" | 8888 | "/path/to/socket"
2101+
"http://somehost:80" | "http://somehost:80" | "somehost" | 80 | "/path/to/socket"
2102+
"https://somehost:8143" | "https://somehost:8143" | "somehost" | 8143 | "/path/to/socket"
2103+
"unix:///another/socket/path" | "unix:///another/socket/path" | "localhost" | 8126 | "/another/socket/path"
2104+
"unix:///another%2Fsocket%2Fpath" | "unix:///another%2Fsocket%2Fpath" | "localhost" | 8126 | "/another/socket/path"
2105+
"http:" | "http://test-host:8888" | "test-host" | 8888 | "/path/to/socket"
2106+
"unix:" | "http://test-host:8888" | "test-host" | 8888 | "/path/to/socket"
2107+
"1234" | "http://test-host:8888" | "test-host" | 8888 | "/path/to/socket"
2108+
":1234" | "http://test-host:8888" | "test-host" | 8888 | "/path/to/socket"
20722109
// spotless:on
20732110
}
20742111

0 commit comments

Comments
 (0)