fix: refactor out a sub-interface from Vault for VaultURL injection #4330

jstrachan · 2019-06-19T12:49:27Z

so we can support local file system vault-like behaviour or real Vault from a small simple interface (which is a small subset of Vault client)
same URL structure works for vault + local file system referencing

Signed-off-by: James Strachan james.strachan@gmail.com

pkg/vaulturl/helpers.go

pmuir · 2019-06-19T12:52:37Z

Looks good to me for the URL parts. @ccojocar you should look too though for a final lgtm :-)

ccojocar · 2019-06-19T13:00:50Z

pkg/vaulturl/vaulturl_client.go

@@ -0,0 +1,17 @@
+package vaulturl


I am wondering if we should name this secreturl instead. it is something more generic than vault. In my mind, vault is just one implementation of this interface.

then I guess the URLs need to be secret: too?

ok refactoring to secreturl package but gonna stick with vault: and local: URLs

pkg/vaulturl/localvault/client.go

ccojocar · 2019-06-19T13:12:03Z

pkg/vaulturl/helpers.go

+var vaultURIRegex = regexp.MustCompile(`vault:[-_\w\/:]*`)
+
+// ReplaceURIs will replace any vault: URIs in a string, using the vault client
+func ReplaceURIs(s string, client Client) (string, error) {


I would rather make this a method part of secreturl.Client, this will allow us to extend it with other URI schemes (e.g. k8s secret).

the main reason I kept it here for now was to avoid touching vault.Client in case it interfered with your refactor ;). Maybe we add a ReplaceURIs function to the secreturl.Client which for vault + localvault can reuse the same code + URL scheme - then do something different for k8s?

how about we support secret: for either vault or local and vault: for only vault and local: for only local? then if folks want to be generic they use secret: URLs?

we could add other URLs for vault - e.g. to use a specific named vault too?

'secret:` sounds better to me in this case

ccojocar · 2019-06-19T13:13:44Z

pkg/vaulturl/helpers.go

+	"github.com/pkg/errors"
+)
+
+var vaultURIRegex = regexp.MustCompile(`vault:[-_\w\/:]*`)


I would be explicit a use a URI scheme per client (e.g. vault:..., local:..., k8s:...) instead of hiding this. In this way, the user will be aware of the place where the secrets are stored.

BTW one of the motivations of this was so that we don't have to search / replace all the values.yaml in a GitOps install if someone decides to switch from local file system secrets to vault - i.e. to use the same URI scheme in values.yaml

I'm hoping we can have a canonical git repo for installing Jenkins X with prow + tekton with + without vault without having to do a global replace of vault: <-> local:

I would use secret instead. It will be difficult to use the same path for both vault and local, because vault is using a relative path to the root of the secrets backend which normally is mounted with a fix name secret for KV secret engine. It will be something like secret/<path-in-vault>:secret-name but the secret prefix is omitted from the URI, which is not totally correct if we want to support multiple vault secrets backends. Another backend will be mounted on a different prefix so the user won't be able to use that backend.

I think the user will be forced to store the secrets file to the same relative path which is similar with the relative path in vault.

Given this example:
https://github.com/jenkins-x/environment-tekton-mole-dev/blob/783a8f6178c050c0b96c61016d1581bd0bc7a3a0/env/values.yaml#L98

the user will have to store the secrets on a local path like this:
<root-folder>/gitOps/jenkins-x/environment-tekton-mole-dev/jenkins-x-demo-sa:data

codecov · 2019-06-19T13:24:16Z

Codecov Report

Merging #4330 into master will decrease coverage by 0.04%.
The diff coverage is 41.79%.

@@            Coverage Diff             @@
##           master    #4330      +/-   ##
==========================================
- Coverage    43.4%   43.36%   -0.05%     
==========================================
  Files         804      807       +3     
  Lines      100748   101016     +268     
==========================================
+ Hits        43726    43801      +75     
- Misses      53392    53559     +167     
- Partials     3630     3656      +26

Flag	Coverage Δ
#e2e	`31.17% <14.53%> (-0.01%)`	⬇️
#integration	`41.7% <40%> (-0.05%)`	⬇️

Impacted Files	Coverage Δ
pkg/vault/helpers.go	`10.71% <ø> (-12.5%)`	⬇️
pkg/vault/vault_client.go	`38.02% <0%> (-1.11%)`	⬇️
pkg/apps/values.go	`63.96% <0%> (ø)`	⬆️
pkg/secreturl/mocks/secreturl_client.go	`31.61% <31.61%> (ø)`
pkg/secreturl/localvault/client.go	`33.33% <33.33%> (ø)`
pkg/vault/mocks/vault_client.go	`16.17% <33.33%> (-5.46%)`	⬇️
pkg/secreturl/helpers.go	`36.66% <36.66%> (ø)`
pkg/cmd/step/helm/step_helm_apply.go	`53.71% <50%> (-0.28%)`	⬇️
pkg/cmd/opts/helm.go	`21.94% <57.14%> (+0.69%)`	⬆️
pkg/cmd/step/create/step_create_install_values.go	`44.24% <60%> (-0.49%)`	⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 442ddfc...3b947c2. Read the comment docs.

jstrachan · 2019-06-19T13:38:58Z

/test integration

pkg/secreturl/localvault/client.go

jstrachan · 2019-06-19T17:16:01Z

/test all

jstrachan · 2019-06-19T19:07:01Z

/test all

pmuir · 2019-06-19T19:20:14Z

/lgtm

jstrachan · 2019-06-19T19:29:38Z

/test all

pkg/cmd/opts/helm.go

pkg/helm/values_tree_template_test.go

pkg/helm/values_tree.go

pkg/cmd/step/helm/step_helm_apply.go

pkg/cmd/opts/helm.go

pkg/helm/values_tree_template_test.go

pkg/helm/values_tree.go

pkg/cmd/step/helm/step_helm_apply.go

* so we can support local file system vault-like behaviour or real Vault from a small simple interface (which is a small subset of Vault client) * same URL structure works for vault + local file system referencing Signed-off-by: James Strachan <james.strachan@gmail.com>

Signed-off-by: James Strachan <james.strachan@gmail.com>

and add support for `vault:` for the vault client and `local:` for the local file system client Signed-off-by: James Strachan <james.strachan@gmail.com> jenkins-x#4328

Signed-off-by: James Strachan <james.strachan@gmail.com>

* also support referencing logical Parameters in a `parameters.yaml` file which can include a logical structure + schema (for nice install tooling) which then contains inline values for simple values or URLs to vault/local secret files for better secret management fixes jenkins-x#4328 Signed-off-by: James Strachan <james.strachan@gmail.com>

Signed-off-by: James Strachan <james.strachan@gmail.com>

jstrachan · 2019-06-20T04:58:36Z

/test integration

jstrachan · 2019-06-20T05:06:33Z

/test lint

Signed-off-by: James Strachan <james.strachan@gmail.com>

and we don't yet have the install config ConfigMap setup Signed-off-by: James Strachan <james.strachan@gmail.com>

jstrachan · 2019-06-20T06:45:48Z

/test bdd

Signed-off-by: James Strachan <james.strachan@gmail.com>

added a test + fix for templating in the root dir as well as any nested `values.yaml` files Signed-off-by: James Strachan <james.strachan@gmail.com>

pkg/cmd/opts/common.go

pkg/helm/helm_helpers.go

pkg/secreturl/client.go

@ccojocar

thanks for the great feedback @ccojocar * renamed `vaultClient` -> `secretURLClient` * fixed up mock generation * zapped the `GetClusterName` and reused the existing helper Signed-off-by: James Strachan <james.strachan@gmail.com>

jenkins-x-bot · 2019-06-20T09:01:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ccojocar, pmuir

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [ccojocar,pmuir]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ccojocar

…enkins-x#4330) * fix: refactor out a sub-interface from Vault for VaultURL injection * so we can support local file system vault-like behaviour or real Vault from a small simple interface (which is a small subset of Vault client) * same URL structure works for vault + local file system referencing Signed-off-by: James Strachan <james.strachan@gmail.com> * chore: fix hound warning Signed-off-by: James Strachan <james.strachan@gmail.com> * chore: refactor vaulturl -> secreturl Signed-off-by: James Strachan <james.strachan@gmail.com> * fix: lets move URL handling into the secreturl.Client and add support for `vault:` for the vault client and `local:` for the local file system client Signed-off-by: James Strachan <james.strachan@gmail.com> jenkins-x#4328 * chore: fix hound warning Signed-off-by: James Strachan <james.strachan@gmail.com> * fix: allow `values.yaml` to include go template functions * also support referencing logical Parameters in a `parameters.yaml` file which can include a logical structure + schema (for nice install tooling) which then contains inline values for simple values or URLs to vault/local secret files for better secret management fixes jenkins-x#4328 Signed-off-by: James Strachan <james.strachan@gmail.com> * chore: fix hound warning Signed-off-by: James Strachan <james.strachan@gmail.com> * chore: fix failing tests due to refactor Signed-off-by: James Strachan <james.strachan@gmail.com> * chore: fix failing tests due to refactor Signed-off-by: James Strachan <james.strachan@gmail.com> * chore: avoid failing when bootstrapping a cluster and we don't yet have the install config ConfigMap setup Signed-off-by: James Strachan <james.strachan@gmail.com> * fix: lets populate the cluster information in the cluster/values.yaml Signed-off-by: James Strachan <james.strachan@gmail.com> * chore: fix broken test Signed-off-by: James Strachan <james.strachan@gmail.com> * fix: lets allow templating in the root `values.yaml` too added a test + fix for templating in the root dir as well as any nested `values.yaml` files Signed-off-by: James Strachan <james.strachan@gmail.com> * chore: polished the code thanks for the great feedback @ccojocar * renamed `vaultClient` -> `secretURLClient` * fixed up mock generation * zapped the `GetClusterName` and reused the existing helper Signed-off-by: James Strachan <james.strachan@gmail.com>