-
Notifications
You must be signed in to change notification settings - Fork 540
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 Active Directory secret engine support #902
Conversation
Will |
@jeffsanicola Not in this release but could happen in a future release. Do you see value in this feature? See my note in the PR:
|
@jeffsanicola Thinking about this more, library does make sense. I will add it to this PR. |
@jasonodonnell Awesome! Thank you for re-evaluating. |
@jeffsanicola |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! A few small suggestions
return strings.Trim(v.(string), "/") | ||
}, | ||
}, | ||
"role": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't checked against other examples in this provider, but it would feel more like idiomatic terraform config to me if this was "name"
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of role
is precedent here. Agreed its strange but I suppose consistency is correct.
"anonymous_group_search": { | ||
Type: schema.TypeBool, | ||
Optional: true, | ||
Description: `Use anonymous binds when performing LDAP group searches (if true the initial credentials will still be used for the initial connection test).`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the assumption that when default is not specified in the description for booleans, it defaults to false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct. Also remember that the backend itself has defaults, so it's not always proper for Terraform to duplicate it, I think.
d.SetId(backend) | ||
|
||
data := map[string]interface{}{} | ||
if v, ok := d.GetOkExists("anonymous_group_search"); ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to use a loop over a slice of strings here?
Definitely not for this PR, but a wider point:
Also I know it's consistent with the style elsewhere, but it scares me that we duplicate all these strings between the schema and implementation. It seems like an easy win to just have some consts, although given everything is currently in the vault
package, perhaps we would want to split into multiple packages to prevent massive top-level name noise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would make a good cleanup across the board. Something we could target in the future.
if err != nil { | ||
return fmt.Errorf("error reading %q: %s", rolePath, err) | ||
} | ||
log.Printf("[DEBUG] Read %q", rolePath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another comment for the wider code base rather than this PR - it would be lovely to get some helper functions for debug/warn/error etc logs
Co-authored-by: Tom Proctor <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some minor things that I think need changing, and otherwise some questions I had or some nitpicks.
I notice that there are a handful of generated resource documents that were moved? I believe those were moved in #871 and should stay where they are. The Terraform registry now uses file structure as a means of document structure, and expects docs to be in specific locations. Let me know if you have questions or need help resolving those
vault/resource_ad_secret_backend.go
Outdated
mountResp, err := client.Sys().MountConfig(path) | ||
if err != nil { | ||
return fmt.Errorf("error reading %q: %s", path, err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the backend is removed out of band, this will fail with a 404
and we'll be stuck. If we get a 404 type error here we need to remove from state like we do on line 369 below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
<li<%= sidebar_current("docs-vault-datasource-ad-access-credentials") %>> | ||
<a href="/docs/providers/vault/d/ad_access_credentials.html">vault_ad_access_credentials</a> | ||
</li> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this file is no longer used at all, and should be removed in a separate PR
Does TF use the same client token across runs? If not, this could be problematic for the library flow if a set was checked out by one run but then later checked in by another apply. @catsby do you see any potential issues with this? |
@calvn This provider does not support checkin/checkout functionality. We only allow you to create the library sets that can be used by checkin/checkout workflows. |
Ah, we're good then! |
@catsby Good call on the transform docs being moved. Not sure how that happened but I fixed it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Co-authored-by: Theron Voran <[email protected]>
Co-authored-by: Theron Voran <[email protected]>
* Add Active Directory secret engine support * Add optional to backend * Update vault/resource_ad_secret_roles.go Co-authored-by: Theron Voran <[email protected]> * Add deprecated flag for length and formatter * Add library support * Update documentation * Update vault/resource_ad_secret_backend.go Co-authored-by: Tom Proctor <[email protected]> * Remove optional from description * Fix typo in library doc * Move documentation back * Update vault/resource_ad_secret_backend.go Co-authored-by: Theron Voran <[email protected]> * Update vault/resource_ad_secret_backend.go Co-authored-by: Theron Voran <[email protected]> * Update ttl description * Add note about seconds to ttl Co-authored-by: Theron Voran <[email protected]> Co-authored-by: Tom Proctor <[email protected]>
This PR adds support for the Active Directory secret engine. The following endpoints are supported.
"/ad/config"
as a resource"/ad/roles/<role name>"
as a resource"/ad/library/<set name>"
as a resource"/ad/creds/<role name>"
as a data sourceNotably the rotate-root and check-in/check-out functionality aren't supported. These aren't really managed resources and don't work well with TF.
Currently automated acceptance testing isn't integrated with CircleCI because the Samba Docker image I'm using requires
privileged
, which is not possible with thedocker-compose
CircleCI service. This needs to be changed to use a machine instead so we can set the privileged flag. Integrated automated testing will be a follow up PR.To test this, you can run the following:
If you want to test this via
terraform
binary, here's an example resource:Related test output