add request labeling service/framework #14388

kaushalmahi12 · 2024-06-17T04:49:56Z

Description

This PR provides the foundational support to add multitenant labels to ThreadContext. This foundation provides a way to implement the LabelingPlugin to define how to compute these labels using implicit/explicit attributes.

Related Issues

Resolves #13341

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

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.

Signed-off-by: Kaushal Kumar <[email protected]>

github-actions · 2024-06-17T05:39:25Z

❌ Gradle check result for 4c3bdd7: 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?

kaushalmahi12 · 2024-06-17T23:31:47Z

@msfroh @sohami @jainankitk @reta
Can you look into this PR ?

ansjcy · 2024-06-18T00:30:12Z

server/src/main/java/org/opensearch/querygroup/LabelingService.java

+ * this class will facilitate access and interactions to different implementations
+ * Usage: This class should be used as a member to orchestrate the working of your {@link LabelingPlugin}
+ */
+public class LabelingService {


I would still recommend adding the request keyword in all the related names, cause essentially what we are doing is adding labels to requests (search for now, but bulk, index etc in the future). Maybe RequestLabelingService ?

ansjcy · 2024-06-18T00:38:21Z

server/src/main/java/org/opensearch/querygroup/LabelingService.java

+    /**
+     * Enum to define what all are currently implementing the plugin
+     */
+    public enum LabelingImplementationType {


This should be moved to a separate file? Also are we suggesting a 1-1 mapping here? (a plugin can only be used for one certain purpose and one implementation can only be mapped to 1 plugin) Can multiple plugin have QUERY_GROUP_RESOURCE_MANAGEMENT type? If not, why?

Yes, I want this to be present since invoke triggers for feature might not be uniform, e,g; for QueryGrouping We will need to label the request at two places i,e; at co-ordinator request and at shard request.
Given this will fall on the hot path it will be redundant re-compute the labels for all of the implementations

Regarding whether there should be a 1:1 mapping between feature and Implementation.
Do you think it makes sense to utilise the same implementation across multiple features ? I am assuming each feature could be specific

I think it make sense to allow each feature to have their specific implementation.

ansjcy · 2024-06-18T00:41:04Z

server/src/main/java/org/opensearch/querygroup/LabelingService.java

+    public LabelingService(List<LabelingPlugin> loadedPlugins) {
+        implementations = new EnumMap<>(LabelingImplementationType.class);
+        for (LabelingPlugin plugin : loadedPlugins) {
+            if (implementations.containsKey(plugin.getImplementationName())) {


This might introduce some confusion and ineffiency when implementing a new LabelingPlugin, in theory, if I'm developing a new LabelingPlugin, I shouldn't be worried about if someone else has already implemented a plugin for the same type.

Why do you think so, can you elaborate more with help of examples ?

For example, if in your feature/plugin A, you alreayd have a LabelingImplementationType1 implemetation and you have already registered with the LabelingService. When I'm implementing my feature/plugin B, if I also want to have a LabelingImplementationType1, but I don't know there's already a LabelingImplementationType1 exists, there might be possibilities your implementation will override my implementation in prod but I won't be able to capture that during development.

ansjcy · 2024-06-18T00:43:57Z

server/src/main/java/org/opensearch/querygroup/LabelingService.java

+        if (plugin == null) {
+            throw new IllegalArgumentException(type + " implementation is not enabled");
+        }
+        plugin.labelRequest(request, threadContext);


I'm still in favor of having multiple plugins labeling a requests at the same time like https://github.com/opensearch-project/OpenSearch/pull/14282/files#diff-f02ca59c8d99a70fdc277b1b311ba8fc0135920ef5a114578187e9388e922f6bR60,
Can we define something like a "order/priority" that a plugin can apply the labels like we did in the ActionFilter:

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

Line 50 in 112704b

int order();

What you are thinking of is I think from a different perspective, I think you are assuming that all of the consuming feature needs are uniform but they are not as I had explained it above for QueryGrouping feature.

So rather than push-down-to-syn mechanism to apply this event. I am more inclined towards pull-to-sync mechanism

I see, could you also give some examples on where this function will be called? if we are using this on demand, we might need to pass labelingService to all plugins that need it? possibly by adding it as a default constructor parameter for LabelingPlugin ?

ansjcy · 2024-06-18T00:46:02Z

server/src/main/java/org/opensearch/plugins/LabelingPlugin.java

+     * @param request
+     * @param threadContext
+     */
+    void labelRequest(final IndicesRequest request, final ThreadContext threadContext);


Are we planning to create a new plugin for the query grouping feature?

public class QueryGroupPlugin extends Plugin implements LabelingPlugin { ... }

Or are we planning to reuse any existing ones?

I think this doesn't concern the specific feature. Yes, This feature is supposed to be plugin driven to maintain segregation and strong cohesion with the features.

Have you considered trying to use the ActionPlugin.getActionFilters extension point? This allows you to run a filter before a transport action is executed on a node.

Here's an example of how Security overrides it and registers a SecurityFilter. The filter needs to implement a method called apply that has access to the Request object. You can also instantiate a filter within createComponents where you have access to the threadPool that core passes to plugins.

💯 this is the way

I see!, I missed this comment. This could also work
@cwperks Will the threadContext have the authN/authZ info ?

I see the following code to sort the filters based on the order() method implementation

public ActionFilters(Set<ActionFilter> actionFilters) { this.filters = actionFilters.toArray(new ActionFilter[0]); Arrays.sort(filters, new Comparator<ActionFilter>() { @Override public int compare(ActionFilter o1, ActionFilter o2) { return Integer.compare(o1.order(), o2.order()); } }); }

Security must be the first action filter to run since that is where authz is performed and that is also where the authenticated user info is serialized to the thread context

Security uses RequestHandlerWrapper if I am not mistaken

@reta Authentication is done in the RestHandlerWrapper and authorization is done as an action filter.

Since 2.11, authentication has been moved further up the netty pipeline into the header_verifier step.

@kaushalmahi12 Security reserves the lowest order (first)

@Override public int order() { return Integer.MIN_VALUE; }

Any other filter should have a higher value to occur after the SecurityFilter

The SecurityFilter calls on PrivilegesEvaluator.evaluate which is the method in which the authenticated user is serialized into the ThrreadContext. This is the line in PrivilegesEvaluator.evaluate where setUserInfoIntoThreadContext is called.

@reta Authentication is done in the RestHandlerWrapper and authorization is done as an action filter.

Ah I see, thanks @cwperks , I think the authentication does set the principal, right?

ansjcy · 2024-06-18T00:53:32Z

server/src/main/java/org/opensearch/querygroup/LabelingService.java

+    }
+
+    /**
+     * populates the threadContext with the labels yielded by the {@param type} against the {@link LabelingHeader} keys


We are delegating the logic to "add labels into thread context" to plugin implementation, which IMO is a little bit dangerous. Plugins can add their arbitrary labels into thread context without complying with the LabelingHeader enum. I think we should have the logic to check/validate and put labels into thread context in this service, plugins should only compute the labels they want to add. Like: https://github.com/opensearch-project/OpenSearch/pull/14282/files#diff-f02ca59c8d99a70fdc277b1b311ba8fc0135920ef5a114578187e9388e922f6bR66

Good Point! I agree with this, I think with return types we can easily enforce the header types as key.

reta · 2024-06-18T13:00:33Z

@msfroh @sohami @jainankitk @reta

@kaushalmahi12 @ansjcy I honestly don't understand the design decisions of this change:

we talked about a possibility to associate tags (key=value pairs) with any REST request, ActionPlugin::getRestHandlerWrapper does that
we talked about a possibility to submit tags (key=value pairs) with any REST request, ActionPlugin::getRestHeaders does that
we talked about a possibility to propagate the the tags (key=value pairs) down the call chain and node boundaries, ThreadContextStatePropagator does that

We have all the pieces already (I believe), they just have to be linked together.

ansjcy · 2024-06-18T23:36:23Z

@reta yes, we can utilize ActionPlugin to associate tags with a request body/header, which can solve the first approach (Let the client do it) in @msfroh 's RFC (#13341). For this PR and also this one (#14282) we are trying to explore potential solutions to add tags based on rules, which solve the second approach (Rule-based labeling).

So, there can be two different approaches to do rule-based labeling, one is let the plugins expose their rules and the core service apply the rules for each request if possible (#14282). Another approach is using plugin interface to make it more generic, especially for Query Grouping features use cases (@kaushalmahi12 can explain more about the advantages here).

reta · 2024-06-18T23:40:01Z

So, there can be two different approaches to do rule-based labeling, one is let the plugins expose their rules and the core service apply the rules for each request if possible (#14282). Another approach is using plugin interface to make it more generic, especially for Query Grouping features use cases (@kaushalmahi12 can explain more about the advantages here).

@ansjcy my point is - there are all the tools in the disposable, there is not need to build framework or alike in core (in my opinion), it is sufficient to have separate plugin (labeling-plugin or tagging-plugin, you name it) that would do labels based on rules or other criteria.

kaushalmahi12 · 2024-06-24T21:50:30Z

@reta Thanks for providing detailed response. I agree that existing constructs might be enough for majority of the use cases but it will not suffice for something which want to classify/tag requests at co-ordinator and data node level.

For QueryGrouping (Query Sandboxing previously) we need to label the request at both levels for resource tracking and enforcement purposes.

I see that the ActionPlugin::getRestHandlerWrapper might do the job for co-ordinator requests but Based on the javadoc of this method only one installed plugin can implement this.

/**
     * Returns a function used to wrap each rest request before handling the request.
     * The returned {@link UnaryOperator} is called for every incoming rest request and receives
     * the original rest handler as it's input. This allows adding arbitrary functionality around
     * rest request handlers to do for instance logging or authentication.
     * A simple example of how to only allow GET request is here:
     * <pre>
     * {@code
     *    UnaryOperator<RestHandler> getRestHandlerWrapper(ThreadContext threadContext) {
     *      return originalHandler -> (RestHandler) (request, channel, client) -> {
     *        if (request.method() != Method.GET) {
     *          throw new IllegalStateException("only GET requests are allowed");
     *        }
     *        originalHandler.handleRequest(request, channel, client);
     *      };
     *    }
     * }
     * </pre>
     *
     * Note: Only one installed plugin may implement a rest wrapper.
     */
    default UnaryOperator<RestHandler> getRestHandlerWrapper(ThreadContext threadContext) {
        return null;
    }

The implementation here should ideally perform authN/authZ I think, ref: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java#L666.

reta · 2024-06-27T12:36:11Z

@reta Thanks for providing detailed response. I agree that existing constructs might be enough for majority of the use cases but it will not suffice for something which want to classify/tag requests at co-ordinator and data node level.

@kaushalmahi12 if I am not mistaken, the headers are propagated from REST (coordinator) to transport layer and we support that, what is missing here to classify/tag requests at co-ordinator and data node level?

I see that the ActionPlugin::getRestHandlerWrapper might do the job for co-ordinator requests but Based on the javadoc of this method only one installed plugin can implement this.

I completely missed that part, thank you for correcting me, the ActionFilter should be the right extension point I believe (same ActionPlugin), filters have no such constraints.

default List<ActionFilter> getActionFilters() {

jainankitk · 2024-06-18T01:21:23Z

server/src/main/java/org/opensearch/querygroup/LabelingHeader.java

+ * compatible open source license.
+ */
+
+package org.opensearch.querygroup;


We should use the wlm as package name. QueryGroup as an independent entity is not enough to warrant a plugin/top level package

jainankitk · 2024-06-27T22:01:44Z

server/src/main/java/org/opensearch/querygroup/LabelingHeader.java

+/**
+ * Main enum define various headers introduced by {@link org.opensearch.plugins.LabelingPlugin}s
+ */
+public enum LabelingHeader {


As @ansjcy mentioned elsewhere, might be good to add Request prefix to class names

jainankitk · 2024-06-27T22:06:23Z

server/src/main/java/org/opensearch/querygroup/LabelingHeader.java

+ * Main enum define various headers introduced by {@link org.opensearch.plugins.LabelingPlugin}s
+ */
+public enum LabelingHeader {
+    QUERY_GROUP_ID("queryGroupId");


Ideally this enum should be part of core as it is used by the RequestLabelingService, but it has plugin implementation specific values. Hence, I am slightly confused whether this should be in core or part of plugin

I think this should be part of core since these headers will be consumed in core.

jainankitk · 2024-06-27T22:09:51Z

server/src/main/java/org/opensearch/querygroup/LabelingService.java

+     * Enum to define what all are currently implementing the plugin
+     */
+    public enum LabelingImplementationType {
+        QUERY_GROUP_RESOURCE_MANAGEMENT,


Again if LabelingImplementationType class is part of core, does the plugin developer need to modify the core for adding their specific value.

This construct is little debatable, and probably need more thought. Let me think a bit about this

jainankitk · 2024-06-27T23:01:30Z

@kaushalmahi12 - It seems we are doing too much in single PR which is probably adding to the confusion. Ideally there should be two separate PRs, one adding the support for labeling extensions within core, second one demonstrating the usage of LabelingService from QueryGroupPlugin

kaushalmahi12 · 2024-07-01T17:42:18Z

@reta Yes! I was also thinking of using ActionFilter first but since ActionFilter is used to allow/disallow transport actions.
Do you think this would be the right construct to leverage this use case ?
In some use cases we might also need the proper ordering for the execution of these ActionFilters which could be imposed using the int order() method.

IMO This approach provides more flexibility on how and when a plugin can label/tag the requests.

reta · 2024-07-01T21:16:27Z

@reta Yes! I was also thinking of using ActionFilter first but since ActionFilter is used to allow/disallow transport actions.
Do you think this would be the right construct to leverage this use case ?

I think so, it looks right for a job

In some use cases we might also need the proper ordering for the execution of these ActionFilters which could be imposed using the int order() method.

We do not provide any ordering guarantees and we could not fulfill this requirement fairly. Why do you need ordering?

kaushalmahi12 · 2024-07-01T22:58:16Z

@reta We will need the ordering because we want the authN/authZ information present in the context. That is why we want this logic to be executed after the authN/authZ filter.
Now that I remember the ActionFilter might not do the job since we wouldn't have access to the ThreadPool/ThreadContext in this apply method to assign labels for QueryGrouping use case.

reta · 2024-07-02T00:13:21Z

@reta We will need the ordering because we want the authN/authZ information present in the context

@kaushalmahi12 the use case does not feel right, we have security plugin in place to handle such cases and it uses the proper abstraction

jainankitk · 2024-07-02T01:40:49Z

@reta We will need the ordering because we want the authN/authZ information present in the context.

@kaushalmahi12 - I don't think we should enforce the ordering assumption between plugins.

Now that I remember the ActionFilter might not do the job since we wouldn't have access to the ThreadPool/ThreadContext in this apply method to assign labels for QueryGrouping use case.

This is a limitation of ActionFilter which makes it difficult to leverage for adding label to search request.

@kaushalmahi12 the use case does not feel right, we have security plugin in place to handle such cases and it uses the proper abstraction

@reta - Labeling logic should not be part of security plugin IMO, and others plugins might not have access to the security context. Do you have any suggestions other than building this in core?

reta · 2024-07-02T12:38:19Z

Now that I remember the ActionFilter might not do the job since we wouldn't have access to the ThreadPool/ThreadContext in this apply method to assign labels for QueryGrouping use case.

@kaushalmahi12 as per my understanding, we enrich request with the headers (tags) which would be propagated to thread context (using ThreadContextStatePropagator).

@reta - Labeling logic should not be part of security plugin IMO, and others plugins might not have access to the security context. Do you have any suggestions other than building this in core?

@jainankitk I don't understand the dependency on authZ/authN and need for ordering here: we run action filters one by one so the end result should be the (thread) context populated with all necessary data before the request is processed on coordinator.

jainankitk · 2024-07-02T19:38:18Z

@jainankitk I don't understand the dependency on authZ/authN and need for ordering here:

@reta - The labeling logic will be based on the security context. For example - if the principal is Joe, the tenant is A. If the principal is Dave, tenant is B.

we run action filters one by one so the end result should be the (thread) context populated with all necessary data before the request is processed on coordinator.

As mentioned above, the labeling action filter will need the principal information provided by the security plugin for populating the tenant header.

reta · 2024-07-02T20:24:00Z

@reta - The labeling logic will be based on the security context. For example - if the principal is Joe, the tenant is A. If the principal is Dave, tenant is B.

@jainankitk here is my understanding of the flow:

the security plugin is using the getRestHandlerWrapper and will be run before filters (if I am not mistaken), the principal / tenant will be present in the context if available
the security plugin could be removed from the cluster so the principal / tenant will not present in the context

If I am not mistaken, the security plugin uses thread context to share principal information, so the labeling / tagging plugin could use it.

opensearch-trigger-bot · 2024-08-03T15:21:36Z

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

kaushalmahi12 added 4 commits June 15, 2024 01:25

add labeling service

c0177e2

Signed-off-by: Kaushal Kumar <[email protected]>

remove * import

ae494f5

Signed-off-by: Kaushal Kumar <[email protected]>

add labeling service tests

b9ea384

Signed-off-by: Kaushal Kumar <[email protected]>

spotlessApply

4c3bdd7

Signed-off-by: Kaushal Kumar <[email protected]>

kaushalmahi12 requested review from anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, dbwiddis, dreamer-89, gbbafna, kotwanikunal, mch2, msfroh, nknize, owaiskazi19, reta, Rishikesh1159, sachinpkale, saratvemulapalli, shwetathareja, sohami, tlfeng and VachaShah as code owners June 17, 2024 04:49

kaushalmahi12 changed the title ~~add labeling service~~ add request labeling service Jun 17, 2024

kaushalmahi12 changed the title ~~add request labeling service~~ add request labeling service/framework Jun 17, 2024

ansjcy reviewed Jun 18, 2024

View reviewed changes

ansjcy mentioned this pull request Jun 25, 2024

[META] Multi-tenancy support in OpenSearch #13516

Open

5 tasks

jainankitk reviewed Jun 27, 2024

View reviewed changes

kaushalmahi12 mentioned this pull request Jul 1, 2024

add multitenant support for search workloads #13713

Closed

8 tasks

opensearch-trigger-bot bot added the stalled Issues that have stalled label Aug 3, 2024

add request labeling service/framework #14388

Are you sure you want to change the base?

add request labeling service/framework #14388

Conversation

kaushalmahi12 commented Jun 17, 2024

Description

Related Issues

Check List

github-actions bot commented Jun 17, 2024

kaushalmahi12 commented Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

ansjcy Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansjcy Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansjcy Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaushalmahi12 Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwperks Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaushalmahi12 Jun 18, 2024 • edited Loading

Choose a reason for hiding this comment

reta commented Jun 18, 2024 • edited Loading

ansjcy commented Jun 18, 2024

reta commented Jun 18, 2024

kaushalmahi12 commented Jun 24, 2024 • edited Loading

reta commented Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jainankitk commented Jun 27, 2024

kaushalmahi12 commented Jul 1, 2024 • edited Loading

reta commented Jul 1, 2024

kaushalmahi12 commented Jul 1, 2024 • edited Loading

reta commented Jul 2, 2024

jainankitk commented Jul 2, 2024

reta commented Jul 2, 2024

jainankitk commented Jul 2, 2024

reta commented Jul 2, 2024 • edited Loading

opensearch-trigger-bot bot commented Aug 3, 2024

kaushalmahi12 commented Jun 17, 2024 •

edited

Loading

ansjcy Jun 18, 2024 •

edited

Loading

ansjcy Jul 1, 2024 •

edited

Loading

ansjcy Jun 18, 2024 •

edited

Loading

kaushalmahi12 Jul 2, 2024 •

edited

Loading

cwperks Jul 4, 2024 •

edited

Loading

kaushalmahi12 Jun 18, 2024 •

edited

Loading

reta commented Jun 18, 2024 •

edited

Loading

kaushalmahi12 commented Jun 24, 2024 •

edited

Loading

reta commented Jun 27, 2024 •

edited

Loading

kaushalmahi12 commented Jul 1, 2024 •

edited

Loading

kaushalmahi12 commented Jul 1, 2024 •

edited

Loading

reta commented Jul 2, 2024 •

edited

Loading