refactor(retrofit2): refactor the code to align with the retrofit2 up…

…grade of fiat-api
spinnaker · Dec 6, 2024 · bbac536 · bbac536
1 parent 2ab0d90
commit bbac536
Show file tree

Hide file tree

Showing 6 changed files with 35 additions and 9 deletions.
diff --git a/front50-core/src/main/java/com/netflix/spinnaker/front50/ApplicationPermissionsService.java b/front50-core/src/main/java/com/netflix/spinnaker/front50/ApplicationPermissionsService.java
@@ -25,6 +25,7 @@
 import com.netflix.spinnaker.front50.model.application.ApplicationDAO;
 import com.netflix.spinnaker.front50.model.application.ApplicationPermissionDAO;
 import com.netflix.spinnaker.kork.exceptions.SystemException;
+import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall;
 import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
 import com.netflix.spinnaker.kork.web.exceptions.NotFoundException;
 import java.util.AbstractMap.SimpleEntry;
@@ -182,7 +183,7 @@ private void syncUsers(Permission newPermission, Permission oldPermission) {
 
     if (fiatConfigurationProperties.getRoleSync().isEnabled()) {
       try {
-        fiatService.get().sync(new ArrayList<>(roles));
+        Retrofit2SyncCall.execute(fiatService.get().sync(new ArrayList<>(roles)));
       } catch (SpinnakerServerException e) {
         log.warn("Error syncing users", e);
       }

diff --git a/front50-core/src/main/java/com/netflix/spinnaker/front50/ServiceAccountsService.java b/front50-core/src/main/java/com/netflix/spinnaker/front50/ServiceAccountsService.java
@@ -23,6 +23,7 @@
 import com.netflix.spinnaker.front50.config.annotations.ConditionalOnAnyProviderExceptRedisIsEnabled;
 import com.netflix.spinnaker.front50.model.serviceaccount.ServiceAccount;
 import com.netflix.spinnaker.front50.model.serviceaccount.ServiceAccountDAO;
+import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall;
 import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException;
 import com.netflix.spinnaker.kork.web.exceptions.NotFoundException;
 import java.util.Collection;
@@ -86,7 +87,8 @@ public void deleteServiceAccounts(Collection<ServiceAccount> serviceAccountsToDe
         sa -> {
           try {
             serviceAccountDAO.delete(sa.getId());
-            fiatService.ifPresent(service -> service.logoutUser(sa.getId()));
+            fiatService.ifPresent(
+                service -> Retrofit2SyncCall.execute(service.logoutUser(sa.getId())));
           } catch (Exception e) {
             log.warn("Could not delete service account user {}", sa.getId(), e);
           }
@@ -129,7 +131,7 @@ private void syncUsers(Collection<ServiceAccount> serviceAccounts) {
               .flatMap(Collection::stream)
               .distinct()
               .collect(Collectors.toList());
-      fiatService.get().sync(rolesToSync);
+      Retrofit2SyncCall.execute(fiatService.get().sync(rolesToSync));
       log.debug("Synced users with roles: {}", rolesToSync);
       // Invalidate the current user's permissions in the local cache
       Authentication auth = SecurityContextHolder.getContext().getAuthentication();
@@ -149,7 +151,10 @@ private void syncServiceAccount(ServiceAccount serviceAccount) {
       return;
     }
     try {
-      fiatService.get().syncServiceAccount(serviceAccount.getId(), serviceAccount.getMemberOf());
+      Retrofit2SyncCall.execute(
+          fiatService
+              .get()
+              .syncServiceAccount(serviceAccount.getId(), serviceAccount.getMemberOf()));
       log.debug(
           "Synced service account {} with roles: {}",
           serviceAccount.getId(),

diff --git a/...re/src/test/groovy/com/netflix/spinnaker/front50/ApplicationPermissionsServiceSpec.groovy b/...re/src/test/groovy/com/netflix/spinnaker/front50/ApplicationPermissionsServiceSpec.groovy
@@ -23,6 +23,8 @@ import com.netflix.spinnaker.front50.config.FiatConfigurationProperties
 import com.netflix.spinnaker.front50.model.application.Application
 import com.netflix.spinnaker.front50.model.application.ApplicationDAO
 import com.netflix.spinnaker.front50.model.application.ApplicationPermissionDAO
+import retrofit2.Call
+import retrofit2.Response
 import spock.lang.Specification
 import spock.lang.Unroll
 
@@ -33,6 +35,7 @@ class ApplicationPermissionsServiceSpec extends Specification {
   def "test application creation will sync roles in fiat"(permission, expectedSyncedRoles) {
     given:
     def fiatService = Mock(FiatService)
+    def syncCall = Mock(Call)
     ApplicationPermissionsService subject = createSubject(
       fiatService,
       Mock(ApplicationPermissionDAO) {
@@ -44,7 +47,8 @@ class ApplicationPermissionsServiceSpec extends Specification {
     subject.createApplicationPermission(permission)
 
     then:
-    1 * fiatService.sync(expectedSyncedRoles)
+    1 * fiatService.sync(expectedSyncedRoles) >> syncCall
+    1 * syncCall.execute() >> Response.success(null)
 
     where:
     permission                                                  | expectedSyncedRoles

diff --git a/front50-core/src/test/groovy/com/netflix/spinnaker/front50/ServiceAccountsServiceSpec.groovy b/front50-core/src/test/groovy/com/netflix/spinnaker/front50/ServiceAccountsServiceSpec.groovy
@@ -26,12 +26,17 @@ import com.netflix.spinnaker.front50.model.serviceaccount.ServiceAccountDAO
 import org.springframework.security.core.Authentication
 import org.springframework.security.core.context.SecurityContext
 import org.springframework.security.core.context.SecurityContextHolder
+import retrofit2.Call
+import retrofit2.Response
 import spock.lang.Specification
 import spock.lang.Subject
 
 class ServiceAccountsServiceSpec extends Specification {
   ServiceAccountDAO serviceAccountDAO = Mock(ServiceAccountDAO)
   FiatService fiatService = Mock(FiatService)
+  Call syncCall = Mock(Call)
+  Call syncSaCall = Mock(Call)
+  Call logoutCall = Mock(Call)
   FiatClientConfigurationProperties fiatClientConfigurationProperties = Mock(FiatClientConfigurationProperties) {
     isEnabled() >> true
   }
@@ -67,13 +72,15 @@ class ServiceAccountsServiceSpec extends Specification {
     }
     SecurityContextHolder.setContext(securityContext)
     fiatConfigurationProperties.isDisableRoleSyncWhenSavingServiceAccounts() >> false
+
     when:
     serviceAccountDAO.create(serviceAccount.id, serviceAccount) >> serviceAccount
     service.createServiceAccount(serviceAccount)
 
     then:
     1 * fiatPermissionsEvaluator.invalidatePermission(_)
-    1 * fiatService.sync(["test-role"])
+    1 * fiatService.sync(["test-role"]) >> syncCall
+    1 * syncCall.execute() >> Response.success(null)
   }
 
   def "deleting multiple service account should call sync once"() {
@@ -92,13 +99,15 @@ class ServiceAccountsServiceSpec extends Specification {
         ]
       )]
     fiatConfigurationProperties.isDisableRoleSyncWhenSavingServiceAccounts() >> false
+
     when:
     service.deleteServiceAccounts(serviceAccounts)
 
     then:
     1 * serviceAccountDAO.delete("test-svc-acct-1")
     1 * serviceAccountDAO.delete("test-svc-acct-2")
-    1 * fiatService.sync(['test-role-1', 'test-role-2'])
+    1 * fiatService.sync(['test-role-1', 'test-role-2']) >> syncCall
+    1 * syncCall.execute() >> Response.success(null)
   }
 
   def "unknown managed service accounts should not throw exception"() {
@@ -118,6 +127,10 @@ class ServiceAccountsServiceSpec extends Specification {
     1 * serviceAccountDAO.findById(test1ServiceAccount.id) >> test1ServiceAccount
     1 * serviceAccountDAO.findById(test2ServiceAccount.id) >> { throw new NotFoundException(test2ServiceAccount.id) }
     1 * serviceAccountDAO.delete(test1ServiceAccount.id)
+    1 * fiatService.logoutUser(_) >> logoutCall
+    1 * logoutCall.execute() >> Response.success(null)
+    1 * fiatService.sync(_) >> syncCall
+    1 * syncCall.execute() >> Response.success(1L)
     0 * serviceAccountDAO.delete(test2ServiceAccount.id)
   }
 
@@ -144,7 +157,8 @@ class ServiceAccountsServiceSpec extends Specification {
 
     then:
     1 * fiatPermissionsEvaluator.invalidatePermission(_)
-    1 * fiatService.syncServiceAccount("test-svc-acct", ["test-role"])
+    1 * fiatService.syncServiceAccount("test-svc-acct", ["test-role"]) >> syncSaCall
+    1 * syncSaCall.execute() >> Response.success(1L)
     0 * fiatService.sync(["test-role"])
   }
 }
diff --git a/front50-web/front50-web.gradle b/front50-web/front50-web.gradle
@@ -32,6 +32,7 @@ dependencies {
   implementation "io.spinnaker.fiat:fiat-core:$fiatVersion"
   implementation "io.spinnaker.kork:kork-artifacts"
   implementation "io.spinnaker.kork:kork-config"
+  implementation "io.spinnaker.kork:kork-retrofit"
   implementation "io.spinnaker.kork:kork-web"
   implementation "io.spinnaker.kork:kork-exceptions"
   implementation "com.squareup.retrofit:converter-jackson"

diff --git a/...eb/src/main/java/com/netflix/spinnaker/front50/controllers/v2/ApplicationsController.java b/...eb/src/main/java/com/netflix/spinnaker/front50/controllers/v2/ApplicationsController.java
@@ -12,6 +12,7 @@
 import com.netflix.spinnaker.front50.model.application.ApplicationDAO;
 import com.netflix.spinnaker.front50.model.application.ApplicationPermissionDAO;
 import com.netflix.spinnaker.front50.model.application.ApplicationService;
+import com.netflix.spinnaker.kork.retrofit.Retrofit2SyncCall;
 import com.netflix.spinnaker.kork.web.exceptions.NotFoundException;
 import io.swagger.v3.oas.annotations.Operation;
 import io.swagger.v3.oas.annotations.tags.Tag;
@@ -125,7 +126,7 @@ public Application create(@RequestBody final Application app) {
         && fiatConfigurationProperties.getRoleSync().isEnabled()
         && fiatService.isPresent()) {
       try {
-        fiatService.get().sync();
+        Retrofit2SyncCall.execute(fiatService.get().sync());
       } catch (Exception e) {
         log.warn("failed to trigger fiat permission sync", e);
       }