From 1ff9649b141e80d3215715bf99cd8cc521c4f226 Mon Sep 17 00:00:00 2001 From: Daniel Widdis Date: Tue, 12 Mar 2024 11:12:12 -0700 Subject: [PATCH] Remove HTTP Host step (#562) Signed-off-by: Daniel Widdis --- CHANGELOG.md | 2 - .../flowframework/common/CommonValue.java | 8 -- .../flowframework/workflow/HttpHostStep.java | 115 ----------------- .../workflow/WorkflowStepFactory.java | 16 +-- .../model/WorkflowValidatorTests.java | 6 +- .../workflow/HttpHostStepTests.java | 121 ------------------ 6 files changed, 2 insertions(+), 266 deletions(-) delete mode 100644 src/main/java/org/opensearch/flowframework/workflow/HttpHostStep.java delete mode 100644 src/test/java/org/opensearch/flowframework/workflow/HttpHostStepTests.java diff --git a/CHANGELOG.md b/CHANGELOG.md index dc4b74fbf..c0fa14f60 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,8 +14,6 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) ## [Unreleased 2.x](https://github.com/opensearch-project/flow-framework/compare/2.12...2.x) ### Features -- Add HttpHost WorkflowStep ([#530](https://github.com/opensearch-project/flow-framework/pull/530)) - ### Enhancements - Substitute REST path or body parameters in Workflow Steps ([#525](https://github.com/opensearch-project/flow-framework/pull/525)) - Added an optional workflow_step param to the get workflow steps API ([#538](https://github.com/opensearch-project/flow-framework/pull/538)) diff --git a/src/main/java/org/opensearch/flowframework/common/CommonValue.java b/src/main/java/org/opensearch/flowframework/common/CommonValue.java index ec88a3778..5acc34b36 100644 --- a/src/main/java/org/opensearch/flowframework/common/CommonValue.java +++ b/src/main/java/org/opensearch/flowframework/common/CommonValue.java @@ -162,14 +162,6 @@ private CommonValue() {} public static final String APP_TYPE_FIELD = "app_type"; /** To include field for an agent response */ public static final String INCLUDE_OUTPUT_IN_AGENT_RESPONSE = "include_output_in_agent_response"; - /** HttpHost */ - public static final String HTTP_HOST_FIELD = "http_host"; - /** Http scheme */ - public static final String SCHEME_FIELD = "scheme"; - /** Http hostname */ - public static final String HOSTNAME_FIELD = "hostname"; - /** Http port */ - public static final String PORT_FIELD = "port"; /* * Constants associated with resource provisioning / state diff --git a/src/main/java/org/opensearch/flowframework/workflow/HttpHostStep.java b/src/main/java/org/opensearch/flowframework/workflow/HttpHostStep.java deleted file mode 100644 index 147073e6a..000000000 --- a/src/main/java/org/opensearch/flowframework/workflow/HttpHostStep.java +++ /dev/null @@ -1,115 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * 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.flowframework.workflow; - -import org.apache.hc.core5.http.HttpHost; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; -import org.opensearch.action.support.PlainActionFuture; -import org.opensearch.core.rest.RestStatus; -import org.opensearch.flowframework.exception.FlowFrameworkException; -import org.opensearch.flowframework.util.ParseUtils; - -import java.util.Collections; -import java.util.Locale; -import java.util.Map; -import java.util.Set; - -import static org.opensearch.flowframework.common.CommonValue.HOSTNAME_FIELD; -import static org.opensearch.flowframework.common.CommonValue.HTTP_HOST_FIELD; -import static org.opensearch.flowframework.common.CommonValue.PORT_FIELD; -import static org.opensearch.flowframework.common.CommonValue.SCHEME_FIELD; - -/** - * Step to register parameters for an HTTP Connection to a Host - */ -public class HttpHostStep implements WorkflowStep { - - private static final Logger logger = LogManager.getLogger(HttpHostStep.class); - PlainActionFuture hostFuture = PlainActionFuture.newFuture(); - static final String NAME = "http_host"; - - @Override - public PlainActionFuture execute( - String currentNodeId, - WorkflowData currentNodeInputs, - Map outputs, - Map previousNodeInputs, - Map params - ) { - Set requiredKeys = Set.of(SCHEME_FIELD, HOSTNAME_FIELD, PORT_FIELD); - // TODO Possibly add credentials fields here - // See ML Commons MLConnectorInput class and its usage - Set optionalKeys = Collections.emptySet(); - - try { - Map inputs = ParseUtils.getInputsFromPreviousSteps( - requiredKeys, - optionalKeys, - currentNodeInputs, - outputs, - previousNodeInputs, - params - ); - - String scheme = validScheme(inputs.get(SCHEME_FIELD)); - String hostname = validHostName(inputs.get(HOSTNAME_FIELD)); - int port = validPort(inputs.get(PORT_FIELD)); - - HttpHost httpHost = new HttpHost(scheme, hostname, port); - - hostFuture.onResponse( - new WorkflowData( - Map.ofEntries(Map.entry(HTTP_HOST_FIELD, httpHost)), - currentNodeInputs.getWorkflowId(), - currentNodeInputs.getNodeId() - ) - ); - - logger.info("Http Host registered successfully {}", httpHost); - - } catch (FlowFrameworkException e) { - hostFuture.onFailure(e); - } - return hostFuture; - } - - private String validScheme(Object o) { - String scheme = o.toString().toLowerCase(Locale.ROOT); - if ("http".equals(scheme) || "https".equals(scheme)) { - return scheme; - } - throw new FlowFrameworkException("http_host scheme must be http or https", RestStatus.BAD_REQUEST); - } - - private String validHostName(Object o) { - // TODO Add validation: - // Prevent use of localhost or private IP address ranges - // See ML Commons MLHttpClientFactory.java methods for examples - // Possibly consider an allowlist of addresses - return o.toString(); - } - - private int validPort(Object o) { - try { - int port = Integer.parseInt(o.toString()); - if ((port & 0xffff0000) != 0) { - throw new FlowFrameworkException("http_host port number must be between 0 and 65535", RestStatus.BAD_REQUEST); - } - return port; - } catch (NumberFormatException e) { - throw new FlowFrameworkException("http_host port must be a number between 0 and 65535", RestStatus.BAD_REQUEST); - } - } - - @Override - public String getName() { - return NAME; - } -} diff --git a/src/main/java/org/opensearch/flowframework/workflow/WorkflowStepFactory.java b/src/main/java/org/opensearch/flowframework/workflow/WorkflowStepFactory.java index 8c72e8481..df7b3aa35 100644 --- a/src/main/java/org/opensearch/flowframework/workflow/WorkflowStepFactory.java +++ b/src/main/java/org/opensearch/flowframework/workflow/WorkflowStepFactory.java @@ -35,8 +35,6 @@ import static org.opensearch.flowframework.common.CommonValue.EMBEDDING_DIMENSION; import static org.opensearch.flowframework.common.CommonValue.FRAMEWORK_TYPE; import static org.opensearch.flowframework.common.CommonValue.FUNCTION_NAME; -import static org.opensearch.flowframework.common.CommonValue.HOSTNAME_FIELD; -import static org.opensearch.flowframework.common.CommonValue.HTTP_HOST_FIELD; import static org.opensearch.flowframework.common.CommonValue.MODEL_CONTENT_HASH_VALUE; import static org.opensearch.flowframework.common.CommonValue.MODEL_FORMAT; import static org.opensearch.flowframework.common.CommonValue.MODEL_GROUP_STATUS; @@ -44,10 +42,8 @@ import static org.opensearch.flowframework.common.CommonValue.NAME_FIELD; import static org.opensearch.flowframework.common.CommonValue.OPENSEARCH_ML; import static org.opensearch.flowframework.common.CommonValue.PARAMETERS_FIELD; -import static org.opensearch.flowframework.common.CommonValue.PORT_FIELD; import static org.opensearch.flowframework.common.CommonValue.PROTOCOL_FIELD; import static org.opensearch.flowframework.common.CommonValue.REGISTER_MODEL_STATUS; -import static org.opensearch.flowframework.common.CommonValue.SCHEME_FIELD; import static org.opensearch.flowframework.common.CommonValue.SUCCESS; import static org.opensearch.flowframework.common.CommonValue.TOOLS_FIELD; import static org.opensearch.flowframework.common.CommonValue.TYPE; @@ -106,7 +102,6 @@ public WorkflowStepFactory( stepMap.put(ToolStep.NAME, ToolStep::new); stepMap.put(RegisterAgentStep.NAME, () -> new RegisterAgentStep(mlClient, flowFrameworkIndicesHandler)); stepMap.put(DeleteAgentStep.NAME, () -> new DeleteAgentStep(mlClient)); - stepMap.put(HttpHostStep.NAME, HttpHostStep::new); } /** @@ -201,16 +196,7 @@ public enum WorkflowSteps { DELETE_AGENT(DeleteAgentStep.NAME, List.of(AGENT_ID), List.of(AGENT_ID), List.of(OPENSEARCH_ML), null), /** Create Tool Step */ - CREATE_TOOL(ToolStep.NAME, List.of(TYPE), List.of(TOOLS_FIELD), List.of(OPENSEARCH_ML), null), - - /** Http Host Step */ - HTTP_HOST( - HttpHostStep.NAME, - List.of(SCHEME_FIELD, HOSTNAME_FIELD, PORT_FIELD), - List.of(HTTP_HOST_FIELD), - Collections.emptyList(), - null - ); + CREATE_TOOL(ToolStep.NAME, List.of(TYPE), List.of(TOOLS_FIELD), List.of(OPENSEARCH_ML), null); private final String workflowStepName; private final List inputs; diff --git a/src/test/java/org/opensearch/flowframework/model/WorkflowValidatorTests.java b/src/test/java/org/opensearch/flowframework/model/WorkflowValidatorTests.java index 5ba5924f3..c52e6b9cf 100644 --- a/src/test/java/org/opensearch/flowframework/model/WorkflowValidatorTests.java +++ b/src/test/java/org/opensearch/flowframework/model/WorkflowValidatorTests.java @@ -44,7 +44,7 @@ public void testParseWorkflowValidator() throws IOException { WorkflowValidator validator = new WorkflowValidator(workflowStepValidators); - assertEquals(15, validator.getWorkflowStepValidators().size()); + assertEquals(14, validator.getWorkflowStepValidators().size()); assertTrue(validator.getWorkflowStepValidators().keySet().contains("create_connector")); assertEquals(7, validator.getWorkflowStepValidators().get("create_connector").getInputs().size()); @@ -101,10 +101,6 @@ public void testParseWorkflowValidator() throws IOException { assertTrue(validator.getWorkflowStepValidators().keySet().contains("noop")); assertEquals(0, validator.getWorkflowStepValidators().get("noop").getInputs().size()); assertEquals(0, validator.getWorkflowStepValidators().get("noop").getOutputs().size()); - - assertTrue(validator.getWorkflowStepValidators().keySet().contains("http_host")); - assertEquals(3, validator.getWorkflowStepValidators().get("http_host").getInputs().size()); - assertEquals(1, validator.getWorkflowStepValidators().get("http_host").getOutputs().size()); } public void testWorkflowStepFactoryHasValidators() throws IOException { diff --git a/src/test/java/org/opensearch/flowframework/workflow/HttpHostStepTests.java b/src/test/java/org/opensearch/flowframework/workflow/HttpHostStepTests.java deleted file mode 100644 index bf4c25f11..000000000 --- a/src/test/java/org/opensearch/flowframework/workflow/HttpHostStepTests.java +++ /dev/null @@ -1,121 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * 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.flowframework.workflow; - -import org.apache.hc.core5.http.HttpHost; -import org.opensearch.action.support.PlainActionFuture; -import org.opensearch.flowframework.exception.FlowFrameworkException; -import org.opensearch.test.OpenSearchTestCase; - -import java.util.Collections; -import java.util.Map; -import java.util.concurrent.ExecutionException; - -import static org.opensearch.flowframework.common.CommonValue.HOSTNAME_FIELD; -import static org.opensearch.flowframework.common.CommonValue.HTTP_HOST_FIELD; -import static org.opensearch.flowframework.common.CommonValue.PORT_FIELD; -import static org.opensearch.flowframework.common.CommonValue.SCHEME_FIELD; - -public class HttpHostStepTests extends OpenSearchTestCase { - - public void testHttpHost() throws InterruptedException, ExecutionException { - HttpHostStep httpHostStep = new HttpHostStep(); - assertEquals(HttpHostStep.NAME, httpHostStep.getName()); - - WorkflowData inputData = new WorkflowData( - Map.ofEntries(Map.entry(SCHEME_FIELD, "http"), Map.entry(HOSTNAME_FIELD, "localhost"), Map.entry(PORT_FIELD, 1234)), - "test-id", - "test-node-id" - ); - - PlainActionFuture future = httpHostStep.execute( - inputData.getNodeId(), - inputData, - Collections.emptyMap(), - Collections.emptyMap(), - Collections.emptyMap() - ); - - assertTrue(future.isDone()); - assertEquals(HttpHost.class, future.get().getContent().get(HTTP_HOST_FIELD).getClass()); - HttpHost host = (HttpHost) future.get().getContent().get(HTTP_HOST_FIELD); - assertEquals("http", host.getSchemeName()); - assertEquals("localhost", host.getHostName()); - assertEquals(1234, host.getPort()); - } - - public void testBadScheme() { - HttpHostStep httpHostStep = new HttpHostStep(); - - WorkflowData badSchemeData = new WorkflowData( - Map.ofEntries(Map.entry(SCHEME_FIELD, "ftp"), Map.entry(HOSTNAME_FIELD, "localhost"), Map.entry(PORT_FIELD, 1234)), - "test-id", - "test-node-id" - ); - - PlainActionFuture future = httpHostStep.execute( - badSchemeData.getNodeId(), - badSchemeData, - Collections.emptyMap(), - Collections.emptyMap(), - Collections.emptyMap() - ); - - assertTrue(future.isDone()); - ExecutionException ex = assertThrows(ExecutionException.class, () -> future.get()); - assertEquals(FlowFrameworkException.class, ex.getCause().getClass()); - assertEquals("http_host scheme must be http or https", ex.getCause().getMessage()); - } - - public void testBadPort() { - HttpHostStep httpHostStep = new HttpHostStep(); - - WorkflowData badPortData = new WorkflowData( - Map.ofEntries(Map.entry(SCHEME_FIELD, "https"), Map.entry(HOSTNAME_FIELD, "localhost"), Map.entry(PORT_FIELD, 123456)), - "test-id", - "test-node-id" - ); - - PlainActionFuture future = httpHostStep.execute( - badPortData.getNodeId(), - badPortData, - Collections.emptyMap(), - Collections.emptyMap(), - Collections.emptyMap() - ); - - assertTrue(future.isDone()); - ExecutionException ex = assertThrows(ExecutionException.class, () -> future.get()); - assertEquals(FlowFrameworkException.class, ex.getCause().getClass()); - assertEquals("http_host port number must be between 0 and 65535", ex.getCause().getMessage()); - } - - public void testNoParsePort() { - HttpHostStep httpHostStep = new HttpHostStep(); - - WorkflowData noParsePortData = new WorkflowData( - Map.ofEntries(Map.entry(SCHEME_FIELD, "https"), Map.entry(HOSTNAME_FIELD, "localhost"), Map.entry(PORT_FIELD, "doesn't parse")), - "test-id", - "test-node-id" - ); - - PlainActionFuture future = httpHostStep.execute( - noParsePortData.getNodeId(), - noParsePortData, - Collections.emptyMap(), - Collections.emptyMap(), - Collections.emptyMap() - ); - - assertTrue(future.isDone()); - ExecutionException ex = assertThrows(ExecutionException.class, () -> future.get()); - assertEquals(FlowFrameworkException.class, ex.getCause().getClass()); - assertEquals("http_host port must be a number between 0 and 65535", ex.getCause().getMessage()); - } -}