Adds a check to skip serialization-deserialization if request is for same node #2765

DarshitChanpura · 2023-05-11T16:20:05Z

Description

Category:
Bug Fix
Why these changes are required?
To improve performance of large requests handled on same node

Issues Resolved

Resolves [Performance Bug] Repeated deserialization of headers for a direct channel request #2724

Testing

Current test suite should pass.

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

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.

DarshitChanpura

Confirmed working behavior by running some test suites locally. Waiting for GH runner to run complete test suite.

DarshitChanpura · 2023-05-11T16:32:27Z

src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java

-                    if(getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS) == null) {
-                        getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS, request.remoteAddress());
-                    }
+            if(skipSecurityIfDualMode) {


fixed indentation to promote better readability, as it was misaligned in the file

DarshitChanpura · 2023-05-11T16:42:58Z

src/main/java/org/opensearch/security/transport/SecurityInterceptor.java

@@ -127,6 +128,8 @@ public <T extends TransportResponse> void sendRequestDecorate(AsyncSender sender
        final String origCCSTransientMf = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_MASKED_FIELD_CCS);

        final boolean isDebugEnabled = log.isDebugEnabled();
+        final boolean isDirectRequest = HeaderHelper.isDirectRequest(getThreadContext());
+
        try (ThreadContext.StoredContext stashedContext = getThreadContext().stashContext()) {


Could not yet find a solution to not stash the context for same node.

Tried with these changes:

try (ThreadContext.StoredContext stashedContext = (isDirectRequest ? null: getThreadContext().stashContext())) { final TransportResponseHandler<T> restoringHandler = isDirectRequest ? handler : new RestoringTransportResponseHandler<T>(handler, stashedContext);

But the test fails with ClusterManagernotDiscoveredException everytime on test start.

It is perhaps good idea to continue stashing from security point of view too. That way, any existing transients assumed to be cleared are not carry forwarded.

For example let us assume current thread context had a,b,c entries and we explicitly passed around only a & b. Now carry forwarding c may be a security concern.

But the test fails with ClusterManagernotDiscoveredException everytime on test start.

I would guess that this is related to the wrong check for isDirectRequest. But, yes, I would also recommend to stash anyway and just restore the transients which you want to keep. That would be the most conservative and most minimal change.

should we say isLocalNodeRequest to add more readability? or are we categorizing local node request to direct channel through out the code-base ?

cwperks · 2023-05-11T16:46:30Z

src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java

                final String injectedRolesHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_HEADER);
                final String injectedUserHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER_HEADER);

-                if(Strings.isNullOrEmpty(userHeader)) {
+                if(user != null) {


Why remove the empty check here?

Because for direct requests we don't pass headers (which would then be serialized and put as a transient), rather we directly put the transient object, User, in the thread context via sendMessageDecorate in SecurityInterceptor and fetch it as an object from thread context here. By doing so we skip serialization and deserialization of direct requests.

krishna-ggk · 2023-05-12T08:50:11Z

src/main/java/org/opensearch/security/transport/SecurityInterceptor.java

@@ -127,6 +128,8 @@ public <T extends TransportResponse> void sendRequestDecorate(AsyncSender sender
        final String origCCSTransientMf = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_MASKED_FIELD_CCS);

        final boolean isDebugEnabled = log.isDebugEnabled();
+        final boolean isDirectRequest = HeaderHelper.isDirectRequest(getThreadContext());


As brought up in the issue by @mgodwan, the channel type isn't populated in the "send" part and hence isn't the right way to check if it is direct connection.

Yep, that is the wrong end.

Trying to think of a correct check:

In SecurityInterceptor we need to check whether connection.getNode() is equal to the local node, similar to how TransportService is doing it here:

https://github.com/opensearch-project/OpenSearch/blob/1e82b894902a4e34ed2cf71e97511ace72d0205c/server/src/main/java/org/opensearch/transport/TransportService.java#L1620

The local node could be retrieved by clusterService.localNode(). I am not sure, though, whether this is a 100% correct approach, as TransportService as a slightly more involved way to get the local node. See:

https://github.com/opensearch-project/OpenSearch/blob/1e82b894902a4e34ed2cf71e97511ace72d0205c/server/src/main/java/org/opensearch/transport/TransportService.java#L306

Alternatively, one might try to get ahold of a reference to TransportService. However, this only works with hacks in a non-Guice context:

security/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

Line 1198 in 5c63420

final TransportService remoteClusterService, IndicesService indicesService, PitService pitService, ExtensionsManager extensionsManager) {

i originally started out with localNode and connection node comparison, however interpreted the discussion on the GH issue as proposing to use headerhelper and so changed it to that. I have now changed it back to the original comparison.

krishna-ggk · 2023-05-12T09:20:42Z

src/main/java/org/opensearch/security/transport/SecurityInterceptor.java

+                if(isDirectRequest) {
+                    // if request is going to be handled by same node, we directly put transient value.
+                    getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS, transportAddress);
+                } else {
+                    getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS_HEADER, Base64Helper.serializeObject(transportAddress.address()));
+                }


Per entry if-else clauses can be prone to error as it needs matching checks on receiver end. Can we generalize to either use transients vs header for all config entries? That way, there is consistent symmetric code for passing configs.

Something along the lines of

if (isDirect) { putTransient(remoteAddress..); putTransient(securityUser..); putTransient(injectedRoles..); putTransient(injectedUser..); } else { putHeader(remoteAddress..); putHeader(securityUser..); putHeader(injectedRoles..); putHeader(injectedUser..); }

FYI direct !== same node - @DarshitChanpura and I figured this out today after debugging the failing tests:

In OpenSearch, the channelType is a parameter that indicates the type of communication channel used for inter-node communication. There are two types of channels: direct and transport.

direct: This type of channel is used for communication between nodes within the same JVM (Java Virtual Machine). It's used for local node-to-node communication, which means it's used when the nodes are on the same machine or in the same OpenSearch process.

transport: This type of channel is for communication between nodes that are not in the same JVM. It's used for remote node-to-node communication, which means it's used when the nodes are on different machines or in different OpenSearch processes.

In summary, the direct channelType is used for local communication within the same JVM, while the transport channelType is used for remote communication between different JVMs.

krishna-ggk · 2023-05-12T09:35:57Z

src/main/java/org/opensearch/security/transport/SecurityInterceptor.java

@@ -127,6 +128,8 @@ public <T extends TransportResponse> void sendRequestDecorate(AsyncSender sender
        final String origCCSTransientMf = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_MASKED_FIELD_CCS);

        final boolean isDebugEnabled = log.isDebugEnabled();
+        final boolean isDirectRequest = HeaderHelper.isDirectRequest(getThreadContext());
+
        try (ThreadContext.StoredContext stashedContext = getThreadContext().stashContext()) {


It is perhaps good idea to continue stashing from security point of view too. That way, any existing transients assumed to be cleared are not carry forwarded.

For example let us assume current thread context had a,b,c entries and we explicitly passed around only a & b. Now carry forwarding c may be a security concern.

DarshitChanpura · 2023-05-12T15:42:51Z

CI is currently failing with following to change introduced in core: opensearch-project/OpenSearch#7465

 java.lang.NoClassDefFoundError: org/opensearch/core/common/io/stream/BaseStreamInput

DarshitChanpura · 2023-05-12T17:11:38Z

CI is currently failing with following to change introduced in core: opensearch-project/OpenSearch#7465
 java.lang.NoClassDefFoundError: org/opensearch/core/common/io/stream/BaseStreamInput

Seems like a stale artifact issue that will be fixed on its own.

hardik-k-shah · 2023-05-12T22:08:47Z

src/main/java/org/opensearch/security/transport/SecurityInterceptor.java

@@ -142,17 +144,15 @@ public <T extends TransportResponse> void sendRequestDecorate(AsyncSender sender
                            || k.equals(ConfigConstants.OPENDISTRO_SECURITY_DOC_ALLOWLIST_HEADER)
                            || k.equals(ConfigConstants.OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONE)
                            || k.equals(ConfigConstants.OPENDISTRO_SECURITY_DLS_MODE_HEADER)
-                            || k.equals(ConfigConstants.OPENDISTRO_SECURITY_DLS_FILTER_LEVEL_QUERY_HEADER)                            
+                            || k.equals(ConfigConstants.OPENDISTRO_SECURITY_DLS_FILTER_LEVEL_QUERY_HEADER)


possible to keep formatting changes separate from this PR?

hardik-k-shah · 2023-05-12T22:12:30Z

src/main/java/org/opensearch/security/transport/SecurityInterceptor.java

@@ -127,6 +128,8 @@ public <T extends TransportResponse> void sendRequestDecorate(AsyncSender sender
        final String origCCSTransientMf = getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_MASKED_FIELD_CCS);

        final boolean isDebugEnabled = log.isDebugEnabled();
+        final boolean isDirectRequest = HeaderHelper.isDirectRequest(getThreadContext());
+
        try (ThreadContext.StoredContext stashedContext = getThreadContext().stashContext()) {


should we say isLocalNodeRequest to add more readability? or are we categorizing local node request to direct channel through out the code-base ?

hardik-k-shah · 2023-05-12T22:15:21Z

src/main/java/org/opensearch/security/transport/SecurityInterceptor.java

        String userHeader = getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER);

-        if(userHeader == null) {
+        if(isDirectRequest) {


can be merge with above if..else..

krishna-ggk

Can we add tests to ensure both direct and non-direct paths are explicitly validated?

parasjain1 · 2023-05-15T11:45:38Z

src/main/java/org/opensearch/security/transport/SecurityInterceptor.java

            }
-            else if(StringUtils.isNotEmpty(injectedRolesString)) {
+        } else {


Do we not intend to retain the existing userHeader == null check for this else code block, or are we assuming that this scenario used to arise only for direct requests?

parasjain1

Left a comment for a query I have regarding the changes.

codecov · 2023-05-16T20:06:18Z

Codecov Report

Merging #2765 (691019b) into main (4eef662) will increase coverage by 0.00%.
The diff coverage is 72.34%.

@@            Coverage Diff            @@
##               main    #2765   +/-   ##
=========================================
  Coverage     62.31%   62.31%           
- Complexity     3337     3342    +5     
=========================================
  Files           266      266           
  Lines         19650    19658    +8     
  Branches       3329     3336    +7     
=========================================
+ Hits          12244    12250    +6     
  Misses         5779     5779           
- Partials       1627     1629    +2

Impacted Files	Coverage Δ
...rch/security/transport/SecurityRequestHandler.java	`54.72% <47.61%> (-2.24%)`	⬇️
...search/security/transport/SecurityInterceptor.java	`72.25% <90.90%> (-0.80%)`	⬇️
.../opensearch/security/OpenSearchSecurityPlugin.java	`84.31% <100.00%> (+0.09%)`	⬆️

... and 5 files with indirect coverage changes

krishna-ggk · 2023-05-18T16:23:39Z

src/main/java/org/opensearch/security/transport/SecurityInterceptor.java

+        boolean isSameNodeRequest = false;
+        try {
+            isSameNodeRequest = cs.localNode().equals(connection.getNode()); // using DiscoveryNode equals comparison here
+        } catch (AssertionError e) {
+            // do nothing
+        }


This doesn't seem right - which assertion is failing?

I'm not sure but in my research, I found that in some instances the ClusterState was not initialized (which is accessed via state()) and this caused a bunch of tests to fail. My understanding was that since cs is initiated upon node spin-up, the state info will always be available, but that doesn't seem to be the case.

In order to form the initial cluster state, it is already necessary to exchange transport messages which pass through here. TransportService uses a different approach to retrieve the reference to the local node. As this also needs to pass through the early transport messages, it is likely that here the local node information is already available:

https://github.com/opensearch-project/OpenSearch/blob/1e82b894902a4e34ed2cf71e97511ace72d0205c/server/src/main/java/org/opensearch/transport/TransportService.java#L306

@nibix Apologies, but I'm not super clear on whether you want the implementation to be changed.

Well, I think catching an Error, especially an AssertionError is a code smell. It is problematic for two reasons:

You could be also catching other AssertionErrors which are more serious and not meant to be ignored.

Assertions are usually only enabled in dev environments. However, that does not mean that the error will just go away in a prod environment. It is possible that it is just deferred to a different code place which then bails with an NPE or similar thing. Thus, code that works in dev could break in prod.

Thus, I would recommend to use an approach which does not require catching AssertionErrors.

Hm, I guess that will be also the very early initialization phase when localNode in TransportService is null, right?

I would still believe that TransportService will know earlier about localNode than ClusterService. Also, if we use the localNode from TransportService, we could use a simple null check instead of catching the AssertionError.

Will the null check not defeat the purpose? How will it be different from catching an AssertionError when cluster state is not accessible from this class?

I don't think it will defeat the purpose. With the null check, you have full control, you know exactly what you are checking, that is the localNode value.

When catching an AssertionError, you have less control:

The source of the assertion error can be also another code place, thus by ignoring it you could obscure other severe issues.

Also, as discussed above the assertion is not performed on prod systems, which gives you a QA liability if you are relying on it.

thank you for the explanation!. I'll update the condition here. If you have any resources handy that I can take a look for assertions and their usage in Prod vs staging, it would be helpful for me.

I have not found good resources on this so far, but I am still looking. I can hopefully update you later on this.

src/main/java/org/opensearch/security/transport/SecurityInterceptor.java

DarshitChanpura · 2023-05-24T15:20:50Z

@nibix After updating GuiceHolder to contain an instance of TransportService and use that instance to fetch the localNode information, I'm getting different results:

cs.localNode() = {DiscoveryNode@12410} "{node_utest_n1_fnull_t353609252151333_num3}{cHyj4v__T_-MMs2s_____w}{_Rk3Y5VWTUeuzUrwCfr9Rg}{127.0.0.1}{127.0.0.1:6212}{m}{shard_indexing_pressure_enabled=true}"

connection.getNode() = {DiscoveryNode@12410} "{node_utest_n1_fnull_t353609252151333_num3}{cHyj4v__T_-MMs2s_____w}{_Rk3Y5VWTUeuzUrwCfr9Rg}{127.0.0.1}{127.0.0.1:6212}{m}{shard_indexing_pressure_enabled=true}"

localNode = {DiscoveryNode@12404} "{node_utest_n1_fnull_t353609252151333_num1}{1S3HEQAAQACnOySu_____w}{LUJGxVdsSoqtJ3kB3RZ4wg}{127.0.0.1}{127.0.0.1:7352}{d}{shard_indexing_pressure_enabled=true}"

cs.localNode() (the localNode value from ClusterState) matches the value of the node the connection originated from (resulting in passing tests), but localNode value (retrieved from TransportService instance) is not matching connection.getNode() resulting in test failures.
Am i missing something here in terms of fetching localNode info from TransportService?

nibix · 2023-05-24T16:37:37Z

@nibix After updating GuiceHolder to contain an instance of TransportService and use that instance to fetch the localNode information, I'm getting different results

Oh, hm, interesting. I think I have an idea where this is coming from. Am I right in the assumption that you get that error when testing in a junit based test with a cluster embedded in the JVM?

If yes: This indeed collides with the GuiceHolder. Unfortunately, the GuiceHolder is at closer consideration a quite dirty hack to get ahold of dependencies which area only available by deprndecy injection. It uses for this a static attribute in the SecurityPlugin class. While in an JVM embedded cluster the nodes and also the instances of the SecurityPlugin can coexist very well together, this static class is a collision. The different nodes write in an undetermined fashion references to their own TransportService into it.

That's why you see other results there, at least if my assumption is correct.

Of course the more important part of the consideration would be "how can this be fixed". To be honest I also don't know at the moment. I will have to look at the code and think about it for a while. 😔

DarshitChanpura · 2023-05-25T14:08:48Z

Oh, hm, interesting. I think I have an idea where this is coming from. Am I right in the assumption that you get that error when testing in a junit based test with a cluster embedded in the JVM?

Yes, I saw this while running one of the failing unit tests.

If yes: This indeed collides with the GuiceHolder. Unfortunately, the GuiceHolder is at closer consideration a quite dirty hack to get ahold of dependencies which area only available by deprndecy injection. It uses for this a static attribute in the SecurityPlugin class. While in an JVM embedded cluster the nodes and also the instances of the SecurityPlugin can coexist very well together, this static class is a collision. The different nodes write in an undetermined fashion references to their own TransportService into it.

If that is indeed the case, then do you think it might be worthwhile to look into using cluster state() as an alternative for getting localNode reference, cause that seems to be available at almost all time (except during cluster bootstrap using it Zen Discover mechanism, although I'm not a 100% sure whether the request will flow through security with some cluster state info during bootstrap).

Of course the more important part of the consideration would be "how can this be fixed". To be honest I also don't know at the moment. I will have to look at the code and think about it for a while. 😔

Thank you for helping with this.

Signed-off-by: Craig Perkins <[email protected]>

Update logic in receiver to look for either transient headers or non-transient headers

cwperks · 2023-07-05T18:09:32Z

src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java

@@ -142,7 +141,31 @@ protected void messageReceivedDecorate(
            }

            // bypass non-netty requests
-            if (channelType.equals("direct")) {
+            if (getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER) != null


This logic in the receiver was simplified using @krishna-ggk 's suggestion here: #2765 (comment)

The receiver does not need any logic to determine where the request came from, if the transient headers are present then there is no need to deserialize. If the serialized (non-transient) headers are present then it will deserialize them.

DarshitChanpura · 2023-07-06T13:58:52Z

@opensearch-project/security Would you please re-review this?

stephen-crawford · 2023-07-07T19:02:03Z

src/main/java/org/opensearch/security/transport/SecurityInterceptor.java

            if (origUser != null) {
-                getThreadContext().putHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER, Base64Helper.serializeObject(origUser));
+                // if request is going to be handled by same node, we directly put transient value as the thread context is not going to be
+                // stah.


Suggested change

// stah.

// stashed.

stephen-crawford

Looks good to me, one comment where I think we want to remove a comment from above some code you changed.

stephen-crawford · 2023-07-07T19:26:42Z

src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java

@@ -272,58 +284,10 @@ protected void messageReceivedDecorate(

                // network intercluster request or cross search cluster request
                // CS-SUPPRESS-SINGLE: RegexpSingleline Used to allow/disallow TLS connections to extensions
-                if (HeaderHelper.isInterClusterRequest(getThreadContext())
+                if (!(HeaderHelper.isInterClusterRequest(getThreadContext())


We either need to update the comment above this or remove the exclamation point I think.

DarshitChanpura · 2023-07-07T20:03:03Z

@hardik-k-shah @krishna-ggk @parasjain1 Any final thoughts?

DarshitChanpura · 2023-07-10T12:43:11Z

Merging now. Any comments can be addressed in follow-ups.

opensearch-trigger-bot · 2023-07-10T12:44:03Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2765-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 8d636c4ea366fca88f28c3226d3997f91992f55e
# Push it to GitHub
git push --set-upstream origin backport/backport-2765-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2765-to-2.x.

…same node (opensearch-project#2765) Signed-off-by: Darshit Chanpura <[email protected]> Signed-off-by: Craig Perkins <[email protected]> Co-authored-by: Craig Perkins <[email protected]> (cherry picked from commit 8d636c4)

…same node (#2765) (#2973) Signed-off-by: Darshit Chanpura <[email protected]> Signed-off-by: Craig Perkins <[email protected]> Co-authored-by: Craig Perkins <[email protected]> (cherry picked from commit 8d636c4)

… is for same node (opensearch-project#2765) (opensearch-project#2973)" This reverts commit ddbe517.

…same node (opensearch-project#2765) Signed-off-by: Darshit Chanpura <[email protected]> Signed-off-by: Craig Perkins <[email protected]> Co-authored-by: Craig Perkins <[email protected]> (cherry picked from commit 8d636c4)

… is for same node (opensearch-project#2765) (opensearch-project#2973)" This reverts commit ddbe517.

… is for same node (opensearch-project#2765) (opensearch-project#2973)" This reverts commit ddbe517. Signed-off-by: Darshit Chanpura <[email protected]>

… is for same node (opensearch-project#2765)" This reverts commit 8d636c4.

DarshitChanpura requested review from cliu123, cwperks, davidlago, peternied, RyanL1997, stephen-crawford and reta as code owners May 11, 2023 16:20

DarshitChanpura marked this pull request as draft May 11, 2023 16:20

DarshitChanpura commented May 11, 2023

View reviewed changes

cwperks reviewed May 11, 2023

View reviewed changes

DarshitChanpura mentioned this pull request May 11, 2023

[Performance Bug] Repeated deserialization of headers for a direct channel request #2724

Closed

krishna-ggk reviewed May 12, 2023

View reviewed changes

DarshitChanpura force-pushed the perf-bug-2724 branch from 3d72b11 to d1198d9 Compare May 12, 2023 15:17

hardik-k-shah reviewed May 12, 2023

View reviewed changes

krishna-ggk reviewed May 15, 2023

View reviewed changes

parasjain1 reviewed May 15, 2023

View reviewed changes

DarshitChanpura marked this pull request as ready for review May 17, 2023 17:02

DarshitChanpura changed the title ~~Adds a check to skip serialization-deserialization if user request is direct~~ Adds a check to skip serialization-deserialization if request is for same node May 17, 2023

krishna-ggk reviewed May 18, 2023

View reviewed changes

RyanL1997 reviewed May 18, 2023

View reviewed changes

src/main/java/org/opensearch/security/transport/SecurityInterceptor.java Outdated Show resolved Hide resolved

DarshitChanpura requested a review from nibix May 18, 2023 16:46

DarshitChanpura force-pushed the perf-bug-2724 branch from a9e31cf to de26a79 Compare May 24, 2023 13:36

cwperks and others added 3 commits July 5, 2023 13:25

Remove add tc header

35de56d

Signed-off-by: Craig Perkins <[email protected]>

Handle skipSecurityInDualMode

219b0b3

Signed-off-by: Craig Perkins <[email protected]>

Merge pull request #6 from cwperks/perf-bug-2724-craig

691019b

Update logic in receiver to look for either transient headers or non-transient headers

DarshitChanpura requested review from peternied, krishna-ggk, stephen-crawford, cwperks and RyanL1997 July 5, 2023 17:58

cwperks reviewed Jul 5, 2023

View reviewed changes

stephen-crawford reviewed Jul 7, 2023

View reviewed changes

stephen-crawford approved these changes Jul 7, 2023

View reviewed changes

RyanL1997 approved these changes Jul 7, 2023

View reviewed changes

DarshitChanpura merged commit 8d636c4 into opensearch-project:main Jul 10, 2023

DarshitChanpura mentioned this pull request Jul 10, 2023

[Backport 2.x] Adds a check to skip serialization-deserialization if request is for same node (#2765) #2973

Merged

2 tasks

cwperks added a commit to cwperks/security that referenced this pull request Jul 21, 2023

Revert "Adds a check to skip serialization-deserialization if request…

360396e

… is for same node (opensearch-project#2765) (opensearch-project#2973)" This reverts commit ddbe517.

cwperks mentioned this pull request Jul 21, 2023

[BUG] [CI] Investigate Flaky test failure for windows CI tasks #3021

Closed

DarshitChanpura added a commit to DarshitChanpura/security that referenced this pull request Jul 24, 2023

Revert "Adds a check to skip serialization-deserialization if request…

8950d21

… is for same node (opensearch-project#2765) (opensearch-project#2973)" This reverts commit ddbe517.

DarshitChanpura added a commit to DarshitChanpura/security that referenced this pull request Jul 28, 2023

Revert "Adds a check to skip serialization-deserialization if request…

f6f2444

… is for same node (opensearch-project#2765)" This reverts commit 8d636c4.

DarshitChanpura mentioned this pull request Jul 31, 2023

Prevent flaky behavior when determining if an request will be executed on the current node. #3066

Merged

3 tasks

DarshitChanpura mentioned this pull request Oct 25, 2023

[BUG] Scroll query is broken for multi node clusters for LDAP users #3582

Closed

3 tasks

cwperks mentioned this pull request Nov 1, 2023

Validate requests are not impacted during security cache invalidation #3637

Merged

3 tasks

cwperks mentioned this pull request Aug 23, 2024

Add Nils Bandener (Github: nibix) as a maintainer #4672

Merged

Adds a check to skip serialization-deserialization if request is for same node #2765

Adds a check to skip serialization-deserialization if request is for same node #2765

Conversation

DarshitChanpura commented May 11, 2023

Description

Issues Resolved

Testing

Check List

DarshitChanpura left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarshitChanpura May 11, 2023 • 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

DarshitChanpura May 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nibix May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krishna-ggk May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarshitChanpura commented May 12, 2023

DarshitChanpura commented May 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krishna-ggk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasjain1 left a comment

Choose a reason for hiding this comment

codecov bot commented May 16, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

DarshitChanpura May 18, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarshitChanpura commented May 24, 2023 • edited Loading

nibix commented May 24, 2023

DarshitChanpura commented May 25, 2023

Choose a reason for hiding this comment

DarshitChanpura commented Jul 6, 2023

Choose a reason for hiding this comment

stephen-crawford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarshitChanpura commented Jul 7, 2023 • edited Loading

DarshitChanpura commented Jul 10, 2023

opensearch-trigger-bot bot commented Jul 10, 2023

DarshitChanpura May 11, 2023 •

edited

Loading

DarshitChanpura May 11, 2023 •

edited

Loading

nibix May 12, 2023 •

edited

Loading

krishna-ggk May 12, 2023 •

edited

Loading

codecov bot commented May 16, 2023 •

edited

Loading

DarshitChanpura May 18, 2023 •

edited

Loading

DarshitChanpura commented May 24, 2023 •

edited

Loading

DarshitChanpura commented Jul 7, 2023 •

edited

Loading