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

Fix issue where removing optional fields in database secrets backend connection resource did not reset the fields to their default values #1737

Merged
merged 9 commits into from
Feb 4, 2023

Conversation

maxcoulombe
Copy link
Contributor

@maxcoulombe maxcoulombe commented Jan 24, 2023

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Overview

The username & disable_escaping fields are not managed correctly causing unresolvable drift or bad updates if a previously defined value is removed from the stanza.

To reproduce:

  • Create a database backend connection resource with a username field and disable_escaping set to true in the DB type's stanza, example for postgre:
provider "vault" {
  address = "http://127.0.0.1:8200"
  token   = "my-token"
}

resource "vault_mount" "my-db" {
  path = "postgres"
  type = "database"
}

resource "vault_database_secret_backend_connection" "my-connection" {
  backend = vault_mount.my-db.path
  name    = "postgres"

  postgresql {
    connection_url     = "postgres://my-username:my-password@localhost:5432"
    username             = "my-username"
    disable_escaping = true
  }
}
  • terraform apply
  • Remove the username and disable_escaping fields from the stanza
resource "vault_database_secret_backend_connection" "my-connection" {
  (...)
  postgresql {
    connection_url       = "postgres://my-username:my-password@localhost:5432"
    #username             = "my-username"
    #disable_escaping = true
  }
}
  • terraform apply
  • even if the apply is successful, subsequent terraform plan will show unexpected drift:
Terraform will perform the following actions:

  # vault_database_secret_backend_connection.postgresql_role will be updated in-place
  ~ resource "vault_database_secret_backend_connection" "postgresql_role" {
        id                       = "postgres/config/postgres"
        name                     = "postgres"
        # (6 unchanged attributes hidden)

      ~ postgresql {
          - username                = "my-username" -> null
          - disable_escaping    = true -> null
            # (5 unchanged attributes hidden)
        }
    }

New behaviour:

  • Following a successful terraform apply, subsequent terraform plan shows everything is up-to-date indicating that the statefile was updated correctly and the artificial drift is gone. The disable_escaping field also reverts back to its default false value in Vault.
No changes. Your infrastructure matches the configuration.

This is backward compatible. Users currently experiencing this issue will have to execute a successful apply once after upgrading their provider to a version with the fix, then all subsequent plans will no longer show the unwanted drift.

Changelog:

BUGS: Fix issue where removing optional fields in database secrets backend connection resource with postgresql did not reset fields to the default values

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccDatabaseSecretBackendConnection_postgresql'
2023/01/24 09:39:05 [INFO] Using Vault token with the following policies: root
=== RUN   TestAccDatabaseSecretBackendConnection_postgresql
--- PASS: TestAccDatabaseSecretBackendConnection_postgresql (0.66s)
PASS

@maxcoulombe maxcoulombe marked this pull request as draft January 24, 2023 20:43
@maxcoulombe maxcoulombe changed the title * fixed issue where removing username caused permanent drift Fix issue where removing the "username" field in database secrets backend connection resources caused unresolvable drift in the terraform plan Jan 24, 2023
@maxcoulombe maxcoulombe changed the title Fix issue where removing the "username" field in database secrets backend connection resources caused unresolvable drift in the terraform plan Fix issue where removing optional fields in database secrets backend connection resource with postgresql did not reset fields to the default values Feb 1, 2023
@github-actions github-actions bot added size/M and removed size/XS labels Feb 1, 2023
@maxcoulombe maxcoulombe marked this pull request as ready for review February 1, 2023 20:36
@maxcoulombe maxcoulombe changed the title Fix issue where removing optional fields in database secrets backend connection resource with postgresql did not reset fields to the default values Fix issue where removing optional fields in database secrets backend connection resource did not reset the fields to their default values Feb 1, 2023
@maxcoulombe maxcoulombe marked this pull request as draft February 1, 2023 21:53
Config: testAccDatabaseSecretBackendConnectionConfig_postgresql_reset_optional_values(name, backend, parsedURL),
PlanOnly: true,
ExpectNonEmptyPlan: false,
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wondering if there's (or should there be?) a more built-in way to check this by default? My understanding is that for 99.9% of cases after a successful terraform apply a subsequent plan should show no changes, otherwise it's an unintended drift bug.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just confirmed that the Terraform SDK by default will run a terraform plan after each test step to ensure that the the updates take place with no drift. Adding a test step that causes the plan to be non-empty results in an error from the SDK after the test step, something like:

=== RUN   TestAccDatabaseSecretBackendConnection_postgresql
    resource_database_secret_backend_connection_test.go:771: Step 2/3 error: After applying this test step, the plan was not empty.
        stdout:
        
        
        Terraform used the selected providers to generate the following execution
        plan. Resource actions are indicated with the following symbols:
          ~ update in-place
        
        Terraform will perform the following actions:
        
          # vault_database_secret_backend_connection.test will be updated in-place
          ~ resource "vault_database_secret_backend_connection" "test" {
                id                       = "tf-test-db-3935684109521394435/config/db-4017655576009088209"
                name                     = "db-4017655576009088209"
                # (6 unchanged attributes hidden)
        
              ~ postgresql {
                  - disable_escaping        = true -> null
                  - username                = "postgres" -> null
                    # (4 unchanged attributes hidden)
                }
            }

@maxcoulombe maxcoulombe marked this pull request as ready for review February 2, 2023 20:46
Copy link
Contributor

@vinay-gopalan vinay-gopalan left a comment

Choose a reason for hiding this comment

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

Looks good after removing the extra test step, and thanks for adding a fix for this!

@vinay-gopalan vinay-gopalan added this to the 3.13.0 milestone Feb 3, 2023
@maxcoulombe maxcoulombe merged commit 56ad1db into main Feb 4, 2023
@maxcoulombe maxcoulombe deleted the vault-7914-UsernameNotUpdated branch February 4, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants