diff --git a/communication/src/main/java/datadog/communication/BackendApiFactory.java b/communication/src/main/java/datadog/communication/BackendApiFactory.java index c1b471ebfc8..eeb960b3a70 100644 --- a/communication/src/main/java/datadog/communication/BackendApiFactory.java +++ b/communication/src/main/java/datadog/communication/BackendApiFactory.java @@ -1,5 +1,7 @@ package datadog.communication; +import static java.util.Collections.emptyList; + import datadog.communication.ddagent.DDAgentFeaturesDiscovery; import datadog.communication.ddagent.SharedCommunicationObjects; import datadog.communication.http.HttpRetryPolicy; @@ -15,13 +17,30 @@ public class BackendApiFactory { private static final Logger log = LoggerFactory.getLogger(BackendApiFactory.class); + private static final int MAX_DNS_LABEL_LENGTH = 63; + private static final int MAX_DNS_HOST_LENGTH = 253; private final Config config; private final SharedCommunicationObjects sharedCommunicationObjects; + private final boolean sendOnce; public BackendApiFactory(Config config, SharedCommunicationObjects sharedCommunicationObjects) { + this(config, sharedCommunicationObjects, false); + } + + /** + * Creates a backend factory with optional send-once transport semantics. + * + *

When {@code sendOnce} is true, automatic connection retries and redirects are disabled, and + * the default HTTP retry policy is {@link HttpRetryPolicy.Factory#NEVER_RETRY}. Explicit caller + * policies may retry proven pre-send failures, but must not replay an ambiguously accepted event + * payload without an idempotency key. + */ + public BackendApiFactory( + Config config, SharedCommunicationObjects sharedCommunicationObjects, boolean sendOnce) { this.config = config; this.sharedCommunicationObjects = sharedCommunicationObjects; + this.sendOnce = sendOnce; } public @Nullable BackendApi createBackendApi(Intake intake) { @@ -67,7 +86,9 @@ public BackendApi createDirectIntakeApi( apiKey, traceId, retryPolicyFactory(), - directIntakeHttpClient(sharedCommunicationObjects.getIntakeHttpClient(), followRedirects), + configureHttpClient( + directIntakeHttpClient( + sharedCommunicationObjects.getIntakeHttpClient(), followRedirects)), responseCompression); } @@ -87,11 +108,14 @@ private static HttpUrl buildDirectIntakeUrl(Intake intake, Config config) { } static HttpUrl buildEventPlatformIntakeUrl(String site) { - if (site == null || site.isEmpty()) { + if (!isValidDnsSuffix(site)) { throw new IllegalArgumentException("Invalid Datadog site"); } String expectedHost = Intake.EVENT_PLATFORM.getUrlPrefix() + "." + site; + if (expectedHost.length() > MAX_DNS_HOST_LENGTH) { + throw new IllegalArgumentException("Invalid Datadog site"); + } HttpUrl url = new HttpUrl.Builder() .scheme("https") @@ -106,6 +130,38 @@ static HttpUrl buildEventPlatformIntakeUrl(String site) { return url; } + private static boolean isValidDnsSuffix(@Nullable String site) { + if (site == null || site.isEmpty()) { + return false; + } + + int labelLength = 0; + for (int i = 0; i < site.length(); i++) { + final char character = site.charAt(i); + if (character == '.') { + if (labelLength == 0 || labelLength > MAX_DNS_LABEL_LENGTH || site.charAt(i - 1) == '-') { + return false; + } + labelLength = 0; + } else { + if ((!isAsciiLetterOrDigit(character) && character != '-') + || (labelLength == 0 && character == '-')) { + return false; + } + labelLength++; + } + } + return labelLength > 0 + && labelLength <= MAX_DNS_LABEL_LENGTH + && site.charAt(site.length() - 1) != '-'; + } + + private static boolean isAsciiLetterOrDigit(final char character) { + return (character >= 'a' && character <= 'z') + || (character >= 'A' && character <= 'Z') + || (character >= '0' && character <= '9'); + } + /** Creates an API client that uses the specified retry policy with a compatible local proxy. */ public @Nullable BackendApi createEvpProxyApi(Intake intake) { return createEvpProxyApi(intake, true); @@ -119,14 +175,63 @@ static HttpUrl buildEventPlatformIntakeUrl(String site) { /** Creates an API client that sends data through a compatible local EVP proxy. */ public @Nullable BackendApi createEvpProxyApi( Intake intake, boolean responseCompression, HttpRetryPolicy.Factory retryPolicyFactory) { + return createEvpProxyApi(intake, responseCompression, retryPolicyFactory, false, emptyList()); + } + + /** + * Creates an EVP proxy client after Agent discovery, optionally forcing a fresh discovery and + * requiring the Agent to advertise every specified forwarding header. + * + *

Callers supply the forwarding capabilities their event protocol requires. + * + *

The {@code forceDiscovery} form is intended for bounded unavailable-route recovery probes. + */ + public @Nullable BackendApi createEvpProxyApi( + Intake intake, + boolean responseCompression, + HttpRetryPolicy.Factory retryPolicyFactory, + boolean forceDiscovery, + Iterable requiredProxyHeaders) { + String endpoint = discoverEvpProxyEndpoint(forceDiscovery, requiredProxyHeaders); + return endpoint == null + ? null + : createEvpProxyApi(intake, responseCompression, retryPolicyFactory, endpoint); + } + + /** + * Discovers a capability-validated endpoint that related senders can share without re-probing. + */ + public @Nullable String discoverEvpProxyEndpoint( + boolean forceDiscovery, Iterable requiredProxyHeaders) { DDAgentFeaturesDiscovery featuresDiscovery = sharedCommunicationObjects.featuresDiscovery(config); - featuresDiscovery.discoverIfOutdated(); - if (!featuresDiscovery.supportsEvpProxy()) { - return null; + if (forceDiscovery) { + featuresDiscovery.discover(); + } else { + featuresDiscovery.discoverIfOutdated(); } String evpProxyEndpoint = featuresDiscovery.getEvpProxyEndpoint(); + if (evpProxyEndpoint != null + && !featuresDiscovery.supportsEvpProxyHeaders(requiredProxyHeaders)) { + evpProxyEndpoint = null; + } + return evpProxyEndpoint; + } + + /** Creates a client for a fixed compatibility or previously discovered endpoint, without I/O. */ + public BackendApi createEvpProxyApiForEndpoint( + Intake intake, + boolean responseCompression, + HttpRetryPolicy.Factory retryPolicyFactory, + String evpProxyEndpoint) { + return createEvpProxyApi(intake, responseCompression, retryPolicyFactory, evpProxyEndpoint); + } + private BackendApi createEvpProxyApi( + Intake intake, + boolean responseCompression, + HttpRetryPolicy.Factory retryPolicyFactory, + String evpProxyEndpoint) { String traceId = config.getIdGenerationStrategy().generateTraceId().toString(); log.debug( "Creating EVP proxy client for {} using endpoint {} with responseCompression={}", @@ -140,11 +245,24 @@ static HttpUrl buildEventPlatformIntakeUrl(String site) { evpProxyUrl, subdomain, retryPolicyFactory, - sharedCommunicationObjects.agentHttpClient, + configureHttpClient(sharedCommunicationObjects.agentHttpClient), responseCompression); } - private static HttpRetryPolicy.Factory retryPolicyFactory() { - return new HttpRetryPolicy.Factory(5, 100, 2.0, true); + OkHttpClient configureHttpClient(final OkHttpClient httpClient) { + return sendOnce + ? httpClient + .newBuilder() + .retryOnConnectionFailure(false) + .followRedirects(false) + .followSslRedirects(false) + .build() + : httpClient; + } + + private HttpRetryPolicy.Factory retryPolicyFactory() { + return sendOnce + ? HttpRetryPolicy.Factory.NEVER_RETRY + : new HttpRetryPolicy.Factory(5, 100, 2.0, true); } } diff --git a/communication/src/main/java/datadog/communication/EvpProxy.java b/communication/src/main/java/datadog/communication/EvpProxy.java index c2453bccb25..301c48f0ff5 100644 --- a/communication/src/main/java/datadog/communication/EvpProxy.java +++ b/communication/src/main/java/datadog/communication/EvpProxy.java @@ -5,6 +5,12 @@ public final class EvpProxy { public static final String SUBDOMAIN_HEADER = "X-Datadog-EVP-Subdomain"; + /** Identifies the SDK that produced an EVP request. */ + public static final String ORIGIN_HEADER = "DD-EVP-ORIGIN"; + + /** Identifies the version of the SDK that produced an EVP request. */ + public static final String ORIGIN_VERSION_HEADER = "DD-EVP-ORIGIN-VERSION"; + /** * Default SDK-side target for uncompressed EVP request bodies. Writers may split batches at or * below this size to keep Agent proxy requests comfortably bounded. diff --git a/communication/src/main/java/datadog/communication/ddagent/DDAgentFeaturesDiscovery.java b/communication/src/main/java/datadog/communication/ddagent/DDAgentFeaturesDiscovery.java index 16be2e84b98..d3696c62f5c 100644 --- a/communication/src/main/java/datadog/communication/ddagent/DDAgentFeaturesDiscovery.java +++ b/communication/src/main/java/datadog/communication/ddagent/DDAgentFeaturesDiscovery.java @@ -24,6 +24,7 @@ import java.nio.ByteBuffer; import java.util.HashSet; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Set; import okhttp3.HttpUrl; @@ -98,6 +99,7 @@ private static class State { String debuggerSnapshotEndpoint; String debuggerDiagnosticsEndpoint; String evpProxyEndpoint; + Set evpProxyAllowedHeaders = emptySet(); String version; String telemetryProxyEndpoint; Set peerTags = emptySet(); @@ -289,6 +291,16 @@ private boolean processInfoResponse(State newState, String response) { break; } } + final Object allowedHeadersObj = map.get("evp_proxy_allowed_headers"); + if (allowedHeadersObj instanceof List) { + final Set allowedHeaders = new HashSet<>(); + for (Object header : (List) allowedHeadersObj) { + if (header instanceof String) { + allowedHeaders.add(((String) header).toLowerCase(Locale.ROOT)); + } + } + newState.evpProxyAllowedHeaders = unmodifiableSet(allowedHeaders); + } for (String endpoint : telemetryProxyEndpoints) { if (containsEndpoint(endpoints, endpoint)) { @@ -435,6 +447,17 @@ public boolean supportsEvpProxy() { return discoveryState.evpProxyEndpoint != null; } + /** Returns whether the Agent advertises forwarding every required EVP request header. */ + public boolean supportsEvpProxyHeaders(final Iterable requiredHeaders) { + final Set allowedHeaders = discoveryState.evpProxyAllowedHeaders; + for (String requiredHeader : requiredHeaders) { + if (!allowedHeaders.contains(requiredHeader.toLowerCase(Locale.ROOT))) { + return false; + } + } + return true; + } + public boolean supportsContentEncodingHeadersWithEvpProxy() { // content encoding headers are supported in /v4 and above final String evpProxyEndpoint = discoveryState.evpProxyEndpoint; diff --git a/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java b/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java index 44690ef1c3f..155b2087789 100644 --- a/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java +++ b/communication/src/test/java/datadog/communication/BackendApiFactoryTest.java @@ -1,10 +1,16 @@ package datadog.communication; +import static datadog.communication.EvpProxy.ORIGIN_HEADER; +import static datadog.communication.EvpProxy.ORIGIN_VERSION_HEADER; +import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V2_EVP_PROXY_ENDPOINT; import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V4_EVP_PROXY_ENDPOINT; +import static java.util.Arrays.asList; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import datadog.communication.ddagent.DDAgentFeaturesDiscovery; import datadog.communication.ddagent.SharedCommunicationObjects; @@ -15,7 +21,9 @@ import datadog.trace.api.intake.Intake; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.util.Arrays; import java.util.Locale; +import java.util.stream.Stream; import okhttp3.HttpUrl; import okhttp3.MediaType; import okhttp3.OkHttpClient; @@ -25,6 +33,7 @@ import okhttp3.mockwebserver.RecordedRequest; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; import org.junit.jupiter.params.provider.NullAndEmptySource; import org.junit.jupiter.params.provider.ValueSource; @@ -33,8 +42,18 @@ class BackendApiFactoryTest { private static final MediaType JSON = MediaType.parse("application/json"); @ParameterizedTest - @ValueSource(strings = {"datadoghq.com", "custom.example", "DATADOGHQ.EU"}) + @ValueSource(strings = {"datadoghq.com", "custom.example", "DATADOGHQ.EU", "mock-intake.invalid"}) void eventPlatformDirectIntakeUsesExactHttpsHost(String site) { + assertEventPlatformIntakeUrl(site); + } + + @ParameterizedTest + @MethodSource("boundaryValidSites") + void eventPlatformDirectIntakeAcceptsDnsLengthBoundaries(String site) { + assertEventPlatformIntakeUrl(site); + } + + private static void assertEventPlatformIntakeUrl(final String site) { final HttpUrl url = BackendApiFactory.buildEventPlatformIntakeUrl(site); assertEquals("https", url.scheme()); @@ -63,13 +82,58 @@ void eventPlatformDirectIntakeUsesExactHttpsHost(String site) { "data doghq.com", " datadoghq.com", "datadoghq.com ", - "datadoghq.com\\evil.example" + "datadoghq.com\\evil.example", + "-foo.bar", + "foo-.bar", + "foo.-bar", + "foo.bar-", + "foo_bar.com", + ".foo.bar", + "foo..bar", + "foo.bar." }) void eventPlatformDirectIntakeRejectsUnsafeSite(String site) { assertThrows( IllegalArgumentException.class, () -> BackendApiFactory.buildEventPlatformIntakeUrl(site)); } + @ParameterizedTest + @MethodSource("invalidLengthSites") + void eventPlatformDirectIntakeRejectsDnsLengthOverflow(String site) { + assertThrows( + IllegalArgumentException.class, () -> BackendApiFactory.buildEventPlatformIntakeUrl(site)); + } + + private static Stream boundaryValidSites() { + return Stream.of( + repeatedAsciiLabel(63) + ".invalid", + repeatedAsciiLabel(63) + + "." + + repeatedAsciiLabel(63) + + "." + + repeatedAsciiLabel(63) + + "." + + repeatedAsciiLabel(39)); + } + + private static Stream invalidLengthSites() { + return Stream.of( + repeatedAsciiLabel(64) + ".invalid", + repeatedAsciiLabel(63) + + "." + + repeatedAsciiLabel(63) + + "." + + repeatedAsciiLabel(63) + + "." + + repeatedAsciiLabel(40)); + } + + private static String repeatedAsciiLabel(final int length) { + final char[] label = new char[length]; + Arrays.fill(label, 'a'); + return new String(label); + } + @ParameterizedTest @ValueSource(ints = {301, 302, 307, 308}) void featureFlagDirectIntakeDoesNotFollowRedirects(final int statusCode) throws Exception { @@ -153,6 +217,272 @@ void advertisedEvpProxyEndpointSupportsDisabledResponseCompression() throws Exce } } + @Test + void evpProxyKeepsHistoricalRelativeUrlResolution() throws Exception { + final MockWebServer agent = new MockWebServer(); + agent.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); + agent.start(); + try { + final FakeFeaturesDiscovery discovery = new FakeFeaturesDiscovery(V4_EVP_PROXY_ENDPOINT); + final BackendApiFactory factory = + new BackendApiFactory( + Config.get(), sharedCommunicationObjects(discovery, agent.url("/agent/base"))); + final BackendApi api = factory.createBackendApi(Intake.EVENT_PLATFORM, false); + + assertNotNull(api); + api.post( + "flagevaluation", + RequestBody.create(JSON, "{}".getBytes(StandardCharsets.UTF_8)), + stream -> null, + null, + false); + + assertEquals("/agent/evp_proxy/v4/api/v2/flagevaluation", agent.takeRequest().getPath()); + } finally { + agent.shutdown(); + } + } + + @Test + void discoveryKeepsHistoricalUrlResolutionAndAcceptsCaseInsensitiveCapabilities() + throws Exception { + final MockWebServer agent = new MockWebServer(); + agent.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"endpoints\":[\"v0.5/traces\",\"/evp_proxy/v4/\"]," + + "\"evp_proxy_allowed_headers\":[\"dd-evp-origin\"," + + "\"DD-EVP-ORIGIN-VERSION\"]}")); + agent.start(); + try { + final DDAgentFeaturesDiscovery discovery = + new DDAgentFeaturesDiscovery( + new OkHttpClient(), + Monitoring.DISABLED, + agent.url("/agent/base"), + ProtocolVersion.V0_5, + true, + false); + + discovery.discover(); + + assertEquals("/agent/info", agent.takeRequest().getPath()); + assertEquals(V4_EVP_PROXY_ENDPOINT, discovery.getEvpProxyEndpoint()); + assertTrue(discovery.supportsEvpProxyHeaders(asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))); + } finally { + agent.shutdown(); + } + } + + @ParameterizedTest + @ValueSource( + strings = { + "{}", + "{\"evp_proxy_allowed_headers\":null}", + "{\"evp_proxy_allowed_headers\":[\"DD-EVP-ORIGIN\"]}", + "{\"evp_proxy_allowed_headers\":[\"DD-EVP-ORIGIN-VERSION\"]}" + }) + void discoveryRejectsMissingOrPartialIdentityCapabilities(final String capabilityJson) + throws Exception { + final MockWebServer agent = new MockWebServer(); + final String fields = capabilityJson.substring(1, capabilityJson.length() - 1); + agent.enqueue( + new MockResponse() + .setResponseCode(200) + .setBody( + "{\"endpoints\":[\"v0.5/traces\",\"evp_proxy/v4/\"]" + + (fields.isEmpty() ? "" : "," + fields) + + "}")); + agent.start(); + try { + final DDAgentFeaturesDiscovery discovery = + new DDAgentFeaturesDiscovery( + new OkHttpClient(), + Monitoring.DISABLED, + agent.url("/"), + ProtocolVersion.V0_5, + true, + false); + + discovery.discover(); + + assertEquals(V4_EVP_PROXY_ENDPOINT, discovery.getEvpProxyEndpoint()); + assertFalse(discovery.supportsEvpProxyHeaders(asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))); + + // The capability gate must not depend on whether the factory emits SDK identity headers. + final BackendApiFactory factory = + new BackendApiFactory( + Config.get(), sharedCommunicationObjects(discovery, agent.url("/")), true); + assertNull( + factory.createEvpProxyApi( + Intake.EVENT_PLATFORM, + false, + HttpRetryPolicy.Factory.NEVER_RETRY, + false, + asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))); + } finally { + agent.shutdown(); + } + } + + @Test + void featureFlagProxyRequestNeverCarriesDirectCredentials() throws Exception { + final MockWebServer agent = new MockWebServer(); + agent.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); + agent.start(); + try { + final FakeFeaturesDiscovery discovery = new FakeFeaturesDiscovery(V4_EVP_PROXY_ENDPOINT); + final BackendApiFactory factory = + new BackendApiFactory( + Config.get(), sharedCommunicationObjects(discovery, agent.url("/")), true); + final BackendApi api = + factory.createEvpProxyApi( + Intake.EVENT_PLATFORM, + false, + HttpRetryPolicy.Factory.NEVER_RETRY, + false, + asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER)); + + assertNotNull(api); + api.post( + "exposures", + RequestBody.create(JSON, "{}".getBytes(StandardCharsets.UTF_8)), + stream -> null, + null, + false); + + final RecordedRequest request = agent.takeRequest(); + assertEquals("/evp_proxy/v4/api/v2/exposures", request.getPath()); + assertEquals("event-platform-intake", request.getHeader("X-Datadog-EVP-Subdomain")); + assertNull(request.getHeader("DD-API-KEY")); + assertEquals(1, agent.getRequestCount()); + } finally { + agent.shutdown(); + } + } + + @Test + void featureFlagDirectRequestCarriesCredentialsExactlyOnce() throws Exception { + final MockWebServer intake = new MockWebServer(); + intake.enqueue(new MockResponse().setResponseCode(500).setBody("failed")); + intake.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); + intake.start(); + final OkHttpClient sharedClient = new OkHttpClient.Builder().build(); + final BackendApiFactory factory = + new BackendApiFactory( + Config.get(), sharedCommunicationObjects(new FakeFeaturesDiscovery(null), null), true); + final OkHttpClient directClient = factory.configureHttpClient(sharedClient); + try { + final IntakeApi api = + new IntakeApi( + intake.url("/api/v2/"), + "api-key", + "123", + HttpRetryPolicy.Factory.NEVER_RETRY, + directClient, + false); + + assertThrows( + IOException.class, + () -> + api.post( + "flagevaluation", + RequestBody.create(JSON, "{}".getBytes(StandardCharsets.UTF_8)), + stream -> null, + null, + false)); + + final RecordedRequest request = intake.takeRequest(); + assertEquals("/api/v2/flagevaluation", request.getPath()); + assertEquals("api-key", request.getHeader("DD-API-KEY")); + assertNull(request.getHeader("X-Datadog-EVP-Subdomain")); + assertEquals(1, intake.getRequestCount()); + assertEquals(false, directClient.retryOnConnectionFailure()); + } finally { + directClient.dispatcher().executorService().shutdownNow(); + directClient.connectionPool().evictAll(); + sharedClient.dispatcher().executorService().shutdownNow(); + sharedClient.connectionPool().evictAll(); + intake.shutdown(); + } + } + + @Test + void fixedV2CompatibilityRouteDoesNotDependOnDiscovery() throws Exception { + final MockWebServer agent = new MockWebServer(); + agent.enqueue(new MockResponse().setResponseCode(500).setBody("ambiguous")); + agent.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); + agent.start(); + try { + final FakeFeaturesDiscovery discovery = new FakeFeaturesDiscovery(null); + final BackendApiFactory factory = + new BackendApiFactory( + Config.get(), sharedCommunicationObjects(discovery, agent.url("/"))); + + final BackendApi api = + factory.createEvpProxyApiForEndpoint( + Intake.EVENT_PLATFORM, + false, + HttpRetryPolicy.Factory.NEVER_RETRY, + V2_EVP_PROXY_ENDPOINT); + + assertNotNull(api); + assertThrows( + HttpResponseException.class, + () -> + api.post( + "flagevaluation", + RequestBody.create(JSON, "{}".getBytes(StandardCharsets.UTF_8)), + stream -> null, + null, + false)); + api.post( + "flagevaluation", + RequestBody.create(JSON, "{}".getBytes(StandardCharsets.UTF_8)), + stream -> null, + null, + false); + assertEquals("/evp_proxy/v2/api/v2/flagevaluation", agent.takeRequest().getPath()); + assertEquals("/evp_proxy/v2/api/v2/flagevaluation", agent.takeRequest().getPath()); + assertEquals(2, agent.getRequestCount()); + } finally { + agent.shutdown(); + } + } + + @Test + void capabilityGatedProxyRequiresHeadersWithoutRequestHeaderConfiguration() { + final FakeFeaturesDiscovery discovery = new FakeFeaturesDiscovery(V4_EVP_PROXY_ENDPOINT, false); + final BackendApiFactory factory = + new BackendApiFactory(Config.get(), sharedCommunicationObjects(discovery, null), true); + + assertNull( + factory.createEvpProxyApi( + Intake.EVENT_PLATFORM, + false, + HttpRetryPolicy.Factory.NEVER_RETRY, + false, + asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))); + } + + @Test + void recoveryRequestForcesFreshDiscovery() { + final FakeFeaturesDiscovery discovery = new FakeFeaturesDiscovery(null); + final BackendApiFactory factory = + new BackendApiFactory(Config.get(), sharedCommunicationObjects(discovery, null)); + + assertNull( + factory.createEvpProxyApi( + Intake.EVENT_PLATFORM, + false, + HttpRetryPolicy.Factory.NEVER_RETRY, + true, + asList(ORIGIN_HEADER, ORIGIN_VERSION_HEADER))); + assertEquals(1, discovery.forcedDiscoveries); + assertEquals(0, discovery.outdatedDiscoveries); + } + @Test void explicitNoRetryProxyPolicyDoesNotReplayAmbiguousFailure() throws Exception { final MockWebServer agent = new MockWebServer(); @@ -208,8 +538,16 @@ public DDAgentFeaturesDiscovery featuresDiscovery(final Config config) { private static final class FakeFeaturesDiscovery extends DDAgentFeaturesDiscovery { private final String evpProxyEndpoint; + private final boolean supportsRequestHeaders; + private int forcedDiscoveries; + private int outdatedDiscoveries; private FakeFeaturesDiscovery(final String evpProxyEndpoint) { + this(evpProxyEndpoint, true); + } + + private FakeFeaturesDiscovery( + final String evpProxyEndpoint, final boolean supportsRequestHeaders) { super( new OkHttpClient(), Monitoring.DISABLED, @@ -218,10 +556,18 @@ private FakeFeaturesDiscovery(final String evpProxyEndpoint) { true, false); this.evpProxyEndpoint = evpProxyEndpoint; + this.supportsRequestHeaders = supportsRequestHeaders; } @Override - public void discoverIfOutdated() {} + public void discover() { + forcedDiscoveries++; + } + + @Override + public void discoverIfOutdated() { + outdatedDiscoveries++; + } @Override public String getEvpProxyEndpoint() { @@ -232,5 +578,10 @@ public String getEvpProxyEndpoint() { public boolean supportsEvpProxy() { return evpProxyEndpoint != null; } + + @Override + public boolean supportsEvpProxyHeaders(final Iterable requiredHeaders) { + return supportsRequestHeaders; + } } } diff --git a/communication/src/test/java/datadog/communication/SendOnceTransportTest.java b/communication/src/test/java/datadog/communication/SendOnceTransportTest.java new file mode 100644 index 00000000000..9d761b98e90 --- /dev/null +++ b/communication/src/test/java/datadog/communication/SendOnceTransportTest.java @@ -0,0 +1,122 @@ +package datadog.communication; + +import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V4_EVP_PROXY_ENDPOINT; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import datadog.communication.ddagent.SharedCommunicationObjects; +import datadog.communication.http.HttpRetryPolicy; +import datadog.trace.api.Config; +import datadog.trace.api.intake.Intake; +import datadog.trace.test.junit.utils.config.WithConfig; +import datadog.trace.test.util.DDJavaSpecification; +import java.io.IOException; +import okhttp3.HttpUrl; +import okhttp3.MediaType; +import okhttp3.OkHttpClient; +import okhttp3.Request; +import okhttp3.RequestBody; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import okhttp3.mockwebserver.SocketPolicy; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.tabletest.junit.TableTest; + +/** Pins send-once on the clients that BackendApiFactory builds, for both EVP routes. */ +class SendOnceTransportTest extends DDJavaSpecification { + private MockWebServer server; + private OkHttpClient agentClient; + private OkHttpClient intakeClient; + + @BeforeEach + void setUp() throws IOException { + server = new MockWebServer(); + server.start(); + agentClient = new OkHttpClient(); + // Direct URLs are fixed to https://event-platform-intake./. Send them to the local + // server. + intakeClient = + new OkHttpClient.Builder() + .addInterceptor( + chain -> { + final Request original = chain.request(); + final HttpUrl local = + original + .url() + .newBuilder() + .scheme("http") + .host(server.getHostName()) + .port(server.getPort()) + .build(); + return chain.proceed(original.newBuilder().url(local).build()); + }) + .build(); + } + + @AfterEach + void tearDown() throws IOException { + for (final OkHttpClient client : new OkHttpClient[] {agentClient, intakeClient}) { + client.dispatcher().executorService().shutdownNow(); + client.connectionPool().evictAll(); + } + server.shutdown(); + } + + @WithConfig(key = "API_KEY", value = "api-key", env = true) + @TableTest({ + "Scenario | Route | Send Once | Requests | Second Post Fails", + "proxy send-once | proxy | true | 2 | true ", + "proxy control | proxy | false | 3 | false ", + "direct send-once | direct | true | 2 | true ", + "direct control | direct | false | 3 | false " + }) + void doesNotReplayAPostAfterThePooledConnectionDrops( + final String route, final boolean sendOnce, final int requests, final boolean secondPostFails) + throws Exception { + // The first call leaves a pooled connection. The server drops it during the second call. + server.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); + server.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AFTER_REQUEST)); + server.enqueue(new MockResponse().setResponseCode(200).setBody("{}")); + final BackendApiFactory factory = factory(sendOnce); + final BackendApi api = + route.equals("proxy") + ? factory.createEvpProxyApiForEndpoint( + Intake.EVENT_PLATFORM, + false, + HttpRetryPolicy.Factory.NEVER_RETRY, + V4_EVP_PROXY_ENDPOINT) + : factory.createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false); + + post(api); + if (secondPostFails) { + assertThrows(IOException.class, () -> post(api)); + } else { + post(api); + } + + assertEquals(requests, server.getRequestCount()); + } + + private BackendApiFactory factory(final boolean sendOnce) { + final SharedCommunicationObjects sco = + new SharedCommunicationObjects() { + @Override + public OkHttpClient getIntakeHttpClient() { + return intakeClient; + } + }; + sco.agentUrl = server.url("/"); + sco.agentHttpClient = agentClient; + return new BackendApiFactory(Config.get(), sco, sendOnce); + } + + private static void post(final BackendApi api) throws IOException { + api.post( + "flagevaluation", + RequestBody.create(MediaType.parse("application/json"), "{}"), + stream -> null, + null, + false); + } +} diff --git a/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/test/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentationTest.java b/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/test/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentationTest.java new file mode 100644 index 00000000000..13736934cda --- /dev/null +++ b/dd-java-agent/instrumentation/java/java-lang/java-lang-1.8/src/test/java/datadog/trace/instrumentation/java/lang/ShutdownInstrumentationTest.java @@ -0,0 +1,58 @@ +package datadog.trace.instrumentation.java.lang; + +import static datadog.trace.agent.tooling.InstrumenterModule.TargetSystem.TRACING; +import static java.util.Collections.emptySet; +import static java.util.Collections.singleton; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import datadog.trace.agent.tooling.InstrumenterModule.TargetSystem; +import datadog.trace.test.junit.utils.config.WithConfig; +import datadog.trace.test.util.DDJavaSpecification; +import java.util.EnumSet; +import org.junit.jupiter.api.Test; + +class ShutdownInstrumentationTest extends DDJavaSpecification { + @Test + void defaultConfigurationDoesNotEnableWholeAgentShutdownWithoutTracing() { + assertFalse(new ShutdownInstrumentation().isApplicable(emptySet())); + } + + @Test + @WithConfig(key = "feature.flags.enabled", value = "true") + void generatedTargetMaskDoesNotDependOnFeatureFlagBuildConfiguration() { + final EnumSet systems = EnumSet.noneOf(TargetSystem.class); + for (TargetSystem system : TargetSystem.values()) { + if (new ShutdownInstrumentation().isApplicable(singleton(system))) { + systems.add(system); + } + } + assertEquals(EnumSet.of(TRACING), systems); + } + + @Test + @WithConfig(key = "feature.flags.enabled", value = "false") + void doesNotInstrumentShutdownWhenTracingAndFeatureFlagsAreDisabled() { + assertFalse(new ShutdownInstrumentation().isApplicable(emptySet())); + } + + @Test + @WithConfig(key = "feature.flags.enabled", value = "false") + void preservesTracingShutdown() { + assertTrue(new ShutdownInstrumentation().isApplicable(singleton(TRACING))); + } + + @Test + @WithConfig(key = "feature.flags.configuration.source", value = "agentless") + void featureFlagsDoNotEnableWholeAgentShutdownWithoutTracing() { + assertFalse(new ShutdownInstrumentation().isApplicable(emptySet())); + } + + @Test + @WithConfig(key = "feature.flags.enabled", value = "false") + @WithConfig(key = "feature.flags.configuration.source", value = "agentless") + void honorsExplicitDisableEvenWithAConfigurationSource() { + assertFalse(new ShutdownInstrumentation().isApplicable(emptySet())); + } +} diff --git a/dd-java-agent/src/test/java/datadog/trace/agent/TraceDisabledFeatureFlaggingShutdownTest.java b/dd-java-agent/src/test/java/datadog/trace/agent/TraceDisabledFeatureFlaggingShutdownTest.java new file mode 100644 index 00000000000..675b60a717a --- /dev/null +++ b/dd-java-agent/src/test/java/datadog/trace/agent/TraceDisabledFeatureFlaggingShutdownTest.java @@ -0,0 +1,115 @@ +package datadog.trace.agent; + +import static java.util.Collections.emptyList; +import static java.util.Collections.singletonMap; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import datadog.trace.agent.test.IntegrationTestUtils; +import java.io.ByteArrayOutputStream; +import java.io.PrintStream; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.TimeUnit; +import jvmbootstraptest.AgentLoadedChecker; +import jvmbootstraptest.FeatureFlaggingShutdownProbe; +import okhttp3.mockwebserver.Dispatcher; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import okhttp3.mockwebserver.RecordedRequest; +import org.junit.jupiter.api.Timeout; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +@Timeout(value = 30, unit = TimeUnit.SECONDS) +class TraceDisabledFeatureFlaggingShutdownTest { + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void unusedFeatureFlagsDoNotEnableWholeAgentShutdown(boolean explicitlyEnabled) throws Exception { + final List options = options(false); + if (explicitlyEnabled) { + options.add("-Ddd.feature.flags.enabled=true"); + } + final String logs = run(AgentLoadedChecker.class, options); + assertFalse(logs.contains("Shutting down agent"), logs); + assertFalse(logs.contains("Feature Flagging system stopped"), logs); + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void activatedProviderFlushesBothEventTypesAtJvmExit(boolean tracing) throws Exception { + final LinkedBlockingQueue events = new LinkedBlockingQueue<>(); + try (MockWebServer server = new MockWebServer()) { + server.setDispatcher( + new Dispatcher() { + @Override + public MockResponse dispatch(RecordedRequest request) { + if (request.getPath().equals("/info")) { + return new MockResponse() + .setBody( + "{\"endpoints\":[\"v0.4/traces\",\"v0.5/traces\",\"evp_proxy/v4/\"]," + + "\"evp_proxy_allowed_headers\":[\"DD-EVP-ORIGIN\",\"DD-EVP-ORIGIN-VERSION\"]}"); + } + if (request.getPath().startsWith("/evp_proxy/")) { + events.add(request); + return new MockResponse().setResponseCode(202).setBody("{}"); + } + return new MockResponse().setResponseCode(404); + } + }); + server.start(); + final List options = options(tracing); + options.add("-Ddd.trace.agent.url=" + server.url("/")); + options.add("-Ddd.feature.flags.configuration.source=agentless"); + options.add( + "-Ddd.feature.flags.configuration.source.agentless.base.url=" + server.url("/config")); + final String logs = run(FeatureFlaggingShutdownProbe.class, options); + assertTrue(logs.contains("EXIT_WITH_QUEUED_FEATURE_FLAG_EVENTS"), logs); + assertEquals(tracing, logs.contains("Shutting down agent"), logs); + assertTrue(logs.contains("Feature Flagging system stopped"), logs); + + final List paths = new ArrayList<>(); + for (int i = 0; i < 2; i++) { + final RecordedRequest event = events.poll(1, TimeUnit.SECONDS); + assertNotNull(event, logs); + paths.add(event.getPath()); + assertEquals("POST", event.getMethod()); + assertNull(event.getHeader("DD-API-KEY")); + assertTrue(event.getBody().readUtf8().contains("shutdown-flag")); + } + assertTrue(paths.contains("/evp_proxy/v4/api/v2/exposures"), paths.toString()); + assertTrue(paths.contains("/evp_proxy/v4/api/v2/flagevaluation"), paths.toString()); + assertEquals(0, events.size(), "Shutdown must not duplicate either batch"); + } + } + + private static List options(boolean tracing) { + return new ArrayList<>( + Arrays.asList( + "-Ddatadog.slf4j.simpleLogger.defaultLogLevel=debug", + "-Ddd.trace.enabled=" + tracing, + "-Ddd.jmxfetch.enabled=false", + "-Ddd.profiling.enabled=false", + "-Ddd.appsec.enabled=false", + "-Ddd.remote_config.enabled=false", + "-Ddd.telemetry.enabled=false")); + } + + private static String run(Class main, List options) throws Exception { + try (ByteArrayOutputStream output = new ByteArrayOutputStream(); + PrintStream stream = new PrintStream(output, true, StandardCharsets.UTF_8.name())) { + final int exit = + IntegrationTestUtils.runOnSeparateJvm( + main.getName(), options, emptyList(), singletonMap("DD_API_KEY", ""), stream); + final String logs = output.toString(StandardCharsets.UTF_8.name()); + assertEquals(0, exit, logs); + return logs; + } + } +} diff --git a/dd-java-agent/src/test/java/jvmbootstraptest/FeatureFlaggingShutdownProbe.java b/dd-java-agent/src/test/java/jvmbootstraptest/FeatureFlaggingShutdownProbe.java new file mode 100644 index 00000000000..3b23b520395 --- /dev/null +++ b/dd-java-agent/src/test/java/jvmbootstraptest/FeatureFlaggingShutdownProbe.java @@ -0,0 +1,64 @@ +package jvmbootstraptest; + +import java.util.Collections; +import java.util.Map; + +/** Activates the agent's production gateway and leaves both event types queued at JVM exit. */ +public class FeatureFlaggingShutdownProbe { + public static void main(String[] args) throws Exception { + final Class gateway = type("FeatureFlaggingGateway"); + // Agent installation can finish asynchronously on Java 8. This probe tests shutdown after + // activation, not provider initialization timing. + final long deadline = System.nanoTime() + java.util.concurrent.TimeUnit.SECONDS.toNanos(10); + Object writer; + do { + gateway.getMethod("activate").invoke(null); + writer = gateway.getMethod("getFlagEvalWriter").invoke(null); + if (writer != null) { + break; + } + Thread.sleep(10); + } while (System.nanoTime() < deadline); + if (writer == null) { + throw new AssertionError("Provider activation did not start the evaluation writer"); + } + final Class exposure = type("exposure.ExposureEvent"); + final Class allocation = type("exposure.Allocation"); + final Class flag = type("exposure.Flag"); + final Class variant = type("exposure.Variant"); + final Class subject = type("exposure.Subject"); + final Object event = + exposure + .getConstructor(long.class, allocation, flag, variant, subject) + .newInstance( + System.currentTimeMillis(), + allocation.getConstructor(String.class).newInstance("allocation"), + flag.getConstructor(String.class).newInstance("shutdown-flag"), + variant.getConstructor(String.class).newInstance("on"), + subject + .getConstructor(String.class, Map.class) + .newInstance("shutdown-user", Collections.emptyMap())); + gateway.getMethod("dispatch", exposure).invoke(null, event); + + final Class evaluation = type("flagevaluation.FlagEvalEvent"); + final Object evalEvent = + evaluation + .getConstructor( + String.class, String.class, String.class, String.class, long.class, Map.class) + .newInstance( + "shutdown-flag", + "on", + "allocation", + "shutdown-user", + System.currentTimeMillis(), + Collections.emptyMap()); + type("flagevaluation.FlagEvaluationWriter") + .getMethod("enqueue", evaluation) + .invoke(writer, evalEvent); + System.out.println("EXIT_WITH_QUEUED_FEATURE_FLAG_EVENTS"); + } + + private static Class type(String suffix) throws ClassNotFoundException { + return Class.forName("datadog.trace.api.featureflag." + suffix, true, null); + } +} diff --git a/internal-api/src/main/java/datadog/trace/util/AgentThreadFactory.java b/internal-api/src/main/java/datadog/trace/util/AgentThreadFactory.java index 81cd9a0b294..a472bdbec9d 100644 --- a/internal-api/src/main/java/datadog/trace/util/AgentThreadFactory.java +++ b/internal-api/src/main/java/datadog/trace/util/AgentThreadFactory.java @@ -69,6 +69,7 @@ public enum AgentThread { LLMOBS_FEEDBACK_PROCESSOR("dd-llmobs-feedback-processor"), FEATURE_FLAG_EXPOSURE_PROCESSOR("dd-ffe-exposure-processor"), + FEATURE_FLAG_SHUTDOWN("dd-feature-flagging-shutdown"), FEATURE_FLAG_EVALUATION_PROCESSOR("dd-ffe-evaluation-processor"), FEATURE_FLAG_CONFIGURATION_POLLER("dd-feature-flagging-http-poller"); diff --git a/products/feature-flagging/feature-flagging-agent/src/main/java/com/datadog/featureflag/FeatureFlaggingSystem.java b/products/feature-flagging/feature-flagging-agent/src/main/java/com/datadog/featureflag/FeatureFlaggingSystem.java index 7f105d00213..1ef61e70328 100644 --- a/products/feature-flagging/feature-flagging-agent/src/main/java/com/datadog/featureflag/FeatureFlaggingSystem.java +++ b/products/feature-flagging/feature-flagging-agent/src/main/java/com/datadog/featureflag/FeatureFlaggingSystem.java @@ -2,6 +2,8 @@ import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_AGENTLESS; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_REMOTE_CONFIG; +import static datadog.trace.util.AgentThreadFactory.AgentThread.FEATURE_FLAG_SHUTDOWN; +import static datadog.trace.util.AgentThreadFactory.newAgentThread; import datadog.communication.ddagent.SharedCommunicationObjects; import datadog.trace.api.Config; @@ -27,6 +29,7 @@ interface SystemInitializer { private static volatile SpanEnrichmentWriter SPAN_ENRICHMENT_WRITER; private static volatile FeatureFlaggingGateway.ActivationListener ACTIVATION_LISTENER; private static volatile boolean STARTED; + private static Thread SHUTDOWN_HOOK; private FeatureFlaggingSystem() {} @@ -82,6 +85,13 @@ private static void initializeOrRollBack( final SystemInitializer systemInitializer) { try { systemInitializer.initialize(sco, config); + if (!config.isTraceEnabled() && (EXPOSURE_WRITER != null || FLAG_EVAL_WRITER != null)) { + // Tracing already owns the pre-application-hook shutdown path. Without it, only an + // activated Feature Flagging subsystem needs a hook; do not stop other products. + final Thread hook = newAgentThread(FEATURE_FLAG_SHUTDOWN, FeatureFlaggingSystem::stop); + Runtime.getRuntime().addShutdownHook(hook); + SHUTDOWN_HOOK = hook; + } } catch (final RuntimeException | Error e) { stop(); throw e; @@ -94,7 +104,8 @@ private static void initializeSystem(final SharedCommunicationObjects sco, final LOGGER.debug("Feature Flagging system disabled by unsupported configuration source"); return; } - final ExposureWriter exposureWriter = new ExposureWriterImpl(sco, config); + final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(); + final ExposureWriter exposureWriter = new ExposureWriterImpl(sco, config, routeSelector); initialize(configService, exposureWriter); final boolean evalCountsEnabled = @@ -103,7 +114,8 @@ private static void initializeSystem(final SharedCommunicationObjects sco, final .getBoolean(FeatureFlaggingConfig.FLAGGING_EVALUATION_COUNTS_ENABLED, true); FeatureFlaggingGateway.setFlagEvaluationEnqueueEnabled(evalCountsEnabled); if (evalCountsEnabled) { - final FlagEvaluationWriterImpl evalWriter = new FlagEvaluationWriterImpl(sco, config); + final FlagEvaluationWriterImpl evalWriter = + new FlagEvaluationWriterImpl(sco, config, routeSelector); // Publish before start() so a failed start is still reachable by the rollback in stop(). FLAG_EVAL_WRITER = evalWriter; evalWriter.start(); @@ -167,6 +179,7 @@ static ConfigurationSourceService createConfigurationSourceService( justification = "Agent-internal class; Class object does not escape to app code and lock only guards the subsystem lifecycle.") public static synchronized void stop() { + final long deadline = FeatureFlagEventWriter.shutdownDeadline(); FeatureFlaggingGateway.setFlagEvaluationEnqueueEnabled(false); FeatureFlaggingGateway.setFlagEvalWriter(null); final FeatureFlaggingGateway.ActivationListener activationListener = ACTIVATION_LISTENER; @@ -180,13 +193,24 @@ public static synchronized void stop() { SPAN_ENRICHMENT_WRITER = null; EXPOSURE_WRITER = null; CONFIG_SERVICE = null; + final Thread shutdownHook = SHUTDOWN_HOOK; + SHUTDOWN_HOOK = null; + if (shutdownHook != null && shutdownHook != Thread.currentThread()) { + try { + Runtime.getRuntime().removeShutdownHook(shutdownHook); + } catch (IllegalStateException ignored) { + // JVM shutdown has already started. The idempotent hook will observe the stopped state. + } + } if (activationListener != null) { FeatureFlaggingGateway.removeActivationListener(activationListener); } - closeQuietly(flagEvalWriter); + requestWriterShutdown(flagEvalWriter); + requestWriterShutdown(exposureWriter); closeQuietly(spanEnrichmentWriter); - closeQuietly(exposureWriter); closeQuietly(configService); + awaitWriterTermination(flagEvalWriter, deadline); + awaitWriterTermination(exposureWriter, deadline); LOGGER.debug("Feature Flagging system stopped"); } @@ -194,6 +218,24 @@ static boolean isAwaitingApplicationActivation() { return ACTIVATION_LISTENER != null; } + static boolean isShutdownHookRegistered() { + return SHUTDOWN_HOOK != null; + } + + private static void requestWriterShutdown(final AutoCloseable writer) { + if (writer instanceof FeatureFlagEventWriter) { + ((FeatureFlagEventWriter) writer).requestShutdown(); + } else { + closeQuietly(writer); + } + } + + private static void awaitWriterTermination(final AutoCloseable writer, final long deadline) { + if (writer instanceof FeatureFlagEventWriter) { + ((FeatureFlagEventWriter) writer).awaitTermination(deadline); + } + } + static boolean isExposureWriterStarted() { return EXPOSURE_WRITER != null; } diff --git a/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlagRouteWiringTest.java b/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlagRouteWiringTest.java new file mode 100644 index 00000000000..8aedc5c06f8 --- /dev/null +++ b/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlagRouteWiringTest.java @@ -0,0 +1,132 @@ +package com.datadog.featureflag; + +import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V4_EVP_PROXY_ENDPOINT; +import static datadog.trace.test.junit.utils.config.WithConfigExtension.injectSysConfig; +import static java.util.Collections.emptyMap; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import com.sun.net.httpserver.HttpServer; +import datadog.communication.ddagent.DDAgentFeaturesDiscovery; +import datadog.communication.ddagent.SharedCommunicationObjects; +import datadog.trace.api.Config; +import datadog.trace.api.featureflag.FeatureFlaggingGateway; +import datadog.trace.api.featureflag.exposure.Allocation; +import datadog.trace.api.featureflag.exposure.ExposureEvent; +import datadog.trace.api.featureflag.exposure.Flag; +import datadog.trace.api.featureflag.exposure.Subject; +import datadog.trace.api.featureflag.exposure.Variant; +import datadog.trace.api.featureflag.flagevaluation.FlagEvalEvent; +import datadog.trace.test.junit.utils.config.WithConfig; +import java.net.InetSocketAddress; +import java.util.List; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import okhttp3.HttpUrl; +import okhttp3.OkHttpClient; +import okhttp3.Protocol; +import okhttp3.Request; +import okhttp3.Response; +import okhttp3.ResponseBody; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +class FeatureFlagRouteWiringTest { + @Test + @Timeout(15) + @WithConfig(key = "feature.flags.configuration.source", value = "agentless") + @WithConfig(key = "API_KEY", value = "test-api-key", env = true) + @WithConfig(key = "site", value = "datadoghq.com") + void productionWritersShareFallbackForFutureBatches() throws Exception { + final HttpServer configurationServer = + HttpServer.create(new InetSocketAddress("127.0.0.1", 0), 0); + configurationServer.createContext( + "/", + exchange -> { + exchange.sendResponseHeaders(404, -1); + exchange.close(); + }); + configurationServer.start(); + injectSysConfig( + "feature.flags.configuration.source.agentless.base.url", + "http://127.0.0.1:" + configurationServer.getAddress().getPort() + "/config"); + final List requests = new CopyOnWriteArrayList<>(); + final CountDownLatch exposureDelivered = new CountDownLatch(1); + final OkHttpClient client = + new OkHttpClient.Builder() + .addInterceptor( + chain -> { + final Request request = chain.request(); + requests.add(request); + final boolean direct = + request.url().host().equals("event-platform-intake.datadoghq.com"); + if (direct && request.url().encodedPath().equals("/api/v2/exposures")) { + exposureDelivered.countDown(); + } + return new Response.Builder() + .request(request) + .protocol(Protocol.HTTP_1_1) + .code(direct ? 202 : 404) + .message("test") + .body(ResponseBody.create(null, "{}")) + .build(); + }) + .build(); + final DDAgentFeaturesDiscovery discovery = mock(DDAgentFeaturesDiscovery.class); + when(discovery.supportsEvpProxyHeaders(any())).thenReturn(true); + when(discovery.supportsEvpProxy()).thenReturn(true); + when(discovery.getEvpProxyEndpoint()).thenReturn(V4_EVP_PROXY_ENDPOINT); + final SharedCommunicationObjects sco = + new SharedCommunicationObjects() { + @Override + public OkHttpClient getIntakeHttpClient() { + return client; + } + + @Override + public DDAgentFeaturesDiscovery featuresDiscovery(Config config) { + return discovery; + } + }; + sco.agentUrl = HttpUrl.get("http://localhost:8126/"); + sco.agentHttpClient = client; + // A terminal local configuration response avoids retry backoff before event activation. + try { + FeatureFlaggingSystem.start(sco); + FeatureFlaggingGateway.activate(); + FeatureFlaggingGateway.dispatch( + new ExposureEvent( + 1, + new Allocation("allocation"), + new Flag("flag"), + new Variant("on"), + new Subject("user", emptyMap()))); + assertTrue(exposureDelivered.await(5, TimeUnit.SECONDS)); + FeatureFlaggingGateway.getFlagEvalWriter() + .enqueue(new FlagEvalEvent("flag", "on", "allocation", "user", 1, emptyMap())); + FeatureFlaggingSystem.stop(); + + assertEquals(3, requests.size(), "One rejected local exposure, then two direct batches"); + assertEquals("/evp_proxy/v4/api/v2/exposures", requests.get(0).url().encodedPath()); + assertNull(requests.get(0).header("DD-API-KEY")); + assertEquals("/api/v2/exposures", requests.get(1).url().encodedPath()); + assertEquals("/api/v2/flagevaluation", requests.get(2).url().encodedPath()); + for (int i = 1; i < requests.size(); i++) { + assertEquals("event-platform-intake.datadoghq.com", requests.get(i).url().host()); + assertEquals("test-api-key", requests.get(i).header("DD-API-KEY")); + assertNull(requests.get(i).header("X-Datadog-EVP-Subdomain")); + } + } finally { + FeatureFlaggingSystem.stop(); + FeatureFlaggingGateway.setFlagEvaluationEnqueueEnabled(true); + client.connectionPool().evictAll(); + client.dispatcher().executorService().shutdownNow(); + configurationServer.stop(0); + } + } +} diff --git a/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlagShutdownBudgetTest.java b/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlagShutdownBudgetTest.java new file mode 100644 index 00000000000..3987d852b13 --- /dev/null +++ b/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlagShutdownBudgetTest.java @@ -0,0 +1,108 @@ +package com.datadog.featureflag; + +import static java.util.Collections.emptyMap; +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static java.util.concurrent.TimeUnit.NANOSECONDS; +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import datadog.communication.BackendApi; +import datadog.trace.api.Config; +import datadog.trace.api.featureflag.FeatureFlaggingGateway; +import datadog.trace.api.featureflag.exposure.Allocation; +import datadog.trace.api.featureflag.exposure.ExposureEvent; +import datadog.trace.api.featureflag.exposure.Flag; +import datadog.trace.api.featureflag.exposure.Subject; +import datadog.trace.api.featureflag.exposure.Variant; +import datadog.trace.api.featureflag.flagevaluation.FlagEvalEvent; +import java.lang.reflect.Field; +import java.util.concurrent.CountDownLatch; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; + +@Timeout(15) +class FeatureFlagShutdownBudgetTest { + private final CountDownLatch bothPostsStarted = new CountDownLatch(2); + private final CountDownLatch releasePosts = new CountDownLatch(1); + private Thread stopping; + + @AfterEach + void cleanup() throws Exception { + releasePosts.countDown(); + if (stopping != null) { + stopping.join(5000); + assertFalse(stopping.isAlive()); + } + FeatureFlaggingSystem.stop(); + FeatureFlaggingGateway.setFlagEvaluationEnqueueEnabled(true); + } + + @Test + void stopSignalsBothWritersBeforeWaitingForEither() throws Exception { + startWriters(); + stopping = new Thread(FeatureFlaggingSystem::stop); + stopping.start(); + assertTrue( + bothPostsStarted.await(500, MILLISECONDS), + "Exposure shutdown was delayed by the evaluation writer's wait"); + } + + @Test + void stopUsesOneBoundedBudgetForBothBlockedPosts() throws Exception { + startWriters(); + final long started = System.nanoTime(); + FeatureFlaggingSystem.stop(); + final long elapsed = NANOSECONDS.toMillis(System.nanoTime() - started); + assertTrue(elapsed < 1300, "Two writer waits consumed " + elapsed + " ms"); + } + + private void startWriters() throws Exception { + final BackendApi api = mock(BackendApi.class); + when(api.post(any(), any(), any(), any(), anyBoolean())) + .thenAnswer( + invocation -> { + bothPostsStarted.countDown(); + boolean interrupted = false; + while (releasePosts.getCount() != 0) { + try { + assertTrue(releasePosts.await(12, SECONDS)); + } catch (InterruptedException ignored) { + interrupted = true; + } + } + if (interrupted) { + Thread.currentThread().interrupt(); + } + return null; + }); + final ExposureWriterImpl exposures = + new ExposureWriterImpl(16, Long.MAX_VALUE, NANOSECONDS, () -> api, Config.get(), 5000); + final FlagEvaluationWriterImpl evaluations = + new FlagEvaluationWriterImpl(16, Long.MAX_VALUE, NANOSECONDS, () -> api, Config.get()); + setWriter("EXPOSURE_WRITER", exposures); + setWriter("FLAG_EVAL_WRITER", evaluations); + FeatureFlaggingGateway.setFlagEvaluationEnqueueEnabled(true); + exposures.init(); + evaluations.start(); + exposures.accept( + new ExposureEvent( + 1, + new Allocation("alloc"), + new Flag("flag"), + new Variant("on"), + new Subject("user", emptyMap()))); + evaluations.enqueue(new FlagEvalEvent("flag", "on", "alloc", "user", 1, emptyMap())); + } + + private static void setWriter(String name, Object writer) throws Exception { + final Field field = FeatureFlaggingSystem.class.getDeclaredField(name); + field.setAccessible(true); + field.set(null, writer); + } +} diff --git a/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlaggingSystemTest.java b/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlaggingSystemTest.java index 5b88389aefc..23b71f97807 100644 --- a/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlaggingSystemTest.java +++ b/products/feature-flagging/feature-flagging-agent/src/test/java/com/datadog/featureflag/FeatureFlaggingSystemTest.java @@ -16,6 +16,7 @@ import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; @@ -37,6 +38,54 @@ import org.junit.jupiter.api.Test; class FeatureFlaggingSystemTest { + @Test + @WithConfig(key = "trace.enabled", value = "false") + @WithConfig(key = FEATURE_FLAGS_CONFIGURATION_SOURCE, value = "agentless") + void traceDisabledHookFollowsActivationStopAndRestart() { + final SharedCommunicationObjects sco = sharedCommunicationObjects(); + final ExposureWriter writer = mock(ExposureWriter.class); + final FeatureFlaggingSystem.SystemInitializer initializer = + (ignoredSco, ignoredConfig) -> FeatureFlaggingSystem.initialize(null, writer); + for (int restart = 0; restart < 2; restart++) { + FeatureFlaggingSystem.start(sco, initializer); + assertFalse(FeatureFlaggingSystem.isShutdownHookRegistered()); + FeatureFlaggingGateway.activate(); + FeatureFlaggingGateway.activate(); + assertTrue(FeatureFlaggingSystem.isShutdownHookRegistered()); + FeatureFlaggingSystem.stop(); + assertFalse(FeatureFlaggingSystem.isShutdownHookRegistered()); + } + verify(writer, times(2)).init(); + verify(writer, times(2)).close(); + } + + @Test + @WithConfig(key = "trace.enabled", value = "false") + @WithConfig(key = FEATURE_FLAGS_CONFIGURATION_SOURCE, value = "agentless") + void failedActivationDoesNotLeaveAShutdownHook() { + final ExposureWriter writer = mock(ExposureWriter.class); + FeatureFlaggingSystem.start( + sharedCommunicationObjects(), + (sco, config) -> { + FeatureFlaggingSystem.initialize(null, writer); + throw new IllegalStateException("failed activation"); + }); + assertThrows(IllegalStateException.class, FeatureFlaggingGateway::activate); + assertFalse(FeatureFlaggingSystem.isShutdownHookRegistered()); + verify(writer).close(); + } + + @Test + @WithConfig(key = "trace.enabled", value = "true") + @WithConfig(key = FEATURE_FLAGS_CONFIGURATION_SOURCE, value = "agentless") + void tracingOwnsShutdownWithoutAnAdditionalFeatureFlagHook() { + FeatureFlaggingSystem.start( + sharedCommunicationObjects(), + (sco, config) -> FeatureFlaggingSystem.initialize(null, mock(ExposureWriter.class))); + FeatureFlaggingGateway.activate(); + assertFalse(FeatureFlaggingSystem.isShutdownHookRegistered()); + } + @AfterEach void resetFlagEvaluationGateway() { FeatureFlaggingSystem.stop(); diff --git a/products/feature-flagging/feature-flagging-lib/build.gradle.kts b/products/feature-flagging/feature-flagging-lib/build.gradle.kts index 4afd818b839..49a37d50b66 100644 --- a/products/feature-flagging/feature-flagging-lib/build.gradle.kts +++ b/products/feature-flagging/feature-flagging-lib/build.gradle.kts @@ -30,6 +30,7 @@ dependencies { testImplementation(libs.bundles.junit5) testImplementation(libs.bundles.mockito) + testImplementation(libs.okhttp3.mockwebserver) testImplementation(project(":products:feature-flagging:feature-flagging-config")) testImplementation(project(":utils:test-utils")) testImplementation(project(":dd-java-agent:testing")) diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApi.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApi.java index 769ebfd1dd1..59919840ffe 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApi.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApi.java @@ -7,32 +7,43 @@ import java.io.IOException; import java.io.InputStream; import java.net.ConnectException; +import java.util.function.Function; import java.util.function.Supplier; import javax.annotation.Nullable; import okhttp3.RequestBody; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -/** Sends Feature Flag events through a local EVP proxy, with a safe direct intake fallback. */ +/** Sends Feature Flag events through the process-wide Agentless EVP route selector. */ final class AgentlessFeatureFlagBackendApi implements BackendApi { private static final Logger LOGGER = LoggerFactory.getLogger(AgentlessFeatureFlagBackendApi.class); - private final BackendApi proxyApi; + private final FeatureFlagRouteSelector routeSelector; + private final Supplier proxyEndpointSupplier; + private final Function proxyApiFactory; private final Supplier directApiSupplier; private final String eventType; - private volatile BackendApi activeApi; + private BackendApi proxyApi; + private FeatureFlagRouteSelector.LocalRoute proxyRoute; + private volatile BackendApi directApi; private volatile boolean directApiCreationAttempted; AgentlessFeatureFlagBackendApi( - final BackendApi proxyApi, + @Nullable final BackendApi directApi, + final Supplier proxyEndpointSupplier, + final Function proxyApiFactory, final Supplier directApiSupplier, - final String eventType) { - this.proxyApi = proxyApi; + final String eventType, + final FeatureFlagRouteSelector routeSelector) { + this.directApi = directApi; + this.proxyEndpointSupplier = proxyEndpointSupplier; + this.proxyApiFactory = proxyApiFactory; this.directApiSupplier = directApiSupplier; this.eventType = eventType; - this.activeApi = proxyApi; + this.routeSelector = routeSelector; + this.directApiCreationAttempted = directApi != null; } @Override @@ -43,59 +54,115 @@ public T post( @Nullable final OkHttpUtils.CustomListener requestListener, final boolean requestCompression) throws IOException { - final BackendApi selectedApi = activeApi; + final SelectedApi selected = selectApi(); try { - return selectedApi.post( + return selected.api.post( uri, requestBody, responseParser, requestListener, requestCompression); } catch (final IOException exception) { - if (selectedApi != proxyApi || !isDefinitiveRejection(exception)) { + if (selected.localRoute == null) { throw exception; } - final BackendApi directApi = getOrCreateDirectApi(); - if (directApi == null) { + final BackendApi fallbackApi = getOrCreateDirectApi(); + // Route selection governs future batches; replay permission applies only to this batch. + // DIRECT is intentionally terminal, even for a transient or ambiguous local failure. + routeSelector.localFailure(selected.localRoute, fallbackApi != null); + if (fallbackApi == null || !isSafeToReplayDirectly(exception)) { throw exception; } - return directApi.post(uri, requestBody, responseParser, requestListener, requestCompression); + return fallbackApi.post( + uri, requestBody, responseParser, requestListener, requestCompression); } } - @Nullable - private BackendApi getOrCreateDirectApi() { - final BackendApi selectedApi = activeApi; - if (selectedApi != proxyApi) { - return selectedApi; + private SelectedApi selectApi() throws IOException { + if (routeSelector.tryBeginLocalRecovery()) { + String endpoint = null; + try { + endpoint = discoverProxyEndpoint(); + } finally { + routeSelector.localRecoveryFinished(endpoint); + } } - synchronized (this) { - final BackendApi currentApi = activeApi; - if (currentApi != proxyApi) { - return currentApi; - } - if (directApiCreationAttempted) { - return null; + final FeatureFlagRouteSelector.LocalRoute localRoute = routeSelector.localRoute(); + if (localRoute != null) { + return new SelectedApi(getOrCreateProxyApi(localRoute), localRoute); + } + + if (routeSelector.current() == FeatureFlagRouteSelector.Route.DIRECT) { + final BackendApi selectedDirectApi = getOrCreateDirectApi(); + if (selectedDirectApi != null) { + return new SelectedApi(selectedDirectApi, null); } + } + throw unavailableRoute(); + } + + @Nullable + private String discoverProxyEndpoint() { + try { + return proxyEndpointSupplier.get(); + } catch (final RuntimeException exception) { + LOGGER.debug("Could not discover the local Feature Flagging {} route", eventType, exception); + return null; + } + } + + private synchronized BackendApi getOrCreateProxyApi( + final FeatureFlagRouteSelector.LocalRoute localRoute) { + if (proxyRoute != localRoute) { + // Client construction uses the shared validated endpoint; it performs no discovery. + proxyApi = proxyApiFactory.apply(localRoute.endpoint); + proxyRoute = localRoute; + } + return proxyApi; + } - final BackendApi directApi = directApiSupplier.get(); - if (directApi != null) { - LOGGER.debug( - "Switching Feature Flagging {} delivery from the local EVP proxy to direct intake", - eventType); - activeApi = directApi; + @Nullable + private BackendApi getOrCreateDirectApi() { + if (directApiCreationAttempted) { + return directApi; + } + synchronized (this) { + if (!directApiCreationAttempted) { + directApi = directApiSupplier.get(); + directApiCreationAttempted = true; } - directApiCreationAttempted = true; return directApi; } } - private static boolean isDefinitiveRejection(final IOException exception) { + private IOException unavailableRoute() { + return new UnavailableRouteException(eventType); + } + + /** Expected drop while the shared selector waits for a usable route. */ + static final class UnavailableRouteException extends IOException { + private UnavailableRouteException(final String eventType) { + super("No Feature Flagging " + eventType + " delivery route is available"); + } + } + + private static boolean isSafeToReplayDirectly(final IOException exception) { if (exception instanceof ConnectException) { return true; } if (exception instanceof HttpResponseException) { final int statusCode = ((HttpResponseException) exception).getStatusCode(); - return statusCode == 403 || statusCode == 404 || statusCode == 405; + return statusCode == 404 || statusCode == 405; } return false; } + + private static final class SelectedApi { + private final BackendApi api; + private final FeatureFlagRouteSelector.LocalRoute localRoute; + + private SelectedApi( + final BackendApi api, @Nullable final FeatureFlagRouteSelector.LocalRoute localRoute) { + this.api = api; + this.localRoute = localRoute; + } + } } diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureWriterImpl.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureWriterImpl.java index fcd50e5dc34..0584e5701ad 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureWriterImpl.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/ExposureWriterImpl.java @@ -2,11 +2,13 @@ import static datadog.trace.api.telemetry.LogCollector.EXCLUDE_TELEMETRY; import static datadog.trace.util.AgentThreadFactory.AgentThread.FEATURE_FLAG_EXPOSURE_PROCESSOR; +import static datadog.trace.util.AgentThreadFactory.THREAD_JOIN_TIMOUT_MS; import static datadog.trace.util.AgentThreadFactory.newAgentThread; import static java.util.concurrent.TimeUnit.SECONDS; import datadog.common.queue.MessagePassingBlockingQueue; import datadog.common.queue.Queues; +import datadog.communication.BackendApi; import datadog.communication.ddagent.SharedCommunicationObjects; import datadog.trace.api.Config; import datadog.trace.api.featureflag.FeatureFlaggingGateway; @@ -17,22 +19,37 @@ import java.util.List; import java.util.Map; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Supplier; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -public class ExposureWriterImpl implements ExposureWriter { +public class ExposureWriterImpl implements ExposureWriter, FeatureFlagEventWriter { private static final Logger LOGGER = LoggerFactory.getLogger(ExposureWriterImpl.class); private static final int DEFAULT_CAPACITY = 1 << 16; // 65536 elements private static final int DEFAULT_FLUSH_INTERVAL_IN_SECONDS = 1; private static final int FLUSH_THRESHOLD = 100; private static final String EXPOSURES_ROUTE = "exposures"; + static final long SHUTDOWN_TIMEOUT_MILLIS = THREAD_JOIN_TIMOUT_MS; private final MessagePassingBlockingQueue queue; + private final ExposureSerializingHandler serializer; private final Thread serializerThread; + private final Object lifecycleLock = new Object(); + private final AtomicBoolean closed = new AtomicBoolean(false); + private final long shutdownTimeoutMillis; - public ExposureWriterImpl(final SharedCommunicationObjects sco, final Config config) { - this(DEFAULT_CAPACITY, DEFAULT_FLUSH_INTERVAL_IN_SECONDS, SECONDS, sco, config); + ExposureWriterImpl( + final SharedCommunicationObjects sco, + final Config config, + final FeatureFlagRouteSelector routeSelector) { + this( + DEFAULT_CAPACITY, + DEFAULT_FLUSH_INTERVAL_IN_SECONDS, + SECONDS, + new FeatureFlagBackendApiFactory(config, sco, FeatureFlagEventType.EXPOSURE, routeSelector), + config); } ExposureWriterImpl( @@ -55,35 +72,96 @@ public ExposureWriterImpl(final SharedCommunicationObjects sco, final Config con final TimeUnit timeUnit, final FeatureFlagBackendApiFactory backendApiFactory, final Config config) { - this.queue = Queues.mpscBlockingConsumerArrayQueue(capacity); - final ExposureSerializingHandler serializer = + this( + capacity, + flushInterval, + timeUnit, + backendApiFactory::create, + config, + SHUTDOWN_TIMEOUT_MILLIS); + } + + ExposureWriterImpl( + final int capacity, + final long flushInterval, + final TimeUnit timeUnit, + final Supplier backendApiSupplier, + final Config config, + final long shutdownTimeoutMillis) { + this( + Queues.mpscBlockingConsumerArrayQueue(capacity), + flushInterval, + timeUnit, + backendApiSupplier, + config, + shutdownTimeoutMillis); + } + + @VisibleForTesting + ExposureWriterImpl( + final MessagePassingBlockingQueue queue, + final long flushInterval, + final TimeUnit timeUnit, + final Supplier backendApiSupplier, + final Config config, + final long shutdownTimeoutMillis) { + this.queue = queue; + this.serializer = new ExposureSerializingHandler( - backendApiFactory, + backendApiSupplier, queue, flushInterval, timeUnit, FeatureFlagEvpContext.from(config), this::close); this.serializerThread = newAgentThread(FEATURE_FLAG_EXPOSURE_PROCESSOR, serializer); + this.shutdownTimeoutMillis = shutdownTimeoutMillis; } @Override public void init() { - FeatureFlaggingGateway.addExposureListener(this); - this.serializerThread.start(); + synchronized (lifecycleLock) { + if (closed.get()) { + return; + } + FeatureFlaggingGateway.addExposureListener(this); + this.serializerThread.start(); + } } @Override public void close() { - FeatureFlaggingGateway.removeExposureListener(this); - if (this.serializerThread.isAlive()) { - this.serializerThread.interrupt(); + final long deadline = System.nanoTime() + TimeUnit.MILLISECONDS.toNanos(shutdownTimeoutMillis); + requestShutdown(); + awaitTermination(deadline); + } + + @Override + public void requestShutdown() { + synchronized (lifecycleLock) { + if (!closed.compareAndSet(false, true)) { + return; + } + // Stop accepting new dispatches without waiting for producers already inside accept(). + FeatureFlaggingGateway.removeExposureListener(this); + // Polling wakes within 100 ms. An interrupt can race with, or abort, the final POST. + serializer.requestShutdown(); } } + @Override + public void awaitTermination(final long deadlineNanos) { + FeatureFlagEventWriter.awaitThread(serializerThread, deadlineNanos); + } + @Override public void accept(final ExposureEvent event) { - queue.offer(event); + // Keep producers lock-free, like flag evaluations. Shutdown is best effort: an offer that + // passes this check can race past the final drain and be dropped. Stale gateway dispatches + // entering after shutdown starts are rejected without extending the shutdown wait. + if (!closed.get()) { + queue.offer(event); + } } @VisibleForTesting @@ -107,9 +185,10 @@ private static class ExposureSerializingHandler implements Runnable { private final List buffer = new ArrayList<>(); private final Runnable errorCallback; + private final AtomicBoolean shutdownRequested = new AtomicBoolean(false); ExposureSerializingHandler( - final FeatureFlagBackendApiFactory backendApiFactory, + final Supplier backendApiSupplier, final MessagePassingBlockingQueue queue, final long flushInterval, final TimeUnit timeUnit, @@ -117,8 +196,7 @@ private static class ExposureSerializingHandler implements Runnable { final Runnable errorCallback) { this.queue = queue; this.cache = new LRUExposureCache(queue.capacity()); - this.evpPublisher = - new FeatureFlagEvpPublisher<>(backendApiFactory::create, ExposuresRequest.class); + this.evpPublisher = new FeatureFlagEvpPublisher<>(backendApiSupplier, ExposuresRequest.class); this.context = context; this.lastTicks = System.nanoTime(); @@ -129,6 +207,10 @@ private static class ExposureSerializingHandler implements Runnable { LOGGER.debug("starting exposure serializer"); } + void requestShutdown() { + shutdownRequested.set(true); + } + @Override public void run() { if (!evpPublisher.start()) { @@ -140,13 +222,23 @@ public void run() { runDutyCycle(); } catch (InterruptedException e) { Thread.currentThread().interrupt(); + } finally { + // Preserve an unrelated interrupt without passing it into OkHttp's final best-effort send. + final boolean wasInterrupted = Thread.interrupted(); + try { + drainAndFlush(); + } finally { + if (wasInterrupted) { + Thread.currentThread().interrupt(); + } + } } LOGGER.debug("exposure processor worker exited. submitting exposures stopped."); } private void runDutyCycle() throws InterruptedException { final Thread thread = Thread.currentThread(); - while (!thread.isInterrupted()) { + while (!thread.isInterrupted() && !shutdownRequested.get()) { ExposureEvent event; while ((event = queue.poll(100, TimeUnit.MILLISECONDS)) != null) { if (addToBuffer(event)) { @@ -162,6 +254,14 @@ private void consumeBatch() { queue.drain(this::addToBuffer, queue.size()); } + private void drainAndFlush() { + ExposureEvent event; + while ((event = queue.poll()) != null) { + addToBuffer(event); + } + flush(); + } + /** Adds an element to the buffer taking care of duplicated exposures thanks to the LRU cache */ private boolean addToBuffer(final ExposureEvent event) { if (cache.add(event)) { @@ -172,35 +272,41 @@ private boolean addToBuffer(final ExposureEvent event) { } protected void flushIfNecessary() { + if (!buffer.isEmpty() && shouldFlush()) { + flush(); + } + } + + private void flush() { if (buffer.isEmpty()) { return; } - if (shouldFlush()) { - final byte[] payload; - try { - final ExposuresRequest exposures = new ExposuresRequest(this.context, this.buffer); - payload = evpPublisher.serialize(exposures); - } catch (RuntimeException e) { - LOGGER.error(EXCLUDE_TELEMETRY, "Could not serialize exposures; dropping batch", e); - this.buffer.clear(); - return; - } - try { - evpPublisher.post(EXPOSURES_ROUTE, payload); - } catch (Exception e) { - LOGGER.debug("Could not submit exposures", e); - } finally { - // Best-effort delivery must not retry an ambiguously accepted batch. A later definitive - // proxy rejection could otherwise replay the same exposures through direct intake. - this.buffer.clear(); - } + final byte[] payload; + try { + final ExposuresRequest exposures = new ExposuresRequest(this.context, this.buffer); + payload = evpPublisher.serialize(exposures); + } catch (RuntimeException e) { + LOGGER.error(EXCLUDE_TELEMETRY, "Could not serialize exposures; dropping batch", e); + this.buffer.clear(); + return; + } + try { + evpPublisher.post(EXPOSURES_ROUTE, payload); + } catch (AgentlessFeatureFlagBackendApi.UnavailableRouteException ignored) { + // The shared selector reports unavailable/recovered transitions once for both writers. + } catch (Exception e) { + LOGGER.debug("Could not submit exposures", e); + } finally { + // Best-effort delivery must not retry an ambiguously accepted batch. A later definitive + // proxy rejection could otherwise replay the same exposures through direct intake. + this.buffer.clear(); } } private boolean shouldFlush() { long nanoTime = System.nanoTime(); long ticks = nanoTime - lastTicks; - if (ticks > ticksRequiredToFlush || queue.size() >= FLUSH_THRESHOLD) { + if (ticks > ticksRequiredToFlush || buffer.size() >= FLUSH_THRESHOLD) { lastTicks = nanoTime; return true; } diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java index 451b903ecc7..40c34395ab8 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagBackendApiFactory.java @@ -1,6 +1,8 @@ package com.datadog.featureflag; +import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V2_EVP_PROXY_ENDPOINT; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_AGENTLESS; +import static java.util.Collections.emptyList; import datadog.communication.BackendApi; import datadog.communication.BackendApiFactory; @@ -8,7 +10,9 @@ import datadog.communication.http.HttpRetryPolicy; import datadog.trace.api.Config; import datadog.trace.api.intake.Intake; +import java.net.ConnectException; import javax.annotation.Nullable; +import okhttp3.Response; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -17,65 +21,130 @@ final class FeatureFlagBackendApiFactory { private static final Logger LOGGER = LoggerFactory.getLogger(FeatureFlagBackendApiFactory.class); + /** Retries connection refusal only; automatic retries and redirects remain disabled. */ + static final HttpRetryPolicy.Factory PRE_SEND_RETRY = + new HttpRetryPolicy.Factory(3, 100, 2.0) { + @Override + public HttpRetryPolicy create() { + return new PreSendRetryPolicy(); + } + }; + + private static final class PreSendRetryPolicy extends HttpRetryPolicy { + private int attemptsLeft = 3; + + private PreSendRetryPolicy() { + super(3, 100, 2.0, false); + } + + @Override + public boolean shouldRetry(final Exception exception) { + if (exception instanceof ConnectException && attemptsLeft > 0) { + attemptsLeft--; + return true; + } + return false; + } + + @Override + public boolean shouldRetry(@Nullable final Response response) { + return false; + } + } + private final Config config; private final BackendApiFactory backendApiFactory; private final FeatureFlagEventType eventType; + private final FeatureFlagRouteSelector routeSelector; FeatureFlagBackendApiFactory( final Config config, final SharedCommunicationObjects sharedCommunicationObjects, final FeatureFlagEventType eventType) { - this(config, new BackendApiFactory(config, sharedCommunicationObjects), eventType); + this(config, sharedCommunicationObjects, eventType, new FeatureFlagRouteSelector()); + } + + FeatureFlagBackendApiFactory( + final Config config, + final SharedCommunicationObjects sharedCommunicationObjects, + final FeatureFlagEventType eventType, + final FeatureFlagRouteSelector routeSelector) { + this( + config, + initializedBackendFactory(config, sharedCommunicationObjects), + eventType, + routeSelector); + } + + private static BackendApiFactory initializedBackendFactory( + final Config config, final SharedCommunicationObjects sharedCommunicationObjects) { + if (CONFIGURATION_SOURCE_AGENTLESS.equals(config.getFeatureFlaggingConfigurationSource()) + && config.getApiKey() != null + && !config.getApiKey().isEmpty()) { + // Direct intake is constructed before Agent discovery. Initialize settings first so its + // cached HTTP client uses configured timeouts even when Feature Flagging starts first. + sharedCommunicationObjects.createRemaining(config); + } + return new BackendApiFactory(config, sharedCommunicationObjects, true); } FeatureFlagBackendApiFactory( final Config config, final BackendApiFactory backendApiFactory, final FeatureFlagEventType eventType) { + this(config, backendApiFactory, eventType, new FeatureFlagRouteSelector()); + } + + FeatureFlagBackendApiFactory( + final Config config, + final BackendApiFactory backendApiFactory, + final FeatureFlagEventType eventType, + final FeatureFlagRouteSelector routeSelector) { this.config = config; this.backendApiFactory = backendApiFactory; this.eventType = eventType; + this.routeSelector = routeSelector; } @Nullable BackendApi create() { - final boolean directFallbackAvailable = - CONFIGURATION_SOURCE_AGENTLESS.equals(config.getFeatureFlaggingConfigurationSource()) - && hasDirectCredentials(); - final BackendApi proxyApi = - directFallbackAvailable - ? backendApiFactory.createEvpProxyApi( - Intake.EVENT_PLATFORM, - eventType.responseCompressionEnabled(), - HttpRetryPolicy.Factory.NEVER_RETRY) - : backendApiFactory.createEvpProxyApi( - Intake.EVENT_PLATFORM, eventType.responseCompressionEnabled()); - if (!CONFIGURATION_SOURCE_AGENTLESS.equals(config.getFeatureFlaggingConfigurationSource())) { - if (proxyApi == null) { - LOGGER.warn( - "Feature Flagging {} delivery is disabled because the local Agent does not support the EVP proxy", - eventType.logName()); - } - return proxyApi; - } - - if (proxyApi != null) { - if (directFallbackAvailable) { - return new AgentlessFeatureFlagBackendApi( - proxyApi, this::createDirectApi, eventType.logName()); - } - return proxyApi; + final boolean agentless = + CONFIGURATION_SOURCE_AGENTLESS.equals(config.getFeatureFlaggingConfigurationSource()); + if (!agentless) { + // Remote Config stays Agent-only. Fixed v2 intentionally replaces Java's previous + // discovered endpoint; only connection refusal retains a bounded retry. + return backendApiFactory.createEvpProxyApiForEndpoint( + Intake.EVENT_PLATFORM, + eventType.responseCompressionEnabled(), + PRE_SEND_RETRY, + V2_EVP_PROXY_ENDPOINT); } final BackendApi directApi = createDirectApi(); - if (directApi != null) { - return directApi; - } + routeSelector.initialize(() -> discoverProxyEndpoint(false), directApi != null); + return new AgentlessFeatureFlagBackendApi( + directApi, + () -> discoverProxyEndpoint(true), + endpoint -> + backendApiFactory.createEvpProxyApiForEndpoint( + Intake.EVENT_PLATFORM, + eventType.responseCompressionEnabled(), + HttpRetryPolicy.Factory.NEVER_RETRY, + endpoint), + this::createDirectApi, + eventType.logName(), + routeSelector); + } - LOGGER.warn( - "Feature Flagging {} delivery is disabled because no compatible local EVP proxy or direct intake credentials are available", - eventType.logName()); - return null; + @Nullable + private String discoverProxyEndpoint(final boolean forceDiscovery) { + try { + // Identity-forwarding requirements belong with SDK identity emission in the identity PR. + return backendApiFactory.discoverEvpProxyEndpoint(forceDiscovery, emptyList()); + } catch (final RuntimeException exception) { + LOGGER.debug("Could not discover the local Feature Flagging route", exception); + return null; + } } private boolean hasDirectCredentials() { diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagEventWriter.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagEventWriter.java new file mode 100644 index 00000000000..1c7ad1219da --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagEventWriter.java @@ -0,0 +1,29 @@ +package com.datadog.featureflag; + +import static datadog.trace.util.AgentThreadFactory.THREAD_JOIN_TIMOUT_MS; +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static java.util.concurrent.TimeUnit.NANOSECONDS; + +/** Internal two-phase shutdown for writers sharing one subsystem termination budget. */ +interface FeatureFlagEventWriter extends AutoCloseable { + void requestShutdown(); + + void awaitTermination(long deadlineNanos); + + static long shutdownDeadline() { + return System.nanoTime() + MILLISECONDS.toNanos(THREAD_JOIN_TIMOUT_MS); + } + + static void awaitThread(Thread worker, long deadlineNanos) { + final long remaining = deadlineNanos - System.nanoTime(); + if (worker == Thread.currentThread() || remaining <= 0) { + return; + } + try { + // timedJoin preserves sub-millisecond budgets; never turn an expired budget into join(0). + NANOSECONDS.timedJoin(worker, remaining); + } catch (InterruptedException interrupted) { + Thread.currentThread().interrupt(); + } + } +} diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagRouteSelector.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagRouteSelector.java new file mode 100644 index 00000000000..c18e724cbed --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FeatureFlagRouteSelector.java @@ -0,0 +1,122 @@ +package com.datadog.featureflag; + +import java.util.concurrent.TimeUnit; +import java.util.function.LongSupplier; +import java.util.function.Supplier; +import javax.annotation.Nullable; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** Process-wide route state shared by all Feature Flagging event writers. */ +final class FeatureFlagRouteSelector { + + private static final Logger LOGGER = LoggerFactory.getLogger(FeatureFlagRouteSelector.class); + + static final long DEFAULT_RECOVERY_INTERVAL_NANOS = TimeUnit.MINUTES.toNanos(1); + + enum Route { + UNINITIALIZED, + LOCAL, + DIRECT, + UNAVAILABLE + } + + private final LongSupplier nanoTime; + private final long recoveryIntervalNanos; + private volatile Route route = Route.UNINITIALIZED; + private volatile long nextLocalDiscoveryNanos; + private LocalRoute localRoute; + private boolean recoveryInProgress; + + /** A validated endpoint and its identity, replaced on every successful recovery. */ + static final class LocalRoute { + final String endpoint; + + private LocalRoute(final String endpoint) { + this.endpoint = endpoint; + } + } + + FeatureFlagRouteSelector() { + this(System::nanoTime, DEFAULT_RECOVERY_INTERVAL_NANOS); + } + + FeatureFlagRouteSelector(final LongSupplier nanoTime, final long recoveryIntervalNanos) { + this.nanoTime = nanoTime; + this.recoveryIntervalNanos = recoveryIntervalNanos; + } + + synchronized Route initialize( + final Supplier discoverEndpoint, final boolean directAvailable) { + if (route == Route.UNINITIALIZED) { + // Startup happens once across both writers. Later unavailable probes run outside this lock. + final String endpoint = discoverEndpoint.get(); + if (endpoint != null) { + localRoute = new LocalRoute(endpoint); + route = Route.LOCAL; + } else if (directAvailable) { + route = Route.DIRECT; + } else { + route = Route.UNAVAILABLE; + scheduleLocalDiscovery(); + logUnavailable(); + } + } + return route; + } + + Route current() { + return route; + } + + synchronized @Nullable LocalRoute localRoute() { + return route == Route.LOCAL ? localRoute : null; + } + + synchronized Route localFailure(final LocalRoute failedRoute, final boolean directAvailable) { + // An in-flight send on an old endpoint cannot invalidate a newly recovered route. + if (route == Route.LOCAL && localRoute == failedRoute) { + if (directAvailable) { + route = Route.DIRECT; + } else { + route = Route.UNAVAILABLE; + scheduleLocalDiscovery(); + logUnavailable(); + } + } + return route; + } + + synchronized boolean tryBeginLocalRecovery() { + if (route != Route.UNAVAILABLE + || recoveryInProgress + || nanoTime.getAsLong() - nextLocalDiscoveryNanos < 0) { + return false; + } + recoveryInProgress = true; + return true; + } + + synchronized void localRecoveryFinished(@Nullable final String endpoint) { + recoveryInProgress = false; + if (route == Route.UNAVAILABLE) { + if (endpoint != null) { + localRoute = new LocalRoute(endpoint); + route = Route.LOCAL; + LOGGER.info("Feature Flagging event delivery recovered through the local EVP proxy"); + } else { + scheduleLocalDiscovery(); + } + } + } + + private void scheduleLocalDiscovery() { + nextLocalDiscoveryNanos = nanoTime.getAsLong() + recoveryIntervalNanos; + } + + private static void logUnavailable() { + LOGGER.warn( + "Feature Flagging events are being dropped because no local EVP proxy or direct intake" + + " route is available. Make the Agent reachable, or configure DD_API_KEY and a valid DD_SITE."); + } +} diff --git a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationWriterImpl.java b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationWriterImpl.java index 2da8ffe0256..c2a6231076c 100644 --- a/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationWriterImpl.java +++ b/products/feature-flagging/feature-flagging-lib/src/main/java/com/datadog/featureflag/FlagEvaluationWriterImpl.java @@ -48,10 +48,11 @@ * contend only on the MPSC queue, never on a monitor, so evaluation threads do not serialize * against each other. Shutdown: close() drains the queue and performs a final flush before the * worker thread exits. Because enqueue is lock-free, a producer can still offer during shutdown; - * close() sweeps the queue once the worker has been joined, counting any remainder as a closed drop - * so shutdown loss is observable rather than silent. + * close() sweeps the queue once the worker has been joined, counting the observed remainder as + * closed drops. A producer paused before offer can resume after this sweep; shutdown is best-effort + * for events racing close, not a producer fence. */ -public class FlagEvaluationWriterImpl implements FlagEvaluationWriter { +public class FlagEvaluationWriterImpl implements FlagEvaluationWriter, FeatureFlagEventWriter { private static final Logger LOGGER = LoggerFactory.getLogger(FlagEvaluationWriterImpl.class); @@ -99,12 +100,17 @@ private static void countMetric(final String metricName, final long value, final private final ConcurrentHashMap contextTruncatedCounts = new ConcurrentHashMap<>(); - public FlagEvaluationWriterImpl(final SharedCommunicationObjects sco, final Config config) { + FlagEvaluationWriterImpl( + final SharedCommunicationObjects sco, + final Config config, + final FeatureFlagRouteSelector routeSelector) { this( DEFAULT_CAPACITY, FLUSH_INTERVAL_SECONDS, SECONDS, - new FeatureFlagBackendApiFactory(config, sco, FeatureFlagEventType.FLAG_EVALUATION)::create, + new FeatureFlagBackendApiFactory( + config, sco, FeatureFlagEventType.FLAG_EVALUATION, routeSelector) + ::create, config); } @@ -158,7 +164,13 @@ int aggregatorFullTierSizeForTest() { @Override public void close() { - final boolean workerRunning; + final long deadline = FeatureFlagEventWriter.shutdownDeadline(); + requestShutdown(); + awaitTermination(deadline); + } + + @Override + public void requestShutdown() { synchronized (lifecycleLock) { if (!closed.compareAndSet(false, true)) { return; @@ -166,31 +178,26 @@ public void close() { // Disable and deregister from the gateway so no new events are enqueued. FeatureFlaggingGateway.setFlagEvaluationEnqueueEnabled(false); FeatureFlaggingGateway.setFlagEvalWriter(null); - workerRunning = this.serializerThread.isAlive(); - if (workerRunning) { - // Ask the worker to drain the queue and final-flush, then interrupt to wake it from poll(). - serializer.requestShutdown(); - this.serializerThread.interrupt(); - } + // Polling wakes within 100 ms; do not interrupt a current or final HTTP request. + serializer.requestShutdown(); } + } + + @Override + public void awaitTermination(final long deadlineNanos) { if (Thread.currentThread() == this.serializerThread) { return; } - if (workerRunning) { - try { - // Bounded wait for the worker's final flush so queued events are not lost on shutdown. - this.serializerThread.join(TimeUnit.SECONDS.toMillis(5)); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } - } + FeatureFlagEventWriter.awaitThread(serializerThread, deadlineNanos); // enqueue() is lock-free, so a producer that passed the closed check before close() ran can // still land an event after the worker's final drain. Sweep the remainder so that loss is // counted rather than silently stranded in a queue nobody polls again. Only safe once the // worker is gone: the queue is single-consumer, so sweeping alongside a live worker would // break that contract. If join() timed out the worker is still draining, so skip the sweep. - if (!this.serializerThread.isAlive()) { - sweepAndCountResidualEvents(); + synchronized (lifecycleLock) { + if (!this.serializerThread.isAlive()) { + sweepAndCountResidualEvents(); + } } } @@ -218,8 +225,8 @@ public void enqueue(final FlagEvalEvent event) { // Deliberately lock-free: the hand-off queue is MPSC by design, so serializing producers on a // monitor here would negate that and turn every evaluation in every application thread into // contention on one lock. A producer that passed the check above can still offer after close() - // has started; that residue is accounted for by the worker's bounded post-drain passes and by - // close()'s post-join sweep, so shutdown loss stays observable. + // has started. The final drain and post-join sweep handle observed residue, but cannot account + // for an offer from a producer that resumes after both have finished. // // Safe publication of the event (including the context snapshot built by the hook) comes from // the queue's own offer/poll ordering, not from any monitor held here. @@ -349,12 +356,9 @@ public void run() { // On exit (interrupt or shutdown request), drain everything still buffered and flush it so // queued events are not lost on shutdown. // - // close() interrupts this thread to break it out of poll(). The final flush does socket - // I/O, and OkHttp fails fast on a thread whose interrupt flag is set, so clear the flag - // for the duration of the drain and restore it afterwards. Without this the flush that - // close() exists to guarantee throws IOException, the aggregated rows are discarded by - // the clear() in flush(), and the loss is invisible to the drop counters because those - // rows already left the queue. + // Normal close only signals shutdown. If an unrelated interrupt ends the duty cycle, + // clear it during the final socket I/O and restore it afterwards; OkHttp otherwise fails + // immediately without attempting the remaining batch. final boolean wasInterrupted = Thread.interrupted(); try { drainAndFlush(); @@ -468,6 +472,8 @@ void flush() { for (final byte[] payload : payloads.bodies) { evpPublisher.post(FLAG_EVALUATION_ROUTE, payload); } + } catch (AgentlessFeatureFlagBackendApi.UnavailableRouteException ignored) { + // The shared selector reports unavailable/recovered transitions once for both writers. } catch (Exception e) { LOGGER.error("Could not submit flag evaluations", e); } finally { diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java index 117c72ba2a1..7ae0c3de5c3 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/AgentlessFeatureFlagBackendApiTest.java @@ -3,6 +3,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; import datadog.communication.BackendApi; import datadog.communication.HttpResponseException; @@ -15,7 +16,14 @@ import java.net.SocketTimeoutException; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.LongSupplier; +import java.util.function.Supplier; import java.util.stream.Stream; import javax.annotation.Nullable; import okhttp3.MediaType; @@ -28,16 +36,46 @@ class AgentlessFeatureFlagBackendApiTest { + @Test + void recoveryErrorDoesNotPermanentlyBlockTheSiblingWriter() throws Exception { + final AtomicLong clock = new AtomicLong(); + final FeatureFlagRouteSelector selector = new FeatureFlagRouteSelector(clock::get, 10); + final RecordingBackendApi recovered = new RecordingBackendApi(); + final AgentlessFeatureFlagBackendApi exposures = + createApi( + null, + null, + () -> { + throw new LinkageError("probe failure"); + }, + () -> null, + "exposure", + selector); + final AgentlessFeatureFlagBackendApi evaluations = + createApi(null, null, () -> recovered, () -> null, "flag evaluation", selector); + + clock.set(10); + assertThrows( + LinkageError.class, + () -> exposures.post("exposures", requestBody("first"), stream -> null, null, false)); + clock.set(20); + evaluations.post("flagevaluation", requestBody("second"), stream -> null, null, false); + + assertEquals(1, recovered.calls); + } + @ParameterizedTest - @ValueSource(ints = {403, 404, 405}) + @ValueSource(ints = {404, 405}) void replaysRejectedBatchDirectlyAndKeepsDirectRoute(final int statusCode) throws Exception { final RecordingBackendApi local = new RecordingBackendApi(new HttpResponseException(statusCode, "rejected")); final RecordingBackendApi direct = new RecordingBackendApi(); final AtomicInteger directApiCreations = new AtomicInteger(); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi( + createApi( local, + null, + () -> local, () -> { directApiCreations.incrementAndGet(); return direct; @@ -66,7 +104,7 @@ void fallsBackAfterConnectionRefusal(final String route, final String eventType) new RecordingBackendApi(new ConnectException("connection refused")); final RecordingBackendApi direct = new RecordingBackendApi(); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi(local, () -> direct, eventType); + createApi(local, null, () -> local, () -> direct, eventType); api.post(route, requestBody(eventType), stream -> null, null, false); @@ -75,37 +113,154 @@ void fallsBackAfterConnectionRefusal(final String route, final String eventType) } @Test - void doesNotReturnToLocalRouteAfterSwitchingToDirectIntake() throws Exception { - final RecordingBackendApi local = + void keepsDirectRouteStickyAfterLocalFailure() throws Exception { + final AtomicLong clock = new AtomicLong(); + final RecordingBackendApi unavailableLocal = new RecordingBackendApi(new ConnectException("connection refused")); + final RecordingBackendApi recoveredLocal = new RecordingBackendApi(); final RecordingBackendApi direct = new RecordingBackendApi(); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi(local, () -> direct, "exposure"); + createApi( + unavailableLocal, null, () -> recoveredLocal, () -> direct, "exposure", clock::get, 10); api.post("exposures", requestBody("first"), stream -> null, null, false); - direct.failure = new IOException("direct intake failed"); + clock.set(9); + api.post("exposures", requestBody("second"), stream -> null, null, false); + clock.set(10); + api.post("exposures", requestBody("third"), stream -> null, null, false); - assertThrows( - IOException.class, - () -> api.post("exposures", requestBody("second"), stream -> null, null, false)); - assertEquals(1, local.calls); - assertEquals(2, direct.calls); + assertEquals(1, unavailableLocal.calls); + assertEquals(3, direct.calls); + assertEquals(0, recoveredLocal.calls); } @ParameterizedTest - @ValueSource(ints = {429, 500}) - void doesNotReplayAmbiguousHttpFailure(final int statusCode) { - assertNoDirectReplay(new HttpResponseException(statusCode, "ambiguous")); + @ValueSource(ints = {403, 429, 500}) + void doesNotReplayAmbiguousHttpFailureButSwitchesFutureBatches(final int statusCode) + throws Exception { + assertNoSameBatchReplayButUsesDirectForNext(new HttpResponseException(statusCode, "ambiguous")); } @Test - void doesNotReplayTimeout() { - assertNoDirectReplay(new SocketTimeoutException("timed out")); + void doesNotReplayTimeoutButSwitchesFutureBatches() throws Exception { + assertNoSameBatchReplayButUsesDirectForNext(new SocketTimeoutException("timed out")); + } + + @Test + void doesNotReplayConnectionResetButSwitchesFutureBatches() throws Exception { + assertNoSameBatchReplayButUsesDirectForNext(new SocketException("connection reset")); + } + + @Test + void startsDirectAndNeverProbesLocal() throws Exception { + final AtomicLong clock = new AtomicLong(); + final RecordingBackendApi direct = new RecordingBackendApi(); + final RecordingBackendApi recoveredLocal = new RecordingBackendApi(); + final AtomicInteger proxyApiCreations = new AtomicInteger(); + final AgentlessFeatureFlagBackendApi api = + createApi( + null, + direct, + () -> { + proxyApiCreations.incrementAndGet(); + return recoveredLocal; + }, + () -> direct, + "exposure", + clock::get, + 10); + + api.post("exposures", requestBody("first"), stream -> null, null, false); + clock.set(9); + api.post("exposures", requestBody("second"), stream -> null, null, false); + clock.set(10); + api.post("exposures", requestBody("third"), stream -> null, null, false); + + assertEquals(0, proxyApiCreations.get()); + assertEquals(3, direct.calls); + assertEquals(0, recoveredLocal.calls); + } + + @Test + void concurrentSendersDoNotBlockOnOrDuplicateARecoveryProbe() throws Exception { + final AtomicLong clock = new AtomicLong(10); + final RecordingBackendApi recoveredLocal = new RecordingBackendApi(); + final AtomicInteger proxyApiCreations = new AtomicInteger(); + final CountDownLatch probeStarted = new CountDownLatch(1); + final CountDownLatch releaseProbe = new CountDownLatch(1); + final AgentlessFeatureFlagBackendApi api = + createApi( + null, + null, + () -> { + proxyApiCreations.incrementAndGet(); + probeStarted.countDown(); + try { + assertTrue(releaseProbe.await(5, TimeUnit.SECONDS)); + } catch (final InterruptedException exception) { + Thread.currentThread().interrupt(); + throw new AssertionError(exception); + } + return recoveredLocal; + }, + () -> null, + "flag evaluation", + clock::get, + 10); + + // Construction schedules the first probe for t=20. + clock.set(20); + final CompletableFuture recoveringPost = + CompletableFuture.runAsync( + () -> { + try { + api.post("flagevaluation", requestBody("probe"), stream -> null, null, false); + } catch (final IOException exception) { + throw new AssertionError(exception); + } + }); + assertTrue(probeStarted.await(5, TimeUnit.SECONDS)); + + assertThrows( + IOException.class, + () -> api.post("flagevaluation", requestBody("parallel"), stream -> null, null, false)); + releaseProbe.countDown(); + recoveringPost.get(5, TimeUnit.SECONDS); + + assertEquals(1, proxyApiCreations.get()); + assertEquals(1, recoveredLocal.calls); } @Test - void doesNotReplayConnectionReset() { - assertNoDirectReplay(new SocketException("connection reset")); + void failedRecoveryIsStickyForAnotherCooldown() throws Exception { + final AtomicLong clock = new AtomicLong(); + final AtomicInteger proxyApiCreations = new AtomicInteger(); + final AgentlessFeatureFlagBackendApi api = + createApi( + null, + null, + () -> { + proxyApiCreations.incrementAndGet(); + return null; + }, + () -> null, + "exposure", + clock::get, + 10); + + clock.set(10); + assertThrows( + IOException.class, + () -> api.post("exposures", requestBody("first"), stream -> null, null, false)); + assertThrows( + IOException.class, + () -> api.post("exposures", requestBody("second"), stream -> null, null, false)); + clock.set(20); + assertThrows( + IOException.class, + () -> api.post("exposures", requestBody("third"), stream -> null, null, false)); + + assertEquals(2, proxyApiCreations.get()); } @Test @@ -114,8 +269,10 @@ void doesNotRetryDirectApiCreationWhenFallbackIsUnavailable() { new RecordingBackendApi(new HttpResponseException(404, "rejected")); final AtomicInteger directApiCreations = new AtomicInteger(); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi( + createApi( local, + null, + () -> local, () -> { directApiCreations.incrementAndGet(); return null; @@ -126,20 +283,217 @@ void doesNotRetryDirectApiCreationWhenFallbackIsUnavailable() { HttpResponseException.class, () -> api.post("exposures", requestBody("first"), stream -> null, null, false)); assertThrows( - HttpResponseException.class, + IOException.class, () -> api.post("exposures", requestBody("second"), stream -> null, null, false)); - assertEquals(2, local.calls); + assertEquals(1, local.calls); + assertEquals(1, directApiCreations.get()); + } + + @Test + void permitsUnavailableStartupSoLocalDeliveryCanRecover() { + final AgentlessFeatureFlagBackendApi api = + createApi(null, null, () -> null, () -> null, "flag evaluation"); + + assertThrows( + IOException.class, + () -> api.post("flagevaluation", requestBody("first"), stream -> null, null, false)); + } + + @Test + void propagatesDirectFailuresWithoutTryingToFallback() { + final SocketTimeoutException failure = new SocketTimeoutException("direct timeout"); + final RecordingBackendApi direct = new RecordingBackendApi(failure); + final AtomicInteger proxyApiCreations = new AtomicInteger(); + final AgentlessFeatureFlagBackendApi api = + createApi( + null, + direct, + () -> { + proxyApiCreations.incrementAndGet(); + return null; + }, + () -> direct, + "exposure"); + + assertSame( + failure, + assertThrows( + SocketTimeoutException.class, + () -> api.post("exposures", requestBody("direct"), stream -> null, null, false))); + assertEquals(0, proxyApiCreations.get()); + assertEquals(1, direct.calls); + } + + @Test + void concurrentProxyFailuresOnlyTransitionTheMatchingRouteOnce() throws Exception { + final CountDownLatch localCallsStarted = new CountDownLatch(2); + final CountDownLatch releaseFailures = new CountDownLatch(1); + final BackendApi local = + new BackendApi() { + @Override + public T post( + final String uri, + final RequestBody requestBody, + final IOThrowingFunction responseParser, + @Nullable final OkHttpUtils.CustomListener requestListener, + final boolean requestCompression) + throws IOException { + localCallsStarted.countDown(); + try { + assertTrue(releaseFailures.await(5, TimeUnit.SECONDS)); + } catch (final InterruptedException exception) { + Thread.currentThread().interrupt(); + throw new AssertionError(exception); + } + throw new HttpResponseException(404, "local route missing"); + } + }; + final AtomicInteger directCalls = new AtomicInteger(); + final BackendApi direct = + new BackendApi() { + @Override + public T post( + final String uri, + final RequestBody requestBody, + final IOThrowingFunction responseParser, + @Nullable final OkHttpUtils.CustomListener requestListener, + final boolean requestCompression) { + directCalls.incrementAndGet(); + return null; + } + }; + final AtomicInteger directApiCreations = new AtomicInteger(); + final AgentlessFeatureFlagBackendApi api = + createApi( + local, + null, + () -> local, + () -> { + directApiCreations.incrementAndGet(); + return direct; + }, + "exposure"); + + final CompletableFuture first = + CompletableFuture.runAsync(() -> postWithoutFailure(api, "first concurrent proxy request")); + final CompletableFuture second = + CompletableFuture.runAsync( + () -> postWithoutFailure(api, "second concurrent proxy request")); + assertTrue(localCallsStarted.await(5, TimeUnit.SECONDS)); + releaseFailures.countDown(); + first.get(5, TimeUnit.SECONDS); + second.get(5, TimeUnit.SECONDS); + assertEquals(1, directApiCreations.get()); + assertEquals(2, directCalls.get()); + } + + @Test + void sharesRouteTransitionsAcrossExposureAndFlagEvaluationWriters() throws Exception { + final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(); + final RecordingBackendApi exposureLocal = + new RecordingBackendApi(new SocketTimeoutException("ambiguous timeout")); + final RecordingBackendApi exposureDirect = new RecordingBackendApi(); + final RecordingBackendApi evaluationLocal = new RecordingBackendApi(); + final RecordingBackendApi evaluationDirect = new RecordingBackendApi(); + final AgentlessFeatureFlagBackendApi exposureApi = + createApi( + exposureLocal, + exposureDirect, + () -> exposureLocal, + () -> exposureDirect, + "exposure", + routeSelector); + final AgentlessFeatureFlagBackendApi evaluationApi = + createApi( + evaluationLocal, + evaluationDirect, + () -> evaluationLocal, + () -> evaluationDirect, + "flag evaluation", + routeSelector); + + assertThrows( + SocketTimeoutException.class, + () -> exposureApi.post("exposures", requestBody("first"), stream -> null, null, false)); + evaluationApi.post("flagevaluation", requestBody("second"), stream -> null, null, false); + + assertEquals(1, exposureLocal.calls); + assertEquals(0, exposureDirect.calls); + assertEquals(0, evaluationLocal.calls); + assertEquals(1, evaluationDirect.calls); + } + + @Test + void failedRecoveryExceptionRemainsUnavailable() { + final AtomicLong clock = new AtomicLong(); + final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(clock::get, 10); + final AgentlessFeatureFlagBackendApi api = + createApi( + null, + null, + () -> { + throw new IllegalStateException("discovery failed"); + }, + () -> null, + "flag evaluation", + routeSelector); + clock.set(10); + assertThrows( + IOException.class, + () -> api.post("flagevaluation", requestBody("missing"), stream -> null, null, false)); + assertEquals(FeatureFlagRouteSelector.Route.UNAVAILABLE, routeSelector.current()); } - private static void assertNoDirectReplay(final IOException failure) { + @Test + void sharedDirectRouteWithoutWriterClientIsUnavailable() { + final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(); + routeSelector.initialize(() -> null, true); + final AgentlessFeatureFlagBackendApi api = + createApi(null, null, () -> null, () -> null, "flag evaluation", routeSelector); + + assertThrows( + IOException.class, + () -> api.post("flagevaluation", requestBody("missing"), stream -> null, null, false)); + assertEquals(FeatureFlagRouteSelector.Route.DIRECT, routeSelector.current()); + } + + @Test + void workingDirectRouteDoesNotAttemptRecovery() throws Exception { + final AtomicLong clock = new AtomicLong(); + final RecordingBackendApi direct = new RecordingBackendApi(); + final AtomicInteger proxyApiCreations = new AtomicInteger(); + final AgentlessFeatureFlagBackendApi api = + createApi( + null, + direct, + () -> { + proxyApiCreations.incrementAndGet(); + throw new IllegalStateException("discovery failed"); + }, + () -> direct, + "exposure", + clock::get, + 10); + + clock.set(10); + api.post("exposures", requestBody("survives recovery failure"), stream -> null, null, false); + + assertEquals(1, direct.calls); + assertEquals(0, proxyApiCreations.get()); + } + + private static void assertNoSameBatchReplayButUsesDirectForNext(final IOException failure) + throws Exception { final RecordingBackendApi local = new RecordingBackendApi(failure); final RecordingBackendApi direct = new RecordingBackendApi(); final AtomicInteger directApiCreations = new AtomicInteger(); final AgentlessFeatureFlagBackendApi api = - new AgentlessFeatureFlagBackendApi( + createApi( local, + null, + () -> local, () -> { directApiCreations.incrementAndGet(); return direct; @@ -149,16 +503,74 @@ private static void assertNoDirectReplay(final IOException failure) { assertThrows( IOException.class, () -> api.post("flagevaluation", requestBody("evaluation"), stream -> null, null, false)); + api.post("flagevaluation", requestBody("next"), stream -> null, null, false); assertEquals(1, local.calls); - assertEquals(0, direct.calls); - assertEquals(0, directApiCreations.get()); + assertEquals(1, direct.calls); + assertEquals(1, directApiCreations.get()); } private static RequestBody requestBody(final String value) { return RequestBody.create(MediaType.parse("application/json"), value); } + private static AgentlessFeatureFlagBackendApi createApi( + BackendApi local, + BackendApi direct, + Supplier discoverLocal, + Supplier createDirect, + String eventType) { + return createApi( + local, direct, discoverLocal, createDirect, eventType, new FeatureFlagRouteSelector()); + } + + private static AgentlessFeatureFlagBackendApi createApi( + BackendApi local, + BackendApi direct, + Supplier discoverLocal, + Supplier createDirect, + String eventType, + LongSupplier clock, + long interval) { + return createApi( + local, + direct, + discoverLocal, + createDirect, + eventType, + new FeatureFlagRouteSelector(clock, interval)); + } + + private static AgentlessFeatureFlagBackendApi createApi( + BackendApi local, + BackendApi direct, + Supplier discoverLocal, + Supplier createDirect, + String eventType, + FeatureFlagRouteSelector selector) { + final AtomicReference localApi = new AtomicReference<>(local); + selector.initialize(() -> local == null ? null : "local", direct != null); + return new AgentlessFeatureFlagBackendApi( + direct, + () -> { + localApi.set(discoverLocal.get()); + return localApi.get() == null ? null : "local"; + }, + endpoint -> localApi.get(), + createDirect, + eventType, + selector); + } + + private static void postWithoutFailure( + final AgentlessFeatureFlagBackendApi api, final String body) { + try { + api.post("exposures", requestBody(body), stream -> null, null, false); + } catch (final IOException exception) { + throw new AssertionError(exception); + } + } + private static Stream featureFlagRoutes() { return Stream.of( Arguments.of("exposures", "exposure"), Arguments.of("flagevaluation", "flag evaluation")); diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java index f70a4ff0fa1..ca6d5256268 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/ExposureWriterTests.java @@ -1,16 +1,22 @@ package com.datadog.featureflag; +import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V4_EVP_PROXY_ENDPOINT; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_AGENTLESS; +import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; import static java.util.Collections.singletonMap; import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static java.util.concurrent.TimeUnit.NANOSECONDS; +import static java.util.concurrent.TimeUnit.SECONDS; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.AdditionalAnswers.delegatesTo; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; @@ -19,6 +25,8 @@ import com.squareup.moshi.JsonAdapter; import com.squareup.moshi.Moshi; +import datadog.common.queue.MessagePassingBlockingQueue; +import datadog.common.queue.Queues; import datadog.communication.BackendApi; import datadog.communication.BackendApiFactory; import datadog.communication.IntakeApi; @@ -57,6 +65,8 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; import okhttp3.HttpUrl; import okhttp3.OkHttpClient; import okhttp3.RequestBody; @@ -65,20 +75,19 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; import org.mockito.ArgumentCaptor; import org.tabletest.junit.TableTest; class ExposureWriterTests { - private static final String EXPOSURES_ENDPOINT = "/evp_proxy/api/v2/exposures"; + private static final String EXPOSURES_ENDPOINT = "/evp_proxy/v2/api/v2/exposures"; private static final String DIRECT_EXPOSURES_ENDPOINT = "/api/v2/exposures"; private static final String API_KEY = "test-api-key"; private static final double TIMEOUT_SECONDS = 5; private final PollingConditions poll = new PollingConditions(TIMEOUT_SECONDS); private Queue requests; + private Queue requestAttempts; private Set failed; private JavaTestHttpServer server; private SharedCommunicationObjects sharedCommunicationObjects; @@ -86,6 +95,7 @@ class ExposureWriterTests { @BeforeEach void setUp() { requests = new ConcurrentLinkedQueue<>(); + requestAttempts = new ConcurrentLinkedQueue<>(); failed = Collections.newSetFromMap(new ConcurrentHashMap()); JsonAdapter adapter = new Moshi.Builder().build().adapter(ExposuresRequest.class); @@ -114,6 +124,7 @@ private void handleExposureRequest(HandlerApi api, JsonAdapter adapter.fromJson( Okio.buffer(Okio.source(new ByteArrayInputStream(api.getRequest().getBody())))); String serviceName = exposuresRequest.context.get("service"); + requestAttempts.add(serviceName); boolean failForever = "fail-forever".equals(serviceName); boolean fail = serviceName.startsWith("fail") && (failed.add(serviceName) || failForever); if (fail) { @@ -234,7 +245,9 @@ void testHighLoadScenario() throws Exception { List exposures = buildExposures(threads * exposuresPerThread); CountDownLatch latch = new CountDownLatch(1); - try (ExposureWriterImpl writer = new ExposureWriterImpl(sharedCommunicationObjects, config)) { + try (ExposureWriterImpl writer = + new ExposureWriterImpl( + sharedCommunicationObjects, config, new FeatureFlagRouteSelector())) { writer.init(); List> futures = new ArrayList<>(); for (int index = 0; index < exposures.size(); index += exposuresPerThread) { @@ -262,10 +275,257 @@ void testHighLoadScenario() throws Exception { } } - @ParameterizedTest - @ValueSource(booleans = {false, true}) - void testFailuresAreRetried(boolean finallyFail) throws Exception { - String serviceName = finallyFail ? "fail-forever" : "fail-once"; + @Test + void testQueueThresholdFlushesWithoutWaitingForTheInterval() throws Exception { + Config config = mockConfig("threshold-service"); + List exposures = buildExposures(101); + + try (ExposureWriterImpl writer = + new ExposureWriterImpl( + 1 << 8, Long.MAX_VALUE, NANOSECONDS, sharedCommunicationObjects, config)) { + for (ExposureEvent exposure : exposures) { + writer.accept(exposure); + } + writer.init(); + + poll.eventually(() -> assertExposures(allExposures(), exposures)); + } + } + + @Test + void testCloseDrainsAndFinalFlushesExactlyOnceWithoutWaitingForTheInterval() throws Exception { + Config config = mockConfig("shutdown-service"); + List exposures = buildExposures(5); + ExposureWriterImpl writer = + new ExposureWriterImpl( + 1 << 4, Long.MAX_VALUE, NANOSECONDS, sharedCommunicationObjects, config); + + writer.init(); + for (ExposureEvent exposure : exposures) { + writer.accept(exposure); + } + + writer.close(); + + assertFalse(writer.isSerializerThreadAlive()); + assertEquals(1, requests.size()); + assertExposures(allExposures(), exposures); + + // A repeated close and a stale listener invocation after close must neither replay the batch + // nor leave an event stranded in the queue. + writer.close(); + writer.accept(buildExposure()); + MILLISECONDS.sleep(200); + assertEquals(1, requests.size()); + assertEquals(0, writer.queueSize()); + } + + @Test + void testFinalFlushRunsWithoutTheInterruptFlagSet() throws Exception { + BackendApi backendApi = mock(BackendApi.class); + AtomicBoolean interruptedDuringPost = new AtomicBoolean(true); + when(backendApi.post(eq("exposures"), any(RequestBody.class), any(), any(), eq(false))) + .thenAnswer( + invocation -> { + interruptedDuringPost.set(Thread.currentThread().isInterrupted()); + return null; + }); + ExposureWriterImpl writer = + new ExposureWriterImpl( + 1 << 4, + Long.MAX_VALUE, + NANOSECONDS, + () -> backendApi, + mockConfig("shutdown-service"), + ExposureWriterImpl.SHUTDOWN_TIMEOUT_MILLIS); + + writer.init(); + writer.accept(buildExposure()); + writer.close(); + + verify(backendApi, times(1)) + .post(eq("exposures"), any(RequestBody.class), any(), any(), eq(false)); + assertFalse( + interruptedDuringPost.get(), + "The final HTTP request must not run on an interrupted thread"); + assertFalse(writer.isSerializerThreadAlive()); + } + + @Test + void testCloseWaitIsBoundedWhenFinalPostDoesNotReturn() throws Exception { + BackendApi backendApi = mock(BackendApi.class); + CountDownLatch postStarted = new CountDownLatch(1); + CountDownLatch releasePost = new CountDownLatch(1); + when(backendApi.post(eq("exposures"), any(RequestBody.class), any(), any(), eq(false))) + .thenAnswer( + invocation -> { + postStarted.countDown(); + releasePost.await(); + return null; + }); + ExposureWriterImpl writer = + new ExposureWriterImpl( + 1 << 4, + Long.MAX_VALUE, + NANOSECONDS, + () -> backendApi, + mockConfig("blocked-shutdown-service"), + 100); + + try { + writer.init(); + writer.accept(buildExposure()); + long start = System.nanoTime(); + + writer.close(); + + long elapsedMillis = NANOSECONDS.toMillis(System.nanoTime() - start); + assertTrue(postStarted.await(1, java.util.concurrent.TimeUnit.SECONDS)); + assertTrue(elapsedMillis < 2000, "close exceeded its configured bounded wait"); + assertTrue(writer.isSerializerThreadAlive()); + } finally { + releasePost.countDown(); + } + poll.eventually(() -> assertFalse(writer.isSerializerThreadAlive())); + } + + @Test + @SuppressWarnings("unchecked") + void testCloseDoesNotWaitForProducerAlreadyInsideAccept() throws Exception { + CountDownLatch offerStarted = new CountDownLatch(1); + CountDownLatch releaseOffer = new CountDownLatch(1); + MessagePassingBlockingQueue delegate = Queues.mpscBlockingConsumerArrayQueue(16); + MessagePassingBlockingQueue queue = + mock(MessagePassingBlockingQueue.class, delegatesTo(delegate)); + doAnswer( + invocation -> { + offerStarted.countDown(); + releaseOffer.await(); + return delegate.offer(invocation.getArgument(0)); + }) + .when(queue) + .offer(any(ExposureEvent.class)); + ExposureWriterImpl writer = + new ExposureWriterImpl( + queue, + Long.MAX_VALUE, + NANOSECONDS, + () -> mock(BackendApi.class), + mockConfig("paused-producer-service"), + ExposureWriterImpl.SHUTDOWN_TIMEOUT_MILLIS); + ExecutorService executor = Executors.newFixedThreadPool(2); + try { + writer.init(); + Future producer = executor.submit(() -> writer.accept(buildExposure())); + assertTrue(offerStarted.await(5, SECONDS)); + + // The producer has passed the closed check but cannot finish its offer. Shutdown must not + // wait for that producer, even though its event can miss the final best-effort drain. + Future closing = executor.submit(writer::close); + closing.get(2, SECONDS); + assertFalse(producer.isDone()); + assertFalse(writer.isSerializerThreadAlive()); + + releaseOffer.countDown(); + producer.get(5, SECONDS); + } finally { + releaseOffer.countDown(); + executor.shutdownNow(); + assertTrue(executor.awaitTermination(5, SECONDS)); + writer.close(); + } + } + + @Test + void testStaleGatewayDispatchCannotEnqueueAfterClose() throws Exception { + CountDownLatch dispatchSnapshotTaken = new CountDownLatch(1); + CountDownLatch releaseDispatch = new CountDownLatch(1); + FeatureFlaggingGateway.ExposureListener blocker = + ignored -> { + dispatchSnapshotTaken.countDown(); + try { + releaseDispatch.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }; + FeatureFlaggingGateway.addExposureListener(blocker); + ExposureWriterImpl writer = + new ExposureWriterImpl( + 1 << 4, + Long.MAX_VALUE, + NANOSECONDS, + sharedCommunicationObjects, + mockConfig("stale-dispatch-service")); + Thread dispatcher = + new Thread(() -> FeatureFlaggingGateway.dispatch(buildExposure()), "exposure-dispatcher"); + + try { + writer.init(); + dispatcher.start(); + assertTrue(dispatchSnapshotTaken.await(5, java.util.concurrent.TimeUnit.SECONDS)); + + writer.close(); + releaseDispatch.countDown(); + dispatcher.join(5000); + + assertFalse(dispatcher.isAlive()); + assertFalse(writer.isSerializerThreadAlive()); + assertEquals(0, writer.queueSize()); + assertTrue(requests.isEmpty()); + } finally { + releaseDispatch.countDown(); + dispatcher.join(5000); + FeatureFlaggingGateway.removeExposureListener(blocker); + writer.close(); + } + } + + @Test + void testCloseBeforeInitPreventsLaterStartAndAccept() { + ExposureWriterImpl writer = + new ExposureWriterImpl( + sharedCommunicationObjects, + mockConfig("never-started-service"), + new FeatureFlagRouteSelector()); + + writer.close(); + writer.init(); + writer.accept(buildExposure()); + writer.close(); + + assertFalse(writer.isSerializerThreadAlive()); + assertEquals(0, writer.queueSize()); + assertTrue(requests.isEmpty()); + } + + @Test + void testMissingBackendClosesWriterFromSerializerThread() throws Exception { + CountDownLatch backendRequested = new CountDownLatch(1); + ExposureWriterImpl writer = + new ExposureWriterImpl( + 1 << 4, + Long.MAX_VALUE, + NANOSECONDS, + () -> { + backendRequested.countDown(); + return null; + }, + mockConfig("missing-backend-service"), + 100); + + writer.init(); + + assertTrue(backendRequested.await(5, java.util.concurrent.TimeUnit.SECONDS)); + poll.eventually(() -> assertFalse(writer.isSerializerThreadAlive())); + writer.accept(buildExposure()); + assertEquals(0, writer.queueSize()); + writer.close(); + } + + @Test + void testHttpFailureIsNotRetriedAtTransportLayer() throws Exception { + String serviceName = "fail-once"; Config config = mockConfig(serviceName); try (ExposureWriterImpl writer = @@ -273,13 +533,11 @@ void testFailuresAreRetried(boolean finallyFail) throws Exception { writer.init(); writer.accept(buildExposure()); - MILLISECONDS.sleep(500); // wait for a flush to happen - ExposuresRequest found = findRequest(serviceName); - if (finallyFail) { - assertNull(found, requests.toString()); - } else { - poll.eventually(() -> assertNotNull(findRequest(serviceName), requests.toString())); - } + poll.eventually(() -> assertEquals(1, Collections.frequency(requestAttempts, serviceName))); + MILLISECONDS.sleep(500); + + assertEquals(1, Collections.frequency(requestAttempts, serviceName)); + assertNull(findRequest(serviceName), requests.toString()); } } @@ -308,8 +566,13 @@ void testAmbiguousExposureBatchIsNotRetriedOrReplayedDirectly() throws Exception final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); final BackendApi proxyApi = mock(BackendApi.class); final BackendApi directApi = mock(BackendApi.class); - when(backendApiFactory.createEvpProxyApi( - Intake.EVENT_PLATFORM, true, HttpRetryPolicy.Factory.NEVER_RETRY)) + when(backendApiFactory.discoverEvpProxyEndpoint(false, emptyList())) + .thenReturn(V4_EVP_PROXY_ENDPOINT); + when(backendApiFactory.createEvpProxyApiForEndpoint( + Intake.EVENT_PLATFORM, + true, + HttpRetryPolicy.Factory.NEVER_RETRY, + V4_EVP_PROXY_ENDPOINT)) .thenReturn(proxyApi); when(backendApiFactory.createDirectIntakeApi(eq(Intake.EVENT_PLATFORM), eq(true), eq(false))) .thenReturn(directApi); @@ -353,18 +616,55 @@ void testAmbiguousExposureBatchIsNotRetriedOrReplayedDirectly() throws Exception } @Test - void testWriterStopsReceivingExposuresIfEvpProxyIsNotAvailable() throws Exception { + void testAgentlessWriterWaitsForUnavailableProxyRecovery() throws Exception { SharedCommunicationObjects sharedCommunicationObjects = sharedCommunicationObjects(false); + Config config = mockConfig("unavailable-service"); + when(config.getFeatureFlaggingConfigurationSource()).thenReturn(CONFIGURATION_SOURCE_AGENTLESS); try (ExposureWriterImpl writer = - new ExposureWriterImpl(sharedCommunicationObjects, Config.get())) { + new ExposureWriterImpl( + sharedCommunicationObjects, config, new FeatureFlagRouteSelector())) { writer.init(); - poll.eventually(() -> assertFalse(writer.isSerializerThreadAlive())); - - FeatureFlaggingGateway.dispatch(buildExposure()); + poll.eventually(() -> assertTrue(writer.isSerializerThreadAlive())); + } + } - assertEquals(0, writer.queueSize()); + @Test + void idleUnavailableWriterOnlyProbesForANewBatchAndStillCloses() throws Exception { + final Config config = mockConfig("idle-recovery"); + when(config.getFeatureFlaggingConfigurationSource()).thenReturn(CONFIGURATION_SOURCE_AGENTLESS); + final AtomicLong clock = new AtomicLong(); + final BackendApiFactory backend = mock(BackendApiFactory.class); + final BackendApi proxy = mock(BackendApi.class); + when(backend.discoverEvpProxyEndpoint(true, emptyList())).thenReturn(V4_EVP_PROXY_ENDPOINT); + when(backend.createEvpProxyApiForEndpoint( + Intake.EVENT_PLATFORM, + true, + HttpRetryPolicy.Factory.NEVER_RETRY, + V4_EVP_PROXY_ENDPOINT)) + .thenReturn(proxy); + final FeatureFlagBackendApiFactory factory = + new FeatureFlagBackendApiFactory( + config, + backend, + FeatureFlagEventType.EXPOSURE, + new FeatureFlagRouteSelector(clock::get, 10)); + final ExposureWriterImpl writer = + new ExposureWriterImpl(16, 100, MILLISECONDS, factory, config); + try { + writer.init(); + poll.eventually(() -> verify(backend).discoverEvpProxyEndpoint(false, emptyList())); + clock.set(10); + MILLISECONDS.sleep(300); + verify(backend, never()).discoverEvpProxyEndpoint(true, emptyList()); + verify(proxy, never()).post(any(), any(), any(), any(), eq(false)); + writer.accept(buildExposure()); + poll.eventually(() -> verify(proxy).post(eq("exposures"), any(), any(), any(), eq(false))); + verify(backend).discoverEvpProxyEndpoint(true, emptyList()); + } finally { + writer.close(); } + assertFalse(writer.isSerializerThreadAlive()); } private static Config mockConfig(String serviceName) { diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java index b89cf3ce2b1..fd020562d2d 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagBackendApiFactoryTest.java @@ -2,130 +2,118 @@ import static com.datadog.featureflag.FeatureFlagEventType.EXPOSURE; import static com.datadog.featureflag.FeatureFlagEventType.FLAG_EVALUATION; +import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V2_EVP_PROXY_ENDPOINT; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_AGENTLESS; import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_REMOTE_CONFIG; +import static java.util.Collections.emptyList; import static org.junit.jupiter.api.Assertions.assertInstanceOf; -import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertSame; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; import datadog.communication.BackendApi; import datadog.communication.BackendApiFactory; -import datadog.communication.http.HttpRetryPolicy; +import datadog.communication.ddagent.SharedCommunicationObjects; import datadog.trace.api.Config; import datadog.trace.api.intake.Intake; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.NullAndEmptySource; class FeatureFlagBackendApiFactoryTest { @Test - void remoteConfigUsesOnlyLocalEvpProxy() { + void remoteConfigDoesNotEagerlyInitializeSharedTransport() { final Config config = config(CONFIGURATION_SOURCE_REMOTE_CONFIG, "api-key"); - final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); - final BackendApi proxyApi = mock(BackendApi.class); - when(backendApiFactory.createEvpProxyApi(Intake.EVENT_PLATFORM, false)).thenReturn(proxyApi); + final SharedCommunicationObjects sco = mock(SharedCommunicationObjects.class); - final BackendApi selected = - new FeatureFlagBackendApiFactory(config, backendApiFactory, FLAG_EVALUATION).create(); + new FeatureFlagBackendApiFactory(config, sco, EXPOSURE); - assertSame(proxyApi, selected); - verify(backendApiFactory, never()).createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false); + verifyNoInteractions(sco); } - @Test - void remoteConfigDisablesDeliveryWhenLocalEvpProxyIsUnavailable() { - final Config config = config(CONFIGURATION_SOURCE_REMOTE_CONFIG, "api-key"); - final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); + @ParameterizedTest + @NullAndEmptySource + void agentlessWithoutCredentialsDoesNotEagerlyInitializeSharedTransport(String apiKey) { + final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, apiKey); + final SharedCommunicationObjects sco = mock(SharedCommunicationObjects.class); - final BackendApi selected = - new FeatureFlagBackendApiFactory(config, backendApiFactory, EXPOSURE).create(); + new FeatureFlagBackendApiFactory(config, sco, EXPOSURE); - assertNull(selected); - verify(backendApiFactory).createEvpProxyApi(Intake.EVENT_PLATFORM, true); - verify(backendApiFactory, never()).createDirectIntakeApi(Intake.EVENT_PLATFORM, true, false); + verifyNoInteractions(sco); } @Test - void agentlessPrefersLocalEvpProxyWithDirectFallback() { - final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, "api-key"); + void remoteConfigUsesFixedV2WithoutDiscoveryOrDirectIntake() { + final Config config = config(CONFIGURATION_SOURCE_REMOTE_CONFIG, "api-key"); final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); - when(backendApiFactory.createEvpProxyApi( - Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY)) - .thenReturn(mock(BackendApi.class)); - when(backendApiFactory.createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false)) - .thenReturn(mock(BackendApi.class)); + final BackendApi proxyApi = mock(BackendApi.class); + when(backendApiFactory.createEvpProxyApiForEndpoint( + Intake.EVENT_PLATFORM, + false, + FeatureFlagBackendApiFactory.PRE_SEND_RETRY, + V2_EVP_PROXY_ENDPOINT)) + .thenReturn(proxyApi); final BackendApi selected = new FeatureFlagBackendApiFactory(config, backendApiFactory, FLAG_EVALUATION).create(); - assertInstanceOf(AgentlessFeatureFlagBackendApi.class, selected); - verify(backendApiFactory) - .createEvpProxyApi(Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY); + assertSame(proxyApi, selected); + verify(backendApiFactory, never()).discoverEvpProxyEndpoint(false, emptyList()); verify(backendApiFactory, never()).createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false); } @Test - void agentlessUsesDirectIntakeWhenLocalEvpProxyIsUnavailable() { + void agentlessPrefersLocalRouteWithDirectFallbackReady() { final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, "api-key"); final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); - final BackendApi directApi = mock(BackendApi.class); + when(backendApiFactory.discoverEvpProxyEndpoint(false, emptyList())) + .thenReturn(V2_EVP_PROXY_ENDPOINT); when(backendApiFactory.createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false)) - .thenReturn(directApi); + .thenReturn(mock(BackendApi.class)); final BackendApi selected = new FeatureFlagBackendApiFactory(config, backendApiFactory, FLAG_EVALUATION).create(); - assertSame(directApi, selected); + assertInstanceOf(AgentlessFeatureFlagBackendApi.class, selected); + verify(backendApiFactory).discoverEvpProxyEndpoint(false, emptyList()); + verify(backendApiFactory).createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false); } @Test - void agentlessUsesLocalEvpProxyWhenApiKeyIsUnavailable() { - final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, null); + void agentlessUsesDirectIntakeWhenLocalRouteIsUnavailable() { + final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, "api-key"); final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); - final BackendApi proxyApi = mock(BackendApi.class); - when(backendApiFactory.createEvpProxyApi(Intake.EVENT_PLATFORM, false)).thenReturn(proxyApi); + when(backendApiFactory.createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false)) + .thenReturn(mock(BackendApi.class)); final BackendApi selected = new FeatureFlagBackendApiFactory(config, backendApiFactory, FLAG_EVALUATION).create(); - assertSame(proxyApi, selected); - verify(backendApiFactory, never()).createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false); + assertInstanceOf(AgentlessFeatureFlagBackendApi.class, selected); } @Test - void agentlessDisablesDeliveryWhenNoRouteIsAvailable() { + void agentlessKeepsWriterAliveWhileEveryRouteIsUnavailable() { final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, null); final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); - final BackendApi selected = - new FeatureFlagBackendApiFactory(config, backendApiFactory, FLAG_EVALUATION).create(); - - assertNull(selected); - } - - @Test - void agentlessDisablesDeliveryWhenApiKeyIsEmpty() { - final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, ""); - final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); - final BackendApi selected = new FeatureFlagBackendApiFactory(config, backendApiFactory, EXPOSURE).create(); - assertNull(selected); + assertInstanceOf(AgentlessFeatureFlagBackendApi.class, selected); verify(backendApiFactory, never()).createDirectIntakeApi(Intake.EVENT_PLATFORM, true, false); } @Test - void agentlessDoesNotValidateDirectUrlWhileLocalRouteIsAvailable() { + void agentlessKeepsCompatibleLocalRouteWhenDirectUrlIsInvalid() { final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, "api-key"); final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); - final BackendApi proxyApi = mock(BackendApi.class); - when(backendApiFactory.createEvpProxyApi( - Intake.EVENT_PLATFORM, false, HttpRetryPolicy.Factory.NEVER_RETRY)) - .thenReturn(proxyApi); + when(backendApiFactory.discoverEvpProxyEndpoint(false, emptyList())) + .thenReturn(V2_EVP_PROXY_ENDPOINT); when(backendApiFactory.createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false)) .thenThrow(new IllegalArgumentException("invalid URL")); @@ -133,20 +121,6 @@ void agentlessDoesNotValidateDirectUrlWhileLocalRouteIsAvailable() { new FeatureFlagBackendApiFactory(config, backendApiFactory, FLAG_EVALUATION).create(); assertInstanceOf(AgentlessFeatureFlagBackendApi.class, selected); - verify(backendApiFactory, never()).createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false); - } - - @Test - void agentlessDisablesDeliveryWhenDirectUrlIsInvalidAndLocalRouteIsUnavailable() { - final Config config = config(CONFIGURATION_SOURCE_AGENTLESS, "api-key"); - final BackendApiFactory backendApiFactory = mock(BackendApiFactory.class); - when(backendApiFactory.createDirectIntakeApi(Intake.EVENT_PLATFORM, false, false)) - .thenThrow(new IllegalArgumentException("invalid URL")); - - final BackendApi selected = - new FeatureFlagBackendApiFactory(config, backendApiFactory, FLAG_EVALUATION).create(); - - assertNull(selected); } private static Config config(final String source, final String apiKey) { diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagRouteSelectorTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagRouteSelectorTest.java new file mode 100644 index 00000000000..7fd88d12776 --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagRouteSelectorTest.java @@ -0,0 +1,104 @@ +package com.datadog.featureflag; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; +import java.util.concurrent.atomic.AtomicLong; +import org.junit.jupiter.api.Test; +import org.slf4j.LoggerFactory; + +class FeatureFlagRouteSelectorTest { + + @Test + void unavailableLoggingIsSharedAndOnlyRepeatsAfterRecovery() { + final Logger logger = (Logger) LoggerFactory.getLogger(FeatureFlagRouteSelector.class); + final ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + try { + final AtomicLong clock = new AtomicLong(); + final FeatureFlagRouteSelector selector = new FeatureFlagRouteSelector(clock::get, 10); + selector.initialize(() -> null, false); + selector.initialize(() -> null, false); + for (int i = 1; i <= 3; i++) { + clock.set(i * 10); + assertTrue(selector.tryBeginLocalRecovery()); + selector.localRecoveryFinished(null); + } + assertEquals(1, appender.list.size()); + assertEquals(Level.WARN, appender.list.get(0).getLevel()); + assertTrue(appender.list.get(0).getFormattedMessage().contains("DD_API_KEY")); + clock.set(40); + assertTrue(selector.tryBeginLocalRecovery()); + selector.localRecoveryFinished("v4"); + assertEquals(2, appender.list.size()); + assertEquals(Level.INFO, appender.list.get(1).getLevel()); + selector.localFailure(selector.localRoute(), false); + assertEquals(3, appender.list.size()); + assertEquals(Level.WARN, appender.list.get(2).getLevel()); + } finally { + logger.detachAppender(appender); + appender.stop(); + } + } + + @Test + void directRouteIsTerminalAcrossInitializationAndRecoverySignals() { + final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(); + + assertEquals(FeatureFlagRouteSelector.Route.DIRECT, routeSelector.initialize(() -> null, true)); + assertEquals( + FeatureFlagRouteSelector.Route.DIRECT, routeSelector.initialize(() -> "v4", false)); + assertFalse(routeSelector.tryBeginLocalRecovery()); + + routeSelector.localRecoveryFinished("v4"); + + assertEquals(FeatureFlagRouteSelector.Route.DIRECT, routeSelector.current()); + } + + @Test + void anotherWriterCannotBypassTheRecoveryCooldown() { + final AtomicLong clock = new AtomicLong(); + final FeatureFlagRouteSelector routeSelector = new FeatureFlagRouteSelector(clock::get, 10); + + assertEquals( + FeatureFlagRouteSelector.Route.UNAVAILABLE, routeSelector.initialize(() -> null, false)); + assertEquals( + FeatureFlagRouteSelector.Route.UNAVAILABLE, + routeSelector.initialize( + () -> { + throw new AssertionError("duplicate discovery"); + }, + false)); + assertFalse(routeSelector.tryBeginLocalRecovery()); + clock.set(10); + assertTrue(routeSelector.tryBeginLocalRecovery()); + clock.set(100); + assertFalse(routeSelector.tryBeginLocalRecovery()); + routeSelector.localRecoveryFinished(null); + assertFalse(routeSelector.tryBeginLocalRecovery()); + clock.set(110); + assertTrue(routeSelector.tryBeginLocalRecovery()); + } + + @Test + void staleFailureCannotInvalidateANewGenerationOfTheSameEndpoint() { + final AtomicLong clock = new AtomicLong(); + final FeatureFlagRouteSelector selector = new FeatureFlagRouteSelector(clock::get, 10); + selector.initialize(() -> "v4", false); + final FeatureFlagRouteSelector.LocalRoute oldRoute = selector.localRoute(); + selector.localFailure(oldRoute, false); + clock.set(10); + assertTrue(selector.tryBeginLocalRecovery()); + selector.localRecoveryFinished("v4"); + assertNotSame(oldRoute, selector.localRoute()); + selector.localFailure(oldRoute, true); + assertEquals(FeatureFlagRouteSelector.Route.LOCAL, selector.current()); + } +} diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagSendOnceTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagSendOnceTest.java new file mode 100644 index 00000000000..27348f35421 --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagSendOnceTest.java @@ -0,0 +1,160 @@ +package com.datadog.featureflag; + +import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V4_EVP_PROXY_ENDPOINT; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import datadog.communication.BackendApi; +import datadog.communication.ddagent.DDAgentFeaturesDiscovery; +import datadog.communication.ddagent.SharedCommunicationObjects; +import datadog.trace.api.Config; +import java.io.IOException; +import java.util.concurrent.TimeUnit; +import okhttp3.HttpUrl; +import okhttp3.MediaType; +import okhttp3.OkHttpClient; +import okhttp3.Request; +import okhttp3.RequestBody; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import okhttp3.mockwebserver.RecordedRequest; +import okhttp3.mockwebserver.SocketPolicy; +import org.junit.jupiter.api.Timeout; +import org.tabletest.junit.TableTest; + +@Timeout(15) +class FeatureFlagSendOnceTest { + @TableTest({ + "Source | Route | Event ", + "remote_config | proxy | EXPOSURE ", + "remote_config | proxy | FLAG_EVALUATION", + "agentless | proxy | EXPOSURE ", + "agentless | proxy | FLAG_EVALUATION", + "agentless | direct | EXPOSURE ", + "agentless | direct | FLAG_EVALUATION" + }) + void productionFactoryDoesNotReplayAnAcceptedPostOnConnectionLoss( + String source, String route, FeatureFlagEventType event) throws Exception { + try (MockWebServer server = new MockWebServer()) { + server.start(); + server.enqueue(new MockResponse().setBody("{}")); + server.enqueue(new MockResponse().setSocketPolicy(SocketPolicy.DISCONNECT_AFTER_REQUEST)); + server.enqueue(new MockResponse().setBody("{}")); + final OkHttpClient client = localClient(server); + try { + final BackendApi api = api(server, client, source, route, event); + post(api); + assertThrows(IOException.class, () -> post(api)); + assertEquals(2, server.getRequestCount()); + for (int i = 0; i < 2; i++) { + final RecordedRequest request = server.takeRequest(1, TimeUnit.SECONDS); + assertEquals("POST", request.getMethod()); + if (route.equals("proxy")) { + assertNull(request.getHeader("DD-API-KEY")); + } else { + assertEquals("test-api-key", request.getHeader("DD-API-KEY")); + assertNull(request.getHeader("X-Datadog-EVP-Subdomain")); + } + } + } finally { + client.connectionPool().evictAll(); + client.dispatcher().executorService().shutdownNow(); + } + } + } + + @TableTest({ + "Source | Route ", + "remote_config | proxy ", + "agentless | proxy ", + "agentless | direct" + }) + void redirectDoesNotForwardTheBatchOrCredentials(String source, String route) throws Exception { + try (MockWebServer server = new MockWebServer(); + MockWebServer destination = new MockWebServer()) { + server.start(); + destination.start(); + server.enqueue( + new MockResponse() + .setResponseCode(307) + .addHeader("Location", destination.url("/redirected"))); + destination.enqueue(new MockResponse().setBody("{}")); + final OkHttpClient client = localClient(server); + try { + final BackendApi api = api(server, client, source, route, FeatureFlagEventType.EXPOSURE); + assertThrows(IOException.class, () -> post(api)); + assertEquals(1, server.getRequestCount()); + assertNull(destination.takeRequest(200, TimeUnit.MILLISECONDS)); + } finally { + client.connectionPool().evictAll(); + client.dispatcher().executorService().shutdownNow(); + } + } + } + + private static OkHttpClient localClient(MockWebServer server) { + return new OkHttpClient.Builder() + .addInterceptor( + chain -> { + final Request original = chain.request(); + final HttpUrl local = + original + .url() + .newBuilder() + .scheme("http") + .host(server.getHostName()) + .port(server.getPort()) + .build(); + return chain.proceed(original.newBuilder().url(local).build()); + }) + .build(); + } + + private static BackendApi api( + MockWebServer server, + OkHttpClient client, + String source, + String route, + FeatureFlagEventType event) { + final Config config = mock(Config.class); + when(config.getIdGenerationStrategy()).thenReturn(Config.get().getIdGenerationStrategy()); + when(config.getFeatureFlaggingConfigurationSource()).thenReturn(source); + when(config.getSite()).thenReturn("datadoghq.com"); + if (route.equals("direct")) { + when(config.getApiKey()).thenReturn("test-api-key"); + } + final DDAgentFeaturesDiscovery discovery = mock(DDAgentFeaturesDiscovery.class); + when(discovery.supportsEvpProxyHeaders(any())).thenReturn(true); + when(discovery.supportsEvpProxy()).thenReturn(route.equals("proxy")); + when(discovery.getEvpProxyEndpoint()) + .thenReturn(route.equals("proxy") ? V4_EVP_PROXY_ENDPOINT : null); + final SharedCommunicationObjects sco = + new SharedCommunicationObjects() { + @Override + public OkHttpClient getIntakeHttpClient() { + return client; + } + + @Override + public DDAgentFeaturesDiscovery featuresDiscovery(Config ignored) { + return discovery; + } + }; + sco.agentUrl = server.url("/"); + sco.agentHttpClient = client; + return new FeatureFlagBackendApiFactory(config, sco, event).create(); + } + + private static void post(BackendApi api) throws IOException { + api.post( + "events", + RequestBody.create(MediaType.parse("application/json"), "{}"), + stream -> null, + null, + false); + } +} diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagTransportRegressionTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagTransportRegressionTest.java new file mode 100644 index 00000000000..1b19375ea44 --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagTransportRegressionTest.java @@ -0,0 +1,197 @@ +package com.datadog.featureflag; + +import static com.datadog.featureflag.FeatureFlagEventType.EXPOSURE; +import static org.junit.jupiter.api.Assertions.assertAll; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import datadog.communication.BackendApi; +import datadog.communication.ddagent.SharedCommunicationObjects; +import datadog.trace.api.Config; +import datadog.trace.api.ProtocolVersion; +import datadog.trace.test.junit.utils.config.WithConfig; +import datadog.trace.test.util.DDJavaSpecification; +import java.io.IOException; +import java.net.ConnectException; +import java.net.SocketException; +import java.net.SocketTimeoutException; +import java.util.ArrayList; +import java.util.List; +import okhttp3.HttpUrl; +import okhttp3.MediaType; +import okhttp3.OkHttpClient; +import okhttp3.Protocol; +import okhttp3.Request; +import okhttp3.RequestBody; +import okhttp3.Response; +import okhttp3.ResponseBody; +import okhttp3.mockwebserver.Dispatcher; +import okhttp3.mockwebserver.MockResponse; +import okhttp3.mockwebserver.MockWebServer; +import okhttp3.mockwebserver.RecordedRequest; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.EnumSource; +import org.tabletest.junit.TableTest; + +class FeatureFlagTransportRegressionTest extends DDJavaSpecification { + @ParameterizedTest + @EnumSource(FeatureFlagEventType.class) + @WithConfig(key = "trace.enabled", value = "false") + @WithConfig(key = "remote_config.enabled", value = "false") + @WithConfig(key = "telemetry.enabled", value = "false") + @WithConfig(key = "appsec.enabled", value = "false") + @WithConfig(key = "feature.flags.configuration.source", value = "agentless") + @WithConfig(key = "trace.agent.timeout", value = "2") + @WithConfig(key = "API_KEY", value = "test-api-key", env = true) + void agentlessInitializesDirectTimeoutsBeforeAgentDiscovery(FeatureFlagEventType eventType) + throws Exception { + try (MockWebServer agent = new MockWebServer()) { + agent.setDispatcher( + new Dispatcher() { + @Override + public MockResponse dispatch(RecordedRequest request) { + return new MockResponse().setBody("{\"endpoints\":[\"v0.5/traces\"]}"); + } + }); + final SharedCommunicationObjects sco = new SharedCommunicationObjects(); + sco.agentUrl = agent.url("/"); + try { + assertNotNull(new FeatureFlagBackendApiFactory(Config.get(), sco, eventType).create()); + + final OkHttpClient directClient = sco.getIntakeHttpClient(); + assertAll( + () -> assertEquals(2000, sco.httpClientTimeout), + () -> assertEquals(2000, directClient.connectTimeoutMillis()), + () -> assertEquals(2000, directClient.readTimeoutMillis()), + () -> assertEquals(2000, directClient.writeTimeoutMillis())); + } finally { + if (sco.agentHttpClient != null) { + sco.agentHttpClient.connectionPool().evictAll(); + } + sco.getIntakeHttpClient().connectionPool().evictAll(); + } + } + } + + @TableTest({ + "Scenario | First Attempt | Attempts | Delivered", + "refused then accepted | refused | 2 | true ", + "refused throughout | refused-all | 4 | false ", + "forbidden | 403 | 1 | false ", + "throttled | 429 | 1 | false ", + "internal server error | 500 | 1 | false ", + "unavailable | 503 | 1 | false ", + "read timeout | timeout | 1 | false ", + "reset | reset | 1 | false ", + "broken pipe | broken-pipe | 1 | false " + }) + void remoteConfigRetriesOnlyProvenPreSendFailures( + String firstAttempt, int attempts, boolean delivered) throws Exception { + final List requests = new ArrayList<>(); + final SharedCommunicationObjects sco = new SharedCommunicationObjects(); + sco.agentUrl = HttpUrl.get("http://localhost:8126/"); + sco.agentHttpClient = + new OkHttpClient.Builder() + .addInterceptor( + chain -> { + final Request request = chain.request(); + requests.add(request); + if (firstAttempt.equals("refused-all")) { + throw new ConnectException("Connection refused"); + } + if (requests.size() == 1) { + switch (firstAttempt) { + case "refused": + throw new ConnectException("Connection refused"); + case "timeout": + throw new SocketTimeoutException("read timeout"); + case "reset": + throw new SocketException("Connection reset"); + case "broken-pipe": + throw new SocketException("Broken pipe"); + default: + return response(request, Integer.parseInt(firstAttempt), "{}"); + } + } + return response(request, 202, "{}"); + }) + .build(); + final BackendApi api = + new FeatureFlagBackendApiFactory(config("remote_config"), sco, EXPOSURE).create(); + + if (delivered) { + post(api); + } else { + assertThrows(IOException.class, () -> post(api)); + } + + assertEquals(attempts, requests.size()); + for (Request request : requests) { + assertEquals("/evp_proxy/v2/api/v2/exposures", request.url().encodedPath()); + assertNull(request.header("DD-API-KEY")); + } + } + + @Test + void agentlessWithoutIdentityKeepsAnOlderAgentUsableWithoutDirectCredentials() throws Exception { + final List requests = new ArrayList<>(); + final SharedCommunicationObjects sco = new SharedCommunicationObjects(); + sco.agentUrl = HttpUrl.get("http://localhost:8126/"); + sco.agentHttpClient = + new OkHttpClient.Builder() + .addInterceptor( + chain -> { + final Request request = chain.request(); + requests.add(request); + return response( + request, + 200, + request.url().encodedPath().equals("/info") + ? "{\"endpoints\":[\"v0.5/traces\",\"evp_proxy/v4/\"],\"evp_proxy_allowed_headers\":[]}" + : "{}"); + }) + .build(); + + final BackendApi api = + new FeatureFlagBackendApiFactory(config("agentless"), sco, EXPOSURE).create(); + post(api); + + assertEquals(2, requests.size()); + assertEquals("/info", requests.get(0).url().encodedPath()); + assertEquals("/evp_proxy/v4/api/v2/exposures", requests.get(1).url().encodedPath()); + assertNull(requests.get(1).header("DD-API-KEY")); + assertNull(requests.get(1).header("DD-EVP-ORIGIN")); + } + + private static Config config(String source) { + final Config config = mock(Config.class); + when(config.getFeatureFlaggingConfigurationSource()).thenReturn(source); + when(config.getIdGenerationStrategy()).thenReturn(Config.get().getIdGenerationStrategy()); + when(config.getProtocolVersion()).thenReturn(ProtocolVersion.V0_5); + return config; + } + + private static Response response(Request request, int code, String body) { + return new Response.Builder() + .request(request) + .protocol(Protocol.HTTP_1_1) + .code(code) + .message("test") + .body(ResponseBody.create(null, body)) + .build(); + } + + private static void post(BackendApi api) throws IOException { + api.post( + "exposures", + RequestBody.create(MediaType.parse("application/json"), "{}"), + stream -> null, + null, + false); + } +} diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagWriterShutdownTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagWriterShutdownTest.java new file mode 100644 index 00000000000..9d9b97185b2 --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FeatureFlagWriterShutdownTest.java @@ -0,0 +1,143 @@ +package com.datadog.featureflag; + +import static com.datadog.featureflag.FlagEvaluationTestSupport.cfg; +import static com.datadog.featureflag.FlagEvaluationTestSupport.simpleEvent; +import static java.util.Collections.emptyMap; +import static java.util.concurrent.TimeUnit.MILLISECONDS; +import static java.util.concurrent.TimeUnit.SECONDS; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTimeoutPreemptively; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import datadog.communication.BackendApi; +import datadog.trace.api.featureflag.FeatureFlaggingGateway; +import datadog.trace.api.featureflag.exposure.Allocation; +import datadog.trace.api.featureflag.exposure.ExposureEvent; +import datadog.trace.api.featureflag.exposure.Flag; +import datadog.trace.api.featureflag.exposure.Subject; +import datadog.trace.api.featureflag.exposure.Variant; +import java.time.Duration; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.FutureTask; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +class FeatureFlagWriterShutdownTest { + @Test + void awaitThreadDoesNotJoinItself() { + assertTimeoutPreemptively( + Duration.ofSeconds(2), + () -> + FeatureFlagEventWriter.awaitThread( + Thread.currentThread(), System.nanoTime() + SECONDS.toNanos(30))); + } + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void awaitThreadStopsWaitingWithoutInterruptingWorker(boolean interruptedCaller) + throws Exception { + final CountDownLatch releaseWorker = new CountDownLatch(1); + final FutureTask task = + new FutureTask<>( + () -> { + releaseWorker.await(); + return Thread.currentThread().isInterrupted(); + }); + final Thread worker = new Thread(task); + worker.start(); + try { + assertTimeoutPreemptively( + Duration.ofSeconds(2), + () -> { + try { + if (interruptedCaller) { + Thread.currentThread().interrupt(); + } + final long deadline = + System.nanoTime() + (interruptedCaller ? SECONDS.toNanos(30) : -1); + FeatureFlagEventWriter.awaitThread(worker, deadline); + assertEquals(interruptedCaller, Thread.currentThread().isInterrupted()); + } finally { + Thread.interrupted(); + } + }); + assertTrue(worker.isAlive(), "Stopping the wait must not terminate the worker"); + } finally { + releaseWorker.countDown(); + worker.join(5000); + } + assertFalse(worker.isAlive()); + assertFalse(task.get(5, SECONDS), "Waiting must not interrupt the worker"); + } + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + void closeDoesNotInterruptAnInFlightDelivery(boolean exposures) throws Exception { + final CountDownLatch postStarted = new CountDownLatch(1); + final CountDownLatch releasePost = new CountDownLatch(1); + final CountDownLatch postFinished = new CountDownLatch(1); + final CountDownLatch interrupted = new CountDownLatch(1); + final BackendApi api = mock(BackendApi.class); + when(api.post(any(), any(), any(), any(), anyBoolean())) + .thenAnswer( + invocation -> { + postStarted.countDown(); + try { + assertTrue(releasePost.await(5, SECONDS)); + } catch (InterruptedException failure) { + interrupted.countDown(); + } finally { + postFinished.countDown(); + } + return null; + }); + final AutoCloseable writer; + FeatureFlaggingGateway.setFlagEvaluationEnqueueEnabled(true); + if (exposures) { + final ExposureWriterImpl exposureWriter = + new ExposureWriterImpl(16, 0, MILLISECONDS, () -> api, cfg(), 800); + writer = exposureWriter; + exposureWriter.init(); + exposureWriter.accept( + new ExposureEvent( + 1, + new Allocation("alloc"), + new Flag("flag"), + new Variant("on"), + new Subject("user", emptyMap()))); + } else { + final FlagEvaluationWriterImpl evaluationWriter = + new FlagEvaluationWriterImpl(16, 0, MILLISECONDS, () -> api, cfg()); + writer = evaluationWriter; + evaluationWriter.startForTest(); + evaluationWriter.enqueue(simpleEvent("flag", "on")); + } + final FutureTask close = + new FutureTask<>( + () -> { + writer.close(); + return null; + }); + final Thread closer = new Thread(close); + try { + assertTrue(postStarted.await(5, SECONDS)); + closer.start(); + close.get(5, SECONDS); + assertEquals(1, releasePost.getCount(), "Shutdown must finish before delivery is released"); + assertEquals(1, postFinished.getCount(), "The active delivery must still be pending"); + assertEquals(1, interrupted.getCount(), "Shutdown interrupted the active POST"); + } finally { + releasePost.countDown(); + closer.join(5000); + writer.close(); + FeatureFlaggingGateway.setFlagEvaluationEnqueueEnabled(true); + } + assertFalse(closer.isAlive()); + } +} diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java index 75a767c5c3f..f0bd4476dcf 100644 --- a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/FlagEvaluationWriterImplTest.java @@ -29,6 +29,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import ch.qos.logback.classic.Logger; import datadog.common.queue.MessagePassingBlockingQueue; import datadog.common.queue.Queues; import datadog.communication.BackendApi; @@ -44,15 +45,19 @@ import datadog.trace.api.intake.Intake; import datadog.trace.api.telemetry.CoreMetricCollector; import datadog.trace.api.telemetry.MetricCollector; +import datadog.trace.test.logging.TestLogCollector; +import datadog.trace.test.logging.TestLogbackAppender; import datadog.trace.test.util.PollingConditions; import java.io.IOException; import java.lang.reflect.Field; import java.util.Collection; import java.util.HashMap; import java.util.Map; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; import okhttp3.HttpUrl; import okhttp3.OkHttpClient; import okhttp3.RequestBody; @@ -60,9 +65,47 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.slf4j.LoggerFactory; class FlagEvaluationWriterImplTest { + @Test + void unavailableRouteDoesNotLogAnErrorForEachEvaluationFlush() { + final FeatureFlagRouteSelector selector = new FeatureFlagRouteSelector(); + selector.initialize(() -> null, false); + final BackendApi api = + new AgentlessFeatureFlagBackendApi( + null, () -> null, endpoint -> null, () -> null, "flag evaluation", selector); + final FlagEvaluationTestSupport.TestWriterSetup setup = buildTestWriter(api); + final Logger logger = (Logger) LoggerFactory.getLogger(FlagEvaluationWriterImpl.class); + final TestLogbackAppender appender = new TestLogbackAppender(); + appender.setContext(logger.getLoggerContext()); + appender.start(); + logger.addAppender(appender); + TestLogCollector.enable(); + try { + logger.error("log-capture-positive-control"); + assertEquals(1, TestLogCollector.drainCapturedLogs().size()); + for (int i = 0; i < 3; i++) { + setup.handler.add(simpleEvent("flag", "on")); + setup.handler.drainAndAggregate(); + setup.handler.flush(); + } + assertEquals( + 0, + TestLogCollector.drainCapturedLogs().stream() + .filter( + log -> + log.level.equals("ERROR") + && log.message.contains("Could not submit flag evaluations")) + .count()); + } finally { + TestLogCollector.disable(); + logger.detachAppender(appender); + appender.stop(); + } + } + private static final String DIRECT_FLAG_EVALUATION_ENDPOINT = "/api/v2/flagevaluation"; private static final String API_KEY = "test-api-key"; private static final double TIMEOUT_SECONDS = 5; @@ -119,6 +162,37 @@ void startRegistersWriterAndCloseDeregistersIt() { assertNull(FeatureFlaggingGateway.getFlagEvalWriter()); } + @Test + void unavailableBackendClosesWriterFromItsOwnWorker() throws Exception { + final AtomicReference worker = new AtomicReference<>(); + final CompletableFuture failure = new CompletableFuture<>(); + final FlagEvaluationWriterImpl writer = + new FlagEvaluationWriterImpl( + 16, + Long.MAX_VALUE, + TimeUnit.NANOSECONDS, + () -> { + worker.set(Thread.currentThread()); + worker.get().setUncaughtExceptionHandler((thread, error) -> failure.complete(error)); + return null; + }, + cfg()); + try { + writer.start(); + final Throwable error = failure.get(5, TimeUnit.SECONDS); + assertEquals(IllegalArgumentException.class, error.getClass()); + assertEquals("EVP Proxy not available", error.getMessage()); + worker.get().join(5000); + assertFalse(worker.get().isAlive()); + assertNull(FeatureFlaggingGateway.getFlagEvalWriter()); + + writer.startForTest(); + assertEquals(Thread.State.TERMINATED, worker.get().getState()); + } finally { + writer.close(); + } + } + @Test void queueOverflowIncrementsObservableDropCounter() { final BackendApi mockEvp = mock(BackendApi.class); @@ -371,9 +445,9 @@ void payloadLimitDropsAreCountedOnFlush() { @Test void finalFlushRunsWithoutTheInterruptFlagSet() throws Exception { - // close() interrupts the worker to break it out of poll(). The final flush does socket I/O, - // and OkHttp fails fast on an interrupted thread, so the flag must be clear by the time the - // publisher is called. A mock publisher ignores the flag, so assert on it directly. + // close() signals shutdown without interrupting the worker. The final flush does socket I/O, + // and OkHttp fails fast on an interrupted thread. A mock publisher ignores the flag, so assert + // on it directly. final java.util.concurrent.CountDownLatch posted = new java.util.concurrent.CountDownLatch(1); final boolean[] interruptedDuringPost = {true}; final BackendApi mockEvp = mock(BackendApi.class); @@ -666,7 +740,8 @@ void encodeFailureClearsAggregatorSoLaterFlushesRecover() throws Exception { @Test void scoConstructorCreatesUsableWriter() { final FlagEvaluationWriterImpl writer = - new FlagEvaluationWriterImpl(new SharedCommunicationObjects(), cfg()); + new FlagEvaluationWriterImpl( + new SharedCommunicationObjects(), cfg(), new FeatureFlagRouteSelector()); writer.enqueue(simpleEvent("sco-flag", "on")); assertNotNull(writer.pollQueuedEventForTest()); writer.close(); diff --git a/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/SharedFeatureFlagRouteTest.java b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/SharedFeatureFlagRouteTest.java new file mode 100644 index 00000000000..60d39e6faf1 --- /dev/null +++ b/products/feature-flagging/feature-flagging-lib/src/test/java/com/datadog/featureflag/SharedFeatureFlagRouteTest.java @@ -0,0 +1,207 @@ +package com.datadog.featureflag; + +import static com.datadog.featureflag.FeatureFlagEventType.EXPOSURE; +import static com.datadog.featureflag.FeatureFlagEventType.FLAG_EVALUATION; +import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V2_EVP_PROXY_ENDPOINT; +import static datadog.communication.ddagent.DDAgentFeaturesDiscovery.V4_EVP_PROXY_ENDPOINT; +import static datadog.trace.api.featureflag.config.FeatureFlaggingConfig.CONFIGURATION_SOURCE_AGENTLESS; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import datadog.communication.BackendApi; +import datadog.communication.HttpResponseException; +import datadog.communication.ddagent.DDAgentFeaturesDiscovery; +import datadog.communication.ddagent.SharedCommunicationObjects; +import datadog.trace.api.Config; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; +import okhttp3.HttpUrl; +import okhttp3.MediaType; +import okhttp3.OkHttpClient; +import okhttp3.Protocol; +import okhttp3.Request; +import okhttp3.RequestBody; +import okhttp3.Response; +import okhttp3.ResponseBody; +import org.junit.jupiter.api.Test; + +/** Exercises both production factories and HTTP clients against one shared discovery result. */ +class SharedFeatureFlagRouteTest { + private final AtomicLong clock = new AtomicLong(); + private final FeatureFlagRouteSelector selector = new FeatureFlagRouteSelector(clock::get, 10); + private final DDAgentFeaturesDiscovery discovery = mock(DDAgentFeaturesDiscovery.class); + private final Config config = mock(Config.class); + private final SharedCommunicationObjects sco = new SharedCommunicationObjects(); + private final List requests = new ArrayList<>(); + private int status = 200; + + SharedFeatureFlagRouteTest() { + when(config.getFeatureFlaggingConfigurationSource()).thenReturn(CONFIGURATION_SOURCE_AGENTLESS); + when(config.getIdGenerationStrategy()).thenReturn(Config.get().getIdGenerationStrategy()); + when(discovery.supportsEvpProxyHeaders(any())).thenReturn(true); + sco.setFeaturesDiscovery(discovery); + sco.agentUrl = HttpUrl.get("http://localhost:8126/agent-prefix/"); + sco.agentHttpClient = + new OkHttpClient.Builder() + .addInterceptor( + chain -> { + requests.add(chain.request()); + return new Response.Builder() + .request(chain.request()) + .protocol(Protocol.HTTP_1_1) + .code(status) + .message("test") + .body(ResponseBody.create(null, "{}")) + .build(); + }) + .build(); + } + + @Test + void bothWritersUseOneSuccessfulRecoveryWithoutRediscovery() throws Exception { + final BackendApi exposures = create(EXPOSURE); + final BackendApi evaluations = create(FLAG_EVALUATION); + // Only the first recovery can discover a route. A redundant sibling probe would lose it. + when(discovery.getEvpProxyEndpoint()).thenReturn(V4_EVP_PROXY_ENDPOINT).thenReturn(null); + clock.set(10); + + post(exposures, "exposures"); + post(evaluations, "flagevaluation"); + + assertEquals( + Arrays.asList( + "/agent-prefix/evp_proxy/v4/api/v2/exposures", + "/agent-prefix/evp_proxy/v4/api/v2/flagevaluation"), + paths()); + assertEquals("gzip", requests.get(0).header("Accept-Encoding")); + assertEquals("identity", requests.get(1).header("Accept-Encoding")); + verify(discovery).discover(); + verify(discovery).discoverIfOutdated(); + } + + @Test + void bothWritersReplaceTheirOldEndpointAfterRecovery() throws Exception { + when(discovery.getEvpProxyEndpoint()).thenReturn(V4_EVP_PROXY_ENDPOINT); + final BackendApi exposures = create(EXPOSURE); + final BackendApi evaluations = create(FLAG_EVALUATION); + post(evaluations, "flagevaluation"); + status = 404; + assertThrows(HttpResponseException.class, () -> post(exposures, "exposures")); + when(discovery.getEvpProxyEndpoint()).thenReturn(V2_EVP_PROXY_ENDPOINT); + status = 200; + clock.set(10); + + post(exposures, "exposures"); + post(evaluations, "flagevaluation"); + + assertEquals( + Arrays.asList( + "/agent-prefix/evp_proxy/v4/api/v2/flagevaluation", + "/agent-prefix/evp_proxy/v4/api/v2/exposures", + "/agent-prefix/evp_proxy/v2/api/v2/exposures", + "/agent-prefix/evp_proxy/v2/api/v2/flagevaluation"), + paths()); + verify(discovery, times(1)).discover(); + } + + private BackendApi create(FeatureFlagEventType eventType) { + return new FeatureFlagBackendApiFactory(config, sco, eventType, selector).create(); + } + + @Test + void concurrentInitializationSharesOneDiscovery() throws Exception { + final CountDownLatch started = new CountDownLatch(1); + final CountDownLatch release = new CountDownLatch(1); + when(discovery.getEvpProxyEndpoint()).thenReturn(V4_EVP_PROXY_ENDPOINT); + doAnswer( + invocation -> { + started.countDown(); + assertTrue(release.await(5, TimeUnit.SECONDS)); + return null; + }) + .when(discovery) + .discoverIfOutdated(); + final CompletableFuture exposures = + CompletableFuture.supplyAsync(() -> create(EXPOSURE)); + try { + assertTrue(started.await(5, TimeUnit.SECONDS)); + final CompletableFuture evaluations = + CompletableFuture.supplyAsync(() -> create(FLAG_EVALUATION)); + release.countDown(); + post(exposures.get(5, TimeUnit.SECONDS), "exposures"); + post(evaluations.get(5, TimeUnit.SECONDS), "flagevaluation"); + verify(discovery).discoverIfOutdated(); + assertEquals(2, requests.size()); + } finally { + release.countDown(); + } + } + + @Test + void slowRecoveryDoesNotPermitASecondProbeAfterTheCooldown() throws Exception { + final BackendApi exposures = create(EXPOSURE); + final BackendApi evaluations = create(FLAG_EVALUATION); + final CountDownLatch started = new CountDownLatch(1); + final CountDownLatch release = new CountDownLatch(1); + when(discovery.getEvpProxyEndpoint()).thenReturn(V4_EVP_PROXY_ENDPOINT); + doAnswer( + invocation -> { + started.countDown(); + assertTrue(release.await(5, TimeUnit.SECONDS)); + return null; + }) + .when(discovery) + .discover(); + clock.set(10); + final CompletableFuture recovery = + CompletableFuture.runAsync( + () -> { + try { + post(exposures, "exposures"); + } catch (Exception failure) { + throw new AssertionError(failure); + } + }); + try { + assertTrue(started.await(5, TimeUnit.SECONDS)); + clock.set(100); + assertThrows(IOException.class, () -> post(evaluations, "flagevaluation")); + } finally { + release.countDown(); + } + recovery.get(5, TimeUnit.SECONDS); + post(evaluations, "flagevaluation"); + verify(discovery).discover(); + assertEquals(2, requests.size()); + } + + private List paths() { + final List paths = new ArrayList<>(); + for (Request request : requests) { + paths.add(request.url().encodedPath()); + } + return paths; + } + + private static void post(BackendApi api, String path) throws Exception { + api.post( + path, + RequestBody.create(MediaType.parse("application/json"), "{}"), + stream -> null, + null, + false); + } +}