Add port conflict resolver; remove hostname conflict resolver

Kate Osborn · Kate Osborn · commit 8e60f670e85b · 2023-06-20T10:36:23.000-06:00
diff --git a/docs/gateway-api-compatibility.md b/docs/gateway-api-compatibility.md
@@ -86,12 +86,12 @@ Fields:
       * `Accepted/True/Accepted`
       * `Accepted/False/UnsupportedProtocol`
       * `Accepted/False/InvalidCertificateRef`
-      * `Accepted/False/HostnameConflict`
+      * `Accepted/False/ProtocolConflict`
       * `Accepted/False/UnsupportedValue`: Custom reason for when a value of a field in a Listener is invalid or not supported.
       * `Accepted/False/GatewayConflict`: Custom reason for when the Gateway is ignored due to a conflicting Gateway. NKG only supports a single Gateway.
       * `ResolvedRefs/True/ResolvedRefs`
       * `ResolvedRefs/False/InvalidCertificateRef`
-      * `Conflicted/True/HostnameConflict`
+      * `Conflicted/True/ProtocolConflict`
       * `Conflicted/False/NoConflicts`
 
 ### HTTPRoute
diff --git a/internal/state/change_processor_test.go b/internal/state/change_processor_test.go
@@ -1590,6 +1590,16 @@ var _ = Describe("ChangeProcessor", func() {
 						}
 					}),
 				),
+				Entry("listener hostnames conflict",
+					createInvalidGateway(func(gw *v1beta1.Gateway) {
+						gw.Spec.Listeners = append(gw.Spec.Listeners, v1beta1.Listener{
+							Name:     "listener-80-2",
+							Hostname: nil,
+							Port:     80,
+							Protocol: v1beta1.HTTPProtocolType,
+						})
+					}),
+				),
 			)
 		})
 	})
diff --git a/internal/state/conditions/conditions.go b/internal/state/conditions/conditions.go
@@ -317,19 +317,20 @@ func NewListenerInvalidCertificateRef(msg string) []Condition {
 	}
 }
 
-// NewListenerConflictedHostname returns Conditions that indicate that a hostname of a Listener is conflicted.
-func NewListenerConflictedHostname(msg string) []Condition {
+// NewListenerProtocolConflict returns Conditions that indicate multiple Listeners are specified with the same
+// Listener port number, but have conflicting protocol specifications.
+func NewListenerProtocolConflict(msg string) []Condition {
 	return []Condition{
 		{
 			Type:    string(v1beta1.ListenerConditionAccepted),
 			Status:  metav1.ConditionFalse,
-			Reason:  string(v1beta1.ListenerReasonHostnameConflict),
+			Reason:  string(v1beta1.ListenerReasonProtocolConflict),
 			Message: msg,
 		},
 		{
 			Type:    string(v1beta1.ListenerConditionConflicted),
 			Status:  metav1.ConditionTrue,
-			Reason:  string(v1beta1.ListenerReasonHostnameConflict),
+			Reason:  string(v1beta1.ListenerReasonProtocolConflict),
 			Message: msg,
 		},
 	}
diff --git a/internal/state/graph/gateway_listener.go b/internal/state/graph/gateway_listener.go
@@ -68,7 +68,7 @@ func newListenerConfiguratorFactory(
 	gw *v1beta1.Gateway,
 	secretMemoryMgr secrets.SecretDiskMemoryManager,
 ) *listenerConfiguratorFactory {
-	sharedHostnameConflictResolver := createHostnameConflictResolver()
+	sharedPortConflictResolver := createPortConflictResolver()
 
 	return &listenerConfiguratorFactory{
 		unsupportedProtocol: &listenerConfigurator{
@@ -91,7 +91,7 @@ func newListenerConfiguratorFactory(
 				validateHTTPListener,
 			},
 			conflictResolvers: []listenerConflictResolver{
-				sharedHostnameConflictResolver,
+				sharedPortConflictResolver,
 			},
 		},
 		https: &listenerConfigurator{
@@ -102,7 +102,7 @@ func newListenerConfiguratorFactory(
 				createHTTPSListenerValidator(gw.Namespace),
 			},
 			conflictResolvers: []listenerConflictResolver{
-				sharedHostnameConflictResolver,
+				sharedPortConflictResolver,
 			},
 			externalReferenceResolvers: []listenerExternalReferenceResolver{
 				createExternalReferencesForTLSSecretsResolver(gw.Namespace, secretMemoryMgr),
@@ -335,31 +335,47 @@ func createHTTPSListenerValidator(gwNsName string) listenerValidator {
 	}
 }
 
-func createHostnameConflictResolver() listenerConflictResolver {
-	usedHostnamesByPort := make(map[v1beta1.PortNumber]map[string]*Listener)
+func createPortConflictResolver() listenerConflictResolver {
+	conflictedPorts := make(map[v1beta1.PortNumber]bool)
+	portProtocolOwner := make(map[v1beta1.PortNumber]v1beta1.ProtocolType)
+	listenersByPort := make(map[v1beta1.PortNumber][]*Listener)
+
+	format := "Multiple listeners for the same port %d specify different protocols; " +
+		"ensure only one protocol per port"
 
 	return func(l *Listener) {
 		port := l.Source.Port
-		h := getHostname(l.Source.Hostname)
 
-		if listenersForPort, portExists := usedHostnamesByPort[port]; portExists {
-			if holder, holderExists := listenersForPort[h]; holderExists {
-				l.Valid = false
-				holder.Valid = false // all listeners for the same hostname/port become conflicted
+		// if port is in map of conflictedPorts then we only need to set the current listener to invalid
+		if conflictedPorts[port] {
+			l.Valid = false
 
-				format := "Multiple listeners for the same port use the same hostname %q; " +
-					"ensure only one listener uses that hostname"
-				conflictedConds := conditions.NewListenerConflictedHostname(fmt.Sprintf(format, h))
-				holder.Conditions = append(holder.Conditions, conflictedConds...)
-				l.Conditions = append(l.Conditions, conflictedConds...)
-			}
+			conflictedConds := conditions.NewListenerProtocolConflict(fmt.Sprintf(format, port))
+			l.Conditions = append(l.Conditions, conflictedConds...)
+			return
+		}
+
+		// otherwise, we add the listener to the list of listeners for this port
+		// and then check if the protocol owner for the port is different from the current listener's protocol.
 
-			usedHostnamesByPort[port][h] = l
+		listenersByPort[port] = append(listenersByPort[port], l)
 
+		protocol, ok := portProtocolOwner[port]
+		if !ok {
+			portProtocolOwner[port] = l.Source.Protocol
 			return
 		}
 
-		usedHostnamesByPort[port] = map[string]*Listener{h: l}
+		// if protocol owner doesn't match the listener's protocol we mark the port as conflicted,
+		// and invalidate all listeners we've seen for this port.
+		if protocol != l.Source.Protocol {
+			conflictedPorts[port] = true
+			for _, l := range listenersByPort[port] {
+				l.Valid = false
+				conflictedConds := conditions.NewListenerProtocolConflict(fmt.Sprintf(format, port))
+				l.Conditions = append(l.Conditions, conflictedConds...)
+			}
+		}
 	}
 }
 
diff --git a/internal/state/graph/gateway_test.go b/internal/state/graph/gateway_test.go
@@ -218,15 +218,13 @@ func TestBuildGateway(t *testing.T) {
 
 	// foo http listeners
 	foo80Listener1 := createHTTPListener("foo-80-1", "foo.example.com", 80)
-	foo80Listener2 := createHTTPListener("foo-80-2", "foo.example.com", 80)
 	foo8080Listener := createHTTPListener("foo-8080", "foo.example.com", 8080)
 	foo8081Listener := createHTTPListener("foo-8081", "foo.example.com", 8081)
 	foo443Listener := createHTTPListener("foo-443", "foo.example.com", 443)
 
 	// foo https listeners
 	foo80HTTPSListener := createHTTPSListener("foo-80-https", "foo.example.com", 80, gatewayTLSConfig)
 	foo443HTTPSListener1 := createHTTPSListener("foo-443-https-1", "foo.example.com", 443, gatewayTLSConfig)
-	foo443HTTPSListener2 := createHTTPSListener("foo-443-https-2", "foo.example.com", 443, gatewayTLSConfig)
 	foo8443HTTPSListener := createHTTPSListener("foo-8443-https", "foo.example.com", 8443, gatewayTLSConfig)
 
 	// bar http listener
@@ -255,8 +253,11 @@ func TestBuildGateway(t *testing.T) {
 			"with an alphanumeric character (e.g. 'example.com', regex used for validation is " +
 			`'[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')`
 
-		conflictedHostnamesMsg = `Multiple listeners for the same port use the same hostname "foo.example.com"; ` +
-			"ensure only one listener uses that hostname"
+		conflict80PortMsg = "Multiple listeners for the same port 80 specify different protocols; " +
+			"ensure only one protocol per port"
+
+		conflict443PortMsg = "Multiple listeners for the same port 443 specify different protocols; " +
+			"ensure only one protocol per port"
 
 		secretPath = "/etc/nginx/secrets/test_secret"
 	)
@@ -550,9 +551,11 @@ func TestBuildGateway(t *testing.T) {
 				gatewayCfg{
 					listeners: []v1beta1.Listener{
 						foo80Listener1,
-						foo80Listener2,
+						bar80Listener,
+						foo443Listener,
+						foo80HTTPSListener,
 						foo443HTTPSListener1,
-						foo443HTTPSListener2,
+						bar443HTTPSListener,
 					},
 				},
 			),
@@ -564,78 +567,45 @@ func TestBuildGateway(t *testing.T) {
 						Source:     foo80Listener1,
 						Valid:      false,
 						Routes:     map[types.NamespacedName]*Route{},
-						Conditions: conditions.NewListenerConflictedHostname(conflictedHostnamesMsg),
-					},
-					"foo-80-2": {
-						Source:     foo80Listener2,
-						Valid:      false,
-						Routes:     map[types.NamespacedName]*Route{},
-						Conditions: conditions.NewListenerConflictedHostname(conflictedHostnamesMsg),
-					},
-					"foo-443-https-1": {
-						Source:     foo443HTTPSListener1,
-						Valid:      false,
-						Routes:     map[types.NamespacedName]*Route{},
-						Conditions: conditions.NewListenerConflictedHostname(conflictedHostnamesMsg),
-						SecretPath: "/etc/nginx/secrets/test_secret",
-					},
-					"foo-443-https-2": {
-						Source:     foo443HTTPSListener2,
-						Valid:      false,
-						Routes:     map[types.NamespacedName]*Route{},
-						Conditions: conditions.NewListenerConflictedHostname(conflictedHostnamesMsg),
-						SecretPath: "/etc/nginx/secrets/test_secret",
-					},
-				},
-				Valid: true,
-			},
-			name: "collisions; same hostname, port, and protocol",
-		},
-		{
-			gateway: createGateway(
-				gatewayCfg{
-					listeners: []v1beta1.Listener{
-						foo80Listener1,
-						foo443Listener,
-						foo80HTTPSListener,
-						foo443HTTPSListener1,
+						Conditions: conditions.NewListenerProtocolConflict(conflict80PortMsg),
 					},
-				},
-			),
-			gatewayClass: validGC,
-			expected: &Gateway{
-				Source: getLastCreatedGetaway(),
-				Listeners: map[string]*Listener{
-					"foo-80-1": {
-						Source:     foo80Listener1,
+					"bar-80": {
+						Source:     bar80Listener,
 						Valid:      false,
 						Routes:     map[types.NamespacedName]*Route{},
-						Conditions: conditions.NewListenerConflictedHostname(conflictedHostnamesMsg),
+						Conditions: conditions.NewListenerProtocolConflict(conflict80PortMsg),
 					},
 					"foo-443": {
 						Source:     foo443Listener,
 						Valid:      false,
 						Routes:     map[types.NamespacedName]*Route{},
-						Conditions: conditions.NewListenerConflictedHostname(conflictedHostnamesMsg),
+						Conditions: conditions.NewListenerProtocolConflict(conflict443PortMsg),
 					},
 					"foo-80-https": {
 						Source:     foo80HTTPSListener,
 						Valid:      false,
 						Routes:     map[types.NamespacedName]*Route{},
-						Conditions: conditions.NewListenerConflictedHostname(conflictedHostnamesMsg),
+						Conditions: conditions.NewListenerProtocolConflict(conflict80PortMsg),
 						SecretPath: "/etc/nginx/secrets/test_secret",
 					},
 					"foo-443-https-1": {
 						Source:     foo443HTTPSListener1,
 						Valid:      false,
 						Routes:     map[types.NamespacedName]*Route{},
-						Conditions: conditions.NewListenerConflictedHostname(conflictedHostnamesMsg),
+						Conditions: conditions.NewListenerProtocolConflict(conflict443PortMsg),
+						SecretPath: "/etc/nginx/secrets/test_secret",
+					},
+					"bar-443-https": {
+						Source:     bar443HTTPSListener,
+						Valid:      false,
+						Routes:     map[types.NamespacedName]*Route{},
+						Conditions: conditions.NewListenerProtocolConflict(conflict443PortMsg),
 						SecretPath: "/etc/nginx/secrets/test_secret",
 					},
 				},
 				Valid: true,
 			},
-			name: "collisions; same hostname and port but different protocols",
+			name: "port/protocol collisions",
 		},
 		{
 			gateway: createGateway(

Original file line number	Diff line number	Diff line change
`@@ -317,19 +317,20 @@ func NewListenerInvalidCertificateRef(msg string) []Condition {`
`317`	`317`	`}`
`318`	`318`	`}`
`319`	`319`
`320`		`-// NewListenerConflictedHostname returns Conditions that indicate that a hostname of a Listener is conflicted.`
`321`		`-func NewListenerConflictedHostname(msg string) []Condition {`
	`320`	`+// NewListenerProtocolConflict returns Conditions that indicate multiple Listeners are specified with the same`
	`321`	`+// Listener port number, but have conflicting protocol specifications.`
	`322`	`+func NewListenerProtocolConflict(msg string) []Condition {`
`322`	`323`	`return []Condition{`
`323`	`324`	`{`
`324`	`325`	`Type: string(v1beta1.ListenerConditionAccepted),`
`325`	`326`	`Status: metav1.ConditionFalse,`
`326`		`- Reason: string(v1beta1.ListenerReasonHostnameConflict),`
	`327`	`+ Reason: string(v1beta1.ListenerReasonProtocolConflict),`
`327`	`328`	`Message: msg,`
`328`	`329`	`},`
`329`	`330`	`{`
`330`	`331`	`Type: string(v1beta1.ListenerConditionConflicted),`
`331`	`332`	`Status: metav1.ConditionTrue,`
`332`		`- Reason: string(v1beta1.ListenerReasonHostnameConflict),`
	`333`	`+ Reason: string(v1beta1.ListenerReasonProtocolConflict),`
`333`	`334`	`Message: msg,`
`334`	`335`	`},`
`335`	`336`	`}`