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

Extra config options for auth_backend #245

Merged

Conversation

martinssipenko
Copy link
Contributor

Adds ability to configure following config parameters for auth_backend:

  • default_lease_ttl_seconds
  • max_lease_ttl_seconds
  • listing_visibility
  • local

Test output:

$ make testacc TESTARGS='-run=TestResourceAuth'
==> 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]
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.11s)
PASS
ok      github.com/terraform-providers/terraform-provider-vault/vault   0.139s

Fixes #203

…e_ttl_seconds, listing_visibility & local paramaters
@ghost ghost added the size/M label Nov 23, 2018
@cvbarros
Copy link
Contributor

cvbarros commented Nov 23, 2018

This is a nicely needed improvement! I'm working on a new auth_backend resource and I've noted a few points:

IMO these auth backend resources need a bigger refactor, whereas the resource mounts/unmounts both the auth backend + configures its options.

Adding these configuration options to the "generic" resource auth_backend already increases this debt as put the newly created in fd5deed without these options.

IMHO, if still proceeding this PR, these options should also be added to the gcp_auth_backend plus any other auth backend resources that have the same approach.

@martinssipenko
Copy link
Contributor Author

martinssipenko commented Nov 23, 2018

@cvbarros I agree, probably the best option would be a resource per each mount type (aws, gcp, github, etc).

The update (unmount/mount) part can be fixed using partial update and mount tune, similar to how it has been done in aws_secret_backend.

@martinssipenko
Copy link
Contributor Author

@tyrannosaurus-becks given the above, I'm happy to add a new resource specifically for aws instead of amending the generic one. What's your view on this?

@kmcquade
Copy link

@martinssipenko @tyrannosaurus-becks @cvbarros - any chance you guys could also address the auth tune request here as well? See #234

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.

@martinssipenko @cvbarros I am flexible on the approach around whether to add these parameters per-mount-type or onto the generic backend. This code is good to go so I don't want to create more work over it. Let's use it!

@@ -95,9 +135,7 @@ func authBackendDelete(d *schema.ResourceData, meta interface{}) error {

log.Printf("[DEBUG] Deleting auth %s from Vault", path)

err := client.Sys().DisableAuth(path)

if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for noticing this and cleaning it up while you're at it.

@tyrannosaurus-becks tyrannosaurus-becks merged commit 9e37f5c into hashicorp:master Dec 11, 2018
@martinssipenko martinssipenko deleted the auth_backend_config branch December 11, 2018 17:15
@cvbarros
Copy link
Contributor

@kmcquade There's an approach submitted in #255, which can be leveraged to all auth mounts regarding these options. Once that PR is discussed/merge I can happily extend it to other auth mounts as well

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.

4 participants