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

[ML] New Platform server shim: update analytics routes to use new platform router #53521

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Dec 18, 2019

Summary

Updates all data frame analytics routes to use new platform router.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested the data frame analytics pages, and all LGTM

@alvarezmelissa87 alvarezmelissa87 requested review from joshdover and a team and removed request for pgayvallet and joshdover December 19, 2019 14:36
@@ -180,17 +189,28 @@ export class Plugin {
};
});

// Can access via new platform router's handler function 'context' parameter - context.ml.mlClient
const mlClient = core.elasticsearch.createClient('ml', { plugins: [elasticsearchJsPlugin] });
// @ts-ignore // 'ml' not 'search' or 'security' or 'licensing
Copy link
Contributor

Choose a reason for hiding this comment

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

You can type 'ml' the same way search did it:

declare module 'kibana/server' {
interface RequestHandlerContext {
search?: IRouteHandlerSearchContext;
}
}

This will allow any consumers of this context to get type information as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point - added here: f9a7b5b5c5cd88050be8512f2e59a79c512daa68

Copy link
Member

@jgowdyelastic jgowdyelastic Dec 23, 2019

Choose a reason for hiding this comment

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

looks like this @ts-ignore isn't needed now

});
} catch (e) {
// Case: default
return response.internalError({ body: e });
Copy link
Contributor

Choose a reason for hiding this comment

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

This will expose the stack trace of e which could reveal information about the server like the location of files. This should rather be response.internalError({body: e.message}); (also in other places in the PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for taking a look! 😄
Updated here: 66d9d97048c385885a158804899124f469390672

params: schema.object({
analyticsId: schema.string(),
}),
body: schema.object({ ...dataAnalyticsJobConfigSchema }, { allowUnknowns: true }),
Copy link
Contributor

Choose a reason for hiding this comment

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

Validation is our best protection against prototype pollution attacks, allowUnknowns: true disables a lot of that protection. Is it possible to enumerate all the expected properties in the schema?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call - updated here: 66d9d97048c385885a158804899124f469390672

},
config: {
...commonRouteConfig,
licensePreRoutingFactory(xpackMainPlugin, dfAnalyticsDeleteJobHandler)
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: subjective opinion, but you could inline the handlers as they are all used only once. This will also helps when converting to TS as the context, request, response types will be inferred.

licensePreRoutingFactory(xpackMainPlugin, (context, request, response) {
    try {
      const { analyticsId } = request.params;
      const results = await context.ml.mlClient.callAsCurrentUser('ml.deleteDataFrameAnalytics', {
        analyticsId,
      });
      return response.ok({
        body: { ...results },
      });
    } catch (e) {
      // Case: default
      return response.internalError({ body: e.message });
    }
  })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion! Changed in 1be4d620155fc0186cd7fedcaebb6c7ba5a51c9c

Comment on lines 15 to 18
export const licensePreRoutingFactory = (
xpackMainPlugin: any,
handler: RequestHandler<any, any, any>
): RequestHandler<any, any, any> => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use proper type forwarding here, will be useful when your handlers will be migrated to TS (see src/core/server/http/router/error_wrapper.ts for an example)

export const licensePreRoutingFactory = <
  P extends ObjectType,
  Q extends ObjectType,
  B extends ObjectType
>(
  xpackMainPlugin: any,
  handler: RequestHandler<P, Q, B>
): RequestHandler<P, Q, B> => {
...
}

Comment on lines 28 to 32
return response.customError({
body: {
message: licenseCheckResults.message,
},
statusCode: 403,
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to use customError for common return types. Use response.forbidden instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use forbidden in 8c93d6b4a20f1de2821b6ab6f8835703bdb09abf

body: { ...results },
});
} catch (e) {
// Case: default
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: comment seems strange?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed in 1be4d620155fc0186cd7fedcaebb6c7ba5a51c9c

@alvarezmelissa87 alvarezmelissa87 force-pushed the ml-new-platform-server-shim-update branch from 1be4d62 to b6ac51d Compare December 20, 2019 18:31
body: { ...results },
});
} catch (e) {
return response.forbidden({ body: e.message });
Copy link
Member

@jgowdyelastic jgowdyelastic Dec 23, 2019

Choose a reason for hiding this comment

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

is this going to return a 403 for every error that gets thrown.
Do we not want to do something similar to how the old routes work where the errors thrown from the es endpoints are wrapped in a boom error? or something similar if we don't want to use boom anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep - find and replace gone wrong which I thought I removed but slipped through. Thanks. Fixed in 8c93d6b4a20f1de2821b6ab6f8835703bdb09abf

}
}

export const PLUGIN_ID = 'ml';
Copy link
Member

Choose a reason for hiding this comment

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

nit, it's good practice to keep const vars like this are the top of the file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👌 Moved to top in 8c93d6b4a20f1de2821b6ab6f8835703bdb09abf

@alvarezmelissa87 alvarezmelissa87 force-pushed the ml-new-platform-server-shim-update branch from f9a7b5b to 3c173a0 Compare December 28, 2019 18:55
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

  • 💚 Build #17031 succeeded f9a7b5b5c5cd88050be8512f2e59a79c512daa68
  • 💚 Build #17000 succeeded 9e871bee1f834cabda13ec8871555016c6112bdc
  • 💔 Build #16983 failed b6ac51daa63d364207294b42f36b1bd1b306755d
  • 💔 Build #16827 failed 66d9d97048c385885a158804899124f469390672
  • 💔 Build #16535 failed 8f866e17ff1df5cf9815104aded77dddbab843ce

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@elastic elastic deleted a comment from elasticmachine Dec 28, 2019
@alvarezmelissa87
Copy link
Contributor Author

This has been updated and is ready for a final look when you get a chance. cc @walterra, @peteharverson, @darnautov, @pgayvallet.

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

Code itself LGTM, caveat: I don't know much about all the server side APIs involved but I see you got good feedback from others in that regard.

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM!!

@jgowdyelastic
Copy link
Member

jgowdyelastic commented Jan 2, 2020

I just noticed that i'm seeing server errors when viewing the results for a regression job. the api/ml/data_frame/_evaluate endpoint is throwing a 500.

image

Calling the underlying es endpoint manually returns this error:

{
  "error": {
    "root_cause": [
      {
        "type": "status_exception",
        "reason": "No documents found containing both [httpversion, ml.httpversion_prediction] fields"
      }
    ],
    "type": "status_exception",
    "reason": "No documents found containing both [httpversion, ml.httpversion_prediction] fields"
  },
  "status": 400
}

It looks like the use of boomify inside wrapError requires options containing the status code to be passed to it. This is how the original wrapError worked.
Without this status code, the error is assumed to be a 500.

The old version of wrapError for reference:

export function wrapError(error) {
  return boomify(error, { statusCode: error.status });
}

@jgowdyelastic - yep that status did need to get set as an option in the new error wrapper. The ui still gives the right callout so I missed this. Thanks for catching it. Added the status option here: b8c3bd08ea8efa46ea8c4496e80193987215e9a1

@jgowdyelastic jgowdyelastic self-requested a review January 2, 2020 13:34
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested latest edits and LGTM

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

Latest changes LGTM

@alvarezmelissa87 alvarezmelissa87 force-pushed the ml-new-platform-server-shim-update branch from b8c3bd0 to 5174b07 Compare January 2, 2020 15:41
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

  • 💚 Build #17364 succeeded 8c93d6b4a20f1de2821b6ab6f8835703bdb09abf

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@alvarezmelissa87 alvarezmelissa87 merged commit 9372100 into elastic:master Jan 2, 2020
alvarezmelissa87 added a commit to alvarezmelissa87/kibana that referenced this pull request Jan 2, 2020
…tform router (elastic#53521)

* update dfAnalytics routes to use np router

* add route schemas and only show error message

* convert route file to ts and set handlers inline

* update df analytics param type

* update mlClient type and assert mlClient is not null

* handle errors correctly

* ensure error status gets passed correctly to wrapper
@alvarezmelissa87 alvarezmelissa87 deleted the ml-new-platform-server-shim-update branch January 3, 2020 04:11
alvarezmelissa87 added a commit that referenced this pull request Jan 3, 2020
…tform router (#53521) (#53897)

* update dfAnalytics routes to use np router

* add route schemas and only show error message

* convert route file to ts and set handlers inline

* update df analytics param type

* update mlClient type and assert mlClient is not null

* handle errors correctly

* ensure error status gets passed correctly to wrapper
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 3, 2020
* master:
  Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886)
  Updating transitive dependencies to use [email protected] (elastic#53899)
  [Reporting/Tests] consolidate functional test configs (elastic#52671)
  [Reporting] Correct the docvalue_fields params in the search query Download CSV from Dashboard Panel (elastic#52833)
  [Test/Newsfeed] Re-enable test and add news item to be filtered (elastic#53905)
  cleanup server-log action (elastic#53326)
  [Uptime] Delete uptime eslint rule skip (elastic#50912)
  [skip-ci] Expression Lifecycle Docs (elastic#51494)
  [Endpoint] add react router to endpoint app (elastic#53808)
  [SIEM][Detection Engine] Silence 409 errors on signal creation (elastic#53859)
  [Maps] get max_result_window and max_inner_result_window from index settings (elastic#53500)
  [ML] New Platform server shim: update analytics routes to use new platform router (elastic#53521)
  fixes typo on engine detection page (elastic#53877)
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 3, 2020
* master:
  Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886)
  Updating transitive dependencies to use [email protected] (elastic#53899)
  [Reporting/Tests] consolidate functional test configs (elastic#52671)
  [Reporting] Correct the docvalue_fields params in the search query Download CSV from Dashboard Panel (elastic#52833)
  [Test/Newsfeed] Re-enable test and add news item to be filtered (elastic#53905)
  cleanup server-log action (elastic#53326)
  [Uptime] Delete uptime eslint rule skip (elastic#50912)
  [skip-ci] Expression Lifecycle Docs (elastic#51494)
  [Endpoint] add react router to endpoint app (elastic#53808)
  [SIEM][Detection Engine] Silence 409 errors on signal creation (elastic#53859)
  [Maps] get max_result_window and max_inner_result_window from index settings (elastic#53500)
  [ML] New Platform server shim: update analytics routes to use new platform router (elastic#53521)
  fixes typo on engine detection page (elastic#53877)
  [Maps] push mapbox value extraction from VectorStyle and into DynamicStyleProperty (elastic#53806)
  Fix suggested value for time_zone in range query (elastic#53841)
  Clean up generic hooks, use react-use instead (elastic#53822)
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 3, 2020
…ris/kibana into alerting/created_at-and-updated_at

* 'alerting/created_at-and-updated_at' of github.com:gmmorris/kibana:
  Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886)
  Updating transitive dependencies to use [email protected] (elastic#53899)
  [Reporting/Tests] consolidate functional test configs (elastic#52671)
  [Reporting] Correct the docvalue_fields params in the search query Download CSV from Dashboard Panel (elastic#52833)
  [Test/Newsfeed] Re-enable test and add news item to be filtered (elastic#53905)
  cleanup server-log action (elastic#53326)
  [Uptime] Delete uptime eslint rule skip (elastic#50912)
  [skip-ci] Expression Lifecycle Docs (elastic#51494)
  [Endpoint] add react router to endpoint app (elastic#53808)
  [SIEM][Detection Engine] Silence 409 errors on signal creation (elastic#53859)
  [Maps] get max_result_window and max_inner_result_window from index settings (elastic#53500)
  [ML] New Platform server shim: update analytics routes to use new platform router (elastic#53521)
  fixes typo on engine detection page (elastic#53877)
  [Maps] push mapbox value extraction from VectorStyle and into DynamicStyleProperty (elastic#53806)
  Fix suggested value for time_zone in range query (elastic#53841)
  Clean up generic hooks, use react-use instead (elastic#53822)
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 3, 2020
…t-types

* alerting/created_at-and-updated_at:
  updatedAt should equal createdAt on creation
  Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data (elastic#53794)
  moved Task Manager server code under "server" directory (elastic#53777)
  Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886)
  Updating transitive dependencies to use [email protected] (elastic#53899)
  [Reporting/Tests] consolidate functional test configs (elastic#52671)
  [Reporting] Correct the docvalue_fields params in the search query Download CSV from Dashboard Panel (elastic#52833)
  [Test/Newsfeed] Re-enable test and add news item to be filtered (elastic#53905)
  cleanup server-log action (elastic#53326)
  [Uptime] Delete uptime eslint rule skip (elastic#50912)
  [skip-ci] Expression Lifecycle Docs (elastic#51494)
  [Endpoint] add react router to endpoint app (elastic#53808)
  [SIEM][Detection Engine] Silence 409 errors on signal creation (elastic#53859)
  [Maps] get max_result_window and max_inner_result_window from index settings (elastic#53500)
  [ML] New Platform server shim: update analytics routes to use new platform router (elastic#53521)
  fixes typo on engine detection page (elastic#53877)
  [Maps] push mapbox value extraction from VectorStyle and into DynamicStyleProperty (elastic#53806)
  Fix suggested value for time_zone in range query (elastic#53841)
  Clean up generic hooks, use react-use instead (elastic#53822)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration :ml release_note:skip Skip the PR/issue when compiling release notes v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants