Skip to content

Commit

Permalink
chore(dependencies): Autobump fiatVersion (#1519)
Browse files Browse the repository at this point in the history
* chore(dependencies): Autobump fiatVersion

* refactor(retrofit2): refactor the code to align with the retrofit2 upgrade of fiat-api

* refactor(retrofit2): use retrofit-mock library instead of mocking Call.

---------

Co-authored-by: root <root@4f180b5207b5>
Co-authored-by: kirangodishala <kiran@opsmx.io>
  • Loading branch information
3 people authored Dec 10, 2024
1 parent 2ab0d90 commit 268236a
Show file tree
Hide file tree
Showing 8 changed files with 25 additions and 10 deletions.
1 change: 1 addition & 0 deletions front50-core/front50-core.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -47,5 +47,6 @@ dependencies {
// in front50-test to front50-common, but front50-test seems like a better place.
testImplementation project(":front50-test")
testImplementation "io.spinnaker.kork:kork-sql-test"
testImplementation "com.squareup.retrofit2:retrofit-mock"

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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();
Expand All @@ -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(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ 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.mock.Calls
import spock.lang.Specification
import spock.lang.Unroll

Expand All @@ -44,7 +45,7 @@ class ApplicationPermissionsServiceSpec extends Specification {
subject.createApplicationPermission(permission)

then:
1 * fiatService.sync(expectedSyncedRoles)
1 * fiatService.sync(expectedSyncedRoles) >> Calls.response(null)

where:
permission | expectedSyncedRoles
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ 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.mock.Calls
import spock.lang.Specification
import spock.lang.Subject

Expand Down Expand Up @@ -67,13 +68,14 @@ 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"]) >> Calls.response(null)
}

def "deleting multiple service account should call sync once"() {
Expand All @@ -92,13 +94,14 @@ 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']) >> Calls.response(null)
}

def "unknown managed service accounts should not throw exception"() {
Expand All @@ -118,6 +121,8 @@ 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(_) >> Calls.response(null)
1 * fiatService.sync(_) >> Calls.response(1L)
0 * serviceAccountDAO.delete(test2ServiceAccount.id)
}

Expand All @@ -144,7 +149,7 @@ class ServiceAccountsServiceSpec extends Specification {

then:
1 * fiatPermissionsEvaluator.invalidatePermission(_)
1 * fiatService.syncServiceAccount("test-svc-acct", ["test-role"])
1 * fiatService.syncServiceAccount("test-svc-acct", ["test-role"]) >> Calls.response(1L)
0 * fiatService.sync(["test-role"])
}
}
1 change: 1 addition & 0 deletions front50-web/front50-web.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand Down
2 changes: 1 addition & 1 deletion gradle.properties
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
fiatVersion=1.52.0
fiatVersion=1.53.0
includeProviders=azure,gcs,oracle,redis,s3,swift,sql
korkVersion=7.247.0
org.gradle.parallel=true
Expand Down

0 comments on commit 268236a

Please sign in to comment.