Fix cluster default initialization #1

[willyborankin]

Description

Change the way how the security plugin initialize its configuration.
The current solution uses poll to upload the default security configuration from each node in the cluster. As result it could lead to the races during initialization of the security index.

To solve the problem with possible races the new solution uses cluster state and initializes the default configuration only on the management node and after that sends updated cluster state on all other nodes to load the configuration from the index.

Initialization using cluster state makes things much more simpler and security admin shell script can be removed in the future.

the cluster state for the security plugin is this:

"security": {
    "created": "2024-02-27T15:44:44.194028751Z",
    "configuration": {
      "actiongroups": {
        "hash": "219b41179866fc2419b736aff874a23f",
        "last_modified": null
      },
      "allowlist": {
        "hash": "522ce41bf5755eeb591d0161844830c0",
        "last_modified": null
      },
      "config": {
        "hash": "d97d664526b42ffa583dbb492742d3cc",
        "last_modified": null
      },
      "internalusers": {
        "hash": "f15fbfe131e0beb466313906d207f00b",
        "last_modified": null
      },
      "nodesdn": {
        "hash": "38e29064f142bfdf8632bc1a66cf8d00",
        "last_modified": null
      },
      "roles": {
        "hash": "db58ba06f214bfb53eee783378f5500c",
        "last_modified": null
      },
      "rolesmapping": {
        "hash": "f2b1cbdf73fa861f1321983cb0241964",
        "last_modified": null
      },
      "tenants": {
        "hash": "7942de6dc0f3bade63d8fc4b49d727d2",
        "last_modified": null
      },
      "whitelist": {
        "hash": "b73aadf21bca6a0de199121d982da460",
        "last_modified": null
      }
    }
  }

Next steps as a separate PRs:

• Security configuration update on the manager node only and wait for confirmation from other nodes that configuration successfully updated on them.
• Add migration from old version to new version with the cluster state support
• Change existing documentation to reflect changes here: https://github.com/opensearch-project/security/blob/main/ARCHITECTURE.md#security-configuration

Issues Resolved

• Resolves [FEATURE] Use LocalNodeClusterManagerListener::onClusterManager to initialize security index #3972
• Partially resolve [FEATURE] When internal security configuration is updated, its should be confirmed to be a new value #3275

Check List

• New functionality includes testing
• New functionality has been documented
• Commits are signed per the DCO using --signoff

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.

[willyborankin]

Hi @cliu123, @cwperks, @DarshitChanpura, @davidlago, @peternied , @RyanL1997, @scrawfor99 and @reta,
Lets discuss the new way of the sec plugin configuration initialization. There are test failures I think I know where is the problem. But I think it makes sense to start discussions. Thank you.

[codecov]

Codecov Report

Attention: Patch coverage is 87.62542% with 37 lines in your changes are missing coverage. Please review.

Project coverage is 66.23%. Comparing base (a731e62) to head (27791b4).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4002      +/-   ##
==========================================
+ Coverage   66.04%   66.23%   +0.18%     
==========================================
  Files         300      302       +2     
  Lines       21554    21698     +144     
  Branches     3488     3501      +13     
==========================================
+ Hits        14235    14371     +136     
- Misses       5571     5574       +3     
- Partials     1748     1753       +5     

Files	Coverage Δ
...g/opensearch/security/support/ConfigConstants.java	95.23% <ø> (ø)
.../org/opensearch/security/support/ConfigHelper.java	88.46% <ø> (ø)
.../opensearch/security/OpenSearchSecurityPlugin.java	84.61% <91.66%> (-0.14%)	⬇️
...ecurityconf/impl/SecurityDynamicConfiguration.java	79.52% <0.00%> (+0.95%)	⬆️
.../opensearch/security/support/YamlConfigReader.java	96.66% <96.66%> (ø)
...nsearch/security/support/SecurityIndexHandler.java	96.22% <96.22%> (ø)
.../org/opensearch/security/state/SecurityConfig.java	78.94% <78.94%> (ø)
...rg/opensearch/security/state/SecurityMetadata.java	78.37% <78.37%> (ø)
...ecurity/configuration/ConfigurationRepository.java	72.51% <81.33%> (+2.13%)	⬆️

... and 17 files with indirect coverage changes

[willyborankin]

now all tests passed

[peternied]

Nice work @willyborankin!

Overall comment, it looks like much of code you've added isn't getting registering code coverage hits - something might be wrong with the test execution, can you look into that?

[willyborankin]

Hi @peternied, thanks for your feedback. I will reorganize the code a bit. The way I implemented the index initialization is a bit wrong. The initialization needs to be done by the generic executor and changing of the cluster state via it. The management executor has a 20-second timeout, so it could lead to problems if the index initialization takes more than 20 seconds.

[willyborankin]

Hi @peternied, the latest version which includes:

• Adding of configuration now use bulk update
• Any node fails if it can't read/write configuration
• Addressed all comments you mentioned.

I think we can extend tests for the functionality I added. Lets discuss additional tests.

[willyborankin]

@peternied Looks like tests failures are note related. the new cluster permission appears:

cluster:monitor/term

[willyborankin]

@peternied now all tests passed

I'd like to see better coverage in of buildDynamicConfiguration before merging

[willyborankin]

@peternied I think coverage now around 99 or 98 % for the SecutyIndexHandler :-)

[willyborankin]

@peternied the final score is 87.6 which is good I think

[DarshitChanpura]

Changes look good to me! Thank you for this change and adding a comprehensive test suite @willyborankin!

[opensearch-trigger-bot]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-4002-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b0d26ddbfd584a76cb7eb48ee36c461fd0e9e19b
# Push it to GitHub
git push --set-upstream origin backport/backport-4002-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-4002-to-2.x.