Implement repair functionality for aliases colliding with indices bug

[original-brownbear]

Follow up to #91456 implementing an automated fix for indices corrupted in 8.5.

Not sure this should get an automated test, could write one that corrupts the index in 8.5 and then does a rolling upgrade but maybe manually testing this is good enough in this case?

[elasticsearchmachine]

Pinging @elastic/es-distributed (Team:Distributed)

[original-brownbear]

Jenkins run elasticsearch-ci/part-1

[henningandersen]

could write one that corrupts the index in 8.5 and then does a rolling upgrade

I think the level of testing here is adequate for ensuring it is fixed on upgrade.

[henningandersen]

Is there a risk that adding an alias with the same name as the index for an index created in 8.5 will now succeed and just result in the "corrupted" alias name instead?

Can we perhaps find or add an integration test that verifies that when adding aliases we have validation in place that catches this?

[original-brownbear]

Oh right ... this is quite annoying. I could obviously add the validation for this at another level as well (not sure where, it seems not entirely trivial to add this check elsewhere in a way that would only catch mutations) but it does add some extra complexity to the whole thing. Is it worth doing that for this corner case? The name collision checks are quite complicated as it is already and checking for this specifically would add yet more complexity at some other layer ...

[henningandersen]

Could we add a buildAndRepair method that is used from IndexMetadata.fromXContent, passing down a boolean to a private build method in order to only do the repair from that path?

[henningandersen]

I recon my comment being slightly convoluted, so here is a diff of what I am thinking of:

Index: server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java
--- a/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java	(revision c6565a39e26f6c91f8e6f71f8021e07f8805639d)
+++ b/server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadata.java	(date 1669302455554)
@@ -2016,6 +2016,10 @@
         }
 
         public IndexMetadata build() {
+            return build(false);
+        }
+
+        private IndexMetadata build(boolean repair) {
             /*
              * We expect that the metadata has been properly built to set the number of shards and the number of replicas, and do not rely
              * on the default values here. Those must have been set upstream.
@@ -2144,7 +2148,7 @@
             var aliasesMap = aliases.build();
             for (AliasMetadata alias : aliasesMap.values()) {
                 if (alias.alias().equals(index)) {
-                    if (indexCreatedVersion.equals(Version.V_8_5_0)) {
+                    if (repair && indexCreatedVersion.equals(Version.V_8_5_0)) {
                         var updatedBuilder = ImmutableOpenMap.builder(aliasesMap);
                         final var brokenAlias = updatedBuilder.remove(index);
                         final var fixedAlias = AliasMetadata.newAliasMetadata(brokenAlias, index + "-alias-corrupted-by-8-5");
@@ -2208,6 +2212,11 @@
             );
         }
 
+        // visible for testing.
+        IndexMetadata buildAndRepair() {
+            return build(true);
+        }
+
         @SuppressWarnings("unchecked")
         public static void toXContent(IndexMetadata indexMetadata, XContentBuilder builder, ToXContent.Params params) throws IOException {
             Metadata.XContentContext context = Metadata.XContentContext.valueOf(
@@ -2476,7 +2485,7 @@
             assert settingsVersion : "settings version should be present for indices created on or after 6.5.0";
             assert indexCreatedVersion(builder.settings).before(Version.V_7_2_0) || aliasesVersion
                 : "aliases version should be present for indices created on or after 7.2.0";
-            return builder.build();
+            return builder.buildAndRepair();
         }
 
         /**

[original-brownbear]

I was thinking about that, but the would mean that the fix will only apply with a full cluster restart and not with a rolling restart? Wouldn't that be painful for Cloud?

[henningandersen]

We could also build and repair when receiving the index metadata over the wire maybe?

[original-brownbear]

We could I guess :) lets do it.

[original-brownbear]

Added to from--x-content, reading from the wire and (not sure this is necessary but I found it weird for it to behave different form normal transport read) diff application now :)

[henningandersen]

LGTM.

[henningandersen]

Could we expand this test (or add one) to also verify that for indices created in v8.5, we reject an alias of the same name as the index?

[original-brownbear]

Thanks Henning!

[elasticsearchmachine]

💔 Backport failed

Status	Branch	Result
✅	8.6
❌	8.5	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 91887