diff --git a/fiat-api/fiat-api.gradle b/fiat-api/fiat-api.gradle index 1c1b8f51b..2647e387f 100644 --- a/fiat-api/fiat-api.gradle +++ b/fiat-api/fiat-api.gradle @@ -29,9 +29,8 @@ dependencies { 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" + implementation "com.squareup.retrofit2:retrofit" + implementation "com.squareup.retrofit2:converter-jackson" implementation "org.apache.commons:commons-lang3" compileOnly "javax.servlet:javax.servlet-api" 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 c4d0a6cd0..b0a9aaf0e 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 @@ -20,18 +20,11 @@ import com.fasterxml.jackson.databind.DeserializationFeature; import com.fasterxml.jackson.databind.ObjectMapper; -import com.jakewharton.retrofit.Ok3Client; -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.config.OkHttp3ClientConfiguration; +import com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory; import com.netflix.spinnaker.kork.web.exceptions.ExceptionMessageDecorator; -import com.netflix.spinnaker.okhttp.SpinnakerRequestInterceptor; -import com.netflix.spinnaker.retrofit.Slf4jRetrofitLogger; -import lombok.Setter; import lombok.val; -import okhttp3.OkHttpClient; -import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.autoconfigure.condition.ConditionalOnMissingBean; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.boot.context.properties.EnableConfigurationProperties; @@ -46,9 +39,8 @@ import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter; import org.springframework.security.web.authentication.AnonymousAuthenticationFilter; import org.springframework.security.web.authentication.AuthenticationConverter; -import retrofit.Endpoints; -import retrofit.RestAdapter; -import retrofit.converter.JacksonConverter; +import retrofit2.Retrofit; +import retrofit2.converter.jackson.JacksonConverterFactory; @Import(ErrorConfiguration.class) @EnableWebSecurity @@ -58,33 +50,21 @@ @ComponentScan("com.netflix.spinnaker.fiat.shared") public class FiatAuthenticationConfig { - @Autowired(required = false) - @Setter - private RestAdapter.LogLevel retrofitLogLevel = RestAdapter.LogLevel.BASIC; - @Bean @ConditionalOnMissingBean(FiatService.class) // Allows for override public FiatService fiatService( FiatClientConfigurationProperties fiatConfigurationProperties, - SpinnakerRequestInterceptor interceptor, - OkHttpClientProvider okHttpClientProvider) { + OkHttp3ClientConfiguration okHttpClientConfig) { // New role providers break deserialization if this is not enabled. val objectMapper = new ObjectMapper(); objectMapper.enable(DeserializationFeature.READ_UNKNOWN_ENUM_VALUES_AS_NULL); objectMapper.disable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES); - OkHttpClient okHttpClient = - okHttpClientProvider.getClient( - new DefaultServiceEndpoint("fiat", fiatConfigurationProperties.getBaseUrl())); - - return new RestAdapter.Builder() - .setEndpoint(Endpoints.newFixedEndpoint(fiatConfigurationProperties.getBaseUrl())) - .setRequestInterceptor(interceptor) - .setClient(new Ok3Client(okHttpClient)) - .setConverter(new JacksonConverter(objectMapper)) - .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance()) - .setLogLevel(retrofitLogLevel) - .setLog(new Slf4jRetrofitLogger(FiatService.class)) + return new Retrofit.Builder() + .baseUrl(fiatConfigurationProperties.getBaseUrl()) + .client(okHttpClientConfig.createForRetrofit2().build()) + .addCallAdapterFactory(ErrorHandlingExecutorCallAdapterFactory.getInstance()) + .addConverterFactory(JacksonConverterFactory.create(objectMapper)) .build() .create(FiatService.class); } 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 0248e15ba..b94538427 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,6 +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.retrofit.Retrofit2SyncCall; import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException; import com.netflix.spinnaker.kork.telemetry.caffeine.CaffeineStatsCounter; import com.netflix.spinnaker.security.AccessControlled; @@ -185,7 +186,8 @@ public boolean canCreate(String resourceType, Object resource) { "determine whether " + username + " can create resource " + resource, () -> { try { - fiatService.canCreate(username, resourceType, resource); + Retrofit2SyncCall.execute( + fiatService.canCreate(username, resourceType, resource)); return true; } catch (SpinnakerHttpException e) { if (e.getResponseCode() == HttpStatus.NOT_FOUND.value()) { @@ -312,7 +314,9 @@ public UserPermission.View getPermission(String username) { try { return retryHandler.retry( "getUserPermission for " + loadUserName, - () -> fiatService.getUserPermission(loadUserName)); + () -> + Retrofit2SyncCall.execute( + fiatService.getUserPermission(loadUserName))); } catch (Exception e) { if (!fiatStatus.isLegacyFallbackEnabled()) { throw e; diff --git a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatService.java b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatService.java index 53d5ab886..4c9d7dfcb 100644 --- a/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatService.java +++ b/fiat-api/src/main/java/com/netflix/spinnaker/fiat/shared/FiatService.java @@ -19,13 +19,13 @@ import com.netflix.spinnaker.fiat.model.UserPermission; import java.util.Collection; import java.util.List; -import retrofit.client.Response; -import retrofit.http.Body; -import retrofit.http.DELETE; -import retrofit.http.GET; -import retrofit.http.POST; -import retrofit.http.PUT; -import retrofit.http.Path; +import retrofit2.Call; +import retrofit2.http.Body; +import retrofit2.http.DELETE; +import retrofit2.http.GET; +import retrofit2.http.POST; +import retrofit2.http.PUT; +import retrofit2.http.Path; public interface FiatService { @@ -34,7 +34,7 @@ public interface FiatService { * @return The full UserPermission of the user. */ @GET("/authorize/{userId}") - UserPermission.View getUserPermission(@Path("userId") String userId); + Call getUserPermission(@Path("userId") String userId); /** * @param userId The username of the user @@ -44,7 +44,7 @@ public interface FiatService { * @return True if the user has access to the specified resource. */ @GET("/authorize/{userId}/{resourceType}/{resourceName}/{authorization}") - Response hasAuthorization( + Call hasAuthorization( @Path("userId") String userId, @Path("resourceType") String resourceType, @Path("resourceName") String resourceName, @@ -59,7 +59,7 @@ Response hasAuthorization( * @param resource The resource to check */ @POST("/authorize/{userId}/{resourceType}/create") - Response canCreate( + Call canCreate( @Path("userId") String userId, @Path("resourceType") String resourceType, @Body Object resource); @@ -70,7 +70,7 @@ Response canCreate( * @return The number of non-anonymous users synced. */ @POST("/roles/sync") - long sync(); + Call sync(); /** * Use to update a subset of users. An empty list will update the anonymous/unrestricted user. @@ -79,7 +79,7 @@ Response canCreate( * @return The number of non-anonymous users synced. */ @POST("/roles/sync") - long sync(@Body List roles); + Call sync(@Body List roles); /** * Use to update a service account. As opposed to `sync`, this will not trigger a full sync for @@ -90,17 +90,15 @@ Response canCreate( * @return The number of non-anonymous users synced. */ @POST("/roles/sync/serviceAccount/{serviceAccountId}") - long syncServiceAccount( + Call syncServiceAccount( @Path("serviceAccountId") String serviceAccountId, @Body List roles); /** * @param userId The user being logged in - * @param ignored ignored. * @return ignored. */ @POST("/roles/{userId}") - Response loginUser( - @Path("userId") String userId, @Body String ignored /* retrofit requires this */); + Call loginUser(@Path("userId") String userId); /** * Used specifically for logins that contain the users roles/groups. @@ -110,12 +108,12 @@ Response loginUser( * @return ignored. */ @PUT("/roles/{userId}") - Response loginWithRoles(@Path("userId") String userId, @Body Collection roles); + Call loginWithRoles(@Path("userId") String userId, @Body Collection roles); /** * @param userId The user being logged out * @return ignored. */ @DELETE("/roles/{userId}") - Response logoutUser(@Path("userId") String userId); + Call logoutUser(@Path("userId") String userId); } 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 cf823a19a..4d7a20eb7 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 @@ -27,17 +27,20 @@ import com.netflix.spinnaker.fiat.model.resources.ResourceType import com.netflix.spinnaker.fiat.model.resources.Role import com.netflix.spinnaker.fiat.model.resources.ServiceAccount import com.netflix.spinnaker.kork.common.Header +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException import org.slf4j.MDC import org.springframework.security.core.context.SecurityContextHolder import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken -import retrofit.RetrofitError -import retrofit.client.Response +import okhttp3.MediaType +import okhttp3.ResponseBody +import retrofit2.Call; +import retrofit2.Response +import retrofit2.Retrofit +import retrofit2.converter.jackson.JacksonConverterFactory; import spock.lang.Shared import spock.lang.Subject import spock.lang.Unroll -import javax.servlet.http.HttpServletResponse - class FiatPermissionEvaluatorSpec extends FiatSharedSpecification { @Subject @@ -67,25 +70,29 @@ class FiatPermissionEvaluatorSpec extends FiatSharedSpecification { authorization) then: - 1 * fiatService.getUserPermission("testUser") >> new UserPermission.View(new UserPermission()) !result when: + fiatService = Mock(FiatService) { + getUserPermission("testUser") >> Mock(Call){ + execute() >> Response.success(new UserPermission( + applications: [ + new Application(name: "abc-def", + permissions: Permissions.factory([ + (Authorization.READ): ["testRole"] as Set + ])), + ], + roles: [new Role("testRole")] + ).getView() ) + } + } + evaluator = updateEvaluator(fiatService) result = evaluator.hasPermission(authentication, resource, resourceType.name, authorization) then: - 1 * fiatService.getUserPermission("testUser") >> new UserPermission( - applications: [ - new Application(name: "abc-def", - permissions: Permissions.factory([ - (Authorization.READ): ["testRole"] as Set - ])), - ], - roles: [new Role("testRole")] - ).getView() result where: @@ -105,15 +112,23 @@ class FiatPermissionEvaluatorSpec extends FiatSharedSpecification { .setAuthorizations([Authorization.READ] as Set)] as Set) upv.setServiceAccounts([new ServiceAccount.View().setName(svcAcct) .setMemberOf(["foo"])] as Set) - fiatService.getUserPermission("testUser") >> upv - SecurityContextHolder.getContext().setAuthentication(authentication) Resource resourceCanCreate = new Application().setName("app1") Resource resourceCannotCreate = new Application().setName("app2") - fiatService.canCreate("testUser", 'APPLICATION', resourceCanCreate) >> null // doesn't return anything in case of success - fiatService.canCreate("testUser", 'APPLICATION', resourceCannotCreate) >> { - throw RetrofitError.httpError("", new Response("", HttpServletResponse.SC_NOT_FOUND, "", [], null), null, null) + SpinnakerHttpException spinnakerHttpException = makeSpinnakerHttpException(404) + + FiatService fiatService = Mock(FiatService) { + getUserPermission("testUser") >> Mock(Call) { + execute() >> Response.success(upv) + } + canCreate("testUser", "APPLICATION", resourceCanCreate) >> Mock(Call) { + execute() >> Response.success(null) // doesn't return anything in case of success + } + canCreate("testUser", "APPLICATION", resourceCannotCreate) >> { + throw spinnakerHttpException + } } + evaluator = updateEvaluator(fiatService) expect: evaluator.hasPermission(authentication, resource, 'APPLICATION', 'READ') @@ -127,6 +142,7 @@ class FiatPermissionEvaluatorSpec extends FiatSharedSpecification { evaluator.hasPermission(authentication, svcAcct, 'SERVICE_ACCOUNT', 'WRITE') evaluator.canCreate('APPLICATION', resourceCanCreate) + !evaluator.canCreate('APPLICATION', resourceCannotCreate) } @@ -139,6 +155,11 @@ class FiatPermissionEvaluatorSpec extends FiatSharedSpecification { retryConfiguration.setRetryMultiplier(1.5) and: + FiatService fiatService = Mock(FiatService) { + getUserPermission("testUser") >> { + throw new IllegalStateException("something something something") + } + } FiatPermissionEvaluator evaluator = new FiatPermissionEvaluator( registry, fiatService, @@ -151,8 +172,6 @@ class FiatPermissionEvaluatorSpec extends FiatSharedSpecification { def view = evaluator.getPermission("testUser") then: - 2 * fiatService.getUserPermission("testUser") >> { throw new IllegalStateException("something something something")} - view == null } @@ -163,12 +182,18 @@ class FiatPermissionEvaluatorSpec extends FiatSharedSpecification { MDC.put(Header.ACCOUNTS.header, "account1,account2") when: + FiatService fiatService = Mock(FiatService) { + getUserPermission("testUser") >> { + throw new IllegalStateException("something something something") + } + } + + FiatPermissionEvaluator evaluator = updateEvaluator(fiatService) def permission = evaluator.getPermission("testUser") def hasPermission = evaluator.hasPermission(authentication, "my_application", "APPLICATION", "READ") then: 2 * fiatStatus.isLegacyFallbackEnabled() >> { return legacyFallbackEnabled } - 2 * fiatService.getUserPermission("testUser") >> { throw new IllegalStateException("something something something")} hasPermission == expectedToHavePermission permission?.name == expectedName @@ -183,21 +208,24 @@ class FiatPermissionEvaluatorSpec extends FiatSharedSpecification { @Unroll def "should deny access to an application that has an empty set of authorizations"() { when: + FiatService fiatService = Mock(FiatService) { + getUserPermission("testUser") >> Mock(Call){ + execute() >> Response.success(new UserPermission.View() + .setApplications( + [ + new Application.View() + .setName("my_application") + .setAuthorizations(authorizations as Set) + ] as Set + ) ) + } + } + + evaluator = updateEvaluator(fiatService) def hasReadPermission = evaluator.hasPermission(authentication, "my_application", "APPLICATION", "READ") def hasWritePermission = evaluator.hasPermission(authentication, "my_application", "APPLICATION", "WRITE") then: - 2 * fiatService.getUserPermission("testUser") >> { - return new UserPermission.View() - .setApplications( - [ - new Application.View() - .setName("my_application") - .setAuthorizations(authorizations as Set) - ] as Set - ) - } - !hasReadPermission !hasWritePermission @@ -208,15 +236,18 @@ class FiatPermissionEvaluatorSpec extends FiatSharedSpecification { @Unroll def "should allow access to unknown applications"() { when: + FiatService fiatService = Mock(FiatService) { + getUserPermission("testUser") >> Mock(Call){ + execute() >> Response.success(new UserPermission.View() + .setAllowAccessToUnknownApplications(allowAccessToUnknownApplications) + .setApplications(Collections.emptySet())) + } + } + + evaluator = updateEvaluator(fiatService) def hasPermission = evaluator.hasPermission(authentication, "my_application", "APPLICATION", "READ") then: - 1 * fiatService.getUserPermission("testUser") >> { - return new UserPermission.View() - .setAllowAccessToUnknownApplications(allowAccessToUnknownApplications) - .setApplications(Collections.emptySet()) - } - hasPermission == expectedToHavePermission where: @@ -228,12 +259,16 @@ class FiatPermissionEvaluatorSpec extends FiatSharedSpecification { @Unroll def "should allow an admin to access #resourceType"() { given: - fiatService.getUserPermission("testUser") >> { - return new UserPermission.View() - .setApplications(Collections.emptySet()) - .setAdmin(true) + FiatService fiatService = Mock(FiatService) { + getUserPermission("testUser") >> Mock(Call){ + execute() >> Response.success(new UserPermission.View() + .setApplications(Collections.emptySet()) + .setAdmin(true)) + } } + evaluator = updateEvaluator(fiatService) + expect: evaluator.hasPermission(authentication, "my_resource", resourceType, "READ") evaluator.hasPermission(authentication, "my_resource", resourceType, "WRITE") @@ -244,12 +279,16 @@ class FiatPermissionEvaluatorSpec extends FiatSharedSpecification { def "should support isAdmin check for a user"() { given: - 1 * fiatService.getUserPermission("testUser") >> { - return new UserPermission.View() - .setApplications(Collections.emptySet()) - .setAdmin(isAdmin) + FiatService fiatService = Mock(FiatService) { + getUserPermission("testUser") >> Mock(Call){ + execute() >> Response.success(new UserPermission.View() + .setApplications(Collections.emptySet()) + .setAdmin(isAdmin)) + } } + evaluator = updateEvaluator(fiatService) + expect: evaluator.isAdmin(authentication) == expectedIsAdmin @@ -269,14 +308,19 @@ class FiatPermissionEvaluatorSpec extends FiatSharedSpecification { } when: + FiatService fiatService = Mock(FiatService) { + getUserPermission("testUser") >> Mock(Call){ + execute() >> Response.success(new UserPermission.View() + .setExtensionResources([ + (MY_EXTENSION_RESOURCE): [extensionResource] as Set + ])) + } + } + + evaluator = updateEvaluator(fiatService) def hasPermission = evaluator.hasPermission(authentication, "extension_resource", resourceType, authorization) then: - 1 * fiatService.getUserPermission("testUser") >> new UserPermission.View() - .setExtensionResources([ - (MY_EXTENSION_RESOURCE): [extensionResource] as Set - ]) - hasPermission == expectedHasPermission where: @@ -287,21 +331,6 @@ class FiatPermissionEvaluatorSpec extends FiatSharedSpecification { "YOUR_EXTENSION_RESOURCE" | "WRITE" | false } - private static FiatClientConfigurationProperties buildConfigurationProperties() { - FiatClientConfigurationProperties configurationProperties = new FiatClientConfigurationProperties() - configurationProperties.enabled = true - configurationProperties.cache.maxEntries = 0 - configurationProperties.cache.expiresAfterWriteSeconds = 0 - configurationProperties.grantedAuthorities.enabled = true - return configurationProperties - } - - private static MY_EXTENSION_RESOURCE = new ResourceType("MY_EXTENSION_RESOURCE"); - private static class MyExtensionResourceView implements Authorizable { - String name - Set authorizations - } - def "should evaluate permissions for AccessControlled objects"() { given: def resource = new AccessControlledResource(new Permissions.Builder().add(Authorization.READ, 'test group').build()) @@ -323,4 +352,47 @@ class FiatPermissionEvaluatorSpec extends FiatSharedSpecification { 'WRITE' | false 'EXECUTE' | false } + + private static FiatClientConfigurationProperties buildConfigurationProperties() { + FiatClientConfigurationProperties configurationProperties = new FiatClientConfigurationProperties() + configurationProperties.enabled = true + configurationProperties.cache.maxEntries = 0 + configurationProperties.cache.expiresAfterWriteSeconds = 0 + configurationProperties.grantedAuthorities.enabled = true + return configurationProperties + } + + private static MY_EXTENSION_RESOURCE = new ResourceType("MY_EXTENSION_RESOURCE"); + private static class MyExtensionResourceView implements Authorizable { + String name + Set authorizations + } + + private FiatPermissionEvaluator updateEvaluator(FiatService updatedFiatService) { + new FiatPermissionEvaluator( + registry, + updatedFiatService, + buildConfigurationProperties(), + fiatStatus, + FiatPermissionEvaluator.RetryHandler.NOOP + ) + } + private static SpinnakerHttpException makeSpinnakerHttpException(int status) { + String url = "https://some-url"; + Response retrofit2Response = + Response.error( + status, + ResponseBody.create( + MediaType.parse("application/json"), + "{ \"message\": \"arbitrary message\" }")); + + Retrofit retrofit = + new Retrofit.Builder() + .baseUrl(url) + .addConverterFactory(JacksonConverterFactory.create()) + .build(); + + return new SpinnakerHttpException(retrofit2Response, retrofit); + } + } diff --git a/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatSharedSpecification.groovy b/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatSharedSpecification.groovy index 3891eb924..04f391628 100644 --- a/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatSharedSpecification.groovy +++ b/fiat-api/src/test/groovy/com/netflix/spinnaker/fiat/shared/FiatSharedSpecification.groovy @@ -18,10 +18,17 @@ package com.netflix.spinnaker.fiat.shared import com.netflix.spectator.api.NoopRegistry import com.netflix.spectator.api.Registry +import com.netflix.spinnaker.fiat.model.UserPermission +import retrofit2.Call +import retrofit2.Response import spock.lang.Specification abstract class FiatSharedSpecification extends Specification { - FiatService fiatService = Mock(FiatService) + FiatService fiatService = Mock(FiatService) { + getUserPermission(_ as String) >> Mock(Call) { + execute() >> Response.success(new UserPermission.View(new UserPermission())) + } + } Registry registry = new NoopRegistry() FiatStatus fiatStatus = Mock(FiatStatus) { _ * isEnabled() >> { return true } diff --git a/fiat-github/fiat-github.gradle b/fiat-github/fiat-github.gradle index 42d39ff36..bfd3f5bc7 100644 --- a/fiat-github/fiat-github.gradle +++ b/fiat-github/fiat-github.gradle @@ -5,9 +5,7 @@ dependencies { implementation "org.springframework.boot:spring-boot-starter-web" implementation "org.apache.commons:commons-lang3" implementation "com.github.ben-manes.caffeine:guava" - implementation "com.squareup.retrofit:retrofit" - implementation "com.squareup.retrofit:converter-jackson" - implementation "com.jakewharton.retrofit:retrofit1-okhttp3-client" + implementation "com.squareup.retrofit2:converter-jackson" 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 1ac76f6e2..9bd3db53f 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 @@ -1,23 +1,22 @@ package com.netflix.spinnaker.fiat.config; -import com.jakewharton.retrofit.Ok3Client; -import com.netflix.spinnaker.config.DefaultServiceEndpoint; -import com.netflix.spinnaker.config.okhttp3.OkHttpClientProvider; +import com.netflix.spinnaker.config.OkHttp3ClientConfiguration; 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 com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory; +import java.io.IOException; import lombok.Setter; import lombok.extern.slf4j.Slf4j; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; +import okhttp3.Interceptor; +import okhttp3.Request; +import okhttp3.Response; +import org.jetbrains.annotations.NotNull; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; -import retrofit.Endpoints; -import retrofit.RequestInterceptor; -import retrofit.RestAdapter; -import retrofit.converter.JacksonConverter; +import retrofit2.Retrofit; +import retrofit2.converter.jackson.JacksonConverterFactory; /** * Converts the list of GitHub Configuration properties a collection of clients to access the GitHub @@ -28,55 +27,33 @@ @Slf4j public class GitHubConfig { - @Autowired @Setter private RestAdapter.LogLevel retrofitLogLevel; - @Autowired @Setter private GitHubProperties gitHubProperties; @Bean - public GitHubClient gitHubClient(OkHttpClientProvider clientProvider) { + public GitHubClient gitHubClient(OkHttp3ClientConfiguration okHttpClientConfig) { BasicAuthRequestInterceptor interceptor = new BasicAuthRequestInterceptor().setAccessToken(gitHubProperties.getAccessToken()); - return new RestAdapter.Builder() - .setEndpoint(Endpoints.newFixedEndpoint(gitHubProperties.getBaseUrl())) - .setRequestInterceptor(interceptor) - .setClient( - new Ok3Client( - clientProvider.getClient( - new DefaultServiceEndpoint("github", gitHubProperties.getBaseUrl())))) - .setConverter(new JacksonConverter()) - .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance()) - .setLogLevel(retrofitLogLevel) - .setLog(new Slf4jRetrofitLogger(GitHubClient.class)) + return new Retrofit.Builder() + .baseUrl(gitHubProperties.getBaseUrl()) + .client(okHttpClientConfig.createForRetrofit2().addInterceptor(interceptor).build()) + .addCallAdapterFactory(ErrorHandlingExecutorCallAdapterFactory.getInstance()) + .addConverterFactory(JacksonConverterFactory.create()) .build() .create(GitHubClient.class); } - private static class Slf4jRetrofitLogger implements RestAdapter.Log { - private final Logger logger; - - Slf4jRetrofitLogger(Class type) { - this(LoggerFactory.getLogger(type)); - } - - Slf4jRetrofitLogger(Logger logger) { - this.logger = logger; - } - - @Override - public void log(String message) { - logger.info(message); - } - } - - private static class BasicAuthRequestInterceptor implements RequestInterceptor { + @Setter + private static class BasicAuthRequestInterceptor implements Interceptor { - @Setter private String accessToken; + private String accessToken; @Override - public void intercept(RequestFacade request) { + public @NotNull Response intercept(Chain chain) throws IOException { // See docs at https://developer.github.com/v3/#authentication - request.addHeader("Authorization", "token " + accessToken); + Request request = + chain.request().newBuilder().addHeader("Authorization", "token " + accessToken).build(); + return chain.proceed(request); } } } 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 04e47a7a8..8c452d979 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,7 @@ 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.Retrofit2SyncCall; import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException; import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException; import java.util.*; @@ -269,7 +270,9 @@ private List getTeamsInOrgPaginated(String organization, int page) { List teams = new ArrayList<>(); try { log.debug("Requesting page " + page + " of teams."); - teams = gitHubClient.getOrgTeams(organization, page, gitHubProperties.paginationValue); + teams = + Retrofit2SyncCall.execute( + gitHubClient.getOrgTeams(organization, page, gitHubProperties.paginationValue)); } catch (SpinnakerNetworkException e) { log.error(String.format("Could not find the server %s", gitHubProperties.getBaseUrl()), e); } catch (SpinnakerHttpException e) { @@ -288,7 +291,9 @@ private List getMembersInOrgPaginated(String organization, int page) { List members = new ArrayList<>(); try { log.debug("Requesting page " + page + " of members."); - members = gitHubClient.getOrgMembers(organization, page, gitHubProperties.paginationValue); + members = + Retrofit2SyncCall.execute( + gitHubClient.getOrgMembers(organization, page, gitHubProperties.paginationValue)); } catch (SpinnakerNetworkException e) { log.error(String.format("Could not find the server %s", gitHubProperties.getBaseUrl()), e); } catch (SpinnakerHttpException e) { @@ -308,8 +313,9 @@ private List getMembersInTeamPaginated(String organization, String teamS try { log.debug("Requesting page " + page + " of members team " + teamSlug + "."); members = - gitHubClient.getMembersOfTeam( - organization, teamSlug, page, gitHubProperties.paginationValue); + Retrofit2SyncCall.execute( + gitHubClient.getMembersOfTeam( + organization, teamSlug, page, gitHubProperties.paginationValue)); } catch (SpinnakerNetworkException e) { log.error(String.format("Could not find the server %s", gitHubProperties.getBaseUrl()), e); } catch (SpinnakerHttpException e) { diff --git a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/client/GitHubClient.java b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/client/GitHubClient.java index ce8ccfaba..3d3eed49f 100644 --- a/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/client/GitHubClient.java +++ b/fiat-github/src/main/java/com/netflix/spinnaker/fiat/roles/github/client/GitHubClient.java @@ -19,23 +19,24 @@ import com.netflix.spinnaker.fiat.roles.github.model.Member; import com.netflix.spinnaker.fiat.roles.github.model.Team; import java.util.List; -import retrofit.http.GET; -import retrofit.http.Path; -import retrofit.http.Query; +import retrofit2.Call; +import retrofit2.http.GET; +import retrofit2.http.Path; +import retrofit2.http.Query; /** Retrofit interface for interacting with a GitHub REST API. */ public interface GitHubClient { @GET("/orgs/{org}/teams") - List getOrgTeams( + Call> getOrgTeams( @Path("org") String org, @Query("page") int page, @Query("per_page") int paginationValue); @GET("/orgs/{org}/members") - List getOrgMembers( + Call> getOrgMembers( @Path("org") String org, @Query("page") int page, @Query("per_page") int paginationValue); @GET("/orgs/{org}/teams/{teamSlug}/members") - List getMembersOfTeam( + Call> getMembersOfTeam( @Path("org") String org, @Path("teamSlug") String teamSlug, @Query("page") int page, diff --git a/fiat-roles/fiat-roles.gradle b/fiat-roles/fiat-roles.gradle index 0138fba5f..79a83a3c3 100644 --- a/fiat-roles/fiat-roles.gradle +++ b/fiat-roles/fiat-roles.gradle @@ -18,8 +18,7 @@ dependencies { implementation project(":fiat-core") implementation "com.github.ben-manes.caffeine:caffeine" - implementation "com.squareup.retrofit:retrofit" - implementation "com.squareup.retrofit:converter-jackson" + implementation "com.squareup.retrofit2:retrofit" implementation "io.github.resilience4j:resilience4j-spring-boot2" implementation "org.springframework:spring-core" @@ -29,6 +28,7 @@ dependencies { implementation "org.lz4:lz4-java:1.7.1" implementation "io.spinnaker.kork:kork-jedis" + implementation "io.spinnaker.kork:kork-retrofit" implementation "io.spinnaker.kork:kork-security" implementation "io.spinnaker.kork:kork-telemetry" implementation "redis.clients:jedis" diff --git a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/ClouddriverAccountLoader.java b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/ClouddriverAccountLoader.java index 7e6e2eeff..b55214605 100644 --- a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/ClouddriverAccountLoader.java +++ b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/ClouddriverAccountLoader.java @@ -18,11 +18,12 @@ import com.netflix.spinnaker.fiat.model.resources.Account; import com.netflix.spinnaker.fiat.providers.ProviderHealthTracker; +import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall; public class ClouddriverAccountLoader extends ClouddriverDataLoader { public ClouddriverAccountLoader( ProviderHealthTracker healthTracker, ClouddriverApi clouddriverApi) { - super(healthTracker, clouddriverApi::getAccounts); + super(healthTracker, () -> Retrofit2SyncCall.execute(clouddriverApi.getAccounts())); } } diff --git a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/ClouddriverApi.java b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/ClouddriverApi.java index 654de2db2..cd0e86147 100644 --- a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/ClouddriverApi.java +++ b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/ClouddriverApi.java @@ -19,12 +19,13 @@ import com.netflix.spinnaker.fiat.model.resources.Account; import com.netflix.spinnaker.fiat.model.resources.Application; import java.util.List; -import retrofit.http.GET; +import retrofit2.Call; +import retrofit2.http.GET; public interface ClouddriverApi { @GET("/credentials") - List getAccounts(); + Call> getAccounts(); @GET("/applications?restricted=false&expand=false") - List getApplications(); + Call> getApplications(); } diff --git a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/ClouddriverApplicationLoader.java b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/ClouddriverApplicationLoader.java index d85e98385..aa577be3c 100644 --- a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/ClouddriverApplicationLoader.java +++ b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/ClouddriverApplicationLoader.java @@ -19,6 +19,7 @@ import com.netflix.spinnaker.fiat.config.ResourceProviderConfig.ApplicationProviderConfig; import com.netflix.spinnaker.fiat.model.resources.Application; import com.netflix.spinnaker.fiat.providers.ProviderHealthTracker; +import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall; import lombok.extern.slf4j.Slf4j; import org.springframework.scheduling.annotation.Scheduled; @@ -30,7 +31,7 @@ public ClouddriverApplicationLoader( ProviderHealthTracker healthTracker, ClouddriverApi clouddriverApi, ApplicationProviderConfig applicationProviderConfig) { - super(healthTracker, clouddriverApi::getApplications); + super(healthTracker, () -> Retrofit2SyncCall.execute(clouddriverApi.getApplications())); this.applicationProviderConfig = applicationProviderConfig; } diff --git a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/Front50Api.java b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/Front50Api.java index c814a63c6..3c41c1558 100644 --- a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/Front50Api.java +++ b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/Front50Api.java @@ -19,12 +19,13 @@ import com.netflix.spinnaker.fiat.model.resources.Application; import com.netflix.spinnaker.fiat.model.resources.ServiceAccount; import java.util.List; -import retrofit.http.GET; -import retrofit.http.Query; +import retrofit2.Call; +import retrofit2.http.GET; +import retrofit2.http.Query; public interface Front50Api { @GET("/permissions/applications") - List getAllApplicationPermissions(); + Call> getAllApplicationPermissions(); /** * @deprecated for fiat's usage this is always going to be called with restricted = false, use the @@ -32,11 +33,11 @@ public interface Front50Api { */ @GET("/v2/applications") @Deprecated - List getAllApplications(@Query("restricted") boolean restricted); + Call> getAllApplications(@Query("restricted") boolean restricted); @GET("/v2/applications?restricted=false") - List getAllApplications(); + Call> getAllApplications(); @GET("/serviceAccounts") - List getAllServiceAccounts(); + Call> getAllServiceAccounts(); } diff --git a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/Front50ApplicationLoader.java b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/Front50ApplicationLoader.java index 9b670f57b..71d216904 100644 --- a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/Front50ApplicationLoader.java +++ b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/Front50ApplicationLoader.java @@ -18,10 +18,11 @@ import com.netflix.spinnaker.fiat.model.resources.Application; import com.netflix.spinnaker.fiat.providers.ProviderHealthTracker; +import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall; public class Front50ApplicationLoader extends Front50DataLoader { public Front50ApplicationLoader(ProviderHealthTracker healthTracker, Front50Api front50Api) { - super(healthTracker, front50Api::getAllApplications); + super(healthTracker, () -> Retrofit2SyncCall.execute(front50Api.getAllApplications())); } } diff --git a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/Front50DataLoader.java b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/Front50DataLoader.java index 0aeb0f12c..e125d6cac 100644 --- a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/Front50DataLoader.java +++ b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/Front50DataLoader.java @@ -25,7 +25,7 @@ /** * This class makes and caches live calls to front50. In the event that front50 is unavailable, the - * cached data is returned in stead. Failed calls are logged with the front50 health tracker, which + * cached data is returned instead. Failed calls are logged with the front50 health tracker, which * will turn unhealthy after X number of failed cache refreshes. */ public class Front50DataLoader extends DataLoader { diff --git a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/Front50ServiceAccountLoader.java b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/Front50ServiceAccountLoader.java index a95c5ac05..577e5c5da 100644 --- a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/Front50ServiceAccountLoader.java +++ b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/Front50ServiceAccountLoader.java @@ -18,10 +18,11 @@ import com.netflix.spinnaker.fiat.model.resources.ServiceAccount; import com.netflix.spinnaker.fiat.providers.ProviderHealthTracker; +import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall; public class Front50ServiceAccountLoader extends Front50DataLoader { public Front50ServiceAccountLoader(ProviderHealthTracker healthTracker, Front50Api front50Api) { - super(healthTracker, front50Api::getAllServiceAccounts); + super(healthTracker, () -> Retrofit2SyncCall.execute(front50Api.getAllServiceAccounts())); } } diff --git a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/IgorApi.java b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/IgorApi.java index a9725ece1..8f3ea14c3 100644 --- a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/IgorApi.java +++ b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/IgorApi.java @@ -19,9 +19,10 @@ import com.netflix.spinnaker.fiat.model.resources.BuildService; import java.util.List; -import retrofit.http.GET; +import retrofit2.Call; +import retrofit2.http.GET; public interface IgorApi { @GET("/buildServices") - List getBuildMasters(); + Call> getBuildMasters(); } diff --git a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/IgorBuildServiceLoader.java b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/IgorBuildServiceLoader.java index 7b807bbac..e24e013ad 100644 --- a/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/IgorBuildServiceLoader.java +++ b/fiat-roles/src/main/java/com/netflix/spinnaker/fiat/providers/internal/IgorBuildServiceLoader.java @@ -18,6 +18,7 @@ import com.netflix.spinnaker.fiat.model.resources.BuildService; import com.netflix.spinnaker.fiat.providers.ProviderHealthTracker; +import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall; import io.github.resilience4j.circuitbreaker.annotation.CircuitBreaker; import io.github.resilience4j.retry.annotation.Retry; import java.util.List; @@ -25,12 +26,12 @@ /** * This class makes and caches live calls to igor. In the event that igor is unavailable, the cached - * data is returned in stead. Failed calls are logged with the igor health tracker, which will turn + * data is returned instead. Failed calls are logged with the igor health tracker, which will turn * unhealthy after X number of failed cache refreshes. */ public class IgorBuildServiceLoader extends DataLoader { public IgorBuildServiceLoader(ProviderHealthTracker healthTracker, IgorApi igorApi) { - super(healthTracker, igorApi::getBuildMasters); + super(healthTracker, () -> Retrofit2SyncCall.execute(igorApi.getBuildMasters())); } @Override diff --git a/fiat-roles/src/main/resources/resilience4j-defaults.properties b/fiat-roles/src/main/resources/resilience4j-defaults.properties index 663104af7..ab6bfe8d6 100644 --- a/fiat-roles/src/main/resources/resilience4j-defaults.properties +++ b/fiat-roles/src/main/resources/resilience4j-defaults.properties @@ -27,7 +27,7 @@ resilience4j.circuitbreaker.configs.default.automaticTransitionFromOpenToHalfOpe # Retry defaults resilience4j.retry.configs.default.maxRetryAttempts=3 resilience4j.retry.configs.default.waitDuration=50ms -resilience4j.retry.configs.default.retryExceptions=retrofit.RetrofitError,java.io.IOException +resilience4j.retry.configs.default.retryExceptions=com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException,java.io.IOException # # front50 diff --git a/fiat-web/fiat-web.gradle b/fiat-web/fiat-web.gradle index 0af581f93..971f0a985 100644 --- a/fiat-web/fiat-web.gradle +++ b/fiat-web/fiat-web.gradle @@ -8,9 +8,7 @@ configurations.all { } dependencies { - implementation "com.squareup.retrofit:retrofit" - implementation "com.squareup.retrofit:converter-jackson" - implementation "com.jakewharton.retrofit:retrofit1-okhttp3-client" + implementation "com.squareup.retrofit2:converter-jackson" implementation "com.google.guava:guava" implementation "org.springframework.boot:spring-boot-starter-actuator" 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 d10582a39..b098ae7a7 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 @@ -17,13 +17,11 @@ package com.netflix.spinnaker.fiat.config; import com.fasterxml.jackson.databind.ObjectMapper; -import com.jakewharton.retrofit.Ok3Client; -import com.netflix.spinnaker.config.DefaultServiceEndpoint; +import com.netflix.spinnaker.config.OkHttp3ClientConfiguration; 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 com.netflix.spinnaker.kork.retrofit.ErrorHandlingExecutorCallAdapterFactory; import lombok.Setter; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.beans.factory.annotation.Value; @@ -35,16 +33,13 @@ import org.springframework.context.annotation.Primary; import org.springframework.context.annotation.PropertySource; import org.springframework.context.annotation.Scope; -import retrofit.Endpoints; -import retrofit.RestAdapter; -import retrofit.converter.JacksonConverter; +import retrofit2.Retrofit; +import retrofit2.converter.jackson.JacksonConverterFactory; @Configuration @EnableConfigurationProperties(ProviderCacheConfig.class) @PropertySource("classpath:resilience4j-defaults.properties") public class ResourcesConfig { - @Autowired @Setter private RestAdapter.LogLevel retrofitLogLevel; - @Autowired @Setter private ObjectMapper objectMapper; @Autowired @Setter private OkHttpClientProvider clientProvider; @@ -62,16 +57,12 @@ public class ResourcesConfig { private String igorEndpoint; @Bean - Front50Api front50Api() { - return new RestAdapter.Builder() - .setEndpoint(Endpoints.newFixedEndpoint(front50Endpoint)) - .setClient( - new Ok3Client( - clientProvider.getClient(new DefaultServiceEndpoint("front50", front50Endpoint)))) - .setConverter(new JacksonConverter(objectMapper)) - .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance()) - .setLogLevel(retrofitLogLevel) - .setLog(new Slf4jRetrofitLogger(Front50Api.class)) + Front50Api front50Api(OkHttp3ClientConfiguration okHttpClientConfig) { + return new Retrofit.Builder() + .baseUrl(front50Endpoint) + .client(okHttpClientConfig.createForRetrofit2().build()) + .addCallAdapterFactory(ErrorHandlingExecutorCallAdapterFactory.getInstance()) + .addConverterFactory(JacksonConverterFactory.create(objectMapper)) .build() .create(Front50Api.class); } @@ -96,17 +87,12 @@ Front50Service front50Service( } @Bean - ClouddriverApi clouddriverApi() { - return new RestAdapter.Builder() - .setEndpoint(Endpoints.newFixedEndpoint(clouddriverEndpoint)) - .setClient( - new Ok3Client( - clientProvider.getClient( - new DefaultServiceEndpoint("clouddriver", clouddriverEndpoint)))) - .setConverter(new JacksonConverter(objectMapper)) - .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance()) - .setLogLevel(retrofitLogLevel) - .setLog(new Slf4jRetrofitLogger(ClouddriverApi.class)) + ClouddriverApi clouddriverApi(OkHttp3ClientConfiguration okHttpClientConfig) { + return new Retrofit.Builder() + .baseUrl(clouddriverEndpoint) + .client(okHttpClientConfig.createForRetrofit2().build()) + .addCallAdapterFactory(ErrorHandlingExecutorCallAdapterFactory.getInstance()) + .addConverterFactory(JacksonConverterFactory.create(objectMapper)) .build() .create(ClouddriverApi.class); } @@ -153,16 +139,14 @@ ClouddriverService clouddriverServiceWithoutApplicationLoader( @Bean @ConditionalOnProperty("services.igor.enabled") - IgorApi igorApi(@Value("${services.igor.base-url}") String igorEndpoint) { - return new RestAdapter.Builder() - .setEndpoint(Endpoints.newFixedEndpoint(igorEndpoint)) - .setClient( - new Ok3Client( - clientProvider.getClient(new DefaultServiceEndpoint("igor", igorEndpoint)))) - .setConverter(new JacksonConverter(objectMapper)) - .setErrorHandler(SpinnakerRetrofitErrorHandler.getInstance()) - .setLogLevel(retrofitLogLevel) - .setLog(new Slf4jRetrofitLogger(IgorApi.class)) + IgorApi igorApi( + @Value("${services.igor.base-url}") String igorEndpoint, + OkHttp3ClientConfiguration okHttpClientConfig) { + return new Retrofit.Builder() + .baseUrl(igorEndpoint) + .client(okHttpClientConfig.createForRetrofit2().build()) + .addCallAdapterFactory(ErrorHandlingExecutorCallAdapterFactory.getInstance()) + .addConverterFactory(JacksonConverterFactory.create(objectMapper)) .build() .create(IgorApi.class); } diff --git a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/RetrofitConfig.java b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/RetrofitConfig.java index 102195fce..0646fdd6f 100644 --- a/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/RetrofitConfig.java +++ b/fiat-web/src/main/java/com/netflix/spinnaker/fiat/config/RetrofitConfig.java @@ -35,24 +35,11 @@ import org.springframework.context.annotation.Primary; import org.springframework.util.backoff.BackOffExecution; import org.springframework.util.backoff.ExponentialBackOff; -import retrofit.RestAdapter; /** This package is placed in fiat-core in order to be shared by fiat-web and fiat-shared. */ @Configuration public class RetrofitConfig { - @Value("${ok-http-client.connection-pool.max-idle-connections:5}") - @Setter - private int maxIdleConnections; - - @Value("${ok-http-client.connection-pool.keep-alive-duration-ms:300000}") - @Setter - private int keepAliveDurationMs; - - @Value("${ok-http-client.retry-on-connection-failure:true}") - @Setter - private boolean retryOnConnectionFailure; - @Value("${ok-http-client.retries.max-elapsed-backoff-ms:5000}") @Setter private long maxElapsedBackoffMs; @@ -66,11 +53,6 @@ ObjectMapper objectMapper() { .setSerializationInclusion(JsonInclude.Include.NON_NULL); } - @Bean - RestAdapter.LogLevel retrofitLogLevel(@Value("${retrofit.log-level:BASIC}") String logLevel) { - return RestAdapter.LogLevel.valueOf(logLevel); - } - @Bean RetryingInterceptor retryingInterceptor() { return new RetryingInterceptor(maxElapsedBackoffMs); diff --git a/fiat-web/src/test/groovy/com/netflix/spinnaker/fiat/controllers/RolesControllerSpec.groovy b/fiat-web/src/test/groovy/com/netflix/spinnaker/fiat/controllers/RolesControllerSpec.groovy index 2c9154cfe..e9e03f37f 100644 --- a/fiat-web/src/test/groovy/com/netflix/spinnaker/fiat/controllers/RolesControllerSpec.groovy +++ b/fiat-web/src/test/groovy/com/netflix/spinnaker/fiat/controllers/RolesControllerSpec.groovy @@ -24,6 +24,8 @@ import com.netflix.spinnaker.fiat.providers.internal.ClouddriverAccountLoader import com.netflix.spinnaker.fiat.providers.internal.Front50ApplicationLoader import com.netflix.spinnaker.fiat.providers.internal.Front50ServiceAccountLoader import com.netflix.spinnaker.fiat.providers.internal.IgorBuildServiceLoader +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException +import okhttp3.Request import org.jooq.Configuration import org.jooq.DSLContext import org.jooq.TransactionalRunnable @@ -34,7 +36,6 @@ import org.springframework.test.web.servlet.setup.MockMvcBuilders import org.springframework.web.context.WebApplicationContext import redis.clients.jedis.Jedis import redis.clients.jedis.util.Pool -import retrofit.RetrofitError import spock.lang.Specification import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.* @@ -151,7 +152,7 @@ class RolesControllerSpec extends Specification { then: front50ApplicationLoader.getData() >> { - throw RetrofitError.networkError("test1", new IOException("test2")) + throw new SpinnakerNetworkException(new IOException("test1"), new Request.Builder().url("test2").build()) } when: