Skip to content

Fix detector creation with aliases and data streams - #1792

Open
NelTdS wants to merge 3 commits into
opensearch-project:mainfrom
NelTdS:fix/detector-creation-index-patterns
Open

Fix detector creation with aliases and data streams#1792
NelTdS wants to merge 3 commits into
opensearch-project:mainfrom
NelTdS:fix/detector-creation-index-patterns

Conversation

@NelTdS

@NelTdS NelTdS commented Aug 17, 2026

Copy link
Copy Markdown

Description

Fixes several bugs that prevented detector creation when the configured index is an index pattern, alias, or data stream.

Six root causes addressed:

  1. GetIndexMappingsRequest null index (TransportIndexDetectorAction): IndexUtils.getNewIndexByCreationDate() returns null when no concrete index matches the pattern. The null was passed directly to GetIndexMappingsRequest, triggering Validation Failed: 1: index_name is missing. Fix: resolve pattern to concrete index first; fall back to the original name when resolution returns null.
  2. Strict index existence check (TransportIndexDetectorAction): The pre-creation SearchRequest used default strict IndicesOptions, producing Indices not found for patterns with no current backing indices. Fix: use IndicesOptions.LENIENT_EXPAND_OPEN.
  3. GetMappingsRequest strict resolution (MapperService): Three GetMappingsRequest calls used default options, causing OpenSearch to search for a template named -template when the pattern had no backing index, producing a misleading 404. Fix: add LENIENT_EXPAND_OPEN to all three calls.
  4. GetIndexRequest strict resolution (MapperService): resolveConcreteIndex() used a strict GetIndexRequest, failing with no such index for patterns with no current backing index. Fix: use LENIENT_EXPAND_OPEN; return the original name on empty result.
  5. NoSuchElementException on empty mappings (MapperService): doGetMappingAction and doGetMappingsView called .iterator().next() on the mappings map without an emptiness check. A data stream with no mappings returned an empty map, causing an uncaught exception. Fix: guard both call sites; return an empty response instead.
  6. Race condition on concurrent detector creation (TransportIndexDetectorAction): When multiple detectors are created in parallel, all requests read cluster state simultaneously, see the detectors index absent, and all issue CreateIndexRequest. The first succeeds; the remaining requests received ResourceAlreadyExistsException and failed fatally. Fix: catch ResourceAlreadyExistsException in the onFailure handler of initDetectorIndex and treat it as success.

Related Issues

None

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@github-actions

github-actions Bot commented Aug 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit c19bddd)

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Silent failure on non-status exceptions

In the refactored onFailure handler of checkIndicesAndExecute, when the exception is an OpenSearchStatusException, the listener is notified with a FORBIDDEN error, but the method does not return. Execution then falls through to the else branch check which is skipped, meaning only one path fires the listener — however the previous IndexNotFoundException handling was removed. Any OpenSearchStatusException that is not actually a permissions issue (e.g., other status errors) will still be reported as "User doesn't have read permissions", which is misleading. Consider distinguishing genuine 403 errors from other status exceptions.

 
public void onFailure(Exception e) {
    log.debug("check indices and execute failed", e);
    if (e instanceof OpenSearchStatusException) {
        listener.onFailure(SecurityAnalyticsException.wrap(
                new OpenSearchStatusException(String.format(Locale.getDefault(), "User doesn't have read permissions for one or more configured index %s", (Object) detectorIndices), RestStatus.FORBIDDEN)
        ));
    } else {
        listener.onFailure(SecurityAnalyticsException.wrap(e));
    }
}
Behavior change on invalid index

resolveConcreteIndex previously failed with IllegalArgumentException when getIndex returned zero indices; it now passes the original indexName through unchanged. Downstream callers that assumed the returned value was a concrete existing index may now receive a pattern/alias/non-existent name and fail later with a less clear error, or operate on the wrong target. Confirm all callers handle the pass-through case correctly.

 
private void resolveConcreteIndex(String indexName, ActionListener<String> actionListener) throws IOException {

    indicesClient.getIndex(
            (new GetIndexRequest()).indices(indexName).indicesOptions(IndicesOptions.LENIENT_EXPAND_OPEN),
            new ActionListener<>() {
        @Override
        public void onResponse(GetIndexResponse getIndexResponse) {
            String[] indices = getIndexResponse.indices();
            if (indices.length == 0) {
                // Pattern has no backing indices yet — pass the name through as-is
                actionListener.onResponse(indexName);
            } else if (indices.length == 1) {
                actionListener.onResponse(indices[0]);
            } else if (indices.length > 1) {

@github-actions

github-actions Bot commented Aug 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to c19bddd

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Avoid silently accepting invalid index names

Returning the raw indexName when no backing indices exist can cause downstream code
(which expects a concrete index) to fail in confusing ways for typos or invalid
names. Consider limiting this fallback to cases where indexName looks like a
pattern/alias/datastream (e.g., contains wildcards) and still failing for plain
invalid concrete names.

src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java [610-613]

 @Override
 public void onResponse(GetIndexResponse getIndexResponse) {
     String[] indices = getIndexResponse.indices();
     if (indices.length == 0) {
-        // Pattern has no backing indices yet — pass the name through as-is
-        actionListener.onResponse(indexName);
+        if (indexName.contains("*") || IndexUtils.isDataStream(indexName, clusterService.state()) || IndexUtils.isAlias(indexName, clusterService.state())) {
+            // Pattern/alias/datastream has no backing indices yet — pass through as-is
+            actionListener.onResponse(indexName);
+        } else {
+            actionListener.onFailure(
+                    SecurityAnalyticsException.wrap(
+                            new IllegalArgumentException("Invalid index name: [" + indexName + "]")
+                    )
+            );
+        }
     } else if (indices.length == 1) {
Suggestion importance[1-10]: 6

__

Why: Valid concern that the change replaces a strict failure with a permissive fallback, potentially masking invalid index names. However, the improved_code references methods like IndexUtils.isDataStream/isAlias that may not exist, so the exact fix requires validation.

Low
Preserve clear not-found error handling

Removing the explicit IndexNotFoundException branch means index-not-found errors
will now surface as generic wrapped exceptions, losing the previous clear 404
message. Since LENIENT_EXPAND_OPEN is set on other requests but not on this
SearchRequest, an unresolved concrete index can still throw IndexNotFoundException;
preserving a user-friendly NOT_FOUND response would be safer.

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java [261-268]

 } else if (e instanceof OpenSearchStatusException) {
     listener.onFailure(SecurityAnalyticsException.wrap(
             new OpenSearchStatusException(String.format(Locale.getDefault(), "User doesn't have read permissions for one or more configured index %s", (Object) detectorIndices), RestStatus.FORBIDDEN)
+    ));
+} else if (ExceptionsHelper.unwrapCause(e) instanceof org.opensearch.index.IndexNotFoundException) {
+    listener.onFailure(SecurityAnalyticsException.wrap(
+        new OpenSearchStatusException(String.format(Locale.getDefault(), "Indices not found %s", String.join(", ", detectorIndices)), RestStatus.NOT_FOUND)
     ));
 } else {
     listener.onFailure(SecurityAnalyticsException.wrap(e));
 }
Suggestion importance[1-10]: 6

__

Why: Reasonable point that removing the IndexNotFoundException branch loses the clear 404 message. Since LENIENT_EXPAND_OPEN was added to the search request, this may be less impactful, but restoring user-friendly errors is still valuable.

Low
Guard index resolution against exceptions

getNewIndexByCreationDate may throw for unresolvable patterns/aliases rather than
returning null, which would bypass the null-fallback and crash before the async call
executes. Wrap the resolution in a try/catch and fall back to logIndex (or fail via
listener.onFailure) to keep the async contract intact.

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java [1645-1651]

-String resolvedLogIndex = IndexUtils.getNewIndexByCreationDate(
-        clusterService.state(),
-        indexNameExpressionResolver,
-        logIndex
-);
+String resolvedLogIndex;
+try {
+    resolvedLogIndex = IndexUtils.getNewIndexByCreationDate(
+            clusterService.state(),
+            indexNameExpressionResolver,
+            logIndex
+    );
+} catch (Exception ex) {
+    resolvedLogIndex = null;
+}
 final String concreteLogIndex = resolvedLogIndex != null ? resolvedLogIndex : logIndex;
 client.execute(GetIndexMappingsAction.INSTANCE, new GetIndexMappingsRequest(concreteLogIndex), new ActionListener<>() {
Suggestion importance[1-10]: 4

__

Why: Defensive suggestion; the risk depends on the actual behavior of getNewIndexByCreationDate, which typically returns null rather than throwing. Provides some robustness but is a minor improvement.

Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human: 
Human:

</details></details></td><td align=center>Low

</td></tr><tr><td rowspan=1>Possible issue</td>
<td>



<details><summary>Sync index-updated flag on concurrent create</summary>

___


**When falling through on <code>ResourceAlreadyExistsException</code>, <br><code>IndexUtils.detectorIndexUpdated</code> may still be <code>false</code>, causing subsequent logic to <br>attempt a mapping update or duplicate work. Set the updated flag (or invoke the <br>update-mapping path) before calling <code>prepareDetectorIndexing()</code> to mirror the success <br>path behavior.**

[src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java [1213-1224]](https://github.com/opensearch-project/security-analytics/pull/1792/files#diff-8fa8978eef6244554fff1512eb4ed63755d995cf3e8aa295522657a4eecb6166R1213-R1224)

```diff
 @Override
 public void onFailure(Exception e) {
     if (ExceptionsHelper.unwrapCause(e) instanceof ResourceAlreadyExistsException) {
         // Another concurrent detector create already created the index — proceed
         try {
+            IndexUtils.detectorIndexUpdated();
             prepareDetectorIndexing();
         } catch (Exception ex) {
             onFailures(ex);
         }
     } else {
         onFailures(e);
     }
 }
Suggestion importance[1-10]: 5

__

Why: Points out a possible state inconsistency after concurrent create, but the exact call IndexUtils.detectorIndexUpdated() in the improved code is suspect (it's referenced elsewhere as a boolean field). The concern is valid but the fix may need refinement.

Low

Previous suggestions

Suggestions up to commit c19bddd
CategorySuggestion                                                                                                                                    Impact
General
Only pass-through wildcard patterns when empty

Passing an unresolved index name through when no backing indices exist can cause
downstream failures (e.g., GetMappings returning empty and NPEs) that are hard to
diagnose. Consider still failing fast for concrete/alias names with no backing
indices, and only pass-through when indexName is an index pattern (contains
wildcards) or a datastream template that legitimately may be empty.

src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java [610-613]

 @Override
 public void onResponse(GetIndexResponse getIndexResponse) {
     String[] indices = getIndexResponse.indices();
     if (indices.length == 0) {
-        // Pattern has no backing indices yet — pass the name through as-is
-        actionListener.onResponse(indexName);
+        if (indexName.contains("*")) {
+            // Pattern has no backing indices yet — pass the name through as-is
+            actionListener.onResponse(indexName);
+        } else {
+            actionListener.onFailure(SecurityAnalyticsException.wrap(
+                    new IllegalArgumentException("Invalid index name: [" + indexName + "]")));
+        }
     } else if (indices.length == 1) {
Suggestion importance[1-10]: 6

__

Why: Valid concern: passing an unresolved concrete index name through can cause obscure downstream failures. Restricting pass-through to wildcard patterns preserves the previous error semantics for concrete missing indices while still supporting the empty-pattern use case.

Low
Preserve 404 for missing concrete indices

Removing the IndexNotFoundException branch means users creating a detector with a
truly non-existent index will get a generic 500 error rather than a clear 404. Since
LENIENT_EXPAND_OPEN won't throw IndexNotFoundException for wildcards/aliases but
still can for concrete missing indices in some code paths, retain a specific handler
to return RestStatus.NOT_FOUND for clarity.

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java [261-267]

 if (e instanceof OpenSearchStatusException) {
     listener.onFailure(SecurityAnalyticsException.wrap(
             new OpenSearchStatusException(String.format(Locale.getDefault(), "User doesn't have read permissions for one or more configured index %s", (Object) detectorIndices), RestStatus.FORBIDDEN)
+    ));
+} else if (ExceptionsHelper.unwrapCause(e) instanceof org.opensearch.index.IndexNotFoundException) {
+    listener.onFailure(SecurityAnalyticsException.wrap(
+        new OpenSearchStatusException(String.format(Locale.getDefault(), "Indices not found %s", String.join(", ", detectorIndices)), RestStatus.NOT_FOUND)
     ));
 } else {
     listener.onFailure(SecurityAnalyticsException.wrap(e));
 }
Suggestion importance[1-10]: 6

__

Why: Restoring the IndexNotFoundException handler preserves clearer 404 responses for missing concrete indices, improving API UX and error diagnostics that were lost in the PR.

Low
Refresh mappings on concurrent index creation

When the index already exists due to a concurrent create, mappings may not yet
reflect the latest schema. Consider invoking the same update-mapping path used in
the IndexUtils.detectorIndexUpdated == false branch before calling
prepareDetectorIndexing(), to avoid running against a stale mapping.

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java [1213-1224]

 @Override
 public void onFailure(Exception e) {
     if (ExceptionsHelper.unwrapCause(e) instanceof ResourceAlreadyExistsException) {
-        // Another concurrent detector create already created the index — proceed
+        // Another concurrent detector create already created the index — ensure mapping is up to date
         try {
-            prepareDetectorIndexing();
+            IndexUtils.detectorIndexUpdated = false;
+            onCreateMappingsResponse(new CreateIndexResponse(true, true, Detector.DETECTORS_INDEX));
         } catch (Exception ex) {
             onFailures(ex);
         }
     } else {
         onFailures(e);
     }
 }
Suggestion importance[1-10]: 4

__

Why: The concern about stale mappings is reasonable, but the proposed onCreateMappingsResponse call is speculative and may not exist or match the flow; the suggestion's applicability isn't fully verifiable from the diff.

Low
Suggestions up to commit 18ecb04
CategorySuggestion                                                                                                                                    Impact
General
Avoid returning unresolved index name

Silently returning the unresolved indexName when no backing indices exist can
propagate an unresolvable name downstream to mapping calls, resulting in confusing
failures later. Consider preserving the previous failure behavior (or at least
explicitly handling the "empty pattern" case with a clearer error) unless downstream
code is verified to accept unresolved names via LENIENT_EXPAND_OPEN.

src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java [610-613]

 @Override
 public void onResponse(GetIndexResponse getIndexResponse) {
     String[] indices = getIndexResponse.indices();
     if (indices.length == 0) {
-        // Pattern has no backing indices yet — pass the name through as-is
-        actionListener.onResponse(indexName);
+        actionListener.onFailure(
+                SecurityAnalyticsException.wrap(
+                        new IllegalArgumentException("Invalid index name: [" + indexName + "]")
+                )
+        );
     } else if (indices.length == 1) {
Suggestion importance[1-10]: 5

__

Why: The PR intentionally changed the behavior to pass through the unresolved name for empty patterns; reverting to the previous failure contradicts the PR's intent. The concern about downstream failures has some merit but the suggestion's fix undoes the change.

Low
Possible issue
Guard against resolution exceptions

IndexUtils.getNewIndexByCreationDate may throw if the index/alias/pattern cannot be
resolved (e.g., empty datastream), which would bypass the fallback assignment and
fail the whole detector creation. Wrap the call in try/catch and fall back to the
original logIndex to be consistent with the LENIENT_EXPAND_OPEN handling.

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java [1631-1637]

-String resolvedLogIndex = IndexUtils.getNewIndexByCreationDate(
-        clusterService.state(),
-        indexNameExpressionResolver,
-        logIndex
-);
+String resolvedLogIndex;
+try {
+    resolvedLogIndex = IndexUtils.getNewIndexByCreationDate(
+            clusterService.state(),
+            indexNameExpressionResolver,
+            logIndex
+    );
+} catch (Exception ex) {
+    resolvedLogIndex = null;
+}
 final String concreteLogIndex = resolvedLogIndex != null ? resolvedLogIndex : logIndex;
 client.execute(GetIndexMappingsAction.INSTANCE, new GetIndexMappingsRequest(concreteLogIndex), new ActionListener<>() {
     @Override
     public void onResponse(GetIndexMappingsResponse getMappingsViewResponse) {
         try {
             List<Pair<String, String>> aliasPathPairs;
 
             MappingMetadata mappingMetadata = getMappingsViewResponse.getMappings().get(concreteLogIndex);
Suggestion importance[1-10]: 5

__

Why: Adding a try/catch around getNewIndexByCreationDate provides defensive handling to ensure the fallback works in edge cases. The impact depends on the actual exception behavior of that utility.

Low
Handle resolution exception with fallback

Same as above: getNewIndexByCreationDate can throw on unresolvable inputs
(aliases/datastreams with no backing indices, non-existent patterns). Wrap in
try/catch so the fallback to indices.get(0) actually takes effect instead of failing
bucket-level monitor creation.

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java [1014-1019]

-String resolvedIndex = IndexUtils.getNewIndexByCreationDate(
-        clusterService.state(),
-        indexNameExpressionResolver,
-        indices.get(0) // taking first one is fine because we expect that all indices in list share same mappings
-);
+String resolvedIndex;
+try {
+    resolvedIndex = IndexUtils.getNewIndexByCreationDate(
+            clusterService.state(),
+            indexNameExpressionResolver,
+            indices.get(0) // taking first one is fine because we expect that all indices in list share same mappings
+    );
+} catch (Exception ex) {
+    resolvedIndex = null;
+}
 final String concreteIndex = resolvedIndex != null ? resolvedIndex : indices.get(0);
Suggestion importance[1-10]: 5

__

Why: Similar defensive handling for the bucket-level monitor path. Reasonable improvement if the utility can throw, but marginal without knowing exact throw behavior.

Low
Suggestions up to commit 17c5b60
CategorySuggestion                                                                                                                                    Impact
Possible issue
Guard against null mapping lookup

getMappingsViewResponse.getMappings().get(concreteLogIndex) may return null if the
resolved concrete index name doesn't exactly match the key returned by the mappings
API (e.g., due to case or resolution differences), causing a NullPointerException in
getAllAliasPathPairs. Guard against null or iterate the map's entries instead of
doing a keyed lookup.

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java [1657]

-aliasPathPairs = MapperUtils.getAllAliasPathPairs(getMappingsViewResponse.getMappings().get(concreteLogIndex));
+MappingMetadata mappingMetadata = getMappingsViewResponse.getMappings().get(concreteLogIndex);
+if (mappingMetadata == null && !getMappingsViewResponse.getMappings().isEmpty()) {
+    mappingMetadata = getMappingsViewResponse.getMappings().values().iterator().next();
+}
+if (mappingMetadata == null) {
+    listener.onFailure(new OpenSearchStatusException("No mappings found for index [" + concreteLogIndex + "]", RestStatus.NOT_FOUND));
+    return;
+}
+aliasPathPairs = MapperUtils.getAllAliasPathPairs(mappingMetadata);
Suggestion importance[1-10]: 7

__

Why: Legitimate NPE risk when the resolved concrete index name doesn't match the mappings response key exactly (e.g., datastream/alias resolution). Adding a null guard improves robustness.

Medium
General
Avoid masking truly invalid index names

Silently passing the unresolved indexName through when no backing indices exist can
mask genuinely invalid index names and cause downstream failures with confusing
errors. Consider only passing through when the name is clearly a pattern (contains
wildcards) or a datastream/alias, and otherwise failing with the original
IllegalArgumentException.

src/main/java/org/opensearch/securityanalytics/mapper/MapperService.java [610-613]

 String[] indices = getIndexResponse.indices();
 if (indices.length == 0) {
-    // Pattern has no backing indices yet — pass the name through as-is
-    actionListener.onResponse(indexName);
+    if (indexName.contains("*") || indexName.contains(",")) {
+        actionListener.onResponse(indexName);
+    } else {
+        actionListener.onFailure(
+                SecurityAnalyticsException.wrap(
+                        new IllegalArgumentException("Invalid index name: [" + indexName + "]")
+                )
+        );
+    }
 } else if (indices.length == 1) {
Suggestion importance[1-10]: 6

__

Why: Valid concern: silently returning indexName when there are zero indices could hide genuine invalid-name errors. The proposed heuristic (wildcards/commas) is reasonable, though the PR's intent seems to be supporting patterns that legitimately have no backing indices yet.

Low
Ensure mapping update on concurrent create

On concurrent creation, the detector index may exist but its mapping may not yet be
marked as updated (IndexUtils.detectorIndexUpdated is false). Calling
prepareDetectorIndexing() directly skips the mapping update path performed in the
sibling else if (!IndexUtils.detectorIndexUpdated) branch and can leave the index
with stale mappings. Route through the same update-mapping flow instead.

src/main/java/org/opensearch/securityanalytics/transport/TransportIndexDetectorAction.java [1213-1224]

 @Override
 public void onFailure(Exception e) {
     if (ExceptionsHelper.unwrapCause(e) instanceof ResourceAlreadyExistsException) {
-        // Another concurrent detector create already created the index — proceed
+        // Another concurrent detector create already created the index — ensure mappings are up-to-date, then proceed
         try {
-            prepareDetectorIndexing();
+            IndexUtils.detectorIndexUpdated = false;
+            onCreateMappingsResponse(response);
         } catch (Exception ex) {
             onFailures(ex);
         }
     } else {
         onFailures(e);
     }
 }
Suggestion importance[1-10]: 6

__

Why: Valid concern about skipping the mapping-update flow when the index was concurrently created; the direct call to prepareDetectorIndexing() may bypass necessary mapping updates. However, the exact fix depends on internal method semantics.

Low

@NelTdS
NelTdS force-pushed the fix/detector-creation-index-patterns branch from 17c5b60 to 18ecb04 Compare August 20, 2026 16:28
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 18ecb04

NelTdS added 2 commits August 20, 2026 18:34
@NelTdS
Fix detector creation with aliases and data streams
3d5a453 
Signed-off-by: Nelson Tavares de Sousa <nelson.tavares.de.sousa@sap.com>
@NelTdS
Added null check for non-existent index
1ee8ec4 
Signed-off-by: Nelson Tavares de Sousa <nelson.tavares.de.sousa@sap.com>
@NelTdS
NelTdS force-pushed the fix/detector-creation-index-patterns branch from 18ecb04 to 1ee8ec4 Compare August 20, 2026 16:34
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit c19bddd

1 similar comment
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit c19bddd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amsiglan amsiglan Awaiting requested review from amsiglan amsiglan is a code owner
@AWSHurneyt AWSHurneyt Awaiting requested review from AWSHurneyt AWSHurneyt is a code owner
@getsaurabh02 getsaurabh02 Awaiting requested review from getsaurabh02 getsaurabh02 is a code owner
@lezzago lezzago Awaiting requested review from lezzago lezzago is a code owner
@eirsep eirsep Awaiting requested review from eirsep eirsep is a code owner
@engechas engechas Awaiting requested review from engechas engechas is a code owner
@goyamegh goyamegh Awaiting requested review from goyamegh goyamegh is a code owner
@riysaxen-amzn riysaxen-amzn Awaiting requested review from riysaxen-amzn riysaxen-amzn is a code owner
@toepkerd toepkerd Awaiting requested review from toepkerd toepkerd is a code owner
@vikhy-aws vikhy-aws Awaiting requested review from vikhy-aws vikhy-aws is a code owner
@manaswini1920 manaswini1920 Awaiting requested review from manaswini1920 manaswini1920 is a code owner
@nishtham-amazon nishtham-amazon Awaiting requested review from nishtham-amazon nishtham-amazon is a code owner
@KashKondaka KashKondaka Awaiting requested review from KashKondaka KashKondaka is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@NelTdS