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 2022/06/03 13:43:43 UTC
[GitHub] [knox] smolnar82 opened a new pull request, #590: KNOX-2757 - HadoopGroupProvider parameters should be added to the filter even there is a gateway level property with CENTRAL_GROUP_CONFIG_PREFIX
smolnar82 opened a new pull request, #590:
URL: https://github.com/apache/knox/pull/590
## What changes were proposed in this pull request?
From now on, in Knox's HadoopGroupProvider, the gateway-level `CENTRAL_GROUP_CONFIG_PREFIX` prefixed parameters are added together with any custom provider-level parameters into the final `HadoopGroupProvider` identity assertion filter of the generated web application.
I also needed to re-factor some code out from the `gateway-server` project that implements certain descriptor-related interfaces from `gateway-spi` as a simple POJO. The new Maven module's name is `gateway-spi-common` and I already see the benefit of having this new project serving the same functionality for other developments in the future.
With this new project we now do not need to create/mock already existing classes that we can re-use in our test classes where mocking isn't a really good option.
## How was this patch tested?
Added new unit tests to check if filter properties are generated as expected. Apart from this, I also tested the fix manually with my local Knox instance using the `Steps to reproduce` information from the corresponding JIRA:
```
<filter>
<role>identity-assertion</role>
<name>HadoopGroupProvider</name>
<class>org.apache.knox.gateway.identityasserter.hadoop.groups.filter.HadoopGroupProviderFilter</class>
<param>
<name>hadoop.security.group.mapping.ldap.search.attr.member</name>
<value>member</value>
</param>
<param>
<name>hadoop.security.group.mapping.ldap.search.filter.user</name>
<value>(&(|(objectclass=person)(objectclass=applicationProcess))(cn={0}))</value>
</param>
<param>
<name>hadoop.security.group.mapping.ldap.search.attr.group.name</name>
<value>cn</value>
</param>
<param>
<name>hadoop.security.group.mapping.ldap.url</name>
<value>ldap://localhost:33389</value>
</param>
<param>
<name>hadoop.security.group.mapping</name>
<value>org.apache.hadoop.security.LdapGroupsMapping</value>
</param>
<param>
<name>hadoop.security.group.mapping.ldap.search.filter.group</name>
<value>(objectclass=groupOfNames)</value>
</param>
<param>
<name>hadoop.security.group.mapping.ldap.bind.user</name>
<value>uid=guest,ou=people,dc=hadoop,dc=apache,dc=org</value>
</param>
<param>
<name>hadoop.security.group.mapping.ldap.bind.password</name>
<value>guest-password</value>
</param>
<param>
<name>group.mapping.c_env_assignees_1234</name>
<value>(!= 0 (size groups))</value>
</param>
</filter>
```
--
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.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] zeroflag commented on a diff in pull request #590: KNOX-2757 - HadoopGroupProvider parameters should be added to the filter even there is a gateway level property with CENTRAL_GROUP_CONFIG_PREFIX
Posted by GitBox <gi...@apache.org>.
zeroflag commented on code in PR #590:
URL: https://github.com/apache/knox/pull/590#discussion_r889018034
##########
gateway-provider-identity-assertion-hadoop-groups/src/main/java/org/apache/knox/gateway/identityasserter/hadoop/groups/filter/HadoopGroupProviderDeploymentContributor.java:
##########
@@ -62,46 +65,54 @@ protected String getFilterClassname() {
}
@Override
- public void contributeFilter( DeploymentContext context, Provider provider, Service service,
- ResourceDescriptor resource, List<FilterParamDescriptor> params ) {
- Map<String, String> p = provider.getParams();
- String prefix = p.get("CENTRAL_GROUP_CONFIG_PREFIX");
+ public void contributeFilter(DeploymentContext context, Provider provider, Service service, ResourceDescriptor resource, List<FilterParamDescriptor> params) {
Review Comment:
What happens if we define the same property in gateway-site (via CENTRAL_GROUP_CONFIG_PREFIX) + in the provider config? If the provider config should take priority over the gateway-site config, is this handled by the patch?
##########
gateway-provider-identity-assertion-hadoop-groups/src/main/java/org/apache/knox/gateway/identityasserter/hadoop/groups/filter/HadoopGroupProviderDeploymentContributor.java:
##########
@@ -62,46 +65,54 @@ protected String getFilterClassname() {
}
@Override
- public void contributeFilter( DeploymentContext context, Provider provider, Service service,
- ResourceDescriptor resource, List<FilterParamDescriptor> params ) {
- Map<String, String> p = provider.getParams();
- String prefix = p.get("CENTRAL_GROUP_CONFIG_PREFIX");
+ public void contributeFilter(DeploymentContext context, Provider provider, Service service, ResourceDescriptor resource, List<FilterParamDescriptor> params) {
+ final List<FilterParamDescriptor> filterParams = params == null ? new ArrayList<>() : new ArrayList<>(params);
+
+ // add group mapping parameters from gateway-site.xml, if any
+ final List<FilterParamDescriptor> groupMappingParamsList = getParamsFromGatewaySiteWithCentralGroupConfigPrefix(provider, context, resource);
+ if (groupMappingParamsList != null && !groupMappingParamsList.isEmpty()) {
Review Comment:
Minor: This check might be unnecessary since `getParamsFromGatewaySiteWithCentralGroupConfigPrefix` never returns nulls and nothing would happen if it was empty.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a diff in pull request #590: KNOX-2757 - HadoopGroupProvider parameters should be added to the filter even there is a gateway level property with CENTRAL_GROUP_CONFIG_PREFIX
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on code in PR #590:
URL: https://github.com/apache/knox/pull/590#discussion_r892644197
##########
gateway-provider-identity-assertion-hadoop-groups/src/main/java/org/apache/knox/gateway/identityasserter/hadoop/groups/filter/HadoopGroupProviderDeploymentContributor.java:
##########
@@ -62,46 +65,54 @@ protected String getFilterClassname() {
}
@Override
- public void contributeFilter( DeploymentContext context, Provider provider, Service service,
- ResourceDescriptor resource, List<FilterParamDescriptor> params ) {
- Map<String, String> p = provider.getParams();
- String prefix = p.get("CENTRAL_GROUP_CONFIG_PREFIX");
+ public void contributeFilter(DeploymentContext context, Provider provider, Service service, ResourceDescriptor resource, List<FilterParamDescriptor> params) {
Review Comment:
@zeroflag - I've just submitted a new PS with the changes we discussed above. Please review them whenever possible! Thanks!
--
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.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a diff in pull request #590: KNOX-2757 - HadoopGroupProvider parameters should be added to the filter even there is a gateway level property with CENTRAL_GROUP_CONFIG_PREFIX
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on code in PR #590:
URL: https://github.com/apache/knox/pull/590#discussion_r889275387
##########
gateway-provider-identity-assertion-hadoop-groups/src/main/java/org/apache/knox/gateway/identityasserter/hadoop/groups/filter/HadoopGroupProviderDeploymentContributor.java:
##########
@@ -62,46 +65,54 @@ protected String getFilterClassname() {
}
@Override
- public void contributeFilter( DeploymentContext context, Provider provider, Service service,
- ResourceDescriptor resource, List<FilterParamDescriptor> params ) {
- Map<String, String> p = provider.getParams();
- String prefix = p.get("CENTRAL_GROUP_CONFIG_PREFIX");
+ public void contributeFilter(DeploymentContext context, Provider provider, Service service, ResourceDescriptor resource, List<FilterParamDescriptor> params) {
Review Comment:
Nice catch, @zeroflag ! I'll make the necessary changes and submit a new patchset soon!
##########
gateway-provider-identity-assertion-hadoop-groups/src/main/java/org/apache/knox/gateway/identityasserter/hadoop/groups/filter/HadoopGroupProviderDeploymentContributor.java:
##########
@@ -62,46 +65,54 @@ protected String getFilterClassname() {
}
@Override
- public void contributeFilter( DeploymentContext context, Provider provider, Service service,
- ResourceDescriptor resource, List<FilterParamDescriptor> params ) {
- Map<String, String> p = provider.getParams();
- String prefix = p.get("CENTRAL_GROUP_CONFIG_PREFIX");
+ public void contributeFilter(DeploymentContext context, Provider provider, Service service, ResourceDescriptor resource, List<FilterParamDescriptor> params) {
+ final List<FilterParamDescriptor> filterParams = params == null ? new ArrayList<>() : new ArrayList<>(params);
+
+ // add group mapping parameters from gateway-site.xml, if any
+ final List<FilterParamDescriptor> groupMappingParamsList = getParamsFromGatewaySiteWithCentralGroupConfigPrefix(provider, context, resource);
+ if (groupMappingParamsList != null && !groupMappingParamsList.isEmpty()) {
Review Comment:
Ack
--
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.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 merged pull request #590: KNOX-2757 - HadoopGroupProvider parameters should be added to the filter even there is a gateway level property with CENTRAL_GROUP_CONFIG_PREFIX
Posted by GitBox <gi...@apache.org>.
smolnar82 merged PR #590:
URL: https://github.com/apache/knox/pull/590
--
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.
To unsubscribe, e-mail: dev-unsubscribe@knox.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org