Skip to content
This repository has been archived by the owner on Jul 2, 2021. It is now read-only.

Slim down client from the problematic hashicorp/vault dependency #23

Merged
merged 14 commits into from
Aug 20, 2020

Conversation

wojciecholszewski-form3
Copy link
Contributor

@wojciecholszewski-form3 wojciecholszewski-form3 commented Aug 20, 2020

Objective

The purpose of this PR is to slim down the client from the problematic hashicorp/vault dependency to mitigate the following issues:

1. hashicorp/vault package management

The hashicorp/vault repository is organized in a way that hashicorp/vault/api & hashicorp/vault/sdk are configured as individual modules. According to the hashicorp/vault maintainers, only api and sdk should be imported. Other packages are considered internal and shouldn't be imported.

See thread

No one should be importing github.com/hashicorp/vault, only the api and sdk modules. The top-level vault module is not intended to be a dependency for other projects, we're using go.mod solely to manage our own dependencies here.

2. hashicorp/vault package versioning

Politely speaking packages versioning there is messed up. The most recent, at the moment of writing, release tagged v1.5.0 requires github.com/hashicorp/vault/api v1.0.5-0.20200630205458-1a16f3c699c6, which is impossible to resolve because 1a16f3c699c6 revision is not on the v.1.5.0 tag 🤷‍♂️ 😅

TL;DR Whenever we import hashicorp/vault we have to deal with the messed up versioning and fix it ourselves by incorporating the replace directive (see issue). Examples (only a few, I believe there are more):

Conclusions

  • In our repo hashicorp/vault is solely imported by the tests. The regular client doesn't need it.
  • hashicorp/vault pollutes the main go.mod with the dependency which is imported by the tests only.
  • Our day-to-day work, whenever we deal with vault dependency (bump the library version for instance) gonna be much easier if we could get rid of the dependency on hashicorp/vault and import hashicorp/vault/{api,sdk} only.

Solution

Move the tests to the own module and isolate the hashicorp/vault to the scope of the tests only. Ultimately the list of client dependencies is much thinner and contains only a few entries.

Before: (the most recent commit on master at the moment of writing 50d2e128f7fe83ea61ba912c115839a5398e343e)

require (
	github.com/aws/aws-sdk-go v1.25.41
	github.com/hashicorp/consul/sdk v0.2.0
	github.com/hashicorp/go-hclog v0.12.0
	github.com/hashicorp/go-memdb v1.0.4 // indirect
	github.com/hashicorp/go-retryablehttp v0.6.3 // indirect
	github.com/hashicorp/go-uuid v1.0.2
	github.com/hashicorp/vault v1.4.1
	github.com/hashicorp/vault/api v1.0.5-0.20200430075121-b2b4ab9577e4
	github.com/hashicorp/vault/sdk v0.1.14-0.20200430075121-b2b4ab9577e4
	github.com/jefferai/jsonx v1.0.1 // indirect
	github.com/keybase/go-crypto v0.0.0-20190828182435-a05457805304 // indirect
	github.com/pierrec/lz4 v2.3.0+incompatible // indirect
	github.com/pkg/errors v0.8.1
	github.com/stretchr/testify v1.4.0
	github.com/urfave/cli/v2 v2.2.0
	golang.org/x/time v0.0.0-20191024005414-555d28b269f0 // indirect
	google.golang.org/grpc v1.28.0 // indirect
)

After: (commit ad25fc86f644ed1e87684daf9b191cf631bb2557)

	github.com/aws/aws-sdk-go v1.34.7
	github.com/hashicorp/vault/api v1.0.5-0.20200817232951-d7307fcdfed7
	github.com/hashicorp/vault/sdk v0.1.14-0.20200817232951-d7307fcdfed7
	github.com/pkg/errors v0.9.1
	github.com/stretchr/testify v1.5.1
	github.com/urfave/cli/v2 v2.2.0
)

rkorkosz
rkorkosz previously approved these changes Aug 20, 2020
Copy link
Contributor

@rkorkosz rkorkosz left a comment

Choose a reason for hiding this comment

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

LGTM!

@hallabro
Copy link

Great changes!

owenrumney
owenrumney previously approved these changes Aug 20, 2020
@wojciecholszewski-form3 wojciecholszewski-form3 merged commit 35608d3 into master Aug 20, 2020
@wojciecholszewski-form3 wojciecholszewski-form3 deleted the wojciech-bump-hashicorp-vault-version branch August 20, 2020 13:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants