honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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.
This commit is contained in:
Henning Andersen 2020-01-06 13:24:07 +01:00 committed by Henning Andersen
parent 5533e1172c
commit ec0ec61881
4 changed files with 48 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java
View File

@ -1043,7 +1043,7 @@ public class InternalEngine extends Engine {
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);
@ -1376,7 +1376,7 @@ public class InternalEngine extends Engine {
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);

38
server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java
View File

@ -143,6 +143,7 @@ import org.elasticsearch.test.IndexSettingsModule;
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;
@ -1979,7 +1980,7 @@ public class InternalEngineTests extends EngineTestCase {
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 + ")" : "",
@ -2002,7 +2003,7 @@ public class InternalEngineTests extends EngineTestCase {
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));
@ -2037,8 +2038,9 @@ public class InternalEngineTests extends EngineTestCase {
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 {
@ -4312,6 +4314,36 @@ public class InternalEngineTests extends EngineTestCase {
}
}
/**
* 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:
* <ul>
* <li>Guessing the deleted seqNo makes the operation succeed</li>
* <li>Providing any other seqNo leaks info that the doc was deleted (and its SeqNo)</li>
* </ul>
*/
public void testVersionConflictIgnoreDeletedDoc() throws IOException {
ParsedDocument doc = testParsedDocument("1", null, testDocument(),
new BytesArray("{}".getBytes(Charset.defaultCharset())), null);
engine.delete(new Engine.Delete("test", "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("test", "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.

21
server/src/test/java/org/elasticsearch/indices/settings/UpdateSettingsIT.java
View File

@ -21,13 +21,13 @@ package org.elasticsearch.indices.settings;
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,14 @@ public class UpdateSettingsIT extends ESIntegTestCase {
public void testEngineGCDeletesSetting() throws Exception {
createIndex("test");
client().prepareIndex("test", "type", "1").setSource("f", 1).get();
DeleteResponse response = client().prepareDelete("test", "type", "1").get();
long seqNo = response.getSeqNo();
long primaryTerm = response.getPrimaryTerm();
// delete is still in cache this should work
client().prepareIndex("test", "type", "1").setSource("f", 2).setIfSeqNo(seqNo).setIfPrimaryTerm(primaryTerm).get();
client().prepareIndex("test", "type", "1").setSource("f", 1).setVersionType(VersionType.EXTERNAL).setVersion(1).get();
client().prepareDelete("test", "type", "1").setVersionType(VersionType.EXTERNAL).setVersion(2).get();
// delete is still in cache this should fail
assertThrows(client().prepareIndex("test", "type", "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", "type", "1").get();
seqNo = response.getSeqNo();
client().prepareDelete("test", "type", "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 +464,8 @@ public class UpdateSettingsIT extends ESIntegTestCase {
assertBusy(() -> assertThat(threadPool.relativeTimeInMillis(), greaterThan(startTime)));
}
// delete is should not be in cache
assertThrows(client().prepareIndex("test", "type", "1").setSource("f", 3).setIfSeqNo(seqNo).setIfPrimaryTerm(primaryTerm),
VersionConflictEngineException.class);
// delete should not be in cache
client().prepareIndex("test", "type", "1").setSource("f", 2).setVersionType(VersionType.EXTERNAL).setVersion(1);
}
public void testUpdateSettingsWithBlocks() {

9
server/src/test/java/org/elasticsearch/versioning/SimpleVersioningIT.java
View File

@ -283,13 +283,8 @@ public class SimpleVersioningIT extends ESIntegTestCase {
assertThrows(client().prepareDelete("test", "type", "1").setIfSeqNo(3).setIfPrimaryTerm(12), VersionConflictEngineException.class);
assertThrows(client().prepareDelete("test", "type", "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", "type", "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", "type", "1").setIfSeqNo(2).setIfPrimaryTerm(1), VersionConflictEngineException.class);
}
public void testSimpleVersioningWithFlush() throws Exception {