refactor(retrofit2): replace retrofit client with retrofit2 client (#…

…1195) * refactor(retrofit2): replace retrofit client with retrofit2 client * refactor(retrofit2): address review comments
spinnaker · Dec 10, 2024 · 0aa2c9b · 0aa2c9b
1 parent f364169
commit 0aa2c9b
Show file tree

Hide file tree

Showing 25 changed files with 278 additions and 263 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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<UserPermission.View> 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<Void> 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<Void> 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<Long> 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<String> roles);
+  Call<Long> sync(@Body List<String> 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<Long> syncServiceAccount(
       @Path("serviceAccountId") String serviceAccountId, @Body List<String> 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<Void> 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<String> roles);
+  Call<Void> loginWithRoles(@Path("userId") String userId, @Body Collection<String> roles);
 
   /**
    * @param userId The user being logged out
    * @return ignored.
    */
   @DELETE("/roles/{userId}")
-  Response logoutUser(@Path("userId") String userId);
+  Call<Void> logoutUser(@Path("userId") String userId);
 }