Fix DB engine password overwrite #1876
Conversation
| PreConfig: func() { | ||
| client := testProvider.Meta().(*provider.ProviderMeta).GetClient() | ||
| rotateRootPath := fmt.Sprintf("%s/rotate-root/%s", backend, name) | ||
| resp, err := client.Logical().Write(rotateRootPath, nil) |
There was a problem hiding this comment.
The rotate root does not seem to be working as expected. At present this test passes even when it should fail since no root rotation is actually taking place. Looking into this a bit more
| // if it actually changed to still support updating it for non-rotated cases. | ||
| passwordKey := prefix + consts.FieldPassword | ||
| if v, ok := d.GetOk(passwordKey); ok { | ||
| if d.IsNewResource() || d.HasChange(passwordKey) { |
There was a problem hiding this comment.
The first d.IsNewResource case might be redundant. Will double check and remove if it is so
…ented root password from rotating
| backend = vault_mount.db.path | ||
| name = "%s" | ||
| allowed_roles = ["dev", "prod"] | ||
| root_rotation_statements = [] |
There was a problem hiding this comment.
@vinay-gopalan previously this was [""], which caused Vault to interpret it as a non empty list of statements, so instead of defaulting to the statement ALTER ROLE "{{username}}" WITH PASSWORD '{{password}}';, it just skipped it entirely. That's the reason why the Postgres password never got updated, even though it was changed in Vault.
There was a problem hiding this comment.
Ah interesting! Thanks for catching that and whoops 😅
| MaybeSkipDBTests(t, dbEnginePostgres) | ||
|
|
||
| username := "postgres" | ||
| password := "secret" |
There was a problem hiding this comment.
since there's already a Postgres container running in CI, I've updated the test to use that existing configuration instead.
There was a problem hiding this comment.
Wouldn't that cause the Postgres container in CI to have it's credentials rotated? Thereby causing our other tests that use that same container to fail?
My thought was to have this test be a local only test where we will have to set up a Docker container locally — with the knowledge that the container's password will get rotated out after and it won't be usable after this test. The idea would be the container is only set up for that test and then destroyed once it's no longer usable by other tests
Curious to hear thoughts from folks! I don't think we have any rotate root tests in CI currently so this might be new areas that might need to be documented.
There was a problem hiding this comment.
Yeah I have it noted as a comment here.
This would have to be revisited if we introduced new tests that also relied on the same PG container.
There was a problem hiding this comment.
I'll see if additional reruns causes any flakiness, and if so, I'll skip this test in CI.
There was a problem hiding this comment.
Alternatively we can introduce https://github.com/ory/dockertest, which it looks like we're already using in Vault.
docker-compose.yaml
Outdated
| @@ -56,3 +56,11 @@ services: | |||
| - LDAP_ADMIN_PASSWORD=adminpassword | |||
| - LDAP_USERS=alice,bob,foo | |||
| - LDAP_PASSWORDS=password1,password2,password3 | |||
|
|
|||
| postgres: | |||
There was a problem hiding this comment.
Do we really need an entirely new container for this?
Can we just reuse the existing mysql container for this and the existing rotate root test TestAccDatabaseSecretBackendConnectionTemplatedUpdateExcludePassword_mysql? If we do end up reusing that test can we just assert that the user and password are not changed in the TF state after the rotate root operation?
There was a problem hiding this comment.
That seems reasonable to use mysql to test this change. I'm fine either way, but leaning towards a new unit test to make it obvious what we're testing for. @vinay-gopalan thoughts? I know you had originally wrote the test for Postgres.
There was a problem hiding this comment.
I don't want this comment to block progress. So feel free to disregard if you see fit. However, it seems to me that the TestAccDatabaseSecretBackendConnectionTemplatedUpdateExcludePassword_mysql test is intended to test this exact scenario but it fails to assert that the password is unchanged in the state after the rotate root operation.
There was a problem hiding this comment.
But, presumably, TestAccDatabaseSecretBackendConnectionTemplatedUpdateExcludePassword_mysql would have failed that assertion before this change so maybe I am wrong there.
Adds a fix to only update the password if it is a new resource or if there is a change. This fix helps the provider maintain state even when an external rotate root command has been initiated for the DB connection's password