add validation package and tests for usage report endpoint

AlexFenlon · AlexFenlon · commit 95672a352b92 · 2024-12-12T16:45:47.000Z
diff --git a/internal/configs/configmaps.go b/internal/configs/configmaps.go
@@ -12,6 +12,7 @@ import (
 
 	"github.com/nginxinc/kubernetes-ingress/internal/configs/version1"
 	nl "github.com/nginxinc/kubernetes-ingress/internal/logger"
+	"github.com/nginxinc/kubernetes-ingress/internal/validation"
 )
 
 const (
@@ -714,9 +715,20 @@ func ParseMGMTConfigMap(ctx context.Context, cfgm *v1.ConfigMap, eventLog record
 			mgmtCfgParams.EnforceInitialReport = BoolToPointerBool(enforceInitialReport)
 		}
 	}
+
 	if endpoint, exists := cfgm.Data["usage-report-endpoint"]; exists {
-		mgmtCfgParams.Endpoint = strings.TrimSpace(endpoint)
+		endpoint := strings.TrimSpace(endpoint)
+		err := validation.ValidateHost(endpoint)
+		if err != nil {
+			errorText := fmt.Sprintf("Configmap %s/%s: Invalid value for the usage-report-endpoint key: got %q: %v. Using default endpoint.", cfgm.GetNamespace(), cfgm.GetName(), endpoint, err)
+			nl.Error(l, errorText)
+			eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, errorText)
+			configWarnings = true
+		} else {
+			mgmtCfgParams.Endpoint = strings.TrimSpace(endpoint)
+		}
 	}
+
 	if interval, exists := cfgm.Data["usage-report-interval"]; exists {
 		i := strings.TrimSpace(interval)
 		t, err := time.ParseDuration(i)
diff --git a/internal/configs/configmaps_test.go b/internal/configs/configmaps_test.go
@@ -722,13 +722,72 @@ func TestParseMGMTConfigMapResolverIPV6(t *testing.T) {
 			if result.ResolverIPV6 == nil {
 				t.Errorf("resolver-ipv6: want %v, got nil", *test.want.ResolverIPV6)
 			}
-			if *result.SSLVerify != *test.want.SSLVerify {
+			if *result.ResolverIPV6 != *test.want.ResolverIPV6 {
 				t.Errorf("resolver-ipv6: want %v, got %v", *test.want.ResolverIPV6, *result.ResolverIPV6)
 			}
 		})
 	}
 }
 
+func TestParseMGMTConfigMapUsageReportEndpoint(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		configMap *v1.ConfigMap
+		want      *MGMTConfigParams
+		msg       string
+	}{
+		{
+			configMap: &v1.ConfigMap{
+				Data: map[string]string{
+					"license-token-secret-name": "license-token",
+					"usage-report-endpoint":     "product.connect.nginx.com",
+				},
+			},
+			want: &MGMTConfigParams{
+				Endpoint: "product.connect.nginx.com",
+				Secrets: MGMTSecrets{
+					License: "license-token",
+				},
+			},
+			msg: "usage report endpoint set to product.connect.nginx.com",
+		},
+		{
+			configMap: &v1.ConfigMap{
+				Data: map[string]string{
+					"license-token-secret-name": "license-token",
+					"usage-report-endpoint":     "product.connect.nginx.com:80",
+				},
+			},
+			want: &MGMTConfigParams{
+				Endpoint: "product.connect.nginx.com:80",
+				Secrets: MGMTSecrets{
+					License: "license-token",
+				},
+			},
+			msg: "usage report endpoint set to product.connect.nginx.com with port 80",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.msg, func(t *testing.T) {
+			result, warnings, err := ParseMGMTConfigMap(context.Background(), test.configMap, makeEventLogger())
+			if err != nil {
+				t.Fatal(err)
+			}
+			if warnings {
+				t.Error("Unexpected warnings")
+			}
+
+			if result.Endpoint == "" {
+				t.Errorf("UsageReportEndpoint: want %s, got empty string", test.want.Endpoint)
+			}
+			if result.Endpoint != test.want.Endpoint {
+				t.Errorf("UsageReportEndpoint: want %v, got %v", test.want.Endpoint, result.Endpoint)
+			}
+		})
+	}
+}
+
 func makeEventLogger() record.EventRecorder {
 	return record.NewFakeRecorder(1024)
 }
diff --git a/internal/validation/validation.go b/internal/validation/validation.go
@@ -0,0 +1,41 @@
+package validation
+
+import (
+	"fmt"
+	"regexp"
+	"strconv"
+	"strings"
+)
+
+var (
+	validDNSRegex       = regexp.MustCompile(`^(?:[A-Za-z0-9][A-Za-z0-9-]{1,62}\.)([A-Za-z0-9-]{1,63}\.)*[A-Za-z]{2,6}(?::\d{1,5})?$`)
+	validIPRegex        = regexp.MustCompile(`^(\d{1,3}\.){3}\d{1,3}(?::\d{1,5})?$`)
+	validLocalhostRegex = regexp.MustCompile(`^localhost(?::\d{1,5})?$`)
+)
+
+func ValidatePort(value string) error {
+	port, err := strconv.Atoi(value)
+	if err != nil {
+		return fmt.Errorf("error parsing port number: %w", err)
+	}
+	if port > 65535 || port < 1 {
+		return fmt.Errorf("error parsing port: %v not a valid port number", port)
+	}
+	return nil
+}
+
+func ValidateHost(host string) error {
+	if host == "" {
+		return fmt.Errorf("error parsing host: empty host")
+	}
+
+	if validIPRegex.MatchString(host) || validDNSRegex.MatchString(host) || validLocalhostRegex.MatchString(host) {
+		chunks := strings.Split(host, ":")
+		err := ValidatePort(chunks[1])
+		if err != nil {
+			return fmt.Errorf("invalid port: %w", err)
+		}
+		return nil
+	}
+	return fmt.Errorf("invalid host: %s", host)
+}
diff --git a/internal/validation/validation_test.go b/internal/validation/validation_test.go
@@ -0,0 +1,75 @@
+package validation
+
+import (
+	"strings"
+	"testing"
+)
+
+func TestValidatePort_IsValidOnValidInput(t *testing.T) {
+	t.Parallel()
+
+	ports := []string{"1", "65535"}
+	for _, p := range ports {
+		if err := ValidatePort(p); err != nil {
+			t.Error(err)
+		}
+	}
+}
+
+func TestValidatePort_ErrorsOnInvalidString(t *testing.T) {
+	t.Parallel()
+
+	if err := ValidatePort(""); err == nil {
+		t.Error("want error, got nil")
+	}
+}
+
+func TestValidatePort_ErrorsOnInvalidRange(t *testing.T) {
+	t.Parallel()
+
+	ports := []string{"0", "-1", "65536"}
+	for _, p := range ports {
+		if err := ValidatePort(p); err == nil {
+			t.Error("want error, got nil")
+		}
+	}
+}
+
+func TestValidateHost(t *testing.T) {
+	t.Parallel()
+	// Positive test cases
+	posHosts := []string{
+		"10.10.1.1:514",
+		"localhost:514",
+		"dns.test.svc.cluster.local:514",
+		"cluster.local:514",
+		"dash-test.cluster.local:514",
+		"product.example.com",
+	}
+
+	// Negative test cases item, expected error message
+	negHosts := [][]string{
+		{"NotValid", "invalid host: NotValid"},
+		{"cluster.local", "invalid host: cluster.local"},
+		{"-cluster.local:514", "invalid host: -cluster.local:514"},
+		{"10.10.1.1:99999", "not a valid port number"},
+	}
+
+	for _, tCase := range posHosts {
+		err := ValidateHost(tCase)
+		if err != nil {
+			t.Errorf("expected nil, got %v", err)
+		}
+	}
+
+	for _, nTCase := range negHosts {
+		err := ValidateHost(nTCase[0])
+		if err == nil {
+			t.Errorf("got no error expected error containing '%s'", nTCase[1])
+		} else {
+			if !strings.Contains(err.Error(), nTCase[1]) {
+				t.Errorf("got '%v', expected: '%s'", err, nTCase[1])
+			}
+		}
+	}
+}
diff --git a/pkg/apis/dos/validation/dos.go b/pkg/apis/dos/validation/dos.go
@@ -2,17 +2,14 @@ package validation
 
 import (
 	"fmt"
-	"net"
-	"net/url"
-	"regexp"
-	"strconv"
-	"strings"
-
+	k8svalidation "github.com/nginxinc/kubernetes-ingress/internal/validation"
 	validation2 "github.com/nginxinc/kubernetes-ingress/pkg/apis/configuration/validation"
 	"github.com/nginxinc/kubernetes-ingress/pkg/apis/dos/v1beta1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	"net"
+	"net/url"
 )
 
 var appProtectDosPolicyRequiredFields = [][]string{
@@ -116,34 +113,13 @@ func ValidateAppProtectDosLogConf(logConf *unstructured.Unstructured) (string, e
 	return warning, nil
 }
 
-var (
-	validDNSRegex       = regexp.MustCompile(`^([A-Za-z0-9][A-Za-z0-9-]{1,62}\.)([A-Za-z0-9-]{1,63}\.)*[A-Za-z]{2,6}:\d{1,5}$`)
-	validIPRegex        = regexp.MustCompile(`^(\d{1,3}\.){3}\d{1,3}:\d{1,5}$`)
-	validLocalhostRegex = regexp.MustCompile(`^localhost:\d{1,5}$`)
-)
-
 func validateAppProtectDosLogDest(dstAntn string) error {
 	if dstAntn == "stderr" {
 		return nil
 	}
-	if validIPRegex.MatchString(dstAntn) || validDNSRegex.MatchString(dstAntn) || validLocalhostRegex.MatchString(dstAntn) {
-		chunks := strings.Split(dstAntn, ":")
-		err := validatePort(chunks[1])
-		if err != nil {
-			return fmt.Errorf("invalid log destination: %w", err)
-		}
-		return nil
-	}
-	return fmt.Errorf("invalid log destination: %s, must follow format: <ip-address | localhost | dns name>:<port> or stderr", dstAntn)
-}
-
-func validatePort(value string) error {
-	port, err := strconv.Atoi(value)
+	err := k8svalidation.ValidateHost(dstAntn)
 	if err != nil {
-		return fmt.Errorf("error parsing port number: %w", err)
-	}
-	if port > 65535 || port < 1 {
-		return fmt.Errorf("error parsing port: %v not a valid port number", port)
+		return fmt.Errorf("%w, must follow format: <ip-address | localhost | dns name>:<port> or stderr", err)
 	}
 	return nil
 }
diff --git a/pkg/apis/dos/validation/dos_test.go b/pkg/apis/dos/validation/dos_test.go
@@ -63,7 +63,7 @@ func TestValidateDosProtectedResource(t *testing.T) {
 					DosAccessLogDest: "bad&$%^logdest",
 				},
 			},
-			expectErr: "error validating DosProtectedResource:  invalid field: dosAccessLogDest err: invalid log destination: bad&$%^logdest, must follow format: <ip-address | localhost | dns name>:<port> or stderr",
+			expectErr: "error validating DosProtectedResource:  invalid field: dosAccessLogDest err: invalid host: bad&$%^logdest, must follow format: <ip-address | localhost | dns name>:<port> or stderr",
 			msg:       "invalid DosAccessLogDest specified",
 		},
 		{
@@ -105,7 +105,7 @@ func TestValidateDosProtectedResource(t *testing.T) {
 					DosSecurityLog:   &v1beta1.DosSecurityLog{},
 				},
 			},
-			expectErr: "error validating DosProtectedResource:  invalid field: dosSecurityLog/dosLogDest err: invalid log destination: , must follow format: <ip-address | localhost | dns name>:<port> or stderr",
+			expectErr: "error validating DosProtectedResource:  invalid field: dosSecurityLog/dosLogDest err: error parsing host: empty host, must follow format: <ip-address | localhost | dns name>:<port> or stderr",
 			msg:       "empty DosSecurityLog specified",
 		},
 		{
@@ -191,9 +191,9 @@ func TestValidateAppProtectDosAccessLogDest(t *testing.T) {
 
 	// Negative test cases item, expected error message
 	negDstAntns := [][]string{
-		{"NotValid", "invalid log destination: NotValid, must follow format: <ip-address | localhost | dns name>:<port> or stderr"},
-		{"cluster.local", "invalid log destination: cluster.local, must follow format: <ip-address | localhost | dns name>:<port> or stderr"},
-		{"-cluster.local:514", "invalid log destination: -cluster.local:514, must follow format: <ip-address | localhost | dns name>:<port> or stderr"},
+		{"NotValid", "invalid host: NotValid, must follow format: <ip-address | localhost | dns name>:<port> or stderr"},
+		{"cluster.local", "invalid host: cluster.local, must follow format: <ip-address | localhost | dns name>:<port> or stderr"},
+		{"-cluster.local:514", "invalid host: -cluster.local:514, must follow format: <ip-address | localhost | dns name>:<port> or stderr"},
 		{"10.10.1.1:99999", "not a valid port number"},
 	}
 
@@ -450,36 +450,6 @@ func TestValidateAppProtectDosMonitor(t *testing.T) {
 	}
 }
 
-func TestValidatePort_IsValidOnValidInput(t *testing.T) {
-	t.Parallel()
-
-	ports := []string{"1", "65535"}
-	for _, p := range ports {
-		if err := validatePort(p); err != nil {
-			t.Error(err)
-		}
-	}
-}
-
-func TestValidatePort_ErrorsOnInvalidString(t *testing.T) {
-	t.Parallel()
-
-	if err := validatePort(""); err == nil {
-		t.Error("want error, got nil")
-	}
-}
-
-func TestValidatePort_ErrorsOnInvalidRange(t *testing.T) {
-	t.Parallel()
-
-	ports := []string{"0", "-1", "65536"}
-	for _, p := range ports {
-		if err := validatePort(p); err == nil {
-			t.Error("want error, got nil")
-		}
-	}
-}
-
 func TestValidateAppProtectDosLogDest_ValidOnDestinationStdErr(t *testing.T) {
 	t.Parallel()
 
