Chore/add events to configmap (#6819)

Jim Ryan · web-flow · commit 198467809a08 · 2024-11-18T16:27:07.000Z
* add configmap validation events

* add events to configmap, add python test

* error messages if config is invalid

* remove v1

* add keep alive requests event
diff --git 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
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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",
+        )

Original file line number	Diff line number	Diff line change
`@@ -5,6 +5,7 @@ import (`
`5`	`5`	`"testing"`
`6`	`6`
`7`	`7`	`v1 "k8s.io/api/core/v1"`
	`8`	`+ "k8s.io/client-go/tools/record"`
`8`	`9`	`)`
`9`	`10`
`10`	`11`	`func TestParseConfigMapWithAppProtectCompressedRequestsAction(t *testing.T) {`
`@@ -45,7 +46,7 @@ func TestParseConfigMapWithAppProtectCompressedRequestsAction(t *testing.T) {`
`45`	`46`	`"app-protect-compressed-requests-action": test.action,`
`46`	`47`	`},`
`47`	`48`	`}`
`48`		`- result := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough)`
	`49`	`+ result, _ := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough, makeEventLogger())`
`49`	`50`	`if result.MainAppProtectCompressedRequestsAction != test.expect {`
`50`	`51`	`t.Errorf("ParseConfigMap() returned %q but expected %q for the case %s", result.MainAppProtectCompressedRequestsAction, test.expect, test.msg)`
`51`	`52`	`}`
`@@ -114,7 +115,7 @@ func TestParseConfigMapWithAppProtectReconnectPeriod(t *testing.T) {`
`114`	`115`	`"app-protect-reconnect-period-seconds": test.period,`
`115`	`116`	`},`
`116`	`117`	`}`
`117`		`- result := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough)`
	`118`	`+ result, _ := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough, makeEventLogger())`
`118`	`119`	`if result.MainAppProtectReconnectPeriod != test.expect {`
`119`	`120`	`t.Errorf("ParseConfigMap() returned %q but expected %q for the case %s", result.MainAppProtectReconnectPeriod, test.expect, test.msg)`
`120`	`121`	`}`
`@@ -155,7 +156,7 @@ func TestParseConfigMapWithTLSPassthroughProxyProtocol(t *testing.T) {`
`155`	`156`	`"real-ip-header": test.realIPheader,`
`156`	`157`	`},`
`157`	`158`	`}`
`158`		`- result := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough)`
	`159`	`+ result, _ := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough, makeEventLogger())`
`159`	`160`	`if result.RealIPHeader != test.want {`
`160`	`161`	`t.Errorf("want %q, got %q", test.want, result.RealIPHeader)`
`161`	`162`	`}`
`@@ -197,7 +198,7 @@ func TestParseConfigMapWithoutTLSPassthroughProxyProtocol(t *testing.T) {`
`197`	`198`	`"real-ip-header": test.realIPheader,`
`198`	`199`	`},`
`199`	`200`	`}`
`200`		`- result := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough)`
	`201`	`+ result, _ := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough, makeEventLogger())`
`201`	`202`	`if result.RealIPHeader != test.want {`
`202`	`203`	`t.Errorf("want %q, got %q", test.want, result.RealIPHeader)`
`203`	`204`	`}`
`@@ -244,7 +245,7 @@ func TestParseConfigMapAccessLog(t *testing.T) {`
`244`	`245`	`"access-log-off": test.accessLogOff,`
`245`	`246`	`},`
`246`	`247`	`}`
`247`		`- result := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough)`
	`248`	`+ result, _ := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough, makeEventLogger())`
`248`	`249`	`if result.MainAccessLog != test.want {`
`249`	`250`	`t.Errorf("want %q, got %q", test.want, result.MainAccessLog)`
`250`	`251`	`}`
`@@ -276,10 +277,14 @@ func TestParseConfigMapAccessLogDefault(t *testing.T) {`
`276`	`277`	`"access-log-off": "False",`
`277`	`278`	`},`
`278`	`279`	`}`
`279`		`- result := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough)`
	`280`	`+ result, _ := ParseConfigMap(context.Background(), cm, nginxPlus, hasAppProtect, hasAppProtectDos, hasTLSPassthrough, makeEventLogger())`
`280`	`281`	`if result.MainAccessLog != test.want {`
`281`	`282`	`t.Errorf("want %q, got %q", test.want, result.MainAccessLog)`
`282`	`283`	`}`
`283`	`284`	`})`
`284`	`285`	`}`
`285`	`286`	`}`
	`287`	`+`
	`288`	`+func makeEventLogger() record.EventRecorder {`
	`289`	`+ return record.NewFakeRecorder(1024)`
	`290`	`+}`