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

VAULT-6575 Vault agent respects retry config even with caching set #16970

Merged
merged 3 commits into from
Sep 6, 2022

Conversation

VioletHynes
Copy link
Contributor

@VioletHynes VioletHynes commented Aug 31, 2022

It seems like ignoring retry config with caching set was originally intentional behaviour, but it definitely doesn't seem right to me nor I guess the many, many customers that have reported it as a bug.

With this change, Agent will respect the retry settings, regardless of whether caching is enabled (in other words, enabling caching will have the same retry behaviour as not doing). Note that it will still continue to check the cache and the layering still works fine.

For those interested (as we're new to Agent on Core), I used the following config HCL for this change (I modified bits to reproduce different cases, of course):

violet@violet-TX54KFJWXM vault % cat ../vault_agent.hcl
pid_file = "/tmp/pidfile"

vault {
  address = "http://127.0.0.1:8200"
  tls_skip_verify = true
  retry {
    num_retries = 2
  }
}

cache {
 use_auto_auth_token = true
}

listener "tcp" {
    address = "127.0.0.1:8100"
    tls_disable = true
}

template {
  contents     = "{{ with secret \"secret/my-secret\" }}{{ .Data.data.foo }}{{ end }}"
  destination  = "/tmp/agent/render-content.txt"
}

telemetry {
}

auto_auth {
  method {
    type      = "approle"
    config = {
      role_id_file_path = "/Users/violet/Repositories/vault-agent/role-id"
      secret_id_file_path = "/Users/violet/Repositories/vault-agent/secret-id"
    }
  }
  sink {
    type = "file"
    config = {
      path = "/Users/violet/vault/token"
    }
  }
}

@VioletHynes VioletHynes marked this pull request as ready for review August 31, 2022 20:20
@VioletHynes VioletHynes requested a review from a team August 31, 2022 20:20
Copy link
Contributor

@hghaf099 hghaf099 left a comment

Choose a reason for hiding this comment

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

LGTM, just dropped a minor question.

@VioletHynes
Copy link
Contributor Author

I did some additional testing with a K8S persistent cache enabled just to be extra 100% comfy, i.e. Agent running in K8S with the following config:

cache {
  use_auto_auth_token = true
  persist "kubernetes" {
    path = "/vault/agent-cache"
  }
}

Caching and retries both seem to be working as expected, so I'm especially encouraged that this works as expected.

Copy link
Contributor

@ccapurso ccapurso left a comment

Choose a reason for hiding this comment

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

Looks good to me

@VioletHynes
Copy link
Contributor Author

VioletHynes commented Sep 1, 2022

Note: this PR closes #15725

@calvn
Copy link
Member

calvn commented Sep 2, 2022

I believe that the reason we ignored the retry config is because the cache's client does its own set of retries, which can be set via VAULT_MAX_RETRIES on the host that's running agent. If we enable config retries, it would increase the number of total retries multiplicatively with client retries.

@VioletHynes
Copy link
Contributor Author

VioletHynes commented Sep 2, 2022

I believe that the reason we ignored the retry config is because the cache's client does its own set of retries, which can be set via VAULT_MAX_RETRIES on the host that's running agent. If we enable config retries, it would increase the number of total retries multiplicatively with client retries.

Anecdotally (I could be missing something?), through testing it, this does not work (and this was reported as not working by a customer, too). Setting this environment variable does not seem to do anything to affect retries in Agent, whether or not caching is turned on or off.

@calvn
Copy link
Member

calvn commented Sep 2, 2022

Ah that's right. When caching is enabled, templating hands off retries to that part of Agent since we recently made templating requests route through the cache's listener instead of to Vault directly (to take advantage of Agent's caching capability and reduce number of template requests against Vault).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants