Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KNOX-2757 - HadoopGroupProvider parameters should be added to the filter even there is a gateway level property with CENTRAL_GROUP_CONFIG_PREFIX #590

Merged
merged 1 commit into from Jun 9, 2022

Conversation

smolnar82
Copy link
Contributor

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>(&amp;(|(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>


// add group mapping parameters from gateway-site.xml, if any
final List<FilterParamDescriptor> groupMappingParamsList = getParamsFromGatewaySiteWithCentralGroupConfigPrefix(provider, context, resource);
if (groupMappingParamsList != null && !groupMappingParamsList.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: This check might be unnecessary since getParamsFromGatewaySiteWithCentralGroupConfigPrefix never returns nulls and nothing would happen if it was empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack

Copy link
Contributor

@lmccay lmccay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I concur with @zeroflag's comment that we should be able to override the params from gateway-site.xml with provider level params. The implementation here is MUCH easier to understand now though! Once the change for overrides and tests are added for that you will have my +1. Thanks!

…ter even there is a gateway level property with CENTRAL_GROUP_CONFIG_PREFIX
Copy link
Contributor

@zeroflag zeroflag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@smolnar82 smolnar82 merged commit 6cdd2f0 into apache:master Jun 9, 2022
@smolnar82 smolnar82 deleted the KNOX-2757 branch June 9, 2022 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants