Initialize api tokens index if it doesn't exist #4899
Conversation
Signed-off-by: Derek Ho <dxho@amazon.com>
derek-ho
requested review from
cwperks,
DarshitChanpura,
nibix,
peternied,
RyanL1997,
reta and
willyborankin
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/api-tokens #4899 +/- ##
===================================================
Coverage 71.43% 71.43%
===================================================
Files 334 334
Lines 22509 22511 +2
Branches 3582 3582
===================================================
+ Hits 16079 16081 +2
- Misses 4638 4639 +1
+ Partials 1792 1791 -1
|
Signed-off-by: Derek Ho <dxho@amazon.com>
derek-ho
requested review from
cwperks and
DarshitChanpura
and removed request for
cwperks and
DarshitChanpura
| public void createApiTokenIndex() { | ||
| try { | ||
| // wait for the cluster here until it will finish managed node election | ||
| while (clusterService.state().blocks().hasGlobalBlockWithStatus(RestStatus.SERVICE_UNAVAILABLE)) { |
There was a problem hiding this comment.
why do we need this block?
There was a problem hiding this comment.
This is necessary as without it we could try to create an index before the cluster is ready to handle this request. @DarshitChanpura @cwperks Do you folks think we still need this check though? As that check should already be performed by the creation of the opendistro_security index, depending on the value of "plugins.security.allow_default_init_securityindex" setting. In any case I was just taking inspiration from the creation of the opendistro_security index.
|
|
||
| try { | ||
| final ThreadContext threadContext = threadPool.getThreadContext(); | ||
| try (StoredContext ctx = threadContext.stashContext()) { |
There was a problem hiding this comment.
why do we need to explicitly stashContext here?
There was a problem hiding this comment.
Like the above, this is taken from the creation of opendistro_security index, which I assume has this to handle edge cases since the security index may not be initialized yet. Again not completely sure if its needed but I think it is fine?
src/test/java/org/opensearch/security/configuration/ConfigurationRepositoryTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java
Outdated
Show resolved
Hide resolved
…curity into init-index
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
| @@ -201,8 +202,8 @@ private void initalizeClusterConfiguration(final boolean installDefaultConfig) { | |||
| try (StoredContext ctx = threadContext.stashContext()) { | |||
| threadContext.putHeader(ConfigConstants.OPENDISTRO_SECURITY_CONF_REQUEST_HEADER, "true"); | |||
|
|
|||
| createSecurityIndexIfAbsent(); | |||
| waitForSecurityIndexToBeAtLeastYellow(); | |||
| createIndexIfAbsent(securityIndex); | |||
There was a problem hiding this comment.
I think this class should be reserved for initializing the security index. The security index will always need to be created on bootstrap.
For most plugins, I believe they create a system index when its needed if it doesn't exist instead of creating it on bootstrap. i.e. A user tries to issue an auth token and security tries to store the metadata, but the index doesn't exist so create it at that time.
See this class from AD plugin: https://github.com/opensearch-project/anomaly-detection/blob/main/src/main/java/org/opensearch/ad/indices/ADIndexManagement.java
There was a problem hiding this comment.
Sure this approach makes sense too. I also thought of this approach at first, but thought that it will add too much overhead to try and create this index for every request if it doesn't exist. However, we can limit this to index operations (create, revoke, etc.), and do not need to perform this for every authC request. I will try to do it this way instead.
|
Closing in favor of: https://github.com/opensearch-project/security/pull/4912/files |
Description
Initializes a new index for API tokens if it doesn't exist
Issues Resolved
Closes: #4900
Is this a backport? If so, please add backport PR # and/or commits #, and remove
backport-failedlabel from the original PR.Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end? If so, please open a draft PR in the security dashboards plugin and link the draft PR here
Testing
[Please provide details of testing done: unit testing, integration testing and manual testing]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.