feat(exceptions): Add SpinnakerRetrofitErrorHandler and replace Retro…

…fitError catch blocks (#1034) * add SpinnakerRetrofitErrorHandler and replace RetrofitError catch blocks * use native SpinnakerException setRetryable method instead of using IntegrationException * remove explicit retry configuration from exception handling as it's covered by SpinnakerRetrofitErrorHandler * use SpinnakerHttpException instead of NotFoundException * chore(spotless): apply spotless fixes --------- Co-authored-by: abe garcia <abraham_garcia-ortiz1@homedepot.com> Co-authored-by: David Byron <dbyron@salesforce.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
spinnaker · Mar 30, 2023 · 59dd50a · 59dd50a
1 parent 0cc5ab8
commit 59dd50a
Show file tree

Hide file tree

Showing 8 changed files with 39 additions and 30 deletions.
diff --git a/fiat-api/fiat-api.gradle b/fiat-api/fiat-api.gradle
@@ -28,6 +28,7 @@ dependencies {
   implementation "io.spinnaker.kork:kork-security"
   implementation "io.spinnaker.kork:kork-telemetry"
   implementation "io.spinnaker.kork:kork-web"
+  implementation "io.spinnaker.kork:kork-retrofit"
   implementation "com.squareup.retrofit:retrofit"
   implementation "com.squareup.retrofit:converter-jackson"
   implementation "com.jakewharton.retrofit:retrofit1-okhttp3-client"

diff --git a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatAuthenticationConfig.java
@@ -24,6 +24,7 @@
 import com.netflix.spinnaker.config.DefaultServiceEndpoint;
 import com.netflix.spinnaker.config.ErrorConfiguration;
 import com.netflix.spinnaker.config.okhttp3.OkHttpClientProvider;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler;
 import com.netflix.spinnaker.kork.web.exceptions.ExceptionMessageDecorator;
 import com.netflix.spinnaker.okhttp.SpinnakerRequestInterceptor;
 import com.netflix.spinnaker.retrofit.Slf4jRetrofitLogger;
@@ -81,6 +82,7 @@ public FiatService fiatService(
         .setRequestInterceptor(interceptor)
         .setClient(new Ok3Client(okHttpClient))
         .setConverter(new JacksonConverter(objectMapper))
+        .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
         .setLogLevel(retrofitLogLevel)
         .setLog(new Slf4jRetrofitLogger(FiatService.class))
         .build()

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
@@ -26,7 +26,7 @@
 import com.netflix.spinnaker.fiat.model.resources.Account;
 import com.netflix.spinnaker.fiat.model.resources.Authorizable;
 import com.netflix.spinnaker.fiat.model.resources.ResourceType;
-import com.netflix.spinnaker.kork.exceptions.IntegrationException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
 import com.netflix.spinnaker.kork.telemetry.caffeine.CaffeineStatsCounter;
 import com.netflix.spinnaker.security.AccessControlled;
 import com.netflix.spinnaker.security.AuthenticatedRequest;
@@ -55,7 +55,6 @@
 import org.springframework.stereotype.Component;
 import org.springframework.util.backoff.BackOffExecution;
 import org.springframework.util.backoff.ExponentialBackOff;
-import retrofit.RetrofitError;
 
 @Component
 @Slf4j
@@ -188,19 +187,11 @@ public boolean canCreate(String resourceType, Object resource) {
                       try {
                         fiatService.canCreate(username, resourceType, resource);
                         return true;
-                      } catch (RetrofitError re) {
-                        boolean shouldRetry = true;
-                        if (re.getKind() == RetrofitError.Kind.HTTP) {
-                          switch (HttpStatus.valueOf(re.getResponse().getStatus())) {
-                            case NOT_FOUND:
-                              return false;
-                            case BAD_REQUEST:
-                              shouldRetry = false;
-                          }
+                      } catch (SpinnakerHttpException e) {
+                        if (e.getResponseCode() == HttpStatus.NOT_FOUND.value()) {
+                          return false;
                         }
-                        IntegrationException ie = new IntegrationException(re);
-                        ie.setRetryable(shouldRetry);
-                        throw ie;
+                        throw e;
                       }
                     });
               })

diff --git a/fiat-github/fiat-github.gradle b/fiat-github/fiat-github.gradle
@@ -9,5 +9,6 @@ dependencies {
   implementation "com.squareup.retrofit:converter-jackson"
   implementation "com.jakewharton.retrofit:retrofit1-okhttp3-client"
   implementation "io.spinnaker.kork:kork-web"
+  implementation "io.spinnaker.kork:kork-retrofit"
   implementation "javax.validation:validation-api"
 }
diff --git a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/config/GitHubConfig.java b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/config/GitHubConfig.java
@@ -5,6 +5,7 @@
 import com.netflix.spinnaker.config.okhttp3.OkHttpClientProvider;
 import com.netflix.spinnaker.fiat.roles.github.GitHubProperties;
 import com.netflix.spinnaker.fiat.roles.github.client.GitHubClient;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler;
 import lombok.Setter;
 import lombok.extern.slf4j.Slf4j;
 import org.slf4j.Logger;
@@ -44,6 +45,7 @@ public GitHubClient gitHubClient(OkHttpClientProvider clientProvider) {
                 clientProvider.getClient(
                     new DefaultServiceEndpoint("github", gitHubProperties.getBaseUrl()))))
         .setConverter(new JacksonConverter())
+        .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
         .setLogLevel(retrofitLogLevel)
         .setLog(new Slf4jRetrofitLogger(GitHubClient.class))
         .build()

diff --git a/...b/src/main/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProvider.java b/...b/src/main/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProvider.java
@@ -27,6 +27,8 @@
 import com.netflix.spinnaker.fiat.roles.github.client.GitHubClient;
 import com.netflix.spinnaker.fiat.roles.github.model.Member;
 import com.netflix.spinnaker.fiat.roles.github.model.Team;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException;
 import java.util.*;
 import java.util.concurrent.*;
 import java.util.stream.Collectors;
@@ -38,10 +40,10 @@
 import org.springframework.beans.factory.InitializingBean;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
+import org.springframework.http.HttpHeaders;
+import org.springframework.http.HttpStatus;
 import org.springframework.stereotype.Component;
 import org.springframework.util.Assert;
-import retrofit.RetrofitError;
-import retrofit.client.Header;
 
 @Slf4j
 @Component
@@ -268,8 +270,10 @@ private List<Team> getTeamsInOrgPaginated(String organization, int page) {
     try {
       log.debug("Requesting page " + page + " of teams.");
       teams = gitHubClient.getOrgTeams(organization, page, gitHubProperties.paginationValue);
-    } catch (RetrofitError e) {
-      if (e.getResponse().getStatus() != 404) {
+    } catch (SpinnakerNetworkException e) {
+      log.error(String.format("Could not find the server %s", gitHubProperties.getBaseUrl()), e);
+    } catch (SpinnakerHttpException e) {
+      if (e.getResponseCode() != HttpStatus.NOT_FOUND.value()) {
         handleNon404s(e);
         throw e;
       } else {
@@ -285,8 +289,10 @@ private List<Member> getMembersInOrgPaginated(String organization, int page) {
     try {
       log.debug("Requesting page " + page + " of members.");
       members = gitHubClient.getOrgMembers(organization, page, gitHubProperties.paginationValue);
-    } catch (RetrofitError e) {
-      if (e.getResponse().getStatus() != 404) {
+    } catch (SpinnakerNetworkException e) {
+      log.error(String.format("Could not find the server %s", gitHubProperties.getBaseUrl()), e);
+    } catch (SpinnakerHttpException e) {
+      if (e.getResponseCode() != HttpStatus.NOT_FOUND.value()) {
         handleNon404s(e);
         throw e;
       } else {
@@ -304,8 +310,10 @@ private List<Member> getMembersInTeamPaginated(String organization, String teamS
       members =
           gitHubClient.getMembersOfTeam(
               organization, teamSlug, page, gitHubProperties.paginationValue);
-    } catch (RetrofitError e) {
-      if (e.getResponse().getStatus() != 404) {
+    } catch (SpinnakerNetworkException e) {
+      log.error(String.format("Could not find the server %s", gitHubProperties.getBaseUrl()), e);
+    } catch (SpinnakerHttpException e) {
+      if (e.getResponseCode() != HttpStatus.NOT_FOUND.value()) {
         handleNon404s(e);
         throw e;
       } else {
@@ -325,17 +333,16 @@ private boolean isMemberOfTeam(Team t, String username) {
     return false;
   }
 
-  private void handleNon404s(RetrofitError e) {
+  private void handleNon404s(SpinnakerHttpException e) {
     String msg = "";
-    if (e.getKind() == RetrofitError.Kind.NETWORK) {
-      msg = String.format("Could not find the server %s", gitHubProperties.getBaseUrl());
-    } else if (e.getResponse().getStatus() == 401) {
+    if (e.getResponseCode() == HttpStatus.UNAUTHORIZED.value()) {
       msg = "HTTP 401 Unauthorized.";
-    } else if (e.getResponse().getStatus() == 403) {
+    } else if (e.getResponseCode() == HttpStatus.FORBIDDEN.value()) {
+      HttpHeaders headers = e.getHeaders();
       val rateHeaders =
-          e.getResponse().getHeaders().stream()
-              .filter(header -> RATE_LIMITING_HEADERS.contains(header.getName()))
-              .map(Header::toString)
+          RATE_LIMITING_HEADERS.stream()
+              .filter(headers::containsKey)
+              .map(headers::getFirst)
               .collect(Collectors.toList());
 
       msg = "HTTP 403 Forbidden. Rate limit info: " + StringUtils.join(rateHeaders, ", ");

diff --git a/fiat-web/fiat-web.gradle b/fiat-web/fiat-web.gradle
@@ -22,6 +22,7 @@ dependencies {
   implementation "io.spinnaker.kork:kork-config"
   implementation "io.spinnaker.kork:kork-plugins"
   implementation "io.spinnaker.kork:kork-web"
+  implementation "io.spinnaker.kork:kork-retrofit"
   implementation "io.swagger:swagger-annotations"
   implementation "net.logstash.logback:logstash-logback-encoder"
 

diff --git a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/ResourcesConfig.java
@@ -22,6 +22,7 @@
 import com.netflix.spinnaker.config.okhttp3.OkHttpClientProvider;
 import com.netflix.spinnaker.fiat.providers.ProviderHealthTracker;
 import com.netflix.spinnaker.fiat.providers.internal.*;
+import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerRetrofitErrorHandler;
 import com.netflix.spinnaker.retrofit.Slf4jRetrofitLogger;
 import lombok.Setter;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -68,6 +69,7 @@ Front50Api front50Api() {
             new Ok3Client(
                 clientProvider.getClient(new DefaultServiceEndpoint("front50", front50Endpoint))))
         .setConverter(new JacksonConverter(objectMapper))
+        .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
         .setLogLevel(retrofitLogLevel)
         .setLog(new Slf4jRetrofitLogger(Front50Api.class))
         .build()
@@ -102,6 +104,7 @@ ClouddriverApi clouddriverApi() {
                 clientProvider.getClient(
                     new DefaultServiceEndpoint("clouddriver", clouddriverEndpoint))))
         .setConverter(new JacksonConverter(objectMapper))
+        .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
         .setLogLevel(retrofitLogLevel)
         .setLog(new Slf4jRetrofitLogger(ClouddriverApi.class))
         .build()
@@ -157,6 +160,7 @@ IgorApi igorApi(@Value("${services.igor.base-url}") String igorEndpoint) {
             new Ok3Client(
                 clientProvider.getClient(new DefaultServiceEndpoint("igor", igorEndpoint))))
         .setConverter(new JacksonConverter(objectMapper))
+        .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance())
         .setLogLevel(retrofitLogLevel)
         .setLog(new Slf4jRetrofitLogger(IgorApi.class))
         .build()