You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by GitBox <gi...@apache.org> on 2020/07/01 19:34:17 UTC
[GitHub] [knox] smolnar82 opened a new pull request #358: [WIP] KNOX-2395 - Make Gateway Services Pluggable
smolnar82 opened a new pull request #358:
URL: https://github.com/apache/knox/pull/358
THIS IS STILL IN WIP STAGE: I'm working on the unit tests (they have purposely missed out from this commit) as well as adding logs.
I wanted to create this PR so that you guys can give me early feedback.
## What changes were proposed in this pull request?
From now on end-users can select different implementation for certain Knox services (where there are more than one implementations) through a newly introduced `gateway-site.xml` configuration called `gateway.services`.
The format of this configuration is: `service1:impl1[, services:impl2,...]`
`DefaultGatewayServices` and `CLIGatewayServices` have been modified to use the new service factories.
## How was this patch tested?
Ran the `knoxcli` tool:
- created the master secret
- added/listed some aliases
Started the Knox Gateway:
- checked the home page
- logged into the Admin UI
- modified the 'sandbox' topology
- modified a service definition (one that was listed in `sandbox`, confirmed that it got redeployed)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on pull request #358: KNOX-2395 - Make Gateway Services Pluggable
Posted by GitBox <gi...@apache.org>.
pzampino commented on pull request #358:
URL: https://github.com/apache/knox/pull/358#issuecomment-656163750
> > It still seems that the possible implementations are constrained to some set already known by Knox. Why can't a developer "plug-in" an arbitrary implementation of any gateway service to suit their individual need?
>
> Hm..this is a good idea. I haven't thought about it until now. It'd also mean that the developer will need to make sure the implementation is on the classpath.
> Let me ad it to this PR.
Yes, the implementation being plugged-in would obviously have to be on the classpath. The **ext** dir in a Knox installation is intended for such a purpose.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a change in pull request #358: KNOX-2395 - Make Gateway Services Pluggable
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #358:
URL: https://github.com/apache/knox/pull/358#discussion_r452154403
##########
File path: gateway-release/home/conf/gateway-site.xml
##########
@@ -18,6 +18,11 @@ limitations under the License.
-->
<configuration>
+ <property>
+ <name>gateway.services</name>
Review comment:
Fixed them all. Thanks again for the review, @pzampino !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #358: KNOX-2395 - Make Gateway Services Pluggable
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #358:
URL: https://github.com/apache/knox/pull/358#discussion_r451614042
##########
File path: gateway-release/home/conf/gateway-site.xml
##########
@@ -18,6 +18,11 @@ limitations under the License.
-->
<configuration>
+ <property>
+ <name>gateway.services</name>
Review comment:
I do like the use of the AbstractFactory pattern here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a change in pull request #358: KNOX-2395 - Make Gateway Services Pluggable
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #358:
URL: https://github.com/apache/knox/pull/358#discussion_r453374943
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/factory/RemoteRegistryClientServiceFactory.java
##########
@@ -26,20 +26,26 @@
import org.apache.knox.gateway.services.ServiceLifecycleException;
import org.apache.knox.gateway.services.ServiceType;
import org.apache.knox.gateway.services.config.client.RemoteConfigurationRegistryClientService;
+import org.apache.knox.gateway.services.security.AliasService;
// There are two implementations of 'RemoteConfigurationRegistryClientService':
// - LocalFileSystemRemoteConfigurationRegistryClientService
// - CuratorClientService
// However, the first one - local - is for test-only purposes
public class RemoteRegistryClientServiceFactory extends AbstractServiceFactory {
+ private final AliasServiceFactory aliasServiceFactory = new AliasServiceFactory();
+
@Override
public Service create(GatewayServices gatewayServices, ServiceType serviceType, GatewayConfig gatewayConfig, Map<String, String> options, String implementation)
Review comment:
Fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino edited a comment on pull request #358: KNOX-2395 - Make Gateway Services Pluggable
Posted by GitBox <gi...@apache.org>.
pzampino edited a comment on pull request #358:
URL: https://github.com/apache/knox/pull/358#issuecomment-656163750
> > It still seems that the possible implementations are constrained to some set already known by Knox. Why can't a developer "plug-in" an arbitrary implementation of any gateway service to suit their individual need?
>
> Hm..this is a good idea. I haven't thought about it until now. It'd also mean that the developer will need to make sure the implementation is on the classpath.
> Let me ad it to this PR.
Yes, the implementation being plugged-in would obviously have to be on the classpath. The **ext** dir in a Knox installation is intended for such a purpose. In fact the README in that directory states, "THIS DIRECTORY IS WHERE JARS AND CLASSES CONTAINING CUSTOM EXTENSIONS CAN BE PLACED".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #358: KNOX-2395 - Make Gateway Services Pluggable
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #358:
URL: https://github.com/apache/knox/pull/358#discussion_r451717422
##########
File path: gateway-release/home/conf/gateway-site.xml
##########
@@ -18,6 +18,11 @@ limitations under the License.
-->
<configuration>
+ <property>
+ <name>gateway.services</name>
Review comment:
I am strongly in favor of your suggestion. I believe it's a more flexible pattern than list-value properties. My examples had originally included `gateway.service.$SERVICE.impl` as property names, but I modified them for simplicity, since it was only an example, and not the primary point of my comment.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a change in pull request #358: KNOX-2395 - Make Gateway Services Pluggable
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #358:
URL: https://github.com/apache/knox/pull/358#discussion_r452246927
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/factory/RemoteRegistryClientServiceFactory.java
##########
@@ -26,20 +26,26 @@
import org.apache.knox.gateway.services.ServiceLifecycleException;
import org.apache.knox.gateway.services.ServiceType;
import org.apache.knox.gateway.services.config.client.RemoteConfigurationRegistryClientService;
+import org.apache.knox.gateway.services.security.AliasService;
// There are two implementations of 'RemoteConfigurationRegistryClientService':
// - LocalFileSystemRemoteConfigurationRegistryClientService
// - CuratorClientService
// However, the first one - local - is for test-only purposes
public class RemoteRegistryClientServiceFactory extends AbstractServiceFactory {
+ private final AliasServiceFactory aliasServiceFactory = new AliasServiceFactory();
+
@Override
public Service create(GatewayServices gatewayServices, ServiceType serviceType, GatewayConfig gatewayConfig, Map<String, String> options, String implementation)
Review comment:
I'm not sure if I understood your comment here. Could you please elaborate?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #358: KNOX-2395 - Make Gateway Services Pluggable
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #358:
URL: https://github.com/apache/knox/pull/358#discussion_r452234789
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/factory/AliasServiceFactory.java
##########
@@ -44,22 +41,17 @@ public Service create(GatewayServices gatewayServices, ServiceType serviceType,
final AliasService defaultAliasService = new DefaultAliasService();
((DefaultAliasService) defaultAliasService).setMasterService(getMasterService(gatewayServices));
((DefaultAliasService) defaultAliasService).setKeystoreService(getKeystoreService(gatewayServices));
- defaultAliasService.init(gatewayConfig, options); //invoking init on DefaultAliasService twice is ok (in case implementation is set to 'default')
- switch (implementation) {
- case DEFAULT_IMPLEMENTATION_NAME:
- case "":
+ defaultAliasService.init(gatewayConfig, options); // invoking init on DefaultAliasService twice is ok (in case implementation is set to 'default')
+
+ if (matchesImplementation(implementation, DefaultAliasService.class, true)) {
Review comment:
Why is the DefaultAliasService always created and initialized?
Could this not be done only when there is no implementation, or if the specified implementation is the default one?
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/factory/RemoteRegistryClientServiceFactory.java
##########
@@ -26,20 +26,26 @@
import org.apache.knox.gateway.services.ServiceLifecycleException;
import org.apache.knox.gateway.services.ServiceType;
import org.apache.knox.gateway.services.config.client.RemoteConfigurationRegistryClientService;
+import org.apache.knox.gateway.services.security.AliasService;
// There are two implementations of 'RemoteConfigurationRegistryClientService':
// - LocalFileSystemRemoteConfigurationRegistryClientService
// - CuratorClientService
// However, the first one - local - is for test-only purposes
public class RemoteRegistryClientServiceFactory extends AbstractServiceFactory {
+ private final AliasServiceFactory aliasServiceFactory = new AliasServiceFactory();
+
@Override
public Service create(GatewayServices gatewayServices, ServiceType serviceType, GatewayConfig gatewayConfig, Map<String, String> options, String implementation)
Review comment:
Can this implementation not be overridden?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 merged pull request #358: KNOX-2395 - Make Gateway Services Pluggable
Posted by GitBox <gi...@apache.org>.
smolnar82 merged pull request #358:
URL: https://github.com/apache/knox/pull/358
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a change in pull request #358: KNOX-2395 - Make Gateway Services Pluggable
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #358:
URL: https://github.com/apache/knox/pull/358#discussion_r451715039
##########
File path: gateway-release/home/conf/gateway-site.xml
##########
@@ -18,6 +18,11 @@ limitations under the License.
-->
<configuration>
+ <property>
+ <name>gateway.services</name>
Review comment:
> I had envisioned the use of distinct properties, rather than a single "list" property.
> Something more explicit, like:
> `<property><name>gateway.service.alias</name><value>org.apache.knox.gateway.services.security.impl.RemoteAliasService</value></property> <property><name>gateway.service.tokenstate</name><value>org.apache.knox.gateway.services.token.impl.JournalBasedTokenStateService</value></property>`
>
> I think explicit class name values will make it easier for developers to create and deploy Knox customizations/extensions. Given this, I think a _list_ of such things could quickly become difficult to read.
Thanks, @pzampino for your review comment. It makes sense and I'm in favor of your recommended approach with a small alteration: I'd build the property names as `gateway.service.$SERVICE.$PARAM` where service references the service name and param is an arbitrary parameter name that Knox can interpret later on. In this round, I'd add `impl` only but in upcoming changes, there will be others. For instance:
```
<property>
<name>gateway.service.tokenstate.impl</name>
<value>org.apache.knox.gateway.services.token.impl.ZookeeperTokenStateService</value>
</property>
<property>
<name>gateway.service.tokenstate.useLocalAliasService</name>
<value>false</value>
<description>Indicates whether ZookeeperTokenStateService should use a local alias service when storing tokens or purely do the job in Zookeeper</description>
</property>
Any objection?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #358: KNOX-2395 - Make Gateway Services Pluggable
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #358:
URL: https://github.com/apache/knox/pull/358#discussion_r452261472
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/factory/RemoteRegistryClientServiceFactory.java
##########
@@ -26,20 +26,26 @@
import org.apache.knox.gateway.services.ServiceLifecycleException;
import org.apache.knox.gateway.services.ServiceType;
import org.apache.knox.gateway.services.config.client.RemoteConfigurationRegistryClientService;
+import org.apache.knox.gateway.services.security.AliasService;
// There are two implementations of 'RemoteConfigurationRegistryClientService':
// - LocalFileSystemRemoteConfigurationRegistryClientService
// - CuratorClientService
// However, the first one - local - is for test-only purposes
public class RemoteRegistryClientServiceFactory extends AbstractServiceFactory {
+ private final AliasServiceFactory aliasServiceFactory = new AliasServiceFactory();
+
@Override
public Service create(GatewayServices gatewayServices, ServiceType serviceType, GatewayConfig gatewayConfig, Map<String, String> options, String implementation)
Review comment:
This is related to my most recent comment, I guess. Since there isn't another known implementation, it currently cannot be overridden. So, the heart of the matter is the ability to plug-in an arbitrary gateway service implementation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #358: KNOX-2395 - Make Gateway Services Pluggable
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #358:
URL: https://github.com/apache/knox/pull/358#discussion_r451583820
##########
File path: gateway-release/home/conf/gateway-site.xml
##########
@@ -18,6 +18,11 @@ limitations under the License.
-->
<configuration>
+ <property>
+ <name>gateway.services</name>
Review comment:
I had envisioned the use of distinct properties, rather than a single "list" property.
Something more explicit, like:
`<property><name>gateway.service.alias</name><value>org.apache.knox.gateway.services.security.impl.RemoteAliasService</value></property>
<property><name>gateway.service.tokenstate</name><value>org.apache.knox.gateway.services.token.impl.JournalBasedTokenStateService</value></property>`
I think explicit class name values will make it easier for developers to create and deploy Knox customizations/extensions. Given this, I think a _list_ of such things could quickly become difficult to read.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a change in pull request #358: KNOX-2395 - Make Gateway Services Pluggable
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #358:
URL: https://github.com/apache/knox/pull/358#discussion_r452243627
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/factory/AliasServiceFactory.java
##########
@@ -44,22 +41,17 @@ public Service create(GatewayServices gatewayServices, ServiceType serviceType,
final AliasService defaultAliasService = new DefaultAliasService();
((DefaultAliasService) defaultAliasService).setMasterService(getMasterService(gatewayServices));
((DefaultAliasService) defaultAliasService).setKeystoreService(getKeystoreService(gatewayServices));
- defaultAliasService.init(gatewayConfig, options); //invoking init on DefaultAliasService twice is ok (in case implementation is set to 'default')
- switch (implementation) {
- case DEFAULT_IMPLEMENTATION_NAME:
- case "":
+ defaultAliasService.init(gatewayConfig, options); // invoking init on DefaultAliasService twice is ok (in case implementation is set to 'default')
+
+ if (matchesImplementation(implementation, DefaultAliasService.class, true)) {
Review comment:
If you check out the rest of the implementations, they all use the `defaultAliasService` instance for their purpose. It was the same way (as a local alias service)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on pull request #358: KNOX-2395 - Make Gateway Services Pluggable
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on pull request #358:
URL: https://github.com/apache/knox/pull/358#issuecomment-657289760
> It still seems that the possible implementations are constrained to some set already known by Knox. Why can't a developer "plug-in" an arbitrary implementation of any gateway service to suit their individual need?
Thanks for this very useful comment @pzampino. I've just submitted a new commit making it possible to plug-in an arbitrary gateway service implementation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on pull request #358: KNOX-2395 - Make Gateway Services Pluggable
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on pull request #358:
URL: https://github.com/apache/knox/pull/358#issuecomment-656149035
> It still seems that the possible implementations are constrained to some set already known by Knox. Why can't a developer "plug-in" an arbitrary implementation of any gateway service to suit their individual need?
Hm..this is a good idea. I haven't thought about it until now. It'd also mean that the developer will need to make sure the implementation is on the classpath.
Let me ad it to this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a change in pull request #358: KNOX-2395 - Make Gateway Services Pluggable
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #358:
URL: https://github.com/apache/knox/pull/358#discussion_r452243627
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/factory/AliasServiceFactory.java
##########
@@ -44,22 +41,17 @@ public Service create(GatewayServices gatewayServices, ServiceType serviceType,
final AliasService defaultAliasService = new DefaultAliasService();
((DefaultAliasService) defaultAliasService).setMasterService(getMasterService(gatewayServices));
((DefaultAliasService) defaultAliasService).setKeystoreService(getKeystoreService(gatewayServices));
- defaultAliasService.init(gatewayConfig, options); //invoking init on DefaultAliasService twice is ok (in case implementation is set to 'default')
- switch (implementation) {
- case DEFAULT_IMPLEMENTATION_NAME:
- case "":
+ defaultAliasService.init(gatewayConfig, options); // invoking init on DefaultAliasService twice is ok (in case implementation is set to 'default')
+
+ if (matchesImplementation(implementation, DefaultAliasService.class, true)) {
Review comment:
If you check out the rest of the implementations, they all use the `defaultAliasService` instance for their purpose (as a local alias service).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org