fix ListenerSetHostnameConflict/ListenerSetProtocolConflict conformance test #8361
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ import ( | |
| "github.com/google/cel-go/cel" | ||
| corev1 "k8s.io/api/core/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| utilerrors "k8s.io/apimachinery/pkg/util/errors" | ||
| "k8s.io/apimachinery/pkg/util/sets" | ||
| "k8s.io/utils/ptr" | ||
|
|
@@ -214,19 +215,118 @@ func validClientCertificateRef(ref *gwapiv1.SecretObjectReference) error { | |
| return nil | ||
| } | ||
|
|
||
| // allowedRouteKindsForProtocol returns the route kinds supported by the given listener protocol. | ||
| func allowedRouteKindsForProtocol(protocol gwapiv1.ProtocolType, tlsMode *gwapiv1.TLSModeType) []gwapiv1.Kind { | ||
| switch protocol { | ||
| case gwapiv1.HTTPProtocolType, gwapiv1.HTTPSProtocolType: | ||
| return []gwapiv1.Kind{resource.KindHTTPRoute, resource.KindGRPCRoute} | ||
| case gwapiv1.TLSProtocolType: | ||
| if tlsMode != nil && *tlsMode == gwapiv1.TLSModePassthrough { | ||
| return []gwapiv1.Kind{resource.KindTLSRoute} | ||
| } | ||
| // Terminate mode or unspecified defaults to accept both TCP and TLS routes | ||
| return []gwapiv1.Kind{resource.KindTCPRoute, resource.KindTLSRoute} | ||
| case gwapiv1.TCPProtocolType: | ||
| return []gwapiv1.Kind{resource.KindTCPRoute} | ||
| case gwapiv1.UDPProtocolType: | ||
| return []gwapiv1.Kind{resource.KindUDPRoute} | ||
| default: | ||
| return nil | ||
| } | ||
| } | ||
|
|
||
| // validateProtocolRules validates protocol-specific constraints (hostname, TLS requirements). | ||
| // Returns true if all constraints are satisfied, false otherwise. | ||
| func validateProtocolRules(listener *ListenerContext) bool { | ||
| switch listener.Protocol { | ||
| case gwapiv1.HTTPProtocolType, gwapiv1.HTTPSProtocolType, gwapiv1.TLSProtocolType, | ||
| gwapiv1.TCPProtocolType, gwapiv1.UDPProtocolType: | ||
| // All supported protocols pass basic validation here. | ||
| // Protocol-specific constraints (TLS, hostname) are validated in | ||
| // validateTLSConfiguration and validateHostName respectively, | ||
| // where they can set proper conditions. | ||
| return true | ||
|
|
||
| default: | ||
| // Unsupported protocol handled separately | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| func (t *Translator) validateListenerSpec(listener *ListenerContext, resources *resource.Resources) bool { | ||
| // Validate listener spec directly without relying on conditions. | ||
| // Start with valid assumption and invalidate on failures. | ||
|
|
||
| // Phase 1: Validate fundamental rules | ||
| specValid := t.validateAllowedNamespaces(listener) | ||
| if !validateProtocolRules(listener) { | ||
| specValid = false | ||
| } | ||
|
|
||
| // Phase 2: Validate allowed routes based on protocol | ||
| if listener.Protocol == gwapiv1.HTTPProtocolType || | ||
| listener.Protocol == gwapiv1.HTTPSProtocolType || | ||
| listener.Protocol == gwapiv1.TCPProtocolType || | ||
| listener.Protocol == gwapiv1.UDPProtocolType || | ||
| listener.Protocol == gwapiv1.TLSProtocolType { | ||
| var tlsMode *gwapiv1.TLSModeType | ||
| if listener.TLS != nil { | ||
| tlsMode = listener.TLS.Mode | ||
| } | ||
| allowedKinds := allowedRouteKindsForProtocol(listener.Protocol, tlsMode) | ||
| if !t.validateAllowedRoutes(listener, allowedKinds...) { | ||
| specValid = false | ||
| } | ||
| } else { | ||
|
Comment on lines
+261
to
+280
Contributor
There was a problem hiding this comment. Can this be reworked or simplified? The else block effectively covers the !validateProtocolRules case if I'm reading correctly.
Member
Author
There was a problem hiding this comment. that's why I want merge #8577 first, will make this one cleaner. |
||
| // Unsupported protocol | ||
| specValid = false | ||
| listener.SetSupportedKinds() | ||
| listener.SetCondition( | ||
| gwapiv1.ListenerConditionAccepted, | ||
| metav1.ConditionFalse, | ||
| gwapiv1.ListenerReasonUnsupportedProtocol, | ||
| fmt.Sprintf("Protocol %s is unsupported, must be %s, %s, %s or %s.", listener.Protocol, | ||
| gwapiv1.HTTPProtocolType, gwapiv1.HTTPSProtocolType, gwapiv1.TCPProtocolType, gwapiv1.UDPProtocolType), | ||
| ) | ||
| } | ||
|
|
||
| // Phase 3: Validate TLS configuration details | ||
| if !t.validateTLSConfiguration(listener, resources) { | ||
| specValid = false | ||
| } | ||
|
|
||
| // Phase 4: Validate Hostname configuration | ||
| if !t.validateHostName(listener) { | ||
| specValid = false | ||
| } | ||
|
|
||
| return specValid | ||
| } | ||
|
|
||
| func (t *Translator) ProcessListeners(gateways []*GatewayContext, xdsIR resource.XdsIRMap, infraIR resource.InfraIRMap, resources *resource.Resources) { | ||
| // Infra IR proxy ports must be unique. | ||
| foundPorts := make(map[string][]*protocolPort) | ||
|
|
||
| // Phase 1: Validate each listener's spec independently. | ||
| // This must happen before conflict resolution so that invalid listeners | ||
| // don't block valid ones during conflict detection. | ||
| for _, gateway := range gateways { | ||
| for _, listener := range gateway.listeners { | ||
| listener.specValid = t.validateListenerSpec(listener, resources) | ||
| } | ||
| } | ||
|
|
||
| // Phase 2: Run conflict detection. | ||
| // Only listeners that haven't been marked as invalid will participate in conflict resolution. | ||
| t.validateConflictedProtocolsListeners(gateways) | ||
| t.validateConflictedLayer7Listeners(gateways) | ||
| t.validateConflictedLayer4Listeners(gateways, gwapiv1.TCPProtocolType) | ||
| t.validateConflictedLayer4Listeners(gateways, gwapiv1.UDPProtocolType) | ||
| if t.MergeGateways { | ||
| t.validateConflictedMergedListeners(gateways) | ||
| } | ||
|
|
||
| // Phase 3: Build IR for valid listeners. | ||
| for _, gateway := range gateways { | ||
| irKey := t.getIRKey(gateway.Gateway) | ||
|
|
||
|
|
@@ -235,57 +335,27 @@ func (t *Translator) ProcessListeners(gateways []*GatewayContext, xdsIR resource | |
| } | ||
| t.processProxyReadyListener(xdsIR[irKey], gateway.envoyProxy) | ||
| t.processProxyObservability(gateway, xdsIR[irKey], infraIR[irKey].Proxy, resources) | ||
| gatewayAttachedListenerSets := sets.New[string]() | ||
| for _, listener := range gateway.listeners { | ||
| // Finalize listener conditions and check readiness. | ||
| t.validateListenerConditions(listener) | ||
| if !listener.IsReady() { | ||
| // Skip invalid listeners from IR building | ||
| continue | ||
| } | ||
|
|
||
| // Skip listeners with invalid frontend TLS validation as they are not functional. | ||
| if listener.frontendTLSValidationInvalid() { | ||
| continue | ||
| } | ||
|
|
||
| if listener.isFromListenerSet() { | ||
| gatewayAttachedListenerSets.Insert(types.NamespacedName{ | ||
| Namespace: listener.listenerSet.Namespace, | ||
| Name: listener.listenerSet.Name, | ||
| }.String()) | ||
| } | ||
|
|
||
| address := netutils.IPv4ListenerAddress | ||
| ipFamily := getEnvoyIPFamily(gateway.envoyProxy) | ||
| if ipFamily != nil && (*ipFamily == egv1a1.IPv6 || *ipFamily == egv1a1.DualStack) { | ||
|
|
@@ -365,11 +435,35 @@ func (t *Translator) ProcessListeners(gateways []*GatewayContext, xdsIR resource | |
| foundPorts[irKey] = append(foundPorts[irKey], servicePort) | ||
| } | ||
| } | ||
| gateway.IncreaseAttachedListenerSets(uint32(len(gatewayAttachedListenerSets))) | ||
| } | ||
|
|
||
| t.checkOverlappingTLSConfig(gateways) | ||
| } | ||
|
|
||
| // isListenerReady returns true if the listener is ready (Accepted=True and Programmed=True or no conditions yet). | ||
| // A listener is not ready if it has Accepted=False or Programmed=False. | ||
| func isListenerReady(listener *ListenerContext) bool { | ||
| conditions := listener.GetConditions() | ||
|
|
||
| // No conditions yet means it will be set to ready during validation. | ||
| if len(conditions) == 0 { | ||
| return true | ||
| } | ||
|
|
||
| // Check if Accepted=False or Programmed=False exists. | ||
| for _, cond := range conditions { | ||
| if cond.Type == string(gwapiv1.ListenerConditionAccepted) && cond.Status == metav1.ConditionFalse { | ||
| return false | ||
| } | ||
| if cond.Type == string(gwapiv1.ListenerConditionProgrammed) && cond.Status == metav1.ConditionFalse { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| return true | ||
| } | ||
|
|
||
| // checkOverlappingTLSConfig checks for overlapping hostnames and certificates between listeners and sets | ||
| // the `OverlappingTLSConfig` condition if there are overlapping hostnames or certificates. | ||
| func (t *Translator) checkOverlappingTLSConfig(gateways []*GatewayContext) { | ||
|
|
@@ -378,7 +472,7 @@ func (t *Translator) checkOverlappingTLSConfig(gateways []*GatewayContext) { | |
| httpsListeners := []*ListenerContext{} | ||
| for _, gateway := range gateways { | ||
| for _, listener := range gateway.listeners { | ||
| if listener.Protocol == gwapiv1.HTTPSProtocolType && isListenerReady(listener) { | ||
| httpsListeners = append(httpsListeners, listener) | ||
| } | ||
| } | ||
|
|
@@ -393,7 +487,7 @@ func (t *Translator) checkOverlappingTLSConfig(gateways []*GatewayContext) { | |
| for _, gateway := range gateways { | ||
| httpsListeners := []*ListenerContext{} | ||
| for _, listener := range gateway.listeners { | ||
| if listener.Protocol == gwapiv1.HTTPSProtocolType && isListenerReady(listener) { | ||
| httpsListeners = append(httpsListeners, listener) | ||
| } | ||
| } | ||
|
|
@@ -422,10 +516,18 @@ func checkOverlappingHostnames(httpsListeners []*ListenerContext) { | |
| if overlappingListeners[i] != nil { | ||
| continue | ||
| } | ||
| // Skip listeners that are already marked as invalid from per-listener validation | ||
| if hasInvalidCondition(httpsListeners[i]) { | ||
| continue | ||
| } | ||
| for j := i + 1; j < len(httpsListeners); j++ { | ||
| if overlappingListeners[j] != nil { | ||
| continue | ||
| } | ||
| // Skip listeners that are already marked as invalid from per-listener validation | ||
| if hasInvalidCondition(httpsListeners[j]) { | ||
| continue | ||
| } | ||
| if httpsListeners[i].Port != httpsListeners[j].Port { | ||
| continue | ||
| } | ||
|
|
@@ -504,10 +606,18 @@ func checkOverlappingCertificates(httpsListeners []*ListenerContext) { | |
| if overlappingListeners[i] != nil { | ||
| continue | ||
| } | ||
| // Skip listeners that are already marked as invalid from per-listener validation | ||
| if hasInvalidCondition(httpsListeners[i]) { | ||
| continue | ||
| } | ||
| for j := i + 1; j < len(httpsListeners); j++ { | ||
| if overlappingListeners[j] != nil { | ||
| continue | ||
| } | ||
| // Skip listeners that are already marked as invalid from per-listener validation | ||
| if hasInvalidCondition(httpsListeners[j]) { | ||
| continue | ||
| } | ||
| if httpsListeners[i].Port != httpsListeners[j].Port { | ||
| continue | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we return here, arent we avoid an increment ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we reach here, that means new count is 0 and AttachedListenerSets is nil, so it's safe to skip.