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

Only allow TransportConfigUpdateAction after node has been set as bootstrapped #13

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

cwperks
Copy link
Owner

@cwperks cwperks commented Dec 6, 2023

Introduces another variable on DynamicConfigFactory called bootstrapped that behaves differently than the initialized variable. bootstrapped is a flag that signals to TransportConfigUpdateAction that it can start accepting updates.

There are 2 ways the security index can be created from scratch:

  1. If plugins.security.allow_default_init_securityindex is set to true it will create the security index and load all yaml files
  2. If plugins.security.allow_default_init_securityindex is set to false, the security index is not created on bootstrap and requires a user to run securityadmin to initialize security. When securityadmin is utilized, the cluster does depend on TransportConfigUpdateAction to initialize security so there still needs to be an avenue where this can update the config before initialized is set to true

This PR sets bootstrapped to false on node startup and explicitly sets it to true once its ok for TransportConfigUpdateAction to start accepting transport actions. In case 2) up above, it can be set to true before DynamicConfigFactory is initialized so that it can accept requests from securityadmin.

@codecov-commenter
Copy link

codecov-commenter commented Dec 8, 2023

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (7f1febf) 66.57% compared to head (b3b6de7) 65.19%.
Report is 5 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #13      +/-   ##
==========================================
- Coverage   66.57%   65.19%   -1.38%     
==========================================
  Files         298      298              
  Lines       21189    21217      +28     
  Branches     3454     3460       +6     
==========================================
- Hits        14107    13833     -274     
- Misses       5364     5667     +303     
+ Partials     1718     1717       -1     
Files Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java 84.48% <100.00%> (+0.04%) ⬆️
...ch/security/securityconf/DynamicConfigFactory.java 55.62% <ø> (ø)
...g/opensearch/security/support/ConfigConstants.java 95.23% <ø> (ø)
...tion/configupdate/TransportConfigUpdateAction.java 95.45% <66.66%> (-4.55%) ⬇️
...ecurity/configuration/ConfigurationRepository.java 73.03% <70.37%> (-5.88%) ⬇️

... and 37 files with indirect coverage changes

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants