Skip to content

Commit 2e5d5e9

Browse files
vepatelvepatel
authored
update interval checks (#8043)
1 parent b5860f8 commit 2e5d5e9

File tree

4 files changed

+221
-14
lines changed

4 files changed

+221
-14
lines changed

‎charts/nginx-ingress/values.schema.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,10 +134,11 @@
134134
},
135135
"interval": {
136136
"type": "string",
137-
"pattern": "^[0-9]+[mhd]$",
137+
"pattern": "^[0-9]+[smh]$",
138138
"default": "1h",
139139
"title": "The usage report interval Schema",
140140
"examples": [
141+
"60s",
141142
"1m",
142143
"1h",
143144
"24h"

‎charts/nginx-ingress/values.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ controller:
2424

2525
# usageReport:
2626
# endpoint: "product.connect.nginx.com" # Endpoint for usage report
27-
# interval: 1h
27+
# interval: 1h # Interval for usage report, must be between 60s and 24h,
2828
# proxyHost: "proxy.example.com:3138" # Proxy server for usage report, with optional port
2929
# proxyCredentialsSecretName: "proxy-credentials" # Secret containing proxy credentials, must contain a `username` and `password` field
3030

‎internal/configs/configmaps.go

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323

2424
const (
2525
minimumInterval = 60
26+
maximumInterval = 24 * 60 * 60 // interval cannot be greater than 24h
2627
zoneSyncDefaultPort = 12345
2728
kubeDNSDefault = "kube-dns.kube-system.svc.cluster.local"
2829
)
@@ -909,23 +910,34 @@ func ParseMGMTConfigMap(ctx context.Context, cfgm *v1.ConfigMap, eventLog record
909910

910911
if interval, exists := cfgm.Data["usage-report-interval"]; exists {
911912
i := strings.TrimSpace(interval)
912-
t, err := time.ParseDuration(i)
913-
if err != nil {
914-
errorText := fmt.Sprintf("Configmap %s/%s: Invalid value for the interval key: got %q: %v. Ignoring.", cfgm.GetNamespace(), cfgm.GetName(), i, err)
915-
nl.Error(l, errorText)
916-
eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText)
917-
configWarnings = true
918-
}
919-
if t.Seconds() < minimumInterval {
920-
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)
913+
914+
// Validate interval: check for unsupported units and parse duration in case json schema validation is not used.
915+
if strings.Contains(i, "ms") || strings.Contains(i, "d") {
916+
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)
921917
nl.Error(l, errorText)
922918
eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText)
923919
configWarnings = true
924-
mgmtCfgParams.Interval = ""
925920
} else {
926-
mgmtCfgParams.Interval = i
921+
t, err := time.ParseDuration(i)
922+
if err != nil {
923+
errorText := fmt.Sprintf("Configmap %s/%s: Invalid value for the interval key: got %q: %v. Ignoring.", cfgm.GetNamespace(), cfgm.GetName(), i, err)
924+
nl.Error(l, errorText)
925+
eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText)
926+
configWarnings = true
927+
} else if t.Seconds() < minimumInterval {
928+
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)
929+
nl.Error(l, errorText)
930+
eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText)
931+
configWarnings = true
932+
} else if t.Seconds() > maximumInterval {
933+
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)
934+
nl.Error(l, errorText)
935+
eventLog.Event(cfgm, v1.EventTypeWarning, nl.EventReasonInvalidValue, errorText)
936+
configWarnings = true
937+
} else {
938+
mgmtCfgParams.Interval = i
939+
}
927940
}
928-
929941
}
930942
if trustedCertSecretName, exists := cfgm.Data["ssl-trusted-certificate-secret-name"]; exists {
931943
mgmtCfgParams.Secrets.TrustedCert = strings.TrimSpace(trustedCertSecretName)

‎internal/configs/configmaps_test.go

Lines changed: 194 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -683,6 +683,200 @@ func TestParseMGMTConfigMapUsageReportInterval(t *testing.T) {
683683
}
684684
}
685685

686+
func TestParseMGMTConfigMapUsageReportIntervalMaximum(t *testing.T) {
687+
t.Parallel()
688+
tests := []struct {
689+
configMap *v1.ConfigMap
690+
expectWarnings bool
691+
expectInterval string
692+
msg string
693+
}{
694+
{
695+
configMap: &v1.ConfigMap{
696+
Data: map[string]string{
697+
"license-token-secret-name": "license-token",
698+
"usage-report-interval": "24h",
699+
},
700+
},
701+
expectWarnings: false,
702+
expectInterval: "24h",
703+
msg: "24h should be accepted (maximum allowed)",
704+
},
705+
{
706+
configMap: &v1.ConfigMap{
707+
Data: map[string]string{
708+
"license-token-secret-name": "license-token",
709+
"usage-report-interval": "25h",
710+
},
711+
},
712+
expectWarnings: true,
713+
expectInterval: "",
714+
msg: "25h should be rejected (exceeds maximum)",
715+
},
716+
{
717+
configMap: &v1.ConfigMap{
718+
Data: map[string]string{
719+
"license-token-secret-name": "license-token",
720+
"usage-report-interval": "1440m",
721+
},
722+
},
723+
expectWarnings: false,
724+
expectInterval: "1440m",
725+
msg: "1440m (24h) should be accepted",
726+
},
727+
{
728+
configMap: &v1.ConfigMap{
729+
Data: map[string]string{
730+
"license-token-secret-name": "license-token",
731+
"usage-report-interval": "1441m",
732+
},
733+
},
734+
expectWarnings: true,
735+
expectInterval: "",
736+
msg: "1441m (>24h) should be rejected",
737+
},
738+
}
739+
740+
for _, test := range tests {
741+
t.Run(test.msg, func(t *testing.T) {
742+
result, warnings, err := ParseMGMTConfigMap(context.Background(), test.configMap, makeEventLogger())
743+
if err != nil {
744+
t.Fatal(err)
745+
}
746+
747+
if warnings != test.expectWarnings {
748+
t.Errorf("Expected warnings=%v, got warnings=%v", test.expectWarnings, warnings)
749+
}
750+
751+
if result.Interval != test.expectInterval {
752+
t.Errorf("Expected interval=%q, got interval=%q", test.expectInterval, result.Interval)
753+
}
754+
})
755+
}
756+
}
757+
758+
func TestParseMGMTConfigMapUsageReportIntervalEdgeCases(t *testing.T) {
759+
t.Parallel()
760+
tests := []struct {
761+
configMap *v1.ConfigMap
762+
expectWarnings bool
763+
expectInterval string
764+
msg string
765+
}{
766+
// Test milliseconds (should be rejected due to unsupported unit)
767+
{
768+
configMap: &v1.ConfigMap{
769+
Data: map[string]string{
770+
"license-token-secret-name": "license-token",
771+
"usage-report-interval": "1000ms",
772+
},
773+
},
774+
expectWarnings: true,
775+
expectInterval: "",
776+
msg: "1000ms should be rejected (ms unit not allowed)",
777+
},
778+
{
779+
configMap: &v1.ConfigMap{
780+
Data: map[string]string{
781+
"license-token-secret-name": "license-token",
782+
"usage-report-interval": "60000ms",
783+
},
784+
},
785+
expectWarnings: true,
786+
expectInterval: "",
787+
msg: "60000ms should be rejected (ms unit not allowed)",
788+
},
789+
// Test that large ms values are also rejected for unit, not value
790+
{
791+
configMap: &v1.ConfigMap{
792+
Data: map[string]string{
793+
"license-token-secret-name": "license-token",
794+
"usage-report-interval": "3600000ms",
795+
},
796+
},
797+
expectWarnings: true,
798+
expectInterval: "",
799+
msg: "3600000ms (1h) should be rejected (ms unit not allowed)",
800+
},
801+
// Test days (should be rejected by Go's ParseDuration)
802+
{
803+
configMap: &v1.ConfigMap{
804+
Data: map[string]string{
805+
"license-token-secret-name": "license-token",
806+
"usage-report-interval": "1d",
807+
},
808+
},
809+
expectWarnings: true,
810+
expectInterval: "",
811+
msg: "1d should be rejected (Go doesn't support 'd' unit)",
812+
},
813+
// Test values > 24h (valid in Go but should be rejected by max check)
814+
{
815+
configMap: &v1.ConfigMap{
816+
Data: map[string]string{
817+
"license-token-secret-name": "license-token",
818+
"usage-report-interval": "25h",
819+
},
820+
},
821+
expectWarnings: true,
822+
expectInterval: "",
823+
msg: "25h should be rejected (exceeds 24h maximum)",
824+
},
825+
{
826+
configMap: &v1.ConfigMap{
827+
Data: map[string]string{
828+
"license-token-secret-name": "license-token",
829+
"usage-report-interval": "48h",
830+
},
831+
},
832+
expectWarnings: true,
833+
expectInterval: "",
834+
msg: "48h should be rejected (exceeds 24h maximum)",
835+
},
836+
// exactly 86400 seconds (24h)
837+
{
838+
configMap: &v1.ConfigMap{
839+
Data: map[string]string{
840+
"license-token-secret-name": "license-token",
841+
"usage-report-interval": "86400s",
842+
},
843+
},
844+
expectWarnings: false,
845+
expectInterval: "86400s",
846+
msg: "86400s (24h) should be accepted",
847+
},
848+
// 86401 seconds (24h + 1s)
849+
{
850+
configMap: &v1.ConfigMap{
851+
Data: map[string]string{
852+
"license-token-secret-name": "license-token",
853+
"usage-report-interval": "86401s",
854+
},
855+
},
856+
expectWarnings: true,
857+
expectInterval: "",
858+
msg: "86401s (>24h) should be rejected",
859+
},
860+
}
861+
862+
for _, test := range tests {
863+
t.Run(test.msg, func(t *testing.T) {
864+
result, warnings, err := ParseMGMTConfigMap(context.Background(), test.configMap, makeEventLogger())
865+
if err != nil {
866+
t.Fatal(err)
867+
}
868+
869+
if warnings != test.expectWarnings {
870+
t.Errorf("Expected warnings=%v, got warnings=%v", test.expectWarnings, warnings)
871+
}
872+
873+
if result.Interval != test.expectInterval {
874+
t.Errorf("Expected interval=%q, got interval=%q", test.expectInterval, result.Interval)
875+
}
876+
})
877+
}
878+
}
879+
686880
func TestParseMGMTConfigMapResolverIPV6(t *testing.T) {
687881
t.Parallel()
688882
tests := []struct {

0 commit comments

Comments
 (0)