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.

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

Add cache for application privileges #55836

Merged
merged 57 commits into from
Jun 29, 2020
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
b72d0d3
WIP
ywangd Apr 27, 2020
a02aba3
Add tests
ywangd Apr 27, 2020
6d9fa0a
Add invalidation
ywangd Apr 27, 2020
ee33fff
Add some corrurrency liveness protection
ywangd Apr 27, 2020
4bbd16d
Add API for both rest and transport
ywangd Apr 27, 2020
df28676
Simplify NativePrivilegeStore
ywangd Apr 27, 2020
4a63a16
Minor
ywangd Apr 27, 2020
a1d177c
More tweak
ywangd Apr 27, 2020
05061f9
Add single node tests
ywangd Apr 28, 2020
daafb80
checkstyle
ywangd Apr 28, 2020
545e27c
More checkstyle
ywangd Apr 28, 2020
5987b3b
checkstyle again
ywangd Apr 28, 2020
6094178
Fix json API file
ywangd Apr 28, 2020
1c58309
Update API json file
ywangd Apr 28, 2020
9100c24
WIP
ywangd Apr 30, 2020
3f9c5b7
Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/s…
ywangd Apr 30, 2020
08865dd
Merge branch 'es-54317-app-privilege-cache' of github.com:ywangd/elas…
ywangd Apr 30, 2020
b9538a8
Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/s…
ywangd Apr 30, 2020
9c5e947
Merge branch 'es-54317-app-privilege-cache' of github.com:ywangd/elas…
ywangd Apr 30, 2020
f5b166c
WIP
ywangd Apr 30, 2020
1e7ce7c
Merge remote-tracking branch 'origin/master' into es-54317-app-privil…
ywangd Apr 30, 2020
f1f3e72
Address feedback
ywangd Apr 30, 2020
a7b5b40
Checkstyle
ywangd Apr 30, 2020
5306bc1
Address feedback
ywangd May 4, 2020
03680bc
Skip yaml tests for 7.x
ywangd May 4, 2020
cdfe886
Merge remote-tracking branch 'origin/master' into es-54317-app-privil…
ywangd May 4, 2020
435993d
Fix yaml tests
ywangd May 4, 2020
20be80d
Add HLRC relevant files
ywangd May 4, 2020
d2f2f3e
check style
ywangd May 4, 2020
f1f7035
Fix broken docs
ywangd May 4, 2020
db6020e
Add bwc for the new transport action
ywangd May 4, 2020
73dda1f
Add working rolling upgrade mixed-cluster tests for privilege cache c…
ywangd May 5, 2020
ea9172f
Merge remote-tracking branch 'origin/master' into es-54317-app-privil…
ywangd May 5, 2020
65f9524
Remove bwc handling code and simply report no handler
ywangd May 5, 2020
04bb476
Checkstyle
ywangd May 5, 2020
07b7837
Clear privilege cache on security index changes
ywangd May 15, 2020
34464d7
Merge remote-tracking branch 'origin/master' into es-54317-app-privil…
ywangd May 15, 2020
e87d897
Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/s…
ywangd May 29, 2020
5acc7ec
Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/s…
ywangd May 29, 2020
5e3174a
Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/s…
ywangd May 29, 2020
385fb12
Update x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/s…
ywangd May 29, 2020
e411dc9
Update x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/s…
ywangd May 29, 2020
58b3942
Address feedback
ywangd May 29, 2020
a8f8e81
Merge remote-tracking branch 'origin/master' into es-54317-app-privil…
ywangd May 29, 2020
9a763c3
Address feedback
ywangd May 29, 2020
a43b3ef
Fix docs
ywangd May 29, 2020
ac6c39f
Address feedback
ywangd Jun 2, 2020
8e1dc13
Merge remote-tracking branch 'origin/master' into es-54317-app-privil…
ywangd Jun 2, 2020
21c89fb
Remove todo as it is pointless
ywangd Jun 2, 2020
1499a26
Address feedback for readwritelock and cache invalidation
ywangd Jun 3, 2020
3b7e773
Merge remote-tracking branch 'origin/master' into es-54317-app-privil…
ywangd Jun 3, 2020
83a0e19
add tests for caching behaviour
ywangd Jun 4, 2020
3deef1e
Merge remote-tracking branch 'origin/master' into es-54317-app-privil…
ywangd Jun 4, 2020
9364e36
Add TTL as a safety net. Also consolidate cache size to a single setting
ywangd Jun 15, 2020
0280c3e
Merge remote-tracking branch 'origin/master' into es-54317-app-privil…
ywangd Jun 15, 2020
ae3c784
Address feedback about lock duration
ywangd Jun 29, 2020
68a4dc1
Merge remote-tracking branch 'origin/master' into es-54317-app-privil…
ywangd Jun 29, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

package org.elasticsearch.xpack.core.security.action.privilege;

import org.elasticsearch.action.ActionType;

public class ClearPrivilegesCacheAction extends ActionType<ClearPrivilegesCacheResponse> {

public static final ClearPrivilegesCacheAction INSTANCE = new ClearPrivilegesCacheAction();
public static final String NAME = "cluster:admin/xpack/security/privilege/cache/clear";

protected ClearPrivilegesCacheAction() {
super(NAME, ClearPrivilegesCacheResponse::new);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

package org.elasticsearch.xpack.core.security.action.privilege;

import org.elasticsearch.action.support.nodes.BaseNodesRequest;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.transport.TransportRequest;

import java.io.IOException;

public class ClearPrivilegesCacheRequest extends BaseNodesRequest<ClearPrivilegesCacheRequest> {

String[] applicationNames;

public ClearPrivilegesCacheRequest() {
super((String[]) null);
}

public ClearPrivilegesCacheRequest(StreamInput in) throws IOException {
super(in);
applicationNames = in.readOptionalStringArray();
}

public ClearPrivilegesCacheRequest applicationNames(String... applicationNames) {
this.applicationNames = applicationNames;
return this;
}

public String[] applicationNames() {
return applicationNames;
}

@Override
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
out.writeOptionalStringArray(applicationNames);
}

public static class Node extends TransportRequest {
private String[] applicationNames;

public Node(StreamInput in) throws IOException {
super(in);
applicationNames = in.readOptionalStringArray();
}

public Node(ClearPrivilegesCacheRequest request) {
this.applicationNames = request.applicationNames();
}

public String[] getApplicationNames() {
return applicationNames;
}

@Override
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
out.writeOptionalStringArray(applicationNames);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

package org.elasticsearch.xpack.core.security.action.privilege;

import org.elasticsearch.action.FailedNodeException;
import org.elasticsearch.action.support.nodes.BaseNodeResponse;
import org.elasticsearch.action.support.nodes.BaseNodesResponse;
import org.elasticsearch.cluster.ClusterName;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.ToXContentFragment;
import org.elasticsearch.common.xcontent.XContentBuilder;

import java.io.IOException;
import java.util.List;

public class ClearPrivilegesCacheResponse extends BaseNodesResponse<ClearPrivilegesCacheResponse.Node>
Copy link
Contributor

Choose a reason for hiding this comment

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

Separate to this PR, it feels like we could consolidate these duplicate classes into a common base class.

Copy link
Member Author

Choose a reason for hiding this comment

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

A common base class for all ClearXxxCacheResponse?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Not a priority, but there's a bunch of copy paste here that we could ditch.

implements ToXContentFragment {

public ClearPrivilegesCacheResponse(StreamInput in) throws IOException {
super(in);
}

public ClearPrivilegesCacheResponse(ClusterName clusterName, List<Node> nodes, List<FailedNodeException> failures) {
super(clusterName, nodes, failures);
}

@Override
protected List<Node> readNodesFrom(StreamInput in) throws IOException {
return in.readList(Node::new);
}

@Override
protected void writeNodesTo(StreamOutput out, List<Node> nodes) throws IOException {
out.writeList(nodes);
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject("nodes");
for (Node node : getNodes()) {
builder.startObject(node.getNode().getId());
builder.field("name", node.getNode().getName());
builder.endObject();
}
builder.endObject();
return builder;
}

public static class Node extends BaseNodeResponse {
public Node(StreamInput in) throws IOException {
super(in);
}

public Node(DiscoveryNode node) {
super(node);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import org.elasticsearch.common.util.concurrent.ReleasableLock;

import java.util.Iterator;
import java.util.Map;
import java.util.concurrent.locks.ReadWriteLock;
import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.function.Predicate;
Expand Down Expand Up @@ -56,4 +57,17 @@ public void removeKeysIf(Predicate<K> removeIf) {
}
}
}

public void removeValuesIf(Predicate<V> removeIf) {
ywangd marked this conversation as resolved.
Show resolved Hide resolved
// the cache cannot be modified while doing this operation per the terms of the cache iterator
try (ReleasableLock ignored = this.acquireForIterator()) {
Iterator<K> iterator = cache.keys().iterator();
while (iterator.hasNext()) {
K key = iterator.next();
if (removeIf.test(cache.get(key))) {
iterator.remove();
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@
import org.elasticsearch.xpack.core.security.action.oidc.OpenIdConnectAuthenticateAction;
import org.elasticsearch.xpack.core.security.action.oidc.OpenIdConnectLogoutAction;
import org.elasticsearch.xpack.core.security.action.oidc.OpenIdConnectPrepareAuthenticationAction;
import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheAction;
import org.elasticsearch.xpack.core.security.action.privilege.DeletePrivilegesAction;
import org.elasticsearch.xpack.core.security.action.privilege.GetBuiltinPrivilegesAction;
import org.elasticsearch.xpack.core.security.action.privilege.GetPrivilegesAction;
Expand Down Expand Up @@ -151,6 +152,7 @@
import org.elasticsearch.xpack.security.action.oidc.TransportOpenIdConnectAuthenticateAction;
import org.elasticsearch.xpack.security.action.oidc.TransportOpenIdConnectLogoutAction;
import org.elasticsearch.xpack.security.action.oidc.TransportOpenIdConnectPrepareAuthenticationAction;
import org.elasticsearch.xpack.security.action.privilege.TransportClearPrivilegesCacheAction;
import org.elasticsearch.xpack.security.action.privilege.TransportDeletePrivilegesAction;
import org.elasticsearch.xpack.security.action.privilege.TransportGetBuiltinPrivilegesAction;
import org.elasticsearch.xpack.security.action.privilege.TransportGetPrivilegesAction;
Expand Down Expand Up @@ -217,6 +219,7 @@
import org.elasticsearch.xpack.security.rest.action.oidc.RestOpenIdConnectAuthenticateAction;
import org.elasticsearch.xpack.security.rest.action.oidc.RestOpenIdConnectLogoutAction;
import org.elasticsearch.xpack.security.rest.action.oidc.RestOpenIdConnectPrepareAuthenticationAction;
import org.elasticsearch.xpack.security.rest.action.privilege.RestClearPrivilegesAction;
import org.elasticsearch.xpack.security.rest.action.privilege.RestDeletePrivilegesAction;
import org.elasticsearch.xpack.security.rest.action.privilege.RestGetBuiltinPrivilegesAction;
import org.elasticsearch.xpack.security.rest.action.privilege.RestGetPrivilegesAction;
Expand Down Expand Up @@ -727,6 +730,7 @@ public void onIndexModule(IndexModule module) {
return Arrays.asList(
new ActionHandler<>(ClearRealmCacheAction.INSTANCE, TransportClearRealmCacheAction.class),
new ActionHandler<>(ClearRolesCacheAction.INSTANCE, TransportClearRolesCacheAction.class),
new ActionHandler<>(ClearPrivilegesCacheAction.INSTANCE, TransportClearPrivilegesCacheAction.class),
new ActionHandler<>(GetUsersAction.INSTANCE, TransportGetUsersAction.class),
new ActionHandler<>(PutUserAction.INSTANCE, TransportPutUserAction.class),
new ActionHandler<>(DeleteUserAction.INSTANCE, TransportDeleteUserAction.class),
Expand Down Expand Up @@ -786,6 +790,7 @@ public List<RestHandler> getRestHandlers(Settings settings, RestController restC
new RestAuthenticateAction(settings, securityContext.get(), getLicenseState()),
new RestClearRealmCacheAction(settings, getLicenseState()),
new RestClearRolesCacheAction(settings, getLicenseState()),
new RestClearPrivilegesAction(settings, getLicenseState()),
new RestGetUsersAction(settings, getLicenseState()),
new RestPutUserAction(settings, getLicenseState()),
new RestDeleteUserAction(settings, getLicenseState()),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

package org.elasticsearch.xpack.security.action.privilege;

import org.elasticsearch.action.FailedNodeException;
import org.elasticsearch.action.support.ActionFilters;
import org.elasticsearch.action.support.nodes.TransportNodesAction;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.tasks.Task;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.transport.TransportService;
import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheAction;
import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheRequest;
import org.elasticsearch.xpack.core.security.action.privilege.ClearPrivilegesCacheResponse;
import org.elasticsearch.xpack.security.authz.store.CompositeRolesStore;
import org.elasticsearch.xpack.security.authz.store.NativePrivilegeStore;

import java.io.IOException;
import java.util.Arrays;
import java.util.List;

public class TransportClearPrivilegesCacheAction
extends TransportNodesAction<ClearPrivilegesCacheRequest, ClearPrivilegesCacheResponse, ClearPrivilegesCacheRequest.Node, ClearPrivilegesCacheResponse.Node> {

private final NativePrivilegeStore privilegesStore;
private final CompositeRolesStore rolesStore;

@Inject
public TransportClearPrivilegesCacheAction(
ThreadPool threadPool,
ClusterService clusterService,
TransportService transportService,
ActionFilters actionFilters,
NativePrivilegeStore privilegesStore,
CompositeRolesStore rolesStore) {
super(
ClearPrivilegesCacheAction.NAME,
threadPool,
clusterService,
transportService,
actionFilters,
ClearPrivilegesCacheRequest::new,
ClearPrivilegesCacheRequest.Node::new,
ThreadPool.Names.MANAGEMENT,
ClearPrivilegesCacheResponse.Node.class);
this.privilegesStore = privilegesStore;
this.rolesStore = rolesStore;
}

@Override
protected ClearPrivilegesCacheResponse newResponse(
ClearPrivilegesCacheRequest request, List<ClearPrivilegesCacheResponse.Node> nodes, List<FailedNodeException> failures) {
return new ClearPrivilegesCacheResponse(clusterService.getClusterName(), nodes, failures);
}

@Override
protected ClearPrivilegesCacheRequest.Node newNodeRequest(ClearPrivilegesCacheRequest request) {
return new ClearPrivilegesCacheRequest.Node(request);
}

@Override
protected ClearPrivilegesCacheResponse.Node newNodeResponse(StreamInput in) throws IOException {
return new ClearPrivilegesCacheResponse.Node(in);
}

@Override
protected ClearPrivilegesCacheResponse.Node nodeOperation(ClearPrivilegesCacheRequest.Node request, Task task) {
if (request.getApplicationNames() == null || request.getApplicationNames().length == 0) {
privilegesStore.invalidateAll();
} else {
privilegesStore.invalidate(Arrays.asList(request.getApplicationNames()));
ywangd marked this conversation as resolved.
Show resolved Hide resolved
}
rolesStore.invalidateAll();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure about this. It seems like this API ends up doing something other than what it was supposed to, just because we assume that the caller wants it.
I understand why - if the privileges have changed then the roles cache is probably wrong, but it seems like it's chain side-effects together.

Copy link
Member Author

Choose a reason for hiding this comment

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

From just the API point of view, you are right that these two should not be tied together. There are valid use cases when user only wants to actively clear privileges cache. I did this because the two are always tied together in NativePrivilegesStore since it was clearing role cache before my change.

I tried to avoid nested callbacks (clear role cache then clear privileges cache). It seems OK and more efficient by just looking at NativePrivilegesStore. But it does feel wrong from pure API side.

I could either just go with nested callback or create a transport layer only action to clear both caches. So it is not exposed at REST layer and still has the efficiency on transport layer. But this does lead some code redundancy. Another option is to have a query parameter for the clear privilege cache API. When set to true, it clears both caches. What do you think?

return new ClearPrivilegesCacheResponse.Node(clusterService.localNode());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,7 @@ public static void buildRoleFromDescriptors(Collection<RoleDescriptor> roleDescr
final Set<String> applicationPrivilegeNames = applicationPrivilegesMap.values().stream()
.flatMap(Collection::stream)
.collect(Collectors.toSet());
// Role itself is cached, so skipping caching for application privileges
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 follow this comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! This comment is obsolete and should be removed. It is for the initial iteration where I tried to separate this usages from others.

privilegeStore.getPrivileges(applicationNames, applicationPrivilegeNames, ActionListener.wrap(appPrivileges -> {
applicationPrivilegesMap.forEach((key, names) -> ApplicationPrivilege.get(key.v1(), names, appPrivileges)
.forEach(priv -> builder.addApplicationPrivilege(priv, key.v2())));
Expand Down
Loading