feat: add Config.Validate() to allow validation of configuration options before starting the manager (#3405)

* feat: add Config.Validate() to allow validation of configuration options before starting the manager

* feat: add config validation unit tests and FlagNamespacedName

* lint

Co-authored-by: Grzegorz Burzyński <[email protected]>

Author: pmalek

internal/cmd/rootcmd/rootcmd.go

@@ -14,40 +14,49 @@ import (
 // Execute is the entry point to the controller manager.
 func Execute() {
 	var (
-		cfg     manager.Config
-		rootCmd = &cobra.Command{
-			PersistentPreRunE: bindEnvVars,
-			RunE: func(cmd *cobra.Command, args []string) error {
-				return Run(cmd.Context(), &cfg)
-			},
-			SilenceUsage: true,
-			// We can silence the errors because cobra.CheckErr below will print
-			// the returned error and set the exit code to 1.
-			SilenceErrors: true,
-		}
-		versionCmd = &cobra.Command{
-			Use:   "version",
-			Short: "Show JSON version information",
-			RunE: func(cmd *cobra.Command, args []string) error {
-				type Version struct {
-					Release string `json:"release"`
-					Repo    string `json:"repo"`
-					Commit  string `json:"commit"`
-				}
-				out, err := json.Marshal(Version{
-					Release: metadata.Release,
-					Repo:    metadata.Repo,
-					Commit:  metadata.Commit,
-				})
-				if err != nil {
-					return fmt.Errorf("failed to print version information: %w", err)
-				}
-				fmt.Printf("%s\n", out)
-				return nil
-			},
-		}
+		cfg        manager.Config
+		rootCmd    = GetRootCmd(&cfg)
+		versionCmd = GetVersionCmd()
 	)
 	rootCmd.AddCommand(versionCmd)
-	rootCmd.Flags().AddFlagSet(cfg.FlagSet())
 	cobra.CheckErr(rootCmd.Execute())
 }
+
+func GetRootCmd(cfg *manager.Config) *cobra.Command {
+	cmd := &cobra.Command{
+		PersistentPreRunE: bindEnvVars,
+		RunE: func(cmd *cobra.Command, args []string) error {
+			return Run(cmd.Context(), cfg)
+		},
+		SilenceUsage: true,
+		// We can silence the errors because cobra.CheckErr below will print
+		// the returned error and set the exit code to 1.
+		SilenceErrors: true,
+	}
+	cmd.Flags().AddFlagSet(cfg.FlagSet())
+	return cmd
+}
+
+func GetVersionCmd() *cobra.Command {
+	return &cobra.Command{
+		Use:   "version",
+		Short: "Show JSON version information",
+		RunE: func(cmd *cobra.Command, args []string) error {
+			type Version struct {
+				Release string `json:"release"`
+				Repo    string `json:"repo"`
+				Commit  string `json:"commit"`
+			}
+			out, err := json.Marshal(Version{
+				Release: metadata.Release,
+				Repo:    metadata.Repo,
+				Commit:  metadata.Commit,
+			})
+			if err != nil {
+				return fmt.Errorf("failed to print version information: %w", err)
+			}
+			fmt.Printf("%s\n", out)
+			return nil
+		},
+	}
+}

internal/cmd/rootcmd/rootcmd_test.go

@@ -0,0 +1,44 @@
+package rootcmd
+
+import (
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/kong/kubernetes-ingress-controller/v2/internal/manager"
+)
+
+func TestRootCmd(t *testing.T) {
+	t.Run("root command succeeds by default", func(t *testing.T) {
+		var cfg manager.Config
+		rootCmd := GetRootCmd(&cfg)
+		require.NoError(t, rootCmd.PersistentPreRunE(rootCmd, os.Args[:0]))
+	})
+
+	t.Run("root command succeeds when correct flags where provided", func(t *testing.T) {
+		var cfg manager.Config
+		rootCmd := GetRootCmd(&cfg)
+		require.NoError(t, rootCmd.PersistentPreRunE(rootCmd,
+			append(os.Args[:0],
+				"--publish-service", "namespace/servicename",
+			),
+		))
+	})
+
+	t.Run("binding environment variables succeeds when flag validation passes", func(t *testing.T) {
+		t.Setenv("CONTROLLER_PUBLISH_SERVICE", "namespace/servicename")
+		var cfg manager.Config
+		rootCmd := GetRootCmd(&cfg)
+		require.NoError(t, rootCmd.PersistentPreRunE(rootCmd, os.Args[:0]))
+	})
+
+	t.Run("binding environment variables fails when flag validation fails", func(t *testing.T) {
+		t.Setenv("CONTROLLER_PUBLISH_SERVICE", "servicename")
+		var cfg manager.Config
+		rootCmd := GetRootCmd(&cfg)
+		require.Error(t, rootCmd.PersistentPreRunE(rootCmd, os.Args[:0]),
+			"binding env vars should fail because a non namespaced name of publish service was provided",
+		)
+	})
+}

internal/cmd/rootcmd/run.go

@@ -28,9 +28,14 @@ func RunWithLogger(ctx context.Context, c *manager.Config, deprecatedLogger logr
 		return fmt.Errorf("failed to setup signal handler: %w", err)
 	}
 
+	if err := c.Validate(); err != nil {
+		return fmt.Errorf("failed to validate config: %w", err)
+	}
+
 	diag, err := StartDiagnosticsServer(ctx, manager.DiagnosticsPort, c, logger)
 	if err != nil {
 		return fmt.Errorf("failed to start diagnostics server: %w", err)
 	}
+
 	return manager.Run(ctx, c, diag.ConfigDumps, deprecatedLogger)
 }

internal/manager/config.go

@@ -3,6 +3,7 @@ package manager
 import (
 	"context"
 	"fmt"
+	"regexp"
 	"time"
 
 	"github.com/kong/go-kong/kong"
@@ -57,7 +58,6 @@ type Config struct {
 	// Kubernetes configurations
 	KubeconfigPath           string
 	IngressClassName         string
-	EnableLeaderElection     bool
 	LeaderElectionNamespace  string
 	LeaderElectionID         string
 	Concurrency              int
@@ -66,7 +66,7 @@ type Config struct {
 	GatewayAPIControllerName string
 
 	// Ingress status
-	PublishService       string
+	PublishService       FlagNamespacedName
 	PublishStatusAddress []string
 	UpdateStatus         bool
 
@@ -101,15 +101,27 @@ type Config struct {
 	// helpful for advanced cases with load-balancers so that the ingress
 	// controller can be gracefully removed/drained from their rotation.
 	TermDelay time.Duration
+
+	flagSet *pflag.FlagSet
 }
 
 // -----------------------------------------------------------------------------
 // Controller Manager - Config - Methods
 // -----------------------------------------------------------------------------
 
+// Validate validates the config. With time this logic may grow to invalidate
+// incorrect configurations.
+func (c *Config) Validate() error {
+	if !isControllerNameValid(c.GatewayAPIControllerName) {
+		return fmt.Errorf("--gateway-api-controller-name (%s) is invalid. The expected format is example.com/controller-name", c.GatewayAPIControllerName)
+	}
+
+	return nil
+}
+
 // FlagSet binds the provided Config to commandline flags.
 func (c *Config) FlagSet() *pflag.FlagSet {
-	flagSet := pflag.NewFlagSet("", pflag.ExitOnError)
+	flagSet := pflag.NewFlagSet("", pflag.ContinueOnError)
 
 	// Logging configurations
 	flagSet.StringVar(&c.LogLevel, "log-level", "info", `Level of logging for the controller. Allowed values are trace, debug, info, warn, error, fatal and panic.`)
@@ -163,11 +175,10 @@ func (c *Config) FlagSet() *pflag.FlagSet {
 	flagSet.StringSliceVar(&c.FilterTags, "kong-admin-filter-tag", []string{"managed-by-ingress-controller"}, "The tag used to manage and filter entities in Kong. This flag can be specified multiple times to specify multiple tags. This setting will be silently ignored if the Kong instance has no tags support.")
 	flagSet.IntVar(&c.Concurrency, "kong-admin-concurrency", 10, "Max number of concurrent requests sent to Kong's Admin API.")
 	flagSet.StringSliceVar(&c.WatchNamespaces, "watch-namespace", nil,
-		`Namespace(s) to watch for Kubernetes resources. Defaults to all namespaces.`+
-			`To watch multiple namespaces, use a comma-separated list of namespaces.`)
+		`Namespace(s) to watch for Kubernetes resources. Defaults to all namespaces. To watch multiple namespaces, use a comma-separated list of namespaces.`)
 
 	// Ingress status
-	flagSet.StringVar(&c.PublishService, "publish-service", "",
+	flagSet.Var(&c.PublishService, "publish-service",
 		`Service fronting Ingress resources in "namespace/name" format. The controller will update Ingress status information with this Service's endpoints.`)
 	flagSet.StringSliceVar(&c.PublishStatusAddress, "publish-status-address", []string{},
 		`User-provided addresses in comma-separated string format, for use in lieu of "publish-service" `+
@@ -218,7 +229,7 @@ func (c *Config) FlagSet() *pflag.FlagSet {
 
 	// Deprecated flags
 
-	flagSet.Float32Var(&c.ProxySyncSeconds, "sync-rate-limit", dataplane.DefaultSyncSeconds, "Use --proxy-sync-seconds instead")
+	_ = flagSet.Float32("sync-rate-limit", dataplane.DefaultSyncSeconds, "Use --proxy-sync-seconds instead")
 	_ = flagSet.MarkDeprecated("sync-rate-limit", "Use --proxy-sync-seconds instead")
 
 	_ = flagSet.Int("stderrthreshold", 0, "Has no effect and will be removed in future releases (see github issue #1297)")
@@ -230,9 +241,10 @@ func (c *Config) FlagSet() *pflag.FlagSet {
 	_ = flagSet.String("kong-custom-entities-secret", "", "Will be removed in next major release.")
 	_ = flagSet.MarkDeprecated("kong-custom-entities-secret", "Will be removed in next major release.")
 
-	flagSet.BoolVar(&c.EnableLeaderElection, "leader-elect", false, "DEPRECATED as of 2.1.0 leader election behavior is determined automatically and this flag has no effect")
-	_ = flagSet.MarkDeprecated("leader-elect", "DEPRECATED as of 2.1.0 leader election behavior is determined automatically and this flag has no effect")
+	_ = flagSet.Bool("leader-elect", false, "DEPRECATED as of 2.1.0: leader election behavior is determined automatically based on the Kong database setting and this flag has no effect")
+	_ = flagSet.MarkDeprecated("leader-elect", "DEPRECATED as of 2.1.0: leader election behavior is determined automatically based on the Kong database setting and this flag has no effect")
 
+	c.flagSet = flagSet
 	return flagSet
 }
 
@@ -277,3 +289,9 @@ func (c *Config) GetKubeClient() (client.Client, error) {
 	}
 	return client.New(conf, client.Options{})
 }
+
+func isControllerNameValid(controllerName string) bool {
+	// https://github.com/kubernetes-sigs/gateway-api/blob/547122f7f55ac0464685552898c560658fb40073/apis/v1beta1/shared_types.go#L448-L463
+	re := regexp.MustCompile(`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*\/[A-Za-z0-9\/\-._~%!$&'()*+,;=:]+$`)
+	return re.Match([]byte(controllerName))
+}

internal/manager/config_test.go

@@ -0,0 +1,59 @@
+package manager
+
+import (
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestConfigValidate(t *testing.T) {
+	t.Run("--gateway-api-controller-name", func(t *testing.T) {
+		t.Run("valid config", func(t *testing.T) {
+			var c Config
+			require.NoError(t, c.FlagSet().Parse(
+				[]string{
+					os.Args[0],
+					`--gateway-api-controller-name`, `example.com/controller-name`,
+				},
+			))
+			require.NoError(t, c.Validate())
+		})
+
+		t.Run("invalid config", func(t *testing.T) {
+			var c Config
+			require.NoError(t, c.FlagSet().Parse(
+				[]string{
+					os.Args[0],
+					`--gateway-api-controller-name`, `%invalid_controller_name$`,
+				},
+			))
+			require.Error(t, c.Validate())
+		})
+	})
+
+	t.Run("--publish-service", func(t *testing.T) {
+		t.Run("valid config", func(t *testing.T) {
+			var c Config
+			require.NoError(t, c.FlagSet().Parse(
+				[]string{
+					os.Args[0],
+					`--publish-service`, `namespace/servicename`,
+				},
+			))
+			require.NoError(t, c.Validate())
+		})
+
+		t.Run("invalid config", func(t *testing.T) {
+			var c Config
+			require.Error(t, c.FlagSet().Parse(
+				[]string{
+					os.Args[0],
+					`--publish-service`, `servicename`,
+				},
+			))
+			// publish service is validated through FlagNamespacedName validation logic.
+			require.NoError(t, c.Validate())
+		})
+	})
+}

internal/manager/controllerdef.go

@@ -360,7 +360,7 @@ func setupControllers(
 				Log:                  ctrl.Log.WithName("controllers").WithName(gatewayFeature),
 				Scheme:               mgr.GetScheme(),
 				DataplaneClient:      dataplaneClient,
-				PublishService:       c.PublishService,
+				PublishService:       c.PublishService.String(),
 				WatchNamespaces:      c.WatchNamespaces,
 				EnableReferenceGrant: referenceGrantsEnabled,
 				CacheSyncTimeout:     c.CacheSyncTimeout,

internal/manager/feature_gates.go

@@ -31,12 +31,12 @@ const (
 )
 
 // setupFeatureGates converts feature gates to controller enablement.
-func setupFeatureGates(setupLog logr.Logger, c *Config) (map[string]bool, error) {
+func setupFeatureGates(setupLog logr.Logger, featureGates map[string]bool) (map[string]bool, error) {
 	// generate a map of feature gates by string names to their controller enablement
 	ctrlMap := getFeatureGatesDefaults()
 
 	// override the default settings
-	for feature, enabled := range c.FeatureGates {
+	for feature, enabled := range featureGates {
 		setupLog.Info("found configuration option for gated feature", "feature", feature, "enabled", enabled)
 		_, ok := ctrlMap[feature]
 		if !ok {

internal/manager/feature_gates_test.go

@@ -16,24 +16,23 @@ func TestFeatureGates(t *testing.T) {
 	baseLogger.SetOutput(out)
 	baseLogger.SetLevel(logrus.DebugLevel)
 	setupLog := logrusr.New(baseLogger)
-	config := new(Config)
 
 	t.Log("verifying feature gates setup defaults when no feature gates are configured")
-	fgs, err := setupFeatureGates(setupLog, config)
+	fgs, err := setupFeatureGates(setupLog, nil)
 	assert.NoError(t, err)
 	assert.Len(t, fgs, len(getFeatureGatesDefaults()))
 
 	t.Log("verifying feature gates setup results when valid feature gates options are present")
-	config.FeatureGates = map[string]bool{gatewayFeature: true}
-	fgs, err = setupFeatureGates(setupLog, config)
+	featureGates := map[string]bool{gatewayFeature: true}
+	fgs, err = setupFeatureGates(setupLog, featureGates)
 	assert.NoError(t, err)
 	assert.True(t, fgs[gatewayFeature])
 
 	t.Log("configuring several invalid feature gates options")
-	config.FeatureGates = map[string]bool{"invalidGateway": true}
+	featureGates = map[string]bool{"invalidGateway": true}
 
 	t.Log("verifying feature gates setup results when invalid feature gates options are present")
-	_, err = setupFeatureGates(setupLog, config)
+	_, err = setupFeatureGates(setupLog, featureGates)
 	assert.Error(t, err)
 	assert.Contains(t, err.Error(), "invalidGateway is not a valid feature")
 }

internal/manager/flagnamespacedname.go

@@ -0,0 +1,33 @@
+package manager
+
+import (
+	"fmt"
+	"strings"
+
+	"k8s.io/apimachinery/pkg/types"
+)
+
+// FlagNamespacedName allows parsing command line flags straight to types.NamespacedName.
+type FlagNamespacedName struct {
+	NN types.NamespacedName
+}
+
+func (f *FlagNamespacedName) String() string {
+	return f.NN.String()
+}
+
+func (f *FlagNamespacedName) Set(v string) error {
+	s := strings.SplitN(v, "/", 3)
+	if len(s) != 2 {
+		return fmt.Errorf("namespaced name should be in the format: <namespace>/<name>")
+	}
+	f.NN = types.NamespacedName{
+		Namespace: s[0],
+		Name:      s[1],
+	}
+	return nil
+}
+
+func (f *FlagNamespacedName) Type() string {
+	return "FlagNamespacedName"
+}