More reliable trigger for security index migration (#139028) (#139039)

We always want to trigger an index migration if one is required.

However, we previously would only do that if we detected a change to
the state of the security index on the master node.
But a rolling upgrade might not cause a detectable change in index
state - for example, in a cluster with dedicated masters nodes, if
those nodes were upgraded last, then all index relocation would happen
before the masters knew about the new migration (so they couldn't
trigger it) and once the masters were upgraded they would detect that
nothing had changed so never send a "security index changed" event and
never trigger the migration task.

Now, we say that if a security index exists, and it requires migration
then it also triggers a change event

Author: tvernum

docs/changelog/139028.yaml

@@ -0,0 +1,5 @@
+pr: 139028
+summary: More reliable trigger for security index migration
+area: Security
+type: bug
+issues: []

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityIndexManager.java

@@ -785,7 +785,7 @@ public void clusterChanged(ClusterChangedEvent event) {
             final IndexState previousState = getProjectState(projectId, previousStateByProject);
             final IndexState newState = updateProjectState(clusterState.projectState(projectId));
 
-            if (newState.equals(previousState) == false) {
+            if (shouldNotifyListeners(newState, previousState)) {
                 notifications.add(() -> {
                     for (var listener : stateChangeListeners) {
                         listener.apply(projectId, previousState, newState);
@@ -817,6 +817,25 @@ public void clusterChanged(ClusterChangedEvent event) {
         notifications.forEach(Runnable::run);
     }
 
+    private static boolean shouldNotifyListeners(IndexState newState, IndexState previousState) {
+        if (newState.equals(previousState)) {
+            // If we add a new migration (in code), but don't change anything internal to the index state then the `IndexState`` will never
+            // change (unless the index health changes) but we do want to treat changes to "is-up-to-date-with-migrations" as a state change
+            // However we can't do that (easily) with a flag in `IndexState` because that flag wouldn't change - as soon as the new version
+            // of the code was deployed it would think that the state was "not-up-to-date" and wouldn't detect a change.
+            // Instead we just handle it as a special case here.
+            // But, this class manages multiple different indices, not all of which have migrations defined. So we only trigger this is
+            // the index has had at least 1 migration before (if the index is entirely new then it will be picked up by other state changes)
+            return newState.indexExists()
+                && newState.securityMigrationRunning == false
+                && newState.migrationsVersion != null
+                && newState.migrationsVersion > 0
+                && newState.migrationsVersion < SecurityMigrations.highestMigrationVersion();
+        } else {
+            return true;
+        }
+    }
+
     private IndexState updateProjectState(ProjectState project) {
         final IndexMetadata indexMetadata = resolveConcreteIndex(systemIndexDescriptor.getAliasName(), project.metadata());
         final boolean createdOnLatestVersion = isCreatedOnLatestVersion(indexMetadata);

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/support/SecurityMigrations.java

@@ -115,6 +115,13 @@ default boolean checkPreConditions(SecurityIndexManager.IndexState securityIndex
         )
     );
 
+    /**
+     * @return The highest migration version that is currently defined
+     */
+    public static int highestMigrationVersion() {
+        return MIGRATIONS_BY_VERSION.lastKey();
+    }
+
     public static class RoleMetadataFlattenedMigration implements SecurityMigration {
 
         @Override

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/support/SecurityIndexManagerTests.java

@@ -85,6 +85,8 @@
 import static org.elasticsearch.cluster.metadata.Metadata.DEFAULT_PROJECT_ID;
 import static org.elasticsearch.cluster.routing.GlobalRoutingTableTestHelper.routingTable;
 import static org.elasticsearch.xcontent.XContentFactory.jsonBuilder;
+import static org.elasticsearch.xpack.core.security.action.UpdateIndexMigrationVersionAction.MIGRATION_VERSION_CUSTOM_DATA_KEY;
+import static org.elasticsearch.xpack.core.security.action.UpdateIndexMigrationVersionAction.MIGRATION_VERSION_CUSTOM_KEY;
 import static org.elasticsearch.xpack.security.support.SecurityIndexManager.FILE_SETTINGS_METADATA_NAMESPACE;
 import static org.hamcrest.Matchers.aMapWithSize;
 import static org.hamcrest.Matchers.contains;
@@ -309,6 +311,60 @@ private ClusterChangedEvent event(ClusterState clusterState) {
         return new ClusterChangedEvent("test-event", clusterState, EMPTY_CLUSTER_STATE);
     }
 
+    public void testStateChangeListenerIsCalledIfMigrationIsRequired() {
+        final AtomicBoolean listenerCalled = new AtomicBoolean(false);
+        final AtomicReference<ProjectId> projectIdRef = new AtomicReference<>();
+        final AtomicReference<IndexState> previousState = new AtomicReference<>();
+        final AtomicReference<IndexState> currentState = new AtomicReference<>();
+        final TriConsumer<ProjectId, IndexState, IndexState> listener = (projId, prevState, state) -> {
+            projectIdRef.set(projId);
+            previousState.set(prevState);
+            currentState.set(state);
+            listenerCalled.set(true);
+        };
+        manager.addStateListener(listener);
+
+        // index doesn't exist and now exists
+        ClusterState.Builder clusterStateBuilder = createClusterState(
+            TestRestrictedIndices.INTERNAL_SECURITY_MAIN_INDEX_7,
+            SecuritySystemIndices.SECURITY_MAIN_ALIAS
+        );
+        clusterStateBuilder = setMigrationVersion(
+            clusterStateBuilder.build(),
+            TestRestrictedIndices.INTERNAL_SECURITY_MAIN_INDEX_7,
+            randomIntBetween(1, SecurityMigrations.highestMigrationVersion() - 1)
+        );
+
+        final ClusterState clusterState = markShardsAvailable(clusterStateBuilder);
+        manager.clusterChanged(event(clusterState));
+
+        assertThat(listenerCalled.get(), is(true));
+
+        for (int i = 0; i < 3; i++) {
+            listenerCalled.set(false);
+            ClusterChangedEvent event = new ClusterChangedEvent("same index state", clusterState, clusterState);
+            manager.clusterChanged(event);
+
+            assertThat(listenerCalled.get(), is(true));
+            assertThat(previousState.get(), equalTo(currentState.get()));
+        }
+
+        final var newClusterState = setMigrationVersion(
+            clusterState,
+            TestRestrictedIndices.INTERNAL_SECURITY_MAIN_INDEX_7,
+            SecurityMigrations.highestMigrationVersion()
+        ).build();
+
+        listenerCalled.set(false);
+        manager.clusterChanged(new ClusterChangedEvent("modified index state", newClusterState, clusterState));
+        assertThat(listenerCalled.get(), is(true));
+        assertThat(previousState.get(), not(equalTo(currentState.get())));
+
+        listenerCalled.set(false);
+        manager.clusterChanged(new ClusterChangedEvent("same index state", newClusterState, newClusterState));
+        assertThat(listenerCalled.get(), is(false));
+    }
+
     public void testIndexHealthChangeListeners() {
         final AtomicBoolean listenerCalled = new AtomicBoolean(false);
         final AtomicReference<ProjectId> projectIdRef = new AtomicReference<>();
@@ -1123,6 +1179,22 @@ private static ProjectMetadata.Builder createProjectMetadata(ProjectId projectId
         );
     }
 
+    private ClusterState.Builder setMigrationVersion(ClusterState clusterState, String indexName, Integer version) {
+        final ClusterState.Builder csBuilder = ClusterState.builder(clusterState);
+        clusterState.forEachProject(project -> {
+            final ProjectMetadata.Builder projectBuilder = ProjectMetadata.builder(project.metadata());
+            final IndexMetadata.Builder indexBuilder = IndexMetadata.builder(project.metadata().index(indexName));
+            if (version == null) {
+                indexBuilder.removeCustom(MIGRATION_VERSION_CUSTOM_KEY);
+            } else {
+                indexBuilder.putCustom(MIGRATION_VERSION_CUSTOM_KEY, Map.of(MIGRATION_VERSION_CUSTOM_DATA_KEY, Integer.toString(version)));
+            }
+            projectBuilder.put(indexBuilder);
+            csBuilder.putProjectMetadata(projectBuilder);
+        });
+        return csBuilder;
+    }
+
     private ClusterState markShardsAvailable(ClusterState.Builder clusterStateBuilder) {
         final ClusterState cs = clusterStateBuilder.build();
         final RoutingTable projectRouting = SecurityTestUtils.buildIndexRoutingTable(
@@ -1159,7 +1231,6 @@ private static IndexMetadata.Builder getIndexMetadata(
         if (mappings != null) {
             indexMetadata.putMapping(mappings);
         }
-
         return indexMetadata;
     }