Skip to content

Commit

Permalink
Improve shard level request cache efficiency (#69505) (#69507) (#69514)
Browse files Browse the repository at this point in the history
Shard level request cache is improved to work correctly at all time. Also ensure profiling and suggester are properly disabled when not supported.
  • Loading branch information
ywangd committed Feb 24, 2021
1 parent 8ebfb22 commit b34410d
Show file tree
Hide file tree
Showing 8 changed files with 230 additions and 36 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public class ShardSearchRequest extends TransportRequest implements IndicesReque
private final Scroll scroll;
private final String[] types;
private final float indexBoost;
private final Boolean requestCache;
private Boolean requestCache;
private final long nowInMillis;
private final boolean allowPartialSearchResults;
private final OriginalIndices originalIndices;
Expand Down Expand Up @@ -345,6 +345,10 @@ public Boolean requestCache() {
return requestCache;
}

public void requestCache(Boolean requestCache) {
this.requestCache = requestCache;
}

public boolean allowPartialSearchResults() {
return allowPartialSearchResults;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,8 @@ protected String configUsers() {
"user1:" + usersPasswdHashed + "\n" +
"user2:" + usersPasswdHashed + "\n" +
"user3:" + usersPasswdHashed + "\n" +
"user4:" + usersPasswdHashed + "\n";
"user4:" + usersPasswdHashed + "\n" +
"user5:" + usersPasswdHashed + "\n";
}

@Override
Expand All @@ -138,7 +139,8 @@ protected String configUsersRoles() {
"role1:user1,user2,user3\n" +
"role2:user1,user3\n" +
"role3:user2,user3\n" +
"role4:user4\n";
"role4:user4\n" +
"role5:user5\n";
}

@Override
Expand Down Expand Up @@ -171,7 +173,17 @@ protected String configRoles() {
" - names: '*'\n" +
" privileges: [ ALL ]\n" +
// query that can match nested documents
" query: '{\"bool\": { \"must_not\": { \"term\" : {\"field1\" : \"value2\"}}}}'";
" query: '{\"bool\": { \"must_not\": { \"term\" : {\"field1\" : \"value2\"}}}}'\n" +
"role5:\n" +
" cluster: [ all ]\n" +
" indices:\n" +
" - names: [ 'test' ]\n" +
" privileges: [ read ]\n" +
" query: '{\"term\" : {\"field2\" : \"value2\"}}'\n" +
" - names: [ 'fls-index' ]\n" +
" privileges: [ read ]\n" +
" field_security:\n" +
" grant: [ 'field1', 'other_field', 'suggest_field2' ]\n";
}

@Override
Expand Down Expand Up @@ -1278,6 +1290,15 @@ public void testSuggesters() throws Exception {
.endObject()).get();
refresh("test");

assertAcked(client().admin().indices().prepareCreate("fls-index")
.setSettings(Settings.builder()
.put("index.number_of_shards", 1)
.put("index.number_of_replicas", 0)
)
.addMapping("type1", "field1", "type=text", "suggest_field1", "type=text", "suggest_field2", "type=completion",
"yet_another", "type=text")
);

// Term suggester:
SearchResponse response = client()
.prepareSearch("test")
Expand All @@ -1293,9 +1314,13 @@ public void testSuggesters() throws Exception {
assertThat(termSuggestion.getEntries().get(0).getOptions().size(), equalTo(1));
assertThat(termSuggestion.getEntries().get(0).getOptions().get(0).getText().string(), equalTo("value"));

final String[] indices =
randomFrom(org.elasticsearch.common.collect.List.of(
new String[] { "test" }, new String[] { "fls-index", "test" }, new String[] { "test", "fls-index" }));

Exception e = expectThrows(ElasticsearchSecurityException.class, () -> client()
.filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user2", USERS_PASSWD)))
.prepareSearch("test")
.filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user5", USERS_PASSWD)))
.prepareSearch(indices)
.suggest(new SuggestBuilder()
.setGlobalText("valeu")
.addSuggestion("_name1", new TermSuggestionBuilder("suggest_field1"))
Expand All @@ -1318,8 +1343,8 @@ public void testSuggesters() throws Exception {
assertThat(phraseSuggestion.getEntries().get(0).getOptions().get(0).getText().string(), equalTo("value"));

e = expectThrows(ElasticsearchSecurityException.class, () -> client()
.filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user2", USERS_PASSWD)))
.prepareSearch("test")
.filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user5", USERS_PASSWD)))
.prepareSearch(indices)
.suggest(new SuggestBuilder()
.setGlobalText("valeu")
.addSuggestion("_name1", new PhraseSuggestionBuilder("suggest_field1"))
Expand All @@ -1342,8 +1367,8 @@ public void testSuggesters() throws Exception {
assertThat(completionSuggestion.getEntries().get(0).getOptions().get(0).getText().string(), equalTo("value"));

e = expectThrows(ElasticsearchSecurityException.class, () -> client()
.filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user2", USERS_PASSWD)))
.prepareSearch("test")
.filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user5", USERS_PASSWD)))
.prepareSearch(indices)
.suggest(new SuggestBuilder()
.setGlobalText("valeu")
.addSuggestion("_name1", new CompletionSuggestionBuilder("suggest_field2"))
Expand Down Expand Up @@ -1373,6 +1398,14 @@ public void testProfile() throws Exception {
.endObject()).get();
refresh("test");

assertAcked(client().admin().indices().prepareCreate("fls-index")
.setSettings(Settings.builder()
.put("index.number_of_shards", 1)
.put("index.number_of_replicas", 0)
)
.addMapping("type1", "field1", "type=text", "other_field", "type=text", "yet_another", "type=text")
);

SearchResponse response = client()
.prepareSearch("test")
.setProfile(true)
Expand All @@ -1389,9 +1422,12 @@ public void testProfile() throws Exception {
// ProfileResult profileResult = queryProfileShardResult.getQueryResults().get(0);
// assertThat(profileResult.getLuceneDescription(), equalTo("(other_field:value)^0.8"));

final String[] indices =
randomFrom(org.elasticsearch.common.collect.List.of(
new String[] { "test" }, new String[] { "fls-index", "test" }, new String[] { "test", "fls-index" }));
Exception e = expectThrows(ElasticsearchSecurityException.class, () -> client()
.filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user2", USERS_PASSWD)))
.prepareSearch("test")
.filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user5", USERS_PASSWD)))
.prepareSearch(indices)
.setProfile(true)
.setQuery(new FuzzyQueryBuilder("other_field", "valeu"))
.get());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,11 @@
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.IndicesRequest;
import org.elasticsearch.common.MemoizedSupplier;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.license.XPackLicenseState;
import org.elasticsearch.license.XPackLicenseState.Feature;
import org.elasticsearch.transport.TransportActionProxy;
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine;
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.AuthorizationInfo;
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.RequestInfo;
Expand All @@ -39,28 +41,37 @@ abstract class FieldAndDocumentLevelSecurityRequestInterceptor implements Reques
@Override
public void intercept(RequestInfo requestInfo, AuthorizationEngine authorizationEngine, AuthorizationInfo authorizationInfo,
ActionListener<Void> listener) {
if (requestInfo.getRequest() instanceof IndicesRequest) {
if (requestInfo.getRequest() instanceof IndicesRequest && false == TransportActionProxy.isProxyAction(requestInfo.getAction())) {
IndicesRequest indicesRequest = (IndicesRequest) requestInfo.getRequest();
boolean shouldIntercept = licenseState.isSecurityEnabled();
MemoizedSupplier<Boolean> licenseChecker = new MemoizedSupplier<>(() -> licenseState.checkFeature(Feature.SECURITY_DLS_FLS));
if (supports(indicesRequest) && shouldIntercept) {
final IndicesAccessControl indicesAccessControl =
threadContext.getTransient(AuthorizationServiceField.INDICES_PERMISSIONS_KEY);
for (String index : requestIndices(indicesRequest)) {
boolean fieldLevelSecurityEnabled = false;
boolean documentLevelSecurityEnabled = false;
final String[] requestIndices = requestIndices(indicesRequest);
for (String index : requestIndices) {
IndicesAccessControl.IndexAccessControl indexAccessControl = indicesAccessControl.getIndexPermissions(index);
if (indexAccessControl != null) {
boolean fieldLevelSecurityEnabled = indexAccessControl.getFieldPermissions().hasFieldLevelSecurity();
boolean documentLevelSecurityEnabled = indexAccessControl.getDocumentPermissions().hasDocumentLevelPermissions();
if ((fieldLevelSecurityEnabled || documentLevelSecurityEnabled) && licenseChecker.get()) {
logger.trace("intercepted request for index [{}] with field level access controls [{}] " +
"document level access controls [{}]. disabling conflicting features",
index, fieldLevelSecurityEnabled, documentLevelSecurityEnabled);
disableFeatures(indicesRequest, fieldLevelSecurityEnabled, documentLevelSecurityEnabled, listener);
return;
fieldLevelSecurityEnabled =
fieldLevelSecurityEnabled || indexAccessControl.getFieldPermissions().hasFieldLevelSecurity();
documentLevelSecurityEnabled =
documentLevelSecurityEnabled || indexAccessControl.getDocumentPermissions().hasDocumentLevelPermissions();
if (fieldLevelSecurityEnabled && documentLevelSecurityEnabled) {
break;
}
}
logger.trace("intercepted request for index [{}] without field or document level access controls", index);
}
if ((fieldLevelSecurityEnabled || documentLevelSecurityEnabled) && licenseChecker.get()) {
logger.trace("intercepted request for indices [{}] with field level access controls [{}] " +
"document level access controls [{}]. disabling conflicting features",
Strings.arrayToDelimitedString(requestIndices, ","), fieldLevelSecurityEnabled, documentLevelSecurityEnabled);
disableFeatures(indicesRequest, fieldLevelSecurityEnabled, documentLevelSecurityEnabled, listener);
return;
}
logger.trace("intercepted request for indices [{}] without field or document level access controls",
Strings.arrayToDelimitedString(requestIndices, ","));
}
}
listener.onResponse(null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@
import org.elasticsearch.action.search.SearchRequest;
import org.elasticsearch.license.XPackLicenseState;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.search.builder.SearchSourceBuilder;
import org.elasticsearch.search.internal.ShardSearchRequest;
import org.elasticsearch.threadpool.ThreadPool;

/**
* If field level security is enabled this interceptor disables the request cache for search requests.
* If field level security is enabled this interceptor disables the request cache for search and shardSearch requests.
*/
public class SearchRequestInterceptor extends FieldAndDocumentLevelSecurityRequestInterceptor {

Expand All @@ -26,14 +28,25 @@ public SearchRequestInterceptor(ThreadPool threadPool, XPackLicenseState license
@Override
public void disableFeatures(IndicesRequest indicesRequest, boolean fieldLevelSecurityEnabled, boolean documentLevelSecurityEnabled,
ActionListener<Void> listener) {
final SearchRequest request = (SearchRequest) indicesRequest;
request.requestCache(false);
assert indicesRequest instanceof SearchRequest || indicesRequest instanceof ShardSearchRequest
: "request must be either SearchRequest or ShardSearchRequest";

final SearchSourceBuilder source;
if (indicesRequest instanceof SearchRequest) {
final SearchRequest request = (SearchRequest) indicesRequest;
request.requestCache(false);
source = request.source();
} else {
final ShardSearchRequest request = (ShardSearchRequest) indicesRequest;
request.requestCache(false);
source = request.source();
}

if (documentLevelSecurityEnabled) {
if (request.source() != null && request.source().suggest() != null) {
if (source != null && source.suggest() != null) {
listener.onFailure(new ElasticsearchSecurityException("Suggest isn't supported if document level security is enabled",
RestStatus.BAD_REQUEST));
} else if (request.source() != null && request.source().profile()) {
} else if (source != null && source.profile()) {
listener.onFailure(new ElasticsearchSecurityException("A search request cannot be profiled if document level security " +
"is enabled", RestStatus.BAD_REQUEST));
} else {
Expand All @@ -46,6 +59,6 @@ public void disableFeatures(IndicesRequest indicesRequest, boolean fieldLevelSec

@Override
public boolean supports(IndicesRequest request) {
return request instanceof SearchRequest;
return request instanceof SearchRequest || request instanceof ShardSearchRequest;
}
}
23 changes: 21 additions & 2 deletions x-pack/qa/multi-cluster-search-security/build.gradle
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import org.elasticsearch.gradle.info.BuildParams
import org.elasticsearch.gradle.test.RestIntegTestTask

apply plugin: 'elasticsearch.testclusters'
Expand All @@ -19,6 +20,9 @@ tasks.register('remote-cluster', RestIntegTestTask) {
systemProperty 'tests.rest.suite', 'remote_cluster'
}

// randomise between sniff and proxy modes
boolean proxyMode = (new Random(Long.parseUnsignedLong(BuildParams.testSeed.tokenize(':').get(0), 16))).nextBoolean()

testClusters {
'remote-cluster' {
testDistribution = 'DEFAULT'
Expand All @@ -38,8 +42,15 @@ testClusters {
setting 'xpack.watcher.enabled', 'false'
setting 'xpack.ml.enabled', 'false'
setting 'xpack.license.self_generated.type', 'trial'
setting 'cluster.remote.my_remote_cluster.seeds', {
testClusters.'remote-cluster'.getAllTransportPortURI().collect { "\"$it\"" }.toString()
if (proxyMode) {
setting 'cluster.remote.my_remote_cluster.mode', 'proxy'
setting 'cluster.remote.my_remote_cluster.proxy_address', {
"\"${testClusters.'remote-cluster'.getAllTransportPortURI().get(0)}\""
}
} else {
setting 'cluster.remote.my_remote_cluster.seeds', {
testClusters.'remote-cluster'.getAllTransportPortURI().collect { "\"$it\"" }.toString()
}
}
setting 'cluster.remote.connections_per_cluster', "1"

Expand All @@ -51,6 +62,14 @@ tasks.register('mixed-cluster', RestIntegTestTask) {
dependsOn 'remote-cluster'
useCluster testClusters.'remote-cluster'
systemProperty 'tests.rest.suite', 'multi_cluster'
if (proxyMode) {
systemProperty 'tests.rest.blacklist', [
'multi_cluster/10_basic/Add transient remote cluster based on the preset cluster',
'multi_cluster/20_info/Add transient remote cluster based on the preset cluster and check remote info',
'multi_cluster/20_info/Fetch remote cluster info for existing cluster',
'multi_cluster/70_connection_mode_configuration/*',
].join(",")
}
}

tasks.register("integTest") {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,12 @@
- match: {indices.7.attributes.0: open }
- match: {indices.8.name: my_remote_cluster:secured_via_alias}
- match: {indices.8.attributes.0: open}
- match: {indices.9.name: my_remote_cluster:single_doc_index}
- match: {indices.10.attributes.0: open}
- match: {indices.10.name: my_remote_cluster:test_index}
- match: {indices.10.aliases.0: aliased_test_index}
- match: {indices.10.attributes.0: open}
- match: {indices.9.name: my_remote_cluster:shared_index}
- match: {indices.10.name: my_remote_cluster:single_doc_index}
- match: {indices.11.attributes.0: open}
- match: {indices.11.name: my_remote_cluster:test_index}
- match: {indices.11.aliases.0: aliased_test_index}
- match: {indices.11.attributes.0: open}
- match: {aliases.0.name: my_remote_cluster:.security}
- match: {aliases.0.indices.0: .security-7}
- match: {aliases.1.name: my_remote_cluster:aliased_closed_index}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
---
setup:
- skip:
features: headers

- do:
cluster.health:
wait_for_status: yellow

- do:
security.put_user:
username: "dls_fls_user"
body: >
{
"password": "s3krit-password",
"roles" : [ "dls_fls_role" ]
}
---
teardown:
- do:
security.delete_user:
username: "dls_fls_user"
ignore: 404

---
"Search with document and field level security":
- do:
search:
rest_total_hits_as_int: true
request_cache: true
index: my_remote_cluster:shared_index

- match: { hits.total: 2}
- length: { hits.hits.0._source: 3 }
- match: { hits.hits.0._source.secret: "sesame" }

- do:
headers: { Authorization: "Basic ZGxzX2Zsc191c2VyOnMza3JpdC1wYXNzd29yZA==" }
search:
rest_total_hits_as_int: true
ccs_minimize_roundtrips: false
request_cache: true
index: my_remote_cluster:shared_index

- match: { hits.total: 1}
- length: { hits.hits.0._source: 2 }
- is_true: hits.hits.0._source.public
- match: { hits.hits.0._source.name: "doc 1" }

---
"Async search with document and field level security":
- skip:
version: " - 7.6.99"
reason: "async search available since 7.7"

- do:
async_search.submit:
index: my_remote_cluster:shared_index
wait_for_completion_timeout: 10s

- match: { response.hits.total.value: 2}
- length: { response.hits.hits.0._source: 3 }
- match: { response.hits.hits.0._source.secret: "sesame" }

- do:
headers: { Authorization: "Basic ZGxzX2Zsc191c2VyOnMza3JpdC1wYXNzd29yZA==" }
async_search.submit:
index: my_remote_cluster:shared_index
wait_for_completion_timeout: 10s

- match: { response.hits.total.value: 1}
- length: { response.hits.hits.0._source: 2 }
- is_true: response.hits.hits.0._source.public
- match: { response.hits.hits.0._source.name: "doc 1" }

0 comments on commit b34410d

Please sign in to comment.