[jira] [Comment Edited] (KNOX-1187) Distributed Alias Service

["Phil Zampino (JIRA)" <ji...@apache.org>]

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

Phil Zampino edited comment on KNOX-1187 at 4/2/18 3:33 PM:
------------------------------------------------------------

[~smore]
*RemoteAliasService*
* ctor takes AliasService, but expects/requires it to be DefaultAliasService?
** Should take DefaultAliasService explicitly if that is what is required/expected
* defaultAlias should be named defaultAliasService ?
* buildAliasName() --> buildAliasZNodeName() ?
* buildClusterName() --> buildClusterZNodeName() ?
* getAliasesForCluster
** Does the implementation detail explanation belong in the javadoc?
** defaultAlias.getAliasesForCluster never returns null
** defaultAlias.getAliasesForCluster is invoked before and potentially after querying the remote aliases; why?
** This method structure could be simplified
* checkPathExists(RemoteConfigurationRegistryClient) can be static?
* addAliasForCluster, removeAliasForCluster:
** Is it better to invoke buildAliasName() once, and resuse the result, rather than invoking it multiple times with the same args?
* The service depends on the RemoteConfigurationRegistryClient, which is not ZK-specific, but some of your comments, exceptions and error messages are ZK-specific.
** Even though the only RemoteConfigurationRegistryClient implementation is ZK-specific today, that may not be the case in th future, and this is the motivation for the abstraction. If using the abstraction, we should make as few assumptions as possible about the actual implementation.
* generateAliasForCluster(String clusterName, String alias)
* getPasswordFromAliasForCluster(String clusterName, String alias, boolean generate)
** final else doesn't need a check for password == null, since the method already returns if password had been non-null
* getCertificateForGateway: typo in comment (deligate)
* You have methods xxxAliasForClusterLocally(), which you don't use internally; Should you?
* Do RemoteAliasChildListener and RemoteAliasEntryListener need to be public?
* RemoteAliasChildListener#childEvent(), RemoteAliasEntryListener#entryChanged():
** Is it safe to assume that GatewayServer.getGatewayServices().getService(GatewayServices.ALIAS_SERVICE) returns a RemoteAliasService
***Should the type be verified prior to casting?

*DefaultRemoteConfigurationMonitor*
* Why is this monitor listening for remote aliases? This smells bad to me.
** RemoteAliasService#start() should register the remote alias listener

*EncryptionAlgorithm*
* Is there a reason for the license comment to be placed between the imports and the class declaration, rather than at the beginning of the file content?
* What is the need for this new class?
* Unit tests?

*ZKAliasServiceTest*
* Why tearDownSuite() does not delete PATH_KNOX_SECURITY instead of only PATH_KNOX_ALIAS_STORE_TOPOLOGY?

 *No CLI yet?* This service is +unusable+ without the CLI commands.



was (Author: pzampino):
[~smore]
*RemoteAliasService*
* ctor takes AliasService, but expects/requires it to be DefaultAliasService?
** Should take DefaultAliasService explicitly if that is what is required/expected
* defaultAlias should be named defaultAliasService ?
* buildAliasName() --> buildAliasZNodeName() ?
* buildClusterName() --> buildClusterZNodeName() ?
* getAliasesForCluster
** Does the implementation detail explanation belong in the javadoc?
** defaultAlias.getAliasesForCluster never returns null
** defaultAlias.getAliasesForCluster is invoked before and potentially after querying the remote aliases; why?
** This method structure could be simplified
* checkPathExists(RemoteConfigurationRegistryClient) can be static?
* addAliasForCluster, removeAliasForCluster:
** Is it better to invoke buildAliasName() once, and resuse the result, rather than invoking it multiple times with the same args?
* The service depends on the RemoteConfigurationRegistryClient, which is not ZK-specific, but some of your comments, exceptions and error messages are ZK-specific.
** Even though the only RemoteConfigurationRegistryClient implementation is ZK-specific today, that may not be the case in th future, and this is the motivation for the abstraction. If using the abstraction, we should make as few assumptions as possible about the actual implementation.
* generateAliasForCluster(String clusterName, String alias)
** Should it delegate to this.defaultAlias.generatePassword(), rather than invoking DefaultAliasService.generatePassword() statically?
* getPasswordFromAliasForCluster(String clusterName, String alias, boolean generate)
** final else doesn't need a check for password == null, since the method already returns if password had been non-null
* getCertificateForGateway: typo in comment (deligate)
* You have methods xxxAliasForClusterLocally(), which you don't use internally; Should you?
* Do RemoteAliasChildListener and RemoteAliasEntryListener need to be public?
* RemoteAliasChildListener#childEvent(), RemoteAliasEntryListener#entryChanged():
** Is it safe to assume that GatewayServer.getGatewayServices().getService(GatewayServices.ALIAS_SERVICE) returns a RemoteAliasService
***Should the type be verified prior to casting?

*DefaultRemoteConfigurationMonitor*
* Why is this monitor listening for remote aliases? This smells bad to me.
** RemoteAliasService#start() should register the remote alias listener

*EncryptionAlgorithm*
* Is there a reason for the license comment to be placed between the imports and the class declaration, rather than at the beginning of the file content?
* What is the need for this new class?
* Unit tests?

*ZKAliasServiceTest*
* Why tearDownSuite() does not delete PATH_KNOX_SECURITY instead of only PATH_KNOX_ALIAS_STORE_TOPOLOGY?

 *No CLI yet?* This service is +unusable+ without the CLI commands.


> Distributed Alias Service
> -------------------------
>
>                 Key: KNOX-1187
>                 URL: https://issues.apache.org/jira/browse/KNOX-1187
>             Project: Apache Knox
>          Issue Type: Bug
>          Components: Server
>    Affects Versions: 0.14.0, 1.0.0
>            Reporter: Phil Zampino
>            Assignee: Sandeep More
>            Priority: Major
>             Fix For: 1.1.0
>
>         Attachments: KNOX-1187.001.patch
>
>
> Given the ability to manage provider configurations and descriptors in ZooKeeper, it would also be good to employ ZooKeeper for managing aliases since descriptors reference them for discovery authentication.
> The benefits of ZooKeeper-managed descriptors is limited by the current need to individually define the associated aliases at each and every Knox instance. Any Knox instance for which the referenced alias has not been defined will fail to generate/deploy the topology because service discovery will fail.
> The resolution of this issue will provide a Knox administrator the ability to define aliases in ZooKeeper, which will be consumed and applied by any Knox instance configured to monitor that same ZooKeeper, similar to the way provider configurations and descriptors are supported.
> In fact, the alias-related CLI commands could leverage the remote configuration monitor config to determine whether the aliases should be persisted to / read from ZooKeeper or locally. Knox could use the remote configuration client service to monitor the remote alias configuration, and apply changes locally.
> This will also require some kind of coordination of Knox master secrets; at a minimum, each participating Knox instance will have to have been configured with the same master secret.
>  



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)