-
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
ldap rotation import skip parameter #2128
Conversation
it might be better to update vault to just ignore/warn rather than error instead of this
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! 👍
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.
Thanks @kpcraig! Looks good so far! Can we also update the docs for the new fields?
func updateLDAPStaticRoleResource(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { | ||
// Vault rejects an update request containing skip_import_rotation since the field has no effect. | ||
// maybe Vault should silently ignore it, but for now just remove the offending field | ||
err := d.Set(consts.FieldSkipImportRotation, nil) |
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.
Thinking out loud here: I just realized this was being set to nil. I am not sure about this but it looks like the ldap API does not return skip_import_rotation
from the GET enpoint. In which case I don't think we need to worry about drift in TF's state vs Vault's state. So this is probably ok as-is?
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.
my worry was that if a terraform config includes the field, we need to strip it out or vault will reject it if an update action is triggered (say, if the element is modified?)
In retrospect Vault itself probably should at most warn.
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.
Yeah, Vault usually ignores irrelevant fields without even a warning, which is a bit strange. I agree that we should probably warn instead of error.
Setting to nil is probably ok as-is with the caveat that there may be situations I haven't considered.
Another option might be to have separate Create and Update functions and only send skip_import_rotation
in the Create request. Then we could leave the TF state unaltered. I don't think it is very common to use the TF state as the place where we store the request data. TF state can (and often does) include entries that are not included in the API requests.
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 rewrote this to check if it was an update or not at the point where we add data to the request itself, which I believe will mean that the TF state should retain the data?
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, thanks for working on this!
Co-authored-by: John-Michael Faircloth <[email protected]>
Co-authored-by: John-Michael Faircloth <[email protected]>
…tation-import-ski' into VAULT-23264/ldap-rotation-import-ski
consts.FieldSkipImportRotation: { | ||
Type: schema.TypeBool, | ||
Optional: true, | ||
Description: "Skip rotation of the password on import.", | ||
}, |
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.
We should remove this since they are not relevant to the data source.
// second 1.16 gated check | ||
{ | ||
SkipFunc: func() (bool, error) { | ||
return !testProvider.Meta().(*provider.ProviderMeta).IsAPISupported(provider.VaultVersion116), nil | ||
}, | ||
Config: testLDAPStaticRoleDataSourceWithSkipImportRotation(backend, bindDN, bindPass, url, username, dn), | ||
Check: resource.ComposeTestCheckFunc( | ||
resource.TestCheckResourceAttr("vault_ldap_secret_backend_static_role_with_skip", consts.FieldSkipImportRotation, "true"), | ||
), | ||
}, |
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.
We should remove this since they are not relevant to the data source.
|
||
func testLDAPStaticRoleDataSourceWithSkipImportRotation(path, bindDN, bindPass, url, username, dn string) string { | ||
return fmt.Sprintf(` | ||
resource "vault_ldap_secret_backend" "test" { | ||
path = "%s" | ||
description = "test description" | ||
binddn = "%s" | ||
bindpass = "%s" | ||
url = "%s" | ||
} | ||
|
||
resource "vault_ldap_secret_backend_static_role_with_skip" "role" { | ||
mount = vault_ldap_secret_backend.test.path | ||
username = "%s" | ||
dn = "%s" | ||
role_name = "%s" | ||
rotation_period = 60 | ||
skip_import_rotation = true | ||
} | ||
|
||
data "vault_ldap_static_credentials" "creds" { | ||
mount = vault_ldap_secret_backend.test.path | ||
role_name = vault_ldap_secret_backend_static_role.role.role_name | ||
} | ||
`, path, bindDN, bindPass, url, username, dn, username) | ||
} |
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.
We should remove this since they are not relevant to the data source.
|
||
* `skip_import_rotation` - (Optional) Causes vault to skip the initial rotation on import. Not applicable on updates. | ||
Requires Vault 1.16 or above. |
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.
We should remove this since they are not relevant to the data source.
Since the thing already got merged I'll put in a second PR for the doc/test issues: #2151 |
This PR adds TFVP support for the import-skip parameters for LDAP static roles on both the backend (which applies to all static roles), and on individual roles.
Related PR on the LDAP plugin that this supports: hashicorp/vault-plugin-secrets-openldap#83