fix: properly support --config-path argument

Changes:
- Change the flow how the config is merged: first read the config from
  the file if it is set, merge it with the defaults, then init the rest
  of command line flags using the merged defaults + config file as the
  base.
- Make some config fields pointers to change how it's merged.

Signed-off-by: Artem Chernyshev <[email protected]>

Author: Unix4ever

cmd/omni/cmd/cmd.go

@@ -11,36 +11,27 @@ import (
 	"fmt"
 	"os"
 	"os/signal"
-	"sync"
 	"syscall"
 
 	"github.com/siderolabs/gen/ensure"
-	"github.com/siderolabs/go-debug"
+	"github.com/siderolabs/talos/pkg/machinery/config/merge"
 	"github.com/spf13/cobra"
 	"go.uber.org/zap"
 	"go.uber.org/zap/zapcore"
 
 	"github.com/siderolabs/omni/client/pkg/constants"
 	"github.com/siderolabs/omni/client/pkg/panichandler"
+	"github.com/siderolabs/omni/cmd/omni/pkg/app"
+	"github.com/siderolabs/omni/internal/pkg/auth/actor"
 	"github.com/siderolabs/omni/internal/pkg/config"
 	"github.com/siderolabs/omni/internal/version"
 )
 
-func runDebugServer(ctx context.Context, logger *zap.Logger, bindEndpoint string) {
-	debugLogFunc := func(msg string) {
-		logger.Info(msg)
-	}
-
-	if err := debug.ListenAndServe(ctx, bindEndpoint, debugLogFunc); err != nil {
-		logger.Panic("failed to start debug server", zap.Error(err))
-	}
-}
-
 // rootCmd represents the base command when called without any subcommands.
 var rootCmd = &cobra.Command{
 	Use:          "omni",
 	Short:        "Omni Kubernetes management platform service",
-	Long:         "This executable runs both frontend and backend",
+	Long:         "This executable runs both Omni frontend and Omni backend",
 	SilenceUsage: true,
 	Version:      version.Tag,
 	RunE: func(*cobra.Command, []string) error {
@@ -82,20 +73,14 @@ var rootCmd = &cobra.Command{
 			cancel()
 		}, logger)
 
-		var configs []*config.Params
-
-		if rootCmdArgs.configPath != "" {
-			var cfg *config.Params
-
-			cfg, err = config.LoadFromFile(rootCmdArgs.configPath)
-			if err != nil {
-				return err
-			}
-
-			configs = append(configs, cfg)
+		config, err := app.PrepareConfig(logger, cmdConfig)
+		if err != nil {
+			return err
 		}
 
-		return RunService(ctx, logger, append(configs, cmdConfig)...)
+		ctx = actor.MarkContextAsInternalActor(ctx)
+
+		return app.Run(ctx, config, logger)
 	},
 }
 
@@ -104,13 +89,35 @@ var rootCmdArgs struct {
 	debug      bool
 }
 
-// RootCmd returns the root command.
-func RootCmd() *cobra.Command { return initOnce() }
+// Execute the command.
+func Execute() error {
+	rootCmd.Flags().StringVar(&rootCmdArgs.configPath, "config-path", "", "load the config from the file, flags have bigger priority")
+
+	// Parsing the config path flag early to let it populate the initial configuration
+	// ignore all errors, they will be handled in the Execute
+	rootCmd.Flags().Parse(os.Args[1:]) //nolint:errcheck
+
+	if rootCmdArgs.configPath == "" {
+		return newCommand().Execute()
+	}
+
+	cfg, err := config.LoadFromFile(rootCmdArgs.configPath)
+	if err != nil {
+		return err
+	}
+
+	// override the cmdConfig with the config loaded from the file
+	if err := merge.Merge(cmdConfig, cfg); err != nil {
+		return err
+	}
+
+	return newCommand().Execute()
+}
 
-var cmdConfig = config.InitDefault()
+var cmdConfig = config.Default()
 
-var initOnce = sync.OnceValue(func() *cobra.Command {
-	rootCmd.Flags().BoolVar(&rootCmdArgs.debug, "debug", false, "enable debug logs.")
+func newCommand() *cobra.Command {
+	rootCmd.Flags().BoolVar(&rootCmdArgs.debug, "debug", constants.IsDebugBuild, "enable debug logs.")
 
 	rootCmd.Flags().StringVar(&cmdConfig.Account.ID, "account-id", cmdConfig.Account.ID, "instance account ID, should never be changed.")
 	rootCmd.Flags().StringVar(&cmdConfig.Account.Name, "name", cmdConfig.Account.Name, "instance user-facing name.")
@@ -124,10 +131,8 @@ var initOnce = sync.OnceValue(func() *cobra.Command {
 	defineDebugFlags()
 	defineEtcdBackupsFlags()
 
-	rootCmd.Flags().StringVar(&rootCmdArgs.configPath, "config-path", "", "load the config from the file, flags have bigger priority")
-
 	return rootCmd
-})
+}
 
 func defineServiceFlags() {
 	// API
@@ -146,13 +151,13 @@ func defineServiceFlags() {
 	rootCmd.Flags().StringVar(
 		&cmdConfig.Services.API.KeyFile,
 		"key",
-		"",
+		cmdConfig.Services.API.KeyFile,
 		"TLS key file",
 	)
 	rootCmd.Flags().StringVar(
 		&cmdConfig.Services.API.CertFile,
 		"cert",
-		"",
+		cmdConfig.Services.API.CertFile,
 		"TLS cert file",
 	)
 
@@ -194,13 +199,13 @@ func defineServiceFlags() {
 	rootCmd.Flags().BoolVar(
 		&cmdConfig.Services.Siderolink.DisableLastEndpoint,
 		"siderolink-disable-last-endpoint",
-		false,
+		cmdConfig.Services.Siderolink.DisableLastEndpoint,
 		"do not populate last known peer endpoint for the WireGuard peers",
 	)
 	rootCmd.Flags().BoolVar(
 		&cmdConfig.Services.Siderolink.UseGRPCTunnel,
 		"siderolink-use-grpc-tunnel",
-		false,
+		cmdConfig.Services.Siderolink.UseGRPCTunnel,
 		"use gRPC tunnel to wrap WireGuard traffic instead of UDP. When enabled, "+
 			"the SideroLink connections from Talos machines will be configured to use the tunnel mode, regardless of their individual configuration. ",
 	)
@@ -216,9 +221,10 @@ func defineServiceFlags() {
 		cmdConfig.Services.Siderolink.LogServerPort,
 		"port for TCP log server",
 	)
-	rootCmd.Flags().Var(
+	rootCmd.Flags().StringVar(
 		&cmdConfig.Services.Siderolink.JoinTokensMode,
 		"join-tokens-mode",
+		cmdConfig.Services.Siderolink.JoinTokensMode,
 		"configures Talos machine join flow to use secure node tokens",
 	)
 
@@ -331,10 +337,14 @@ func defineServiceFlags() {
 
 	// DevServerProxy
 	rootCmd.Flags().StringVar(
-		&cmdConfig.Services.DevServerProxy.ProxyTo, "frontend-dst", "",
+		&cmdConfig.Services.DevServerProxy.ProxyTo,
+		"frontend-dst",
+		cmdConfig.Services.DevServerProxy.ProxyTo,
 		"destination address non API requests from proxy server.")
 	rootCmd.Flags().StringVar(
-		&cmdConfig.Services.DevServerProxy.BindEndpoint, "frontend-bind", "",
+		&cmdConfig.Services.DevServerProxy.BindEndpoint,
+		"frontend-bind",
+		cmdConfig.Services.DevServerProxy.BindEndpoint,
 		"proxy server which will redirect all non API requests to the definied frontend server.")
 }
 
@@ -540,8 +550,6 @@ func defineStorageFlags() {
 		"file containing private key to use for decrypting master key slot.",
 	)
 
-	ensure.NoError(rootCmd.MarkFlagRequired("private-key-source"))
-
 	rootCmd.Flags().StringSliceVar(
 		&cmdConfig.Storage.Default.Etcd.PublicKeyFiles,
 		"public-key-files",
@@ -585,7 +593,7 @@ func defineRegistriesFlags() {
 	rootCmd.Flags().StringSliceVar(
 		&cmdConfig.Registries.Mirrors,
 		"registry-mirror",
-		[]string{},
+		cmdConfig.Registries.Mirrors,
 		"list of registry mirrors to use in format: <registry host>=<mirror URL>",
 	)
 }

cmd/omni/main.go

@@ -7,14 +7,14 @@
 package main
 
 import (
-	"os"
+	"log"
 
 	_ "github.com/siderolabs/omni/cmd/acompat" // this package should always be imported first for init->set env to work
 	"github.com/siderolabs/omni/cmd/omni/cmd"
 )
 
 func main() {
-	if err := cmd.RootCmd().Execute(); err != nil {
-		os.Exit(1)
+	if err := cmd.Execute(); err != nil {
+		log.Fatalf("failed to run Omni: %s", err)
 	}
 }

cmd/omni/cmd/run.go renamed to cmd/omni/pkg/app/app.go

@@ -3,23 +3,20 @@
 // Use of this software is governed by the Business Source License
 // included in the LICENSE file.
 
-package cmd
+// Package app contains the Omni startup methods.
+package app
 
 import (
 	"context"
 	"fmt"
 
 	"github.com/cosi-project/runtime/pkg/resource"
 	"github.com/cosi-project/runtime/pkg/state"
-	"github.com/go-logr/zapr"
 	"github.com/prometheus/client_golang/prometheus"
-	"github.com/siderolabs/talos/pkg/machinery/config/merge"
+	"github.com/siderolabs/go-debug"
 	"go.uber.org/zap"
 	"go.uber.org/zap/zapcore"
-	"k8s.io/klog/v2"
 
-	"github.com/siderolabs/omni/client/pkg/compression"
-	"github.com/siderolabs/omni/client/pkg/constants"
 	authres "github.com/siderolabs/omni/client/pkg/omni/resources/auth"
 	omnires "github.com/siderolabs/omni/client/pkg/omni/resources/omni"
 	"github.com/siderolabs/omni/client/pkg/panichandler"
@@ -40,44 +37,32 @@ import (
 	"github.com/siderolabs/omni/internal/pkg/ctxstore"
 	"github.com/siderolabs/omni/internal/pkg/features"
 	"github.com/siderolabs/omni/internal/pkg/siderolink"
-	"github.com/siderolabs/omni/internal/version"
 )
 
-// RunService starts the main Omni server.
-func RunService(ctx context.Context, logger *zap.Logger, paramsList ...*config.Params) error {
-	cfg := config.InitDefault()
+// PrepareConfig prepare the Omni configuration.
+func PrepareConfig(logger *zap.Logger, params ...*config.Params) (*config.Params, error) {
+	var err error
 
-	for _, params := range paramsList {
-		if err := merge.Merge(cfg, params); err != nil {
-			return err
-		}
+	config.Config, err = config.Init(logger, params...)
+	if err != nil {
+		return nil, err
 	}
 
-	cfg.PopulateFallbacks()
+	return config.Config, nil
+}
 
-	if err := cfg.Validate(); err != nil {
-		return err
+func runDebugServer(ctx context.Context, logger *zap.Logger, bindEndpoint string) {
+	debugLogFunc := func(msg string) {
+		logger.Info(msg)
 	}
 
-	config.Config = cfg
-
-	if err := compression.InitConfig(config.Config.Features.EnableConfigDataCompression); err != nil {
-		return err
+	if err := debug.ListenAndServe(ctx, bindEndpoint, debugLogFunc); err != nil {
+		logger.Panic("failed to start debug server", zap.Error(err))
 	}
+}
 
-	logger.Info("initialized resource compression config", zap.Bool("enabled", config.Config.Features.EnableConfigDataCompression))
-
-	// set kubernetes logger to use warn log level and use zap
-	klog.SetLogger(zapr.NewLogger(logger.WithOptions(zap.IncreaseLevel(zapcore.WarnLevel)).With(logging.Component("kubernetes"))))
-
-	if constants.IsDebugBuild {
-		logger.Warn("running debug build")
-	}
-
-	logger.Info("starting Omni", zap.String("version", version.Tag))
-
-	logger.Debug("using config", zap.Any("config", config.Config))
-
+// Run the Omni service.
+func Run(ctx context.Context, cfg *config.Params, logger *zap.Logger) error {
 	if cfg.Debug.Server.Endpoint != "" {
 		panichandler.Go(func() {
 			runDebugServer(ctx, logger, cfg.Debug.Server.Endpoint)

internal/backend/runtime/omni/export_test.go

@@ -30,7 +30,7 @@ func EtcdElections(ctx context.Context, client *clientv3.Client, electionKey str
 	return etcdElections(ctx, client, electionKey, logger, f)
 }
 
-func ClusterValidationOptions(st state.State, etcdBackupConfig config.EtcdBackup, embeddedDiscoveryServiceConfig config.EmbeddedDiscoveryService) []validated.StateOption {
+func ClusterValidationOptions(st state.State, etcdBackupConfig config.EtcdBackup, embeddedDiscoveryServiceConfig *config.EmbeddedDiscoveryService) []validated.StateOption {
 	return clusterValidationOptions(st, etcdBackupConfig, embeddedDiscoveryServiceConfig)
 }
 

internal/backend/runtime/omni/state_validation.go

@@ -42,7 +42,7 @@ import (
 // Validation is only syntactic - they are checked whether they are valid semver strings.
 //
 //nolint:gocognit,gocyclo,cyclop
-func clusterValidationOptions(st state.State, etcdBackupConfig config.EtcdBackup, embeddedDiscoveryServiceConfig config.EmbeddedDiscoveryService) []validated.StateOption {
+func clusterValidationOptions(st state.State, etcdBackupConfig config.EtcdBackup, embeddedDiscoveryServiceConfig *config.EmbeddedDiscoveryService) []validated.StateOption {
 	validateVersions := func(ctx context.Context, existingRes *omni.Cluster, res *omni.Cluster, skipTalosVersion, skipKubernetesVersion bool) error {
 		if skipTalosVersion && skipKubernetesVersion {
 			return nil

internal/backend/runtime/omni/state_validation_test.go

@@ -58,7 +58,7 @@ func TestClusterValidation(t *testing.T) {
 	}
 
 	innerSt := state.WrapCore(namespaced.NewState(inmem.Build))
-	st := validated.NewState(innerSt, omni.ClusterValidationOptions(state.WrapCore(innerSt), etcdBackupConfig, config.EmbeddedDiscoveryService{})...)
+	st := validated.NewState(innerSt, omni.ClusterValidationOptions(state.WrapCore(innerSt), etcdBackupConfig, &config.EmbeddedDiscoveryService{})...)
 
 	talosVersion1 := omnires.NewTalosVersion(resources.DefaultNamespace, "1.4.0")
 	talosVersion1.TypedSpec().Value.CompatibleKubernetesVersions = []string{"1.27.0", "1.27.1"}
@@ -172,7 +172,7 @@ func TestClusterUseEmbeddedDiscoveryServiceValidation(t *testing.T) {
 	ctx, cancel := context.WithTimeout(t.Context(), 3*time.Second)
 	t.Cleanup(cancel)
 
-	buildState := func(conf config.EmbeddedDiscoveryService) (inner, outer state.State) {
+	buildState := func(conf *config.EmbeddedDiscoveryService) (inner, outer state.State) {
 		innerSt := state.WrapCore(namespaced.NewState(inmem.Build))
 		st := validated.NewState(innerSt, omni.ClusterValidationOptions(state.WrapCore(innerSt), config.EtcdBackup{}, conf)...)
 
@@ -182,7 +182,7 @@ func TestClusterUseEmbeddedDiscoveryServiceValidation(t *testing.T) {
 	t.Run("disabled instance-wide - create", func(t *testing.T) {
 		t.Parallel()
 
-		_, st := buildState(config.EmbeddedDiscoveryService{
+		_, st := buildState(&config.EmbeddedDiscoveryService{
 			Enabled: false,
 		})
 
@@ -202,7 +202,7 @@ func TestClusterUseEmbeddedDiscoveryServiceValidation(t *testing.T) {
 		t.Parallel()
 
 		// prepare a cluster which has the feature enabled, while it is disabled instance-wide
-		innerSt, st := buildState(config.EmbeddedDiscoveryService{
+		innerSt, st := buildState(&config.EmbeddedDiscoveryService{
 			Enabled: false,
 		})
 
@@ -231,7 +231,7 @@ func TestClusterUseEmbeddedDiscoveryServiceValidation(t *testing.T) {
 	t.Run("enabled instance-wide", func(t *testing.T) {
 		t.Parallel()
 
-		_, st := buildState(config.EmbeddedDiscoveryService{
+		_, st := buildState(&config.EmbeddedDiscoveryService{
 			Enabled: true,
 		})