Update vault ca provider namespace configuration #19095
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cthain
added
type/bug
github-actions
bot
added
the
theme/connect
cthain
commented
Oct 5, 2023
Comment on lines
+1472
to
+1478
| if len(c.namespaces) > 0 { | ||
| // If explicit namespaces are provided, try to create the provider before any of the namespaces | ||
| // have been created. Verify that the provider fails to initialize. | ||
| provider, err := createVaultProviderE(t, true, testVault.Addr, token, providerConfig) | ||
| require.Error(t, err) | ||
| require.NotNil(t, provider) | ||
| } |
There was a problem hiding this comment.
This check here isn't strictly necessary but it does duplicate the problem reported in the issue where the provider fails to initialize because the namespace doesn't exist.
Comment on lines
+64
to
+66
| type vaultRequirements struct { | ||
| Enterprise bool | ||
| } |
There was a problem hiding this comment.
Maybe this mechanism is overkill but it would allow us to have other tests be discriminant about what Vault features are available to test with. For example, we could skip tests for certain Vault versions if the feature isn't applicable. Maybe this falls in the category of YNGNI, but it was super easy to implement it this way, so I did.
Comment on lines
+256
to
+257
| if rune(leafPEM[len(leafPEM)-1]) != '\n' { | ||
| t.Fatalf("cert does not end with a new line") |
There was a problem hiding this comment.
Just cleaning up code flagged by go-static-check. "Yoda" statements, like it does not.
cthain
force-pushed
the
cthain/net-5807/vault-ca-namespace-config
branch
from
434524e
to
d4cff3b
Compare
kisunji
approved these changes
Oct 6, 2023
cthain
force-pushed
the
cthain/net-5807/vault-ca-namespace-config
branch
from
d4cff3b
to
c55f1a4
Compare
cthain
force-pushed
the
cthain/net-5807/vault-ca-namespace-config
branch
from
c55f1a4
to
c721f5c
Compare
cthain
force-pushed
the
cthain/net-5807/vault-ca-namespace-config
branch
from
c721f5c
to
8868f67
Compare
This was referenced Oct 10, 2023
cthain
added a commit
that referenced
this pull request
Oct 11, 2023
cthain
added a commit
that referenced
this pull request
Oct 11, 2023
cthain
added a commit
that referenced
this pull request
Oct 11, 2023
cthain
added a commit
that referenced
this pull request
Oct 11, 2023
cthain
added a commit
that referenced
this pull request
Oct 11, 2023
cthain
added a commit
that referenced
this pull request
Oct 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes the regression described in #19051. The Vault CA provider has been updated to only set the namespace on the Vault client if it is not empty, otherwise it falls back to the base namespace configured for the provider.
Testing & Reproduction steps
A new unit test
TestVaultCAProvider_EnterpriseNamespacehas been created to exercise the update. This requires a local Vault Enterprise binary and license to run, otherwise the test will automatically be skipped.PR Checklist
external facing docs updated