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

Jump to bottom

Implement support for Vault KV v2 backends #6115

Merged
merged 7 commits into from Jan 20, 2021
Merged

Implement support for Vault KV v2 backends #6115

merged 7 commits into from Jan 20, 2021

Conversation

daviddob
Copy link
Contributor

@daviddob daviddob commented Oct 5, 2020 •
edited

Changes proposed by this PR:

Enables support for Vault's KV version 2 backends to enable versioning
and support default deployments of new vault instances. This implements a backwards
compatible change to detect kv2 backends and resolve the paths accordingly.

While RFC #39 was proposed to take this on it appears to still propose the use of a
vault type. As such hopefully this is not considered a quick-fix antipattern and
can be used to improve the credential-manager var sources as well.

Tested with all gingko + testflight cases.

Fixes #2374

Notes to reviewer:

Leverages the existing vault test suite as the changes are made to the underlying api client.

Release Note

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).

@daviddob daviddob mentioned this pull request Oct 5, 2020
Merged
@vito vito self-assigned this Oct 6, 2020
vito
vito requested changes Oct 6, 2020
View reviewed changes
Copy link
Member

@vito vito left a comment

Choose a reason for hiding this comment

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

Hey, thanks for picking this up!

It's unfortunate that we have to carry over so much internal code from Vault, but I get it, not sure what else to do there besides requesting upstream API changes.

There's a part of the code that mutates the client that I'm not sure will be safe though.

atc/creds/vault/vault_kvhelpers.go Outdated Show resolved Hide resolved
@daviddob daviddob requested a review from vito October 7, 2020 14:02
vito
vito reviewed Nov 2, 2020
View reviewed changes
Copy link
Member

@vito vito left a comment

Choose a reason for hiding this comment

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

Thanks for waiting - getting back to this now that v6.7 is out!

How would you feel about adding tests for this? Originally the API client was meant to be a thin layer that can be faked out for tests, but now it's getting a little thick. 🤔 There aren't currently tests for the API client, but they could probably be added by faking out the API endpoint using ghttp.NewServer.

@daviddob
Copy link
Contributor Author

I think adding tests are a solid idea as more complexity gets added - though with it being holiday season my time is a bit limited. I'll see what I can do in the meantime but it may have to sit for a while on my end.

@vito
Copy link
Member

vito commented Nov 19, 2020

@daviddob No worries - thanks for checking in!

Just as an FYI, we're working on a pretty big new feature that will allow all this code to be extracted into prototypes. It doesn't need to impact this PR either way, but I thought you might be interested in keeping it on your radar: #5229

@daviddob
atc: behavior: Implement support for Vault KV v2 backends
7a29e4c 
As addressed in this issue (i.e. concourse#2374)
we would like support for Vault's KV version 2 backends to enable versioning
and support default deployments of new vault instances. This implements a backwards
compatible change to detect kv2 backends and resolve the paths accordingly.
Tested with all gingko + testflight cases.

concourse#2374

Signed-off-by: David Dobmeier <david.dobmeier@dcsg.com>
@daviddob
Remove wrapping and cli specific code that modifies the client
631de68 
Signed-off-by: David Dobmeier <david.dobmeier@dcsg.com>
@daviddob
Implement vault api mocks for api_client testing
565fd71 
This adds support for testing the api_client for vault kv1 and kv2
support. It leverages the ghttp library handlers for this and the
behavior of the AppendHandlers is a bit awkward due to the fact that
each handler is only used once in a specific order. If there is a
better way of registering a route or handler to clean this up id
be happy to refactor a bit.

Signed-off-by: David Dobmeier <david.dobmeier@dcsg.com>
@daviddob
Add more protection around values to allow more complex tests in the …
94dbbaf 
…future

Signed-off-by: David Dobmeier <david.dobmeier@dcsg.com>
@daviddob
Remove VariableDefinition to match changes since PR was submitted
6afe892 
Signed-off-by: David Dobmeier <david.dobmeier@dcsg.com>
@daviddob daviddob requested a review from vito January 7, 2021 17:27
@daviddob
Copy link
Contributor Author

daviddob commented Jan 7, 2021

@vito Been a little while, finally got some time and finished writing tests using ghttp for both KV2 and KV1 vault apis to test the api_client. The way the ghttp handlers work make it a bit awkward and verbose as the requests much match the order of the handlers but everything seems to be functioning as expected after bringing my changes up to date with master.

@vito
Copy link
Member

vito commented Jan 19, 2021

@daviddob Awesome, thanks for circling back to this! Just resolved a tiny conflict, commencing review now.

vito
vito requested changes Jan 19, 2021
View reviewed changes
Copy link
Member

@vito vito 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 and verified that the tests fail if the v2 support is removed. Left some nitpicky suggestions. I would have just merged and touched them up myself but it looks like some small changes need to be made since NewAPIClient changed on master recently. (I suggested changes for them.)

Once those NewAPIClient calls are fixed up I'm happy to merge this as-is.

atc/creds/vault/vault_test.go Show resolved Hide resolved
atc/creds/vault/vault_test.go Outdated Show resolved Hide resolved
atc/creds/vault/vault_test.go Outdated Show resolved Hide resolved
atc/creds/vault/vault_test.go Outdated Show resolved Hide resolved
atc/creds/vault/vault_test.go Outdated Show resolved Hide resolved
@daviddob
Update tests to be compatible with master and cleanup cases
5096c73 
Used RouteToHandler to clean up all the mount api calls.

Signed-off-by: David Dobmeier <david.dobmeier@dcsg.com>
@daviddob
Copy link
Contributor Author

Updated as per your feedback - should be good to go. Thanks @vito!

vito
vito approved these changes Jan 20, 2021
View reviewed changes
Copy link
Member

@vito vito left a comment

Choose a reason for hiding this comment

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

lgtm - thanks!

@vito vito merged commit ed04933 into concourse:master Jan 20, 2021
@daviddob daviddob deleted the vault_kv2_support branch January 20, 2021 15:46
aoldershaw pushed a commit that referenced this pull request Jan 28, 2021
topgun: skip Vault tests
5139db9 
After #6115 was merged, all the Bosh Topgun tests exercising Vault
started failing. This is not due to the Vault integration being broken
(well, not really) - Bosh topgun is just using such an old (custom)
version of Vault that is no longer compatible I gather.

Since we're going to end up replacing all these tests shortly with a new
test suite (introduced in #6479), let's just skip these Vault tests for
now

Signed-off-by: Aidan Oldershaw <aoldershaw@pivotal.io>
@aoldershaw aoldershaw mentioned this pull request Jan 28, 2021
Merged
11 tasks
@aoldershaw aoldershaw added the release/documented Documentation and release notes have been updated. label Feb 16, 2021
@aoldershaw aoldershaw changed the title atc: behavior: Implement support for Vault KV v2 backends Implement support for Vault KV v2 backends Feb 16, 2021
@taylorsilva taylorsilva mentioned this pull request Feb 17, 2021
Closed
@Jasstkn Jasstkn mentioned this pull request Jun 16, 2021
Closed
This was referenced Mar 22, 2023
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vito vito vito approved these changes

Assignees

@vito vito

Labels
enhancement release/documented Documentation and release notes have been updated.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable to versioning credential with vault.
3 participants
@daviddob @vito @aoldershaw