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

Move default token helper to api #25744

Merged
merged 13 commits into from
Mar 4, 2024
Merged

Conversation

tomhjp
Copy link
Contributor

@tomhjp tomhjp commented Mar 1, 2024

The Vault Terraform provider currently depends on github.com/hashicorp/vault.

As documented elsewhere, this isn't a supported dependency and puts the provider at risk of being broken by any updates. Most of the code depended on is trivial like constants, or test dependencies like preparing test containers. The hardest dependency to remove is the fact that it depends on the CLI's DefaultTokenHelper: https://github.com/hashicorp/terraform-provider-vault/blob/cb919ae6c040cdfc5513b635ec9c4b4762e24ca4/internal/provider/meta.go#L627

This PR moves the two related packages (command/config and command/token) over to the api package so that the provider can continue to share that non-trivial functionality and work towards removing the hashicorp/vault dependency.

For the initial draft I've chosen api over sdk because it seems like a reasonable piece of functionality for any consumer of the API to want to use.

Note that I've tried to create a sequence of commits that do one thing each with very limited scope, so it may be easier to review commit-by-commit.

@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Mar 1, 2024
@tomhjp tomhjp requested a review from benashz March 1, 2024 15:15
Copy link

github-actions bot commented Mar 1, 2024

CI Results:
All Go tests succeeded! ✅

Copy link

github-actions bot commented Mar 1, 2024

Build Results:
All builds succeeded! ✅

@tomhjp tomhjp force-pushed the move-default-token-helper-to-api branch from 0035810 to b429d8a Compare March 1, 2024 15:31
Copy link
Contributor

@VioletHynes VioletHynes 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 good to me! I gave thought to api vs sdk and ultimately I think this makes the most sense: a token is primarily to access Vault's APIs.

Thanks for this -- excited to reduce usage of the Vault dependency.

I wanted to note that there's a related community PR we should close when we merge this: #9786

Copy link

@peteski22 peteski22 left a comment

Choose a reason for hiding this comment

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

@tomhjp
Copy link
Contributor Author

tomhjp commented Mar 4, 2024

Thank you both!

@tomhjp tomhjp merged commit 9ed0082 into main Mar 4, 2024
83 checks passed
@tomhjp tomhjp deleted the move-default-token-helper-to-api branch March 4, 2024 18:29
@orirawlings
Copy link
Contributor

I just want to say thanks. I've been hoping for this for several years, but didn't have the time to dig in myself. I hope that default token helper behavior becomes a more common fixture in the vault libraries for other languages because it's a great way to encapsulate simultaneous login sessions to multiple clusters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed pr/no-milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants