From fdd413370ef4401cac20596a18b9c213eae16f65 Mon Sep 17 00:00:00 2001 From: Henning Andersen <33268011+henningandersen@users.noreply.github.com> Date: Mon, 6 Jan 2020 13:24:07 +0100 Subject: [PATCH] Deleted docs disregarded for if_seq_no check (#50526) Previously, as long as a deleted version value was kept as a tombstone, another index or delete operation against the same id would leak that the doc had existed (through seq_no info) or would allow the operation if the client forged the seq_no. Fixed to disregard info on deleted docs when doing seq_no based optimistic concurrency check. --- .../index/engine/InternalEngine.java | 4 +- .../index/engine/InternalEngineTests.java | 38 +++++++++++++++++-- .../indices/settings/UpdateSettingsIT.java | 22 +++++------ .../versioning/SimpleVersioningIT.java | 9 +---- 4 files changed, 49 insertions(+), 24 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java b/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java index a090501ca0de4..0ea50322b173b 100644 --- a/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java +++ b/server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java @@ -1032,7 +1032,7 @@ private IndexingStrategy planIndexingAsPrimary(Index index) throws IOException { currentVersion = versionValue.version; currentNotFoundOrDeleted = versionValue.isDelete(); } - if (index.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO && versionValue == null) { + if (index.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO && currentNotFoundOrDeleted) { final VersionConflictEngineException e = new VersionConflictEngineException(shardId, index.id(), index.getIfSeqNo(), index.getIfPrimaryTerm(), SequenceNumbers.UNASSIGNED_SEQ_NO, SequenceNumbers.UNASSIGNED_PRIMARY_TERM); @@ -1361,7 +1361,7 @@ private DeletionStrategy planDeletionAsPrimary(Delete delete) throws IOException currentlyDeleted = versionValue.isDelete(); } final DeletionStrategy plan; - if (delete.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO && versionValue == null) { + if (delete.getIfSeqNo() != SequenceNumbers.UNASSIGNED_SEQ_NO && currentlyDeleted) { final VersionConflictEngineException e = new VersionConflictEngineException(shardId, delete.id(), delete.getIfSeqNo(), delete.getIfPrimaryTerm(), SequenceNumbers.UNASSIGNED_SEQ_NO, SequenceNumbers.UNASSIGNED_PRIMARY_TERM); plan = DeletionStrategy.skipDueToVersionConflict(e, currentVersion, true); diff --git a/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java b/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java index b9fe4c7dc4d98..c71730b6494f9 100644 --- a/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java +++ b/server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java @@ -138,6 +138,7 @@ import org.elasticsearch.test.VersionUtils; import org.elasticsearch.threadpool.ThreadPool; import org.hamcrest.MatcherAssert; +import org.hamcrest.Matchers; import java.io.Closeable; import java.io.IOException; @@ -1719,7 +1720,7 @@ private int assertOpsOnPrimary(List ops, long currentOpVersion currentTerm.set(currentTerm.get() + 1L); engine.rollTranslogGeneration(); } - final long correctVersion = docDeleted && randomBoolean() ? Versions.MATCH_DELETED : lastOpVersion; + final long correctVersion = docDeleted ? Versions.MATCH_DELETED : lastOpVersion; logger.info("performing [{}]{}{}", op.operationType().name().charAt(0), versionConflict ? " (conflict " + conflictingVersion + ")" : "", @@ -1742,7 +1743,7 @@ private int assertOpsOnPrimary(List ops, long currentOpVersion final Engine.IndexResult result; if (versionedOp) { // TODO: add support for non-existing docs - if (randomBoolean() && lastOpSeqNo != SequenceNumbers.UNASSIGNED_SEQ_NO) { + if (randomBoolean() && lastOpSeqNo != SequenceNumbers.UNASSIGNED_SEQ_NO && docDeleted == false) { result = engine.index(indexWithSeq.apply(lastOpSeqNo, lastOpTerm, index)); } else { result = engine.index(indexWithVersion.apply(correctVersion, index)); @@ -1777,8 +1778,9 @@ private int assertOpsOnPrimary(List ops, long currentOpVersion assertThat(result.getFailure(), instanceOf(VersionConflictEngineException.class)); } else { final Engine.DeleteResult result; + long correctSeqNo = docDeleted ? UNASSIGNED_SEQ_NO : lastOpSeqNo; if (versionedOp && lastOpSeqNo != UNASSIGNED_SEQ_NO && randomBoolean()) { - result = engine.delete(delWithSeq.apply(lastOpSeqNo, lastOpTerm, delete)); + result = engine.delete(delWithSeq.apply(correctSeqNo, lastOpTerm, delete)); } else if (versionedOp) { result = engine.delete(delWithVersion.apply(correctVersion, delete)); } else { @@ -4031,6 +4033,36 @@ public void testOutOfOrderSequenceNumbersWithVersionConflict() throws IOExceptio } } + /** + * Test that we do not leak out information on a deleted doc due to it existing in version map. There are at least 2 cases: + *
    + *
  • Guessing the deleted seqNo makes the operation succeed
    • + *
  • Providing any other seqNo leaks info that the doc was deleted (and its SeqNo)
    • + *
+ */ + public void testVersionConflictIgnoreDeletedDoc() throws IOException { + ParsedDocument doc = testParsedDocument("1", null, testDocument(), + new BytesArray("{}".getBytes(Charset.defaultCharset())), null); + engine.delete(new Engine.Delete("1", newUid("1"), 1)); + for (long seqNo : new long[]{0, 1, randomNonNegativeLong()}) { + assertDeletedVersionConflict(engine.index(new Engine.Index(newUid("1"), doc, UNASSIGNED_SEQ_NO, 1, + Versions.MATCH_ANY, VersionType.INTERNAL, + PRIMARY, randomNonNegativeLong(), IndexRequest.UNSET_AUTO_GENERATED_TIMESTAMP, false, seqNo, 1)), + "update: " + seqNo); + + assertDeletedVersionConflict(engine.delete(new Engine.Delete("1", newUid("1"), UNASSIGNED_SEQ_NO, 1, + Versions.MATCH_ANY, VersionType.INTERNAL, PRIMARY, randomNonNegativeLong(), seqNo, 1)), + "delete: " + seqNo); + } + } + + private void assertDeletedVersionConflict(Engine.Result result, String operation) { + assertNotNull("Must have failure for " + operation, result.getFailure()); + assertThat(operation, result.getFailure(), Matchers.instanceOf(VersionConflictEngineException.class)); + VersionConflictEngineException exception = (VersionConflictEngineException) result.getFailure(); + assertThat(operation, exception.getMessage(), containsString("but no document was found")); + } + /* * This test tests that a no-op does not generate a new sequence number, that no-ops can advance the local checkpoint, and that no-ops * are correctly added to the translog. diff --git a/server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java b/server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java index 3d063ac3690bb..e79c0a15d21c9 100644 --- a/server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java +++ b/server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java @@ -21,13 +21,13 @@ import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse; import org.elasticsearch.action.admin.indices.settings.get.GetSettingsResponse; -import org.elasticsearch.action.delete.DeleteResponse; import org.elasticsearch.cluster.metadata.IndexMetaData; import org.elasticsearch.common.Priority; import org.elasticsearch.common.settings.Setting; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.index.IndexModule; import org.elasticsearch.index.IndexService; +import org.elasticsearch.index.VersionType; import org.elasticsearch.index.engine.VersionConflictEngineException; import org.elasticsearch.indices.IndicesService; import org.elasticsearch.plugins.Plugin; @@ -449,16 +449,15 @@ public void testOpenCloseUpdateSettings() throws Exception { public void testEngineGCDeletesSetting() throws Exception { createIndex("test"); - client().prepareIndex("test").setId("1").setSource("f", 1).get(); - DeleteResponse response = client().prepareDelete("test", "1").get(); - long seqNo = response.getSeqNo(); - long primaryTerm = response.getPrimaryTerm(); - // delete is still in cache this should work - client().prepareIndex("test").setId("1").setSource("f", 2).setIfSeqNo(seqNo).setIfPrimaryTerm(primaryTerm).get(); + client().prepareIndex("test").setId("1").setSource("f", 1).setVersionType(VersionType.EXTERNAL).setVersion(1).get(); + client().prepareDelete("test", "1").setVersionType(VersionType.EXTERNAL).setVersion(2).get(); + // delete is still in cache this should fail + assertThrows(client().prepareIndex("test").setId("1").setSource("f", 3).setVersionType(VersionType.EXTERNAL).setVersion(1), + VersionConflictEngineException.class); + assertAcked(client().admin().indices().prepareUpdateSettings("test").setSettings(Settings.builder().put("index.gc_deletes", 0))); - response = client().prepareDelete("test", "1").get(); - seqNo = response.getSeqNo(); + client().prepareDelete("test", "1").setVersionType(VersionType.EXTERNAL).setVersion(4).get(); // Make sure the time has advanced for InternalEngine#resolveDocVersion() for (ThreadPool threadPool : internalCluster().getInstances(ThreadPool.class)) { @@ -466,9 +465,8 @@ public void testEngineGCDeletesSetting() throws Exception { assertBusy(() -> assertThat(threadPool.relativeTimeInMillis(), greaterThan(startTime))); } - // delete is should not be in cache - assertThrows(client().prepareIndex("test").setId("1").setSource("f", 3).setIfSeqNo(seqNo).setIfPrimaryTerm(primaryTerm), - VersionConflictEngineException.class); + // delete should not be in cache + client().prepareIndex("test").setId("1").setSource("f", 2).setVersionType(VersionType.EXTERNAL).setVersion(1); } public void testUpdateSettingsWithBlocks() { diff --git a/server/src/test/java/org/elasticsearch/versioning/SimpleVersioningIT.java b/server/src/test/java/org/elasticsearch/versioning/SimpleVersioningIT.java index 9acc0c572039a..7709100cca2b7 100644 --- a/server/src/test/java/org/elasticsearch/versioning/SimpleVersioningIT.java +++ b/server/src/test/java/org/elasticsearch/versioning/SimpleVersioningIT.java @@ -283,13 +283,8 @@ public void testCompareAndSet() { assertThrows(client().prepareDelete("test", "1").setIfSeqNo(3).setIfPrimaryTerm(12), VersionConflictEngineException.class); assertThrows(client().prepareDelete("test", "1").setIfSeqNo(1).setIfPrimaryTerm(2), VersionConflictEngineException.class); - - // This is intricate - the object was deleted but a delete transaction was with the right version. We add another one - // and thus the transaction is increased. - deleteResponse = client().prepareDelete("test", "1").setIfSeqNo(2).setIfPrimaryTerm(1).get(); - assertEquals(DocWriteResponse.Result.NOT_FOUND, deleteResponse.getResult()); - assertThat(deleteResponse.getSeqNo(), equalTo(3L)); - assertThat(deleteResponse.getPrimaryTerm(), equalTo(1L)); + // the doc is deleted. Even when we hit the deleted seqNo, a conditional delete should fail. + assertThrows(client().prepareDelete("test", "1").setIfSeqNo(2).setIfPrimaryTerm(1), VersionConflictEngineException.class); } public void testSimpleVersioningWithFlush() throws Exception {