Migrate privilege/role/user-related operations to a new Elasticsearch client. #84641
Conversation
azasypkin
added
chore
Feature:New Platform
Team:Security
azasypkin
force-pushed
the
issue-xxx-new-es-client-2
branch
from
f0b6ff0 to
2104689
Compare
azasypkin
changed the title
azasypkin
force-pushed
the
issue-xxx-new-es-client-2
branch
from
2104689 to
9c981cd
Compare
|
Pinging @elastic/kibana-security (Team:Security) |
azasypkin
added
the
release_note:skip
azasypkin
changed the title
| @@ -47,9 +47,9 @@ export function checkPrivilegesWithRequestFactory( | |||
| : []; | |||
| const allApplicationPrivileges = uniq([actions.version, actions.login, ...kibanaPrivileges]); | |||
|
|
|||
| const hasPrivilegesResponse = (await clusterClient | |||
| const { body: hasPrivilegesResponse } = await (await getClusterClient()) | |||
There was a problem hiding this comment.
nit: I would try and avoid double await as it's quite hard to read and understand what's going on.
| const startServicesPromise = core.getStartServices().then(([coreServices, depsServices]) => ({ | ||
| elasticsearch: coreServices.elasticsearch, | ||
| features: depsServices.features, | ||
| })); |
There was a problem hiding this comment.
I have no idea yet how we're going to solve this but I had to do the same for audit logging and it's a bit of an architectural smell. Lifecycles should flow in one direction. A lifecycle can certainly have access to a previous phase but not the other way around. If we need access to a start contract in the setup phase then either the setup contract we're depending on isn't exposing everything it should or the lifecycles themselves aren't really separate lifecycles.
Would love to start thinking about improving this. (Obviously not as part of this PR)
@elastic/kibana-core Are you guys aware this is happening throughout the codebase? Any ideas or plans how this situation could be avoided in the future?
There was a problem hiding this comment.
it's a bit of an architectural smell.
It's! And one of the main reasons why we still use this "trick", in Security at least, is that we need all of that stuff in the HTTP routes and we can only register them at the setup stage right now. Would definitely love to get rid of this.
There was a problem hiding this comment.
Are you guys aware this is happening throughout the codebase?
We are.
Any ideas or plans how this situation could be avoided in the future?
Using context-based wrapper around all APIs that might require start contract when registered in setup would be a solution. (such as what is done with RouteHandlerContext for http handler for example). This is some heavy code restructuring though, and is not planned atm.
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
|
7.x/7.11.0: b710b81 |
…fields * 'master' of github.com:elastic/kibana: [ILM] Fix delete phase serialization bug (elastic#84870) chore(NA): removes auto install of pre-commit hook (elastic#83566) chore(NA): upgrade node-sass into last v4.14.1 to stop shipping old node-gyp (elastic#84935) [Alerting] Enables AlertTypes to define the custom recovery action groups (elastic#84408) [ML] Functional tests - add missing test data cleanup (elastic#84998) Migrate privilege/role/user-related operations to a new Elasticsearch client. (elastic#84641) # Conflicts: # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/deserializer_and_serializer.test.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/serializer/serializer.ts
We should eventually remove all dependencies on the Legacy ES Client, and in this PR I migrated all privilege/role/user-related operations to use a new ES client.
Related to: #83910