Skip to content

Commit

Permalink
feat(perf): cache the unrestricted users permissions (#751)
Browse files Browse the repository at this point in the history 
The unrestricted user is loaded from redis each time a lookup for a user happens, and
this user may end up with a lot of data associated with it if there are a large number
of secured entities set up for anonymous read access.

This saves the step of fetching and deserializing that data if there is a copy cached.

Adds a new last modified key to track (via redis server time) the last time the
unrestricted user was written to redis, and uses that to ensure a reload happens when
the permissions are rebuilt.
  • Loading branch information
@cfieber
cfieber committed Aug 25, 2020
1 parent edea932 commit 61f1e36
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 32 deletions.
1 change: 1 addition & 0 deletions fiat-roles/fiat-roles.gradle
View file
Expand Up @@ -17,6 +17,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.okhttp:okhttp"
Expand Down
80 changes: 63 additions & 17 deletions ...oles/src/main/java/com/netflix/spinnaker/fiat/permissions/RedisPermissionsRepository.java
View file
Expand Up @@ -19,6 +19,8 @@
import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.util.ArrayIterator;
import com.github.benmanes.caffeine.cache.Caffeine;
import com.github.benmanes.caffeine.cache.LoadingCache;
import com.google.common.collect.ArrayTable;
import com.google.common.collect.HashBasedTable;
import com.google.common.collect.Lists;
Expand All @@ -29,11 +31,13 @@
import com.netflix.spinnaker.fiat.model.resources.ResourceType;
import com.netflix.spinnaker.fiat.model.resources.Role;
import com.netflix.spinnaker.kork.jedis.RedisClientDelegate;
import java.time.Duration;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.NoSuchElementException;
import java.util.Optional;
import java.util.Set;
import java.util.UUID;
Expand Down Expand Up @@ -71,12 +75,18 @@ public class RedisPermissionsRepository implements PermissionsRepository {
private static final String KEY_ROLES = "roles";
private static final String KEY_ALL_USERS = "users";
private static final String KEY_ADMIN = "admin";
private static final String KEY_LAST_MODIFIED = "last_modified";

private static final String UNRESTRICTED = UnrestrictedResourceConfig.UNRESTRICTED_USERNAME;
private static final String NO_LAST_MODIFIED = "unknown_last_modified";

private final ObjectMapper objectMapper;
private final RedisClientDelegate redisClientDelegate;
private final List<Resource> resources;
private final LoadingCache<String, UserPermission> unrestrictedPermission =
Caffeine.newBuilder()
.expireAfterAccess(Duration.ofSeconds(10))
.build(k -> reloadUnrestricted());

private final String prefix;

Expand All @@ -92,6 +102,22 @@ public RedisPermissionsRepository(
this.resources = resources;
}

private UserPermission reloadUnrestricted() {
return getFromRedis(UNRESTRICTED).orElseThrow(NoSuchElementException::new);
}

private UserPermission getUnrestrictedUserPermission() {
String serverLastModified =
redisClientDelegate.withCommandsClient(
c -> {
return c.get(unrestrictedLastModifiedKey());
});
if (serverLastModified == null) {
serverLastModified = NO_LAST_MODIFIED;
}
return unrestrictedPermission.get(serverLastModified);
}

@Override
public RedisPermissionsRepository put(@NonNull UserPermission permission) {
val resourceTypes =
Expand Down Expand Up @@ -126,6 +152,7 @@ public RedisPermissionsRepository put(@NonNull UserPermission permission) {
.collect(Collectors.toSet());
});

AtomicReference<Response<List<String>>> serverTime = new AtomicReference<>();
redisClientDelegate.withMultiKeyPipeline(
pipeline -> {
String userId = permission.getId();
Expand Down Expand Up @@ -156,8 +183,18 @@ public RedisPermissionsRepository put(@NonNull UserPermission permission) {
pipeline.del(userResourceKey);
}
});

serverTime.set(pipeline.time());

pipeline.sync();
});
if (UNRESTRICTED.equals(permission.getId())) {
String lastModified = serverTime.get().get().get(0);
redisClientDelegate.withCommandsClient(
c -> {
c.set(unrestrictedLastModifiedKey(), lastModified);
});
}
} catch (Exception e) {
log.error("Storage exception writing " + permission.getId() + " entry.", e);
}
Expand All @@ -166,43 +203,44 @@ public RedisPermissionsRepository put(@NonNull UserPermission permission) {

@Override
public Optional<UserPermission> get(@NonNull String id) {
if (UNRESTRICTED.equals(id)) {
return Optional.of(getUnrestrictedUserPermission());
}
return getFromRedis(id);
}

private Optional<UserPermission> getFromRedis(@NonNull String id) {
try {
boolean userExists =
redisClientDelegate.withCommandsClient(
c -> {
return c.sismember(allUsersKey(), id);
});
UNRESTRICTED.equals(id)
|| redisClientDelegate.withCommandsClient(
c -> {
return c.sismember(allUsersKey(), id);
});
if (!userExists) {
return Optional.empty();
}
final RawUserPermission userResponseMap = new RawUserPermission();
final RawUserPermission unrestrictedResponseMap = new RawUserPermission();
redisClientDelegate.withMultiKeyPipeline(
p -> {
resources.stream()
.map(Resource::getResourceType)
.forEach(
r -> {
String userKey = userKey(id, r);
String unrestrictedUserKey = unrestrictedUserKey(r);
Response<Map<String, String>> resourceMap = p.hgetAll(userKey);
userResponseMap.put(r, resourceMap);
if (userKey.equals(unrestrictedUserKey)) {
unrestrictedResponseMap.put(r, resourceMap);
} else {
Response<Map<String, String>> unrestrictedMap =
p.hgetAll(unrestrictedUserKey);
unrestrictedResponseMap.put(r, unrestrictedMap);
}
log.info("Resource: {}; map size: {}", r, unrestrictedResponseMap.size());
});
Response<Boolean> admin = p.sismember(adminKey(), id);
p.sync();
userResponseMap.isAdmin = admin.get();
});

UserPermission unrestrictedUser = getUserPermission(UNRESTRICTED, unrestrictedResponseMap);
return Optional.of(getUserPermission(id, userResponseMap).merge(unrestrictedUser));
UserPermission userPermission = getUserPermission(id, userResponseMap);
if (!UNRESTRICTED.equals(id)) {
userPermission.merge(getUnrestrictedUserPermission());
}
return Optional.of(userPermission);
} catch (Exception e) {
log.error("Storage exception reading " + id + " entry.", e);
}
Expand Down Expand Up @@ -236,7 +274,7 @@ public Map<String, UserPermission> getAllByRoles(List<String> anyRoles) {
if (anyRoles == null) {
return getAllById();
} else if (anyRoles.isEmpty()) {
val unrestricted = get(UNRESTRICTED);
val unrestricted = getFromRedis(UNRESTRICTED);
if (unrestricted.isPresent()) {
val map = new HashMap<String, UserPermission>();
map.put(UNRESTRICTED, unrestricted.get());
Expand Down Expand Up @@ -404,6 +442,14 @@ private String roleKey(String role) {
return String.format("%s:%s:%s", prefix, KEY_ROLES, role);
}

private String lastModifiedKey(String userId) {
return String.format("%s:%s:%s", prefix, KEY_LAST_MODIFIED, userId);
}

private String unrestrictedLastModifiedKey() {
return lastModifiedKey(UNRESTRICTED);
}

private Set<Resource> extractResources(ResourceType r, Map<String, String> resourceMap) {
val modelClazz =
resources.stream()
Expand Down
44 changes: 29 additions & 15 deletions .../test/groovy/com/netflix/spinnaker/fiat/permissions/RedisPermissionsRepositorySpec.groovy
View file
Expand Up @@ -76,6 +76,19 @@ class RedisPermissionsRepositorySpec extends Specification {
jedis.flushDB()
}

def "should set last modified for unrestricted user on save"() {
expect:
!jedis.sismember("unittests:users", UNRESTRICTED)
!jedis.exists("unittests:last_modified:__unrestricted_user__")

when:
repo.put(new UserPermission().setId(UNRESTRICTED))

then:
jedis.sismember("unittests:users", UNRESTRICTED)
jedis.exists("unittests:last_modified:__unrestricted_user__")
}

def "should put the specified permission in redis"() {
setup:
Account account1 = new Account().setName("account")
Expand Down Expand Up @@ -113,10 +126,10 @@ class RedisPermissionsRepositorySpec extends Specification {
jedis.sadd("unittests:roles:role1", "testUser")
jedis.hset("unittests:permissions:testUser:accounts",
"account",
'{"name":"account","requiredGroupMembership":[]}')
'{"name":"account","permissions":{}}')
jedis.hset("unittests:permissions:testUser:applications",
"app",
'{"name":"app","requiredGroupMembership":[]}')
'{"name":"app","permissions":{}}')
jedis.hset("unittests:permissions:testUser:service_accounts",
"serviceAccount",
'{"name":"serviceAccount"}')
Expand Down Expand Up @@ -145,10 +158,10 @@ class RedisPermissionsRepositorySpec extends Specification {
jedis.sadd("unittests:users", "testUser");
jedis.hset("unittests:permissions:testUser:accounts",
"account",
'{"name":"account","requiredGroupMembership":["abc"]}')
'{"name":"account","permissions":{"READ":["abc"]}}')
jedis.hset("unittests:permissions:testUser:applications",
"app",
'{"name":"app","requiredGroupMembership":["abc"]}')
'{"name":"app","permissions":{"READ":["abc"]}}')
jedis.hset("unittests:permissions:testUser:service_accounts",
"serviceAccount",
'{"name":"serviceAccount"}')
Expand All @@ -157,19 +170,19 @@ class RedisPermissionsRepositorySpec extends Specification {
def result = repo.get("testUser").get()

then:
def abcRead = new Permissions.Builder().add(Authorization.READ, "abc").build();
def expected = new UserPermission()
.setId("testUser")
.setAccounts([new Account().setName("account")
.setRequiredGroupMembership(["abc"])] as Set)
.setApplications([new Application().setName("app")
.setRequiredGroupMembership(["abc"])] as Set)
.setAccounts([new Account().setName("account").setPermissions(abcRead)] as Set)
.setApplications([new Application().setName("app").setPermissions(abcRead)] as Set)
.setServiceAccounts([new ServiceAccount().setName("serviceAccount")] as Set)
result == expected

when:
jedis.hset("unittests:permissions:__unrestricted_user__:accounts",
"account",
'{"name":"unrestrictedAccount","requiredGroupMembership":[]}')
'{"name":"unrestrictedAccount","permissions":{}}')
jedis.set("unittests:last_modified:__unrestricted_user__", "1")
result = repo.get("testUser").get()

then:
Expand All @@ -187,24 +200,25 @@ class RedisPermissionsRepositorySpec extends Specification {
ServiceAccount serviceAccount1 = new ServiceAccount().setName("serviceAccount1")
jedis.hset("unittests:permissions:testUser1:accounts",
"account1",
'{"name":"account1","requiredGroupMembership":[]}')
'{"name":"account1","permissions":{}}')
jedis.hset("unittests:permissions:testUser1:applications",
"app1",
'{"name":"app1","requiredGroupMembership":[]}')
'{"name":"app1","permissions":{}}')
jedis.hset("unittests:permissions:testUser1:service_accounts",
"serviceAccount1",
'{"name":"serviceAccount1"}')

and:
Account account2 = new Account().setName("account2").setRequiredGroupMembership(["abc"])
Application app2 = new Application().setName("app2").setRequiredGroupMembership(["abc"])
def abcRead = new Permissions.Builder().add(Authorization.READ, "abc").build()
Account account2 = new Account().setName("account2").setPermissions(abcRead)
Application app2 = new Application().setName("app2").setPermissions(abcRead)
ServiceAccount serviceAccount2 = new ServiceAccount().setName("serviceAccount2")
jedis.hset("unittests:permissions:testUser2:accounts",
"account2",
'{"name":"account2","requiredGroupMembership":["abc"]}')
'{"name":"account2","permissions":{"READ":["abc"]}}')
jedis.hset("unittests:permissions:testUser2:applications",
"app2",
'{"name":"app2","requiredGroupMembership":["abc"]}')
'{"name":"app2","permissions":{"READ":["abc"]}}')
jedis.hset("unittests:permissions:testUser2:service_accounts",
"serviceAccount2",
'{"name":"serviceAccount2"}')
Expand Down

0 comments on commit 61f1e36

Please sign in to comment.