[
https://issues.apache.org/jira/browse/KNOX-1187?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16422727#comment-16422727
]
Sandeep More commented on KNOX-1187:
------------------------------------
Thanks a detailed and thorough review [~pzampino] ! Following are my comments,
I will update my patch with suggested changes.
*RemoteAliasService*
constructor takes AliasService, but expects/requires it to be
DefaultAliasService?
SRM: I wanted to decouple DefaultAliasService from RemoteAliasService, if in
the future we decide to update the implementation it would be easy to update.
defaultAlias should be named defaultAliasService ?
SRM: For the reason listed above I am using defaultAlias
buildAliasName() --> buildAliasZNodeName() ?
SRM: OK
buildClusterName() --> buildClusterZNodeName() ?
SRM: OK
getAliasesForCluster
Does the implementation detail explanation belong in the javadoc?
SRM: Correct, I will get rid of the implementation details.
defaultAlias.getAliasesForCluster never returns null
SRM: DefaultAliasService does not, other implementation might (in the
future), hence the null checks.
defaultAlias.getAliasesForCluster is invoked before and potentially after
querying the remote aliases; why?
SRM: You are right, this does not look correct, this is the remnant of the
local cache implementation that I later took out, sorry about that.
This method structure could be simplified
SRM: Agreed
checkPathExists(RemoteConfigurationRegistryClient) can be static?
SRM: yes, thanks!
addAliasForCluster, removeAliasForCluster:
Is it better to invoke buildAliasName() once, and resuse the result,
rather than invoking it multiple times with the same args?
SRM: We could, at the beginning of the method.
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 the 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.
SRM: Agreed
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
SRM: Ok
getCertificateForGateway: typo in comment (deligate)
SRM: :(
You have methods xxxAliasForClusterLocally(), which you don't use internally;
Should you?
SRM: It get's called by RemoteAliasChildListener
Do RemoteAliasChildListener and RemoteAliasEntryListener need to be public?
SRM: They need to be public since I am adding them to
DefaultRemoteConfigurationMonitor, and I would like to keep all the code
related to Remote Alias in one place.
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?
SRM: I thought I had a type check there, sorry about that, will add a
type check.
*DefaultRemoteConfigurationMonitor*
Why is this monitor listening for remote aliases? This smells bad to me.
RemoteAliasService#start() should register the remote alias listener
SRM: I was planning on putting all the monitors in one place which was the
reason for adding it remote aliases there, I don't mind moving it to
RemoteAliasService#start().
*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?
SRM: I use a template that auto-generates the template, not sure why IntelliJ
decides to inset it there, I'll update it.
What is the need for this new class?
SRM: a) I wanted to move as much security stuff away from RemoteAliasService as
I can b) This class can be used by other Knox classes
Unit tests?
SRM: yes, I need to add UnitTests for CLI and Listener
*ZKAliasServiceTest*
Why tearDownSuite() does not delete PATH_KNOX_SECURITY instead of only
PATH_KNOX_ALIAS_STORE_TOPOLOGY?
SRM: Correct, I'll update it.
*No CLI yet? This service is unusable without the CLI commands.*
SRM: We can just use the existing CLI commands to add/remove the alias. The
decision to use remote service will depend on whether remote service is
configured on Knox or not.
> 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)
