connect: ensure all vault connect CA tests use limited privilege tokens #15669
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
rboyer
added
theme/consul-vault
github-actions
bot
added
the
theme/connect
kisunji
reviewed
Dec 5, 2022
| SkipIfVaultNotPresent(t) | ||
|
|
||
| provider, testVault := testVaultProviderWithConfig(t, false, nil) | ||
| defer testVault.Stop() | ||
| t.Parallel() |
There was a problem hiding this comment.
Are these safe to run parallel now?
There was a problem hiding this comment.
They always were I believe. Each test gets its own vault process.
I added this b/c it shaved the runtime down by a factor of ~5. Locally it was ~28s -> ~6s.
| tests := CASigningKeyTypeCases() | ||
|
|
||
| for _, tc := range tests { | ||
| tc := tc |
There was a problem hiding this comment.
We actually need this now that we're running t.Parallel(), right?
There was a problem hiding this comment.
I did the run(t, tc) thing here instead.
kisunji
reviewed
Dec 5, 2022
| for _, tc := range KeyTestCases { | ||
| tc := tc |
There was a problem hiding this comment.
I just did the run(t, tc) approach for the rest of the test table tests so it shouldn't be necessary.
kisunji
reviewed
Dec 5, 2022
rboyer
commented
Dec 5, 2022
| } | ||
| } | ||
|
|
||
| func CreateVaultTokenWithAttrs(t testing.T, client *vaultapi.Client, attr *VaultTokenAttributes) string { |
There was a problem hiding this comment.
New test helper for use outside of the package too.
rboyer
commented
Dec 5, 2022
| @@ -87,7 +87,7 @@ func (s *Server) checkBindingRuleUUID(id string) (bool, error) { | |||
| } | |||
|
|
|||
| func (s *Server) InPrimaryDatacenter() bool { | |||
| return s.config.PrimaryDatacenter == "" || s.config.Datacenter == s.config.PrimaryDatacenter | |||
| return s.config.InPrimaryDatacenter() | |||
There was a problem hiding this comment.
Convenience for a test in leader_connect_ca_test.go
rboyer
commented
Dec 5, 2022
| @@ -415,6 +415,10 @@ type Config struct { | |||
| *EnterpriseConfig | |||
| } | |||
|
|
|||
| func (c *Config) InPrimaryDatacenter() bool { | |||
There was a problem hiding this comment.
Convenience for a test in leader_connect_ca_test.go
rboyer
commented
Dec 5, 2022
| @@ -778,37 +826,62 @@ func TestCAManager_Initialize_Vault_WithExternalTrustedCA(t *testing.T) { | |||
|
|
|||
There was a problem hiding this comment.
For the record TestCAManager_Initialize_Vault_WithExternalTrustedCA was a test I invested a lot in because it surfaced a bunch of strange things such as #15661
rboyer
commented
Dec 5, 2022
| rawConfig: map[string]interface{}{}, | ||
| name: "DefaultConfig", | ||
| rawConfig: map[string]any{ | ||
| "RootPKIPath": "pki-root/", |
There was a problem hiding this comment.
To avoid accidentally triggering two providers to share some vault mount paths in tests, I changed it so that test authors need to explicitly set the paths all the time.
rboyer
commented
Dec 5, 2022
| @@ -1118,23 +1326,8 @@ func getIntermediateCertTTL(t *testing.T, caConf *structs.CAConfiguration) time. | |||
| return dur | |||
| } | |||
|
|
|||
| func testVaultProviderWithConfig(t *testing.T, isPrimary bool, rawConf map[string]interface{}) (*VaultProvider, *TestVaultServer) { | |||
There was a problem hiding this comment.
I inlined testVaultProviderWithConfig so that we can create the token in between the two steps.
rboyer
commented
Dec 5, 2022
| RootPath: "pki-root", | ||
| IntermediatePath: "pki-intermediate", | ||
| ConsulManaged: true, | ||
| WithSudo: withSudo, |
There was a problem hiding this comment.
This validates the continued need for the sudo capability now.
rboyer
commented
Dec 5, 2022
| assertCorrectKeyType(t, tc.CSRKeyType, intPEM) | ||
|
|
||
| if expectFailure { | ||
| testCrossSignProvidersShouldFail(t, provider1, provider2) |
There was a problem hiding this comment.
When we repeat the test without sudo we expect it to fail.
rboyer
commented
Dec 5, 2022
| @@ -224,6 +215,9 @@ func (v *TestVaultServer) Stop() error { | |||
| // wait for the process to exit to be sure that the data dir can be | |||
| // deleted on all platforms. | |||
| if err := v.cmd.Wait(); err != nil { | |||
| if strings.Contains(err.Error(), "exec: Wait was already called") { | |||
There was a problem hiding this comment.
In some cases when I was repeatably executing the tests it would hit this double-wait issue which crashed the test and flaked it. Now those get ignored to make double-Stop ok.
rboyer
commented
Dec 5, 2022
|
|
||
| case a.VaultManaged: | ||
| // Vault-managed PKI root. | ||
| t.Fatal("TODO: implement this and use it in tests") |
There was a problem hiding this comment.
This stub is here as a reminder to add more tests for the other policy in our docs.
rboyer
commented
Dec 5, 2022
| @@ -1069,7 +1068,7 @@ func (c *CAManager) secondaryRequestNewSigningCert(provider ca.Provider, newActi | |||
| } | |||
|
|
|||
| if err := setLeafSigningCert(newActiveRoot, intermediatePEM); err != nil { | |||
| return err | |||
| return fmt.Errorf("Failed to set the leaf signing cert to the intermediate: %w", err) | |||
There was a problem hiding this comment.
Wrapped the error here to help figure out which line caused the error (all other returns from this function wrap the errors).
kisunji
approved these changes
Dec 5, 2022
rboyer
force-pushed
the
vault-tests-use-tokens
branch
from
b0f628b
to
97cd22b
Compare
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ |
This was referenced Dec 6, 2022
rboyer
added a commit
that referenced
this pull request
Dec 6, 2022
…flakes (#15691) It turns out that by default the dev mode vault server will attempt to interact with the filesystem to store the provided root token. If multiple vault instances are running they'll all awkwardly share the filesystem and if timing results in one server stopping while another one is starting then the starting one will error with: Error initializing Dev mode: rename /home/circleci/.vault-token.tmp /home/circleci/.vault-token: no such file or directory This change uses `-dev-no-store-token` to bypass that source of flakes. Also the stdout/stderr from the vault process is included if the test fails. The introduction of more `t.Parallel` use in #15669 increased the likelihood of this failure, but any of the tests with multiple vaults in use (or running multiple package tests in parallel that all use vault) were eventually going to flake on this.
This was referenced Dec 6, 2022
rboyer
added a commit
that referenced
this pull request
Dec 6, 2022
…flakes (#15691) It turns out that by default the dev mode vault server will attempt to interact with the filesystem to store the provided root token. If multiple vault instances are running they'll all awkwardly share the filesystem and if timing results in one server stopping while another one is starting then the starting one will error with: Error initializing Dev mode: rename /home/circleci/.vault-token.tmp /home/circleci/.vault-token: no such file or directory This change uses `-dev-no-store-token` to bypass that source of flakes. Also the stdout/stderr from the vault process is included if the test fails. The introduction of more `t.Parallel` use in #15669 increased the likelihood of this failure, but any of the tests with multiple vaults in use (or running multiple package tests in parallel that all use vault) were eventually going to flake on this.
rboyer
added a commit
that referenced
this pull request
Dec 6, 2022
…flakes (#15691) It turns out that by default the dev mode vault server will attempt to interact with the filesystem to store the provided root token. If multiple vault instances are running they'll all awkwardly share the filesystem and if timing results in one server stopping while another one is starting then the starting one will error with: Error initializing Dev mode: rename /home/circleci/.vault-token.tmp /home/circleci/.vault-token: no such file or directory This change uses `-dev-no-store-token` to bypass that source of flakes. Also the stdout/stderr from the vault process is included if the test fails. The introduction of more `t.Parallel` use in #15669 increased the likelihood of this failure, but any of the tests with multiple vaults in use (or running multiple package tests in parallel that all use vault) were eventually going to flake on this.
rboyer
added a commit
that referenced
this pull request
Dec 6, 2022
…flakes (#15691) It turns out that by default the dev mode vault server will attempt to interact with the filesystem to store the provided root token. If multiple vault instances are running they'll all awkwardly share the filesystem and if timing results in one server stopping while another one is starting then the starting one will error with: Error initializing Dev mode: rename /home/circleci/.vault-token.tmp /home/circleci/.vault-token: no such file or directory This change uses `-dev-no-store-token` to bypass that source of flakes. Also the stdout/stderr from the vault process is included if the test fails. The introduction of more `t.Parallel` use in #15669 increased the likelihood of this failure, but any of the tests with multiple vaults in use (or running multiple package tests in parallel that all use vault) were eventually going to flake on this.
rboyer
added a commit
that referenced
this pull request
Dec 6, 2022
…flakes (#15691) (#15694) It turns out that by default the dev mode vault server will attempt to interact with the filesystem to store the provided root token. If multiple vault instances are running they'll all awkwardly share the filesystem and if timing results in one server stopping while another one is starting then the starting one will error with: Error initializing Dev mode: rename /home/circleci/.vault-token.tmp /home/circleci/.vault-token: no such file or directory This change uses `-dev-no-store-token` to bypass that source of flakes. Also the stdout/stderr from the vault process is included if the test fails. The introduction of more `t.Parallel` use in #15669 increased the likelihood of this failure, but any of the tests with multiple vaults in use (or running multiple package tests in parallel that all use vault) were eventually going to flake on this. Co-authored-by: R.B. Boyer <4903+rboyer@users.noreply.github.com>
rboyer
added a commit
that referenced
this pull request
Dec 6, 2022
…flakes (#15691) (#15693) It turns out that by default the dev mode vault server will attempt to interact with the filesystem to store the provided root token. If multiple vault instances are running they'll all awkwardly share the filesystem and if timing results in one server stopping while another one is starting then the starting one will error with: Error initializing Dev mode: rename /home/circleci/.vault-token.tmp /home/circleci/.vault-token: no such file or directory This change uses `-dev-no-store-token` to bypass that source of flakes. Also the stdout/stderr from the vault process is included if the test fails. The introduction of more `t.Parallel` use in #15669 increased the likelihood of this failure, but any of the tests with multiple vaults in use (or running multiple package tests in parallel that all use vault) were eventually going to flake on this. Co-authored-by: R.B. Boyer <4903+rboyer@users.noreply.github.com>
rboyer
added a commit
that referenced
this pull request
Dec 6, 2022
…flakes (#15691) (#15692) It turns out that by default the dev mode vault server will attempt to interact with the filesystem to store the provided root token. If multiple vault instances are running they'll all awkwardly share the filesystem and if timing results in one server stopping while another one is starting then the starting one will error with: Error initializing Dev mode: rename /home/circleci/.vault-token.tmp /home/circleci/.vault-token: no such file or directory This change uses `-dev-no-store-token` to bypass that source of flakes. Also the stdout/stderr from the vault process is included if the test fails. The introduction of more `t.Parallel` use in #15669 increased the likelihood of this failure, but any of the tests with multiple vaults in use (or running multiple package tests in parallel that all use vault) were eventually going to flake on this. Co-authored-by: R.B. Boyer <4903+rboyer@users.noreply.github.com>
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
All of the current integration tests where Vault is the Connect CA now use non-root tokens for the test. This helps us detect privilege changes in the vault model so we can keep our guides up to date.
One larger change was that the
RenewIntermediatefunction got refactored slightly so it could be used from a test, rather than the large duplicated function we were testing in a test which seemed error prone.