From 7ad0887f7c138e630302e8dae15e41a7a277d051 Mon Sep 17 00:00:00 2001 From: Michael Pleshakov Date: Mon, 27 Nov 2023 17:52:59 -0500 Subject: [PATCH] Fix attaching of HTTPRoutes to Gateway Listeners (#1275) Problem: - Rules for traffic attachment of HTTPRoutes to Gateways were clarified in https://github.com/kubernetes-sigs/gateway-api/pull/2396 -- successful attachment should depend only on parentRefs in an HTTPRoute and AllowedRoutes of a Listener, even if either or both of them are invalid. - The corresponding conformance test GatewayWithAttachedRoutes was added in https://github.com/kubernetes-sigs/gateway-api/pull/2477 , which fails for NGINX Gateway Fabric. - NGINX Gateway Fabric will not try to attach an HTTPRoute to a Listener if either or both of them are invalid. Solution: - Make NGF compliant with the Gateway API and make the corresponding test pass. - Introduce Attachable fields for Listener and HTTPRoute types of the Graph in the graph package. - Update the validation logic: - NGF considers a Listener attachable if (a) its hostname is valid, (b) protocol is supported by NGF and (c) AllowedRoutes are valid. - NGF considers an HTTPRoute attachable if (d) its hostnames are valid. - Attach an HTTPRoute to a Listener if both are attachable. Note: (a), (b) and (d) are not mentioned in https://github.com/kubernetes-sigs/gateway-api/pull/2396 However, they are necessary: For (b), NGF doesn't know how to attach to non-supported protocols like TCP. For (a), Listener hostname needed for HTTPRoute attaching, because it affects if an HTTPRoute can attach or not (per Gateway API spec). For (c), HTTPRoute hostnames are also needed, because they affect if an HTTPRoute can attach or not per Gateway API spec). See https://github.com/kubernetes-sigs/gateway-api/blob/52c2994ed9de1c287a37465490b91cfcf01bf16e/apis/v1/httproute_types.go#L71-L73 Testing: - Unit tests are updated and extended - Failing conformance test GatewayWithAttachedRoutes now passes. CLOSES https://github.com/nginxinc/nginx-gateway-fabric/issues/1148 Co-authored-by: Saylor Berman --- .../static/state/change_processor_test.go | 56 +++-- .../static/state/dataplane/configuration.go | 115 ++++++---- .../state/dataplane/configuration_test.go | 109 ++++++++- .../static/state/graph/gateway_listener.go | 73 +++--- .../state/graph/gateway_listener_test.go | 14 +- .../mode/static/state/graph/gateway_test.go | 108 ++++++--- .../mode/static/state/graph/graph_test.go | 20 +- internal/mode/static/state/graph/httproute.go | 44 ++-- .../mode/static/state/graph/httproute_test.go | 213 +++++++++++++++--- 9 files changed, 543 insertions(+), 209 deletions(-) diff --git a/internal/mode/static/state/change_processor_test.go b/internal/mode/static/state/change_processor_test.go index 8d1a39799..1e50371b2 100644 --- a/internal/mode/static/state/change_processor_test.go +++ b/internal/mode/static/state/change_processor_test.go @@ -407,7 +407,8 @@ var _ = Describe("ChangeProcessor", func() { ValidFilters: true, }, }, - Valid: true, + Valid: true, + Attachable: true, Conditions: []conditions.Condition{ staticConds.NewRouteBackendRefRefBackendNotFound( "spec.rules[0].backendRefs[0].name: Not found: \"service\"", @@ -434,8 +435,9 @@ var _ = Describe("ChangeProcessor", func() { Idx: 1, }, }, - Rules: []graph.Rule{{ValidMatches: true, ValidFilters: true}}, - Valid: true, + Rules: []graph.Rule{{ValidMatches: true, ValidFilters: true}}, + Valid: true, + Attachable: true, } // This is the base case expected graph. Tests will manipulate this to add or remove elements @@ -449,16 +451,18 @@ var _ = Describe("ChangeProcessor", func() { Source: gw1, Listeners: map[string]*graph.Listener{ "listener-80-1": { - Source: gw1.Spec.Listeners[0], - Valid: true, + Source: gw1.Spec.Listeners[0], + Valid: true, + Attachable: true, Routes: map[types.NamespacedName]*graph.Route{ {Namespace: "test", Name: "hr-1"}: expRouteHR1, }, SupportedKinds: []v1.RouteGroupKind{{Kind: "HTTPRoute"}}, }, "listener-443-1": { - Source: gw1.Spec.Listeners[1], - Valid: true, + Source: gw1.Spec.Listeners[1], + Valid: true, + Attachable: true, Routes: map[types.NamespacedName]*graph.Route{ {Namespace: "test", Name: "hr-1"}: expRouteHR1, }, @@ -541,32 +545,40 @@ var _ = Describe("ChangeProcessor", func() { It("returns updated graph", func() { processor.CaptureUpsertChange(gc) - // no ref grant exists yet for gw1 - expGraph.Gateway.Listeners["listener-443-1"] = &graph.Listener{ - Source: gw1.Spec.Listeners[1], - Valid: false, - Routes: map[types.NamespacedName]*graph.Route{}, - Conditions: staticConds.NewListenerRefNotPermitted( - "Certificate ref to secret cert-ns/different-ns-tls-secret not permitted by any ReferenceGrant", - ), - SupportedKinds: []v1.RouteGroupKind{{Kind: "HTTPRoute"}}, + // No ref grant exists yet for gw1 + // so the listener is not valid, but still attachable + expGraph.Gateway.Listeners["listener-443-1"].Valid = false + expGraph.Gateway.Listeners["listener-443-1"].ResolvedSecret = nil + expGraph.Gateway.Listeners["listener-443-1"].Conditions = staticConds.NewListenerRefNotPermitted( + "Certificate ref to secret cert-ns/different-ns-tls-secret not permitted by any ReferenceGrant", + ) + + expAttachment80 := &graph.ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "listener-80-1": {"foo.example.com"}, + }, + Attached: true, } - expAttachment := &graph.ParentRefAttachmentStatus{ - AcceptedHostnames: map[string][]string{}, - FailedCondition: staticConds.NewRouteInvalidListener(), - Attached: false, + expAttachment443 := &graph.ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "listener-443-1": {"foo.example.com"}, + }, + Attached: true, } - expGraph.Gateway.Listeners["listener-80-1"].Routes[hr1Name].ParentRefs[1].Attachment = expAttachment + expGraph.Gateway.Listeners["listener-80-1"].Routes[hr1Name].ParentRefs[0].Attachment = expAttachment80 + expGraph.Gateway.Listeners["listener-443-1"].Routes[hr1Name].ParentRefs[1].Attachment = expAttachment443 // no ref grant exists yet for hr1 - expGraph.Routes[hr1Name].ParentRefs[1].Attachment = expAttachment expGraph.Routes[hr1Name].Conditions = []conditions.Condition{ + staticConds.NewRouteInvalidListener(), staticConds.NewRouteBackendRefRefNotPermitted( "Backend ref to Service service-ns/service not permitted by any ReferenceGrant", ), } + expGraph.Routes[hr1Name].ParentRefs[0].Attachment = expAttachment80 + expGraph.Routes[hr1Name].ParentRefs[1].Attachment = expAttachment443 expGraph.ReferencedSecrets = nil diff --git a/internal/mode/static/state/dataplane/configuration.go b/internal/mode/static/state/dataplane/configuration.go index 887467b13..8c4eb4f0b 100644 --- a/internal/mode/static/state/dataplane/configuration.go +++ b/internal/mode/static/state/dataplane/configuration.go @@ -7,6 +7,7 @@ import ( apiv1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" v1 "sigs.k8s.io/gateway-api/apis/v1" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph" @@ -202,70 +203,80 @@ func (hpr *hostPathRules) upsertListener(l *graph.Listener) { hpr.httpsListeners = append(hpr.httpsListeners, l) } - for routeNsName, r := range l.Routes { - var hostnames []string - for _, p := range r.ParentRefs { - if val, exist := p.Attachment.AcceptedHostnames[string(l.Source.Name)]; exist { - hostnames = val - } + for _, r := range l.Routes { + if !r.Valid { + continue } - for _, h := range hostnames { - if prevListener, exists := hpr.listenersForHost[h]; exists { - // override the previous listener if the new one has a more specific hostname - if listenerHostnameMoreSpecific(l.Source.Hostname, prevListener.Source.Hostname) { - hpr.listenersForHost[h] = l - } - } else { - hpr.listenersForHost[h] = l - } + hpr.upsertRoute(r, l) + } +} - if _, exist := hpr.rulesPerHost[h]; !exist { - hpr.rulesPerHost[h] = make(map[pathAndType]PathRule) - } +func (hpr *hostPathRules) upsertRoute(route *graph.Route, listener *graph.Listener) { + var hostnames []string + for _, p := range route.ParentRefs { + if val, exist := p.Attachment.AcceptedHostnames[string(listener.Source.Name)]; exist { + hostnames = val } + } - for i, rule := range r.Source.Spec.Rules { - if !r.Rules[i].ValidMatches { - continue + for _, h := range hostnames { + if prevListener, exists := hpr.listenersForHost[h]; exists { + // override the previous listener if the new one has a more specific hostname + if listenerHostnameMoreSpecific(listener.Source.Hostname, prevListener.Source.Hostname) { + hpr.listenersForHost[h] = listener } + } else { + hpr.listenersForHost[h] = listener + } - var filters HTTPFilters - if r.Rules[i].ValidFilters { - filters = createHTTPFilters(rule.Filters) - } else { - filters = HTTPFilters{ - InvalidFilter: &InvalidHTTPFilter{}, - } + if _, exist := hpr.rulesPerHost[h]; !exist { + hpr.rulesPerHost[h] = make(map[pathAndType]PathRule) + } + } + + for i, rule := range route.Source.Spec.Rules { + if !route.Rules[i].ValidMatches { + continue + } + + var filters HTTPFilters + if route.Rules[i].ValidFilters { + filters = createHTTPFilters(rule.Filters) + } else { + filters = HTTPFilters{ + InvalidFilter: &InvalidHTTPFilter{}, } + } - for _, h := range hostnames { - for _, m := range rule.Matches { - path := getPath(m.Path) + for _, h := range hostnames { + for _, m := range rule.Matches { + path := getPath(m.Path) - key := pathAndType{ - path: path, - pathType: *m.Path.Type, - } + key := pathAndType{ + path: path, + pathType: *m.Path.Type, + } - rule, exist := hpr.rulesPerHost[h][key] - if !exist { - rule.Path = path - rule.PathType = convertPathType(*m.Path.Type) - } + rule, exist := hpr.rulesPerHost[h][key] + if !exist { + rule.Path = path + rule.PathType = convertPathType(*m.Path.Type) + } - // create iteration variable inside the loop to fix implicit memory aliasing - om := r.Source.ObjectMeta + // create iteration variable inside the loop to fix implicit memory aliasing + om := route.Source.ObjectMeta - rule.MatchRules = append(rule.MatchRules, MatchRule{ - Source: &om, - BackendGroup: newBackendGroup(r.Rules[i].BackendRefs, routeNsName, i), - Filters: filters, - Match: convertMatch(m), - }) + routeNsName := client.ObjectKeyFromObject(route.Source) - hpr.rulesPerHost[h][key] = rule - } + rule.MatchRules = append(rule.MatchRules, MatchRule{ + Source: &om, + BackendGroup: newBackendGroup(route.Rules[i].BackendRefs, routeNsName, i), + Filters: filters, + Match: convertMatch(m), + }) + + hpr.rulesPerHost[h][key] = rule } } } @@ -371,6 +382,10 @@ func buildUpstreams( } for _, route := range l.Routes { + if !route.Valid { + continue + } + for _, rule := range route.Rules { if !rule.ValidMatches || !rule.ValidFilters { // don't generate upstreams for rules that have invalid matches or filters diff --git a/internal/mode/static/state/dataplane/configuration_test.go b/internal/mode/static/state/dataplane/configuration_test.go index be671119c..3759e2ad7 100644 --- a/internal/mode/static/state/dataplane/configuration_test.go +++ b/internal/mode/static/state/dataplane/configuration_test.go @@ -138,6 +138,7 @@ func TestBuildConfiguration(t *testing.T) { r := &graph.Route{ Source: source, Rules: createRules(source, paths), + Valid: true, ParentRefs: []graph.ParentRef{ { Attachment: &graph.ParentRefAttachmentStatus{ @@ -187,6 +188,14 @@ func TestBuildConfiguration(t *testing.T) { "listener-80-1", pathAndType{path: "/", pathType: prefix}, ) + hr1Invalid, _, routeHR1Invalid := createTestResources( + "hr-1", + "foo.example.com", + "listener-80-1", + pathAndType{path: "/", pathType: prefix}, + ) + routeHR1Invalid.Valid = false + hr2, expHR2Groups, routeHR2 := createTestResources( "hr-2", "bar.example.com", @@ -254,6 +263,13 @@ func TestBuildConfiguration(t *testing.T) { "listener-443-1", pathAndType{path: "/", pathType: prefix}, ) + httpsHR1Invalid, _, httpsRouteHR1Invalid := createTestResources( + "https-hr-1", + "foo.example.com", + "listener-443-1", + pathAndType{path: "/", pathType: prefix}, + ) + httpsRouteHR1Invalid.Valid = false httpsHR2, expHTTPSHR2Groups, httpsRouteHR2 := createTestResources( "https-hr-2", @@ -465,6 +481,66 @@ func TestBuildConfiguration(t *testing.T) { }, msg: "http listener with no routes", }, + { + graph: &graph.Graph{ + GatewayClass: &graph.GatewayClass{ + Source: &v1.GatewayClass{}, + Valid: true, + }, + Gateway: &graph.Gateway{ + Source: &v1.Gateway{}, + Listeners: map[string]*graph.Listener{ + "listener-80-1": { + Source: listener80, + Valid: true, + Routes: map[types.NamespacedName]*graph.Route{ + client.ObjectKeyFromObject(hr1Invalid): routeHR1Invalid, + }, + }, + "listener-443-1": { + Source: listener443, // nil hostname + Valid: true, + Routes: map[types.NamespacedName]*graph.Route{ + client.ObjectKeyFromObject(httpsHR1Invalid): httpsRouteHR1Invalid, + }, + ResolvedSecret: &secret1NsName, + }, + }, + }, + Routes: map[types.NamespacedName]*graph.Route{ + client.ObjectKeyFromObject(hr1Invalid): routeHR1Invalid, + }, + ReferencedSecrets: map[types.NamespacedName]*graph.Secret{ + secret1NsName: secret1, + }, + }, + expConf: Configuration{ + HTTPServers: []VirtualServer{ + { + IsDefault: true, + Port: 80, + }, + }, + SSLServers: []VirtualServer{ + { + IsDefault: true, + Port: 443, + }, + { + Hostname: wildcardHostname, + SSL: &SSL{KeyPairID: "ssl_keypair_test_secret-1"}, + Port: 443, + }, + }, + SSLKeyPairs: map[SSLKeyPairID]SSLKeyPair{ + "ssl_keypair_test_secret-1": { + Cert: []byte("cert-1"), + Key: []byte("privateKey-1"), + }, + }, + }, + msg: "http and https listeners with no valid routes", + }, { graph: &graph.Graph{ GatewayClass: &graph.GatewayClass{ @@ -1749,6 +1825,13 @@ func TestBuildUpstreams(t *testing.T) { }, } + abcEndpoints := []resolver.Endpoint{ + { + Address: "14.0.0.0", + Port: 80, + }, + } + createBackendRefs := func(serviceNames ...string) []graph.BackendRef { var backends []graph.BackendRef for _, name := range serviceNames { @@ -1775,36 +1858,50 @@ func TestBuildUpstreams(t *testing.T) { hr4Refs1 := createBackendRefs("baz2") - invalidRefs := createBackendRefs("invalid") + nonExistingRefs := createBackendRefs("non-existing") + + invalidHRRefs := createBackendRefs("abc") routes := map[types.NamespacedName]*graph.Route{ {Name: "hr1", Namespace: "test"}: { + Valid: true, Rules: refsToValidRules(hr1Refs0, hr1Refs1), }, {Name: "hr2", Namespace: "test"}: { + Valid: true, Rules: refsToValidRules(hr2Refs0, hr2Refs1), }, {Name: "hr3", Namespace: "test"}: { + Valid: true, Rules: refsToValidRules(hr3Refs0), }, } routes2 := map[types.NamespacedName]*graph.Route{ {Name: "hr4", Namespace: "test"}: { + Valid: true, Rules: refsToValidRules(hr4Refs0, hr4Refs1), }, } + routesWithNonExistingRefs := map[types.NamespacedName]*graph.Route{ + {Name: "non-existing", Namespace: "test"}: { + Valid: true, + Rules: refsToValidRules(nonExistingRefs), + }, + } + invalidRoutes := map[types.NamespacedName]*graph.Route{ {Name: "invalid", Namespace: "test"}: { - Rules: refsToValidRules(invalidRefs), + Valid: false, + Rules: refsToValidRules(invalidHRRefs), }, } listeners := map[string]*graph.Listener{ "invalid-listener": { Valid: false, - Routes: invalidRoutes, // shouldn't be included since listener is invalid + Routes: routesWithNonExistingRefs, // shouldn't be included since listener is invalid }, "listener-1": { Valid: true, @@ -1814,6 +1911,10 @@ func TestBuildUpstreams(t *testing.T) { Valid: true, Routes: routes2, }, + "listener-3": { + Valid: true, + Routes: invalidRoutes, // shouldn't be included since routes are invalid + }, } emptyEndpointsErrMsg := "empty endpoints error" @@ -1863,6 +1964,8 @@ func TestBuildUpstreams(t *testing.T) { return fooEndpoints, nil case "nil-endpoints": return nil, errors.New(nilEndpointsErrMsg) + case "abc": + return abcEndpoints, nil default: return nil, fmt.Errorf("unexpected service %s", svc.Name) } diff --git a/internal/mode/static/state/graph/gateway_listener.go b/internal/mode/static/state/graph/gateway_listener.go index 8927cbf67..4e6eb02ea 100644 --- a/internal/mode/static/state/graph/gateway_listener.go +++ b/internal/mode/static/state/graph/gateway_listener.go @@ -34,6 +34,9 @@ type Listener struct { // Valid shows whether the Listener is valid. // A Listener is considered valid if NGF can generate valid NGINX configuration for it. Valid bool + // Attachable shows whether Routes can attach to the Listener. + // Listener can be invalid but still attachable. + Attachable bool } func buildListeners( @@ -80,13 +83,13 @@ func newListenerConfiguratorFactory( return &listenerConfiguratorFactory{ unsupportedProtocol: &listenerConfigurator{ validators: []listenerValidator{ - func(listener v1.Listener) []conditions.Condition { + func(listener v1.Listener) ([]conditions.Condition, bool) { valErr := field.NotSupported( field.NewPath("protocol"), listener.Protocol, []string{string(v1.HTTPProtocolType), string(v1.HTTPSProtocolType)}, ) - return staticConds.NewListenerUnsupportedProtocol(valErr.Error()) + return staticConds.NewListenerUnsupportedProtocol(valErr.Error()), false /* not attachable */ }, }, }, @@ -119,7 +122,8 @@ func newListenerConfiguratorFactory( } // listenerValidator validates a listener. If the listener is invalid, the validator will return appropriate conditions. -type listenerValidator func(v1.Listener) []conditions.Condition +// It also returns whether the listener is attachable, which is independent of whether the listener is valid. +type listenerValidator func(v1.Listener) (conds []conditions.Condition, attachable bool) // listenerConflictResolver resolves conflicts between listeners. In case of a conflict, the resolver will make // the conflicting listeners invalid - i.e. it will modify the passed listener and the previously processed conflicting @@ -136,9 +140,7 @@ type listenerExternalReferenceResolver func(listener *Listener) // don't need to include the full path to the field (e.g. "spec.listeners[0].hostname"). They will include // a path starting from the field of a listener (e.g. "hostname", "tls.options"). type listenerConfigurator struct { - // validators must not depend on the order of execution. validators []listenerValidator - // conflictResolvers can depend on validators - they will only be executed if all validators pass. conflictResolvers []listenerConflictResolver // externalReferenceResolvers can depend on validators - they will only be executed if all validators pass. @@ -148,11 +150,18 @@ type listenerConfigurator struct { func (c *listenerConfigurator) configure(listener v1.Listener) *Listener { var conds []conditions.Condition + attachable := true + // validators might return different conditions, so we run them all. for _, validator := range c.validators { - conds = append(conds, validator(listener)...) + currConds, currAttachable := validator(listener) + conds = append(conds, currConds...) + + attachable = attachable && currAttachable } + valid := len(conds) == 0 + var allowedRouteSelector labels.Selector if selector := GetAllowedRouteLabelSelector(listener); selector != nil { var err error @@ -160,28 +169,26 @@ func (c *listenerConfigurator) configure(listener v1.Listener) *Listener { if err != nil { msg := fmt.Sprintf("invalid label selector: %s", err.Error()) conds = append(conds, staticConds.NewListenerUnsupportedValue(msg)...) + valid = false } } supportedKinds := getListenerSupportedKinds(listener) - if len(conds) > 0 { - return &Listener{ - Source: listener, - Conditions: conds, - Valid: false, - SupportedKinds: supportedKinds, - } - } - l := &Listener{ Source: listener, + Conditions: conds, AllowedRouteLabelSelector: allowedRouteSelector, Routes: make(map[types.NamespacedName]*Route), - Valid: true, + Valid: valid, + Attachable: attachable, SupportedKinds: supportedKinds, } + if !l.Valid { + return l + } + // resolvers might add different conditions to the listener, so we run them all. for _, resolver := range c.conflictResolvers { @@ -195,23 +202,23 @@ func (c *listenerConfigurator) configure(listener v1.Listener) *Listener { return l } -func validateListenerHostname(listener v1.Listener) []conditions.Condition { +func validateListenerHostname(listener v1.Listener) (conds []conditions.Condition, attachable bool) { if listener.Hostname == nil { - return nil + return nil, true } h := string(*listener.Hostname) if h == "" { - return nil + return nil, true } if err := validateHostname(h); err != nil { path := field.NewPath("hostname") valErr := field.Invalid(path, listener.Hostname, err.Error()) - return staticConds.NewListenerUnsupportedValue(valErr.Error()) + return staticConds.NewListenerUnsupportedValue(valErr.Error()), false } - return nil + return nil, true } func getAndValidateListenerSupportedKinds(listener v1.Listener) ( @@ -253,9 +260,9 @@ func getAndValidateListenerSupportedKinds(listener v1.Listener) ( return conds, supportedKinds } -func validateListenerAllowedRouteKind(listener v1.Listener) []conditions.Condition { - conds, _ := getAndValidateListenerSupportedKinds(listener) - return conds +func validateListenerAllowedRouteKind(listener v1.Listener) (conds []conditions.Condition, attachable bool) { + conds, _ = getAndValidateListenerSupportedKinds(listener) + return conds, len(conds) == 0 } func getListenerSupportedKinds(listener v1.Listener) []v1.RouteGroupKind { @@ -263,23 +270,21 @@ func getListenerSupportedKinds(listener v1.Listener) []v1.RouteGroupKind { return kinds } -func validateListenerLabelSelector(listener v1.Listener) []conditions.Condition { +func validateListenerLabelSelector(listener v1.Listener) (conds []conditions.Condition, attachable bool) { if listener.AllowedRoutes != nil && listener.AllowedRoutes.Namespaces != nil && listener.AllowedRoutes.Namespaces.From != nil && *listener.AllowedRoutes.Namespaces.From == v1.NamespacesFromSelector && listener.AllowedRoutes.Namespaces.Selector == nil { msg := "Listener's AllowedRoutes Selector must be set when From is set to type Selector" - return staticConds.NewListenerUnsupportedValue(msg) + return staticConds.NewListenerUnsupportedValue(msg), false } - return nil + return nil, true } func createHTTPListenerValidator(protectedPorts ProtectedPorts) listenerValidator { - return func(listener v1.Listener) []conditions.Condition { - var conds []conditions.Condition - + return func(listener v1.Listener) (conds []conditions.Condition, attachable bool) { if err := validateListenerPort(listener.Port, protectedPorts); err != nil { path := field.NewPath("port") valErr := field.Invalid(path, listener.Port, err.Error()) @@ -290,7 +295,7 @@ func createHTTPListenerValidator(protectedPorts ProtectedPorts) listenerValidato panicForBrokenWebhookAssumption(fmt.Errorf("tls is not nil for HTTP listener %q", listener.Name)) } - return conds + return conds, true } } @@ -307,9 +312,7 @@ func validateListenerPort(port v1.PortNumber, protectedPorts ProtectedPorts) err } func createHTTPSListenerValidator(protectedPorts ProtectedPorts) listenerValidator { - return func(listener v1.Listener) []conditions.Condition { - var conds []conditions.Condition - + return func(listener v1.Listener) (conds []conditions.Condition, attachable bool) { if err := validateListenerPort(listener.Port, protectedPorts); err != nil { path := field.NewPath("port") valErr := field.Invalid(path, listener.Port, err.Error()) @@ -364,7 +367,7 @@ func createHTTPSListenerValidator(protectedPorts ProtectedPorts) listenerValidat conds = append(conds, staticConds.NewListenerUnsupportedValue(valErr.Error())...) } - return conds + return conds, true } } diff --git a/internal/mode/static/state/graph/gateway_listener_test.go b/internal/mode/static/state/graph/gateway_listener_test.go index 7adb93590..4e54ade56 100644 --- a/internal/mode/static/state/graph/gateway_listener_test.go +++ b/internal/mode/static/state/graph/gateway_listener_test.go @@ -52,9 +52,10 @@ func TestValidateHTTPListener(t *testing.T) { v := createHTTPListenerValidator(protectedPorts) - result := v(test.l) + result, attachable := v(test.l) g.Expect(result).To(Equal(test.expected)) + g.Expect(attachable).To(BeTrue()) }) } } @@ -195,8 +196,9 @@ func TestValidateHTTPSListener(t *testing.T) { v := createHTTPSListenerValidator(protectedPorts) - result := v(test.l) + result, attachable := v(test.l) g.Expect(result).To(Equal(test.expected)) + g.Expect(attachable).To(BeTrue()) }) } } @@ -238,12 +240,14 @@ func TestValidateListenerHostname(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewWithT(t) - conds := validateListenerHostname(v1.Listener{Hostname: test.hostname}) + conds, attachable := validateListenerHostname(v1.Listener{Hostname: test.hostname}) if test.expectErr { g.Expect(conds).ToNot(BeEmpty()) + g.Expect(attachable).To(BeFalse()) } else { g.Expect(conds).To(BeEmpty()) + g.Expect(attachable).To(BeTrue()) } }) } @@ -405,11 +409,13 @@ func TestValidateListenerLabelSelector(t *testing.T) { }, } - conds := validateListenerLabelSelector(listener) + conds, attachable := validateListenerLabelSelector(listener) if test.expectErr { g.Expect(conds).ToNot(BeEmpty()) + g.Expect(attachable).To(BeFalse()) } else { g.Expect(conds).To(BeEmpty()) + g.Expect(attachable).To(BeTrue()) } }) } diff --git a/internal/mode/static/state/graph/gateway_test.go b/internal/mode/static/state/graph/gateway_test.go index fa550c085..c378d7118 100644 --- a/internal/mode/static/state/graph/gateway_test.go +++ b/internal/mode/static/state/graph/gateway_test.go @@ -297,7 +297,7 @@ func TestBuildGateway(t *testing.T) { 443, tlsConfigInvalidSecret, ) - invalidHTTPSPortListener := createHTTPSListener("invalid-https-port", "foo.example.com", 0, gatewayTLSConfigSameNs) + invalidHTTPSPortListener := createHTTPSListener("invalid-https-port", "foo.example.com", 65536, gatewayTLSConfigSameNs) const ( invalidHostnameMsg = `hostname: Invalid value: "$example.com": a lowercase RFC 1123 subdomain ` + @@ -356,17 +356,19 @@ func TestBuildGateway(t *testing.T) { Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ "foo-80-1": { - Source: foo80Listener1, - Valid: true, - Routes: map[types.NamespacedName]*Route{}, + Source: foo80Listener1, + Valid: true, + Attachable: true, + Routes: map[types.NamespacedName]*Route{}, SupportedKinds: []v1.RouteGroupKind{ {Kind: "HTTPRoute"}, }, }, "foo-8080": { - Source: foo8080Listener, - Valid: true, - Routes: map[types.NamespacedName]*Route{}, + Source: foo8080Listener, + Valid: true, + Attachable: true, + Routes: map[types.NamespacedName]*Route{}, SupportedKinds: []v1.RouteGroupKind{ {Kind: "HTTPRoute"}, }, @@ -387,6 +389,7 @@ func TestBuildGateway(t *testing.T) { "foo-443-https-1": { Source: foo443HTTPSListener1, Valid: true, + Attachable: true, Routes: map[types.NamespacedName]*Route{}, ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), SupportedKinds: []v1.RouteGroupKind{ @@ -396,6 +399,7 @@ func TestBuildGateway(t *testing.T) { "foo-8443-https": { Source: foo8443HTTPSListener, Valid: true, + Attachable: true, Routes: map[types.NamespacedName]*Route{}, ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), SupportedKinds: []v1.RouteGroupKind{ @@ -416,6 +420,7 @@ func TestBuildGateway(t *testing.T) { "listener-with-allowed-routes": { Source: listenerAllowedRoutes, Valid: true, + Attachable: true, AllowedRouteLabelSelector: labels.SelectorFromSet(labels.Set(labelSet)), Routes: map[types.NamespacedName]*Route{}, SupportedKinds: []v1.RouteGroupKind{ @@ -460,6 +465,7 @@ func TestBuildGateway(t *testing.T) { "listener-cross-ns-secret": { Source: crossNamespaceSecretListener, Valid: true, + Attachable: true, Routes: map[types.NamespacedName]*Route{}, ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretDiffNamespace)), SupportedKinds: []v1.RouteGroupKind{ @@ -478,8 +484,9 @@ func TestBuildGateway(t *testing.T) { Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ "listener-cross-ns-secret": { - Source: crossNamespaceSecretListener, - Valid: false, + Source: crossNamespaceSecretListener, + Valid: false, + Attachable: true, Conditions: staticConds.NewListenerRefNotPermitted( `Certificate ref to secret diff-ns/secret not permitted by any ReferenceGrant`, ), @@ -491,7 +498,7 @@ func TestBuildGateway(t *testing.T) { }, Valid: true, }, - name: "invalid https listener with cross-namespace secret; no reference grant", + name: "invalid attachable https listener with cross-namespace secret; no reference grant", }, { gateway: createGateway(gatewayCfg{listeners: []v1.Listener{listenerInvalidSelector}}), @@ -500,11 +507,13 @@ func TestBuildGateway(t *testing.T) { Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ "listener-with-invalid-selector": { - Source: listenerInvalidSelector, - Valid: false, + Source: listenerInvalidSelector, + Valid: false, + Attachable: true, Conditions: staticConds.NewListenerUnsupportedValue( `invalid label selector: "invalid" is not a valid label selector operator`, ), + Routes: map[types.NamespacedName]*Route{}, SupportedKinds: []v1.RouteGroupKind{ {Kind: "HTTPRoute", Group: helpers.GetPointer[v1.Group](v1.GroupName)}, }, @@ -512,7 +521,7 @@ func TestBuildGateway(t *testing.T) { }, Valid: true, }, - name: "http listener with invalid label selector", + name: "attachable http listener with invalid label selector", }, { gateway: createGateway(gatewayCfg{listeners: []v1.Listener{invalidProtocolListener}}), @@ -521,11 +530,13 @@ func TestBuildGateway(t *testing.T) { Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ "invalid-protocol": { - Source: invalidProtocolListener, - Valid: false, + Source: invalidProtocolListener, + Valid: false, + Attachable: false, Conditions: staticConds.NewListenerUnsupportedProtocol( `protocol: Unsupported value: "TCP": supported values: "HTTP", "HTTPS"`, ), + Routes: map[types.NamespacedName]*Route{}, SupportedKinds: []v1.RouteGroupKind{ {Kind: "HTTPRoute"}, }, @@ -550,31 +561,37 @@ func TestBuildGateway(t *testing.T) { Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ "invalid-port": { - Source: invalidPortListener, - Valid: false, + Source: invalidPortListener, + Valid: false, + Attachable: true, Conditions: staticConds.NewListenerUnsupportedValue( `port: Invalid value: 0: port must be between 1-65535`, ), + Routes: map[types.NamespacedName]*Route{}, SupportedKinds: []v1.RouteGroupKind{ {Kind: "HTTPRoute"}, }, }, "invalid-https-port": { - Source: invalidHTTPSPortListener, - Valid: false, + Source: invalidHTTPSPortListener, + Valid: false, + Attachable: true, Conditions: staticConds.NewListenerUnsupportedValue( - `port: Invalid value: 0: port must be between 1-65535`, + `port: Invalid value: 65536: port must be between 1-65535`, ), + Routes: map[types.NamespacedName]*Route{}, SupportedKinds: []v1.RouteGroupKind{ {Kind: "HTTPRoute"}, }, }, "invalid-protected-port": { - Source: invalidProtectedPortListener, - Valid: false, + Source: invalidProtectedPortListener, + Valid: false, + Attachable: true, Conditions: staticConds.NewListenerUnsupportedValue( `port: Invalid value: 9113: port is already in use as MetricsPort`, ), + Routes: map[types.NamespacedName]*Route{}, SupportedKinds: []v1.RouteGroupKind{ {Kind: "HTTPRoute"}, }, @@ -596,6 +613,7 @@ func TestBuildGateway(t *testing.T) { Source: invalidHostnameListener, Valid: false, Conditions: staticConds.NewListenerUnsupportedValue(invalidHostnameMsg), + Routes: map[types.NamespacedName]*Route{}, SupportedKinds: []v1.RouteGroupKind{ {Kind: "HTTPRoute"}, }, @@ -604,6 +622,7 @@ func TestBuildGateway(t *testing.T) { Source: invalidHTTPSHostnameListener, Valid: false, Conditions: staticConds.NewListenerUnsupportedValue(invalidHostnameMsg), + Routes: map[types.NamespacedName]*Route{}, SupportedKinds: []v1.RouteGroupKind{ {Kind: "HTTPRoute"}, }, @@ -620,9 +639,10 @@ func TestBuildGateway(t *testing.T) { Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ "invalid-tls-config": { - Source: invalidTLSConfigListener, - Valid: false, - Routes: map[types.NamespacedName]*Route{}, + Source: invalidTLSConfigListener, + Valid: false, + Attachable: true, + Routes: map[types.NamespacedName]*Route{}, Conditions: staticConds.NewListenerInvalidCertificateRef( `tls.certificateRefs[0]: Invalid value: test/does-not-exist: secret does not exist`, ), @@ -655,33 +675,37 @@ func TestBuildGateway(t *testing.T) { Source: getLastCreatedGetaway(), Listeners: map[string]*Listener{ "foo-80-1": { - Source: foo80Listener1, - Valid: true, - Routes: map[types.NamespacedName]*Route{}, + Source: foo80Listener1, + Valid: true, + Attachable: true, + Routes: map[types.NamespacedName]*Route{}, SupportedKinds: []v1.RouteGroupKind{ {Kind: "HTTPRoute"}, }, }, "foo-8080": { - Source: foo8080Listener, - Valid: true, - Routes: map[types.NamespacedName]*Route{}, + Source: foo8080Listener, + Valid: true, + Attachable: true, + Routes: map[types.NamespacedName]*Route{}, SupportedKinds: []v1.RouteGroupKind{ {Kind: "HTTPRoute"}, }, }, "foo-8081": { - Source: foo8081Listener, - Valid: true, - Routes: map[types.NamespacedName]*Route{}, + Source: foo8081Listener, + Valid: true, + Attachable: true, + Routes: map[types.NamespacedName]*Route{}, SupportedKinds: []v1.RouteGroupKind{ {Kind: "HTTPRoute"}, }, }, "bar-80": { - Source: bar80Listener, - Valid: true, - Routes: map[types.NamespacedName]*Route{}, + Source: bar80Listener, + Valid: true, + Attachable: true, + Routes: map[types.NamespacedName]*Route{}, SupportedKinds: []v1.RouteGroupKind{ {Kind: "HTTPRoute"}, }, @@ -689,6 +713,7 @@ func TestBuildGateway(t *testing.T) { "foo-443-https-1": { Source: foo443HTTPSListener1, Valid: true, + Attachable: true, Routes: map[types.NamespacedName]*Route{}, ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), SupportedKinds: []v1.RouteGroupKind{ @@ -698,6 +723,7 @@ func TestBuildGateway(t *testing.T) { "foo-8443-https": { Source: foo8443HTTPSListener, Valid: true, + Attachable: true, Routes: map[types.NamespacedName]*Route{}, ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), SupportedKinds: []v1.RouteGroupKind{ @@ -707,6 +733,7 @@ func TestBuildGateway(t *testing.T) { "bar-443-https": { Source: bar443HTTPSListener, Valid: true, + Attachable: true, Routes: map[types.NamespacedName]*Route{}, ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), SupportedKinds: []v1.RouteGroupKind{ @@ -716,6 +743,7 @@ func TestBuildGateway(t *testing.T) { "bar-8443-https": { Source: bar8443HTTPSListener, Valid: true, + Attachable: true, Routes: map[types.NamespacedName]*Route{}, ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), SupportedKinds: []v1.RouteGroupKind{ @@ -747,6 +775,7 @@ func TestBuildGateway(t *testing.T) { "foo-80-1": { Source: foo80Listener1, Valid: false, + Attachable: true, Routes: map[types.NamespacedName]*Route{}, Conditions: staticConds.NewListenerProtocolConflict(conflict80PortMsg), SupportedKinds: []v1.RouteGroupKind{ @@ -756,6 +785,7 @@ func TestBuildGateway(t *testing.T) { "bar-80": { Source: bar80Listener, Valid: false, + Attachable: true, Routes: map[types.NamespacedName]*Route{}, Conditions: staticConds.NewListenerProtocolConflict(conflict80PortMsg), SupportedKinds: []v1.RouteGroupKind{ @@ -765,6 +795,7 @@ func TestBuildGateway(t *testing.T) { "foo-443": { Source: foo443Listener, Valid: false, + Attachable: true, Routes: map[types.NamespacedName]*Route{}, Conditions: staticConds.NewListenerProtocolConflict(conflict443PortMsg), SupportedKinds: []v1.RouteGroupKind{ @@ -774,6 +805,7 @@ func TestBuildGateway(t *testing.T) { "foo-80-https": { Source: foo80HTTPSListener, Valid: false, + Attachable: true, Routes: map[types.NamespacedName]*Route{}, Conditions: staticConds.NewListenerProtocolConflict(conflict80PortMsg), ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), @@ -784,6 +816,7 @@ func TestBuildGateway(t *testing.T) { "foo-443-https-1": { Source: foo443HTTPSListener1, Valid: false, + Attachable: true, Routes: map[types.NamespacedName]*Route{}, Conditions: staticConds.NewListenerProtocolConflict(conflict443PortMsg), ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), @@ -794,6 +827,7 @@ func TestBuildGateway(t *testing.T) { "bar-443-https": { Source: bar443HTTPSListener, Valid: false, + Attachable: true, Routes: map[types.NamespacedName]*Route{}, Conditions: staticConds.NewListenerProtocolConflict(conflict443PortMsg), ResolvedSecret: helpers.GetPointer(client.ObjectKeyFromObject(secretSameNs)), diff --git a/internal/mode/static/state/graph/graph_test.go b/internal/mode/static/state/graph/graph_test.go index 84091377e..a1ec83713 100644 --- a/internal/mode/static/state/graph/graph_test.go +++ b/internal/mode/static/state/graph/graph_test.go @@ -231,8 +231,9 @@ func TestBuildGraph(t *testing.T) { } routeHR1 := &Route{ - Valid: true, - Source: hr1, + Valid: true, + Attachable: true, + Source: hr1, ParentRefs: []ParentRef{ { Idx: 0, @@ -247,8 +248,9 @@ func TestBuildGraph(t *testing.T) { } routeHR3 := &Route{ - Valid: true, - Source: hr3, + Valid: true, + Attachable: true, + Source: hr3, ParentRefs: []ParentRef{ { Idx: 0, @@ -272,16 +274,18 @@ func TestBuildGraph(t *testing.T) { Source: gw1, Listeners: map[string]*Listener{ "listener-80-1": { - Source: gw1.Spec.Listeners[0], - Valid: true, + Source: gw1.Spec.Listeners[0], + Valid: true, + Attachable: true, Routes: map[types.NamespacedName]*Route{ {Namespace: "test", Name: "hr-1"}: routeHR1, }, SupportedKinds: []gatewayv1.RouteGroupKind{{Kind: "HTTPRoute"}}, }, "listener-443-1": { - Source: gw1.Spec.Listeners[1], - Valid: true, + Source: gw1.Spec.Listeners[1], + Valid: true, + Attachable: true, Routes: map[types.NamespacedName]*Route{ {Namespace: "test", Name: "hr-3"}: routeHR3, }, diff --git a/internal/mode/static/state/graph/httproute.go b/internal/mode/static/state/graph/httproute.go index 4baa67e58..4f9da0fbc 100644 --- a/internal/mode/static/state/graph/httproute.go +++ b/internal/mode/static/state/graph/httproute.go @@ -68,6 +68,9 @@ type Route struct { // Valid tells if the Route is valid. // If it is invalid, NGF should not generate any configuration for it. Valid bool + // Attachable tells if the Route can be attached to any of the Gateways. + // Route can be invalid but still attachable. + Attachable bool } // buildRoutesForGateways builds routes from HTTPRoutes that reference any of the specified Gateways. @@ -191,6 +194,7 @@ func buildRoute( } r.Valid = true + r.Attachable = true r.Rules = make([]Rule, len(ghr.Spec.Rules)) @@ -260,7 +264,7 @@ func bindRoutesToListeners( } func bindRouteToListeners(r *Route, gw *Gateway, namespaces map[types.NamespacedName]*apiv1.Namespace) { - if !r.Valid { + if !r.Attachable { return } @@ -302,19 +306,25 @@ func bindRouteToListeners(r *Route, gw *Gateway, namespaces map[types.Namespaced // Case 4 - winning Gateway // Try to attach Route to all matching listeners + cond, attached := tryToAttachRouteToListeners(ref.Attachment, routeRef.SectionName, r, gw, namespaces) if !attached { attachment.FailedCondition = cond continue } + if cond != (conditions.Condition{}) { + r.Conditions = append(r.Conditions, cond) + } attachment.Attached = true } } // tryToAttachRouteToListeners tries to attach the route to the listeners that match the parentRef and the hostnames. -// If it succeeds in attaching at least one listener it will return true and the condition will be empty. -// If it fails to attach the route, it will return false and the failure condition. +// There are two cases: +// (1) If it succeeds in attaching at least one listener it will return true. The returned condition will be empty if +// at least one of the listeners is valid. Otherwise, it will return the failure condition. +// (2) If it fails to attach the route, it will return false and the failure condition. func tryToAttachRouteToListeners( refStatus *ParentRefAttachmentStatus, sectionName *v1.SectionName, @@ -322,13 +332,13 @@ func tryToAttachRouteToListeners( gw *Gateway, namespaces map[types.NamespacedName]*apiv1.Namespace, ) (conditions.Condition, bool) { - validListeners, listenerExists := findValidListeners(getSectionName(sectionName), gw.Listeners) + attachableListeners, listenerExists := findAttachableListeners(getSectionName(sectionName), gw.Listeners) if !listenerExists { return staticConds.NewRouteNoMatchingParent(), false } - if len(validListeners) == 0 { + if len(attachableListeners) == 0 { return staticConds.NewRouteInvalidListener(), false } @@ -348,11 +358,14 @@ func tryToAttachRouteToListeners( return true, true } + var attachedToAtLeastOneValidListener bool + var allowed, attached bool - for _, l := range validListeners { + for _, l := range attachableListeners { routeAllowed, routeAttached := bind(l) allowed = allowed || routeAllowed attached = attached || routeAttached + attachedToAtLeastOneValidListener = attachedToAtLeastOneValidListener || (routeAttached && l.Valid) } if !attached { @@ -362,34 +375,39 @@ func tryToAttachRouteToListeners( return staticConds.NewRouteNoMatchingListenerHostname(), false } + if !attachedToAtLeastOneValidListener { + return staticConds.NewRouteInvalidListener(), true + } + return conditions.Condition{}, true } -// findValidListeners returns a list of valid listeners and whether the listener exists for a non-empty sectionName. -func findValidListeners(sectionName string, listeners map[string]*Listener) ([]*Listener, bool) { +// findAttachableListeners returns a list of attachable listeners and whether the listener exists for a non-empty +// sectionName. +func findAttachableListeners(sectionName string, listeners map[string]*Listener) ([]*Listener, bool) { if sectionName != "" { l, exists := listeners[sectionName] if !exists { return nil, false } - if l.Valid { + if l.Attachable { return []*Listener{l}, true } return nil, true } - validListeners := make([]*Listener, 0, len(listeners)) + attachableListeners := make([]*Listener, 0, len(listeners)) for _, l := range listeners { - if !l.Valid { + if !l.Attachable { continue } - validListeners = append(validListeners, l) + attachableListeners = append(attachableListeners, l) } - return validListeners, true + return attachableListeners, true } func findAcceptedHostnames(listenerHostname *v1.Hostname, routeHostnames []v1.Hostname) []string { diff --git a/internal/mode/static/state/graph/httproute_test.go b/internal/mode/static/state/graph/httproute_test.go index fc4222f2e..b0a87f7e4 100644 --- a/internal/mode/static/state/graph/httproute_test.go +++ b/internal/mode/static/state/graph/httproute_test.go @@ -105,7 +105,8 @@ func TestBuildRoutes(t *testing.T) { Gateway: gwNsName, }, }, - Valid: true, + Valid: true, + Attachable: true, Rules: []Rule{ { ValidMatches: true, @@ -399,7 +400,8 @@ func TestBuildRoute(t *testing.T) { Gateway: gatewayNsName, }, }, - Valid: true, + Valid: true, + Attachable: true, Rules: []Rule{ { ValidMatches: true, @@ -423,8 +425,9 @@ func TestBuildRoute(t *testing.T) { validator: &validationfakes.FakeHTTPFieldsValidator{}, hr: hrInvalidHostname, expected: &Route{ - Source: hrInvalidHostname, - Valid: false, + Source: hrInvalidHostname, + Valid: false, + Attachable: false, ParentRefs: []ParentRef{ { Idx: 0, @@ -443,8 +446,9 @@ func TestBuildRoute(t *testing.T) { validator: validatorInvalidFieldsInRule, hr: hrInvalidMatches, expected: &Route{ - Source: hrInvalidMatches, - Valid: false, + Source: hrInvalidMatches, + Valid: false, + Attachable: true, ParentRefs: []ParentRef{ { Idx: 0, @@ -469,8 +473,9 @@ func TestBuildRoute(t *testing.T) { validator: validatorInvalidFieldsInRule, hr: hrInvalidFilters, expected: &Route{ - Source: hrInvalidFilters, - Valid: false, + Source: hrInvalidFilters, + Valid: false, + Attachable: true, ParentRefs: []ParentRef{ { Idx: 0, @@ -496,8 +501,9 @@ func TestBuildRoute(t *testing.T) { validator: validatorInvalidFieldsInRule, hr: hrDroppedInvalidMatches, expected: &Route{ - Source: hrDroppedInvalidMatches, - Valid: true, + Source: hrDroppedInvalidMatches, + Valid: true, + Attachable: true, ParentRefs: []ParentRef{ { Idx: 0, @@ -527,8 +533,9 @@ func TestBuildRoute(t *testing.T) { validator: validatorInvalidFieldsInRule, hr: hrDroppedInvalidMatchesAndInvalidFilters, expected: &Route{ - Source: hrDroppedInvalidMatchesAndInvalidFilters, - Valid: true, + Source: hrDroppedInvalidMatchesAndInvalidFilters, + Valid: true, + Attachable: true, ParentRefs: []ParentRef{ { Idx: 0, @@ -563,8 +570,9 @@ func TestBuildRoute(t *testing.T) { validator: validatorInvalidFieldsInRule, hr: hrDroppedInvalidFilters, expected: &Route{ - Source: hrDroppedInvalidFilters, - Valid: true, + Source: hrDroppedInvalidFilters, + Valid: true, + Attachable: true, ParentRefs: []ParentRef{ { Idx: 0, @@ -612,8 +620,9 @@ func TestBindRouteToListeners(t *testing.T) { Name: gatewayv1.SectionName(name), Hostname: (*gatewayv1.Hostname)(helpers.GetPointer("foo.example.com")), }, - Valid: true, - Routes: map[types.NamespacedName]*Route{}, + Valid: true, + Attachable: true, + Routes: map[types.NamespacedName]*Route{}, } } createModifiedListener := func(name string, m func(*Listener)) *Listener { @@ -676,8 +685,9 @@ func TestBindRouteToListeners(t *testing.T) { var normalRoute *Route createNormalRoute := func(gateway *gatewayv1.Gateway) *Route { normalRoute = &Route{ - Source: hr, - Valid: true, + Source: hr, + Valid: true, + Attachable: true, ParentRefs: []ParentRef{ { Idx: 0, @@ -691,9 +701,33 @@ func TestBindRouteToListeners(t *testing.T) { return normalRoute } + invalidAttachableRoute1 := &Route{ + Source: hr, + Valid: false, + Attachable: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + }, + }, + } + invalidAttachableRoute2 := &Route{ + Source: hr, + Valid: false, + Attachable: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + }, + }, + } + routeWithMissingSectionName := &Route{ - Source: hrWithNilSectionName, - Valid: true, + Source: hrWithNilSectionName, + Valid: true, + Attachable: true, ParentRefs: []ParentRef{ { Idx: 0, @@ -702,8 +736,9 @@ func TestBindRouteToListeners(t *testing.T) { }, } routeWithEmptySectionName := &Route{ - Source: hrWithEmptySectionName, - Valid: true, + Source: hrWithEmptySectionName, + Valid: true, + Attachable: true, ParentRefs: []ParentRef{ { Idx: 0, @@ -712,8 +747,9 @@ func TestBindRouteToListeners(t *testing.T) { }, } routeWithNonExistingListener := &Route{ - Source: hrWithNonExistingListener, - Valid: true, + Source: hrWithNonExistingListener, + Valid: true, + Attachable: true, ParentRefs: []ParentRef{ { Idx: 0, @@ -722,8 +758,9 @@ func TestBindRouteToListeners(t *testing.T) { }, } routeWithPort := &Route{ - Source: hrWithPort, - Valid: true, + Source: hrWithPort, + Valid: true, + Attachable: true, ParentRefs: []ParentRef{ { Idx: 0, @@ -733,8 +770,9 @@ func TestBindRouteToListeners(t *testing.T) { } ignoredGwNsName := types.NamespacedName{Namespace: "test", Name: "ignored-gateway"} routeWithIgnoredGateway := &Route{ - Source: hr, - Valid: true, + Source: hr, + Valid: true, + Attachable: true, ParentRefs: []ParentRef{ { Idx: 0, @@ -742,7 +780,7 @@ func TestBindRouteToListeners(t *testing.T) { }, }, } - notValidRoute := &Route{ + invalidRoute := &Route{ Valid: false, ParentRefs: []ParentRef{ { @@ -752,8 +790,9 @@ func TestBindRouteToListeners(t *testing.T) { }, } - notValidListener := createModifiedListener("", func(l *Listener) { + invalidNotAttachableListener := createModifiedListener("", func(l *Listener) { l.Valid = false + l.Attachable = false }) nonMatchingHostnameListener := createModifiedListener("", func(l *Listener) { l.Source.Hostname = helpers.GetPointer[gatewayv1.Hostname]("bar.example.com") @@ -765,6 +804,7 @@ func TestBindRouteToListeners(t *testing.T) { expectedGatewayListeners map[string]*Listener name string expectedSectionNameRefs []ParentRef + expectedConditions []conditions.Condition }{ { route: createNormalRoute(gw), @@ -869,7 +909,7 @@ func TestBindRouteToListeners(t *testing.T) { Source: gw, Valid: true, Listeners: map[string]*Listener{ - "listener-80-1": notValidListener, + "listener-80-1": invalidNotAttachableListener, }, }, expectedSectionNameRefs: []ParentRef{ @@ -884,9 +924,9 @@ func TestBindRouteToListeners(t *testing.T) { }, }, expectedGatewayListeners: map[string]*Listener{ - "listener-80-1": notValidListener, + "listener-80-1": invalidNotAttachableListener, }, - name: "empty section name with no valid listeners", + name: "empty section name with no valid and attachable listeners", }, { route: routeWithPort, @@ -946,7 +986,7 @@ func TestBindRouteToListeners(t *testing.T) { Source: gw, Valid: true, Listeners: map[string]*Listener{ - "listener-80-1": notValidListener, + "listener-80-1": invalidNotAttachableListener, }, }, expectedSectionNameRefs: []ParentRef{ @@ -961,9 +1001,9 @@ func TestBindRouteToListeners(t *testing.T) { }, }, expectedGatewayListeners: map[string]*Listener{ - "listener-80-1": notValidListener, + "listener-80-1": invalidNotAttachableListener, }, - name: "listener isn't valid", + name: "listener isn't valid and attachable", }, { route: createNormalRoute(gw), @@ -1016,7 +1056,7 @@ func TestBindRouteToListeners(t *testing.T) { name: "gateway is ignored", }, { - route: notValidRoute, + route: invalidRoute, gateway: &Gateway{ Source: gw, Valid: true, @@ -1061,6 +1101,104 @@ func TestBindRouteToListeners(t *testing.T) { }, name: "invalid gateway", }, + { + route: createNormalRoute(gw), + gateway: &Gateway{ + Source: gw, + Valid: true, + Listeners: map[string]*Listener{ + "listener-80-1": createModifiedListener("listener-80-1", func(l *Listener) { + l.Valid = false + }), + }, + }, + expectedSectionNameRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + Attachment: &ParentRefAttachmentStatus{ + Attached: true, + AcceptedHostnames: map[string][]string{ + "listener-80-1": {"foo.example.com"}, + }, + }, + }, + }, + expectedGatewayListeners: map[string]*Listener{ + "listener-80-1": createModifiedListener("listener-80-1", func(l *Listener) { + l.Valid = false + l.Routes = map[types.NamespacedName]*Route{ + client.ObjectKeyFromObject(hr): getLastNormalRoute(), + } + }), + }, + expectedConditions: []conditions.Condition{staticConds.NewRouteInvalidListener()}, + name: "invalid attachable listener", + }, + { + route: invalidAttachableRoute1, + gateway: &Gateway{ + Source: gw, + Valid: true, + Listeners: map[string]*Listener{ + "listener-80-1": createListener("listener-80-1"), + }, + }, + expectedSectionNameRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + Attachment: &ParentRefAttachmentStatus{ + Attached: true, + AcceptedHostnames: map[string][]string{ + "listener-80-1": {"foo.example.com"}, + }, + }, + }, + }, + expectedGatewayListeners: map[string]*Listener{ + "listener-80-1": createModifiedListener("listener-80-1", func(l *Listener) { + l.Routes = map[types.NamespacedName]*Route{ + client.ObjectKeyFromObject(hr): invalidAttachableRoute1, + } + }), + }, + name: "invalid attachable route", + }, + { + route: invalidAttachableRoute2, + gateway: &Gateway{ + Source: gw, + Valid: true, + Listeners: map[string]*Listener{ + "listener-80-1": createModifiedListener("listener-80-1", func(l *Listener) { + l.Valid = false + }), + }, + }, + expectedSectionNameRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + Attachment: &ParentRefAttachmentStatus{ + Attached: true, + AcceptedHostnames: map[string][]string{ + "listener-80-1": {"foo.example.com"}, + }, + }, + }, + }, + expectedGatewayListeners: map[string]*Listener{ + "listener-80-1": createModifiedListener("listener-80-1", func(l *Listener) { + l.Valid = false + l.Routes = map[types.NamespacedName]*Route{ + client.ObjectKeyFromObject(hr): invalidAttachableRoute2, + } + }), + }, + expectedConditions: []conditions.Condition{staticConds.NewRouteInvalidListener()}, + name: "invalid attachable listener with invalid attachable route", + }, { route: createNormalRoute(gw), gateway: &Gateway{ @@ -1284,6 +1422,7 @@ func TestBindRouteToListeners(t *testing.T) { g.Expect(test.route.ParentRefs).To(Equal(test.expectedSectionNameRefs)) g.Expect(helpers.Diff(test.gateway.Listeners, test.expectedGatewayListeners)).To(BeEmpty()) + g.Expect(helpers.Diff(test.route.Conditions, test.expectedConditions)).To(BeEmpty()) }) } }