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

only update the password if it has changed #2263

Merged
merged 1 commit into from
Dec 12, 2018

Conversation

kung-foo
Copy link
Contributor

@kung-foo kung-foo commented Nov 7, 2018

When updating an azurerm_sql_server resource, all of the properties are pushed to the Azure API, even if nothing has changed.

This PR addresses only the administrator_login_password field, and not any other fields.

Motivation: When creating an azurerm_sql_server instance, the API requires setting a new password. But this means it is both in source code and the state file. So our approach is to set it to something random, and then later, in a separate process (e.g. using data from Key Vault) set it to something secure. The problem comes when something else changes on the resource the results in an update (e.g. adding a tag). The existing random password is then sent along with the update.

I am not 100% sure this won't introduce a different bug since the read API doesn't return the password. So I am not sure how the diff routine works in that case. Meaning usually terraform will complain if you change something server side, but in this case, terraform has no way of knowing if it has changed.

@ghost ghost added the size/XS label Nov 7, 2018
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accidentally commented

@katbyte katbyte added this to the 1.19.0 milestone Nov 8, 2018
@katbyte katbyte added service/mssql Microsoft SQL Server bug labels Nov 8, 2018
katbyte
katbyte previously requested changes Nov 8, 2018
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @kung-foo,

Thank you for fixing this, the change LGTM however I would like to see acceptance tests for this to make sure of two things:

  1. changes to the password in terraform are still persisted
  2. that if the password is changed outside terraform it is not overwritten

I would suggest using the standard GO db library to connect to the DB and confirm the password / change it.

Let me know if you would like any more details or help writing the tests 🙂

@katbyte katbyte removed this from the 1.19.0 milestone Nov 8, 2018
@tombuildsstuff tombuildsstuff self-assigned this Dec 4, 2018
@tombuildsstuff
Copy link
Contributor

I've taken a quick look into writing some tests for this and that approach seems more complicated than we first thought - as such I'm going to suggest we skip that for the moment.

Tests pass:

screenshot 2018-12-12 at 17 38 01

@tombuildsstuff tombuildsstuff dismissed katbyte’s stale review December 12, 2018 17:40

looked into this but this isn't so easy due to DNS replication

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks for this @kung-foo

@ghost
Copy link

ghost commented Mar 5, 2019

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 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 5, 2019
@ghost ghost removed the waiting-response label Mar 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants