[ML][Data Frame] Add optional defer_validation param to PUT #44455

benwtrent · 2019-07-16T20:42:01Z

This adds the optional boolean parameter defer_validation to the PUT transform API. This parameter will defer any validations that are considered addressable AFTER the configuration has been created.

We do many validations eagerly against the current state of the cluster on PUT. This is so any issues can be captured early to prevent failures later when the data frame transform starts running. But, certain validations could be deferred until _start as the overall cluster state could have mutated between PUT and _start.

The default behavior is to run the validations eagerly.

I did not add any reference or HLRC documentation yet. But will if desired.
@droberts195
Do we want to document this parameter? It seems like a super user parameter and should almost never be used.

closes #43439

elasticmachine · 2019-07-16T20:42:04Z

Pinging @elastic/ml-core

...src/main/java/org/elasticsearch/xpack/core/dataframe/action/PutDataFrameTransformAction.java

...a/org/elasticsearch/xpack/core/dataframe/action/PutDataFrameTransformActionRequestTests.java

...src/main/java/org/elasticsearch/xpack/core/dataframe/action/PutDataFrameTransformAction.java

...main/java/org/elasticsearch/xpack/dataframe/action/TransportPutDataFrameTransformAction.java

.../rest-high-level/src/test/java/org/elasticsearch/client/DataFrameRequestConvertersTests.java

...igh-level/src/main/java/org/elasticsearch/client/dataframe/PutDataFrameTransformRequest.java

...main/java/org/elasticsearch/xpack/dataframe/action/TransportPutDataFrameTransformAction.java

...igh-level/src/main/java/org/elasticsearch/client/dataframe/PutDataFrameTransformRequest.java

davidkyle · 2019-07-17T07:35:26Z

...ta-frame/src/main/java/org/elasticsearch/xpack/dataframe/transforms/SourceDestValidator.java

 public final class SourceDestValidator {

-    private SourceDestValidator() {}
+    interface SourceDestValidation {


hendrikmuhs

added some comments.

I think we should document this new parameter but make very clear what the usecase is. Nit: It would be good to add some logging if the parameter is used (debug level).

hendrikmuhs · 2019-07-17T08:34:08Z

...src/main/java/org/elasticsearch/xpack/core/dataframe/action/PutDataFrameTransformAction.java

        public Request(StreamInput in) throws IOException {
            super(in);
            this.config = new DataFrameTransformConfig(in);
+            if (in.getVersion().onOrAfter(Version.CURRENT)) {


as for check in BWC code like this: you can alternatively set the version, disable BWC in build.gradle, do the backports and then re-enable BWC. You need the same number of commits in the end, but using the "Version.CURRENT approach" still broke CI, disabling BWC seems to be the better approach now.

@hendrikmuhs I usually mute the tests before I backport in a separate PR. This way BWC can actually run against my changes and I can find any issues earlier rather than later.

...main/java/org/elasticsearch/xpack/dataframe/action/TransportPutDataFrameTransformAction.java

hendrikmuhs · 2019-07-17T08:42:32Z

...ta-frame/src/main/java/org/elasticsearch/xpack/dataframe/transforms/SourceDestValidator.java

-    private SourceDestValidator() {}
+    interface SourceDestValidation {
+        boolean isDeferrable();
+        void check(DataFrameTransformConfig config, ClusterState clusterState, IndexNameExpressionResolver indexNameExpressionResolver);


nit: I still find it confusing that I have to call check and not validate on a "validator". (Vice versa it could also be a SourceDestChecker if you prefer to keep check)

davidkyle · 2019-07-17T09:07:42Z

I think we should document this new parameter but make very clear what the usecase

Yes, the source is open after all we should document and explain when it should be use. Remember it only defers so any error will be hit when validations are run on start anyway.

Also if the has privilege validation is deferrable it should checked on start

droberts195 · 2019-07-17T11:57:08Z

Also if the has privilege validation is deferrable it should checked on start

I don't think this is, because the rule is that the transform accesses the source and destination indices with the security privileges of the user who put it, not the user who starts it. The has-privileges API can only return the privileges of the current user, so could only check if the user calling start had the necessary privileges, not the user who called put.

This doesn't create a security issue - in the end if the user who put the transform doesn't have permission to read the source index and write to the destination then it won't do these things. But it does make the error reporting less friendly.

Maybe this can just be documented as a caveat of deferring validation: if you defer validation we assume you've thought about the security aspects of the transform and the transform will run but fail if you haven't.

I think this has actually opened up a can of worms though relating to the security aspects of the envisaged use case for this functionality: none of the built-in Beats roles have permission to put a data frame transform nor create an index other than those currently used by Beats. If Beats is going to use data frame transforms then the user that Beats connects as needs to have permission to manage data frame transforms and create and write to the destination indices configured in those transforms. Presumably to restrict abuse we'd have to set aside a pattern for those destination indices and only give the Beats user index permissions on that pattern. Then we'd have to document what that pattern is and warn users not to let indices storing unrelated confidential data match it.

davidkyle · 2019-07-17T12:14:55Z

👍 makes sense.

Assuming insufficient privileges causes a detectable exception we should handle that as an irrecoverable failure without retry. That change is not necessary for this PR

elasticsearch/x-pack/plugin/data-frame/src/main/java/org/elasticsearch/xpack/dataframe/transforms/DataFrameTransformTask.java

Line 803 in 92709f5

    
           return e instanceof IndexNotFoundException || e instanceof AggregationResultUtils.AggregationExtractionException;

davidkyle · 2019-07-18T12:23:42Z

...main/java/org/elasticsearch/xpack/dataframe/action/TransportPutDataFrameTransformAction.java

-                new RuntimeException(DataFrameMessages.REST_PUT_DATA_FRAME_FAILED_TO_VALIDATE_DATA_FRAME_CONFIGURATION,
-                    validationException))
+            validationException -> {
+                if (validationException instanceof Pivot.TestQueryException && request.isDeferValidation()) {


pivot.validateQuery() is called only when isDeferValidation() == false

This can be simplified by removing the if...else leaving the call to listener.onFailure

davidkyle · 2019-07-18T12:30:28Z

...ta-frame/src/main/java/org/elasticsearch/xpack/dataframe/transforms/SourceDestValidator.java

     * @param indexNameExpressionResolver A valid IndexNameExpressionResolver object
     * @throws ElasticsearchStatusException when a validation fails
     */
    public static void check(DataFrameTransformConfig config,


Per Hendrik's comment this should also be validate()

davidkyle · 2019-07-18T13:39:49Z

docs/reference/data-frames/apis/put-transform.asciidoc

+==== {api-query-parms-title}
+
+`defer_validations`::
+ (Optional, boolean) When `true`, this will cause deferrable validations to not run.


We should include more detail about deferred validations before the second sentence.

Suggested change

(Optional, boolean) When `true`, this will cause deferrable validations to not run.

(Optional, boolean) When `true`, this will cause deferrable validations to not run.

Deferrable validations include checks for the existence of the source index and that the

user has sufficient privileges, skipping these validations at creation is useful for

situations where the source index does not exist but will be created later. Deferred

validations are always run when the {dataframe-transform} is started.

Maybe the very last sentence of that suggested change should be changed to: "Deferred validations are always run when the {dataframe-transform} is started, with the exception of privilege checks. If the user who created the transform does not have the required privileges on the source and destination indices then the transform will start but then fail when it attempts the unauthorized operation."

These details are great, thanks!

przemekwitek

LGTM

przemekwitek · 2019-07-19T07:22:06Z

.../rest-high-level/src/test/java/org/elasticsearch/client/DataFrameRequestConvertersTests.java

        Request request = DataFrameRequestConverters.putDataFrameTransform(putRequest);
-
+        assertThat(request.getParameters(), not(hasKey("defer_validation")));
        assertEquals(HttpPut.METHOD_NAME, request.getMethod());


While you are, could you change "assertEquals" to "assertThat(..., equalTo(...)) so that the usage of assertions is consistent in the whole method.

przemekwitek · 2019-07-19T07:26:03Z

docs/java-rest/high-level/dataframe/put_data_frame.asciidoc

 include-tagged::{doc-tests-file}[{api}-request]
 --------------------------------------------------
 <1> The configuration of the {dataframe-transform} to create
+<2> Whether or not to wait to run deferrable validations until `_start` is called.


[nit] "Whether or not to wait to run deferrable validations" sounds a bit complicated, but I guess you wanted to avoid "Whether or not to defer deferrable validations until ...". Anyway, I would try to somehow simplify it.

Maybe Postpone deferrable validations until _start is called.

przemekwitek · 2019-07-19T07:27:09Z

docs/reference/data-frames/apis/put-transform.asciidoc

+[[put-data-frame-transform-query-parms]]
+==== {api-query-parms-title}
+
+`defer_validations`::


I think it is now singular everywhere.

hendrikmuhs

LGTM

davidkyle

LGTM

…44455) * [ML][Data Frame] Add optional defer_validation param to PUT * addressing PR comments * reverting bad replace * addressing pr comments * Update put-transform.asciidoc * Update put-transform.asciidoc * Update put-transform.asciidoc

…4455) (#44697) * [ML][Data Frame] Add optional defer_validation param to PUT (#44455) * [ML][Data Frame] Add optional defer_validation param to PUT * addressing PR comments * reverting bad replace * addressing pr comments * Update put-transform.asciidoc * Update put-transform.asciidoc * Update put-transform.asciidoc * adjusting for backport * fixing imports * [DOCS] Fixes formatting in create data frame transform API

[ML][Data Frame] Add optional defer_validation param to PUT

eba9748

benwtrent added >enhancement v8.0.0 :ml/Transform Transform v7.4.0 labels Jul 16, 2019

przemekwitek reviewed Jul 17, 2019

View reviewed changes

davidkyle reviewed Jul 17, 2019

View reviewed changes

hendrikmuhs reviewed Jul 17, 2019

View reviewed changes

benwtrent and others added 2 commits July 17, 2019 12:45

addressing PR comments

49b1f79

Merge branch 'master' into feature/ml-df-add-defer-validation-option

95ef94d

benwtrent requested review from davidkyle, hendrikmuhs and przemekwitek July 18, 2019 11:33

reverting bad replace

0a6aa35

davidkyle reviewed Jul 18, 2019

View reviewed changes

benwtrent and others added 2 commits July 18, 2019 09:37

addressing pr comments

7995b13

Update put-transform.asciidoc

41191e5

przemekwitek approved these changes Jul 19, 2019

View reviewed changes

Update put-transform.asciidoc

32e2ca6

benwtrent requested a review from davidkyle July 19, 2019 13:28

hendrikmuhs approved these changes Jul 22, 2019

View reviewed changes

Update put-transform.asciidoc

161e186

davidkyle approved these changes Jul 22, 2019

View reviewed changes

benwtrent merged commit a9cc0e1 into elastic:master Jul 22, 2019

benwtrent deleted the feature/ml-df-add-defer-validation-option branch July 22, 2019 14:07

benwtrent mentioned this pull request Jul 22, 2019

[7.x] [ML][Data Frame] Add optional defer_validation param to PUT (#44455) #44697

Merged

lcawl mentioned this pull request Jul 22, 2019

[DOCS] Adds more validation details for data frame transforms #44714

Merged

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

- (Optional, boolean) When `true`, this will cause deferrable validations to not run.
+ (Optional, boolean) When `true`, this will cause deferrable validations to not run.
+ Deferrable validations include checks for the existence of the source index and that the
+user has sufficient privileges, skipping these validations at creation is useful for
+situations where the source index does not exist but will be created later. Deferred
+validations are always run when the {dataframe-transform} is started.

[ML][Data Frame] Add optional defer_validation param to PUT #44455

[ML][Data Frame] Add optional defer_validation param to PUT #44455

Uh oh!

Conversation

benwtrent commented Jul 16, 2019

Uh oh!

elasticmachine commented Jul 16, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hendrikmuhs 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidkyle commented Jul 17, 2019

Uh oh!

droberts195 commented Jul 17, 2019

Uh oh!

davidkyle commented Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidkyle Jul 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

przemekwitek 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!

davidkyle Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hendrikmuhs left a comment

Choose a reason for hiding this comment

Uh oh!

davidkyle left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

davidkyle commented Jul 17, 2019 •

edited

Loading

davidkyle Jul 18, 2019 •

edited

Loading

davidkyle Jul 22, 2019 •

edited

Loading