From 3d000d96893ec507c8faef369643cbaf9988405a Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Sun, 26 Jan 2025 14:55:27 -0700 Subject: [PATCH 1/4] listener isolation for hostnames --- .../mode/static/state/graph/route_common.go | 96 ++- .../static/state/graph/route_common_test.go | 557 ++++++++++++++++++ tests/Makefile | 2 +- 3 files changed, 649 insertions(+), 6 deletions(-) diff --git a/internal/mode/static/state/graph/route_common.go b/internal/mode/static/state/graph/route_common.go index 2db3e9e0b4..ef64b9b85f 100644 --- a/internal/mode/static/state/graph/route_common.go +++ b/internal/mode/static/state/graph/route_common.go @@ -337,22 +337,103 @@ func bindRoutesToListeners( bindL7RouteToListeners(r, gw, namespaces) } - var routes []*L4Route + l7routes := make([]*L7Route, 0, len(l7Routes)) + for _, r := range l7Routes { + l7routes = append(l7routes, r) + } + + isolateL7RouteListeners(l7routes, gw.Listeners) + + l4routes := make([]*L4Route, 0, len(l4Routes)) for _, r := range l4Routes { - routes = append(routes, r) + l4routes = append(l4routes, r) } // Sort the slice by timestamp and name so that we process the routes in the priority order - sort.Slice(routes, func(i, j int) bool { - return ngfSort.LessClientObject(routes[i].Source, routes[j].Source) + sort.Slice(l4routes, func(i, j int) bool { + return ngfSort.LessClientObject(l4routes[i].Source, l4routes[j].Source) }) // portHostnamesMap exists to detect duplicate hostnames on the same port portHostnamesMap := make(map[string]struct{}) - for _, r := range routes { + for _, r := range l4routes { bindL4RouteToListeners(r, gw, namespaces, portHostnamesMap) } + + isolateL4RouteListeners(l4routes, gw.Listeners) +} + +// isolateL7RouteListeners ensures listener isolation for all L7Routes. +func isolateL7RouteListeners(routes []*L7Route, listeners []*Listener) { + listenerHostnameMap := make(map[string]string, len(listeners)) + for _, l := range listeners { + listenerHostnameMap[l.Name] = getHostname(l.Source.Hostname) + } + + for _, route := range routes { + isolateHostnamesForParentRefs(route.ParentRefs, listenerHostnameMap) + } +} + +// isolateL4RouteListeners ensures listener isolation for all L4Routes. +func isolateL4RouteListeners(routes []*L4Route, listeners []*Listener) { + listenerHostnameMap := make(map[string]string, len(listeners)) + for _, l := range listeners { + listenerHostnameMap[l.Name] = getHostname(l.Source.Hostname) + } + + for _, route := range routes { + isolateHostnamesForParentRefs(route.ParentRefs, listenerHostnameMap) + } +} + +// isolateHostnamesForParentRefs iterates through the parentRefs of a route to identify the list of accepted hostnames +// for each listener. If any accepted hostname belongs to another listener, +// it removes those hostnames to ensure listener isolation. +func isolateHostnamesForParentRefs(parentRef []ParentRef, listenerHostnameMap map[string]string) { + for _, ref := range parentRef { + acceptedHostnames := ref.Attachment.AcceptedHostnames + + hostnamesToRemoves := make([]string, 0, len(acceptedHostnames)) + for listenerName, hostnames := range acceptedHostnames { + if len(hostnames) == 0 { + continue + } + for _, h := range hostnames { + for lName, lHostname := range listenerHostnameMap { + // skip comparison if it is a catch all listener block + if lHostname == "" { + continue + } + if h == lHostname && listenerName != lName { + hostnamesToRemoves = append(hostnamesToRemoves, h) + } + } + } + + isolatedHostnames := removeHostnames(hostnames, hostnamesToRemoves) + ref.Attachment.AcceptedHostnames[listenerName] = isolatedHostnames + } + } +} + +// removeHostnames removes the hostnames that are part of toRemove slice. +func removeHostnames(hostnames []string, toRemove []string) []string { + result := make([]string, 0, len(hostnames)) + for _, h := range hostnames { + keep := true + for _, r := range toRemove { + if h == r { + keep = false + break + } + } + if keep { + result = append(result, h) + } + } + return result } func validateParentRef( @@ -617,6 +698,11 @@ func tryToAttachL7RouteToListeners( rk := CreateRouteKey(route.Source) + listenerHostnameMap := make(map[string]string, len(attachableListeners)) + for _, l := range attachableListeners { + listenerHostnameMap[l.Name] = getHostname(l.Source.Hostname) + } + bind := func(l *Listener) (allowed, attached bool) { if !isRouteNamespaceAllowedByListener(l, route.Source.GetNamespace(), gw.Source.Namespace, namespaces) { return false, false diff --git a/internal/mode/static/state/graph/route_common_test.go b/internal/mode/static/state/graph/route_common_test.go index 42f601e5df..7faceddffd 100644 --- a/internal/mode/static/state/graph/route_common_test.go +++ b/internal/mode/static/state/graph/route_common_test.go @@ -2175,3 +2175,560 @@ func TestTryToAttachL4RouteToListeners_NoAttachableListeners(t *testing.T) { g.Expect(cond).To(Equal(staticConds.NewRouteInvalidListener())) g.Expect(attachable).To(BeFalse()) } + +func TestIsolateL4Listeners(t *testing.T) { + gw := &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "gateway", + }, + } + + createTLSRouteWithSectionNameAndPort := func( + name string, + sectionName *gatewayv1.SectionName, + ns string, + hostnames ...gatewayv1.Hostname, + ) *v1alpha2.TLSRoute { + return &v1alpha2.TLSRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns, + Name: name, + }, + Spec: v1alpha2.TLSRouteSpec{ + CommonRouteSpec: gatewayv1.CommonRouteSpec{ + ParentRefs: []gatewayv1.ParentReference{ + { + Name: gatewayv1.ObjectName(gw.Name), + SectionName: sectionName, + }, + }, + }, + Hostnames: hostnames, + }, + } + } + + tr1 := createTLSRouteWithSectionNameAndPort( + "tr1", + helpers.GetPointer[gatewayv1.SectionName]("empty-hostname"), + "test", + []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + ) + tr2 := createTLSRouteWithSectionNameAndPort( + "tr2", + helpers.GetPointer[gatewayv1.SectionName]("wildcard-example-com"), + "test", + []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + ) + tr3 := createTLSRouteWithSectionNameAndPort( + "tr3", + helpers.GetPointer[gatewayv1.SectionName]("foo-wildcard-example-com"), + "test", + []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + ) + tr4 := createTLSRouteWithSectionNameAndPort( + "tr4", + helpers.GetPointer[gatewayv1.SectionName]("abc-com"), + "test", + []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + ) + tr5 := createTLSRouteWithSectionNameAndPort( + "tr5", + helpers.GetPointer[gatewayv1.SectionName]("no-match"), + "test", + []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + ) + + createL4RoutewithAcceptedHostnames := func( + source *v1alpha2.TLSRoute, + acceptedHostnames map[string][]string, + hostnames []gatewayv1.Hostname, + sectionName *gatewayv1.SectionName, + ) *L4Route { + return &L4Route{ + Source: source, + Spec: L4RouteSpec{ + Hostnames: hostnames, + }, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKey{ + Namespace: gw.Namespace, + Name: gw.Name, + }, + SectionName: sectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: acceptedHostnames, + Attached: true, + }, + }, + }, + } + } + + createListener := func(name string, hostname string) *Listener { + return &Listener{ + Name: name, + Source: gatewayv1.Listener{ + Name: gatewayv1.SectionName(name), + Hostname: (*gatewayv1.Hostname)(helpers.GetPointer(hostname)), + }, + Valid: true, + Attachable: true, + } + } + + acceptedHostnames1 := map[string][]string{ + "empty-hostname": { + "bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com", + }, + } + acceptedHostnames2 := map[string][]string{ + "wildcard-example-com": { + "*.example.com", "*.foo.example.com", "abc.foo.example.com", + }, + } + + acceptedHostnames3 := map[string][]string{ + "foo-wildcard-example-com": { + "*.foo.example.com", "abc.foo.example.com", + }, + } + + acceptedHostnames4 := map[string][]string{ + "abc-com": { + "abc.foo.example.com", + }, + } + acceptedHostnames5 := map[string][]string{ + "no-match": {}, + } + + tests := []struct { + expectedResult map[string][]ParentRef + name string + routes []*L4Route + listeners []*Listener + }{ + { + name: "l4 routes with hostname intersection", + routes: []*L4Route{ + createL4RoutewithAcceptedHostnames( + tr1, acceptedHostnames1, + []gatewayv1.Hostname{"bar.com"}, + helpers.GetPointer[gatewayv1.SectionName]("empty-hostname"), + ), + createL4RoutewithAcceptedHostnames( + tr2, + acceptedHostnames2, + []gatewayv1.Hostname{"*.example.com"}, + helpers.GetPointer[gatewayv1.SectionName]("wildcard-example-com"), + ), + createL4RoutewithAcceptedHostnames( + tr3, + acceptedHostnames3, + []gatewayv1.Hostname{"*.foo.example.com"}, + helpers.GetPointer[gatewayv1.SectionName]("foo-wildcard-example-com"), + ), + createL4RoutewithAcceptedHostnames( + tr4, + acceptedHostnames4, + []gatewayv1.Hostname{"abc.foo.example.com"}, + helpers.GetPointer[gatewayv1.SectionName]("abc-com"), + ), + createL4RoutewithAcceptedHostnames( + tr5, + acceptedHostnames5, + []gatewayv1.Hostname{"cafe.example.com"}, + helpers.GetPointer[gatewayv1.SectionName]("no-match"), + ), + }, + listeners: []*Listener{ + createListener("empty-hostname", ""), + createListener("wildcard-example-com", "*.example.com"), + createListener("foo-wildcard-example-com", "*.foo.example.com"), + createListener("abc-com", "abc.foo.example.com"), + createListener("no-match", "no-match.cafe.com"), + }, + expectedResult: map[string][]ParentRef{ + "tr1": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: tr1.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "empty-hostname": {"bar.com"}, + }, + Attached: true, + }, + }, + }, + "tr2": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: tr2.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "wildcard-example-com": {"*.example.com"}, + }, + Attached: true, + }, + }, + }, + "tr3": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: tr3.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "foo-wildcard-example-com": {"*.foo.example.com"}, + }, + Attached: true, + }, + }, + }, + "tr4": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: tr4.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "abc-com": {"abc.foo.example.com"}, + }, + Attached: true, + }, + }, + }, + "tr5": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: tr5.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "no-match": {}, + }, + Attached: true, + }, + }, + }, + }, + }, + } + + for _, tt := range tests { + g := NewWithT(t) + isolateL4RouteListeners(tt.routes, tt.listeners) + + for _, route := range tt.routes { + for routeName, refs := range tt.expectedResult { + if route.Source.GetName() == routeName { + g.Expect(helpers.Diff(route.ParentRefs, refs)).To(BeEmpty()) + } + } + } + } +} + +func TestIsolateL7Listeners(t *testing.T) { + gw := &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "gateway", + }, + } + + createHTTPRouteWithSectionNameAndPort := func( + name string, + sectionName *gatewayv1.SectionName, + ns string, + hostnames ...gatewayv1.Hostname, + ) *gatewayv1.HTTPRoute { + return &gatewayv1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns, + Name: name, + }, + Spec: gatewayv1.HTTPRouteSpec{ + CommonRouteSpec: gatewayv1.CommonRouteSpec{ + ParentRefs: []gatewayv1.ParentReference{ + { + Name: gatewayv1.ObjectName(gw.Name), + SectionName: sectionName, + }, + }, + }, + Hostnames: hostnames, + }, + } + } + + hr1 := createHTTPRouteWithSectionNameAndPort( + "hr1", + helpers.GetPointer[gatewayv1.SectionName]("empty-hostname"), + "test", + []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + ) + hr2 := createHTTPRouteWithSectionNameAndPort( + "hr2", + helpers.GetPointer[gatewayv1.SectionName]("wildcard-example-com"), + "test", + []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + ) + hr3 := createHTTPRouteWithSectionNameAndPort( + "hr3", + helpers.GetPointer[gatewayv1.SectionName]("foo-wildcard-example-com"), + "test", + []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + ) + hr4 := createHTTPRouteWithSectionNameAndPort( + "hr4", + helpers.GetPointer[gatewayv1.SectionName]("abc-com"), + "test", + []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + ) + hr5 := createHTTPRouteWithSectionNameAndPort( + "hr5", + helpers.GetPointer[gatewayv1.SectionName]("no-match"), + "test", + []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + ) + + createL7RoutewithAcceptedHostnames := func( + source *gatewayv1.HTTPRoute, + acceptedHostnames map[string][]string, + hostnames []gatewayv1.Hostname, + sectionName *gatewayv1.SectionName, + ) *L7Route { + return &L7Route{ + Source: source, + Spec: L7RouteSpec{ + Hostnames: hostnames, + }, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKey{ + Namespace: gw.Namespace, + Name: gw.Name, + }, + SectionName: sectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: acceptedHostnames, + Attached: true, + }, + }, + }, + } + } + + createListener := func(name string, hostname string) *Listener { + return &Listener{ + Name: name, + Source: gatewayv1.Listener{ + Name: gatewayv1.SectionName(name), + Hostname: (*gatewayv1.Hostname)(helpers.GetPointer(hostname)), + }, + Valid: true, + Attachable: true, + } + } + + acceptedHostnames1 := map[string][]string{ + "empty-hostname": { + "bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com", + }, + } + acceptedHostnames2 := map[string][]string{ + "wildcard-example-com": { + "*.example.com", "*.foo.example.com", "abc.foo.example.com", + }, + } + + acceptedHostnames3 := map[string][]string{ + "foo-wildcard-example-com": { + "*.foo.example.com", "abc.foo.example.com", + }, + } + + acceptedHostnames4 := map[string][]string{ + "abc-com": { + "abc.foo.example.com", + }, + } + acceptedHostnames5 := map[string][]string{ + "no-match": {}, + } + + tests := []struct { + expectedResult map[string][]ParentRef + name string + routes []*L7Route + listeners []*Listener + }{ + { + name: "l7 routes with hostname intersection", + routes: []*L7Route{ + createL7RoutewithAcceptedHostnames( + hr1, + acceptedHostnames1, + []gatewayv1.Hostname{"bar.com"}, + helpers.GetPointer[gatewayv1.SectionName]("empty-hostname"), + ), + createL7RoutewithAcceptedHostnames( + hr2, + acceptedHostnames2, + []gatewayv1.Hostname{"*.example.com"}, + helpers.GetPointer[gatewayv1.SectionName]("wildcard-example-com"), + ), + createL7RoutewithAcceptedHostnames( + hr3, + acceptedHostnames3, + []gatewayv1.Hostname{"*.foo.example.com"}, + helpers.GetPointer[gatewayv1.SectionName]("foo-wildcard-example-com"), + ), + createL7RoutewithAcceptedHostnames( + hr4, + acceptedHostnames4, + []gatewayv1.Hostname{"abc.foo.example.com"}, + helpers.GetPointer[gatewayv1.SectionName]("abc-com"), + ), + createL7RoutewithAcceptedHostnames( + hr5, + acceptedHostnames5, + []gatewayv1.Hostname{"cafe.example.com"}, + helpers.GetPointer[gatewayv1.SectionName]("no-match"), + ), + }, + listeners: []*Listener{ + createListener("empty-hostname", ""), + createListener("wildcard-example-com", "*.example.com"), + createListener("foo-wildcard-example-com", "*.foo.example.com"), + createListener("abc-com", "abc.foo.example.com"), + createListener("no-match", "no-match.cafe.com"), + }, + expectedResult: map[string][]ParentRef{ + "hr1": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: hr1.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "empty-hostname": {"bar.com"}, + }, + Attached: true, + }, + }, + }, + "hr2": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: hr2.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "wildcard-example-com": {"*.example.com"}, + }, + Attached: true, + }, + }, + }, + "hr3": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: hr3.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "foo-wildcard-example-com": {"*.foo.example.com"}, + }, + Attached: true, + }, + }, + }, + "hr4": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: hr4.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "abc-com": {"abc.foo.example.com"}, + }, + Attached: true, + }, + }, + }, + "hr5": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: hr5.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "no-match": {}, + }, + Attached: true, + }, + }, + }, + }, + }, + } + + for _, tt := range tests { + g := NewWithT(t) + isolateL7RouteListeners(tt.routes, tt.listeners) + + for _, route := range tt.routes { + for routeName, refs := range tt.expectedResult { + if route.Source.GetName() == routeName { + g.Expect(helpers.Diff(route.ParentRefs, refs)).To(BeEmpty()) + } + } + } + } +} + +func TestRemoveHostnames(t *testing.T) { + tests := []struct { + name string + hostnames []string + removeHostnames []string + expectedHostnames []string + }{ + { + name: "remove one hostname", + hostnames: []string{"foo.example.com", "bar.example.com", "bar.com", "*.wildcard.com"}, + removeHostnames: []string{"foo.example.com", "bar.example.com"}, + expectedHostnames: []string{"bar.com", "*.wildcard.com"}, + }, + { + name: "remove all hostnames", + hostnames: []string{"foo.example.com", "bar.example.com", "bar.com", "*.wildcard.com"}, + removeHostnames: []string{"foo.example.com", "bar.example.com", "bar.com", "*.wildcard.com"}, + expectedHostnames: []string{}, + }, + { + name: "remove no hostnames", + hostnames: []string{"foo.example.com", "bar.example.com", "bar.com", "*.wildcard.com"}, + removeHostnames: []string{}, + expectedHostnames: []string{"foo.example.com", "bar.example.com", "bar.com", "*.wildcard.com"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + result := removeHostnames(tt.hostnames, tt.removeHostnames) + g.Expect(result).To(Equal(tt.expectedHostnames)) + }) + } +} diff --git a/tests/Makefile b/tests/Makefile index 2fa1922162..c95f337d19 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -14,7 +14,7 @@ NGF_VERSION ?= edge## NGF version to be tested PULL_POLICY = Never## Pull policy for the images NGINX_CONF_DIR = internal/mode/static/nginx/conf PROVISIONER_MANIFEST = conformance/provisioner/provisioner.yaml -SUPPORTED_EXTENDED_FEATURES = HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect,HTTPRouteHostRewrite,HTTPRoutePathRewrite,GatewayPort8080,HTTPRouteResponseHeaderModification,HTTPRoutePathRedirect +SUPPORTED_EXTENDED_FEATURES = HTTPRouteQueryParamMatching,HTTPRouteMethodMatching,HTTPRoutePortRedirect,HTTPRouteSchemeRedirect,HTTPRouteHostRewrite,HTTPRoutePathRewrite,GatewayPort8080,HTTPRouteResponseHeaderModification,HTTPRoutePathRedirect,GatewayHTTPListenerIsolation STANDARD_CONFORMANCE_PROFILES = GATEWAY-HTTP,GATEWAY-GRPC EXPERIMENTAL_CONFORMANCE_PROFILES = GATEWAY-TLS CONFORMANCE_PROFILES = $(STANDARD_CONFORMANCE_PROFILES) # by default we use the standard conformance profiles. If experimental is enabled we override this and add the experimental profiles. From fb48ff0c8eff7c21608cb6d088847592da8bcedc Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Mon, 27 Jan 2025 12:50:54 -0700 Subject: [PATCH 2/4] update based on reviews --- .../mode/static/state/graph/route_common.go | 11 +- .../static/state/graph/route_common_test.go | 475 +++++++++--------- 2 files changed, 230 insertions(+), 256 deletions(-) diff --git a/internal/mode/static/state/graph/route_common.go b/internal/mode/static/state/graph/route_common.go index ef64b9b85f..7cc445eb57 100644 --- a/internal/mode/static/state/graph/route_common.go +++ b/internal/mode/static/state/graph/route_common.go @@ -337,12 +337,12 @@ func bindRoutesToListeners( bindL7RouteToListeners(r, gw, namespaces) } - l7routes := make([]*L7Route, 0, len(l7Routes)) + routes := make([]*L7Route, 0, len(l7Routes)) for _, r := range l7Routes { - l7routes = append(l7routes, r) + routes = append(routes, r) } - isolateL7RouteListeners(l7routes, gw.Listeners) + isolateL7RouteListeners(routes, gw.Listeners) l4routes := make([]*L4Route, 0, len(l4Routes)) for _, r := range l4Routes { @@ -698,11 +698,6 @@ func tryToAttachL7RouteToListeners( rk := CreateRouteKey(route.Source) - listenerHostnameMap := make(map[string]string, len(attachableListeners)) - for _, l := range attachableListeners { - listenerHostnameMap[l.Name] = getHostname(l.Source.Hostname) - } - bind := func(l *Listener) (allowed, attached bool) { if !isRouteNamespaceAllowedByListener(l, route.Source.GetNamespace(), gw.Source.Namespace, namespaces) { return false, false diff --git a/internal/mode/static/state/graph/route_common_test.go b/internal/mode/static/state/graph/route_common_test.go index 7faceddffd..08abef4cb9 100644 --- a/internal/mode/static/state/graph/route_common_test.go +++ b/internal/mode/static/state/graph/route_common_test.go @@ -2209,35 +2209,36 @@ func TestIsolateL4Listeners(t *testing.T) { } } + routeHostnames := []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"} tr1 := createTLSRouteWithSectionNameAndPort( "tr1", helpers.GetPointer[gatewayv1.SectionName]("empty-hostname"), "test", - []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + routeHostnames..., ) tr2 := createTLSRouteWithSectionNameAndPort( "tr2", helpers.GetPointer[gatewayv1.SectionName]("wildcard-example-com"), "test", - []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + routeHostnames..., ) tr3 := createTLSRouteWithSectionNameAndPort( "tr3", helpers.GetPointer[gatewayv1.SectionName]("foo-wildcard-example-com"), "test", - []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + routeHostnames..., ) tr4 := createTLSRouteWithSectionNameAndPort( "tr4", helpers.GetPointer[gatewayv1.SectionName]("abc-com"), "test", - []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + routeHostnames..., ) tr5 := createTLSRouteWithSectionNameAndPort( "tr5", helpers.GetPointer[gatewayv1.SectionName]("no-match"), "test", - []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + routeHostnames..., ) createL4RoutewithAcceptedHostnames := func( @@ -2280,160 +2281,148 @@ func TestIsolateL4Listeners(t *testing.T) { } } - acceptedHostnames1 := map[string][]string{ + acceptedHostnamesEmptyHostname := map[string][]string{ "empty-hostname": { "bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com", }, } - acceptedHostnames2 := map[string][]string{ + acceptedHostnamesWildcardExample := map[string][]string{ "wildcard-example-com": { "*.example.com", "*.foo.example.com", "abc.foo.example.com", }, } - acceptedHostnames3 := map[string][]string{ + acceptedHostnamesFooWildcardExample := map[string][]string{ "foo-wildcard-example-com": { "*.foo.example.com", "abc.foo.example.com", }, } - acceptedHostnames4 := map[string][]string{ + acceptedHostnamesAbcCom := map[string][]string{ "abc-com": { "abc.foo.example.com", }, } - acceptedHostnames5 := map[string][]string{ + acceptedHostnamesNoMatch := map[string][]string{ "no-match": {}, } - tests := []struct { - expectedResult map[string][]ParentRef - name string - routes []*L4Route - listeners []*Listener - }{ - { - name: "l4 routes with hostname intersection", - routes: []*L4Route{ - createL4RoutewithAcceptedHostnames( - tr1, acceptedHostnames1, - []gatewayv1.Hostname{"bar.com"}, - helpers.GetPointer[gatewayv1.SectionName]("empty-hostname"), - ), - createL4RoutewithAcceptedHostnames( - tr2, - acceptedHostnames2, - []gatewayv1.Hostname{"*.example.com"}, - helpers.GetPointer[gatewayv1.SectionName]("wildcard-example-com"), - ), - createL4RoutewithAcceptedHostnames( - tr3, - acceptedHostnames3, - []gatewayv1.Hostname{"*.foo.example.com"}, - helpers.GetPointer[gatewayv1.SectionName]("foo-wildcard-example-com"), - ), - createL4RoutewithAcceptedHostnames( - tr4, - acceptedHostnames4, - []gatewayv1.Hostname{"abc.foo.example.com"}, - helpers.GetPointer[gatewayv1.SectionName]("abc-com"), - ), - createL4RoutewithAcceptedHostnames( - tr5, - acceptedHostnames5, - []gatewayv1.Hostname{"cafe.example.com"}, - helpers.GetPointer[gatewayv1.SectionName]("no-match"), - ), - }, - listeners: []*Listener{ - createListener("empty-hostname", ""), - createListener("wildcard-example-com", "*.example.com"), - createListener("foo-wildcard-example-com", "*.foo.example.com"), - createListener("abc-com", "abc.foo.example.com"), - createListener("no-match", "no-match.cafe.com"), - }, - expectedResult: map[string][]ParentRef{ - "tr1": { - { - Idx: 0, - Gateway: client.ObjectKeyFromObject(gw), - SectionName: tr1.Spec.ParentRefs[0].SectionName, - Attachment: &ParentRefAttachmentStatus{ - AcceptedHostnames: map[string][]string{ - "empty-hostname": {"bar.com"}, - }, - Attached: true, - }, + routes := []*L4Route{ + createL4RoutewithAcceptedHostnames( + tr1, acceptedHostnamesEmptyHostname, + []gatewayv1.Hostname{"bar.com"}, + helpers.GetPointer[gatewayv1.SectionName]("empty-hostname"), + ), + createL4RoutewithAcceptedHostnames( + tr2, + acceptedHostnamesWildcardExample, + []gatewayv1.Hostname{"*.example.com"}, + helpers.GetPointer[gatewayv1.SectionName]("wildcard-example-com"), + ), + createL4RoutewithAcceptedHostnames( + tr3, + acceptedHostnamesFooWildcardExample, + []gatewayv1.Hostname{"*.foo.example.com"}, + helpers.GetPointer[gatewayv1.SectionName]("foo-wildcard-example-com"), + ), + createL4RoutewithAcceptedHostnames( + tr4, + acceptedHostnamesAbcCom, + []gatewayv1.Hostname{"abc.foo.example.com"}, + helpers.GetPointer[gatewayv1.SectionName]("abc-com"), + ), + createL4RoutewithAcceptedHostnames( + tr5, + acceptedHostnamesNoMatch, + []gatewayv1.Hostname{"cafe.example.com"}, + helpers.GetPointer[gatewayv1.SectionName]("no-match"), + ), + } + + listeners := []*Listener{ + createListener("empty-hostname", ""), + createListener("wildcard-example-com", "*.example.com"), + createListener("foo-wildcard-example-com", "*.foo.example.com"), + createListener("abc-com", "abc.foo.example.com"), + createListener("no-match", "no-match.cafe.com"), + } + + expectedResult := map[string][]ParentRef{ + "tr1": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: tr1.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "empty-hostname": {"bar.com"}, }, + Attached: true, }, - "tr2": { - { - Idx: 0, - Gateway: client.ObjectKeyFromObject(gw), - SectionName: tr2.Spec.ParentRefs[0].SectionName, - Attachment: &ParentRefAttachmentStatus{ - AcceptedHostnames: map[string][]string{ - "wildcard-example-com": {"*.example.com"}, - }, - Attached: true, - }, + }, + }, + "tr2": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: tr2.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "wildcard-example-com": {"*.example.com"}, }, + Attached: true, }, - "tr3": { - { - Idx: 0, - Gateway: client.ObjectKeyFromObject(gw), - SectionName: tr3.Spec.ParentRefs[0].SectionName, - Attachment: &ParentRefAttachmentStatus{ - AcceptedHostnames: map[string][]string{ - "foo-wildcard-example-com": {"*.foo.example.com"}, - }, - Attached: true, - }, + }, + }, + "tr3": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: tr3.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "foo-wildcard-example-com": {"*.foo.example.com"}, }, + Attached: true, }, - "tr4": { - { - Idx: 0, - Gateway: client.ObjectKeyFromObject(gw), - SectionName: tr4.Spec.ParentRefs[0].SectionName, - Attachment: &ParentRefAttachmentStatus{ - AcceptedHostnames: map[string][]string{ - "abc-com": {"abc.foo.example.com"}, - }, - Attached: true, - }, + }, + }, + "tr4": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: tr4.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "abc-com": {"abc.foo.example.com"}, }, + Attached: true, }, - "tr5": { - { - Idx: 0, - Gateway: client.ObjectKeyFromObject(gw), - SectionName: tr5.Spec.ParentRefs[0].SectionName, - Attachment: &ParentRefAttachmentStatus{ - AcceptedHostnames: map[string][]string{ - "no-match": {}, - }, - Attached: true, - }, + }, + }, + "tr5": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: tr5.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "no-match": {}, }, + Attached: true, }, }, }, } - for _, tt := range tests { - g := NewWithT(t) - isolateL4RouteListeners(tt.routes, tt.listeners) - - for _, route := range tt.routes { - for routeName, refs := range tt.expectedResult { - if route.Source.GetName() == routeName { - g.Expect(helpers.Diff(route.ParentRefs, refs)).To(BeEmpty()) - } - } - } + g := NewWithT(t) + isolateL4RouteListeners(routes, listeners) + + result := map[string][]ParentRef{} + for _, route := range routes { + result[route.Source.GetName()] = route.ParentRefs } + g.Expect(helpers.Diff(result, expectedResult)).To(BeEmpty()) } func TestIsolateL7Listeners(t *testing.T) { @@ -2469,35 +2458,36 @@ func TestIsolateL7Listeners(t *testing.T) { } } + routeHostnames := []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"} hr1 := createHTTPRouteWithSectionNameAndPort( "hr1", helpers.GetPointer[gatewayv1.SectionName]("empty-hostname"), "test", - []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + routeHostnames..., ) hr2 := createHTTPRouteWithSectionNameAndPort( "hr2", helpers.GetPointer[gatewayv1.SectionName]("wildcard-example-com"), "test", - []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + routeHostnames..., ) hr3 := createHTTPRouteWithSectionNameAndPort( "hr3", helpers.GetPointer[gatewayv1.SectionName]("foo-wildcard-example-com"), "test", - []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + routeHostnames..., ) hr4 := createHTTPRouteWithSectionNameAndPort( "hr4", helpers.GetPointer[gatewayv1.SectionName]("abc-com"), "test", - []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + routeHostnames..., ) hr5 := createHTTPRouteWithSectionNameAndPort( "hr5", helpers.GetPointer[gatewayv1.SectionName]("no-match"), "test", - []gatewayv1.Hostname{"bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com"}..., + routeHostnames..., // no matching hostname ) createL7RoutewithAcceptedHostnames := func( @@ -2540,161 +2530,150 @@ func TestIsolateL7Listeners(t *testing.T) { } } - acceptedHostnames1 := map[string][]string{ + acceptedHostnamesEmptyHostname := map[string][]string{ "empty-hostname": { "bar.com", "*.example.com", "*.foo.example.com", "abc.foo.example.com", }, } - acceptedHostnames2 := map[string][]string{ + acceptedHostnamesWildcardExample := map[string][]string{ "wildcard-example-com": { "*.example.com", "*.foo.example.com", "abc.foo.example.com", }, } - acceptedHostnames3 := map[string][]string{ + acceptedHostnamesFooWildcardExample := map[string][]string{ "foo-wildcard-example-com": { "*.foo.example.com", "abc.foo.example.com", }, } - acceptedHostnames4 := map[string][]string{ + acceptedHostnamesAbcCom := map[string][]string{ "abc-com": { "abc.foo.example.com", }, } - acceptedHostnames5 := map[string][]string{ + acceptedHostnamesNoMatch := map[string][]string{ "no-match": {}, } - tests := []struct { - expectedResult map[string][]ParentRef - name string - routes []*L7Route - listeners []*Listener - }{ - { - name: "l7 routes with hostname intersection", - routes: []*L7Route{ - createL7RoutewithAcceptedHostnames( - hr1, - acceptedHostnames1, - []gatewayv1.Hostname{"bar.com"}, - helpers.GetPointer[gatewayv1.SectionName]("empty-hostname"), - ), - createL7RoutewithAcceptedHostnames( - hr2, - acceptedHostnames2, - []gatewayv1.Hostname{"*.example.com"}, - helpers.GetPointer[gatewayv1.SectionName]("wildcard-example-com"), - ), - createL7RoutewithAcceptedHostnames( - hr3, - acceptedHostnames3, - []gatewayv1.Hostname{"*.foo.example.com"}, - helpers.GetPointer[gatewayv1.SectionName]("foo-wildcard-example-com"), - ), - createL7RoutewithAcceptedHostnames( - hr4, - acceptedHostnames4, - []gatewayv1.Hostname{"abc.foo.example.com"}, - helpers.GetPointer[gatewayv1.SectionName]("abc-com"), - ), - createL7RoutewithAcceptedHostnames( - hr5, - acceptedHostnames5, - []gatewayv1.Hostname{"cafe.example.com"}, - helpers.GetPointer[gatewayv1.SectionName]("no-match"), - ), - }, - listeners: []*Listener{ - createListener("empty-hostname", ""), - createListener("wildcard-example-com", "*.example.com"), - createListener("foo-wildcard-example-com", "*.foo.example.com"), - createListener("abc-com", "abc.foo.example.com"), - createListener("no-match", "no-match.cafe.com"), - }, - expectedResult: map[string][]ParentRef{ - "hr1": { - { - Idx: 0, - Gateway: client.ObjectKeyFromObject(gw), - SectionName: hr1.Spec.ParentRefs[0].SectionName, - Attachment: &ParentRefAttachmentStatus{ - AcceptedHostnames: map[string][]string{ - "empty-hostname": {"bar.com"}, - }, - Attached: true, - }, + routes := []*L7Route{ + createL7RoutewithAcceptedHostnames( + hr1, + acceptedHostnamesEmptyHostname, + []gatewayv1.Hostname{"bar.com"}, + helpers.GetPointer[gatewayv1.SectionName]("empty-hostname"), + ), + createL7RoutewithAcceptedHostnames( + hr2, + acceptedHostnamesWildcardExample, + []gatewayv1.Hostname{"*.example.com"}, + helpers.GetPointer[gatewayv1.SectionName]("wildcard-example-com"), + ), + createL7RoutewithAcceptedHostnames( + hr3, + acceptedHostnamesFooWildcardExample, + []gatewayv1.Hostname{"*.foo.example.com"}, + helpers.GetPointer[gatewayv1.SectionName]("foo-wildcard-example-com"), + ), + createL7RoutewithAcceptedHostnames( + hr4, + acceptedHostnamesAbcCom, + []gatewayv1.Hostname{"abc.foo.example.com"}, + helpers.GetPointer[gatewayv1.SectionName]("abc-com"), + ), + createL7RoutewithAcceptedHostnames( + hr5, + acceptedHostnamesNoMatch, + []gatewayv1.Hostname{"cafe.example.com"}, + helpers.GetPointer[gatewayv1.SectionName]("no-match"), + ), + } + + listeners := []*Listener{ + createListener("empty-hostname", ""), + createListener("wildcard-example-com", "*.example.com"), + createListener("foo-wildcard-example-com", "*.foo.example.com"), + createListener("abc-com", "abc.foo.example.com"), + createListener("no-match", "no-match.cafe.com"), + } + + expectedResult := map[string][]ParentRef{ + "hr1": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: hr1.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "empty-hostname": {"bar.com"}, }, + Attached: true, }, - "hr2": { - { - Idx: 0, - Gateway: client.ObjectKeyFromObject(gw), - SectionName: hr2.Spec.ParentRefs[0].SectionName, - Attachment: &ParentRefAttachmentStatus{ - AcceptedHostnames: map[string][]string{ - "wildcard-example-com": {"*.example.com"}, - }, - Attached: true, - }, + }, + }, + "hr2": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: hr2.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "wildcard-example-com": {"*.example.com"}, }, + Attached: true, }, - "hr3": { - { - Idx: 0, - Gateway: client.ObjectKeyFromObject(gw), - SectionName: hr3.Spec.ParentRefs[0].SectionName, - Attachment: &ParentRefAttachmentStatus{ - AcceptedHostnames: map[string][]string{ - "foo-wildcard-example-com": {"*.foo.example.com"}, - }, - Attached: true, - }, + }, + }, + "hr3": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: hr3.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "foo-wildcard-example-com": {"*.foo.example.com"}, }, + Attached: true, }, - "hr4": { - { - Idx: 0, - Gateway: client.ObjectKeyFromObject(gw), - SectionName: hr4.Spec.ParentRefs[0].SectionName, - Attachment: &ParentRefAttachmentStatus{ - AcceptedHostnames: map[string][]string{ - "abc-com": {"abc.foo.example.com"}, - }, - Attached: true, - }, + }, + }, + "hr4": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: hr4.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "abc-com": {"abc.foo.example.com"}, }, + Attached: true, }, - "hr5": { - { - Idx: 0, - Gateway: client.ObjectKeyFromObject(gw), - SectionName: hr5.Spec.ParentRefs[0].SectionName, - Attachment: &ParentRefAttachmentStatus{ - AcceptedHostnames: map[string][]string{ - "no-match": {}, - }, - Attached: true, - }, + }, + }, + "hr5": { + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + SectionName: hr5.Spec.ParentRefs[0].SectionName, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{ + "no-match": {}, }, + Attached: true, }, }, }, } - for _, tt := range tests { - g := NewWithT(t) - isolateL7RouteListeners(tt.routes, tt.listeners) - - for _, route := range tt.routes { - for routeName, refs := range tt.expectedResult { - if route.Source.GetName() == routeName { - g.Expect(helpers.Diff(route.ParentRefs, refs)).To(BeEmpty()) - } - } - } + g := NewWithT(t) + isolateL7RouteListeners(routes, listeners) + + result := map[string][]ParentRef{} + for _, route := range routes { + result[route.Source.GetName()] = route.ParentRefs } + + g.Expect(helpers.Diff(result, expectedResult)).To(BeEmpty()) } func TestRemoveHostnames(t *testing.T) { @@ -2705,7 +2684,7 @@ func TestRemoveHostnames(t *testing.T) { expectedHostnames []string }{ { - name: "remove one hostname", + name: "remove multiple hostnames", hostnames: []string{"foo.example.com", "bar.example.com", "bar.com", "*.wildcard.com"}, removeHostnames: []string{"foo.example.com", "bar.example.com"}, expectedHostnames: []string{"bar.com", "*.wildcard.com"}, From d867c86a4d9da01015e8b0cbbd228b9fd8d97ce7 Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Mon, 27 Jan 2025 15:23:23 -0700 Subject: [PATCH 3/4] fix remove hostnames --- .../mode/static/state/graph/route_common.go | 31 +++++++------------ .../static/state/graph/route_common_test.go | 24 +++++++++----- 2 files changed, 28 insertions(+), 27 deletions(-) diff --git a/internal/mode/static/state/graph/route_common.go b/internal/mode/static/state/graph/route_common.go index 7cc445eb57..e80a18f872 100644 --- a/internal/mode/static/state/graph/route_common.go +++ b/internal/mode/static/state/graph/route_common.go @@ -344,24 +344,24 @@ func bindRoutesToListeners( isolateL7RouteListeners(routes, gw.Listeners) - l4routes := make([]*L4Route, 0, len(l4Routes)) + l4RouteSlice := make([]*L4Route, 0, len(l4Routes)) for _, r := range l4Routes { - l4routes = append(l4routes, r) + l4RouteSlice = append(l4RouteSlice, r) } // Sort the slice by timestamp and name so that we process the routes in the priority order - sort.Slice(l4routes, func(i, j int) bool { - return ngfSort.LessClientObject(l4routes[i].Source, l4routes[j].Source) + sort.Slice(l4RouteSlice, func(i, j int) bool { + return ngfSort.LessClientObject(l4RouteSlice[i].Source, l4RouteSlice[j].Source) }) // portHostnamesMap exists to detect duplicate hostnames on the same port portHostnamesMap := make(map[string]struct{}) - for _, r := range l4routes { + for _, r := range l4RouteSlice { bindL4RouteToListeners(r, gw, namespaces, portHostnamesMap) } - isolateL4RouteListeners(l4routes, gw.Listeners) + isolateL4RouteListeners(l4RouteSlice, gw.Listeners) } // isolateL7RouteListeners ensures listener isolation for all L7Routes. @@ -395,7 +395,7 @@ func isolateHostnamesForParentRefs(parentRef []ParentRef, listenerHostnameMap ma for _, ref := range parentRef { acceptedHostnames := ref.Attachment.AcceptedHostnames - hostnamesToRemoves := make([]string, 0, len(acceptedHostnames)) + hostnamesToRemoves := make(map[string]struct{}, len(acceptedHostnames)) for listenerName, hostnames := range acceptedHostnames { if len(hostnames) == 0 { continue @@ -407,7 +407,7 @@ func isolateHostnamesForParentRefs(parentRef []ParentRef, listenerHostnameMap ma continue } if h == lHostname && listenerName != lName { - hostnamesToRemoves = append(hostnamesToRemoves, h) + hostnamesToRemoves[h] = struct{}{} } } } @@ -419,18 +419,11 @@ func isolateHostnamesForParentRefs(parentRef []ParentRef, listenerHostnameMap ma } // removeHostnames removes the hostnames that are part of toRemove slice. -func removeHostnames(hostnames []string, toRemove []string) []string { +func removeHostnames(hostnames []string, toRemove map[string]struct{}) []string { result := make([]string, 0, len(hostnames)) - for _, h := range hostnames { - keep := true - for _, r := range toRemove { - if h == r { - keep = false - break - } - } - if keep { - result = append(result, h) + for _, hostname := range hostnames { + if _, exists := toRemove[hostname]; !exists { + result = append(result, hostname) } } return result diff --git a/internal/mode/static/state/graph/route_common_test.go b/internal/mode/static/state/graph/route_common_test.go index 08abef4cb9..87c0844c05 100644 --- a/internal/mode/static/state/graph/route_common_test.go +++ b/internal/mode/static/state/graph/route_common_test.go @@ -2680,25 +2680,33 @@ func TestRemoveHostnames(t *testing.T) { tests := []struct { name string hostnames []string - removeHostnames []string + removeHostnames map[string]struct{} expectedHostnames []string }{ { - name: "remove multiple hostnames", - hostnames: []string{"foo.example.com", "bar.example.com", "bar.com", "*.wildcard.com"}, - removeHostnames: []string{"foo.example.com", "bar.example.com"}, + name: "remove multiple hostnames", + hostnames: []string{"foo.example.com", "bar.example.com", "bar.com", "*.wildcard.com"}, + removeHostnames: map[string]struct{}{ + "foo.example.com": {}, + "bar.example.com": {}, + }, expectedHostnames: []string{"bar.com", "*.wildcard.com"}, }, { - name: "remove all hostnames", - hostnames: []string{"foo.example.com", "bar.example.com", "bar.com", "*.wildcard.com"}, - removeHostnames: []string{"foo.example.com", "bar.example.com", "bar.com", "*.wildcard.com"}, + name: "remove all hostnames", + hostnames: []string{"foo.example.com", "bar.example.com", "bar.com", "*.wildcard.com"}, + removeHostnames: map[string]struct{}{ + "foo.example.com": {}, + "bar.example.com": {}, + "bar.com": {}, + "*.wildcard.com": {}, + }, expectedHostnames: []string{}, }, { name: "remove no hostnames", hostnames: []string{"foo.example.com", "bar.example.com", "bar.com", "*.wildcard.com"}, - removeHostnames: []string{}, + removeHostnames: map[string]struct{}{}, expectedHostnames: []string{"foo.example.com", "bar.example.com", "bar.com", "*.wildcard.com"}, }, } From 1bc173194373fb685321dada79275a57bd23fa51 Mon Sep 17 00:00:00 2001 From: salonichf5 <146118978+salonichf5@users.noreply.github.com> Date: Wed, 29 Jan 2025 09:51:11 -0700 Subject: [PATCH 4/4] update based on reviews --- internal/mode/static/state/graph/route_common.go | 2 +- internal/mode/static/state/graph/route_common_test.go | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/mode/static/state/graph/route_common.go b/internal/mode/static/state/graph/route_common.go index e80a18f872..31a7c1528f 100644 --- a/internal/mode/static/state/graph/route_common.go +++ b/internal/mode/static/state/graph/route_common.go @@ -395,7 +395,7 @@ func isolateHostnamesForParentRefs(parentRef []ParentRef, listenerHostnameMap ma for _, ref := range parentRef { acceptedHostnames := ref.Attachment.AcceptedHostnames - hostnamesToRemoves := make(map[string]struct{}, len(acceptedHostnames)) + hostnamesToRemoves := make(map[string]struct{}) for listenerName, hostnames := range acceptedHostnames { if len(hostnames) == 0 { continue diff --git a/internal/mode/static/state/graph/route_common_test.go b/internal/mode/static/state/graph/route_common_test.go index 87c0844c05..32df93de4a 100644 --- a/internal/mode/static/state/graph/route_common_test.go +++ b/internal/mode/static/state/graph/route_common_test.go @@ -2177,6 +2177,7 @@ func TestTryToAttachL4RouteToListeners_NoAttachableListeners(t *testing.T) { } func TestIsolateL4Listeners(t *testing.T) { + t.Parallel() gw := &gatewayv1.Gateway{ ObjectMeta: metav1.ObjectMeta{ Namespace: "test", @@ -2426,6 +2427,7 @@ func TestIsolateL4Listeners(t *testing.T) { } func TestIsolateL7Listeners(t *testing.T) { + t.Parallel() gw := &gatewayv1.Gateway{ ObjectMeta: metav1.ObjectMeta{ Namespace: "test", @@ -2677,6 +2679,7 @@ func TestIsolateL7Listeners(t *testing.T) { } func TestRemoveHostnames(t *testing.T) { + t.Parallel() tests := []struct { name string hostnames []string @@ -2713,6 +2716,7 @@ func TestRemoveHostnames(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() g := NewWithT(t) result := removeHostnames(tt.hostnames, tt.removeHostnames) g.Expect(result).To(Equal(tt.expectedHostnames))