address review comments

internal/manager/config.go

@@ -21,6 +21,9 @@ import (
 	"github.com/kong/kubernetes-ingress-controller/v2/internal/manager/featuregates"
 )
 
+// Type override to be used with types.NamespacedName variables to override their type name printed in the help text.
+var nnTypeNameOverride = WithTypeNameOverride[mo.Option[types.NamespacedName]]("namespacedName")
+
 // -----------------------------------------------------------------------------
 // Controller Manager - Config
 // -----------------------------------------------------------------------------
@@ -151,7 +154,7 @@ func (c *Config) FlagSet() *pflag.FlagSet {
 	flagSet.StringSliceVar(&c.KongAdminURLs, "kong-admin-url", []string{"http://localhost:8001"},
 		`Kong Admin URL(s) to connect to in the format "protocol://address:port". `+
 			`More than 1 URL can be provided, in such case the flag should be used multiple times or a corresponding env variable should use comma delimited addresses.`)
-	flagSet.Var(NewValidatedValue(&c.KongAdminSvc, namespacedNameFromFlagValue), "kong-admin-svc",
+	flagSet.Var(NewValidatedValue(&c.KongAdminSvc, namespacedNameFromFlagValue, nnTypeNameOverride), "kong-admin-svc",
 		`Kong Admin API Service namespaced name in "namespace/name" format, to use for Kong Gateway service discovery.`)
 	flagSet.StringSliceVar(&c.KondAdminSvcPortNames, "kong-admin-svc-port-names", []string{"admin", "admin-tls", "kong-admin", "kong-admin-tls"},
 		"Names of ports on Kong Admin API service to take into account when doing gateway discovery.")
@@ -170,7 +173,7 @@ func (c *Config) FlagSet() *pflag.FlagSet {
 	)
 
 	// Kubernetes configurations
-	flagSet.Var(NewValidatedValueWithDefault(&c.GatewayAPIControllerName, gatewayAPIControllerNameFromFlagValue, string(gateway.GetControllerName())), "gateway-api-controller-name", "The controller name to match on Gateway API resources.")
+	flagSet.Var(NewValidatedValue(&c.GatewayAPIControllerName, gatewayAPIControllerNameFromFlagValue, WithDefault(string(gateway.GetControllerName()))), "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.`)
@@ -181,12 +184,12 @@ 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(NewValidatedValue(&c.PublishService, namespacedNameFromFlagValue), "publish-service",
+	flagSet.Var(NewValidatedValue(&c.PublishService, namespacedNameFromFlagValue, nnTypeNameOverride), "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" `+
 			`when that Service lacks useful address information (for example, in bare-metal environments).`)
-	flagSet.Var(NewValidatedValue(&c.PublishServiceUDP, namespacedNameFromFlagValue), "publish-service-udp", `Service fronting UDP routing resources in `+
+	flagSet.Var(NewValidatedValue(&c.PublishServiceUDP, namespacedNameFromFlagValue, nnTypeNameOverride), "publish-service-udp", `Service fronting UDP routing resources in `+
 		`"namespace/name" format. The controller will update UDP route status information with this Service's `+
 		`endpoints. If omitted, the same Service will be used for both TCP and UDP routes.`)
 	flagSet.StringSliceVar(&c.PublishStatusAddressUDP, "publish-status-address-udp", []string{},

internal/manager/config_validation.go

@@ -20,6 +20,13 @@ func namespacedNameFromFlagValue(flagValue string) (mo.Option[types.NamespacedNa
 	if len(parts) != 2 {
 		return mo.Option[types.NamespacedName]{}, errors.New("the expected format is namespace/name")
 	}
+	if parts[0] == "" {
+		return mo.Option[types.NamespacedName]{}, errors.New("namespace cannot be empty")
+	}
+	if parts[1] == "" {
+		return mo.Option[types.NamespacedName]{}, errors.New("name cannot be empty")
+	}
+
 	return mo.Some(types.NamespacedName{
 		Namespace: parts[0],
 		Name:      parts[1],

internal/manager/config_validation_test.go

@@ -55,6 +55,23 @@ func TestConfigValidatedVars(t *testing.T) {
 				ExpectedErrorContains: "the expected format is namespace/name",
 			},
 		},
+		"--kong-admin-svc": {
+			{
+				Input: "namespace/servicename",
+				ExtractValueFn: func(c manager.Config) any {
+					return c.KongAdminSvc
+				},
+				ExpectedValue: mo.Some(types.NamespacedName{Namespace: "namespace", Name: "servicename"}),
+			},
+			{
+				Input:                 "namespace/",
+				ExpectedErrorContains: "name cannot be empty",
+			},
+			{
+				Input:                 "/name",
+				ExpectedErrorContains: "namespace cannot be empty",
+			},
+		},
 	}
 
 	for flag, flagTestCases := range testCasesGroupedByFlag {

internal/manager/setup.go

@@ -211,15 +211,15 @@ func setupDataplaneAddressFinder(mgrc client.Client, c *Config, log logr.Logger)
 	return defaultAddressFinder, udpAddressFinder, nil
 }
 
-func buildDataplaneAddressFinder(mgrc client.Client, publishStatusAddress []string, publishServiceNn mo.Option[types.NamespacedName]) (*dataplane.AddressFinder, error) {
+func buildDataplaneAddressFinder(mgrc client.Client, publishStatusAddress []string, publishServiceNN mo.Option[types.NamespacedName]) (*dataplane.AddressFinder, error) {
 	addressFinder := dataplane.NewAddressFinder()
 
 	if len(publishStatusAddress) > 0 {
 		addressFinder.SetOverrides(publishStatusAddress)
 		return addressFinder, nil
 	}
-	if serviceNn, ok := publishServiceNn.Get(); ok {
-		addressFinder.SetGetter(generateAddressFinderGetter(mgrc, serviceNn))
+	if serviceNN, ok := publishServiceNN.Get(); ok {
+		addressFinder.SetGetter(generateAddressFinderGetter(mgrc, serviceNN))
 		return addressFinder, nil
 	}
 
@@ -269,8 +269,8 @@ func (c *Config) adminAPIClients(ctx context.Context) ([]*adminapi.Client, error
 
 	// If kong-admin-svc flag has been specified then use it to get the list
 	// of Kong Admin API endpoints.
-	if adminSvc, ok := c.KongAdminSvc.Get(); ok {
-		return c.adminAPIClientFromServiceDiscovery(ctx, httpclient, adminSvc)
+	if c.KongAdminSvc.IsPresent() {
+		return c.adminAPIClientFromServiceDiscovery(ctx, httpclient)
 	}
 
 	// Otherwise fallback to the list of kong admin URLs.
@@ -288,12 +288,17 @@ func (c *Config) adminAPIClients(ctx context.Context) ([]*adminapi.Client, error
 	return clients, nil
 }
 
-func (c *Config) adminAPIClientFromServiceDiscovery(ctx context.Context, httpclient *http.Client, adminSvc types.NamespacedName) ([]*adminapi.Client, error) {
+func (c *Config) adminAPIClientFromServiceDiscovery(ctx context.Context, httpclient *http.Client) ([]*adminapi.Client, error) {
 	kubeClient, err := c.GetKubeClient()
 	if err != nil {
 		return nil, err
 	}
 
+	kongAdminSvcNN, ok := c.KongAdminSvc.Get()
+	if !ok {
+		return nil, errors.New("kong admin service namespaced name not provided")
+	}
+
 	// Retry this as we may encounter an error of getting 0 addresses,
 	// which can mean that Kong instances meant to be configured by this controller
 	// are not yet ready.
@@ -303,12 +308,12 @@ func (c *Config) adminAPIClientFromServiceDiscovery(ctx context.Context, httpcli
 	// configuration validation and sending code.
 	var adminAPIs []adminapi.DiscoveredAdminAPI
 	err = retry.Do(func() error {
-		s, err := adminapi.GetAdminAPIsForService(ctx, kubeClient, adminSvc, sets.New(c.KondAdminSvcPortNames...))
+		s, err := adminapi.GetAdminAPIsForService(ctx, kubeClient, kongAdminSvcNN, sets.New(c.KondAdminSvcPortNames...))
 		if err != nil {
 			return err
 		}
 		if s.Len() == 0 {
-			return fmt.Errorf("no endpoints for kong admin service: %q", adminSvc)
+			return fmt.Errorf("no endpoints for kong admin service: %q", kongAdminSvcNN)
 		}
 		adminAPIs = s.UnsortedList()
 		return nil

internal/manager/validated.go

@@ -2,32 +2,45 @@ package manager
 
 import "fmt"
 
+type ValidatedValueOpt[T any] func(*ValidatedValue[T])
+
+// WithDefault sets the default value for the validated variable.
+func WithDefault[T any](defaultValue T) ValidatedValueOpt[T] {
+	return func(v *ValidatedValue[T]) {
+		*v.variable = defaultValue
+	}
+}
+
+// WithTypeNameOverride overrides the type name that's printed in the help message.
+func WithTypeNameOverride[T any](typeName string) ValidatedValueOpt[T] {
+	return func(v *ValidatedValue[T]) {
+		v.typeName = typeName
+	}
+}
+
 // 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)
+	typeName    string
 }
 
 // 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]{
+// It accepts a variadic list of options that can be used e.g. to set the default value or override the type name.
+func NewValidatedValue[T any](variable *T, constructor func(flagValue string) (T, error), opts ...ValidatedValueOpt[T]) ValidatedValue[T] {
+	v := 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,
+	for _, opt := range opts {
+		opt(&v)
 	}
+	return v
 }
 
 func (v ValidatedValue[T]) String() string {
@@ -45,6 +58,10 @@ func (v ValidatedValue[T]) Set(s string) error {
 }
 
 func (v ValidatedValue[T]) Type() string {
+	if v.typeName != "" {
+		return v.typeName
+	}
+
 	var t T
-	return fmt.Sprintf("Validated%T", t)
+	return fmt.Sprintf("%T", t)
 }

internal/manager/validated_test.go

@@ -71,12 +71,12 @@ func TestValidatedValue(t *testing.T) {
 	t.Run("with default", func(t *testing.T) {
 		flags := flags()
 		var validatedString string
-		flags.Var(manager.NewValidatedValueWithDefault(&validatedString, func(s string) (string, error) {
+		flags.Var(manager.NewValidatedValue(&validatedString, func(s string) (string, error) {
 			if !strings.Contains(s, "magic-token") {
 				return "", errors.New("no magic token passed")
 			}
 			return s, nil
-		}, "default-value"), "flag-with-default", "")
+		}, manager.WithDefault("default-value")), "flag-with-default", "")
 
 		t.Run("empty", func(t *testing.T) {
 			err := flags.Parse(nil)
@@ -101,3 +101,31 @@ func TestValidatedValue(t *testing.T) {
 		})
 	})
 }
+
+func TestValidatedValue_Type(t *testing.T) {
+	t.Run("string", func(t *testing.T) {
+		var validatedString string
+		vv := manager.NewValidatedValue(&validatedString, func(s string) (string, error) {
+			return s, nil
+		})
+		require.Equal(t, "string", vv.Type())
+	})
+
+	t.Run("struct", func(t *testing.T) {
+		type customType struct{}
+		var customTypeVar customType
+		vv := manager.NewValidatedValue(&customTypeVar, func(s string) (customType, error) {
+			return customType{}, nil
+		})
+		require.Equal(t, "manager_test.customType", vv.Type())
+	})
+
+	t.Run("overridden type name", func(t *testing.T) {
+		type customType struct{}
+		var customTypeVar customType
+		vv := manager.NewValidatedValue(&customTypeVar, func(s string) (customType, error) {
+			return customType{}, nil
+		}, manager.WithTypeNameOverride[customType]("custom-type-override"))
+		require.Equal(t, "custom-type-override", vv.Type())
+	})
+}