add correlation rule layer for events-correlation-engine #7132

sbcd90 · 2023-04-12T21:08:46Z

Description

add correlation rule layer for events-correlation-engine

Issues Resolved

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2023-04-12T21:38:19Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/13959/
CommitID: c07e6cc

github-actions · 2023-04-12T21:42:00Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/13960/
CommitID: 147baaa

nknize

First round review. Let's get a solid foundation here and iterate quickly in followup PRs.

...relation-engine/src/main/java/org/opensearch/plugin/correlation/EventsCorrelationPlugin.java

gradle/missing-javadoc.gradle

...src/main/java/org/opensearch/plugin/correlation/rules/action/IndexCorrelationRuleAction.java

...ion-engine/src/main/java/org/opensearch/plugin/correlation/rules/model/CorrelationQuery.java

...tion-engine/src/main/java/org/opensearch/plugin/correlation/rules/model/CorrelationRule.java

...ion-engine/src/main/java/org/opensearch/plugin/correlation/rules/model/CorrelationQuery.java

...nts-correlation-engine/src/main/java/org/opensearch/plugin/correlation/utils/IndexUtils.java

CHANGELOG.md

github-actions · 2023-04-13T19:55:41Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/14010/
CommitID: ca63ad7
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-04-13T20:09:29Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/14011/
CommitID: 9f43a02
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

nknize

Next round cleanup comments. It's coming together. We need to add the explicit javadoc checker and I left some cleanup comments. I think we can simplify the CorrelationConstants approach and possibly just remove that class altogether? If the public facing API needs it we might figure a cleaner way of doing it.

nknize · 2023-04-14T14:52:03Z

...alClusterTest/java/org/opensearch/plugin/correlation/EventsCorrelationPluginTransportIT.java

+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+public class EventsCorrelationPluginTransportIT extends OpenSearchIntegTestCase {


Need javadocs here. all public classes will need javadocs.

added javadocs

gradle/missing-javadoc.gradle

nknize · 2023-04-14T15:02:39Z

...java/org/opensearch/plugin/correlation/rules/resthandler/RestIndexCorrelationRuleAction.java

+    @Override
+    public List<Route> routes() {
+        return List.of(
+            new Route(RestRequest.Method.POST, EventsCorrelationPlugin.CORRELATION_RULES_BASE_URI),


From the RFC it looks like users can only create rules through the global _correlation endpoint.

POST /_correlation/rules Request Body: { "correlate": [ { "index": "vpc_flow", "query": "dstaddr:4.5.6.7 or dstaddr:4.5.6.6" }, { "index": "windows", "query": "winlog.event_data.SubjectDomainName:NTAUTHORI*" }, { "index": "ad_logs", "query": "ResultType:50126" }, { "index": "app_logs", "query": "endpoint:/customer_records.txt" } ] }

Would it also make sense to enable users to do this on a per index basis?

POST /my_ndex/_correlation/rule/{$rule_id} where {$rule_id} is the (optional) existing rule

If so, we can do this in a follow up PR.

will address this in follow-up pr.

Maybe open an issue for this?

created the issue: #7184

nknize · 2023-04-14T15:05:27Z

...a/org/opensearch/plugin/correlation/rules/transport/TransportIndexCorrelationRuleAction.java

+
+        void start() {
+            try {
+                if (!correlationRuleIndices.correlationRuleIndexExists()) {


nit, avoid negative conditionals.

Change to be explicit:

Suggested change

if (!correlationRuleIndices.correlationRuleIndexExists()) {

if (correlationRuleIndices.correlationRuleIndexExists() == false) {

changed this.

...a/org/opensearch/plugin/correlation/rules/transport/TransportIndexCorrelationRuleAction.java

nknize · 2023-04-14T15:08:22Z

...gine/src/main/java/org/opensearch/plugin/correlation/settings/EventsCorrelationSettings.java

+/**
+ * Settings for events-correlation-engine.
+ *
+ * @opensearch.api


do you also want to mark as @opensearch.experimental for now? It's largely superficial but a nice flag if you think these Settings might change.

marked as @opensearch.experimental

nknize · 2023-04-14T15:17:49Z

...tion-engine/src/main/java/org/opensearch/plugin/correlation/rules/model/CorrelationRule.java

+     * @param correlationQueries list of correlation queries part of rule
+     */
+    public CorrelationRule(String id, Long version, String name, List<CorrelationQuery> correlationQueries) {
+        this.id = id != null ? id : NO_ID;


can we just have another ctor that doesn't take an ID as a parameter?

added a new ctor.

nknize · 2023-04-14T15:18:40Z

...tion-engine/src/main/java/org/opensearch/plugin/correlation/rules/model/CorrelationRule.java

+     */
+    public CorrelationRule(String id, Long version, String name, List<CorrelationQuery> correlationQueries) {
+        this.id = id != null ? id : NO_ID;
+        this.version = version != null ? version : NO_VERSION;


Same, just have a ctor that doesn't take a version?

I think we can remove the CorrelationConstants#NO_ID and CorrelationConstants#NO_VERSION altogether.

added a new ctor.
removed CorrelationConstants class.

nknize · 2023-04-14T19:24:14Z

Also make sure you rebase from main to pick up some of the BWC version check fixes.

sbcd90 · 2023-04-15T10:53:02Z

hi @nknize , addressed all the review comments. also rebased from main. please have a look.

github-actions · 2023-04-15T11:15:11Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.client.ReindexIT.testReindexTask

URL: https://build.ci.opensearch.org/job/gradle-check/14071/
CommitID: 54b53c5
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

nknize

Left a few minor nits. Otherwise LGTM!

nknize · 2023-04-15T17:16:01Z

plugins/events-correlation-engine/src/main/resources/mappings/correlation-rules.json

+          }
+        },
+        "timestampField": {
+          "type": "text",


Shouldn't this be a long or date type?

this is the timestamp field name which is a string.

...ion-engine/src/main/java/org/opensearch/plugin/correlation/utils/CorrelationRuleIndices.java

...a/org/opensearch/plugin/correlation/rules/transport/TransportIndexCorrelationRuleAction.java

nknize · 2023-04-15T18:54:29Z

...java/org/opensearch/plugin/correlation/rules/resthandler/RestIndexCorrelationRuleAction.java

+    @Override
+    public List<Route> routes() {
+        return List.of(
+            new Route(RestRequest.Method.POST, EventsCorrelationPlugin.CORRELATION_RULES_BASE_URI),


Maybe open an issue for this?

...tion-engine/src/main/java/org/opensearch/plugin/correlation/rules/model/CorrelationRule.java

Signed-off-by: Subhobrata Dey <[email protected]>

sbcd90 · 2023-04-17T04:07:15Z

hi @nknize , thanks a lot for reviewing the pr. created a issue for follow-up on the comment above: #7184

github-actions · 2023-04-17T04:29:49Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/14098/
CommitID: 1b3b15b

nknize

LGTM

andrross · 2023-04-17T14:49:29Z

CHANGELOG.md

@@ -8,6 +8,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Support for HTTP/2 (server-side) ([#3847](https://github.com/opensearch-project/OpenSearch/pull/3847))
 - Add getter for path field in NestedQueryBuilder ([#4636](https://github.com/opensearch-project/OpenSearch/pull/4636))
 - Allow mmap to use new JDK-19 preview APIs in Apache Lucene 9.4+ ([#5151](https://github.com/opensearch-project/OpenSearch/pull/5151))
+- Add events correlation engine plugin ([#6854](https://github.com/opensearch-project/OpenSearch/issues/6854))


We'll need to move this to the 2.x section assuming this is to be backported

andrross · 2023-04-17T14:51:57Z

...alClusterTest/java/org/opensearch/plugin/correlation/EventsCorrelationPluginTransportIT.java

+            .collect(Collectors.toList());
+        Assert.assertTrue(
+            pluginInfos.stream()


nitpick...no need to collect to a list here when you just use it as a stream again

andrross · 2023-04-17T16:19:38Z

libs/core/src/main/java/org/opensearch/core/ParseField.java

@@ -204,5 +204,6 @@ public static class CommonFields {
        public static final ParseField FORMAT = new ParseField("format");
        public static final ParseField MISSING = new ParseField("missing");
        public static final ParseField TIME_ZONE = new ParseField("time_zone");
+        public static final ParseField _META = new ParseField("_meta");


It's not really a "common field throughout the project" if it is only used in one place. If you're sure this will be used any multiple places this is fine, otherwise it might be simpler to keep it defined where it is used.

Also, the leading underscore (in the Java constant name, not the text) technically violates the standard convention for constant names.

Maybe add a comment too unless you think the meaning of a "meta" field is self-evident.

Agree, this could me moved to scope local once we start using it.

andrross · 2023-04-17T16:23:18Z

...rc/main/java/org/opensearch/plugin/correlation/rules/action/IndexCorrelationRuleRequest.java

+    private String correlationRuleId;
+
+    private CorrelationRule correlationRule;
+
+    private RestRequest.Method method;


Can these be final?

andrross · 2023-04-17T16:23:35Z

...c/main/java/org/opensearch/plugin/correlation/rules/action/IndexCorrelationRuleResponse.java

+    private String id;
+
+    private Long version;
+
+    private RestStatus status;
+
+    private CorrelationRule correlationRule;


Can these be final?

andrross · 2023-04-17T16:28:27Z

...rc/main/java/org/opensearch/plugin/correlation/rules/action/IndexCorrelationRuleRequest.java

+     */
+    public IndexCorrelationRuleRequest(CorrelationRule correlationRule, RestRequest.Method method) {
+        super();
+        this.correlationRuleId = "";


Is it really okay for one of these instances to exist without a rule id? The meaning of a magic empty string is usually not clear. If this is really an optional field, it is better to store it as null and codify the behavior in the getter type by returning an Optional so that the caller has to explicitly deal with the set/not set cases.

reta · 2023-04-17T17:58:21Z

@nknize shouldn't the plugin go to sandbox?

dblock · 2023-04-17T20:29:51Z

What’s the plan for getting this into 2.x?

@timestamp

…project#7132) Adds new correlation engine feature by way of a new `:plugins:events-correlation-engine` plugin. The endpoint is /_correlation and users can create new rules using the following example DSL: { "name": "s3 to app logs", "correlate": [ { "index": "s3_access_logs", "query": "aws.cloudtrail.eventName:ReplicateObject", "timestampField": "@timestamp", "tags": [ "s3" ] } ] } Signed-off-by: Subhobrata Dey <[email protected]>

sbcd90 requested review from reta, anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, gbbafna, setiah, kartg, kotwanikunal, mch2, nknize, owaiskazi19, Rishikesh1159, ryanbogan, saratvemulapalli, shwetathareja, dreamer-89, tlfeng, VachaShah and xuezhou25 as code owners April 12, 2023 21:08

sbcd90 force-pushed the corr2 branch from c07e6cc to 147baaa Compare April 12, 2023 21:10

sbcd90 mentioned this pull request Apr 12, 2023

[META] OpenSearch Events Correlation Engine #6854

Open

8 tasks

nknize requested changes Apr 12, 2023

View reviewed changes

andrross reviewed Apr 12, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

sbcd90 force-pushed the corr2 branch 2 times, most recently from ca63ad7 to 9f43a02 Compare April 13, 2023 19:39

nknize requested changes Apr 14, 2023

View reviewed changes

sbcd90 force-pushed the corr2 branch from 9f43a02 to 54b53c5 Compare April 15, 2023 10:46

nknize approved these changes Apr 15, 2023

View reviewed changes

sbcd90 added 2 commits April 17, 2023 03:51

add correlation rule layer for events-correlation-engine

2c38caa

Signed-off-by: Subhobrata Dey <[email protected]>

add correlation rule layer for events-correlation-engine

1b3b15b

Signed-off-by: Subhobrata Dey <[email protected]>

sbcd90 force-pushed the corr2 branch from 54b53c5 to 1b3b15b Compare April 17, 2023 04:01

nknize approved these changes Apr 17, 2023

View reviewed changes

nknize merged commit 2f5bf5b into opensearch-project:main Apr 17, 2023

andrross reviewed Apr 17, 2023

View reviewed changes

sbcd90 mentioned this pull request Jun 27, 2023

[RFC] OpenSearch Events Correlation Engine #6779

Open

	if (!correlationRuleIndices.correlationRuleIndexExists()) {
	if (correlationRuleIndices.correlationRuleIndexExists() == false) {

add correlation rule layer for events-correlation-engine #7132

add correlation rule layer for events-correlation-engine #7132

Conversation

sbcd90 commented Apr 12, 2023

Description

Issues Resolved

Check List

github-actions bot commented Apr 12, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Apr 12, 2023

Gradle Check (Jenkins) Run Completed with:

nknize left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 13, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Apr 13, 2023

Gradle Check (Jenkins) Run Completed with:

nknize left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nknize commented Apr 14, 2023

sbcd90 commented Apr 15, 2023

github-actions bot commented Apr 15, 2023

Gradle Check (Jenkins) Run Completed with:

nknize left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbcd90 commented Apr 17, 2023 • edited Loading

github-actions bot commented Apr 17, 2023

Gradle Check (Jenkins) Run Completed with:

nknize left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reta commented Apr 17, 2023

dblock commented Apr 17, 2023

sbcd90 commented Apr 17, 2023 •

edited

Loading