VAULT-31409: trace postUnseal function #28895

bosouza · 2024-11-13T15:11:57Z

Description

This PR adds 2 new settings to the top-level Vault server configuration: enable_post_unseal_trace that when enabled will cause Vault to generate a Go trace during the execution of core.postUnseal and save it to disk under the directory configured in post_unseal_trace_dir, defaulting to /tmp/vault-traces/ instead if the field isn't set. This file can be inspected using the go tool trace command and will help us debug cases where the leadership transfer operation takes too long.

Most of the feature was split into a reusable StartDebugTrace function in a separate package. It could be used for similar debug purposes in the future but most importantly for now it avoids adding more code and dependencies to the already-bloated core package.

Manually tested the functionality to ensure that it was possible to generate the traces by updating the new fields in the config, then sending a SIGHUP signal to the running Vault process and triggering a leadership election via vault operator step-down. Disabling the generation of the traces via the same SIGHUP process also works. Post-unseal only runs on the active node so restarting a follower won't generate the post-unseal traces.

Jira: VAULT-31409

TODO only if you're a HashiCorp employee

Backport Labels: If this PR is in the ENT repo and needs to be backported, backport
to N, N-1, and N-2, using the backport/ent/x.x.x+ent labels. If this PR is in the CE repo, you should only backport to N, using the backport/x.x.x label, not the enterprise labels.
- If this fixes a critical security vulnerability or severity 1 bug, it will also need to be backported to the current LTS versions of Vault. To ensure this, use all available enterprise labels.
ENT Breakage: If this PR either 1) removes a public function OR 2) changes the signature
of a public function, even if that change is in a CE file, double check that
applying the patch for this PR to the ENT repo and running tests doesn't
break any tests. Sometimes ENT only tests rely on public functions in CE
files.
Jira: If this change has an associated Jira, it's referenced either
in the PR description, commit message, or branch name.
RFC: If this change has an associated RFC, please link it in the description.
ENT PR: If this change has an associated ENT PR, please link it in the
description. Also, make sure the changelog is in this PR, not in your ENT PR.

github-actions · 2024-11-13T15:27:11Z

CI Results: failed ❌
Failures:

Test Type	Package	Test	Logs
race	command/server	TestConfig_Sanitized	view test results
race	http	TestSysConfigState_Sanitized	view test results
race	http	TestSysConfigState_Sanitized/inmem_storage,_no_HA_storage	view test results
race	http	TestSysConfigState_Sanitized/inmem_storage,_raft_HA_storage	view test results
race	http	TestSysConfigState_Sanitized/raft_storage	view test results
standard	command/server	TestConfig_Sanitized	view test results
standard	http	TestSysConfigState_Sanitized	view test results
standard	http	TestSysConfigState_Sanitized/inmem_storage,_no_HA_storage	view test results
standard	http	TestSysConfigState_Sanitized/inmem_storage,_raft_HA_storage	view test results
standard	http	TestSysConfigState_Sanitized/raft_storage	view test results

helper/trace/debug_trace.go

vault/core.go

didn't bother to check the error from traceFile.Close()

bosouza · 2024-11-14T21:10:34Z

helper/trace/debug_trace.go

+	path := fmt.Sprintf("%s/%s_%s", os.TempDir(), filePrefix, time.Now().Format(time.RFC3339))
+	traceFile, err := os.Create(path)


still want to make this more similar to VAULT_STACKTRACE_WRITE_TO_FILE for the sake of consistency

after talking to Kuba it sounds like it's a good idea to try to move stuff out of core, so even if there's no immediate need for a generic debug trace function it's still fair to add it

also some usability improvements from manual testing

miagilepner · 2024-11-19T10:29:13Z

helper/trace/debug_trace.go

+		}
+	}
+
+	traceFile, err := filepath.Abs(fmt.Sprintf("%s/%s-%s.trace", dir, filePrefix, time.Now().Format(time.RFC3339)))


can we switch this to using filepath.Join to create the path, rather than Sprintf?

miagilepner · 2024-11-19T10:40:20Z

command/server/config.go

@@ -115,6 +115,9 @@ type Config struct {
 	License          string `hcl:"-"`
 	LicensePath      string `hcl:"license_path"`
 	DisableSSCTokens bool   `hcl:"-"`
+
+	EnablePostUnsealTrace bool   `hcl:"enable_post_unseal_trace"`
+	PostUnsealTraceDir    string `hcl:"post_unseal_trace_dir"`


let's also add these fields to the (*Config).Merge function

ahh good catch, wouldn't have noticed this unless I'd tested with multiple files. I wonder why we didn't go for a more generic implementation here (maybe didn't want to use reflect?) but we could at least have some semgrep validation to check that all fields are merged (or explicitly ignored)

bosouza · 2024-11-18T20:28:32Z

vault/core.go

+	if stopTrace := c.tracePostUnsealIfEnabled(); stopTrace != nil {
+		defer stopTrace()
+	}
+
 	defer metrics.MeasureSince([]string{"core", "post_unseal"}, time.Now())


not sure what is the best order between this metric and the new trace, but I think it's fair to include the metric in the captured trace even tho that will make the time spend on the tracing setup invisible to the metric

bosouza · 2024-11-19T12:44:18Z

command/server/config.go

@@ -115,6 +115,9 @@ type Config struct {
 	License          string `hcl:"-"`
 	LicensePath      string `hcl:"license_path"`
 	DisableSSCTokens bool   `hcl:"-"`
+
+	EnablePostUnsealTrace bool   `hcl:"enable_post_unseal_trace"`
+	PostUnsealTraceDir    string `hcl:"post_unseal_trace_dir"`


ahh good catch, wouldn't have noticed this unless I'd tested with multiple files. I wonder why we didn't go for a more generic implementation here (maybe didn't want to use reflect?) but we could at least have some semgrep validation to check that all fields are merged (or explicitly ignored)

there were concerns about using the /tmp directory because of permissions, or having a default dir at all, so now it's required to set a dir in order to generate the traces.

sounds like it might be forbidden in Windows and possibly cause problems in some MacOS applications.

github-actions · 2024-11-22T16:08:04Z

Build Results:
All builds succeeded! ✅

vault/testing.go

miagilepner · 2024-11-22T16:16:21Z

helper/trace/debug_trace.go

+		return "", nil, fmt.Errorf("trace directory %q does not exist", dir)
+	}
+
+	if !os.IsNotExist(err) && !d.IsDir() {


do we need the !os.IsNotExist(err) portion of this condition?

yes, I think we need it. If os.IsNotExist(err) evaluates to true then we don't want to evaluate d.IsDir() since d would be nil.

The alternative would be to use a nested if-else structure that interleaves error returns with additional operations (creation of default dir), which I don't like as much:

d, err := os.Stat(dir) if err != nil { if os.IsNotExist(err) { if dirMustExist { return "", nil, fmt.Errorf("trace directory %q does not exist", dir) } else { if err := os.Mkdir(dir, 0o700); err != nil { return "", nil, fmt.Errorf("failed to create trace directory %q: %s", dir, err) } } } else { return "", nil, fmt.Errorf("failed to stat trace directory %q: %s", dir, err) } } else if !d.IsDir() { return "", nil, fmt.Errorf("trace directory %q is not a directory", dir) }

helper/trace/debug_trace_test.go

CI was complaining about missing comments on the new test function. It feels a bit silly to require this of tests but whatever XD

initial implementation of unseal trace

9e89242

bosouza requested review from raskchanky, miagilepner and kubawi November 13, 2024 15:11

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Nov 13, 2024

miagilepner reviewed Nov 14, 2024

View reviewed changes

helper/trace/debug_trace.go Show resolved Hide resolved

helper/trace/debug_trace.go Show resolved Hide resolved

vault/core.go Outdated Show resolved Hide resolved

bosouza added 3 commits November 14, 2024 10:10

close file if we fail to start the trace

cef2b03

didn't bother to check the error from traceFile.Close()

use reloadable config instead of env var

f4a81cd

license

84a7f45

bosouza commented Nov 14, 2024

View reviewed changes

bosouza added 4 commits November 14, 2024 18:11

remove leftover

5ef2d7b

allow setting custom dir and remove new package

8b5e8cf

bring back StartDebugTrace

0739284

after talking to Kuba it sounds like it's a good idea to try to move stuff out of core, so even if there's no immediate need for a generic debug trace function it's still fair to add it

track postUnseal instead of unsealInternal

2dd31a7

also some usability improvements from manual testing

bosouza changed the title ~~VAULT-31409: trace unsealInternal function~~ VAULT-31409: trace postUnseal function Nov 18, 2024

miagilepner reviewed Nov 19, 2024

View reviewed changes

bosouza commented Nov 19, 2024

View reviewed changes

bosouza added 4 commits November 19, 2024 11:54

address PR comments

42b18af

address security review

a46f103

there were concerns about using the /tmp directory because of permissions, or having a default dir at all, so now it's required to set a dir in order to generate the traces.

add unit tests to StartDebugTrace

2cc5d37

Merge branch 'main' into bosouza/VAULT-31409-unseal-trace

e126200

bosouza added the backport/1.18.x label Nov 19, 2024

vercel bot deployed to Preview November 19, 2024 21:01 View deployment

bosouza added 2 commits November 20, 2024 15:57

move back to default dir

00efb75

document new parameters

29caf28

vercel bot deployed to Preview November 20, 2024 21:07 View deployment

bosouza added 2 commits November 22, 2024 11:26

add tiny integration test

002d93e

Merge branch 'main' into bosouza/VAULT-31409-unseal-trace

6110c74

vercel bot deployed to Preview November 22, 2024 14:36 View deployment

avoid column in trace filename

9f1ad5a

sounds like it might be forbidden in Windows and possibly cause problems in some MacOS applications.

bosouza requested a review from miagilepner November 22, 2024 15:48

bosouza marked this pull request as ready for review November 22, 2024 15:49

bosouza requested review from a team as code owners November 22, 2024 15:49

miagilepner reviewed Nov 22, 2024

View reviewed changes

bosouza added 2 commits November 22, 2024 17:57

address PR feedback

d93be1a

add go doc to test

b0b266a

CI was complaining about missing comments on the new test function. It feels a bit silly to require this of tests but whatever XD

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VAULT-31409: trace postUnseal function #28895

VAULT-31409: trace postUnseal function #28895

bosouza commented Nov 13, 2024 •

edited

Loading

github-actions bot commented Nov 13, 2024 •

edited

Loading

bosouza Nov 14, 2024

miagilepner Nov 19, 2024

miagilepner Nov 19, 2024

bosouza Nov 19, 2024

bosouza Nov 18, 2024

bosouza Nov 19, 2024

github-actions bot commented Nov 22, 2024

miagilepner Nov 22, 2024

bosouza Nov 22, 2024

		path := fmt.Sprintf("%s/%s_%s", os.TempDir(), filePrefix, time.Now().Format(time.RFC3339))
		traceFile, err := os.Create(path)

VAULT-31409: trace postUnseal function #28895

Are you sure you want to change the base?

VAULT-31409: trace postUnseal function #28895

Conversation

bosouza commented Nov 13, 2024 • edited Loading

Description

TODO only if you're a HashiCorp employee

github-actions bot commented Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bosouza commented Nov 13, 2024 •

edited

Loading

github-actions bot commented Nov 13, 2024 •

edited

Loading