Skip to content

Commit

Permalink
Improve shard level request cache efficiency (#69505) (#69522)
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 authored Feb 24, 2021
1 parent 0d8d346 commit 111afab
Show file tree
Hide file tree
Showing 10 changed files with 188 additions and 25 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,11 @@ public Boolean requestCache() {
return requestCache;
}

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

@Override
public Boolean allowPartialSearchResults() {
return allowPartialSearchResults;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ public interface ShardSearchRequest {

Boolean requestCache();

void requestCache(Boolean requestCache);

Boolean allowPartialSearchResults();

Scroll scroll();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,11 @@ public Boolean requestCache() {
return shardSearchLocalRequest.requestCache();
}

@Override
public void requestCache(Boolean requestCache) {
shardSearchLocalRequest.requestCache(requestCache);
}

@Override
public Boolean allowPartialSearchResults() {
return shardSearchLocalRequest.allowPartialSearchResults();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,10 @@ public Boolean requestCache() {
return null;
}

@Override
public void requestCache(Boolean requestCache) {
}

@Override
public Boolean allowPartialSearchResults() {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,10 @@ public Boolean requestCache() {
return null;
}

@Override
public void requestCache(Boolean requestCache) {
}

@Override
public Boolean allowPartialSearchResults() {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@
import org.apache.logging.log4j.Logger;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.IndicesRequest;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.license.XPackLicenseState;
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 @@ -36,26 +38,35 @@ 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();
if (supports(indicesRequest) && licenseState.isDocumentAndFieldLevelSecurityAllowed()) {
final IndicesAccessControl indicesAccessControl =
threadContext.getTransient(AuthorizationServiceField.INDICES_PERMISSIONS_KEY);
for (String index : indicesRequest.indices()) {
boolean fieldLevelSecurityEnabled = false;
boolean documentLevelSecurityEnabled = false;
final String[] requestIndices = indicesRequest.indices();
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) {
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) {
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 @@ -11,6 +11,8 @@
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.ShardSearchTransportRequest;
import org.elasticsearch.threadpool.ThreadPool;

/**
Expand All @@ -25,14 +27,26 @@ 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 ShardSearchTransportRequest
: "request must be either SearchRequest or ShardSearchTransportRequest";

final SearchSourceBuilder source;
if (indicesRequest instanceof SearchRequest) {
final SearchRequest request = (SearchRequest) indicesRequest;
request.requestCache(false);
source = request.source();
} else {
final ShardSearchTransportRequest request = (ShardSearchTransportRequest) 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 @@ -45,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 ShardSearchTransportRequest;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,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 @@ -107,7 +108,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 @@ -140,7 +142,18 @@ 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 @@ -922,6 +935,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 @@ -937,9 +959,12 @@ 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(Arrays.asList(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 @@ -962,7 +987,7 @@ 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)))
.filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user5", USERS_PASSWD)))
.prepareSearch("test")
.suggest(new SuggestBuilder()
.setGlobalText("valeu")
Expand All @@ -986,7 +1011,7 @@ 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)))
.filterWithHeader(Collections.singletonMap(BASIC_AUTH_HEADER, basicAuthHeaderValue("user5", USERS_PASSWD)))
.prepareSearch("test")
.suggest(new SuggestBuilder()
.setGlobalText("valeu")
Expand Down Expand Up @@ -1017,6 +1042,15 @@ 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", "suggest_field1", "type=text", "suggest_field2", "type=completion",
"yet_another", "type=text")
);

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

final String[] indices =
randomFrom(Arrays.asList(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
@@ -0,0 +1,48 @@
---
setup:
- skip:
features: headers

- do:
cluster.health:
wait_for_status: yellow

- do:
xpack.security.put_user:
username: "dls_fls_user"
body: >
{
"password": "s3krit-password",
"roles" : [ "dls_fls_role" ]
}
---
teardown:
- do:
xpack.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
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" }
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,22 @@ setup:
}
]
}
- do:
xpack.security.put_role:
name: "dls_fls_role"
body: >
{
"cluster": ["monitor"],
"indices": [
{
"names": ["shared_index"],
"privileges": ["read", "read_cross_cluster"],
"query": "{ \"term\": { \"public\" : true } }",
"field_security": { "grant": ["*"], "except": ["secret"] }
}
]
}
---
"Index data and search on the remote cluster":

Expand Down Expand Up @@ -196,3 +212,21 @@ setup:
"roles" : [ ]
}
- match: { user: { created: false } }

- do:
indices.create:
index: shared_index
body:
settings:
index:
number_of_shards: 1
number_of_replicas: 0

- do:
bulk:
refresh: true
body:
- '{"index": {"_index": "shared_index", "_id": 1, "_type": "test_type"}}'
- '{"public": true, "name": "doc 1", "secret": "sesame"}'
- '{"index": {"_index": "shared_index", "_id": 2, "_type": "test_type"}}'
- '{"public": false, "name": "doc 2", "secret": "sesame"}'

0 comments on commit 111afab

Please sign in to comment.