From a0c3531756bfbd24e62804daf909cdc318d697a9 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Thu, 12 Dec 2024 11:09:29 +0000 Subject: [PATCH 1/4] add tests for ssl-verify, resolver-ipv6 and usage-report-interval --- internal/configs/configmaps_test.go | 279 ++++++++++++++++++++++++++++ 1 file changed, 279 insertions(+) diff --git a/internal/configs/configmaps_test.go b/internal/configs/configmaps_test.go index 084c2f25d8..8a7481627f 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,213 @@ 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.SSLVerify != *test.want.SSLVerify { + t.Errorf("resolver-ipv6: want %v, got %v", *test.want.ResolverIPV6, *result.ResolverIPV6) + } + }) + } +} + func makeEventLogger() record.EventRecorder { return record.NewFakeRecorder(1024) } From 95672a352b92b77a291b530ee802b204ff5b8500 Mon Sep 17 00:00:00 2001 From: Alex Fenlon Date: Thu, 12 Dec 2024 16:45:47 +0000 Subject: [PATCH 2/4] add validation package and tests for usage report endpoint --- internal/configs/configmaps.go | 14 ++++- internal/configs/configmaps_test.go | 61 ++++++++++++++++++++- internal/validation/validation.go | 41 ++++++++++++++ internal/validation/validation_test.go | 75 ++++++++++++++++++++++++++ pkg/apis/dos/validation/dos.go | 34 ++---------- pkg/apis/dos/validation/dos_test.go | 40 ++------------ 6 files changed, 199 insertions(+), 66 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 8a7481627f..e6bea6af79 100644 --- 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 new file mode 100644 index 0000000000..f7c6953f8f --- /dev/null +++ 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 new file mode 100644 index 0000000000..d644856683 --- /dev/null +++ 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 index 6ab0b8c4d9..7274e03fd6 100644 --- 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: : 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: : or stderr", err) } return nil } diff --git a/pkg/apis/dos/validation/dos_test.go b/pkg/apis/dos/validation/dos_test.go index 550abc6a85..ba2bbecae3 100644 --- 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: : or stderr", + expectErr: "error validating DosProtectedResource: invalid field: dosAccessLogDest err: invalid host: bad&$%^logdest, must follow format: : 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: : or stderr", + expectErr: "error validating DosProtectedResource: invalid field: dosSecurityLog/dosLogDest err: error parsing host: empty host, must follow format: : 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: : or stderr"}, - {"cluster.local", "invalid log destination: cluster.local, must follow format: : or stderr"}, - {"-cluster.local:514", "invalid log destination: -cluster.local:514, must follow format: : or stderr"}, + {"NotValid", "invalid host: NotValid, must follow format: : or stderr"}, + {"cluster.local", "invalid host: cluster.local, must follow format: : or stderr"}, + {"-cluster.local:514", "invalid host: -cluster.local:514, must follow format: : 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() From 4a3e136151fbaf52a2b46e3deaf594b891dcacb5 Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Fri, 13 Dec 2024 14:52:49 +0000 Subject: [PATCH 3/4] update hostname/fqdn/ip validation --- internal/validation/validation.go | 20 +++-- internal/validation/validation_test.go | 24 ++++-- pkg/apis/dos/validation/dos.go | 27 +++++-- pkg/apis/dos/validation/dos_test.go | 103 ++----------------------- 4 files changed, 53 insertions(+), 121 deletions(-) diff --git a/internal/validation/validation.go b/internal/validation/validation.go index f7c6953f8f..bfeaaef452 100644 --- a/internal/validation/validation.go +++ b/internal/validation/validation.go @@ -8,11 +8,12 @@ import ( ) 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})?$`) + 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 { @@ -24,18 +25,21 @@ func ValidatePort(value string) error { 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) || validLocalhostRegex.MatchString(host) { + if validIPRegex.MatchString(host) || validDNSRegex.MatchString(host) || validHostnameRegex.MatchString(host) { chunks := strings.Split(host, ":") - err := ValidatePort(chunks[1]) - if err != nil { - return fmt.Errorf("invalid port: %w", err) + if len(chunks) > 1 { + err := ValidatePort(chunks[1]) + if err != nil { + return fmt.Errorf("invalid port: %w", err) + } } return nil } - return fmt.Errorf("invalid host: %s", host) + 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 index d644856683..9ed4cf6b2c 100644 --- a/internal/validation/validation_test.go +++ b/internal/validation/validation_test.go @@ -39,20 +39,28 @@ 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", + "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", "invalid host: NotValid"}, - {"cluster.local", "invalid host: cluster.local"}, - {"-cluster.local:514", "invalid host: -cluster.local:514"}, + {"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 { diff --git a/pkg/apis/dos/validation/dos.go b/pkg/apis/dos/validation/dos.go index 7274e03fd6..9f3970fc3e 100644 --- a/pkg/apis/dos/validation/dos.go +++ b/pkg/apis/dos/validation/dos.go @@ -2,14 +2,17 @@ package validation import ( "fmt" - k8svalidation "github.com/nginxinc/kubernetes-ingress/internal/validation" + "net" + "net/url" + "regexp" + "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" "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" - "net" - "net/url" ) var appProtectDosPolicyRequiredFields = [][]string{ @@ -113,15 +116,25 @@ 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 } - err := k8svalidation.ValidateHost(dstAntn) - if err != nil { - return fmt.Errorf("%w, must follow format: : or stderr", err) + if validIPRegex.MatchString(dstAntn) || validDNSRegex.MatchString(dstAntn) || validLocalhostRegex.MatchString(dstAntn) { + chunks := strings.Split(dstAntn, ":") + err := internalValidation.ValidatePort(chunks[1]) + if err != nil { + return fmt.Errorf("invalid log destination: %w", err) + } + return nil } - return nil + return fmt.Errorf("invalid log destination: %s, must follow format: : or stderr", dstAntn) } func validateAppProtectDosName(name string) error { diff --git a/pkg/apis/dos/validation/dos_test.go b/pkg/apis/dos/validation/dos_test.go index ba2bbecae3..b70dcd921c 100644 --- 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 host: bad&$%^logdest, must follow format: : or stderr", + expectErr: "error validating DosProtectedResource: invalid field: dosAccessLogDest err: invalid log destination: bad&$%^logdest, must follow format: : 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: error parsing host: empty host, must follow format: : or stderr", + expectErr: "error validating DosProtectedResource: invalid field: dosSecurityLog/dosLogDest err: invalid log destination: , must follow format: : or stderr", msg: "empty DosSecurityLog specified", }, { @@ -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 { @@ -191,9 +190,9 @@ func TestValidateAppProtectDosAccessLogDest(t *testing.T) { // Negative test cases item, expected error message negDstAntns := [][]string{ - {"NotValid", "invalid host: NotValid, must follow format: : or stderr"}, - {"cluster.local", "invalid host: cluster.local, must follow format: : or stderr"}, - {"-cluster.local:514", "invalid host: -cluster.local:514, must follow format: : or stderr"}, + {"NotValid", "invalid log destination: NotValid, must follow format: : or stderr"}, + {"cluster.local", "invalid log destination: cluster.local, must follow format: : or stderr"}, + {"-cluster.local:514", "invalid log destination: -cluster.local:514, must follow format: : or stderr"}, {"10.10.1.1:99999", "not a valid port number"}, } @@ -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 { @@ -216,97 +214,6 @@ func TestValidateAppProtectDosAccessLogDest(t *testing.T) { } } -func TestValidateAppProtectDosLogConf(t *testing.T) { - t.Parallel() - tests := []struct { - logConf *unstructured.Unstructured - expectErr bool - expectWarn bool - msg string - }{ - { - logConf: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "filter": map[string]interface{}{}, - }, - }, - }, - expectErr: false, - expectWarn: false, - msg: "valid log conf", - }, - { - logConf: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{}, - }, - }, - expectErr: true, - expectWarn: false, - msg: "invalid log conf with no filter field", - }, - { - logConf: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "something": map[string]interface{}{ - "filter": map[string]interface{}{}, - }, - }, - }, - expectErr: true, - expectWarn: false, - msg: "invalid log conf with no spec field", - }, - { - logConf: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "content": map[string]interface{}{ - "format": "user-defined", - }, - "filter": map[string]interface{}{}, - }, - }, - }, - expectErr: false, - expectWarn: true, - msg: "Support only splunk format", - }, - { - logConf: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "spec": map[string]interface{}{ - "filter": map[string]interface{}{}, - "content": map[string]interface{}{ - "format": "user-defined", - }, - }, - }, - }, - expectErr: false, - expectWarn: true, - msg: "valid log conf with warning filter field", - }, - } - - for _, test := range tests { - warn, err := ValidateAppProtectDosLogConf(test.logConf) - if test.expectErr && err == nil { - t.Errorf("validateAppProtectDosLogConf() returned no error for the case of %s", test.msg) - } - if !test.expectErr && err != nil { - t.Errorf("validateAppProtectDosLogConf() returned unexpected error %v for the case of %s", err, test.msg) - } - if test.expectWarn && warn == "" { - t.Errorf("validateAppProtectDosLogConf() returned no warning for the case of %s", test.msg) - } - if !test.expectWarn && warn != "" { - t.Errorf("validateAppProtectDosLogConf() returned unexpected warning: %s, for the case of %s", warn, test.msg) - } - } -} - func TestValidateAppProtectDosPolicy(t *testing.T) { t.Parallel() tests := []struct { From 6d39517fe517cadfb8fd676ee3c2bda52c6df900 Mon Sep 17 00:00:00 2001 From: Paul Abel Date: Fri, 13 Dec 2024 15:04:45 +0000 Subject: [PATCH 4/4] add removed test --- pkg/apis/dos/validation/dos_test.go | 91 +++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/pkg/apis/dos/validation/dos_test.go b/pkg/apis/dos/validation/dos_test.go index b70dcd921c..6fa90a6766 100644 --- a/pkg/apis/dos/validation/dos_test.go +++ b/pkg/apis/dos/validation/dos_test.go @@ -214,6 +214,97 @@ func TestValidateAppProtectDosAccessLogDest(t *testing.T) { } } +func TestValidateAppProtectDosLogConf(t *testing.T) { + t.Parallel() + tests := []struct { + logConf *unstructured.Unstructured + expectErr bool + expectWarn bool + msg string + }{ + { + logConf: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "filter": map[string]interface{}{}, + }, + }, + }, + expectErr: false, + expectWarn: false, + msg: "valid log conf", + }, + { + logConf: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{}, + }, + }, + expectErr: true, + expectWarn: false, + msg: "invalid log conf with no filter field", + }, + { + logConf: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "something": map[string]interface{}{ + "filter": map[string]interface{}{}, + }, + }, + }, + expectErr: true, + expectWarn: false, + msg: "invalid log conf with no spec field", + }, + { + logConf: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "content": map[string]interface{}{ + "format": "user-defined", + }, + "filter": map[string]interface{}{}, + }, + }, + }, + expectErr: false, + expectWarn: true, + msg: "Support only splunk format", + }, + { + logConf: &unstructured.Unstructured{ + Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "filter": map[string]interface{}{}, + "content": map[string]interface{}{ + "format": "user-defined", + }, + }, + }, + }, + expectErr: false, + expectWarn: true, + msg: "valid log conf with warning filter field", + }, + } + + for _, test := range tests { + warn, err := ValidateAppProtectDosLogConf(test.logConf) + if test.expectErr && err == nil { + t.Errorf("validateAppProtectDosLogConf() returned no error for the case of %s", test.msg) + } + if !test.expectErr && err != nil { + t.Errorf("validateAppProtectDosLogConf() returned unexpected error %v for the case of %s", err, test.msg) + } + if test.expectWarn && warn == "" { + t.Errorf("validateAppProtectDosLogConf() returned no warning for the case of %s", test.msg) + } + if !test.expectWarn && warn != "" { + t.Errorf("validateAppProtectDosLogConf() returned unexpected warning: %s, for the case of %s", warn, test.msg) + } + } +} + func TestValidateAppProtectDosPolicy(t *testing.T) { t.Parallel() tests := []struct {