cli: support encrypted fs with pebble debug tool

[andyyang890]

Release note (cli change): The cockroach debug pebble tool
can now be used with encrypted stores.

[cockroach-teamcity]

This change is 

[petermattis]

Can we add a test that this is working correctly to cli/interactive_tests/test_encryption.tcl? I'm imagining something where you call one of the pebble commands on an encrypted store.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890 and @jbowens)

---

pkg/cli/debug.go, line 716 at r1 (raw file):

		}
		cfg.Opts.Cache = pebble.NewCache(server.DefaultCacheSize)
		defer cfg.Opts.Cache.Unref()

Are these two lines doing anything useful here? I don't see cfg.Opts.Cache being used anywhere in ResolveEncryptedEnvOptions. I suspect all this is currently doing is creating a cache and then destroying it, but perhaps I'm missing something.

---

pkg/cli/debug.go, line 1329 at r1 (raw file):

// swap replaces the FS in a swappableFS.
func (s swappableFS) swap(fs vfs.FS) {

The verb swap suggests that the old value would be returned. Perhaps s/swap/set/g to make it clearer that you're only setting the FS.

[andyyang890]

Okay, will add a test once I get it working. It's still a WIP right now.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jbowens and @petermattis)

---

pkg/cli/debug.go, line 716 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Are these two lines doing anything useful here? I don't see cfg.Opts.Cache being used anywhere in ResolveEncryptedEnvOptions. I suspect all this is currently doing is creating a cache and then destroying it, but perhaps I'm missing something.

Good point, I removed it.

---

pkg/cli/debug.go, line 1329 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

The verb swap suggests that the old value would be returned. Perhaps s/swap/set/g to make it clearer that you're only setting the FS.

Done.

[knz]

Reviewed 1 of 3 files at r1, 1 of 2 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @jbowens, and @petermattis)

---

pkg/cli/debug.go, line 699 at r2 (raw file):

Allows the use of pebble tools, such as to introspect manifests, SSTables, etc.
`,
	PersistentPreRunE: func(cmd *cobra.Command, args []string) error {

As you've already established by testing, PersistentPreRunE will not help you here unfortunately.

The next best thing is to iterate (with a recursive function) on debugPebbleCmd.Commands() and add your initializer there. Like this:

func pebbleCryptoInitializer(*cobra.Command, args) { /* this code here */ }

...
func initPebbleCmds(cmd *cobra.Command) {
   for _, c := range cmd.Commands() {
         wrapped := c.PreRunE
         c.PreRunE := func(...) { 
                if wrapped != nil { if err := wrapped(...); err != nil { return err } }
                pebbleCryptoInitializer(...)
          }
          initPebbleCmds(c)
    }

(it's a shame that cobra doesn't give us a .VisitSubCommands() so here we are)

Then you'll also need to modify cli.go to move the call to setupLogging above the call to the wrapped PreRunE (so that it runs before).

[knz]

can you also file two issues in the upstream spf13/cobra package:

• missing VisitSubCommands visitor
• complain that there cannot be multiple PersistentPreRunE initializers at different levels (causing one at the root to never been called if a child command defines one)

[sumeerbhola]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @jbowens, @knz, and @petermattis)

---

pkg/ccl/cliccl/debug.go, line 95 at r2 (raw file):

	for _, cmd := range cli.DebugCmdsForRocksDB {
		// storeEncryptionSpecs is in start.go.
		cli.VarFlag(cmd.PersistentFlags(), &storeEncryptionSpecs, cliflagsccl.EnterpriseEncryption)

@knz do you have an opinion on this use of PersistentFlags, which is now also being used for all DebugCmdsForRocksDB which I think prior to this change only contained leaf commands.

The alternative would be to treat debugPebbleCommand differently since it is the only one where we are having to work with the root command in a tree of commands (as done in sumeerbhola@3b3a641)

---

pkg/cli/debug.go, line 1329 at r1 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

Done.

I suppose we don't have to worry about a race between set and get in a tool.

---

pkg/cli/debug.go, line 699 at r2 (raw file):

Previously, knz (kena) wrote…

As you've already established by testing, PersistentPreRunE will not help you here unfortunately.

The next best thing is to iterate (with a recursive function) on debugPebbleCmd.Commands() and add your initializer there. Like this:

func pebbleCryptoInitializer(*cobra.Command, args) { /* this code here */ }

...
func initPebbleCmds(cmd *cobra.Command) {
   for _, c := range cmd.Commands() {
         wrapped := c.PreRunE
         c.PreRunE := func(...) { 
                if wrapped != nil { if err := wrapped(...); err != nil { return err } }
                pebbleCryptoInitializer(...)
          }
          initPebbleCmds(c)
    }

(it's a shame that cobra doesn't give us a .VisitSubCommands() so here we are)

Then you'll also need to modify cli.go to move the call to setupLogging above the call to the wrapped PreRunE (so that it runs before).

I think the difference between what you are suggesting wrt attaching to PreRunE for all Pebble commands in initPebbleCmds and what is done for logging in doMain is that the latter is done for only the command that is being run. In sumeerbhola@3b3a641 I did something similar to the latter for initializing the encrypted fs.
Is there a reason why the logging setup PreRunE is done in the way it is?

---

pkg/cli/flags.go, line 862 at r2 (raw file):

	{
		f := debugPebbleCmd.PersistentFlags()
		varFlag(f, &serverCfg.Stores, cliflags.Store)

As we discussed today, needing this is a bit unfortunate since one has to pass the same dir in two places, via --store and in the argument to the pebble command. Consider adding a code comment about it so we can potentially clean it up in the future.

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @jbowens, @petermattis, and @sumeerbhola)

---

pkg/cli/debug.go, line 699 at r2 (raw file):

Previously, sumeerbhola wrote…

I think the difference between what you are suggesting wrt attaching to PreRunE for all Pebble commands in initPebbleCmds and what is done for logging in doMain is that the latter is done for only the command that is being run. In sumeerbhola@3b3a641 I did something similar to the latter for initializing the encrypted fs.
Is there a reason why the logging setup PreRunE is done in the way it is?

There's a comment just above in cli.go that explains why it's done the way it's done. Is there something specific you'd like to clarify?

                        // We use a PreRun function, to ensure setupLogging() is only
                        // called after the command line flags have been parsed.
                        //
                        // NB: we cannot use PersistentPreRunE,like in flags.go, because
                        // overriding that here will prevent the persistent pre-run from
                        // running on parent commands. (See the difference between PreRun
                        // and PersistentPreRun in `(*cobra.Command) execute()`.)
                        wrapped := cmd.PreRunE
                        cmd.PreRunE = func(cmd *cobra.Command, args []string) error {
                                if wrapped != nil {
                                        if err := wrapped(cmd, args); err != nil {
                                                return err
                                        }
                                }

                                return setupLogging(context.Background(), cmd,
                                        false /* isServerCmd */, true /* applyConfig */)
                        }

[sumeerbhola]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @jbowens, @knz, and @petermattis)

---

pkg/cli/debug.go, line 699 at r2 (raw file):

Previously, knz (kena) wrote…

There's a comment just above in cli.go that explains why it's done the way it's done. Is there something specific you'd like to clarify?

                        // We use a PreRun function, to ensure setupLogging() is only
                        // called after the command line flags have been parsed.
                        //
                        // NB: we cannot use PersistentPreRunE,like in flags.go, because
                        // overriding that here will prevent the persistent pre-run from
                        // running on parent commands. (See the difference between PreRun
                        // and PersistentPreRun in `(*cobra.Command) execute()`.)
                        wrapped := cmd.PreRunE
                        cmd.PreRunE = func(cmd *cobra.Command, args []string) error {
                                if wrapped != nil {
                                        if err := wrapped(cmd, args); err != nil {
                                                return err
                                        }
                                }

                                return setupLogging(context.Background(), cmd,
                                        false /* isServerCmd */, true /* applyConfig */)
                        }

That comment talks about why PreRunE is used. I was asking about the difference in how PreRunE is used -- the logging initialization code is only attaching to PreRunE for the command being executed, while IIUC initPebbleCmds would attach up-front to all Pebble commands. It is possible my question doesn't make any sense, or the choice is arbitrary.

[andyyang890]

can you also file two issues in the upstream spf13/cobra package:

• missing VisitSubCommands visitor
• complain that there cannot be multiple PersistentPreRunE initializers at different levels (causing one at the root to never been called if a child command defines one)

@knz Thanks for the review!

I filed spf13/cobra#1397 for the first one.

I noticed for the second one that spf13/cobra#1198 was filed before and closed. Do you still want to open a new one?

[knz]

Thanks for this. I hadn't noticed that issue 1198 before, and sadly the recommendation expressed in that thread wouldn't help us here.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @jbowens, @petermattis, and @sumeerbhola)

---

pkg/cli/debug.go, line 699 at r2 (raw file):

Previously, sumeerbhola wrote…

That comment talks about why PreRunE is used. I was asking about the difference in how PreRunE is used -- the logging initialization code is only attaching to PreRunE for the command being executed, while IIUC initPebbleCmds would attach up-front to all Pebble commands. It is possible my question doesn't make any sense, or the choice is arbitrary.

We're not attaching the logging config code to all commands because certain commands (e.g. start, demo) have their own logging setup code already elsewhere. Hence the conditional and ad-hoc attaching.

[andyyang890]

Added a test to cli/interactive_tests/test_encryption.tcl.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jbowens, @knz, @petermattis, and @sumeerbhola)

---

pkg/ccl/cliccl/debug.go, line 95 at r2 (raw file):

Previously, sumeerbhola wrote…

@knz do you have an opinion on this use of PersistentFlags, which is now also being used for all DebugCmdsForRocksDB which I think prior to this change only contained leaf commands.

The alternative would be to treat debugPebbleCommand differently since it is the only one where we are having to work with the root command in a tree of commands (as done in sumeerbhola@3b3a641)

I followed your suggestion and made only DebugPebbleCmd use PersistentFlags.

---

pkg/cli/debug.go, line 699 at r2 (raw file):

Previously, knz (kena) wrote…

We're not attaching the logging config code to all commands because certain commands (e.g. start, demo) have their own logging setup code already elsewhere. Hence the conditional and ad-hoc attaching.

Thanks for the suggestion, I gave it a go and it looks like it works.

---

pkg/cli/flags.go, line 862 at r2 (raw file):

Previously, sumeerbhola wrote…

As we discussed today, needing this is a bit unfortunate since one has to pass the same dir in two places, via --store and in the argument to the pebble command. Consider adding a code comment about it so we can potentially clean it up in the future.

Done.

[knz]

Reviewed 8 of 8 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @jbowens, @petermattis, and @sumeerbhola)

---

pkg/cli/absolute_fs.go, line 22 at r3 (raw file):

// before calling the underlying interface implementation for each function.
type absoluteFS struct {
	vfs.FS

This design is not safe. If vfs.FS is ever extended with new methods, we will not see this via a compile error and your absoluteFS abstraction will expose the underlying method without the wrappers.

The proper design is to do struct {fs vfs.FS}.
This way if a new method ever appears in the interface we'll get a compile error as a reminder to do the new wrapper.

---

pkg/cli/debug.go, line 1328 at r3 (raw file):

	pebbleTool := tool.New(tool.Mergers(storage.MVCCMerger),
		tool.DefaultComparer(storage.EngineComparer),
		tool.FS(&absoluteFS{pebbleToolFS}),

out of curiosity, can you explain (to me) why the pebble tool needs absolute paths?

[andyyang890]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @jbowens, @knz, @petermattis, and @sumeerbhola)

---

pkg/cli/debug.go, line 1328 at r3 (raw file):

Previously, knz (kena) wrote…

out of curiosity, can you explain (to me) why the pebble tool needs absolute paths?

The reasoning is that the PebbleFileRegistry stores a map from filenames to FileEntry structs and the relative path used by Pebble is preprended with the store directory, which is incompatible with the function tryMakeRelativePath in:

func (r *PebbleFileRegistry) GetFileEntry(filename string) *enginepb.FileEntry {
	filename = r.tryMakeRelativePath(filename)
	r.mu.Lock()
	defer r.mu.Unlock()
	return r.mu.currProto.Files[filename]
}

[sumeerbhola]

Reviewed 8 of 8 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @jbowens, @knz, @petermattis, and @sumeerbhola)

---

pkg/cli/absolute_fs.go, line 20 at r3 (raw file):

// absoluteFS is a wrapper vfs.FS that converts filepath names to absolute paths
// before calling the underlying interface implementation for each function.

can you add the following to this comment
// This is needed when using encryptedFS, since the PebbleFileRegistry used in that context attempts to convert function input paths to relative paths using the DBDir. Both the DBDir and function input paths in a CockroachDB node are absolute paths, but when using the Pebble tool, the function input paths are based on what the cli user passed to the pebble command. We do not wish for a user using the cli to remember to pass an absolute path to the various pebble tool commands that accept paths. Note that the pebble tool commands taking a path parameter are quite varied: ranging from "pebble db

" to "pebble lsm ", so it is simplest to intercept the function input paths here.

---

pkg/cli/cli.go, line 105 at r3 (raw file):

				// that function may perform logging.
				err := setupLogging(context.Background(), cmd,
					false /* isServerCmd */, true /* applyConfig */)

This is simpler than what I did in sumeerbhola@3b3a641 which added a second wrapping of PreRunE. Is this change safe for all tool commands, and are there any tool tests that would validate that?
@knz

[andyyang890]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @jbowens, @knz, @petermattis, and @sumeerbhola)

---

pkg/cli/absolute_fs.go, line 20 at r3 (raw file):

Previously, sumeerbhola wrote…

can you add the following to this comment
// This is needed when using encryptedFS, since the PebbleFileRegistry used in that context attempts to convert function input paths to relative paths using the DBDir. Both the DBDir and function input paths in a CockroachDB node are absolute paths, but when using the Pebble tool, the function input paths are based on what the cli user passed to the pebble command. We do not wish for a user using the cli to remember to pass an absolute path to the various pebble tool commands that accept paths. Note that the pebble tool commands taking a path parameter are quite varied: ranging from "pebble db

" to "pebble lsm ", so it is simplest to intercept the function input paths here.

Actually, now that I think about it, would it be better to do something like this:

func (r *PebbleFileRegistry) tryMakeRelativePath(filename string) string {
	// ...
	absolutePath, err := filepath.Abs(filename)
	if err == nil {
		filename = absolutePath
	}

Then we can do away with absoluteFS. I think from the fact the function name includes "try", we can skip returning the error if one occurs.

[sumeerbhola]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @jbowens, @knz, @petermattis, and @sumeerbhola)

---

pkg/cli/absolute_fs.go, line 20 at r3 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

Actually, now that I think about it, would it be better to do something like this:

func (r *PebbleFileRegistry) tryMakeRelativePath(filename string) string {
	// ...
	absolutePath, err := filepath.Abs(filename)
	if err == nil {
		filename = absolutePath
	}

Then we can do away with absoluteFS. I think from the fact the function name includes "try", we can skip returning the error if one occurs.

We have multiple vfs.FS implementations, and an implementation, like MemFS, may have nothing to do with the underlying filesystem. filepath.Abs eventually needs to call os.Getwd, which means it will not work for those. And there is no Getwd equivalent in the vfs.FS interface and I don't think there is any real need to add one just for this tool use case.

Can you add something like the following to the comment here
// Note that absoluteFS assumes the wrapped vfs.FS corresponds to the underlying OS filesystem and will not work for the general case of a vfs.FS. This limitation is acceptable for this tool's use cases.

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @jbowens, @knz, @petermattis, and @sumeerbhola)

---

pkg/cli/absolute_fs.go, line 22 at r3 (raw file):

Previously, knz (kena) wrote…

This design is not safe. If vfs.FS is ever extended with new methods, we will not see this via a compile error and your absoluteFS abstraction will expose the underlying method without the wrappers.

The proper design is to do struct {fs vfs.FS}.
This way if a new method ever appears in the interface we'll get a compile error as a reminder to do the new wrapper.

this is still an issue

---

pkg/cli/cli.go, line 105 at r3 (raw file):

Previously, sumeerbhola wrote…

This is simpler than what I did in sumeerbhola@3b3a641 which added a second wrapping of PreRunE. Is this change safe for all tool commands, and are there any tool tests that would validate that?
@knz

I believe this is safe.

[andyyang890]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jbowens, @knz, @petermattis, and @sumeerbhola)

---

pkg/cli/absolute_fs.go, line 20 at r3 (raw file):

Previously, sumeerbhola wrote…

We have multiple vfs.FS implementations, and an implementation, like MemFS, may have nothing to do with the underlying filesystem. filepath.Abs eventually needs to call os.Getwd, which means it will not work for those. And there is no Getwd equivalent in the vfs.FS interface and I don't think there is any real need to add one just for this tool use case.

Can you add something like the following to the comment here
// Note that absoluteFS assumes the wrapped vfs.FS corresponds to the underlying OS filesystem and will not work for the general case of a vfs.FS. This limitation is acceptable for this tool's use cases.

Thanks for the explanation, I added both of those passages to the comment.

---

pkg/cli/absolute_fs.go, line 22 at r3 (raw file):

Previously, knz (kena) wrote…

this is still an issue

Fixed now.

[jbowens]

👌 chef's kiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @knz, @petermattis, and @sumeerbhola)

---

pkg/cli/absolute_fs.go, line 22 at r4 (raw file):

// absoluteFS is a wrapper vfs.fs that converts filepath names to absolute paths
// before calling the underlying interface implementation for some functions.

Maybe explicitly state in the first paragraph of this comment that this absoluteFS is only used by the pebble tool.

---

pkg/cli/absolute_fs.go, line 133 at r4 (raw file):

}

func wrapWithAbsolute3(fn func(string, string) error, oldname, newname string) error {

It seems like only wrapWithAbsolute3 has multiple callers. Inline all the others and rename this one wrapWithAbsolute?

---

pkg/cli/flags.go, line 862 at r2 (raw file):

Previously, andyyang890 (Andy Yang) wrote…

Done.

Avoiding this would require threading through all the various paths provided to the pebble tool when constructing the vfs.FS and identify the store root based on the path, right?

[andyyang890]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jbowens, @knz, @petermattis, and @sumeerbhola)

---

pkg/cli/absolute_fs.go, line 22 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

Maybe explicitly state in the first paragraph of this comment that this absoluteFS is only used by the pebble tool.

Done.

---

pkg/cli/absolute_fs.go, line 133 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

It seems like only wrapWithAbsolute3 has multiple callers. Inline all the others and rename this one wrapWithAbsolute?

Done.

---

pkg/cli/flags.go, line 862 at r2 (raw file):

Previously, jbowens (Jackson Owens) wrote…

Avoiding this would require threading through all the various paths provided to the pebble tool when constructing the vfs.FS and identify the store root based on the path, right?

Yep, I believe that's the case.

[sumeerbhola]

Reviewed 1 of 1 files at r5.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jbowens, @knz, @petermattis, and @sumeerbhola)

[andyyang890]

bors r=sumeerbhola

[craig]

Build succeeded:

• GitHub CI (Cockroach)