[ML] Add optional ability to delete target index and index pattern when deleting DFA job #66934

qn895 · 2020-05-18T21:19:05Z

Summary

This PR adds ability to optionally delete 1) the target index and 2) the index pattern associated with the DFA jobs

Client

It will check if user has permission to delete the target index. If user doesn't have permission, it won't show the first option.
It will also check the index pattern with the same name as the target index exists in the current Kibana's savedObject. If index pattern does exist, it will show. If not, it will hide the option.

Example 1: Index pattern does exist

Example 2: Index pattern does not exist for this job

Server

Modified the current DELETE `${basePath()}/data_frame/analytics/${analyticsId} route to check for an optional query object which includes the destinationIndex passed from the UI.
The route will do another check again to check if user either has permission to delete the index, if yes, then delete.
It will also do another check to get the index pattern Id, and if index pattern exists that matches the target index, it will delete

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

…ndex

elasticmachine · 2020-05-18T21:19:07Z

Pinging @elastic/ml-ui (:ml)

.../data_frame_analytics/pages/analytics_management/components/analytics_list/action_delete.tsx

darnautov · 2020-05-19T08:45:10Z

.../data_frame_analytics/pages/analytics_management/components/analytics_list/action_delete.tsx

+
+  // Check if an user has permission to delete the index & index pattern
+  useEffect(() => {
+    const toastNotifications = getToastNotifications();


Please access toast notifications from the context instead of importing from the cache

Done in d14fafa

.../data_frame_analytics/pages/analytics_management/components/analytics_list/action_delete.tsx

darnautov · 2020-05-19T08:46:56Z

.../data_frame_analytics/pages/analytics_management/components/analytics_list/action_delete.tsx

              {i18n.translate('xpack.ml.dataframe.analyticsList.deleteModalBody', {
-                defaultMessage: `Are you sure you want to delete this analytics job? The analytics job's destination index and optional Kibana index pattern will not be deleted.`,
+                defaultMessage: `Are you sure you want to delete this analytics job?`,
              })}
            </p>


I see you just updated the string, but would be better to use FormattedMessage component instead of i18n

Done in d14fafa

x-pack/plugins/ml/server/routes/data_frame_analytics.ts

darnautov · 2020-05-19T09:11:58Z

x-pack/plugins/ml/server/routes/data_frame_analytics.ts

+    if (
+      err.message === 'no handler found for uri [/_security/user/_has_privileges] and method [POST]'
+    ) {
+      return err;


Your function should return a Promise<boolean>, so returning an error seems wrong 🤔

Fixed in b867ef2

darnautov · 2020-05-19T09:13:03Z

x-pack/plugins/ml/server/routes/data_frame_analytics.ts

+    return privilege.index[destinationIndex].delete_index === true;
+  } catch (err) {
+    if (
+      err.message === 'no handler found for uri [/_security/user/_has_privileges] and method [POST]'


the error message could change at any time. you might want to check for status code instead if it's possible

Looking at verifyHasPrivileges from x-pack/plugins/monitoring/server/lib/elasticsearch/verify_monitoring_auth.js I think there's a particular reason why it's checked it like that. Let me follow up with the authors of that API.

If we move to a check similar to our other privilege checks, this error message comparison won't be needed.
I believe the reason this text is checked it because ignoreUnavailable is set, so the error returned could be a 404 rather than just a privilege error.
Our other privilege checks first confirm that security is enabled before attempting a privilege check so this error message comparison isn't needed.
See my comment further down in this PR regarding our
api/ml/ml_node_count endpoint.

x-pack/plugins/ml/server/routes/data_frame_analytics.ts

x-pack/plugins/ml/server/routes/schemas/data_analytics_schema.ts

darnautov · 2020-05-19T09:20:21Z

x-pack/plugins/ml/server/routes/data_frame_analytics.ts

+        const indexPatternId = await getIndexPatternId(context, destinationIndex);
+
+        if (indexPatternId) {
+          const result = deleteIndexPatternById(context, indexPatternId);


You don't anything with result, so a variable definition is redundant. You should get an error from eslint about issues like this one 🤔 . Worth to set up a eslint plugin for your IDE of choice

...cation/data_frame_analytics/pages/analytics_management/components/analytics_list/_index.scss

peteharverson · 2020-05-19T09:20:30Z

x-pack/plugins/ml/server/models/data_frame_analytics/index_patterns.ts

+import { IndexPatternAttributes } from 'src/plugins/data/server';
+// import { findObjectByTitle } from 'src/plugins/saved_objects/';
+import { mlLog } from '../../client/log';
+export const SAVED_OBJECT_TYPES = {


We also define these in ml/server/models/data_recognizer/data_recognizer.ts. Probably worth moving them out to the common folder - in ml/common/types/kibana.ts perhaps? But is it being used here?

peteharverson · 2020-05-19T09:34:04Z

x-pack/plugins/ml/server/models/data_frame_analytics/index_patterns.ts

+};
+
+export class IndexPatternHandler {
+  modulesDir = `${__dirname}/modules`;


Is this needed?

Removed in d14fafa

peteharverson · 2020-05-19T09:37:14Z

x-pack/plugins/ml/server/models/data_frame_analytics/index_patterns.ts

+  indexPatternId: string | undefined = undefined;
+
+  constructor(
+    private callAsCurrentUser: APICaller,


I don't think you are using callAsCurrentUser

Removed in d14fafa#diff-d71b0ca06f7c6aa048b8b3f9bb7c2eebR11

peteharverson · 2020-05-19T09:44:25Z

.../data_frame_analytics/pages/analytics_management/components/analytics_list/action_delete.tsx

+                    label={i18n.translate(
+                      'xpack.ml.dataframe.analyticsList.deleteTargetIndexTitle',
+                      {
+                        defaultMessage: 'Delete index pattern',


I think it is worth clarifying which index pattern is deleted here, using a message of Delete destination index pattern {indexName}

Done in e5e618f

peteharverson · 2020-05-19T09:45:07Z

.../data_frame_analytics/pages/analytics_management/components/analytics_list/action_delete.tsx

+                {userCanDeleteIndex && indexPatternExists && (
+                  <EuiSwitch
+                    label={i18n.translate(
+                      'xpack.ml.dataframe.analyticsList.deleteTargetIndexTitle',


This looks like it is using the same i18n ID as the message above xpack.ml.dataframe.analyticsList.deleteTargetIndexTitle

Fixed in e5e618f

peteharverson · 2020-05-19T09:46:02Z

.../data_frame_analytics/pages/analytics_management/components/analytics_list/action_delete.tsx

+                    label={i18n.translate(
+                      'xpack.ml.dataframe.analyticsList.deleteTargetIndexTitle',
+                      {
+                        defaultMessage: 'Delete destination index',


I think it is worth clarifying the name of the index name that is being deleted here Delete destination index {indexName}

Done in e5e618f

peteharverson · 2020-05-19T11:21:09Z

x-pack/plugins/ml/server/routes/data_frame_analytics.ts

      validate: {
        params: analyticsIdSchema,
+        query: analyticsIdIndexSchema,


I wonder if rather than passing the name of the index in the query, it should instead pass a boolean deleteDestinationIndex, which if true then goes and deletes the index. As it is, someone could in theory pass a completely different index to the endpoint, which seems a bit dangerous. It might make sense to pass two parameters - deleteDestinationIndex and deleteDestinationIndexPattern as the modal does allow zero, one or both to be deleted. It would mean one extra query on the server side, to get the destination index for the job, but this approach would make more sense to me.

You're right! This could be a security issue. I'll refactor it to use the boolean instead.

Done in d14fafa

jgowdyelastic · 2020-05-19T14:22:05Z

x-pack/plugins/ml/server/routes/data_frame_analytics.ts

+): Promise<boolean> {
+  let privilege;
+  try {
+    privilege = await context.ml!.mlClient.callAsCurrentUser('transport.request', {


this can be callAsCurrentUser('ml.privilegeCheck', { body: ... });

Switched call to ml.privilgeCheck in d14fafa

jgowdyelastic · 2020-05-19T14:34:16Z

x-pack/plugins/ml/server/routes/data_frame_analytics.ts

+        if (destinationIndex) {
+          // Verify if user has privilege to delete the destination index
+
+          const userCanDeleteDestIndex = userCanDeleteIndex(context, destinationIndex);


/api/ml/ml_node_count might be a good example of how this endpoint can be written.
If the delete index flag is true, you'll need to look up the destination index and if they don't have permission to delete, we should return a response.forbidden(); immediately.

Done in d14fafa#diff-d4f061dec5ae8fcd9e871fa0ce0ed8bcR374

qn895 · 2020-05-21T14:29:51Z

@elasticmachine merge upstream

[ML] Clarify delete index & ip with names in modal

peteharverson · 2020-05-21T15:00:12Z

x-pack/plugins/ml/server/routes/schemas/data_analytics_schema.ts

@@ -60,6 +60,14 @@ export const analyticsIdSchema = schema.object({
  analyticsId: schema.string(),
 });

+export const analyticsIdIndexSchema = schema.object({


I think this should be renamed to something like deleteDataFrameAnalyticsJobSchema as it is specific to deleting the job.

Done in ba82079

jgowdyelastic · 2020-05-21T16:14:09Z

x-pack/plugins/ml/server/routes/data_frame_analytics.ts

+      });
+      return privilege.index[destinationIndex].delete_index === true;
+    } catch (err) {
+      throw err;


this try/catch is redundant as throwing err is the same as not having the catch

Removed in b867ef2

jgowdyelastic · 2020-05-21T16:15:16Z

x-pack/plugins/ml/server/models/data_frame_analytics/index_patterns.ts

+
+      return ip !== undefined ? ip.id : undefined;
+    } catch (error) {
+      throw error;


these try/catches are redundant as just throwing error is the same as not having the try/catch

Removed in ba82079

jgowdyelastic · 2020-05-21T16:19:25Z

x-pack/plugins/ml/server/models/data_frame_analytics/index_patterns.ts

+        obj => obj.attributes.title.toLowerCase() === indexName.toLowerCase()
+      );
+
+      return ip !== undefined ? ip.id : undefined;


nit, could be return ip?.id

Updated in ba82079

jgowdyelastic · 2020-05-21T16:33:44Z

x-pack/plugins/ml/server/routes/data_frame_analytics.ts

+                deleteTargetIndexAcknowledged = true;
+              } catch (deleteIndexError) {
+                errorsEncountered.push({
+                  msg: i18n.translate(


we don't translate error messages from endpoints

Removed in ba82079

…csJobSchema, and clean up

qn895 · 2020-05-25T21:21:07Z

@elasticmachine merge upstream

alvarezmelissa87 · 2020-05-28T15:06:41Z

x-pack/plugins/ml/server/routes/data_frame_analytics.ts

+        ],
+      },
+    });
+    return privilege.index[destinationIndex].delete_index === true;


Will privilege.index[destinationIndex] always be defined? Might be worth ensuring privilege and privilege.index[destinationIndex] are defined to avoid a possible TypeError if one of these is undefined.

privilege.has_all_requested can be used here too

Done in 5bd97e9

this change has also introduced a check for privilege.securityDisabled === true which doesn't exist on the server side.

alvarezmelissa87 · 2020-05-28T15:12:17Z

Looks good overall 👍 Just left a few minor comments.

…ndle undefined privilege

qn895 · 2020-05-28T22:41:28Z

@elasticmachine merge upstream

elasticmachine · 2020-05-28T22:41:30Z

merge conflict between base and head

…-when-del-job

x-pack/test/functional/services/machine_learning/api.ts

x-pack/test/functional/services/machine_learning/test_resources.ts

x-pack/test/functional/services/machine_learning/api.ts

jgowdyelastic · 2020-05-29T10:21:40Z

x-pack/plugins/ml/server/routes/data_frame_analytics.ts

+    if (!privilege) {
+      return false;
+    }
+    return privilege.securityDisabled === true || privilege.has_all_requested === true;


securityDisabled does not exist in privilege, it is only part of our client side ml.hasPrivileges function.

pheyos

API integration tests LGTM

darnautov · 2020-05-29T12:40:28Z

..._frame_analytics/pages/analytics_management/components/analytics_list/action_delete.test.tsx

@@ -46,4 +67,22 @@ describe('DeleteAction', () => {

    expect(getByTestId('mlAnalyticsJobDeleteButton')).toHaveAttribute('disabled');
  });
+
+  test('When delete modal is open, modal lets user delete target index by default.', () => {


Usually, test suite starts with should, e.g. should allow to delete target index by default.
If you have a common arrangement for tests you should combine them in group, for example

describe('When delete model is open', () => { beforeEach(async () => { // your arrangment here to prepare desired condition }) test('should allow to delete target index by default', async () => { }) })

darnautov · 2020-05-29T12:53:21Z

x-pack/plugins/ml/public/application/services/ml_api_service/data_frame_analytics.ts

+  success: boolean;
+  error?: CustomHttpResponseOptions<ResponseError>;
+}
+interface DeleteDataFrameAnalyticsWithIndexResponse {


I guess it'd be better to define somewhere in x-pack/plugins/ml/common/types because we need this interface both server and client-side

jgowdyelastic

LGTM

peteharverson

Tested after latest edits and LGTM

alvarezmelissa87

Latest edits LGTM ⚡

qn895 · 2020-06-01T13:51:50Z

@elasticmachine merge upstream

kibanamachine · 2020-06-01T15:42:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c118898

History

💚 Build #50552 succeeded e9e1d93
💚 Build #50536 succeeded 9e51430
💚 Build #50513 succeeded 725d56d
💚 Build #50472 succeeded f67115d
💚 Build #50460 succeeded d906bff

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

… DFA job (elastic#66934) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…leting DFA job (#66934) (#67875) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

qn895 added 3 commits May 15, 2020 13:50

[ML] Add additional arg to deleteDFA to allow delete of destination i…

df67c40

…ndex

[ML] Add option to delete target idx & idx pattern

23ee850

[ML] Modify styling of delete DFA job to use EUI

f880eac

qn895 added release_note:enhancement :ml v8.0.0 Feature:Data Frame Analytics ML data frame analytics features v7.9.0 labels May 18, 2020

qn895 requested review from darnautov, alvarezmelissa87 and peteharverson May 18, 2020 21:19

qn895 requested review from a team as code owners May 18, 2020 21:19

qn895 self-assigned this May 18, 2020

darnautov requested changes May 19, 2020

View reviewed changes

darnautov reviewed May 19, 2020

View reviewed changes

...cation/data_frame_analytics/pages/analytics_management/components/analytics_list/_index.scss Outdated Show resolved Hide resolved

peteharverson reviewed May 19, 2020

View reviewed changes

jgowdyelastic reviewed May 19, 2020

View reviewed changes

qn895 added 2 commits May 21, 2020 08:52

[ML] Refactor delete analytics endpoint

d14fafa

[ML] Refactor toggleDeleteIndex & toggleDeleteIndexPattern before render

4b8d140

elasticmachine and others added 2 commits May 21, 2020 07:29

Merge branch 'master' into add-opt-del-index-when-del-job

01b4956

[ML] Clarify delete index & ip with names in modal

e5e618f

[ML] Clarify delete index & ip with names in modal

peteharverson reviewed May 21, 2020

View reviewed changes

jgowdyelastic reviewed May 21, 2020

View reviewed changes

[ML] Update test for DFA action_delete, rename deleteDataFrameAnalyti…

ba82079

…csJobSchema, and clean up

alvarezmelissa87 reviewed May 28, 2020

View reviewed changes

peteharverson mentioned this pull request May 28, 2020

[ML] Increase API integration test coverage #63700

Closed

90 tasks

qn895 added 2 commits May 28, 2020 12:02

[ML] Change names to match dest schema, replace JSON.stringify(e), ha…

5bd97e9

…ndle undefined privilege

[ML] Update test for delete dfa endpoint

d906bff

qn895 added 2 commits May 28, 2020 18:22

Merge remote-tracking branch 'upstream/master' into add-opt-del-index…

1144296

…-when-del-job

[ML] Update common imports for delete dfa test

f67115d

pheyos requested changes May 29, 2020

View reviewed changes

[ML] Update dfa delete tests to re-use code

725d56d

jgowdyelastic reviewed May 29, 2020

View reviewed changes

pheyos approved these changes May 29, 2020

View reviewed changes

[ML] Remove securityDisabled check on server side

9e51430

darnautov reviewed May 29, 2020

View reviewed changes

jgowdyelastic approved these changes May 29, 2020

View reviewed changes

[ML] Refactor common use types and reformat jest test block

e9e1d93

peteharverson approved these changes May 29, 2020

View reviewed changes

alvarezmelissa87 approved these changes May 29, 2020

View reviewed changes

Merge branch 'master' into add-opt-del-index-when-del-job

c118898

darnautov approved these changes Jun 1, 2020

View reviewed changes

qn895 merged commit f31330a into elastic:master Jun 1, 2020

qn895 deleted the add-opt-del-index-when-del-job branch June 1, 2020 15:53

qn895 mentioned this pull request Jun 1, 2020

[7.x] [ML] Add ability to delete target index & index pattern when deleting DFA job (#66934) #67875

Merged

qn895 added a commit to qn895/kibana that referenced this pull request Jun 1, 2020

[ML] Add ability to delete target index & index pattern when deleting…

bd8b502

… DFA job (elastic#66934) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

qn895 added a commit that referenced this pull request Jun 2, 2020

[7.x] [ML] Add ability to delete target index & index pattern when de…

cae1831

…leting DFA job (#66934) (#67875) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

walterra mentioned this pull request Jun 18, 2020

[ML] Data Frame Analytics: User can optionally delete target index and index pattern when deleting a job. #69501

Closed

[ML] Add optional ability to delete target index and index pattern when deleting DFA job #66934

[ML] Add optional ability to delete target index and index pattern when deleting DFA job #66934

Conversation

qn895 commented May 18, 2020

Summary

Checklist

elasticmachine commented May 18, 2020

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

jgowdyelastic May 19, 2020 • edited Loading

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

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

qn895 commented May 21, 2020

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

qn895 commented May 25, 2020

alvarezmelissa87 May 28, 2020 • edited Loading

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

alvarezmelissa87 commented May 28, 2020

qn895 commented May 28, 2020

elasticmachine commented May 28, 2020

Choose a reason for hiding this comment

pheyos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgowdyelastic left a comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

alvarezmelissa87 left a comment

Choose a reason for hiding this comment

qn895 commented Jun 1, 2020

kibanamachine commented Jun 1, 2020

💚 Build Succeeded

History

jgowdyelastic May 19, 2020 •

edited

Loading

alvarezmelissa87 May 28, 2020 •

edited

Loading