Fix: move DefaultTokenHelper to vault/sdk

[greut]

Some projects, such as terraform-provider-nomad are relying on the DefaultTokenHelper which breaks the idea of only using vault/api, or vault/sdk. This moves those to a new location.

As vault 1.5.0 cannot be used via go.mod, it's shouldn't be adding more breakage. cf. #9575

[jmthvt]

Facing the exact same case 2+ years later.
Importing vault is unsupported and discouraged, wouldn't this PR make sense?

[greut]

@jmthvt maybe. I've updated the branch to resolve the conflicts they were.

[maxb]

Wouldn't it make more sense to put this in vault/api rather than vault/sdk ?

After all, vault/api is (mostly) about writing code that talks to the Vault API, whereas vault/sdk is (mostly) about implementing Vault plugins.

[greut]

Wouldn't it make more sense to put this in vault/api rather than vault/sdk ?

One if full with helpers already.

https://github.com/hashicorp/vault/tree/main/sdk/helper

I didn't see where I could put it in the other:

https://github.com/hashicorp/vault/tree/main/api

[ncabatoff]

Before we worry about api vs sdk, can someone explain what the purpose behind making this available to other projects is? What problem does it solve?

In other words, maybe the solution isn't making it easier to import, maybe the entire idea behind depending on this func in other projects is wrongheaded.

[ncabatoff]

Before we worry about api vs sdk, can someone explain what the purpose behind making this available to other projects is? What problem does it solve?

Ah nevermind, I looked at #4055, as I should've before commenting. I get it now. I prefer sdk over api, as this isn't something that a typical vault client should care about.

[ncabatoff]

I dug a bit more into this and I have a different proposal. Instead of making DefaultTokenHelper available to other repos like these terraform providers, how about we add the desired functionality into api.Client itself? In other words, these projects are using DefaultTokenHelper because they want to behave like Vault CLI commands, in that $HOME/.vault-token gets used if $VAULT_TOKEN isn’t set. So let's make that easy by adding an option to api.Config, say a bool UseTokenFile, and when that's set NewClient will read $HOME/.vault-token. This will be less disruptive than moving these packages to the sdk, and will make it easier for other projects to take advantage of this functionality without adding another dependency. It will also make it easier to discover.

[jmthvt]

For clarity, DefaultTokenHelper looks for a token in $HOME/.vault-token and from external token helpers, if a token_helper path is defined.

[james0209]

Is there any movement on this PR? Did we come to a consensus? I am facing the same issue regarding the discouraged import for DefaultTokenHelper almost a year after the last comment on this PR.

[banks]

@ncabatoff and I discussed this PR again last week and came away with the following thoughts:

We still somewhat like the idea of this being more generally the behaviour of the API package that Nick considered before, however that has a few shortcomings too:

1. It's a larger change that requires more thought
2. As the comments on the helper itself point out, this isn't always the ideal behaviour. It's mainly useful for short-lived scripts or CLI commands since there is no provision for rotation of tokens, so making it the default API client behaviour is risky and probably incorrect for a large portion of clients that would be embedded in long-running services.

On that basis, it seems perfectly reasonable and more pragmatic to merge this PR essentially as it is (once it's been rebased).

There was a question raised about whether this should live in api instead of sdk. While that does make some sense on the current usage of sdk, we prefer to keep this in sdk for now because:

• Long term we hope to move to a different API package that is generated code. Having extra helpers like this in api would then need to be duplicated there or involve needing to import two different versions of the api package. We might review this later.
• It's useful but it's not universally needed by most api clients currently so it feels reasonable to import the more complete sdk if you want this additional functionality.

We may review either in the future, but in the interest of getting this merged it seems like the best near-term solution is to merge it pretty much as is!

Thanks @greut for the contribution and sorry it got stuck in decision mode for so long. If you would like to retain all the commit credit please feel free to rebase/merge latest changes here so we can get it merged. But equally happy to pick it up since it's been a while!

[tomhjp]

Apologies, I didn't see this issue before opening #25744, wanting to solve exactly the same issue myself. I also then didn't read it carefully enough before merging because I see now that this thread/PR was leaning towards putting it in the sdk package, whereas I put it in the api package. However, on the plus side we're very close to solving the underlying problem (just need to tag the module to make it properly consumable).

Before I tag it, I just wanted to give a brief opportunity for anyone in this thread to strongly object to the api module choice I made.

[tomhjp]

I've just pushed api tag v1.12.1 with the DefaultTokenHelper() function exported from the cliconfig package, so I'll close this now. Sorry we didn't get this merged but thank you very much for the contribution ❤️

[greut]

@tomhjp thanks a bunch! The Nomad provider can get rid of a ton of dependencies now, hashicorp/terraform-provider-nomad#445