[jira] [Comment Edited] (SOLR-16192) Add ZK credentials injectors

["Lamine (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/SOLR-16192?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17603238#comment-17603238 ] 

Lamine edited comment on SOLR-16192 at 9/12/22 7:43 PM:
--------------------------------------------------------

Hi [~dsmiley]  Updated the description.

There was an old JIRA before this PR got split into two "sub PRs". The second PR is in progress.


was (Author: JIRAUSER282315):
Hi [~dsmiley]  Updated the description.

There was an [old JIRA|https://issues.apache.org/jira/browse/SOLR-15857] before this PR got split into two "sub PRs".

> Add ZK credentials injectors
> ----------------------------
>
>                 Key: SOLR-16192
>                 URL: https://issues.apache.org/jira/browse/SOLR-16192
>             Project: Solr
>          Issue Type: Improvement
>            Reporter: Lamine
>            Priority: Minor
>          Time Spent: 7h
>  Remaining Estimate: 0h
>
> 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.
> h2. 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 _VMParamsSingleSetCredentialsDigestZkCredentialsProvider_ 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.
> h2. 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.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org