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

Load LDAP password from secret and update guideline #659

Merged

Conversation

hungtran84
Copy link
Contributor

This PR is created for 2 things:

  • Load LDAP bind password from k8s secret instead of extra settings due to security concern
  • Update README with the instructions and sample of LDAP integration via extra settings
  • Also add missing LDAPSearch module

Already tested in Azure AKS with a self-hosted windows Active Directory.

The most important things is to have LDAP integration with AWX deployment without any human intervention require and have LDAP setting checked in code.

@shanemcd
Copy link
Member

Hello. Apologies for the delay here. Can you please resolve the conflicts? Once you do that and the tests pass we will merge this.

@hungtran84
Copy link
Contributor Author

hi Shane, could you help to review my PR again (after resolving the conflict)?

@pat-s
Copy link

pat-s commented Mar 6, 2022

I would be very interested in seeing this getting merged. Thanks for your work @hungtran84!

@Cl0udius
Copy link

We are facing the same problem here. Looking forward to see this getting merged.

@r0b1ndot
Copy link

We are also Looking forward to see this getting merged because this feature is required and appreciated :)

@suukit
Copy link
Contributor

suukit commented Apr 1, 2022

+1 from my side. Desperately waiting for this one.

@suukit
Copy link
Contributor

suukit commented Apr 5, 2022

To the maintainers / @shanemcd : As hungtran84 does not seem to react any more - would it help if I recreate the PR starting with a fresh branch of the devel branch to resolve the conflicts? We could then close this PR as stale.
regards
Max

@hungtran84
Copy link
Contributor Author

I'm still waiting for approval from the maintainer. Actually, no update for months that why the conflict shows. I can resolve it but still need his review and approval, otherwise, it ends up with another conflict by the time.

Cc: @shanemcd

Copy link
Contributor

@suukit suukit left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you for the extended documentation!

@suukit
Copy link
Contributor

suukit commented Apr 5, 2022

@hungtran84 : Ah, sorry. So we got some chicken-egg situation here. I did a first review, maybe this helps the maintainers.

@suukit
Copy link
Contributor

suukit commented Apr 13, 2022

@hungtran84 : Maybe we should just resolve the conflicts, if you got no time you can add me as collaborator and I'll rebase.. best regards Max

@hungtran84
Copy link
Contributor Author

feel free, I've added you add contributor to my forked repo @suukit

@suukit
Copy link
Contributor

suukit commented Apr 13, 2022

Rebased this PR to devel branch.
@shanemcd, @rooftopcellist or any other maintainer, can you help to bring this upstream?
thank you
max

@shanemcd shanemcd merged commit 5b73ad1 into ansible:devel Apr 25, 2022
@lagunary
Copy link

Hello Guys,

Unfortunately, it seems the ldap_bind_password will never be used in any playbook just read it from secrets.
Ive tested and tcpdump the whole communication AWX with LDAP server and it is 100% sure the password is missing from request. (Further symptoms: on the webui the password field is empty, db tables are empty, credentials.py is empty regarding ldap_password or any similar key)
We double checked our configs in yaml, and secrets, they are proper based on docs.
Please check it, cause the 0.21.0 cant be used with LDAP.

@hungtran84
Copy link
Contributor Author

hi @lagunary , it is my mistake when resolving the conflict, I remove 2 lines of code that causing the error. I'm about to submit another PR to fix that bug

@erlisb
Copy link

erlisb commented Jun 9, 2022

@hungtran84 maybe I am confused but I am missing here how the secret is consumed.
Shouldn't we have something similar as follows, in the deployment.yaml.j2 :

{% if ldap_cacert_ca_crt %}
        - name: "{{ ansible_operator_meta.name }}-ldap-cacert"
          secret:
            secretName: "{{ ldap_cacert_secret }}"
            items:
              - key: ldap-ca.crt
                path: 'ldap-ca.crt'

which is then mounted as follows :

      volumes:
      - name: awx-demo-ldap-cacert
        secret:
          defaultMode: 420
          items:
          - key: ldap-ca.crt
            path: ldap-ca.crt
          secretName: ldap-custom-cert

Thank you.

tu-doan pushed a commit to tu-doan/awx-operator that referenced this pull request Jun 23, 2022
* Load LDAP password from secret and update guideline

* Add pod_labels for custom pod labels

Signed-off-by: Loc Mai <[email protected]>

* Omit tls secret if using wildcard cert

* Resolve conflicts

* Remove the ingress changes

* Remove the config changes

* Load LDAP password from secret and update guideline

* Omit tls secret if using wildcard cert

* Resolve conflicts

* Remove the ingress changes

* Remove the config changes

Co-authored-by: hungts <[email protected]>
Co-authored-by: Loc Mai <[email protected]>
Co-authored-by: Max Bidlingmaier <[email protected]>
Co-authored-by: Max Bidlingmaier <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants