From cc90d59fb93481e74c8b74ea2fa19654c77f7e05 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 27 Aug 2026 12:54:04 -0700 Subject: [PATCH 1/2] feat(config): install what sei.toml supplies for the keys a reader looks up A node's app.toml settings are read one key at a time, when the thing that wants them asks, and that happens after the boot has built the source they are read from. So a value can be put into that source and the later lookup finds it. This installs the declared keys an operator's sei.toml supplied, and nothing else. Only what a source supplied, not the whole resolution. Resolve answers for every declared key, so installing all of it would write this binary's own defaults over an operator's file for every key they did not mention. The keys of a section whose reader decodes its file whole are skipped, because putting a value into the source is no delivery at all for those: their file is read into a struct before this runs and nothing consults the source for them afterwards. They are marked here as needing that second delivery, which is a separate change. Nothing here can stop a node starting, and the guard that makes that true is new. 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, and letting it escape would refuse a boot for the one reason this path promises never to refuse one. An unreadable sei.toml is no longer reported as an absent one. A node with no such file is every node today, so that stays quiet; a node whose file will not parse, or records a schema this binary does not know, or names no node kind, is a node where somebody wrote the file and it is doing nothing. Collapsing the two meant the only signal an operator had for their mistake was the one that got collapsed. Verified by mutation: collapsing every read failure back to absent fails the new distinction, and removing the guard lets a panic escape the install path. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/seid/cmd/boot_install_test.go | 267 ++++++++++++++ cmd/seid/cmd/configmanager/configmanager.go | 43 ++- cmd/seid/cmd/configmanager/install.go | 330 ++++++++++++++++++ .../cmd/configmanager/install_read_test.go | 79 +++++ config/registry/delivery.go | 113 ++++++ config/registry/environment.go | 46 +++ config/tendermintbase/tendermintbase.go | 9 + 7 files changed, 886 insertions(+), 1 deletion(-) create mode 100644 cmd/seid/cmd/boot_install_test.go create mode 100644 cmd/seid/cmd/configmanager/install.go create mode 100644 cmd/seid/cmd/configmanager/install_read_test.go create mode 100644 config/registry/delivery.go create mode 100644 config/registry/environment.go 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..cd1b62376b 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -21,6 +21,13 @@ import ( var logger = seilog.NewLogger("cmd", "seid", "configmanager") +// loggerName is the name the logger above is registered under, and ownReportingFloor is the level its +// reports are held at. +const ( + loggerName = "cmd/seid/configmanager" + ownReportingFloor = slog.LevelInfo +) + // EnvVar gates which configuration manager seid uses. const EnvVar = "SEI_CONFIG_MANAGER" @@ -55,6 +62,27 @@ type SeiConfigManager struct { } // log returns the logger to report through, and never returns nil. +// 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() { + if seilog.SetLevel(loggerName, ownReportingFloor) == 0 { + // Nothing to hold, which happens when a caller supplied a logger of its own. + return + } +} + func (m SeiConfigManager) log() *slog.Logger { if m.logger != nil { return m.logger @@ -80,10 +108,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. diff --git a/cmd/seid/cmd/configmanager/install.go b/cmd/seid/cmd/configmanager/install.go new file mode 100644 index 0000000000..6c1e9267f0 --- /dev/null +++ b/cmd/seid/cmd/configmanager/install.go @@ -0,0 +1,330 @@ +package configmanager + +import ( + "context" + "errors" + "io/fs" + "log/slog" + "os" + "path/filepath" + "runtime/debug" + "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, which nothing else imports. 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, which nothing else imports + // either. These are the sections the delivery beside this one decodes rather than 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 can stop a node starting. A node with no sei.toml, an unreadable one, or one recording a +// mode this binary does not know 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. +func installResolved(cmd *cobra.Command, typed map[string]string, log *slog.Logger) { + // The claim above is only true with this. What follows walks the node's own configuration types by + // reflection and decodes through two libraries, so a panic here is a shape nobody predicted rather + // than a value an operator wrote, and letting it escape would refuse the boot for the one reason this + // path promises never to refuse it. 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 + } + + // Every channel an operator can use. Omitting one installs a lower layer over the top of what they + // 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. + resolved, err := registry.Resolve(registry.Mode(mode), registry.Sources{ + File: written, + LookupEnv: os.LookupEnv, + Flags: flagValues(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 + } + log.Info("configuration installed", "mode", mode, + "installed", strings.Join(report.Installed, ",")) +} + +// 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 no section declares is one this file cannot +// deliver, so it reads as a setting and changes nothing. And a variable set for a key no environment +// variable can carry is ignored on purpose, with the reason recorded where the key is declared. +// +// Reported once each rather than per key, because a node resolves over a hundred declared keys and a line +// each would bury the two or three that matter in the noise it creates. +func reportWhatTheFileDidNotReach(resolved registry.Resolved, log *slog.Logger) { + if len(resolved.Unknown) > 0 { + log.Warn("sei.toml writes keys no section declares; they have no effect", + "count", len(resolved.Unknown), "keys", strings.Join(resolved.Unknown, ",")) + } + if len(resolved.Ignored) == 0 { + return + } + cannot := registry.EnvCannotDeliver() + for _, key := range 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", cannot[key]) + } +} + +// 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 that has not generated one is the expected state while sections are still moving, so that is not +// a warning. A file that exists and will not parse is, because somebody wrote it and it is not doing what +// they think. +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): + // The common case, and not a mistake: a node with no sei.toml is every node today. + 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..ecbbe6d856 --- /dev/null +++ b/cmd/seid/cmd/configmanager/install_read_test.go @@ -0,0 +1,79 @@ +package configmanager + +import ( + "errors" + "io" + "io/fs" + "log/slog" + "os" + "path/filepath" + "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) + } + }() + // A command with no server context is the cheapest way in: the install reads one out of the command + // and the path under test is whatever it does with what it finds. + installResolved(nil, map[string]string{}, slog.New(slog.NewTextHandler(io.Discard, nil))) +} diff --git a/config/registry/delivery.go b/config/registry/delivery.go new file mode 100644 index 0000000000..5967511b62 --- /dev/null +++ b/config/registry/delivery.go @@ -0,0 +1,113 @@ +package registry + +import ( + "fmt" + "sort" +) + +// 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. +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 { + owning := DecodedSections() + + 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 Sections() { + 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 +} + +// UndeliveredSections returns the registered sections that named no delivery, sorted. +// +// A section reaches its reader one of two ways and the registry cannot tell which, so the answer is +// declared. A section that declares nothing is treated as read by a lookup, which is right for almost all +// of them and silently wrong for the rest: its keys resolve, install into the source, and change nothing +// the node runs. That is the failure this package exists to remove, so the set is reported and a caller +// linking every section holds it to what it expects. +func UndeliveredSections(expectDecoded map[string]bool) []string { + var out []string + for _, section := range Sections() { + if expectDecoded[section.Name] != DecodedNotLookedUp(section.Name) { + out = append(out, section.Name) + } + } + sort.Strings(out) + return out +} + +// DecodedNotLookedUp reports whether a section's values reach their reader by a decode. +func DecodedNotLookedUp(section string) bool { + mu.RLock() + defer mu.RUnlock() + _, ok := decodedNotLookedUp[section] + return ok +} diff --git a/config/registry/environment.go b/config/registry/environment.go new file mode 100644 index 0000000000..d3f440b178 --- /dev/null +++ b/config/registry/environment.go @@ -0,0 +1,46 @@ +package registry + +import "fmt" + +// envCannotDeliver holds the keys an environment variable cannot supply, with the reason. +var envCannotDeliver = map[string]string{} + +// RefuseFromEnvironment records that an environment variable cannot supply a key. +// +// An environment carries one string per name. Most readers cast that string into whatever the setting +// needs, so the environment works for them. A reader that takes its value's exact type instead cannot be +// handed a string at all, and no spelling of the variable would satisfy it. +// +// Resolving such a key from the environment puts an unusable value at the top of the order, and installing +// it stops the node. Leaving the channel out means the file's value applies and the node runs. That is +// deliberately not what the machinery this replaces does, which resolves the variable and refuses to +// start, so the difference is recorded rather than assumed. A value silently doing nothing is the failure +// this whole surface exists to remove, which is why the reason is required and not optional. +// +// section is the section that declares the key, so a refused key is attributable to a registration the +// way every other defect is. Whether the key is one that section declares is answered when something +// resolves, because a refusal may be recorded before the registration it belongs to. +// +// Called from the owning package, beside its registration, so the reason sits with the code that knows it. +func RefuseFromEnvironment(section, key, reason string) { + mu.Lock() + defer mu.Unlock() + if reason == "" { + defects = append(defects, Defect{Section: section, Err: fmt.Errorf( + "refusing %q from the environment with no reason; an operator whose variable is ignored has "+ + "to be told why", key)}) + return + } + envCannotDeliver[key] = reason +} + +// EnvCannotDeliver returns the keys an environment variable cannot supply, and why. +func EnvCannotDeliver() map[string]string { + mu.RLock() + defer mu.RUnlock() + out := make(map[string]string, len(envCannotDeliver)) + for key, reason := range envCannotDeliver { + out[key] = reason + } + return out +} 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. From 0becc6b89fa850021544b31245d7658363e1838c Mon Sep 17 00:00:00 2001 From: bdchatham Date: Thu, 27 Aug 2026 14:05:11 -0700 Subject: [PATCH 2/2] fix(config): name the source of an undeclared key, and bound what a file can cost An operator was told their sei.toml wrote keys it does not contain. Every unmatched flag name reached the resolution under its own name, and the resolution merged the file's undeclared keys with the flags' before reporting them, so `--home` and `--trace` were reported as the file's mistakes on every boot. That is the only signal there is for a mistyped key, and it fired whether or not one was typed. Resolved now carries UnknownInFile and UnknownFromFlags apart, and the install reports only the file's own. The environment needs no set of its own: it is asked only for names derived from declared keys, so it cannot carry one that is not declared. Ignored carries its reason. The resolver already built a reason per key and threw it away, so the one warning an operator gets for an ignored variable told them nothing about why. The reason now travels with the key, and a parallel mechanism for recording the same fact per section is deleted: it had no callers, nothing read what it recorded, and the package already states that the reason belongs to the channel rather than to any section. A misspelled delivery declaration is reported. A section states its keys and how they are delivered from two calls side by side, and only the first was checked, so `memool` beside a section registered as `mempool` left the real section's keys installed into a source its reader never asks. Derived at every read, because nothing fixes the order of the two calls. One acquisition where there were two. The sections and the delivery declarations are halves of one answer, and read separately a section arriving between them is described by one half and absent from the other. Reset clears the declarations too, so a fresh registry cannot hold a declaration naming a section it does not have. A file is read within a bound. A 200 KB sei.toml of one deep heading cost 7 GB and a 400 KB one killed the process, on every restart, and a recover cannot catch that. Size, key depth and array nesting are now bounded before the bytes are parsed, and the refusal that names an over-deep key no longer renders the whole key: that message was 400 KB for a 200 KB key. The unbounded reports are bounded, the install names the keys a node reads here for the first time, and the package documentation describes the delivery that exists. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/seid/cmd/configmanager/configmanager.go | 55 ++++--- .../cmd/configmanager/configmanager_test.go | 10 +- cmd/seid/cmd/configmanager/doc.go | 60 ++++--- cmd/seid/cmd/configmanager/install.go | 114 ++++++++----- .../cmd/configmanager/install_read_test.go | 20 ++- config/cosmosbase/cosmosbase_test.go | 12 +- config/registry/delivery.go | 41 ++--- config/registry/delivery_test.go | 151 ++++++++++++++++++ config/registry/environment.go | 46 ------ config/registry/registry.go | 56 +++++-- config/registry/resolve.go | 96 +++++++---- config/registry/rootkeys_test.go | 20 ++- config/registry/spec_test.go | 91 ++++++++--- config/seitoml/file.go | 57 ++++++- config/seitoml/guards_test.go | 60 +++++++ 15 files changed, 639 insertions(+), 250 deletions(-) create mode 100644 config/registry/delivery_test.go delete mode 100644 config/registry/environment.go diff --git a/cmd/seid/cmd/configmanager/configmanager.go b/cmd/seid/cmd/configmanager/configmanager.go index cd1b62376b..d3ee9cba29 100644 --- a/cmd/seid/cmd/configmanager/configmanager.go +++ b/cmd/seid/cmd/configmanager/configmanager.go @@ -19,14 +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"} -// loggerName is the name the logger above is registered under, and ownReportingFloor is the level its -// reports are held at. -const ( - loggerName = "cmd/seid/configmanager" - ownReportingFloor = slog.LevelInfo -) +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" @@ -61,7 +67,6 @@ type SeiConfigManager struct { logger *slog.Logger } -// log returns the logger to report through, and never returns nil. // 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 @@ -77,12 +82,16 @@ type SeiConfigManager struct { // 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 { - // Nothing to hold, which happens when a caller supplied a logger of its own. - return + 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 { return m.logger @@ -268,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) { @@ -306,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. @@ -314,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 index 6c1e9267f0..d411e4a91f 100644 --- a/cmd/seid/cmd/configmanager/install.go +++ b/cmd/seid/cmd/configmanager/install.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "runtime/debug" + "sort" "strings" "github.com/spf13/cobra" @@ -18,14 +19,13 @@ import ( "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, which nothing else imports. 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. + // 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, which nothing else imports - // either. These are the sections the delivery beside this one decodes rather than installs. + // 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" ) @@ -34,16 +34,19 @@ const seiTomlName = "sei.toml" // installResolved puts the values sei.toml supplies into the source the boot has just built. // -// Nothing here can stop a node starting. A node with no sei.toml, an unreadable one, or one recording a -// mode this binary does not know 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. +// 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) { - // The claim above is only true with this. What follows walks the node's own configuration types by - // reflection and decodes through two libraries, so a panic here is a shape nobody predicted rather - // than a value an operator wrote, and letting it escape would refuse the boot for the one reason this - // path promises never to refuse it. The nested recover keeps a panic from the logging itself inside, - // the way the advisory reporter does. + // 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() }() @@ -72,15 +75,7 @@ func installResolved(cmd *cobra.Command, typed map[string]string, log *slog.Logg return } - // Every channel an operator can use. Omitting one installs a lower layer over the top of what they - // 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. - resolved, err := registry.Resolve(registry.Mode(mode), registry.Sources{ - File: written, - LookupEnv: os.LookupEnv, - Flags: flagValues(typed), - }) + 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) @@ -103,8 +98,30 @@ func installResolved(cmd *cobra.Command, typed map[string]string, log *slog.Logg "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, - "installed", strings.Join(report.Installed, ",")) + "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 @@ -145,28 +162,41 @@ func onlyWhatALookupSourceSupplied(resolved registry.Resolved) registry.Resolved // reportWhatTheFileDidNotReach says what an operator asked for that had no effect. // -// Two things, and neither is visible anywhere else. A key no section declares is one this file cannot -// deliver, so it reads as a setting and changes nothing. And a variable set for a key no environment -// variable can carry is ignored on purpose, with the reason recorded where the key is declared. +// 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. // -// Reported once each rather than per key, because a node resolves over a hundred declared keys and a line -// each would bury the two or three that matter in the noise it creates. +// 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.Unknown) > 0 { + 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.Unknown), "keys", strings.Join(resolved.Unknown, ",")) - } - if len(resolved.Ignored) == 0 { - return + "count", len(resolved.UnknownInFile), "keys", strings.Join(shown, ","), "omitted", omitted) } - cannot := registry.EnvCannotDeliver() - for _, key := range resolved.Ignored { + // 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", cannot[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 @@ -216,9 +246,9 @@ func OwnReportingEnabledForTest() bool { // readSeiToml loads the node's sei.toml, reporting the ordinary absence quietly. // -// A node that has not generated one is the expected state while sections are still moving, so that is not -// a warning. A file that exists and will not parse is, because somebody wrote it and it is not doing what -// they think. +// 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 { @@ -228,7 +258,7 @@ func readSeiToml(cmd *cobra.Command, log *slog.Logger) (*seitoml.File, bool) { file, err := readSeiTomlAt(home) switch { case errors.Is(err, fs.ErrNotExist): - // The common case, and not a mistake: a node with no sei.toml is every node today. + // 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: diff --git a/cmd/seid/cmd/configmanager/install_read_test.go b/cmd/seid/cmd/configmanager/install_read_test.go index ecbbe6d856..160ae56a9c 100644 --- a/cmd/seid/cmd/configmanager/install_read_test.go +++ b/cmd/seid/cmd/configmanager/install_read_test.go @@ -1,12 +1,13 @@ package configmanager import ( + "bytes" "errors" - "io" "io/fs" "log/slog" "os" "path/filepath" + "strings" "testing" ) @@ -73,7 +74,18 @@ func TestAPanicWhileInstallingDoesNotRefuseTheBoot(t *testing.T) { t.Fatalf("a panic escaped the install path and would have refused the boot: %v", r) } }() - // A command with no server context is the cheapest way in: the install reads one out of the command - // and the path under test is whatever it does with what it finds. - installResolved(nil, map[string]string{}, slog.New(slog.NewTextHandler(io.Discard, nil))) + + 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 index 5967511b62..4049fccac5 100644 --- a/config/registry/delivery.go +++ b/config/registry/delivery.go @@ -2,7 +2,6 @@ package registry import ( "fmt" - "sort" ) // decodedNotLookedUp holds the sections whose values reach their reader by a decode, and why. @@ -19,6 +18,13 @@ var decodedNotLookedUp = map[string]string{} // 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() @@ -61,7 +67,10 @@ func DecodedSections() map[string]string { // 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 { - owning := DecodedSections() + // 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 { @@ -69,7 +78,7 @@ func SuppliedByDecodedSection(resolved Resolved) map[string]map[string]any { } out := map[string]map[string]any{} - for _, section := range Sections() { + for _, section := range registered { if _, owned := owning[section.Name]; !owned { continue } @@ -85,29 +94,3 @@ func SuppliedByDecodedSection(resolved Resolved) map[string]map[string]any { } return out } - -// UndeliveredSections returns the registered sections that named no delivery, sorted. -// -// A section reaches its reader one of two ways and the registry cannot tell which, so the answer is -// declared. A section that declares nothing is treated as read by a lookup, which is right for almost all -// of them and silently wrong for the rest: its keys resolve, install into the source, and change nothing -// the node runs. That is the failure this package exists to remove, so the set is reported and a caller -// linking every section holds it to what it expects. -func UndeliveredSections(expectDecoded map[string]bool) []string { - var out []string - for _, section := range Sections() { - if expectDecoded[section.Name] != DecodedNotLookedUp(section.Name) { - out = append(out, section.Name) - } - } - sort.Strings(out) - return out -} - -// DecodedNotLookedUp reports whether a section's values reach their reader by a decode. -func DecodedNotLookedUp(section string) bool { - mu.RLock() - defer mu.RUnlock() - _, ok := decodedNotLookedUp[section] - return ok -} 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/environment.go b/config/registry/environment.go deleted file mode 100644 index d3f440b178..0000000000 --- a/config/registry/environment.go +++ /dev/null @@ -1,46 +0,0 @@ -package registry - -import "fmt" - -// envCannotDeliver holds the keys an environment variable cannot supply, with the reason. -var envCannotDeliver = map[string]string{} - -// RefuseFromEnvironment records that an environment variable cannot supply a key. -// -// An environment carries one string per name. Most readers cast that string into whatever the setting -// needs, so the environment works for them. A reader that takes its value's exact type instead cannot be -// handed a string at all, and no spelling of the variable would satisfy it. -// -// Resolving such a key from the environment puts an unusable value at the top of the order, and installing -// it stops the node. Leaving the channel out means the file's value applies and the node runs. That is -// deliberately not what the machinery this replaces does, which resolves the variable and refuses to -// start, so the difference is recorded rather than assumed. A value silently doing nothing is the failure -// this whole surface exists to remove, which is why the reason is required and not optional. -// -// section is the section that declares the key, so a refused key is attributable to a registration the -// way every other defect is. Whether the key is one that section declares is answered when something -// resolves, because a refusal may be recorded before the registration it belongs to. -// -// Called from the owning package, beside its registration, so the reason sits with the code that knows it. -func RefuseFromEnvironment(section, key, reason string) { - mu.Lock() - defer mu.Unlock() - if reason == "" { - defects = append(defects, Defect{Section: section, Err: fmt.Errorf( - "refusing %q from the environment with no reason; an operator whose variable is ignored has "+ - "to be told why", key)}) - return - } - envCannotDeliver[key] = reason -} - -// EnvCannotDeliver returns the keys an environment variable cannot supply, and why. -func EnvCannotDeliver() map[string]string { - mu.RLock() - defer mu.RUnlock() - out := make(map[string]string, len(envCannotDeliver)) - for key, reason := range envCannotDeliver { - out[key] = reason - } - return out -} 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())) + } + }) + } +}