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

feat: Add salt-api ldap eauth tests. #269

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

mrichar1
Copy link
Contributor

A first pass at adding salt-api ldap integration tests as promised.

The osixia/openldap container comes with 2 'builtin' users: admin and readonly, and we add an ldap group called saltadmins containing the readonly user.

We then set salt-api.conf to connect to the LDAP server, and set the eauth rules to give access 'directly' to the admin user and also to members of the saltadmins group.

We then get a token and run a command via the for both users, testing both eauth rules.

I have tweaked things to try to handle the start/stop of the openldap container and cleanup neatly - but you might prefer the existing cleanup function being extended to also shut down the openldap container, if found?

I also haven't added this test to the github workflow as I wasn't sure if it should go in as it's own entry, or will be called from the existing salt-api/test.sh

Let me know what you think, and if you have any other changes you would like to see.

@mrichar1 mrichar1 force-pushed the ldap_integration_tests branch from da5965b to 0077415 Compare October 25, 2024 18:37
@cdalvaro
Copy link
Owner

Wow! That was fast! Thank you very much for adding this test!

I think it is perfect, and a great documentation complement for people needing a full example of how to setup the image with an ldap db.

You can call this test directly from the salt-api section of the workflow:

- name: Execute salt-api tests
        if: always()
        run: |
          tests/salt-api/test.sh
          tests/salt-api/salt-api-ldap.sh

Currently all tests are failing because of Salt Project migration: https://saltproject.io/blog/upcoming-salt-project-docs-and-repo-migration/

I hope the migration finishes soon so we can run these tests without errors.

@mrichar1 mrichar1 force-pushed the ldap_integration_tests branch from 0077415 to 6ee4635 Compare October 26, 2024 10:07
@mrichar1
Copy link
Contributor Author

It was actually much easier than I expected to get working due to the container already having default baseDN, user accounts etc set up.

I've updated the tests to run ldapadd inside the Docker container, and have added them to the github workflow

@mrichar1 mrichar1 force-pushed the ldap_integration_tests branch 2 times, most recently from 18c4c34 to 2eea147 Compare October 26, 2024 10:15
@cdalvaro
Copy link
Owner

Hi @mrichar1, could you rebase your branch with the latest changes in the main branch?

@mrichar1 mrichar1 force-pushed the ldap_integration_tests branch from 838382b to 0bf4835 Compare October 31, 2024 10:12
@mrichar1
Copy link
Contributor Author

That's the rebase completed and the shellcheck fix added and pushed.

@mrichar1 mrichar1 force-pushed the ldap_integration_tests branch from 0bf4835 to c4741ed Compare October 31, 2024 10:13
@cdalvaro
Copy link
Owner

Thank you, @mrichar1!

Your test has passed, so I'm going to merge the PR!

I have to solve an issue with the gitfs tests, but I think it must be something related with secrets permissions.

Thank you for your contribution!

@cdalvaro cdalvaro merged commit 637a08f into cdalvaro:main Oct 31, 2024
5 of 7 checks passed
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.

2 participants