From 16ed0fdb0507a4ef16f9b8de33ad1ba6d8c6508c Mon Sep 17 00:00:00 2001 From: nginx-bot <68849795+nginx-bot@users.noreply.github.com> Date: Mon, 16 Dec 2024 06:09:46 -0800 Subject: [PATCH] Expand mgmt configmap tests (#7000) --- internal/configs/configmaps.go | 14 +- internal/configs/configmaps_test.go | 338 +++++++++++++++++++++++++ internal/validation/validation.go | 45 ++++ internal/validation/validation_test.go | 83 ++++++ pkg/apis/dos/validation/dos.go | 15 +- pkg/apis/dos/validation/dos_test.go | 32 --- 6 files changed, 481 insertions(+), 46 deletions(-) create mode 100644 internal/validation/validation.go create mode 100644 internal/validation/validation_test.go diff --git a/internal/configs/configmaps.go b/internal/configs/configmaps.go index ef37831b21..beb53abb06 100644 --- 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 index 084c2f25d8..e6bea6af79 100644 --- a/internal/configs/configmaps_test.go +++ b/internal/configs/configmaps_test.go @@ -342,6 +342,78 @@ func TestParseMGMTConfigMapWarnings(t *testing.T) { }, msg: "enforce-initial-report set empty", }, + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "usage-report-interval": "", + }, + }, + msg: "usage-report-interval set empty", + }, + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "usage-report-interval": "1s", + }, + }, + msg: "usage-report-interval set below allowed value", + }, + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "usage-report-interval": "1s", + }, + }, + msg: "usage-report-interval set below allowed value", + }, + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "ssl-verify": "10", + }, + }, + msg: "ssl-verify set to an invalid int", + }, + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "ssl-verify": "test", + }, + }, + msg: "ssl-verify set to an invalid value", + }, + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "ssl-verify": "", + }, + }, + msg: "ssl-verify set to an empty string", + }, + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "resolver-ipv6": "", + }, + }, + msg: "resolver-ipv6 set to an empty string", + }, + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "resolver-ipv6": "10", + }, + }, + msg: "resolver-ipv6 set to an invalid int", + }, } for _, test := range tests { @@ -450,6 +522,272 @@ func TestParseMGMTConfigMapEnforceInitialReport(t *testing.T) { } } +func TestParseMGMTConfigMapSSLVerify(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", + "ssl-verify": "false", + }, + }, + want: &MGMTConfigParams{ + SSLVerify: BoolToPointerBool(false), + Secrets: MGMTSecrets{ + License: "license-token", + }, + }, + msg: "ssl-verify set to false", + }, + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "ssl-verify": "true", + }, + }, + want: &MGMTConfigParams{ + SSLVerify: BoolToPointerBool(true), + Secrets: MGMTSecrets{ + License: "license-token", + }, + }, + msg: "ssl-verify set to true", + }, + } + + 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.SSLVerify == nil { + t.Errorf("ssl-verify: want %v, got nil", *test.want.SSLVerify) + } + if *result.SSLVerify != *test.want.SSLVerify { + t.Errorf("ssl-verify: want %v, got %v", *test.want.SSLVerify, *result.SSLVerify) + } + }) + } +} + +func TestParseMGMTConfigMapUsageReportInterval(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-interval": "120s", + }, + }, + want: &MGMTConfigParams{ + Interval: "120s", + Secrets: MGMTSecrets{ + License: "license-token", + }, + }, + msg: "usage report interval set to 120s", + }, + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "usage-report-interval": "20m", + }, + }, + want: &MGMTConfigParams{ + Interval: "20m", + Secrets: MGMTSecrets{ + License: "license-token", + }, + }, + msg: "usage report interval set to 20m", + }, + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "usage-report-interval": "1h", + }, + }, + want: &MGMTConfigParams{ + Interval: "1h", + Secrets: MGMTSecrets{ + License: "license-token", + }, + }, + msg: "usage report interval set to 1h", + }, + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "usage-report-interval": "24h", + }, + }, + want: &MGMTConfigParams{ + Interval: "24h", + Secrets: MGMTSecrets{ + License: "license-token", + }, + }, + msg: "usage report interval set to 24h", + }, + } + + 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.Interval == "" { + t.Errorf("UsageReportInterval: want %s, got empty string", test.want.Interval) + } + if result.Interval != test.want.Interval { + t.Errorf("UsageReportInterval: want %v, got %v", test.want.Interval, result.Interval) + } + }) + } +} + +func TestParseMGMTConfigMapResolverIPV6(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", + "resolver-ipv6": "false", + }, + }, + want: &MGMTConfigParams{ + ResolverIPV6: BoolToPointerBool(false), + Secrets: MGMTSecrets{ + License: "license-token", + }, + }, + msg: "resolver-ipv6 set to false", + }, + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "resolver-ipv6": "true", + }, + }, + want: &MGMTConfigParams{ + ResolverIPV6: BoolToPointerBool(true), + Secrets: MGMTSecrets{ + License: "license-token", + }, + }, + msg: "resolver-ipv6 set to true", + }, + } + + 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.ResolverIPV6 == nil { + t.Errorf("resolver-ipv6: want %v, got nil", *test.want.ResolverIPV6) + } + 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 new file mode 100644 index 0000000000..bfeaaef452 --- /dev/null +++ b/internal/validation/validation.go @@ -0,0 +1,45 @@ +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(`^(?:(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9][0-9]|[0-9])\.){3}(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9][0-9]|[0-9])(?::\d{1,5})?$`) + validHostnameRegex = regexp.MustCompile(`^[a-z][A-Za-z0-9-]{1,62}(?::\d{1,5})?$`) +) + +// ValidatePort ensure port matches rfc6335 https://www.rfc-editor.org/rfc/rfc6335.html +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 +} + +// ValidateHost ensures the host is a valid hostname/IP address or FQDN with an optional :port appended +func ValidateHost(host string) error { + if host == "" { + return fmt.Errorf("error parsing host: empty host") + } + + if validIPRegex.MatchString(host) || validDNSRegex.MatchString(host) || validHostnameRegex.MatchString(host) { + chunks := strings.Split(host, ":") + if len(chunks) > 1 { + err := ValidatePort(chunks[1]) + if err != nil { + return fmt.Errorf("invalid port: %w", err) + } + } + return nil + } + return fmt.Errorf("error parsing host: %s not a valid host", host) +} diff --git a/internal/validation/validation_test.go b/internal/validation/validation_test.go new file mode 100644 index 0000000000..9ed4cf6b2c --- /dev/null +++ b/internal/validation/validation_test.go @@ -0,0 +1,83 @@ +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:443", + "10.10.1.1", + "123.112.224.43:443", + "172.120.3.222", + "localhost:80", + "localhost", + "myhost:54321", + "myhost", + "my-host:54321", + "my-host", + "dns.test.svc.cluster.local:8443", + "cluster.local:8443", + "product.example.com", + "product.example.com:443", + } + + // Negative test cases item, expected error message + negHosts := [][]string{ + {"NotValid", "not a valid host"}, + {"-cluster.local:514", "not a valid host"}, + {"10.10.1.1:99999", "not a valid port number"}, + {"333.333.333.333", "not a valid host"}, + } + + 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 index 6ab0b8c4d9..9f3970fc3e 100644 --- a/pkg/apis/dos/validation/dos.go +++ b/pkg/apis/dos/validation/dos.go @@ -5,9 +5,9 @@ import ( "net" "net/url" "regexp" - "strconv" "strings" + internalValidation "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" @@ -128,7 +128,7 @@ func validateAppProtectDosLogDest(dstAntn string) error { } if validIPRegex.MatchString(dstAntn) || validDNSRegex.MatchString(dstAntn) || validLocalhostRegex.MatchString(dstAntn) { chunks := strings.Split(dstAntn, ":") - err := validatePort(chunks[1]) + err := internalValidation.ValidatePort(chunks[1]) if err != nil { return fmt.Errorf("invalid log destination: %w", err) } @@ -137,17 +137,6 @@ func validateAppProtectDosLogDest(dstAntn string) error { return fmt.Errorf("invalid log destination: %s, must follow format: : or stderr", dstAntn) } -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 validateAppProtectDosName(name string) error { if len(name) > maxNameLength { return fmt.Errorf("app Protect Dos Name max length is %v", maxNameLength) diff --git a/pkg/apis/dos/validation/dos_test.go b/pkg/apis/dos/validation/dos_test.go index 550abc6a85..6fa90a6766 100644 --- a/pkg/apis/dos/validation/dos_test.go +++ b/pkg/apis/dos/validation/dos_test.go @@ -159,7 +159,6 @@ func TestValidateDosProtectedResource(t *testing.T) { msg: "DosSecurityLog with valid apDosLogConf", }, } - for _, test := range tests { err := ValidateDosProtectedResource(test.protected) if err != nil { @@ -203,7 +202,6 @@ func TestValidateAppProtectDosAccessLogDest(t *testing.T) { t.Errorf("expected nil, got %v", err) } } - for _, nTCase := range negDstAntns { err := validateAppProtectDosLogDest(nTCase[0]) if err == nil { @@ -450,36 +448,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()