VAULT-31409: trace postUnseal function by bosouza · Pull Request #28895 · hashicorp/vault

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:
All Go tests succeeded! ✅

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

 	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

+		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

 	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) }

makes sense, it's good as is!

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

miagilepner

looks good, though a config test is still failing. once that's passing i'm good to approve it!

miagilepner · 2024-11-25T15:58:17Z

helper/trace/debug_trace.go

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


makes sense, it's good as is!

miagilepner

💯

* initial implementation of unseal trace * close file if we fail to start the trace didn't bother to check the error from traceFile.Close() * use reloadable config instead of env var * license * remove leftover * allow setting custom dir and remove new package * bring back StartDebugTrace 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 also some usability improvements from manual testing * address PR comments * address security review 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 * move back to default dir * document new parameters * add tiny integration test * avoid column in trace filename sounds like it might be forbidden in Windows and possibly cause problems in some MacOS applications. * address PR feedback * add go doc to test CI was complaining about missing comments on the new test function. It feels a bit silly to require this of tests but whatever XD * fix tests

initial implementation of unseal trace

9e89242

bosouza requested review from kubawi, miagilepner and raskchanky 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

bosouza requested a review from miagilepner November 25, 2024 12:32

bosouza added backport/ent/1.17.x+ent labels Nov 25, 2024

kubawi added this to the 1.18.3 milestone Nov 25, 2024

miagilepner reviewed Nov 25, 2024

View reviewed changes

fix tests

b4355a1

miagilepner approved these changes Nov 26, 2024

View reviewed changes

bosouza merged commit a2c467c into main Nov 26, 2024

bosouza deleted the bosouza/VAULT-31409-unseal-trace branch November 26, 2024 18:04

hc-github-team-secure-vault-core mentioned this pull request Nov 26, 2024

Backport of VAULT-31409: trace postUnseal function into release/1.18.x #29030

Merged

6 tasks

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

Conversation

bosouza commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

TODO only if you're a HashiCorp employee

Uh oh!

github-actions bot commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 22, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

miagilepner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miagilepner left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bosouza commented Nov 13, 2024 •

edited

Loading

github-actions bot commented Nov 13, 2024 •

edited

Loading