Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

separate doc-level monitor query indices created by detectors #1324

Merged
merged 4 commits into from
Sep 26, 2024
Merged
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 @@ -508,7 +508,8 @@ public List<Setting<?>> getSettings() {
SecurityAnalyticsSettings.THREAT_INTEL_TIMEOUT,
SecurityAnalyticsSettings.IOC_INDEX_RETENTION_PERIOD,
SecurityAnalyticsSettings.IOC_MAX_INDICES_PER_INDEX_PATTERN,
SecurityAnalyticsSettings.IOC_SCAN_MAX_TERMS_COUNT
SecurityAnalyticsSettings.IOC_SCAN_MAX_TERMS_COUNT,
SecurityAnalyticsSettings.ENABLE_DETECTORS_WITH_DEDICATED_QUERY_INDICES
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
package org.opensearch.securityanalytics.config.monitors;

import java.util.List;
import java.util.Random;
import java.util.UUID;
import java.util.stream.Collectors;
import org.opensearch.common.inject.Inject;
import org.opensearch.securityanalytics.logtype.LogTypeService;
Expand All @@ -25,6 +27,10 @@ public static String getRuleIndex(String logType) {
return String.format(Locale.getDefault(), ".opensearch-sap-%s-detectors-queries", logType);
}

public static String getRuleIndexOptimized(String logType) {
return String.format(Locale.getDefault(), ".opensearch-sap-%s-detectors-queries-optimized-%s", logType, UUID.randomUUID());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are moving from index patttern to index?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no. this method is utilized by few integ tests & hence the change.

}

public static String getAlertsIndex(String logType) {
return String.format(Locale.getDefault(), ".opensearch-sap-%s-alerts", logType);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -249,4 +249,10 @@ public static final List<Setting<?>> settings() {
Setting.Property.NodeScope, Setting.Property.Dynamic
);

public static final Setting<Boolean> ENABLE_DETECTORS_WITH_DEDICATED_QUERY_INDICES = Setting.boolSetting(
"plugins.security_analytics.enable_detectors_with_dedicated_query_indices",
false,
Setting.Property.NodeScope, Setting.Property.Dynamic
);

}
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ private Monitor buildThreatIntelMonitor(IndexThreatIntelMonitorRequest request)
triggers,
Collections.emptyMap(),
new DataSources(),
false,
PLUGIN_OWNER_FIELD
);
} catch (Exception e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,8 @@ public class TransportIndexDetectorAction extends HandledTransportAction<IndexDe

private volatile Boolean enabledWorkflowUsage;

private volatile Boolean enableDetectorWithDedicatedQueryIndices;

private final Settings settings;

private final NamedWriteableRegistry namedWriteableRegistry;
Expand Down Expand Up @@ -202,11 +204,13 @@ public TransportIndexDetectorAction(TransportService transportService,
this.indexTimeout = SecurityAnalyticsSettings.INDEX_TIMEOUT.get(this.settings);
this.filterByEnabled = SecurityAnalyticsSettings.FILTER_BY_BACKEND_ROLES.get(this.settings);
this.enabledWorkflowUsage = SecurityAnalyticsSettings.ENABLE_WORKFLOW_USAGE.get(this.settings);
this.enableDetectorWithDedicatedQueryIndices = SecurityAnalyticsSettings.ENABLE_DETECTORS_WITH_DEDICATED_QUERY_INDICES.get(this.settings);
this.monitorService = new MonitorService(client);
this.workflowService = new WorkflowService(client, monitorService);

this.clusterService.getClusterSettings().addSettingsUpdateConsumer(SecurityAnalyticsSettings.FILTER_BY_BACKEND_ROLES, this::setFilterByEnabled);
this.clusterService.getClusterSettings().addSettingsUpdateConsumer(SecurityAnalyticsSettings.ENABLE_WORKFLOW_USAGE, this::setEnabledWorkflowUsage);
this.clusterService.getClusterSettings().addSettingsUpdateConsumer(SecurityAnalyticsSettings.ENABLE_DETECTORS_WITH_DEDICATED_QUERY_INDICES, this::setEnabledDetectorsWithDedicatedQueryIndices);
this.exceptionChecker = exceptionChecker;
}

Expand Down Expand Up @@ -793,7 +797,7 @@ private IndexMonitorRequest createDocLevelMonitorRequest(List<Pair<String, Rule>
detector.getAlertsHistoryIndex(),
detector.getAlertsHistoryIndexPattern(),
DetectorMonitorConfig.getRuleIndexMappingsByType(),
true), PLUGIN_OWNER_FIELD);
true), enableDetectorWithDedicatedQueryIndices, PLUGIN_OWNER_FIELD);

return new IndexMonitorRequest(monitorId, SequenceNumbers.UNASSIGNED_SEQ_NO, SequenceNumbers.UNASSIGNED_PRIMARY_TERM, refreshPolicy, restMethod, monitor, null);
}
Expand Down Expand Up @@ -887,14 +891,14 @@ private IndexMonitorRequest createDocLevelMonitorMatchAllRequest(

Monitor monitor = new Monitor(monitorId, Monitor.NO_VERSION, monitorName, false, detector.getSchedule(), detector.getLastUpdateTime(), null,
Monitor.MonitorType.DOC_LEVEL_MONITOR.getValue(), detector.getUser(), 1, docLevelMonitorInputs, triggers, Map.of(),
new DataSources(detector.getRuleIndex(),
new DataSources(enableDetectorWithDedicatedQueryIndices? detector.getRuleIndex() + "_chained_findings": detector.getRuleIndex(),
detector.getFindingsIndex(),
detector.getFindingsIndexPattern(),
detector.getAlertsIndex(),
detector.getAlertsHistoryIndex(),
detector.getAlertsHistoryIndexPattern(),
DetectorMonitorConfig.getRuleIndexMappingsByType(),
true), PLUGIN_OWNER_FIELD);
true), enableDetectorWithDedicatedQueryIndices, PLUGIN_OWNER_FIELD);

return new IndexMonitorRequest(monitorId, SequenceNumbers.UNASSIGNED_SEQ_NO, SequenceNumbers.UNASSIGNED_PRIMARY_TERM, refreshPolicy, restMethod, monitor, null);
}
Expand Down Expand Up @@ -1068,7 +1072,7 @@ public void onResponse(GetIndexMappingsResponse getIndexMappingsResponse) {
detector.getAlertsHistoryIndex(),
detector.getAlertsHistoryIndexPattern(),
DetectorMonitorConfig.getRuleIndexMappingsByType(),
true), PLUGIN_OWNER_FIELD);
true), false, PLUGIN_OWNER_FIELD);

listener.onResponse(new IndexMonitorRequest(monitorId, SequenceNumbers.UNASSIGNED_SEQ_NO, SequenceNumbers.UNASSIGNED_PRIMARY_TERM, refreshPolicy, restMethod, monitor, null));
}
Expand Down Expand Up @@ -1252,7 +1256,13 @@ void createDetector() {
request.getDetector().setAlertsHistoryIndexPattern(DetectorMonitorConfig.getAlertsHistoryIndexPattern(ruleTopic));
request.getDetector().setFindingsIndex(DetectorMonitorConfig.getFindingsIndex(ruleTopic));
request.getDetector().setFindingsIndexPattern(DetectorMonitorConfig.getFindingsIndexPattern(ruleTopic));
request.getDetector().setRuleIndex(DetectorMonitorConfig.getRuleIndex(ruleTopic));

if (enableDetectorWithDedicatedQueryIndices) {
// disabling the setting after enabling it will mean delete & re-create the detector
request.getDetector().setRuleIndex(DetectorMonitorConfig.getRuleIndexOptimized(ruleTopic));
} else {
request.getDetector().setRuleIndex(DetectorMonitorConfig.getRuleIndex(ruleTopic));
}

User originalContextUser = this.user;
log.debug("user from original context is {}", originalContextUser);
Expand Down Expand Up @@ -1369,7 +1379,16 @@ void onGetResponse(Detector currentDetector, User user) {
request.getDetector().setAlertsHistoryIndexPattern(DetectorMonitorConfig.getAlertsHistoryIndexPattern(ruleTopic));
request.getDetector().setFindingsIndex(DetectorMonitorConfig.getFindingsIndex(ruleTopic));
request.getDetector().setFindingsIndexPattern(DetectorMonitorConfig.getFindingsIndexPattern(ruleTopic));
request.getDetector().setRuleIndex(DetectorMonitorConfig.getRuleIndex(ruleTopic));
if (currentDetector.getRuleIndex().contains("optimized")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if name contains optimized shouldn't we set rule index name as getRuleIndexOptimized value?

is this if else condition flipped?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the if-else condition checks if the detector is already using a dedicated query index, then keep on using it. else, switch from shared query index to new dedicated query index as the detector is updated.

request.getDetector().setRuleIndex(currentDetector.getRuleIndex());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plz add comment that if we turn OFF setting after turning on, updating detector will not change from optimized to normal and we need to re-create detector

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a comment. added a test too.

} else {
if (enableDetectorWithDedicatedQueryIndices) {
// disabling the setting after enabling it will mean delete & re-create the detector
request.getDetector().setRuleIndex(DetectorMonitorConfig.getRuleIndexOptimized(ruleTopic));
} else {
request.getDetector().setRuleIndex(DetectorMonitorConfig.getRuleIndex(ruleTopic));
}
}
request.getDetector().setUser(user);

if (!detector.getInputs().isEmpty()) {
Expand Down Expand Up @@ -1805,4 +1824,8 @@ private void setFilterByEnabled(boolean filterByEnabled) {
private void setEnabledWorkflowUsage(boolean enabledWorkflowUsage) {
this.enabledWorkflowUsage = enabledWorkflowUsage;
}

private void setEnabledDetectorsWithDedicatedQueryIndices(boolean enabledDetectorsWithDedicatedQueryIndices) {
this.enableDetectorWithDedicatedQueryIndices = enabledDetectorsWithDedicatedQueryIndices;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,9 @@ public static String ruleTopicIndexSettings() throws IOException {
public void initRuleTopicIndexTemplate(ActionListener<AcknowledgedResponse> actionListener) throws IOException {
getAllRuleIndices(ActionListener.wrap(allRuleIndices -> {
// Compose list of all patterns to cover all query indices
List<String> indexPatterns = new ArrayList<>();
for(String ruleIndex : allRuleIndices) {
indexPatterns.add(ruleIndex + "*");
}

ComposableIndexTemplate template = new ComposableIndexTemplate(
indexPatterns,
allRuleIndices,
new Template(
Settings.builder().loadFromSource(ruleTopicIndexSettings(), XContentType.JSON).build(),
null,
Expand Down Expand Up @@ -87,7 +83,8 @@ private void getAllRuleIndices(ActionListener<List<String>> listener) {
listener.onResponse(
logTypes
.stream()
.map(logType -> DetectorMonitorConfig.getRuleIndex(logType))
// use index pattern here to define rule topic index template for all query indices which match the pattern
.map(logType -> DetectorMonitorConfig.getRuleIndex(logType) + "*")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be detector specific?
why do we have this method? getAllRuleIndices

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a single index template containing specific settings for sigma rule executions are applied to all rule indices.

.collect(Collectors.toList())
);
}, listener::onFailure));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public void testCreateDetectorWithThreatIntelEnabled_updateDetectorWithThreatInt
" }\n" +
" }\n" +
"}";
SearchResponse response = executeSearchAndGetResponse(DetectorMonitorConfig.getRuleIndex(randomDetectorType()), request, true);
Copy link
Member

@eirsep eirsep Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why has this been removed ?

Plz dont reduce test coverage..
fix test as necessary

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

query indices are created & deleted now. So, it is difficult to assert number of queries in query index now.

SearchResponse response = executeSearchAndGetResponse(DetectorMonitorConfig.getRuleIndex(randomDetectorType()) + "*", request, true);


assertEquals(2, response.getHits().getTotalHits().value);
Expand Down Expand Up @@ -275,7 +275,7 @@ public void testCreateDetectorWithThreatIntelDisabled_updateDetectorWithThreatIn
" }\n" +
" }\n" +
"}";
SearchResponse response = executeSearchAndGetResponse(DetectorMonitorConfig.getRuleIndex(randomDetectorType()), request, true);
SearchResponse response = executeSearchAndGetResponse(DetectorMonitorConfig.getRuleIndex(randomDetectorType()) + "*", request, true);


assertEquals(1, response.getHits().getTotalHits().value);
Expand Down Expand Up @@ -372,7 +372,7 @@ public void testCreateDetectorWithThreatIntelEnabledAndNoRules_triggerDetectionT
" }\n" +
" }\n" +
"}";
SearchResponse response = executeSearchAndGetResponse(DetectorMonitorConfig.getRuleIndex(randomDetectorType()), request, true);
SearchResponse response = executeSearchAndGetResponse(DetectorMonitorConfig.getRuleIndex(randomDetectorType()) + "*", request, true);


assertEquals(1, response.getHits().getTotalHits().value);
Expand Down Expand Up @@ -466,7 +466,7 @@ public void testCreateDetectorWithThreatIntelEnabled_triggerDetectionTypeOnlyThr
" }\n" +
" }\n" +
"}";
SearchResponse response = executeSearchAndGetResponse(DetectorMonitorConfig.getRuleIndex(randomDetectorType()), request, true);
SearchResponse response = executeSearchAndGetResponse(DetectorMonitorConfig.getRuleIndex(randomDetectorType()) + "*", request, true);


assertEquals(1, response.getHits().getTotalHits().value);
Expand Down Expand Up @@ -561,7 +561,7 @@ public void testCreateDetectorWithThreatIntelEnabled_triggerWithBothDetectionTyp
" }\n" +
" }\n" +
"}";
SearchResponse response = executeSearchAndGetResponse(DetectorMonitorConfig.getRuleIndex(randomDetectorType()), request, true);
SearchResponse response = executeSearchAndGetResponse(DetectorMonitorConfig.getRuleIndex(randomDetectorType()) + "*", request, true);


assertEquals(1, response.getHits().getTotalHits().value);
Expand Down Expand Up @@ -653,7 +653,7 @@ public void testCreateDetectorWithThreatIntelDisabled_triggerWithThreatIntelDete
" }\n" +
" }\n" +
"}";
SearchResponse response = executeSearchAndGetResponse(DetectorMonitorConfig.getRuleIndex(randomDetectorType()), request, true);
SearchResponse response = executeSearchAndGetResponse(DetectorMonitorConfig.getRuleIndex(randomDetectorType()) + "*", request, true);


assertEquals(1, response.getHits().getTotalHits().value);
Expand Down Expand Up @@ -745,7 +745,7 @@ public void testCreateDetectorWithThreatIntelDisabled_triggerWithRulesDetectionT
" }\n" +
" }\n" +
"}";
SearchResponse response = executeSearchAndGetResponse(DetectorMonitorConfig.getRuleIndex(randomDetectorType()), request, true);
SearchResponse response = executeSearchAndGetResponse(DetectorMonitorConfig.getRuleIndex(randomDetectorType()) + "*", request, true);


assertEquals(1, response.getHits().getTotalHits().value);
Expand Down Expand Up @@ -802,4 +802,4 @@ public void testCreateDetectorWithThreatIntelDisabled_triggerWithRulesDetectionT
/** findings are present but alerts are NOT generated as detection type mentioned in trigger is threat_intel only but finding is from rules*/
Assert.assertEquals(3, getAlertsBody.get("total_alerts"));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,7 @@ void setDebugLogLevel() throws IOException, InterruptedException {


makeRequest(client(), "PUT", "_cluster/settings", Collections.emptyMap(), se, new BasicHeader("Content-Type", "application/json"));
updateClusterSetting("plugins.security_analytics.enable_detectors_with_dedicated_query_indices", "true");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are there tests with setting turned off

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a test for setting turned off.

}

protected final List<String> clusterPermissions = List.of(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ public void testGetAlerts_success() {
List.of(),
Map.of(),
new DataSources(),
true,
TransportIndexDetectorAction.PLUGIN_OWNER_FIELD
),
new DocumentLevelTrigger("trigger_id_1", "my_trigger", "severity_low", List.of(), new Script("")),
Expand Down Expand Up @@ -129,6 +130,7 @@ public void testGetAlerts_success() {
List.of(),
Map.of(),
new DataSources(),
true,
TransportIndexDetectorAction.PLUGIN_OWNER_FIELD
),
new DocumentLevelTrigger("trigger_id_1", "my_trigger", "severity_low", List.of(), new Script("")),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ public void testGetAlertsByStartTimeAndEndTimeSuccess() throws IOException, Inte
}

public void testGetAlerts_noDetector_failure() throws IOException {
// Call GetAlerts API
// Call GetAlerts API
Map<String, String> params = new HashMap<>();
params.put("detector_id", "nonexistent_detector_id");
try {
Expand Down Expand Up @@ -821,7 +821,7 @@ public void testMultipleAggregationAndDocRules_alertSuccess() throws IOException
Collections.emptyList());
Detector detector = randomDetectorWithInputsAndTriggers(List.of(input),
List.of(new DetectorTrigger("randomtrigegr", "test-trigger", "1", List.of(randomDetectorType()), List.of(), List.of(), List.of(), List.of(), List.of()))
);
);

Response createResponse = makeRequest(client(), "POST", SecurityAnalyticsPlugin.DETECTOR_BASE_URI, Collections.emptyMap(), toHttpEntity(detector));

Expand All @@ -832,7 +832,7 @@ public void testMultipleAggregationAndDocRules_alertSuccess() throws IOException
" }\n" +
" }\n" +
"}";
SearchResponse response = executeSearchAndGetResponse(DetectorMonitorConfig.getRuleIndex(randomDetectorType()), request, true);
SearchResponse response = executeSearchAndGetResponse(DetectorMonitorConfig.getRuleIndex(randomDetectorType()) + "*", request, true);

assertEquals(1, response.getHits().getTotalHits().value); // 5 for rules, 1 for match_all query in chained findings monitor

Expand Down Expand Up @@ -870,8 +870,8 @@ public void testMultipleAggregationAndDocRules_alertSuccess() throws IOException
}
}

assertEquals(1, numberOfMonitorTypes.get(Monitor.MonitorType.BUCKET_LEVEL_MONITOR.getValue()).intValue());
assertEquals(1, numberOfMonitorTypes.get(Monitor.MonitorType.DOC_LEVEL_MONITOR.getValue()).intValue());
assertEquals(1, numberOfMonitorTypes.get(Monitor.MonitorType.BUCKET_LEVEL_MONITOR.getValue()).intValue());
assertEquals(1, numberOfMonitorTypes.get(Monitor.MonitorType.DOC_LEVEL_MONITOR.getValue()).intValue());

Map<String, String> params = new HashMap<>();
params.put("detector_id", detectorId);
Expand All @@ -895,13 +895,13 @@ public void testMultipleAggregationAndDocRules_alertSuccess() throws IOException
List<Map<String, Object>> queries = (List<Map<String, Object>>) finding.get("queries");
Set<String> findingRuleIds = queries.stream().map(it -> it.get("id").toString()).collect(Collectors.toSet());

// In the case of bucket level monitors, queries will always contain one value
String aggRuleId = findingRuleIds.iterator().next();
List<String> findingDocs = (List<String>) finding.get("related_doc_ids");
// In the case of bucket level monitors, queries will always contain one value
String aggRuleId = findingRuleIds.iterator().next();
List<String> findingDocs = (List<String>) finding.get("related_doc_ids");

if (aggRuleId.equals(sumRuleId)) {
assertTrue(List.of("1", "2", "3", "4", "5", "6", "7").containsAll(findingDocs));
}
if (aggRuleId.equals(sumRuleId)) {
assertTrue(List.of("1", "2", "3", "4", "5", "6", "7").containsAll(findingDocs));
}
}

assertTrue(Arrays.asList("1", "2", "3", "4", "5", "6", "7", "8").containsAll(docLevelFinding));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1215,7 +1215,7 @@ public void testCreateDetectorWithNotCondition_verifyFindingsAndNoFindings_succe
" }\n" +
" }\n" +
"}";
SearchResponse response = executeSearchAndGetResponse(DetectorMonitorConfig.getRuleIndex(randomDetectorType()), request, true);
SearchResponse response = executeSearchAndGetResponse(DetectorMonitorConfig.getRuleIndex(randomDetectorType()) + "*", request, true);

assertEquals(1, response.getHits().getTotalHits().value);

Expand Down
Loading
Loading