fix(feature-flags): add safe agentless EVP fallback #12299
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,13 +11,25 @@ | |
| import datadog.remoteconfig.DefaultConfigurationPoller; | ||
| import datadog.trace.api.Config; | ||
| import datadog.trace.api.civisibility.config.BazelMode; | ||
| import datadog.trace.util.AgentProxySelector; | ||
| import datadog.trace.util.AgentTaskScheduler; | ||
| import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; | ||
| import java.io.IOException; | ||
| import java.net.InetSocketAddress; | ||
| import java.net.Proxy; | ||
| import java.net.ProxySelector; | ||
| import java.net.SocketAddress; | ||
| import java.net.URI; | ||
| import java.security.Security; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.Locale; | ||
| import java.util.Set; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.function.Supplier; | ||
| import javax.annotation.Nullable; | ||
| import okhttp3.Credentials; | ||
| import okhttp3.HttpUrl; | ||
| import okhttp3.OkHttpClient; | ||
| import org.slf4j.Logger; | ||
|
|
@@ -44,6 +56,9 @@ public class SharedCommunicationObjects { | |
| */ | ||
| private volatile OkHttpClient intakeHttpClient; | ||
|
|
||
| private volatile HttpUrl intakeHttpsProxy; | ||
| private volatile Set<String> intakeNoProxyHosts = Collections.emptySet(); | ||
|
|
||
| @SuppressFBWarnings("PA_PUBLIC_PRIMITIVE_ATTRIBUTE") | ||
| public long httpClientTimeout; | ||
|
|
||
|
|
@@ -78,6 +93,8 @@ public void createRemaining(Config config) { | |
| : TimeUnit.SECONDS.toMillis(config.getAgentTimeout()); | ||
|
|
||
| forceClearTextHttpForIntakeClient = config.isForceClearTextHttpForIntakeClient(); | ||
| intakeHttpsProxy = parseHttpsProxy(config.getHttpsProxy()); | ||
| intakeNoProxyHosts = config.getNoProxyHosts(); | ||
|
|
||
| if (agentUrl == null) { | ||
| agentUrl = parseAgentUrl(config); | ||
|
|
@@ -269,11 +286,94 @@ public OkHttpClient getIntakeHttpClient() { | |
|
|
||
| synchronized (this) { | ||
| if (this.intakeHttpClient == null) { | ||
| OkHttpClient intakeClient = | ||
| OkHttpUtils.buildHttpClient( | ||
| forceClearTextHttpForIntakeClient, null, null, httpClientTimeout); | ||
| if (intakeHttpsProxy != null) { | ||
| final Proxy proxy = | ||
| new Proxy( | ||
| Proxy.Type.HTTP, | ||
| new InetSocketAddress(intakeHttpsProxy.host(), intakeHttpsProxy.port())); | ||
| final OkHttpClient.Builder builder = | ||
| intakeClient | ||
| .newBuilder() | ||
| .proxySelector(new IntakeProxySelector(proxy, intakeNoProxyHosts)); | ||
| if (!intakeHttpsProxy.username().isEmpty()) { | ||
| final String credential = | ||
| Credentials.basic(intakeHttpsProxy.username(), intakeHttpsProxy.password()); | ||
| builder.proxyAuthenticator( | ||
| (route, response) -> | ||
| response | ||
| .request() | ||
| .newBuilder() | ||
| .header("Proxy-Authorization", credential) | ||
| .build()); | ||
| } | ||
| intakeClient = builder.build(); | ||
| } | ||
| this.intakeHttpClient = intakeClient; | ||
| } | ||
| return this.intakeHttpClient; | ||
| } | ||
| } | ||
|
|
||
| @Nullable | ||
| static HttpUrl parseHttpsProxy(@Nullable final String configuredProxy) { | ||
| if (configuredProxy == null || configuredProxy.trim().isEmpty()) { | ||
| return null; | ||
| } | ||
| final String candidate = | ||
| configuredProxy.contains("://") ? configuredProxy : "http://" + configuredProxy; | ||
| final HttpUrl proxy = HttpUrl.parse(candidate); | ||
| if (proxy == null || !"http".equalsIgnoreCase(proxy.scheme())) { | ||
| log.warn("Ignoring invalid HTTPS proxy configuration"); | ||
|
There was a problem hiding this comment. 🤖 Claude working with Aaron: (nit) Falling back here is fine. Please make the warning say what was ignored and what happens next. An operator who sets an Two accuracy points for the new message. The value can arrive from if (proxy == null || !"http".equalsIgnoreCase(proxy.scheme())) {
// Only an http:// proxy URL is supported. Falling back is intentional; the
// client keeps the default selector, which may still apply a JVM proxy.
log.warn(
"Ignoring configured HTTPS proxy: only an http:// proxy URL is supported. "
+ "Intake traffic will fall back to the default proxy selection.");
return null;
}Keeping the configured value out of the message is right, since it can contain credentials. Worth a short comment saying so, to stop someone adding it back for debuggability. There was a problem hiding this comment. 🤖 From Codex working with Aaron: The warning should be clearer, but the suggested text is slightly inaccurate because |
||
| return null; | ||
| } | ||
| return proxy; | ||
| } | ||
|
|
||
| static final class IntakeProxySelector extends ProxySelector { | ||
| private static final List<Proxy> DIRECT = Collections.singletonList(Proxy.NO_PROXY); | ||
|
|
||
| private final Proxy proxy; | ||
| private final Set<String> noProxyHosts; | ||
|
|
||
| IntakeProxySelector(final Proxy proxy, final Set<String> noProxyHosts) { | ||
| this.proxy = proxy; | ||
| this.noProxyHosts = noProxyHosts; | ||
| } | ||
|
|
||
| @Override | ||
| public List<Proxy> select(final URI uri) { | ||
| final String host = uri.getHost(); | ||
| if (host != null && shouldBypassProxy(host)) { | ||
| return DIRECT; | ||
| } | ||
| if ("https".equalsIgnoreCase(uri.getScheme())) { | ||
| return Collections.singletonList(proxy); | ||
| } | ||
| return AgentProxySelector.INSTANCE.select(uri); | ||
| } | ||
|
|
||
| @Override | ||
| public void connectFailed( | ||
| final URI uri, final SocketAddress address, final IOException failure) { | ||
| AgentProxySelector.INSTANCE.connectFailed(uri, address, failure); | ||
|
There was a problem hiding this comment. 🤖 Claude working with Aaron: (nit) This reports our own proxy's failure to a selector that never returned it. Please make that explicit. The failed address comes from @Override
public void connectFailed(
final URI uri, final SocketAddress address, final IOException failure) {
if (address.equals(proxy.address())) {
// This proxy came from our own configuration, not from the default
// selector, so reporting the failure there would be meaningless.
log.debug("Intake proxy connection failed for {}", uri, failure);
return;
}
AgentProxySelector.INSTANCE.connectFailed(uri, address, failure);
} |
||
| } | ||
|
|
||
| private boolean shouldBypassProxy(final String host) { | ||
|
There was a problem hiding this comment. 🤖 Claude working with Aaron: Please move this matcher into one shared helper and have One config value now behaves two ways in one JVM. I ran both against We would like the case-insensitive behaviour to win, so please conform the agent selector to it. // datadog.trace.util.NoProxyHosts (new) - one matcher, used by both selectors
public static boolean matches(final String host, final Set<String> noProxyHosts) {
// body moved verbatim from shouldBypassProxy
}
// AgentProxySelector.select
if (uri.getHost() != null && NoProxyHosts.matches(uri.getHost(), noProxyHosts)) {
return DIRECT;
}
return defaultProxySelector.select(uri);One shared helper is also the natural home for the bare-domain suffix fix that both bots raised on line 372. To be explicit about the risk: conforming makes the agent selector bypass the proxy more often than it does today. For someone who relies on a JVM-configured proxy to reach the Agent, that means new direct egress. It is a deliberate compatibility change, not a free one, so it belongs in the release notes. There was a problem hiding this comment. 🤖 From Codex working with Aaron: The divergence is real, but changing |
||
| final String normalizedHost = host.toLowerCase(Locale.ROOT); | ||
| for (final String configuredHost : noProxyHosts) { | ||
| final String normalized = configuredHost.trim().toLowerCase(Locale.ROOT); | ||
|
There was a problem hiding this comment. 🤖 Claude working with Aaron: (nit) Please normalize the no-proxy set once, in the constructor.
Both halves of the change, since normalizing in the constructor only helps if the loop stops re-normalizing: // needs java.util.HashSet
IntakeProxySelector(final Proxy proxy, final Set<String> noProxyHosts) {
this.proxy = proxy;
final Set<String> normalized = new HashSet<>(noProxyHosts.size());
for (final String host : noProxyHosts) {
normalized.add(host.trim().toLowerCase(Locale.ROOT));
}
this.noProxyHosts = normalized;
}
private boolean shouldBypassProxy(final String host) {
final String normalizedHost = host.toLowerCase(Locale.ROOT);
for (final String normalized : noProxyHosts) { // already normalized
if ("*".equals(normalized)
|| normalizedHost.equals(normalized)
|| (normalized.startsWith(".")
&& (normalizedHost.equals(normalized.substring(1))
|| normalizedHost.endsWith(normalized)))) {
return true;
}
}
return false;
}There was a problem hiding this comment. 🤖 From Codex working with Aaron: The repeated normalization is real, but it runs per outbound batch, not per span, and the list is normally small. Keep this as an optional micro-optimization, not requested review work. It becomes reasonable if a shared matcher is introduced separately. |
||
| if ("*".equals(normalized) | ||
| || normalizedHost.equals(normalized) | ||
| || (normalized.startsWith(".") | ||
| && (normalizedHost.equals(normalized.substring(1)) | ||
| || normalizedHost.endsWith(normalized)))) { | ||
|
Comment on lines
+369
to
+372
There was a problem hiding this comment.
When a standard domain entry such as Useful? React with 👍 / 👎. There was a problem hiding this comment. 🤖 From Codex working with Aaron: This is correct only if this PR keeps standard There was a problem hiding this comment.
Direct Feature Flags intake can fail when the proxy blocks Datadog traffic. Assertion details
Was this helpful? React 👍 or 👎 |
||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
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.
🤖 Claude working with Aaron:
Two separate points: the second flag is inert, and the HTTPS test you would want is cheap to add.
followSslRedirects(false)is inert.followRedirects(false)already blocks every redirect, including scheme-changing ones, so OkHttp never consults it. I proved it by flipping the second flag totrueand re-runningBackendApiFactoryTest: still green. Either drop it or say why it stays.An HTTPS
MockWebServercase is worth adding. The current test redirects plaintext to plaintext, but production intake ishttps://.okhttp-tlsis published at the same3.12.12you already pin formockwebserver, and it shipsHandshakeCertificates, so this is one catalog entry plus a regeneration ofcommunication/gradle.lockfile.One caveat so this is not oversold: the HTTPS case will not turn red if
followSslRedirects(false)is removed, becausefollowRedirects(false)covers it. Add it for realistic transport coverage, not as justification for the flag.The redirect rejection itself is correctly enforced. Flipping
followRedirectstotrueturns the test red on all four status codes.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.
🤖 From Codex working with Aaron: The
followSslRedirectspoint is correct, but the proposed TLS setup will fail.heldCertificate(...)sets server identity; it does not make the client trust the self-signed certificate, which also needs alocalhostSAN. Use separate server/clientHandshakeCertificates,addSubjectAlternativeName("localhost"), andclient.addTrustedCertificate(serverCertificate.certificate()). The HTTPS test is optional becausefollowRedirects(false)already covers the behavior.