Skip to content

Commit

Permalink
[7.17] Backport DLS changes (#108330)
Browse files Browse the repository at this point in the history 
This commit introduced stricter DLS rules and is a manual backport of #105709 and #105714 
with additional node level settings to optionally disable the stricter DLS rules. 
Since these settings are not present in 8.x the needed deprecation info API entries have also 
been added to help inform any users that may have set these values to remove them before upgrading.
  • Loading branch information
@jakelandis
jakelandis authored Jun 3, 2024
1 parent 7fe654b commit 4e08df5
Show file tree
Hide file tree
Showing 23 changed files with 1,280 additions and 401 deletions.
17 changes: 17 additions & 0 deletions docs/reference/migration/migrate_7_17.asciidoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,23 @@ To avoid deprecation warnings, remove any SSL truststores that do not
contain any trusted entries.
====

[[deprecation_for_dls_settings]]
.Deprecation for DLS settings
[%collapsible]
====
*Details* +
Two settings available in the latest versions of 7.17 are not available in the next major version.
Newer versions of 7.17 default to stricter Document Level Security (DLS) rules and the follow
settings can be used to disable those stricter DLS rules:
`xpack.security.dls.force_terms_aggs_to_exclude_deleted_docs.enabled` and
`xpack.security.dls.error_when_validate_query_with_rewrite.enabled`.
Newer versions, of next major version of {es}, also default to the stricter DLS rules but don't allow
usage of the less strict rules.
*Impact* +
To avoid deprecation warnings, remove these settings from elasticsearch.yml.
====

[discrete]
[[deprecations_717_mapping]]
==== Mapping deprecations
Expand Down
30 changes: 30 additions & 0 deletions docs/reference/release-notes/7.17.22.asciidoc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,36 @@ coming[7.17.22]

Also see <<breaking-changes-7.17,Breaking changes in 7.17>>.

[[breaking-7.17.22]]
[float]
=== Breaking changes

[discrete]
[[breaking_7_17_22_dls_changes]]
==== Stricter Document Level Security (DLS)

[[stricter_dls_7_17_22]]
.Document Level Security (DLS) applies stricter checks for the validate query API and for terms aggregations when min_doc_count is set to 0.

[%collapsible]
====
*Details* +
When Document Level Security (DLS) is applied to terms aggregations and min_doc_count is set to 0, stricter security rules apply.
When Document Level Security (DLS) is applied to the validate query API with the rewrite parameter, stricter security rules apply.
*Impact* +
If needed, test workflows with DLS enabled to ensure that the stricter security rules do not impact your application.
*Remediation* +
Set min_doc_count to a value greater than 0 in terms aggregations or use an account not constrained by DLS for the validate query API calls.
Set `xpack.security.dls.force_terms_aggs_to_exclude_deleted_docs.enabled` to `false` in the Elasticsearch configuration
to revert to the previous behavior.
Set `xpack.security.dls.error_when_validate_query_with_rewrite.enabled` to `false` in the Elasticsearch configuration
to revert to the previous behavior.
====

[[bug-7.17.22]]
[float]
=== Bug fixes
Expand Down
43 changes: 43 additions & 0 deletions server/src/main/java/org/elasticsearch/search/aggregations/AggregatorFactories.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import java.util.Locale;
import java.util.Map;
import java.util.Objects;
import java.util.Queue;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
Expand Down Expand Up @@ -330,6 +331,48 @@ public boolean mustVisitAllDocs() {
return false;
}

/**
* Return true if any of the builders is a terms aggregation with min_doc_count=0
*/
public boolean hasZeroMinDocTermsAggregation() {
final Queue<AggregationBuilder> queue = new LinkedList<>(aggregationBuilders);
while (queue.isEmpty() == false) {
final AggregationBuilder current = queue.poll();
if (current == null) {
continue;
}
if (current instanceof TermsAggregationBuilder) {
TermsAggregationBuilder termsBuilder = (TermsAggregationBuilder) current;
if (termsBuilder.minDocCount() == 0) {
return true;
}
}
queue.addAll(current.getSubAggregations());
}
return false;
}

/**
* Force all min_doc_count=0 terms aggregations to exclude deleted docs.
*/
public void forceTermsAggsToExcludeDeletedDocs() {
assert hasZeroMinDocTermsAggregation();
final Queue<AggregationBuilder> queue = new LinkedList<>(aggregationBuilders);
while (queue.isEmpty() == false) {
final AggregationBuilder current = queue.poll();
if (current == null) {
continue;
}
if (current instanceof TermsAggregationBuilder) {
TermsAggregationBuilder termsBuilder = (TermsAggregationBuilder) current;
if (termsBuilder.minDocCount() == 0) {
termsBuilder.excludeDeletedDocs(true);
}
}
queue.addAll(current.getSubAggregations());
}
}

public Builder addAggregator(AggregationBuilder factory) {
if (names.add(factory.name) == false) {
throw new IllegalArgumentException("Two sibling aggregations cannot have the same name: [" + factory.name + "]");
Expand Down
152 changes: 130 additions & 22 deletions ...g/elasticsearch/search/aggregations/bucket/terms/GlobalOrdinalsStringTermsAggregator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,19 @@
package org.elasticsearch.search.aggregations.bucket.terms;

import org.apache.lucene.index.DocValues;
import org.apache.lucene.index.LeafReader;
import org.apache.lucene.index.LeafReaderContext;
import org.apache.lucene.index.SortedDocValues;
import org.apache.lucene.index.SortedSetDocValues;
import org.apache.lucene.util.ArrayUtil;
import org.apache.lucene.util.Bits;
import org.apache.lucene.util.BytesRef;
import org.apache.lucene.util.PriorityQueue;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.util.BigArrays;
import org.elasticsearch.common.util.LongArray;
import org.elasticsearch.common.util.LongHash;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.core.Releasable;
import org.elasticsearch.core.Releasables;
import org.elasticsearch.search.DocValueFormat;
Expand Down Expand Up @@ -82,7 +86,8 @@ public GlobalOrdinalsStringTermsAggregator(
SubAggCollectionMode collectionMode,
boolean showTermDocCountError,
CardinalityUpperBound cardinality,
Map<String, Object> metadata
Map<String, Object> metadata,
boolean excludeDeletedDocs
) throws IOException {
super(name, factories, context, parent, order, format, bucketCountThresholds, collectionMode, showTermDocCountError, metadata);
this.resultStrategy = resultStrategy.apply(this); // ResultStrategy needs a reference to the Aggregator to do its job.
Expand All @@ -91,13 +96,13 @@ public GlobalOrdinalsStringTermsAggregator(
this.lookupGlobalOrd = values::lookupOrd;
this.acceptedGlobalOrdinals = acceptedOrds;
if (remapGlobalOrds) {
this.collectionStrategy = new RemapGlobalOrds(cardinality);
this.collectionStrategy = new RemapGlobalOrds(cardinality, excludeDeletedDocs);
} else {
this.collectionStrategy = cardinality.map(estimate -> {
if (estimate > 1) {
throw new AggregationExecutionException("Dense ords don't know how to collect from many buckets");
}
return new DenseGlobalOrds();
return new DenseGlobalOrds(excludeDeletedDocs);
});
}
}
Expand Down Expand Up @@ -274,7 +279,8 @@ static class LowCardinality extends GlobalOrdinalsStringTermsAggregator {
boolean remapGlobalOrds,
SubAggCollectionMode collectionMode,
boolean showTermDocCountError,
Map<String, Object> metadata
Map<String, Object> metadata,
boolean excludeDeletedDocs
) throws IOException {
super(
name,
Expand All @@ -292,7 +298,8 @@ static class LowCardinality extends GlobalOrdinalsStringTermsAggregator {
collectionMode,
showTermDocCountError,
CardinalityUpperBound.ONE,
metadata
metadata,
excludeDeletedDocs
);
assert factories == null || factories.countAggregators() == 0;
this.segmentDocCounts = context.bigArrays().newLongArray(1, true);
Expand Down Expand Up @@ -441,6 +448,13 @@ interface BucketInfoConsumer {
* bucket ordinal.
*/
class DenseGlobalOrds extends CollectionStrategy {

private final boolean excludeDeletedDocs;

DenseGlobalOrds(boolean excludeDeletedDocs) {
this.excludeDeletedDocs = excludeDeletedDocs;
}

@Override
String describe() {
return "dense";
Expand Down Expand Up @@ -471,6 +485,14 @@ long globalOrdToBucketOrd(long owningBucketOrd, long globalOrd) {
@Override
void forEach(long owningBucketOrd, BucketInfoConsumer consumer) throws IOException {
assert owningBucketOrd == 0;
if (excludeDeletedDocs) {
forEachExcludeDeletedDocs(consumer);
} else {
forEachAllowDeletedDocs(consumer);
}
}

private void forEachAllowDeletedDocs(BucketInfoConsumer consumer) throws IOException {
for (long globalOrd = 0; globalOrd < valueCount; globalOrd++) {
if (false == acceptedGlobalOrdinals.test(globalOrd)) {
continue;
Expand All @@ -482,6 +504,39 @@ void forEach(long owningBucketOrd, BucketInfoConsumer consumer) throws IOExcepti
}
}

/**
* Excludes deleted docs in the results by cross-checking with liveDocs.
*/
private void forEachExcludeDeletedDocs(BucketInfoConsumer consumer) throws IOException {
try (LongHash accepted = new LongHash(20, new BigArrays(null, null, ""))) {
for (LeafReaderContext ctx : searcher().getTopReaderContext().leaves()) {
LeafReader reader = ctx.reader();
Bits liveDocs = reader.getLiveDocs();
SortedSetDocValues globalOrds = null;
for (int docId = 0; docId < reader.maxDoc(); ++docId) {
if (liveDocs == null || liveDocs.get(docId)) { // document is not deleted
globalOrds = globalOrds == null ? valuesSource.globalOrdinalsValues(ctx) : globalOrds;
if (globalOrds.advanceExact(docId)) {
for (long globalOrd = globalOrds.nextOrd(); globalOrd != NO_MORE_ORDS; globalOrd = globalOrds.nextOrd()) {
if (accepted.find(globalOrd) >= 0) {
continue;
}
if (false == acceptedGlobalOrdinals.test(globalOrd)) {
continue;
}
long docCount = bucketDocCount(globalOrd);
if (bucketCountThresholds.getMinDocCount() == 0 || docCount > 0) {
consumer.accept(globalOrd, globalOrd, docCount);
accepted.add(globalOrd);
}
}
}
}
}
}
}
}

@Override
public void close() {}
}
Expand All @@ -494,9 +549,11 @@ public void close() {}
*/
private class RemapGlobalOrds extends CollectionStrategy {
private final LongKeyedBucketOrds bucketOrds;
private final boolean excludeDeletedDocs;

private RemapGlobalOrds(CardinalityUpperBound cardinality) {
private RemapGlobalOrds(CardinalityUpperBound cardinality, boolean excludeDeletedDocs) {
bucketOrds = LongKeyedBucketOrds.buildForValueRange(bigArrays(), cardinality, 0, valueCount - 1);
this.excludeDeletedDocs = excludeDeletedDocs;
}

@Override
Expand Down Expand Up @@ -530,27 +587,20 @@ long globalOrdToBucketOrd(long owningBucketOrd, long globalOrd) {

@Override
void forEach(long owningBucketOrd, BucketInfoConsumer consumer) throws IOException {
if (excludeDeletedDocs) {
forEachExcludeDeletedDocs(owningBucketOrd, consumer);
} else {
forEachAllowDeletedDocs(owningBucketOrd, consumer);
}
}

void forEachAllowDeletedDocs(long owningBucketOrd, BucketInfoConsumer consumer) throws IOException {
if (bucketCountThresholds.getMinDocCount() == 0) {
for (long globalOrd = 0; globalOrd < valueCount; globalOrd++) {
if (false == acceptedGlobalOrdinals.test(globalOrd)) {
continue;
}
/*
* Use `add` instead of `find` here to assign an ordinal
* even if the global ord wasn't found so we can build
* sub-aggregations without trouble even though we haven't
* hit any documents for them. This is wasteful, but
* settings minDocCount == 0 is wasteful in general.....
*/
long bucketOrd = bucketOrds.add(owningBucketOrd, globalOrd);
long docCount;
if (bucketOrd < 0) {
bucketOrd = -1 - bucketOrd;
docCount = bucketDocCount(bucketOrd);
} else {
docCount = 0;
}
consumer.accept(globalOrd, bucketOrd, docCount);
addBucketForMinDocCountZero(owningBucketOrd, globalOrd, consumer, null);
}
} else {
LongKeyedBucketOrds.BucketOrdsEnum ordsEnum = bucketOrds.ordsEnum(owningBucketOrd);
Expand All @@ -563,6 +613,64 @@ void forEach(long owningBucketOrd, BucketInfoConsumer consumer) throws IOExcepti
}
}

/**
* Excludes deleted docs in the results by cross-checking with liveDocs.
*/
void forEachExcludeDeletedDocs(long owningBucketOrd, BucketInfoConsumer consumer) throws IOException {
assert bucketCountThresholds.getMinDocCount() == 0;
try (LongHash accepted = new LongHash(20, new BigArrays(null, null, ""))) {
for (LeafReaderContext ctx : searcher().getTopReaderContext().leaves()) {
LeafReader reader = ctx.reader();
Bits liveDocs = reader.getLiveDocs();
SortedSetDocValues globalOrds = null;
for (int docId = 0; docId < reader.maxDoc(); ++docId) {
if (liveDocs == null || liveDocs.get(docId)) { // document is not deleted
globalOrds = globalOrds == null ? valuesSource.globalOrdinalsValues(ctx) : globalOrds;
if (globalOrds.advanceExact(docId)) {
for (long globalOrd = globalOrds.nextOrd(); globalOrd != NO_MORE_ORDS; globalOrd = globalOrds.nextOrd()) {
if (accepted.find(globalOrd) >= 0) {
continue;
}
if (false == acceptedGlobalOrdinals.test(globalOrd)) {
continue;
}
addBucketForMinDocCountZero(owningBucketOrd, globalOrd, consumer, accepted);
}
}
}
}
}
}
}

private void addBucketForMinDocCountZero(
long owningBucketOrd,
long globalOrd,
BucketInfoConsumer consumer,
@Nullable LongHash accepted
) throws IOException {
/*
* Use `add` instead of `find` here to assign an ordinal
* even if the global ord wasn't found so we can build
* sub-aggregations without trouble even though we haven't
* hit any documents for them. This is wasteful, but
* settings minDocCount == 0 is wasteful in general.....
*/
long bucketOrd = bucketOrds.add(owningBucketOrd, globalOrd);
long docCount;
if (bucketOrd < 0) {
bucketOrd = -1 - bucketOrd;
docCount = bucketDocCount(bucketOrd);
} else {
docCount = 0;
}
assert globalOrd >= 0;
consumer.accept(globalOrd, bucketOrd, docCount);
if (accepted != null) {
accepted.add(globalOrd);
}
}

@Override
public void close() {
bucketOrds.close();
Expand Down
Loading

0 comments on commit 4e08df5

Please sign in to comment.