refactor: replace config validation with generic ValidatedValue implementation (#3415)

Author: czeslavo

internal/manager/config.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/kong/go-kong/kong"
 	"github.com/spf13/pflag"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd"
 	cliflag "k8s.io/component-base/cli/flag"
@@ -66,7 +67,7 @@ type Config struct {
 	GatewayAPIControllerName string
 
 	// Ingress status
-	PublishService       FlagNamespacedName
+	PublishService       types.NamespacedName
 	PublishStatusAddress []string
 	UpdateStatus         bool
 
@@ -109,13 +110,9 @@ type Config struct {
 // Controller Manager - Config - Methods
 // -----------------------------------------------------------------------------
 
-// Validate validates the config. With time this logic may grow to invalidate
-// incorrect configurations.
+// Validate validates the config. It should be used to validate the config variables' interdependencies.
+// When a single variable is to be validated, NewValidatedValue should be used.
 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
 }
 
@@ -167,7 +164,7 @@ func (c *Config) FlagSet() *pflag.FlagSet {
 	)
 
 	// Kubernetes configurations
-	flagSet.StringVar(&c.GatewayAPIControllerName, "gateway-api-controller-name", string(gateway.ControllerName), "The controller name to match on Gateway API resources.")
+	flagSet.Var(NewValidatedValueWithDefault(&c.GatewayAPIControllerName, gatewayAPIControllerNameFromFlagValue, string(gateway.ControllerName)), "gateway-api-controller-name", "The controller name to match on Gateway API resources.")
 	flagSet.StringVar(&c.KubeconfigPath, "kubeconfig", "", "Path to the kubeconfig file.")
 	flagSet.StringVar(&c.IngressClassName, "ingress-class", annotations.DefaultIngressClass, `Name of the ingress class to route through this controller.`)
 	flagSet.StringVar(&c.LeaderElectionID, "election-id", "5b374a9e.konghq.com", `Election id to use for status update.`)
@@ -178,7 +175,7 @@ func (c *Config) FlagSet() *pflag.FlagSet {
 		`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.Var(&c.PublishService, "publish-service",
+	flagSet.Var(NewValidatedValue(&c.PublishService, namespacedNameFromFlagValue), "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" `+

internal/manager/config_test.go

@@ -0,0 +1,29 @@
+package manager
+
+import (
+	"errors"
+	"strings"
+
+	"k8s.io/apimachinery/pkg/types"
+)
+
+// This file contains a set of constructors that are used to validate and set validated values in Config.
+// They're meant to be used together with ValidatedValue[T] type.
+
+func namespacedNameFromFlagValue(flagValue string) (types.NamespacedName, error) {
+	parts := strings.SplitN(flagValue, "/", 3)
+	if len(parts) != 2 {
+		return types.NamespacedName{}, errors.New("the expected format is namespace/name")
+	}
+	return types.NamespacedName{
+		Namespace: parts[0],
+		Name:      parts[1],
+	}, nil
+}
+
+func gatewayAPIControllerNameFromFlagValue(flagValue string) (string, error) {
+	if !isControllerNameValid(flagValue) {
+		return "", errors.New("the expected format is example.com/controller-name")
+	}
+	return flagValue, nil
+}

internal/manager/config_validation.go

@@ -0,0 +1,77 @@
+package manager
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/types"
+
+	"github.com/kong/kubernetes-ingress-controller/v2/internal/controllers/gateway"
+)
+
+func TestConfigValidatedVars(t *testing.T) {
+	type testCase struct {
+		Input                 string
+		ExpectedValue         any
+		ExtractValueFn        func(c Config) any
+		ExpectedErrorContains string
+	}
+
+	testCasesGroupedByFlag := map[string][]testCase{
+		"--gateway-api-controller-name": {
+			{
+				Input: "example.com/controller-name",
+				ExtractValueFn: func(c Config) any {
+					return c.GatewayAPIControllerName
+				},
+				ExpectedValue: "example.com/controller-name",
+			},
+			{
+				Input: "",
+				ExtractValueFn: func(c Config) any {
+					return c.GatewayAPIControllerName
+				},
+				ExpectedValue: string(gateway.ControllerName),
+			},
+			{
+				Input:                 "%invalid_controller_name$",
+				ExpectedErrorContains: "the expected format is example.com/controller-name",
+			},
+		},
+		"--publish-service": {
+			{
+				Input: "namespace/servicename",
+				ExtractValueFn: func(c Config) any {
+					return c.PublishService
+				},
+				ExpectedValue: types.NamespacedName{Namespace: "namespace", Name: "servicename"},
+			},
+			{
+				Input:                 "servicename",
+				ExpectedErrorContains: "the expected format is namespace/name",
+			},
+		},
+	}
+
+	for flag, flagTestCases := range testCasesGroupedByFlag {
+		for _, tc := range flagTestCases {
+			t.Run(fmt.Sprintf("%s=%s", flag, tc.Input), func(t *testing.T) {
+				var c Config
+				var input []string
+				if tc.Input != "" {
+					input = []string{flag, tc.Input}
+				}
+
+				err := c.FlagSet().Parse(input)
+				if tc.ExpectedErrorContains != "" {
+					require.ErrorContains(t, err, tc.ExpectedErrorContains)
+					return
+				}
+
+				require.NoError(t, err)
+				require.Equal(t, tc.ExpectedValue, tc.ExtractValueFn(c))
+			})
+		}
+	}
+}

internal/manager/config_validation_test.go

@@ -135,9 +135,6 @@ func Run(ctx context.Context, c *Config, diagnostic util.ConfigDumpDiagnostic, d
 		return err
 	}
 
-	if !isControllerNameValid(c.GatewayAPIControllerName) {
-		return errors.New("--gateway-api-controller-name is invalid. The expected format is example.com/controller-name")
-	}
 	gateway.ControllerName = gatewayv1beta1.GatewayController(c.GatewayAPIControllerName)
 
 	setupLog.Info("Starting Enabled Controllers")

internal/manager/flagnamespacedname.go

@@ -92,8 +92,8 @@ func setupControllerOptions(logger logr.Logger, c *Config, dbmode string) (ctrl.
 
 		// if publish service has been provided the namespace for it should be
 		// watched so that controllers can see updates to the service.
-		if c.PublishService.NN.String() != "" {
-			watchNamespaces = append(c.WatchNamespaces, c.PublishService.NN.Namespace)
+		if c.PublishService.String() != "" {
+			watchNamespaces = append(c.WatchNamespaces, c.PublishService.Namespace)
 		}
 		controllerOpts.NewCache = cache.MultiNamespacedCacheBuilder(watchNamespaces)
 	}
@@ -192,7 +192,7 @@ func setupDataplaneAddressFinder(ctx context.Context, mgrc client.Client, c *Con
 		if overrideAddrs := c.PublishStatusAddress; len(overrideAddrs) > 0 {
 			dataplaneAddressFinder.SetOverrides(overrideAddrs)
 		} else if c.PublishService.String() != "" {
-			publishServiceNn := c.PublishService.NN
+			publishServiceNn := c.PublishService
 			dataplaneAddressFinder.SetGetter(func() ([]string, error) {
 				svc := new(corev1.Service)
 				if err := mgrc.Get(ctx, publishServiceNn, svc); err != nil {

internal/manager/run.go

@@ -0,0 +1,50 @@
+package manager
+
+import "fmt"
+
+// ValidatedValue implements `pflag.Value` interface. It can be used for hooking up arbitrary validation logic to any type.
+// It should be passed to `pflag.FlagSet.Var()`.
+type ValidatedValue[T any] struct {
+	origin      string
+	variable    *T
+	constructor func(string) (T, error)
+}
+
+// NewValidatedValue creates a validated variable of type T. Constructor should validate the input and return an error
+// in case of any failures. If validation passes, constructor should return a value that's to be set in the variable.
+// The constructor accepts a flagValue that is raw input from user's command line (or an env variable that was bind to
+// the flag, see: bindEnvVars).
+func NewValidatedValue[T any](variable *T, constructor func(flagValue string) (T, error)) ValidatedValue[T] {
+	return ValidatedValue[T]{
+		constructor: constructor,
+		variable:    variable,
+	}
+}
+
+// NewValidatedValueWithDefault creates a validated variable of type T with a default value.
+func NewValidatedValueWithDefault[T any](variable *T, constructor func(flagValue string) (T, error), value T) ValidatedValue[T] {
+	*variable = value
+	return ValidatedValue[T]{
+		constructor: constructor,
+		variable:    variable,
+	}
+}
+
+func (v ValidatedValue[T]) String() string {
+	return v.origin
+}
+
+func (v ValidatedValue[T]) Set(s string) error {
+	value, err := v.constructor(s)
+	if err != nil {
+		return err
+	}
+
+	*v.variable = value
+	return nil
+}
+
+func (v ValidatedValue[T]) Type() string {
+	var t T
+	return fmt.Sprintf("Validated%T", t)
+}