From f40c25c54f9b21bc5d75e2bdf533e189231db196 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 10 May 2023 22:09:25 -0400 Subject: [PATCH 01/17] WIP on extension settings Signed-off-by: Craig Perkins --- .../identity/shiro/ShiroSubject.java | 5 ++ .../opensearch/common/settings/Setting.java | 5 ++ .../extensions/ExtensionSecuritySettings.java | 57 +++++++++++++++++++ .../extensions/ExtensionsManager.java | 23 +++++++- .../extensions/ExtensionsSettings.java | 15 ++++- .../extensions/NoopExtensionsManager.java | 2 + .../opensearch/identity/IdentityService.java | 8 +++ .../java/org/opensearch/identity/Subject.java | 5 ++ .../opensearch/identity/noop/NoopSubject.java | 5 ++ .../main/java/org/opensearch/node/Node.java | 1 + .../opensearch/plugins/IdentityPlugin.java | 16 +++++- .../extensions/ExtensionsManagerTests.java | 5 ++ 12 files changed, 144 insertions(+), 3 deletions(-) create mode 100644 server/src/main/java/org/opensearch/extensions/ExtensionSecuritySettings.java diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSubject.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSubject.java index 3208d2bb5d8ca..ffe10aea411d6 100644 --- a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSubject.java +++ b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSubject.java @@ -88,4 +88,9 @@ public void authenticate(AuthToken authenticationToken) { .orElseThrow(() -> new UnsupportedAuthenticationToken()); shiroSubject.login(authToken); } + + @Override + public boolean isAuthenticated() { + return shiroSubject.isAuthenticated(); + } } diff --git a/server/src/main/java/org/opensearch/common/settings/Setting.java b/server/src/main/java/org/opensearch/common/settings/Setting.java index a0cdf35ee0ad2..32a6686398a2a 100644 --- a/server/src/main/java/org/opensearch/common/settings/Setting.java +++ b/server/src/main/java/org/opensearch/common/settings/Setting.java @@ -132,6 +132,11 @@ public enum Property { */ Deprecated, + /** + * Extension scope + */ + ExtensionScope, + /** * Node scope */ diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionSecuritySettings.java b/server/src/main/java/org/opensearch/extensions/ExtensionSecuritySettings.java new file mode 100644 index 0000000000000..38d9720418142 --- /dev/null +++ b/server/src/main/java/org/opensearch/extensions/ExtensionSecuritySettings.java @@ -0,0 +1,57 @@ +/* + * 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. + */ + +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +/* + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.extensions; + +import org.opensearch.common.settings.AbstractScopedSettings; +import org.opensearch.common.settings.Setting; +import org.opensearch.common.settings.Setting.Property; +import org.opensearch.common.settings.SettingUpgrader; +import org.opensearch.common.settings.Settings; + +import java.util.Collections; +import java.util.Set; + +/** + * Encapsulates all valid extension level settings. + * + * @opensearch.internal + */ +public final class ExtensionSecuritySettings extends AbstractScopedSettings { + + public ExtensionSecuritySettings(final Set> settingsSet) { + this(settingsSet, Collections.emptySet()); + } + + public ExtensionSecuritySettings(final Set> settingsSet, final Set> settingUpgraders) { + super(Settings.EMPTY, settingsSet, settingUpgraders, Property.ExtensionScope); + } +} diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java index 94ad9ff84cfdb..45cabc6024c52 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java @@ -16,10 +16,12 @@ import java.nio.file.Path; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Set; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.TimeUnit; @@ -38,6 +40,7 @@ import org.opensearch.cluster.ClusterSettingsResponse; import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.settings.Setting; import org.opensearch.core.util.FileSystemUtils; import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.settings.Settings; @@ -57,6 +60,7 @@ import org.opensearch.extensions.rest.RestActionsRequestHandler; import org.opensearch.extensions.settings.CustomSettingsRequestHandler; import org.opensearch.extensions.settings.RegisterCustomSettingsRequest; +import org.opensearch.identity.IdentityService; import org.opensearch.index.IndexModule; import org.opensearch.index.IndexService; import org.opensearch.index.IndicesModuleRequest; @@ -114,6 +118,7 @@ public static enum OpenSearchRequestType { private CustomSettingsRequestHandler customSettingsRequestHandler; private TransportService transportService; private ClusterService clusterService; + private IdentityService identityService; private Settings environmentSettings; private AddSettingsUpdateConsumerRequestHandler addSettingsUpdateConsumerRequestHandler; private NodeClient client; @@ -132,6 +137,7 @@ public ExtensionsManager(Path extensionsPath) throws IOException { // will be initialized in initializeServicesAndRestHandler which is called after the Node is initialized this.transportService = null; this.clusterService = null; + this.identityService = null; this.client = null; this.extensionTransportActionsHandler = null; @@ -158,6 +164,7 @@ public void initializeServicesAndRestHandler( SettingsModule settingsModule, TransportService transportService, ClusterService clusterService, + IdentityService identityService, Settings initialEnvironmentSettings, NodeClient client ) { @@ -165,6 +172,7 @@ public void initializeServicesAndRestHandler( this.customSettingsRequestHandler = new CustomSettingsRequestHandler(settingsModule); this.transportService = transportService; this.clusterService = clusterService; + this.identityService = identityService; this.environmentSettings = initialEnvironmentSettings; this.addSettingsUpdateConsumerRequestHandler = new AddSettingsUpdateConsumerRequestHandler( clusterService, @@ -612,6 +620,14 @@ private ExtensionsSettings readFromExtensionsYml(Path filePath) throws IOExcepti } } + Set securitySettingsKeys = identityService.getExtensionSettings().stream().map(s -> s.getKey()).collect(Collectors.toSet()); + Map securitySettings = extensionMap.entrySet() + .stream() + .filter(kv -> securitySettingsKeys.contains(kv.getKey())) + .collect(Collectors.toMap(map -> map.getKey(), map -> map.getValue())); + Settings.Builder output = Settings.builder(); + output.loadFromMap(securitySettings); + // Create extension read from yml config readExtensions.add( new Extension( @@ -622,7 +638,8 @@ private ExtensionsSettings readFromExtensionsYml(Path filePath) throws IOExcepti extensionMap.get("version").toString(), extensionMap.get("opensearchVersion").toString(), extensionMap.get("minimumCompatibleVersion").toString(), - extensionDependencyList + extensionDependencyList, + output.build() ) ); } catch (IOException e) { @@ -694,6 +711,10 @@ void setClusterService(ClusterService clusterService) { this.clusterService = clusterService; } + void setIdentityService(IdentityService identityService) { + this.identityService = identityService; + } + CustomSettingsRequestHandler getCustomSettingsRequestHandler() { return customSettingsRequestHandler; } diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsSettings.java b/server/src/main/java/org/opensearch/extensions/ExtensionsSettings.java index fd11aec973d42..b5f8a792b6a91 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsSettings.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsSettings.java @@ -8,9 +8,15 @@ package org.opensearch.extensions; +import org.opensearch.common.settings.Setting; +import org.opensearch.common.settings.Settings; +import org.opensearch.identity.IdentityService; + import java.util.ArrayList; import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; /** * List of extension configurations from extension.yml @@ -44,6 +50,7 @@ public static class Extension { private String opensearchVersion; private String minimumCompatibleVersion; private List dependencies = Collections.emptyList(); + private Settings securitySettings = Settings.EMPTY; public Extension( String name, @@ -53,7 +60,8 @@ public Extension( String version, String opensearchVersion, String minimumCompatibleVersion, - List dependencies + List dependencies, + Settings securitySettings ) { this.name = name; this.uniqueId = uniqueId; @@ -63,6 +71,7 @@ public Extension( this.opensearchVersion = opensearchVersion; this.minimumCompatibleVersion = minimumCompatibleVersion; this.dependencies = dependencies; + this.securitySettings = securitySettings; } public Extension() { @@ -127,6 +136,10 @@ public List getDependencies() { return dependencies; } + public Settings getSecuritySettings() { + return securitySettings; + } + public String getMinimumCompatibleVersion() { return minimumCompatibleVersion; } diff --git a/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java index 6165423b767ce..273c5d5c2d9c3 100644 --- a/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java @@ -22,6 +22,7 @@ import org.opensearch.extensions.action.ExtensionActionRequest; import org.opensearch.extensions.action.ExtensionActionResponse; import org.opensearch.extensions.action.RemoteExtensionActionResponse; +import org.opensearch.identity.IdentityService; import org.opensearch.index.IndexModule; import org.opensearch.transport.TransportService; @@ -42,6 +43,7 @@ public void initializeServicesAndRestHandler( SettingsModule settingsModule, TransportService transportService, ClusterService clusterService, + IdentityService identityService, Settings initialEnvironmentSettings, NodeClient client ) { diff --git a/server/src/main/java/org/opensearch/identity/IdentityService.java b/server/src/main/java/org/opensearch/identity/IdentityService.java index e7e785ba75231..4a2009d6235a4 100644 --- a/server/src/main/java/org/opensearch/identity/IdentityService.java +++ b/server/src/main/java/org/opensearch/identity/IdentityService.java @@ -8,6 +8,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.opensearch.OpenSearchException; +import org.opensearch.common.settings.Setting; import org.opensearch.identity.noop.NoopIdentityPlugin; import java.util.List; import org.opensearch.common.settings.Settings; @@ -48,4 +49,11 @@ public IdentityService(final Settings settings, final List ident public Subject getSubject() { return identityPlugin.getSubject(); } + + /** + * Gets a list of settings to register for extensions + */ + public List> getExtensionSettings() { + return identityPlugin.getExtensionSettings(); + } } diff --git a/server/src/main/java/org/opensearch/identity/Subject.java b/server/src/main/java/org/opensearch/identity/Subject.java index cbfdadb5cf6a7..5518c58d70e4f 100644 --- a/server/src/main/java/org/opensearch/identity/Subject.java +++ b/server/src/main/java/org/opensearch/identity/Subject.java @@ -29,4 +29,9 @@ public interface Subject { * throws SubjectDisabled */ void authenticate(final AuthToken token); + + /** + * Method that returns whether the current subject of the running thread is authenticated + */ + boolean isAuthenticated(); } diff --git a/server/src/main/java/org/opensearch/identity/noop/NoopSubject.java b/server/src/main/java/org/opensearch/identity/noop/NoopSubject.java index 801225fb16ad3..bfced9a0e2895 100644 --- a/server/src/main/java/org/opensearch/identity/noop/NoopSubject.java +++ b/server/src/main/java/org/opensearch/identity/noop/NoopSubject.java @@ -54,4 +54,9 @@ public String toString() { public void authenticate(AuthToken AuthToken) { // Do nothing as noop subject is always logged in } + + @Override + public boolean isAuthenticated() { + return true; + } } diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index 3827041a60aa3..e350aa8b0f477 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -851,6 +851,7 @@ protected Node( settingsModule, transportService, clusterService, + identityService, environment.settings(), client ); diff --git a/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java b/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java index 4cb15f4ab3cbe..7ac714359b540 100644 --- a/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java +++ b/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java @@ -8,7 +8,14 @@ package org.opensearch.plugins; +import org.opensearch.common.settings.Setting; +import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.identity.Subject; +import org.opensearch.rest.RestHandler; + +import java.util.Collections; +import java.util.List; +import java.util.function.UnaryOperator; /** * Plugin that provides identity and access control for OpenSearch @@ -22,5 +29,12 @@ public interface IdentityPlugin { * * Should never return null * */ - public Subject getSubject(); + Subject getSubject(); + + /** + * Returns a list of additional {@link Setting} definitions for that this plugin adds for extensions + */ + default List> getExtensionSettings() { + return Collections.emptyList(); + } } diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index 0a9fd2e6b94fe..9988d6249d13f 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -71,6 +71,7 @@ import org.opensearch.extensions.rest.RegisterRestActionsRequest; import org.opensearch.extensions.settings.RegisterCustomSettingsRequest; import org.opensearch.identity.IdentityService; +import org.opensearch.identity.noop.NoopIdentityPlugin; import org.opensearch.index.IndexModule; import org.opensearch.index.IndexSettings; import org.opensearch.index.analysis.AnalysisRegistry; @@ -101,6 +102,7 @@ public class ExtensionsManagerTests extends OpenSearchTestCase { private RestController restController; private SettingsModule settingsModule; private ClusterService clusterService; + private IdentityService identityService; private NodeClient client; private MockNioTransport transport; private Path extensionDir; @@ -173,6 +175,7 @@ public void setup() throws Exception { when(actionModule.getRestController()).thenReturn(restController); settingsModule = new SettingsModule(Settings.EMPTY, emptyList(), emptyList(), emptySet()); clusterService = createClusterService(threadPool); + identityService = new IdentityService(settings, List.of()); // NoopIdentityPlugin extensionDir = createTempDir(); @@ -825,6 +828,7 @@ public void testRegisterHandler() throws Exception { settingsModule, mockTransportService, clusterService, + identityService, settings, client ); @@ -905,6 +909,7 @@ private void initialize(ExtensionsManager extensionsManager) { settingsModule, transportService, clusterService, + identityService, settings, client ); From 0acc4505a4d3d4f065593ae355cc6e0f087842af Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 11 May 2023 08:40:17 -0400 Subject: [PATCH 02/17] Use getExtensionSettings from the identity service Signed-off-by: Craig Perkins --- .../extensions/ExtensionsManager.java | 29 +++++++++++++++++-- .../extensions/NoopExtensionsManager.java | 1 - .../main/java/org/opensearch/node/Node.java | 3 +- .../extensions/ExtensionsManagerTests.java | 5 ---- 4 files changed, 28 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java index bc71e92f192c2..d549807741de8 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java @@ -151,6 +151,33 @@ public ExtensionsManager(Path extensionsPath) throws IOException { } + /** + * Instantiate a new ExtensionsManager object to handle requests and responses from extensions. This is called during Node bootstrap. + * + * @param extensionsPath Path to a directory containing extensions. + * @param identityService Identity Service + * @throws IOException If the extensions discovery file is not properly retrieved. + */ + public ExtensionsManager(Path extensionsPath, IdentityService identityService) throws IOException { + logger.info("ExtensionsManager initialized"); + this.extensionsPath = extensionsPath; + this.initializedExtensions = new HashMap(); + this.extensionIdMap = new HashMap(); + this.extensionSettingsMap = new HashMap(); + // will be initialized in initializeServicesAndRestHandler which is called after the Node is initialized + this.transportService = null; + this.clusterService = null; + this.identityService = identityService; + this.client = null; + this.extensionTransportActionsHandler = null; + + /* + * Now Discover extensions + */ + discover(); + + } + /** * Initializes the {@link RestActionsRequestHandler}, {@link TransportService}, {@link ClusterService} and environment settings. This is called during Node bootstrap. * Lists/maps of extensions have already been initialized but not yet populated. @@ -167,7 +194,6 @@ public void initializeServicesAndRestHandler( SettingsModule settingsModule, TransportService transportService, ClusterService clusterService, - IdentityService identityService, Settings initialEnvironmentSettings, NodeClient client ) { @@ -175,7 +201,6 @@ public void initializeServicesAndRestHandler( this.customSettingsRequestHandler = new CustomSettingsRequestHandler(settingsModule); this.transportService = transportService; this.clusterService = clusterService; - this.identityService = identityService; this.environmentSettings = initialEnvironmentSettings; this.addSettingsUpdateConsumerRequestHandler = new AddSettingsUpdateConsumerRequestHandler( clusterService, diff --git a/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java index 273c5d5c2d9c3..7f64d2f0ce503 100644 --- a/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java @@ -43,7 +43,6 @@ public void initializeServicesAndRestHandler( SettingsModule settingsModule, TransportService transportService, ClusterService clusterService, - IdentityService identityService, Settings initialEnvironmentSettings, NodeClient client ) { diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index e350aa8b0f477..1dd1ee92988a3 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -466,7 +466,7 @@ protected Node( final IdentityService identityService = new IdentityService(settings, identityPlugins); if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS)) { - this.extensionsManager = new ExtensionsManager(initialEnvironment.extensionDir()); + this.extensionsManager = new ExtensionsManager(initialEnvironment.extensionDir(), identityService); } else { this.extensionsManager = new NoopExtensionsManager(); } @@ -851,7 +851,6 @@ protected Node( settingsModule, transportService, clusterService, - identityService, environment.settings(), client ); diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index 4f0e874e8870d..65fb0322ffa2f 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -71,7 +71,6 @@ import org.opensearch.extensions.rest.RegisterRestActionsRequest; import org.opensearch.extensions.settings.RegisterCustomSettingsRequest; import org.opensearch.identity.IdentityService; -import org.opensearch.identity.noop.NoopIdentityPlugin; import org.opensearch.index.IndexModule; import org.opensearch.index.IndexSettings; import org.opensearch.index.analysis.AnalysisRegistry; @@ -102,7 +101,6 @@ public class ExtensionsManagerTests extends OpenSearchTestCase { private RestController restController; private SettingsModule settingsModule; private ClusterService clusterService; - private IdentityService identityService; private NodeClient client; private MockNioTransport transport; private Path extensionDir; @@ -175,7 +173,6 @@ public void setup() throws Exception { when(actionModule.getRestController()).thenReturn(restController); settingsModule = new SettingsModule(Settings.EMPTY, emptyList(), emptyList(), emptySet()); clusterService = createClusterService(threadPool); - identityService = new IdentityService(settings, List.of()); // NoopIdentityPlugin extensionDir = createTempDir(); @@ -829,7 +826,6 @@ public void testRegisterHandler() throws Exception { settingsModule, mockTransportService, clusterService, - identityService, settings, client ); @@ -910,7 +906,6 @@ private void initialize(ExtensionsManager extensionsManager) { settingsModule, transportService, clusterService, - identityService, settings, client ); From 9717fd647ac86c05a98068eec64ffd9f17032532 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 11 May 2023 12:24:00 -0400 Subject: [PATCH 03/17] Add extension scoped settings and add area for additional settings Signed-off-by: Craig Perkins --- ....java => ExtensionAdditionalSettings.java} | 6 +- .../extensions/ExtensionsManager.java | 22 +- .../extensions/ExtensionsSettings.java | 14 +- .../ExtensionsManagerWithIdentityTests.java | 236 ++++++++++++++++++ 4 files changed, 257 insertions(+), 21 deletions(-) rename server/src/main/java/org/opensearch/extensions/{ExtensionSecuritySettings.java => ExtensionAdditionalSettings.java} (85%) create mode 100644 server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionSecuritySettings.java b/server/src/main/java/org/opensearch/extensions/ExtensionAdditionalSettings.java similarity index 85% rename from server/src/main/java/org/opensearch/extensions/ExtensionSecuritySettings.java rename to server/src/main/java/org/opensearch/extensions/ExtensionAdditionalSettings.java index 38d9720418142..01d0e976594ab 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionSecuritySettings.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionAdditionalSettings.java @@ -45,13 +45,13 @@ * * @opensearch.internal */ -public final class ExtensionSecuritySettings extends AbstractScopedSettings { +public final class ExtensionAdditionalSettings extends AbstractScopedSettings { - public ExtensionSecuritySettings(final Set> settingsSet) { + public ExtensionAdditionalSettings(final Set> settingsSet) { this(settingsSet, Collections.emptySet()); } - public ExtensionSecuritySettings(final Set> settingsSet, final Set> settingUpgraders) { + public ExtensionAdditionalSettings(final Set> settingsSet, final Set> settingUpgraders) { super(Settings.EMPTY, settingsSet, settingUpgraders, Property.ExtensionScope); } } diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java index d549807741de8..9b8c8029a205b 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java @@ -16,7 +16,6 @@ import java.nio.file.Path; import java.util.ArrayList; import java.util.Collection; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -40,7 +39,6 @@ import org.opensearch.cluster.ClusterSettingsResponse; import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.cluster.service.ClusterService; -import org.opensearch.common.settings.Setting; import org.opensearch.core.util.FileSystemUtils; import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.settings.Settings; @@ -660,13 +658,19 @@ private ExtensionsSettings readFromExtensionsYml(Path filePath) throws IOExcepti } } - Set securitySettingsKeys = identityService.getExtensionSettings().stream().map(s -> s.getKey()).collect(Collectors.toSet()); - Map securitySettings = extensionMap.entrySet() - .stream() - .filter(kv -> securitySettingsKeys.contains(kv.getKey())) - .collect(Collectors.toMap(map -> map.getKey(), map -> map.getValue())); Settings.Builder output = Settings.builder(); - output.loadFromMap(securitySettings); + ExtensionAdditionalSettings additionalSettings = new ExtensionAdditionalSettings(Set.of()); + + if (identityService != null) { + additionalSettings = new ExtensionAdditionalSettings(identityService.getExtensionSettings().stream().collect(Collectors.toSet())); + Set securitySettingsKeys = identityService.getExtensionSettings().stream().map(s -> s.getKey()).collect(Collectors.toSet()); + Map securitySettings = extensionMap.entrySet() + .stream() + .filter(kv -> securitySettingsKeys.contains(kv.getKey())) + .collect(Collectors.toMap(map -> map.getKey(), map -> map.getValue())); + output.loadFromMap(securitySettings); + additionalSettings.applySettings(output.build()); + } // Create extension read from yml config readExtensions.add( @@ -679,7 +683,7 @@ private ExtensionsSettings readFromExtensionsYml(Path filePath) throws IOExcepti extensionMap.get("opensearchVersion").toString(), extensionMap.get("minimumCompatibleVersion").toString(), extensionDependencyList, - output.build() + additionalSettings ) ); } catch (IOException e) { diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsSettings.java b/server/src/main/java/org/opensearch/extensions/ExtensionsSettings.java index b5f8a792b6a91..2c59031aa27b9 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsSettings.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsSettings.java @@ -8,15 +8,11 @@ package org.opensearch.extensions; -import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Settings; -import org.opensearch.identity.IdentityService; import java.util.ArrayList; import java.util.Collections; -import java.util.HashMap; import java.util.List; -import java.util.Map; /** * List of extension configurations from extension.yml @@ -50,7 +46,7 @@ public static class Extension { private String opensearchVersion; private String minimumCompatibleVersion; private List dependencies = Collections.emptyList(); - private Settings securitySettings = Settings.EMPTY; + private ExtensionAdditionalSettings additionalSettings; public Extension( String name, @@ -61,7 +57,7 @@ public Extension( String opensearchVersion, String minimumCompatibleVersion, List dependencies, - Settings securitySettings + ExtensionAdditionalSettings additionalSettings ) { this.name = name; this.uniqueId = uniqueId; @@ -71,7 +67,7 @@ public Extension( this.opensearchVersion = opensearchVersion; this.minimumCompatibleVersion = minimumCompatibleVersion; this.dependencies = dependencies; - this.securitySettings = securitySettings; + this.additionalSettings = additionalSettings; } public Extension() { @@ -136,8 +132,8 @@ public List getDependencies() { return dependencies; } - public Settings getSecuritySettings() { - return securitySettings; + public ExtensionAdditionalSettings getAdditionalSettings() { + return additionalSettings; } public String getMinimumCompatibleVersion() { diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java new file mode 100644 index 0000000000000..61386df1a3832 --- /dev/null +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java @@ -0,0 +1,236 @@ +/* + * 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.extensions; + +import org.junit.After; +import org.junit.Before; +import org.opensearch.Version; +import org.opensearch.action.ActionModule; +import org.opensearch.client.node.NodeClient; +import org.opensearch.cluster.metadata.IndexMetadata; +import org.opensearch.cluster.node.DiscoveryNode; +import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.io.stream.NamedWriteableRegistry; +import org.opensearch.common.network.NetworkService; +import org.opensearch.common.settings.Setting; +import org.opensearch.common.settings.Setting.Property; +import org.opensearch.common.settings.Settings; +import org.opensearch.common.settings.SettingsModule; +import org.opensearch.common.transport.TransportAddress; +import org.opensearch.common.util.FeatureFlags; +import org.opensearch.common.util.PageCacheRecycler; +import org.opensearch.env.Environment; +import org.opensearch.identity.IdentityService; +import org.opensearch.identity.Subject; +import org.opensearch.identity.noop.NoopSubject; +import org.opensearch.indices.breaker.NoneCircuitBreakerService; +import org.opensearch.plugins.IdentityPlugin; +import org.opensearch.rest.RestController; +import org.opensearch.test.FeatureFlagSetter; +import org.opensearch.test.OpenSearchTestCase; +import org.opensearch.test.client.NoOpNodeClient; +import org.opensearch.test.transport.MockTransportService; +import org.opensearch.threadpool.TestThreadPool; +import org.opensearch.threadpool.ThreadPool; +import org.opensearch.transport.TransportService; +import org.opensearch.transport.nio.MockNioTransport; +import org.opensearch.usage.UsageService; + +import java.net.InetAddress; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.concurrent.TimeUnit; + +import static java.util.Collections.emptyList; +import static java.util.Collections.emptyMap; +import static java.util.Collections.emptySet; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import static org.opensearch.test.ClusterServiceUtils.createClusterService; + +public class ExtensionsManagerWithIdentityTests extends OpenSearchTestCase { + + private FeatureFlagSetter featureFlagSetter; + private TransportService transportService; + private ActionModule actionModule; + private RestController restController; + private SettingsModule settingsModule; + private ClusterService clusterService; + private IdentityService identityService; + + private Setting customSetting = Setting.simpleString("custom_extension_setting", "none", Property.ExtensionScope); + private NodeClient client; + private MockNioTransport transport; + private Path extensionDir; + private final ThreadPool threadPool = new TestThreadPool(ExtensionsManagerWithIdentityTests.class.getSimpleName()); + private final Settings settings = Settings.builder() + .put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT) + .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toString()) + .build(); + private final List extensionsYmlLines = Arrays.asList( + "extensions:", + " - name: firstExtension", + " uniqueId: uniqueid1", + " hostAddress: '127.0.0.0'", + " port: '9300'", + " version: '0.0.7'", + " opensearchVersion: '3.0.0'", + " minimumCompatibleVersion: '3.0.0'", + " custom_extension_setting: 'custom_setting'", + " - name: secondExtension", + " uniqueId: 'uniqueid2'", + " hostAddress: '127.0.0.1'", + " port: '9301'", + " version: '3.14.16'", + " opensearchVersion: '2.0.0'", + " minimumCompatibleVersion: '2.0.0'" + ); + + private DiscoveryExtensionNode extensionNode; + + @Before + public void setup() throws Exception { + featureFlagSetter = FeatureFlagSetter.set(FeatureFlags.EXTENSIONS); + Settings settings = Settings.builder().put("cluster.name", "test").build(); + transport = new MockNioTransport( + settings, + Version.CURRENT, + threadPool, + new NetworkService(Collections.emptyList()), + PageCacheRecycler.NON_RECYCLING_INSTANCE, + new NamedWriteableRegistry(Collections.emptyList()), + new NoneCircuitBreakerService() + ); + transportService = new MockTransportService( + settings, + transport, + threadPool, + TransportService.NOOP_TRANSPORT_INTERCEPTOR, + (boundAddress) -> new DiscoveryNode( + "test_node", + "test_node", + boundAddress.publishAddress(), + emptyMap(), + emptySet(), + Version.CURRENT + ), + null, + Collections.emptySet() + ); + actionModule = mock(ActionModule.class); + IdentityPlugin identityPlugin = new IdentityPlugin() { + @Override + public Subject getSubject() { + return new NoopSubject(); + } + + @Override + public List> getExtensionSettings() { + List> settings = new ArrayList>(); + settings.add(customSetting); + return settings; + } + }; + identityService = new IdentityService(Settings.EMPTY, List.of(identityPlugin)); + restController = new RestController( + emptySet(), + null, + new NodeClient(Settings.EMPTY, threadPool), + new NoneCircuitBreakerService(), + new UsageService(), + identityService + ); + when(actionModule.getRestController()).thenReturn(restController); + settingsModule = new SettingsModule(Settings.EMPTY, emptyList(), emptyList(), emptySet()); + clusterService = createClusterService(threadPool); + + extensionDir = createTempDir(); + + extensionNode = new DiscoveryExtensionNode( + "firstExtension", + "uniqueid1", + new TransportAddress(InetAddress.getByName("127.0.0.0"), 9300), + new HashMap(), + Version.fromString("3.0.0"), + Version.fromString("3.0.0"), + Collections.emptyList() + ); + client = new NoOpNodeClient(this.getTestName()); + } + + @Override + @After + public void tearDown() throws Exception { + super.tearDown(); + transportService.close(); + client.close(); + ThreadPool.terminate(threadPool, 30, TimeUnit.SECONDS); + featureFlagSetter.close(); + } + + public void testAdditionalExtensionSettings() throws Exception { + Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); + + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, identityService); + + List expectedExtensions = new ArrayList(); + + String expectedUniqueId = "uniqueid0"; + + expectedExtensions.add( + new DiscoveryExtensionNode( + "firstExtension", + "uniqueid1", + new TransportAddress(InetAddress.getByName("127.0.0.0"), 9300), + new HashMap(), + Version.fromString("3.0.0"), + Version.fromString("3.0.0"), + Collections.emptyList() + ) + ); + + expectedExtensions.add( + new DiscoveryExtensionNode( + "secondExtension", + "uniqueid2", + new TransportAddress(InetAddress.getByName("127.0.0.1"), 9301), + new HashMap(), + Version.fromString("2.0.0"), + Version.fromString("2.0.0"), + List.of() + ) + ); + assertEquals(expectedExtensions.size(), extensionsManager.getExtensionIdMap().values().size()); + for (DiscoveryExtensionNode extension : expectedExtensions) { + DiscoveryExtensionNode initializedExtension = extensionsManager.getExtensionIdMap().get(extension.getId()); + assertEquals(extension.getName(), initializedExtension.getName()); + assertEquals(extension.getId(), initializedExtension.getId()); + assertEquals(extension.getAddress(), initializedExtension.getAddress()); + assertEquals(extension.getAttributes(), initializedExtension.getAttributes()); + assertEquals(extension.getVersion(), initializedExtension.getVersion()); + assertEquals(extension.getMinimumCompatibleVersion(), initializedExtension.getMinimumCompatibleVersion()); + assertEquals(extension.getDependencies(), initializedExtension.getDependencies()); + assertTrue(extensionsManager.lookupExtensionSettingsById(extension.getId()).isPresent()); + if ("firstExtension".equals(extension.getName())) { + assertEquals("custom_setting", extensionsManager.lookupExtensionSettingsById(extension.getId()).get().getAdditionalSettings().get(customSetting)); + } else if ("secondExtension".equals(extension.getName())) { + assertEquals("none", extensionsManager.lookupExtensionSettingsById(extension.getId()).get().getAdditionalSettings().get(customSetting)); + } + } + } +} From 72bb09903a526fbad735aae73b0298781a79733b Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 11 May 2023 12:49:46 -0400 Subject: [PATCH 04/17] Run spotlessApply Signed-off-by: Craig Perkins --- CHANGELOG.md | 3 ++- .../opensearch/extensions/ExtensionsManager.java | 9 +++++++-- .../opensearch/extensions/ExtensionsSettings.java | 2 -- .../extensions/NoopExtensionsManager.java | 1 - .../java/org/opensearch/plugins/IdentityPlugin.java | 3 --- .../ExtensionsManagerWithIdentityTests.java | 13 ++++++++----- 6 files changed, 17 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ffe72c402db8f..ed16315ff9149 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Add events correlation engine plugin ([#6854](https://github.com/opensearch-project/OpenSearch/issues/6854)) - Add connectToNodeAsExtension in TransportService ([#6866](https://github.com/opensearch-project/OpenSearch/pull/6866)) - Adds ExtensionsManager.lookupExtensionSettingsById ([#7466](https://github.com/opensearch-project/OpenSearch/pull/7466)) +- [Extensions] Allow IdentityPlugin to add settings for extensions/extensions.yml ([#7526](https://github.com/opensearch-project/OpenSearch/pull/7526)) ### Dependencies - Bump `log4j-core` from 2.18.0 to 2.19.0 @@ -120,4 +121,4 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Security [Unreleased 3.0]: https://github.com/opensearch-project/OpenSearch/compare/2.x...HEAD -[Unreleased 2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.7...2.x \ No newline at end of file +[Unreleased 2.x]: https://github.com/opensearch-project/OpenSearch/compare/2.7...2.x diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java index 9b8c8029a205b..6239ee28cd237 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java @@ -662,8 +662,13 @@ private ExtensionsSettings readFromExtensionsYml(Path filePath) throws IOExcepti ExtensionAdditionalSettings additionalSettings = new ExtensionAdditionalSettings(Set.of()); if (identityService != null) { - additionalSettings = new ExtensionAdditionalSettings(identityService.getExtensionSettings().stream().collect(Collectors.toSet())); - Set securitySettingsKeys = identityService.getExtensionSettings().stream().map(s -> s.getKey()).collect(Collectors.toSet()); + additionalSettings = new ExtensionAdditionalSettings( + identityService.getExtensionSettings().stream().collect(Collectors.toSet()) + ); + Set securitySettingsKeys = identityService.getExtensionSettings() + .stream() + .map(s -> s.getKey()) + .collect(Collectors.toSet()); Map securitySettings = extensionMap.entrySet() .stream() .filter(kv -> securitySettingsKeys.contains(kv.getKey())) diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsSettings.java b/server/src/main/java/org/opensearch/extensions/ExtensionsSettings.java index 2c59031aa27b9..4b566e0cf45aa 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsSettings.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsSettings.java @@ -8,8 +8,6 @@ package org.opensearch.extensions; -import org.opensearch.common.settings.Settings; - import java.util.ArrayList; import java.util.Collections; import java.util.List; diff --git a/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java index 7f64d2f0ce503..6165423b767ce 100644 --- a/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java @@ -22,7 +22,6 @@ import org.opensearch.extensions.action.ExtensionActionRequest; import org.opensearch.extensions.action.ExtensionActionResponse; import org.opensearch.extensions.action.RemoteExtensionActionResponse; -import org.opensearch.identity.IdentityService; import org.opensearch.index.IndexModule; import org.opensearch.transport.TransportService; diff --git a/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java b/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java index 7ac714359b540..b838d94bee18a 100644 --- a/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java +++ b/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java @@ -9,13 +9,10 @@ package org.opensearch.plugins; import org.opensearch.common.settings.Setting; -import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.identity.Subject; -import org.opensearch.rest.RestHandler; import java.util.Collections; import java.util.List; -import java.util.function.UnaryOperator; /** * Plugin that provides identity and access control for OpenSearch diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java index 61386df1a3832..261dcf9eebcba 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java @@ -56,10 +56,7 @@ import static java.util.Collections.emptyList; import static java.util.Collections.emptyMap; import static java.util.Collections.emptySet; -import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.spy; -import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.opensearch.test.ClusterServiceUtils.createClusterService; @@ -227,9 +224,15 @@ public void testAdditionalExtensionSettings() throws Exception { assertEquals(extension.getDependencies(), initializedExtension.getDependencies()); assertTrue(extensionsManager.lookupExtensionSettingsById(extension.getId()).isPresent()); if ("firstExtension".equals(extension.getName())) { - assertEquals("custom_setting", extensionsManager.lookupExtensionSettingsById(extension.getId()).get().getAdditionalSettings().get(customSetting)); + assertEquals( + "custom_setting", + extensionsManager.lookupExtensionSettingsById(extension.getId()).get().getAdditionalSettings().get(customSetting) + ); } else if ("secondExtension".equals(extension.getName())) { - assertEquals("none", extensionsManager.lookupExtensionSettingsById(extension.getId()).get().getAdditionalSettings().get(customSetting)); + assertEquals( + "none", + extensionsManager.lookupExtensionSettingsById(extension.getId()).get().getAdditionalSettings().get(customSetting) + ); } } } From 531447f8abcffbbe6c77cf8922256615f51b68ea Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 11 May 2023 14:59:06 -0400 Subject: [PATCH 05/17] Re-run CI Signed-off-by: Craig Perkins --- .../extensions/ExtensionsManagerWithIdentityTests.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java index 261dcf9eebcba..eb7a9083db18d 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java @@ -75,10 +75,6 @@ public class ExtensionsManagerWithIdentityTests extends OpenSearchTestCase { private MockNioTransport transport; private Path extensionDir; private final ThreadPool threadPool = new TestThreadPool(ExtensionsManagerWithIdentityTests.class.getSimpleName()); - private final Settings settings = Settings.builder() - .put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT) - .put(Environment.PATH_HOME_SETTING.getKey(), createTempDir().toString()) - .build(); private final List extensionsYmlLines = Arrays.asList( "extensions:", " - name: firstExtension", @@ -187,8 +183,6 @@ public void testAdditionalExtensionSettings() throws Exception { List expectedExtensions = new ArrayList(); - String expectedUniqueId = "uniqueid0"; - expectedExtensions.add( new DiscoveryExtensionNode( "firstExtension", From 23d19e8a62c457244fb7fb2597654f757d454c4b Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 11 May 2023 15:16:07 -0400 Subject: [PATCH 06/17] spotlessApply Signed-off-by: Craig Perkins --- .../extensions/ExtensionsManagerWithIdentityTests.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java index eb7a9083db18d..dcd3e49df3423 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java @@ -13,7 +13,6 @@ import org.opensearch.Version; import org.opensearch.action.ActionModule; import org.opensearch.client.node.NodeClient; -import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.io.stream.NamedWriteableRegistry; @@ -25,7 +24,6 @@ import org.opensearch.common.transport.TransportAddress; import org.opensearch.common.util.FeatureFlags; import org.opensearch.common.util.PageCacheRecycler; -import org.opensearch.env.Environment; import org.opensearch.identity.IdentityService; import org.opensearch.identity.Subject; import org.opensearch.identity.noop.NoopSubject; From b74d52340ccd535f60307401f17ea131af3b02bd Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 18 May 2023 12:36:13 -0400 Subject: [PATCH 07/17] Change contructor to take list of additionalSettings Signed-off-by: Craig Perkins --- .../ExtensionAdditionalSettings.java | 23 -- .../extensions/ExtensionsManager.java | 48 ++-- .../main/java/org/opensearch/node/Node.java | 3 +- .../opensearch/plugins/IdentityPlugin.java | 2 +- .../extensions/ExtensionsManagerTests.java | 82 +++++++ .../ExtensionsManagerWithIdentityTests.java | 231 ------------------ 6 files changed, 105 insertions(+), 284 deletions(-) delete mode 100644 server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionAdditionalSettings.java b/server/src/main/java/org/opensearch/extensions/ExtensionAdditionalSettings.java index 01d0e976594ab..4cc3464e08305 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionAdditionalSettings.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionAdditionalSettings.java @@ -6,29 +6,6 @@ * compatible open source license. */ -/* - * Licensed to Elasticsearch under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ -/* - * Modifications Copyright OpenSearch Contributors. See - * GitHub history for details. - */ - package org.opensearch.extensions; import org.opensearch.common.settings.AbstractScopedSettings; diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java index 6239ee28cd237..f6fe62de95db7 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java @@ -17,6 +17,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Optional; @@ -39,6 +40,7 @@ import org.opensearch.cluster.ClusterSettingsResponse; import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.settings.Setting; import org.opensearch.core.util.FileSystemUtils; import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.settings.Settings; @@ -58,7 +60,6 @@ import org.opensearch.extensions.rest.RestActionsRequestHandler; import org.opensearch.extensions.settings.CustomSettingsRequestHandler; import org.opensearch.extensions.settings.RegisterCustomSettingsRequest; -import org.opensearch.identity.IdentityService; import org.opensearch.index.IndexModule; import org.opensearch.index.IndexService; import org.opensearch.index.IndicesModuleRequest; @@ -118,7 +119,7 @@ public static enum OpenSearchRequestType { private CustomSettingsRequestHandler customSettingsRequestHandler; private TransportService transportService; private ClusterService clusterService; - private IdentityService identityService; + private Set> additionalSettings; private Settings environmentSettings; private AddSettingsUpdateConsumerRequestHandler addSettingsUpdateConsumerRequestHandler; private NodeClient client; @@ -138,7 +139,7 @@ public ExtensionsManager(Path extensionsPath) throws IOException { // will be initialized in initializeServicesAndRestHandler which is called after the Node is initialized this.transportService = null; this.clusterService = null; - this.identityService = null; + this.additionalSettings = new HashSet<>(); this.client = null; this.extensionTransportActionsHandler = null; @@ -153,10 +154,10 @@ public ExtensionsManager(Path extensionsPath) throws IOException { * Instantiate a new ExtensionsManager object to handle requests and responses from extensions. This is called during Node bootstrap. * * @param extensionsPath Path to a directory containing extensions. - * @param identityService Identity Service + * @param additionalSettings Additional settings to read in from extensions.yml * @throws IOException If the extensions discovery file is not properly retrieved. */ - public ExtensionsManager(Path extensionsPath, IdentityService identityService) throws IOException { + public ExtensionsManager(Path extensionsPath, Set> additionalSettings) throws IOException { logger.info("ExtensionsManager initialized"); this.extensionsPath = extensionsPath; this.initializedExtensions = new HashMap(); @@ -165,7 +166,11 @@ public ExtensionsManager(Path extensionsPath, IdentityService identityService) t // will be initialized in initializeServicesAndRestHandler which is called after the Node is initialized this.transportService = null; this.clusterService = null; - this.identityService = identityService; + if (additionalSettings == null) { + this.additionalSettings = new HashSet<>(); + } else { + this.additionalSettings = additionalSettings; + } this.client = null; this.extensionTransportActionsHandler = null; @@ -659,23 +664,14 @@ private ExtensionsSettings readFromExtensionsYml(Path filePath) throws IOExcepti } Settings.Builder output = Settings.builder(); - ExtensionAdditionalSettings additionalSettings = new ExtensionAdditionalSettings(Set.of()); - - if (identityService != null) { - additionalSettings = new ExtensionAdditionalSettings( - identityService.getExtensionSettings().stream().collect(Collectors.toSet()) - ); - Set securitySettingsKeys = identityService.getExtensionSettings() - .stream() - .map(s -> s.getKey()) - .collect(Collectors.toSet()); - Map securitySettings = extensionMap.entrySet() - .stream() - .filter(kv -> securitySettingsKeys.contains(kv.getKey())) - .collect(Collectors.toMap(map -> map.getKey(), map -> map.getValue())); - output.loadFromMap(securitySettings); - additionalSettings.applySettings(output.build()); - } + ExtensionAdditionalSettings extAdditionalSettings = new ExtensionAdditionalSettings(additionalSettings); + Set additionalSettingsKeys = additionalSettings.stream().map(s -> s.getKey()).collect(Collectors.toSet()); + Map additionalSettingsMap = extensionMap.entrySet() + .stream() + .filter(kv -> additionalSettingsKeys.contains(kv.getKey())) + .collect(Collectors.toMap(map -> map.getKey(), map -> map.getValue())); + output.loadFromMap(additionalSettingsMap); + extAdditionalSettings.applySettings(output.build()); // Create extension read from yml config readExtensions.add( @@ -688,7 +684,7 @@ private ExtensionsSettings readFromExtensionsYml(Path filePath) throws IOExcepti extensionMap.get("opensearchVersion").toString(), extensionMap.get("minimumCompatibleVersion").toString(), extensionDependencyList, - additionalSettings + extAdditionalSettings ) ); } catch (IOException e) { @@ -760,10 +756,6 @@ void setClusterService(ClusterService clusterService) { this.clusterService = clusterService; } - void setIdentityService(IdentityService identityService) { - this.identityService = identityService; - } - CustomSettingsRequestHandler getCustomSettingsRequestHandler() { return customSettingsRequestHandler; } diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index 1dd1ee92988a3..147f33e1ea142 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -466,7 +466,8 @@ protected Node( final IdentityService identityService = new IdentityService(settings, identityPlugins); if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS)) { - this.extensionsManager = new ExtensionsManager(initialEnvironment.extensionDir(), identityService); + Set> additionalSettings = identityService.getExtensionSettings().stream().collect(Collectors.toSet()); + this.extensionsManager = new ExtensionsManager(initialEnvironment.extensionDir(), additionalSettings); } else { this.extensionsManager = new NoopExtensionsManager(); } diff --git a/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java b/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java index b838d94bee18a..01bb71d4f528d 100644 --- a/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java +++ b/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java @@ -29,7 +29,7 @@ public interface IdentityPlugin { Subject getSubject(); /** - * Returns a list of additional {@link Setting} definitions for that this plugin adds for extensions + * Returns a list of additional {@link Setting} definitions that this plugin adds for extensions */ default List> getExtensionSettings() { return Collections.emptyList(); diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index 7bee57bb2beec..b8a5526f0b938 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -32,6 +32,7 @@ import java.util.Collections; import java.util.HashMap; import java.util.List; +import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; @@ -71,12 +72,15 @@ import org.opensearch.extensions.rest.RegisterRestActionsRequest; import org.opensearch.extensions.settings.RegisterCustomSettingsRequest; import org.opensearch.identity.IdentityService; +import org.opensearch.identity.Subject; +import org.opensearch.identity.noop.NoopSubject; import org.opensearch.index.IndexModule; import org.opensearch.index.IndexSettings; import org.opensearch.index.analysis.AnalysisRegistry; import org.opensearch.index.engine.EngineConfigFactory; import org.opensearch.index.engine.InternalEngineFactory; import org.opensearch.indices.breaker.NoneCircuitBreakerService; +import org.opensearch.plugins.IdentityPlugin; import org.opensearch.rest.RestController; import org.opensearch.test.FeatureFlagSetter; import org.opensearch.test.IndexSettingsModule; @@ -99,6 +103,8 @@ public class ExtensionsManagerTests extends OpenSearchTestCase { private RestController restController; private SettingsModule settingsModule; private ClusterService clusterService; + private IdentityService identityService; + private Setting customSetting = Setting.simpleString("custom_extension_setting", "none", Property.ExtensionScope); private NodeClient client; private MockNioTransport transport; private Path extensionDir; @@ -116,6 +122,7 @@ public class ExtensionsManagerTests extends OpenSearchTestCase { " version: '0.0.7'", " opensearchVersion: '3.0.0'", " minimumCompatibleVersion: '3.0.0'", + " custom_extension_setting: 'custom_setting'", " - name: secondExtension", " uniqueId: 'uniqueid2'", " hostAddress: '127.0.0.1'", @@ -160,6 +167,20 @@ public void setup() throws Exception { Collections.emptySet() ); actionModule = mock(ActionModule.class); + IdentityPlugin identityPlugin = new IdentityPlugin() { + @Override + public Subject getSubject() { + return new NoopSubject(); + } + + @Override + public List> getExtensionSettings() { + List> settings = new ArrayList>(); + settings.add(customSetting); + return settings; + } + }; + identityService = new IdentityService(Settings.EMPTY, List.of(identityPlugin)); restController = new RestController( emptySet(), null, @@ -895,6 +916,67 @@ public void testIncompatibleExtensionRegistration() throws IOException, IllegalA } } + public void testAdditionalExtensionSettings() throws Exception { + Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); + + Set> additionalSettings = identityService.getExtensionSettings().stream().collect(Collectors.toSet()); + + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, additionalSettings); + + List expectedExtensions = new ArrayList(); + + String expectedUniqueId = "uniqueid0"; + Version expectedVersion = Version.fromString("2.0.0"); + ExtensionDependency expectedDependency = new ExtensionDependency(expectedUniqueId, expectedVersion); + + expectedExtensions.add( + new DiscoveryExtensionNode( + "firstExtension", + "uniqueid1", + new TransportAddress(InetAddress.getByName("127.0.0.0"), 9300), + new HashMap(), + Version.fromString("3.0.0"), + Version.fromString("3.0.0"), + Collections.emptyList() + ) + ); + + expectedExtensions.add( + new DiscoveryExtensionNode( + "secondExtension", + "uniqueid2", + new TransportAddress(InetAddress.getByName("127.0.0.1"), 9301), + new HashMap(), + Version.fromString("2.0.0"), + Version.fromString("2.0.0"), + List.of(expectedDependency) + ) + ); + assertEquals(expectedExtensions.size(), extensionsManager.getExtensionIdMap().values().size()); + for (DiscoveryExtensionNode extension : expectedExtensions) { + DiscoveryExtensionNode initializedExtension = extensionsManager.getExtensionIdMap().get(extension.getId()); + assertEquals(extension.getName(), initializedExtension.getName()); + assertEquals(extension.getId(), initializedExtension.getId()); + assertEquals(extension.getAddress(), initializedExtension.getAddress()); + assertEquals(extension.getAttributes(), initializedExtension.getAttributes()); + assertEquals(extension.getVersion(), initializedExtension.getVersion()); + assertEquals(extension.getMinimumCompatibleVersion(), initializedExtension.getMinimumCompatibleVersion()); + assertEquals(extension.getDependencies(), initializedExtension.getDependencies()); + assertTrue(extensionsManager.lookupExtensionSettingsById(extension.getId()).isPresent()); + if ("firstExtension".equals(extension.getName())) { + assertEquals( + "custom_setting", + extensionsManager.lookupExtensionSettingsById(extension.getId()).get().getAdditionalSettings().get(customSetting) + ); + } else if ("secondExtension".equals(extension.getName())) { + assertEquals( + "none", + extensionsManager.lookupExtensionSettingsById(extension.getId()).get().getAdditionalSettings().get(customSetting) + ); + } + } + } + private void initialize(ExtensionsManager extensionsManager) { transportService.start(); transportService.acceptIncomingRequests(); diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java deleted file mode 100644 index dcd3e49df3423..0000000000000 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerWithIdentityTests.java +++ /dev/null @@ -1,231 +0,0 @@ -/* - * 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.extensions; - -import org.junit.After; -import org.junit.Before; -import org.opensearch.Version; -import org.opensearch.action.ActionModule; -import org.opensearch.client.node.NodeClient; -import org.opensearch.cluster.node.DiscoveryNode; -import org.opensearch.cluster.service.ClusterService; -import org.opensearch.common.io.stream.NamedWriteableRegistry; -import org.opensearch.common.network.NetworkService; -import org.opensearch.common.settings.Setting; -import org.opensearch.common.settings.Setting.Property; -import org.opensearch.common.settings.Settings; -import org.opensearch.common.settings.SettingsModule; -import org.opensearch.common.transport.TransportAddress; -import org.opensearch.common.util.FeatureFlags; -import org.opensearch.common.util.PageCacheRecycler; -import org.opensearch.identity.IdentityService; -import org.opensearch.identity.Subject; -import org.opensearch.identity.noop.NoopSubject; -import org.opensearch.indices.breaker.NoneCircuitBreakerService; -import org.opensearch.plugins.IdentityPlugin; -import org.opensearch.rest.RestController; -import org.opensearch.test.FeatureFlagSetter; -import org.opensearch.test.OpenSearchTestCase; -import org.opensearch.test.client.NoOpNodeClient; -import org.opensearch.test.transport.MockTransportService; -import org.opensearch.threadpool.TestThreadPool; -import org.opensearch.threadpool.ThreadPool; -import org.opensearch.transport.TransportService; -import org.opensearch.transport.nio.MockNioTransport; -import org.opensearch.usage.UsageService; - -import java.net.InetAddress; -import java.nio.charset.StandardCharsets; -import java.nio.file.Files; -import java.nio.file.Path; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.concurrent.TimeUnit; - -import static java.util.Collections.emptyList; -import static java.util.Collections.emptyMap; -import static java.util.Collections.emptySet; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; -import static org.opensearch.test.ClusterServiceUtils.createClusterService; - -public class ExtensionsManagerWithIdentityTests extends OpenSearchTestCase { - - private FeatureFlagSetter featureFlagSetter; - private TransportService transportService; - private ActionModule actionModule; - private RestController restController; - private SettingsModule settingsModule; - private ClusterService clusterService; - private IdentityService identityService; - - private Setting customSetting = Setting.simpleString("custom_extension_setting", "none", Property.ExtensionScope); - private NodeClient client; - private MockNioTransport transport; - private Path extensionDir; - private final ThreadPool threadPool = new TestThreadPool(ExtensionsManagerWithIdentityTests.class.getSimpleName()); - private final List extensionsYmlLines = Arrays.asList( - "extensions:", - " - name: firstExtension", - " uniqueId: uniqueid1", - " hostAddress: '127.0.0.0'", - " port: '9300'", - " version: '0.0.7'", - " opensearchVersion: '3.0.0'", - " minimumCompatibleVersion: '3.0.0'", - " custom_extension_setting: 'custom_setting'", - " - name: secondExtension", - " uniqueId: 'uniqueid2'", - " hostAddress: '127.0.0.1'", - " port: '9301'", - " version: '3.14.16'", - " opensearchVersion: '2.0.0'", - " minimumCompatibleVersion: '2.0.0'" - ); - - private DiscoveryExtensionNode extensionNode; - - @Before - public void setup() throws Exception { - featureFlagSetter = FeatureFlagSetter.set(FeatureFlags.EXTENSIONS); - Settings settings = Settings.builder().put("cluster.name", "test").build(); - transport = new MockNioTransport( - settings, - Version.CURRENT, - threadPool, - new NetworkService(Collections.emptyList()), - PageCacheRecycler.NON_RECYCLING_INSTANCE, - new NamedWriteableRegistry(Collections.emptyList()), - new NoneCircuitBreakerService() - ); - transportService = new MockTransportService( - settings, - transport, - threadPool, - TransportService.NOOP_TRANSPORT_INTERCEPTOR, - (boundAddress) -> new DiscoveryNode( - "test_node", - "test_node", - boundAddress.publishAddress(), - emptyMap(), - emptySet(), - Version.CURRENT - ), - null, - Collections.emptySet() - ); - actionModule = mock(ActionModule.class); - IdentityPlugin identityPlugin = new IdentityPlugin() { - @Override - public Subject getSubject() { - return new NoopSubject(); - } - - @Override - public List> getExtensionSettings() { - List> settings = new ArrayList>(); - settings.add(customSetting); - return settings; - } - }; - identityService = new IdentityService(Settings.EMPTY, List.of(identityPlugin)); - restController = new RestController( - emptySet(), - null, - new NodeClient(Settings.EMPTY, threadPool), - new NoneCircuitBreakerService(), - new UsageService(), - identityService - ); - when(actionModule.getRestController()).thenReturn(restController); - settingsModule = new SettingsModule(Settings.EMPTY, emptyList(), emptyList(), emptySet()); - clusterService = createClusterService(threadPool); - - extensionDir = createTempDir(); - - extensionNode = new DiscoveryExtensionNode( - "firstExtension", - "uniqueid1", - new TransportAddress(InetAddress.getByName("127.0.0.0"), 9300), - new HashMap(), - Version.fromString("3.0.0"), - Version.fromString("3.0.0"), - Collections.emptyList() - ); - client = new NoOpNodeClient(this.getTestName()); - } - - @Override - @After - public void tearDown() throws Exception { - super.tearDown(); - transportService.close(); - client.close(); - ThreadPool.terminate(threadPool, 30, TimeUnit.SECONDS); - featureFlagSetter.close(); - } - - public void testAdditionalExtensionSettings() throws Exception { - Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - - ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, identityService); - - List expectedExtensions = new ArrayList(); - - expectedExtensions.add( - new DiscoveryExtensionNode( - "firstExtension", - "uniqueid1", - new TransportAddress(InetAddress.getByName("127.0.0.0"), 9300), - new HashMap(), - Version.fromString("3.0.0"), - Version.fromString("3.0.0"), - Collections.emptyList() - ) - ); - - expectedExtensions.add( - new DiscoveryExtensionNode( - "secondExtension", - "uniqueid2", - new TransportAddress(InetAddress.getByName("127.0.0.1"), 9301), - new HashMap(), - Version.fromString("2.0.0"), - Version.fromString("2.0.0"), - List.of() - ) - ); - assertEquals(expectedExtensions.size(), extensionsManager.getExtensionIdMap().values().size()); - for (DiscoveryExtensionNode extension : expectedExtensions) { - DiscoveryExtensionNode initializedExtension = extensionsManager.getExtensionIdMap().get(extension.getId()); - assertEquals(extension.getName(), initializedExtension.getName()); - assertEquals(extension.getId(), initializedExtension.getId()); - assertEquals(extension.getAddress(), initializedExtension.getAddress()); - assertEquals(extension.getAttributes(), initializedExtension.getAttributes()); - assertEquals(extension.getVersion(), initializedExtension.getVersion()); - assertEquals(extension.getMinimumCompatibleVersion(), initializedExtension.getMinimumCompatibleVersion()); - assertEquals(extension.getDependencies(), initializedExtension.getDependencies()); - assertTrue(extensionsManager.lookupExtensionSettingsById(extension.getId()).isPresent()); - if ("firstExtension".equals(extension.getName())) { - assertEquals( - "custom_setting", - extensionsManager.lookupExtensionSettingsById(extension.getId()).get().getAdditionalSettings().get(customSetting) - ); - } else if ("secondExtension".equals(extension.getName())) { - assertEquals( - "none", - extensionsManager.lookupExtensionSettingsById(extension.getId()).get().getAdditionalSettings().get(customSetting) - ); - } - } - } -} From 5b72fc65a604436d599041d4da2f65224c14e72d Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 18 May 2023 12:53:49 -0400 Subject: [PATCH 08/17] One constructor Signed-off-by: Craig Perkins --- .../extensions/ExtensionsManager.java | 26 ------------ .../extensions/NoopExtensionsManager.java | 3 +- .../extensions/ExtensionsManagerTests.java | 40 +++++++++---------- 3 files changed, 22 insertions(+), 47 deletions(-) diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java index f6fe62de95db7..761dc1c5eb77a 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java @@ -124,32 +124,6 @@ public static enum OpenSearchRequestType { private AddSettingsUpdateConsumerRequestHandler addSettingsUpdateConsumerRequestHandler; private NodeClient client; - /** - * Instantiate a new ExtensionsManager object to handle requests and responses from extensions. This is called during Node bootstrap. - * - * @param extensionsPath Path to a directory containing extensions. - * @throws IOException If the extensions discovery file is not properly retrieved. - */ - public ExtensionsManager(Path extensionsPath) throws IOException { - logger.info("ExtensionsManager initialized"); - this.extensionsPath = extensionsPath; - this.initializedExtensions = new HashMap(); - this.extensionIdMap = new HashMap(); - this.extensionSettingsMap = new HashMap(); - // will be initialized in initializeServicesAndRestHandler which is called after the Node is initialized - this.transportService = null; - this.clusterService = null; - this.additionalSettings = new HashSet<>(); - this.client = null; - this.extensionTransportActionsHandler = null; - - /* - * Now Discover extensions - */ - discover(); - - } - /** * Instantiate a new ExtensionsManager object to handle requests and responses from extensions. This is called during Node bootstrap. * diff --git a/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java index 6165423b767ce..21129da3af6a2 100644 --- a/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/NoopExtensionsManager.java @@ -12,6 +12,7 @@ import java.net.UnknownHostException; import java.nio.file.Path; import java.util.Optional; +import java.util.Set; import org.opensearch.action.ActionModule; import org.opensearch.client.node.NodeClient; @@ -33,7 +34,7 @@ public class NoopExtensionsManager extends ExtensionsManager { public NoopExtensionsManager() throws IOException { - super(Path.of("")); + super(Path.of(""), Set.of()); } @Override diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index b8a5526f0b938..4a0baccf1080e 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -219,7 +219,7 @@ public void tearDown() throws Exception { public void testDiscover() throws Exception { Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, Set.of()); List expectedExtensions = new ArrayList(); @@ -272,7 +272,7 @@ public void testNonUniqueExtensionsDiscovery() throws Exception { .collect(Collectors.toList()); Files.write(emptyExtensionDir.resolve("extensions.yml"), nonUniqueYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(emptyExtensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(emptyExtensionDir, Set.of()); List expectedExtensions = new ArrayList(); @@ -321,7 +321,7 @@ public void testMissingRequiredFieldsInExtensionDiscovery() throws Exception { ) ); - extensionsManager = new ExtensionsManager(emptyExtensionDir); + extensionsManager = new ExtensionsManager(emptyExtensionDir, Set.of()); mockLogAppender.assertAllExpectationsMatched(); } @@ -406,7 +406,7 @@ public void testNonAccessibleDirectory() throws Exception { AccessControlException e = expectThrows( AccessControlException.class, - () -> new ExtensionsManager(PathUtils.get("")) + () -> new ExtensionsManager(PathUtils.get(""), Set.of()) ); assertEquals("access denied (\"java.io.FilePermission\" \"\" \"read\")", e.getMessage()); } @@ -425,7 +425,7 @@ public void testNoExtensionsFile() throws Exception { ) ); - new ExtensionsManager(extensionDir); + new ExtensionsManager(extensionDir, Set.of()); mockLogAppender.assertAllExpectationsMatched(); } @@ -439,12 +439,12 @@ public void testEmptyExtensionsFile() throws Exception { Settings settings = Settings.builder().build(); - expectThrows(IOException.class, () -> new ExtensionsManager(emptyExtensionDir)); + expectThrows(IOException.class, () -> new ExtensionsManager(emptyExtensionDir, Set.of())); } public void testInitialize() throws Exception { Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, Set.of()); initialize(extensionsManager); @@ -487,7 +487,7 @@ public void testInitialize() throws Exception { public void testHandleRegisterRestActionsRequest() throws Exception { Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, Set.of()); initialize(extensionsManager); String uniqueIdStr = "uniqueid1"; @@ -502,7 +502,7 @@ public void testHandleRegisterRestActionsRequest() throws Exception { public void testHandleRegisterSettingsRequest() throws Exception { Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, Set.of()); initialize(extensionsManager); String uniqueIdStr = "uniqueid1"; @@ -518,7 +518,7 @@ public void testHandleRegisterSettingsRequest() throws Exception { } public void testHandleRegisterRestActionsRequestWithInvalidMethod() throws Exception { - ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, Set.of()); initialize(extensionsManager); String uniqueIdStr = "uniqueid1"; @@ -532,7 +532,7 @@ public void testHandleRegisterRestActionsRequestWithInvalidMethod() throws Excep } public void testHandleRegisterRestActionsRequestWithInvalidDeprecatedMethod() throws Exception { - ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, Set.of()); initialize(extensionsManager); String uniqueIdStr = "uniqueid1"; @@ -546,7 +546,7 @@ public void testHandleRegisterRestActionsRequestWithInvalidDeprecatedMethod() th } public void testHandleRegisterRestActionsRequestWithInvalidUri() throws Exception { - ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, Set.of()); initialize(extensionsManager); String uniqueIdStr = "uniqueid1"; List actionsList = List.of("GET", "PUT /bar", "POST /baz"); @@ -559,7 +559,7 @@ public void testHandleRegisterRestActionsRequestWithInvalidUri() throws Exceptio } public void testHandleRegisterRestActionsRequestWithInvalidDeprecatedUri() throws Exception { - ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, Set.of()); initialize(extensionsManager); String uniqueIdStr = "uniqueid1"; List actionsList = List.of("GET /foo", "PUT /bar", "POST /baz"); @@ -572,7 +572,7 @@ public void testHandleRegisterRestActionsRequestWithInvalidDeprecatedUri() throw } public void testHandleExtensionRequest() throws Exception { - ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, Set.of()); initialize(extensionsManager); ExtensionRequest clusterStateRequest = new ExtensionRequest(ExtensionRequestProto.RequestType.REQUEST_EXTENSION_CLUSTER_STATE); @@ -728,7 +728,7 @@ public void testEnvironmentSettingsDefaultValue() throws Exception { public void testAddSettingsUpdateConsumerRequest() throws Exception { Path extensionDir = createTempDir(); Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, Set.of()); initialize(extensionsManager); List> componentSettings = List.of( @@ -775,7 +775,7 @@ public void testHandleAddSettingsUpdateConsumerRequest() throws Exception { Path extensionDir = createTempDir(); Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, Set.of()); initialize(extensionsManager); List> componentSettings = List.of( @@ -797,7 +797,7 @@ public void testHandleAddSettingsUpdateConsumerRequest() throws Exception { public void testUpdateSettingsRequest() throws Exception { Path extensionDir = createTempDir(); Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, Set.of()); initialize(extensionsManager); Setting componentSetting = Setting.boolSetting("falseSetting", false, Property.Dynamic); @@ -826,7 +826,7 @@ public void testUpdateSettingsRequest() throws Exception { public void testRegisterHandler() throws Exception { - ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, Set.of()); TransportService mockTransportService = spy( new TransportService( @@ -853,7 +853,7 @@ public void testRegisterHandler() throws Exception { public void testOnIndexModule() throws Exception { Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, Set.of()); initialize(extensionsManager); Environment environment = TestEnvironment.newEnvironment(settings); @@ -910,7 +910,7 @@ public void testIncompatibleExtensionRegistration() throws IOException, IllegalA ); Files.write(extensionDir.resolve("extensions.yml"), incompatibleExtension, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir); + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, Set.of()); assertEquals(0, extensionsManager.getExtensionIdMap().values().size()); mockLogAppender.assertAllExpectationsMatched(); } From 8229628a7897cf9b6ec3996f8009a35c97048fbf Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 18 May 2023 13:27:22 -0400 Subject: [PATCH 09/17] Remove isAuthenticated Signed-off-by: Craig Perkins --- .../java/org/opensearch/identity/shiro/ShiroSubject.java | 5 ----- server/src/main/java/org/opensearch/identity/Subject.java | 5 ----- .../main/java/org/opensearch/identity/noop/NoopSubject.java | 5 ----- 3 files changed, 15 deletions(-) diff --git a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSubject.java b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSubject.java index ffe10aea411d6..3208d2bb5d8ca 100644 --- a/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSubject.java +++ b/plugins/identity-shiro/src/main/java/org/opensearch/identity/shiro/ShiroSubject.java @@ -88,9 +88,4 @@ public void authenticate(AuthToken authenticationToken) { .orElseThrow(() -> new UnsupportedAuthenticationToken()); shiroSubject.login(authToken); } - - @Override - public boolean isAuthenticated() { - return shiroSubject.isAuthenticated(); - } } diff --git a/server/src/main/java/org/opensearch/identity/Subject.java b/server/src/main/java/org/opensearch/identity/Subject.java index 5518c58d70e4f..cbfdadb5cf6a7 100644 --- a/server/src/main/java/org/opensearch/identity/Subject.java +++ b/server/src/main/java/org/opensearch/identity/Subject.java @@ -29,9 +29,4 @@ public interface Subject { * throws SubjectDisabled */ void authenticate(final AuthToken token); - - /** - * Method that returns whether the current subject of the running thread is authenticated - */ - boolean isAuthenticated(); } diff --git a/server/src/main/java/org/opensearch/identity/noop/NoopSubject.java b/server/src/main/java/org/opensearch/identity/noop/NoopSubject.java index bfced9a0e2895..801225fb16ad3 100644 --- a/server/src/main/java/org/opensearch/identity/noop/NoopSubject.java +++ b/server/src/main/java/org/opensearch/identity/noop/NoopSubject.java @@ -54,9 +54,4 @@ public String toString() { public void authenticate(AuthToken AuthToken) { // Do nothing as noop subject is always logged in } - - @Override - public boolean isAuthenticated() { - return true; - } } From 11fa4404111704f5f262cbec53e573f229251272 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 19 May 2023 09:35:14 -0400 Subject: [PATCH 10/17] Re-run CI Signed-off-by: Craig Perkins --- .../main/java/org/opensearch/extensions/ExtensionsManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java index d242635796c5f..01ed14fd3a3e5 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java @@ -639,13 +639,13 @@ private ExtensionsSettings readFromExtensionsYml(Path filePath) throws IOExcepti } } - Settings.Builder output = Settings.builder(); ExtensionAdditionalSettings extAdditionalSettings = new ExtensionAdditionalSettings(additionalSettings); Set additionalSettingsKeys = additionalSettings.stream().map(s -> s.getKey()).collect(Collectors.toSet()); Map additionalSettingsMap = extensionMap.entrySet() .stream() .filter(kv -> additionalSettingsKeys.contains(kv.getKey())) .collect(Collectors.toMap(map -> map.getKey(), map -> map.getValue())); + Settings.Builder output = Settings.builder(); output.loadFromMap(additionalSettingsMap); extAdditionalSettings.applySettings(output.build()); From 4ba38fa606411b9d11c833796d8751636c6cf19d Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 19 May 2023 10:56:13 -0400 Subject: [PATCH 11/17] Re-run CI Signed-off-by: Craig Perkins --- .../main/java/org/opensearch/extensions/ExtensionsManager.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java index 01ed14fd3a3e5..f6ccc01ed0765 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java @@ -645,6 +645,7 @@ private ExtensionsSettings readFromExtensionsYml(Path filePath) throws IOExcepti .stream() .filter(kv -> additionalSettingsKeys.contains(kv.getKey())) .collect(Collectors.toMap(map -> map.getKey(), map -> map.getValue())); + Settings.Builder output = Settings.builder(); output.loadFromMap(additionalSettingsMap); extAdditionalSettings.applySettings(output.build()); From 1fe5ea625c8ee37bef370024377996dbb696b824 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 23 May 2023 12:03:00 -0400 Subject: [PATCH 12/17] Create ExtensionAwarePlugin extension point Signed-off-by: Craig Perkins --- .../opensearch/identity/IdentityService.java | 8 ------ .../main/java/org/opensearch/node/Node.java | 9 +++++- .../plugins/ExtensionAwarePlugin.java | 28 +++++++++++++++++++ .../opensearch/plugins/IdentityPlugin.java | 11 -------- .../extensions/ExtensionsManagerTests.java | 15 +++------- 5 files changed, 40 insertions(+), 31 deletions(-) create mode 100644 server/src/main/java/org/opensearch/plugins/ExtensionAwarePlugin.java diff --git a/server/src/main/java/org/opensearch/identity/IdentityService.java b/server/src/main/java/org/opensearch/identity/IdentityService.java index 4a2009d6235a4..e7e785ba75231 100644 --- a/server/src/main/java/org/opensearch/identity/IdentityService.java +++ b/server/src/main/java/org/opensearch/identity/IdentityService.java @@ -8,7 +8,6 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.opensearch.OpenSearchException; -import org.opensearch.common.settings.Setting; import org.opensearch.identity.noop.NoopIdentityPlugin; import java.util.List; import org.opensearch.common.settings.Settings; @@ -49,11 +48,4 @@ public IdentityService(final Settings settings, final List ident public Subject getSubject() { return identityPlugin.getSubject(); } - - /** - * Gets a list of settings to register for extensions - */ - public List> getExtensionSettings() { - return identityPlugin.getExtensionSettings(); - } } diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index 147f33e1ea142..f48369bc051b0 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -54,6 +54,7 @@ import org.opensearch.extensions.NoopExtensionsManager; import org.opensearch.monitor.fs.FsInfo; import org.opensearch.monitor.fs.FsProbe; +import org.opensearch.plugins.ExtensionAwarePlugin; import org.opensearch.plugins.SearchPipelinePlugin; import org.opensearch.search.backpressure.SearchBackpressureService; import org.opensearch.search.backpressure.settings.SearchBackpressureSettings; @@ -234,6 +235,7 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; @@ -465,8 +467,13 @@ protected Node( final IdentityService identityService = new IdentityService(settings, identityPlugins); + final List extensionAwarePlugins = new ArrayList<>(); if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS)) { - Set> additionalSettings = identityService.getExtensionSettings().stream().collect(Collectors.toSet()); + extensionAwarePlugins.addAll(pluginsService.filterPlugins(ExtensionAwarePlugin.class)); + Set> additionalSettings = new HashSet<>(); + for (ExtensionAwarePlugin extAwarePlugin : extensionAwarePlugins) { + additionalSettings.addAll(extAwarePlugin.getExtensionSettings()); + } this.extensionsManager = new ExtensionsManager(initialEnvironment.extensionDir(), additionalSettings); } else { this.extensionsManager = new NoopExtensionsManager(); diff --git a/server/src/main/java/org/opensearch/plugins/ExtensionAwarePlugin.java b/server/src/main/java/org/opensearch/plugins/ExtensionAwarePlugin.java new file mode 100644 index 0000000000000..5cdf3e582d775 --- /dev/null +++ b/server/src/main/java/org/opensearch/plugins/ExtensionAwarePlugin.java @@ -0,0 +1,28 @@ +/* + * 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.common.settings.Setting; + +import java.util.Collections; +import java.util.List; + +/** + * Plugin that provides extra settings for extensions + * + * @opensearch.experimental + */ +public interface ExtensionAwarePlugin { + /** + * Returns a list of additional {@link Setting} definitions that this plugin adds for extensions + */ + default List> getExtensionSettings() { + return Collections.emptyList(); + } +} diff --git a/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java b/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java index 01bb71d4f528d..511b5595c5328 100644 --- a/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java +++ b/server/src/main/java/org/opensearch/plugins/IdentityPlugin.java @@ -8,12 +8,8 @@ package org.opensearch.plugins; -import org.opensearch.common.settings.Setting; import org.opensearch.identity.Subject; -import java.util.Collections; -import java.util.List; - /** * Plugin that provides identity and access control for OpenSearch * @@ -27,11 +23,4 @@ public interface IdentityPlugin { * Should never return null * */ Subject getSubject(); - - /** - * Returns a list of additional {@link Setting} definitions that this plugin adds for extensions - */ - default List> getExtensionSettings() { - return Collections.emptyList(); - } } diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index b1d859add5aba..22f6f2d008e7a 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -73,15 +73,13 @@ import org.opensearch.extensions.rest.RegisterRestActionsRequest; import org.opensearch.extensions.settings.RegisterCustomSettingsRequest; import org.opensearch.identity.IdentityService; -import org.opensearch.identity.Subject; -import org.opensearch.identity.noop.NoopSubject; import org.opensearch.index.IndexModule; import org.opensearch.index.IndexSettings; import org.opensearch.index.analysis.AnalysisRegistry; import org.opensearch.index.engine.EngineConfigFactory; import org.opensearch.index.engine.InternalEngineFactory; import org.opensearch.indices.breaker.NoneCircuitBreakerService; -import org.opensearch.plugins.IdentityPlugin; +import org.opensearch.plugins.ExtensionAwarePlugin; import org.opensearch.rest.RestController; import org.opensearch.test.FeatureFlagSetter; import org.opensearch.test.IndexSettingsModule; @@ -105,7 +103,7 @@ public class ExtensionsManagerTests extends OpenSearchTestCase { private RestController restController; private SettingsModule settingsModule; private ClusterService clusterService; - private IdentityService identityService; + private ExtensionAwarePlugin extAwarePlugin; private Setting customSetting = Setting.simpleString("custom_extension_setting", "none", Property.ExtensionScope); private NodeClient client; private MockNioTransport transport; @@ -169,11 +167,7 @@ public void setup() throws Exception { Collections.emptySet() ); actionModule = mock(ActionModule.class); - IdentityPlugin identityPlugin = new IdentityPlugin() { - @Override - public Subject getSubject() { - return new NoopSubject(); - } + extAwarePlugin = new ExtensionAwarePlugin() { @Override public List> getExtensionSettings() { @@ -182,7 +176,6 @@ public List> getExtensionSettings() { return settings; } }; - identityService = new IdentityService(Settings.EMPTY, List.of(identityPlugin)); dynamicActionRegistry = mock(DynamicActionRegistry.class); restController = new RestController( emptySet(), @@ -927,7 +920,7 @@ public void testIncompatibleExtensionRegistration() throws IOException, IllegalA public void testAdditionalExtensionSettings() throws Exception { Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - Set> additionalSettings = identityService.getExtensionSettings().stream().collect(Collectors.toSet()); + Set> additionalSettings = extAwarePlugin.getExtensionSettings().stream().collect(Collectors.toSet()); ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, additionalSettings); From 6dfa28444dc20469fc74e669a90f00ec919e18e1 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 23 May 2023 12:16:11 -0400 Subject: [PATCH 13/17] Update CHANGELOG Signed-off-by: Craig Perkins --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b013e98956432..4e50d8b0c2195 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -93,7 +93,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Provide mechanism to configure XContent parsing constraints (after update to Jackson 2.15.0 and above) ([#7550](https://github.com/opensearch-project/OpenSearch/pull/7550)) - Support to clear filecache using clear indices cache API ([#7498](https://github.com/opensearch-project/OpenSearch/pull/7498)) - Create NamedRoute to map extension routes to a shortened name ([#6870](https://github.com/opensearch-project/OpenSearch/pull/6870)) -- [Extensions] Allow IdentityPlugin to add settings for extensions/extensions.yml ([#7526](https://github.com/opensearch-project/OpenSearch/pull/7526)) +- [Extensions] Add ExtensionAwarePlugin extension point to add custom settings for extensions ([#7526](https://github.com/opensearch-project/OpenSearch/pull/7526)) ### Dependencies - Bump `com.netflix.nebula:gradle-info-plugin` from 12.0.0 to 12.1.3 (#7564) From 689ef48ffa037a80c5f86c0c9a965ab28b7443f6 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 23 May 2023 14:03:11 -0400 Subject: [PATCH 14/17] Address code review feedback Signed-off-by: Craig Perkins --- ...onalSettings.java => ExtensionScopedSettings.java} | 6 +++--- .../org/opensearch/extensions/ExtensionsManager.java | 11 +++++------ .../org/opensearch/extensions/ExtensionsSettings.java | 6 +++--- .../org/opensearch/plugins/ExtensionAwarePlugin.java | 1 + 4 files changed, 12 insertions(+), 12 deletions(-) rename server/src/main/java/org/opensearch/extensions/{ExtensionAdditionalSettings.java => ExtensionScopedSettings.java} (74%) diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionAdditionalSettings.java b/server/src/main/java/org/opensearch/extensions/ExtensionScopedSettings.java similarity index 74% rename from server/src/main/java/org/opensearch/extensions/ExtensionAdditionalSettings.java rename to server/src/main/java/org/opensearch/extensions/ExtensionScopedSettings.java index 4cc3464e08305..0c87ce31df737 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionAdditionalSettings.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionScopedSettings.java @@ -22,13 +22,13 @@ * * @opensearch.internal */ -public final class ExtensionAdditionalSettings extends AbstractScopedSettings { +public final class ExtensionScopedSettings extends AbstractScopedSettings { - public ExtensionAdditionalSettings(final Set> settingsSet) { + public ExtensionScopedSettings(final Set> settingsSet) { this(settingsSet, Collections.emptySet()); } - public ExtensionAdditionalSettings(final Set> settingsSet, final Set> settingUpgraders) { + public ExtensionScopedSettings(final Set> settingsSet, final Set> settingUpgraders) { super(Settings.EMPTY, settingsSet, settingUpgraders, Property.ExtensionScope); } } diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java index f6ccc01ed0765..392822c42ad83 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java @@ -119,7 +119,7 @@ public static enum OpenSearchRequestType { private CustomSettingsRequestHandler customSettingsRequestHandler; private TransportService transportService; private ClusterService clusterService; - private Set> additionalSettings; + private final Set> additionalSettings; private Settings environmentSettings; private AddSettingsUpdateConsumerRequestHandler addSettingsUpdateConsumerRequestHandler; private NodeClient client; @@ -140,10 +140,9 @@ public ExtensionsManager(Path extensionsPath, Set> additionalSettings // will be initialized in initializeServicesAndRestHandler which is called after the Node is initialized this.transportService = null; this.clusterService = null; - if (additionalSettings == null) { - this.additionalSettings = new HashSet<>(); - } else { - this.additionalSettings = additionalSettings; + this.additionalSettings = new HashSet<>(); + if (additionalSettings != null) { + this.additionalSettings.addAll(additionalSettings); } this.client = null; this.extensionTransportActionsHandler = null; @@ -639,7 +638,7 @@ private ExtensionsSettings readFromExtensionsYml(Path filePath) throws IOExcepti } } - ExtensionAdditionalSettings extAdditionalSettings = new ExtensionAdditionalSettings(additionalSettings); + ExtensionScopedSettings extAdditionalSettings = new ExtensionScopedSettings(additionalSettings); Set additionalSettingsKeys = additionalSettings.stream().map(s -> s.getKey()).collect(Collectors.toSet()); Map additionalSettingsMap = extensionMap.entrySet() .stream() diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsSettings.java b/server/src/main/java/org/opensearch/extensions/ExtensionsSettings.java index 4b566e0cf45aa..9d21469c8fa28 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsSettings.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsSettings.java @@ -44,7 +44,7 @@ public static class Extension { private String opensearchVersion; private String minimumCompatibleVersion; private List dependencies = Collections.emptyList(); - private ExtensionAdditionalSettings additionalSettings; + private ExtensionScopedSettings additionalSettings; public Extension( String name, @@ -55,7 +55,7 @@ public Extension( String opensearchVersion, String minimumCompatibleVersion, List dependencies, - ExtensionAdditionalSettings additionalSettings + ExtensionScopedSettings additionalSettings ) { this.name = name; this.uniqueId = uniqueId; @@ -130,7 +130,7 @@ public List getDependencies() { return dependencies; } - public ExtensionAdditionalSettings getAdditionalSettings() { + public ExtensionScopedSettings getAdditionalSettings() { return additionalSettings; } diff --git a/server/src/main/java/org/opensearch/plugins/ExtensionAwarePlugin.java b/server/src/main/java/org/opensearch/plugins/ExtensionAwarePlugin.java index 5cdf3e582d775..c8426bc964287 100644 --- a/server/src/main/java/org/opensearch/plugins/ExtensionAwarePlugin.java +++ b/server/src/main/java/org/opensearch/plugins/ExtensionAwarePlugin.java @@ -19,6 +19,7 @@ * @opensearch.experimental */ public interface ExtensionAwarePlugin { + /** * Returns a list of additional {@link Setting} definitions that this plugin adds for extensions */ From e588b2261aa68d407af7183c3e4f7b0ec876df6a Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 23 May 2023 14:46:10 -0400 Subject: [PATCH 15/17] Compute additionalSettingsKeys outside of loop Signed-off-by: Craig Perkins --- .../main/java/org/opensearch/extensions/ExtensionsManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java index 392822c42ad83..31eda81837c19 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java @@ -604,6 +604,7 @@ private ExtensionsSettings readFromExtensionsYml(Path filePath) throws IOExcepti } List> unreadExtensions = new ArrayList<>((Collection>) obj.get("extensions")); List readExtensions = new ArrayList(); + Set additionalSettingsKeys = additionalSettings.stream().map(s -> s.getKey()).collect(Collectors.toSet()); for (HashMap extensionMap : unreadExtensions) { try { // checking to see whether any required fields are missing from extension.yml file or not @@ -639,7 +640,6 @@ private ExtensionsSettings readFromExtensionsYml(Path filePath) throws IOExcepti } ExtensionScopedSettings extAdditionalSettings = new ExtensionScopedSettings(additionalSettings); - Set additionalSettingsKeys = additionalSettings.stream().map(s -> s.getKey()).collect(Collectors.toSet()); Map additionalSettingsMap = extensionMap.entrySet() .stream() .filter(kv -> additionalSettingsKeys.contains(kv.getKey())) From 1d1273fb5722afcb731e6fb76b9cd20a9fc149cb Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 23 May 2023 17:26:17 -0400 Subject: [PATCH 16/17] Address code review feedback Signed-off-by: Craig Perkins --- .../main/java/org/opensearch/node/Node.java | 3 +- .../extensions/ExtensionsManagerTests.java | 89 +++++++++---------- 2 files changed, 41 insertions(+), 51 deletions(-) diff --git a/server/src/main/java/org/opensearch/node/Node.java b/server/src/main/java/org/opensearch/node/Node.java index 9e8078f57cc13..a25bac60f49b6 100644 --- a/server/src/main/java/org/opensearch/node/Node.java +++ b/server/src/main/java/org/opensearch/node/Node.java @@ -467,9 +467,8 @@ protected Node( final IdentityService identityService = new IdentityService(settings, identityPlugins); - final List extensionAwarePlugins = new ArrayList<>(); if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS)) { - extensionAwarePlugins.addAll(pluginsService.filterPlugins(ExtensionAwarePlugin.class)); + final List extensionAwarePlugins = pluginsService.filterPlugins(ExtensionAwarePlugin.class); Set> additionalSettings = new HashSet<>(); for (ExtensionAwarePlugin extAwarePlugin : extensionAwarePlugins) { additionalSettings.addAll(extAwarePlugin.getExtensionSettings()); diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index cb672eca1d3ec..75a1d9ec62c82 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -873,65 +873,56 @@ public void testIncompatibleExtensionRegistration() throws IOException, IllegalA } } - public void testAdditionalExtensionSettings() throws Exception { + public void testAdditionalExtensionSettingsForExtensionWithCustomSettingSet() throws Exception { Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); Set> additionalSettings = extAwarePlugin.getExtensionSettings().stream().collect(Collectors.toSet()); ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, additionalSettings); - List expectedExtensions = new ArrayList(); + DiscoveryExtensionNode extension = new DiscoveryExtensionNode( + "firstExtension", + "uniqueid1", + new TransportAddress(InetAddress.getByName("127.0.0.1"), 9300), + new HashMap(), + Version.fromString("3.0.0"), + Version.fromString("3.0.0"), + List.of() + ); + DiscoveryExtensionNode initializedExtension = extensionsManager.getExtensionIdMap().get(extension.getId()); + assertEquals(extension.getName(), initializedExtension.getName()); + assertEquals(extension.getId(), initializedExtension.getId()); + assertTrue(extensionsManager.lookupExtensionSettingsById(extension.getId()).isPresent()); + assertEquals( + "custom_setting", + extensionsManager.lookupExtensionSettingsById(extension.getId()).get().getAdditionalSettings().get(customSetting) + ); + } - String expectedUniqueId = "uniqueid0"; - Version expectedVersion = Version.fromString("2.0.0"); - ExtensionDependency expectedDependency = new ExtensionDependency(expectedUniqueId, expectedVersion); + public void testAdditionalExtensionSettingsForExtensionWithoutCustomSettingSet() throws Exception { + Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - expectedExtensions.add( - new DiscoveryExtensionNode( - "firstExtension", - "uniqueid1", - new TransportAddress(InetAddress.getByName("127.0.0.0"), 9300), - new HashMap(), - Version.fromString("3.0.0"), - Version.fromString("3.0.0"), - Collections.emptyList() - ) - ); + Set> additionalSettings = extAwarePlugin.getExtensionSettings().stream().collect(Collectors.toSet()); - expectedExtensions.add( - new DiscoveryExtensionNode( - "secondExtension", - "uniqueid2", - new TransportAddress(InetAddress.getByName("127.0.0.1"), 9301), - new HashMap(), - Version.fromString("2.0.0"), - Version.fromString("2.0.0"), - List.of(expectedDependency) - ) + ExtensionsManager extensionsManager = new ExtensionsManager(extensionDir, additionalSettings); + + DiscoveryExtensionNode extension = new DiscoveryExtensionNode( + "secondExtension", + "uniqueid2", + new TransportAddress(InetAddress.getByName("127.0.0.1"), 9301), + new HashMap(), + Version.fromString("2.0.0"), + Version.fromString("2.0.0"), + List.of() + ); + DiscoveryExtensionNode initializedExtension = extensionsManager.getExtensionIdMap().get(extension.getId()); + assertEquals(extension.getName(), initializedExtension.getName()); + assertEquals(extension.getId(), initializedExtension.getId()); + assertTrue(extensionsManager.lookupExtensionSettingsById(extension.getId()).isPresent()); + assertEquals( + "none", + extensionsManager.lookupExtensionSettingsById(extension.getId()).get().getAdditionalSettings().get(customSetting) ); - assertEquals(expectedExtensions.size(), extensionsManager.getExtensionIdMap().values().size()); - for (DiscoveryExtensionNode extension : expectedExtensions) { - DiscoveryExtensionNode initializedExtension = extensionsManager.getExtensionIdMap().get(extension.getId()); - assertEquals(extension.getName(), initializedExtension.getName()); - assertEquals(extension.getId(), initializedExtension.getId()); - assertEquals(extension.getAddress(), initializedExtension.getAddress()); - assertEquals(extension.getAttributes(), initializedExtension.getAttributes()); - assertEquals(extension.getVersion(), initializedExtension.getVersion()); - assertEquals(extension.getMinimumCompatibleVersion(), initializedExtension.getMinimumCompatibleVersion()); - assertEquals(extension.getDependencies(), initializedExtension.getDependencies()); - assertTrue(extensionsManager.lookupExtensionSettingsById(extension.getId()).isPresent()); - if ("firstExtension".equals(extension.getName())) { - assertEquals( - "custom_setting", - extensionsManager.lookupExtensionSettingsById(extension.getId()).get().getAdditionalSettings().get(customSetting) - ); - } else if ("secondExtension".equals(extension.getName())) { - assertEquals( - "none", - extensionsManager.lookupExtensionSettingsById(extension.getId()).get().getAdditionalSettings().get(customSetting) - ); - } - } } private void initialize(ExtensionsManager extensionsManager) { From 9aa929c12e7e6d348e5f4c16e9881a59a87850f6 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 23 May 2023 18:26:56 -0400 Subject: [PATCH 17/17] Add comment Signed-off-by: Craig Perkins --- .../main/java/org/opensearch/extensions/ExtensionsManager.java | 1 + 1 file changed, 1 insertion(+) diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java index 30710e0c8fcf6..9d74e8f22d2b1 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java @@ -130,6 +130,7 @@ public ExtensionsManager(Path extensionsPath, Set> additionalSettings // will be initialized in initializeServicesAndRestHandler which is called after the Node is initialized this.transportService = null; this.clusterService = null; + // Settings added to extensions.yml by ExtensionAwarePlugins, such as security settings this.additionalSettings = new HashSet<>(); if (additionalSettings != null) { this.additionalSettings.addAll(additionalSettings);