Security: Remove SecurityLifecycleService #30526
Conversation
This commit removes the SecurityLifecycleService, relegating its former functions of listening for cluster state updates to SecurityIndexManager and IndexAuditTrail.
rjernst
added
v7.0.0
>refactoring
:Security/Security
|
Pinging @elastic/es-security |
|
|
||
| } | ||
|
|
||
| public State state() { | ||
| return state.get(); | ||
| } | ||
|
|
||
| @Override | ||
| public void clusterChanged(ClusterChangedEvent event) { |
There was a problem hiding this comment.
Earlier in the SecurityLifeCycleService the cluster changed event was handled in order - first, to handle the event in SecurityIndexManager later it would start the index audit trail. Is there any order defined which does not change the behavior of handling cluster event? Not sure if this would be of any concern.
There was a problem hiding this comment.
The order should not matter. The services have no interdependencies.
| @Override | ||
| public void clusterChanged(ClusterChangedEvent event) { | ||
| try { | ||
| if (state() == IndexAuditTrail.State.INITIALIZED && canStart(event)) { |
There was a problem hiding this comment.
We have removed the condition Security.indexAuditLoggingEnabled(settings), is the check performed somewhere else or not required anymore?
There was a problem hiding this comment.
The IndexAuditTrail object is only created if that method returns true, so it is not necessary to check within the instance.
|
Hi @rjernst, Could you please add some background info that would help understand the problem that this PR solves? I could not locate any issue/defect to understand more on the problem statement. Thank you. |
This PR is moving towards being able to wait during node startup on additional plugin provided services to be "ready". This will enable, for example, removing the waitCondition xpack tests have in gradle, because we will know when the ports file written by node startup is written that xpack is ready to service requests (eg security index is ready). These wait conditions will happen in a followup PR; this PR is only a precursor in separating out the existing services so that each one can have code to say "wait on this being ready". |
| @@ -261,6 +263,8 @@ | |||
| private final SetOnce<ThreadContext> threadContext = new SetOnce<>(); | |||
| private final SetOnce<TokenService> tokenService = new SetOnce<>(); | |||
| private final SetOnce<SecurityActionFilter> securityActionFilter = new SetOnce<>(); | |||
| private final SetOnce<SecurityIndexManager> securityIndex = new SetOnce<>(); | |||
There was a problem hiding this comment.
this is a nit and was there before this change, but maybe we should call this securityIndexManager everywhere instead of securityIndex
There was a problem hiding this comment.
I actually like securityIndex, since the methods all reflect the state of the index, not the manager itself.
| @@ -261,6 +263,8 @@ | |||
| private final SetOnce<ThreadContext> threadContext = new SetOnce<>(); | |||
| private final SetOnce<TokenService> tokenService = new SetOnce<>(); | |||
| private final SetOnce<SecurityActionFilter> securityActionFilter = new SetOnce<>(); | |||
| private final SetOnce<SecurityIndexManager> securityIndex = new SetOnce<>(); | |||
| private final SetOnce<IndexAuditTrail> indexAuditTrail = new SetOnce<>(); | |||
There was a problem hiding this comment.
also why are we using a setonce for these? It looks like we only use them in the createComponents method currently?
There was a problem hiding this comment.
These will be used in a followup, to wait on their readiness in onNodeStarted()
This commit removes the SecurityLifecycleService, relegating its former functions of listening for cluster state updates to SecurityIndexManager and IndexAuditTrail.
* es/6.x: (44 commits) SQL: Remove dependency for server's version from JDBC driver (#30631) Make xpack modules instead of a meta plugin (#30589) Security: Remove SecurityLifecycleService (#30526) Build: Add task interdependencies for ssl configuration (#30633) Mute ShrinkIndexIT [ML] DeleteExpiredDataAction should use client with origin (#30646) Reindex: Fixed typo in assertion failure message (#30619) [DOCS] Fixes list of unconverted snippets in build.gradle Use readFully() to read bytes from CipherInputStream (#30640) Add Create Repository High Level REST API (#30501) [DOCS] Reorganizes RBAC documentation Test: increase search logging for LicensingTests Delay _uid field data deprecation warning (#30651) Deprecate Empty Templates (#30194) Remove unused DirectoryUtils class. (#30582) Mitigate date histogram slowdowns with non-fixed timezones. (#30534) [TEST] Remove AwaitsFix in IndicesOptionsTests#testSerialization S3 repo plugin populates SettingsFilter (#30652) Rest High Level client: Add List Tasks (#29546) Fixes IndiceOptionsTests to serialise correctly (#30644) ...
This commit removes the SecurityLifecycleService, relegating its former
functions of listening for cluster state updates to SecurityIndexManager
and IndexAuditTrail.