fix(core): fix the issues caused due to the use of SpinnakerRetrofitErrorHandler in building FiatService (#1838)

* refactor(core): Convert PermissionServiceSpec tests from groovy to java in a new class PermissionServiceTest

* fix(core): fix the issues caused due to the use of SpinnakerRetrofitErrorHandler in building FiatService

* fix(test): remove conversion errors with error codes not in [200, 300) as they never occur

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Author: kirangodishala

gate-core/gate-core.gradle

@@ -33,6 +33,7 @@ dependencies {
   implementation "io.spinnaker.fiat:fiat-core:$fiatVersion"
 
   implementation "io.spinnaker.kork:kork-core"
+  implementation "io.spinnaker.kork:kork-retrofit"
   implementation "io.spinnaker.kork:kork-web"
   implementation "io.spinnaker.kork:kork-security"
   implementation "com.netflix.spectator:spectator-api"

gate-core/src/main/java/com/netflix/spinnaker/gate/retrofit/UpstreamBadRequest.java

@@ -20,6 +20,8 @@
 import static retrofit.RetrofitError.Kind.HTTP;
 
 import com.netflix.spinnaker.kork.exceptions.SpinnakerException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
 import java.util.Collection;
 import retrofit.RetrofitError;
 
@@ -36,6 +38,14 @@ private UpstreamBadRequest(RetrofitError cause) {
     error = cause.getBody();
   }
 
+  private UpstreamBadRequest(SpinnakerHttpException cause) {
+    super(cause.getMessage(), cause);
+    this.setRetryable(cause.getRetryable());
+    status = cause.getResponseCode();
+    url = cause.getUrl();
+    error = cause.getResponseBody();
+  }
+
   public int getStatus() {
     return status;
   }
@@ -66,4 +76,13 @@ public static RuntimeException classifyError(
       return error;
     }
   }
+
+  public static RuntimeException classifyError(SpinnakerServerException error) {
+    if (error instanceof SpinnakerHttpException
+        && ((SpinnakerHttpException) error).getResponseCode() < INTERNAL_SERVER_ERROR.value()) {
+      return new UpstreamBadRequest((SpinnakerHttpException) error);
+    } else {
+      return error;
+    }
+  }
 }

gate-core/src/main/java/com/netflix/spinnaker/gate/services/PermissionService.java

@@ -29,6 +29,8 @@
 import com.netflix.spinnaker.kork.core.RetrySupport;
 import com.netflix.spinnaker.kork.exceptions.SpinnakerException;
 import com.netflix.spinnaker.kork.exceptions.SystemException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
 import com.netflix.spinnaker.security.AuthenticatedRequest;
 import com.netflix.spinnaker.security.User;
 import java.time.Duration;
@@ -47,7 +49,6 @@
 import org.springframework.stereotype.Component;
 import org.springframework.util.CollectionUtils;
 import org.springframework.util.StringUtils;
-import retrofit.RetrofitError;
 
 @Log4j2
 @Component
@@ -83,7 +84,7 @@ public void login(final String userId) {
               permissionEvaluator.invalidatePermission(userId);
               return null;
             });
-      } catch (RetrofitError e) {
+      } catch (SpinnakerServerException e) {
         throw UpstreamBadRequest.classifyError(e);
       }
     }
@@ -98,7 +99,7 @@ public void loginWithRoles(final String userId, final Collection<String> roles)
               permissionEvaluator.invalidatePermission(userId);
               return null;
             });
-      } catch (RetrofitError e) {
+      } catch (SpinnakerServerException e) {
         throw UpstreamBadRequest.classifyError(e);
       }
     }
@@ -109,7 +110,7 @@ public void logout(String userId) {
       try {
         getFiatServiceForLogin().logoutUser(userId);
         permissionEvaluator.invalidatePermission(userId);
-      } catch (RetrofitError e) {
+      } catch (SpinnakerServerException e) {
         throw UpstreamBadRequest.classifyError(e);
       }
     }
@@ -119,7 +120,7 @@ public void sync() {
     if (fiatStatus.isEnabled()) {
       try {
         getFiatServiceForLogin().sync(List.of());
-      } catch (RetrofitError e) {
+      } catch (SpinnakerServerException e) {
         throw UpstreamBadRequest.classifyError(e);
       }
     }
@@ -133,22 +134,22 @@ public Set<Role.View> getRoles(String userId) {
       var permission = permissionEvaluator.getPermission(userId);
       var roles = permission != null ? permission.getRoles() : null;
       return roles != null ? roles : Set.of();
-    } catch (RetrofitError e) {
+    } catch (SpinnakerServerException e) {
       throw UpstreamBadRequest.classifyError(e);
     }
   }
 
   List<UserPermission.View> lookupServiceAccounts(String userId) {
     try {
       return extendedFiatService.getUserServiceAccounts(userId);
-    } catch (RetrofitError re) {
-      var response = re.getResponse();
-      if (response != null && response.getStatus() == HttpStatus.NOT_FOUND.value()) {
+    } catch (SpinnakerHttpException e) {
+      if (e.getResponseCode() == 404) {
         return List.of();
       }
-      boolean shouldRetry =
-          response == null || HttpStatus.valueOf(response.getStatus()).is5xxServerError();
-      throw new SystemException("getUserServiceAccounts failed", re).setRetryable(shouldRetry);
+      boolean shouldRetry = HttpStatus.valueOf(e.getResponseCode()).is5xxServerError();
+      throw new SystemException("getUserServiceAccounts failed", e).setRetryable(shouldRetry);
+    } catch (SpinnakerServerException e) {
+      throw new SystemException("getUserServiceAccounts failed", e).setRetryable(e.getRetryable());
     }
   }
 
@@ -217,8 +218,8 @@ public List<String> getServiceAccounts(@SpinnakerUser User user) {
       return permission.getServiceAccounts().stream()
           .map(ServiceAccount.View::getName)
           .collect(Collectors.toList());
-    } catch (RetrofitError re) {
-      throw UpstreamBadRequest.classifyError(re);
+    } catch (SpinnakerServerException e) {
+      throw UpstreamBadRequest.classifyError(e);
     }
   }