[Enhancement] Pass localNode info to all plugins on node start #7919

DarshitChanpura · 2023-06-06T00:05:29Z

Updates Node start to now pass localNode info so that it is available directly to a plugin instead of hack-y implementation

This deficit was encountered while trying to solve opensearch-project/security#2724 which required localNode information in order to check whether the intercepted requests were for/from localNode.
The approaches tried:

Modifying GuiceHolder to holder localNode value fetched from TransportService. <- unreliable.
Using ClusterState's getLocalNode() <- fails with AssertionError stating that the calling class is not an instance of ClusterStateObserver.
Using ClusterService's localNode() method in OpenSearchSecurityPlugin class's onNodeStarted() implementation <- doesn't contain any value.

And so with recommendation from @reta , implemented this approach.

Related Issues

opensearch-project/security#2724

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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.

… directly to a plugin instead of hacky implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

github-actions · 2023-06-06T00:39:44Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchTaskCancellationWithHighCpu

URL: https://build.ci.opensearch.org/job/gradle-check/16873/
CommitID: 58a2a31
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

codecov · 2023-06-06T00:43:03Z

Codecov Report

Merging #7919 (d2a6db3) into main (5dce570) will decrease coverage by 0.15%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #7919      +/-   ##
============================================
- Coverage     70.93%   70.78%   -0.15%     
+ Complexity    56633    56559      -74     
============================================
  Files          4719     4719              
  Lines        267555   267557       +2     
  Branches      39206    39206              
============================================
- Hits         189778   189393     -385     
- Misses        61721    62140     +419     
+ Partials      16056    16024      -32

Impacted Files	Coverage Δ
server/src/main/java/org/opensearch/node/Node.java	`85.85% <100.00%> (ø)`
...ain/java/org/opensearch/plugins/ClusterPlugin.java	`83.33% <100.00%> (+33.33%)`	⬆️

... and 480 files with indirect coverage changes

github-actions · 2023-06-06T00:54:04Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchTaskCancellationWithHighCpu

URL: https://build.ci.opensearch.org/job/gradle-check/16874/
CommitID: c6b0a8f
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

server/src/main/java/org/opensearch/node/Node.java

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

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

github-actions · 2023-06-14T14:39:22Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/17559/
CommitID: fefdf00
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?

DarshitChanpura · 2023-06-14T15:45:25Z

CI failed with Execution failed for task ':test:fixtures:krb5kdc-fixture:composeBuild'.. It seems unrelated to the changes in this PR.

server/src/test/java/org/opensearch/plugins/ClusterPluginTests.java

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

github-actions · 2023-06-14T16:20:31Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchShardTaskCancellationWithHighCpu
      1 org.opensearch.index.ShardIndexingPressureIT.testShardIndexingPressureTrackingDuringBulkWrites
      1 org.opensearch.cluster.allocation.AwarenessAllocationIT.testThreeZoneOneReplicaWithForceZoneValueAndLoadAwareness

URL: https://build.ci.opensearch.org/job/gradle-check/17568/
CommitID: fefdf00
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

…ed and adds a test to check that current plugins are not affect with this implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

github-actions · 2023-06-14T18:22:18Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/17589/
CommitID: 56c5f84

peternied

Thanks for adding these tests, looks good to me

DarshitChanpura · 2023-06-16T16:03:36Z

@dblock Would you please review this again?

github-actions · 2023-06-19T17:26:29Z

Gradle Check (Jenkins) Run Completed with:

RESULT: ❌
URL: https://build.ci.opensearch.org/job/gradle-check/17928/
CommitID: 2bb26c2
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?

DarshitChanpura · 2023-06-19T19:57:16Z

The CI test failures seem flaky: https://build.ci.opensearch.org/job/gradle-check/17928/

github-actions · 2023-06-20T15:52:35Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      3 org.opensearch.remotestore.SegmentReplicationUsingRemoteStoreIT.testDropPrimaryDuringReplication
      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchTaskCancellationWithHighCpu

URL: https://build.ci.opensearch.org/job/gradle-check/18001/
CommitID: d2a6db3
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@dblock

@dblock dismissing your review as tests are added. Feel free to come back to this.

opensearch-trigger-bot · 2023-06-20T21:31:37Z

The backport to 2.x failed:

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

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
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-7919-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 40d758387bd9227314a64a73035260829bfc3c10
# Push it to GitHub
git push --set-upstream origin backport/backport-7919-to-2.x
# Go back to the original working tree
popd
# 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-7919-to-2.x.

reta · 2023-06-20T22:03:04Z

@DarshitChanpura sadly needs manual backport to 2.x

…earch-project#7919) * Updates Node start to now pass localNode info so that it is available directly to a plugin instead of hacky implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds this PR to changelog Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Makes onNodeStarted call plugin agnostic Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Marking onNodeStarted() as deprecated Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds a test to verify the behaviour of new signature of onNodeStarted Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Updates test to also check for nodeId Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds instructions to use the overloaded implementation of onNodeStarted and adds a test to check that current plugins are not affect with this implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com> --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com> Signed-off-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com> (cherry picked from commit 40d7583)

* [Enhancement] Pass localNode info to all plugins on node start (#7919) * Updates Node start to now pass localNode info so that it is available directly to a plugin instead of hacky implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds this PR to changelog Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Makes onNodeStarted call plugin agnostic Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Marking onNodeStarted() as deprecated Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds a test to verify the behaviour of new signature of onNodeStarted Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Updates test to also check for nodeId Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds instructions to use the overloaded implementation of onNodeStarted and adds a test to check that current plugins are not affect with this implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com> --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com> Signed-off-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com> (cherry picked from commit 40d7583) * Adds a CHANGELOG entry Signed-off-by: Darshit Chanpura <dchanp@amazon.com> --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

…earch-project#7919) * Updates Node start to now pass localNode info so that it is available directly to a plugin instead of hacky implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds this PR to changelog Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Makes onNodeStarted call plugin agnostic Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Marking onNodeStarted() as deprecated Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds a test to verify the behaviour of new signature of onNodeStarted Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Updates test to also check for nodeId Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds instructions to use the overloaded implementation of onNodeStarted and adds a test to check that current plugins are not affect with this implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com> --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com> Signed-off-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com>

…search-project#8192) * [Enhancement] Pass localNode info to all plugins on node start (opensearch-project#7919) * Updates Node start to now pass localNode info so that it is available directly to a plugin instead of hacky implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds this PR to changelog Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Makes onNodeStarted call plugin agnostic Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Marking onNodeStarted() as deprecated Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds a test to verify the behaviour of new signature of onNodeStarted Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Updates test to also check for nodeId Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds instructions to use the overloaded implementation of onNodeStarted and adds a test to check that current plugins are not affect with this implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com> --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com> Signed-off-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com> (cherry picked from commit 40d7583) * Adds a CHANGELOG entry Signed-off-by: Darshit Chanpura <dchanp@amazon.com> --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

…earch-project#7919) * Updates Node start to now pass localNode info so that it is available directly to a plugin instead of hacky implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds this PR to changelog Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Makes onNodeStarted call plugin agnostic Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Marking onNodeStarted() as deprecated Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds a test to verify the behaviour of new signature of onNodeStarted Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Updates test to also check for nodeId Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds instructions to use the overloaded implementation of onNodeStarted and adds a test to check that current plugins are not affect with this implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com> --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com> Signed-off-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com> Signed-off-by: Rishab Nahata <rnnahata@amazon.com>

…earch-project#7919) * Updates Node start to now pass localNode info so that it is available directly to a plugin instead of hacky implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds this PR to changelog Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Makes onNodeStarted call plugin agnostic Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Marking onNodeStarted() as deprecated Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds a test to verify the behaviour of new signature of onNodeStarted Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Updates test to also check for nodeId Signed-off-by: Darshit Chanpura <dchanp@amazon.com> * Adds instructions to use the overloaded implementation of onNodeStarted and adds a test to check that current plugins are not affect with this implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com> --------- Signed-off-by: Darshit Chanpura <dchanp@amazon.com> Signed-off-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com> Signed-off-by: Shivansh Arora <hishiv@amazon.com>

Updates Node start to now pass localNode info so that it is available…

58a2a31

… directly to a plugin instead of hacky implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura requested review from reta, anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, gbbafna, setiah, kartg, kotwanikunal, mch2, nknize, owaiskazi19, Rishikesh1159, ryanbogan, saratvemulapalli, shwetathareja, dreamer-89, tlfeng, VachaShah and dbwiddis as code owners June 6, 2023 00:05

Adds this PR to changelog

c6b0a8f

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura mentioned this pull request Jun 6, 2023

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

Merged

3 tasks

reta reviewed Jun 6, 2023

View reviewed changes

server/src/main/java/org/opensearch/node/Node.java Outdated Show resolved Hide resolved

reta reviewed Jun 6, 2023

View reviewed changes

server/src/main/java/org/opensearch/plugins/ClusterPlugin.java Outdated Show resolved Hide resolved

reta reviewed Jun 6, 2023

View reviewed changes

server/src/main/java/org/opensearch/plugins/ClusterPlugin.java Outdated Show resolved Hide resolved

Updates test to also check for nodeId

fefdf00

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

peternied reviewed Jun 14, 2023

View reviewed changes

server/src/test/java/org/opensearch/plugins/ClusterPluginTests.java Show resolved Hide resolved

server/src/main/java/org/opensearch/plugins/ClusterPlugin.java Show resolved Hide resolved

DarshitChanpura added 2 commits June 14, 2023 13:48

Adds instructions to use the overloaded implementation of onNodeStart…

6e157ea

…ed and adds a test to check that current plugins are not affect with this implementation Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Merge remote-tracking branch 'upstream/main' into passing-localNode-info

56c5f84

peternied approved these changes Jun 14, 2023

View reviewed changes

Merge branch 'main' into passing-localNode-info

2bb26c2

cwperks approved these changes Jun 19, 2023

View reviewed changes

Merge branch 'main' into passing-localNode-info

d2a6db3

saratvemulapalli approved these changes Jun 20, 2023

View reviewed changes

saratvemulapalli requested a review from dblock June 20, 2023 17:45

saratvemulapalli merged commit 40d7583 into opensearch-project:main Jun 20, 2023

saratvemulapalli added the backport 2.x Backport to 2.x branch label Jun 20, 2023

DarshitChanpura mentioned this pull request Jun 21, 2023

[Backport 2.x] Pass localNode info to all plugins on node start #8192

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Enhancement] Pass localNode info to all plugins on node start #7919

[Enhancement] Pass localNode info to all plugins on node start #7919

DarshitChanpura commented Jun 6, 2023

github-actions bot commented Jun 6, 2023

codecov bot commented Jun 6, 2023 •

edited

Loading

github-actions bot commented Jun 6, 2023

github-actions bot commented Jun 14, 2023

DarshitChanpura commented Jun 14, 2023

github-actions bot commented Jun 14, 2023

github-actions bot commented Jun 14, 2023

peternied left a comment

DarshitChanpura commented Jun 16, 2023

github-actions bot commented Jun 19, 2023

DarshitChanpura commented Jun 19, 2023

github-actions bot commented Jun 20, 2023

opensearch-trigger-bot bot commented Jun 20, 2023

reta commented Jun 20, 2023

[Enhancement] Pass localNode info to all plugins on node start #7919

[Enhancement] Pass localNode info to all plugins on node start #7919

Conversation

DarshitChanpura commented Jun 6, 2023

Related Issues

Check List

github-actions bot commented Jun 6, 2023

Gradle Check (Jenkins) Run Completed with:

codecov bot commented Jun 6, 2023 • edited Loading

Codecov Report

github-actions bot commented Jun 6, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jun 14, 2023

Gradle Check (Jenkins) Run Completed with:

DarshitChanpura commented Jun 14, 2023

github-actions bot commented Jun 14, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jun 14, 2023

Gradle Check (Jenkins) Run Completed with:

peternied left a comment

Choose a reason for hiding this comment

DarshitChanpura commented Jun 16, 2023

github-actions bot commented Jun 19, 2023

Gradle Check (Jenkins) Run Completed with:

DarshitChanpura commented Jun 19, 2023

github-actions bot commented Jun 20, 2023

Gradle Check (Jenkins) Run Completed with:

opensearch-trigger-bot bot commented Jun 20, 2023

reta commented Jun 20, 2023

codecov bot commented Jun 6, 2023 •

edited

Loading