diff --git a/server/src/main/java/org/opensearch/env/EnvironmentSettingsResponse.java b/server/src/main/java/org/opensearch/env/EnvironmentSettingsResponse.java index 0f541ed8ce51b..3fbe636803e66 100644 --- a/server/src/main/java/org/opensearch/env/EnvironmentSettingsResponse.java +++ b/server/src/main/java/org/opensearch/env/EnvironmentSettingsResponse.java @@ -10,16 +10,10 @@ import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.io.stream.StreamOutput; -import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Settings; import org.opensearch.transport.TransportResponse; -import org.opensearch.common.settings.WriteableSetting; import java.io.IOException; -import java.util.Collections; -import java.util.HashMap; -import java.util.List; -import java.util.Map; import java.util.Objects; /** @@ -28,47 +22,28 @@ * @opensearch.internal */ public class EnvironmentSettingsResponse extends TransportResponse { - private Map, Object> componentSettingValues; + private final Settings environmentSettings; - public EnvironmentSettingsResponse(Settings environmentSettings, List> componentSettings) { - Map, Object> componentSettingValues = new HashMap<>(); - for (Setting componentSetting : componentSettings) { - - // Retrieve component setting value from enviornment settings, or default value if it does not exist - Object componentSettingValue = componentSetting.get(environmentSettings); - componentSettingValues.put(componentSetting, componentSettingValue); - } - this.componentSettingValues = componentSettingValues; + public EnvironmentSettingsResponse(Settings environmentSettings) { + this.environmentSettings = environmentSettings; } public EnvironmentSettingsResponse(StreamInput in) throws IOException { - super(in); - Map, Object> componentSettingValues = new HashMap<>(); - int componentSettingValuesCount = in.readVInt(); - for (int i = 0; i < componentSettingValuesCount; i++) { - Setting componentSetting = new WriteableSetting(in).getSetting(); - Object componentSettingValue = in.readGenericValue(); - componentSettingValues.put(componentSetting, componentSettingValue); - } - this.componentSettingValues = componentSettingValues; + this.environmentSettings = Settings.readSettingsFromStream(in); } @Override public void writeTo(StreamOutput out) throws IOException { - out.writeVInt(componentSettingValues.size()); - for (Map.Entry, Object> entry : componentSettingValues.entrySet()) { - new WriteableSetting(entry.getKey()).writeTo(out); - out.writeGenericValue(entry.getValue()); - } + Settings.writeSettingsToStream(this.environmentSettings, out); } - public Map, Object> getComponentSettingValues() { - return Collections.unmodifiableMap(this.componentSettingValues); + public Settings getEnvironmentSettings() { + return environmentSettings; } @Override public String toString() { - return "EnvironmentSettingsResponse{componentSettingValues=" + componentSettingValues.toString() + '}'; + return "EnvironmentSettingsResponse{environmentSettings=" + environmentSettings.toString() + '}'; } @Override @@ -76,11 +51,11 @@ public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; EnvironmentSettingsResponse that = (EnvironmentSettingsResponse) o; - return Objects.equals(componentSettingValues, that.componentSettingValues); + return Objects.equals(environmentSettings, that.environmentSettings); } @Override public int hashCode() { - return Objects.hash(componentSettingValues); + return Objects.hash(environmentSettings); } } diff --git a/server/src/main/java/org/opensearch/extensions/EnvironmentSettingsRequest.java b/server/src/main/java/org/opensearch/extensions/EnvironmentSettingsRequest.java deleted file mode 100644 index ab470087f8ec9..0000000000000 --- a/server/src/main/java/org/opensearch/extensions/EnvironmentSettingsRequest.java +++ /dev/null @@ -1,79 +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.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; -import org.opensearch.common.io.stream.StreamInput; -import org.opensearch.common.io.stream.StreamOutput; -import org.opensearch.transport.TransportRequest; -import org.opensearch.common.settings.Setting; -import org.opensearch.common.settings.WriteableSetting; - -import java.io.IOException; -import java.util.ArrayList; -import java.util.List; -import java.util.Objects; - -/** - * Environment Settings Request for Extensibility - * - * @opensearch.internal - */ -public class EnvironmentSettingsRequest extends TransportRequest { - private static final Logger logger = LogManager.getLogger(EnvironmentSettingsRequest.class); - private List> componentSettings; - - public EnvironmentSettingsRequest(List> componentSettings) { - this.componentSettings = new ArrayList<>(componentSettings); - } - - public EnvironmentSettingsRequest(StreamInput in) throws IOException { - super(in); - int componentSettingsCount = in.readVInt(); - List> componentSettings = new ArrayList<>(componentSettingsCount); - for (int i = 0; i < componentSettingsCount; i++) { - WriteableSetting writeableSetting = new WriteableSetting(in); - componentSettings.add(writeableSetting.getSetting()); - } - this.componentSettings = componentSettings; - } - - @Override - public void writeTo(StreamOutput out) throws IOException { - super.writeTo(out); - out.writeVInt(this.componentSettings.size()); - for (Setting componentSetting : componentSettings) { - new WriteableSetting(componentSetting).writeTo(out); - } - } - - public List> getComponentSettings() { - return new ArrayList<>(componentSettings); - } - - @Override - public String toString() { - return "EnvironmentSettingsRequest{componentSettings=" + componentSettings.toString() + "}"; - } - - @Override - public boolean equals(Object obj) { - if (this == obj) return true; - if (obj == null || getClass() != obj.getClass()) return false; - EnvironmentSettingsRequest that = (EnvironmentSettingsRequest) obj; - return Objects.equals(componentSettings, that.componentSettings); - } - - @Override - public int hashCode() { - return Objects.hash(componentSettings); - } - -} diff --git a/server/src/main/java/org/opensearch/extensions/EnvironmentSettingsRequestHandler.java b/server/src/main/java/org/opensearch/extensions/EnvironmentSettingsRequestHandler.java deleted file mode 100644 index 723eb482d7c44..0000000000000 --- a/server/src/main/java/org/opensearch/extensions/EnvironmentSettingsRequestHandler.java +++ /dev/null @@ -1,43 +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.opensearch.common.settings.Settings; -import org.opensearch.env.EnvironmentSettingsResponse; -import org.opensearch.transport.TransportResponse; - -/** - * Handles requests to retrieve environment settings. - * - * @opensearch.internal - */ -public class EnvironmentSettingsRequestHandler { - - private final Settings initialEnvironmentSettings; - - /** - * Instantiates a new Environment Settings Request Handler with the environment settings - * - * @param initialEnvironmentSettings the finalized view of environment {@link Settings} - */ - public EnvironmentSettingsRequestHandler(Settings initialEnvironmentSettings) { - this.initialEnvironmentSettings = initialEnvironmentSettings; - } - - /** - * Handles a {@link EnvironmentSettingsRequest}. - * - * @param environmentSettingsRequest The request to handle. - * @return A {@link EnvironmentSettingsResponse} - * @throws Exception if the request is not handled properly. - */ - TransportResponse handleEnvironmentSettingsRequest(EnvironmentSettingsRequest environmentSettingsRequest) throws Exception { - return new EnvironmentSettingsResponse(this.initialEnvironmentSettings, environmentSettingsRequest.getComponentSettings()); - } -} diff --git a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java index cda8b40ef1b2d..334ded61cae7c 100644 --- a/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java +++ b/server/src/main/java/org/opensearch/extensions/ExtensionsManager.java @@ -56,6 +56,7 @@ import org.opensearch.transport.TransportResponse; import org.opensearch.transport.TransportResponseHandler; import org.opensearch.transport.TransportService; +import org.opensearch.env.EnvironmentSettingsResponse; import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.dataformat.yaml.YAMLFactory; @@ -96,6 +97,7 @@ public static enum RequestType { REQUEST_EXTENSION_ACTION_LISTENER_ON_FAILURE, REQUEST_EXTENSION_REGISTER_REST_ACTIONS, REQUEST_EXTENSION_REGISTER_SETTINGS, + REQUEST_EXTENSION_ENVIRONMENT_SETTINGS, CREATE_COMPONENT, ON_INDEX_MODULE, GET_SETTINGS @@ -120,7 +122,7 @@ public static enum OpenSearchRequestType { private ClusterService clusterService; private ExtensionActionListener listener; private ExtensionActionListenerHandler listenerHandler; - private EnvironmentSettingsRequestHandler environmentSettingsRequestHandler; + private Settings environmentSettings; private AddSettingsUpdateConsumerRequestHandler addSettingsUpdateConsumerRequestHandler; public ExtensionsManager() { @@ -172,7 +174,7 @@ public void initializeServicesAndRestHandler( this.customSettingsRequestHandler = new CustomSettingsRequestHandler(settingsModule); this.transportService = transportService; this.clusterService = clusterService; - this.environmentSettingsRequestHandler = new EnvironmentSettingsRequestHandler(initialEnvironmentSettings); + this.environmentSettings = initialEnvironmentSettings; this.addSettingsUpdateConsumerRequestHandler = new AddSettingsUpdateConsumerRequestHandler( clusterService, transportService, @@ -235,8 +237,8 @@ private void registerRequestHandler() { ThreadPool.Names.GENERIC, false, false, - EnvironmentSettingsRequest::new, - ((request, channel, task) -> channel.sendResponse(environmentSettingsRequestHandler.handleEnvironmentSettingsRequest(request))) + ExtensionRequest::new, + ((request, channel, task) -> channel.sendResponse(handleExtensionRequest(request))) ); transportService.registerRequestHandler( REQUEST_EXTENSION_ADD_SETTINGS_UPDATE_CONSUMER, @@ -417,6 +419,8 @@ TransportResponse handleExtensionRequest(ExtensionRequest extensionRequest) thro return new LocalNodeResponse(clusterService); case REQUEST_EXTENSION_CLUSTER_SETTINGS: return new ClusterSettingsResponse(clusterService); + case REQUEST_EXTENSION_ENVIRONMENT_SETTINGS: + return new EnvironmentSettingsResponse(this.environmentSettings); default: throw new IllegalArgumentException("Handler not present for the provided request"); } @@ -661,14 +665,6 @@ public static String getRequestExtensionUpdateSettings() { return REQUEST_EXTENSION_UPDATE_SETTINGS; } - public EnvironmentSettingsRequestHandler getEnvironmentSettingsRequestHandler() { - return environmentSettingsRequestHandler; - } - - public void setEnvironmentSettingsRequestHandler(EnvironmentSettingsRequestHandler environmentSettingsRequestHandler) { - this.environmentSettingsRequestHandler = environmentSettingsRequestHandler; - } - public AddSettingsUpdateConsumerRequestHandler getAddSettingsUpdateConsumerRequestHandler() { return addSettingsUpdateConsumerRequestHandler; } @@ -691,4 +687,12 @@ public void setListenerHandler(ExtensionActionListenerHandler listenerHandler) { this.listenerHandler = listenerHandler; } + public Settings getEnvironmentSettings() { + return environmentSettings; + } + + public void setEnvironmentSettings(Settings environmentSettings) { + this.environmentSettings = environmentSettings; + } + } diff --git a/server/src/main/java/org/opensearch/extensions/UpdateSettingsRequest.java b/server/src/main/java/org/opensearch/extensions/UpdateSettingsRequest.java index 3191f189ac18b..6ed7b9a5a6d36 100644 --- a/server/src/main/java/org/opensearch/extensions/UpdateSettingsRequest.java +++ b/server/src/main/java/org/opensearch/extensions/UpdateSettingsRequest.java @@ -25,7 +25,7 @@ * @opensearch.internal */ public class UpdateSettingsRequest extends TransportRequest { - private static final Logger logger = LogManager.getLogger(EnvironmentSettingsRequest.class); + private static final Logger logger = LogManager.getLogger(UpdateSettingsRequest.class); private WriteableSetting.SettingType settingType; private Setting componentSetting; diff --git a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java index ae4f61ffbec99..6ffa683cf6349 100644 --- a/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java +++ b/server/src/test/java/org/opensearch/extensions/ExtensionsManagerTests.java @@ -454,6 +454,11 @@ public void testHandleExtensionRequest() throws Exception { ExtensionRequest localNodeRequest = new ExtensionRequest(ExtensionsManager.RequestType.REQUEST_EXTENSION_LOCAL_NODE); assertEquals(LocalNodeResponse.class, extensionsManager.handleExtensionRequest(localNodeRequest).getClass()); + ExtensionRequest environmentSettingsRequest = new ExtensionRequest( + ExtensionsManager.RequestType.REQUEST_EXTENSION_ENVIRONMENT_SETTINGS + ); + assertEquals(EnvironmentSettingsResponse.class, extensionsManager.handleExtensionRequest(environmentSettingsRequest).getClass()); + ExtensionRequest exceptionRequest = new ExtensionRequest(ExtensionsManager.RequestType.GET_SETTINGS); Exception exception = expectThrows( IllegalArgumentException.class, @@ -479,96 +484,65 @@ public void testHandleActionListenerOnFailureRequest() throws Exception { assertEquals("Test failure", extensionsManager.getListener().getExceptionList().get(0).getMessage()); } - public void testEnvironmentSettingsRequest() throws Exception { - - Path extensionDir = createTempDir(); - Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(settings, extensionDir); - extensionsManager.initializeServicesAndRestHandler(restController, settingsModule, transportService, clusterService, settings); - - List> componentSettings = List.of( - Setting.boolSetting("falseSetting", false, Property.IndexScope, Property.NodeScope), - Setting.simpleString("fooSetting", "foo", Property.Dynamic) - ); + public void testEnvironmentSettingsResponse() throws Exception { - // Test EnvironmentSettingsRequest arg constructor - EnvironmentSettingsRequest environmentSettingsRequest = new EnvironmentSettingsRequest(componentSettings); - List> requestComponentSettings = environmentSettingsRequest.getComponentSettings(); - assertEquals(componentSettings.size(), requestComponentSettings.size()); - assertTrue(requestComponentSettings.containsAll(componentSettings)); - assertTrue(componentSettings.containsAll(requestComponentSettings)); + // Test EnvironmentSettingsResponse arg constructor + EnvironmentSettingsResponse environmentSettingsResponse = new EnvironmentSettingsResponse(settings); + assertEquals(settings, environmentSettingsResponse.getEnvironmentSettings()); - // Test EnvironmentSettingsRequest StreamInput constructor + // Test EnvironmentSettingsResponse StreamInput constructor try (BytesStreamOutput out = new BytesStreamOutput()) { - environmentSettingsRequest.writeTo(out); + environmentSettingsResponse.writeTo(out); out.flush(); try (BytesStreamInput in = new BytesStreamInput(BytesReference.toBytes(out.bytes()))) { - environmentSettingsRequest = new EnvironmentSettingsRequest(in); - requestComponentSettings = environmentSettingsRequest.getComponentSettings(); - assertEquals(componentSettings.size(), requestComponentSettings.size()); - assertTrue(requestComponentSettings.containsAll(componentSettings)); - assertTrue(componentSettings.containsAll(requestComponentSettings)); + environmentSettingsResponse = new EnvironmentSettingsResponse(in); + assertEquals(settings, environmentSettingsResponse.getEnvironmentSettings()); } } } - public void testEnvironmentSettingsResponse() throws Exception { + public void testEnvironmentSettingsRegisteredValue() throws Exception { + // Create setting with value false + Setting boolSetting = Setting.boolSetting("boolSetting", false, Property.Dynamic); - List> componentSettings = List.of( - Setting.boolSetting("falseSetting", false, Property.IndexScope, Property.NodeScope), - Setting.simpleString("fooSetting", "foo", Property.Dynamic) - ); - - // Test EnvironmentSettingsResponse arg constructor - EnvironmentSettingsResponse environmentSettingsResponse = new EnvironmentSettingsResponse(settings, componentSettings); - assertEquals(componentSettings.size(), environmentSettingsResponse.getComponentSettingValues().size()); - - List> responseSettings = new ArrayList<>(); - responseSettings.addAll(environmentSettingsResponse.getComponentSettingValues().keySet()); - assertTrue(responseSettings.containsAll(componentSettings)); - assertTrue(componentSettings.containsAll(responseSettings)); + // Create Settings with registered bool setting with value true + Settings environmentSettings = Settings.builder().put("boolSetting", "true").build(); - // Test EnvironmentSettingsResponse StreamInput constrcutor + EnvironmentSettingsResponse environmentSettingsResponse = new EnvironmentSettingsResponse(environmentSettings); try (BytesStreamOutput out = new BytesStreamOutput()) { environmentSettingsResponse.writeTo(out); out.flush(); try (BytesStreamInput in = new BytesStreamInput(BytesReference.toBytes(out.bytes()))) { environmentSettingsResponse = new EnvironmentSettingsResponse(in); - assertEquals(componentSettings.size(), environmentSettingsResponse.getComponentSettingValues().size()); + assertEquals(environmentSettings, environmentSettingsResponse.getEnvironmentSettings()); - responseSettings = new ArrayList<>(); - responseSettings.addAll(environmentSettingsResponse.getComponentSettingValues().keySet()); - assertTrue(responseSettings.containsAll(componentSettings)); - assertTrue(componentSettings.containsAll(responseSettings)); + // bool setting is registered in Settings object, thus the expected return value is the registered setting value + assertEquals(true, boolSetting.get(environmentSettingsResponse.getEnvironmentSettings())); } } } - public void testHandleEnvironmentSettingsRequest() throws Exception { - - Path extensionDir = createTempDir(); - Files.write(extensionDir.resolve("extensions.yml"), extensionsYmlLines, StandardCharsets.UTF_8); - ExtensionsManager extensionsManager = new ExtensionsManager(settings, extensionDir); - extensionsManager.initializeServicesAndRestHandler(restController, settingsModule, transportService, clusterService, settings); - - List> componentSettings = List.of( - Setting.boolSetting("falseSetting", false, Property.Dynamic), - Setting.boolSetting("trueSetting", true, Property.Dynamic) - ); + public void testEnvironmentSettingsDefaultValue() throws Exception { + // Create setting with value false + Setting boolSetting = Setting.boolSetting("boolSetting", false, Property.Dynamic); - EnvironmentSettingsRequest environmentSettingsRequest = new EnvironmentSettingsRequest(componentSettings); - TransportResponse response = extensionsManager.getEnvironmentSettingsRequestHandler() - .handleEnvironmentSettingsRequest(environmentSettingsRequest); + // Create settings object without registered bool setting + Settings environmentSettings = Settings.builder().put("testSetting", "true").build(); - assertEquals(EnvironmentSettingsResponse.class, response.getClass()); - assertEquals(componentSettings.size(), ((EnvironmentSettingsResponse) response).getComponentSettingValues().size()); + EnvironmentSettingsResponse environmentSettingsResponse = new EnvironmentSettingsResponse(environmentSettings); + try (BytesStreamOutput out = new BytesStreamOutput()) { + environmentSettingsResponse.writeTo(out); + out.flush(); + try (BytesStreamInput in = new BytesStreamInput(BytesReference.toBytes(out.bytes()))) { - List> responseSettings = new ArrayList<>(); - responseSettings.addAll(((EnvironmentSettingsResponse) response).getComponentSettingValues().keySet()); - assertTrue(responseSettings.containsAll(componentSettings)); - assertTrue(componentSettings.containsAll(responseSettings)); + environmentSettingsResponse = new EnvironmentSettingsResponse(in); + assertEquals(environmentSettings, environmentSettingsResponse.getEnvironmentSettings()); + // bool setting is not registered in Settings object, thus the expected return value is the default setting value + assertEquals(false, boolSetting.get(environmentSettingsResponse.getEnvironmentSettings())); + } + } } public void testAddSettingsUpdateConsumerRequest() throws Exception {