Explicit index resolution API

[nibix]

Description

This PR introduces a mechanism that allows ActionFilter implementations to retrieve information about the actual indices a transport action will operate on - based on the current request. So far, the ActionFilter had access to the index information that is directly available in the ActionRequest objects. However, transport actions have quite varying opinions on how to interpret this information. Some evaluate index patterns, some only date math, some allow remote indices, and some only allow a single concrete index. The new mechanism creates interfaces that give transport actions the ability to communicate their interpretation explicitly.

This PR does only add the interface, but not any implementation that uses the new information. The first implementation that consumes the information will be in the security plugin; a draft PR that shows how the information will be processed is located at opensearch-project/security#5399.

Intended use

The primary intended client will be the security plugin. In order to implement index-based access controls, the security plugin needs a reliable way to retrieve information about the indices a request refers to.

At the moment, this needs to be hard-coded for each known action request in the security plugin. This results in a very large class containing many special cases: https://github.com/opensearch-project/security/blob/c51ce46dc0d64326fe9f719e83f2cbb53147117a/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java#L652

This brings several issues:

• It is fragile. Changes to action behavior in core or the additions of new actions might require the modification of the code in the security plugin. This is likely to be forgotten.
• It is slow. Currently, in order to be as safe as possible, the security plugin assembles a big chunk of information for each request. Especially on clusters with many indices, this creates a significant performance impact.

See opensearch-project/security#5367 for more details.

Reading this PR

This is is a large PR, as it adds both the interfaces and infrastructure and also the individual method implementations to TransportAction classes. When reviewing, you should probably start with the inferfaces and infrastructure; if you got an overview of this, you can go ahead to the individual implementations.

Main components

TransportIndicesResolvingAction

This PR introduces a new Java interface that can be implemented by TransportActions:

OpenSearch/server/src/main/java/org/opensearch/action/support/TransportIndicesResolvingAction.java

Lines 14 to 30 in 09a308b

	/**
	* An additional interface that should be implemented by TransportAction implementations which need to resolve
	* IndicesRequests or other action requests which specify indices. This interface allows other components to retrieve
	* precise information about the indices an action is going to operate on. This is particularly useful for access
	* control implementations, but can be also used for other purposes, such as monitoring, audit logging, etc.
	* <p>
	* Classes implementing this interface should make sure that the reported indices are also actually the indices
	* the action will operate on. The best way to achieve this, is to move the index extraction code from the execute
	* methods into reusable methods and to depend on these both for execution and reporting.
	*/
	public interface TransportIndicesResolvingAction<Request extends ActionRequest> {
	/**
	* Returns the actual indices the action will operate on, given the specified request and cluster state.
	*/
	ResolvedIndices resolveIndices(Request request);
	}

The method resolveIndices expects a request object and is supposed to return the indices, aliases and/or data streams, the respective transport action would operate on, if the execute() method is called.

ActionRequestMetadata

A new class ActionRequestMetadata is introduced which is passed as additional parameter to all ActionFilter.apply() implementations:

OpenSearch/server/src/main/java/org/opensearch/action/support/ActionFilter.java

Lines 56 to 63 in b008fd2

	<Request extends ActionRequest, Response extends ActionResponse> void apply(
	Task task,
	String action,
	Request request,
	ActionRequestMetadata<Request, Response> actionRequestMetadata,
	ActionListener<Response> listener,
	ActionFilterChain<Request, Response> chain
	);

OpenSearch/server/src/main/java/org/opensearch/action/support/ActionRequestMetadata.java

Lines 21 to 76 in b008fd2

	public class ActionRequestMetadata<Request extends ActionRequest, Response extends ActionResponse> {
	/**
	* Returns an empty meta data object which will just report unknown results.
	*/
	public static <Request extends ActionRequest, Response extends ActionResponse> ActionRequestMetadata<Request, Response> empty() {
	@SuppressWarnings("unchecked")
	ActionRequestMetadata<Request, Response> result = (ActionRequestMetadata<Request, Response>) EMPTY;
	return result;
	}
	private static final ActionRequestMetadata<?, ?> EMPTY = new ActionRequestMetadata<>(null, null);
	private final TransportAction<Request, Response> transportAction;
	private final Request request;
	private OptionallyResolvedIndices resolvedIndices;
	ActionRequestMetadata(TransportAction<Request, Response> transportAction, Request request) {
	this.transportAction = transportAction;
	this.request = request;
	}
	/**
	* If the current action request references indices, this method actually referenced indices. That means that any
	* expressions or patterns will be resolved.
	* <p>
	* If the request cannot reference indices OR if the respective action does not support resolving of requests,
	* this returns an {@link OptionallyResolvedIndices} with unknown = true. If indices can be resolved, actually
	* a {@link ResolvedIndices} object will be returned.
	*/
	public OptionallyResolvedIndices resolvedIndices() {
	if (!(transportAction instanceof TransportIndicesResolvingAction<?>)) {
	return OptionallyResolvedIndices.unknown();
	}
	if (this.resolvedIndices != null) {
	return this.resolvedIndices;
	} else {
	return resolveIndices();
	}
	}
	/**
	* Performs the actual index resolution. Index resolution can be relatively costly on big clusters, so we
	* perform it lazily only when requested.
	*/
	private OptionallyResolvedIndices resolveIndices() {
	@SuppressWarnings("unchecked")
	TransportIndicesResolvingAction<Request> indicesResolvingAction = (TransportIndicesResolvingAction<Request>) this.transportAction;
	OptionallyResolvedIndices result = indicesResolvingAction.resolveIndices(request);
	this.resolvedIndices = result;
	return result;
	}
	}

OptionallyResolvedIndices, ResolvedIndices

The ActionRequestMetadata class gives ActionFilter implementations access to instances of ResolvedIndices and OptionallyResolvedIndices. The reason why the class OptionallyResolvedIndices exists is the following: There can be situations when the resolved indices are not known. For example, that's the case when a transport action does not implement the TransportIndicesResolvingAction interface. The existance of the OptionallyResolvedIndices class provides a type safe and null safe way of checking whether the information is known or not. Implementations that use the classes should implement this pattern:

void process(OptionallyResolvedIndices optionallyResolvedIndices) {
   if (optionallyResolvedIndices instanceof ResolvedIndices resolvedIndices) {
     // Now it is safe to work on the resolved index information
   } else {
     // No resolved index information present.
   }
}

Still, also the OptionallyResolvedIndices class implements a couple of methods which allow certain checks to be done without the instanceof checks.

OpenSearch/server/src/main/java/org/opensearch/cluster/metadata/OptionallyResolvedIndices.java

Lines 18 to 117 in b008fd2

	/**
	* A class that possibly encapsulates resolved indices. See the documentation of {@link ResolvedIndices} for a full
	* description.
	* <p>
	* In contrast to ResolvedIndices, objects of this class may convey the message "the resolved indices are unknown".
	* This may be for several reasons:
	* <ul>
	* <li>The information can be only obtained on a master node</li>
	* <li>The particular action does not implement the TransportIndicesResolvingAction interface</li>
	* <li>It is infeasible to collect the information</li>
	* </ul>
	* For authorization purposes, the case of unknown resolved indices should be usually treated as a "requires
	* privileges for all indices" case.
	* <p>
	* The class {@link ResolvedIndices} extends OptionallyResolvedIndices. A safe usage pattern would be thus:
	* <pre>
	* if (optionallyResolvedIndices instanceof ResolvedIndices resolvedIndices) {
	* Set&lt;String;gt names = resolvedIndices.local().names();
	* }
	* </pre>
	*/
	@ExperimentalApi
	public class OptionallyResolvedIndices {
	private static final OptionallyResolvedIndices NOT_PRESENT = new OptionallyResolvedIndices();
	public static OptionallyResolvedIndices unknown() {
	return NOT_PRESENT;
	}
	public OptionallyResolvedIndices.Local local() {
	return Local.NOT_PRESENT;
	}
	@Override
	public String toString() {
	return "ResolvedIndices{unknown=true}";
	}
	@Override
	public boolean equals(Object other) {
	if (!(other instanceof OptionallyResolvedIndices otherResolvedIndices)) {
	return false;
	}
	return this.local() == otherResolvedIndices.local();
	}
	@Override
	public int hashCode() {
	return 92;
	}
	/**
	* Represents the local (i.e., non-remote) indices referenced by the respective request.
	*/
	@ExperimentalApi
	public static class Local {
	private static final OptionallyResolvedIndices.Local NOT_PRESENT = new OptionallyResolvedIndices.Local();
	/**
	* Returns all the local names. These names might be indices, aliases or data streams, depending on the usage.
	* <p>
	* In case this is an isUnknown() object, this will return a set of all concrete indices on the cluster (incl.
	* hidden and closed indices). This might be a large object. Be prepared to handle such a large object.
	* <p>
	* <strong>This method will be only rarely needed. In most cases <code>ResolvedIndices.names()</code> will be sufficient.</strong>
	*/
	public Set<String> names(ClusterState clusterState) {
	return clusterState.metadata().getIndicesLookup().keySet();
	}
	/**
	* Returns always true. For the unknown resolved indices, we always assume that these are not empty.
	*/
	public boolean isEmpty() {
	return false;
	}
	/**
	* Returns always true, as we need to assume that an index is potentially contained if the set of indices is unknown.
	*/
	public boolean contains(String name) {
	return true;
	}
	/**
	* Returns always true, as we need to assume that an index is potentially contained if the set of indices is unknown.
	*/
	public boolean containsAny(Collection<String> names) {
	return true;
	}
	/**
	* Returns always true, as we need to assume that an index is potentially contained if the set of indices is unknown.
	*/
	public boolean containsAny(Predicate<String> namePredicate) {
	return true;
	}
	}
	}

OpenSearch/server/src/main/java/org/opensearch/cluster/metadata/ResolvedIndices.java

Lines 31 to 573 in b008fd2

	/**
	* A class that encapsulates resolved indices. Resolved indices do not any wildcards or date math expressions.
	* However, in contrast to the concept of "concrete indices", resolved indices might not exist yet, or might
	* refer to aliases or data streams.
	* <p>
	* ResolvedIndices classes are primarily created by the resolveIndices() method in TransportIndicesResolvingAction.
	* <p>
	* Instances of ResolvedIndices are immutable. It will be not possible to modify the returned collection instances.
	* All methods which add/modify elements will return a new ResolvedIndices instance.
	* <p>
	* How resolved indices are obtained depends on the respective action and the associated requests:
	* <ul>
	* <li>If a request carries an index expression (i.e, might contain patterns or date math expressions), the index
	* expression must be resolved using the appropriate index options; these might be request-specific or action-specific.</li>
	* <li>Some requests already carry concrete indices; in these cases, the names of the concrete indices can be
	* just taken without further evaluation</li>
	* </ul>
	*/
	@ExperimentalApi
	public class ResolvedIndices extends OptionallyResolvedIndices {
	public static ResolvedIndices of(String... indicesAliasesAndDataStreams) {
	return new ResolvedIndices(new Local(Set.of(indicesAliasesAndDataStreams), null, Map.of()), Remote.EMPTY);
	}
	public static ResolvedIndices of(Index... indices) {
	return new ResolvedIndices(
	new Local.Concrete(
	Set.of(indices),
	Stream.of(indices).map(Index::getName).collect(Collectors.toUnmodifiableSet()),
	null,
	Map.of(),
	List.of()
	),
	Remote.EMPTY
	);
	}
	public static ResolvedIndices of(Collection<String> indicesAliasesAndDataStreams) {
	return new ResolvedIndices(new Local(Set.copyOf(indicesAliasesAndDataStreams), null, Map.of()), Remote.EMPTY);
	}
	public static ResolvedIndices of(Local local) {
	return new ResolvedIndices(local, Remote.EMPTY);
	}
	public static OptionallyResolvedIndices unknown() {
	return OptionallyResolvedIndices.unknown();
	}
	public static ResolvedIndices ofNonNull(String... indicesAliasesAndDataStreams) {
	Set<String> indexSet = new HashSet<>(indicesAliasesAndDataStreams.length);
	for (String index : indicesAliasesAndDataStreams) {
	if (index != null) {
	indexSet.add(index);
	}
	}
	return new ResolvedIndices(new Local(Collections.unmodifiableSet(indexSet), null, Map.of()), Remote.EMPTY);
	}
	private final Local local;
	private final Remote remote;
	private ResolvedIndices(Local local, Remote remote) {
	this.local = local;
	this.remote = remote;
	}
	@Override
	public Local local() {
	return this.local;
	}
	public Remote remote() {
	return this.remote;
	}
	public ResolvedIndices withRemoteIndices(Map<String, OriginalIndices> remoteIndices) {
	if (remoteIndices.isEmpty()) {
	return this;
	}
	Map<String, OriginalIndices> newRemoteIndices = new HashMap<>(remoteIndices);
	newRemoteIndices.putAll(this.remote.clusterToOriginalIndicesMap);
	return new ResolvedIndices(this.local, new Remote(Collections.unmodifiableMap(newRemoteIndices)));
	}
	/**
	* Returns a ResolvedIndices object associated with the given OriginalIndices object for the local part. This is only for
	* convenience, no semantics are implied.
	*/
	public ResolvedIndices withLocalOriginalIndices(OriginalIndices originalIndices) {
	return new ResolvedIndices(this.local.withOriginalIndices(originalIndices), this.remote);
	}
	public ResolvedIndices withLocalSubActions(ActionType<?> actionType, ResolvedIndices.Local local) {
	return new ResolvedIndices(this.local.withSubActions(actionType, local), this.remote);
	}
	public boolean isEmpty() {
	return this.local.isEmpty() && this.remote.isEmpty();
	}
	@Override
	public String toString() {
	return "ResolvedIndices{" + "local=" + local + ", remote=" + remote + '}';
	}
	@Override
	public boolean equals(Object other) {
	if (!(other instanceof ResolvedIndices otherResolvedIndices)) {
	return false;
	}
	return this.local.equals(otherResolvedIndices.local) && this.remote.equals(otherResolvedIndices.remote);
	}
	@Override
	public int hashCode() {
	return this.local.hashCode() + this.remote.hashCode() * 31;
	}
	/**
	* Represents the local (i.e., non-remote) indices referenced by the respective request.
	*/
	@ExperimentalApi
	public static class Local extends OptionallyResolvedIndices.Local {
	protected final Set<String> names;
	protected final OriginalIndices originalIndices;
	protected final Map<String, Local> subActions;
	private Set<String> namesOfIndices;
	public static Local of(Collection<String> names) {
	return new Local(Set.copyOf(names), null, Map.of());
	}
	public static Local of(String... names) {
	return of(Arrays.asList(names));
	}
	/**
	* Creates a new instance.
	* <p>
	* Note: The caller of this method must make sure that the passed objects are immutable.
	* For this reason, this constructor is private. This contract is guaranteed by the static
	* constructor methods in this file.
	*/
	private Local(Set<String> names, OriginalIndices originalIndices, Map<String, Local> subActions) {
	this.names = names;
	this.originalIndices = originalIndices;
	this.subActions = subActions;
	}
	/**
	* Returns all the local names. These names might be indices, aliases or data streams, depending on the usage.
	*
	* @return an unmodifiable set of names of indices, aliases and/or data streams.
	*/
	public Set<String> names() {
	return this.names;
	}
	/**
	* Returns all the local names. These names might be indices, aliases or data streams, depending on the usage.
	*
	* @return an array of names of indices, aliases and/or data streams.
	*/
	public String[] namesAsArray() {
	return this.names().toArray(new String[0]);
	}
	/**
	* Returns all the local names. These names might be indices, aliases or data streams, depending on the usage.
	* <p>
	* In case this is an isUnknown() object, this will return a set of all concrete indices on the cluster (incl.
	* hidden and closed indices). This might be a large object. Be prepared to handle such a large object.
	* <p>
	* <strong>This method will be only rarely needed. In most cases <code>ResolvedIndices.names()</code> will be sufficient.</strong>
	*/
	@Override
	public Set<String> names(ClusterState clusterState) {
	return this.names;
	}
	/**
	* Returns all the local names. Any data streams or aliases will be replaced by the member index names.
	* In contrast to namesOfConcreteIndices(), this will keep the names of non-existing indices and will never
	* throw an exception.
	*
	* @return an unmodifiable set of index names
	*/
	public Set<String> namesOfIndices(ClusterState clusterState) {
	Set<String> result = this.namesOfIndices;
	if (result == null) {
	Map<String, IndexAbstraction> indicesLookup = clusterState.metadata().getIndicesLookup();
	result = new HashSet<>(this.names.size());
	for (String name : this.names) {
	IndexAbstraction indexAbstraction = indicesLookup.get(name);
	if (indexAbstraction == null) {
	// We keep the names of non existing indices
	result.add(name);
	} else if (indexAbstraction instanceof IndexAbstraction.Index) {
	// For normal indices, we just keep its name
	result.add(name);
	} else {
	// This is an alias or data stream
	for (IndexMetadata index : indexAbstraction.getIndices()) {
	result.add(index.getIndex().getName());
	}
	}
	}
	result = Collections.unmodifiableSet(result);
	this.namesOfIndices = result;
	}
	return result;
	}
	/**
	* Returns any OriginalIndices object associated with this object.
	* Note: This is just a convenience method for code that passes around ResolvedIndices objects for managing
	* information. This object will be only present if you add it to the object.
	*/
	public OriginalIndices originalIndices() {
	return this.originalIndices;
	}
	/**
	* Sub-actions can be used to specify indices which play a different role in the action processing.
	* For example, the swiss-army-knife IndicesAliases action can delete indices. The subActions() property
	* can be used to specify indices with such special roles.
	*/
	public Map<String, Local> subActions() {
	return this.subActions;
	}
	/**
	* Returns true if there are no local indices.
	*/
	@Override
	public boolean isEmpty() {
	return this.names.isEmpty();
	}
	/**
	* Returns true if the local names contain an entry with the given name.
	*/
	@Override
	public boolean contains(String name) {
	return this.names.contains(name);
	}
	/**
	* Returns true if the local names contain any of the specified names.
	*/
	@Override
	public boolean containsAny(Collection<String> names) {
	return names.stream().anyMatch(this.names::contains);
	}
	/**
	* Returns true if any of the local names match the given predicate.
	*/
	@Override
	public boolean containsAny(Predicate<String> namePredicate) {
	return this.names.stream().anyMatch(namePredicate);
	}
	/**
	* Returns a ResolvedIndices.Local object associated with the given OriginalIndices object. This is only for
	* convenience, no semantics are implied.
	*/
	public ResolvedIndices.Local withOriginalIndices(OriginalIndices originalIndices) {
	return new Local(this.names, originalIndices, this.subActions);
	}
	public ResolvedIndices.Local withSubActions(String key, ResolvedIndices.Local local) {
	Map<String, Local> subActions = new HashMap<>(this.subActions);
	subActions.put(key, local);
	return new Local(this.names, this.originalIndices, Collections.unmodifiableMap(subActions));
	}
	public ResolvedIndices.Local withSubActions(ActionType<?> actionType, ResolvedIndices.Local local) {
	return this.withSubActions(actionType.name(), local);
	}
	@Override
	public String toString() {
	if (this.subActions.isEmpty()) {
	return "{names=" + names() + "}";
	} else {
	return "{names=" + names() + ", subActions=" + subActions + '}';
	}
	}
	@Override
	public boolean equals(Object other) {
	if (!(other instanceof ResolvedIndices.Local otherLocal)) {
	return false;
	}
	return this.names.equals(otherLocal.names) && this.subActions.equals(otherLocal.subActions);
	}
	@Override
	public int hashCode() {
	return this.names.hashCode() + this.subActions.hashCode() * 31;
	}
	/**
	* This is a specialization of the {@link ResolvedIndices.Local} class which additionally
	* carries {@link Index} objects. The {@link IndexNameExpressionResolver} produces such objects.
	* <p>
	* <strong>Important:</strong> The methods that give access to the concrete indices can throw
	* exceptions such as {@link org.opensearch.index.IndexNotFoundException} if the concrete indices
	* could not be determined. The methods from the Local super class, such as names(), still give
	* access to index information.
	*/
	@ExperimentalApi
	public static class Concrete extends Local {
	private static final Concrete EMPTY = new Concrete(Set.of(), Set.of(), null, Map.of(), List.of());
	public static Concrete empty() {
	return EMPTY;
	}
	public static Concrete of(Index... concreteIndices) {
	return new Concrete(
	Set.of(concreteIndices),
	Stream.of(concreteIndices).map(Index::getName).collect(Collectors.toSet()),
	null,
	Map.of(),
	List.of()
	);
	}
	/**
	* Creates a new RemoteIndices.Local.Concrete object with the given concrete indices and names.
	* This is primarily used by IndexNameExpressionResolver to construct return values, that's why it is
	* package private. There may be more names than concrete indices, for example when a referenced index does
	* not exist. Thus, names should be usually a super set of concreteIndices. This method does not verify
	* that, it is the duty of the caller to make this sure.
	*/
	static Concrete of(Set<Index> concreteIndices, Set<String> names) {
	return new Concrete(Set.copyOf(concreteIndices), Set.copyOf(names), null, Map.of(), List.of());
	}
	private final Set<Index> concreteIndices;
	private final List<RuntimeException> resolutionErrors;
	private Concrete(
	Set<Index> concreteIndices,
	Set<String> names,
	OriginalIndices originalIndices,
	Map<String, Local> subActions,
	List<RuntimeException> resolutionErrors
	) {
	super(names, originalIndices, subActions);
	this.concreteIndices = concreteIndices;
	this.resolutionErrors = resolutionErrors;
	}
	/**
	* Returns the concrete indices. This might throw an exception if there were issues during index resolution.
	* If you need access to index information while avoiding exceptions, use the names() method instead.
	*
	* @throws org.opensearch.index.IndexNotFoundException This exception is thrown for several conditions:
	* 1. If one of the index expression pointed to a missing index, alias or data stream and the IndicesOptions
	* used during resolution do not allow such a case. 2. If the set of resolved concrete indices is empty and
	* the IndicesOptions used during resolution do not allow such a case.
	* @throws IllegalArgumentException if one of the aliases resolved to multiple indices and
	* IndicesOptions.FORBID_ALIASES_TO_MULTIPLE_INDICES was set.
	*/
	public Set<Index> concreteIndices() {
	checkResolutionErrors();
	return this.concreteIndices;
	}
	public Index[] concreteIndicesAsArray() {
	return this.concreteIndices().toArray(Index.EMPTY_ARRAY);
	}
	public Set<String> namesOfConcreteIndices() {
	return this.concreteIndices().stream().map(Index::getName).collect(Collectors.toSet());
	}
	public String[] namesOfConcreteIndicesAsArray() {
	return this.concreteIndices().stream().map(Index::getName).toArray(String[]::new);
	}
	@Override
	public ResolvedIndices.Local.Concrete withOriginalIndices(OriginalIndices originalIndices) {
	return new Concrete(this.concreteIndices, this.names, originalIndices, this.subActions, resolutionErrors);
	}
	@Override
	public ResolvedIndices.Local withSubActions(String actionType, ResolvedIndices.Local local) {
	Map<String, Local> subActions = new HashMap<>(this.subActions);
	subActions.put(actionType, local);
	return new Concrete(this.concreteIndices, this.names, this.originalIndices, subActions, resolutionErrors);
	}
	public ResolvedIndices.Local.Concrete withResolutionErrors(List<RuntimeException> resolutionErrors) {
	if (resolutionErrors.isEmpty()) {
	return this;
	} else {
	return new Concrete(
	this.concreteIndices,
	this.names(),
	originalIndices,
	this.subActions,
	Stream.concat(this.resolutionErrors.stream(), resolutionErrors.stream()).toList()
	);
	}
	}
	public ResolvedIndices.Local.Concrete withResolutionErrors(RuntimeException... resolutionErrors) {
	return withResolutionErrors(Arrays.asList(resolutionErrors));
	}
	public ResolvedIndices.Local.Concrete withoutResolutionErrors() {
	return new Concrete(this.concreteIndices, this.names(), this.originalIndices, this.subActions, List.of());
	}
	@Override
	public boolean equals(Object other) {
	if (!super.equals(other)) {
	return false;
	}
	if (!(other instanceof ResolvedIndices.Local.Concrete otherLocal)) {
	return false;
	}
	return this.concreteIndices.equals(otherLocal.concreteIndices);
	}
	@Override
	public int hashCode() {
	return super.hashCode() * 31 + this.concreteIndices.hashCode();
	}
	List<RuntimeException> resolutionErrors() {
	return this.resolutionErrors;
	}
	private void checkResolutionErrors() {
	if (!this.resolutionErrors.isEmpty()) {
	throw this.resolutionErrors.getFirst();
	}
	}
	@Override
	public String toString() {
	return "{" + "concreteIndices=" + concreteIndices + ", names=" + names() + ", resolutionErrors=" + resolutionErrors + '}';
	}
	}
	}
	/**
	* Represents the remote indices part of the respective request.
	*/
	@ExperimentalApi
	public static class Remote {
	static final Remote EMPTY = new Remote(Collections.emptyMap(), Collections.emptyList());
	private final Map<String, OriginalIndices> clusterToOriginalIndicesMap;
	private List<String> rawExpressions;
	private Remote(Map<String, OriginalIndices> clusterToOriginalIndicesMap) {
	this.clusterToOriginalIndicesMap = clusterToOriginalIndicesMap;
	}
	private Remote(Map<String, OriginalIndices> clusterToOriginalIndicesMap, List<String> rawExpressions) {
	this.clusterToOriginalIndicesMap = clusterToOriginalIndicesMap;
	this.rawExpressions = rawExpressions;
	}
	public Map<String, OriginalIndices> asClusterToOriginalIndicesMap() {
	return this.clusterToOriginalIndicesMap;
	}
	public List<String> asRawExpressions() {
	List<String> result = this.rawExpressions;
	if (result == null) {
	result = this.rawExpressions = buildRawExpressions();
	}
	return result;
	}
	public String[] asRawExpressionsArray() {
	return this.asRawExpressions().toArray(new String[0]);
	}
	public boolean isEmpty() {
	return this.clusterToOriginalIndicesMap.isEmpty();
	}
	@Override
	public String toString() {
	return this.asRawExpressions().toString();
	}
	@Override
	public boolean equals(Object other) {
	if (!(other instanceof ResolvedIndices.Remote otherRemote)) {
	return false;
	}
	return this.asRawExpressions().equals(otherRemote.asRawExpressions());
	}
	@Override
	public int hashCode() {
	return this.asRawExpressions().hashCode();
	}
	private List<String> buildRawExpressions() {
	if (this.clusterToOriginalIndicesMap.isEmpty()) {
	return Collections.emptyList();
	}
	List<String> result = new ArrayList<>();
	for (Map.Entry<String, OriginalIndices> entry : this.clusterToOriginalIndicesMap.entrySet()) {
	for (String remoteIndex : entry.getValue().indices()) {
	result.add(RemoteClusterService.buildRemoteIndexName(entry.getKey(), remoteIndex));
	}
	}
	return Collections.unmodifiableList(result);
	}
	}
	}

IndexNameExpressionResolver

The class IndexNameExpressionResolver has been extended by methods that return ResolvedIndices.Local.Concrete objects. This allows transport actions to use the resolution logic from IndexNameExpressionResolver both for its internal purposes and also for implementing the TransportIndicesResolvingAction interface.

There is one fundamental different in the use of the resolution logic between the actual action execution and the implementation of TransportIndicesResolvingAction: When the actual action is executed, IndexNameExpressionResolver performs certain validation steps: Do indices exist, does the request index fit other specified parameters, etc. For the metadata reporting, however, we want the metadata even when the validation fails. Thus, the returned ResolvedIndices.Local.Concrete provide methods to access validated indices and method to access unvalidated names.

Related Issues

Partially resolves opensearch-project/security#5367

Check List

• Functionality includes testing.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

❌ Gradle check result for 09a308b: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

✅ Gradle check result for e721867: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 80.79710% with 106 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.10%. Comparing base (df704b7) to head (9e8cffa).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...n/indices/alias/TransportIndicesAliasesAction.java	68.96%	13 Missing and 5 partials ⚠️
...rch/cluster/metadata/IndexAbstractionResolver.java	16.66%	8 Missing and 2 partials ⚠️
...g/opensearch/cluster/metadata/ResolvedIndices.java	94.44%	3 Missing and 5 partials ⚠️
.../cluster/metadata/IndexNameExpressionResolver.java	87.27%	4 Missing and 3 partials ⚠️
...script/mustache/TransportSearchTemplateAction.java	0.00%	5 Missing ⚠️
...dmin/indices/rollover/TransportRolloverAction.java	61.53%	5 Missing ⚠️
...on/fieldcaps/TransportFieldCapabilitiesAction.java	73.33%	3 Missing and 1 partial ⚠️
...dices/tiering/TransportHotToWarmTieringAction.java	0.00%	3 Missing ⚠️
...ch/cluster/metadata/OptionallyResolvedIndices.java	81.25%	1 Missing and 2 partials ⚠️
...min/indices/datastream/DataStreamsStatsAction.java	77.77%	1 Missing and 1 partial ⚠️
... and 36 more

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #18523      +/-   ##
============================================
+ Coverage     73.01%   73.10%   +0.09%     
- Complexity    70585    70762     +177     
============================================
  Files          5723     5726       +3     
  Lines        323509   323905     +396     
  Branches      46852    46889      +37     
============================================
+ Hits         236222   236803     +581     
+ Misses        68213    68017     -196     
- Partials      19074    19085      +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[nibix]

@reta @andrross Even though this PR is only a draft yet, I'd like to gather a couple of opinions on the approach. @cwperks recommended you as a good peer to ask :-)

This PR is part of significant performance and robustness improvements for the security plugin.

[cwperks]

As discussed on slack, can this be done in a backward compatible way? There are many existing ActionFilters across the plugins that would need to be updated to account for this change.

[nibix]

I have been thinking about this, but I did not find really good options.

1. We could keep the existing method and add a new one as a default implementation:

public interface ActionFilter {
    
    <Request extends ActionRequest, Response extends ActionResponse> void apply(
        Task task,
        String action,
        Request request,
        ActionListener<Response> listener,
        ActionFilterChain<Request, Response> chain
    );

    default <Request extends ActionRequest, Response extends ActionResponse> void apply(
        Task task,
        String action,
        Request request,
        ActionRequestMetadata<Request, Response> actionRequestMetadata,
        ActionListener<Response> listener,
        ActionFilterChain<Request, Response> chain
    ) {
        this.apply(task, action, request, listener, chain);
    }

This has the advantage that existing code does not need to be touched. However, any code which wants to use the new interface actually has to implement two methods: The first apply() without the new parameter, which would be actually dead code. And the second apply() with the new parameter.

2. To fix this, one could make both methods default methods:

public interface ActionFilter {

    @Deprecated
    default <Request extends ActionRequest, Response extends ActionResponse> void apply(
        Task task,
        String action,
        Request request,
        ActionListener<Response> listener,
        ActionFilterChain<Request, Response> chain
    ) {
        throw new UnsupportedOperationException("Not implemented");
    }

    default <Request extends ActionRequest, Response extends ActionResponse> void apply(
        Task task,
        String action,
        Request request,
        ActionRequestMetadata<Request, Response> actionRequestMetadata,
        ActionListener<Response> listener,
        ActionFilterChain<Request, Response> chain
    ) {
        this.apply(task, action, request, listener, chain);
    }

This would probably work, but it seems to be messy and potentially confusing to developers.

3. That's the reason why I chose the present approach for now. It creates a bit of work, which is however straight-forward. And the result is clean. Actually, back in the ES days, it was a regular occurence that Java APIs changed in breaking ways across minor release versions.

[nibix]

Additionally, the interface is marked as @opensearch.internal which created the impression that there are no compatiblity guarantees here.

[cwperks]

nit: can we rename this variable as it indicates that resolution has completed. Maybe isIndexResolutionCompleted() or something to that effect?

[nibix]

sure!

[cwperks]

I strongly support offloading this to the existing transport actions and obviating the need for custom logic in the security plugin to handle all of the different action types: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java#L645-L840

nit: I see that Request is used as a generic here. Can we shorten that to a single char like R instead?

[nibix]

In the transport action class hierarchy, it seems to be common to use Request and (if needed) Response as the names for the generic parameters:

OpenSearch/server/src/main/java/org/opensearch/action/support/TransportAction.java

Lines 58 to 60 in 63921b8

	@PublicApi(since = "1.0.0")
	public abstract class TransportAction<Request extends ActionRequest, Response extends ActionResponse> {

I have followed this pattern.

[cwperks]

@saratvemulapalli @sohami @owaiskazi19 @dbwiddis @jainankitk wdyt of the proposed changes in this PR?

The main goal is to offload concrete index resolution to the core instead of security knowing how to extract from all different types of requests as it does in https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java#L661-L837

[owaiskazi19]

Using an interface is a good approach for exposing index resolution logic, rather than requiring each plugin to implement it independently. I like the idea of having the core provide this functionality.

@nibix @cwperks The interface can also be extended with other helpful methods for plugins, such as:
getOperationType, requiresClusterState or a method to see if the action involves system indices.

Additionally, we could consider renaming the interface to TransportIndicesAwareAction, making it more flexible and future-proof for supporting additional capabilities.

[nibix]

Thank you for the feedback!

I also believe that there is potential for more meta data retrieval methods.

However, I am wondering whether it might make sense to have separate interfaces for such methods. The reasons:

• Adding methods to interfaces retroactively is difficult, because it requires the immediate adaption of all implementing classes. Alternatively, one can resort to default methods - which is however in my perception mostly a kludge.
• I can imagine that not all transport actions will support the same meta data methods. There are certainly more transport actions that operate on the cluster state than actions that resolve index names.

Thus, maybe, it would be better to define an own interface for each purpose?

[owaiskazi19]

Or we could start with TransportIndicesAwareAction interface first and then if there's a need for more retrieval methods in the future, we can decide on if creating a new interface or adding in TransportIndicesAwareAction would be better.

[nibix]

Sounds good to me! 👍

[aparajita31pandey]

@nibix Can ResolvedIndices comprise of aliases, allIndices, originalRequested and remoteIndices ? Any request with wildcard Index pattern will ResolvedIndices contain all the indices with that wildcard ?

[nibix]

@aparajita31pandey

an ResolvedIndices comprise of aliases, allIndices, originalRequested and remoteIndices ?

The current version has separate attributes for remote indices and local indices, aliases and data streams. Currently, local indices, alises and datastreams are however in one bucket.

Any request with wildcard Index pattern will ResolvedIndices contain all the indices with that wildcard ?

Yes, wildcards will be resolved in the ResolvedIndices object (that's one of the main objectives of the class :)

[opensearch-trigger-bot]

This PR is stalled because it has been open for 30 days with no activity.

[cwperks]

@nibix what is remaining before this can be taken out of Draft? This will be a huge improvement to the way index resolution is currently done.

FYI @kumargu was also dealing with a similar issue in Index-Level Encryption (ILE) plugin so solving this in the core is for sure the right way to go.

[nibix]

@cwperks

what is remaining before this can be taken out of Draft? This will be a huge improvement to the way index resolution is currently done.

Additionally, I need an API extension to let IndexNameExpressionResolver to optionally not fail on non-existant indices. This is also mostly done, I will try to push this ASAP.

Another thing I still have to think about: How to test this.

[nibix]

FYI @kumargu was also dealing with a similar issue in Index-Level Encryption (ILE) plugin so solving this in the core is for sure the right way to go.

I would be interested to learn about the details of this case, just to make sure that we are aligned

[cwperks]

FYI @kumargu was also dealing with a similar issue in Index-Level Encryption (ILE) plugin so solving this in the core is for sure the right way to go.

I would be interested to learn about the details of this case, just to make sure that we are aligned

@kumargu to comment further, but my understanding is they need to rolve the concrete indices from the request to then determine the correct set of keys for decryption (and whether the user has access).

[kumargu]

FYI @kumargu was also dealing with a similar issue in Index-Level Encryption (ILE) plugin so solving this in the core is for sure the right way to go.

I would be interested to learn about the details of this case, just to make sure that we are aligned

@kumargu to comment further, but my understanding is they need to rolve the concrete indices from the request to then determine the correct set of keys for decryption (and whether the user has access).

Hi @nibix , for index-level encryption we need to know -

1. if an index creation request also carries an index setting which tell if the index should be encrypted with an encryption key.
2. if the caller is an anonymous user, in which case, in environment like AWS we don't allow passing an encryption key because we cannot AuthZ against the user.

[cwperks]

FYI @nibix I found another area of the core trying to do extraction of indices from an ActionRequest in an ActionFilter. See: https://github.com/opensearch-project/OpenSearch/blob/main/plugins/workload-management/src/main/java/org/opensearch/plugin/wlm/rule/attribute_extractor/IndicesExtractor.java

@kaushalmahi12 @ruai0511 FYI that IndicesExtractor is missing a lot of cases for resolving to concrete indices from a generic ActionRequest. See the IndexResolverReplacer from the security plugin which currently has the most exhaustive logic. The change @nibix is introducing in this PR is for resolving to concrete indices from any type of transport action associated with indices so WLM would benefit from these changes as well.

@kkhatua tagging you as well.

[nibix]

@kumargu

Hm, so is that only about CreateIndexRequest?

[kumargu]

@kumargu

Hm, so is that only about CreateIndexRequest?

Yes. Index Level Encryption can be only updated on family of CreateIndexRequest.

[github-actions]

❌ Gradle check result for 4266dbc: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for ea276fb: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 004f937: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

✅ Gradle check result for 4c83f2b: SUCCESS

[cwperks]

Apologies @nibix , since this PR changed the ActionFilter extension point I did not want to move forward until the 3.4 version bump. The core is now bumped to 3.4. Could you please sync with the latest from main and I will take another pass first thing tomorrow morning?

[cwperks]

I'm trying to wrap my head around when withLocalSubActions should be used. I understand that some transport actions call others under the hood, but wouldn't that then delegate to the sub-transport action to figure out how to resolve the indices? Why must this always be done from the parent action?

[nibix]

On a high level: Some transport actions perform several kinds of actions without using further underlying transport actions. A very notorious example is https://docs.opensearch.org/latest/im-plugin/index-alias/ ...

For the get aliases action we also have the challenge that the user can request aliases and indices indepentently, this is modeled here.

[github-actions]

✅ Gradle check result for 9e8cffa: SUCCESS