fix(cf): Improved caching agent robustness (#4351)

* fix(cf): Skip caching CF apps having frigga incompatible names

* fix(cf): Stop swallowing exception causes.

Makes for deeper and informative stack traces.

* fix(cf): Race condition causing NPE getting token in the HTTP Client.

Was happening at startup when both the LoadBalancer and Application caching agents were first ran and shared the same client.

* fix(cf): NPE after on-demand cache refresh run

An on-demand cache refresh creates an ON_DEMAND cache entry that never been processed so the `processedCount` was null instead of 0 and the scheduled cache agent wasn't expecting that.

* fix(cf): Adjusted log verbosity level

* fix(cf): Re-enabling cache eviction of a serverGroup on-demand

When not found, a serverGroup wouldn't be evicted from the cache upon an on-demand request.

* fix(cf): Sensible defaults for cache data retrieval

clouddriver-cloudfoundry/src/main/java/com/netflix/spinnaker/clouddriver/cloudfoundry/client/Applications.java

@@ -169,6 +169,13 @@ public List<CloudFoundryApplication> all() {
 
     for (CloudFoundryServerGroup serverGroup : serverGroupCache.asMap().values()) {
       Names names = Names.parseName(serverGroup.getName());
+      if (names.getCluster() == null) {
+        log.debug(
+            "Skipping app '{}' from foundation '{}' because the name isn't following the frigga naming schema.",
+            serverGroup.getName(),
+            this.account);
+        continue;
+      }
       serverGroupsByClusters
           .computeIfAbsent(names.getCluster(), clusterName -> new HashSet<>())
           .add(serverGroup);
@@ -417,7 +424,7 @@ private CloudFoundryServerGroup checkHealthStatus(
                       })
                   .collect(toSet());
 
-          log.debug(
+          log.trace(
               "Successfully retrieved "
                   + instances.size()
                   + " instances for application '"

clouddriver-cloudfoundry/src/main/java/com/netflix/spinnaker/clouddriver/cloudfoundry/client/CloudFoundryApiException.java

@@ -36,14 +36,15 @@ public CloudFoundryApiException(ErrorDescription errorCause, RetrofitError retro
     super(
         Optional.ofNullable(errorCause)
             .map(e -> getMessage(e.getErrors().toArray(new String[0])))
-            .orElse(getRetrofitErrorMessage(retrofitError)));
+            .orElse(getRetrofitErrorMessage(retrofitError)),
+        retrofitError);
     if (errorCause != null) {
       this.errorCode = errorCause.getCode();
     }
   }
 
   public CloudFoundryApiException(RetrofitError retrofitError) {
-    super(getRetrofitErrorMessage(retrofitError));
+    super(getRetrofitErrorMessage(retrofitError), retrofitError);
   }
 
   public CloudFoundryApiException(Throwable t, String... errors) {

clouddriver-cloudfoundry/src/main/java/com/netflix/spinnaker/clouddriver/cloudfoundry/client/HttpCloudFoundryClient.java

@@ -41,7 +41,7 @@
 import java.time.Duration;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -77,8 +77,9 @@ public class HttpCloudFoundryClient implements CloudFoundryClient {
   private Logger logger = LoggerFactory.getLogger(HttpCloudFoundryClient.class);
 
   private AuthenticationService uaaService;
-  private AtomicLong tokenExpirationNs = new AtomicLong(System.nanoTime());
-  private volatile Token token;
+  private long tokenExpiration = System.currentTimeMillis();
+  private Token token;
+  private final Object tokenLock = new Object();
 
   private JacksonConverter jacksonConverter;
 
@@ -93,13 +94,7 @@ public class HttpCloudFoundryClient implements CloudFoundryClient {
   private Logs logs;
 
   private final RequestInterceptor oauthInterceptor =
-      new RequestInterceptor() {
-        @Override
-        public void intercept(RequestFacade request) {
-          refreshTokenIfNecessary();
-          request.addHeader("Authorization", "bearer " + token.getAccessToken());
-        }
-      };
+      request -> request.addHeader("Authorization", "bearer " + getToken(false).getAccessToken());
 
   private static class RetryableApiException extends RuntimeException {
     RetryableApiException(String message) {
@@ -120,7 +115,7 @@ Response createRetryInterceptor(Interceptor.Chain chain) {
                 .intervalFunction(IntervalFunction.ofExponentialBackoff(Duration.ofSeconds(10), 3))
                 .retryExceptions(RetryableApiException.class)
                 .build());
-    logger.debug("cf request: " + chain.request().urlString());
+    logger.trace("cf request: " + chain.request().urlString());
     AtomicReference<Response> lastResponse = new AtomicReference<>();
     try {
       return retry.executeCallable(
@@ -135,13 +130,12 @@ Response createRetryInterceptor(Interceptor.Chain chain) {
                 Buffer buffer = source.buffer();
                 String body = buffer.clone().readString(Charset.forName("UTF-8"));
                 if (!body.contains("Bad credentials")) {
-                  refreshToken();
                   response =
                       chain.proceed(
                           chain
                               .request()
                               .newBuilder()
-                              .header("Authorization", "bearer " + token.getAccessToken())
+                              .header("Authorization", "bearer " + getToken(true).getAccessToken())
                               .build());
                   lastResponse.set(response);
                 }
@@ -188,6 +182,7 @@ public HttpCloudFoundryClient(
     this.password = password;
 
     this.okHttpClient = createHttpClient(skipSslValidation);
+    this.okHttpClient.setReadTimeout(20, TimeUnit.SECONDS);
 
     okHttpClient.interceptors().add(this::createRetryInterceptor);
 
@@ -284,23 +279,19 @@ public X509Certificate[] getAcceptedIssuers() {
     return client;
   }
 
-  private void refreshTokenIfNecessary() {
-    long currentExpiration = tokenExpirationNs.get();
-    long now = System.nanoTime();
-    long comp = Math.min(currentExpiration, now);
-    if (tokenExpirationNs.compareAndSet(comp, now)) {
-      this.refreshToken();
-    }
-  }
-
-  private void refreshToken() {
-    try {
-      token = uaaService.passwordToken("password", user, password, "cf", "");
-    } catch (Exception e) {
-      log.warn("Failed to obtain a token", e);
-      throw e;
+  private Token getToken(boolean forceRefresh) {
+    synchronized (tokenLock) {
+      if (forceRefresh || (token == null || System.currentTimeMillis() >= tokenExpiration)) {
+        try {
+          token = uaaService.passwordToken("password", user, password, "cf", "");
+        } catch (Exception e) {
+          log.warn("Failed to obtain a token", e);
+          throw e;
+        }
+        tokenExpiration = System.currentTimeMillis() + ((token.getExpiresIn() - 120) * 1000);
+      }
+      return token;
     }
-    tokenExpirationNs.addAndGet(Duration.ofSeconds(token.getExpiresIn()).toNanos());
   }
 
   private <S> S createService(Class<S> serviceClass) {

clouddriver-cloudfoundry/src/main/java/com/netflix/spinnaker/clouddriver/cloudfoundry/provider/agent/CloudFoundryLoadBalancerCachingAgent.java

@@ -77,7 +77,7 @@ public CacheResult loadData(ProviderCache providerCache) {
         cacheData -> {
           long cacheTime = (long) cacheData.getAttributes().get("cacheTime");
           if (cacheTime < loadDataStart
-              && (int) cacheData.getAttributes().get("processedCount") > 0) {
+              && (int) cacheData.getAttributes().computeIfAbsent("processedCount", s -> 0) > 0) {
             toEvict.add(cacheData.getId());
           } else {
             toKeep.put(cacheData.getId(), cacheData);

clouddriver-cloudfoundry/src/main/java/com/netflix/spinnaker/clouddriver/cloudfoundry/provider/agent/CloudFoundryServerGroupCachingAgent.java

@@ -92,7 +92,7 @@ public CacheResult loadData(ProviderCache providerCache) {
         cacheData -> {
           long cacheTime = (long) cacheData.getAttributes().get("cacheTime");
           if (cacheTime < loadDataStart
-              && (int) cacheData.getAttributes().get("processedCount") > 0) {
+              && (int) cacheData.getAttributes().computeIfAbsent("processedCount", s -> 0) > 0) {
             toEvict.add(cacheData.getId());
           } else {
             toKeep.put(cacheData.getId(), cacheData);
@@ -150,13 +150,6 @@ public OnDemandResult handle(ProviderCache providerCache, Map<String, ?> data) {
     if (serverGroupName == null) {
       return null;
     }
-    CloudFoundryServerGroup serverGroup =
-        this.getClient()
-            .getApplications()
-            .findServerGroupByNameAndSpaceId(serverGroupName, space.getId());
-    if (serverGroup == null) {
-      return null;
-    }
 
     log.info("On Demand cache refresh triggered, waiting for Server group loadData to be called");
     CloudFoundryServerGroup cloudFoundryServerGroup =

clouddriver-cloudfoundry/src/main/java/com/netflix/spinnaker/clouddriver/cloudfoundry/security/CloudFoundryCredentials.java

@@ -76,7 +76,7 @@ public CloudFoundryCredentials(
     this.password = password;
     this.environment = Optional.ofNullable(environment).orElse("dev");
     this.skipSslValidation = skipSslValidation;
-    this.resultsPerPage = Optional.ofNullable(resultsPerPage).orElse(500);
+    this.resultsPerPage = Optional.ofNullable(resultsPerPage).orElse(100);
     this.maxCapiConnectionsForCache = Optional.ofNullable(maxCapiConnectionsForCache).orElse(16);
   }
 

clouddriver-cloudfoundry/src/test/java/com/netflix/spinnaker/clouddriver/cloudfoundry/provider/agent/CloudFoundryServerGroupCachingAgentTest.java

@@ -115,7 +115,7 @@ void handleShouldReturnNullWhenServerGroupNameIsUnspecified() {
   }
 
   @Test
-  void handleShouldReturnNullWhenServerGroupDoesNotExist() {
+  void handleShouldReturnAnEvictResultWhenServerGroupDoesNotExists() {
     String region = "org > space";
     String serverGroupName = "server-group";
     Map<String, Object> data =
@@ -133,12 +133,15 @@ void handleShouldReturnNullWhenServerGroupDoesNotExist() {
     when(cloudFoundryClient.getApplications()).thenReturn(mockApplications);
     when(mockApplications.findServerGroupByNameAndSpaceId(any(), any())).thenReturn(null);
 
+    when(mockProviderCache.filterIdentifiers(any(), any()))
+        .thenReturn(Collections.singletonList("key"));
+
     OnDemandAgent.OnDemandResult result =
         cloudFoundryServerGroupCachingAgent.handle(mockProviderCache, data);
 
-    assertThat(result).isNull();
-    verify(organizations).findSpaceByRegion(eq(region));
-    verify(mockApplications).findServerGroupByNameAndSpaceId(eq(serverGroupName), eq(spaceId));
+    assertThat(result).isNotNull();
+    assertThat(result.getEvictions()).hasSize(1);
+    assertThat(result.getEvictions().get(SERVER_GROUPS.getNs())).containsExactly("key");
   }
 
   @Test

clouddriver-cloudfoundry/src/test/java/com/netflix/spinnaker/clouddriver/cloudfoundry/security/CloudFoundryCredentialsSynchronizerTest.java

@@ -149,7 +149,7 @@ private CloudFoundryConfigurationProperties.ManagedAccount createAccount(String
 
   private CloudFoundryCredentials createCredentials(String name) {
     return new CloudFoundryCredentials(
-        name, null, null, "api." + name, "user-" + name, "pwd-" + name, null, false, 500, 16);
+        name, null, null, "api." + name, "user-" + name, "pwd-" + name, null, false, null, 16);
   }
 
   private void loadProviderFromRepository() {