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

user: handle unknown superuser passwords #59

Merged
merged 2 commits into from
Apr 19, 2022

Conversation

bendrucker
Copy link
Contributor

Currently, it's not possible to define configuration like this:

resource "redshift_user" "terraform" {
  name      = "terraform"
  superuser = true
  password  = random_password.terraform.result
}

resource "random_password" "terraform" {
  length      = 64
  min_lower   = 1
  min_upper   = 1
  min_numeric = 1
}

Given this configuration, the provider returns an error:

Users that are superusers must define a password.

Upon further investigation, this error is returned from CustomizeDiff. This hook runs before a plan is rendered. This means that on first run, before the Create method for random_password has run, the value for result is not yet known.

GetOk will still return _, true, because it is known that some value is set. When the value is unknown, Get will return the zero value. Any plan time inspection can only occur when the new value is known at plan time.

While I didn't set up real cluster credentials to run the acceptance tests to success for this, I did execute them to ensure that the added test case fails with the original error and proceeds to a connection error with the fix.

@mtesch-um
Copy link
Contributor

does this also fix #56 ?

@bendrucker
Copy link
Contributor Author

bendrucker commented Mar 4, 2022

This only changes the behavior when password is unknown, not when it is unset.

I did find the validation implemented in the CustomizeDiff helpful. Originally I planned to use IAM auth only for the superuser but added a generated password as above since it's required. A viable approach to #56 that doesn't remove this plan-time validation might be something like this:

old, _ := d.GetChange(userPasswordAttr)
if d.Id() != "" && old.(string) == "" {
  // valid: resource is not being created (has id) and no existing password is in state (was imported)
}

Where d is ResourceDiff.

@bendrucker
Copy link
Contributor Author

There's probably some logic missing there though. Like you'd also need to check whether superuser's value has changed. If the user was previously not a superuser and had no password but is being converted, a password is presumably required.

@winglot winglot merged commit beadcd1 into brainly:master Apr 19, 2022
@winglot winglot added the bug Something isn't working label Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants