diff --git a/cmd/seid/cmd/boot_install_test.go b/cmd/seid/cmd/boot_install_test.go new file mode 100644 index 0000000000..8e12d33f73 --- /dev/null +++ b/cmd/seid/cmd/boot_install_test.go @@ -0,0 +1,267 @@ +package cmd + +import ( + "fmt" + "os" + "path/filepath" + "testing" + + "go.opentelemetry.io/otel/sdk/trace" + + "github.com/sei-protocol/sei-chain/cmd/seid/cmd/configmanager" + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + "github.com/sei-protocol/sei-chain/testutil/configtest" +) + +// Keys these tests measure through, and why each one. +// +// The first two are declared keys that nothing else in a booting node answers: no start flag carries them +// and the generated app.toml does not name them, so a value read back for one came from this install and +// from nowhere else. Of a hundred and fifty declared keys only eleven are like that, and the rest are +// reachable by a flag of the same name, whose registration default answers before the lookup comes back +// empty. Measuring through one of those would be reading the flag's default and calling it an install. +// +// The third is the opposite case on purpose: a key a start flag does carry, so it is the one that can show +// the flag channel reaching a declared key at all. +const ( + bootProbeKey = "evm.max_tx_pool_txs" + bootUntouchedKey = "state-commit.sc-snapshot-writer-limit" + bootFlagKey = "state-sync.snapshot-keep-recent" +) + +// bootWith runs a real boot against a sei.toml and returns the source a node would read. +// +// Flags are set through the command rather than handed to the install, because it is the flag being marked +// changed that the snapshot reads. A value poked in directly would hold even if the boot never looked at +// the command line. +func bootWith(t *testing.T, body string, typed map[string]string) *server.Context { + t.Helper() + home := configtest.NewHome(t) + if err := os.MkdirAll(filepath.Join(home.Root, "config"), 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + if body != "" { + path := filepath.Join(home.Root, "config", "sei.toml") + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatalf("write sei.toml: %v", err) + } + } + + cmd := server.StartCmd(nil, home.Root, []trace.TracerProviderOption{}) + if err := cmd.Flags().Set("home", home.Root); err != nil { + t.Fatalf("set --home: %v", err) + } + for name, value := range typed { + if err := cmd.Flags().Set(name, value); err != nil { + t.Fatalf("set --%s=%s: %v", name, value, err) + } + } + ctx, err := runManager(t, configmanager.SeiConfigManager{}, cmd) + if err != nil { + t.Fatalf("the boot was refused: %v", err) + } + return ctx +} + +// seiTomlWriting returns a file body that writes one key, wherever that key belongs. +// +// A key with no section goes above every table. Once a table heading is open every bare key after it +// belongs to that table, so a node-wide setting written after one would be read under the wrong name. +func seiTomlWriting(key, value string) string { + const header = "schema_version = 1\nnode_mode = \"validator\"\n" + if i := indexOf(key, '.'); i >= 0 { + return header + "\n[" + key[:i] + "]\n" + key[i+1:] + " = " + value + "\n" + } + return header + key + " = " + value + "\n" +} + +func indexOf(s string, c byte) int { + for i := 0; i < len(s); i++ { + if s[i] == c { + return i + } + } + return -1 +} + +// TestEachChannelWinsOverTheOneBelowIt drives the declared order through a real boot. +// +// Every channel that can carry a value has to reach the resolution. A channel that is not wired does not +// fail, it stops applying: a value an operator supplied through it loses to a lower layer and nothing +// reports it. So each one is supplied a value and the declared order has to hold. +func TestEachChannelWinsOverTheOneBelowIt(t *testing.T) { + t.Run("nothing written leaves the key as it was", func(t *testing.T) { + configtest.Isolate(t) + ctx := bootWith(t, "schema_version = 1\nnode_mode = \"validator\"\n", nil) + if got := ctx.Viper.Get(bootProbeKey); got != nil { + t.Errorf("%s reads %#v with nothing written. A file that supplies no value installs nothing, "+ + "so this key should read as it did before the manager ran", bootProbeKey, got) + } + }) + + t.Run("the file beats the default", func(t *testing.T) { + configtest.Isolate(t) + ctx := bootWith(t, seiTomlWriting(bootProbeKey, "111"), nil) + if got := ctx.Viper.Get(bootProbeKey); !sameSetting(got, int64(111)) { + t.Errorf("%s reads %#v with 111 written, want 111. A file channel that is not passed to the "+ + "resolution leaves the operator's value losing to the default", bootProbeKey, got) + } + }) + + t.Run("the environment beats the file", func(t *testing.T) { + configtest.Isolate(t) + t.Setenv(registry.EnvName(bootProbeKey), "222") + ctx := bootWith(t, seiTomlWriting(bootProbeKey, "111"), nil) + if got := ctx.Viper.Get(bootProbeKey); !sameSetting(got, "222") { + t.Errorf("%s reads %#v with 111 in the file and 222 in the environment, want 222", bootProbeKey, got) + } + }) + + t.Run("a typed flag beats both", func(t *testing.T) { + configtest.Isolate(t) + t.Setenv(registry.EnvName(bootFlagKey), "222") + ctx := bootWith(t, seiTomlWriting(bootFlagKey, "111"), map[string]string{bootFlagKey: "333"}) + if got := ctx.Viper.Get(bootFlagKey); !sameSetting(got, "333") { + t.Errorf("%s reads %#v with 111 in the file, 222 in the environment and --%s=333 typed, "+ + "want 333. An operator who types a flag to override a file has to win, and a flag "+ + "whose name never reaches the resolution loses to both", bootFlagKey, got, bootFlagKey) + } + }) +} + +// TestOnlyWhatASourceSuppliedIsInstalled is the property that makes this safe to enable. +// +// A resolution answers for every declared key. Installing all of it would write a default over whatever a +// node's app.toml holds for every key its sei.toml does not mention, so moving one setting would replace a +// hundred and fifty. This installs only what a source supplied, so a key reaches a node exactly when +// somebody asked for it. +// +// Measured as an absence rather than against a value read back from a second boot. A baseline taken through +// this same install would carry whatever the install wrote, so an install that wrote a default over every +// key would write the same one twice and the two runs would agree. The assertion is that the key is not +// there at all, which no install can satisfy by being wrong the same way twice. +func TestOnlyWhatASourceSuppliedIsInstalled(t *testing.T) { + configtest.Isolate(t) + + // The declared value is read out first, because a key whose declaration answers nothing would pass + // this whether the install was contained or not. + const untouched = bootUntouchedKey + resolved, err := registry.Resolve(registry.ModeValidator, registry.Sources{}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if resolved.Values[untouched] == nil { + t.Fatalf("%s declares no value, so an install that wrote every declared default would leave it "+ + "absent too and this would measure nothing", untouched) + } + + ctx := bootWith(t, seiTomlWriting(bootProbeKey, "111"), nil) + + if got := ctx.Viper.Get(untouched); got != nil { + t.Errorf("%s reads %#v after a file that never mentions it, and %s declares %#v. Installing a "+ + "declared default over a key nobody wrote replaces an operator's configuration rather than "+ + "moving one setting of it", untouched, got, untouched, resolved.Values[untouched]) + } + if got := ctx.Viper.Get(bootProbeKey); !sameSetting(got, int64(111)) { + t.Errorf("%s reads %#v, so nothing was installed at all and the check above holds for an install "+ + "that does nothing", bootProbeKey, got) + } +} + +// TestAppTomlDoesNotReachTheFlagChannel is the guard on where the flag snapshot is taken. +// +// The handler this manager re-enters copies configuration values into flags, so that a file can supply a +// flag's default: for every flag whose name its source knows a value for, it calls Set, and Set marks the +// flag changed. After that has run, a flag an operator typed and a key their app.toml holds cannot be told +// apart. +// +// A flag channel built from that state puts app.toml at the top of the order, above sei.toml, which is a +// worse inversion than the one the channel exists to prevent. Taking the snapshot at the entry to Apply is +// what keeps the two apart, and there is no later point where the truth survives. +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) + } + configtest.Isolate(t) + + home := configtest.NewHome(t) + // app.toml holds one value and sei.toml another, and the operator typed no flag at all. + home.WriteAppTOML(t, []byte("[state-sync]\nsnapshot-keep-recent = 77\n")) + if err := os.MkdirAll(filepath.Join(home.Root, "config"), 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + body := "schema_version = 1\nnode_mode = \"validator\"\n\n[state-sync]\nsnapshot-keep-recent = 111\n" + if err := os.WriteFile(filepath.Join(home.Root, "config", "sei.toml"), []byte(body), 0o600); err != nil { + t.Fatalf("write sei.toml: %v", err) + } + + cmd := server.StartCmd(nil, home.Root, []trace.TracerProviderOption{}) + if err := cmd.Flags().Set("home", home.Root); err != nil { + t.Fatalf("set --home: %v", err) + } + ctx, err := runManager(t, configmanager.SeiConfigManager{}, cmd) + if err != nil { + t.Fatalf("the boot was refused: %v", err) + } + + if got := ctx.Viper.Get(key); !sameSetting(got, int64(111)) { + t.Errorf("%s reads %#v with 77 in app.toml, 111 in sei.toml and no flag typed, want 111.\n\n"+ + "A value of 77 means app.toml arrived through the flag channel, because the handler marked "+ + "the flag changed on its behalf. The snapshot has to be taken before the handler runs", key, got) + } +} + +// TestAFileThisBinaryCannotUseLeavesTheNodeAsItWas is the promise that makes the switch safe. +// +// Selecting this manager is a switch rather than a configuration change, so a file it cannot use installs +// nothing and the node reads what it always read. Refusing instead would turn a mistyped line in a +// hand-editable file into an outage on the next restart. +// +// Every case writes a value for a declared key, so a file that was wrongly accepted would install one and +// the assertion would see it. A case supplying nothing would read as unusable whether it was refused or +// 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, + } { + t.Run(name, func(t *testing.T) { + configtest.Isolate(t) + ctx := bootWith(t, 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 "+ + "answers while being configured as another", bootProbeKey, got) + } + }) + } +} + +// sameSetting compares two resolved values without caring which shape carried them. +// +// A value reaches a source as its own Go type from a default, as whatever the file format decoded to from +// a file, and as one string from a variable. A comparison that insisted on the type would be asserting +// which channel answered rather than what the node reads. +func sameSetting(a, b any) bool { + if a == nil || b == nil { + return a == nil && b == nil + } + return fmt.Sprint(a) == fmt.Sprint(b) +} + +// declaredKey reports whether the registry declares a key. +func declaredKey(key string) (string, bool) { + for _, section := range registry.Sections() { + for _, k := range section.Keys { + if k == key { + return section.Name, true + } + } + } + return "", false +} diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index c6e6eec499..d3ee9cba29 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -19,7 +19,20 @@ import ( "github.com/sei-protocol/sei-chain/sei-cosmos/server" ) -var logger = seilog.NewLogger("cmd", "seid", "configmanager") +// loggerSegments name this package's logger. +// +// The name it is registered under is derived from these rather than written out a second time. Held apart, +// a segment edited on one side leaves the other addressing a logger that does not exist, and the only +// symptom is that this package's reports go quiet. +var loggerSegments = []string{"cmd", "seid", "configmanager"} + +var logger = seilog.NewLogger(loggerSegments[0], loggerSegments[1:]...) + +// loggerName is the name the logger above is registered under. +var loggerName = strings.Join(loggerSegments, "/") + +// ownReportingFloor is the level this package's reports are held at. +const ownReportingFloor = slog.LevelInfo // EnvVar gates which configuration manager seid uses. const EnvVar = "SEI_CONFIG_MANAGER" @@ -54,6 +67,30 @@ type SeiConfigManager struct { logger *slog.Logger } +// keepOwnReportingVisible holds this package's own logger at a level its reports survive. +// +// Called after anything that may have set a level, and it is called more than once for that reason: the +// handler sets one, and a level this manager resolves sets another. Both set every logger in the process, so +// a floor applied before either is simply overwritten. +// +// The handler this manager re-enters sets one level across every logger in the process, from a key an +// operator writes, and a fleet that runs its nodes quiet sets it above the level these reports use. Every +// outcome here is a report: what was applied, what moved, what was refused and what had no effect. Silenced, +// the manager becomes a component that changes what a node runs and says nothing about it, and the file +// stops being something an operator can reason about from the node itself. +// +// So this one logger keeps a floor, and only this one. Raising the level for the rest of the process is +// still the operator's to choose. +func keepOwnReportingVisible() { + // A count of zero means the name matched no registered logger, so the floor was not applied and every + // report this manager makes is left at whatever level the process is running. Reported rather than + // ignored, because a silenced manager is one that changes what a node runs and says nothing about it. + if seilog.SetLevel(loggerName, ownReportingFloor) == 0 { + logger.Warn("this package's own reporting level could not be held, so its reports may be "+ + "silenced", "logger", loggerName) + } +} + // log returns the logger to report through, and never returns nil. func (m SeiConfigManager) log() *slog.Logger { if m.logger != nil { @@ -80,10 +117,23 @@ func (m SeiConfigManager) log() *slog.Logger { // handler and return nil, turning a boot the legacy path aborts into a successful one. // TestApplyPropagatesALegacyHandlerPanic fails on that combination. func (m SeiConfigManager) Apply(cmd *cobra.Command, customAppConfigTemplate string, customAppConfig any) error { + // Before the handler, because the handler copies configuration values into flags and marks them + // changed. Afterwards there is no way to tell a flag an operator typed from a key their app.toml + // holds, and treating the second as the first would put app.toml above sei.toml. + typed := TypedFlags(cmd) + out := validateAdvisory(cmd) err := server.InterceptConfigsPreRunHandler(cmd, customAppConfigTemplate, customAppConfig) + keepOwnReportingVisible() reportAdvisory(m.log(), out) - return err + if err != nil { + return err + } + + // After the handler, because the source it builds is the one the resolved values go into and it does + // not exist before. Nothing this does can refuse the boot. + installResolved(cmd, typed, m.log()) + return nil } // reportAdvisory logs an advisory outcome, containing a panic from the logging itself. @@ -227,11 +277,10 @@ func validateAdvisory(cmd *cobra.Command) (out advisoryOutcome) { return out } -// maxLoggedDiagnostics bounds the rendered list in one log line. A badly broken -// config can produce a diagnostic per field, and count is what an operator alerts -// on, so the full set is left to be re-derived from the file rather than emitted as -// one unbounded line. -const maxLoggedDiagnostics = 10 +// maxLoggedItems bounds the rendered list in one log line. A badly broken config can produce an item per +// field, and count is what an operator alerts on, so the full set is left to be re-derived from the file +// rather than emitted as one unbounded line. +const maxLoggedItems = 10 // logAdvisory reports an outcome through seilog. Nothing here refuses boot. func logAdvisory(lg *slog.Logger, out advisoryOutcome) { @@ -265,7 +314,7 @@ func logAdvisory(lg *slog.Logger, out advisoryOutcome) { if len(out.Diagnostics) == 0 { return } - shown, omitted := capDiagnostics(out.Diagnostics) + shown, omitted := capLoggedItems(out.Diagnostics) // The home is reported because a resolveHomeDir that drifted from the legacy // handler would have these diagnostics describe a directory the node is not // booting on, and without the path in the line there is no way to tell from a log. @@ -273,14 +322,15 @@ func logAdvisory(lg *slog.Logger, out advisoryOutcome) { "home", out.Home, "count", len(out.Diagnostics), "diagnostics", shown, "omitted", omitted) } -// capDiagnostics splits a diagnostic list into the part to render and the number left -// out. It is separate from logAdvisory so the arithmetic can be asserted directly: -// an off-by-one or an inverted omitted count is not visible in a log line anyone reads. -func capDiagnostics(diags []string) (shown []string, omitted int) { - if len(diags) <= maxLoggedDiagnostics { - return diags, 0 +// capLoggedItems splits a list bound for one log line into the part to render and the number left out. +// +// Separate from the callers that log so the arithmetic can be asserted directly: an off-by-one or an +// inverted omitted count is not visible in a log line anyone reads. +func capLoggedItems(items []string) (shown []string, omitted int) { + if len(items) <= maxLoggedItems { + return items, 0 } - return diags[:maxLoggedDiagnostics], len(diags) - maxLoggedDiagnostics + return items[:maxLoggedItems], len(items) - maxLoggedItems } // resolveHomeDir resolves --home the same way the legacy handler does diff --git a/cmd/seid/cmd/configmanager/configmanager_test.go b/cmd/seid/cmd/configmanager/configmanager_test.go index eef90f4741..84e36b6ce4 100644 --- a/cmd/seid/cmd/configmanager/configmanager_test.go +++ b/cmd/seid/cmd/configmanager/configmanager_test.go @@ -263,14 +263,14 @@ func TestCapDiagnostics(t *testing.T) { }{ {"none", 0, 0, 0}, {"one", 1, 1, 0}, - {"exactly at the cap", maxLoggedDiagnostics, maxLoggedDiagnostics, 0}, - {"one over the cap", maxLoggedDiagnostics + 1, maxLoggedDiagnostics, 1}, - {"far over the cap", maxLoggedDiagnostics + 15, maxLoggedDiagnostics, 15}, + {"exactly at the cap", maxLoggedItems, maxLoggedItems, 0}, + {"one over the cap", maxLoggedItems + 1, maxLoggedItems, 1}, + {"far over the cap", maxLoggedItems + 15, maxLoggedItems, 15}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { in := diags(tc.in) - shown, omitted := capDiagnostics(in) + shown, omitted := capLoggedItems(in) require.Len(t, shown, tc.wantShown) require.Equal(t, tc.wantOmitted, omitted) @@ -299,7 +299,7 @@ func TestLogAdvisoryHandlesEveryOutcome(t *testing.T) { lg := SeiConfigManager{}.log() require.NotNil(t, lg, "the zero-value manager must resolve to a usable logger") - many := make([]string, maxLoggedDiagnostics+3) + many := make([]string, maxLoggedItems+3) for i := range many { many[i] = fmt.Sprintf("[ERROR] field%d: broken", i) } diff --git a/cmd/seid/cmd/configmanager/doc.go b/cmd/seid/cmd/configmanager/doc.go index 3db2733fe2..00063e50bd 100644 --- a/cmd/seid/cmd/configmanager/doc.go +++ b/cmd/seid/cmd/configmanager/doc.go @@ -1,23 +1,39 @@ -// Package configmanager selects how seid loads its configuration, behind the -// SEI_CONFIG_MANAGER gate. Every manager boots the node identically; the only -// variable is an advisory validation pass that never rewrites a file and never -// refuses boot. -// -// SEI_CONFIG_MANAGER picks the manager: unset or "legacy" uses the legacy -// loader unchanged; "v2" uses the sei-config-backed manager. root.go calls -// Select once, during PersistentPreRunE. -// -// Both managers boot from the same two channels — serverCtx.Config and -// serverCtx.Viper — and v2 populates them exactly as legacy does: it re-enters -// the legacy reader on the operator's own files instead of rewriting them. Its -// validation pass is advisory because sei-config's read fidelity is still being -// hardened, and a gap in the model must not fail a valid node. -// -// The node boots on those two channels, never on the SeiConfig model; that is -// why differential tests suffice — proving v2's channels equal legacy's, which -// legacy is already trusted to boot on, is the whole correctness argument. -// -// Two things are deferred: making validation fatal, and authoring a canonical -// sei.toml to render the legacy files from (the generate path). See PLT-775 and -// the design (bdchatham-designs designs/config-manager/DESIGN.md). +// Package configmanager selects how seid loads its configuration, behind the SEI_CONFIG_MANAGER gate. +// +// SEI_CONFIG_MANAGER picks the manager: unset or "legacy" uses the legacy loader unchanged, and "v2" uses +// the one built on the configuration registry. root.go calls Select once, during PersistentPreRunE. +// +// The legacy manager forwards to the legacy interception handler and does nothing else. The v2 manager runs +// that same handler on the operator's own files, so every key a node reads is answered the way it has +// 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 +// +// 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 +// 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. +// +// Only what a source supplied is installed. A resolution answers for every declared key, so installing all +// of it would write a default over whatever an operator's own file holds for every key their sei.toml does +// not mention. +// +// # 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. +// +// Deferred: a path that writes sei.toml, so a node's configuration can be rendered from it rather than only +// read into it. package configmanager diff --git a/cmd/seid/cmd/configmanager/install.go b/cmd/seid/cmd/configmanager/install.go new file mode 100644 index 0000000000..d411e4a91f --- /dev/null +++ b/cmd/seid/cmd/configmanager/install.go @@ -0,0 +1,360 @@ +package configmanager + +import ( + "context" + "errors" + "io/fs" + "log/slog" + "os" + "path/filepath" + "runtime/debug" + "sort" + "strings" + + "github.com/spf13/cobra" + "github.com/spf13/pflag" + + "github.com/sei-protocol/sei-chain/config/appopts" + "github.com/sei-protocol/sei-chain/config/registry" + "github.com/sei-protocol/sei-chain/config/seitoml" + "github.com/sei-protocol/sei-chain/sei-cosmos/server" + + // The sections whose keys belong to the upstream server. A section reaches the registry through its + // owning package's initialisation, so a section nothing imports is absent from what this installs, and + // absent silently, since an undeclared key is left to whatever answered it before. + _ "github.com/sei-protocol/sei-chain/config/cosmosbase" + + // The sections whose keys belong to the node's own configuration file. These are the ones read by a + // decode, so they are deliberately left out of what this installs. + _ "github.com/sei-protocol/sei-chain/config/tendermintbase" +) + +// seiTomlName is the file this manager reads. +const seiTomlName = "sei.toml" + +// installResolved puts the values sei.toml supplies into the source the boot has just built. +// +// Nothing here refuses a boot. A node with no sei.toml, an unreadable one, one recording a mode this +// binary does not know, or a value the install cannot use installs nothing and reads exactly as it always +// has, so selecting this manager is a switch rather than a configuration change. Refusing instead would +// turn a mistyped line in a hand-editable file into an outage on the next restart. +// +// The recover below is not what carries that on its own, because a cost is not a panic: a file whose +// reading outgrows the memory the process has is killed rather than recovered, and no recover runs. The +// file is read within a bound stated where it is read, and that bound is the other half of this promise. +func installResolved(cmd *cobra.Command, typed map[string]string, log *slog.Logger) { + // A panic here would refuse the boot, which this path exists to never do. What follows walks the + // node's own configuration types by reflection and decodes through two libraries, so a panic is a + // shape nobody predicted rather than a value an operator wrote. The nested recover keeps a panic from + // the logging itself inside, the way the advisory reporter does. + defer func() { + if r := recover(); r != nil { + defer func() { _ = recover() }() + log.Error("installing this node's configuration panicked; every key reads as it always has", + "panic", r, "stack", string(debug.Stack())) + } + }() + + ctx := server.GetServerContextFromCmd(cmd) + if ctx == nil || ctx.Viper == nil { + log.Warn("no configuration source to install into; every key reads as it always has") + return + } + + file, ok := readSeiToml(cmd, log) + if !ok { + return + } + mode, ok := recordedMode(file, log) + if !ok { + return + } + written, err := file.Values() + if err != nil { + log.Warn("cannot read the values sei.toml writes; every key reads as it always has", "err", err) + return + } + + resolved, err := registry.Resolve(registry.Mode(mode), everyChannelAnOperatorCanUse(written, typed)) + if err != nil { + log.Warn("cannot resolve this node's configuration; every key reads as it always has", + "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. + reportWhatTheFileDidNotReach(resolved, log) + + reportWhatTheFileSaysTheNodeIs(ctx, mode, 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) + return + } + report, err := appopts.Install(ctx.Viper, supplied) + if err != nil { + log.Warn("cannot install the values sei.toml supplies; every key reads as it always has", + "err", err) + return + } + // Added names the keys the source did not already carry, so a node reads them from the registry for + // the first time. That is the set most likely to change what it runs, and it was computed and thrown + // away. + installed, omittedInstalled := capLoggedItems(report.Installed) + added, omittedAdded := capLoggedItems(report.Added) + log.Info("configuration installed", "mode", mode, + "count", len(report.Installed), "installed", strings.Join(installed, ","), + "omitted", omittedInstalled, + "read_here_first_count", len(report.Added), "read_here_first", strings.Join(added, ","), + "read_here_first_omitted", omittedAdded) +} + +// everyChannelAnOperatorCanUse names the sources a resolution for this node reads. +// +// Omitting one installs a lower layer over the top of what an operator chose, which is a value silently +// ignored rather than a value overridden. The flag channel matters most: an installed value sits above a +// bound flag, so a declared key a flag also delivers would resolve without ever seeing the command line +// and then bury it. +func everyChannelAnOperatorCanUse(written map[string]any, typed map[string]string) registry.Sources { + return registry.Sources{ + File: written, + LookupEnv: os.LookupEnv, + Flags: flagValues(typed), + } +} + +// onlyWhatALookupSourceSupplied narrows a resolution to the keys something other than the defaults +// answered, for the sections whose readers look a key up rather than decoding one. +// +// This is the whole difference between moving a setting and replacing a file. A resolution answers for +// every declared key, so installing all of it would write a default over whatever an operator's app.toml +// holds for every key their sei.toml does not mention: a hundred and fifty settings replaced because they +// moved one. Installing only what a source supplied means a key reaches the node exactly when somebody +// asked for it, and every other key reads as it always has. +// +// 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 + } + } + + out := registry.Resolved{Values: make(map[string]any, len(resolved.Overrides))} + for _, key := range resolved.Overrides { + // A key both deliveries carried would be installed into the source as well as decoded, and the + // install refuses a key its own contract does not cover, which would take the whole install down + // and with it every key of every other section. + if owning[key] { + continue + } + out.Values[key] = resolved.Values[key] + } + return out +} + +// reportWhatTheFileDidNotReach says what an operator asked for that had no effect. +// +// Two things, and neither is visible anywhere else. A key this file writes that no section declares reads +// as a setting and changes nothing. And a variable set for a key the environment cannot carry is skipped on +// purpose, so whatever the operator wrote elsewhere is what applies. +// +// Only the file's own keys are named. The same resolution reports flag names that match no declared key, +// and those are not a mistake: a command carries flags that name no setting at all, so reporting them would +// put this warning on every boot and bury the typo it exists to surface. +// +// The key list is capped, because a file that is broken in one way is usually broken in many, and the count +// is what an operator alerts on. +func reportWhatTheFileDidNotReach(resolved registry.Resolved, log *slog.Logger) { + if len(resolved.UnknownInFile) > 0 { + shown, omitted := capLoggedItems(resolved.UnknownInFile) + log.Warn("sei.toml writes keys no section declares; they have no effect", + "count", len(resolved.UnknownInFile), "keys", strings.Join(shown, ","), "omitted", omitted) + } + // Sorted, so a log line does not vary between runs for a configuration that did not change. + for _, key := range sortedKeys(resolved.Ignored) { + log.Warn("an environment variable is set for a key the environment cannot supply; it has no "+ + "effect and the file's value applies", "key", key, "variable", registry.EnvName(key), + "why", resolved.Ignored[key]) + } +} + +// sortedKeys returns a map's keys in a fixed order, so a report does not vary between runs for a +// configuration that did not change. +func sortedKeys[V any](m map[string]V) []string { + out := make([]string, 0, len(m)) + for key := range m { + out = append(out, key) + } + sort.Strings(out) + return out +} + +// reportWhatTheFileSaysTheNodeIs names a disagreement about what kind of node this is. +// +// Two files state that, under different names. sei.toml records it at the top, and every value resolved +// through this manager is the answer for that kind of node. The node's own configuration file states it +// again in a key of its own, and that one is what the node runs as. +// +// This manager does not declare the second, on purpose: two keys for one fact can be written to disagree, +// and then a resolution answers for one while the node is the other. Not declaring it means nothing here +// can change it, which leaves the disagreement possible and unreported. A node whose file says validator +// while it runs as a full node resolves a validator's values and serves queries, and every report about it +// reads correctly. +// +// So it is compared and reported. Reported rather than corrected, because what kind of node this is gets +// decided when it is provisioned, and a configuration manager is not the thing that should change it. +func reportWhatTheFileSaysTheNodeIs(ctx *server.Context, mode string, log *slog.Logger) { + if ctx == nil || ctx.Config == nil || ctx.Config.Mode == "" { + return + } + running := ctx.Config.Mode + if !modesDisagree(mode, running) { + return + } + log.Error("sei.toml says this is one kind of node and the node's own configuration file says another; "+ + "every value resolved here is the answer for the first and the node runs as the second", + "sei.toml", mode, "running", running) +} + +// modesDisagree reports whether the kind of node sei.toml records and the kind the node runs as are +// different kinds. +// +// One pairing is not a disagreement. The kind that keeps every version of history has no name of its own in +// the node's own configuration file, so the command that writes that file writes the query-serving name +// instead, and the difference between them lives in settings the node's own file does not carry. +func modesDisagree(recorded, running string) bool { + if recorded == running { + return false + } + return recorded != string(registry.ModeArchive) || running != string(registry.ModeFull) +} + +// OwnReportingEnabledForTest reports whether this package's logger would emit at the level its reports use. +// +// Exported for the test that holds the floor, because the thing under test is a level and not a message. +func OwnReportingEnabledForTest() bool { + return logger.Enabled(context.Background(), ownReportingFloor) +} + +// readSeiToml loads the node's sei.toml, reporting the ordinary absence quietly. +// +// A node with no sei.toml reads every key the way it always has, which is a state this manager supports +// rather than a mistake, so its absence is not a warning. A file that exists and will not parse is one +// somebody wrote that is not doing what they think, so that is. +func readSeiToml(cmd *cobra.Command, log *slog.Logger) (*seitoml.File, bool) { + home, err := resolveHomeDir(cmd) + if err != nil { + log.Warn("cannot resolve the home directory; every key reads as it always has", "err", err) + return nil, false + } + file, err := readSeiTomlAt(home) + switch { + case errors.Is(err, fs.ErrNotExist): + // Not a mistake. A node without this file resolves nothing and reads as it always has. + log.Debug("no sei.toml; every key reads as it always has", "home", home) + return nil, false + case err != nil: + // Somebody wrote this file and it is not doing what they think. Reported at a level an operator + // sees, because the alternative is a file that is silently ignored for the reason it was written. + log.Warn("this node's sei.toml cannot be read; every key reads as it always has", + "home", home, "err", err) + return nil, false + } + return file, true +} + +// readSeiTomlAt loads the sei.toml under a home directory. +// +// Separate from the reporting, so the check command can ask the same question without a logger and get the +// same answer for the same file. +func readSeiTomlAt(home string) (*seitoml.File, error) { + file, err := seitoml.Load(filepath.Join(home, "config", seiTomlName)) + if err != nil { + return nil, err + } + return file, nil +} + +// recordedMode reads the node mode the file records. +// +// Every value a node reads through the registry is the resolution for one mode, so a file that does not +// say which cannot be used at all. Reported rather than guessed: guessing picks one mode's answers for a +// node configured as another. +// +// Whether the mode is one this binary knows is not checked here. The resolution refuses a mode no section +// declares defaults for, and it names the modes there are, so a check here would be the same guard a +// second time and a worse message. +func recordedMode(file *seitoml.File, log *slog.Logger) (string, bool) { + mode, err := file.Mode() + if err != nil { + log.Warn("sei.toml records no usable node mode; every key reads as it always has", "err", err) + return "", false + } + return mode, true +} + +// TypedFlags records which flags this invocation carried, and has to run before anything else touches +// them. +// +// A flag reports itself changed when something called Set on it, and the handler this manager re-enters +// calls Set on every flag whose name its configuration knows a value for, so that a file can supply a +// flag's default. After that has run, a flag an operator typed and a key their app.toml holds are +// indistinguishable, and a flag channel built from that state would put app.toml above sei.toml. That is +// a worse inversion than the one the channel exists to prevent: the file an operator is being migrated +// onto would lose to the file they are being migrated off. +// +// So the snapshot is taken at the one point before that happens, which is the entry to Apply. Taking it +// there rather than inside the install is the difference between an invariant and a convention, because +// there is no later point at which the truth is still available. +func TypedFlags(cmd *cobra.Command) map[string]string { + out := map[string]string{} + cmd.Flags().VisitAll(func(f *pflag.Flag) { + if f.Changed { + out[strings.ToLower(f.Name)] = f.Value.String() + } + }) + return out +} + +// flagValues renders a snapshot of typed flags as a configuration source, under the keys the sections +// declare. +// +// A flag's name and the key it carries are not always spelled the same. The node's own flags separate words +// with an underscore where the tag they decode through uses a hyphen, so a flag named for a declared key is +// not equal to it, and comparing the two by string leaves an operator's typed flag looking like a name +// nothing declares. It is then dropped, and the file wins over the command line: the one channel somebody +// reaches for during an incident is the one that loses. +// +// Matched through the environment spelling, where a dot and a hyphen and an underscore are all the same +// character. That is an equivalence the registry already refuses to let two declared keys share, so a flag +// matches at most one key and no ambiguity is possible here. +// +// A flag matching no declared key is left under its own name. Most of the flags a node starts with were +// never configuration keys, and the resolution reports the unmatched ones from the file alone. +func flagValues(typed map[string]string) map[string]any { + if len(typed) == 0 { + return nil + } + byEnvName := map[string]string{} + for _, key := range registry.Keys() { + byEnvName[registry.EnvName(key)] = key + } + + out := make(map[string]any, len(typed)) + for name, value := range typed { + key := name + if declared, ok := byEnvName[registry.EnvName(name)]; ok { + key = declared + } + out[key] = value + } + return out +} diff --git a/cmd/seid/cmd/configmanager/install_read_test.go b/cmd/seid/cmd/configmanager/install_read_test.go new file mode 100644 index 0000000000..160ae56a9c --- /dev/null +++ b/cmd/seid/cmd/configmanager/install_read_test.go @@ -0,0 +1,91 @@ +package configmanager + +import ( + "bytes" + "errors" + "io/fs" + "log/slog" + "os" + "path/filepath" + "strings" + "testing" +) + +// TestAnUnreadableSeiTomlIsDistinguishedFromAnAbsentOne holds the difference an operator depends on. +// +// A node with no sei.toml is every node today, so that case is quiet. A node whose sei.toml cannot be read +// is a node where somebody wrote the file and it is doing nothing, and the two cannot share an answer: an +// error collapsed into "no file" is a mistake reported as the normal case, and the only signal the operator +// has for it is the one that got collapsed. +func TestAnUnreadableSeiTomlIsDistinguishedFromAnAbsentOne(t *testing.T) { + for _, tc := range []struct { + name string + body string + write bool + wantErr func(error) bool + }{ + { + name: "absent", + wantErr: func(err error) bool { return errors.Is(err, fs.ErrNotExist) }, + }, + { + name: "unparseable", + body: "[evm\n", + write: true, + wantErr: func(err error) bool { return err != nil && !errors.Is(err, fs.ErrNotExist) }, + }, + { + name: "no node mode", + body: "schema_version = 1\n", + write: true, + wantErr: func(err error) bool { return err != nil && !errors.Is(err, fs.ErrNotExist) }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + home := t.TempDir() + if err := os.MkdirAll(filepath.Join(home, "config"), 0o750); err != nil { + t.Fatalf("make a home: %v", err) + } + if tc.write { + if err := os.WriteFile(filepath.Join(home, "config", seiTomlName), + []byte(tc.body), 0o600); err != nil { + t.Fatalf("write the file: %v", err) + } + } + + _, err := readSeiTomlAt(home) + if !tc.wantErr(err) { + t.Errorf("reading a %s sei.toml returned %v, which does not tell an operator which of "+ + "the two happened", tc.name, err) + } + }) + } +} + +// TestAPanicWhileInstallingDoesNotRefuseTheBoot holds the claim the install path makes about itself. +// +// Selecting this manager is meant to be a switch rather than a configuration change: a node with nothing +// written, or something wrong written, starts exactly as before. Reflection over the node's own types and +// two decoding libraries sit under this, so a panic is a shape nobody predicted, and letting one escape +// would refuse a boot for the single reason this path promises never to refuse one. +func TestAPanicWhileInstallingDoesNotRefuseTheBoot(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("a panic escaped the install path and would have refused the boot: %v", r) + } + }() + + var reported bytes.Buffer + // No command at all. Reading a server context out of one is the first thing the install does, and the + // path under test is whatever happens when that cannot be done. + installResolved(nil, map[string]string{}, + slog.New(slog.NewTextHandler(&reported, &slog.HandlerOptions{Level: slog.LevelDebug}))) + + // The recover has to be shown to have run. Without this the test passes whether or not anything + // panicked, so the day the library this reaches grows a guard of its own, the recover stops being + // exercised and nothing here says so. + if !strings.Contains(reported.String(), "panicked") { + t.Fatalf("nothing panicked, so the recover this test exists for was never entered. What the "+ + "install reported instead: %q", reported.String()) + } +} diff --git a/config/cosmosbase/cosmosbase_test.go b/config/cosmosbase/cosmosbase_test.go index b8199582fa..ad83d6141b 100644 --- a/config/cosmosbase/cosmosbase_test.go +++ b/config/cosmosbase/cosmosbase_test.go @@ -161,16 +161,16 @@ func TestTheLabelSetIsRefusedFromTheEnvironment(t *testing.T) { if err != nil { t.Fatalf("Resolve: %v", err) } - var reported bool - for _, key := range resolved.Ignored { - if key == globalLabelsKey { - reported = true - } - } + reason, reported := resolved.Ignored[globalLabelsKey] if !reported { t.Errorf("a variable was set for %s and nothing reports that it did nothing. An operator whose "+ "variable is ignored has to be told", globalLabelsKey) } + if reported && reason == "" { + t.Errorf("%s is reported as ignored and carries no reason. The report is the only place an "+ + "operator learns their variable did nothing, and without a reason it does not tell them "+ + "which channel to use instead", globalLabelsKey) + } if got := resolved.Values[globalLabelsKey]; !reflect.DeepEqual(got, []any{}) { t.Errorf("%s resolved to %#v (%T), want the declared default it was left to", globalLabelsKey, got, got) diff --git a/config/registry/delivery.go b/config/registry/delivery.go new file mode 100644 index 0000000000..4049fccac5 --- /dev/null +++ b/config/registry/delivery.go @@ -0,0 +1,96 @@ +package registry + +import ( + "fmt" +) + +// decodedNotLookedUp holds the sections whose values reach their reader by a decode, and why. +var decodedNotLookedUp = map[string]string{} + +// DeclareDecodedNotLookedUp records that a section's values reach their reader by being decoded into a +// struct, rather than by a lookup in the source a node reads. +// +// Almost every section is read the other way: a reader asks for a key by name, so putting the resolved +// value into that source is the whole delivery. The sections this names are read once, by decoding a file +// into a struct before any of that happens, and a value put into the source afterwards reaches nothing. +// They need delivering a second way. +// +// The reason is required and names the struct the values are decoded into, which is what a reader has to +// check the claim against. A section declared with no reason is recorded as a defect rather than accepted, +// because the claim is the whole basis for delivering its keys differently. +// +// The section name is not checked here. A section registers itself and declares its delivery from two +// calls, and nothing fixes the order between them, so a name absent now may be registered a moment later. +// Defects answers instead, deriving it from the registry at every read: a name here that no section carries +// is reported, which is what catches the misspelling this call is most likely to contain. Left unreported, +// the section named delivers nothing and the correctly spelled section's keys install into a source its +// reader never asks. +func DeclareDecodedNotLookedUp(section, why string) { + mu.Lock() + defer mu.Unlock() + if why == "" { + defects = append(defects, Defect{Section: section, Err: fmt.Errorf( + "declared as decoded rather than looked up with no reason; the reason names the struct its " + + "values are decoded into, which is what a reader checks the claim against")}) + return + } + decodedNotLookedUp[section] = why +} + +// DecodedSections returns the sections whose values reach their reader by a decode, with the reason each +// gave, so a caller can report what it is about to do and to what. +func DecodedSections() map[string]string { + mu.RLock() + defer mu.RUnlock() + out := make(map[string]string, len(decodedNotLookedUp)) + for name, why := range decodedNotLookedUp { + out[name] = why + } + return out +} + +// SuppliedByDecodedSection splits a resolution into the values each decoded section has to deliver +// itself, keyed by section name and then by dotted key. +// +// Split per section rather than pooled, because a decode is all or nothing for whatever it is handed. One +// value a decoder refuses would otherwise cost every key in the file rather than the keys of the one +// section it appeared in, and an operator who fixed one setting and mistyped another would boot with +// neither applied and no way to tell which. +// +// The defaults are deliberately left out, and this is the difference between delivering a value and +// overwriting one. A section read by a lookup can be delivered whole, because its reader has nowhere else +// to get a value from. A section read by a decode already holds what its own file said, put there before +// any of this ran. Delivering a default over that replaces the operator's file with one nobody chose, on +// every boot, for every key their file does not mention. +// +// So a key that took its default is skipped and a key any other layer answered is delivered. That includes +// an operator writing the default value explicitly, because what is recorded is which layer answered and +// not whether the answer differs from the default: writing false where the file says true has to arrive. +func SuppliedByDecodedSection(resolved Resolved) map[string]map[string]any { + // One read for the sections and the declarations together. Asking for each on its own leaves a window a + // registration fits through, and a section arriving in it is declared by one answer and absent from the + // other, so its keys are silently left undelivered. + registered, _, owning := snapshot() + + supplied := make(map[string]bool, len(resolved.Overrides)) + for _, key := range resolved.Overrides { + supplied[key] = true + } + + out := map[string]map[string]any{} + for _, section := range registered { + if _, owned := owning[section.Name]; !owned { + continue + } + for _, key := range section.Keys { + if !supplied[key] { + continue + } + if out[section.Name] == nil { + out[section.Name] = map[string]any{} + } + out[section.Name][key] = resolved.Values[key] + } + } + return out +} diff --git a/config/registry/delivery_test.go b/config/registry/delivery_test.go new file mode 100644 index 0000000000..b50ecc7293 --- /dev/null +++ b/config/registry/delivery_test.go @@ -0,0 +1,151 @@ +package registry_test + +import ( + "reflect" + "strings" + "testing" + + "github.com/sei-protocol/sei-chain/config/registry" +) + +// probeSection is the struct the tests here register. +type probeSection struct { + A string `mapstructure:"a"` + B string `mapstructure:"b"` +} + +// registerProbe registers one section named for the caller and returns its defaults. +func registerProbe(t *testing.T, name string) { + t.Helper() + registry.RegisterSection(name, &probeSection{}, func(registry.Mode) any { + return probeSection{A: "from the default", B: "from the default"} + }) +} + +// TestADeclarationNamingNoSectionIsReported covers the misspelling these two calls invite. +// +// A section states its own keys and states how they are delivered from two calls side by side, and only +// the first is checked against a struct. A misspelling in the second is accepted by the compiler, names a +// section that does not exist, and leaves the section that does exist delivered the wrong way: its keys +// resolve, install into the source, and its reader never asks that source for them. +// +// Nothing else can catch it. The delivery is a claim about a reader in another package, so there is no +// second statement of it to compare against, which is why the registry answers for the name itself. +func TestADeclarationNamingNoSectionIsReported(t *testing.T) { + registry.Reset() + registerProbe(t, "mempool") + registry.DeclareDecodedNotLookedUp("memool", "decoded into tmcfg.Config") + + var named []string + for _, d := range registry.Defects() { + named = append(named, d.Section) + if !strings.Contains(d.Err.Error(), "no section of this name is registered") { + t.Errorf("the defect for %q says %q, and it has to say the name matches no section", + d.Section, d.Err) + } + } + if !reflect.DeepEqual(named, []string{"memool"}) { + t.Fatalf("the reported defects name %v, want the misspelled declaration alone. Unreported, the "+ + "section that is registered is delivered the wrong way and nothing says so", named) + } +} + +// TestADeclarationMatchingItsSectionIsNotReported is what keeps the check above from refusing every +// correct pair. +// +// The two calls are made in one package's initialisation and nothing fixes the order between them, so a +// declaration is routinely recorded before the registration it belongs to. A check at the moment of +// declaring would refuse the correct pair that happened to declare first, which is why the name is +// answered for at every read instead. +func TestADeclarationMatchingItsSectionIsNotReported(t *testing.T) { + for _, order := range []string{"registration first", "declaration first"} { + t.Run(order, func(t *testing.T) { + registry.Reset() + if order == "registration first" { + registerProbe(t, "mempool") + registry.DeclareDecodedNotLookedUp("mempool", "decoded into tmcfg.Config") + } else { + registry.DeclareDecodedNotLookedUp("mempool", "decoded into tmcfg.Config") + registerProbe(t, "mempool") + } + requireNoDefects(t) + if _, declared := registry.DecodedSections()["mempool"]; !declared { + t.Error("the section is not reported as decoded, so its values would be delivered by a " + + "lookup its reader never makes") + } + }) + } +} + +// TestADeclarationWithNoReasonIsReported holds the reason to being required. +// +// The reason names the struct the values are decoded into, which is the only thing a reader can check the +// claim against. Accepted empty, the claim that a section is delivered differently rests on nothing. +func TestADeclarationWithNoReasonIsReported(t *testing.T) { + registry.Reset() + registerProbe(t, "mempool") + registry.DeclareDecodedNotLookedUp("mempool", "") + + if len(registry.Defects()) != 1 { + t.Fatalf("declaring with no reason produced %d defects, want one", len(registry.Defects())) + } + if _, declared := registry.DecodedSections()["mempool"]; declared { + t.Error("the section was recorded as decoded despite being refused, so the refusal changed " + + "nothing about how it is delivered") + } +} + +// TestResetClearsHowSectionsAreDelivered covers the isolation the registry offers a test. +// +// Reset exists so one test's registrations cannot reach another's declared set. A delivery declaration +// left behind survives into a registry that no longer holds the section it names, which is both a defect +// the next test did not cause and, if that test registers the same name, a delivery it never declared. +func TestResetClearsHowSectionsAreDelivered(t *testing.T) { + registry.Reset() + registerProbe(t, "mempool") + registry.DeclareDecodedNotLookedUp("mempool", "decoded into tmcfg.Config") + requireNoDefects(t) + + registry.Reset() + if got := registry.DecodedSections(); len(got) != 0 { + t.Errorf("a fresh registry reports %v as delivered by a decode, and it holds no sections at "+ + "all", got) + } + if got := registry.Defects(); len(got) != 0 { + t.Errorf("a fresh registry reports %d defects, which the next test to read them did not cause", + len(got)) + } +} + +// TestOnlyASuppliedValueReachesADecodedSection is the difference between delivering a value and +// replacing an operator's file. +// +// A section read by a decode already holds what its own file said. Handing it a default would rewrite +// that on every boot for every key the operator's file does not mention, so a key that took its default +// is skipped and a key any other layer answered is delivered. +func TestOnlyASuppliedValueReachesADecodedSection(t *testing.T) { + registry.Reset() + registerProbe(t, "mempool") + registerProbe(t, "api") + registry.DeclareDecodedNotLookedUp("mempool", "decoded into tmcfg.Config") + requireNoDefects(t) + + resolved, err := registry.Resolve(registry.ModeFull, registry.Sources{ + File: map[string]any{"mempool.a": "from the file", "api.a": "from the file"}, + }) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + + got := registry.SuppliedByDecodedSection(resolved) + want := map[string]map[string]any{"mempool": {"mempool.a": "from the file"}} + if !reflect.DeepEqual(got, want) { + t.Errorf("the decoded sections are handed %v, want %v. A key nobody wrote arriving here replaces "+ + "an operator's own value, and a section delivered by a lookup arriving here is delivered "+ + "twice", got, want) + } + if _, held := got["mempool"]["mempool.b"]; held { + t.Error("mempool.b took its default and was handed to the decode anyway, which writes a value " + + "nobody chose over whatever the node's own file holds") + } +} diff --git a/config/registry/registry.go b/config/registry/registry.go index 4de8ece030..c3e85bce0b 100644 --- a/config/registry/registry.go +++ b/config/registry/registry.go @@ -265,7 +265,7 @@ func (s Section) detached() Section { // Sections returns every registered section, sorted by name. func Sections() []Section { - registered, _ := snapshot() + registered, _, _ := snapshot() return registered } @@ -277,13 +277,15 @@ func Lookup(name string) (Section, bool) { return s.detached(), ok } -// snapshot returns every registered section and every refused registration, read together. +// snapshot returns every registered section, every defect, and how each section's values reach their +// reader, read together. // -// One acquisition for both, because the two are halves of one answer: the sections are the key space and -// the refusals say which keys are missing from it. Read separately, a registration arriving between them -// is refused in one half and absent from the other, so the answer describes two registries and the report -// of what is missing does not match what is actually missing. -func snapshot() ([]Section, []Defect) { +// One acquisition for all three, because they are parts of one answer: the sections are the key space, the +// defects say which keys are missing from it, and the delivery declarations say how the rest of them get +// where they are read. Read separately, a registration arriving between two reads is described by one part +// and absent from another, so the answer describes two registries and no part of it holds against the +// others. +func snapshot() ([]Section, []Defect, map[string]string) { mu.RLock() defer mu.RUnlock() out := make([]Section, 0, len(sections)) @@ -291,14 +293,45 @@ func snapshot() ([]Section, []Defect) { out = append(out, s.detached()) } sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) - return out, append([]Defect(nil), defects...) + decoded := make(map[string]string, len(decodedNotLookedUp)) + for name, why := range decodedNotLookedUp { + decoded[name] = why + } + return out, allDefects(), decoded +} + +// allDefects returns the recorded defects and the ones derived from the registry's own state. +// +// A declaration naming a section nothing registered is derived at every read rather than recorded when the +// declaration arrives. A section registers itself and declares how it is delivered from two calls in its +// own package's initialisation, and nothing fixes the order between them, so refusing at the moment of +// declaring would refuse a correct pair that happened to declare first. +// +// The caller holds mu. +func allDefects() []Defect { + out := append([]Defect(nil), defects...) + names := make([]string, 0, len(decodedNotLookedUp)) + for name := range decodedNotLookedUp { + if _, registered := sections[name]; !registered { + names = append(names, name) + } + } + sort.Strings(names) + for _, name := range names { + out = append(out, Defect{Section: name, Err: fmt.Errorf( + "declared as decoded rather than looked up (%s) and no section of this name is registered. "+ + "Nothing delivers the section this names, and if the name is a misspelling of a section "+ + "that is registered, that section's keys install into a source its reader never asks", + decodedNotLookedUp[name])}) + } + return out } // Defects returns every registration this package could not use. func Defects() []Defect { mu.RLock() defer mu.RUnlock() - return append([]Defect(nil), defects...) + return allDefects() } // Keys returns every declared key across every section, sorted. @@ -574,11 +607,16 @@ func isLeaf(t reflect.Type) bool { // Reset clears the registry. For tests only, so one test's registrations cannot leak into // another's declared set. +// +// Every piece of registration state, not only the sections. A delivery declaration left behind names a +// section that no longer exists, which is the one thing a fresh registry is supposed to guarantee cannot +// happen. func Reset() { mu.Lock() defer mu.Unlock() sections = map[string]Section{} defects = nil + decodedNotLookedUp = map[string]string{} } // envPrefix is the environment namespace for every derived key. diff --git a/config/registry/resolve.go b/config/registry/resolve.go index 6e289f99b2..0b35277c6a 100644 --- a/config/registry/resolve.go +++ b/config/registry/resolve.go @@ -25,34 +25,49 @@ type Resolved struct { // The keys an operator has taken responsibility for, as distinct from the ones tracking the // binary's judgement. This is what a diff renders. Overrides []string - // Ignored are declared keys an environment variable was set for and could not supply, sorted. + // Ignored are declared keys an environment variable was set for and could not supply, and the reason + // for each. // - // Separate from Unknown because the two are different mistakes. An unknown key is one nothing reads. - // An ignored one is read, and the operator reached for the one channel that cannot carry it, so the - // value they wrote elsewhere is what applies. Ignored carries the keys; the reason is the same for all of them, because it is a fact about - // the channel rather than about any section. - Ignored []string - // Refused are the registrations this package could not use, so the key space is missing every key - // each of them declared. + // Separate from the undeclared keys because the two are different mistakes. An undeclared key is one + // nothing reads. An ignored one is read, and the operator reached for the one channel that cannot + // carry it, so the value they wrote elsewhere is what applies. + // + // The reason travels with the key rather than being looked up beside it. A caller reporting an ignored + // key is the only thing that can tell the operator why their variable did nothing, and a reason it has + // to fetch from somewhere else is one it can be handed empty without noticing. + Ignored map[string]string + // Refused are the registrations and delivery declarations this package could not use. + // + // A refused registration means the key space is missing every key it declared. A refused declaration + // leaves the key space whole and means a section's values do not reach the thing that reads them. // // Reported rather than an error, for the reason Defect is recorded rather than panicked: every one // of these comes from a call in this binary's own source, so a defect is a mistake a compiler could // have caught and never something an operator wrote. Refusing to resolve would turn that mistake // into a node that will not start, which is the fleet-wide incident the recording exists to avoid. // - // What to do about one is the caller's, the same division Unknown draws: a path that writes a - // configuration file has cause to refuse, because it would render a file missing whole sections, - // while a booting node has cause to say so and run. Neither is stated as a rule here because no - // caller consumes this yet: the one path that installs a resolution neither refuses on it nor - // carries it into what it reports, so a resolution over a diminished key space installs quietly - // today. A section named here is absent from Values, so an operator's written value for one of its - // keys arrives in Unknown rather than being applied. + // What to do about one is the caller's, the same division the undeclared keys draw: a path that writes + // a configuration file has cause to refuse, because it would render a file missing whole sections, + // while a booting node has cause to say so and run. + // + // A section named here is absent from Values, so an operator's written value for one of its keys is + // not applied and arrives in UnknownInFile, reading as a key nothing declares. Refused []Defect - // Unknown are keys a source carried that no section declares, sorted. + // UnknownInFile are keys the file carried that no section declares, sorted. // // Reported rather than an error, because what to do about one is the caller's decision: a // generate path may want to refuse, while a boot on an operator's existing file must not. - Unknown []string + UnknownInFile []string + // UnknownFromFlags are flag names the caller passed that match no declared key, sorted. + // + // Held apart from the file's keys rather than counted with them, because the two have different + // authors and only one of them is a mistake. A caller passing its whole flag set passes flags that + // name no setting at all, and every one of those arrives here on every invocation. A key in the file + // is something an operator typed meaning to change a setting. + // + // Counted together, a report about the file names flags the file does not contain, and the one signal + // an operator has for catching a typo fires on every run whether or not they made one. + UnknownFromFlags []string } // Sources are a node's configuration sources other than its defaults, which Resolve derives itself. @@ -118,7 +133,7 @@ func Resolve(mode Mode, from Sources) (Resolved, error) { // from the same read, because they are what says which keys the key space is missing, and a list of // missing keys taken separately from the space it describes can name a section the space has or omit // one it lacks. - registered, refused := snapshot() + registered, refused, _ := snapshot() out.Refused = refused defaults, err := defaultValues(mode, registered) @@ -137,21 +152,18 @@ func Resolve(mode Mode, from Sources) (Resolved, error) { } overrides := map[string]bool{} - unknown := map[string]bool{} - // Lowest precedence first, so a later source overwrites an earlier one. The one statement of the - // order, which is why nothing exports it. + unknownInFile := map[string]bool{} + unknownFromFlags := map[string]bool{} + fromEnv, ignored := envValues(declared, undeliverable, from.LookupEnv) out.Ignored = ignored - for _, values := range []map[string]any{ - fileValues(from.File), - fromEnv, - from.Flags, - } { + + // resolveFrom writes one source's values over what is resolved so far, and collects the keys it + // carried that no section declares. Dropping one silently is how an operator's typo becomes invisible. + resolveFrom := func(values map[string]any, undeclared map[string]bool) { for key, v := range values { if !declared[key] { - // A key nothing declares cannot be resolved into anything, and silently dropping it is - // how an operator's typo becomes invisible. - unknown[key] = true + undeclared[key] = true continue } out.Values[key] = v @@ -159,8 +171,20 @@ func Resolve(mode Mode, from Sources) (Resolved, error) { } } + // Lowest precedence first, so a later source overwrites an earlier one. The one statement of the + // order, which is why nothing exports it. + // + // The file's undeclared keys and the flags' are kept apart, because what a caller tells an operator + // about one is not what it tells them about the other. The environment needs no set of its own: a + // variable is found only by asking for a declared key's own name, so every value it answers with is + // declared, and the set handed here stays empty. + resolveFrom(fileValues(from.File), unknownInFile) + resolveFrom(fromEnv, map[string]bool{}) + resolveFrom(from.Flags, unknownFromFlags) + out.Overrides = sortedKeys(overrides) - out.Unknown = sortedKeys(unknown) + out.UnknownInFile = sortedKeys(unknownInFile) + out.UnknownFromFlags = sortedKeys(unknownFromFlags) return out, nil } @@ -450,12 +474,12 @@ func isSingleValue(k reflect.Kind) bool { } func envValues(declared map[string]bool, undeliverable map[string]string, - lookup func(string) (string, bool)) (map[string]any, []string) { + lookup func(string) (string, bool)) (map[string]any, map[string]string) { if lookup == nil { return nil, nil } out := map[string]any{} - var ignored []string + ignored := map[string]string{} for key := range declared { // A key no variable can carry is left to the sources that can. Resolving it would put a string // at the top of the order for a reader that takes the exact type, and installing that stops the @@ -464,9 +488,9 @@ func envValues(declared map[string]bool, undeliverable map[string]string, // The variable is still read, and the value still discarded. Asking is what turns this from a // silent skip into something a caller can report: a reason nothing can attach to an operator's // own action is a reason nobody is ever told. - if _, refused := undeliverable[key]; refused { + if reason, refused := undeliverable[key]; refused { if v, set := lookup(EnvName(key)); set && v != "" { - ignored = append(ignored, key) + ignored[key] = reason } continue } @@ -478,7 +502,9 @@ func envValues(declared map[string]bool, undeliverable map[string]string, out[key] = v } } - sort.Strings(ignored) + if len(ignored) == 0 { + return out, nil + } return out, ignored } diff --git a/config/registry/rootkeys_test.go b/config/registry/rootkeys_test.go index b3b154bc9d..27e2de8b16 100644 --- a/config/registry/rootkeys_test.go +++ b/config/registry/rootkeys_test.go @@ -2,7 +2,6 @@ package registry_test import ( "reflect" - "slices" "sort" "strings" "testing" @@ -205,9 +204,14 @@ func TestAVariableSetForARefusedKeyIsReported(t *testing.T) { t.Fatalf("Resolve: %v", err) } - if got := strings.Join(resolved.Ignored, ","); got != "probe.rows" { - t.Errorf("the ignored variables are %q, want probe.rows. An operator set it and nothing here can "+ - "tell them it did nothing", got) + reason, ignored := resolved.Ignored["probe.rows"] + if !ignored || len(resolved.Ignored) != 1 { + t.Errorf("the ignored variables are %v, want probe.rows alone. An operator set it and nothing here "+ + "can tell them it did nothing", resolved.Ignored) + } + if reason == "" { + t.Error("probe.rows is ignored and carries no reason. The reason is what tells an operator which " + + "channel to reach for instead, and a caller reporting the key cannot invent one") } if !reflect.DeepEqual(resolved.Values["probe.rows"], []any{}) { t.Errorf("probe.rows resolved to %#v, and the channel was supposed to be skipped", @@ -279,12 +283,12 @@ func TestTheEnvironmentCarriesAListOfWordsAndNotAListOfLists(t *testing.T) { "string and this setting is a list of unconstrained elements, so the string would reach a "+ "reader that asked for rows", got) } - if !slices.Contains(resolved.Ignored, "shapes.rows") { - t.Errorf("Ignored is %v and does not name shapes.rows. A variable was set for it and did "+ - "nothing, and an operator has to be told that", resolved.Ignored) + if reason := resolved.Ignored["shapes.rows"]; reason == "" { + t.Errorf("Ignored is %v and carries no reason for shapes.rows. A variable was set for it and did "+ + "nothing, and an operator has to be told that and why", resolved.Ignored) } for _, key := range []string{"shapes.one", "shapes.words"} { - if slices.Contains(resolved.Ignored, key) { + if _, ignored := resolved.Ignored[key]; ignored { t.Errorf("%s is reported as ignored and its variable answered", key) } } diff --git a/config/registry/spec_test.go b/config/registry/spec_test.go index 594f14f07c..7fe86cdbbd 100644 --- a/config/registry/spec_test.go +++ b/config/registry/spec_test.go @@ -477,9 +477,9 @@ func TestAFileKeyIsMatchedRegardlessOfCase(t *testing.T) { if err != nil { t.Fatalf("Resolve: %v", err) } - if len(got.Unknown) != 0 { + if len(got.UnknownInFile) != 0 { t.Errorf("the file's key was reported unknown %v; an operator whose only mistake was case would "+ - "be told their key does not exist", got.Unknown) + "be told their key does not exist", got.UnknownInFile) } if got.Values["giga_executor.occ_enabled"] != "written" { t.Errorf("the key resolved to %#v, want the file's value", got.Values["giga_executor.occ_enabled"]) @@ -504,9 +504,9 @@ func TestAKeyNoSectionDeclaresIsReportedNotDropped(t *testing.T) { t.Fatalf("Resolve: %v", err) } - if !reflect.DeepEqual(got.Unknown, []string{"giga_executor.typo"}) { - t.Errorf("Unknown is %v, want the one undeclared key. Dropped silently, an operator's typo is "+ - "invisible and their intended value never applies", got.Unknown) + if !reflect.DeepEqual(got.UnknownInFile, []string{"giga_executor.typo"}) { + t.Errorf("UnknownInFile is %v, want the one undeclared key. Dropped silently, an operator's typo "+ + "is invisible and their intended value never applies", got.UnknownInFile) } if _, ok := got.Values["giga_executor.typo"]; ok { t.Error("the undeclared key resolved anyway, so it would reach a consumer that cannot use it") @@ -548,8 +548,10 @@ func TestTheEnvironmentIsReadByTheDeclaredSet(t *testing.T) { if !reflect.DeepEqual(asked, want) { t.Errorf("the environment was asked for %v, want the declared spellings %v", asked, want) } - if len(got.Unknown) != 0 { - t.Errorf("the environment produced unknown keys %v", got.Unknown) + if len(got.UnknownInFile) != 0 || len(got.UnknownFromFlags) != 0 { + t.Errorf("the environment produced undeclared keys, in the file's set %v and the flags' set %v. "+ + "It is asked only for names derived from declared keys, so it can produce neither", + got.UnknownInFile, got.UnknownFromFlags) } if !reflect.DeepEqual(got.Overrides, []string{"giga_executor.occ_enabled"}) { t.Errorf("Overrides is %v, want the one declared key the environment supplied", got.Overrides) @@ -886,24 +888,29 @@ func TestRegistrationAndResolutionAreConcurrencySafe(t *testing.T) { if _, ok := res.Values["first.a"]; !ok { t.Fatalf("round %d: first.a was registered before the call and did not resolve", round) } - if len(res.Unknown) != 0 { - t.Fatalf("round %d: no source was passed and %v is reported unknown", round, res.Unknown) + if len(res.UnknownInFile) != 0 || len(res.UnknownFromFlags) != 0 { + t.Fatalf("round %d: no source was passed and %v / %v are reported undeclared", + round, res.UnknownInFile, res.UnknownFromFlags) } } } -// TestNoUnknownKeyIsOneTheRegistryDeclares holds every source against one snapshot of the registry. +// TestNoUndeclaredKeyIsOneTheSameAnswerResolved holds a resolution to being consistent with itself. // -// Resolve derives the declared set once and checks every source against it. A source built from a -// second read of the registry can carry a key the first read did not hold, and that key comes back -// reported as one no section declares. An operator whose environment variable is real would be told it -// matches nothing, and their value would be dropped. +// Resolve derives the declared set once and checks every source against that one set. What that buys is +// an answer whose halves agree: a key it reports as one nothing declares is not a key it also resolved a +// value for. An operator told their key matches nothing, for a key the same answer applied, has no way to +// find out which half is true. +// +// Held against the answer rather than against the registry, because a section that registers after the +// snapshot is declared by a later read and was never part of this resolution. Comparing the two would +// report that as a defect when it is the snapshot doing its job. // // The trigger is a section registering while Resolve runs, which is deterministic here rather than // raced: Resolve calls each section's Defaults between reading the registry and reading the // environment, and Defaults is caller-supplied code. A goroutine cannot be relied on to land in that // window, so a concurrent version of this test passes whether the defect is present or not. -func TestNoUnknownKeyIsOneTheRegistryDeclares(t *testing.T) { +func TestNoUndeclaredKeyIsOneTheSameAnswerResolved(t *testing.T) { type one struct { A string `mapstructure:"a"` } @@ -924,7 +931,10 @@ func TestNoUnknownKeyIsOneTheRegistryDeclares(t *testing.T) { registry.EnvName("first.a"): "from the environment", registry.EnvName("second.a"): "from the environment", } + // A file as well as the environment. The environment is asked only for names the snapshot declared, so + // it cannot carry the late section's key; a file is read as written and can. res, err := registry.Resolve(registry.ModeFull, registry.Sources{ + File: map[string]any{"first.a": "from the file", "second.a": "from the file"}, LookupEnv: func(name string) (string, bool) { v, ok := env[name] return v, ok @@ -941,10 +951,16 @@ func TestNoUnknownKeyIsOneTheRegistryDeclares(t *testing.T) { if !declared["second.a"] { t.Fatal("the late section did not register, so this test cannot see the defect it exists for") } - for _, key := range res.Unknown { - if declared[key] { - t.Errorf("%q is reported as a key no section declares, and the registry declares it. An "+ - "operator setting it would be told it matches nothing and their value would be dropped", key) + if !slices.Contains(res.UnknownInFile, "second.a") { + t.Fatalf("the file wrote second.a for a section that registered after the snapshot and the "+ + "undeclared keys are %v. Without it reported there is nothing here to be consistent with", + res.UnknownInFile) + } + + for _, key := range append(append([]string(nil), res.UnknownInFile...), res.UnknownFromFlags...) { + if _, resolved := res.Values[key]; resolved { + t.Errorf("%q is reported as a key no section declares and the same answer resolved a value "+ + "for it. One of the two is wrong and an operator is told whichever they read first", key) } } } @@ -1570,3 +1586,40 @@ func TestANestedFileSourceIsRefusedRatherThanResolvedPast(t *testing.T) { t.Errorf("Overrides is %v, so an install would skip the one key the file wrote", resolved.Overrides) } } + +// TestAFlagNamingNoKeyIsNotReportedAsTheFilesMistake separates two sources of the same shape. +// +// A caller passes its whole flag set, because a flag is a channel an operator uses and leaving it out +// installs a lower layer over the top of what they chose. Most of those flags name no setting at all: +// --home says where the files are, --trace says how errors print. They arrive here exactly as a mistyped +// key in the file does, and only one of the two is a mistake. +// +// Counted together, the report an operator reads about their file names flags their file does not contain, +// and it says so on every invocation whether or not they typed anything wrong. That is the one signal +// there is for a typo, and a signal that fires every time carries nothing. +func TestAFlagNamingNoKeyIsNotReportedAsTheFilesMistake(t *testing.T) { + type one struct { + A string `mapstructure:"a"` + } + registry.Reset() + registry.RegisterSection("probe", &one{}, func(registry.Mode) any { return one{A: "from the default"} }) + requireNoDefects(t) + + got, err := registry.Resolve(registry.ModeFull, registry.Sources{ + File: map[string]any{"probe.a": "from the file", "probe.typo": "from the file"}, + Flags: map[string]any{"home": "/var/lib/sei", "trace": "true"}, + }) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + + if !reflect.DeepEqual(got.UnknownInFile, []string{"probe.typo"}) { + t.Errorf("the file's undeclared keys are %v, want the typo alone. A flag counted here puts a "+ + "warning about the file on every boot and buries the typo it exists to surface", + got.UnknownInFile) + } + if !reflect.DeepEqual(got.UnknownFromFlags, []string{"home", "trace"}) { + t.Errorf("the flags matching no key are %v, want home and trace. A caller that cannot tell them "+ + "from the file's keys has to either report both or neither", got.UnknownFromFlags) + } +} diff --git a/config/seitoml/file.go b/config/seitoml/file.go index 9868738654..c31b2ee9a2 100644 --- a/config/seitoml/file.go +++ b/config/seitoml/file.go @@ -106,7 +106,7 @@ func (f *File) refuseUnsupportedShapes() error { "holds one value, so a repeated section has no reading", s.Name) } if err := keyIsAddressable(s.Name); err != nil { - return fmt.Errorf("table [%s]: %w", s.Name, err) + return fmt.Errorf("table [%s]: %w", shortKey(s.Name), err) } name := s.Name.String() if headings[name] { @@ -166,6 +166,11 @@ func (f *File) refuseUnsupportedShapes() error { // in the file, and a segment carrying a dot or a space cannot be split back into the segments it came // from. func keyIsAddressable(key parser.Key) error { + if len(key) > maxKeyDepth { + return fmt.Errorf("%s is %d segments deep and this file is read to %d. A setting here is a "+ + "section and a key inside it, so nothing legitimate reaches that depth", shortKey(key), + len(key), maxKeyDepth) + } for _, segment := range key { if segment == "" { return fmt.Errorf("%s has an empty segment, which names nothing", key) @@ -204,6 +209,19 @@ func notBareKeyRune(r rune) bool { // space a table's do, so a caller works in that space and an edit there defines the table a second // time, producing a file a conforming reader refuses to load. func valueIsAddressable(key parser.Key, v parser.Value) error { + return valueIsAddressableWithin(key, v, 0) +} + +// valueIsAddressableWithin is valueIsAddressable, carrying how deep into nested arrays it already is. +// +// The depth is carried rather than derived because the walk is what finds it: an array holds arrays, and +// the cost of reading one grows faster than the bytes that describe it, so a small file can nest deeply +// enough to exhaust the process. Nothing downstream can refuse a boot, so the refusal happens here. +func valueIsAddressableWithin(key parser.Key, v parser.Value, depth int) error { + if depth > maxArrayDepth { + return fmt.Errorf("%s nests arrays %d deep and this file is read to %d. No setting here is a "+ + "list of lists, so nothing legitimate reaches that depth", key, depth, maxArrayDepth) + } switch x := v.X.(type) { case parser.Token: switch x.Type { @@ -221,7 +239,7 @@ func valueIsAddressable(key parser.Key, v parser.Value) error { if !ok { continue } - if err := valueIsAddressable(key, element); err != nil { + if err := valueIsAddressableWithin(key, element, depth+1); err != nil { return err } } @@ -229,11 +247,46 @@ func valueIsAddressable(key parser.Key, v parser.Value) error { return nil } +// The bounds this file is read within. +// +// None of them is a limit an operator can reach by writing configuration. They exist because nothing +// downstream of reading can refuse a boot, so a file whose cost grows faster than its size has to be +// refused before it is parsed rather than after it has taken the memory. +const ( + // maxFileBytes bounds what Load reads. A file stating every declared key is a few tens of kilobytes. + maxFileBytes = 1 << 20 + // maxKeyDepth bounds the segments in one key. A setting is a section and a key inside it. + maxKeyDepth = 8 + // maxArrayDepth bounds nesting inside a value. No setting here is a list of lists. + maxArrayDepth = 8 +) + +// shortKey renders a key for a message, bounded. +// +// The message that refuses a key for being too deep is the one place that key is certain to be rendered, +// and rendering it whole makes the refusal as large as the file. Bounded here rather than at each message, +// because the caller holding the key is the one that cannot know how deep it is. +func shortKey(key parser.Key) string { + if len(key) <= maxKeyDepth { + return key.String() + } + return fmt.Sprintf("%s and %d more segments", key[:maxKeyDepth].String(), len(key)-maxKeyDepth) +} + // Load reads the document at path. // // A path with no file there reports fs.ErrNotExist, which errors.Is matches. That is the one outcome a // caller acts on rather than reports, since a node with no sei.toml yet needs New instead. func Load(path string) (*File, error) { + info, err := os.Stat(path) + if err != nil { + return nil, err + } + if info.Size() > maxFileBytes { + return nil, fmt.Errorf("%s holds %d bytes and this file is read up to %d. A file stating every "+ + "key this binary declares is a small fraction of that, so one this large is not that file", + path, info.Size(), maxFileBytes) + } raw, err := os.ReadFile(path) //nolint:gosec // the caller's configured path is the subject if err != nil { return nil, err diff --git a/config/seitoml/guards_test.go b/config/seitoml/guards_test.go index 7b308ba66e..4ea1856d81 100644 --- a/config/seitoml/guards_test.go +++ b/config/seitoml/guards_test.go @@ -1,6 +1,8 @@ package seitoml import ( + "os" + "path/filepath" "reflect" "strings" "testing" @@ -119,3 +121,61 @@ func TestAReadReusesItsDecodeAndNeverAStaleOne(t *testing.T) { }) } } + +// TestAFileWhoseCostOutgrowsItsSizeIsRefusedBeforeItIsRead covers the one refusal that has to happen at +// the door. +// +// Nothing downstream of reading this file can refuse a boot, which is the promise the whole surface rests +// on. A file whose cost grows faster than the bytes describing it breaks that promise from outside: it is +// not refused, it exhausts the process, and a recover cannot catch a kernel kill. So the cost is bounded +// here, before the bytes are parsed, and the bound is a refusal an operator is told about. +// +// The three shapes are the ones that grow: many segments in one heading, arrays inside arrays, and a file +// that is simply enormous. Each is written far past its bound so a change that loosens one of them fails +// rather than merely slowing down. +func TestAFileWhoseCostOutgrowsItsSizeIsRefusedBeforeItIsRead(t *testing.T) { + const header = "schema_version = 1\nnode_mode = \"validator\"\n" + for _, tc := range []struct { + name string + body string + says string + }{ + { + name: "one heading of many segments", + body: "[" + strings.Repeat("a.", maxKeyDepth+4) + "a]\nx = 1\n", + says: "segments deep", + }, + { + name: "arrays inside arrays", + body: "x = " + strings.Repeat("[", maxArrayDepth+4) + strings.Repeat("]", maxArrayDepth+4) + "\n", + says: "nests arrays", + }, + { + name: "more bytes than this file is read to", + body: strings.Repeat("# padding\n", maxFileBytes/8), + says: "read up to", + }, + } { + t.Run(tc.name, func(t *testing.T) { + path := filepath.Join(t.TempDir(), "sei.toml") + if err := os.WriteFile(path, []byte(header+tc.body), 0o600); err != nil { + t.Fatalf("write the probe file: %v", err) + } + _, err := Load(path) + if err == nil { + t.Fatal("the file was accepted, so its cost reaches the node rather than being refused") + } + if !strings.Contains(err.Error(), tc.says) { + t.Errorf("the refusal says %q and has to say %q, which is what tells an operator what "+ + "about their file was refused", err, tc.says) + } + // The message is the one place an over-large key is certain to be rendered, so rendering it + // whole makes the refusal as large as the file it refused. + if len(err.Error()) > 4096 { + t.Errorf("the refusal is %d bytes long. It reaches a log line and an operator's terminal, "+ + "so a message that grows with the file is the same problem in another place", + len(err.Error())) + } + }) + } +} diff --git a/config/tendermintbase/tendermintbase.go b/config/tendermintbase/tendermintbase.go index 956e054180..59f1bca696 100644 --- a/config/tendermintbase/tendermintbase.go +++ b/config/tendermintbase/tendermintbase.go @@ -137,6 +137,15 @@ func init() { selfRemediationDefaults, reachesNoReactor) declareRootKeys(RootSectionName, &nodeRootSchema{}, rootDefaults, append(append([]string{}, notWritableInThisFile...), removedFromTheNode...)...) + + // Every section here reaches its reader by a decode rather than a lookup, so the boot has to deliver + // them a second way. Walked over what the registrations recorded rather than a list beside them, so a + // section registered above cannot be left undelivered. + for _, name := range registeredHere { + registry.DeclareDecodedNotLookedUp(name, + "decoded into the node's own configuration struct by the boot's handler, which reads that "+ + "file once; nothing looks these keys up afterwards") + } } // registeredHere are the sections this package put in the registry, recorded as each one is registered.