Skip to content

Commit

Permalink
fix(credentials): Remove hystrix from CredentialsService (#518)
Browse files Browse the repository at this point in the history
There is no longer a remote call in the direct call path of
`CredentialsService.getAccounts()`, thus no need for hystrix.

The `CredentialsService` was originally getting called for every x.509
authentication request. The default hystrix configuration had placed
an artificial limit on the number of concurrent api requests that could
be served by `gate`.
  • Loading branch information
ajordens committed Mar 18, 2018
1 parent eb2120b commit 1f71a6b
Showing 1 changed file with 19 additions and 27 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,14 @@ package com.netflix.spinnaker.gate.services

import com.netflix.spinnaker.fiat.model.Authorization
import com.netflix.spinnaker.fiat.shared.FiatClientConfigurationProperties
import com.netflix.spinnaker.gate.services.commands.HystrixFactory
import com.netflix.spinnaker.gate.services.internal.ClouddriverService
import com.netflix.spinnaker.gate.services.internal.ClouddriverService.Account
import com.netflix.spinnaker.gate.services.internal.ClouddriverService.AccountDetails
import com.netflix.spinnaker.gate.services.internal.ClouddriverServiceSelector
import groovy.util.logging.Slf4j
import org.springframework.beans.factory.annotation.Autowired
import org.springframework.stereotype.Service

@Slf4j
@Service
class CredentialsService {
private static final String GROUP = "credentials"

@Autowired
AccountLookupService accountLookupService

Expand All @@ -50,30 +44,28 @@ class CredentialsService {
* Returns all account names that a user with the specified list of userRoles has access to.
*/
List<AccountDetails> getAccounts(Collection<String> userRoles) {
HystrixFactory.newListCommand(GROUP, "getAccounts") {
return accountLookupService.getAccounts().findAll { AccountDetails account ->
if (fiatConfig.enabled) {
return true // Returned list is filtered later.
}
return accountLookupService.getAccounts().findAll { AccountDetails account ->
if (fiatConfig.enabled) {
return true // Returned list is filtered later.
}

Set<String> permissions = []
//support migration from requiredGroupMemberships config to permissions config.
//prefer permissions.WRITE over requiredGroupMemberships if non-empty permissions present
if (account.permissions) {
if (account.requiredGroupMembership) {
log.warn("on Account $account.name: preferring permissions: $account.permissions over requiredGroupMemberships: $account.requiredGroupMembership for authz decision")
}
permissions.addAll(account.permissions.get(Authorization.WRITE.toString()).collect { it.toLowerCase() })
} else if (account.requiredGroupMembership) {
permissions.addAll(account.requiredGroupMembership*.toLowerCase())
} else {
return true // anonymous account.
Set<String> permissions = []
//support migration from requiredGroupMemberships config to permissions config.
//prefer permissions.WRITE over requiredGroupMemberships if non-empty permissions present
if (account.permissions) {
if (account.requiredGroupMembership) {
log.warn("on Account $account.name: preferring permissions: $account.permissions over requiredGroupMemberships: $account.requiredGroupMembership for authz decision")
}
permissions.addAll(account.permissions.get(Authorization.WRITE.toString()).collect { it.toLowerCase() })
} else if (account.requiredGroupMembership) {
permissions.addAll(account.requiredGroupMembership*.toLowerCase())
} else {
return true // anonymous account.
}

def userRolesLower = userRoles*.toLowerCase() as Set<String>
def userRolesLower = userRoles*.toLowerCase() as Set<String>

return userRolesLower.intersect(permissions) as Boolean
} ?: []
} execute()
return userRolesLower.intersect(permissions) as Boolean
} ?: []
}
}

0 comments on commit 1f71a6b

Please sign in to comment.