search

aminueza / terraform-provider-minio

Terraform provider for managing MinIO S3 buckets and IAM Users.
https://registry.terraform.io/providers/aminueza/minio
GNU Affero General Public License v3.0
233 stars 69 forks source link

Parse service account user from LDAP str #547

Closed pjsier closed 5 months ago

pjsier commented 10 months ago

Parse service account target user from LDAP string

The recent change in #525 to track target_user in state for minio_service_account is breaking for LDAP users. It looks like madmin is returning ParentUser as a full LDAP string such as "CN=minio-user,DC=example,DC=org" for minio-user.

This PR fixes that issue by attempting to parse each returned ParentUser string as an LDAP string, and if it fails return the string as-is. I saw some regex utilities for checking whether or not a string was LDAP, but it didn't look like they helped much with parsing, and since this ignores non-LDAP strings it felt simple enough to do this simply.

My big question is that I'm not sure how to test it other than the unit tests here. It looks like we don't have a ton of tools for testing LDAP users, so another set of eyes would be helpful

Reference

felladrin commented 9 months ago

I don't know how to test, but as no one is against this change and no one else has proposed a different solution, we should try it.

ℹ️ For releasing a new version, all we need to do is push a new tag (in this case it would be v1.20.2), and the CI will create a release here on GitHub, which in turn will trigger an update in Terraform Registry via webhook :) Could you do it after merging this, @pjsier?

pjsier commented 8 months ago

@felladrin Sorry for the delay! Can we cut a beta release on this so that it's easier to test but not a full release?

felladrin commented 5 months ago

Sorry for being also absent. It's getting harder and harder to find a gap. We need new collaborators :D

Currently, we don't have a way to do beta releases, but I love the idea. Will check if it's possible.

pjsier commented 5 months ago

It looks like you can use sources from GitHub commits? That might be the safest way to test and I’ll update the issue

and no problem! I’ve also been swamped

felladrin commented 5 months ago

Thanks! After testing it, will you create the tag v2.2.0 on https://github.com/aminueza/terraform-provider-minio/commit/ac8137654479b1ac374bccd81bc5c3ef70435fa9? :)

pjsier commented 5 months ago

@felladrin that was the original issue is it wasn’t clear how to test this one because we couldn’t reproduce it, so we would need someone experiencing it

felladrin commented 5 months ago

I think we might have a chicken-egg problem here. We're hesitant to release it because we're waiting for someone to test it, but no one will test it until we release it, as they probably won't build it just to test it.

I've just merged another PR that I think is worth releasing along with this. Let me tag v2.2.0 on the last commit from main and we get the feedback from users this time, ok?

About releasing a beta version, I've checked that Terraform Registry doesn't provide a way to create pre-releases, and what others are doing is creating a second account just to release the beta releases. (e.g. Google / Google-Beta). But it also requires having two different repositories on GitHub.