Improves startup behavior when dependent services are down. (#69)

spinnaker · Sep 26, 2016 · 71501a1 · 71501a1
1 parent 6652189
commit 71501a1
Show file tree

Hide file tree

Showing 11 changed files with 212 additions and 107 deletions.
diff --git a/...nfig/ResourceProviderHealthIndicator.java → ...fig/ResourceProvidersHealthIndicator.java b/...nfig/ResourceProviderHealthIndicator.java → ...fig/ResourceProvidersHealthIndicator.java
@@ -17,21 +17,28 @@
 package com.netflix.spinnaker.fiat.config;
 
 import com.netflix.spinnaker.fiat.providers.BaseProvider;
+import com.netflix.spinnaker.fiat.providers.ProviderException;
+import com.netflix.spinnaker.fiat.providers.ResourceProvider;
 import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.actuate.health.AbstractHealthIndicator;
 import org.springframework.boot.actuate.health.Health;
 import org.springframework.stereotype.Component;
 
 import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
 
+@Slf4j
 @Component
-public class ResourceProviderHealthIndicator extends AbstractHealthIndicator {
+public class ResourceProvidersHealthIndicator extends AbstractHealthIndicator {
 
   @Autowired
   @Setter
   List<BaseProvider> providers;
 
+  private AtomicBoolean previousHealthCheckIsUp = new AtomicBoolean(false);
+
   @Override
   protected void doHealthCheck(Health.Builder builder) throws Exception {
     boolean isDown = false;
@@ -41,8 +48,14 @@ protected void doHealthCheck(Health.Builder builder) throws Exception {
     }
 
     if (isDown) {
+      if (previousHealthCheckIsUp.getAndSet(false)) {
+        log.warn("Server is now UNHEALTHY");
+      }
       builder.down();
     } else {
+      if (!previousHealthCheckIsUp.getAndSet(true)) {
+        log.info("Server is now HEALTHY. Hooray!");
+      }
       builder.up();
     }
   }

diff --git a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/config/UnrestrictedResourceConfig.java b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/config/UnrestrictedResourceConfig.java
@@ -16,6 +16,7 @@
 
 package com.netflix.spinnaker.fiat.config;
 
+import com.netflix.spinnaker.fiat.model.UserPermission;
 import com.netflix.spinnaker.fiat.permissions.PermissionsRepository;
 import com.netflix.spinnaker.fiat.permissions.PermissionsResolver;
 import org.springframework.context.annotation.Bean;
@@ -27,9 +28,8 @@ public class UnrestrictedResourceConfig {
   public static String UNRESTRICTED_USERNAME = "__unrestricted_user__";
 
   @Bean
-  String addUnrestrictedUser(PermissionsRepository permissionsRepository,
-                             PermissionsResolver permissionsResolver) {
-    permissionsResolver.resolveUnrestrictedUser().ifPresent(permissionsRepository::put);
+  String addUnrestrictedUser(PermissionsRepository permissionsRepository) {
+    permissionsRepository.put(new UserPermission().setId(UNRESTRICTED_USERNAME));
     return UNRESTRICTED_USERNAME;
   }
 }
diff --git a/...oles/src/main/java/com/netflix/spinnaker/fiat/permissions/DefaultPermissionsResolver.java b/...oles/src/main/java/com/netflix/spinnaker/fiat/permissions/DefaultPermissionsResolver.java
@@ -54,24 +54,23 @@ public class DefaultPermissionsResolver implements PermissionsResolver {
   private List<ResourceProvider> resourceProviders;
 
   @Override
-  public Optional<UserPermission> resolveUnrestrictedUser() {
+  public UserPermission resolveUnrestrictedUser() {
     return getUserPermission(UnrestrictedResourceConfig.UNRESTRICTED_USERNAME,
                              Collections.emptySet() /* groups */);
   }
 
   @Override
-  public Optional<UserPermission> resolve(@NonNull String userId) {
+  public UserPermission resolve(@NonNull String userId) {
     return resolveAndMerge(userId, Collections.emptySet());
   }
 
   @Override
-  public Optional<UserPermission> resolveAndMerge(@NonNull String userId, Collection<Role> externalRoles) {
+  public UserPermission resolveAndMerge(@NonNull String userId, Collection<Role> externalRoles) {
     List<Role> roles;
     try {
       roles = userRolesProvider.loadRoles(userId);
-    } catch (ProviderException e) {
-      log.warn("Failed to resolve user permission for user " + userId, e);
-      return Optional.empty();
+    } catch (ProviderException pe) {
+      throw new PermissionResolutionException("Failed to resolve user permission for user " + userId, pe);
     }
     Set<Role> combo = Stream.concat(roles.stream(), externalRoles.stream())
                       .collect(Collectors.toSet());
@@ -80,7 +79,7 @@ public Optional<UserPermission> resolveAndMerge(@NonNull String userId, Collecti
   }
 
   @SuppressWarnings("unchecked")
-  private Optional<UserPermission> getUserPermission(String userId, Set<Role> roles) {
+  private UserPermission getUserPermission(String userId, Set<Role> roles) {
     UserPermission permission = new UserPermission().setId(userId).setRoles(roles);
 
     for (ResourceProvider provider : resourceProviders) {
@@ -91,15 +90,10 @@ private Optional<UserPermission> getUserPermission(String userId, Set<Role> role
           permission.addResources(provider.getAllUnrestricted());
         }
       } catch (ProviderException pe) {
-        log.warn("Can't resolve permission for '{}' due to error with '{}': {}",
-                 userId,
-                 provider.getClass().getSimpleName(),
-                 pe.getMessage());
-        log.debug("Error resolving UserPermission", pe);
-        return Optional.empty();
+        throw new PermissionResolutionException(pe);
       }
     }
-    return Optional.of(permission);
+    return permission;
   }
 
   @Override
@@ -126,11 +120,7 @@ public Map<String, UserPermission> resolve(@NonNull Collection<String> userIds)
               }
             });
       } catch (ProviderException pe) {
-        log.warn("Can't getAllRestricted '{}' resources: {}",
-                 provider.getClass().getSimpleName(),
-                 pe.getMessage());
-        log.debug("Error resolving UserPermission", pe);
-        return Collections.emptyMap();
+        throw new PermissionResolutionException(pe);
       }
     }
 

diff --git a/...s/src/main/java/com/netflix/spinnaker/fiat/permissions/PermissionResolutionException.java b/...s/src/main/java/com/netflix/spinnaker/fiat/permissions/PermissionResolutionException.java
@@ -0,0 +1,38 @@
+/*
+ * Copyright 2016 Google, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License")
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.netflix.spinnaker.fiat.permissions;
+
+public class PermissionResolutionException extends RuntimeException {
+  public PermissionResolutionException() {
+  }
+
+  public PermissionResolutionException(String message) {
+    super(message);
+  }
+
+  public PermissionResolutionException(String message, Throwable cause) {
+    super(message, cause);
+  }
+
+  public PermissionResolutionException(Throwable cause) {
+    super(cause);
+  }
+
+  public PermissionResolutionException(String message, Throwable cause, boolean enableSuppression, boolean writableStackTrace) {
+    super(message, cause, enableSuppression, writableStackTrace);
+  }
+}
diff --git a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/permissions/PermissionsResolver.java b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/permissions/PermissionsResolver.java
@@ -26,24 +26,23 @@
 public interface PermissionsResolver {
 
   /**
-   * @return The UserPermission for an anonymous user. May return empty if anonymous users are
-   * disabled.
+   * @return The UserPermission for an anonymous user.
    */
-  Optional<UserPermission> resolveUnrestrictedUser();
+  UserPermission resolveUnrestrictedUser() throws PermissionResolutionException;
 
   /**
    * Resolves a single user's permissions.
    */
-  Optional<UserPermission> resolve(String userId);
+  UserPermission resolve(String userId) throws PermissionResolutionException;
 
   /**
    * Resolves a single user's permissions, taking into account externally
    * provided list of roles.
    */
-  Optional<UserPermission> resolveAndMerge(String userId, Collection<Role> externalRoles);
+  UserPermission resolveAndMerge(String userId, Collection<Role> externalRoles) throws PermissionResolutionException;
 
   /**
    * Resolves multiple user's permissions. Returned map is keyed by userId.
    */
-  Map<String, UserPermission> resolve(Collection<String> userIds);
+  Map<String, UserPermission> resolve(Collection<String> userIds) throws PermissionResolutionException;
 }
diff --git a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/roles/UserRolesSyncer.java b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/roles/UserRolesSyncer.java
@@ -16,8 +16,12 @@
 
 package com.netflix.spinnaker.fiat.roles;
 
+import com.diffplug.common.base.Functions;
+import com.netflix.spinnaker.fiat.config.ResourceProvidersHealthIndicator;
 import com.netflix.spinnaker.fiat.config.UnrestrictedResourceConfig;
 import com.netflix.spinnaker.fiat.model.UserPermission;
+import com.netflix.spinnaker.fiat.model.resources.ServiceAccount;
+import com.netflix.spinnaker.fiat.permissions.PermissionResolutionException;
 import com.netflix.spinnaker.fiat.permissions.PermissionsRepository;
 import com.netflix.spinnaker.fiat.permissions.PermissionsResolver;
 import com.netflix.spinnaker.fiat.providers.ProviderException;
@@ -27,13 +31,19 @@
 import lombok.val;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.Value;
+import org.springframework.boot.actuate.health.Status;
 import org.springframework.scheduling.annotation.Scheduled;
 import org.springframework.stereotype.Component;
 import org.springframework.util.backoff.BackOffExecution;
 import org.springframework.util.backoff.FixedBackOff;
 
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
 import java.util.Map;
-import java.util.concurrent.atomic.AtomicInteger;
+import java.util.Set;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 @Slf4j
 @Component
@@ -51,6 +61,10 @@ public class UserRolesSyncer {
   @Setter
   private ServiceAccountProvider serviceAccountProvider;
 
+  @Autowired
+  @Setter
+  private ResourceProvidersHealthIndicator healthIndicator;
+
   @Value("${auth.userSync.retryIntervalMs:10000}")
   @Setter
   private long retryIntervalMs;
@@ -62,57 +76,78 @@ public void sync() {
   }
 
   public long syncAndReturn() {
-    updateServiceAccounts();
-    return updateUserPermissions();
-  }
-
-  private void updateServiceAccounts() {
-    AtomicInteger updateCount = new AtomicInteger(0);
-
     FixedBackOff backoff = new FixedBackOff();
     backoff.setInterval(retryIntervalMs);
     BackOffExecution backOffExec = backoff.start();
 
+    if (!isServerHealthy()) {
+      log.warn("Server is currently UNHEALTHY. User permission role synchronization and " +
+                   "resolution may not complete until this server becomes healthy again.");
+    }
+
     while (true) {
       try {
-        serviceAccountProvider
-            .getAll()
-            .forEach(serviceAccount -> permissionsResolver
-                .resolve(serviceAccount.getName())
-                .ifPresent(permission -> {
-                  permissionsRepository.put(permission);
-                  updateCount.incrementAndGet();
-                })
-            );
-        log.info("Synced " + updateCount.get() + " service accounts");
-        return;
-      } catch (ProviderException pe) {
+        Map<String, UserPermission> combo = new HashMap<>();
+        Map<String, UserPermission> temp;
+        if (!(temp = getUserPermissions()).isEmpty()) {
+          combo.putAll(temp);
+        }
+        if (!(temp = getServiceAccountsAsMap()).isEmpty()) {
+          combo.putAll(temp);
+        }
+
+        return updateUserPermissions(combo);
+      } catch (ProviderException|PermissionResolutionException ex) {
+        Status status = healthIndicator.health().getStatus();
         long waitTime = backOffExec.nextBackOff();
         if (waitTime == BackOffExecution.STOP) {
-          return;
+          log.error("Unable to resolve service account permissions.", ex);
+          return 0;
+        }
+        String message = new StringBuilder("User permission sync failed. ")
+            .append("Server status is ")
+            .append(status)
+            .append(". Trying again in ")
+            .append(waitTime)
+            .append(" ms. Cause:")
+            .append(ex.getMessage())
+            .toString();
+        if (log.isDebugEnabled()) {
+          log.debug(message, ex);
+        } else {
+          log.warn(message);
         }
-        log.warn("Service account resolution failed. Trying again in " + waitTime + "ms.", pe);
 
         try {
           Thread.sleep(waitTime);
         } catch (InterruptedException ignored) {
-          return;
         }
+      } finally {
+        isServerHealthy();
       }
     }
   }
 
-  public long updateUserPermissions() {
-    val permissionMap = permissionsRepository.getAllById();
-    return updateUserPermissions(permissionMap);
+  private boolean isServerHealthy() {
+    return healthIndicator.health().getStatus() == Status.UP;
+  }
+
+  private Map<String, UserPermission> getServiceAccountsAsMap() {
+    return serviceAccountProvider
+        .getAll()
+        .stream()
+        .map(serviceAccount -> new UserPermission().setId(serviceAccount.getName()))
+        .collect(Collectors.toMap(UserPermission::getId, Functions.identity()));
+  }
+
+  private Map<String, UserPermission> getUserPermissions() {
+    return permissionsRepository.getAllById();
   }
 
   public long updateUserPermissions(Map<String, UserPermission> permissionsById) {
     if (permissionsById.remove(UnrestrictedResourceConfig.UNRESTRICTED_USERNAME) != null) {
-      permissionsResolver.resolveUnrestrictedUser().ifPresent(permission -> {
-        permissionsRepository.put(permission);
-        log.info("Synced anonymous user role.");
-      });
+      permissionsRepository.put(permissionsResolver.resolveUnrestrictedUser());
+      log.info("Synced anonymous user role.");
     }
 
     long count = permissionsResolver.resolve(permissionsById.keySet())

diff --git a/.../test/groovy/com/netflix/spinnaker/fiat/permissions/DefaultPermissionsResolverSpec.groovy b/.../test/groovy/com/netflix/spinnaker/fiat/permissions/DefaultPermissionsResolverSpec.groovy
@@ -81,7 +81,7 @@ class DefaultPermissionsResolverSpec extends Specification {
         .setResourceProviders(resourceProviders)
 
     when:
-    def result = resolver.resolveUnrestrictedUser().get()
+    def result = resolver.resolveUnrestrictedUser()
 
     then:
     result == new UserPermission().setId("__unrestricted_user__")
@@ -106,15 +106,15 @@ class DefaultPermissionsResolverSpec extends Specification {
     thrown IllegalArgumentException
 
     when:
-    def result = resolver.resolve(testUserId).get()
+    def result = resolver.resolve(testUserId)
 
     then:
     1 * userRolesProvider.loadRoles(testUserId) >> []
     def expected = new UserPermission().setId(testUserId)
     result == expected
 
     when:
-    result = resolver.resolve(testUserId).get()
+    result = resolver.resolve(testUserId)
 
     then:
     1 * userRolesProvider.loadRoles(testUserId) >> [role2]
@@ -124,7 +124,7 @@ class DefaultPermissionsResolverSpec extends Specification {
     result == expected
 
     when: "merge externally provided roles"
-    result = resolver.resolveAndMerge(testUserId, [role1]).get()
+    result = resolver.resolveAndMerge(testUserId, [role1])
 
     then:
     1 * userRolesProvider.loadRoles(testUserId) >> [role2]