Additional group attributes

[hanneshayashi]

Description

This pull request adds the following new features:

• Add managed_by (managedBy attribute) to groups
• Add custom_attributes to groups

It should also fix some issues that I have encountered with the provider:

• Mark dn (distinguishedName attribute) for re-computation if the name or container of the group changes
• A permadiff when passing an empty JSON "{}" to custom_attributes (for users as well as groups)
• An issue with custom_attributes being in double quotes (e.g. "foo" = ""bar"")
• An issue where clearing multiple attributes didn't work, because ";" was used in the join (needs to be "," because PowerShell expects an array)
• An error when clearing a previously set custom_attributes attribute ("Unexpected end of JSON"):
  1. Set custom_attributes
  2. Run terraform apply
  3. Verify that attributes are set
  4. Remove custom_attributes from resources
  5. Run terraform apply
  6. Notice error
  7. Notice that attributes are not cleared
  8. Run terraform apply again
  9. No error and attributes are cleared

I also did some slight refactoring to enable code reuse. None of the changes should be breaking.
I think there is still some room for improvement, but I didn't want to make any overly broad changes to the code base. Although I did add a new package to the vendor directory for the customDiff. It's part of the already used helper package, so it should hopefully be fine.

References

• Add managedBy: Add built-in attribute ManagedBy to ad_group resource #131
• Add distinguishedName: Add distinguished_name attribute to user and group data sources and resources #123 (Partly - I only added it to the group resource)
• Additional attributes for groups: additional attributes in resource ad_group #91

Community Note

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

[hashicorp-cla]

All committers have signed the CLA.

[jpatigny]

@redeux @koikonom , can we have a review on this PR please ?

[hanneshayashi]

I updated the merge request to resolve the conflicts that came up after #133 was merged (renamed distinguished_name to dn).

I also updated the description of the merge request to reflect that it still fixes an issue with the distinguished_name, where it doesn't properly update when the group is renamed or moved.

[hanneshayashi]

Has anyone had a chance to review this yet? For what it's worth, we've been using the provider with these changes in production for about half a year now, but it would be nice to use the official build again.

Also, I have some more minor changes lined up that would be based on these changes and I'm waiting for this to be merged before opening another PR.

[vanderboon]

Can this be reviewed? This is the only thing blocking us from using this provider.

[eperdeme]

I think this stops local winrm working, I get this error on a compiled edition of the PR;

╷
│ Error: Missing required argument
│ 
│ The argument "winrm_username" is required, but was not set.
╵
╷
│ Error: Missing required argument
│ 
│ The argument "winrm_password" is required, but was not set.
╵
╷
│ Error: Missing required argument
│ 
│ The argument "winrm_hostname" is required, but was not set.

In the existing provider I can do;

provider "ad" {
  winrm_hostname = ""
  winrm_username = ""
  winrm_password = ""
}

[hanneshayashi]

@eperdeme Hm... I would not expect the provider to work without any of those configurations. How or where would it connect? If you use environment variables, sure. But explicitly setting all of those values to an empty string seems to me like it shouldn't work. Maybe I'm missing something? We use my branch in production that I have released here: https://registry.terraform.io/providers/hanneshayashi/ad/latest

We configure the provider for Kerberos (that PR was also from me) like this:

provider "ad" {
  winrm_hostname = "<our DC>"
  winrm_username = "<our service account>"
  winrm_password = ""
  winrm_port     = 5986
  winrm_proto    = "https"
  winrm_insecure = true
  krb_realm      = "<our domain>"
  krb_conf       = "/etc/krb5.conf"
  krb_keytab     = "<our keytab>"
}

If you try my release, be sure to specify the source in your required_providers like this:

terraform {
  required_providers {
    ad = {
      source = "hanneshayashi/ad"
    }
  }
}

[eperdeme]

@hanneshayashi the provider supports local execution as per https://github.com/hashicorp/terraform-provider-ad/blob/main/docs/index.md#note-about-local-execution-windows-only

This method occurs when for example you run ci runners on a Windows server and bind that runner to the svc amount. This is how we currently run this provider.

[hanneshayashi]

@eperdeme I see. I must admit, that is not a setup I have personally tried. I just did a merge from main into this branch to make sure that it is still up to date. There were a lot of changes to the vendored code, but nothing that should have any impact on authentication. Can you please check again? Also can you confirm that the behaviour you are experiencing does not happen with the latest version of the official provider? As far as I know, the last change to the authentication was done when the Kerberos code was merged and I just want to make sure that didn't break anything :)

[bh0520]

Is there a timeline on when this might be merged and released?

[aggallim]

Really in need of this, could it be merged please?

[Tbohunek]

Hi, anybody worked on this yet? Is it viable? What is missing to complete it?

[MaxiHorn]

Hey Guys, any updates on this ? :)

[hanneshayashi]

I just resolved a minor merge conflict that seems to have popped up at some point. I don't know why the triage step is failing but it seems to only be relevant for the PR labels and the unit tests still seem to pass.
For those who are looking for a version of the provider with these changes, please try: https://registry.terraform.io/providers/hanneshayashi/ad/latest
Big disclaimer: I do not actively maintain this fork anymore as it was done for a customer project a few years ago but as far as I know, this is still used in production and I am not aware of any issues. But still: Use at your own risk.

[Tbohunek]

Thanks a lot @hanneshayashi !

Turning now to @manicminer and @koikonom who initially reviewed the linked issue #91 and apparently worked on implementation of the same for ad_user.... Please, what's now blocking this from being merged? 🙏