From 43ab701fb913fe17f1458eb3d34f0dbf634e1e77 Mon Sep 17 00:00:00 2001 From: Eli Mallon Date: Wed, 30 Sep 2026 15:18:44 -0700 Subject: [PATCH 1/3] config: split startup validation into pure CheckConfig; add validate-config subcommand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLI.Validate interleaved flag-combination checks with in-memory default preparation, so the checks could not run on their own and each reported only its first error. Split into CheckConfig (pure, error-collecting, the single home for 'this flag combination will take the node down' checks) and PrepareConfig (defaults and process-local setup). Validate runs both, unchanged from a starting node's perspective. New: signing-key path is now checked at startup (unreadable or non-PEM --signing-key previously only exploded mid-boot in the app-updates path). New: 'streamplace validate-config' runs exactly what a starting node runs (cli.Validate) and exits without opening listeners, databases, or media pipelines, for deploy tooling to call before a restart. The gstreamer self-test is skipped for it so it works even when the media stack is broken — that is when you are diagnosing a crashloop. --- pkg/cmd/streamplace.go | 28 ++++++++++ pkg/cmd/validate_config_test.go | 41 +++++++++++++++ pkg/config/config.go | 59 ++++++++++++++++----- pkg/config/config_check_test.go | 93 +++++++++++++++++++++++++++++++++ 4 files changed, 209 insertions(+), 12 deletions(-) create mode 100644 pkg/cmd/validate_config_test.go create mode 100644 pkg/config/config_check_test.go diff --git a/pkg/cmd/streamplace.go b/pkg/cmd/streamplace.go index 60321a964..81716e191 100644 --- a/pkg/cmd/streamplace.go +++ b/pkg/cmd/streamplace.go @@ -92,6 +92,7 @@ func start(build *config.BuildFlags, platformJobs []jobFunc) error { makeMigrateStateCommand(), makeSyncCommand(build), makeBrandingCommand(build), + makeValidateConfigCommand(build), makeE2eCommand(build), } // Add the verbosity flag @@ -101,6 +102,12 @@ func start(build *config.BuildFlags, platformJobs []jobFunc) error { // Value: "3", // }) app.Before = func(ctx context.Context, cmd *urfavecli.Command) (context.Context, error) { + // validate-config must run wherever the binary runs, media stack or + // not: diagnosing a crashloop with a broken gstreamer install is + // exactly when you want pure config validation. + if cmd.Name == "validate-config" { + return ctx, nil + } // Run self-test before starting selfTest := cmd.Name == "self-test" err := media.RunSelfTest(ctx) @@ -924,6 +931,27 @@ func makeSelfTestCommand(build *config.BuildFlags) *urfavecli.Command { } } +// makeValidateConfigCommand checks the current flags and environment exactly +// the way a starting node would (cli.Validate) and exits without opening any +// listeners, databases, or media pipelines. Point deploy tooling at it: it +// fails before a misconfiguration can crashloop the node on restart. +func makeValidateConfigCommand(build *config.BuildFlags) *urfavecli.Command { + cli := config.CLI{Build: build} + cmd := cli.NewCommand("validate-config") + cmd.Usage = "validate configuration and exit without starting the node" + // NewCommand's Before hook already runs cli.Validate, so a bad + // configuration fails the command before we ever get here; this action + // only reports success. + cmd.Action = func(ctx context.Context, cmd *urfavecli.Command) error { + if err := cli.Validate(cmd); err != nil { + return err + } + fmt.Println("configuration OK") + return nil + } + return cmd +} + // makeVODTestCommand runs the VOD gstreamer pipeline on a local file // and prints probe results. Useful for reproducing gstreamer-side // crashes against the static binary without needing the full server diff --git a/pkg/cmd/validate_config_test.go b/pkg/cmd/validate_config_test.go new file mode 100644 index 000000000..3cb26eb10 --- /dev/null +++ b/pkg/cmd/validate_config_test.go @@ -0,0 +1,41 @@ +package cmd + +import ( + "testing" + + "github.com/stretchr/testify/require" + "stream.place/streamplace/pkg/config" +) + +// TestValidateConfigCommand checks the registration, which is the part that +// is easy to get wrong: the command has to carry the server's flags so an +// operator can validate the exact same flag set the node would start with. +func TestValidateConfigCommand(t *testing.T) { + cmd := makeValidateConfigCommand(&config.BuildFlags{Version: "test"}) + require.Equal(t, "validate-config", cmd.Name) + require.NotNil(t, cmd.Action) + + names := map[string]bool{} + for _, flag := range cmd.Flags { + for _, name := range flag.Names() { + names[name] = true + } + } + require.True(t, names["data-dir"], "validate-config needs the data dir") + require.True(t, names["vod-cdn-provider"], "validate-config needs the CDN flags") + require.True(t, names["bunny-token-auth-key"], "validate-config needs the bunny flags") +} + +// TestValidateConfigCommandRuns runs the command for real: a bad flag +// combination fails with the same error a starting node would die on, and a +// clean configuration passes without opening anything. +func TestValidateConfigCommandRuns(t *testing.T) { + cmd := makeValidateConfigCommand(&config.BuildFlags{Version: "test"}) + err := cmd.Run(t.Context(), []string{"validate-config", "--data-dir", t.TempDir(), + "--bunny-token-auth-key", "k"}) + require.Error(t, err) + require.Contains(t, err.Error(), "--vod-cdn-provider=bunny") + + err = cmd.Run(t.Context(), []string{"validate-config", "--data-dir", t.TempDir()}) + require.NoError(t, err) +} diff --git a/pkg/config/config.go b/pkg/config/config.go index 6c69c0cf6..8dcbe066c 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -1431,16 +1431,60 @@ func EnableSQLLogging() { ) } +// Validate checks the configuration and applies the in-memory defaults a +// starting node needs. The flag-combination checks live in CheckConfig so +// they can also run on their own; streamplace validate-config calls that to +// tell an operator what is wrong before a restart. Anything this method does +// beyond checking is in-memory only: no files written, no listeners opened. func (cli *CLI) Validate(cmd *urfavecli.Command) error { + if err := cli.CheckConfig(); err != nil { + return err + } + return cli.PrepareConfig() +} + +// CheckConfig reports every configuration problem it can find, all at once, +// without changing anything: no defaults are applied, nothing is read from +// disk except to prove a configured path is usable, no loggers are swapped. +// It is the single home for "this flag combination will take the node down" +// checks. If a startup crash can be caused by flags or the environment alone, +// it belongs here, so `streamplace validate-config` can catch it before a +// restart does. +func (cli *CLI) CheckConfig() error { + var errs []error if cli.DataDir == "" { - return fmt.Errorf("could not determine default data dir (no $HOME) and none provided, please set --data-dir") + errs = append(errs, fmt.Errorf("could not determine default data dir (no $HOME) and none provided, please set --data-dir")) } if cli.LivepeerGateway && cli.LivepeerGatewayURL != "" { - return fmt.Errorf("defining both livepeer-gateway and livepeer-gateway-url doesn't make sense. do you want an embedded gateway or an external one?") + errs = append(errs, fmt.Errorf("defining both livepeer-gateway and livepeer-gateway-url doesn't make sense. do you want an embedded gateway or an external one?")) } if _, err := renditions.Ladder(cli.TranscodeRenditions); err != nil { - return fmt.Errorf("--transcode-renditions: %w", err) + errs = append(errs, fmt.Errorf("--transcode-renditions: %w", err)) + } + if cli.FirebaseServiceAccount != "" && cli.FirebaseServiceAccountFile != "" { + errs = append(errs, fmt.Errorf("defining both firebase-service-account and firebase-service-account-file doesn't make sense. do you want a base64-encoded string or a file?")) + } + if cli.SigningKeyPath != "" { + if _, err := cli.ParseSigningKey(); err != nil { + errs = append(errs, fmt.Errorf("--signing-key: %w", err)) + } + } + if err := cli.validateVODCDN(); err != nil { + errs = append(errs, err) + } + if err := cli.validateLiveCDN(); err != nil { + errs = append(errs, err) } + return errors.Join(errs...) +} + +// PrepareConfig applies the defaults and process-level setup that used to be +// interleaved with the checks in Validate: data-dir substitution, SQL logging +// verbosity, host fallbacks, the firebase service account file, and the +// default replicator list. Everything here is in-memory or process-local, so +// callers that only want to know whether the configuration is sound can call +// CheckConfig instead. +func (cli *CLI) PrepareConfig() error { if cli.LivepeerGateway { log.MonkeypatchStderr() // Livepeer gateway configuration will be handled in the caller @@ -1464,9 +1508,6 @@ func (cli *CLI) Validate(cmd *urfavecli.Command) error { if cli.PublicOAuth { log.Warn(context.Background(), "--dev-public-oauth is set, this is not recommended for production") } - if cli.FirebaseServiceAccount != "" && cli.FirebaseServiceAccountFile != "" { - return fmt.Errorf("defining both firebase-service-account and firebase-service-account-file doesn't make sense. do you want a base64-encoded string or a file?") - } if cli.FirebaseServiceAccountFile != "" { bs, err := os.ReadFile(cli.FirebaseServiceAccountFile) if err != nil { @@ -1478,12 +1519,6 @@ func (cli *CLI) Validate(cmd *urfavecli.Command) error { if len(cli.Replicators) == 0 { cli.Replicators = []string{ReplicatorWebsocket} } - if err := cli.validateVODCDN(); err != nil { - return err - } - if err := cli.validateLiveCDN(); err != nil { - return err - } return nil } diff --git a/pkg/config/config_check_test.go b/pkg/config/config_check_test.go new file mode 100644 index 000000000..d9a743d41 --- /dev/null +++ b/pkg/config/config_check_test.go @@ -0,0 +1,93 @@ +package config + +import ( + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "encoding/pem" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +// TestCheckConfig covers the pure, error-collecting configuration checks: +// every problem is reported at once, nothing is mutated, and a sound +// configuration passes without touching anything. +func TestCheckConfig(t *testing.T) { + ok := func(c CLI) { + t.Helper() + require.NoError(t, c.CheckConfig()) + } + bad := func(c CLI, want ...string) { + t.Helper() + err := c.CheckConfig() + require.Error(t, err) + for _, w := range want { + require.Contains(t, err.Error(), w) + } + } + + ok(CLI{DataDir: t.TempDir()}) + + bad(CLI{}, "data-dir") + bad(CLI{DataDir: t.TempDir(), LivepeerGateway: true, LivepeerGatewayURL: "http://127.0.0.1:8935"}, + "livepeer-gateway") + bad(CLI{DataDir: t.TempDir(), TranscodeRenditions: "not-a-rendition"}, + "--transcode-renditions") + bad(CLI{DataDir: t.TempDir(), + FirebaseServiceAccount: "{}", FirebaseServiceAccountFile: "whatever.json"}, + "firebase-service-account") + // The crashloop case this whole thing exists for: provider flags without + // the provider. + bad(CLI{DataDir: t.TempDir(), BunnyTokenAuthKey: "k"}, + "--vod-cdn-provider=bunny") + + // Multiple problems are reported together, not one at a time. + multi := CLI{LivepeerGateway: true, LivepeerGatewayURL: "http://127.0.0.1:8935", + VODCDNProvider: "bunny"} + err := multi.CheckConfig() + require.Error(t, err) + require.Contains(t, err.Error(), "livepeer-gateway") + require.Contains(t, err.Error(), "vod-cdn-url") +} + +// TestCheckConfigSigningKey proves that a broken --signing-key file — a +// classic restart-crashloop cause — is caught by the checks, not discovered +// mid-boot by the app-updates path that reads it. +func TestCheckConfigSigningKey(t *testing.T) { + dir := t.TempDir() + key, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + good := filepath.Join(dir, "good.pem") + bs := pem.EncodeToMemory(&pem.Block{ + Type: "RSA PRIVATE KEY", + Bytes: x509.MarshalPKCS1PrivateKey(key), + }) + require.NoError(t, os.WriteFile(good, bs, 0o600)) + garbage := filepath.Join(dir, "garbage.pem") + require.NoError(t, os.WriteFile(garbage, []byte("not a key"), 0o600)) + + ok := CLI{DataDir: t.TempDir(), SigningKeyPath: good} + require.NoError(t, ok.CheckConfig()) + + for name, path := range map[string]string{ + "garbage": garbage, + "missing": filepath.Join(dir, "nope.pem"), + } { + cli := CLI{DataDir: t.TempDir(), SigningKeyPath: path} + err := cli.CheckConfig() + require.Error(t, err, name) + require.Contains(t, err.Error(), "--signing-key", name) + } +} + +// TestValidatePreparesDefaults checks that the starting-node path still +// applies the in-memory defaults after the check/prepare split. +func TestValidatePreparesDefaults(t *testing.T) { + cli := CLI{DataDir: t.TempDir(), BroadcasterHost: "example.com"} + require.NoError(t, cli.Validate(nil)) + require.Equal(t, []string{ReplicatorWebsocket}, cli.Replicators) + require.Equal(t, "example.com", cli.ServerHost) +} -- 2.51.2 From d7ee3e6c77a82a8c2116a56738b14e69927b5d20 Mon Sep 17 00:00:00 2001 From: Eli Mallon Date: Wed, 30 Sep 2026 15:30:10 -0700 Subject: [PATCH 2/3] config: make Validate idempotent; surface firebase file errors in CheckConfig MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile review follow-ups: - Validate ran twice per startup (urfavecli Before hook + action path) on the same CLI, and the second pass saw the defaults the first pass filled in — the gateway URL, the firebase account — as operator-set values and rejected valid configurations. Validate now runs its checks exactly once per CLI and is a no-op afterwards, which also fixes the pre-existing node-startup variant of this for --livepeer-gateway and --firebase-service-account-file users. validate-config's action no longer re-validates for the same reason. - An unreadable --firebase-service-account-file is now reported by CheckConfig, so it is collected alongside every other problem instead of being hidden behind the first failure. --- pkg/cmd/streamplace.go | 11 +++++------ pkg/cmd/validate_config_test.go | 4 ++++ pkg/config/config.go | 25 +++++++++++++++++++++++-- pkg/config/config_check_test.go | 25 +++++++++++++++++++++++++ 4 files changed, 57 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/streamplace.go b/pkg/cmd/streamplace.go index 81716e191..c6ed145c6 100644 --- a/pkg/cmd/streamplace.go +++ b/pkg/cmd/streamplace.go @@ -939,13 +939,12 @@ func makeValidateConfigCommand(build *config.BuildFlags) *urfavecli.Command { cli := config.CLI{Build: build} cmd := cli.NewCommand("validate-config") cmd.Usage = "validate configuration and exit without starting the node" - // NewCommand's Before hook already runs cli.Validate, so a bad - // configuration fails the command before we ever get here; this action - // only reports success. + // NewCommand's Before hook runs cli.Validate, so a bad configuration + // fails the command before we ever get here. Do not check again in this + // action: the checks must run against pristine flag values, and any + // second pass would see the defaults PrepareConfig filled in as + // operator-set conflicts. cmd.Action = func(ctx context.Context, cmd *urfavecli.Command) error { - if err := cli.Validate(cmd); err != nil { - return err - } fmt.Println("configuration OK") return nil } diff --git a/pkg/cmd/validate_config_test.go b/pkg/cmd/validate_config_test.go index 3cb26eb10..8f07621cc 100644 --- a/pkg/cmd/validate_config_test.go +++ b/pkg/cmd/validate_config_test.go @@ -36,6 +36,10 @@ func TestValidateConfigCommandRuns(t *testing.T) { require.Error(t, err) require.Contains(t, err.Error(), "--vod-cdn-provider=bunny") + err = cmd.Run(t.Context(), []string{"validate-config", "--data-dir", t.TempDir(), + "--livepeer-gateway"}) + require.NoError(t, err) + err = cmd.Run(t.Context(), []string{"validate-config", "--data-dir", t.TempDir()}) require.NoError(t, err) } diff --git a/pkg/config/config.go b/pkg/config/config.go index 8dcbe066c..45b4f53a4 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -132,6 +132,7 @@ type CLI struct { AccessJWK jwk.Key ServiceAuthKey jwk.Key dataDirFlags []*string + validated bool DiscordWebhooks []*discordtypes.Webhook AppleTeamID string AndroidCertFingerprint string @@ -1433,14 +1434,27 @@ func EnableSQLLogging() { // Validate checks the configuration and applies the in-memory defaults a // starting node needs. The flag-combination checks live in CheckConfig so -// they can also run on their own; streamplace validate-config calls that to +// they can also run on their own; streamplace validate-config uses that to // tell an operator what is wrong before a restart. Anything this method does // beyond checking is in-memory only: no files written, no listeners opened. +// +// Validate is idempotent: urfavecli runs both a Before hook and the action +// path against the same CLI, and a second pass would see the defaults +// PrepareConfig filled in (the gateway URL, the firebase account) as +// operator-set values and misreport them as conflicts. The checks therefore +// run once, against pristine flag values. func (cli *CLI) Validate(cmd *urfavecli.Command) error { + if cli.validated { + return nil + } if err := cli.CheckConfig(); err != nil { return err } - return cli.PrepareConfig() + if err := cli.PrepareConfig(); err != nil { + return err + } + cli.validated = true + return nil } // CheckConfig reports every configuration problem it can find, all at once, @@ -1464,6 +1478,13 @@ func (cli *CLI) CheckConfig() error { if cli.FirebaseServiceAccount != "" && cli.FirebaseServiceAccountFile != "" { errs = append(errs, fmt.Errorf("defining both firebase-service-account and firebase-service-account-file doesn't make sense. do you want a base64-encoded string or a file?")) } + if cli.FirebaseServiceAccountFile != "" { + // Read-only proof that the configured path is usable. PrepareConfig + // reads the file again to actually load the account. + if _, err := os.ReadFile(cli.FirebaseServiceAccountFile); err != nil { + errs = append(errs, fmt.Errorf("--firebase-service-account-file: %w", err)) + } + } if cli.SigningKeyPath != "" { if _, err := cli.ParseSigningKey(); err != nil { errs = append(errs, fmt.Errorf("--signing-key: %w", err)) diff --git a/pkg/config/config_check_test.go b/pkg/config/config_check_test.go index d9a743d41..029750f7e 100644 --- a/pkg/config/config_check_test.go +++ b/pkg/config/config_check_test.go @@ -43,6 +43,14 @@ func TestCheckConfig(t *testing.T) { // the provider. bad(CLI{DataDir: t.TempDir(), BunnyTokenAuthKey: "k"}, "--vod-cdn-provider=bunny") + // An unreadable firebase file is reported on its own and alongside an + // unrelated problem, not swallowed by the other failure. + bad(CLI{DataDir: t.TempDir(), + FirebaseServiceAccountFile: filepath.Join(t.TempDir(), "nope.json")}, + "--firebase-service-account-file") + bad(CLI{TranscodeRenditions: "not-a-rendition", + FirebaseServiceAccountFile: filepath.Join(t.TempDir(), "nope.json")}, + "--transcode-renditions", "--firebase-service-account-file") // Multiple problems are reported together, not one at a time. multi := CLI{LivepeerGateway: true, LivepeerGatewayURL: "http://127.0.0.1:8935", @@ -83,6 +91,23 @@ func TestCheckConfigSigningKey(t *testing.T) { } } +// TestValidateIsIdempotent pins the reason Validate runs its checks exactly +// once: urfavecli invokes it from both the Before hook and the action path, +// and a second pass must not mistake the defaults PrepareConfig filled in +// (the gateway URL, the firebase account) for operator-set conflicts. +func TestValidateIsIdempotent(t *testing.T) { + cli := CLI{DataDir: t.TempDir(), LivepeerGateway: true} + require.NoError(t, cli.Validate(nil)) + require.NoError(t, cli.Validate(nil)) + require.Equal(t, "http://127.0.0.1:8935", cli.LivepeerGatewayURL) + + fbFile := filepath.Join(t.TempDir(), "firebase.json") + require.NoError(t, os.WriteFile(fbFile, []byte("{}"), 0o600)) + cli = CLI{DataDir: t.TempDir(), FirebaseServiceAccountFile: fbFile} + require.NoError(t, cli.Validate(nil)) + require.Equal(t, "{}", cli.FirebaseServiceAccount) +} + // TestValidatePreparesDefaults checks that the starting-node path still // applies the in-memory defaults after the check/prepare split. func TestValidatePreparesDefaults(t *testing.T) { -- 2.51.2 From 0542b8f00e5046272e195b707efd1e294a040c82 Mon Sep 17 00:00:00 2001 From: Eli Mallon Date: Wed, 30 Sep 2026 16:53:44 -0700 Subject: [PATCH 3/3] config: drop the validated memo; remove redundant Validate calls instead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile P1 follow-up: the memoized Validate skipped its checks whenever the same command object was run again with different flags, so a reused run could report 'configuration OK' for a bad configuration. The memo existed to paper over double validation on one CLI: NewCommand attaches cli.Validate as the Before hook, and runSync plus the branding opener called it again on the action path, where PrepareConfig's filled-in defaults (gateway URL, firebase account) look like operator-set conflicts. Those redundant calls are now removed instead — their Before hooks already validated — which also fixes streamplace sync and branding crashing for valid --livepeer-gateway and --firebase-service-account-file users. Validate is stateless again: every run checks its own flags, pinned by a bad run after a good one in TestValidateConfigCommandRuns. --- pkg/cmd/streamplace.go | 10 ++++------ pkg/cmd/validate_config_test.go | 20 +++++++++++--------- pkg/config/config.go | 21 +++++++-------------- pkg/config/config_check_test.go | 25 ++++++++----------------- 4 files changed, 30 insertions(+), 46 deletions(-) diff --git a/pkg/cmd/streamplace.go b/pkg/cmd/streamplace.go index c6ed145c6..74bb64402 100644 --- a/pkg/cmd/streamplace.go +++ b/pkg/cmd/streamplace.go @@ -1365,9 +1365,8 @@ func makeSyncCommand(build *config.BuildFlags) *urfavecli.Command { // manager, no firehose: this process talks to other people's PDSes and to the // two databases, and then it is done. func runSync(ctx context.Context, build *config.BuildFlags, cmd *urfavecli.Command, cli *config.CLI) error { - if err := cli.Validate(cmd); err != nil { - return err - } + // The sync command's Before hook already ran cli.Validate; calling it + // again would see PrepareConfig's defaults as operator-set conflicts. log.SetColorLogger(cli.Color) ctx = log.WithDebugValue(ctx, cli.Debug) log.Log(ctx, "streamplace sync", "version", build.Version, "dataDir", cli.DataDir) @@ -1430,9 +1429,8 @@ func makeBrandingCommand(build *config.BuildFlags) *urfavecli.Command { root := cli.NewCommand("branding") root.Usage = "export or import the node's branding as a bundle (zip with branding.yaml)" open := func(ctx context.Context, cmd *urfavecli.Command) (*statedb.StatefulDB, string, error) { - if err := cli.Validate(cmd); err != nil { - return nil, "", err - } + // The branding command's Before hook already ran cli.Validate; see + // runSync for why a second pass would be wrong. log.SetColorLogger(cli.Color) mod, err := model.MakeDBConns(cli.DataFilePath([]string{"index"}), cli.IndexDBConnections) if err != nil { diff --git a/pkg/cmd/validate_config_test.go b/pkg/cmd/validate_config_test.go index 8f07621cc..f3f30c2ef 100644 --- a/pkg/cmd/validate_config_test.go +++ b/pkg/cmd/validate_config_test.go @@ -26,20 +26,22 @@ func TestValidateConfigCommand(t *testing.T) { require.True(t, names["bunny-token-auth-key"], "validate-config needs the bunny flags") } -// TestValidateConfigCommandRuns runs the command for real: a bad flag -// combination fails with the same error a starting node would die on, and a -// clean configuration passes without opening anything. +// TestValidateConfigCommandRuns runs the command for real: a clean +// configuration passes without opening anything, and a bad flag combination +// fails with the same error a starting node would die on — including a bad +// run AFTER a good one, which pins that every run validates its own flags +// rather than skipping after a previous success. func TestValidateConfigCommandRuns(t *testing.T) { cmd := makeValidateConfigCommand(&config.BuildFlags{Version: "test"}) - err := cmd.Run(t.Context(), []string{"validate-config", "--data-dir", t.TempDir(), - "--bunny-token-auth-key", "k"}) - require.Error(t, err) - require.Contains(t, err.Error(), "--vod-cdn-provider=bunny") + err := cmd.Run(t.Context(), []string{"validate-config", "--data-dir", t.TempDir()}) + require.NoError(t, err) err = cmd.Run(t.Context(), []string{"validate-config", "--data-dir", t.TempDir(), "--livepeer-gateway"}) require.NoError(t, err) - err = cmd.Run(t.Context(), []string{"validate-config", "--data-dir", t.TempDir()}) - require.NoError(t, err) + err = cmd.Run(t.Context(), []string{"validate-config", "--data-dir", t.TempDir(), + "--bunny-token-auth-key", "k"}) + require.Error(t, err) + require.Contains(t, err.Error(), "--vod-cdn-provider=bunny") } diff --git a/pkg/config/config.go b/pkg/config/config.go index 45b4f53a4..4e398adc3 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -132,7 +132,6 @@ type CLI struct { AccessJWK jwk.Key ServiceAuthKey jwk.Key dataDirFlags []*string - validated bool DiscordWebhooks []*discordtypes.Webhook AppleTeamID string AndroidCertFingerprint string @@ -1438,23 +1437,17 @@ func EnableSQLLogging() { // tell an operator what is wrong before a restart. Anything this method does // beyond checking is in-memory only: no files written, no listeners opened. // -// Validate is idempotent: urfavecli runs both a Before hook and the action -// path against the same CLI, and a second pass would see the defaults -// PrepareConfig filled in (the gateway URL, the firebase account) as -// operator-set values and misreport them as conflicts. The checks therefore -// run once, against pristine flag values. +// Call this exactly once per parsed configuration. The checks must run +// against pristine flag values, because PrepareConfig fills in values (the +// gateway URL, the firebase account) that a second pass would mistake for +// operator-set conflicts. Every command gets this: NewCommand's Before hook +// for subcommands, runMain for the node — which is why callers further down +// the action path must not call Validate again. func (cli *CLI) Validate(cmd *urfavecli.Command) error { - if cli.validated { - return nil - } if err := cli.CheckConfig(); err != nil { return err } - if err := cli.PrepareConfig(); err != nil { - return err - } - cli.validated = true - return nil + return cli.PrepareConfig() } // CheckConfig reports every configuration problem it can find, all at once, diff --git a/pkg/config/config_check_test.go b/pkg/config/config_check_test.go index 029750f7e..999069c62 100644 --- a/pkg/config/config_check_test.go +++ b/pkg/config/config_check_test.go @@ -91,15 +91,15 @@ func TestCheckConfigSigningKey(t *testing.T) { } } -// TestValidateIsIdempotent pins the reason Validate runs its checks exactly -// once: urfavecli invokes it from both the Before hook and the action path, -// and a second pass must not mistake the defaults PrepareConfig filled in -// (the gateway URL, the firebase account) for operator-set conflicts. -func TestValidateIsIdempotent(t *testing.T) { - cli := CLI{DataDir: t.TempDir(), LivepeerGateway: true} - require.NoError(t, cli.Validate(nil)) +// TestValidatePreparesDefaults checks that the starting-node path still +// applies the in-memory defaults after the check/prepare split, and that +// Validate is called exactly once per parsed configuration — see the contract +// on Validate for why a second pass would be wrong. +func TestValidatePreparesDefaults(t *testing.T) { + cli := CLI{DataDir: t.TempDir(), BroadcasterHost: "example.com"} require.NoError(t, cli.Validate(nil)) - require.Equal(t, "http://127.0.0.1:8935", cli.LivepeerGatewayURL) + require.Equal(t, []string{ReplicatorWebsocket}, cli.Replicators) + require.Equal(t, "example.com", cli.ServerHost) fbFile := filepath.Join(t.TempDir(), "firebase.json") require.NoError(t, os.WriteFile(fbFile, []byte("{}"), 0o600)) @@ -107,12 +107,3 @@ func TestValidateIsIdempotent(t *testing.T) { require.NoError(t, cli.Validate(nil)) require.Equal(t, "{}", cli.FirebaseServiceAccount) } - -// TestValidatePreparesDefaults checks that the starting-node path still -// applies the in-memory defaults after the check/prepare split. -func TestValidatePreparesDefaults(t *testing.T) { - cli := CLI{DataDir: t.TempDir(), BroadcasterHost: "example.com"} - require.NoError(t, cli.Validate(nil)) - require.Equal(t, []string{ReplicatorWebsocket}, cli.Replicators) - require.Equal(t, "example.com", cli.ServerHost) -} -- 2.51.2