Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent snapshots to be mounted as system indices #61517

Merged
merged 5 commits into from
Aug 31, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.IndexNotFoundException;
import org.elasticsearch.indices.InvalidIndexNameException;
import org.elasticsearch.license.XPackLicenseState;
import org.elasticsearch.repositories.IndexId;
import org.elasticsearch.repositories.RepositoriesService;
Expand Down Expand Up @@ -132,6 +133,11 @@ protected void masterOperation(
) {
SearchableSnapshots.ensureValidLicense(licenseState);

final String mountedIndexName = request.mountedIndexName();
if (mountedIndexName.charAt(0) == '.') {
Copy link
Member

@jaymode jaymode Aug 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be best to pull in the SystemIndices class since we're in a transport action. We can bind the instance in Node and make it available for guice to inject. Then a simple modification to the the SystemIndices class could be made that adds a method to check the name (currently there is a method that takes an Index object but only looks at the name) against the defined set of system indices.

The primary reason for this is users could have data indices in 7.x that start with a . so we shouldn't break that for them restrict them from being able to use this feature on that data.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also hidden indices that start with a dot (e.g. .watcher-history*), which there's no reason to prevent being backed by a searchable snapshot. Using the SystemIndices class would avoid that issue as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I merged #61540 which adds the method in SystemIndices to check a string name without needing the Index object.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks everybody. I'm waiting for #60522 to be merged and I'll update this PR.

throw new InvalidIndexNameException(mountedIndexName, "system indices cannot be mounted as searchable snapshots");
}

final String repoName = request.repositoryName();
final String snapName = request.snapshotName();
final String indexName = request.snapshotIndexName();
Expand Down Expand Up @@ -168,7 +174,7 @@ protected void masterOperation(
.indices(indexName)
// Always rename it to the desired mounted index name
.renamePattern(".+")
.renameReplacement(request.mountedIndexName())
.renameReplacement(mountedIndexName)
// Pass through index settings, adding the index-level settings required to use searchable snapshots
.indexSettings(
Settings.builder()
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

package org.elasticsearch.xpack.searchablesnapshots;

import org.elasticsearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse;
import org.elasticsearch.action.bulk.BulkRequest;
import org.elasticsearch.action.bulk.BulkResponse;
import org.elasticsearch.action.index.IndexRequest;
import org.elasticsearch.client.Client;
import org.elasticsearch.client.OriginSettingClient;
import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.indices.InvalidIndexNameException;
import org.elasticsearch.indices.SystemIndexDescriptor;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.plugins.SystemIndexPlugin;
import org.elasticsearch.xpack.core.ClientHelper;
import org.elasticsearch.xpack.core.searchablesnapshots.MountSearchableSnapshotAction;
import org.elasticsearch.xpack.core.searchablesnapshots.MountSearchableSnapshotRequest;

import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Locale;

import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;

public class SearchableSnapshotsSystemIndicesIntegTests extends BaseSearchableSnapshotsIntegTestCase {

@Override
protected Collection<Class<? extends Plugin>> nodePlugins() {
final List<Class<? extends Plugin>> plugins = new ArrayList<>(super.nodePlugins());
plugins.add(TestSystemIndexPlugin.class);
return plugins;
}

public void testCannotMountSystemIndex() {
final String systemIndexName = '.' + randomAlphaOfLength(10).toLowerCase(Locale.ROOT);
executeTest(systemIndexName, client());
}

public void testCannotMountSystemIndexWithDescriptor() {
// TODO replace STACK_ORIGIN with searchable snapshot origin
executeTest(TestSystemIndexPlugin.INDEX_NAME, new OriginSettingClient(client(), ClientHelper.STACK_ORIGIN));
}

private void executeTest(final String indexName, final Client client) {
final boolean isHidden = randomBoolean();
assertAcked(
client.admin()
.indices()
.prepareCreate(indexName)
.setSettings(Settings.builder().put(IndexMetadata.SETTING_INDEX_HIDDEN, isHidden).build())
);

final int nbDocs = scaledRandomIntBetween(0, 100);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we could use createAndPopulateIndex instead of manually creating and populating the index?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we can - I pushed 3e173cb

if (nbDocs > 0) {
final BulkRequest bulkRequest = new BulkRequest();
for (int i = 0; i < nbDocs; i++) {
IndexRequest indexRequest = new IndexRequest(indexName);
indexRequest.source("value", i);
bulkRequest.add(indexRequest);
}
final BulkResponse bulkResponse = client.bulk(bulkRequest).actionGet();
assertThat(bulkResponse.hasFailures(), is(false));
}
flushAndRefresh(indexName);
assertHitCount(client.prepareSearch(indexName).get(), nbDocs);

final String repositoryName = randomAlphaOfLength(10).toLowerCase(Locale.ROOT);
createRepo(repositoryName);

final String snapshotName = randomAlphaOfLength(10).toLowerCase(Locale.ROOT);
final CreateSnapshotResponse snapshotResponse = client.admin()
.cluster()
.prepareCreateSnapshot(repositoryName, snapshotName)
.setIndices(indexName)
.setWaitForCompletion(true)
.get();

final int numPrimaries = getNumShards(indexName).numPrimaries;
assertThat(snapshotResponse.getSnapshotInfo().successfulShards(), equalTo(numPrimaries));
assertThat(snapshotResponse.getSnapshotInfo().failedShards(), equalTo(0));

if (randomBoolean()) {
assertAcked(client.admin().indices().prepareClose(indexName));
} else {
assertAcked(client.admin().indices().prepareDelete(indexName));
}

final MountSearchableSnapshotRequest mountRequest = new MountSearchableSnapshotRequest(
indexName,
repositoryName,
snapshotName,
indexName,
Settings.builder().put(IndexMetadata.SETTING_INDEX_HIDDEN, randomBoolean()).build(),
Strings.EMPTY_ARRAY,
true
);

final InvalidIndexNameException exception = expectThrows(
InvalidIndexNameException.class,
() -> client.execute(MountSearchableSnapshotAction.INSTANCE, mountRequest).actionGet()
);
assertThat(exception.getIndex().getName(), equalTo(indexName));
assertThat(exception.getMessage(), containsString("system indices cannot be mounted as searchable snapshots"));
}

public static class TestSystemIndexPlugin extends Plugin implements SystemIndexPlugin {

static final String INDEX_NAME = ".test-system-index";

@Override
public Collection<SystemIndexDescriptor> getSystemIndexDescriptors(Settings settings) {
return List.of(new SystemIndexDescriptor(INDEX_NAME, "System index for [" + getTestClass().getName() + ']'));
}
}
}