Load FLS suggestions on-demand #98681

legrego · 2021-04-28T20:12:07Z

Summary

Improves the loading of field-level security ("FLS") suggestions on the role management page:

Fields suggestions are no longer eagerly loaded

Previously, the role management page would load all FLS suggestions on page load. This caused a lot of unnecessary overhead, and has resulted in a number of failure conditions over the past couple of years.

With this change, FLS suggestions are loaded "just in time":

When the FLS fields are brought into focus
When the selected indices have changed, and FLS is enabled
When FLS changes from disabled -> enabled

As with the previous implementation, the suggestions are cached on the client, so that we do not unnecessarily tax the cluster or Kibana's server.

The field mapping response has been filtered to only include the necessary fields from Elasticsearch

The response from Elasticsearch's Get Field Mappings API is quite verbose. If we are trying to retrieve field names for a large number of indices, or for indices with a large number of fields, then this can put undue strain on the cluster or Kibana server. We have seen OOM errors on the Kibana side in the past as a result of this.

It turns out that we only need a small subset of the response in order to retrieve the available fields. Luckily, ES has a common filter_path query parameter that we can use to prevent including unneeded fields in the response.

In my local testing, this dramatically reduced the number of bytes sent over the wire, which is both faster to send, and faster to parse/process in Node.js.

My test had an initial payload of ~1.2MB, while the filtered payload was merely 260KB - this is roughly a 76% payload reduction!

Resolves #47378

elasticmachine · 2021-04-29T13:59:38Z

Pinging @elastic/kibana-security (Team:Security)

legrego · 2021-05-03T12:08:46Z

@elasticmachine merge upstream

legrego · 2021-05-03T14:49:18Z

@elasticmachine merge upstream

azasypkin

Looks and works perfectly except for one weird thing: for some reason, I cannot select fields with the mouse clicks anymore, only with the keyboard (for Chromium and Firefox). I tried to re-bootstrap, but it didn't help. Do you see the same, or I just messed up with my environment somehow?

x-pack/test/api_integration/apis/security/index_fields.ts

x-pack/plugins/security/public/management/roles/indices_api_client.ts

azasypkin · 2021-05-04T06:53:03Z

x-pack/plugins/security/server/routes/indices/get_fields.ts

@@ -25,6 +25,7 @@ export function defineGetFieldsRoutes({ router }: RouteDefinitionParams) {
          fields: '*',
          allow_no_indices: false,
          include_defaults: true,
+          filter_path: '*.mappings.*.mapping.*.type',


Great find! 🏅

x-pack/plugins/security/public/management/roles/edit_role/privileges/es/index_privileges.tsx

azasypkin · 2021-05-04T07:28:40Z

...ck/plugins/security/public/management/roles/edit_role/privileges/es/index_privilege_form.tsx

+  // This is distinct from the field within `this.state`.
+  // We want to make sure that only one request for fields is in-flight at a time,
+  // and relying on state for this is error prone.
+  private isFieldListLoading: boolean = false;


question: just for my own education, would you mind explaining a bit when storing this flag only is state leads to errors?

React can decide to batch/defer state updates, so calling this.setState() doesn't guarantee that your state will be updated within a specific period of time. Having this private field outside of state works around this by giving us a synchronously updating field that always reflects reality.

…47378

watson

Do we need to backport this to 7.13.1. It seems more like a feature than a bug fix to me

...ck/plugins/security/public/management/roles/edit_role/privileges/es/index_privilege_form.tsx

legrego · 2021-05-04T13:44:43Z

Do we need to backport this to 7.13.1. It seems more like a feature than a bug fix to me

Not a strict requirement. We originally marked this as an enhancement, but recently decided to mark it as a bug due to the rather nasty side effects the current implementation has (OOM on the Kibana server, ES cluster instability, etc.).

Happy to wait for 7.14 if we're not comfortable with this scope of work being backported.

watson · 2021-05-04T13:57:38Z

[...] but recently decided to mark it as a bug due to the rather nasty side effects the current implementation has (OOM on the Kibana server, ES cluster instability, etc.).

Sounds reasonable - Just wanted to hear if it was intentional

…egrego/issue47378

legrego · 2021-05-04T14:03:20Z

Looks and works perfectly except for one weird thing: for some reason, I cannot select fields with the mouse clicks anymore, only with the keyboard (for Chromium and Firefox). I tried to re-bootstrap, but it didn't help. Do you see the same, or I just messed up with my environment somehow?

@azasypkin great catch! It looks like the onFocus callbacks I registered on the comboboxes were to blame. I removed them, and instead reverted to loading FLS suggestions on page load instead. This is less than ideal, but I'm hoping the other mitigations we've put in place will resolve the issues we've seen in the past.

kibanamachine · 2021-05-04T20:26:01Z

💚 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
`security`	764.3KB	764.9KB	+600.0B

History

💔 Build #124139 failed 8886467
💚 Build #123850 succeeded 1f49b6e
💔 Build #123819 failed 3fccd1e
💚 Build #123205 succeeded 8f1ae02
💔 Build #123088 failed 428d0b8

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

azasypkin

LGTM!

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-05-05T14:25:07Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Larry Gregory <larry.gregory@elastic.co>

legrego added 4 commits April 28, 2021 13:54

Add filter_path option to reduce size of response payload

52281ef

Only load FLS suggestions on-demand

428d0b8

Additional testing

d0aca90

Add API integration test for 404 condition

8f1ae02

legrego added release_note:fix Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.13.1 v7.14.0 v8.0.0 labels Apr 29, 2021

legrego marked this pull request as ready for review April 29, 2021 13:59

legrego requested a review from a team as a code owner April 29, 2021 13:59

legrego requested review from watson and azasypkin April 30, 2021 13:19

Merge branch 'master' into legrego/issue47378

3fccd1e

Merge branch 'master' into legrego/issue47378

1f49b6e

azasypkin reviewed May 4, 2021

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into legrego/issue…

9af7274

…47378

watson reviewed May 4, 2021

View reviewed changes

...ck/plugins/security/public/management/roles/edit_role/privileges/es/index_privilege_form.tsx Show resolved Hide resolved

legrego added 2 commits May 4, 2021 09:58

Address PR feedback

99fd55f

Merge branch 'legrego/issue47378' of github.com:legrego/kibana into l…

8886467

…egrego/issue47378

Fix unit test

ee310d3

legrego requested a review from azasypkin May 4, 2021 20:52

azasypkin approved these changes May 5, 2021

View reviewed changes

legrego added auto-backport Deprecated: Automatically backport this PR after it's merged and removed v7.13.1 labels May 5, 2021

legrego requested a review from watson May 5, 2021 14:05

watson approved these changes May 5, 2021

View reviewed changes

legrego merged commit 656206c into elastic:master May 5, 2021

legrego deleted the legrego/issue47378 branch May 5, 2021 14:22

kibanamachine mentioned this pull request May 5, 2021

[7.x] Load FLS suggestions on-demand (#98681) #99349

Merged

kibanamachine added a commit to kibanamachine/kibana that referenced this pull request May 5, 2021

Load FLS suggestions on-demand (elastic#98681)

46dc3ed

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine added a commit that referenced this pull request May 5, 2021

Load FLS suggestions on-demand (#98681) (#99349)

dca374c

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Larry Gregory <larry.gregory@elastic.co>

legrego mentioned this pull request Aug 16, 2021

Role with access to all indices kills Elasticsearch #108533

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load FLS suggestions on-demand #98681

Load FLS suggestions on-demand #98681

legrego commented Apr 28, 2021 •

edited

elasticmachine commented Apr 29, 2021

legrego commented May 3, 2021

legrego commented May 3, 2021

azasypkin left a comment

azasypkin May 4, 2021

azasypkin May 4, 2021

legrego May 4, 2021

watson left a comment

legrego commented May 4, 2021

watson commented May 4, 2021

legrego commented May 4, 2021

kibanamachine commented May 4, 2021

azasypkin left a comment

kibanamachine commented May 5, 2021

Load FLS suggestions on-demand #98681

Load FLS suggestions on-demand #98681

Conversation

legrego commented Apr 28, 2021 • edited

Summary

Fields suggestions are no longer eagerly loaded

The field mapping response has been filtered to only include the necessary fields from Elasticsearch

elasticmachine commented Apr 29, 2021

legrego commented May 3, 2021

legrego commented May 3, 2021

azasypkin left a comment

Choose a reason for hiding this comment

azasypkin May 4, 2021

Choose a reason for hiding this comment

azasypkin May 4, 2021

Choose a reason for hiding this comment

legrego May 4, 2021

Choose a reason for hiding this comment

watson left a comment

Choose a reason for hiding this comment

legrego commented May 4, 2021

watson commented May 4, 2021

legrego commented May 4, 2021

kibanamachine commented May 4, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

azasypkin left a comment

Choose a reason for hiding this comment

kibanamachine commented May 5, 2021

💚 Backport successful

legrego commented Apr 28, 2021 •

edited