FFM-7214 - When FF SDK is not supplied API Key it retries causing tim…

…eout for Client APIs (#144)

* FFM-7214 - When FF SDK is not supplied API Key it retries causing timeout for Client APIs

What
If authentication fails the SDK will no longer attempt to retry constantly.
HarnessConnector will now throw MissingSdkKeyException if a blank/empty key is given.

Why
Platform team are reporting authentication getting stuck in a loop when a blank SDK key is given

Testing
New unit tests to ensure xVariations methods return default values after authentication has failed

README.md

@@ -69,7 +69,7 @@ The first step is to install the FF SDK as a dependency in your application usin
 
 Refer to the [Harness Feature Flag Java Server SDK](https://mvnrepository.com/artifact/io.harness/ff-java-server-sdk) to identify the latest version for your build automation tool.
 
-This section lists dependencies for Maven and Gradle and uses the 1.1.11 version as an example:
+This section lists dependencies for Maven and Gradle and uses the 1.2.1 version as an example:
 
 #### Maven
 
@@ -78,14 +78,14 @@ Add the following Maven dependency in your project's pom.xml file:
 <dependency>
     <groupId>io.harness</groupId>
     <artifactId>ff-java-server-sdk</artifactId>
-    <version>1.1.11</version>
+    <version>1.2.1</version>
 </dependency>
 ```
 
 #### Gradle
 
 ```
-implementation group: 'io.harness', name: 'ff-java-server-sdk', version: '1.1.11'
+implementation group: 'io.harness', name: 'ff-java-server-sdk', version: '1.2.1'
 ```
 
 ### Code Sample

examples/pom.xml

@@ -6,7 +6,7 @@
 
     <groupId>io.harness.featureflags</groupId>
     <artifactId>examples</artifactId>
-    <version>1.2.0</version>
+    <version>1.2.1</version>
 
     <properties>
         <maven.compiler.source>8</maven.compiler.source>
@@ -33,7 +33,7 @@
         <dependency>
             <groupId>io.harness</groupId>
             <artifactId>ff-java-server-sdk</artifactId>
-            <version>1.2.0</version>
+            <version>1.2.1</version>
         </dependency>
 
         <dependency>

pom.xml

@@ -6,7 +6,7 @@
 
     <groupId>io.harness</groupId>
     <artifactId>ff-java-server-sdk</artifactId>
-    <version>1.2.0</version>
+    <version>1.2.1</version>
     <packaging>jar</packaging>
     <name>Harness Feature Flag Java Server SDK</name>
     <description>Harness Feature Flag Java Server SDK</description>

src/main/java/io/harness/cf/client/api/InnerClient.java

@@ -2,7 +2,6 @@
 
 import static com.google.common.util.concurrent.Service.State.*;
 
-import com.google.common.base.Strings;
 import com.google.common.util.concurrent.Service;
 import com.google.gson.JsonObject;
 import io.harness.cf.client.connector.Connector;
@@ -65,10 +64,6 @@ public InnerClient(@NonNull final String sdkKey) {
 
   @Deprecated
   public InnerClient(@NonNull final String sdkKey, @NonNull final Config options) {
-    if (Strings.isNullOrEmpty(sdkKey)) {
-      log.error(MISSING_SDK_KEY);
-      throw new IllegalArgumentException(MISSING_SDK_KEY);
-    }
     HarnessConfig config =
         HarnessConfig.builder()
             .configUrl(options.getConfigUrl())
@@ -82,11 +77,6 @@ public InnerClient(@NonNull final String sdkKey, @NonNull final Config options)
   }
 
   public InnerClient(@NonNull final String sdkKey, @NonNull final BaseConfig options) {
-    if (Strings.isNullOrEmpty(sdkKey)) {
-      log.error(MISSING_SDK_KEY);
-      throw new IllegalArgumentException(MISSING_SDK_KEY);
-    }
-
     HarnessConfig config = HarnessConfig.builder().build();
     HarnessConnector harnessConnector = new HarnessConnector(sdkKey, config);
     setUp(harnessConnector, options);

src/main/java/io/harness/cf/client/api/MissingSdkKeyException.java

@@ -0,0 +1,19 @@
+package io.harness.cf.client.api;
+
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class MissingSdkKeyException extends RuntimeException {
+
+  private static final String MISSING_SDK_KEY = "SDK key cannot be empty!";
+
+  public MissingSdkKeyException() {
+    super(MISSING_SDK_KEY);
+    log.error(MISSING_SDK_KEY);
+  }
+
+  public MissingSdkKeyException(Exception ex) {
+    super(MISSING_SDK_KEY, ex);
+    log.error(MISSING_SDK_KEY, ex);
+  }
+}

src/main/java/io/harness/cf/client/connector/HarnessConnector.java

@@ -5,6 +5,7 @@
 import io.harness.cf.ApiException;
 import io.harness.cf.api.ClientApi;
 import io.harness.cf.api.MetricsApi;
+import io.harness.cf.client.api.MissingSdkKeyException;
 import io.harness.cf.client.dto.Claim;
 import io.harness.cf.client.logger.LogUtil;
 import io.harness.cf.model.*;
@@ -48,6 +49,10 @@ public HarnessConnector(@NonNull String apiKey) {
   }
 
   public HarnessConnector(@NonNull final String apiKey, @NonNull final HarnessConfig options) {
+    if (isNullOrEmpty(apiKey)) {
+      throw new MissingSdkKeyException();
+    }
+
     this.apiKey = apiKey;
     this.options = options;
     this.api = new ClientApi(makeApiClient(2000));
@@ -152,9 +157,12 @@ public String authenticate() throws ConnectorException {
       return token;
     } catch (ApiException apiException) {
       if (apiException.getCode() == 401 || apiException.getCode() == 403) {
-        String errorMsg = String.format("Invalid apiKey %s. SDK will serve default values", apiKey);
+        String errorMsg =
+            String.format(
+                "HTTP error code %d returned for authentication endpoint. Check API key. SDK will serve default values",
+                apiException.getCode());
         log.error(errorMsg);
-        throw new ConnectorException(errorMsg, apiException.getCode(), apiException.getMessage());
+        throw new ConnectorException(errorMsg, false, apiException);
       }
       log.error("Failed to get auth token", apiException);
       throw new ConnectorException(
@@ -382,10 +390,19 @@ private void setupTls(ApiClient apiClient) {
     }
   }
 
+  private static boolean isNullOrEmpty(String string) {
+    return string == null || string.trim().isEmpty();
+  }
+
   /* package private - should not be used outside of tests */
 
   HarnessConnector(
       @NonNull final String apiKey, @NonNull final HarnessConfig options, int retryBackOffDelay) {
+
+    if (isNullOrEmpty(apiKey)) {
+      throw new MissingSdkKeyException();
+    }
+
     this.apiKey = apiKey;
     this.options = options;
     this.api = new ClientApi(makeApiClient(retryBackOffDelay));

src/test/java/io/harness/cf/client/api/CfClientTest.java

@@ -4,6 +4,7 @@
 import static io.harness.cf.client.api.TestUtils.*;
 import static io.harness.cf.client.api.dispatchers.CannedResponses.makeDummyJwtToken;
 import static io.harness.cf.client.api.dispatchers.CannedResponses.makeMockJsonResponse;
+import static io.harness.cf.client.api.dispatchers.Endpoints.AUTH_ENDPOINT;
 import static io.harness.cf.client.connector.HarnessConnectorUtils.*;
 import static org.junit.jupiter.api.Assertions.*;
 
@@ -31,6 +32,7 @@
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
+import org.junit.jupiter.params.provider.ValueSource;
 
 class CfClientTest {
 
@@ -116,8 +118,101 @@ void testConstructors() throws IOException {
   void shouldFailIfEmptySdkKeyGiven() {
     Exception thrown =
         assertThrows(
-            IllegalArgumentException.class, () -> new CfClient(""), "Exception was not thrown");
-    assertInstanceOf(IllegalArgumentException.class, thrown);
+            MissingSdkKeyException.class, () -> new CfClient(""), "Exception was not thrown");
+    assertInstanceOf(MissingSdkKeyException.class, thrown);
+  }
+
+  @Test
+  void shouldFailIfNullSdkKeyGiven() {
+    Exception thrown =
+        assertThrows(
+            NullPointerException.class,
+            () -> new CfClient((String) null),
+            "Exception was not thrown");
+    assertInstanceOf(NullPointerException.class, thrown);
+  }
+
+  @ParameterizedTest
+  @ValueSource(booleans = {true, false})
+  void shouldAllowEvaluationsToContinueWhenAuthFails(boolean shouldWaitForInit)
+      throws InterruptedException, IOException {
+
+    BaseConfig config =
+        BaseConfig.builder()
+            .pollIntervalInSeconds(1)
+            .analyticsEnabled(false)
+            .streamEnabled(true)
+            .build();
+
+    Http403OnAuthDispatcher webserverDispatcher = new Http403OnAuthDispatcher(4); // auth error
+
+    try (MockWebServer mockSvr = new MockWebServer()) {
+      mockSvr.setDispatcher(webserverDispatcher);
+      mockSvr.start();
+
+      try (CfClient client =
+          new CfClient(
+              makeConnectorWithMinimalRetryBackOff(mockSvr.getHostName(), mockSvr.getPort()),
+              config)) {
+
+        if (shouldWaitForInit) {
+          try {
+            client.waitForInitialization();
+          } catch (FeatureFlagInitializeException ex) {
+            // ignore
+          }
+        }
+
+        // Iterate a few times, check we're getting defaults and not blocking
+        for (int i = 0; i < 1000; i++) {
+          if (i % 100 == 0)
+            System.out.printf(
+                "Test that xVariation doesn't block and serves default when auth fails, iteration #%d%n",
+                i);
+
+          boolean b1 = client.boolVariation("test", null, true);
+          boolean b2 =
+              client.boolVariation("test", Target.builder().identifier("test").build(), true);
+          assertTrue(b1);
+          assertTrue(b2);
+
+          String s1 = client.stringVariation("test", null, "def");
+          String s2 =
+              client.stringVariation("test", Target.builder().identifier("test").build(), "def");
+          assertEquals("def", s1);
+          assertEquals("def", s2);
+
+          double n1 = client.numberVariation("test", null, 321);
+          double n2 =
+              client.numberVariation("test", Target.builder().identifier("test").build(), 321);
+          assertEquals(321, n1);
+          assertEquals(321, n2);
+
+          JsonObject json = new JsonObject();
+          json.addProperty("prop", "val");
+
+          JsonObject j1 = client.jsonVariation("test", null, json);
+          JsonObject j2 =
+              client.jsonVariation("test", Target.builder().identifier("test").build(), json);
+          assertEquals("val", j1.get("prop").getAsString());
+          assertEquals("val", j2.get("prop").getAsString());
+        }
+
+        webserverDispatcher.waitForAllConnections(15);
+
+        assertEquals(
+            3 + 1,
+            webserverDispatcher.getUrlMap().get(AUTH_ENDPOINT),
+            "not enough authentication attempts");
+
+        assertEquals(
+            1, webserverDispatcher.getUrlMap().size(), "not enough authentication attempts");
+
+        assertTrue(
+            webserverDispatcher.getUrlMap().containsKey(AUTH_ENDPOINT),
+            "only auth endpoint should have been called");
+      }
+    }
   }
 
   @Test
@@ -366,7 +461,7 @@ void shouldRetryThenReAuthenticateWithoutThrowingIllegalStateException() throws
 
         client.waitForInitialization();
 
-        // First 3 attempts to connect to auth endpoint will return a 4xx, followed by a 200 success
+        // First 3 attempts to connect to auth endpoint will return a 408, followed by a 200 success
         webserverDispatcher.waitForAllConnections(15);
 
         final int expectedAuths = 3 + 3 + 1; // 3+3 failed retries (4xx), 1 success (200)

src/test/java/io/harness/cf/client/api/dispatchers/Endpoints.java

@@ -2,12 +2,12 @@
 
 public class Endpoints {
 
-  static final String AUTH_ENDPOINT = "/api/1.0/client/auth";
-  static final String FEATURES_ENDPOINT =
+  public static final String AUTH_ENDPOINT = "/api/1.0/client/auth";
+  public static final String FEATURES_ENDPOINT =
       "/api/1.0/client/env/00000000-0000-0000-0000-000000000000/feature-configs?cluster=1";
-  static final String SEGMENTS_ENDPOINT =
+  public static final String SEGMENTS_ENDPOINT =
       "/api/1.0/client/env/00000000-0000-0000-0000-000000000000/target-segments?cluster=1";
-  static final String STREAM_ENDPOINT = "/api/1.0/stream?cluster=1";
-  static final String SIMPLE_BOOL_FLAG_ENDPOINT =
+  public static final String STREAM_ENDPOINT = "/api/1.0/stream?cluster=1";
+  public static final String SIMPLE_BOOL_FLAG_ENDPOINT =
       "/api/1.0/client/env/00000000-0000-0000-0000-000000000000/feature-configs/simplebool?cluster=1";
 }

src/test/java/io/harness/cf/client/api/dispatchers/Http403OnAuthDispatcher.java

@@ -0,0 +1,35 @@
+package io.harness.cf.client.api.dispatchers;
+
+import com.google.common.util.concurrent.AtomicLongMap;
+import io.harness.cf.client.api.testutils.PollingAtomicLong;
+import java.util.Objects;
+import lombok.Getter;
+import lombok.SneakyThrows;
+import okhttp3.mockwebserver.*;
+import okhttp3.mockwebserver.MockResponse;
+import okhttp3.mockwebserver.RecordedRequest;
+
+public class Http403OnAuthDispatcher extends Dispatcher {
+
+  private final PollingAtomicLong connectAttempts;
+
+  public Http403OnAuthDispatcher(int minConnectToWaitFor) {
+    connectAttempts = new PollingAtomicLong(minConnectToWaitFor);
+  }
+
+  @Getter final AtomicLongMap<String> urlMap = AtomicLongMap.create();
+
+  @Override
+  @SneakyThrows
+  public MockResponse dispatch(RecordedRequest recordedRequest) {
+    System.out.println("DISPATCH GOT ------> " + recordedRequest.getPath());
+    connectAttempts.getAndIncrement();
+    urlMap.incrementAndGet(Objects.requireNonNull(recordedRequest.getPath()));
+    return new MockResponse().setResponseCode(403);
+  }
+
+  public void waitForAllConnections(int waitTimeSeconds) throws InterruptedException {
+    connectAttempts.waitForMinimumValueToBeReached(
+        waitTimeSeconds, "any", "Did not get minimum number of connection attempts");
+  }
+}

src/test/java/io/harness/cf/client/api/dispatchers/Http4xxOnFirstAuthDispatcher.java

@@ -26,7 +26,7 @@ private MockResponse dispatchAuthResp() {
     System.out.println("DISPATCH authAttempts = " + authAttempts.get());
 
     if (authAttempts.getAndIncrement() < 6) {
-      return makeAuthResponse(403);
+      return makeAuthResponse(408);
     }
     return makeAuthResponse(200);
   }

src/test/java/io/harness/cf/client/connector/HarnessConnectorTest.java

@@ -0,0 +1,33 @@
+package io.harness.cf.client.connector;
+
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.Mockito.mock;
+
+import io.harness.cf.client.api.MissingSdkKeyException;
+import org.junit.jupiter.api.Test;
+
+class HarnessConnectorTest {
+
+  @Test
+  void shouldThrowExceptionWhenEmptyApiKeyIsGiven() {
+
+    Exception thrown =
+        assertThrows(
+            MissingSdkKeyException.class,
+            () -> new HarnessConnector("", mock(HarnessConfig.class)),
+            "Exception was not thrown");
+    assertInstanceOf(MissingSdkKeyException.class, thrown);
+  }
+
+  @Test
+  void shouldThrowExceptionWhenNullApiKeyIsGiven() {
+
+    Exception thrown =
+        assertThrows(
+            NullPointerException.class,
+            () -> new HarnessConnector(null, mock(HarnessConfig.class)),
+            "Exception was not thrown");
+    assertInstanceOf(NullPointerException.class, thrown);
+  }
+}