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 support for LDAP user and group in policy attachments #446

Merged
merged 2 commits into from
Apr 11, 2023

Conversation

acolombier
Copy link
Contributor

This PR implements the following changes:

  • Add support for LDAP DN as group_name in the resource minio_iam_group_policy_attachment
  • Add support for LDAP DN as user_name in the resource minio_iam_user_policy_attachment

Reference

Closing issues

Note

When running tfplugindocs (v0.14.1), it did update couple of syntax unrelated to this change; it now seems to make the difference between optional arguments and read-only arguments.

Currently, I have only pushed those that are relevant to this PR (doc related to the proposed change), but let me know if you would like me to push the other updates in a new PR.

@acolombier acolombier force-pushed the feat/policy-attachment-ldap-support branch from abc1424 to e3b3244 Compare March 27, 2023 14:27
@acolombier acolombier force-pushed the feat/policy-attachment-ldap-support branch from e3b3244 to 7ddd072 Compare March 27, 2023 14:29
Copy link
Collaborator

@BuJo BuJo left a comment

Choose a reason for hiding this comment

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

I had a look at the changes, the only thing that I found a little odd is that minio can't provide information about LDAP users/groups.

@acolombier
Copy link
Contributor Author

I had a look at the changes, the only thing that I found a little odd is that minio can't provide information about LDAP users/groups.

Minio can actually provide information about LDAP users/groups that it has a configuration for. Removing the section that perform LDAP user/group assert removes the ability for the provider to perform drift detection/reconciliation.

@acolombier acolombier requested a review from BuJo April 4, 2023 17:33
@acolombier
Copy link
Contributor Author

@BuJo after further digging, it looks like only GetUserInfo needs the special error handling in case it is a LDAP user, so I have remove the group one. Also added specific catch for LDAP user, so it wouldn't obfuscate any errors returned by madmin.AdminClient.GetUserInfo

Tested locally with a mix set of static users and group, as well as LDAP user and group too. Tested happy path and reconciliation.

I feel like the user custom logic should definitely be covered with unit tests, but AFAIS, this would require some refactoring to support DI in order to allow madmin.AdminClient.GetUserInfo mocking.

IMHO, this refactoring should neither happen in this PR as it is out of scope with the task at hand, and unfortunately I'm unsure I currently have the time to put that effort in too. I have added couple of debug output so if this ever regress in future versions, there should at least be some debugging starting point.

Hopefully this is acceptable.

@acolombier acolombier force-pushed the feat/policy-attachment-ldap-support branch from d6e76b0 to 969b158 Compare April 7, 2023 09:45
Copy link
Collaborator

@BuJo BuJo left a comment

Choose a reason for hiding this comment

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

Allright, I think it's good to go. While I'd prefer some tests for feeling more comfortable, I'm quite sure it won't break a current usecase.
Thank you for your work!

@BuJo BuJo merged commit ae4966e into aminueza:master Apr 11, 2023
@zeqk
Copy link

zeqk commented Apr 12, 2023

Hi! Thank you very much for work in this feature, I need this.
When will this change be released?
https://registry.terraform.io/providers/aminueza/minio/latest
https://github.com/aminueza/terraform-provider-minio/releases/

@felladrin
Copy link
Collaborator

Hi! Thank you very much for work in this feature, I need this. When will this change be released?

✅ Published in release 1.14.0.

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.

Add support for LDAP users and groups on policy attachment resources
4 participants