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 support for standard auth mount tune configuration to vault_auth_backend resource #650

Conversation

ianferguson
Copy link
Contributor

@ianferguson ianferguson commented Jan 15, 2020

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

Description

Currently, updates to default_lease_ttl_seconds, max_lease_ttl_seconds or listing_visibility attributes of the vault_auth_backend resource force creation of a new resource, which invalidates existing tokens previously issued by the backend, and can result in roles under that backend no longer exiting/not being recreated, leading to outages for users depending on that backend to login.

This PR follows up on the PR @leominov opened in #557, but implemented using the suggestion by @shwuandwing in this comment: #557 (comment)

It does not strictly speaking resolve #315, in that the existing parameters will still force a new resource, but it addresses the root issue but deprecating those existing parameters and introducing a tune configuration block consistent with what has been done in the vault_github_auth_backend and vault_jwt_auth_backend.

The common tune schema and implementation does allow for updating those values without forcing a new resource.

Relates #269
Relates #315
Relates #557

Release note for CHANGELOG:

- Adds support to `vault_auth_backend` for common backend tune parameters. Also allows updating Max TTL, Default TTL and Visibility Listing tuning settings on `vault_auth_backend` without forcing a new resource.

Output from acceptance testing:

> env TESTARGS="-run TestAccAuthBackend.*" make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestAccAuthBackend.* -timeout 120m
?       github.com/terraform-providers/terraform-provider-vault [no test files]
?       github.com/terraform-providers/terraform-provider-vault/cmd/coverage    [no test files]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/util    (cached) [no tests to run]
=== RUN   TestAccAuthBackend_importBasic
--- PASS: TestAccAuthBackend_importBasic (0.14s)
PASS
ok      github.com/terraform-providers/terraform-provider-vault/vault   0.168s

> env TESTARGS="-run TestResourceAuth.*" make testacc
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run TestResourceAuth.* -timeout 120m
?       github.com/terraform-providers/terraform-provider-vault [no test files]
?       github.com/terraform-providers/terraform-provider-vault/cmd/coverage    [no test files]
testing: warning: no tests to run
PASS
ok      github.com/terraform-providers/terraform-provider-vault/util    (cached) [no tests to run]
=== RUN   TestResourceAuth
--- PASS: TestResourceAuth (0.23s)
=== RUN   TestResourceAuthTune
--- PASS: TestResourceAuthTune (0.21s)
=== RUN   TestResourceAuthTuneTtlConflict
--- PASS: TestResourceAuthTuneTtlConflict (0.02s)
=== RUN   TestResourceAuthMigrateToTune
--- PASS: TestResourceAuthMigrateToTune (0.23s)
PASS
ok      github.com/terraform-providers/terraform-provider-vault/vault   0.734s
...

@ghost ghost added the size/L label Jan 15, 2020
@ianferguson ianferguson changed the title Add support for standard auth mount tune configuration to vault_auth_mount resource Add support for standard auth mount tune configuration to vault_auth_backend resource Jan 15, 2020
@tyrannosaurus-becks tyrannosaurus-becks self-assigned this Jan 31, 2020
Copy link
Contributor

@tyrannosaurus-becks tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

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

Hi! Thanks for working on this, it's much appreciated.

I have a general question regarding this PR - is it fully backwards compatible? So, would folks who are presently using this need to make any changes to their configs?

vault/resource_auth_backend.go Show resolved Hide resolved
@ianferguson
Copy link
Contributor Author

@tyrannosaurus-becks thank you for the review! To answer you questions:

I have a general question regarding this PR - is it fully backwards compatible?

Yes!

So, would folks who are presently using this need to make any changes to their configs?

Current users will not need to make any changes, and users currently using the existing attributes will be able to move from the now deprecated attributes to the new/standard tune block attributes safely without generating a new resource.

I think this test case exercises a scenario where the user has just the configuration that exists today (the first step) as well as what happens when a user edits the configuration to use the same ttl attributes that are now in the tune block, in the second step of the test:

https://github.com/terraform-providers/terraform-provider-vault/pull/650/files#diff-e2b851590904030e7bfcdb6a3f03663fR302-R339

Copy link
Contributor

@tyrannosaurus-becks tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

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

Fantastic! Thank you! I circled back to this and tested it all out and it looks great. Much appreciated.

@tyrannosaurus-becks tyrannosaurus-becks merged commit 8d85663 into hashicorp:master Feb 4, 2020
@ianferguson
Copy link
Contributor Author

@tyrannosaurus-becks thank you so much!

@ianferguson ianferguson deleted the support_auth_backend_tune_without_force_create branch February 4, 2020 18:04
@Ninir
Copy link
Contributor

Ninir commented Feb 6, 2020

@tyrannosaurus-becks can we add this PR to the Changelog? 🙏 it seems to be missing

@tyrannosaurus-becks
Copy link
Contributor

@Ninir , whoops, oversight on my part! Just added, thanks for bringing this to my attention.

@Andor
Copy link
Contributor

Andor commented Feb 13, 2020

I think you need to update the documentation as well: mark old parameters as deprecated and provide the example.

@threemachines
Copy link

Agreed, this new tune parameter needs to be in the docs.

@ianferguson
Copy link
Contributor Author

@Andor @threemachines I've opened #697 to update the documentation to match these changes, sorry about that.

dandandy pushed a commit to dandandy/terraform-provider-vault that referenced this pull request Jun 17, 2021
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.

vault_auth_backend should not ForceNew on every field
5 participants