fix udp/http listener validation logic (#6406)

* fix udp/http listener validation logic

* lint and change field name

* fix listener validation algo

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* remove values.yaml change

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* fix test

* add more tests

* remove commented out listener

* fix test

* delete comment

* improve error message for duplicate ip port protocol combination

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Jakub Jarosz <[email protected]>

pkg/apis/configuration/validation/globalconfiguration.go

@@ -50,38 +50,33 @@ func (gcv *GlobalConfigurationValidator) validateGlobalConfigurationSpec(spec *c
 
 func (gcv *GlobalConfigurationValidator) getValidListeners(listeners []conf_v1.Listener, fieldPath *field.Path) ([]conf_v1.Listener, field.ErrorList) {
 	allErrs := field.ErrorList{}
-
 	listenerNames := sets.Set[string]{}
-	ipv4PortProtocolCombinations := make(map[string]map[int]string) // map[IP]map[Port]Protocol
-	ipv6PortProtocolCombinations := make(map[string]map[int]string)
+	ipv4PortProtocolCombinations := make(map[string]map[int][]string) // map[IP]map[Port][]Protocol
+	ipv6PortProtocolCombinations := make(map[string]map[int][]string)
 	var validListeners []conf_v1.Listener
-
 	for i, l := range listeners {
 		idxPath := fieldPath.Index(i)
 		listenerErrs := gcv.validateListener(l, idxPath)
 		if len(listenerErrs) > 0 {
 			allErrs = append(allErrs, listenerErrs...)
 			continue
 		}
-
 		if err := gcv.checkForDuplicateName(listenerNames, l, idxPath); err != nil {
 			allErrs = append(allErrs, err)
 			continue
 		}
-
 		if err := gcv.checkIPPortProtocolConflicts(ipv4PortProtocolCombinations, ipv4, l, fieldPath); err != nil {
 			allErrs = append(allErrs, err)
+			gcv.updatePortProtocolCombinations(ipv4PortProtocolCombinations, ipv4, l)
 			continue
 		}
-
 		if err := gcv.checkIPPortProtocolConflicts(ipv6PortProtocolCombinations, ipv6, l, fieldPath); err != nil {
 			allErrs = append(allErrs, err)
+			gcv.updatePortProtocolCombinations(ipv6PortProtocolCombinations, ipv6, l)
 			continue
 		}
-
 		gcv.updatePortProtocolCombinations(ipv4PortProtocolCombinations, ipv4, l)
 		gcv.updatePortProtocolCombinations(ipv6PortProtocolCombinations, ipv6, l)
-
 		validListeners = append(validListeners, l)
 	}
 	return validListeners, allErrs
@@ -97,33 +92,37 @@ func (gcv *GlobalConfigurationValidator) checkForDuplicateName(listenerNames set
 }
 
 // checkIPPortProtocolConflicts ensures no duplicate or conflicting port/protocol combinations exist.
-func (gcv *GlobalConfigurationValidator) checkIPPortProtocolConflicts(combinations map[string]map[int]string, ipType ipType, listener conf_v1.Listener, fieldPath *field.Path) *field.Error {
+func (gcv *GlobalConfigurationValidator) checkIPPortProtocolConflicts(combinations map[string]map[int][]string, ipType ipType, listener conf_v1.Listener, fieldPath *field.Path) *field.Error {
 	ip := getIP(ipType, listener)
-
 	if combinations[ip] == nil {
-		combinations[ip] = make(map[int]string) // map[ip]map[port]protocol
+		combinations[ip] = make(map[int][]string) // map[ip]map[port][]protocol
 	}
-
-	existingProtocol, exists := combinations[ip][listener.Port]
-	if exists {
-		if existingProtocol == listener.Protocol {
-			return field.Duplicate(fieldPath, fmt.Sprintf("Listener %s: Duplicated port/protocol combination %d/%s", listener.Name, listener.Port, listener.Protocol))
-		} else if listener.Protocol == "HTTP" || existingProtocol == "HTTP" {
-			return field.Invalid(fieldPath.Child("port"), listener.Port, fmt.Sprintf("Listener %s: Port %d is used with a different protocol (current: %s, new: %s)", listener.Name, listener.Port, existingProtocol, listener.Protocol))
+	existingProtocols, exists := combinations[ip][listener.Port]
+	if !exists {
+		return nil
+	}
+	for _, existingProtocol := range existingProtocols {
+		switch listener.Protocol {
+		case "HTTP", "TCP":
+			if existingProtocol == "HTTP" || existingProtocol == "TCP" {
+				return field.Invalid(fieldPath.Child("protocol"), listener.Protocol, fmt.Sprintf("Listener %s: Duplicated ip:port protocol combination %s:%d %s", listener.Name, ip, listener.Port, listener.Protocol))
+			}
+		case "UDP":
+			if existingProtocol == "UDP" {
+				return field.Invalid(fieldPath.Child("protocol"), listener.Protocol, fmt.Sprintf("Listener %s: Duplicated ip:port protocol combination %s:%d %s", listener.Name, ip, listener.Port, listener.Protocol))
+			}
 		}
 	}
-
 	return nil
 }
 
 // updatePortProtocolCombinations updates the port/protocol combinations map with the given listener's details for both IPv4 and IPv6.
-func (gcv *GlobalConfigurationValidator) updatePortProtocolCombinations(combinations map[string]map[int]string, ipType ipType, listener conf_v1.Listener) {
+func (gcv *GlobalConfigurationValidator) updatePortProtocolCombinations(combinations map[string]map[int][]string, ipType ipType, listener conf_v1.Listener) {
 	ip := getIP(ipType, listener)
-
 	if combinations[ip] == nil {
-		combinations[ip] = make(map[int]string)
+		combinations[ip] = make(map[int][]string)
 	}
-	combinations[ip][listener.Port] = listener.Protocol
+	combinations[ip][listener.Port] = append(combinations[ip][listener.Port], listener.Protocol)
 }
 
 // getIP returns the appropriate IP address for the given ipType and listener.

pkg/apis/configuration/validation/globalconfiguration_test.go

@@ -234,6 +234,27 @@ func TestValidateListeners_PassesOnValidIPListeners(t *testing.T) {
 				{Name: "listener-2", IPv6IP: "2001:0db8:85a3:0000:0000:8a2e:0370:7334", Port: 8080, Protocol: "HTTP"},
 			},
 		},
+		{
+			name: "UDP and HTTP Listeners with Same Port",
+			listeners: []conf_v1.Listener{
+				{Name: "listener-1", IPv4IP: "127.0.0.1", Port: 8080, Protocol: "UDP"},
+				{Name: "listener-2", IPv4IP: "127.0.0.1", Port: 8080, Protocol: "HTTP"},
+			},
+		},
+		{
+			name: "HTTP Listeners with Same Port but different IPv4 and IPv6 ip addresses",
+			listeners: []conf_v1.Listener{
+				{Name: "listener-1", IPv4IP: "127.0.0.2", IPv6IP: "::1", Port: 8080, Protocol: "HTTP"},
+				{Name: "listener-2", IPv4IP: "127.0.0.1", Port: 8080, Protocol: "HTTP"},
+			},
+		},
+		{
+			name: "UDP and TCP Listeners with Same Port",
+			listeners: []conf_v1.Listener{
+				{Name: "listener-1", IPv4IP: "127.0.0.1", Port: 8080, Protocol: "UDP"},
+				{Name: "listener-2", IPv4IP: "127.0.0.1", Port: 8080, Protocol: "TCP"},
+			},
+		},
 	}
 
 	gcv := createGlobalConfigurationValidator()
@@ -587,7 +608,7 @@ func TestValidateListenerProtocol_FailsOnHttpListenerUsingSamePortAsTCPListener(
 	}
 }
 
-func TestValidateListenerProtocol_FailsOnHttpListenerUsingSamePortAsUDPListener(t *testing.T) {
+func TestValidateListenerProtocol_PassesOnHttpListenerUsingSamePortAsUDPListener(t *testing.T) {
 	t.Parallel()
 	listeners := []conf_v1.Listener{
 		{
@@ -607,18 +628,23 @@ func TestValidateListenerProtocol_FailsOnHttpListenerUsingSamePortAsUDPListener(
 			Port:     53,
 			Protocol: "UDP",
 		},
+		{
+			Name:     "http-listener",
+			Port:     53,
+			Protocol: "HTTP",
+		},
 	}
 	gcv := createGlobalConfigurationValidator()
 	listeners, allErrs := gcv.getValidListeners(listeners, field.NewPath("listeners"))
 	if diff := cmp.Diff(listeners, wantListeners); diff != "" {
 		t.Errorf("getValidListeners() returned unexpected result: (-want +got):\n%s", diff)
 	}
-	if len(allErrs) == 0 {
-		t.Errorf("validateListeners() returned no errors %v for invalid input", allErrs)
+	if len(allErrs) != 0 {
+		t.Errorf("validateListeners() returned errors %v invalid input", allErrs)
 	}
 }
 
-func TestValidateListenerProtocol_FailsOnHttpListenerUsingSamePortAsTCPAndUDPListener(t *testing.T) {
+func TestValidateListenerProtocol_FailsOnHttpListenerUsingSamePortAsTCP(t *testing.T) {
 	t.Parallel()
 	listeners := []conf_v1.Listener{
 		{
@@ -649,15 +675,13 @@ func TestValidateListenerProtocol_FailsOnHttpListenerUsingSamePortAsTCPAndUDPLis
 			Protocol: "UDP",
 		},
 	}
-
 	gcv := createGlobalConfigurationValidator()
-
 	listeners, allErrs := gcv.getValidListeners(listeners, field.NewPath("listeners"))
 	if diff := cmp.Diff(listeners, wantListeners); diff != "" {
 		t.Errorf("getValidListeners() returned unexpected result: (-want +got):\n%s", diff)
 	}
-	if len(allErrs) == 0 {
-		t.Errorf("validateListeners() returned no errors %v for invalid input", allErrs)
+	if len(allErrs) != 1 {
+		t.Errorf("getValidListeners() returned unexpected number of errors. Got %d, want 1", len(allErrs))
 	}
 }
 
@@ -694,7 +718,7 @@ func TestValidateListenerProtocol_FailsOnTCPListenerUsingSamePortAsHTTPListener(
 	}
 }
 
-func TestValidateListenerProtocol_FailsOnUDPListenerUsingSamePortAsHTTPListener(t *testing.T) {
+func TestValidateListenerProtocol_PassesOnUDPListenerUsingSamePortAsHTTPListener(t *testing.T) {
 	t.Parallel()
 	listeners := []conf_v1.Listener{
 		{
@@ -714,6 +738,11 @@ func TestValidateListenerProtocol_FailsOnUDPListenerUsingSamePortAsHTTPListener(
 			Port:     53,
 			Protocol: "HTTP",
 		},
+		{
+			Name:     "udp-listener",
+			Port:     53,
+			Protocol: "UDP",
+		},
 	}
 
 	gcv := createGlobalConfigurationValidator()
@@ -722,7 +751,7 @@ func TestValidateListenerProtocol_FailsOnUDPListenerUsingSamePortAsHTTPListener(
 	if diff := cmp.Diff(listeners, wantListeners); diff != "" {
 		t.Errorf("getValidListeners() returned unexpected result: (-want +got):\n%s", diff)
 	}
-	if len(allErrs) == 0 {
-		t.Errorf("validateListeners() returned no errors %v for invalid input", allErrs)
+	if len(allErrs) != 0 {
+		t.Errorf("validateListeners() returned errors %v for valid input", allErrs)
 	}
 }

tests/data/udp-http-listeners-together/global-configuration.yaml

@@ -0,0 +1,13 @@
+apiVersion: k8s.nginx.org/v1
+kind: GlobalConfiguration
+metadata:
+  name: nginx-configuration
+  namespace: nginx-ingress
+spec:
+  listeners:
+  - name: udp-listener
+    port: 5454
+    protocol: UDP
+  - name: http-listener
+    port: 5454
+    protocol: HTTP

tests/data/udp-http-listeners-together/transport-server.yaml

@@ -0,0 +1,14 @@
+apiVersion: k8s.nginx.org/v1
+kind: TransportServer
+metadata:
+  name: transport-server
+spec:
+  listener:
+    name: udp-listener
+    protocol: UDP
+  upstreams:
+    - name: dns-app
+      service: coredns
+      port: 5353
+  action:
+    pass: dns-app

tests/data/udp-http-listeners-together/virtual-server.yaml

@@ -0,0 +1,22 @@
+apiVersion: k8s.nginx.org/v1
+kind: VirtualServer
+metadata:
+  name: virtual-server-status
+spec:
+  listener:
+    http: http-listener
+  host: virtual-server-status.example.com
+  upstreams:
+  - name: backend2
+    service: backend2-svc
+    port: 80
+  - name: backend1
+    service: backend1-svc
+    port: 80
+  routes:
+  - path: /backend1
+    action:
+      pass: backend1
+  - path: /backend2
+    action:
+      pass: backend2

tests/suite/test_udp_http_listeners_together.py

@@ -0,0 +1,104 @@
+import time
+
+import pytest
+from settings import TEST_DATA
+from suite.utils.custom_resources_utils import (
+    create_ts_from_yaml,
+    delete_ts,
+    patch_gc_from_yaml,
+    read_custom_resource,
+    read_ts,
+)
+from suite.utils.resources_utils import get_first_pod_name, get_ts_nginx_template_conf, wait_before_test
+from suite.utils.vs_vsr_resources_utils import get_vs_nginx_template_conf, patch_virtual_server_from_yaml
+
+
+@pytest.mark.vs
+@pytest.mark.ts
+@pytest.mark.parametrize(
+    "crd_ingress_controller, virtual_server_setup, transport_server_setup",
+    [
+        (
+            {
+                "type": "complete",
+                "extra_args": [
+                    "-enable-custom-resources",
+                    "-global-configuration=nginx-ingress/nginx-configuration",
+                ],
+            },
+            {"example": "virtual-server-status", "app_type": "simple"},
+            {"example": "transport-server-status", "app_type": "simple"},
+        )
+    ],
+    indirect=True,
+)
+class TestUDPandHTTPListenersTogether:
+    def test_udp_and_http_listeners_together(
+        self,
+        kube_apis,
+        ingress_controller_prerequisites,
+        crd_ingress_controller,
+        virtual_server_setup,
+        transport_server_setup,
+    ):
+
+        wait_before_test()
+        existing_ts = read_ts(kube_apis.custom_objects, transport_server_setup.namespace, transport_server_setup.name)
+        delete_ts(kube_apis.custom_objects, existing_ts, transport_server_setup.namespace)
+
+        global_config_file = f"{TEST_DATA}/udp-http-listeners-together/global-configuration.yaml"
+        transport_server_file = f"{TEST_DATA}/udp-http-listeners-together/transport-server.yaml"
+        virtual_server_file = f"{TEST_DATA}/udp-http-listeners-together/virtual-server.yaml"
+        gc_resource_name = "nginx-configuration"
+        gc_namespace = "nginx-ingress"
+
+        patch_gc_from_yaml(kube_apis.custom_objects, gc_resource_name, global_config_file, gc_namespace)
+        create_ts_from_yaml(kube_apis.custom_objects, transport_server_file, transport_server_setup.namespace)
+        patch_virtual_server_from_yaml(
+            kube_apis.custom_objects, "virtual-server-status", virtual_server_file, virtual_server_setup.namespace
+        )
+        wait_before_test()
+
+        ic_pod_name = get_first_pod_name(kube_apis.v1, ingress_controller_prerequisites.namespace)
+        ts_config = get_ts_nginx_template_conf(
+            kube_apis.v1,
+            transport_server_setup.namespace,
+            transport_server_setup.name,
+            ic_pod_name,
+            ingress_controller_prerequisites.namespace,
+        )
+        vs_config = get_vs_nginx_template_conf(
+            kube_apis.v1,
+            virtual_server_setup.namespace,
+            virtual_server_setup.vs_name,
+            ic_pod_name,
+            ingress_controller_prerequisites.namespace,
+        )
+        assert "listen 5454;" in vs_config
+        assert "listen 5454 udp;" in ts_config
+
+        for _ in range(30):
+            transport_server_response = read_custom_resource(
+                kube_apis.custom_objects,
+                transport_server_setup.namespace,
+                "transportservers",
+                "transport-server",
+            )
+            if "status" in transport_server_response and transport_server_response["status"]["state"] == "Valid":
+                break
+            time.sleep(1)
+        else:
+            pytest.fail("TransportServer status did not become 'Valid' within the timeout period")
+
+        for _ in range(30):
+            virtual_server_response = read_custom_resource(
+                kube_apis.custom_objects,
+                virtual_server_setup.namespace,
+                "virtualservers",
+                "virtual-server-status",
+            )
+            if "status" in virtual_server_response and virtual_server_response["status"]["state"] == "Valid":
+                break
+            time.sleep(1)
+        else:
+            pytest.fail("VirtualServer status did not become 'Valid' within the timeout period")

tests/suite/test_virtual_server_custom_listeners.py

@@ -158,7 +158,7 @@ class TestVirtualServerCustomListeners:
                 "https_listener_in_config": True,
                 "expected_response_codes": [404, 404, 0, 200],
                 "expected_vs_error_msg": "Listener http-8085 is not defined in GlobalConfiguration",
-                "expected_gc_error_msg": "Listener http-8085: Duplicated port/protocol combination 8085/HTTP",
+                "expected_gc_error_msg": "Listener http-8085: Duplicated ip:port protocol combination 0.0.0.0:8085 HTTP",
             },
             {
                 "gc_yaml": "global-configuration-forbidden-port-http",
@@ -339,7 +339,7 @@ def test_custom_listeners(
                 "https_listener_in_config": True,
                 "expected_response_codes": [404, 404, 0, 200],
                 "expected_vs_error_msg": "Listener http-8085 is not defined in GlobalConfiguration",
-                "expected_gc_error_msg": "Listener http-8085: Duplicated port/protocol combination 8085/HTTP",
+                "expected_gc_error_msg": "Listener http-8085: Duplicated ip:port protocol combination 0.0.0.0:8085 HTTP",
             },
             {
                 "gc_yaml": "global-configuration-forbidden-port-http",