Use an enum to track status

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

@@ -88,6 +88,13 @@ public enum Availability {
         PRIMARY_SHARDS
     }
 
+    public enum RoleMappingsCleanupMigrationStatus {
+        READY,
+        NOT_READY,
+        SKIP,
+        DONE
+    }
+
     private final Client client;
     private final SystemIndexDescriptor systemIndexDescriptor;
 
@@ -271,29 +278,44 @@ private static boolean isCreatedOnLatestVersion(IndexMetadata indexMetadata) {
     }
 
     /**
-     * Check to see if there are any file based role mappings and if they have been loaded into cluster state. If the
-     * {@link ReservedStateMetadata} file_settings namespace contains role mapping names, it means that there should be the same number of
-     * cluster state role mappings available. If they're not available yet using {@code RoleMappingMetadata.getFromClusterState()}, they
-     * have not yet been synchronized.
+     * Check if a role mappings cleanup migration is needed or has already been performed and if the cluster is ready for a cleanup
+     * migration
      */
-    private static boolean isReadyForRoleMappingCleanupMigration(ClusterState clusterState) {
+    private static RoleMappingsCleanupMigrationStatus getRoleMappingsCleanupMigrationStatus(
+        ClusterState clusterState,
+        int migrationsVersion
+    ) {
+        // Migration already finished
+        if (migrationsVersion >= SecurityMigrations.CLEANUP_ROLE_MAPPING_DUPLICATES_MIGRATION_VERSION) {
+            return RoleMappingsCleanupMigrationStatus.DONE;
+        }
+
         ReservedStateMetadata fileSettingsMetadata = clusterState.metadata().reservedStateMetadata().get(FILE_SETTINGS_METADATA_NAMESPACE);
         boolean hasFileSettingsMetadata = fileSettingsMetadata != null;
         // If there is no fileSettingsMetadata, there should be no reserved state (this is to catch bugs related to
         // name changes to FILE_SETTINGS_METADATA_NAMESPACE)
         assert hasFileSettingsMetadata || clusterState.metadata().reservedStateMetadata().isEmpty();
 
+        // If no file based role mappings available -> migration not needed
+        if (hasFileSettingsMetadata == false) {
+            return RoleMappingsCleanupMigrationStatus.SKIP;
+        }
+
         RoleMappingMetadata roleMappingMetadata = RoleMappingMetadata.getFromClusterState(clusterState);
-        return hasFileSettingsMetadata == false
-            || (reservedMappingsHasExpectedSize(roleMappingMetadata, fileSettingsMetadata)
-                && roleMappingMetadata.hasAnyMappingWithFallbackName() == false);
+
+        // If there are file based role mappings, make sure they have the latest format (name available) and that they have all been
+        // synced to cluster state (same size as the reserved state keys)
+        if (roleMappingMetadata.getRoleMappings().size() == fileSettingsMetadata.keys(ReservedRoleMappingAction.NAME).size()
+            && roleMappingMetadata.hasAnyMappingWithFallbackName() == false) {
+            return RoleMappingsCleanupMigrationStatus.READY;
+        }
+
+        // If none of the above conditions are met, wait for a state change to re-evaluate if the cluster is ready for migration
+        return RoleMappingsCleanupMigrationStatus.NOT_READY;
     }
 
-    private static boolean reservedMappingsHasExpectedSize(
-        RoleMappingMetadata roleMappingMetadata,
-        ReservedStateMetadata fileSettingsMetadata
-    ) {
-        return roleMappingMetadata.getRoleMappings().size() == fileSettingsMetadata.keys(ReservedRoleMappingAction.NAME).size();
+    public RoleMappingsCleanupMigrationStatus getRoleMappingsCleanupMigrationStatus() {
+        return state.roleMappingsCleanupMigrationStatus;
     }
 
     @Override
@@ -313,9 +335,12 @@ public void clusterChanged(ClusterChangedEvent event) {
         Tuple<Boolean, Boolean> available = checkIndexAvailable(event.state());
         final boolean indexAvailableForWrite = available.v1();
         final boolean indexAvailableForSearch = available.v2();
-        final boolean readyForRoleMappingCleanupMigration = isReadyForRoleMappingCleanupMigration(event.state());
-        final boolean mappingIsUpToDate = indexMetadata == null || checkIndexMappingUpToDate(event.state());
         final int migrationsVersion = getMigrationVersionFromIndexMetadata(indexMetadata);
+        final RoleMappingsCleanupMigrationStatus roleMappingsCleanupMigrationStatus = getRoleMappingsCleanupMigrationStatus(
+            event.state(),
+            migrationsVersion
+        );
+        final boolean mappingIsUpToDate = indexMetadata == null || checkIndexMappingUpToDate(event.state());
         final SystemIndexDescriptor.MappingsVersion minClusterMappingVersion = getMinSecurityIndexMappingVersion(event.state());
         final int indexMappingVersion = loadIndexMappingVersion(systemIndexDescriptor.getAliasName(), event.state());
         final String concreteIndexName = indexMetadata == null
@@ -344,7 +369,7 @@ public void clusterChanged(ClusterChangedEvent event) {
             indexAvailableForWrite,
             mappingIsUpToDate,
             createdOnLatestVersion,
-            readyForRoleMappingCleanupMigration,
+            roleMappingsCleanupMigrationStatus,
             migrationsVersion,
             minClusterMappingVersion,
             indexMappingVersion,
@@ -703,7 +728,7 @@ public static class State {
             false,
             false,
             false,
-            false,
+            null,
             null,
             null,
             null,
@@ -719,7 +744,7 @@ public static class State {
         public final boolean indexAvailableForWrite;
         public final boolean mappingUpToDate;
         public final boolean createdOnLatestVersion;
-        public final boolean readyForRoleMappingCleanupMigration;
+        public final RoleMappingsCleanupMigrationStatus roleMappingsCleanupMigrationStatus;
         public final Integer migrationsVersion;
         // Min mapping version supported by the descriptors in the cluster
         public final SystemIndexDescriptor.MappingsVersion minClusterMappingVersion;
@@ -738,7 +763,7 @@ public State(
             boolean indexAvailableForWrite,
             boolean mappingUpToDate,
             boolean createdOnLatestVersion,
-            boolean readyForRoleMappingCleanupMigration,
+            RoleMappingsCleanupMigrationStatus roleMappingsCleanupMigrationStatus,
             Integer migrationsVersion,
             SystemIndexDescriptor.MappingsVersion minClusterMappingVersion,
             Integer indexMappingVersion,
@@ -755,7 +780,7 @@ public State(
             this.mappingUpToDate = mappingUpToDate;
             this.migrationsVersion = migrationsVersion;
             this.createdOnLatestVersion = createdOnLatestVersion;
-            this.readyForRoleMappingCleanupMigration = readyForRoleMappingCleanupMigration;
+            this.roleMappingsCleanupMigrationStatus = roleMappingsCleanupMigrationStatus;
             this.minClusterMappingVersion = minClusterMappingVersion;
             this.indexMappingVersion = indexMappingVersion;
             this.concreteIndexName = concreteIndexName;
@@ -776,7 +801,7 @@ public boolean equals(Object o) {
                 && indexAvailableForWrite == state.indexAvailableForWrite
                 && mappingUpToDate == state.mappingUpToDate
                 && createdOnLatestVersion == state.createdOnLatestVersion
-                && readyForRoleMappingCleanupMigration == state.readyForRoleMappingCleanupMigration
+                && roleMappingsCleanupMigrationStatus == state.roleMappingsCleanupMigrationStatus
                 && Objects.equals(indexMappingVersion, state.indexMappingVersion)
                 && Objects.equals(migrationsVersion, state.migrationsVersion)
                 && Objects.equals(minClusterMappingVersion, state.minClusterMappingVersion)
@@ -799,7 +824,7 @@ public int hashCode() {
                 indexAvailableForWrite,
                 mappingUpToDate,
                 createdOnLatestVersion,
-                readyForRoleMappingCleanupMigration,
+                roleMappingsCleanupMigrationStatus,
                 migrationsVersion,
                 minClusterMappingVersion,
                 indexMappingVersion,
@@ -824,8 +849,8 @@ public String toString() {
                 + mappingUpToDate
                 + ", createdOnLatestVersion="
                 + createdOnLatestVersion
-                + ", readyForRoleMappingCleanupMigration="
-                + readyForRoleMappingCleanupMigration
+                + ", roleMappingsCleanupMigrationStatus="
+                + roleMappingsCleanupMigrationStatus
                 + ", migrationsVersion="
                 + migrationsVersion
                 + ", minClusterMappingVersion="

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

@@ -152,6 +152,11 @@ public int minMappingVersion() {
     public static class CleanupRoleMappingDuplicatesMigration implements SecurityMigration {
         @Override
         public void migrate(SecurityIndexManager indexManager, Client client, ActionListener<Void> listener) {
+            if (indexManager.getRoleMappingsCleanupMigrationStatus() == SecurityIndexManager.RoleMappingsCleanupMigrationStatus.SKIP) {
+                listener.onResponse(null);
+                return;
+            }
+
             getRoleMappings(client, ActionListener.wrap(roleMappings -> {
                 List<String> roleMappingsToDelete = getDuplicateRoleMappingNames(roleMappings.mappings());
                 if (roleMappingsToDelete.isEmpty() == false) {
@@ -206,7 +211,7 @@ private void deleteNativeRoleMappings(Client client, List<String> names, ActionL
         @Override
         public boolean checkPreConditions(SecurityIndexManager.State securityIndexManagerState) {
             // If there are operator defined role mappings, make sure they've been loaded in to cluster state before launching migration
-            return securityIndexManagerState.readyForRoleMappingCleanupMigration;
+            return securityIndexManagerState.roleMappingsCleanupMigrationStatus == SecurityIndexManager.RoleMappingsCleanupMigrationStatus.READY;
         }
 
         @Override

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java

@@ -2515,7 +2515,7 @@ private SecurityIndexManager.State dummyState(ClusterHealthStatus indexStatus) {
             true,
             true,
             true,
-            true,
+            null,
             null,
             null,
             null,

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmTests.java

@@ -40,7 +40,7 @@ private SecurityIndexManager.State dummyState(ClusterHealthStatus indexStatus) {
             true,
             true,
             true,
-            true,
+            null,
             null,
             null,
             null,

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/mapper/NativeRoleMappingStoreTests.java

@@ -412,7 +412,7 @@ private SecurityIndexManager.State indexState(boolean isUpToDate, ClusterHealthS
             true,
             true,
             true,
-            true,
+            null,
             null,
             null,
             null,

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

@@ -1618,7 +1618,7 @@ public SecurityIndexManager.State dummyIndexState(boolean isIndexUpToDate, Clust
             true,
             true,
             true,
-            true,
+            null,
             null,
             null,
             null,

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java

@@ -901,7 +901,7 @@ private SecurityIndexManager.State dummyState(
             true,
             true,
             true,
-            true,
+            null,
             null,
             null,
             null,

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

@@ -62,7 +62,7 @@ public void testSecurityIndexStateChangeWillInvalidateAllRegisteredInvalidators(
             true,
             true,
             true,
-            true,
+            null,
             null,
             new SystemIndexDescriptor.MappingsVersion(SecurityMainIndexMappingVersion.latest().id(), 0),
             null,