[ConfigManager] Check a Node's sei.toml 3/3

cmd/seid/cmd/check_through_root_test.go

@@ -0,0 +1,144 @@
+package cmd
+
+import (
+	"bytes"
+	"context"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/sei-protocol/sei-chain/sei-cosmos/client"
+	"github.com/sei-protocol/sei-chain/sei-cosmos/client/flags"
+	"github.com/sei-protocol/sei-chain/sei-cosmos/server"
+	tmcli "github.com/sei-protocol/sei-chain/sei-tendermint/libs/cli"
+	"github.com/sei-protocol/sei-chain/testutil/configtest"
+)
+
+// runCheckThroughRoot runs `sei-config check --home <dir>` the way an operator and a runbook run it.
+//
+// Through the real root command, because that is what makes the difference. A command executed on its own
+// has no parent, so nothing the root does before it happens: no hook runs, no flag is marked changed by
+// anything but the caller, and no file is generated. Every one of those is a thing this command has to be
+// right about, and a test that builds the command directly cannot see any of them.
+func runCheckThroughRoot(t *testing.T, home string, extraArgs ...string) (string, error) {
+	t.Helper()
+
+	root, _ := NewRootCmd()
+	var out bytes.Buffer
+	root.SetOut(&out)
+	root.SetErr(&out)
+	root.SilenceUsage = true
+	root.SilenceErrors = true
+
+	// The same wiring the binary uses. --home is registered by PrepareBaseCmd rather than by the root
+	// command itself, so a root built without it is not the command an operator runs.
+	srvCtx := server.NewDefaultContext()
+	ctx := context.WithValue(context.Background(), client.ClientContextKey, &client.Context{})
+	ctx = context.WithValue(ctx, server.ServerContextKey, srvCtx)
+	root.PersistentFlags().String(flags.FlagLogLevel, "", "")
+	root.PersistentFlags().String(flags.FlagLogFormat, "", "")
+	executor := tmcli.PrepareBaseCmd(root, "", home)
+
+	root.SetArgs(append([]string{"sei-config", "check", "--home", home}, extraArgs...))
+	err := executor.ExecuteContext(ctx)
+	return out.String(), err
+}
+
+// writeSeiToml puts a sei.toml in a home that holds nothing else.
+func writeSeiToml(t *testing.T, body string) string {
+	t.Helper()
+	home := t.TempDir()
+	if err := os.MkdirAll(filepath.Join(home, "config"), 0o750); err != nil {
+		t.Fatalf("mkdir: %v", err)
+	}
+	if err := os.WriteFile(filepath.Join(home, "config", "sei.toml"), []byte(body), 0o600); err != nil {
+		t.Fatalf("write sei.toml: %v", err)
+	}
+	return home
+}
+
+// TestTheCheckPassesACorrectFileWhenRunAsAnOperatorRunsIt is the answer the command exists to give.
+//
+// The one flag every real invocation carries is --home, and it names no setting: the node is told where its
+// files are, not what to put in them. It arrives in the resolution beside the file's own keys, and counted
+// with them it is a key nothing declares, so the command failed on a correct file and named a key the file
+// does not contain. A pre-flight that fails every time carries nothing, and it is the whole compensating
+// control for a boot that may not refuse a file.
+func TestTheCheckPassesACorrectFileWhenRunAsAnOperatorRunsIt(t *testing.T) {
+	configtest.Isolate(t)
+	home := writeSeiToml(t, "schema_version = 1\nnode_mode = \"validator\"\n\n[mempool]\nsize = 4321\n")
+
+	out, err := runCheckThroughRoot(t, home)
+	if err != nil {
+		t.Errorf("a correct file was refused when the command was run with --home: %v\n%s", err, out)
+	}
+	for _, flag := range []string{"home", "trace", "chain-id"} {
+		if strings.Contains(out, flag+": sei.toml writes this") {
+			t.Errorf("the report names --%s as something sei.toml wrote. The file does not contain it, "+
+				"and an operator told this on every run stops reading the one signal they have", flag)
+		}
+	}
+}
+
+// TestTheCheckDoesNotGenerateTheFilesItIsAskedAbout holds the command to answering rather than acting.
+//
+// The root command's hook runs the configuration handler, which writes config.toml and app.toml when they
+// are absent. A command that reports on one file would then create two others as a side effect, on a node
+// an operator was only asking a question about.
+func TestTheCheckDoesNotGenerateTheFilesItIsAskedAbout(t *testing.T) {
+	configtest.Isolate(t)
+	home := writeSeiToml(t, "schema_version = 1\nnode_mode = \"validator\"\n\n[mempool]\nsize = 4321\n")
+
+	// The verdict is not what this measures, and asserting it first would stop the measurement whenever
+	// something else about the check is wrong.
+	if out, err := runCheckThroughRoot(t, home); err != nil {
+		t.Logf("the check reported a problem, which is not what this test is about: %v\n%s", err, out)
+	}
+	for _, name := range []string{"config.toml", "app.toml"} {
+		if _, err := os.Stat(filepath.Join(home, "config", name)); err == nil {
+			t.Errorf("%s was created by a command that answers a question about a different file", name)
+		}
+	}
+}
+
+// TestTheCheckStillFailsAFileWithSomethingWrongInIt keeps the fixes above from making it answer nothing.
+//
+// Passing a correct file and generating no files are both satisfied by a command that does nothing at all,
+// so the failure has to be shown to still happen through the same path.
+func TestTheCheckStillFailsAFileWithSomethingWrongInIt(t *testing.T) {
+	configtest.Isolate(t)
+	for _, tc := range []struct {
+		name string
+		body string
+		says string
+	}{
+		{
+			name: "a key no section declares",
+			body: "schema_version = 1\nnode_mode = \"validator\"\n\n[mempool]\nsizze = 4321\n",
+			says: "no section declares it",
+		},
+		{
+			name: "a length of time as a plain number",
+			body: "schema_version = 1\nnode_mode = \"validator\"\n\n[mempool]\nttl-duration = 60\n",
+			says: "nanoseconds",
+		},
+		{
+			name: "a number larger than the setting holds",
+			body: "schema_version = 1\nnode_mode = \"validator\"\n\n[p2p]\nmax-connections = 1e20\n",
+			says: "larger than this setting can hold",
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			out, err := runCheckThroughRoot(t, writeSeiToml(t, tc.body))
+			if err == nil {
+				t.Fatalf("the file was accepted, so a boot would apply what it could and the operator "+
+					"would find out afterwards\n%s", out)
+			}
+			if !strings.Contains(out, tc.says) {
+				t.Errorf("the report does not say %q, so it does not tell an operator what to change:\n%s",
+					tc.says, out)
+			}
+		})
+	}
+}

cmd/seid/cmd/configmanager/check.go

@@ -0,0 +1,205 @@
+package configmanager
+
+import (
+	"errors"
+	"fmt"
+	"io"
+	"io/fs"
+	"os"
+	"path/filepath"
+	"sort"
+	"strings"
+
+	"github.com/spf13/cobra"
+	"github.com/spf13/viper"
+
+	"github.com/sei-protocol/sei-chain/config/registry"
+	tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config"
+)
+
+// CheckCmd answers, without starting a node, whether this binary can use a sei.toml.
+//
+// A boot may not refuse a file. A node that stopped because one line was mistyped is worse than a node
+// running the value it ran yesterday, so every failure at boot is a report and the node keeps going. That
+// makes the report the only signal, and a fleet rolling a configuration change forward reads it after the
+// change is already on every node.
+//
+// The same questions have exact answers before then. The file, the binary and the environment are all the
+// input, so the same file against the same binary gives the same answer here as it will at boot, for the
+// same environment. That last part is a real condition and not a formality: this reads the environment of
+// whoever runs it, and a node started by an init system or a container runtime has a different one. A
+// variable that answers a declared key is a variable this cannot see unless it is set here too.
+//
+// What it does not rehearse is the install into the source a node builds, because that source does not
+// exist until a boot builds it. A key can be refused there for a reason nothing here can see, and the whole
+// install is dropped when it is. That is worth adding when the surface it covers is more than a handful of
+// keys on a live node.
+//
+// This asks what it can answer where an answer costs a failed check rather than a restart.
+func CheckCmd() *cobra.Command {
+	cmd := &cobra.Command{
+		Use:   "check",
+		Short: "Report whether this binary can use the node's sei.toml",
+		Long: "Resolves the node's sei.toml the way a boot resolves it and reports every value this " +
+			"binary would refuse, without starting anything. Exits non-zero if there is one.\n\n" +
+			"A boot cannot refuse a file, so it applies what it can and reports the rest. Running this " +
+			"first is how a mistyped value costs a failed check rather than a restart.",
+		Args: cobra.NoArgs,
+		RunE: func(cmd *cobra.Command, _ []string) error {
+			problems, found, err := checkSeiToml(cmd)
+			if err != nil {
+				return err
+			}
+			if !found {
+				report(cmd.OutOrStdout(), "this node has no sei.toml, so every key reads as it always "+
+					"has and there is nothing here to be wrong")
+				return nil
+			}
+			out := cmd.OutOrStdout()
+			reportWhetherABootWouldReadThisFile(out, os.Getenv)
+			for _, line := range problems {
+				report(out, line)
+			}
+			if len(problems) > 0 {
+				return fmt.Errorf("%d problem(s); a boot would apply what it could and report the rest",
+					len(problems))
+			}
+			report(out, "every value this file supplies is one this binary can use")
+			return nil
+		},
+	}
+	return cmd
+}
+
+// reportWhetherABootWouldReadThisFile says whether this node is set up to use the file at all.
+//
+// Without it, a passing check reads as "this file is in use and correct" on a node where a boot ignores it
+// completely, which is the state every node is in until an operator switches the gate. That is the wrong
+// conclusion in the more dangerous direction: it invites somebody to trust a file nothing reads.
+//
+// Answered from the environment this command runs in, which is the same limitation the resolution has.
+func reportWhetherABootWouldReadThisFile(out io.Writer, getenv func(string) string) {
+	if _, err := Select(getenv); err != nil {
+		report(out, fmt.Sprintf("%s is set to something this binary does not accept, so a boot would "+
+			"refuse before reaching this file: %v", EnvVar, err))
+		return
+	}
+	if getenv(EnvVar) != "v2" {
+		report(out, fmt.Sprintf("%s is not set to v2 for this command, so a boot in the same environment "+
+			"reads none of this file. What follows is what it would reach if it were", EnvVar))
+	}
+}
+
+// report writes one line of the answer.
+//
+// A failed write is dropped rather than returned. Where this runs the answer is the exit status, and a
+// caller that cannot read the report still gets that.
+func report(out io.Writer, line string) { _, _ = fmt.Fprintln(out, line) }
+
+// checkSeiToml resolves the node's file and returns what a boot would refuse, in the order it would.
+//
+// The absence of a file is not a problem to report: a node without one reads exactly as it always has, so
+// there is nothing here that could be wrong. A file that exists and will not read is the opposite, and is
+// reported as a problem of a file that was found.
+func checkSeiToml(cmd *cobra.Command) (problems []string, found bool, err error) {
+	home, err := resolveHomeDir(cmd)
+	if err != nil {
+		return nil, false, fmt.Errorf("resolve the home directory: %w", err)
+	}
+	file, err := readSeiTomlAt(home)
+	switch {
+	case errors.Is(err, fs.ErrNotExist):
+		// The absence of a file is the one case with nothing to report.
+		return nil, false, nil
+	case err != nil:
+		// A file that exists and will not read is the case this command exists for. Reported as a problem
+		// of a file that was found, so the command exits non-zero: an operator running this before a
+		// restart is asking whether their file is right, and answering that they have no file is both
+		// wrong and the answer least likely to make them look.
+		return []string{fmt.Sprintf("sei.toml cannot be read: %v", err)}, true, nil
+	}
+	mode, err := file.Mode()
+	if err != nil {
+		return []string{fmt.Sprintf("sei.toml records no usable node mode: %v", err)}, true, nil
+	}
+	written, err := file.Values()
+	if err != nil {
+		return []string{fmt.Sprintf("sei.toml cannot be read: %v", err)}, true, nil
+	}
+
+	resolved, err := registry.Resolve(registry.Mode(mode), registry.Sources{
+		File:      written,
+		LookupEnv: os.LookupEnv,
+		Flags:     flagValues(TypedFlags(cmd)),
+	})
+	if err != nil {
+		return []string{fmt.Sprintf("this node's configuration cannot be resolved: %v", err)}, true, nil
+	}
+
+	// Only the file's own keys. A flag matching no declared key arrives in the same resolution and is not
+	// a mistake: every command carries flags that name no setting, so reporting those would fail this
+	// check on every invocation that types one, including a correct file.
+	for _, key := range resolved.UnknownInFile {
+		problems = append(problems, fmt.Sprintf("%s: sei.toml writes this and no section declares it, "+
+			"so it has no effect", key))
+	}
+	if running := theModeTheNodesOwnFileRecords(home); modesDisagree(mode, running) {
+		problems = append(problems, fmt.Sprintf("sei.toml says this is a %s and the node's own "+
+			"configuration file says %s. Every value resolved here is the answer for the first and the "+
+			"node would run as the second", mode, running))
+	}
+	problems = append(problems, whatADecodeWouldRefuse(resolved)...)
+	return problems, true, nil
+}
+
+// theModeTheNodesOwnFileRecords reads what kind of node the node's own configuration file says this is.
+//
+// Read from the file here rather than taken from a running node, because nothing is running. An absent file
+// or an absent key answers empty, which is not a disagreement: a node that was never initialised has
+// nothing to disagree with.
+func theModeTheNodesOwnFileRecords(home string) string {
+	v := viper.New()
+	v.SetConfigFile(filepath.Join(home, "config", "config.toml"))
+	if err := v.ReadInConfig(); err != nil {
+		return ""
+	}
+	return v.GetString("mode")
+}
+
+// whatADecodeWouldRefuse rehearses each decoded section the way the boot's delivery does.
+//
+// Rehearsed against a fresh configuration rather than a running node's, because there is no node here. That
+// is a weaker target than the delivery uses, and the difference is the point: a value this accepts may still
+// be refused at boot if the field it lands on holds something this cannot see. It is why this reports what
+// it can answer and the boot still reports what it finds.
+func whatADecodeWouldRefuse(resolved registry.Resolved) []string {
+	bySection := registry.SuppliedByDecodedSection(resolved)
+	var problems []string
+	for _, name := range sortedKeys(bySection) {
+		values := bySection[name]
+		base := tmcfg.DefaultConfig()
+
+		// Each message says what is wrong with the value it names, and there is more than one thing that
+		// can be. Stating one of them here would describe the others wrongly.
+		if bad := whatDecodesToSomethingElse(base, values); len(bad) > 0 {
+			problems = append(problems, fmt.Sprintf("[%s]: %s", name, strings.Join(bad, "; ")))
+			continue
+		}
+
+		source := viper.New()
+		for key, value := range values {
+			source.Set(key, value)
+		}
+		candidate, err := copyNodeConfig(base)
+		if err != nil {
+			problems = append(problems, fmt.Sprintf("[%s]: cannot be rehearsed: %v", name, err))
+			continue
+		}
+		if err := source.Unmarshal(candidate); err != nil {
+			problems = append(problems, fmt.Sprintf("[%s]: %v, so none of this section would apply "+
+				"(keys: %s)", name, err, strings.Join(sortedKeys(values), ",")))
+		}
+	}
+	sort.Strings(problems)
+	return problems
+}