Skip to content

Fix udp/http listener validation logic #6406

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 17 commits into from
Sep 17, 2024
Merged

Fix udp/http listener validation logic #6406

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
d996555
fix udp/http listener validation logic
Sep 12, 2024
57a9d76
lint and change field name
Sep 12, 2024
294be4a
fix listener validation algo
Sep 12, 2024
9d3c9ca
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Sep 12, 2024
279d8b1
remove values.yaml change
Sep 12, 2024
2d13f1d
remove values.yaml change
Sep 12, 2024
737a7a3
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Sep 12, 2024
0637f16
fix test
Sep 12, 2024
c86a627
Merge branch 'main' into fix/udp-http-listener-validation-logic
jjngx Sep 13, 2024
d571c46
add more tests
Sep 13, 2024
ee6cf80
remove commented out listener
Sep 13, 2024
538ec90
fix test
Sep 16, 2024
0f9cc4d
delete comment
Sep 16, 2024
3636bce
Merge branch 'main' into fix/udp-http-listener-validation-logic
Sep 16, 2024
264ec42
improve error message for duplicate ip port protocol combination
Sep 17, 2024
8042336
improve error message for duplicate ip port protocol combination
Sep 17, 2024
d91e730
Merge branch 'main' into fix/udp-http-listener-validation-logic
Sep 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 23 additions & 24 deletions pkg/apis/configuration/validation/globalconfiguration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 %d/%s", listener.Name, 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 %d/%s", listener.Name, 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.
Expand Down
44 changes: 33 additions & 11 deletions pkg/apis/configuration/validation/globalconfiguration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,20 @@ 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: "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()
Expand Down Expand Up @@ -587,7 +601,7 @@ func TestValidateListenerProtocol_FailsOnHttpListenerUsingSamePortAsTCPListener(
}
}

func TestValidateListenerProtocol_FailsOnHttpListenerUsingSamePortAsUDPListener(t *testing.T) {
func TestValidateListenerProtocol_PassesOnHttpListenerUsingSamePortAsUDPListener(t *testing.T) {
t.Parallel()
listeners := []conf_v1.Listener{
{
Expand All @@ -607,18 +621,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{
{
Expand Down Expand Up @@ -649,15 +668,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))
}
}

Expand Down Expand Up @@ -694,7 +711,7 @@ func TestValidateListenerProtocol_FailsOnTCPListenerUsingSamePortAsHTTPListener(
}
}

func TestValidateListenerProtocol_FailsOnUDPListenerUsingSamePortAsHTTPListener(t *testing.T) {
func TestValidateListenerProtocol_PassesOnUDPListenerUsingSamePortAsHTTPListener(t *testing.T) {
t.Parallel()
listeners := []conf_v1.Listener{
{
Expand All @@ -714,6 +731,11 @@ func TestValidateListenerProtocol_FailsOnUDPListenerUsingSamePortAsHTTPListener(
Port: 53,
Protocol: "HTTP",
},
{
Name: "udp-listener",
Port: 53,
Protocol: "UDP",
},
}

gcv := createGlobalConfigurationValidator()
Expand All @@ -722,7 +744,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)
}
}
4 changes: 2 additions & 2 deletions tests/suite/test_virtual_server_custom_listeners.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 8085/HTTP",
},
{
"gc_yaml": "global-configuration-forbidden-port-http",
Expand Down Expand Up @@ -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 8085/HTTP",
},
{
"gc_yaml": "global-configuration-forbidden-port-http",
Expand Down