From 2dd6c2949d8540604554abe65f45ff43bbe4e8be Mon Sep 17 00:00:00 2001 From: Venktesh Shivam Patel Date: Fri, 18 Jul 2025 15:25:30 +0100 Subject: [PATCH] update interval checks (#8043) --- charts/nginx-ingress/values.schema.json | 3 +- charts/nginx-ingress/values.yaml | 2 +- internal/configs/configmaps.go | 36 +++-- internal/configs/configmaps_test.go | 194 ++++++++++++++++++++++++ 4 files changed, 221 insertions(+), 14 deletions(-) diff --git a/charts/nginx-ingress/values.schema.json b/charts/nginx-ingress/values.schema.json index 0e3cc235b2..5d255b3ad2 100644 --- a/charts/nginx-ingress/values.schema.json +++ b/charts/nginx-ingress/values.schema.json @@ -134,10 +134,11 @@ }, "interval": { "type": "string", - "pattern": "^[0-9]+[mhd]$", + "pattern": "^[0-9]+[smh]$", "default": "1h", "title": "The usage report interval Schema", "examples": [ + "60s", "1m", "1h", "24h" diff --git a/charts/nginx-ingress/values.yaml b/charts/nginx-ingress/values.yaml index 2f7815316e..d635bd445b 100644 --- a/charts/nginx-ingress/values.yaml +++ b/charts/nginx-ingress/values.yaml @@ -24,7 +24,7 @@ controller: # usageReport: # endpoint: "product.connect.nginx.com" # Endpoint for usage report - # interval: 1h + # interval: 1h # Interval for usage report, must be between 60s and 24h, # proxyHost: "proxy.example.com:3138" # Proxy server for usage report, with optional port # proxyCredentialsSecretName: "proxy-credentials" # Secret containing proxy credentials, must contain a `username` and `password` field diff --git a/internal/configs/configmaps.go b/internal/configs/configmaps.go index 3c8436e175..7a1c7cd001 100644 --- a/internal/configs/configmaps.go +++ b/internal/configs/configmaps.go @@ -23,6 +23,7 @@ import ( const ( minimumInterval = 60 + maximumInterval = 24 * 60 * 60 // interval cannot be greater than 24h zoneSyncDefaultPort = 12345 kubeDNSDefault = "kube-dns.kube-system.svc.cluster.local" ) @@ -909,23 +910,34 @@ func ParseMGMTConfigMap(ctx context.Context, cfgm *v1.ConfigMap, eventLog record if interval, exists := cfgm.Data["usage-report-interval"]; exists { i := strings.TrimSpace(interval) - t, err := time.ParseDuration(i) - if err != nil { - errorText := fmt.Sprintf("Configmap %s/%s: Invalid value for the interval key: got %q: %v. Ignoring.", cfgm.GetNamespace(), cfgm.GetName(), i, err) - nl.Error(l, errorText) - eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText) - configWarnings = true - } - if t.Seconds() < minimumInterval { - errorText := fmt.Sprintf("Configmap %s/%s: Value too low for the interval key, got: %v, need higher than %ds. Ignoring.", cfgm.GetNamespace(), cfgm.GetName(), i, minimumInterval) + + // Validate interval: check for unsupported units and parse duration in case json schema validation is not used. + if strings.Contains(i, "ms") || strings.Contains(i, "d") { + errorText := fmt.Sprintf("Configmap %s/%s: Invalid unit for the interval key: got %q. Only seconds (s), minutes (m), and hours (h) are allowed. Ignoring.", cfgm.GetNamespace(), cfgm.GetName(), i) nl.Error(l, errorText) eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText) configWarnings = true - mgmtCfgParams.Interval = "" } else { - mgmtCfgParams.Interval = i + t, err := time.ParseDuration(i) + if err != nil { + errorText := fmt.Sprintf("Configmap %s/%s: Invalid value for the interval key: got %q: %v. Ignoring.", cfgm.GetNamespace(), cfgm.GetName(), i, err) + nl.Error(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText) + configWarnings = true + } else if t.Seconds() < minimumInterval { + errorText := fmt.Sprintf("Configmap %s/%s: Value too low for the interval key, got: %v, need higher than %ds. Ignoring.", cfgm.GetNamespace(), cfgm.GetName(), i, minimumInterval) + nl.Error(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText) + configWarnings = true + } else if t.Seconds() > maximumInterval { + errorText := fmt.Sprintf("Configmap %s/%s: Value too high for the interval key, got: %v, maximum allowed is %ds (24h). Ignoring.", cfgm.GetNamespace(), cfgm.GetName(), i, maximumInterval) + nl.Error(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText) + configWarnings = true + } else { + mgmtCfgParams.Interval = i + } } - } if trustedCertSecretName, exists := cfgm.Data["ssl-trusted-certificate-secret-name"]; exists { mgmtCfgParams.Secrets.TrustedCert = strings.TrimSpace(trustedCertSecretName) diff --git a/internal/configs/configmaps_test.go b/internal/configs/configmaps_test.go index 0ddf61c9e2..b4afbd6613 100644 --- a/internal/configs/configmaps_test.go +++ b/internal/configs/configmaps_test.go @@ -683,6 +683,200 @@ func TestParseMGMTConfigMapUsageReportInterval(t *testing.T) { } } +func TestParseMGMTConfigMapUsageReportIntervalMaximum(t *testing.T) { + t.Parallel() + tests := []struct { + configMap *v1.ConfigMap + expectWarnings bool + expectInterval string + msg string + }{ + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "usage-report-interval": "24h", + }, + }, + expectWarnings: false, + expectInterval: "24h", + msg: "24h should be accepted (maximum allowed)", + }, + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "usage-report-interval": "25h", + }, + }, + expectWarnings: true, + expectInterval: "", + msg: "25h should be rejected (exceeds maximum)", + }, + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "usage-report-interval": "1440m", + }, + }, + expectWarnings: false, + expectInterval: "1440m", + msg: "1440m (24h) should be accepted", + }, + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "usage-report-interval": "1441m", + }, + }, + expectWarnings: true, + expectInterval: "", + msg: "1441m (>24h) should be rejected", + }, + } + + 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 != test.expectWarnings { + t.Errorf("Expected warnings=%v, got warnings=%v", test.expectWarnings, warnings) + } + + if result.Interval != test.expectInterval { + t.Errorf("Expected interval=%q, got interval=%q", test.expectInterval, result.Interval) + } + }) + } +} + +func TestParseMGMTConfigMapUsageReportIntervalEdgeCases(t *testing.T) { + t.Parallel() + tests := []struct { + configMap *v1.ConfigMap + expectWarnings bool + expectInterval string + msg string + }{ + // Test milliseconds (should be rejected due to unsupported unit) + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "usage-report-interval": "1000ms", + }, + }, + expectWarnings: true, + expectInterval: "", + msg: "1000ms should be rejected (ms unit not allowed)", + }, + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "usage-report-interval": "60000ms", + }, + }, + expectWarnings: true, + expectInterval: "", + msg: "60000ms should be rejected (ms unit not allowed)", + }, + // Test that large ms values are also rejected for unit, not value + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "usage-report-interval": "3600000ms", + }, + }, + expectWarnings: true, + expectInterval: "", + msg: "3600000ms (1h) should be rejected (ms unit not allowed)", + }, + // Test days (should be rejected by Go's ParseDuration) + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "usage-report-interval": "1d", + }, + }, + expectWarnings: true, + expectInterval: "", + msg: "1d should be rejected (Go doesn't support 'd' unit)", + }, + // Test values > 24h (valid in Go but should be rejected by max check) + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "usage-report-interval": "25h", + }, + }, + expectWarnings: true, + expectInterval: "", + msg: "25h should be rejected (exceeds 24h maximum)", + }, + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "usage-report-interval": "48h", + }, + }, + expectWarnings: true, + expectInterval: "", + msg: "48h should be rejected (exceeds 24h maximum)", + }, + // exactly 86400 seconds (24h) + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "usage-report-interval": "86400s", + }, + }, + expectWarnings: false, + expectInterval: "86400s", + msg: "86400s (24h) should be accepted", + }, + // 86401 seconds (24h + 1s) + { + configMap: &v1.ConfigMap{ + Data: map[string]string{ + "license-token-secret-name": "license-token", + "usage-report-interval": "86401s", + }, + }, + expectWarnings: true, + expectInterval: "", + msg: "86401s (>24h) should be rejected", + }, + } + + 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 != test.expectWarnings { + t.Errorf("Expected warnings=%v, got warnings=%v", test.expectWarnings, warnings) + } + + if result.Interval != test.expectInterval { + t.Errorf("Expected interval=%q, got interval=%q", test.expectInterval, result.Interval) + } + }) + } +} + func TestParseMGMTConfigMapResolverIPV6(t *testing.T) { t.Parallel() tests := []struct {