fix: by default, if a token is found in Vaults ~/.vault-token, use thi…

[jurgenweber]

…s and renew when necessary...

as per the outrage outlined here which seems to have been ignored;

#1182

[hashicorp-cla]

All committers have signed the CLA.

[eikenb]

Thanks for the PR @jurgenweber,

This does look like a pretty bad regression. My guess is that it was missed before due to the comments being on the merged PR instead of in a new issue. I know I missed it completely due to that as I only reviewed open issues and PRs when picking up the project.

I'm sort of confused by a something thouh... Why does setting vault_agent_token_file to a value, set renew_token to false? Why not respect both configuration fields so that if you want the token file without renewing, you set your config up that way? That would allow for both the use case where you don't want it renewed (which seems was part of the initial ask for that option) and where you do what it renewed (as is the case here).

[eikenb]

To put this another way... why isn't the fix for this to just remove this line...

consul-template/config/vault.go

Line 261 in cb2ee2f

c.RenewToken = Bool(false)

Maybe with something to expand $HOME in vault_agent_token_file if that seems useful?

[eikenb]

Spoke with @kyhavlov about this and he agrees that the renew field should still be respected even though the vault_agent_token_file is set. That we should remove that above line.

He also thought it might be a good idea to just still accept ~/.vault-token as implemented here to keep backwards compatibility for now. That it would remain undocumented and only remain for compatibility.

[eikenb]

Reviewing issues and came across #1189 which this will directly fix.

[eikenb]

Oh.. and I think I like the idea presented there of making renew_token=false the default if vault_agent_token_file is set to help maintain compatibility for those who are already using it.

[jurgenweber]

Yeah, removing that line makes sense.. It should be honoured. I updated it.

[eikenb]

Thanks @jurgenweber ... but thinking about it there are probably a couple more things needed.

First is that I do think we should make an allowance for people who picked this up since the changed behavior and make renew_token default to false if vault_agent_token_file is set. Maybe something like what you have plus changing..

consul-template/config/vault.go

Lines 211 to 215 in cb2ee2f

	if c.RenewToken == nil {
	c.RenewToken = boolFromEnv([]string{
	"VAULT_RENEW_TOKEN",
	}, DefaultVaultRenewToken)
	}

to..

	if c.RenewToken == nil {
		default_renew := DefaultVaultRenewToken
		if c.VaultAgentTokenFile != nil {
			default_renew = false
		}
		c.RenewToken = boolFromEnv([]string{
			"VAULT_RENEW_TOKEN",
		}, default_renew)
	}

Or something to that end (please feel free to come up with something better).

Second, the documentation in the README.md for the vault_agent_token_file needs to be updated. Specifically the line "# - Consul Template will not try to renew the Vault token." needs to go away. I'm not sure if something needs to be said about the renew_token or not. I'll probably ask one of documentation writers for their opinion on it before finalizing it.

I can do any/all of this, but thought you might be interested.

Thanks!

[jurgenweber]

ok, added. I did not remove that comment but just explained further.. Maybe that makes more sense?

[eikenb]

Thanks. Looks good and I like your change to the comment. I think I'll add a test, but I'll commit that later.

[devlounge]

Awesome!
When will this be released?

[eikenb]

At this point I have one more issue I'd like to fix before putting out a release. So soon-ish.. depending on how tough that issue is.