Removes the use of the authorized indices list for bulk items authorization

[albertzaharovits]

This PR reuses the resolved indices of bulk shard requests for authorizing their items.

Background

We use two sets of indices in authorization:

• "authorizedIndices" this is the long, troublesome, list that is constructed based on the action, the list of indices in the cluster state, and the user's roles. Crucially, it is not related to the request's indices expression.
• "resolvedIndices" this is the request's indices expression where wildcards have been replaced by the matching items from the aforementioned "authorizedIndices" list

For this PR the observation is that both the BulkShardRequest and its constituent BulkItemRequests cannot contain wildcards, therefore evaluating the resolved indices does not necessitate using the authorized indices.

The ultimate goal is to avoid computing the authorized indices list as much as possible, possibly even replacing it with a predicate and a one time traversal of the metadata#getIndicesLookup() names.

[elasticmachine]

Pinging @elastic/es-security (Team:Security)

[albertzaharovits]

combine the two async suppliers into one.

[ywangd]

LGTM

I read through the changes several times and I am convinced that it is safe. IIUC, there is no real functional change, e.g. it does not avoid loading the authorized indices when authorizing bulk items. I think it might be worth to clarify it in case people look for performance improvements.

[ywangd]

The change here relies on the fact that the last argument authorizedIndices is not used in resolveIndicesAndAliases. Therefore, we can pass localIndices to it. In fact, we can even just pass null. I think this effect is somewhat hidden and hard for another person to comprehend. It works for now. But I am concerned that it could get misused in future (e.g. due to refactoring etc). Can we make it more obvious? I don't have great suggestions, maybe have a separate method IndicesAndAliasesResolver#resolveWritableIndices?

Also, the first sentence of the PR description

This PR reuses the resolved indices of bulk shard requests for authorizing their items.

is not entirely accurate because there is no actual reuse, but rather not needed at all, i.e. the last argument can be null. It would be great if it can be made clearer as well. Thanks!

[albertzaharovits]

Yang, from this comment, I get the impression that you don't like the approach I took in this PR, but you have still approved.
My goal is to avoid passing the authorized indices list computed for the parent BulkShardRequest when resolving the indices for the containing BulkItemRequests. As you say, what we replace it with is not important since it is not used.

I think we have two options:

• have a new method for IndicesAndAliasesResolver#resolveIndicesAndAliases for BulItemRequests only
• use the existing method but pass a different argument

These are the two big options that I have weighted.
I stand by my choice and I can get into details if you are interested, but I think it would help if you also state your preference so that we can compare them.

This PR reuses the resolved indices of bulk shard requests for authorizing their items.

is not entirely accurate because there is no actual reuse, but rather not needed at all, i.e. the last argument can be null. It would be great if it can be made clearer as well. Thanks!

I think it is accurate because this is what the PR does, it reuses the said variable as an argument.
The fact that the parameter is not used and can be null is not a change that this PR does.

[ywangd]

To clarify myself: I am happy with the overall goal of this PR, i.e. IIUC, removing the need of having a separate authorizedIndicesSupplier and passing it around. That's why I ticked approval.

The concern that I have is only with what we should pass to resolveIndicesAndAliases now that authorizedIndices is no longer available in authorizeBulkItems. I think passing localIndices is not the right answer because:

1. The parameter is called authorizedIndices and this causes confusion
2. It works because the passed-in argument is never used for this specific case. I am aware that it is an existing behaviour that the method resolveIndicesAndAliases sometimes does not use authorizedIndices. But the difference is: before this change, the callers do not need to know whether the argument is going to be used by the callee. They work off the method signature (a contract) and just pass in whatever is asked consistently in all call sites. But the current change makes the caller aware of this implementation detail. If the method implementation changes in future, it could cause subtle bugs. I think if the caller relies on the fact that the argument is not used, we should make it clearer, e.g. passing a null instead of localIndices.
3. If we just read the changes literally, resolved indices indeed get "reused" as the method argument. But this "reuse" is superficial since underlying it is never used. It is not really much different than a placeholder to satisfy the method signature (and compiler). So if it is indeed more of a placeholder, we should make the point clearer.

Based on the above, I think passing null as the last argument to resolveIndicesAndAliases feels better. Imagine if we need to write javadocs for resolveIndicesAndAliases, a nullable argument is likely easier to explain than localIndices.

In longer term, we might still want to think about refactoring existing methods to help reduce the complexity and increases readibility. But I can understand that more refactoring is outside scope of this PR.

[tvernum]

I don't follow what the purpose of this PR is.
Is it simply a refactoring to allow us to do something in the future?

It doesn't reduce the number of times we call loadAuthorizedIndices, it just hides it away inside the resolvedIndicesAsyncSupplier.

[albertzaharovits]

I read through the changes several times and I am convinced that it is safe. IIUC, there is no real functional change, e.g. it does not avoid loading the authorized indices when authorizing bulk items. I think it might be worth to clarify it in case people look for performance improvements.

There should be no behavior change indeed. I will change the description of this PR to specifically mention that no performance improvements are to be expected. Is this how you suggest we clarify it?

I don't follow what the purpose of this PR is.
Is it simply a refactoring to allow us to do something in the future?
It doesn't reduce the number of times we call loadAuthorizedIndices, it just hides it away inside the resolvedIndicesAsyncSupplier.

Yes, the purpose of this PR is to allow refactoring in the future.
If authorized indices is traversed only once, it is not required to build an in-memory list and then traversing it.
This PR avoid passing the list as a parameter when it is not used.
We can then more easily follow its lifetime.
The loadAuthorizedIndices is already hidden inside the resolvedIndicesAsyncSupplier . This PR doesn't change it.
In fact, passing both as arguments to the same method is error prone.

[albertzaharovits]

@tvernum to be clear, I'm going to wait for your approval as well, unless you say otherwise.

[albertzaharovits]

This got merged in some other form.