SOLR-16192: Add ZK credentials injectors support

[laminelam]

https://issues.apache.org/jira/browse/SOLR-16192 https://issues.apache.org/jira/browse/SOLR-15857

Splitting this PR into 2 PRs (keeping the "old" one for now for little bit more context).

TLTR;  This PR adds ZkCredentialsInjector support. What is a ZkCredentialsInjector? It’s a class (interface) that loads ZK/Solr creds from an external source and injects them into ZkACLProvider/ZkCredentialsProvider. This PR uses a Strategy pattern approach where the credentials are injected as a dependency. This allows decoupling the process of reading the creds from the one using them (Single responsibility).

Benefits:

• No more code duplication.
• Adding a new ZK credentials source is as easy as adding a new ZkCredentialsInjector implementation (one class, instead of two).
• Classes implementing ZkCredentialsInjector are completely encapsulated. Therefore, the implementation can be changed without affecting the existing ZkACLProvider/ZkCredntialsProvider classes.
• Different ZkCredentialsInjector can be used interchangeably to alter the creds source without changing the whole architecture.
• Run time interchangeability (think of those situations where you have a client that has to hit two different clusters).
• Backward compatible. Existing and custom providers can still work as we are not breaking the existing ZkACLProvider/ZkCredntialsProvider interfaces.

Problems with the current design

The same code to load the credentials is called twice, first by ZkCredentialsProvider to connect to Zookeeper and a second time by ZkACLProvider to create ACLs. The code is also duplicated, although it's only reading from system props.


Adding a custom pair of ZkCredentialsProvider/ZkACLProvider to load the credentials from another source (ex a Secret Manager) would require duplicating the code and making repetitive calls to extract the same credentials.

The purpose of this PR is to allow retrieving Solr/ZL creds from any source before injecting them into ZK creds/acl providers without duplicating the existing code.

Without this new feature, adding custom ZK credentials provider requires:

1 - Either, duplicating VMParamsAllAndReadonlyDigestZkACLProvider and VMParamsAllAndReadonlyDigestZkACLProvider code. That means a new pair of classes, 90% identical to the existing classes, for every ZK credentials provider. Adding another ZK credentials provider? Add 2 more duplicate classes…

2 - Or, creating a super abstract class and use inheritance to reuse the code. Still, the code to load the creds would be duplicated and executed twice (two calls to load the same creds from the same source)

I think 1) is not an option. For 2) I believe this is one of those situations where composition should be favored over inheritance.

Proposed solution

• Refactor the way how the credentials are injected by passing them as a dependency. One code, called once and injected into the client class. Here the client classes are ZkCredentialsProvider and ZkACLProvider.

• Favor composition over inheritance to inject custom credentials loaders without changing the composing (container) class.

• Add a third interface ZkCredentialsInjector whose implementations load ZK credentials from a credentials source to be injected into ZkCredentialsProvider and ZkACLProvider

• The dataflow is: Credentials source —> ZkCredentialsInjector —> ZkCredentialsProvider/ZkACLProvider —> Zookeeper

• The ZkCredentialsInjector gets the creds from an external source which get injected into zkCredentialsProvider and zkACLProvider. The "external source" here can be system props, a file, a Secret Manager, or any other local or remote source.

public interface ZkCredentialsInjector { 
    List<ZkCredential> getZkCredentials();
    ...
}

• Any class implementing ZkCredentialsInjector can be injected via system props in solr.ini.sh/cmd and solr.xml.

In the below example VMParamsZkCredentialsInjector is injected. Note: VMParamsAllAndReadonlyDigestZkACLProvider and VMParamsSingleSetCredentialsDigestZkCredentialsProvider would be deprecated and replaced with a combination of DigestZkACLProvider/DigestZkCredentialsProvider and VMParamsZkCredentialsInjector.

  SOLR_ZK_CREDS_AND_ACLS=“
     -DzkACLProvider=org.apache.solr.common.cloud.acl.DigestZkACLProvider \
     -DzkCredentialsProvider=org.apache.solr.common.cloud.acl.DigestZkCredentialsProvider \
     -DzkCredentialsInjector=org.apache.solr.common.cloud.acl.VMParamsZkCredentialsInjector \
     -DzkDigestUsername=admin-user -DzkDigestPassword=CHANGEME-ADMIN-PASSWORD \
     -DzkDigestReadonlyUsername=readonly-user -DzkDigestReadonlyPassword=CHANGEME-READONLY-PASSWORD"
 SOLR_OPTS="$SOLR_OPTS $SOLR_ZK_CREDS_AND_ACLS"

• Add DigestZkACLProvider/DigestZkCredentialsProvider classes to support Digest based scheme ZK authentication/authorization

Class DigestZkACLProvider implements ZkACLProvider{
	CredentialsInjector credentialsInjector;
	...
}

Class DigestZkCredentialsProvider implements ZkCredentialsProvider{
	CredentialsInjector credentialsInjector;
	...
}

This concept can be generalized to non-digest schemes (a kind of Strategy pattern) but that would require more refactoring, it can be achieved in a future contribution if this one is accepted.

Thank you in advance for your time and your comments.

Checklist

Please review the following and check all that apply:

• [x] I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
• [x] I have created a Jira issue and added the issue ID to my pull request title.
• [x] I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
• [x] I have developed this patch against the main branch.
• [x] I have run ./gradlew check.
• [x] I have added tests for my changes.
• [] I have added documentation for the Reference Guide

[laminelam]

Thank you @madrob for your time reviewing this. I have pushed a new commit and left some answers to your comments. Will add the ref guide as soon as this is accepted.

[laminelam]

I think we need to have a bigger discussion around backwards compatibility and how we achieve that.

I would prefer to keep everything nice and neat, instead of spreading classes across packages. Also this will impact whether we have to keep the old VMParamsAllAnd.....Provider classes around, which hopefully we will not have to.

From "outside" perspective, we are only changing this

-DzkACLProvider=org.apache.solr.common.cloud.VMParamsAllAndReadonlyDigestZkACLProvider 
-DzkCredentialsProvider=org.apache.solr.common.cloud.VMParamsSingleSetCredentialsDigestZkCredentialsProvider

to this

-DzkACLProvider=org.apache.solr.common.cloud.acl.DigestZkACLProvider 
-DzkCredentialsProvider=org.apache.solr.common.cloud.acl.DigestZkCredentialsProvider 
-DzkCredentialsInjector=org.apache.solr.common.cloud.acl.VMParamsZkCredentialsInjector

The existing configs (solr.ini.* and zkcli.* as well as solr.xml) using the old VM params will still work as the existing classes (VMParamsAllAndReadonlyDigestZkACLProvider and VMParamsSingleSetCredentialsDigestZkCredentialsProvider) are not removed (only deprecated).

[laminelam]

Hi @HoustonPutman, as discussed, removed the acl package and added the ref guide, plus other minor changes.

[laminelam]

Thank you @madrob and @HoustonPutman for your review. Pushed the changes.

[madrob]

It looks like something went wrong, maybe this branch is out of date? Can you check that it is current, I wanted to give it a final pass review but am not able to do that.

[HoustonPutman]

@laminelam I'm not sure what the merge commit is, but you need to remove it and re-do the merge, like the second merge commit. Otherwise we can't tell which files this PR is actually touching.