[7.x] Disallow ILM searchable snapshot actions that use different repositories (#68856) (#68866)

This commit adds validation when updating or creating an ILM policy that mulitple searchable
snapshot actions all use the same repository. We currently do not have the infrastructure to support
switching a searchable snapshot from one repository to another, so until we have that we will
disallow this (edge case) when creating a policy.

Relates to #68714

Author: dakrone

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/SearchableSnapshotAction.java

@@ -117,6 +117,10 @@ public MountSearchableSnapshotRequest.Storage getStorageType() {
         return storageType;
     }
 
+    public String getSnapshotRepository() {
+        return snapshotRepository;
+    }
+
     @Override
     public List<Step> toSteps(Client client, String phase, StepKey nextStepKey) {
         StepKey preActionBranchingKey = new StepKey(phase, NAME, CONDITIONAL_SKIP_ACTION_STEP);

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleType.java

@@ -290,6 +290,7 @@ public void validate(Collection<Phase> phases) {
         }
 
         validateActionsFollowingSearchableSnapshot(phases);
+        validateAllSearchableSnapshotActionsUseSameRepository(phases);
     }
 
     static void validateActionsFollowingSearchableSnapshot(Collection<Phase> phases) {
@@ -313,6 +314,22 @@ static void validateActionsFollowingSearchableSnapshot(Collection<Phase> phases)
         }
     }
 
+    static void validateAllSearchableSnapshotActionsUseSameRepository(Collection<Phase> phases) {
+        Set<String> allRepos = phases.stream()
+            .flatMap(phase -> phase.getActions().entrySet().stream())
+            .filter(e -> e.getKey().equals(SearchableSnapshotAction.NAME))
+            .map(Map.Entry::getValue)
+            .map(action -> (SearchableSnapshotAction) action)
+            .map(SearchableSnapshotAction::getSnapshotRepository)
+            .collect(Collectors.toSet());
+
+        if (allRepos.size() > 1) {
+            throw new IllegalArgumentException("policy specifies [" + SearchableSnapshotAction.NAME +
+                "] action multiple times with differing repositories " + allRepos +
+                ", the same repository must be used for all searchable snapshot actions");
+        }
+    }
+
     private static boolean definesAllocationRules(AllocateAction action) {
         return action.getRequire().isEmpty() == false || action.getInclude().isEmpty() == false || action.getExclude().isEmpty() == false;
     }

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/LifecyclePolicyTests.java

@@ -30,6 +30,7 @@
 import java.util.Set;
 import java.util.function.Function;
 
+import static org.elasticsearch.xpack.core.ilm.SearchableSnapshotActionTests.randomStorageType;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.mockito.Mockito.mock;
@@ -207,7 +208,7 @@ private static Function<String, LifecycleAction> getNameToActionFunction() {
                     case UnfollowAction.NAME:
                         return new UnfollowAction();
                     case SearchableSnapshotAction.NAME:
-                        return new SearchableSnapshotAction(randomAlphaOfLengthBetween(1, 10));
+                        return new SearchableSnapshotAction("repo", randomBoolean(), randomStorageType());
                     case MigrateAction.NAME:
                         return new MigrateAction(false);
                     case RollupILMAction.NAME:

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java

@@ -39,6 +39,7 @@
 import static org.elasticsearch.xpack.core.ilm.TimeseriesLifecycleType.VALID_PHASES;
 import static org.elasticsearch.xpack.core.ilm.TimeseriesLifecycleType.VALID_WARM_ACTIONS;
 import static org.elasticsearch.xpack.core.ilm.TimeseriesLifecycleType.WARM_PHASE;
+import static org.elasticsearch.xpack.core.ilm.TimeseriesLifecycleType.validateAllSearchableSnapshotActionsUseSameRepository;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
@@ -598,6 +599,40 @@ public void testShouldMigrateDataToTiers() {
         }
     }
 
+    public void testValidatingSearchableSnapshotRepos() {
+        Map<String, LifecycleAction> hotActions = new HashMap<>();
+        Map<String, LifecycleAction> coldActions = new HashMap<>();
+        Map<String, LifecycleAction> frozenActions = new HashMap<>();
+
+        {
+            hotActions.put(SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo1", randomBoolean(), null));
+            coldActions.put(SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo1", randomBoolean(), null));
+            frozenActions.put(SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo1", randomBoolean(), null));
+
+            Phase hotPhase = new Phase(HOT_PHASE, TimeValue.ZERO, hotActions);
+            Phase coldPhase = new Phase(HOT_PHASE, TimeValue.ZERO, coldActions);
+            Phase frozenPhase = new Phase(HOT_PHASE, TimeValue.ZERO, frozenActions);
+
+            validateAllSearchableSnapshotActionsUseSameRepository(Arrays.asList(hotPhase, coldPhase, frozenPhase));
+        }
+
+        {
+            hotActions.put(SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo1", randomBoolean(), null));
+            coldActions.put(SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo2", randomBoolean(), null));
+            frozenActions.put(SearchableSnapshotAction.NAME, new SearchableSnapshotAction("repo1", randomBoolean(), null));
+
+            Phase hotPhase = new Phase(HOT_PHASE, TimeValue.ZERO, hotActions);
+            Phase coldPhase = new Phase(HOT_PHASE, TimeValue.ZERO, coldActions);
+            Phase frozenPhase = new Phase(HOT_PHASE, TimeValue.ZERO, frozenActions);
+
+            IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
+                () -> validateAllSearchableSnapshotActionsUseSameRepository(Arrays.asList(hotPhase, coldPhase, frozenPhase)));
+            assertThat(e.getMessage(), containsString("policy specifies [searchable_snapshot]" +
+                " action multiple times with differing repositories [repo2, repo1]," +
+                " the same repository must be used for all searchable snapshot actions"));
+        }
+    }
+
     private void assertNextActionName(String phaseName, String currentAction, String expectedNextAction, String... availableActionNames) {
         Map<String, LifecycleAction> availableActions = convertActionNamesToActions(availableActionNames);
         Phase phase = new Phase(phaseName, TimeValue.ZERO, availableActions);

x-pack/plugin/ilm/qa/multi-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/actions/SearchableSnapshotActionIT.java

@@ -57,6 +57,7 @@
 import static org.elasticsearch.xpack.TimeSeriesRestDriver.getStepKeyForIndex;
 import static org.elasticsearch.xpack.TimeSeriesRestDriver.indexDocument;
 import static org.elasticsearch.xpack.TimeSeriesRestDriver.rolloverMaxOneDocCondition;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.greaterThan;
 import static org.hamcrest.Matchers.is;
@@ -378,6 +379,7 @@ public void testIdenticalSearchableSnapshotActionIsNoop() throws Exception {
             .put(LifecycleSettings.LIFECYCLE_NAME, policy)
             .build());
         ensureGreen(index);
+        indexDocument(client(), index, true);
 
         final String searchableSnapMountedIndexName = (storage == MountSearchableSnapshotRequest.Storage.FULL_COPY ?
             SearchableSnapshotAction.FULL_RESTORED_INDEX_PREFIX : SearchableSnapshotAction.PARTIAL_RESTORED_INDEX_PREFIX) + index;
@@ -401,6 +403,10 @@ public void testIdenticalSearchableSnapshotActionIsNoop() throws Exception {
         }
         assertThat("expected to have only one snapshot, but got: " + responseMap,
             ((List<Object>) responseMap.get("snapshots")).size(), equalTo(1));
+
+        Request hitCount = new Request("GET", "/" + searchableSnapMountedIndexName + "/_count");
+        Map<String, Object> count = entityAsMap(client().performRequest(hitCount));
+        assertThat("expected a single document but got: " + count, (int) count.get("count"), equalTo(1));
     }
 
     @SuppressWarnings("unchecked")
@@ -421,7 +427,7 @@ public void testConvertingSearchableSnapshotFromFullToPartial() throws Exception
             .put(LifecycleSettings.LIFECYCLE_NAME, policy)
             .build());
         ensureGreen(index);
-        indexDocument(client(), index);
+        indexDocument(client(), index, true);
 
         final String searchableSnapMountedIndexName = SearchableSnapshotAction.PARTIAL_RESTORED_INDEX_PREFIX +
             SearchableSnapshotAction.FULL_RESTORED_INDEX_PREFIX + index;
@@ -445,6 +451,10 @@ public void testConvertingSearchableSnapshotFromFullToPartial() throws Exception
         }
         assertThat("expected to have only one snapshot, but got: " + responseMap,
             ((List<Object>) responseMap.get("snapshots")).size(), equalTo(1));
+
+        Request hitCount = new Request("GET", "/" + searchableSnapMountedIndexName + "/_count");
+        Map<String, Object> count = entityAsMap(client().performRequest(hitCount));
+        assertThat("expected a single document but got: " + count, (int) count.get("count"), equalTo(1));
     }
 
     @SuppressWarnings("unchecked")
@@ -465,7 +475,7 @@ public void testConvertingPartialSearchableSnapshotIntoFull() throws Exception {
             .put(LifecycleSettings.LIFECYCLE_NAME, policy)
             .build());
         ensureGreen(index);
-        indexDocument(client(), index);
+        indexDocument(client(), index, true);
 
         final String searchableSnapMountedIndexName = SearchableSnapshotAction.FULL_RESTORED_INDEX_PREFIX +
             SearchableSnapshotAction.PARTIAL_RESTORED_INDEX_PREFIX + index;
@@ -489,71 +499,28 @@ public void testConvertingPartialSearchableSnapshotIntoFull() throws Exception {
         }
         assertThat("expected to have only one snapshot, but got: " + responseMap,
                     ((List<Object>) responseMap.get("snapshots")).size(), equalTo(1));
+
+        Request hitCount = new Request("GET", "/" + searchableSnapMountedIndexName + "/_count");
+        Map<String, Object> count = entityAsMap(client().performRequest(hitCount));
+        assertThat("expected a single document but got: " + count, (int) count.get("count"), equalTo(1));
     }
 
-    @SuppressWarnings("unchecked")
-    @AwaitsFix(bugUrl = "functionality not yet implemented")
-    public void testSecondSearchableSnapshotChangesRepo() throws Exception {
-        String index = "myindex-" + randomAlphaOfLength(4).toLowerCase(Locale.ROOT);
+    public void testSecondSearchableSnapshotUsingDifferentRepoThrows() throws Exception {
         String secondRepo = randomAlphaOfLengthBetween(10, 20);
         createSnapshotRepo(client(), snapshotRepo, randomBoolean());
         createSnapshotRepo(client(), secondRepo, randomBoolean());
-        createPolicy(client(), policy, null, null,
-            new Phase("cold", TimeValue.ZERO,
-                singletonMap(SearchableSnapshotAction.NAME, new SearchableSnapshotAction(snapshotRepo, randomBoolean(),
-                    MountSearchableSnapshotRequest.Storage.FULL_COPY))),
-            new Phase("frozen", TimeValue.ZERO,
-                singletonMap(SearchableSnapshotAction.NAME, new SearchableSnapshotAction(secondRepo, randomBoolean(),
-                    MountSearchableSnapshotRequest.Storage.SHARED_CACHE))),
-            null
-        );
-
-        createIndex(index, Settings.builder()
-            .put(LifecycleSettings.LIFECYCLE_NAME, policy)
-            .build());
-        ensureGreen(index);
-        indexDocument(client(), index);
-
-        final String searchableSnapMountedIndexName = SearchableSnapshotAction.PARTIAL_RESTORED_INDEX_PREFIX +
-            SearchableSnapshotAction.FULL_RESTORED_INDEX_PREFIX + index;
-
-        assertBusy(() -> {
-            logger.info("--> waiting for [{}] to exist...", searchableSnapMountedIndexName);
-            assertTrue(indexExists(searchableSnapMountedIndexName));
-        }, 30, TimeUnit.SECONDS);
-
-        assertBusy(() -> {
-            Step.StepKey stepKeyForIndex = getStepKeyForIndex(client(), searchableSnapMountedIndexName);
-            assertThat(stepKeyForIndex.getPhase(), is("frozen"));
-            assertThat(stepKeyForIndex.getName(), is(PhaseCompleteStep.NAME));
-        }, 30, TimeUnit.SECONDS);
-
-        // Check first repo has exactly 1 snapshot
-        {
-            Request getSnaps = new Request("GET", "/_snapshot/" + snapshotRepo + "/_all");
-            Response response = client().performRequest(getSnaps);
-            Map<String, Object> responseMap;
-            try (InputStream is = response.getEntity().getContent()) {
-                responseMap = XContentHelper.convertToMap(XContentType.JSON.xContent(), is, true);
-            }
-            assertThat("expected to have only one snapshot, but got: " + responseMap,
-                ((List<Map<String, Object>>)
-                    ((Map<String, Object>)
-                        ((List<Object>) responseMap.get("responses")).get(0)).get("snapshots")).size(), equalTo(1));
-        }
-
-        // Check second repo has exactly 1 snapshot
-        {
-            Request getSnaps = new Request("GET", "/_snapshot/" + secondRepo + "/_all");
-            Response response = client().performRequest(getSnaps);
-            Map<String, Object> responseMap;
-            try (InputStream is = response.getEntity().getContent()) {
-                responseMap = XContentHelper.convertToMap(XContentType.JSON.xContent(), is, true);
-            }
-            assertThat("expected to have only one snapshot, but got: " + responseMap,
-                ((List<Map<String, Object>>)
-                    ((Map<String, Object>)
-                        ((List<Object>) responseMap.get("responses")).get(0)).get("snapshots")).size(), equalTo(1));
-        }
+        IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () ->
+            createPolicy(client(), policy, null, null,
+                new Phase("cold", TimeValue.ZERO,
+                    singletonMap(SearchableSnapshotAction.NAME, new SearchableSnapshotAction(snapshotRepo, randomBoolean(),
+                        MountSearchableSnapshotRequest.Storage.FULL_COPY))),
+                new Phase("frozen", TimeValue.ZERO,
+                    singletonMap(SearchableSnapshotAction.NAME, new SearchableSnapshotAction(secondRepo, randomBoolean(),
+                        MountSearchableSnapshotRequest.Storage.SHARED_CACHE))),
+                null
+            ));
+
+        assertThat(e.getMessage(),
+            containsString("policy specifies [searchable_snapshot] action multiple times with differing repositories"));
     }
 }