honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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
This commit is contained in:
Jay Modi 2018-09-26 14:27:35 -06:00 committed by GitHub
parent 85e4ef3429
commit fcb60acc34
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 102 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java
View File

@ -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<String> 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<Set<String>> keyIter = roleCache.keys().iterator();
while (keyIter.hasNext()) {
Set<String> key = keyIter.next();
if (Sets.haveEmptyIntersection(key, roles) == false) {
keyIter.remove();
}
}
}
negativeLookupCache.removeAll(roles);
}
public void usageStats(ActionListener<Map<String, Object>> listener) {
final Map<String, Object> usage = new HashMap<>(2);
usage.put("file", fileRolesStore.usageStats());

45
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java
View File

@ -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<Runnable> listeners = new ArrayList<>();
private final List<Consumer<Set<String>>> listeners = new ArrayList<>();
private volatile Map<String, RoleDescriptor> 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<Set<String>> listener,
XPackLicenseState licenseState) throws IOException {
super(settings);
this.file = resolveFile(env);
@ -76,9 +80,10 @@ public class FileRolesStore extends AbstractComponent {
}
public Set<RoleDescriptor> roleDescriptors(Set<String> roleNames) {
final Map<String, RoleDescriptor> localPermissions = permissions;
Set<RoleDescriptor> 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<String, Object> usageStats() {
final Map<String, RoleDescriptor> localPermissions = permissions;
Map<String, Object> 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<Set<String>> 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<String> 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<String, RoleDescriptor> 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<String> changedOrMissingRoles = Sets.difference(previousPermissions.entrySet(), permissions.entrySet())
.stream()
.map(Map.Entry::getKey)
.collect(Collectors.toSet());
final Set<String> addedRoles = Sets.difference(permissions.keySet(), previousPermissions.keySet());
final Set<String> changedRoles = Collections.unmodifiableSet(Sets.union(changedOrMissingRoles, addedRoles));
listeners.forEach(c -> c.accept(changedRoles));
}
}
}

3
x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStoreTests.java
View File

@ -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<Role> future = new PlainActionFuture<>();

53
x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java
View File

@ -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<String> modifiedRoles = new HashSet<>();
FileRolesStore store = new FileRolesStore(settings, env, watcherService, roleSet -> {
modifiedRoles.addAll(roleSet);
latch.countDown();
}, new XPackLicenseState(Settings.EMPTY));
Set<RoleDescriptor> 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<String> 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<String> 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<String> 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();