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 token name provider argument #594

Merged
merged 4 commits into from
Dec 5, 2019

Conversation

tstoermer
Copy link
Contributor

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

Introduce a provider argument to define the Vault child token name used by Terraform. This is useful to provide a reference of the Terraform run traceable in Vault audit log, e.g. commit hash or id of the CI/CD execution job.

Release note for CHANGELOG:

Add `token_name` provider argument defining the Vault child token name

Output from acceptance testing:
N/A

@tstoermer
Copy link
Contributor Author

This will no longer work with #561

@tstoermer tstoermer marked this pull request as ready for review November 4, 2019 17:35
@tyrannosaurus-becks tyrannosaurus-becks self-assigned this Nov 4, 2019
@tyrannosaurus-becks
Copy link
Contributor

Hi @tstoermer ! Thanks for this PR!

Happy to look at it when it's ready for review. Per CONTRIBUTING.md it still needs test coverage.

Also, the linked PR will likely move forward because it simply provides the option of not using a child token. I haven't checked out this code yet, but if there's some way to ensure this code works with that PR, that would be awesome.

@tstoermer
Copy link
Contributor Author

Hi @tyrannosaurus-becks, thanks for your first feedback.

Please correct me, if I overlook anything - I was thinking about the two PRs.

The linked PR will skip the child token creation for all tokens that are orphan tokens, e.g.:

  • a token created with Auth methods AppRole or AWS for CI/CD
  • personal token created with OIDC

This will limit the usage of token name for the vault provider child token.

I would propose to change the other PR using a provider argument flag controlling the creation of a child token (default: create child token for backward compatibility). This would:

  • provide full control for terraform user
  • enable full usage of both PR features

Another thing to consider:
If one of the above orphan token examples will create a child token (that will have orphan=false) and pass it to terraform, the provider would again create a child token.
What would be the intention for this? Maybe I'm missing a fact?

@ghost ghost added size/L and removed size/XS labels Nov 20, 2019
@tstoermer
Copy link
Contributor Author

Hi @tyrannosaurus-becks,
I added some acceptance tests checking the token_name usage. Please have a look, hope I did it the right way :)

Local results:

make testacc TESTARGS='-run=TestAccTokenName'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccTokenName -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   TestAccTokenName
--- PASS: TestAccTokenName (1.94s)
PASS
ok  	github.com/terraform-providers/terraform-provider-vault/vault	1.975s

Regarding the other failed tests in circleci build:
Seems they are no longer working with new vault:1.3.0

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.

Thanks for pointing out the tests were broken on master! I merged a fix. This code looks really good. Just a couple minor things and then when the tests are passing, happy to merge it in.

Also, FWIW, I merged in master and pushed it up to a separate branch to see if the tests would pass, and they do presently: https://github.com/terraform-providers/terraform-provider-vault/compare/tstoermer-token-name.

vault/provider.go Outdated Show resolved Hide resolved
vault/provider.go Show resolved Hide resolved
vault/provider_test.go Outdated Show resolved Hide resolved
@ghost ghost added size/M and removed size/L labels Dec 5, 2019
@tstoermer
Copy link
Contributor Author

Thanks @tyrannosaurus-becks, I added your suggestions and performed a merge with current master, to make sure everything is ok.

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.

This looks fantastic! Thank you so much for doing this!

@tyrannosaurus-becks tyrannosaurus-becks merged commit 48c1f9a into hashicorp:master Dec 5, 2019
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants