diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 881e82985f..a1ec4f0cf1 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -151,7 +151,7 @@ func main() { mustProcessGlobalConfiguration(ctx) cfgParams := configs.NewDefaultConfigParams(ctx, *nginxPlus) - cfgParams = processConfigMaps(kubeClient, cfgParams, nginxManager, templateExecutor) + cfgParams = processConfigMaps(kubeClient, cfgParams, nginxManager, templateExecutor, eventRecorder) staticCfgParams := &configs.StaticConfigParams{ DisableIPV6: *disableIPV6, @@ -854,7 +854,7 @@ func mustProcessGlobalConfiguration(ctx context.Context) { } } -func processConfigMaps(kubeClient *kubernetes.Clientset, cfgParams *configs.ConfigParams, nginxManager nginx.Manager, templateExecutor *version1.TemplateExecutor) *configs.ConfigParams { +func processConfigMaps(kubeClient *kubernetes.Clientset, cfgParams *configs.ConfigParams, nginxManager nginx.Manager, templateExecutor *version1.TemplateExecutor, eventLog record.EventRecorder) *configs.ConfigParams { l := nl.LoggerFromContext(cfgParams.Context) if *nginxConfigMaps != "" { ns, name, err := k8s.ParseNamespaceName(*nginxConfigMaps) @@ -865,7 +865,7 @@ func processConfigMaps(kubeClient *kubernetes.Clientset, cfgParams *configs.Conf if err != nil { nl.Fatalf(l, "Error when getting %v: %v", *nginxConfigMaps, err) } - cfgParams = configs.ParseConfigMap(cfgParams.Context, cfm, *nginxPlus, *appProtect, *appProtectDos, *enableTLSPassthrough) + cfgParams, _ = configs.ParseConfigMap(cfgParams.Context, cfm, *nginxPlus, *appProtect, *appProtectDos, *enableTLSPassthrough, eventLog) if cfgParams.MainServerSSLDHParamFileContent != nil { fileName, err := nginxManager.CreateDHParam(*cfgParams.MainServerSSLDHParamFileContent) if err != nil { diff --git a/internal/configs/configmaps.go b/internal/configs/configmaps.go index bb8db2d0d1..c1f2111932 100644 --- a/internal/configs/configmaps.go +++ b/internal/configs/configmaps.go @@ -2,27 +2,41 @@ package configs import ( "context" + "fmt" "strings" v1 "k8s.io/api/core/v1" + "k8s.io/client-go/tools/record" "github.com/nginxinc/kubernetes-ingress/internal/configs/version1" nl "github.com/nginxinc/kubernetes-ingress/internal/logger" ) +const ( + minimumInterval = 60 + invalidValueReason = "InvalidValue" +) + // ParseConfigMap parses ConfigMap into ConfigParams. // //nolint:gocyclo -func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, hasAppProtect bool, hasAppProtectDos bool, hasTLSPassthrough bool) *ConfigParams { +func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, hasAppProtect bool, hasAppProtectDos bool, hasTLSPassthrough bool, eventLog record.EventRecorder) (*ConfigParams, bool) { l := nl.LoggerFromContext(ctx) cfgParams := NewDefaultConfigParams(ctx, nginxPlus) + configOk := true + // valid values for server token are on | off | build | string; + // oss can only use on | off if serverTokens, exists, err := GetMapKeyAsBool(cfgm.Data, "server-tokens", cfgm); exists { + // this may be a build | string if err != nil { if nginxPlus { cfgParams.ServerTokens = cfgm.Data["server-tokens"] } else { - nl.Error(l, err) + errorText := fmt.Sprintf("ConfigMap %s/%s: 'server-tokens' must be a bool for OSS, ignoring", cfgm.GetNamespace(), cfgm.GetName()) + nl.Error(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, errorText) + configOk = false } } else { cfgParams.ServerTokens = "off" @@ -35,13 +49,19 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if lbMethod, exists := cfgm.Data["lb-method"]; exists { if nginxPlus { if parsedMethod, err := ParseLBMethodForPlus(lbMethod); err != nil { - nl.Errorf(l, "Configmap %s/%s: Invalid value for the lb-method key: got %q: %v", cfgm.GetNamespace(), cfgm.GetName(), lbMethod, err) + errorText := fmt.Sprintf("ConfigMap %s/%s: invalid value for 'lb-method': %q: %v, ignoring", cfgm.GetNamespace(), cfgm.GetName(), lbMethod, err) + nl.Error(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, errorText) + configOk = false } else { cfgParams.LBMethod = parsedMethod } } else { if parsedMethod, err := ParseLBMethod(lbMethod); err != nil { - nl.Errorf(l, "Configmap %s/%s: Invalid value for the lb-method key: got %q: %v", cfgm.GetNamespace(), cfgm.GetName(), lbMethod, err) + errorText := fmt.Sprintf("Configmap %s/%s: Invalid value for the lb-method key: got %q: %v", cfgm.GetNamespace(), cfgm.GetName(), lbMethod, err) + nl.Error(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, errorText) + configOk = false } else { cfgParams.LBMethod = parsedMethod } @@ -91,6 +111,8 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if HTTP2, exists, err := GetMapKeyAsBool(cfgm.Data, "http2", cfgm); exists { if err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) + configOk = false } else { cfgParams.HTTP2 = HTTP2 } @@ -99,6 +121,8 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if redirectToHTTPS, exists, err := GetMapKeyAsBool(cfgm.Data, "redirect-to-https", cfgm); exists { if err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) + configOk = false } else { cfgParams.RedirectToHTTPS = redirectToHTTPS } @@ -107,6 +131,8 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if sslRedirect, exists, err := GetMapKeyAsBool(cfgm.Data, "ssl-redirect", cfgm); exists { if err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) + configOk = false } else { cfgParams.SSLRedirect = sslRedirect } @@ -115,27 +141,38 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if hsts, exists, err := GetMapKeyAsBool(cfgm.Data, "hsts", cfgm); exists { if err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) + configOk = false } else { parsingErrors := false hstsMaxAge, existsMA, err := GetMapKeyAsInt64(cfgm.Data, "hsts-max-age", cfgm) if existsMA && err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) parsingErrors = true + configOk = false } hstsIncludeSubdomains, existsIS, err := GetMapKeyAsBool(cfgm.Data, "hsts-include-subdomains", cfgm) if existsIS && err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) parsingErrors = true + configOk = false } hstsBehindProxy, existsBP, err := GetMapKeyAsBool(cfgm.Data, "hsts-behind-proxy", cfgm) if existsBP && err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) parsingErrors = true + configOk = false } if parsingErrors { - nl.Errorf(l, "Configmap %s/%s: There are configuration issues with hsts annotations, skipping options for all hsts settings", cfgm.GetNamespace(), cfgm.GetName()) + errorText := fmt.Sprintf("ConfigMap %s/%s: there are configuration issues with HSTS settings, ignoring all HSTS options", cfgm.GetNamespace(), cfgm.GetName()) + nl.Error(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, errorText) + configOk = false } else { cfgParams.HSTS = hsts if existsMA { @@ -154,6 +191,8 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if proxyProtocol, exists, err := GetMapKeyAsBool(cfgm.Data, "proxy-protocol", cfgm); exists { if err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) + configOk = false } else { cfgParams.ProxyProtocol = proxyProtocol } @@ -161,11 +200,13 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if realIPHeader, exists := cfgm.Data["real-ip-header"]; exists { if hasTLSPassthrough { - msg := "Configmap %s/%s: key real-ip-header is ignored, directive real_ip_header is automatically set to 'proxy_protocol' when TLS passthrough is enabled." + errorText := fmt.Sprintf("ConfigMap %s/%s: 'real-ip-header' is ignored because 'real_ip_header' is automatically set to 'proxy_protocol' when TLS passthrough is enabled, ignoring", cfgm.GetNamespace(), cfgm.GetName()) if realIPHeader == "proxy_protocol" { - nl.Infof(l, msg, cfgm.GetNamespace(), cfgm.GetName()) + nl.Info(l, errorText) } else { - nl.Errorf(l, msg, cfgm.GetNamespace(), cfgm.GetName()) + nl.Error(l, errorText) + configOk = false + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, errorText) } } else { cfgParams.RealIPHeader = realIPHeader @@ -179,6 +220,8 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if realIPRecursive, exists, err := GetMapKeyAsBool(cfgm.Data, "real-ip-recursive", cfgm); exists { if err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) + configOk = false } else { cfgParams.RealIPRecursive = realIPRecursive } @@ -191,6 +234,8 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if sslPreferServerCiphers, exists, err := GetMapKeyAsBool(cfgm.Data, "ssl-prefer-server-ciphers", cfgm); exists { if err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) + configOk = false } else { cfgParams.MainServerSSLPreferServerCiphers = sslPreferServerCiphers } @@ -211,7 +256,10 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if accessLog, exists := cfgm.Data["access-log"]; exists { if !strings.HasPrefix(accessLog, "syslog:") { - nl.Warnf(l, "Configmap %s/%s: Invalid value for key access-log: %q", cfgm.GetNamespace(), cfgm.GetName(), accessLog) + errorText := fmt.Sprintf("ConfigMap %s/%s: invalid value for 'access-log': %q, ignoring", cfgm.GetNamespace(), cfgm.GetName(), accessLog) + nl.Warn(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, errorText) + configOk = false } else { cfgParams.MainAccessLog = accessLog } @@ -220,6 +268,8 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if accessLogOff, exists, err := GetMapKeyAsBool(cfgm.Data, "access-log-off", cfgm); exists { if err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) + configOk = false } else { if accessLogOff { cfgParams.MainAccessLog = "off" @@ -252,6 +302,8 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if defaultServerAccessLogOff, exists, err := GetMapKeyAsBool(cfgm.Data, "default-server-access-log-off", cfgm); exists { if err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) + configOk = false } else { cfgParams.DefaultServerAccessLogOff = defaultServerAccessLogOff } @@ -264,6 +316,8 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if proxyBuffering, exists, err := GetMapKeyAsBool(cfgm.Data, "proxy-buffering", cfgm); exists { if err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) + configOk = false } else { cfgParams.ProxyBuffering = proxyBuffering } @@ -299,7 +353,9 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if _, exists, err := GetMapKeyAsInt(cfgm.Data, "worker-processes", cfgm); exists { if err != nil && cfgm.Data["worker-processes"] != "auto" { - nl.Errorf(l, "Configmap %s/%s: Invalid value for worker-processes key: must be an integer or the string 'auto', got %q", cfgm.GetNamespace(), cfgm.GetName(), cfgm.Data["worker-processes"]) + nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) + configOk = false } else { cfgParams.MainWorkerProcesses = cfgm.Data["worker-processes"] } @@ -324,6 +380,8 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if keepalive, exists, err := GetMapKeyAsInt(cfgm.Data, "keepalive", cfgm); exists { if err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) + configOk = false } else { cfgParams.Keepalive = keepalive } @@ -332,6 +390,8 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if maxFails, exists, err := GetMapKeyAsInt(cfgm.Data, "max-fails", cfgm); exists { if err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) + configOk = false } else { cfgParams.MaxFails = maxFails } @@ -377,18 +437,26 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if nginxPlus { cfgParams.ResolverAddresses = resolverAddresses } else { - nl.Warn(l, "ConfigMap key 'resolver-addresses' requires NGINX Plus") + errorText := fmt.Sprintf("ConfigMap %s/%s key %s requires NGINX Plus", cfgm.Namespace, cfgm.Name, "resolver-addresses") + nl.Warn(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, errorText) + configOk = false } } if resolverIpv6, exists, err := GetMapKeyAsBool(cfgm.Data, "resolver-ipv6", cfgm); exists { if err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) + configOk = false } else { if nginxPlus { cfgParams.ResolverIPV6 = resolverIpv6 } else { - nl.Warn(l, "ConfigMap key 'resolver-ipv6' requires NGINX Plus") + errorText := fmt.Sprintf("ConfigMap %s/%s key %s requires NGINX Plus", cfgm.Namespace, cfgm.Name, "resolver-ipv6") + nl.Warn(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, errorText) + configOk = false } } } @@ -397,7 +465,10 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if nginxPlus { cfgParams.ResolverValid = resolverValid } else { - nl.Warn(l, "ConfigMap key 'resolver-valid' requires NGINX Plus") + errorText := fmt.Sprintf("ConfigMap %s/%s key %s requires NGINX Plus", cfgm.Namespace, cfgm.Name, "resolver-valid") + nl.Warn(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, errorText) + configOk = false } } @@ -405,7 +476,10 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if nginxPlus { cfgParams.ResolverTimeout = resolverTimeout } else { - nl.Warn(l, "ConfigMap key 'resolver-timeout' requires NGINX Plus") + errorText := fmt.Sprintf("ConfigMap %s/%s key %s requires NGINX Plus", cfgm.Namespace, cfgm.Name, "resolver-timeout") + nl.Warn(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, errorText) + configOk = false } } @@ -416,6 +490,8 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if keepaliveRequests, exists, err := GetMapKeyAsInt64(cfgm.Data, "keepalive-requests", cfgm); exists { if err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) + configOk = false } else { cfgParams.MainKeepaliveRequests = keepaliveRequests } @@ -424,6 +500,8 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if varHashBucketSize, exists, err := GetMapKeyAsUint64(cfgm.Data, "variables-hash-bucket-size", cfgm, true); exists { if err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) + configOk = false } else { cfgParams.VariablesHashBucketSize = varHashBucketSize } @@ -432,6 +510,8 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if varHashMaxSize, exists, err := GetMapKeyAsUint64(cfgm.Data, "variables-hash-max-size", cfgm, false); exists { if err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) + configOk = false } else { cfgParams.VariablesHashMaxSize = varHashMaxSize } @@ -452,11 +532,16 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if openTracing, exists, err := GetMapKeyAsBool(cfgm.Data, "opentracing", cfgm); exists { if err != nil { nl.Error(l, err) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, err.Error()) + configOk = false } else { if cfgParams.MainOpenTracingLoadModule { cfgParams.MainOpenTracingEnabled = openTracing } else { - nl.Error(l, "ConfigMap Key 'opentracing' requires both 'opentracing-tracer' and 'opentracing-tracer-config' Keys configured, Opentracing will be disabled") + errorText := "ConfigMap key 'opentracing' requires both 'opentracing-tracer' and 'opentracing-tracer-config' keys configured, Opentracing will be disabled, ignoring" + nl.Error(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, errorText) + configOk = false } } } @@ -466,7 +551,15 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if appProtectFailureModeAction == "pass" || appProtectFailureModeAction == "drop" { cfgParams.MainAppProtectFailureModeAction = appProtectFailureModeAction } else { - nl.Error(l, "ConfigMap Key 'app-protect-failure-mode-action' must have value 'pass' or 'drop'. Ignoring.") + errorText := fmt.Sprintf( + "ConfigMap %s/%s: invalid value for 'app-protect-failure-mode-action': %q, must be 'pass' or 'drop', ignoring", + cfgm.GetNamespace(), + cfgm.GetName(), + appProtectFailureModeAction, + ) + nl.Error(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, errorText) + configOk = false } } @@ -474,7 +567,15 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if appProtectCompressedRequestsAction == "pass" || appProtectCompressedRequestsAction == "drop" { cfgParams.MainAppProtectCompressedRequestsAction = appProtectCompressedRequestsAction } else { - nl.Error(l, "ConfigMap Key 'app-protect-compressed-requests-action' must have value 'pass' or 'drop'. Ignoring.") + errorText := fmt.Sprintf( + "ConfigMap %s/%s: invalid value for 'app-protect-compressed-requests-action': %q, must be 'pass' or 'drop', ignoring", + cfgm.GetNamespace(), + cfgm.GetName(), + appProtectCompressedRequestsAction, + ) + nl.Error(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, errorText) + configOk = false } } @@ -486,24 +587,48 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has if VerifyAppProtectThresholds(appProtectCPUThresholds) { cfgParams.MainAppProtectCPUThresholds = appProtectCPUThresholds } else { - nl.Error(l, "ConfigMap Key 'app-protect-cpu-thresholds' must follow pattern: 'high=<0 - 100> low=<0 - 100>'. Ignoring.") + errorText := fmt.Sprintf( + "ConfigMap %s/%s: invalid value for 'app-protect-cpu-thresholds': %q, must follow pattern 'high=<0 - 100> low=<0 - 100>', ignoring", + cfgm.GetNamespace(), + cfgm.GetName(), + appProtectCPUThresholds, + ) + nl.Error(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, errorText) + configOk = false } } if appProtectPhysicalMemoryThresholds, exists := cfgm.Data["app-protect-physical-memory-util-thresholds"]; exists { - cfgParams.MainAppProtectPhysicalMemoryThresholds = appProtectPhysicalMemoryThresholds if VerifyAppProtectThresholds(appProtectPhysicalMemoryThresholds) { cfgParams.MainAppProtectPhysicalMemoryThresholds = appProtectPhysicalMemoryThresholds } else { - nl.Error(l, "ConfigMap Key 'app-protect-physical-memory-thresholds' must follow pattern: 'high=<0 - 100> low=<0 - 100>'. Ignoring.") + errorText := fmt.Sprintf( + "ConfigMap %s/%s: invalid value for 'app-protect-physical-memory-util-thresholds': %q, must follow pattern 'high=<0 - 100> low=<0 - 100>', ignoring", + cfgm.GetNamespace(), + cfgm.GetName(), + appProtectPhysicalMemoryThresholds, + ) + nl.Error(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, errorText) + configOk = false } } + if appProtectReconnectPeriod, exists := cfgm.Data["app-protect-reconnect-period-seconds"]; exists { period, err := ParseFloat64(appProtectReconnectPeriod) if err == nil && period > 0 && period <= 60 { cfgParams.MainAppProtectReconnectPeriod = appProtectReconnectPeriod } else { - nl.Error(l, "ConfigMap Key 'app-protect-reconnect-period-second' must have value between '0' and '60'. '0' is illegal. Ignoring.") + errorText := fmt.Sprintf( + "ConfigMap %s/%s: invalid value for 'app-protect-reconnect-period-seconds': %q, must be between '0' and '60' (exclusive), '0' is illegal, ignoring", + cfgm.GetNamespace(), + cfgm.GetName(), + appProtectReconnectPeriod, + ) + nl.Error(l, errorText) + eventLog.Event(cfgm, v1.EventTypeWarning, invalidValueReason, errorText) + configOk = false } } } @@ -528,7 +653,7 @@ func ParseConfigMap(ctx context.Context, cfgm *v1.ConfigMap, nginxPlus bool, has } } - return cfgParams + return cfgParams, configOk } // GenerateNginxMainConfig generates MainConfig. diff --git a/internal/configs/configmaps_test.go b/internal/configs/configmaps_test.go index b14c56a869..57b031dd7c 100644 --- a/internal/configs/configmaps_test.go +++ b/internal/configs/configmaps_test.go @@ -5,6 +5,7 @@ import ( "testing" v1 "k8s.io/api/core/v1" + "k8s.io/client-go/tools/record" ) func TestParseConfigMapWithAppProtectCompressedRequestsAction(t *testing.T) { @@ -45,7 +46,7 @@ func TestParseConfigMapWithAppProtectCompressedRequestsAction(t *testing.T) { "app-protect-compressed-requests-action": test.action, }, } - result := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough) + result, _ := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough, makeEventLogger()) if result.MainAppProtectCompressedRequestsAction != test.expect { t.Errorf("ParseConfigMap() returned %q but expected %q for the case %s", result.MainAppProtectCompressedRequestsAction, test.expect, test.msg) } @@ -114,7 +115,7 @@ func TestParseConfigMapWithAppProtectReconnectPeriod(t *testing.T) { "app-protect-reconnect-period-seconds": test.period, }, } - result := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough) + result, _ := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough, makeEventLogger()) if result.MainAppProtectReconnectPeriod != test.expect { t.Errorf("ParseConfigMap() returned %q but expected %q for the case %s", result.MainAppProtectReconnectPeriod, test.expect, test.msg) } @@ -155,7 +156,7 @@ func TestParseConfigMapWithTLSPassthroughProxyProtocol(t *testing.T) { "real-ip-header": test.realIPheader, }, } - result := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough) + result, _ := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough, makeEventLogger()) if result.RealIPHeader != test.want { t.Errorf("want %q, got %q", test.want, result.RealIPHeader) } @@ -197,7 +198,7 @@ func TestParseConfigMapWithoutTLSPassthroughProxyProtocol(t *testing.T) { "real-ip-header": test.realIPheader, }, } - result := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough) + result, _ := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough, makeEventLogger()) if result.RealIPHeader != test.want { t.Errorf("want %q, got %q", test.want, result.RealIPHeader) } @@ -244,7 +245,7 @@ func TestParseConfigMapAccessLog(t *testing.T) { "access-log-off": test.accessLogOff, }, } - result := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough) + result, _ := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough, makeEventLogger()) if result.MainAccessLog != test.want { t.Errorf("want %q, got %q", test.want, result.MainAccessLog) } @@ -276,10 +277,14 @@ func TestParseConfigMapAccessLogDefault(t *testing.T) { "access-log-off": "False", }, } - result := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough) + result, _ := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough, makeEventLogger()) if result.MainAccessLog != test.want { t.Errorf("want %q, got %q", test.want, result.MainAccessLog) } }) } } + +func makeEventLogger() record.EventRecorder { + return record.NewFakeRecorder(1024) +} diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 50ff55987c..581e7713da 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -841,9 +841,10 @@ func (lbc *LoadBalancerController) createExtendedResources(resources []Resource) func (lbc *LoadBalancerController) updateAllConfigs() { ctx := nl.ContextWithLogger(context.Background(), lbc.Logger) cfgParams := configs.NewDefaultConfigParams(ctx, lbc.isNginxPlus) + var isNGINXConfigValid bool if lbc.configMap != nil { - cfgParams = configs.ParseConfigMap(ctx, lbc.configMap, lbc.isNginxPlus, lbc.appProtectEnabled, lbc.appProtectDosEnabled, lbc.configuration.isTLSPassthroughEnabled) + cfgParams, isNGINXConfigValid = configs.ParseConfigMap(ctx, lbc.configMap, lbc.isNginxPlus, lbc.appProtectEnabled, lbc.appProtectDosEnabled, lbc.configuration.isTLSPassthroughEnabled, lbc.recorder) } resources := lbc.configuration.GetResources() @@ -869,8 +870,11 @@ func (lbc *LoadBalancerController) updateAllConfigs() { } if lbc.configMap != nil { - key := getResourceKey(&lbc.configMap.ObjectMeta) - lbc.recorder.Eventf(lbc.configMap, eventType, eventTitle, "Configuration from %v was updated %s", key, eventWarningMessage) + if isNGINXConfigValid { + lbc.recorder.Event(lbc.configMap, api_v1.EventTypeNormal, "Updated", fmt.Sprintf("ConfigMap %s/%s updated without error", lbc.configMap.GetNamespace(), lbc.configMap.GetName())) + } else { + lbc.recorder.Event(lbc.configMap, api_v1.EventTypeWarning, "UpdatedWithError", fmt.Sprintf("ConfigMap %s/%s updated with errors. Ignoring invalid values", lbc.configMap.GetNamespace(), lbc.configMap.GetName())) + } } gc := lbc.configuration.GetGlobalConfiguration() diff --git a/tests/data/virtual-server-configmap-keys/configmap-invalid.yaml b/tests/data/virtual-server-configmap-keys/configmap-invalid.yaml new file mode 100644 index 0000000000..3516c143ba --- /dev/null +++ b/tests/data/virtual-server-configmap-keys/configmap-invalid.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: nginx-config + namespace: nginx-ingress +data: + proxy-buffering: "invalid" # Invalid boolean + proxy-read-timeout: "60s" diff --git a/tests/data/virtual-server-configmap-keys/configmap-valid.yaml b/tests/data/virtual-server-configmap-keys/configmap-valid.yaml new file mode 100644 index 0000000000..63ebf74cdd --- /dev/null +++ b/tests/data/virtual-server-configmap-keys/configmap-valid.yaml @@ -0,0 +1,8 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: nginx-config + namespace: nginx-ingress +data: + proxy-buffering: "on" + proxy-read-timeout: "60s" diff --git a/tests/suite/test_virtual_server_configmap_keys.py b/tests/suite/test_virtual_server_configmap_keys.py index 36e0860553..3fd4c6345d 100644 --- a/tests/suite/test_virtual_server_configmap_keys.py +++ b/tests/suite/test_virtual_server_configmap_keys.py @@ -2,6 +2,7 @@ from settings import DEPLOYMENTS, TEST_DATA from suite.utils.resources_utils import ( get_events, + get_events_for_object, get_file_contents, get_first_pod_name, get_pods_amount, @@ -124,6 +125,23 @@ def assert_defaults_of_ssl_keys(config): assert "http2 on;" not in config +def assert_event(event_list, event_type, reason, message_substring): + """ + Assert that an event with specific type, reason, and message substring exists. + + :param event_list: List of events + :param event_type: 'Normal' or 'Warning' + :param reason: Event reason + :param message_substring: Substring expected in the event message + """ + for event in event_list: + if event.type == event_type and event.reason == reason and message_substring in event.message: + return + assert ( + False + ), f"Expected event with type '{event_type}', reason '{reason}', and message containing '{message_substring}' not found." + + @pytest.fixture(scope="function") def clean_up(request, kube_apis, ingress_controller_prerequisites, test_namespace) -> None: """ @@ -404,3 +422,69 @@ def test_ssl_keys( ) assert_update_event_count_increased(virtual_server_setup, step_2_events, step_1_events) assert_defaults_of_ssl_keys(step_2_config) + + def test_configmap_events( + self, + kube_apis, + ingress_controller_prerequisites, + crd_ingress_controller, + virtual_server_setup, + clean_up, + ): + ingress_controller_prerequisites.namespace + configmap_name = ingress_controller_prerequisites.config_map["metadata"]["name"] + configmap_namespace = ingress_controller_prerequisites.config_map["metadata"]["namespace"] + + # Step 1: Update ConfigMap with valid parameters + print("Updating ConfigMap with valid parameters") + replace_configmap_from_yaml( + kube_apis.v1, + configmap_name, + configmap_namespace, + f"{TEST_DATA}/virtual-server-configmap-keys/configmap-valid.yaml", + ) + wait_before_test(1) + + # Get events for the ConfigMap + events = get_events_for_object( + kube_apis.v1, + configmap_namespace, + configmap_name, + ) + + assert len(events) >= 1 + + # Assert that the 'updated without error' event is present + assert_event( + events, + "Normal", + "Updated", + f"ConfigMap {configmap_namespace}/{configmap_name} updated without error", + ) + + # Step 2: Update ConfigMap with invalid parameters + print("Updating ConfigMap with invalid parameters") + replace_configmap_from_yaml( + kube_apis.v1, + configmap_name, + configmap_namespace, + f"{TEST_DATA}/virtual-server-configmap-keys/configmap-invalid.yaml", + ) + wait_before_test(1) + + # Get events for the ConfigMap + events = get_events_for_object( + kube_apis.v1, + configmap_namespace, + configmap_name, + ) + + assert len(events) >= 1 + + # Assert that the 'updated with errors' event is present + assert_event( + events, + "Warning", + "UpdatedWithError", + f"ConfigMap {configmap_namespace}/{configmap_name} updated with errors. Ignoring invalid values", + )