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

Migrate Index Management to the new ES JS client #105863

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Jul 15, 2021

Closes #73973

This PR migrates Index Management to the new ES JS client. Other changes:

  • CCR, ILM, and Rollup index enrichers were also migrated.
  • Migrated API integration and unit tests.
  • Migrated route error handling to modern handleEsError pattern.

Note that I added numerous @ts-expect-error comments to silence TS complaints. I expect that we'll address these in subsequent PRs. I also discovered UX flaws and test coverage gaps, which I noted under my "Notes" section below. We'll also address these in subsequent PRs. All of these are noted under the "Follow-up items" section below.

Things to test

I've made suggestions about how to break up this testing work, but feel free to disregard this if you'd prefer to test in a different way.

  • (@sabarasaba) Test the CCR, ILM, and Rollup enrichers by following the testing steps in each of those plugins' READMEs.
  • (@sabarasaba) Try out the different index-level actions in Index Management: freeze, unfreeze, open, close, and so on. Perform these actions on both individual indices and on bulk indices. See "Special test cases" below for further testing.
  • (@sabarasaba) Follow the instructions in the Index Management README to create a data stream. Verify that it appears in the data streams tab and that toggling "Include stats" surfaces additional stats in the table. See "Special test cases" below for further testing.
  • (@alisonelizabeth) Go through the indices, index templates, and component templates tabs and verify that the basic CRUD operations work. See "Special test cases" below for further testing.
  • (@alisonelizabeth) Examine each tab in the index details flyout and verify there aren't any errors. Edit an index's settings.

Special test cases

Indices with special characters

We added support for special characters in index names like %{[@metadata][beat]}-%{[@metadata][version]}-2020.08.23 with #76584. You can create this index in Console like this:

PUT %25%7B%5B%40metadata%5D%5Bbeat%5D%7D-%25%7B%5B%40metadata%5D%5Bversion%5D%7D-2020.08.23

Test that all actions can be performed on this index.

Data streams with special characters

Similar to the above, data streams also support special characters in their names and we need to test that we can fetch them, both with and without stats, and also delete them. You can create this kind of data stream in Console like this:

# Configure template for creating a data stream
PUT _index_template/special_ds
{
  "index_patterns": ["%{[@metadata][beat]}-%{[@metadata][version]}-2020.08.23"],
  "data_stream": {}
}

# Add a document to the data stream
POST %25%7B%5B%40metadata%5D%5Bbeat%5D%7D-%25%7B%5B%40metadata%5D%5Bversion%5D%7D-2020.08.23/_doc
{
  "@timestamp": "2020-01-27"
}

Legacy index templates

Follow the Index Management README instructions to get legacy index templates to appear in the UI.

Follow-up items

I'll create individual issues to track these items.

Notes

To-do

  • Go through each file and update the API call
    • Check diff that I'm destructuring body from all responses
    • Check force merge route for error
  • Double check index enrichers
  • Migrate rollup enricher
  • Migrate all routes to handleEsError
  • Test each API route and note any not covered by API integration tests
    • Test freeze in bulk with encoded characters
    • Test unfreeze in bulk with encoded characters
    • Test consumers of templates lib (e.g. doesTemplateExist)
    • Test get_managed_templates (Cloud stuff)
    • Test batch deleting component templates and reporting errors
    • Test ES errors in component templates privileges route, since it's not using handleEsError -- just shows regular SectionError
    • Test both legacy and composable index templates
  • Test indices with special characters per URI encode the index names we fetch in the fetchIndices lib function #76584
  • Test ILM enricher
  • Test Rollup enricher
  • Test CCR enricher
  • Fix API integration tests, run locally
  • Delete commented out code from fetch_indices
  • Investigate comment TODO update when elasticsearch client has update requestParams for 'indices.getDataStream' -- ignoring for now to minimize scope
  • Fix TS errors

Route audit

Component templates

Route Notes API test / error Local test
Create component template Collision check ✅ 🚫
Delete component template Split on comma ✅ ✅
Get all component templates ✅ 🚫
Get one component template ✅ 🚫
Component template privileges Test is sparse, made up for by unit tests ✅ 🚫
Update component template ✅ ✅

Data streams

Route Notes API test / error Local test
Delete data stream(s) 🔧 🚫
Get all data streams Include stats option, encodeURIComponent ✅ 🚫
Get one data stream Include stats option, encodeURIComponent 🔧 🚫

Indices

Notes: Tested listing, freezing, and unfreezing with indices with special characters, per #76584.

Route Notes API test / error Local test
Clear cache Bulk ✅ 🚫
Close Bulk 🔧 🚫
Delete Bulk 🔧 🚫
Flush Bulk 🔧 🚫
Force merge Bulk, maxNumSegments option 🔧 🚫
Freeze Bulk 🔧 🚫
List Bulk, enrich ✅ 🚫
Open Bulk 🔧 🚫
Refresh Bulk 🔧 🚫
Reload Bulk, enrich ✅ 🚫
Unfreeze Bulk 🔧 🚫

Mapping

Notes: Tested with special characters, per #76584.

Route Notes API test / error Local test
Get mapping ✅ 🚫

Settings

Notes: Tested with special characters, per #76584.

Route Notes API test / error Local test
Load settings ✅ 🚫
Update settings ✅ 🚫

Stats

Notes: Tested with special characters, per #76584.

Route Notes API test / error Local test
Get stats ✅ 🚫

Index templates

Route Notes API test / error Local test
Create index template Collision check, legacy option 🔧 ✅
Delete index template Bulk, legacy option 🔧 🚫
Get all index templates Cloud-managed, includes legacy 🔧 🚫
Get one index template Cloud-managed, legacy option (has no error message) 🔧 🚫
Simulate 🚫 🚫
Update index template Exist check, legacy option 🔧 ✅

Error-handling

Index Management privileges

When testing with a user with the following privileges, Index Management is accessible, but the Indices and Data Streams tabs report the privileges error.

index_management_user: {
  elasticsearch: {
    cluster: ['monitor', 'manage_index_templates'],
  },
},

image

image

Reducing it further shows error for index and component templates too:

index_management_user: {
  elasticsearch: {
    cluster: ['monitor'],
  },
},

image

Note: Component templates has a specific privileges check for manage_index_templates and renders a custom error message.

image

Deleting index templates

If you delete an index template that's in use by a data stream, you'll get back lots of error details but they aren't surfaced in the UI. Ideally you should be able to click a button in the error toast to get more info.

{
  "templatesDeleted": [
    "special%"
  ],
  "errors": [
    {
      "name": "special",
      "error": {
        "status": 400,
        "payload": {
          "message": "unable to remove composable templates [special] as they are in use by a data streams [special%]",
          "attributes": {
            "error": {
              "root_cause": [
                {
                  "type": "illegal_argument_exception",
                  "reason": "unable to remove composable templates [special] as they are in use by a data streams [special%]"
                }
              ],
              "type": "illegal_argument_exception",
              "reason": "unable to remove composable templates [special] as they are in use by a data streams [special%]"
            }
          }
        },
        "options": {
          "statusCode": 400,
          "body": {
            "message": "unable to remove composable templates [special] as they are in use by a data streams [special%]",
            "attributes": {
              "error": {
                "root_cause": [
                  {
                    "type": "illegal_argument_exception",
                    "reason": "unable to remove composable templates [special] as they are in use by a data streams [special%]"
                  }
                ],
                "type": "illegal_argument_exception",
                "reason": "unable to remove composable templates [special] as they are in use by a data streams [special%]"
              }
            }
          }
        }
      }
    }
  ]
}

Deleting component templates

Similarly, if you delete component templates that no longer exist, you'll get back lots of error details but they aren't surfaced in the UI. Ideally you should be able to click a button in the error toast to get more info.

{
  "itemsDeleted": [],
  "errors": [
    {
      "name": "trvdfvds",
      "error": {
        "status": 404,
        "payload": {
          "message": "trvdfvds",
          "attributes": {
            "error": {
              "root_cause": [
                {
                  "type": "resource_not_found_exception",
                  "reason": "trvdfvds"
                }
              ],
              "type": "resource_not_found_exception",
              "reason": "trvdfvds"
            }
          }
        },
        "options": {
          "statusCode": 404,
          "body": {
            "message": "trvdfvds",
            "attributes": {
              "error": {
                "root_cause": [
                  {
                    "type": "resource_not_found_exception",
                    "reason": "trvdfvds"
                  }
                ],
                "type": "resource_not_found_exception",
                "reason": "trvdfvds"
              }
            }
          }
        }
      }
    },
    {
      "name": "test",
      "error": {
        "status": 404,
        "payload": {
          "message": "test",
          "attributes": {
            "error": {
              "root_cause": [
                {
                  "type": "resource_not_found_exception",
                  "reason": "test"
                }
              ],
              "type": "resource_not_found_exception",
              "reason": "test"
            }
          }
        },
        "options": {
          "statusCode": 404,
          "body": {
            "message": "test",
            "attributes": {
              "error": {
                "root_cause": [
                  {
                    "type": "resource_not_found_exception",
                    "reason": "test"
                  }
                ],
                "type": "resource_not_found_exception",
                "reason": "test"
              }
            }
          }
        }
      }
    }
  ]
}

@cjcenizal cjcenizal force-pushed the index-management/migrate-legacy-es-js-client branch 3 times, most recently from 2138084 to c2d901f Compare July 19, 2021 18:57
@cjcenizal cjcenizal force-pushed the index-management/migrate-legacy-es-js-client branch 2 times, most recently from 4db07da to cb25a9b Compare July 19, 2021 22:55
@cjcenizal cjcenizal force-pushed the index-management/migrate-legacy-es-js-client branch from 42424de to 0d53fc5 Compare July 20, 2021 14:57
@cjcenizal cjcenizal changed the title Migrate Index Management to the new ES JS client [skip-ci] Migrate Index Management to the new ES JS client Jul 20, 2021
* Destructure index API request bodies consistently.
* Update tests.
* Migrate enrichers.
- Migrate create and update index template routes to handleEsError.
* Fix simulate endpoint and move elasticsearch/issues/59152 workaround to server.
* Remove commented-out code and unnecessary types from fetch_indices.
* Convert to IScopedClusterClient in a few places.
* Delete wrapEsError helpers.
* Remove unnecessary calls to encodeURIComponent on the server.
@cjcenizal cjcenizal changed the title [skip-ci] Migrate Index Management to the new ES JS client Migrate Index Management to the new ES JS client Jul 20, 2021
@cjcenizal cjcenizal added the Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more label Jul 20, 2021
@cjcenizal cjcenizal marked this pull request as ready for review July 20, 2021 18:17
@cjcenizal cjcenizal requested a review from a team as a code owner July 20, 2021 18:17
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-management (Team:Stack Management)

@cjcenizal cjcenizal force-pushed the index-management/migrate-legacy-es-js-client branch from 0d53fc5 to fead8c1 Compare July 20, 2021 18:17
@cjcenizal cjcenizal added release_note:skip Skip the PR/issue when compiling release notes and removed blocked labels Jul 20, 2021
} = template;

// Verify the template exists (ES will throw 404 if not)
const { body: templateExists } = await doesTemplateExist({ name, client, isLegacy });
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 the logic inside the try/catch so we could catch any ES errors from the helpers.

@@ -281,8 +281,19 @@ export default function ({ getService }: FtrProviderContext) {
expect(body).to.eql({
statusCode: 404,
error: 'Not Found',
message:
'[resource_not_found_exception] component template matching [component_does_not_exist] not found',
message: 'component template matching [component_does_not_exist] not found',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The errors we get now are more consistent with what we get from ES, which I think is a good thing for making our error-handling UX more consistent (see #84801).

includeDefaults: true,
const {
body: { persistent, transient, defaults },
} = await client.asCurrentUser.cluster.getSettings({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't test this -- could someone look into setting these settings manually and verifying there are no errors?

Copy link
Contributor

Choose a reason for hiding this comment

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

Implemented via #43901.

Looks like it is working as expected for both composable and legacy index templates.

Screen Shot 2021-07-26 at 12 10 03 PM

Screen Shot 2021-07-26 at 12 10 09 PM

@@ -193,8 +193,8 @@ export default function ({ getService }) {
});

it('should parse the ES error and return the cause', async () => {
const templateName = `template-${getRandomString()}`;
const payload = getTemplatePayload(templateName, [getRandomString()]);
const templateName = `template-create-parse-es-error}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found these hardcoded strings very handy when I was debugging these tests by looking at the functional test runner logs.

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Nice work on this @cjcenizal! Great to see all of our apps migrated 🎉 . I left a few comments/questions in the code. Let me know what you think.

(@alisonelizabeth) Go through the indices, index templates, and component templates tabs and verify that the basic CRUD operations work. See "Special test cases" below for further testing.
(@alisonelizabeth) Examine each tab in the index details flyout and verify there aren't any errors. Edit an index's settings.

👍 Tested these items and everything worked as expected. I'll defer to @sabarasaba on the other items.

Also, now that we've migrated all of our apps, can we safely delete the is_es_error util from es_ui_shared? (Perhaps as a follow-up item.)


## Index templates tab

### Quick steps for texting
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### Quick steps for texting
### Quick steps for testing

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we also add a section on how to test cloud-managed templates?

See instructions on #43901.

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 catch, done.

documents: hit['docs.count'],
size: hit['store.size'],
isFrozen: hit.sth === 'true', // sth value coming back as a string from ES
aliases: aliases.length ? aliases : 'none',
// @ts-expect-error Property 'index' does not exist on type 'IndicesIndexSettings | IndicesIndexStatePrefixedSettings'.
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe any ts errors due to lack of coverage/incorrect types in the es js client should be commented as // @ts-expect-error @elastic/elasticsearch ....

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure it's incorrect typing - according to @elastic/elasticsearch typings it should be refactored to hidden: index.settings.hidden

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested this and verified that the original path (index.settings.index.hidden) is correct. I've updated all the annotations to be // @ts-expect-error @elastic/elasticsearch.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cjcenizal if you are sure there is a problem you should report it to the client team.

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. This is listed as a follow-up item in the PR description.

includeDefaults: true,
const {
body: { persistent, transient, defaults },
} = await client.asCurrentUser.cluster.getSettings({
Copy link
Contributor

Choose a reason for hiding this comment

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

Implemented via #43901.

Looks like it is working as expected for both composable and legacy index templates.

Screen Shot 2021-07-26 at 12 10 03 PM

Screen Shot 2021-07-26 at 12 10 09 PM

const getDataStreamsStats = (client: ElasticsearchClient, name = '*') => {
return client.transport.request({
const getDataStreamsStats = (client: IScopedClusterClient, name = '*') => {
return client.asCurrentUser.transport.request({
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this comment apply here as well, or can we use client.indices.dataStreamsStats?

  // TODO update when elasticsearch client has update requestParams...

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 catch, fixed.

@@ -68,9 +68,9 @@ const enhanceDataStreams = ({
});
};

const getDataStreams = (client: ElasticsearchClient, name = '*') => {
const getDataStreams = (client: IScopedClusterClient, name = '*') => {
// TODO update when elasticsearch client has update requestParams for 'indices.getDataStream'
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to open an issue for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

The method is already available in the new client

const { body } = await esClient.indices.getDataStream({ name: `${templateName}-*` });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also fixed.


return response.ok({ body: responseBody });
} catch (error) {
return handleEsError({ error, response });
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the handleEsError util provide the same functionality as before? I see previously it looks like we were parsing the error and passing it to body.attributes.

const error = lib.parseEsError(e.response);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested this. Looks like errors are surfaced correctly by the UI:

image

image


return response.ok({ body });
} catch (error) {
if (isEsError(error)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the handleEsError util here instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might be the only case where we're still using isEsError, so if we're able to remove it here, we should also be able to remove it from shared_imports and the api routes setup.

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 idea! Done.

throw e;
return response.ok({ body: templatePreview });
} catch (error) {
return handleEsError({ error, response });
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here about the previous behavior of parsing the ES error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested this and it shows up in the UI. We should probably add error-handling logic so it's rendered as an error.

image

Copy link
Member

@sabarasaba sabarasaba left a comment

Choose a reason for hiding this comment

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

Nice one @cjcenizal! I tested all the following items items and I didn't find any regressions:

(@sabarasaba) Test the CCR, ILM, and Rollup enrichers by following the testing steps in each of those plugins' READMEs.
(@sabarasaba) Try out the different index-level actions in Index Management: freeze, unfreeze, open, close, and so on. Perform these actions on both individual indices and on bulk indices. See "Special test cases" below for further testing.
(@sabarasaba) Follow the instructions in the Index Management README to create a data stream. Verify that it appears in the data streams tab and that toggling "Include stats" surfaces additional stats in the table. See "Special test cases" below for further testing.
+special cases

@cjcenizal cjcenizal force-pushed the index-management/migrate-legacy-es-js-client branch from b84b51c to 88a2e09 Compare July 31, 2021 00:11
@cjcenizal
Copy link
Contributor Author

Thanks for the reviews @sabarasaba and @alisonelizabeth! Alison, I believe I've addressed your comments. Could you please take another look?

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my feedback @cjcenizal! Did not retest, but reviewed the changes and everything LGTM.

@cjcenizal cjcenizal force-pushed the index-management/migrate-legacy-es-js-client branch from 0b4f32c to 0ed2d64 Compare July 31, 2021 14:30
@sabarasaba
Copy link
Member

Thanks @cjcenizal! Latest changes lgtm also 🥳

@cjcenizal cjcenizal merged commit 3491f05 into elastic:master Aug 2, 2021
@cjcenizal cjcenizal deleted the index-management/migrate-legacy-es-js-client branch August 2, 2021 17:20
cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Aug 2, 2021
…ic#105863)

* Destructure index API request bodies consistently.
* Remove unnecessary calls to encodeURIComponent on the server.
* Migrate routes to handleEsError. Delete wrapEsError helpers. Remove unused isEsError and parseEsError dependencies. Remove isEsError from es_ui_shared.
* Update tests and migrate API integration tests.
* Clarify test details in CCR README. Update Index Management README with steps for testing Cloud-managed index templates and steps for testing indices and data streams that contain special characters.
# Conflicts:
#	x-pack/plugins/index_management/server/routes/api/templates/register_create_route.ts
#	x-pack/plugins/index_management/server/routes/api/templates/register_get_routes.ts
#	x-pack/plugins/index_management/server/routes/api/templates/register_update_route.ts
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
indexManagement 1.3MB 1.3MB -56.0B
Unknown metric groups

API count

id before after diff
esUiShared 92 90 -2

API count missing comments

id before after diff
esUiShared 90 88 -2

References to deprecated APIs

id before after diff
crossClusterReplication 2 0 -2
indexLifecycleManagement 2 0 -2
indexManagement 12 0 -12
total -16

History

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

cjcenizal added a commit that referenced this pull request Aug 2, 2021
…105863) (#107431)

* Migrate Index Management and enrichers to the new ES JS client (#105863)

* Destructure index API request bodies consistently.
* Remove unnecessary calls to encodeURIComponent on the server.
* Migrate routes to handleEsError. Delete wrapEsError helpers. Remove unused isEsError and parseEsError dependencies. Remove isEsError from es_ui_shared.
* Update tests and migrate API integration tests.
* Clarify test details in CCR README. Update Index Management README with steps for testing Cloud-managed index templates and steps for testing indices and data streams that contain special characters.
# Conflicts:
#	x-pack/plugins/index_management/server/routes/api/templates/register_create_route.ts
#	x-pack/plugins/index_management/server/routes/api/templates/register_get_routes.ts
#	x-pack/plugins/index_management/server/routes/api/templates/register_update_route.ts

* Fix include_type_name type.
streamich pushed a commit to vadimkibana/kibana that referenced this pull request Aug 8, 2021
…ic#105863)

* Destructure index API request bodies consistently.
* Remove unnecessary calls to encodeURIComponent on the server.
* Migrate routes to handleEsError. Delete wrapEsError helpers. Remove unused isEsError and parseEsError dependencies. Remove isEsError from es_ui_shared.
* Update tests and migrate API integration tests.
* Clarify test details in CCR README. Update Index Management README with steps for testing Cloud-managed index templates and steps for testing indices and data streams that contain special characters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:Index Management Index and index templates UI release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stack Management elasticsearch-js client migration
6 participants