From b6bdc2ea9a43bf1b481b84dfb4a559d79759f16e Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Wed, 25 Apr 2018 09:15:33 +0200 Subject: [PATCH 1/4] Fix NullPointerException when creating or deleting a snapshot A NullPointerException is thrown when trying to create or delete a snapshot in a repository that has been used by a pre-5.5 cluster and a post-5.5 cluster. The way snapshots are formatted in the repository snapshots index file changed in #24477 and it can cause a NPE if a very specific set of operations is written in the repository. --- docs/CHANGELOG.asciidoc | 2 + docs/reference/modules/snapshots.asciidoc | 4 +- .../TransportSnapshotsStatusAction.java | 6 +-- .../repositories/RepositoryData.java | 40 ++++++++------- .../snapshots/SnapshotsService.java | 3 +- .../repositories/RepositoryDataTests.java | 49 ++++++++++++++++++- 6 files changed, 79 insertions(+), 25 deletions(-) diff --git a/docs/CHANGELOG.asciidoc b/docs/CHANGELOG.asciidoc index fde295b56ba72..615b4c0ab1876 100644 --- a/docs/CHANGELOG.asciidoc +++ b/docs/CHANGELOG.asciidoc @@ -24,6 +24,8 @@ === Bug Fixes +Fix NullPointerException when creating or deleting a snapshot ({pull}TODO[#TODO]) + === Regressions === Known Issues diff --git a/docs/reference/modules/snapshots.asciidoc b/docs/reference/modules/snapshots.asciidoc index ea3f99debb94e..7a8bfdd8944bb 100644 --- a/docs/reference/modules/snapshots.asciidoc +++ b/docs/reference/modules/snapshots.asciidoc @@ -44,9 +44,9 @@ If you register same snapshot repository with multiple clusters, only one cluster should have write access to the repository. All other clusters connected to that repository should set the repository to `readonly` mode. -NOTE: The snapshot format can change across major versions, so if you have +IMPORTANT: The snapshot format can change across major versions, so if you have clusters on different major versions trying to write the same repository, -new snapshots written by one version will not be visible to the other. While +snapshots written by one version may not be visible to the other. While setting the repository to `readonly` on all but one of the clusters should work with multiple clusters differing by one major version, it is not a supported configuration. diff --git a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/TransportSnapshotsStatusAction.java b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/TransportSnapshotsStatusAction.java index dc13c8dab5188..949918f88a10a 100644 --- a/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/TransportSnapshotsStatusAction.java +++ b/server/src/main/java/org/elasticsearch/action/admin/cluster/snapshots/status/TransportSnapshotsStatusAction.java @@ -230,9 +230,9 @@ private SnapshotsStatusResponse buildResponse(SnapshotsStatusRequest request, Li SnapshotInfo snapshotInfo = snapshotsService.snapshot(repositoryName, snapshotId); List shardStatusBuilder = new ArrayList<>(); if (snapshotInfo.state().completed()) { - Map shardStatues = - snapshotsService.snapshotShards(request.repository(), snapshotInfo); - for (Map.Entry shardStatus : shardStatues.entrySet()) { + Map shardStatuses = + snapshotsService.snapshotShards(repositoryName, repositoryData, snapshotInfo); + for (Map.Entry shardStatus : shardStatuses.entrySet()) { IndexShardSnapshotStatus.Copy lastSnapshotStatus = shardStatus.getValue().asCopy(); shardStatusBuilder.add(new SnapshotIndexShardStatus(shardStatus.getKey(), lastSnapshotStatus)); } diff --git a/server/src/main/java/org/elasticsearch/repositories/RepositoryData.java b/server/src/main/java/org/elasticsearch/repositories/RepositoryData.java index 102bc5a5f0524..1025cd4de8607 100644 --- a/server/src/main/java/org/elasticsearch/repositories/RepositoryData.java +++ b/server/src/main/java/org/elasticsearch/repositories/RepositoryData.java @@ -230,13 +230,6 @@ public Set getSnapshots(final IndexId indexId) { return snapshotIds; } - /** - * Initializes the indices in the repository metadata; returns a new instance. - */ - public RepositoryData initIndices(final Map> indexSnapshots) { - return new RepositoryData(genId, snapshotIds, snapshotStates, indexSnapshots, incompatibleSnapshotIds); - } - @Override public boolean equals(Object obj) { if (this == obj) { @@ -352,9 +345,10 @@ public XContentBuilder snapshotsToXContent(final XContentBuilder builder, final * Reads an instance of {@link RepositoryData} from x-content, loading the snapshots and indices metadata. */ public static RepositoryData snapshotsFromXContent(final XContentParser parser, long genId) throws IOException { - Map snapshots = new HashMap<>(); - Map snapshotStates = new HashMap<>(); - Map> indexSnapshots = new HashMap<>(); + final Map snapshots = new HashMap<>(); + final Map snapshotStates = new HashMap<>(); + final Map> indexSnapshots = new HashMap<>(); + if (parser.nextToken() == XContentParser.Token.START_OBJECT) { while (parser.nextToken() == XContentParser.Token.FIELD_NAME) { String field = parser.currentName(); @@ -397,17 +391,18 @@ public static RepositoryData snapshotsFromXContent(final XContentParser parser, throw new ElasticsearchParseException("start object expected [indices]"); } while (parser.nextToken() != XContentParser.Token.END_OBJECT) { - String indexName = parser.currentName(); - String indexId = null; - Set snapshotIds = new LinkedHashSet<>(); + final String indexName = parser.currentName(); + final Set snapshotIds = new LinkedHashSet<>(); + + IndexId indexId = null; if (parser.nextToken() != XContentParser.Token.START_OBJECT) { throw new ElasticsearchParseException("start object expected index[" + indexName + "]"); } while (parser.nextToken() != XContentParser.Token.END_OBJECT) { - String indexMetaFieldName = parser.currentName(); + final String indexMetaFieldName = parser.currentName(); parser.nextToken(); if (INDEX_ID.equals(indexMetaFieldName)) { - indexId = parser.text(); + indexId = new IndexId(indexName, parser.text()); } else if (SNAPSHOTS.equals(indexMetaFieldName)) { if (parser.currentToken() != XContentParser.Token.START_ARRAY) { throw new ElasticsearchParseException("start array expected [snapshots]"); @@ -428,12 +423,23 @@ public static RepositoryData snapshotsFromXContent(final XContentParser parser, // since we already have the name/uuid combo in the snapshots array uuid = parser.text(); } - snapshotIds.add(snapshots.get(uuid)); + if (uuid != null) { + SnapshotId snapshotId = snapshots.get(uuid); + if (snapshotId != null) { + snapshotIds.add(snapshotId); + } else { + // A snapshotted index references a snapshot which does not exist in + // the list of snapshots. This can happen when multiple clusters in + // different versions create or delete snapshot in the same repository. + throw new ElasticsearchParseException("Unknown snapshot uuid [" + uuid + + "] for index " + indexId); + } + } } } } assert indexId != null; - indexSnapshots.put(new IndexId(indexName, indexId), snapshotIds); + indexSnapshots.put(indexId, snapshotIds); } } else { throw new ElasticsearchParseException("unknown field name [" + field + "]"); diff --git a/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java b/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java index daf5c78b78cee..5665680fd9c57 100644 --- a/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java +++ b/server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java @@ -592,10 +592,9 @@ public List currentSnapshots(final String repository, * @return map of shard id to snapshot status */ public Map snapshotShards(final String repositoryName, + final RepositoryData repositoryData, final SnapshotInfo snapshotInfo) throws IOException { final Repository repository = repositoriesService.repository(repositoryName); - final RepositoryData repositoryData = repository.getRepositoryData(); - final Map shardStatus = new HashMap<>(); for (String index : snapshotInfo.indices()) { IndexId indexId = repositoryData.resolveIndexId(index); diff --git a/server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java b/server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java index 8c1e242b3262f..8dd80806af63e 100644 --- a/server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java +++ b/server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java @@ -19,11 +19,14 @@ package org.elasticsearch.repositories; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.UUIDs; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContent; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.snapshots.SnapshotId; import org.elasticsearch.snapshots.SnapshotState; @@ -40,6 +43,7 @@ import java.util.Set; import static org.elasticsearch.repositories.RepositoryData.EMPTY_REPO_GEN; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; /** @@ -101,15 +105,18 @@ public void testAddSnapshots() { public void testInitIndices() { final int numSnapshots = randomIntBetween(1, 30); final Map snapshotIds = new HashMap<>(numSnapshots); + final Map snapshotStates = new HashMap<>(numSnapshots); for (int i = 0; i < numSnapshots; i++) { final SnapshotId snapshotId = new SnapshotId(randomAlphaOfLength(8), UUIDs.randomBase64UUID()); snapshotIds.put(snapshotId.getUUID(), snapshotId); + snapshotStates.put(snapshotId.getUUID(), randomFrom(SnapshotState.values())); } RepositoryData repositoryData = new RepositoryData(EMPTY_REPO_GEN, snapshotIds, Collections.emptyMap(), Collections.emptyMap(), Collections.emptyList()); // test that initializing indices works Map> indices = randomIndices(snapshotIds); - RepositoryData newRepoData = repositoryData.initIndices(indices); + RepositoryData newRepoData = new RepositoryData(repositoryData.getGenId(), snapshotIds, snapshotStates, indices, + new ArrayList<>(repositoryData.getIncompatibleSnapshotIds())); List expected = new ArrayList<>(repositoryData.getSnapshotIds()); Collections.sort(expected); List actual = new ArrayList<>(newRepoData.getSnapshotIds()); @@ -153,6 +160,46 @@ public void testGetSnapshotState() { assertNull(repositoryData.getSnapshotState(new SnapshotId(randomAlphaOfLength(8), UUIDs.randomBase64UUID()))); } + public void testUnknownSnapshot() throws IOException { + final XContent xContent = randomFrom(XContentType.values()).xContent(); + final RepositoryData repositoryData = generateRandomRepoData(); + + XContentBuilder builder = XContentBuilder.builder(xContent); + repositoryData.snapshotsToXContent(builder, ToXContent.EMPTY_PARAMS); + RepositoryData parsedRepositoryData = RepositoryData.snapshotsFromXContent(createParser(builder), repositoryData.getGenId()); + assertEquals(repositoryData, parsedRepositoryData); + + Map snapshotIds = new HashMap<>(); + Map snapshotStates = new HashMap<>(); + for (SnapshotId snapshotId : parsedRepositoryData.getSnapshotIds()) { + snapshotIds.put(snapshotId.getUUID(), snapshotId); + snapshotStates.put(snapshotId.getUUID(), parsedRepositoryData.getSnapshotState(snapshotId)); + } + + final IndexId corruptedIndexId = randomFrom(parsedRepositoryData.getIndices().values()); + + Map> indexSnapshots = new HashMap<>(); + for (Map.Entry snapshottedIndex : parsedRepositoryData.getIndices().entrySet()) { + IndexId indexId = snapshottedIndex.getValue(); + Set snapshotsIds = new LinkedHashSet<>(parsedRepositoryData.getSnapshots(indexId)); + if (corruptedIndexId.equals(indexId)) { + snapshotsIds.add(new SnapshotId("_uuid", "_does_not_exist")); + } + indexSnapshots.put(indexId, snapshotsIds); + } + assertNotNull(corruptedIndexId); + + RepositoryData corruptedRepositoryData = new RepositoryData(parsedRepositoryData.getGenId(), snapshotIds, snapshotStates, + indexSnapshots, new ArrayList<>(parsedRepositoryData.getIncompatibleSnapshotIds())); + + final XContentBuilder corruptedBuilder = XContentBuilder.builder(xContent); + corruptedRepositoryData.snapshotsToXContent(corruptedBuilder, ToXContent.EMPTY_PARAMS); + + ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () -> + RepositoryData.snapshotsFromXContent(createParser(corruptedBuilder), corruptedRepositoryData.getGenId())); + assertThat(e.getMessage(), equalTo("Unknown snapshot uuid [_does_not_exist] for index " + corruptedIndexId)); + } + public static RepositoryData generateRandomRepoData() { final int numIndices = randomIntBetween(1, 30); final List indices = new ArrayList<>(numIndices); From ff7234752e40f474194b25e981631648ef631066 Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Wed, 25 Apr 2018 18:45:05 +0200 Subject: [PATCH 2/4] Update PR number --- docs/CHANGELOG.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/CHANGELOG.asciidoc b/docs/CHANGELOG.asciidoc index 615b4c0ab1876..4fd61bfce0434 100644 --- a/docs/CHANGELOG.asciidoc +++ b/docs/CHANGELOG.asciidoc @@ -24,7 +24,7 @@ === Bug Fixes -Fix NullPointerException when creating or deleting a snapshot ({pull}TODO[#TODO]) +Fix NullPointerException when creating or deleting a snapshot ({pull}30140[#30140]) === Regressions From 46d474879b48de35b445d3dec43c27003c336091 Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Thu, 26 Apr 2018 13:36:09 +0200 Subject: [PATCH 3/4] Apply feedback --- docs/CHANGELOG.asciidoc | 3 +- docs/reference/modules/snapshots.asciidoc | 10 ++--- .../repositories/RepositoryData.java | 21 +++++----- .../repositories/RepositoryDataTests.java | 42 ++++++++++++++++++- 4 files changed, 57 insertions(+), 19 deletions(-) diff --git a/docs/CHANGELOG.asciidoc b/docs/CHANGELOG.asciidoc index 4fd61bfce0434..f4de4e23ece72 100644 --- a/docs/CHANGELOG.asciidoc +++ b/docs/CHANGELOG.asciidoc @@ -24,7 +24,8 @@ === Bug Fixes -Fix NullPointerException when creating or deleting a snapshot ({pull}30140[#30140]) +Fix NullPointerException when creating or deleting a snapshot on a repository that has been +written to by an older Elasticsearch after writing to it with a newer Elasticsearch version. ({pull}30140[#30140]) === Regressions diff --git a/docs/reference/modules/snapshots.asciidoc b/docs/reference/modules/snapshots.asciidoc index 7a8bfdd8944bb..693d537d732c1 100644 --- a/docs/reference/modules/snapshots.asciidoc +++ b/docs/reference/modules/snapshots.asciidoc @@ -45,11 +45,11 @@ one cluster should have write access to the repository. All other clusters connected to that repository should set the repository to `readonly` mode. IMPORTANT: The snapshot format can change across major versions, so if you have -clusters on different major versions trying to write the same repository, -snapshots written by one version may not be visible to the other. While -setting the repository to `readonly` on all but one of the clusters should work -with multiple clusters differing by one major version, it is not a supported -configuration. +clusters on different versions trying to write the same repository, snapshots +written by one version may not be visible to the other and the repository could +be corrupted. While setting the repository to `readonly` on all but one of the +clusters should work with multiple clusters differing by one major version, it +is not a supported configuration. [source,js] ----------------------------------- diff --git a/server/src/main/java/org/elasticsearch/repositories/RepositoryData.java b/server/src/main/java/org/elasticsearch/repositories/RepositoryData.java index 1025cd4de8607..7a8d8327d5e3a 100644 --- a/server/src/main/java/org/elasticsearch/repositories/RepositoryData.java +++ b/server/src/main/java/org/elasticsearch/repositories/RepositoryData.java @@ -423,17 +423,16 @@ public static RepositoryData snapshotsFromXContent(final XContentParser parser, // since we already have the name/uuid combo in the snapshots array uuid = parser.text(); } - if (uuid != null) { - SnapshotId snapshotId = snapshots.get(uuid); - if (snapshotId != null) { - snapshotIds.add(snapshotId); - } else { - // A snapshotted index references a snapshot which does not exist in - // the list of snapshots. This can happen when multiple clusters in - // different versions create or delete snapshot in the same repository. - throw new ElasticsearchParseException("Unknown snapshot uuid [" + uuid - + "] for index " + indexId); - } + + SnapshotId snapshotId = snapshots.get(uuid); + if (snapshotId != null) { + snapshotIds.add(snapshotId); + } else { + // A snapshotted index references a snapshot which does not exist in + // the list of snapshots. This can happen when multiple clusters in + // different versions create or delete snapshot in the same repository. + throw new ElasticsearchParseException("Detected a corrupted repository, index " + indexId + + " references an unknown snapshot uuid [" + uuid + "]"); } } } diff --git a/server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java b/server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java index 8dd80806af63e..db8aa615c1440 100644 --- a/server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java +++ b/server/src/test/java/org/elasticsearch/repositories/RepositoryDataTests.java @@ -42,7 +42,10 @@ import java.util.Map; import java.util.Set; +import static java.util.Collections.emptySet; +import static java.util.Collections.singleton; import static org.elasticsearch.repositories.RepositoryData.EMPTY_REPO_GEN; +import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.greaterThan; @@ -160,7 +163,7 @@ public void testGetSnapshotState() { assertNull(repositoryData.getSnapshotState(new SnapshotId(randomAlphaOfLength(8), UUIDs.randomBase64UUID()))); } - public void testUnknownSnapshot() throws IOException { + public void testIndexThatReferencesAnUnknownSnapshot() throws IOException { final XContent xContent = randomFrom(XContentType.values()).xContent(); final RepositoryData repositoryData = generateRandomRepoData(); @@ -197,7 +200,42 @@ public void testUnknownSnapshot() throws IOException { ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () -> RepositoryData.snapshotsFromXContent(createParser(corruptedBuilder), corruptedRepositoryData.getGenId())); - assertThat(e.getMessage(), equalTo("Unknown snapshot uuid [_does_not_exist] for index " + corruptedIndexId)); + assertThat(e.getMessage(), equalTo("Detected a corrupted repository, index " + corruptedIndexId + " references an unknown " + + "snapshot uuid [_does_not_exist]")); + } + + public void testIndexThatReferenceANullSnapshot() throws IOException { + final XContentBuilder builder = XContentBuilder.builder(randomFrom(XContentType.JSON).xContent()); + builder.startObject(); + { + builder.startArray("snapshots"); + builder.value(new SnapshotId("_name", "_uuid")); + builder.endArray(); + + builder.startObject("indices"); + { + builder.startObject("docs"); + { + builder.field("id", "_id"); + builder.startArray("snapshots"); + { + builder.startObject(); + if (randomBoolean()) { + builder.field("name", "_name"); + } + builder.endObject(); + } + builder.endArray(); + } + builder.endObject(); + } + builder.endObject(); + } + builder.endObject(); + + ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () -> + RepositoryData.snapshotsFromXContent(createParser(builder), randomNonNegativeLong())); + assertThat(e.getMessage(), equalTo("Detected a corrupted repository, index [docs/_id] references an unknown snapshot uuid [null]")); } public static RepositoryData generateRandomRepoData() { From 2177da2c6fe5c23b51b7e1e49d736191e2ba1d44 Mon Sep 17 00:00:00 2001 From: Tanguy Leroux Date: Fri, 27 Apr 2018 09:37:31 +0200 Subject: [PATCH 4/4] Update Changelog --- docs/CHANGELOG.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/CHANGELOG.asciidoc b/docs/CHANGELOG.asciidoc index f4de4e23ece72..6fb989de7c1be 100644 --- a/docs/CHANGELOG.asciidoc +++ b/docs/CHANGELOG.asciidoc @@ -24,7 +24,7 @@ === Bug Fixes -Fix NullPointerException when creating or deleting a snapshot on a repository that has been +Fail snapshot operations early when creating or deleting a snapshot on a repository that has been written to by an older Elasticsearch after writing to it with a newer Elasticsearch version. ({pull}30140[#30140]) === Regressions