Prevent migration of system indices that match templates (#87933)

docs/changelog/87933.yaml

@@ -0,0 +1,5 @@
+pr: 87933
+summary: Prevent migration of indices that match templates
+area: Infra/Core
+type: bug
+issues: [86801, 87827]

modules/kibana/src/main/java/org/elasticsearch/kibana/KibanaPlugin.java

@@ -27,6 +27,7 @@ public class KibanaPlugin extends Plugin implements SystemIndexPlugin {
         .setAliasName(".kibana")
         .setType(Type.EXTERNAL_UNMANAGED)
         .setAllowedElasticProductOrigins(KIBANA_PRODUCT_ORIGIN)
+        .setAllowsTemplates()
         .build();
 
     public static final SystemIndexDescriptor REPORTING_INDEX_DESCRIPTOR = SystemIndexDescriptor.builder()

modules/reindex/src/internalClusterTest/java/org/elasticsearch/migration/AbstractFeatureMigrationIntegTest.java

@@ -114,6 +114,18 @@ public abstract class AbstractFeatureMigrationIntegTest extends ESIntegTestCase
     static final int EXTERNAL_UNMANAGED_FLAG_VALUE = 4;
     static final String ASSOCIATED_INDEX_NAME = ".my-associated-idx";
 
+    public static final SystemIndexDescriptor KIBANA_MOCK_INDEX_DESCRIPTOR = SystemIndexDescriptor.builder()
+        .setIndexPattern(".kibana_*")
+        .setDescription("Kibana saved objects system index")
+        .setAliasName(".kibana")
+        .setType(SystemIndexDescriptor.Type.EXTERNAL_UNMANAGED)
+        .setAllowedElasticProductOrigins(Collections.emptyList())
+        .setAllowedElasticProductOrigins(Collections.singletonList(ORIGIN))
+        .setMinimumNodeVersion(NEEDS_UPGRADE_VERSION)
+        .setPriorSystemIndexDescriptors(Collections.emptyList())
+        .setAllowsTemplates()
+        .build();
+
     protected String masterAndDataNode;
     protected String masterName;
 
@@ -262,7 +274,7 @@ public abstract class AbstractFeatureMigrationIntegTest extends ESIntegTestCase
 
         @Override
         public Collection<SystemIndexDescriptor> getSystemIndexDescriptors(Settings settings) {
-            return Arrays.asList(INTERNAL_MANAGED, INTERNAL_UNMANAGED, EXTERNAL_MANAGED, EXTERNAL_UNMANAGED);
+            return Arrays.asList(INTERNAL_MANAGED, INTERNAL_UNMANAGED, EXTERNAL_MANAGED, EXTERNAL_UNMANAGED, KIBANA_MOCK_INDEX_DESCRIPTOR);
         }
 
         @Override

modules/reindex/src/internalClusterTest/java/org/elasticsearch/migration/FeatureMigrationIT.java

@@ -15,16 +15,24 @@ import org.elasticsearch.action.admin.cluster.migration.GetFeatureUpgradeStatusR
 import org.elasticsearch.action.admin.cluster.migration.PostFeatureUpgradeAction;
 import org.elasticsearch.action.admin.cluster.migration.PostFeatureUpgradeRequest;
 import org.elasticsearch.action.admin.cluster.migration.PostFeatureUpgradeResponse;
+import org.elasticsearch.action.admin.indices.alias.Alias;
 import org.elasticsearch.action.admin.indices.create.CreateIndexRequestBuilder;
 import org.elasticsearch.action.admin.indices.create.CreateIndexResponse;
+import org.elasticsearch.action.admin.indices.template.put.PutComponentTemplateAction;
+import org.elasticsearch.action.admin.indices.template.put.PutComposableIndexTemplateAction;
 import org.elasticsearch.action.support.ActiveShardCount;
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.ClusterStateUpdateTask;
+import org.elasticsearch.cluster.metadata.ComponentTemplate;
+import org.elasticsearch.cluster.metadata.ComposableIndexTemplate;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
 import org.elasticsearch.cluster.metadata.Metadata;
+import org.elasticsearch.cluster.metadata.Template;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.Strings;
+import org.elasticsearch.common.compress.CompressedXContent;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.indices.SystemIndexDescriptor;
 import org.elasticsearch.plugins.Plugin;
 import org.elasticsearch.reindex.ReindexPlugin;
 import org.elasticsearch.upgrades.FeatureMigrationResults;
@@ -309,4 +317,145 @@ public class FeatureMigrationIT extends AbstractFeatureMigrationIntegTest {
             assertThat(statusResp.getUpgradeStatus(), equalTo(GetFeatureUpgradeStatusResponse.UpgradeStatus.NO_MIGRATION_NEEDED));
         });
     }
+
+    private String featureUpgradeErrorResponse(GetFeatureUpgradeStatusResponse statusResp) {
+        return statusResp.getFeatureUpgradeStatuses()
+            .stream()
+            .map(f -> f.getIndexVersions())
+            .flatMap(List::stream)
+            .map(i -> (i.getException() == null) ? "" : i.getException().getMessage())
+            .collect(Collectors.joining(" "));
+    }
+
+    private void migrateWithTemplatesV1(String templatePrefix, SystemIndexDescriptor... descriptors) throws Exception {
+        for (SystemIndexDescriptor descriptor : descriptors) {
+            createSystemIndexForDescriptor(descriptor);
+        }
+
+        client().admin()
+            .indices()
+            .preparePutTemplate("bad_template")
+            .setPatterns(Collections.singletonList(templatePrefix + "*"))
+            .addAlias(new Alias(templatePrefix + "-legacy-alias"))
+            .get();
+
+        ensureGreen();
+
+        PostFeatureUpgradeResponse migrationResponse = client().execute(PostFeatureUpgradeAction.INSTANCE, new PostFeatureUpgradeRequest())
+            .get();
+
+        assertTrue(migrationResponse.isAccepted());
+    }
+
+    public void testBailOnMigrateWithTemplatesV1() throws Exception {
+        migrateWithTemplatesV1(".int", INTERNAL_UNMANAGED);
+
+        assertBusy(() -> {
+            GetFeatureUpgradeStatusResponse statusResp = client().execute(
+                GetFeatureUpgradeStatusAction.INSTANCE,
+                new GetFeatureUpgradeStatusRequest()
+            ).get();
+            logger.info(Strings.toString(statusResp));
+            assertThat(statusResp.getUpgradeStatus(), equalTo(GetFeatureUpgradeStatusResponse.UpgradeStatus.ERROR));
+            assertTrue(featureUpgradeErrorResponse(statusResp).contains(" because it would match legacy templates "));
+        });
+    }
+
+    public void testMigrateWithTemplatesV1() throws Exception {
+        // this should pass for both, kibana allows templates, the unmanaged doesn't match the template
+        migrateWithTemplatesV1(".kibana", KIBANA_MOCK_INDEX_DESCRIPTOR, INTERNAL_UNMANAGED);
+
+        assertBusy(() -> {
+            GetFeatureUpgradeStatusResponse statusResp = client().execute(
+                GetFeatureUpgradeStatusAction.INSTANCE,
+                new GetFeatureUpgradeStatusRequest()
+            ).get();
+            logger.info(Strings.toString(statusResp));
+            assertThat(statusResp.getUpgradeStatus(), equalTo(GetFeatureUpgradeStatusResponse.UpgradeStatus.NO_MIGRATION_NEEDED));
+        });
+    }
+
+    private void migrateWithTemplatesV2(String prefix, SystemIndexDescriptor... descriptors) throws Exception {
+        for (SystemIndexDescriptor descriptor : descriptors) {
+            createSystemIndexForDescriptor(descriptor);
+        }
+
+        ComponentTemplate ct = new ComponentTemplate(
+            new Template(
+                null,
+                new CompressedXContent(
+                    "{\n"
+                        + "      \"dynamic\": false,\n"
+                        + "      \"properties\": {\n"
+                        + "        \"field1\": {\n"
+                        + "          \"type\": \"text\"\n"
+                        + "        }\n"
+                        + "      }\n"
+                        + "    }"
+                ),
+                null
+            ),
+            3L,
+            Collections.singletonMap("foo", "bar")
+        );
+        client().execute(PutComponentTemplateAction.INSTANCE, new PutComponentTemplateAction.Request("a-ct").componentTemplate(ct)).get();
+
+        ComposableIndexTemplate cit = new ComposableIndexTemplate(
+            Collections.singletonList(prefix + "*"),
+            new Template(
+                null,
+                new CompressedXContent(
+                    "{\n"
+                        + "      \"dynamic\": false,\n"
+                        + "      \"properties\": {\n"
+                        + "        \"field2\": {\n"
+                        + "          \"type\": \"keyword\"\n"
+                        + "        }\n"
+                        + "      }\n"
+                        + "    }"
+                ),
+                null
+            ),
+            Collections.singletonList("a-ct"),
+            4L,
+            5L,
+            Collections.singletonMap("baz", "thud")
+        );
+        client().execute(PutComposableIndexTemplateAction.INSTANCE, new PutComposableIndexTemplateAction.Request("a-it").indexTemplate(cit))
+            .get();
+
+        ensureGreen();
+
+        PostFeatureUpgradeResponse migrationResponse = client().execute(PostFeatureUpgradeAction.INSTANCE, new PostFeatureUpgradeRequest())
+            .get();
+        assertTrue(migrationResponse.isAccepted());
+    }
+
+    public void testBailOnMigrateWithTemplatesV2() throws Exception {
+        migrateWithTemplatesV2(".int", INTERNAL_UNMANAGED);
+
+        assertBusy(() -> {
+            GetFeatureUpgradeStatusResponse statusResp = client().execute(
+                GetFeatureUpgradeStatusAction.INSTANCE,
+                new GetFeatureUpgradeStatusRequest()
+            ).get();
+            logger.info(Strings.toString(statusResp));
+            assertThat(statusResp.getUpgradeStatus(), equalTo(GetFeatureUpgradeStatusResponse.UpgradeStatus.ERROR));
+            assertTrue(featureUpgradeErrorResponse(statusResp).contains(" it would match composable template [a-it]"));
+        });
+    }
+
+    public void testMigrateWithTemplatesV2() throws Exception {
+        // this should pass for both, kibana allows templates, the unmanaged doesn't match the template
+        migrateWithTemplatesV2(".kibana", KIBANA_MOCK_INDEX_DESCRIPTOR, INTERNAL_UNMANAGED);
+
+        assertBusy(() -> {
+            GetFeatureUpgradeStatusResponse statusResp = client().execute(
+                GetFeatureUpgradeStatusAction.INSTANCE,
+                new GetFeatureUpgradeStatusRequest()
+            ).get();
+            logger.info(Strings.toString(statusResp));
+            assertThat(statusResp.getUpgradeStatus(), equalTo(GetFeatureUpgradeStatusResponse.UpgradeStatus.NO_MIGRATION_NEEDED));
+        });
+    }
 }

server/src/internalClusterTest/java/org/elasticsearch/indices/TestSystemIndexDescriptor.java

@@ -54,6 +54,7 @@ public class TestSystemIndexDescriptor extends SystemIndexDescriptor {
             List.of(),
             List.of(),
             null,
+            false,
             false
         );
     }

server/src/main/java/org/elasticsearch/action/admin/cluster/migration/TransportGetFeatureUpgradeStatusAction.java

@@ -38,6 +38,7 @@ import static org.elasticsearch.action.admin.cluster.migration.GetFeatureUpgrade
 import static org.elasticsearch.action.admin.cluster.migration.GetFeatureUpgradeStatusResponse.UpgradeStatus.IN_PROGRESS;
 import static org.elasticsearch.action.admin.cluster.migration.GetFeatureUpgradeStatusResponse.UpgradeStatus.MIGRATION_NEEDED;
 import static org.elasticsearch.action.admin.cluster.migration.GetFeatureUpgradeStatusResponse.UpgradeStatus.NO_MIGRATION_NEEDED;
+import static org.elasticsearch.indices.SystemIndices.UPGRADED_INDEX_SUFFIX;
 import static org.elasticsearch.upgrades.SystemIndexMigrationTaskParams.SYSTEM_INDEX_UPGRADE_TASK_NAME;
 
 /**
@@ -150,6 +151,7 @@ public class TransportGetFeatureUpgradeStatusAction extends TransportMasterNodeA
         ).map(FeatureMigrationResults::getFeatureStatuses).map(results -> results.get(feature.getName())).orElse(null);
 
         final String failedFeatureName = featureStatus == null ? null : featureStatus.getFailedIndexName();
+        final String failedFeatureUpgradedName = failedFeatureName == null ? null : failedFeatureName + UPGRADED_INDEX_SUFFIX;
         final Exception exception = featureStatus == null ? null : featureStatus.getException();
 
         return feature.getIndexDescriptors()
@@ -161,7 +163,8 @@ public class TransportGetFeatureUpgradeStatusAction extends TransportMasterNodeA
                 indexMetadata -> new GetFeatureUpgradeStatusResponse.IndexInfo(
                     indexMetadata.getIndex().getName(),
                     indexMetadata.getCreationVersion(),
-                    indexMetadata.getIndex().getName().equals(failedFeatureName) ? exception : null
+                    (indexMetadata.getIndex().getName().equals(failedFeatureName)
+                        || indexMetadata.getIndex().getName().equals(failedFeatureUpgradedName)) ? exception : null
                 )
             )
             .toList();

server/src/main/java/org/elasticsearch/indices/SystemIndexDescriptor.java

@@ -161,6 +161,13 @@ public class SystemIndexDescriptor implements IndexPatternMatcher, Comparable<Sy
      */
     private final boolean isNetNew;
 
+    /**
+     * We typically don't want to apply user defined templates on system indices, since they may have unexpected
+     * behaviour when upgrading Elasticsearch versions. Currently, only the .kibana_ indices use templates, so we
+     * are making this property by default as false.
+     */
+    private final boolean allowsTemplates;
+
     /**
      * The thread pools that actions will use to operate on this descriptor's system indices
      */
@@ -190,6 +197,7 @@ public class SystemIndexDescriptor implements IndexPatternMatcher, Comparable<Sy
             List.of(),
             List.of(),
             null,
+            false,
             false
         );
     }
@@ -221,6 +229,7 @@ public class SystemIndexDescriptor implements IndexPatternMatcher, Comparable<Sy
             allowedElasticProductOrigins,
             List.of(),
             null,
+            false,
             false
         );
     }
@@ -248,6 +257,7 @@ public class SystemIndexDescriptor implements IndexPatternMatcher, Comparable<Sy
      *                                     indices
      * @param priorSystemIndexDescriptors A list of system index descriptors that describe the same index in a way that is compatible with
      *                                    older versions of Elasticsearch
+     * @param allowsTemplates if this system index descriptor allows templates to affect its settings (e.g. .kibana_ indices)
      */
     SystemIndexDescriptor(
         String indexPattern,
@@ -264,7 +274,8 @@ public class SystemIndexDescriptor implements IndexPatternMatcher, Comparable<Sy
         List<String> allowedElasticProductOrigins,
         List<SystemIndexDescriptor> priorSystemIndexDescriptors,
         ExecutorNames executorNames,
-        boolean isNetNew
+        boolean isNetNew,
+        boolean allowsTemplates
     ) {
         Objects.requireNonNull(indexPattern, "system index pattern must not be null");
         if (indexPattern.length() < 2) {
@@ -421,6 +432,7 @@ public class SystemIndexDescriptor implements IndexPatternMatcher, Comparable<Sy
         this.priorSystemIndexDescriptors = sortedPriorSystemIndexDescriptors;
         this.executorNames = Objects.nonNull(executorNames) ? executorNames : ExecutorNames.DEFAULT_SYSTEM_INDEX_THREAD_POOLS;
         this.isNetNew = isNetNew;
+        this.allowsTemplates = allowsTemplates;
     }
 
     /**
@@ -548,6 +560,10 @@ public class SystemIndexDescriptor implements IndexPatternMatcher, Comparable<Sy
         return isNetNew;
     }
 
+    public boolean allowsTemplates() {
+        return allowsTemplates;
+    }
+
     public Version getMappingVersion() {
         if (isAutomaticallyManaged() == false) {
             throw new IllegalStateException(this + " is not managed so there are no mappings or version");
@@ -673,6 +689,7 @@ public class SystemIndexDescriptor implements IndexPatternMatcher, Comparable<Sy
         private List<SystemIndexDescriptor> priorSystemIndexDescriptors = List.of();
         private ExecutorNames executorNames;
         private boolean isNetNew = false;
+        private boolean allowsTemplates = false;
 
         private Builder() {}
 
@@ -764,12 +781,16 @@ public class SystemIndexDescriptor implements IndexPatternMatcher, Comparable<Sy
             return this;
         }
 
+        public Builder setAllowsTemplates() {
+            this.allowsTemplates = true;
+            return this;
+        }
+
         /**
          * Builds a {@link SystemIndexDescriptor} using the fields supplied to this builder.
          * @return a populated descriptor.
          */
         public SystemIndexDescriptor build() {
-
             return new SystemIndexDescriptor(
                 indexPattern,
                 primaryIndex,
@@ -785,7 +806,8 @@ public class SystemIndexDescriptor implements IndexPatternMatcher, Comparable<Sy
                 allowedElasticProductOrigins,
                 priorSystemIndexDescriptors,
                 executorNames,
-                isNetNew
+                isNetNew,
+                allowsTemplates
             );
         }
     }

server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrationInfo.java

@@ -44,6 +44,7 @@ class SystemIndexMigrationInfo implements Comparable<SystemIndexMigrationInfo> {
     private final String mapping;
     private final String origin;
     private final SystemIndices.Feature owningFeature;
+    private final boolean allowsTemplates;
 
     private static final Comparator<SystemIndexMigrationInfo> SAME_CLASS_COMPARATOR = Comparator.comparing(
         SystemIndexMigrationInfo::getFeatureName
@@ -55,7 +56,8 @@ class SystemIndexMigrationInfo implements Comparable<SystemIndexMigrationInfo> {
         Settings settings,
         String mapping,
         String origin,
-        SystemIndices.Feature owningFeature
+        SystemIndices.Feature owningFeature,
+        boolean allowsTemplates
     ) {
         this.currentIndex = currentIndex;
         this.featureName = featureName;
@@ -63,6 +65,7 @@ class SystemIndexMigrationInfo implements Comparable<SystemIndexMigrationInfo> {
         this.mapping = mapping;
         this.origin = origin;
         this.owningFeature = owningFeature;
+        this.allowsTemplates = allowsTemplates;
     }
 
     /**
@@ -114,6 +117,16 @@ class SystemIndexMigrationInfo implements Comparable<SystemIndexMigrationInfo> {
         return origin;
     }
 
+    /**
+     * By default, system indices should not be affected by user defined templates, so this
+     * method should return false in almost all cases. At the moment certain Kibana indices use
+     * templates, therefore we allow templates to be used on Kibana created system indices until
+     * Kibana removes the template use on system index creation.
+     */
+    boolean allowsTemplates() {
+        return allowsTemplates;
+    }
+
     /**
      * Invokes the pre-migration hook for the feature that owns this index.
      * See {@link SystemIndexPlugin#prepareForIndicesMigration(ClusterService, Client, ActionListener)}.
@@ -197,7 +210,15 @@ class SystemIndexMigrationInfo implements Comparable<SystemIndexMigrationInfo> {
             // Copy mapping from the old index
             mapping = currentIndex.mapping().source().string();
         }
-        return new SystemIndexMigrationInfo(currentIndex, feature.getName(), settings, mapping, descriptor.getOrigin(), feature);
+        return new SystemIndexMigrationInfo(
+            currentIndex,
+            feature.getName(),
+            settings,
+            mapping,
+            descriptor.getOrigin(),
+            feature,
+            descriptor.allowsTemplates()
+        );
     }
 
     private static Settings copySettingsForNewIndex(Settings currentIndexSettings, IndexScopedSettings indexScopedSettings) {

server/src/main/java/org/elasticsearch/upgrades/SystemIndexMigrator.java

@@ -25,8 +25,10 @@ import org.elasticsearch.client.internal.ParentTaskAssigningClient;
 import org.elasticsearch.cluster.ClusterState;
 import org.elasticsearch.cluster.ClusterStateUpdateTask;
 import org.elasticsearch.cluster.metadata.IndexMetadata;
+import org.elasticsearch.cluster.metadata.IndexTemplateMetadata;
 import org.elasticsearch.cluster.metadata.Metadata;
 import org.elasticsearch.cluster.metadata.MetadataCreateIndexService;
+import org.elasticsearch.cluster.metadata.MetadataIndexTemplateService;
 import org.elasticsearch.cluster.metadata.MetadataUpdateSettingsService;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.Strings;
@@ -378,6 +380,47 @@ public class SystemIndexMigrator extends AllocatedPersistentTask {
         }
         Index oldIndex = imd.getIndex();
         String newIndexName = migrationInfo.getNextIndexName();
+
+        /**
+         * This should be on for all System indices except for .kibana_ indices. See allowsTemplates in KibanaPlugin.java for more info.
+         */
+        if (migrationInfo.allowsTemplates() == false) {
+            final String v2template = MetadataIndexTemplateService.findV2Template(clusterState.metadata(), newIndexName, false);
+            if (Objects.nonNull(v2template)) {
+                logger.error(
+                    "unable to create new index [{}] from feature [{}] because it would match composable template [{}]",
+                    newIndexName,
+                    migrationInfo.getFeatureName(),
+                    v2template
+                );
+                markAsFailed(
+                    new IllegalStateException(
+                        "unable to create new index [" + newIndexName + "] because it would match composable template [" + v2template + "]"
+                    )
+                );
+                return;
+            }
+            final List<IndexTemplateMetadata> v1templates = MetadataIndexTemplateService.findV1Templates(
+                clusterState.metadata(),
+                newIndexName,
+                false
+            );
+            if (v1templates.isEmpty() == false) {
+                logger.error(
+                    "unable to create new index [{}] from feature [{}] because it would match legacy templates [{}]",
+                    newIndexName,
+                    migrationInfo.getFeatureName(),
+                    v1templates
+                );
+                markAsFailed(
+                    new IllegalStateException(
+                        "unable to create new index [" + newIndexName + "] because it would match legacy templates [" + v1templates + "]"
+                    )
+                );
+                return;
+            }
+        }
+
         logger.info("migrating index [{}] from feature [{}] to new index [{}]", oldIndexName, migrationInfo.getFeatureName(), newIndexName);
         ActionListener<BulkByScrollResponse> innerListener = ActionListener.wrap(listener::accept, this::markAsFailed);
         try {