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.

Sign up for GitHub

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

remove redundant vault api retry logic #16143

Merged
merged 2 commits into from
Feb 7, 2023
Merged

remove redundant vault api retry logic #16143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
82c26e4
remove redundant vault api retry logic
eikenb Feb 2, 2023
4978de0
add mention of re-configuring provider in comments
eikenb Feb 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 8 additions & 28 deletions agent/connect/ca/provider_vault.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"github.com/hashicorp/consul/agent/structs"
"github.com/hashicorp/consul/lib"
"github.com/hashicorp/consul/lib/decode"
"github.com/hashicorp/consul/lib/retry"
)

const (
Expand All @@ -46,14 +45,12 @@ const (
VaultAuthMethodTypeUserpass = "userpass"

defaultK8SServiceAccountTokenPath = "/var/run/secrets/kubernetes.io/serviceaccount/token"

retryMin = 1 * time.Second
retryMax = 5 * time.Second
retryJitter = 20
)

var ErrBackendNotMounted = fmt.Errorf("backend not mounted")
var ErrBackendNotInitialized = fmt.Errorf("backend not initialized")
var (
ErrBackendNotMounted = fmt.Errorf("backend not mounted")
ErrBackendNotInitialized = fmt.Errorf("backend not initialized")
)

type VaultProvider struct {
config *structs.VaultCAProviderConfig
Expand Down Expand Up @@ -100,6 +97,7 @@ func vaultTLSConfig(config *structs.VaultCAProviderConfig) *vaultapi.TLSConfig {
}

// Configure sets up the provider using the given configuration.
// Configure supports being called multiple times to re-configure the provider.
func (v *VaultProvider) Configure(cfg ProviderConfig) error {
config, err := ParseVaultCAConfig(cfg.RawConfig)
if err != nil {
Expand Down Expand Up @@ -176,6 +174,7 @@ func (v *VaultProvider) Configure(cfg ProviderConfig) error {

ctx, cancel := context.WithCancel(context.Background())
if v.stopWatcher != nil {
// stop the running watcher loop if we are re-configuring
v.stopWatcher()
}
v.stopWatcher = cancel
Expand Down Expand Up @@ -225,41 +224,25 @@ func (v *VaultProvider) renewToken(ctx context.Context, watcher *vaultapi.Lifeti
go watcher.Start()
defer watcher.Stop()

// TODO: Once we've upgraded to a later version of protobuf we can upgrade to github.com/hashicorp/vault/api@1.1.1
// or later and rip this out.
retrier := retry.Waiter{
MinFailures: 5,
MinWait: retryMin,
MaxWait: retryMax,
Jitter: retry.NewJitter(retryJitter),
}

for {
select {
case <-ctx.Done():
return

case err := <-watcher.DoneCh():
// In the event we fail to login to Vault or our token is no longer valid we can overwhelm a Vault instance
// with rate limit configured. We would make these requests to Vault as fast as we possibly could and start
// causing all client's to receive 429 response codes. To mitigate that we're sleeping 1 second or less
// before moving on to login again and restart the lifetime watcher. Once we can upgrade to
// github.com/hashicorp/vault/api@v1.1.1 or later the LifetimeWatcher _should_ perform that backoff for us.
// Watcher has stopped
if err != nil {
v.logger.Error("Error renewing token for Vault provider", "error", err)
}

// wait at least 1 second after returning from the lifetime watcher
retrier.Wait(ctx)

// If the watcher has exited and auth method is enabled,
// re-authenticate using the auth method and set up a new watcher.
if v.config.AuthMethod != nil {
// Login to Vault using the auth method.
loginResp, err := vaultLogin(v.client, v.config.AuthMethod)
if err != nil {
v.logger.Error("Error login in to Vault with %q auth method", v.config.AuthMethod.Type)
// Restart the watcher
Copy link
Collaborator

Choose a reason for hiding this comment

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

most inconsequential nit: these comment still help draw attention to the watcher.Start() lines below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've always thought of comments as useful to say why something is done. That if what or how isn't clear it would indicate a code refactor is needed. That is, IMO, that it reads cleaner/clearer this way as comment doesn't break the flow of the code.


go watcher.Start()
continue
}
Expand All @@ -279,12 +262,10 @@ func (v *VaultProvider) renewToken(ctx context.Context, watcher *vaultapi.Lifeti
continue
}
}
// Restart the watcher.

go watcher.Start()

case <-watcher.RenewCh():
retrier.Reset()
v.logger.Info("Successfully renewed token for Vault provider")
}
}
Expand Down Expand Up @@ -430,7 +411,6 @@ func (v *VaultProvider) setupIntermediatePKIPath() error {
"error", err,
)
}

}
}

Expand Down
2 changes: 1 addition & 1 deletion agent/connect/ca/provider_vault_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ func TestVaultCAProvider_RenewToken(t *testing.T) {
firstRenewal, err := secret.Data["last_renewal_time"].(json.Number).Int64()
require.NoError(t, err)

// Wait past the TTL and make sure the token has been renewed.
// Retry past the TTL and make sure the token has been renewed.
retry.Run(t, func(r *retry.R) {
secret, err = testVault.client.Auth().Token().Lookup(providerToken)
require.NoError(r, err)
Expand Down