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

Allow configuring login and query timeouts for Vault #6362

Merged
merged 6 commits into from
Jan 12, 2021

Conversation

evanchaoli
Copy link
Contributor

@evanchaoli evanchaoli commented Dec 11, 2020

What does this PR accomplish?

closes #6347 .

Changes proposed by this PR:

  1. Add some new Vault configuration parameter for timeout values.
  2. For VarSourcePool, add a helper class inPoolSecret to wait for Vault to login, so that varSourcePool.FindOrCreate won't be blocked.

Notes to reviewer:

The first commit is for your initial review from the design perspective. Code is not cleaned up yet, and tests not added yet.

EDIT: Now it's ready for review. Code cleaned up, tests added.

Release Note

  • These timeouts can be configured using CONCOURSE_VAULT_LOGIN_TIMEOUT and CONCOURSE_VAULT_QUERY_TIMEOUT respectively
  • The new default login timeout is 60s

Contributor Checklist

Reviewer Checklist

  • Code reviewed
  • Tests reviewed
  • Documentation reviewed
  • Release notes reviewed
  • PR acceptance performed
  • New config flags added? Ensure that they are added to the
    BOSH and
    Helm packaging; otherwise, ignored for
    the integration
    tests

    (for example, if they are Garden configs that are not displayed in the
    --help text).

… managers).

Signed-off-by: Chao Li <chaol@vmware.com>
Signed-off-by: Chao Li <chaol@vmware.com>
Signed-off-by: Chao Li <chaol@vmware.com>
Copy link
Contributor

@aoldershaw aoldershaw left a comment

Choose a reason for hiding this comment

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

Hey @evanchaoli, really sorry about the slow review! I was on vacation for much of December, so haven't had a chance to look at this until now

atc/creds/dummy/manager_factory.go Outdated Show resolved Hide resolved
atc/creds/vault/manager.go Outdated Show resolved Hide resolved
Signed-off-by: Chao Li <chaol@vmware.com>
Signed-off-by: Chao Li <chaol@vmware.com>
@evanchaoli evanchaoli changed the title Enhance var_sources to handle slow Vault (as well as other credential managers). Enhance var_sources to handle slow Vault Jan 8, 2021
atc/creds/vault/vault.go Outdated Show resolved Hide resolved
atc/creds/vault/manager.go Show resolved Hide resolved
Signed-off-by: Chao Li <chaol@vmware.com>
@evanchaoli
Copy link
Contributor Author

evanchaoli commented Jan 12, 2021

@aoldershaw I backport this PR to 6.7.x with #6413 because we keep seeing "missing client token" errors on our prod cluster, so I want to solve the problem in 6.7.3. Please also help review #6413.

@aoldershaw aoldershaw changed the title Enhance var_sources to handle slow Vault Allow configuring the login and query timeouts for Vault Jan 12, 2021
@aoldershaw aoldershaw merged commit 3557dba into concourse:master Jan 12, 2021
aoldershaw added a commit to concourse/concourse-bosh-release that referenced this pull request Jan 14, 2021
from concourse/concourse#6362

Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>
aoldershaw added a commit that referenced this pull request Jan 14, 2021
Backport #6362 to 6.7.x: handle slow Vault
aoldershaw added a commit to concourse/concourse-bosh-release that referenced this pull request Jan 21, 2021
from concourse/concourse#6362

Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>
@aoldershaw aoldershaw changed the title Allow configuring the login and query timeouts for Vault Allow configuring login and query timeouts for Vault Jan 25, 2021
@aoldershaw aoldershaw added the release/undocumented This didn't warrant being documented or put in release notes. label Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement release/undocumented This didn't warrant being documented or put in release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vault query hits "missing client token"
2 participants