fix(roles): Better logging (and metrics) if a user roles sync fails (#…

…369)
spinnaker · Apr 11, 2019 · 9645925 · 9645925
1 parent 2734743
commit 9645925
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 1 deletion.
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
@@ -18,6 +18,8 @@
 
 import com.netflix.appinfo.InstanceInfo;
 import com.netflix.discovery.DiscoveryClient;
+import com.netflix.spectator.api.Gauge;
+import com.netflix.spectator.api.Registry;
 import com.netflix.spinnaker.fiat.config.ResourceProvidersHealthIndicator;
 import com.netflix.spinnaker.fiat.config.UnrestrictedResourceConfig;
 import com.netflix.spinnaker.fiat.model.UserPermission;
@@ -71,8 +73,11 @@ public class UserRolesSyncer implements ApplicationListener<RemoteStatusChangedE
 
   private final AtomicBoolean isEnabled;
 
+  private final Gauge userRolesSyncCount;
+
   @Autowired
   public UserRolesSyncer(Optional<DiscoveryClient> discoveryClient,
+                         Registry registry,
                          LockManager lockManager,
                          PermissionsRepository permissionsRepository,
                          PermissionsResolver permissionsResolver,
@@ -99,6 +104,8 @@ public UserRolesSyncer(Optional<DiscoveryClient> discoveryClient,
         // default to enabled iff discovery is not available
         !discoveryClient.isPresent()
     );
+
+    this.userRolesSyncCount = registry.gauge("fiat.userRoles.syncCount");
   }
 
   @Override
@@ -109,6 +116,7 @@ public void onApplicationEvent(RemoteStatusChangedEvent event) {
   @Scheduled(fixedDelay = 30000L)
   public void schedule() {
     if (syncDelayMs < 0 || !isEnabled.get()) {
+      log.warn("User roles syncing is disabled (syncDelayMs: {}, isEnabled: {})", syncDelayMs, isEnabled.get());
       return;
     }
 
@@ -118,7 +126,14 @@ public void schedule() {
         .withSuccessInterval(Duration.ofMillis(syncDelayMs))
         .withFailureInterval(Duration.ofMillis(syncFailureDelayMs));
 
-    lockManager.acquireLock(lockOptions, () -> this.syncAndReturn(new ArrayList<>()));
+    lockManager.acquireLock(lockOptions, () -> {
+      try {
+        userRolesSyncCount.set(this.syncAndReturn(new ArrayList<>()));
+      } catch (Exception e) {
+        log.error("User roles synchronization failed", e);
+        userRolesSyncCount.set(-1);
+      }
+    });
   }
 
   public long syncAndReturn(List<String> roles) {

diff --git a/fiat-roles/src/test/groovy/com/netflix/spinnaker/fiat/roles/UserRolesSyncerSpec.groovy b/fiat-roles/src/test/groovy/com/netflix/spinnaker/fiat/roles/UserRolesSyncerSpec.groovy
@@ -20,6 +20,8 @@ import com.fasterxml.jackson.annotation.JsonInclude
 import com.fasterxml.jackson.databind.ObjectMapper
 import com.netflix.appinfo.InstanceInfo.InstanceStatus
 import com.netflix.discovery.DiscoveryClient
+import com.netflix.spectator.api.NoopRegistry
+import com.netflix.spectator.api.Registry
 import com.netflix.spinnaker.fiat.config.ResourceProvidersHealthIndicator
 import com.netflix.spinnaker.fiat.config.UnrestrictedResourceConfig
 import com.netflix.spinnaker.fiat.model.UserPermission
@@ -47,6 +49,9 @@ class UserRolesSyncerSpec extends Specification {
 
   private static final String UNRESTRICTED = UnrestrictedResourceConfig.UNRESTRICTED_USERNAME;
 
+  @Shared
+  Registry registry = new NoopRegistry()
+
   @Shared
   @AutoCleanup("destroy")
   EmbeddedRedis embeddedRedis
@@ -133,6 +138,7 @@ class UserRolesSyncerSpec extends Specification {
     @Subject
     def syncer = new UserRolesSyncer(
         Optional.ofNullable(null),
+        registry,
         lockManager,
         repo,
         permissionsResolver,
@@ -205,6 +211,7 @@ class UserRolesSyncerSpec extends Specification {
     def lockManager = Mock(LockManager)
     def userRolesSyncer = new UserRolesSyncer(
         Optional.ofNullable(discoveryClient),
+        registry,
         lockManager,
         null,
         null,