[ML] add auditor to data frame plugin #40012

benwtrent · 2019-03-13T20:10:16Z

This adds a DataFrameAuditor, a index for audit messages, and an audit messages class.

I took the basic functionality found in the ML Auditor, and created two new abstract classes in
org.elasticsearch.xpack.core.common.notifications for consolidating what would be commonalities between any plugin using an Auditor-like system.

I opted to simply copy the Level class found in the ml notifications package. I thought it best do any refactoring for ML in a different PR.

As for what to audit and when, that question is still up in the air, but at least after this PR that functionality will be available.

elasticmachine · 2019-03-13T20:10:17Z

Pinging @elastic/ml-core

hendrikmuhs

added some comments

I think we should discuss this offline.

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/common/notifications/Level.java

...re/src/main/java/org/elasticsearch/xpack/core/common/notifications/AbstractAuditMessage.java

hendrikmuhs · 2019-03-14T08:24:24Z

...rame/src/main/java/org/elasticsearch/xpack/dataframe/persistence/DataFrameInternalIndex.java

@@ -27,11 +30,16 @@
    public static final String INDEX_TEMPLATE_PATTERN = ".data-frame-internal-";
    public static final String INDEX_TEMPLATE_NAME = INDEX_TEMPLATE_PATTERN + INDEX_TEMPLATE_VERSION;
    public static final String INDEX_NAME = INDEX_TEMPLATE_NAME;
+    public static final String AUDIT_INDEX = ".data-frame-notifications";


although the chance is low, I would version it

...rame/src/main/java/org/elasticsearch/xpack/dataframe/persistence/DataFrameInternalIndex.java

dimitris-athanasiou

Left a few comments. Are we going to implement this for anomaly detection?

dimitris-athanasiou · 2019-03-14T08:25:23Z

...in/core/src/main/java/org/elasticsearch/xpack/core/common/notifications/AbstractAuditor.java

+import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
+import static org.elasticsearch.xpack.core.ClientHelper.executeAsyncWithOrigin;
+
+public abstract class AbstractAuditor<T extends AbstractAuditMessage> {


I wonder if this needs to be abstact. We could pass executionOrigin and auditIndex in the constructor. And we could simply assume all implementation would simply log-trace success/failure, or at least those 2 could have a default implementation which most implementors would not need override.

@dimitris-athanasiou I waffled back and forth while writing this on it being abstract or not. Comes down to inheritance vs injection. I am fine with simply passing the appropriate strings if that is preferred. Especially since nothing really interacts with the AbstractAuditor type by-itself.

dimitris-athanasiou · 2019-03-14T08:26:50Z

...re/src/main/java/org/elasticsearch/xpack/core/common/notifications/AbstractAuditMessage.java

+
+    protected abstract String getResourceField();
+
+    public static final class AuditMessageBuilder<T extends AbstractAuditMessage> {


Rename to Builder?

For sure, since it is already nested in that class.

dimitris-athanasiou · 2019-03-14T08:36:39Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/common/notifications/Level.java

+import java.util.Locale;
+
+public enum Level implements Writeable {
+    INFO, ACTIVITY, WARNING, ERROR;


Where would we use activity instead of info?

It's true that we don't use this at all in the current product. In the old product where we did use it it is interesting to see who added it:

f52b8ead288 (Dimitrios Athanasiou 2016-05-05 13:36:31 +0100 101) auditor.activity(buildUsageMessage(activeJobs));

In other words, the usage information similar to what the X-Pack usage endpoint returns and is now stored by monitoring used to be stored in the notifications index with type activity.

But assuming I'm correct that we don't use it any more (please double check @benwtrent) now would be a good time to remove it.

One more thing about removing activity - usually this would be a BWC breaking change. But in this case I don't think we ever serialise Level or AuditMessage on the wire, so we can do it. This leads to the question, should we even support serialising Level or AuditMessage on the wire? If you remove wire serialisation from Level and AuditMessage and their tests does the code still compile?

@droberts195 @dimitris-athanasiou I tested on ML by removing serialization and ACTIVITY and there are no calls to serialization for AuditMessage and therefore no calls to Level serialization.

dimitris-athanasiou · 2019-03-14T08:38:08Z

...re/src/main/java/org/elasticsearch/xpack/core/common/notifications/AbstractAuditMessage.java

+    }
+
+    AbstractAuditMessage(String resourceId, String message, Level level, String nodeName) {
+        this.resourceId = resourceId;


Add Objects.requireNotNull to those that need it. Probably all?

...rame/src/main/java/org/elasticsearch/xpack/dataframe/persistence/DataFrameInternalIndex.java

droberts195 · 2019-03-14T09:24:04Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/common/notifications/Level.java

+import java.util.Locale;
+
+public enum Level implements Writeable {
+    INFO, ACTIVITY, WARNING, ERROR;


One more thing about removing activity - usually this would be a BWC breaking change. But in this case I don't think we ever serialise Level or AuditMessage on the wire, so we can do it. This leads to the question, should we even support serialising Level or AuditMessage on the wire? If you remove wire serialisation from Level and AuditMessage and their tests does the code still compile?

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/common/notifications/Level.java

dimitris-athanasiou · 2019-03-14T10:14:39Z

@droberts195 I think we do need to serialize it. If say, a master node action issues the request to index a notification, it will have to travel to a data node to be written in the index.

droberts195 · 2019-03-14T10:26:17Z

I think we do need to serialize it. If say, a master node action issues the request to index a notification, it will have to travel to a data node to be written in the index

Yes, but in that scenario it's the source of the index request created here that gets serialised. And that just contains the JSON doc to be indexed. Is there a place (apart from unit tests) where we serialise the higher level AuditMessage or Level objects?

benwtrent · 2019-03-14T15:01:55Z

@jasontedor ping This PR adds some commonly useful code to xpack.common

benwtrent · 2019-03-14T16:28:55Z

run elasticsearch-ci/bwc

droberts195 · 2019-03-15T11:23:28Z

...core/src/main/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStore.java

+                        new RoleDescriptor.IndicesPrivileges[]{
+                            RoleDescriptor.IndicesPrivileges.builder()
+                                .indices(".data-frame-notifications*")
+                                .privileges("view_index_metadata", "read").build()


Please open a separate PR on the stack docs similar to elastic/stack-docs#246 that updates the docs for the definition of these two roles. It causes problems for people who create their own custom roles if we don't accurately state which privileges our built in roles provide.

@droberts195, I definitely will if this PR gets approved and we are going to use the Auditor.

benwtrent · 2019-03-19T12:46:05Z

run elasticsearch-ci/2

dimitris-athanasiou

LGTM

hendrikmuhs

LGTM

benwtrent · 2019-03-22T19:23:06Z

run elasticsearch-ci/2

benwtrent · 2019-03-22T20:18:46Z

run elasticsearch-ci/2

* [Data Frame] add auditor * Adjusting Level, Auditor, and message to address pr comments * Addressing PR comments

[Data Frame] add auditor

a194836

benwtrent added >non-issue :ml Machine learning v8.0.0 v7.2.0 labels Mar 13, 2019

hendrikmuhs reviewed Mar 14, 2019

View reviewed changes

dimitris-athanasiou reviewed Mar 14, 2019

View reviewed changes

droberts195 reviewed Mar 14, 2019

View reviewed changes

benwtrent added :ml/Transform Transform and removed :ml Machine learning labels Mar 14, 2019

benwtrent changed the title ~~[Data Frame] add auditor to data frame plugin~~ [ML] add auditor to data frame plugin Mar 14, 2019

Adjusting Level, Auditor, and message to address pr comments

ad006db

Addressing PR comments

c55d74c

droberts195 reviewed Mar 15, 2019

View reviewed changes

Merge branch 'master' into feature/data-frame-add-auditor

e0cdfab

benwtrent mentioned this pull request Mar 19, 2019

[ML] adds support for non-numeric mapped types #40220

Merged

benwtrent requested review from droberts195, dimitris-athanasiou and hendrikmuhs March 20, 2019 16:47

dimitris-athanasiou approved these changes Mar 21, 2019

View reviewed changes

hendrikmuhs approved these changes Mar 22, 2019

View reviewed changes

benwtrent added 2 commits March 22, 2019 07:32

Merge branch 'master' into feature/data-frame-add-auditor

d7b9b0b

Merge branch 'master' into feature/data-frame-add-auditor

4b089a5

Merge branch 'master' into feature/data-frame-add-auditor

c066494

benwtrent merged commit 3b6d42d into elastic:master Mar 23, 2019

benwtrent deleted the feature/data-frame-add-auditor branch March 23, 2019 19:08

benwtrent added a commit to benwtrent/elasticsearch that referenced this pull request Mar 23, 2019

[ML] add auditor to data frame plugin (elastic#40012)

1bcf2cb

* [Data Frame] add auditor * Adjusting Level, Auditor, and message to address pr comments * Addressing PR comments

benwtrent added a commit that referenced this pull request Mar 23, 2019

[ML] add auditor to data frame plugin (#40012) (#40394)

a30bf27

* [Data Frame] add auditor * Adjusting Level, Auditor, and message to address pr comments * Addressing PR comments

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021


		protected abstract String getResourceField();

		public static final class AuditMessageBuilder<T extends AbstractAuditMessage> {

[ML] add auditor to data frame plugin #40012

[ML] add auditor to data frame plugin #40012

Uh oh!

Conversation

benwtrent commented Mar 13, 2019

Uh oh!

elasticmachine commented Mar 13, 2019

Uh oh!

hendrikmuhs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dimitris-athanasiou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dimitris-athanasiou commented Mar 14, 2019

Uh oh!

droberts195 commented Mar 14, 2019

Uh oh!

benwtrent commented Mar 14, 2019

Uh oh!

benwtrent commented Mar 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benwtrent commented Mar 19, 2019

Uh oh!

dimitris-athanasiou left a comment

Choose a reason for hiding this comment

Uh oh!

hendrikmuhs left a comment

Choose a reason for hiding this comment

Uh oh!

benwtrent commented Mar 22, 2019

Uh oh!

benwtrent commented Mar 22, 2019

Uh oh!

Uh oh!