From 0d47fbf1a2b1a7fb5d9de3b797db2ead8581a9ed Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Thu, 14 Jul 2022 14:31:04 -0400 Subject: [PATCH 1/9] WIP: origin based updates. --- .../elasticsearch/indices/SystemIndices.java | 84 +++++++++++++++---- 1 file changed, 67 insertions(+), 17 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/indices/SystemIndices.java b/server/src/main/java/org/elasticsearch/indices/SystemIndices.java index e25d72345ca8d..4af6c175e069d 100644 --- a/server/src/main/java/org/elasticsearch/indices/SystemIndices.java +++ b/server/src/main/java/org/elasticsearch/indices/SystemIndices.java @@ -17,8 +17,10 @@ import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateResponse.ResetFeatureStateStatus; import org.elasticsearch.action.admin.indices.delete.DeleteIndexAction; import org.elasticsearch.action.admin.indices.delete.DeleteIndexRequest; +import org.elasticsearch.action.support.ListenableActionFuture; import org.elasticsearch.action.support.master.AcknowledgedResponse; import org.elasticsearch.client.internal.Client; +import org.elasticsearch.client.internal.OriginSettingClient; import org.elasticsearch.cluster.metadata.Metadata; import org.elasticsearch.cluster.service.ClusterService; import org.elasticsearch.common.Strings; @@ -33,6 +35,7 @@ import org.elasticsearch.plugins.SystemIndexPlugin; import org.elasticsearch.snapshots.SnapshotsService; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -44,6 +47,11 @@ import java.util.Map.Entry; import java.util.Optional; import java.util.Set; +import java.util.concurrent.ConcurrentLinkedDeque; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; +import java.util.function.Consumer; import java.util.function.Function; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -328,6 +336,10 @@ public ExecutorSelector getExecutorSelector() { * @return The matching {@link SystemIndexDescriptor} or {@code null} if no descriptor is found */ public @Nullable SystemIndexDescriptor findMatchingDescriptor(String name) { + return findMatchingDescriptor(indexDescriptors, name); + } + + @Nullable static SystemIndexDescriptor findMatchingDescriptor(SystemIndexDescriptor[] indexDescriptors, String name) { SystemIndexDescriptor matchingDescriptor = null; for (SystemIndexDescriptor systemIndexDescriptor : indexDescriptors) { if (systemIndexDescriptor.matchesIndexPattern(name)) { @@ -854,6 +866,29 @@ public MigrationCompletionHandler getPostMigrationFunction() { return postMigrationFunction; } + private static ListenableActionFuture cleanUpFeatureForIndices( + Client client, + String[] indexNames, + final Consumer errorListener + ) { + var actionListener = new ActionListener() { + @Override + public void onResponse(AcknowledgedResponse acknowledgedResponse) {} + + @Override + public void onFailure(Exception e) { + errorListener.accept(e); + } + }; + var actionFuture = new ListenableActionFuture(); + actionFuture.addListener(actionListener); + DeleteIndexRequest deleteIndexRequest = new DeleteIndexRequest(); + deleteIndexRequest.indices(indexNames); + client.execute(DeleteIndexAction.INSTANCE, deleteIndexRequest, actionFuture); + + return actionFuture; + } + /** * Clean up the state of a feature * @param indexDescriptors List of descriptors of a feature's system indices @@ -864,7 +899,7 @@ public MigrationCompletionHandler getPostMigrationFunction() { * @param listener A listener to return success or failure of cleanup */ public static void cleanUpFeature( - Collection indexDescriptors, + Collection indexDescriptors, Collection associatedIndexDescriptors, String name, ClusterService clusterService, @@ -872,31 +907,46 @@ public static void cleanUpFeature( ActionListener listener ) { Metadata metadata = clusterService.state().getMetadata(); + List> actionFutures = new ArrayList<>(); + var errors = new ConcurrentLinkedDeque(); - List allIndices = Stream.concat(indexDescriptors.stream(), associatedIndexDescriptors.stream()) + List associatedIndices = associatedIndexDescriptors.stream() .map(descriptor -> descriptor.getMatchingIndices(metadata)) .flatMap(List::stream) .toList(); - if (allIndices.isEmpty()) { - // if no actual indices match the pattern, we can stop here - listener.onResponse(ResetFeatureStateStatus.success(name)); - return; + if (associatedIndices.isEmpty() == false) { + actionFutures.add(cleanUpFeatureForIndices(client, associatedIndices.toArray(Strings.EMPTY_ARRAY), e -> errors.add(e))); } - DeleteIndexRequest deleteIndexRequest = new DeleteIndexRequest(); - deleteIndexRequest.indices(allIndices.toArray(Strings.EMPTY_ARRAY)); - client.execute(DeleteIndexAction.INSTANCE, deleteIndexRequest, new ActionListener<>() { - @Override - public void onResponse(AcknowledgedResponse acknowledgedResponse) { - listener.onResponse(ResetFeatureStateStatus.success(name)); - } + for (var indexDescriptor : indexDescriptors) { + List matchingIndices = indexDescriptor.getMatchingIndices(metadata); - @Override - public void onFailure(Exception e) { - listener.onResponse(ResetFeatureStateStatus.failure(name, e)); + final OriginSettingClient clientWithOrigin = new OriginSettingClient(client, indexDescriptor.getOrigin()); + actionFutures.add(cleanUpFeatureForIndices( + clientWithOrigin, + matchingIndices.toArray(Strings.EMPTY_ARRAY), + e -> errors.add(e)) + ); + } + + for (var future : actionFutures) { + try { + future.get(5, TimeUnit.SECONDS); + } catch (TimeoutException ignore) { + } catch (Exception e) { + throw new IllegalStateException("Unexpected error while waiting on action", e); } - }); + } + + if (errors.isEmpty()) { + listener.onResponse(ResetFeatureStateStatus.success(name)); + } else { + StringBuilder exceptions = new StringBuilder('['); + exceptions.append(errors.stream().map(Exception::getMessage).collect(Collectors.joining(", "))); + exceptions.append(']'); + listener.onResponse(ResetFeatureStateStatus.failure(name, new Exception(exceptions.toString()))); + } } // No-op pre-migration function to be used as the default in case none are provided. From 3322b6cf5c415100e66cb7c92254137e3976cdec Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 18 Jul 2022 11:59:10 -0400 Subject: [PATCH 2/9] Finish work for separate origin --- .../elasticsearch/indices/SystemIndices.java | 97 ++++++++++++------- 1 file changed, 62 insertions(+), 35 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/indices/SystemIndices.java b/server/src/main/java/org/elasticsearch/indices/SystemIndices.java index 4af6c175e069d..2b1149a0546b4 100644 --- a/server/src/main/java/org/elasticsearch/indices/SystemIndices.java +++ b/server/src/main/java/org/elasticsearch/indices/SystemIndices.java @@ -35,7 +35,6 @@ import org.elasticsearch.plugins.SystemIndexPlugin; import org.elasticsearch.snapshots.SnapshotsService; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -48,10 +47,7 @@ import java.util.Optional; import java.util.Set; import java.util.concurrent.ConcurrentLinkedDeque; -import java.util.concurrent.Future; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.TimeoutException; -import java.util.function.Consumer; +import java.util.concurrent.atomic.AtomicLong; import java.util.function.Function; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -339,7 +335,8 @@ public ExecutorSelector getExecutorSelector() { return findMatchingDescriptor(indexDescriptors, name); } - @Nullable static SystemIndexDescriptor findMatchingDescriptor(SystemIndexDescriptor[] indexDescriptors, String name) { + @Nullable + static SystemIndexDescriptor findMatchingDescriptor(SystemIndexDescriptor[] indexDescriptors, String name) { SystemIndexDescriptor matchingDescriptor = null; for (SystemIndexDescriptor systemIndexDescriptor : indexDescriptors) { if (systemIndexDescriptor.matchesIndexPattern(name)) { @@ -869,15 +866,17 @@ public MigrationCompletionHandler getPostMigrationFunction() { private static ListenableActionFuture cleanUpFeatureForIndices( Client client, String[] indexNames, - final Consumer errorListener + final ActionListener listener ) { var actionListener = new ActionListener() { @Override - public void onResponse(AcknowledgedResponse acknowledgedResponse) {} + public void onResponse(AcknowledgedResponse acknowledgedResponse) { + listener.onResponse(null); + } @Override public void onFailure(Exception e) { - errorListener.accept(e); + listener.onFailure(e); } }; var actionFuture = new ListenableActionFuture(); @@ -889,6 +888,16 @@ public void onFailure(Exception e) { return actionFuture; } + private static long countInnerTasks( + Metadata metadata, + Collection indexDescriptors, + List associatedIndices + ) { + return associatedIndices.size() + indexDescriptors.stream() + .filter(id -> id.getMatchingIndices(metadata).isEmpty() == false) + .count(); + } + /** * Clean up the state of a feature * @param indexDescriptors List of descriptors of a feature's system indices @@ -904,49 +913,67 @@ public static void cleanUpFeature( String name, ClusterService clusterService, Client client, - ActionListener listener + final ActionListener listener ) { Metadata metadata = clusterService.state().getMetadata(); - List> actionFutures = new ArrayList<>(); - var errors = new ConcurrentLinkedDeque(); List associatedIndices = associatedIndexDescriptors.stream() .map(descriptor -> descriptor.getMatchingIndices(metadata)) .flatMap(List::stream) .toList(); + final long taskCount = countInnerTasks(metadata, indexDescriptors, associatedIndices); + + // check if there's nothing to do and take an early out + if (taskCount == 0) { + listener.onResponse(ResetFeatureStateStatus.success(name)); + return; + } + + ActionListener taskListener = new ActionListener<>() { + AtomicLong tasksCounter = new AtomicLong(taskCount); + Collection errors = new ConcurrentLinkedDeque<>(); + + @Override + public void onResponse(Void resetFeatureStateStatus) { + taskCompleted(); + } + + @Override + public void onFailure(Exception e) { + errors.add(e); + taskCompleted(); + } + + private void taskCompleted() { + if (tasksCounter.decrementAndGet() == 0) { + if (errors.isEmpty()) { + listener.onResponse(ResetFeatureStateStatus.success(name)); + } else { + StringBuilder exceptions = new StringBuilder("["); + exceptions.append(errors.stream().map(Exception::getMessage).collect(Collectors.joining(", "))); + exceptions.append(']'); + listener.onResponse(ResetFeatureStateStatus.failure(name, new Exception(exceptions.toString()))); + } + } + } + }; + + // Send cleanup for the associated indices, they don't need special origin since they are not protected if (associatedIndices.isEmpty() == false) { - actionFutures.add(cleanUpFeatureForIndices(client, associatedIndices.toArray(Strings.EMPTY_ARRAY), e -> errors.add(e))); + cleanUpFeatureForIndices(client, associatedIndices.toArray(Strings.EMPTY_ARRAY), taskListener); } + // One descriptor at a time, create an originating client and clean up the feature for (var indexDescriptor : indexDescriptors) { List matchingIndices = indexDescriptor.getMatchingIndices(metadata); - final OriginSettingClient clientWithOrigin = new OriginSettingClient(client, indexDescriptor.getOrigin()); - actionFutures.add(cleanUpFeatureForIndices( - clientWithOrigin, - matchingIndices.toArray(Strings.EMPTY_ARRAY), - e -> errors.add(e)) - ); - } + if (matchingIndices.isEmpty() == false) { + final OriginSettingClient clientWithOrigin = new OriginSettingClient(client, indexDescriptor.getOrigin()); - for (var future : actionFutures) { - try { - future.get(5, TimeUnit.SECONDS); - } catch (TimeoutException ignore) { - } catch (Exception e) { - throw new IllegalStateException("Unexpected error while waiting on action", e); + cleanUpFeatureForIndices(clientWithOrigin, matchingIndices.toArray(Strings.EMPTY_ARRAY), taskListener); } } - - if (errors.isEmpty()) { - listener.onResponse(ResetFeatureStateStatus.success(name)); - } else { - StringBuilder exceptions = new StringBuilder('['); - exceptions.append(errors.stream().map(Exception::getMessage).collect(Collectors.joining(", "))); - exceptions.append(']'); - listener.onResponse(ResetFeatureStateStatus.failure(name, new Exception(exceptions.toString()))); - } } // No-op pre-migration function to be used as the default in case none are provided. From cc736b121aa2e7f870c92fe9f79590034b423eb5 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 18 Jul 2022 22:04:14 -0400 Subject: [PATCH 3/9] Add tests --- .../core/LocalStateCompositeXPackPlugin.java | 14 +++- .../SecurityFeatureResetTests.java | 82 +++++++++++++++++++ 2 files changed, 92 insertions(+), 4 deletions(-) create mode 100644 x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java index 40e6b3fe2bf6f..02f6b24d17db1 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/LocalStateCompositeXPackPlugin.java @@ -719,10 +719,16 @@ public void cleanUpFeature( List systemPlugins = filterPlugins(SystemIndexPlugin.class); GroupedActionListener allListeners = new GroupedActionListener<>( - ActionListener.wrap( - listenerResults -> finalListener.onResponse(ResetFeatureStateStatus.success(getFeatureName())), - finalListener::onFailure - ), + ActionListener.wrap(listenerResults -> { + // If the clean-up produced only one result, use that to pass along. In most + // cases it should be 1-1 mapping of feature to response. Passing back success + // prevents us from writing validation tests on this API. + if (listenerResults != null && listenerResults.size() == 1) { + finalListener.onResponse(listenerResults.stream().findFirst().get()); + } else { + finalListener.onResponse(ResetFeatureStateStatus.success(getFeatureName())); + } + }, finalListener::onFailure), systemPlugins.size() ); systemPlugins.forEach(plugin -> plugin.cleanUpFeature(clusterService, client, allListeners)); diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java new file mode 100644 index 0000000000000..95db8910babfe --- /dev/null +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java @@ -0,0 +1,82 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.integration; + +import org.elasticsearch.action.ActionListener; +import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateAction; +import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateRequest; +import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateResponse; +import org.elasticsearch.common.settings.SecureString; +import org.elasticsearch.test.SecurityIntegTestCase; +import org.elasticsearch.test.SecuritySettingsSourceField; + +import java.util.Collections; + +import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.BASIC_AUTH_HEADER; +import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue; + +public class SecurityFeatureResetTests extends SecurityIntegTestCase { + private static final SecureString SUPER_USER_PASSWD = SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING; + + @Override + protected String configUsers() { + final String usersPasswHashed = new String(getFastStoredHashAlgoForTests().hash(SUPER_USER_PASSWD)); + return super.configUsers() + "su:" + usersPasswHashed + "\n" + "manager:" + usersPasswHashed + "\n"; + + } + + @Override + protected String configUsersRoles() { + return super.configUsersRoles() + """ + superuser:su + role1:manager"""; + } + + @Override + protected String configRoles() { + return super.configRoles() + """ + %s + role1: + cluster: [ all ] + indices: + - names: '*' + privileges: [ manage ] + """; + } + + public void testFeatureResetSuperuser() { + assertCanReset("su", SUPER_USER_PASSWD); + } + + public void testFeatureResetManageRole() { + assertCanReset("manager", SUPER_USER_PASSWD); + } + + private void assertCanReset(String user, SecureString password) { + final ResetFeatureStateRequest req = new ResetFeatureStateRequest(); + + client().filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue(user, password))) + .admin() + .cluster() + .execute(ResetFeatureStateAction.INSTANCE, req, new ActionListener<>() { + @Override + public void onResponse(ResetFeatureStateResponse response) { + long failures = response.getFeatureStateResetStatuses() + .stream() + .filter(status -> status.getStatus() == ResetFeatureStateResponse.ResetFeatureStateStatus.Status.FAILURE) + .count(); + assertEquals(0, failures); + } + + @Override + public void onFailure(Exception e) { + fail("Shouldn't reach here"); + } + }); + } +} From a7c9011492b957e47121f00ee870c9d38e5ae8bc Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 18 Jul 2022 23:01:49 -0400 Subject: [PATCH 4/9] Refactor code --- .../elasticsearch/indices/SystemIndices.java | 79 ++++++++----------- .../SecurityFeatureResetTests.java | 55 ++++++++++--- .../test/NativeRealmIntegTestCase.java | 8 +- 3 files changed, 85 insertions(+), 57 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/indices/SystemIndices.java b/server/src/main/java/org/elasticsearch/indices/SystemIndices.java index 2b1149a0546b4..8c6037c5ac4c9 100644 --- a/server/src/main/java/org/elasticsearch/indices/SystemIndices.java +++ b/server/src/main/java/org/elasticsearch/indices/SystemIndices.java @@ -14,10 +14,11 @@ import org.apache.lucene.util.automaton.MinimizationOperations; import org.apache.lucene.util.automaton.Operations; import org.elasticsearch.action.ActionListener; +import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateResponse; import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateResponse.ResetFeatureStateStatus; import org.elasticsearch.action.admin.indices.delete.DeleteIndexAction; import org.elasticsearch.action.admin.indices.delete.DeleteIndexRequest; -import org.elasticsearch.action.support.ListenableActionFuture; +import org.elasticsearch.action.support.GroupedActionListener; import org.elasticsearch.action.support.master.AcknowledgedResponse; import org.elasticsearch.client.internal.Client; import org.elasticsearch.client.internal.OriginSettingClient; @@ -46,8 +47,6 @@ import java.util.Map.Entry; import java.util.Optional; import java.util.Set; -import java.util.concurrent.ConcurrentLinkedDeque; -import java.util.concurrent.atomic.AtomicLong; import java.util.function.Function; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -863,37 +862,33 @@ public MigrationCompletionHandler getPostMigrationFunction() { return postMigrationFunction; } - private static ListenableActionFuture cleanUpFeatureForIndices( + private static void cleanUpFeatureForIndices( + String name, Client client, String[] indexNames, - final ActionListener listener + final ActionListener listener ) { - var actionListener = new ActionListener() { + DeleteIndexRequest deleteIndexRequest = new DeleteIndexRequest(); + deleteIndexRequest.indices(indexNames); + client.execute(DeleteIndexAction.INSTANCE, deleteIndexRequest, new ActionListener<>() { @Override public void onResponse(AcknowledgedResponse acknowledgedResponse) { - listener.onResponse(null); + listener.onResponse(ResetFeatureStateStatus.success(name)); } @Override public void onFailure(Exception e) { - listener.onFailure(e); + listener.onResponse(ResetFeatureStateStatus.failure(name, e)); } - }; - var actionFuture = new ListenableActionFuture(); - actionFuture.addListener(actionListener); - DeleteIndexRequest deleteIndexRequest = new DeleteIndexRequest(); - deleteIndexRequest.indices(indexNames); - client.execute(DeleteIndexAction.INSTANCE, deleteIndexRequest, actionFuture); - - return actionFuture; + }); } - private static long countInnerTasks( + private static int countInnerTasks( Metadata metadata, Collection indexDescriptors, List associatedIndices ) { - return associatedIndices.size() + indexDescriptors.stream() + return associatedIndices.size() + (int)indexDescriptors.stream() .filter(id -> id.getMatchingIndices(metadata).isEmpty() == false) .count(); } @@ -922,7 +917,7 @@ public static void cleanUpFeature( .flatMap(List::stream) .toList(); - final long taskCount = countInnerTasks(metadata, indexDescriptors, associatedIndices); + final int taskCount = countInnerTasks(metadata, indexDescriptors, associatedIndices); // check if there's nothing to do and take an early out if (taskCount == 0) { @@ -930,38 +925,28 @@ public static void cleanUpFeature( return; } - ActionListener taskListener = new ActionListener<>() { - AtomicLong tasksCounter = new AtomicLong(taskCount); - Collection errors = new ConcurrentLinkedDeque<>(); - - @Override - public void onResponse(Void resetFeatureStateStatus) { - taskCompleted(); - } + GroupedActionListener groupedListener = new GroupedActionListener<>( + ActionListener.wrap(listenerResults -> { + List errors = listenerResults + .stream() + .filter(status -> status.getStatus() == ResetFeatureStateResponse.ResetFeatureStateStatus.Status.FAILURE) + .collect(Collectors.toList()); - @Override - public void onFailure(Exception e) { - errors.add(e); - taskCompleted(); - } - - private void taskCompleted() { - if (tasksCounter.decrementAndGet() == 0) { - if (errors.isEmpty()) { - listener.onResponse(ResetFeatureStateStatus.success(name)); - } else { - StringBuilder exceptions = new StringBuilder("["); - exceptions.append(errors.stream().map(Exception::getMessage).collect(Collectors.joining(", "))); - exceptions.append(']'); - listener.onResponse(ResetFeatureStateStatus.failure(name, new Exception(exceptions.toString()))); - } + if (errors.isEmpty()) { + listener.onResponse(ResetFeatureStateStatus.success(name)); + } else { + StringBuilder exceptions = new StringBuilder("["); + exceptions.append(errors.stream().map(e -> e.getException().getMessage()).collect(Collectors.joining(", "))); + exceptions.append(']'); + listener.onResponse(ResetFeatureStateStatus.failure(name, new Exception(exceptions.toString()))); } - } - }; + }, listener::onFailure), + taskCount + ); // Send cleanup for the associated indices, they don't need special origin since they are not protected if (associatedIndices.isEmpty() == false) { - cleanUpFeatureForIndices(client, associatedIndices.toArray(Strings.EMPTY_ARRAY), taskListener); + cleanUpFeatureForIndices(name, client, associatedIndices.toArray(Strings.EMPTY_ARRAY), groupedListener); } // One descriptor at a time, create an originating client and clean up the feature @@ -971,7 +956,7 @@ private void taskCompleted() { if (matchingIndices.isEmpty() == false) { final OriginSettingClient clientWithOrigin = new OriginSettingClient(client, indexDescriptor.getOrigin()); - cleanUpFeatureForIndices(clientWithOrigin, matchingIndices.toArray(Strings.EMPTY_ARRAY), taskListener); + cleanUpFeatureForIndices(name, clientWithOrigin, matchingIndices.toArray(Strings.EMPTY_ARRAY), groupedListener); } } } diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java index 95db8910babfe..b249ac7aa34f5 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java @@ -12,29 +12,57 @@ import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateRequest; import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateResponse; import org.elasticsearch.common.settings.SecureString; -import org.elasticsearch.test.SecurityIntegTestCase; +import org.elasticsearch.test.NativeRealmIntegTestCase; +import org.elasticsearch.test.SecuritySettingsSource; import org.elasticsearch.test.SecuritySettingsSourceField; +import org.elasticsearch.test.TestSecurityClient; +import org.elasticsearch.xpack.core.security.user.User; +import org.junit.Before; import java.util.Collections; import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.BASIC_AUTH_HEADER; import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue; -public class SecurityFeatureResetTests extends SecurityIntegTestCase { +public class SecurityFeatureResetTests extends NativeRealmIntegTestCase { private static final SecureString SUPER_USER_PASSWD = SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING; + @Override + public boolean shouldDeleteSecurityIndex() { + return false; + } + + @Before + public void setupForTests() throws Exception { + // adds a dummy user to the native realm to force .security index creation + new TestSecurityClient(getRestClient(), SecuritySettingsSource.SECURITY_REQUEST_OPTIONS).putUser( + new User("dummy_user", "missing_role"), + SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING + ); + assertSecurityIndexActive(); + } + @Override protected String configUsers() { final String usersPasswHashed = new String(getFastStoredHashAlgoForTests().hash(SUPER_USER_PASSWD)); - return super.configUsers() + "su:" + usersPasswHashed + "\n" + "manager:" + usersPasswHashed + "\n"; - + return super.configUsers() + + "su:" + + usersPasswHashed + + "\n" + + "manager:" + + usersPasswHashed + + "\n" + + "usr:" + + usersPasswHashed + + "\n"; } @Override protected String configUsersRoles() { return super.configUsersRoles() + """ superuser:su - role1:manager"""; + role1:manager + role2:usr"""; } @Override @@ -46,18 +74,27 @@ protected String configRoles() { indices: - names: '*' privileges: [ manage ] + role2: + cluster: [ all ] + indices: + - names: '*' + privileges: [ read ] """; } public void testFeatureResetSuperuser() { - assertCanReset("su", SUPER_USER_PASSWD); + assertReset("su", SUPER_USER_PASSWD, 0); } public void testFeatureResetManageRole() { - assertCanReset("manager", SUPER_USER_PASSWD); + assertReset("manager", SUPER_USER_PASSWD, 0); } - private void assertCanReset(String user, SecureString password) { + /*public void testFeatureResetUsrRole() { + assertReset("usr", SUPER_USER_PASSWD, 1); + }*/ + + private void assertReset(String user, SecureString password, final int numFailures) { final ResetFeatureStateRequest req = new ResetFeatureStateRequest(); client().filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue(user, password))) @@ -70,7 +107,7 @@ public void onResponse(ResetFeatureStateResponse response) { .stream() .filter(status -> status.getStatus() == ResetFeatureStateResponse.ResetFeatureStateStatus.Status.FAILURE) .count(); - assertEquals(0, failures); + assertEquals(numFailures, failures); } @Override diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/test/NativeRealmIntegTestCase.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/test/NativeRealmIntegTestCase.java index b7b5574368137..5841acd8a659e 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/test/NativeRealmIntegTestCase.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/test/NativeRealmIntegTestCase.java @@ -43,9 +43,15 @@ public void ensureNativeStoresStarted() throws Exception { } } + public boolean shouldDeleteSecurityIndex() { + return true; + } + @After public void stopESNativeStores() throws Exception { - deleteSecurityIndex(); + if (shouldDeleteSecurityIndex()) { + deleteSecurityIndex(); + } if (getCurrentClusterScope() == Scope.SUITE) { // Clear the realm cache for all realms since we use a SUITE scoped cluster From 7d33eecdbac5190d427895e56284d26f317f9043 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Tue, 19 Jul 2022 11:10:50 -0400 Subject: [PATCH 5/9] Fix roles in tests --- .../elasticsearch/indices/SystemIndices.java | 5 +- .../SecurityFeatureResetTests.java | 48 ++++++++++++++----- .../test/NativeRealmIntegTestCase.java | 8 +--- 3 files changed, 38 insertions(+), 23 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/indices/SystemIndices.java b/server/src/main/java/org/elasticsearch/indices/SystemIndices.java index 8c6037c5ac4c9..c9680c5d8ac7f 100644 --- a/server/src/main/java/org/elasticsearch/indices/SystemIndices.java +++ b/server/src/main/java/org/elasticsearch/indices/SystemIndices.java @@ -888,7 +888,7 @@ private static int countInnerTasks( Collection indexDescriptors, List associatedIndices ) { - return associatedIndices.size() + (int)indexDescriptors.stream() + return associatedIndices.size() + (int) indexDescriptors.stream() .filter(id -> id.getMatchingIndices(metadata).isEmpty() == false) .count(); } @@ -927,8 +927,7 @@ public static void cleanUpFeature( GroupedActionListener groupedListener = new GroupedActionListener<>( ActionListener.wrap(listenerResults -> { - List errors = listenerResults - .stream() + List errors = listenerResults.stream() .filter(status -> status.getStatus() == ResetFeatureStateResponse.ResetFeatureStateStatus.Status.FAILURE) .collect(Collectors.toList()); diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java index b249ac7aa34f5..e09cd0e45f0d6 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java @@ -12,7 +12,7 @@ import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateRequest; import org.elasticsearch.action.admin.cluster.snapshots.features.ResetFeatureStateResponse; import org.elasticsearch.common.settings.SecureString; -import org.elasticsearch.test.NativeRealmIntegTestCase; +import org.elasticsearch.test.SecurityIntegTestCase; import org.elasticsearch.test.SecuritySettingsSource; import org.elasticsearch.test.SecuritySettingsSourceField; import org.elasticsearch.test.TestSecurityClient; @@ -23,13 +23,14 @@ import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.BASIC_AUTH_HEADER; import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue; +import static org.hamcrest.Matchers.containsString; -public class SecurityFeatureResetTests extends NativeRealmIntegTestCase { +public class SecurityFeatureResetTests extends SecurityIntegTestCase { private static final SecureString SUPER_USER_PASSWD = SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING; @Override - public boolean shouldDeleteSecurityIndex() { - return false; + protected boolean addMockHttpTransport() { + return false; // enable http } @Before @@ -70,12 +71,12 @@ protected String configRoles() { return super.configRoles() + """ %s role1: - cluster: [ all ] + cluster: [ manage ] indices: - names: '*' privileges: [ manage ] role2: - cluster: [ all ] + cluster: [ monitor ] indices: - names: '*' privileges: [ read ] @@ -83,18 +84,39 @@ protected String configRoles() { } public void testFeatureResetSuperuser() { - assertReset("su", SUPER_USER_PASSWD, 0); + assertResetSuccessful("su", SUPER_USER_PASSWD); } public void testFeatureResetManageRole() { - assertReset("manager", SUPER_USER_PASSWD, 0); + assertResetSuccessful("manager", SUPER_USER_PASSWD); } - /*public void testFeatureResetUsrRole() { - assertReset("usr", SUPER_USER_PASSWD, 1); - }*/ + public void testFeatureResetNoManageRole() { + final ResetFeatureStateRequest req = new ResetFeatureStateRequest(); + + client().filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("usr", SUPER_USER_PASSWD))) + .admin() + .cluster() + .execute(ResetFeatureStateAction.INSTANCE, req, new ActionListener<>() { + @Override + public void onResponse(ResetFeatureStateResponse response) { + fail("Shouldn't reach here"); + } + + @Override + public void onFailure(Exception e) { + assertThat( + e.getMessage(), + containsString("action [cluster:admin/features/reset] is unauthorized for user [usr] with roles [role2]") + ); + } + }); + + // Manually delete the security index, reset shouldn't work + deleteSecurityIndex(); + } - private void assertReset(String user, SecureString password, final int numFailures) { + private void assertResetSuccessful(String user, SecureString password) { final ResetFeatureStateRequest req = new ResetFeatureStateRequest(); client().filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue(user, password))) @@ -107,7 +129,7 @@ public void onResponse(ResetFeatureStateResponse response) { .stream() .filter(status -> status.getStatus() == ResetFeatureStateResponse.ResetFeatureStateStatus.Status.FAILURE) .count(); - assertEquals(numFailures, failures); + assertEquals(0, failures); } @Override diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/test/NativeRealmIntegTestCase.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/test/NativeRealmIntegTestCase.java index 5841acd8a659e..b7b5574368137 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/test/NativeRealmIntegTestCase.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/test/NativeRealmIntegTestCase.java @@ -43,15 +43,9 @@ public void ensureNativeStoresStarted() throws Exception { } } - public boolean shouldDeleteSecurityIndex() { - return true; - } - @After public void stopESNativeStores() throws Exception { - if (shouldDeleteSecurityIndex()) { - deleteSecurityIndex(); - } + deleteSecurityIndex(); if (getCurrentClusterScope() == Scope.SUITE) { // Clear the realm cache for all realms since we use a SUITE scoped cluster From af577ea03475d0c52a43174d72f554e64e622a1f Mon Sep 17 00:00:00 2001 From: Nikola Grcevski <6207777+grcevski@users.noreply.github.com> Date: Tue, 19 Jul 2022 12:48:18 -0400 Subject: [PATCH 6/9] Update docs/changelog/88622.yaml --- docs/changelog/88622.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 docs/changelog/88622.yaml diff --git a/docs/changelog/88622.yaml b/docs/changelog/88622.yaml new file mode 100644 index 0000000000000..62c4080a69b01 --- /dev/null +++ b/docs/changelog/88622.yaml @@ -0,0 +1,6 @@ +pr: 88622 +summary: Use origin for the client when running _features/_reset +area: Infra/Core +type: bug +issues: + - 88617 From a313749f90d21f1e35daa1fe6b34fe7ece42cf1f Mon Sep 17 00:00:00 2001 From: Nikola Grcevski <6207777+grcevski@users.noreply.github.com> Date: Tue, 19 Jul 2022 20:03:54 -0400 Subject: [PATCH 7/9] Update x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java Co-authored-by: Gordon Brown --- .../elasticsearch/integration/SecurityFeatureResetTests.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java index e09cd0e45f0d6..cf877fa10b4fe 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java @@ -25,6 +25,11 @@ import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue; import static org.hamcrest.Matchers.containsString; +/** + * These tests ensure that the Feature Reset API works for users with default superuser and manage roles. + * This can be complex due to restrictions on system indices and the need to use the correct origin for + * each index. See also https://github.com/elastic/elasticsearch/issues/88617 + */ public class SecurityFeatureResetTests extends SecurityIntegTestCase { private static final SecureString SUPER_USER_PASSWD = SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING; From e5f6f9f66287fe8109a32f98c154c2557780679a Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Tue, 19 Jul 2022 20:13:44 -0400 Subject: [PATCH 8/9] Fix test origin --- .../system/indices/SystemIndicesQA.java | 7 +++--- .../elasticsearch/indices/SystemIndices.java | 23 +++++++++---------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/qa/system-indices/src/main/java/org/elasticsearch/system/indices/SystemIndicesQA.java b/qa/system-indices/src/main/java/org/elasticsearch/system/indices/SystemIndicesQA.java index 4fea260d49378..f55b350520106 100644 --- a/qa/system-indices/src/main/java/org/elasticsearch/system/indices/SystemIndicesQA.java +++ b/qa/system-indices/src/main/java/org/elasticsearch/system/indices/SystemIndicesQA.java @@ -38,6 +38,7 @@ import java.util.List; import java.util.function.Supplier; +import static org.elasticsearch.action.admin.cluster.node.tasks.get.GetTaskAction.TASKS_ORIGIN; import static org.elasticsearch.index.mapper.MapperService.SINGLE_MAPPING_NAME; import static org.elasticsearch.rest.RestRequest.Method.POST; import static org.elasticsearch.rest.RestRequest.Method.PUT; @@ -73,7 +74,7 @@ public Collection getSystemIndexDescriptors(Settings sett .put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-1") .build() ) - .setOrigin("net-new") + .setOrigin(TASKS_ORIGIN) .setVersionMetaKey("version") .setPrimaryIndex(".net-new-system-index-" + Version.CURRENT.major) .build(), @@ -81,7 +82,7 @@ public Collection getSystemIndexDescriptors(Settings sett .setIndexPattern(INTERNAL_UNMANAGED_INDEX_NAME) .setDescription("internal unmanaged system index") .setType(SystemIndexDescriptor.Type.INTERNAL_UNMANAGED) - .setOrigin("qa") + .setOrigin(TASKS_ORIGIN) .setAliasName(".internal-unmanaged-alias") .build(), SystemIndexDescriptor.builder() @@ -96,7 +97,7 @@ public Collection getSystemIndexDescriptors(Settings sett .put(IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "0-1") .build() ) - .setOrigin("qa") + .setOrigin(TASKS_ORIGIN) .setVersionMetaKey("version") .setPrimaryIndex(".internal-managed-index-" + Version.CURRENT.major) .setAliasName(".internal-managed-alias") diff --git a/server/src/main/java/org/elasticsearch/indices/SystemIndices.java b/server/src/main/java/org/elasticsearch/indices/SystemIndices.java index c9680c5d8ac7f..56c5be35ec908 100644 --- a/server/src/main/java/org/elasticsearch/indices/SystemIndices.java +++ b/server/src/main/java/org/elasticsearch/indices/SystemIndices.java @@ -8,6 +8,8 @@ package org.elasticsearch.indices; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.apache.lucene.util.automaton.Automata; import org.apache.lucene.util.automaton.Automaton; import org.apache.lucene.util.automaton.CharacterRunAutomaton; @@ -108,6 +110,8 @@ public class SystemIndices { private static final Automaton EMPTY = Automata.makeEmpty(); + private static final Logger logger = LogManager.getLogger(SystemIndices.class); + /** * This is the source for non-plugin system features. */ @@ -883,16 +887,6 @@ public void onFailure(Exception e) { }); } - private static int countInnerTasks( - Metadata metadata, - Collection indexDescriptors, - List associatedIndices - ) { - return associatedIndices.size() + (int) indexDescriptors.stream() - .filter(id -> id.getMatchingIndices(metadata).isEmpty() == false) - .count(); - } - /** * Clean up the state of a feature * @param indexDescriptors List of descriptors of a feature's system indices @@ -917,7 +911,9 @@ public static void cleanUpFeature( .flatMap(List::stream) .toList(); - final int taskCount = countInnerTasks(metadata, indexDescriptors, associatedIndices); + final int taskCount = associatedIndices.size() + (int) indexDescriptors.stream() + .filter(id -> id.getMatchingIndices(metadata).isEmpty() == false) + .count(); // check if there's nothing to do and take an early out if (taskCount == 0) { @@ -937,6 +933,7 @@ public static void cleanUpFeature( StringBuilder exceptions = new StringBuilder("["); exceptions.append(errors.stream().map(e -> e.getException().getMessage()).collect(Collectors.joining(", "))); exceptions.append(']'); + errors.forEach(e -> logger.warn(format("Encountered error while resetting feature [%s]", name), e)); listener.onResponse(ResetFeatureStateStatus.failure(name, new Exception(exceptions.toString()))); } }, listener::onFailure), @@ -953,7 +950,9 @@ public static void cleanUpFeature( List matchingIndices = indexDescriptor.getMatchingIndices(metadata); if (matchingIndices.isEmpty() == false) { - final OriginSettingClient clientWithOrigin = new OriginSettingClient(client, indexDescriptor.getOrigin()); + final Client clientWithOrigin = (indexDescriptor.getOrigin() == null) + ? client + : new OriginSettingClient(client, indexDescriptor.getOrigin()); cleanUpFeatureForIndices(name, clientWithOrigin, matchingIndices.toArray(Strings.EMPTY_ARRAY), groupedListener); } From b1308730a7a9432a2069918f663fbc0226e5fa8e Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Tue, 19 Jul 2022 20:54:59 -0400 Subject: [PATCH 9/9] Fix task counting issue --- .../main/java/org/elasticsearch/indices/SystemIndices.java | 4 ++-- .../elasticsearch/integration/SecurityFeatureResetTests.java | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/indices/SystemIndices.java b/server/src/main/java/org/elasticsearch/indices/SystemIndices.java index 56c5be35ec908..28750d6635821 100644 --- a/server/src/main/java/org/elasticsearch/indices/SystemIndices.java +++ b/server/src/main/java/org/elasticsearch/indices/SystemIndices.java @@ -911,7 +911,7 @@ public static void cleanUpFeature( .flatMap(List::stream) .toList(); - final int taskCount = associatedIndices.size() + (int) indexDescriptors.stream() + final int taskCount = ((associatedIndices.size() > 0) ? 1 : 0) + (int) indexDescriptors.stream() .filter(id -> id.getMatchingIndices(metadata).isEmpty() == false) .count(); @@ -933,7 +933,7 @@ public static void cleanUpFeature( StringBuilder exceptions = new StringBuilder("["); exceptions.append(errors.stream().map(e -> e.getException().getMessage()).collect(Collectors.joining(", "))); exceptions.append(']'); - errors.forEach(e -> logger.warn(format("Encountered error while resetting feature [%s]", name), e)); + errors.forEach(e -> logger.warn(() -> "error while resetting feature [" + name + "]", e.getException())); listener.onResponse(ResetFeatureStateStatus.failure(name, new Exception(exceptions.toString()))); } }, listener::onFailure), diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java index cf877fa10b4fe..3e75e2d95e8b2 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/integration/SecurityFeatureResetTests.java @@ -25,7 +25,7 @@ import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue; import static org.hamcrest.Matchers.containsString; -/** +/** * These tests ensure that the Feature Reset API works for users with default superuser and manage roles. * This can be complex due to restrictions on system indices and the need to use the correct origin for * each index. See also https://github.com/elastic/elasticsearch/issues/88617