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

Add module ldap_attrs; deprecate ldap_attr #31664

Closed
wants to merge 3 commits into from

Conversation

Noles
Copy link

@Noles Noles commented Oct 12, 2017

SUMMARY

The current "ldap_attr" module does not support ldap attribute insertations or deletions with objectClass dependencies.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

ldap_attrs

ANSIBLE VERSION
ansible 2.8.0
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/akorinek/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/dist-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.13 (default, Jan 19 2017, 14:48:08) [GCC 6.3.0 20170118]
ADDITIONAL INFORMATION

For example, if you have the following ldap entry:

dn: dc=test,dc=local
o: test.local
dc: test
objectClass: top
objectClass: dcObject
objectClass: organization

and you like to add:

gosaAclEntry: 0:subtree:Y249YWRtaW4sb3U9YWxjcm9sZXMsZGM9dGVzdCxkYz1sb2NhbAo=
 :dWlkPWZkLWFkbWluLG91PXBlb3BsZSxkYz10ZXN0LGRjPWxvY2FsCg==

you need the additional objectClass gosaAcl.

But because of the schema definition:

objectclass ( 1.3.6.1.4.1.10098.1.2.1.19.18 NAME 'gosaAcl'
  DESC 'GOsa - ACL container to define single ACLs'
  SUP top AUXILIARY
  MUST ( gosaAclEntry  ))

gosaAclEntry must be added on gosaAcl add as well.

With the ldap_attr module these insertation is not possible because every entry will be added one by one and can not be combined in one run.
This results in:
OBJECT_CLASS_VIOLATION: {'info': "object class 'gosaAcl' requires attribute 'gosaAclEntry'", 'desc': 'Object class violation'}
and
OBJECT_CLASS_VIOLATION: {'info': "attribute 'gosaAclEntry' not allowed", 'desc': 'Object class violation'}

For backward compatibility I created a new module with different configuration parameters.

@ansibot

This comment has been minimized.

@ansibot
Copy link
Contributor

ansibot commented Oct 12, 2017

cc @jtyr
click here for bot help

@ansibot ansibot added affects_2.5 This issue/PR affects Ansible v2.5 ci_verified Changes made in this PR are causing tests to fail. module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. needs_triage Needs a first human triage before being processed. net_tools Net-tools category new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. and removed ci_verified Changes made in this PR are causing tests to fail. labels Oct 12, 2017
@ansibot

This comment has been minimized.

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Oct 12, 2017
@s-hertel s-hertel removed the needs_triage Needs a first human triage before being processed. label Oct 13, 2017
@ansibot ansibot added the new_contributor This PR is the first contribution by a new community member. label Oct 18, 2017
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Oct 26, 2017
@ansibot ansibot removed the new_contributor This PR is the first contribution by a new community member. label Nov 3, 2017
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Nov 11, 2017
@ansibot ansibot added the support:core This issue/PR relates to code supported by the Ansible Engineering Team. label Nov 19, 2017
@ansibot ansibot added the new_contributor This PR is the first contribution by a new community member. label Feb 1, 2018
@Xaroth
Copy link
Contributor

Xaroth commented Feb 1, 2018

I'm testing this module under python3 in my own test lab; apart from the minor issue I found, it seems to work like a charm. Mind, the current ldap_attr and ldap_entry currently have the same bytes-vs-str issue, so it might be worth a separate PR to fix the py3 compatibility simultaneously

@Noles
Copy link
Author

Noles commented Feb 2, 2018

Good point. I never verified the module with py3. But as I remember this code block comes directly from current ldap_attr module. So it should be indeed fixed there and in ldap_entry as well.

@ansibot ansibot removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Feb 2, 2018
@CtrlC-Root
Copy link

Actually I think the proper way would be to modify line 193 to from ansible.module_utils._text import to_native, to_bytes and then use to_bytes() instead of six.b().

@Xaroth
Copy link
Contributor

Xaroth commented Nov 16, 2018

@CtrlC-Root using ansible natives is better, yes.

drybjed added a commit to drybjed/debops that referenced this pull request Mar 4, 2019
The 'ldap_attrs' Ansible module is a replacement for the 'ldap_attr'
module created by Alexander Korinek. It supports management of multiple
LDAP attributes at once, which solves several issues with LDAP
configuration.

At the moment the 'ldap_attrs' module is not part of Ansible core,
therefore it will be included in the 'debops.ansible_plugins' role for
the forseeable future.

Ref: ansible/ansible#31664
drybjed added a commit to drybjed/debops that referenced this pull request Mar 4, 2019
This change should make the 'ldap_attrs' Ansible module usable in
Python 3 environments.

Ref: ansible/ansible#31664 (review)
drybjed added a commit to drybjed/debops that referenced this pull request Mar 28, 2019
The 'ldap_attrs' Ansible module is a replacement for the 'ldap_attr'
module created by Alexander Korinek. It supports management of multiple
LDAP attributes at once, which solves several issues with LDAP
configuration.

At the moment the 'ldap_attrs' module is not part of Ansible core,
therefore it will be included in the 'debops.ansible_plugins' role for
the forseeable future.

Ref: ansible/ansible#31664
drybjed added a commit to drybjed/debops that referenced this pull request Mar 28, 2019
This change should make the 'ldap_attrs' Ansible module usable in
Python 3 environments.

Ref: ansible/ansible#31664 (review)
@drybjed
Copy link
Contributor

drybjed commented Apr 29, 2019

@Noles, @jtyr: I'd like to ask what are the plans for inclusion of this module in Ansible?

I think that the ldap_attrs module works much better with ldap_entry module than the current ldap_attr module found in Ansible. You can see this in the debops.ldap role and the debops.slapd role where I was able to define a set of tasks performed in the LDAP directory itself using just one YAML list.

I implemented support for the X-ORDERED extension in the ldap_attrs module which should make LDAP ACL configuration much easier to manage, but it seems that @Noles wants me to create a new PR directly... Since then I'm considering a more secure LDAP environment with TLS Client Certificates being a requirement, which would require additional parameters in the ldap_entry and ldap_attrs modules. Because this PR is now outdated, and parts of the Ansible modules related to python-ldap authentication have been moved to a different file, should I just create a new PR with ldap_attrs and updated LDAP authentication in Ansible that supports Client Certificates?

@Noles Noles force-pushed the feature-module_ldap_attrs branch from 1c978e8 to 582fbc0 Compare May 20, 2019 17:07
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels May 20, 2019
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. deprecated This issue/PR relates to a deprecated module. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels May 20, 2019
@Noles
Copy link
Author

Noles commented May 20, 2019

I rebased my changes but without any client certificate extensions. I think its better to realize this step by step.

@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label May 28, 2019
@gundalow gundalow added the pr_day Has been reviewed during a PR review Day label Sep 19, 2019
@gundalow
Copy link
Contributor

Thank you everybody for the work that's gone into this.
I've just spoken with drybjed, he has an updated version of this code that's used in debops.ldap and debops.slapd roles and supports the X-ORDERED extension as well as other bug fixes.

So I'm going to close this and @drybjed will raise a new PR

@drybjed
Copy link
Contributor

drybjed commented Sep 21, 2019

@gundalow The new pull request: #62701

ganto pushed a commit to ganto/ansible-debops-ansible_plugins that referenced this pull request Oct 9, 2019
The 'ldap_attrs' Ansible module is a replacement for the 'ldap_attr'
module created by Alexander Korinek. It supports management of multiple
LDAP attributes at once, which solves several issues with LDAP
configuration.

At the moment the 'ldap_attrs' module is not part of Ansible core,
therefore it will be included in the 'debops.ansible_plugins' role for
the forseeable future.

Ref: ansible/ansible#31664
ganto pushed a commit to ganto/ansible-debops-ansible_plugins that referenced this pull request Oct 9, 2019
This change should make the 'ldap_attrs' Ansible module usable in
Python 3 environments.

Ref: ansible/ansible#31664 (review)
@ansible ansible locked and limited conversation to collaborators Oct 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.5 This issue/PR affects Ansible v2.5 community_review In order to be merged, this PR must follow the community review workflow. deprecated This issue/PR relates to a deprecated module. has_issue module This issue/PR relates to a module. net_tools Net-tools category new_contributor This PR is the first contribution by a new community member. new_module This PR includes a new module. new_plugin This PR includes a new plugin. pr_day Has been reviewed during a PR review Day stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants