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 vault namespaces in connect CA #12735

Closed
wants to merge 7 commits into from

Conversation

markan
Copy link
Contributor

@markan markan commented Apr 8, 2022

Follow on to some missed items from #12655

From an internal ticket "Support standard "Vault namespace in the
path" semantics for Connect Vault CA Provider"

Vault allows the namespace to be specified as a prefix in the path of
a PKI definition, but our usage of the Vault API includes calls that
don't support a namespaced key. In particular the sys.* family of
calls simply appends the key, instead of prefixing the namespace in
front of the path.

Signed-off-by: Mark Anderson [email protected]

@github-actions github-actions bot added the theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies label Apr 8, 2022
@markan markan added the pr/no-changelog PR does not need a corresponding .changelog entry label Apr 8, 2022
@markan markan force-pushed the ma/vault-namespace-intermediate-provider branch from 98526ff to 77edda8 Compare April 11, 2022 18:40
@vercel vercel bot temporarily deployed to Preview – consul April 11, 2022 18:40 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 11, 2022 18:40 Inactive
@markan markan force-pushed the ma/vault-namespace-intermediate-provider branch from 77edda8 to 017477e Compare April 13, 2022 05:02
@vercel vercel bot temporarily deployed to Preview – consul April 13, 2022 05:03 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 13, 2022 05:03 Inactive
@vercel vercel bot temporarily deployed to Preview – consul April 13, 2022 16:38 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 13, 2022 16:38 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 13, 2022 16:41 Inactive
@vercel vercel bot temporarily deployed to Preview – consul April 13, 2022 16:41 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 13, 2022 16:56 Inactive
@vercel vercel bot temporarily deployed to Preview – consul April 13, 2022 16:56 Inactive
@markan markan force-pushed the ma/vault-namespace-intermediate-provider branch from 253c54e to 5a095e5 Compare April 13, 2022 16:59
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 13, 2022 16:59 Inactive
@vercel vercel bot temporarily deployed to Preview – consul April 13, 2022 16:59 Inactive
Copy link
Contributor

@kyhavlov kyhavlov 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 👍 , left a couple non-blocking minor whitespace nits

@@ -852,3 +852,34 @@ func vaultProviderConfig(t *testing.T, addr, token string, rawConf map[string]in

return cfg
}

func TestVaultProvider_potentialMountPaths(t *testing.T) {

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a leftover extra whitespace line

@@ -284,7 +286,8 @@ func (v *VaultProvider) GenerateRoot() (RootResult, error) {
rootPEM, err := v.getCA(v.config.RootPKIPath)
switch err {
case ErrBackendNotMounted:
err := v.client.Sys().Mount(v.config.RootPKIPath, &vaultapi.MountInput{

Copy link
Contributor

Choose a reason for hiding this comment

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

Leftover extra whitespace line

@vercel vercel bot temporarily deployed to Preview – consul April 13, 2022 19:05 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 13, 2022 19:05 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 13, 2022 19:29 Inactive
@vercel vercel bot temporarily deployed to Preview – consul April 13, 2022 19:29 Inactive
markan and others added 7 commits April 13, 2022 13:38
Follow on to some missed items from #12655

From an internal ticket "Support standard "Vault namespace in the
path" semantics for Connect Vault CA Provider"

Vault allows the namespace to be specified as a prefix in the path of
a PKI definition, but our usage of the Vault API includes calls that
don't support a namespaced key. In particular the sys.* family of
calls simply appends the key, instead of prefixing the namespace in
front of the path.

Signed-off-by: Mark Anderson <[email protected]>
Signed-off-by: Mark Anderson <[email protected]>
Signed-off-by: Mark Anderson <[email protected]>
@markan markan force-pushed the ma/vault-namespace-intermediate-provider branch from b93c96e to 0ba6baf Compare April 13, 2022 20:38
@vercel vercel bot temporarily deployed to Preview – consul April 13, 2022 20:38 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging April 13, 2022 20:38 Inactive
@jkirschner-hashicorp jkirschner-hashicorp added the pr/do-not-merge PR cannot be merged in its current form. label Apr 15, 2022
@jkirschner-hashicorp
Copy link
Contributor

Temporarily marking do-not-merge while we continue work on this

@markan
Copy link
Contributor Author

markan commented Apr 30, 2022

Closing this PR in favor of a new approach in PR #12904

@markan markan closed this Apr 30, 2022
@markan markan deleted the ma/vault-namespace-intermediate-provider branch April 30, 2022 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/do-not-merge PR cannot be merged in its current form. pr/no-changelog PR does not need a corresponding .changelog entry theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants