-
Notifications
You must be signed in to change notification settings - Fork 25.8k
Update View CRUD Actions to be Index Actions #141570
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
base: main
Are you sure you want to change the base?
Conversation
5986166 to
bbeb05a
Compare
| * @param resolveAliases, aliases will be included in the result, if false we treat them like they do not exist | ||
| * @param allowEmptyExpressions, when an expression does not result in any indices, if false it throws an error if true it treats it as | ||
| * an empty result | ||
| * @param resolveViews, views will be included in the result, if false we treat them like they do not exist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IndicesOptions.wildcardOptions defines how wildcard expressions should be expanded. The new resolveViews wildcard option is added to allow index expressions to be resolved to views by the authorization engine. In addition to that the new flag on the wilcard index option should:
- Not be populated from REST requests, since there is no support for it in the APIs (it's hardcoded to always resolve views where it's used)
- Not be populated when transport requests are serialized (
writeIndicesOptions) or deserialized (readIndicesOptions) since it's only used for local actions (this may change if delete starts supporting multi-target syntax since the request will be sent to the master node and authorized there).
| defaultResolveAliasForThisRequest, | ||
| randomBoolean() | ||
| randomBoolean(), | ||
| false // Specifying views in the create snapshot request is not supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifying views in a restore/create snapshot request is currently not part of the views feature. Not sure if we want to support this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Views are already part of snapshots in that we capture all views as part of the global state in a snapshot, from
elasticsearch/server/src/main/java/org/elasticsearch/cluster/metadata/ViewMetadata.java
Line 90 in 8b34252
| return Metadata.ALL_CONTEXTS; |
I think eventually the ESQL team will need to decide how to treat views individually, since they cannot be selected currently other than all-or-nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@craigtaverner FYI might be useful with an issue/task for this.
| instance.indicesOptions().ignoreAliases() == false, | ||
| randomBoolean() | ||
| randomBoolean(), | ||
| false // Specifying views in the restore snapshot request is not supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifying views in a restore/create snapshot request is currently not part of the views feature. Not sure if we want to support this?
|
|
||
| public static class Request extends AcknowledgedRequest<Request> { | ||
| public static class Request extends AcknowledgedRequest<Request> implements IndicesRequest { | ||
| // TODO this currently doesn't support multi-target syntax, but should probably if action.destructive_requires_name=false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be done in a follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| List<String> names = request.names(); | ||
| if (names.isEmpty()) { | ||
| String[] names = request.indices(); | ||
| // TODO currently doesn't support multi-target when security is off |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code will be replaced in the follow up PR since it adds the new resolve views transport action that can handle multi-target syntax even if security is off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is multi-target syntax? Do you mean wildcards? Will FROM view* not work until the next PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.elastic.co/docs/reference/elasticsearch/rest-apis/api-conventions#api-multi-index
It will work with security on, but not with security off since we don't do any wildcard expansion here. I'll add it in the next PR since it contains the view expression resolver transport action that can be reused here that covers both security on and off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FROM view* still works as it currently does, this is only the GET /_query/view/<name> API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd rephrase the comment as:
// TODO fully support wildcarded expressions when security is disabled
It's less about multi-syntax because that can be an expression like logs,logs-a which works fine and more about wildcards like logs*
|
Pinging @elastic/es-security (Team:Security) |
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
Hi @jfreden, I've created a changelog YAML for you. |
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
9749ae2 to
5cccaea
Compare
craigtaverner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I did have some questions, comments and suggestions.
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/view/DeleteViewAction.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/view/PutViewAction.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/view/GetViewAction.java
Outdated
Show resolved
Hide resolved
| ); | ||
| GetViewAction.Request req = new GetViewAction.Request(RestUtils.getMasterNodeTimeout(request)); | ||
| var requestedViews = Strings.splitStringByCommaToArray(request.param("name")); | ||
| req.indices(requestedViews.length == 0 ? new String[] { "*" } : requestedViews); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this not be done inside the constructor for GetViewAction.Request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I notice that the tests that call this constructor do not follow this pattern and pass in the "*", they just leave it empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see the TransportGetViewAction explicitly checks for null, empty and "*" and treats them the same. Why do we bother with "*" here then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're telling the authorization engine that we want all views we're authorized to see. I'll investigate if there is a better way to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to handle it like this. Is it deliberate that we are not supporting _all as another way to get back all views?
| List<String> names = request.names(); | ||
| if (names.isEmpty()) { | ||
| String[] names = request.indices(); | ||
| // TODO currently doesn't support multi-target when security is off |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is multi-target syntax? Do you mean wildcards? Will FROM view* not work until the next PR?
...ugin/esql/qa/security/src/javaRestTest/java/org/elasticsearch/xpack/esql/EsqlSecurityIT.java
Show resolved
Hide resolved
...ugin/esql/qa/security/src/javaRestTest/java/org/elasticsearch/xpack/esql/EsqlSecurityIT.java
Show resolved
Hide resolved
…view/DeleteViewAction.java Co-authored-by: Craig Taverner <craig@amanzi.com>
…view/PutViewAction.java Co-authored-by: Craig Taverner <craig@amanzi.com>
dakrone
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I left only super minor comments
server/src/main/java/org/elasticsearch/cluster/metadata/IndexNameExpressionResolver.java
Outdated
Show resolved
Hide resolved
| defaultResolveAliasForThisRequest, | ||
| randomBoolean() | ||
| randomBoolean(), | ||
| false // Specifying views in the create snapshot request is not supported |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Views are already part of snapshots in that we capture all views as part of the global state in a snapshot, from
elasticsearch/server/src/main/java/org/elasticsearch/cluster/metadata/ViewMetadata.java
Line 90 in 8b34252
| return Metadata.ALL_CONTEXTS; |
I think eventually the ESQL team will need to decide how to treat views individually, since they cannot be selected currently other than all-or-nothing.
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/view/GetViewAction.java
Outdated
Show resolved
Hide resolved
|
|
||
| import static org.elasticsearch.rest.RestRequest.Method.DELETE; | ||
|
|
||
| @ServerlessScope(Scope.PUBLIC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@craigtaverner I removed public here until we have a complete story for Serverless/CPS so this doesn't accidentally get exposed.
n1v0lg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Perhaps this is going in a follow-up PR but it'd be great to add some view specific tests to IndicesAndAliasesResolverTests:
- SearchRequest (resolveViews=false) returns no views even if they exist
- GetViewRequest returns views if they exist
And some more variations thereof.
| ImmutableOpenMap<String, IndexMetadata> indices | ||
| ) { | ||
| if (indices.isEmpty()) { | ||
| if (indices.isEmpty() && viewMetadata.views().isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the reason that views are included here but data streams are not is that views can exist without indices whereas data streams can't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I got some strange results when testing views without creating indices first. Since a view can be valid and even return results without referencing an index, we need this check.
| if (names == null || names.length == 0 || (names.length == 1 && Regex.isMatchAllPattern(names[0]))) { | ||
| views = viewService.getMetadata(projectId).views().values(); | ||
| } else { | ||
| } else if (names != IndicesAndAliasesResolverField.NO_INDICES_OR_ALIASES_ARRAY) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit preference would be to use Array.equals(names, IndicesAndAliasesResolverField.NO_INDICES_OR_ALIASES_ARRAY) here
| entry("cross_cluster_replication", CROSS_CLUSTER_REPLICATION), | ||
| entry("cross_cluster_replication_internal", CROSS_CLUSTER_REPLICATION_INTERNAL) | ||
| entry("cross_cluster_replication_internal", CROSS_CLUSTER_REPLICATION_INTERNAL), | ||
| entry("manage_view", MANAGE_VIEW), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no views feature flag behind which we could hide these right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is. Refactored this to be behind a feature flag.
| List<String> names = request.names(); | ||
| if (names.isEmpty()) { | ||
| String[] names = request.indices(); | ||
| // TODO currently doesn't support multi-target when security is off |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd rephrase the comment as:
// TODO fully support wildcarded expressions when security is disabled
It's less about multi-syntax because that can be an expression like logs,logs-a which works fine and more about wildcards like logs*
| assertThat(views.getFirst().get("name"), equalTo("view-user2")); | ||
| } | ||
| { | ||
| var resp = getView("test-admin", null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to cover _all and "*" here as well.
| var respMap = entityAsMap(resp); | ||
| var views = (List<Map<String, Object>>) respMap.get("views"); | ||
| assertThat(views.size(), equalTo(3)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also cover:
- calling get view on an index, data stream, or alias returns an error; same for mixing views and indices
- a wildcard case that exercises the no_indices path (i.e. user without permissions on any views targeted by a wildcard expression gets an empty result)
- mix of wildcard expressions and concrete expressions
| ); | ||
| GetViewAction.Request req = new GetViewAction.Request(RestUtils.getMasterNodeTimeout(request)); | ||
| var requestedViews = Strings.splitStringByCommaToArray(request.param("name")); | ||
| req.indices(requestedViews.length == 0 ? new String[] { "*" } : requestedViews); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to handle it like this. Is it deliberate that we are not supporting _all as another way to get back all views?
The PR updates View CRUD actions to be Index actions, adds new named index privileges for the new actions and adds support to the authorization engine to resolve views.
Index Actions
DeleteViewAction,GetViewAction,PutViewActionare now implemented as index actions.New Privileges
Authorization Engine
resolveViewsboolean toIndicesOptions.WildcardOptions.IndexNameExpressionResolverandIndexAbstractionResolverrespect resolveViews when resolving index expressions.resolveViews=trueFollow up work
CI Failures
After the views PR was merged, the open security model PR tests started failing with failures related to the put views action. Along with the delete views action, it's the only action that isn't local since it needs to update cluster state on the master node. In the initial views PR it's a cluster action, but in this PR it's updated to be an index (view) action (so it has a new name). There is a problem in the mixed cluster scenario, where the cluster is being upgraded and is trying to call the new index action, but the master has not yet been upgraded, so it can't find it. Since this is behind a feature flag there is no need to add any additional protection, these tests should start working as soon as this has been merged.