Skip to content

Update vault ca provider namespace configuration#19095

Merged
cthain merged 2 commits intomainfrom
cthain/net-5807/vault-ca-namespace-config
Oct 10, 2023
Merged

Update vault ca provider namespace configuration#19095
cthain merged 2 commits intomainfrom
cthain/net-5807/vault-ca-namespace-config

Conversation

@cthain
Copy link
Contributor

@cthain cthain commented Oct 5, 2023

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_EnterpriseNamespace has been created to exercise the update. This requires a local Vault Enterprise binary and license to run, otherwise the test will automatically be skipped.

$ vault version
Vault v1.15.0+ent (d3729711f875a9dedea802079cd7f0e4b1d6e8d5), built 2023-09-22T21:04:53Z
$ export VAULT_LICENSE=$(cat /path/to/vault.license)
$ go test ./agent/connect/ca -run '^TestVaultCAProvider_EnterpriseNamespace$'
ok  	github.com/hashicorp/consul/agent/connect/ca	1.916s

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@cthain cthain added type/bug Feature does not function as expected pr/no-docs PR does not include docs and should not trigger reminder for cherrypicking them. backport/1.14 backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. backport/1.16 This release series is no longer active on CE. Use backport/ent/1.16. labels Oct 5, 2023
@cthain cthain requested review from a team, kisunji and roncodingenthusiast October 5, 2023 22:51
@cthain cthain self-assigned this Oct 5, 2023
@github-actions github-actions bot added the theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies label Oct 5, 2023
Comment on lines 1472 to 1478
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

Good

Comment on lines 64 to 66
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just cleaning up code flagged by go-static-check. "Yoda" statements, like it does not.

@cthain cthain force-pushed the cthain/net-5807/vault-ca-namespace-config branch from 434524e to d4cff3b Compare October 5, 2023 23:05
Copy link
Contributor

@kisunji kisunji left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the thorough integration testing

@cthain cthain force-pushed the cthain/net-5807/vault-ca-namespace-config branch from d4cff3b to c55f1a4 Compare October 6, 2023 17:46
@cthain cthain enabled auto-merge (squash) October 6, 2023 17:48
@cthain cthain force-pushed the cthain/net-5807/vault-ca-namespace-config branch from c55f1a4 to c721f5c Compare October 6, 2023 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. backport/1.16 This release series is no longer active on CE. Use backport/ent/1.16. pr/no-docs PR does not include docs and should not trigger reminder for cherrypicking them. theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies type/bug Feature does not function as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants