-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Clarifications about admin user and password. #3308
Conversation
Document two facts: * changing the user name causes the resource to be dropped and recreated. * One can use a blank value for password to stop terraform from attemting to update the password.
@stack72 You were the original contributor of |
|
||
* `administrator_login_password` - (Required) The password associated with the `administrator_login` user. Needs to comply with Azure's [Password Policy](https://msdn.microsoft.com/library/ms161959.aspx) | ||
* `administrator_login_password` - (Required) The password associated with the `administrator_login` user. Needs to comply with Azure's [Password Policy](https://msdn.microsoft.com/library/ms161959.aspx). If the value is blank, and the server already exists, it will not attempt to change the password. |
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 i'm understanding this correcting, if the password is blank and you try to add one it doesn't work (and this you need to recreate the resource), while if ifs been set you can change the password?
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.
To clarify here are the 4 possible scenarios:
- Creating the resource with password blank: I didn't test, but the azure password policy would not let this succeed if terraform tried.
- Creating the resource with password non-blank: This works in the obvious manner
- Updating the resource with the password blank: Terraform will not attempt to change or verify the password on the resource.
- Updating the resource password value not blank: Terraform confirms the password in its config is the same as on azure. If it does not match it will change the password
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.
ahh thank you for the clarification, if azure won't allow a blank password (and i'd like to verify this) the best course of action is to validate the property so the user simply cannot pass in an empty string, thus negating any documentation change 🙂
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 implore you to reconsider that logic, and I'm pretty sure this was intended behavior that's going to break peoples environments. Tell me if you think I'm mistaken.
Consider that in many cases when terraform apply
or terraform plan
is being run from CI/CD you are deploying to existing infrastructure, and your SQL server is probably not changing, Also consider that the azure sql server password is a sensitive secret. Its possibly the only secret in a CI/CD build process for a simple web app.
That's pretty much my use case right now. And I might be able to avoid using Azure Key Vault at all in my use case because that admin password is honestly a secret I don't care about and never need again. Even in a "create" scenario, I could provide a random password that I throw away.
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 that is how it works i don't think it was intended by design looking at the code, if anything it's a bug (but i could be wrong)
If i understand correctly your saying you don't want to have to save the value so your CI system can disregard it and not keep it around? It'll still be in the state saved in your CI wouldn't 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.
I just checked and there is no password stored in the tfstate. Also, I never new the password. Here is what I did:
- I was given access to an Azure Ad tenant with an existing sql server that I don't know the sql password for.
- I was given ownership of the resource group that contained that SQL server
- I created a new terraform project and started importing some of those resources
- I used this script to generate terraform files for all the resources in the resource group in second terraform project. this is where I discovered the blank password feature/bug
- I copied all the terraform configurations from the second autogenerated project to the first one I made, and refactored to add variables, etc.
- I used terraform plan and terraform apply to test the first script.
At no point did I ever know the sql admin password. Neither tfstate file contains the administrative password.
I really like this feature. This feature is really practical in a CI/CD use case. I do agree that having a blank string in a required field as a "magic value" is unintuitive. OTOH, if you make the password field optional, you'd have to make an output value of a randomly generated password on creation.
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 from the POV of terraform this is a bug, what is in the cfg/tfstate should match what reality is and additionally if a blank password isn't allowed then we should prohibit the user for setting one during creation.
With respect to your workflow however there is an alternative that I think would work: terraform's lifecycle ignore_changes
resource "aws_instance" "example" {
# ...
lifecycle {
ignore_changes = [
# Ignore changes to tags, e.g. because a management agent
# updates these based on some ruleset managed elsewhere.
tags,
]
}
}
that way you should be able to use a password such as unknown
after importation and terraform should ignore that attribute.
Thanks for the PR @zippy1981, i've left some comments inline |
@katbyte Edited and clarified. Thanks for the feedback and quick response! |
Ok that should work. I'll close this PR after verifying.
…On Sun, Apr 28, 2019, 6:17 PM kt ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In website/docs/r/sql_server.html.markdown
<#3308 (comment)>
:
>
-* `administrator_login_password` - (Required) The password associated with the `administrator_login` user. Needs to comply with Azure's [Password Policy](https://msdn.microsoft.com/library/ms161959.aspx)
+* `administrator_login_password` - (Required) The password associated with the `administrator_login` user. Needs to comply with Azure's [Password Policy](https://msdn.microsoft.com/library/ms161959.aspx). If the value is blank, and the server already exists, it will not attempt to change the password.
Unfortunately from the POV of terraform this is a bug, what is in the
cfg/tfstate should match what reality is and additionally if a blank
password isn't allowed then we should prohibit the user for setting one
during creation.
With respect to your workflow however there is an alternative that I think
would work: terraform's lifecycle ignore_changes
<https://www.terraform.io/docs/configuration/resources.html#ignore_changes>
resource "aws_instance" "example" {
# ...
lifecycle {
ignore_changes = [
# Ignore changes to tags, e.g. because a management agent
# updates these based on some ruleset managed elsewhere.
tags,
]
}
}
that way you should be able to use a password such as unknown after
importation and terraform should ignore that attribute.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3308 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABD34XNKSXRP3567XRN273PSYPBFANCNFSM4HIO46IQ>
.
|
Tested and ignore_changes works. |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
Document two facts: