From d7ee3e6c77a82a8c2116a56738b14e69927b5d20 Mon Sep 17 00:00:00 2001 From: Eli Mallon Date: Wed, 30 Sep 2026 15:30:10 -0700 Subject: [PATCH] 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) { -- knotmirror 0.0.1