Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Native roles store uses mget to retrieve roles #33531

Merged
merged 21 commits into from
Oct 30, 2018
Merged

Native roles store uses mget to retrieve roles #33531

Changes from 3 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
629b4a2
Native roles store uses mget to retrieve roles
jaymode Sep 7, 2018
2be96aa
Merge branch 'master' into roles_use_mget
jaymode Sep 11, 2018
a4016e4
fix checkstyle
jaymode Sep 11, 2018
df8e6b9
Merge branch 'master' into roles_use_mget
jaymode Sep 12, 2018
ac0eda9
ensure role exists before trying to transform
jaymode Sep 12, 2018
c5d2a63
Merge branch 'master' into roles_use_mget
jaymode Oct 15, 2018
7c9d0af
fix error handling
jaymode Oct 15, 2018
7266147
Merge branch 'master' into roles_use_mget
jaymode Oct 16, 2018
3aaf5af
fix retrieve all
jaymode Oct 16, 2018
ada770a
Merge branch 'master' into roles_use_mget
jaymode Oct 16, 2018
453323d
Merge branch 'master' into roles_use_mget
jaymode Oct 22, 2018
3da780b
Merge branch 'master' into roles_use_mget
jaymode Oct 22, 2018
f958999
Merge branch 'master' into roles_use_mget
jaymode Oct 24, 2018
7b707cd
any failure means failure in native store
jaymode Oct 24, 2018
27dabe6
Merge branch 'master' into roles_use_mget
jaymode Oct 24, 2018
fbadafd
Merge branch 'master' into roles_use_mget
jaymode Oct 24, 2018
aae1861
Merge branch 'master' into roles_use_mget
jaymode Oct 25, 2018
1b4af47
Merge branch 'master' into roles_use_mget
jaymode Oct 26, 2018
bab7819
add missing return statement
jaymode Oct 26, 2018
80f645c
Merge branch 'master' into roles_use_mget
jaymode Oct 29, 2018
c8db29e
fix wrong keepalive value
jaymode Oct 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 31 additions & 22 deletions ...security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
import org.elasticsearch.action.delete.DeleteRequest;
import org.elasticsearch.action.delete.DeleteResponse;
import org.elasticsearch.action.get.GetResponse;
import org.elasticsearch.action.get.MultiGetRequest;
import org.elasticsearch.action.get.MultiGetResponse;
import org.elasticsearch.action.index.IndexRequest;
import org.elasticsearch.action.index.IndexResponse;
import org.elasticsearch.action.search.MultiSearchResponse;
Expand Down Expand Up @@ -56,6 +58,7 @@
import java.util.Map;
import java.util.Objects;
import java.util.function.Supplier;
import java.util.stream.Collectors;

import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
import static org.elasticsearch.index.query.QueryBuilders.existsQuery;
Expand Down Expand Up @@ -104,39 +107,45 @@ public void getRoleDescriptors(String[] names, final ActionListener<Collection<R
if (securityIndex.indexExists() == false) {
// TODO remove this short circuiting and fix tests that fail without this!
listener.onResponse(Collections.emptyList());
} else if (names != null && names.length == 1) {
getRoleDescriptor(Objects.requireNonNull(names[0]), ActionListener.wrap(roleDescriptor ->
listener.onResponse(roleDescriptor == null ? Collections.emptyList() : Collections.singletonList(roleDescriptor)),
listener::onFailure));
} else {
} else if (names == null || names.length == 0) {
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> {
QueryBuilder query;
if (names == null || names.length == 0) {
query = QueryBuilders.termQuery(RoleDescriptor.Fields.TYPE.getPreferredName(), ROLE_TYPE);
} else {
final String[] roleNames = Arrays.stream(names).map(s -> getIdForUser(s)).toArray(String[]::new);
query = QueryBuilders.boolQuery().filter(QueryBuilders.idsQuery(ROLE_DOC_TYPE).addIds(roleNames));
}
QueryBuilder query = QueryBuilders.termQuery(RoleDescriptor.Fields.TYPE.getPreferredName(), ROLE_TYPE);
final Supplier<ThreadContext.StoredContext> supplier = client.threadPool().getThreadContext().newRestorableContext(false);
try (ThreadContext.StoredContext ignore = stashWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN)) {
SearchRequest request = client.prepareSearch(SecurityIndexManager.SECURITY_INDEX_NAME)
.setScroll(TimeValue.timeValueSeconds(10L))
.setQuery(query)
.setSize(1000)
.setFetchSource(true)
.request();
.setScroll(TimeValue.timeValueSeconds(10L))
jaymode marked this conversation as resolved.
Show resolved Hide resolved
.setQuery(query)
.setSize(1000)
.setFetchSource(true)
.request();
request.indicesOptions().ignoreUnavailable();
ScrollHelper.fetchAllByEntity(client, request, new ContextPreservingActionListener<>(supplier, listener),
(hit) -> transformRole(hit.getId(), hit.getSourceRef(), logger, licenseState));
(hit) -> transformRole(hit.getId(), hit.getSourceRef(), logger, licenseState));
}
});
} else if (names.length == 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this branch anymore.

getRoleDescriptor(Objects.requireNonNull(names[0]), ActionListener.wrap(roleDescriptor ->
listener.onResponse(roleDescriptor == null ? Collections.emptyList() : Collections.singletonList(roleDescriptor)),
listener::onFailure));
} else {
final String[] roleIds = Arrays.stream(names).map(NativeRolesStore::getIdForRole).toArray(String[]::new);
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> {
MultiGetRequest multiGetRequest = client.prepareMultiGet().add(SECURITY_INDEX_NAME, ROLE_DOC_TYPE, roleIds).request();
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, multiGetRequest,
ActionListener.<MultiGetResponse>wrap(mGetResponse ->
listener.onResponse(Arrays.stream(mGetResponse.getResponses())
.filter(item -> item.isFailed() == false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is silently dropping failures the right thing?

If the .security index is No shard available, then won't we just return an empty list of roles which will be stored in the negative cache?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that signaling this failure up the stack would be a mess, I wonder if partial results really bring us any advantage. I suggest we fail (call the listener's failure handle). When we'll move the security searches over a different threadpool, partial results would be even scarier.

.map(item -> transformRole(item.getResponse()))
.collect(Collectors.toList())),
listener::onFailure), client::multiGet);
});
}
}

public void deleteRole(final DeleteRoleRequest deleteRoleRequest, final ActionListener<Boolean> listener) {
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () -> {
DeleteRequest request = client.prepareDelete(SecurityIndexManager.SECURITY_INDEX_NAME,
ROLE_DOC_TYPE, getIdForUser(deleteRoleRequest.name())).request();
ROLE_DOC_TYPE, getIdForRole(deleteRoleRequest.name())).request();
request.setRefreshPolicy(deleteRoleRequest.getRefreshPolicy());
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN, request,
new ActionListener<DeleteResponse>() {
Expand Down Expand Up @@ -175,7 +184,7 @@ void innerPutRole(final PutRoleRequest request, final RoleDescriptor role, final
listener.onFailure(e);
return;
}
final IndexRequest indexRequest = client.prepareIndex(SECURITY_INDEX_NAME, ROLE_DOC_TYPE, getIdForUser(role.getName()))
final IndexRequest indexRequest = client.prepareIndex(SECURITY_INDEX_NAME, ROLE_DOC_TYPE, getIdForRole(role.getName()))
.setSource(xContentBuilder)
.setRefreshPolicy(request.getRefreshPolicy())
.request();
Expand Down Expand Up @@ -293,7 +302,7 @@ private void executeGetRoleRequest(String role, ActionListener<GetResponse> list
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () ->
executeAsyncWithOrigin(client.threadPool().getThreadContext(), SECURITY_ORIGIN,
client.prepareGet(SECURITY_INDEX_NAME,
ROLE_DOC_TYPE, getIdForUser(role)).request(),
ROLE_DOC_TYPE, getIdForRole(role)).request(),
listener,
client::get));
}
Expand Down Expand Up @@ -373,7 +382,7 @@ public static void addSettings(List<Setting<?>> settings) {
/**
* Gets the document's id field for the given role name.
*/
private static String getIdForUser(final String roleName) {
private static String getIdForRole(final String roleName) {
return ROLE_TYPE + "-" + roleName;
}
}