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

add password_policy to vault_database_secret_backend_connection #2244

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

joshRooz
Copy link

@joshRooz joshRooz commented May 21, 2024

Description

This PR updates vault_database_secret_backend_connection resource so that we can specify a password_policy.


One observation I would like to bubble up - there's potential for drift to be introduced if a password policy is set and applied, then subsequently removed from the configuration. In that scenario, the resource will not actually reconcile the engine's configuration to a defined default. The next plan or apply will show drift that can only be resolved by the resource if a new password policy is specified.

I modeled the existing behavior defined for other resource arguments, but I would be interested in feedback on the use of d.Get in place of d.GetOk.

Checklist

  • Added CHANGELOG entry (only for user-facing changes)
  • Acceptance tests where run against all supported Vault Versions

Output from acceptance testing:

Only a subset of databases have been updated as part of the PR. I passed in the verbose argument to highlight which tests were updated. Only the tests that passed have been updated; tests that are skipped have not been changed.

$ make testacc TESTARGS='-run=^TestAccDatabaseSecretBackendConnection_\(postgresql\|mysql\)' | grep -v -E "testing: warning: no tests to run|^PASS$" 

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test -v -count 1 -run=^TestAccDatabaseSecretBackendConnection_\(postgresql\|mysql\) -timeout 30m ./...
?   	github.com/hashicorp/terraform-provider-vault	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/cmd/coverage	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/cmd/generate	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/helper	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/internal/consts	[no test files]
ok  	github.com/hashicorp/terraform-provider-vault/codegen	0.298s [no tests to run]
?   	github.com/hashicorp/terraform-provider-vault/internal/identity/group	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/internal/identity/mfa	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/internal/pki	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/internal/sync	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/schema	[no test files]
?   	github.com/hashicorp/terraform-provider-vault/util/mountutil	[no test files]
ok  	github.com/hashicorp/terraform-provider-vault/internal/identity/entity	0.422s [no tests to run]
ok  	github.com/hashicorp/terraform-provider-vault/internal/provider	0.742s [no tests to run]
ok  	github.com/hashicorp/terraform-provider-vault/testutil	0.979s [no tests to run]
ok  	github.com/hashicorp/terraform-provider-vault/util	0.832s [no tests to run]
=== RUN   TestAccDatabaseSecretBackendConnection_postgresql_import
--- PASS: TestAccDatabaseSecretBackendConnection_postgresql_import (1.15s)
=== RUN   TestAccDatabaseSecretBackendConnection_mysql_cloud
    resource_database_secret_backend_connection_test.go:448: "MYSQL_CLOUD_CONNECTION_URL" must be set
--- SKIP: TestAccDatabaseSecretBackendConnection_mysql_cloud (0.00s)
=== RUN   TestAccDatabaseSecretBackendConnection_mysql
--- PASS: TestAccDatabaseSecretBackendConnection_mysql (2.61s)
=== RUN   TestAccDatabaseSecretBackendConnection_mysql_tls
    resource_database_secret_backend_connection_test.go:730: "MYSQL_CA" must be set
--- SKIP: TestAccDatabaseSecretBackendConnection_mysql_tls (0.00s)
=== RUN   TestAccDatabaseSecretBackendConnection_postgresql
--- PASS: TestAccDatabaseSecretBackendConnection_postgresql (1.48s)
=== RUN   TestAccDatabaseSecretBackendConnection_postgresql_cloud
    resource_database_secret_backend_connection_test.go:847: "POSTGRES_CLOUD_URL" must be set
--- SKIP: TestAccDatabaseSecretBackendConnection_postgresql_cloud (0.00s)
ok  	github.com/hashicorp/terraform-provider-vault/vault	5.918s

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

@joshRooz joshRooz marked this pull request as ready for review May 21, 2024 18:54
@@ -197,6 +197,11 @@ func getCommonDatabaseSchema() schemaMap {
Type: schema.TypeString,
},
},
"password_policy": {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should set Computed: True since this returns a default if not set. That will allow TFVP to fallback to the value returned by Vault.

https://developer.hashicorp.com/vault/api-docs/secret/databases#password_policy

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for having a preliminary look @fairclothjm. Much appreciated!

I have a limited familiarity with Vault's source code, but it seems that the default password policy is never externally exposed. From a client's perspective, the default password policy is an empty string. Does that align with your reasoning for setting Computed: true?


Enabling the computed behavior resolves the drift initially observed, but the attribute will quietly remain at the previously applied value. I believe such a scenario (set and applied -> removed from configuration) would be rare, but in my opinion, it makes for an obscure UX.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants