Separate validating types & review feedback

Author: ciarams87

cmd/gateway/commands.go

@@ -39,68 +39,20 @@ var (
 	}
 
 	// Backing values for static subcommand cli flags.
-	updateGCStatus    bool
-	disableMetrics    bool
-	metricsListenPort int
-	metricsSecure     bool
+	updateGCStatus bool
+	disableMetrics bool
+	metricsSecure  bool
 
+	metricsListenPort = intValidatingValue{
+		validator: validatePort,
+		value:     9113,
+	}
 	gateway    = namespacedNameValue{}
 	configName = stringValidatingValue{
 		validator: validateResourceName,
 	}
 )
 
-// stringValidatingValue is a string flag value with custom validation logic.
-// it implements the pflag.Value interface.
-type stringValidatingValue struct {
-	validator func(v string) error
-	value     string
-}
-
-func (v *stringValidatingValue) String() string {
-	return v.value
-}
-
-func (v *stringValidatingValue) Set(param string) error {
-	if err := v.validator(param); err != nil {
-		return err
-	}
-	v.value = param
-	return nil
-}
-
-func (v *stringValidatingValue) Type() string {
-	return "string"
-}
-
-// namespacedNameValue is a string flag value that represents a namespaced name.
-// it implements the pflag.Value interface.
-type namespacedNameValue struct {
-	value types.NamespacedName
-}
-
-func (v *namespacedNameValue) String() string {
-	if (v.value == types.NamespacedName{}) {
-		// if we don't do that, the default value in the help message will be printed as "/"
-		return ""
-	}
-	return v.value.String()
-}
-
-func (v *namespacedNameValue) Set(param string) error {
-	nsname, err := parseNamespacedResourceName(param)
-	if err != nil {
-		return err
-	}
-
-	v.value = nsname
-	return nil
-}
-
-func (v *namespacedNameValue) Type() string {
-	return "string"
-}
-
 func createRootCommand() *cobra.Command {
 	rootCmd := &cobra.Command{
 		Use: "gateway",
@@ -158,11 +110,8 @@ func createStaticModeCommand() *cobra.Command {
 
 			metricsConfig := config.MetricsConfig{}
 			if !disableMetrics {
-				if err := validatePort(metricsListenPort); err != nil {
-					return fmt.Errorf("error validating metrics listen port: %w", err)
-				}
 				metricsConfig.Enabled = true
-				metricsConfig.Port = metricsListenPort
+				metricsConfig.Port = metricsListenPort.value
 				metricsConfig.Secure = metricsSecure
 			}
 
@@ -219,10 +168,9 @@ func createStaticModeCommand() *cobra.Command {
 		"Disable exposing metrics in the Prometheus format.",
 	)
 
-	cmd.Flags().IntVar(
+	cmd.Flags().Var(
 		&metricsListenPort,
 		"metrics-port",
-		9113,
 		"Set the port where the metrics are exposed. Format: [1023 - 65535]",
 	)
 

cmd/gateway/commands_test.go

@@ -118,6 +118,9 @@ func TestStaticModeCmdFlagValidation(t *testing.T) {
 				"--gateway=nginx-gateway/nginx",
 				"--config=nginx-gateway-config",
 				"--update-gatewayclass-status=true",
+				"--metrics-port=9114",
+				"--metrics-disable",
+				"--metrics-secure-serving",
 			},
 			wantErr: false,
 		},
@@ -175,6 +178,42 @@ func TestStaticModeCmdFlagValidation(t *testing.T) {
 			wantErr:           true,
 			expectedErrPrefix: `invalid argument "invalid" for "--update-gatewayclass-status" flag: strconv.ParseBool`,
 		},
+		{
+			name: "metrics-port is invalid type",
+			args: []string{
+				"--metrics-port=invalid", // not an int
+			},
+			wantErr: true,
+			expectedErrPrefix: `invalid argument "invalid" for "--metrics-port" flag: failed to parse int value:` +
+				` strconv.ParseInt: parsing "invalid": invalid syntax`,
+		},
+		{
+			name: "metrics-port is outside of range",
+			args: []string{
+				"--metrics-port=999", // outside of range
+			},
+			wantErr: true,
+			expectedErrPrefix: `invalid argument "999" for "--metrics-port" flag:` +
+				` port outside of valid port range [1024 - 65535]: 999`,
+		},
+		{
+			name: "metrics-disable is not a bool",
+			args: []string{
+				"--metrics-disable=999", // not a bool
+			},
+			wantErr: true,
+			expectedErrPrefix: `invalid argument "999" for "--metrics-disable" flag: strconv.ParseBool:` +
+				` parsing "999": invalid syntax`,
+		},
+		{
+			name: "metrics-secure-serving is not a bool",
+			args: []string{
+				"--metrics-secure-serving=999", // not a bool
+			},
+			wantErr: true,
+			expectedErrPrefix: `invalid argument "999" for "--metrics-secure-serving" flag: strconv.ParseBool:` +
+				` parsing "999": invalid syntax`,
+		},
 	}
 
 	for _, test := range tests {

cmd/gateway/validating_types.go

@@ -0,0 +1,86 @@
+package main
+
+import (
+	"fmt"
+	"strconv"
+
+	"k8s.io/apimachinery/pkg/types"
+)
+
+// stringValidatingValue is a string flag value with custom validation logic.
+// it implements the pflag.Value interface.
+type stringValidatingValue struct {
+	validator func(v string) error
+	value     string
+}
+
+func (v *stringValidatingValue) String() string {
+	return v.value
+}
+
+func (v *stringValidatingValue) Set(param string) error {
+	if err := v.validator(param); err != nil {
+		return err
+	}
+	v.value = param
+	return nil
+}
+
+func (v *stringValidatingValue) Type() string {
+	return "string"
+}
+
+type intValidatingValue struct {
+	validator func(v int) error
+	value     int
+}
+
+func (v *intValidatingValue) String() string {
+	return strconv.Itoa(v.value)
+}
+
+func (v *intValidatingValue) Set(param string) error {
+	intVal, err := strconv.ParseInt(param, 10, 32)
+	if err != nil {
+		return fmt.Errorf("failed to parse int value: %w", err)
+	}
+
+	if err := v.validator(int(intVal)); err != nil {
+		return err
+	}
+
+	v.value = int(intVal)
+	return nil
+}
+
+func (v *intValidatingValue) Type() string {
+	return "int"
+}
+
+// namespacedNameValue is a string flag value that represents a namespaced name.
+// it implements the pflag.Value interface.
+type namespacedNameValue struct {
+	value types.NamespacedName
+}
+
+func (v *namespacedNameValue) String() string {
+	if (v.value == types.NamespacedName{}) {
+		// if we don't do that, the default value in the help message will be printed as "/"
+		return ""
+	}
+	return v.value.String()
+}
+
+func (v *namespacedNameValue) Set(param string) error {
+	nsname, err := parseNamespacedResourceName(param)
+	if err != nil {
+		return err
+	}
+
+	v.value = nsname
+	return nil
+}
+
+func (v *namespacedNameValue) Type() string {
+	return "string"
+}

docs/architecture.md

@@ -96,7 +96,7 @@ parentheses. To enhance readability, the suffix "process" has been omitted from
 
 1. (HTTPS) *NKG* reads the *Kubernetes API* to get the latest versions of the resources in the cluster and writes to the
 API to update the handled resources' statuses and emit events.
-2. (HTTP) *Prometheus* fetches the `controller-runtime` and NGINX metrics via an HTTP endpoint that *NKG* exposes.
+2. (HTTP, HTTPS) *Prometheus* fetches the `controller-runtime` and NGINX metrics via an HTTP endpoint that *NKG* exposes.
    The default is :9113/metrics. Note: Prometheus is not required by NKG, the endpoint can be turned off.
 3. (File I/O)
    - Write: *NKG* generates NGINX *configuration* based on the cluster resources and writes them as `.conf` files to the

internal/mode/static/nginx/runtime/manager.go

@@ -70,6 +70,7 @@ func (m *ManagerImpl) Reload(ctx context.Context) error {
 	return nil
 }
 
+// EnsureNginxRunning ensures NGINX is running by locating the main process.
 func EnsureNginxRunning(ctx context.Context) error {
 	if _, err := findMainProcess(ctx, os.Stat, os.ReadFile, pidFileTimeout); err != nil {
 		return fmt.Errorf("failed to find NGINX main process: %w", err)

internal/mode/static/state/graph/gateway_listener_test.go

@@ -114,7 +114,7 @@ func TestValidateHTTPSListener(t *testing.T) {
 			l: v1beta1.Listener{
 				Port: 9113,
 				TLS: &v1beta1.GatewayTLSConfig{
-					Mode:            helpers.GetTLSModePointer(v1beta1.TLSModeTerminate),
+					Mode:            helpers.GetPointer(v1beta1.TLSModeTerminate),
 					CertificateRefs: []v1beta1.SecretObjectReference{validSecretRef},
 				},
 			},