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

Support custom Vault secret prefix #6283

Merged
merged 28 commits into from
Apr 14, 2022

Conversation

sam-heilbron
Copy link
Contributor

@sam-heilbron sam-heilbron commented Apr 8, 2022

Description

Allow injecting a custom pathPrefix for Vault secrets, historically this was hard-coded to "secret".

Context

Why is this configuration valuable?

It allows for users to use custom secrets engines and for multitenancy-ness for organizations with different/separated secrets paths.

How did this work before?

This value was previously hardcoded in solo-kit https://github.com/solo-io/solo-kit/blob/1d799ae290c2f516f01fc4ad20272d7d2d5db1e7/pkg/api/v1/clients/vault/resource_client.go#L309

How did we verify this?

  • Created a test to verify that our vault client can interact with the custom --vault-path-prefix fed in by user. The test can create a secret using the custom path prefix, and can also read from Vault.
  • We updated our test setup to allow for the creation of a custom -path for the Vault test server to test the pathPrefix. This matches how Vault customers enable new secrets engines with custom path.

Checklist:

  • I included a concise, user-facing changelog (for details, see https://github.com/solo-io/go-utils/tree/master/changelogutils) which references the issue that is resolved.
  • If I updated APIs (our protos) or helm values, I ran make -B install-go-tools generated-code to ensure there will be no code diff
  • I followed guidelines laid out in the Gloo Edge contribution guide
  • I opened a draft PR or added the work in progress label if my PR is not ready for review
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

BOT NOTES:
resolves #6184

@github-actions github-actions bot added the keep pr updated signals bulldozer to keep pr up to date with base branch label Apr 8, 2022
@github-actions
Copy link

github-actions bot commented Apr 8, 2022

Visit the preview URL for this PR (updated for commit c5ec0ea):

https://gloo-edge--pr6283-expose-vault-secret-q9p2e8to.web.app

(expires Thu, 21 Apr 2022 15:16:49 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@inFocus7 inFocus7 added the work in progress signals bulldozer to keep pr open (don't auto-merge) label Apr 12, 2022
@inFocus7 inFocus7 changed the title [DNM] Support custom Vault secret prefix Support custom Vault secret prefix Apr 12, 2022
@solo-changelog-bot
Copy link

Issues linked to changelog:
#6184

@inFocus7
Copy link
Contributor

re-running build, flake #2838

@sam-heilbron
Copy link
Contributor Author

The most recent build-bot failure: [Fail] Happy path Rest Eds Enabled: (V3) in memory ssl sni [It] should work with ssl is one we have not seen before. My instinct is to say this is due to test pollution.

In the logs I noticed a message that seems to occur whenever we see this type of new flake:

Step #10 - "test": WARN	gloo.setup.gloosnapshot.event_loop.envoyTranslatorSyncer	Proxy had invalid config after xds sanitization	{"proxy": "name:\"gateway-proxy\" namespace:\"gloo-system\"", "error": "2 errors occurred:\n\t* invalid resource gloo-system.gateway-proxy\n\t* WARN: \n  [Route Warning: InvalidDestinationWarning. Reason: *v1.Upstream { default.local-test-upstream-70-ssl } not found]\n\n"}
Step #10 - "test": INFO	gloo	respond open watch 1[] with new version "14695981039346656037"

I believe that #6301 will resolve this issue. I vote that for now we kick this flake and track it. And if I can confirm that this linked PR will resolve the issue then I can close any new flake issues

@sam-heilbron
Copy link
Contributor Author

/kick

@sam-heilbron
Copy link
Contributor Author

/kick New failed test, same core issue as mentioned above https://storage.googleapis.com/solo-public-build-logs/logs.html?buildid=dc8ff3f1-81c8-4704-9a2a-119c3250e4b6

@inFocus7
Copy link
Contributor

had flake #6253

@inFocus7
Copy link
Contributor

/kick

@inFocus7
Copy link
Contributor

/kick no fail reason shown, CI build failed during "docker-push-extended": Pushed

@inFocus7
Copy link
Contributor

/kick

  Expected
      <core.Status_State>: 1
  to equal
      <core.Status_State>: 2

kicking due to possible flake in test

@nfuden
Copy link
Contributor

nfuden commented Apr 14, 2022

/kick Locally the kv test that just failed doesnt. So its a flake... though we should consider how we can fix it if this turns out to be truly a flake Edit: too many times to be a real flake minimally a weird test setup

@inFocus7
Copy link
Contributor

test flake in CI / k8s regression tests (glooctl, false) (pull_request). Istio/istioctl failed to install, created new issue for it #6316

@inFocus7 inFocus7 marked this pull request as ready for review April 14, 2022 13:24
@inFocus7 inFocus7 requested a review from a team as a code owner April 14, 2022 13:24
Copy link
Contributor

@Rachael-Graham Rachael-Graham left a comment

Choose a reason for hiding this comment

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

doc

projects/gloo/api/v1/settings.proto Outdated Show resolved Hide resolved
projects/gloo/cli/pkg/flagutils/common.go Outdated Show resolved Hide resolved
Copy link
Contributor

@nfuden nfuden 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. Most of the decisions were in the solokit PR.

@inFocus7 inFocus7 removed the work in progress signals bulldozer to keep pr open (don't auto-merge) label Apr 14, 2022
@soloio-bulldozer soloio-bulldozer bot merged commit ff89b3c into master Apr 14, 2022
@soloio-bulldozer soloio-bulldozer bot deleted the expose-vault-secret-engine-name branch April 14, 2022 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keep pr updated signals bulldozer to keep pr up to date with base branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow configuration of the "root" path for Hashi Vault Integration
4 participants