From fcb60acc3481ce648e9c8fc810b00d8b3f5cb81b Mon Sep 17 00:00:00 2001 From: Jay Modi Date: Wed, 26 Sep 2018 14:27:35 -0600 Subject: [PATCH] Calculate changed roles on roles.yml reload (#33525) In order to optimize the use of the role cache, when the roles.yml file is reloaded we now calculate the names of removed, changed, and added roles so that they may be passed to any listeners. This allows a listener to selectively clear cache for only the roles that have been modified. The CompositeRolesStore has been adapted to do exactly that so that we limit the need to reload roles from sources such as the native roles stores or external role providers. See #33205 --- .../authz/store/CompositeRolesStore.java | 21 ++++++-- .../security/authz/store/FileRolesStore.java | 45 +++++++++++----- .../authz/store/CompositeRolesStoreTests.java | 3 +- .../authz/store/FileRolesStoreTests.java | 53 ++++++++++++++++++- 4 files changed, 102 insertions(+), 20 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java index beb2ca60fb2..7e1cc49e2c0 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java @@ -97,9 +97,7 @@ public class CompositeRolesStore extends AbstractComponent { ThreadContext threadContext, XPackLicenseState licenseState) { super(settings); this.fileRolesStore = fileRolesStore; - // invalidating all on a file based role update is heavy handed to say the least, but in general this should be infrequent so the - // impact isn't really worth the added complexity of only clearing the changed values - fileRolesStore.addListener(this::invalidateAll); + fileRolesStore.addListener(this::invalidate); this.nativeRolesStore = nativeRolesStore; this.reservedRolesStore = reservedRolesStore; this.privilegeStore = privilegeStore; @@ -356,6 +354,23 @@ public class CompositeRolesStore extends AbstractComponent { negativeLookupCache.remove(role); } + public void invalidate(Set roles) { + numInvalidation.incrementAndGet(); + + // the cache cannot be modified while doing this operation per the terms of the cache iterator + try (ReleasableLock ignored = writeLock.acquire()) { + Iterator> keyIter = roleCache.keys().iterator(); + while (keyIter.hasNext()) { + Set key = keyIter.next(); + if (Sets.haveEmptyIntersection(key, roles) == false) { + keyIter.remove(); + } + } + } + + negativeLookupCache.removeAll(roles); + } + public void usageStats(ActionListener> listener) { final Map usage = new HashMap<>(2); usage.put("file", fileRolesStore.usageStats()); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java index 59bc8042fba..868a7076b8b 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java @@ -12,6 +12,7 @@ import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentParser; @@ -34,13 +35,16 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.function.Consumer; import java.util.regex.Pattern; +import java.util.stream.Collectors; import static java.util.Collections.emptyMap; import static java.util.Collections.unmodifiableMap; @@ -52,16 +56,16 @@ public class FileRolesStore extends AbstractComponent { private final Path file; private final XPackLicenseState licenseState; - private final List listeners = new ArrayList<>(); + private final List>> listeners = new ArrayList<>(); private volatile Map permissions; public FileRolesStore(Settings settings, Environment env, ResourceWatcherService watcherService, XPackLicenseState licenseState) throws IOException { - this(settings, env, watcherService, () -> {}, licenseState); + this(settings, env, watcherService, null, licenseState); } - FileRolesStore(Settings settings, Environment env, ResourceWatcherService watcherService, Runnable listener, + FileRolesStore(Settings settings, Environment env, ResourceWatcherService watcherService, Consumer> listener, XPackLicenseState licenseState) throws IOException { super(settings); this.file = resolveFile(env); @@ -76,9 +80,10 @@ public class FileRolesStore extends AbstractComponent { } public Set roleDescriptors(Set roleNames) { + final Map localPermissions = permissions; Set descriptors = new HashSet<>(); roleNames.forEach((name) -> { - RoleDescriptor descriptor = permissions.get(name); + RoleDescriptor descriptor = localPermissions.get(name); if (descriptor != null) { descriptors.add(descriptor); } @@ -87,12 +92,13 @@ public class FileRolesStore extends AbstractComponent { } public Map usageStats() { + final Map localPermissions = permissions; Map usageStats = new HashMap<>(3); - usageStats.put("size", permissions.size()); + usageStats.put("size", localPermissions.size()); boolean dls = false; boolean fls = false; - for (RoleDescriptor descriptor : permissions.values()) { + for (RoleDescriptor descriptor : localPermissions.values()) { for (IndicesPrivileges indicesPrivileges : descriptor.getIndicesPrivileges()) { fls = fls || indicesPrivileges.getGrantedFields() != null || indicesPrivileges.getDeniedFields() != null; dls = dls || indicesPrivileges.getQuery() != null; @@ -107,10 +113,10 @@ public class FileRolesStore extends AbstractComponent { return usageStats; } - public void addListener(Runnable runnable) { - Objects.requireNonNull(runnable); + public void addListener(Consumer> consumer) { + Objects.requireNonNull(consumer); synchronized (this) { - listeners.add(runnable); + listeners.add(consumer); } } @@ -118,6 +124,11 @@ public class FileRolesStore extends AbstractComponent { return file; } + // package private for testing + Set getAllRoleNames() { + return permissions.keySet(); + } + public static Path resolveFile(Environment env) { return XPackPlugin.resolveConfigFile(env, "roles.yml"); } @@ -319,11 +330,13 @@ public class FileRolesStore extends AbstractComponent { } @Override - public void onFileChanged(Path file) { + public synchronized void onFileChanged(Path file) { if (file.equals(FileRolesStore.this.file)) { + final Map previousPermissions = permissions; try { permissions = parseFile(file, logger, settings, licenseState); - logger.info("updated roles (roles file [{}] {})", file.toAbsolutePath(), Files.exists(file) ? "changed" : "removed"); + logger.info("updated roles (roles file [{}] {})", file.toAbsolutePath(), + Files.exists(file) ? "changed" : "removed"); } catch (Exception e) { logger.error( (Supplier) () -> new ParameterizedMessage( @@ -331,9 +344,13 @@ public class FileRolesStore extends AbstractComponent { return; } - synchronized (FileRolesStore.this) { - listeners.forEach(Runnable::run); - } + final Set changedOrMissingRoles = Sets.difference(previousPermissions.entrySet(), permissions.entrySet()) + .stream() + .map(Map.Entry::getKey) + .collect(Collectors.toSet()); + final Set addedRoles = Sets.difference(permissions.keySet(), previousPermissions.keySet()); + final Set changedRoles = Collections.unmodifiableSet(Sets.union(changedOrMissingRoles, addedRoles)); + listeners.forEach(c -> c.accept(changedRoles)); } } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java index 0c2ab1ecc76..9f1490856d6 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java @@ -53,6 +53,7 @@ import java.util.Set; import java.util.concurrent.ExecutionException; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.BiConsumer; +import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Predicate; @@ -213,7 +214,7 @@ public class CompositeRolesStoreTests extends ESTestCase { new CompositeRolesStore(SECURITY_ENABLED_SETTINGS, fileRolesStore, nativeRolesStore, reservedRolesStore, mock(NativePrivilegeStore.class), Collections.emptyList(), new ThreadContext(SECURITY_ENABLED_SETTINGS), new XPackLicenseState(SECURITY_ENABLED_SETTINGS)); - verify(fileRolesStore).addListener(any(Runnable.class)); // adds a listener in ctor + verify(fileRolesStore).addListener(any(Consumer.class)); // adds a listener in ctor final String roleName = randomAlphaOfLengthBetween(1, 10); PlainActionFuture future = new PlainActionFuture<>(); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java index 5cb93b898ba..0763ff65ec5 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java @@ -37,6 +37,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardOpenOption; import java.util.Collections; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; @@ -319,8 +320,11 @@ public class FileRolesStoreTests extends ESTestCase { threadPool = new TestThreadPool("test"); watcherService = new ResourceWatcherService(settings, threadPool); final CountDownLatch latch = new CountDownLatch(1); - FileRolesStore store = new FileRolesStore(settings, env, watcherService, latch::countDown, - new XPackLicenseState(Settings.EMPTY)); + final Set modifiedRoles = new HashSet<>(); + FileRolesStore store = new FileRolesStore(settings, env, watcherService, roleSet -> { + modifiedRoles.addAll(roleSet); + latch.countDown(); + }, new XPackLicenseState(Settings.EMPTY)); Set descriptors = store.roleDescriptors(Collections.singleton("role1")); assertThat(descriptors, notNullValue()); @@ -344,6 +348,8 @@ public class FileRolesStoreTests extends ESTestCase { fail("Waited too long for the updated file to be picked up"); } + assertEquals(1, modifiedRoles.size()); + assertTrue(modifiedRoles.contains("role5")); final TransportRequest request = mock(TransportRequest.class); descriptors = store.roleDescriptors(Collections.singleton("role5")); assertThat(descriptors, notNullValue()); @@ -354,6 +360,49 @@ public class FileRolesStoreTests extends ESTestCase { assertThat(role.cluster().check("cluster:monitor/foo/bar", request), is(true)); assertThat(role.cluster().check("cluster:admin/foo/bar", request), is(false)); + // truncate to remove some + final Set truncatedFileRolesModified = new HashSet<>(); + final CountDownLatch truncateLatch = new CountDownLatch(1); + store = new FileRolesStore(settings, env, watcherService, roleSet -> { + truncatedFileRolesModified.addAll(roleSet); + truncateLatch.countDown(); + }, new XPackLicenseState(Settings.EMPTY)); + + final Set allRolesPreTruncate = store.getAllRoleNames(); + try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.TRUNCATE_EXISTING)) { + writer.append("role5:").append(System.lineSeparator()); + writer.append(" cluster:").append(System.lineSeparator()); + writer.append(" - 'MONITOR'"); + } + + truncateLatch.await(); + assertEquals(allRolesPreTruncate.size() - 1, truncatedFileRolesModified.size()); + assertTrue(allRolesPreTruncate.contains("role5")); + assertFalse(truncatedFileRolesModified.contains("role5")); + descriptors = store.roleDescriptors(Collections.singleton("role5")); + assertThat(descriptors, notNullValue()); + assertEquals(1, descriptors.size()); + + // modify + final Set modifiedFileRolesModified = new HashSet<>(); + final CountDownLatch modifyLatch = new CountDownLatch(1); + store = new FileRolesStore(settings, env, watcherService, roleSet -> { + modifiedFileRolesModified.addAll(roleSet); + modifyLatch.countDown(); + }, new XPackLicenseState(Settings.EMPTY)); + + try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.TRUNCATE_EXISTING)) { + writer.append("role5:").append(System.lineSeparator()); + writer.append(" cluster:").append(System.lineSeparator()); + writer.append(" - 'ALL'"); + } + + modifyLatch.await(); + assertEquals(1, modifiedFileRolesModified.size()); + assertTrue(modifiedFileRolesModified.contains("role5")); + descriptors = store.roleDescriptors(Collections.singleton("role5")); + assertThat(descriptors, notNullValue()); + assertEquals(1, descriptors.size()); } finally { if (watcherService != null) { watcherService.stop();