From 8a72a1164074003c74820a1797ea317ade901652 Mon Sep 17 00:00:00 2001 From: Cameron Fieber Date: Wed, 1 Aug 2018 12:27:04 -0700 Subject: [PATCH] feat(api): FiatPermissionEvaluator reliablity (#254) Adds retry support for calls to fiat in FiatPermissionEvaluator Invalidates cached users if they contain legacyFallback permissions --- .../FiatClientConfigurationProperties.java | 10 +++ .../fiat/shared/FiatPermissionEvaluator.java | 82 +++++++++++++++++-- .../shared/FiatPermissionEvaluatorSpec.groovy | 23 +++++- 3 files changed, 105 insertions(+), 10 deletions(-) diff --git a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatClientConfigurationProperties.java b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatClientConfigurationProperties.java index 4887d9bae..e0366dd6f 100644 --- a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatClientConfigurationProperties.java +++ b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatClientConfigurationProperties.java @@ -33,10 +33,20 @@ public class FiatClientConfigurationProperties { @NestedConfigurationProperty private PermissionsCache cache = new PermissionsCache(); + @NestedConfigurationProperty + private RetryConfiguration retry = new RetryConfiguration(); + @Data class PermissionsCache { private Integer maxEntries = 1000; private Integer expiresAfterWriteSeconds = 20; } + + @Data + class RetryConfiguration { + private long maxBackoffMillis = 10000; + private long initialBackoffMillis = 500; + private double retryMultiplier = 1.5; + } } diff --git a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java index 98a8ae689..5b6c76742 100644 --- a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java +++ b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluator.java @@ -37,10 +37,13 @@ import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.stereotype.Component; +import org.springframework.util.backoff.BackOffExecution; +import org.springframework.util.backoff.ExponentialBackOff; import java.io.Serializable; import java.util.Arrays; import java.util.Set; +import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; @@ -59,14 +62,73 @@ public class FiatPermissionEvaluator implements PermissionEvaluator { private final Id getPermissionCounterId; + private final RetryHandler retryHandler; + + + interface RetryHandler { + default T retry(String description, Callable callable) throws Exception { + return callable.call(); + } + + RetryHandler NOOP = new RetryHandler() {}; + } + + /** + * @see ExponentialBackOff + */ + static class ExponentialBackoffRetryHandler implements RetryHandler { + private final long maxBackoff; + private final long initialBackoff; + private final double backoffMultiplier; + + public ExponentialBackoffRetryHandler(long maxBackoff, long initialBackoff, double backoffMultiplier) { + this.maxBackoff = maxBackoff; + this.initialBackoff = initialBackoff; + this.backoffMultiplier = backoffMultiplier; + } + + public T retry(String description, Callable callable) throws Exception { + ExponentialBackOff backOff = new ExponentialBackOff(initialBackoff, backoffMultiplier); + backOff.setMaxElapsedTime(maxBackoff); + BackOffExecution backOffExec = backOff.start(); + while (true) { + try { + return callable.call(); + } catch (Throwable e) { + long waitTime = backOffExec.nextBackOff(); + if (waitTime == BackOffExecution.STOP) { + throw e; + } + log.warn(description + " failed. Retrying in " + waitTime + "ms", e); + TimeUnit.MILLISECONDS.sleep(waitTime); + } + } + } + + } + @Autowired public FiatPermissionEvaluator(Registry registry, FiatService fiatService, FiatClientConfigurationProperties configProps, FiatStatus fiatStatus) { + this(registry, fiatService, configProps, fiatStatus, buildRetryHandler(configProps)); + } + + private static RetryHandler buildRetryHandler(FiatClientConfigurationProperties fiatClientConfigurationProperties) { + FiatClientConfigurationProperties.RetryConfiguration retry = fiatClientConfigurationProperties.getRetry(); + return new ExponentialBackoffRetryHandler(retry.getMaxBackoffMillis(), retry.getInitialBackoffMillis(), retry.getRetryMultiplier()); + } + + FiatPermissionEvaluator(Registry registry, + FiatService fiatService, + FiatClientConfigurationProperties configProps, + FiatStatus fiatStatus, + RetryHandler retryHandler) { this.registry = registry; this.fiatService = fiatService; this.fiatStatus = fiatStatus; + this.retryHandler = retryHandler; this.permissionsCache = CacheBuilder .newBuilder() @@ -147,7 +209,7 @@ public UserPermission.View getPermission(String username) { cacheHit.set(false); return AuthenticatedRequest.propagate(() -> { try { - return fiatService.getUserPermission(username); + return retryHandler.retry("getUserPermission for " + username, () -> fiatService.getUserPermission(username)); } catch (Exception e) { if (!fiatStatus.isLegacyFallbackEnabled()) { throw e; @@ -176,22 +238,24 @@ public UserPermission.View getPermission(String username) { exception.set(e.getCause() != null ? e.getCause() : e); } - if (!successfulLookup.get()) { - log.error( - "Cannot get whole user permission for user {}, reason: {}", - username, - exception.get().getMessage() - ); - } - Id id = getPermissionCounterId .withTag("cached", cacheHit.get()) .withTag("success", successfulLookup.get()); if (!successfulLookup.get()) { + log.error( + "Cannot get whole user permission for user {}, reason: {}", + username, + exception.get().getMessage(), + exception + ); id = id.withTag("legacyFallback", legacyFallback.get()); } + if (legacyFallback.get()) { + permissionsCache.invalidate(username); + } + registry.counter(id).increment(); return view; diff --git a/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy b/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy index 1e6a3e5e3..cf7e5b0ba 100644 --- a/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy +++ b/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatPermissionEvaluatorSpec.groovy @@ -45,7 +45,8 @@ class FiatPermissionEvaluatorSpec extends Specification { registry, fiatService, buildConfigurationProperties(), - fiatStatus + fiatStatus, + FiatPermissionEvaluator.RetryHandler.NOOP ) def cleanup() { @@ -149,6 +150,26 @@ class FiatPermissionEvaluatorSpec extends Specification { hasPermission } + @Unroll + def "should retry fiat requests"() { + given: + FiatPermissionEvaluator evaluator = new FiatPermissionEvaluator( + registry, + fiatService, + buildConfigurationProperties(), + fiatStatus, + new FiatPermissionEvaluator.ExponentialBackoffRetryHandler(10, 15, 1) + ) + + when: + def view = evaluator.getPermission("testUser") + + then: + 2 * fiatService.getUserPermission("testUser") >> { throw new IllegalStateException("something something something")} + + view == null + } + @Unroll def "should support legacy fallback when fiat is unavailable"() { given: