Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -539,4 +539,24 @@ private MLCommonsSettings() {}
.boolSetting(ML_PLUGIN_SETTING_PREFIX + "ag_ui_enabled", false, Setting.Property.NodeScope, Setting.Property.Dynamic);
public static final String ML_COMMONS_AG_UI_DISABLED_MESSAGE =
"The AG-UI agent feature is not enabled. To enable, please update the setting " + ML_COMMONS_AG_UI_ENABLED.getKey();

// When enabled, registering an Agent with a name that already exists (per tenant) is rejected.
// Defaults to false for backward compatibility.
public static final Setting<Boolean> ML_COMMONS_AGENT_NAME_UNIQUENESS_ENABLED = Setting
.boolSetting(
ML_PLUGIN_SETTING_PREFIX + "agent_name_uniqueness_enabled",
false,
Setting.Property.NodeScope,
Setting.Property.Dynamic
);

// When enabled, creating an Agentic Memory Container with a name that already exists (per
// tenant) is rejected. Defaults to false for backward compatibility.
public static final Setting<Boolean> ML_COMMONS_AGENTIC_MEMORY_NAME_UNIQUENESS_ENABLED = Setting
.boolSetting(
ML_PLUGIN_SETTING_PREFIX + "agentic_memory_name_uniqueness_enabled",
false,
Setting.Property.NodeScope,
Setting.Property.Dynamic
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
package org.opensearch.ml.common.settings;

import static org.opensearch.ml.common.settings.MLCommonsSettings.ML_COMMONS_AGENTIC_MEMORY_ENABLED;
import static org.opensearch.ml.common.settings.MLCommonsSettings.ML_COMMONS_AGENTIC_MEMORY_NAME_UNIQUENESS_ENABLED;
import static org.opensearch.ml.common.settings.MLCommonsSettings.ML_COMMONS_AGENT_FRAMEWORK_ENABLED;
import static org.opensearch.ml.common.settings.MLCommonsSettings.ML_COMMONS_AGENT_NAME_UNIQUENESS_ENABLED;
import static org.opensearch.ml.common.settings.MLCommonsSettings.ML_COMMONS_AG_UI_ENABLED;
import static org.opensearch.ml.common.settings.MLCommonsSettings.ML_COMMONS_CONNECTOR_PRIVATE_IP_ENABLED;
import static org.opensearch.ml.common.settings.MLCommonsSettings.ML_COMMONS_CONTROLLER_ENABLED;
Expand Down Expand Up @@ -76,6 +78,10 @@ public class MLFeatureEnabledSetting {

private volatile Boolean isAGUIEnabled;

private volatile Boolean isAgentNameUniquenessEnabled;

private volatile Boolean isAgenticMemoryNameUniquenessEnabled;

private final List<SettingsChangeListener> listeners = new ArrayList<>();

public MLFeatureEnabledSetting(ClusterService clusterService, Settings settings) {
Expand All @@ -101,6 +107,8 @@ public MLFeatureEnabledSetting(ClusterService clusterService, Settings settings)
maxJsonSize = MLCommonsSettings.ML_COMMONS_MAX_JSON_SIZE.get(settings);
isMcpHeaderPassthroughEnabled = ML_COMMONS_MCP_HEADER_PASSTHROUGH_ENABLED.get(settings);
isAGUIEnabled = ML_COMMONS_AG_UI_ENABLED.get(settings);
isAgentNameUniquenessEnabled = ML_COMMONS_AGENT_NAME_UNIQUENESS_ENABLED.get(settings);
isAgenticMemoryNameUniquenessEnabled = ML_COMMONS_AGENTIC_MEMORY_NAME_UNIQUENESS_ENABLED.get(settings);

clusterService
.getClusterSettings()
Expand Down Expand Up @@ -140,6 +148,12 @@ public MLFeatureEnabledSetting(ClusterService clusterService, Settings settings)
.getClusterSettings()
.addSettingsUpdateConsumer(ML_COMMONS_MCP_HEADER_PASSTHROUGH_ENABLED, it -> isMcpHeaderPassthroughEnabled = it);
clusterService.getClusterSettings().addSettingsUpdateConsumer(ML_COMMONS_AG_UI_ENABLED, it -> isAGUIEnabled = it);
clusterService
.getClusterSettings()
.addSettingsUpdateConsumer(ML_COMMONS_AGENT_NAME_UNIQUENESS_ENABLED, it -> isAgentNameUniquenessEnabled = it);
clusterService
.getClusterSettings()
.addSettingsUpdateConsumer(ML_COMMONS_AGENTIC_MEMORY_NAME_UNIQUENESS_ENABLED, it -> isAgenticMemoryNameUniquenessEnabled = it);
clusterService.getClusterSettings().addSettingsUpdateConsumer(ML_COMMONS_STATIC_METRIC_COLLECTION_ENABLED, it -> {
isStaticMetricCollectionEnabled = it;
for (SettingsChangeListener listener : listeners) {
Expand Down Expand Up @@ -314,4 +328,24 @@ public boolean isMcpHeaderPassthroughEnabled() {
public boolean isAGUIEnabled() {
return isAGUIEnabled;
}

/**
* Whether agent name uniqueness is enforced. When enabled, registering an agent with a name
* that already exists within the same tenant is rejected. Defaults to false for backward
* compatibility.
* @return whether agent name uniqueness is enforced.
*/
public boolean isAgentNameUniquenessEnabled() {
return isAgentNameUniquenessEnabled;
}

/**
* Whether agentic memory container name uniqueness is enforced. When enabled, creating a
* memory container with a name that already exists within the same tenant is rejected.
* Defaults to false for backward compatibility.
* @return whether agentic memory container name uniqueness is enforced.
*/
public boolean isAgenticMemoryNameUniquenessEnabled() {
return isAgenticMemoryNameUniquenessEnabled;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import java.util.List;
import java.util.regex.Pattern;

import org.apache.commons.lang3.StringUtils;
import org.opensearch.OpenSearchParseException;
import org.opensearch.core.common.io.stream.StreamInput;
import org.opensearch.core.common.io.stream.StreamOutput;
Expand Down Expand Up @@ -52,8 +53,8 @@ public MLCreateMemoryContainerInput(
String tenantId,
List<String> backendRoles
) {
if (name == null) {
throw new IllegalArgumentException("name is null");
if (StringUtils.isBlank(name)) {
throw new IllegalArgumentException("name cannot be null or blank");
}
this.name = name;
this.description = description;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import java.util.ArrayList;
import java.util.List;

import org.apache.commons.lang3.StringUtils;
import org.opensearch.core.common.io.stream.StreamInput;
import org.opensearch.core.common.io.stream.StreamOutput;
import org.opensearch.core.common.io.stream.Writeable;
Expand All @@ -39,6 +40,10 @@ public class MLUpdateMemoryContainerInput implements ToXContentObject, Writeable

@Builder
public MLUpdateMemoryContainerInput(String name, String description, List<String> backendRoles, MemoryConfiguration configuration) {
// Null = "no rename" (name is optional on update), but a non-null blank string is always wrong.
if (name != null && StringUtils.isBlank(name)) {
throw new IllegalArgumentException("name cannot be blank");
}
this.name = name;
this.description = description;
validateBackendRoles(backendRoles);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@ public void setUp() {
MLCommonsSettings.ML_COMMONS_STREAM_ENABLED,
MLCommonsSettings.ML_COMMONS_MAX_JSON_SIZE,
MLCommonsSettings.ML_COMMONS_MCP_HEADER_PASSTHROUGH_ENABLED,
MLCommonsSettings.ML_COMMONS_AG_UI_ENABLED
MLCommonsSettings.ML_COMMONS_AG_UI_ENABLED,
MLCommonsSettings.ML_COMMONS_AGENT_NAME_UNIQUENESS_ENABLED,
MLCommonsSettings.ML_COMMONS_AGENTIC_MEMORY_NAME_UNIQUENESS_ENABLED
)
);
when(mockClusterService.getClusterSettings()).thenReturn(mockClusterSettings);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,16 @@ public void testConstructorWithNullNameDirectConstructor() {
.build();
}

@Test(expected = IllegalArgumentException.class)
public void testConstructorWithBlankName() {
MLCreateMemoryContainerInput.builder().name(" ").description("blank name").build();
}

@Test(expected = IllegalArgumentException.class)
public void testConstructorWithEmptyName() {
MLCreateMemoryContainerInput.builder().name("").description("empty name").build();
}

@Test
public void testStreamInputOutput() throws IOException {
BytesStreamOutput bytesStreamOutput = new BytesStreamOutput();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,23 @@

public class MLUpdateMemoryContainerInputTests {

@Test(expected = IllegalArgumentException.class)
public void testConstructorRejectsBlankName() {
MLUpdateMemoryContainerInput.builder().name(" ").build();
}

@Test(expected = IllegalArgumentException.class)
public void testConstructorRejectsEmptyName() {
MLUpdateMemoryContainerInput.builder().name("").build();
}

@Test
public void testConstructorAllowsNullName() {
// Null name on update means "no rename" - must be allowed.
MLUpdateMemoryContainerInput input = MLUpdateMemoryContainerInput.builder().name(null).description("desc").build();
assertNull(input.getName());
}

@Test
public void testConstructor() {
List<String> backendRoles = Arrays.asList("role1", "role2");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import org.opensearch.ml.engine.algorithms.agent.MLPlanExecuteAndReflectAgentRunner;
import org.opensearch.ml.engine.function_calling.FunctionCallingFactory;
import org.opensearch.ml.engine.indices.MLIndicesHandler;
import org.opensearch.ml.helper.NameUniquenessHelper;
import org.opensearch.ml.utils.RestActionUtils;
import org.opensearch.ml.utils.TenantAwareHelper;
import org.opensearch.remote.metadata.client.PutDataObjectRequest;
Expand Down Expand Up @@ -99,13 +100,78 @@ protected void doExecute(Task task, ActionRequest request, ActionListener<MLRegi
return;
}

// Check if this agent needs model creation
if (mlAgent.usesUnifiedInterface()) {
createModelAndRegisterAgent(mlAgent, listener);
// Validate tenant id before any metadata search so a missing tenant fails fast in
// multi-tenant mode, rather than leaking existence via a 409 from a cross-tenant search.
if (!TenantAwareHelper.validateTenantId(mlFeatureEnabledSetting, mlAgent.getTenantId(), listener)) {
return;
}

registerAgent(mlAgent, listener);
validateAgentNameUniqueness(mlAgent, ActionListener.wrap(unused -> {
// Check if this agent needs model creation
if (mlAgent.usesUnifiedInterface()) {
createModelAndRegisterAgent(mlAgent, listener);
return;
}
registerAgent(mlAgent, listener);
}, listener::onFailure));
}

/**
* When {@code plugins.ml_commons.agent_name_uniqueness_enabled} is enabled, reject the
* registration if an agent with the same name already exists in the same tenant. When the
* setting is disabled (default), this is a no-op so existing clusters remain backward compatible.
*
* <p>For PLAN_EXECUTE_AND_REFLECT agents without a pre-existing executor-agent-id, an internal
* "{@code <name> (ReAct)}" executor agent is auto-created; this method also checks that
* derived name so the auto-created agent cannot collide with an existing one.
*
* <p>Note: this is a best-effort check, not a transactional guard. Two concurrent register
* requests with the same name can both pass this check before either write is visible.
* See the PR description for a follow-up plan if stricter semantics are required.
*/
private void validateAgentNameUniqueness(MLAgent mlAgent, ActionListener<Void> listener) {
if (!mlFeatureEnabledSetting.isAgentNameUniquenessEnabled()) {
listener.onResponse(null);
return;
}

// MLAgent.validate() already rejects null/blank/over-length names upstream, so we rely on
// that invariant here and don't re-check.
String name = mlAgent.getName();
String tenantId = mlAgent.getTenantId();
checkAgentNameAvailable(name, tenantId, ActionListener.wrap(unused -> {
// If this is a PLAN_EXECUTE_AND_REFLECT registration that will auto-create an
// executor agent named "<name> (ReAct)", validate that derived name too.
if (MLAgentType.from(mlAgent.getType()) == MLAgentType.PLAN_EXECUTE_AND_REFLECT
&& mlAgent.getParameters() != null
&& !mlAgent.getParameters().containsKey(MLPlanExecuteAndReflectAgentRunner.EXECUTOR_AGENT_ID_FIELD)) {
checkAgentNameAvailable(name + " (ReAct)", tenantId, listener);
} else {
listener.onResponse(null);
}
}, listener::onFailure));
}

private void checkAgentNameAvailable(String name, String tenantId, ActionListener<Void> listener) {
NameUniquenessHelper.searchByExactName(client, sdkClient, ML_AGENT_INDEX, name, tenantId, ActionListener.wrap(response -> {
if (response == null) {
// Index not yet created - no duplicate possible.
listener.onResponse(null);
return;
}
long totalHits = response.getHits().getTotalHits() == null ? 0 : response.getHits().getTotalHits().value();
if (totalHits > 0) {
listener
.onFailure(
new OpenSearchStatusException(
"An agent with name [" + name + "] already exists. Agent names must be unique.",
RestStatus.CONFLICT
)
);
} else {
listener.onResponse(null);
}
}, listener::onFailure));
}

private void createModelAndRegisterAgent(MLAgent mlAgent, ActionListener<MLRegisterAgentResponse> listener) {
Expand Down Expand Up @@ -185,9 +251,6 @@ private void proceedWithAgentRegistration(MLAgent agent, ActionListener<MLRegist
boolean isHiddenAgent = RestActionUtils.isSuperAdminUser(clusterService, client);
MLAgent mlAgent = agent.toBuilder().createdTime(now).lastUpdateTime(now).isHidden(isHiddenAgent).build();
String tenantId = agent.getTenantId();
if (!TenantAwareHelper.validateTenantId(mlFeatureEnabledSetting, tenantId, listener)) {
return;
}

// If the agent is a PLAN_EXECUTE_AND_REFLECT agent and does not have an executor agent id, create an executor (reAct) agent
if (MLAgentType.from(mlAgent.getType()) == MLAgentType.PLAN_EXECUTE_AND_REFLECT
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import java.time.Instant;

import org.apache.commons.lang3.StringUtils;
import org.opensearch.OpenSearchStatusException;
import org.opensearch.action.ActionRequest;
import org.opensearch.action.DocWriteResponse;
Expand All @@ -32,6 +33,7 @@
import org.opensearch.ml.common.transport.agent.MLAgentUpdateAction;
import org.opensearch.ml.common.transport.agent.MLAgentUpdateInput;
import org.opensearch.ml.common.transport.agent.MLAgentUpdateRequest;
import org.opensearch.ml.helper.NameUniquenessHelper;
import org.opensearch.ml.utils.RestActionUtils;
import org.opensearch.ml.utils.TenantAwareHelper;
import org.opensearch.remote.metadata.client.GetDataObjectRequest;
Expand Down Expand Up @@ -127,7 +129,15 @@ protected void doExecute(Task task, ActionRequest request, ActionListener<Update
)
);
} else {
updateAgent(agentId, mlAgentUpdateInput, retrievedAgent, wrappedListener);
validateAgentNameUniquenessForUpdate(
mlAgentUpdateInput,
retrievedAgent,
ActionListener
.wrap(
unused -> updateAgent(agentId, mlAgentUpdateInput, retrievedAgent, wrappedListener),
wrappedListener::onFailure
)
);
}
} else {
log.error("Failed to validate tenant for Agent ID {}", agentId);
Expand Down Expand Up @@ -180,6 +190,54 @@ private void updateAgent(
});
}

/**
* When {@code plugins.ml_commons.agent_name_uniqueness_enabled} is enabled and the update
* would rename the agent to a new value, reject the update if another agent in the same
* tenant already has that name. Skipped if the setting is off, if the update omits a new
* name (or provides a blank one), or if the new name equals the existing name (so a PUT
* that doesn't actually rename does not 409 against itself). Mirrors the gate shape used
* by {@code TransportUpdateModelGroupAction#updateModelGroup}.
*
* <p>Same best-effort semantics as the register path: two concurrent updates racing on the
* same name can both see zero hits and both succeed.
*/
private void validateAgentNameUniquenessForUpdate(
MLAgentUpdateInput updateInput,
MLAgent originalAgent,
ActionListener<Void> listener
) {
if (!mlFeatureEnabledSetting.isAgentNameUniquenessEnabled()) {
listener.onResponse(null);
return;
}
String newName = updateInput.getName();
if (StringUtils.isBlank(newName) || newName.equals(originalAgent.getName())) {
// No rename (name omitted/blank), or a no-op rename to the current name.
listener.onResponse(null);
return;
}

NameUniquenessHelper
.searchByExactName(client, sdkClient, ML_AGENT_INDEX, newName, originalAgent.getTenantId(), ActionListener.wrap(response -> {
if (response == null) {
listener.onResponse(null);
return;
}
long totalHits = response.getHits().getTotalHits() == null ? 0 : response.getHits().getTotalHits().value();
if (totalHits > 0) {
listener
.onFailure(
new OpenSearchStatusException(
"An agent with name [" + newName + "] already exists. Agent names must be unique.",
RestStatus.CONFLICT
)
);
} else {
listener.onResponse(null);
}
}, listener::onFailure));
}

@VisibleForTesting
boolean isSuperAdminUserWrapper(ClusterService clusterService, Client client) {
return RestActionUtils.isSuperAdminUser(clusterService, client);
Expand Down
Loading
Loading