diff --git a/fiat-api/fiat-api.gradle b/fiat-api/fiat-api.gradle index c10714475..1c1b8f51b 100644 --- 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 index d9d1b652c..c4d0a6cd0 100644 --- 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 index 098ebec8f..46f1ab8db 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 @@ -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 index 3ab8d1a75..42d39ff36 100644 --- 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 index f78159c7e..1ac76f6e2 100644 --- 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/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProvider.java b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProvider.java index cc2c7a14b..04e47a7a8 100644 --- a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/GithubTeamsUserRolesProvider.java +++ b/fiat-github/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 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 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 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 index 8a5b27a7e..24c769b80 100644 --- 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 index ea8728a61..d10582a39 100644 --- 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()