From 58a2a313936fc28b5d0a71fd9ccaaa1013694b9a Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Mon, 5 Jun 2023 20:04:14 -0400 Subject: [PATCH 1/7] 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 --- server/src/main/java/org/opensearch/node/Node.java | 9 ++++++++- .../java/org/opensearch/plugins/ClusterPlugin.java | 13 +++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index a25bac60f49b6..21e2668a5214e 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -1342,7 +1342,14 @@ public void onTimeout(TimeValue timeout) { logger.info("started"); - pluginsService.filterPlugins(ClusterPlugin.class).forEach(ClusterPlugin::onNodeStarted); + pluginsService.filterPlugins(ClusterPlugin.class).forEach(plugin -> { + // TODO: analyse whether the localNode info needs to be passed in all plugin calls + if (plugin.getClass().getSimpleName().equals("OpenSearchSecurityPlugin")) { + plugin.onNodeStarted(clusterService.localNode()); + } else { + plugin.onNodeStarted(); + } + }); return this; } diff --git a/server/src/main/java/org/opensearch/plugins/ClusterPlugin.java b/server/src/main/java/org/opensearch/plugins/ClusterPlugin.java index b0ed91dd4c468..c23fe2c27935f 100644 --- a/server/src/main/java/org/opensearch/plugins/ClusterPlugin.java +++ b/server/src/main/java/org/opensearch/plugins/ClusterPlugin.java @@ -32,11 +32,13 @@ package org.opensearch.plugins; +import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.cluster.routing.allocation.ExistingShardsAllocator; import org.opensearch.cluster.routing.allocation.allocator.ShardsAllocator; import org.opensearch.cluster.routing.allocation.decider.AllocationDecider; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; +import org.opensearch.node.Node; import java.util.Collection; import java.util.Collections; @@ -89,4 +91,15 @@ default Map getExistingShardsAllocators() { */ default void onNodeStarted() {} + /** + * Called when node is started. DiscoveryNode argument is passed to allow referring localNode value inside plugin + * NOTE: Currently this function implementation is limited to OpenSearchSecurityPlugin inside {@link Node#start()} + * + * TODO: modify javadoc when the restriction to SecurityPlugin is lifted + * @param localNode Current Node info + */ + default void onNodeStarted(DiscoveryNode localNode) { + onNodeStarted(); + } + } From c6b0a8f63f7c9046470d963bf914b6eb5f63478d Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Mon, 5 Jun 2023 20:12:42 -0400 Subject: [PATCH 2/7] Adds this PR to changelog Signed-off-by: Darshit Chanpura --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 64274e86a4f74..eadc5616be4e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Add ZSTD compression for snapshotting ([#2996](https://github.com/opensearch-project/OpenSearch/pull/2996)) - Change `com.amazonaws.sdk.ec2MetadataServiceEndpointOverride` to `aws.ec2MetadataServiceEndpoint` ([7372](https://github.com/opensearch-project/OpenSearch/pull/7372/)) - Change `com.amazonaws.sdk.stsEndpointOverride` to `aws.stsEndpointOverride` ([7372](https://github.com/opensearch-project/OpenSearch/pull/7372/)) +- Pass localNode info to security plugin on node start ([#7919](https://github.com/opensearch-project/OpenSearch/pull/7919)) ### Deprecated From af12fafca1468e4dbf85c3356ba0b42d8866dbf4 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Tue, 6 Jun 2023 11:48:51 -0400 Subject: [PATCH 3/7] Makes onNodeStarted call plugin agnostic Signed-off-by: Darshit Chanpura --- CHANGELOG.md | 2 +- server/src/main/java/org/opensearch/node/Node.java | 9 +-------- .../main/java/org/opensearch/plugins/ClusterPlugin.java | 5 +---- 3 files changed, 3 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eadc5616be4e2..a17a81a342b3e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,7 +57,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Add ZSTD compression for snapshotting ([#2996](https://github.com/opensearch-project/OpenSearch/pull/2996)) - Change `com.amazonaws.sdk.ec2MetadataServiceEndpointOverride` to `aws.ec2MetadataServiceEndpoint` ([7372](https://github.com/opensearch-project/OpenSearch/pull/7372/)) - Change `com.amazonaws.sdk.stsEndpointOverride` to `aws.stsEndpointOverride` ([7372](https://github.com/opensearch-project/OpenSearch/pull/7372/)) -- Pass localNode info to security plugin on node start ([#7919](https://github.com/opensearch-project/OpenSearch/pull/7919)) +- Pass localNode info to all plugins on node start ([#7919](https://github.com/opensearch-project/OpenSearch/pull/7919)) ### Deprecated diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index 21e2668a5214e..83c64f6309173 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -1342,14 +1342,7 @@ public void onTimeout(TimeValue timeout) { logger.info("started"); - pluginsService.filterPlugins(ClusterPlugin.class).forEach(plugin -> { - // TODO: analyse whether the localNode info needs to be passed in all plugin calls - if (plugin.getClass().getSimpleName().equals("OpenSearchSecurityPlugin")) { - plugin.onNodeStarted(clusterService.localNode()); - } else { - plugin.onNodeStarted(); - } - }); + pluginsService.filterPlugins(ClusterPlugin.class).forEach(plugin -> plugin.onNodeStarted(clusterService.localNode())); return this; } diff --git a/server/src/main/java/org/opensearch/plugins/ClusterPlugin.java b/server/src/main/java/org/opensearch/plugins/ClusterPlugin.java index c23fe2c27935f..71039aed25ae9 100644 --- a/server/src/main/java/org/opensearch/plugins/ClusterPlugin.java +++ b/server/src/main/java/org/opensearch/plugins/ClusterPlugin.java @@ -38,7 +38,6 @@ import org.opensearch.cluster.routing.allocation.decider.AllocationDecider; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.Settings; -import org.opensearch.node.Node; import java.util.Collection; import java.util.Collections; @@ -93,10 +92,8 @@ default void onNodeStarted() {} /** * Called when node is started. DiscoveryNode argument is passed to allow referring localNode value inside plugin - * NOTE: Currently this function implementation is limited to OpenSearchSecurityPlugin inside {@link Node#start()} * - * TODO: modify javadoc when the restriction to SecurityPlugin is lifted - * @param localNode Current Node info + * @param localNode local Node info */ default void onNodeStarted(DiscoveryNode localNode) { onNodeStarted(); From 171797c4295107fa2ac4a6177c9d4e99a995aba6 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Tue, 6 Jun 2023 16:38:06 -0400 Subject: [PATCH 4/7] Marking onNodeStarted() as deprecated Signed-off-by: Darshit Chanpura --- server/src/main/java/org/opensearch/plugins/ClusterPlugin.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/main/java/org/opensearch/plugins/ClusterPlugin.java b/server/src/main/java/org/opensearch/plugins/ClusterPlugin.java index 71039aed25ae9..5c570c80970fd 100644 --- a/server/src/main/java/org/opensearch/plugins/ClusterPlugin.java +++ b/server/src/main/java/org/opensearch/plugins/ClusterPlugin.java @@ -88,6 +88,7 @@ default Map getExistingShardsAllocators() { /** * Called when the node is started */ + @Deprecated default void onNodeStarted() {} /** From 3d921d3c1e22d78321678981fbbad576f59bebcb Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Wed, 14 Jun 2023 10:10:13 -0400 Subject: [PATCH 5/7] Adds a test to verify the behaviour of new signature of onNodeStarted Signed-off-by: Darshit Chanpura --- .../plugins/ClusterPluginTests.java | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 server/src/test/java/org/opensearch/plugins/ClusterPluginTests.java diff --git a/server/src/test/java/org/opensearch/plugins/ClusterPluginTests.java b/server/src/test/java/org/opensearch/plugins/ClusterPluginTests.java new file mode 100644 index 0000000000000..044af56e2618c --- /dev/null +++ b/server/src/test/java/org/opensearch/plugins/ClusterPluginTests.java @@ -0,0 +1,46 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.plugins; + +import org.opensearch.cluster.node.DiscoveryNode; +import org.opensearch.test.OpenSearchSingleNodeTestCase; + +import java.util.Collection; + +public class ClusterPluginTests extends OpenSearchSingleNodeTestCase { + + @Override + protected Collection> getPlugins() { + return pluginList(DummyClusterPlugin.class); + } + + public void testOnNodeStarted_shouldContainLocalNodeInfo() { + + DiscoveryNode localNode = DummyClusterPlugin.getLocalNode(); + + assertTrue(localNode != null); + } + +} + +final class DummyClusterPlugin extends Plugin implements ClusterPlugin { + + private static volatile DiscoveryNode localNode; + + public DummyClusterPlugin() {} + + @Override + public void onNodeStarted(DiscoveryNode localNode) { + DummyClusterPlugin.localNode = localNode; + } + + public static DiscoveryNode getLocalNode() { + return localNode; + } +} From fefdf000abaccc9e42ebe8fcfebd9dcb8c9f6cc8 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Wed, 14 Jun 2023 10:31:58 -0400 Subject: [PATCH 6/7] Updates test to also check for nodeId Signed-off-by: Darshit Chanpura --- .../test/java/org/opensearch/plugins/ClusterPluginTests.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/server/src/test/java/org/opensearch/plugins/ClusterPluginTests.java b/server/src/test/java/org/opensearch/plugins/ClusterPluginTests.java index 044af56e2618c..c38c6d04fdb49 100644 --- a/server/src/test/java/org/opensearch/plugins/ClusterPluginTests.java +++ b/server/src/test/java/org/opensearch/plugins/ClusterPluginTests.java @@ -25,6 +25,8 @@ public void testOnNodeStarted_shouldContainLocalNodeInfo() { DiscoveryNode localNode = DummyClusterPlugin.getLocalNode(); assertTrue(localNode != null); + // TODO Figure out if there is a way to check ephemeralId + assertTrue(localNode.getId().equals(node().getNodeEnvironment().nodeId())); } } From 6e157eaad200eb94c407aa346d288687da4c005d Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Wed, 14 Jun 2023 13:48:00 -0400 Subject: [PATCH 7/7] 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 --- .../org/opensearch/plugins/ClusterPlugin.java | 2 ++ .../plugins/ClusterPluginTests.java | 32 ++++++++++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/plugins/ClusterPlugin.java b/server/src/main/java/org/opensearch/plugins/ClusterPlugin.java index 5c570c80970fd..c2e147b86d17f 100644 --- a/server/src/main/java/org/opensearch/plugins/ClusterPlugin.java +++ b/server/src/main/java/org/opensearch/plugins/ClusterPlugin.java @@ -87,6 +87,8 @@ default Map getExistingShardsAllocators() { /** * Called when the node is started + * + * DEPRECATED: Use {@link #onNodeStarted(DiscoveryNode)} for newer implementations. */ @Deprecated default void onNodeStarted() {} diff --git a/server/src/test/java/org/opensearch/plugins/ClusterPluginTests.java b/server/src/test/java/org/opensearch/plugins/ClusterPluginTests.java index c38c6d04fdb49..c90d4e2789e82 100644 --- a/server/src/test/java/org/opensearch/plugins/ClusterPluginTests.java +++ b/server/src/test/java/org/opensearch/plugins/ClusterPluginTests.java @@ -13,11 +13,15 @@ import java.util.Collection; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + public class ClusterPluginTests extends OpenSearchSingleNodeTestCase { @Override protected Collection> getPlugins() { - return pluginList(DummyClusterPlugin.class); + return pluginList(DummyClusterPlugin.class, DummyClusterPlugin2.class); } public void testOnNodeStarted_shouldContainLocalNodeInfo() { @@ -29,6 +33,15 @@ public void testOnNodeStarted_shouldContainLocalNodeInfo() { assertTrue(localNode.getId().equals(node().getNodeEnvironment().nodeId())); } + public void testOnNodeStarted_shouldCallDeprecatedMethod() { + DummyClusterPlugin2 dummyClusterPlugin2 = mock(DummyClusterPlugin2.class); + dummyClusterPlugin2.onNodeStarted(); + verify(dummyClusterPlugin2, times(1)).onNodeStarted(); + + DiscoveryNode localNode = DummyClusterPlugin2.getLocalNode(); + assertTrue(localNode != null); + } + } final class DummyClusterPlugin extends Plugin implements ClusterPlugin { @@ -46,3 +59,20 @@ public static DiscoveryNode getLocalNode() { return localNode; } } + +class DummyClusterPlugin2 extends Plugin implements ClusterPlugin { + + private static volatile DiscoveryNode localNode; + + public DummyClusterPlugin2() {} + + @Override + public void onNodeStarted() { + localNode = mock(DiscoveryNode.class); + } + + public static DiscoveryNode getLocalNode() { + return localNode; + } + +}