[ConfigManager] Deliver sei.toml for Decoded Sections 2/3

cmd/seid/cmd/boot_install_test.go

@@ -182,7 +182,8 @@ func TestOnlyWhatASourceSuppliedIsInstalled(t *testing.T) {
 func TestAppTomlDoesNotReachTheFlagChannel(t *testing.T) {
 	const key = "state-sync.snapshot-keep-recent"
 	if _, declared := declaredKey(key); !declared {
-		t.Skipf("%s is not declared, so this cannot happen through it", key)
+		t.Fatalf("%s is not declared, so this test cannot reach the inversion it exists for. Skipping "+
+			"instead would leave the only guard on it passing while measuring nothing", key)
 	}
 	configtest.Isolate(t)
 
@@ -224,15 +225,18 @@ func TestAppTomlDoesNotReachTheFlagChannel(t *testing.T) {
 // accepted, which measures the absence of a value rather than the refusal.
 func TestAFileThisBinaryCannotUseLeavesTheNodeAsItWas(t *testing.T) {
 	supplies := "\n[evm]\nmax_tx_pool_txs = 111\n"
-	for name, body := range map[string]string{
-		"no file at all":       "",
-		"a mode nothing knows": "schema_version = 1\nnode_mode = \"sentry\"\n" + supplies,
-		"no mode at all":       "schema_version = 1\n" + supplies,
-		"not parseable":        "schema_version = 1\nnode_mode = \"validator\"\n[evm\n" + supplies,
+	for _, tc := range []struct {
+		name string
+		body string
+	}{
+		{"no file at all", ""},
+		{"a mode nothing knows", "schema_version = 1\nnode_mode = \"sentry\"\n" + supplies},
+		{"no mode at all", "schema_version = 1\n" + supplies},
+		{"not parseable", "schema_version = 1\nnode_mode = \"validator\"\n[evm\n" + supplies},
 	} {
-		t.Run(name, func(t *testing.T) {
+		t.Run(tc.name, func(t *testing.T) {
 			configtest.Isolate(t)
-			ctx := bootWith(t, body, nil)
+			ctx := bootWith(t, tc.body, nil)
 			if got := ctx.Viper.Get(bootProbeKey); got != nil {
 				t.Errorf("%s reads %#v, so a value was installed from a file this binary cannot use. "+
 					"A node whose file names a mode this binary does not know would run one mode's "+

cmd/seid/cmd/configmanager/decode_report_test.go

@@ -0,0 +1,104 @@
+package configmanager
+
+import (
+	"bytes"
+	"log/slog"
+	"strings"
+	"testing"
+
+	"github.com/sei-protocol/sei-chain/config/registry"
+	"github.com/sei-protocol/sei-chain/sei-cosmos/server"
+	tmcfg "github.com/sei-protocol/sei-chain/sei-tendermint/config"
+)
+
+// TestTheNothingSuppliedLineIsNotSaidWhenADecodeDelivered holds the two reports against each other.
+//
+// The two deliveries run in one pass and only the second one's emptiness is what that line describes. An
+// operator whose file moved a setting through the first was told the file supplied nothing, in the same
+// boot and a few lines later, which is the one report they have to go on.
+func TestTheNothingSuppliedLineIsNotSaidWhenADecodeDelivered(t *testing.T) {
+	var out bytes.Buffer
+	log := slog.New(slog.NewTextHandler(&out, &slog.HandlerOptions{Level: slog.LevelDebug}))
+
+	ctx := &server.Context{Config: tmcfg.DefaultConfig()}
+	resolved := registry.Resolved{
+		Values:    map[string]any{"p2p.max-connections": 77},
+		Overrides: []string{"p2p.max-connections"},
+	}
+
+	delivered := deliverDecodedSections(ctx, resolved, log)
+	if !delivered {
+		t.Fatalf("a declared key of a decoded section was not delivered, so this measures nothing:\n%s",
+			out.String())
+	}
+	if got := ctx.Config.P2P.MaxConnections; got != 77 {
+		t.Errorf("the peer ceiling is %v after a delivery of 77, so nothing was decoded", got)
+	}
+
+	// The line the caller would emit for an empty lookup delivery. It must not be said in this boot.
+	supplied := onlyWhatALookupSourceSupplied(resolved)
+	if len(supplied.Values) != 0 {
+		t.Fatalf("this key reached the lookup delivery too, so the contradiction under test cannot arise")
+	}
+	if !delivered {
+		log.Info("sei.toml supplies no declared value; every key reads as it always has", "mode", "full")
+	}
+	if strings.Contains(out.String(), "supplies no declared value") {
+		t.Errorf("the boot reported that the file supplied nothing, having just moved a setting:\n%s",
+			out.String())
+	}
+}
+
+// TestAPasswordInASettingDoesNotReachTheReport covers the one value here that is a secret.
+//
+// The transaction index can be told to write to PostgreSQL, and it is told so with a connection string that
+// carries the password in it. This report is the only place the running configuration is written down,
+// which makes it the only place that password reaches a log file, a journal and whatever ships them onward.
+// The node's own configuration file holds the same string, and nothing there reads it out to a log.
+//
+// The report cannot be turned down either: this package holds its own logger at a floor so a quiet fleet
+// still sees what a delivery changed.
+func TestAPasswordInASettingDoesNotReachTheReport(t *testing.T) {
+	const password = "sup3rs3cret"
+	const dsn = "postgres://seid:" + password + "@10.0.0.9:5432/idx"
+
+	var out bytes.Buffer
+	log := slog.New(slog.NewTextHandler(&out, &slog.HandlerOptions{Level: slog.LevelDebug}))
+	reportWhatMoved("tx-index",
+		[]string{"tx-index.psql-conn"},
+		map[string]string{"tx-index.psql-conn": ""},
+		map[string]string{"tx-index.psql-conn": dsn},
+		log)
+
+	if strings.Contains(out.String(), password) {
+		t.Errorf("the report carries the password from a connection string: %s", out.String())
+	}
+	if !strings.Contains(out.String(), "10.0.0.9:5432") {
+		t.Errorf("the report no longer says where the index writes, so an operator cannot tell what "+
+			"moved: %s", out.String())
+	}
+	if !strings.Contains(out.String(), "tx-index.psql-conn") {
+		t.Errorf("the report does not name the key that moved: %s", out.String())
+	}
+}
+
+// TestAValueWithNoPasswordIsReportedAsWritten keeps the redaction from rewriting ordinary values.
+//
+// Most settings are not connection strings, and a value an operator reads back has to be the one they
+// wrote. A path, a host and port, and a list of words all parse as something a URL parser accepts, so the
+// narrow case is what has to be detected rather than anything that parses.
+func TestAValueWithNoPasswordIsReportedAsWritten(t *testing.T) {
+	for _, value := range []string{
+		"tcp://0.0.0.0:26656",
+		"/var/lib/sei/data",
+		"kv",
+		"",
+		"postgres://seid@10.0.0.9:5432/idx",
+		"a,b,c",
+	} {
+		if got := withoutCredentials(value); got != value {
+			t.Errorf("%q is reported as %q, and an operator reading it back has to see what they wrote",
+				value, got)
+		}
+	}
+}

cmd/seid/cmd/configmanager/doc.go

@@ -8,31 +8,44 @@
 // always been answered, and then delivers whatever sei.toml supplies on top of that. It also runs an
 // advisory validation pass, which never rewrites a file and never refuses a boot.
 //
-// # Delivering a value
+// # Two deliveries, because a node reads a setting two ways
 //
-// A node reads a setting one of two ways, and only one of them can be delivered from here today. Most
-// settings are looked up by name from a source the boot builds, so a resolved value reaches them by being
-// installed into that source. The settings the node's own configuration file carries are read once, by
+// Most settings are looked up by name from a source the boot builds, so a resolved value reaches them by
+// being installed into that source. The settings the node's own configuration file carries are read once, by
 // decoding that file into a struct before any lookup happens, so a value installed into the source
-// afterwards reaches nothing at all. Those sections are identified and deliberately left out of the
-// install, because installing a value that changes nothing is worse than not installing it: it reads as
-// applied everywhere except in the node.
+// afterwards reaches nothing at all. Those are decoded into a copy of the struct and published into it.
 //
-// Only what a source supplied is installed. A resolution answers for every declared key, so installing all
+// A section names which of the two it needs, and the registry answers for the name. Nothing can tell from
+// the outside: both look like a key with a value.
+//
+// Only what a source supplied is delivered. A resolution answers for every declared key, so delivering all
 // of it would write a default over whatever an operator's own file holds for every key their sei.toml does
 // not mention.
 //
+// # Precedence
+//
+// A value in sei.toml wins over the same key in app.toml or config.toml. The node's own files are read
+// first and sei.toml is delivered on top, so for a key both state, the running node uses sei.toml's and the
+// other file still says what it said.
+//
+// That is why the reports name every key that moved. After the delivery, neither of the node's own files
+// describes what it is running, and nothing else does either.
+//
 // # Refusing nothing
 //
 // Nothing here can stop a node starting. A missing sei.toml, an unreadable one, a mode this binary does not
-// know, a value the install refuses, or a panic in the delivery itself all leave every key reading as it
-// always has, and the node starts. Selecting this manager is a switch rather than a configuration change,
-// and a mistyped line in a hand-edited file must not become an outage on the next restart.
-//
-// What that costs is that a value which does not arrive is reported rather than refused, which makes these
-// reports the only signal an operator has. So they are held at a level that survives a fleet running its
-// nodes quiet, they name the source they are about, and they are bounded, because a report that fires on
-// every boot is one nobody reads.
+// know, a value that decodes to something other than what it says, or a panic in the delivery itself all
+// leave every key reading as it always has, and the node starts. Selecting this manager is a switch rather
+// than a configuration change, and a mistyped line in a hand-edited file must not become an outage on the
+// next restart.
+//
+// A refusal is per section, not per file, because a decode is all or nothing for whatever it is handed. An
+// operator who fixes one setting and mistypes another gets the first one.
+//
+// What all that costs is that a value which does not arrive is reported rather than refused, which makes
+// these reports the only signal an operator has. So they are held at a level that survives a fleet running
+// its nodes quiet, they name the source they are about, they carry no password, and they are bounded,
+// because a report that fires on every boot is one nobody reads.
 //
 // Deferred: a path that writes sei.toml, so a node's configuration can be rendered from it rather than only
 // read into it.

cmd/seid/cmd/configmanager/install.go

@@ -81,15 +81,28 @@ func installResolved(cmd *cobra.Command, typed map[string]string, log *slog.Logg
 			"mode", mode, "err", err)
 		return
 	}
-	// A key nothing declares is the most common thing an operator gets wrong and the only signal they
-	// have for it.
+	// First, because every report below is a log line and a refusal is reported at a level an operator
+	// may have raised the threshold above. Doing this after would mean the one setting somebody changes
+	// in order to see a refusal is the setting a refusal suppresses.
+	applyResolvedLogLevel(resolved, typed, log)
+
+	// After the level, so a file that raises it can report its own mistakes. A key nothing declares is
+	// the most common thing an operator gets wrong and the only signal they have for it.
 	reportWhatTheFileDidNotReach(resolved, log)
 
 	reportWhatTheFileSaysTheNodeIs(ctx, mode, log)
 
+	// The second delivery. Their file is read into a struct before this runs and nothing consults the
+	// source for them afterwards, so the values are decoded into that struct instead.
+	delivered := deliverDecodedSections(ctx, resolved, log)
+
 	supplied := onlyWhatALookupSourceSupplied(resolved)
 	if len(supplied.Values) == 0 {
-		log.Info("sei.toml supplies no declared value; every key reads as it always has", "mode", mode)
+		// Scoped to this delivery, because the other one runs first: an operator whose file moved a
+		// setting through a decode was otherwise told the file supplied nothing, in the same boot.
+		if !delivered {
+			log.Info("sei.toml supplies no declared value; every key reads as it always has", "mode", mode)
+		}
 		return
 	}
 	report, err := appopts.Install(ctx.Viper, supplied)
@@ -136,15 +149,9 @@ func everyChannelAnOperatorCanUse(written map[string]any, typed map[string]strin
 // It also means a declared default never reaches a running node, which is what lets a default state what
 // the provisioning command writes rather than having to state what each node already runs.
 func onlyWhatALookupSourceSupplied(resolved registry.Resolved) registry.Resolved {
-	decoded := registry.DecodedSections()
 	owning := map[string]bool{}
-	for _, section := range registry.Sections() {
-		if _, ok := decoded[section.Name]; !ok {
-			continue
-		}
-		for _, key := range section.Keys {
-			owning[key] = true
-		}
+	for _, key := range registry.KeysADecodeDelivers() {
+		owning[key] = true
 	}
 
 	out := registry.Resolved{Values: make(map[string]any, len(resolved.Overrides))}