From 5edbbcd0adb2a9ae354b0ce25533ff73a4f39a8a Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 29 May 2025 15:12:08 -0700 Subject: [PATCH 1/8] Add fix to parent ref creation logic --- internal/controller/state/graph/policies.go | 6 ++ .../controller/state/graph/policies_test.go | 22 +++++++- .../controller/state/graph/route_common.go | 41 +++++++++++--- .../state/graph/route_common_test.go | 55 +++++++++++++------ 4 files changed, 97 insertions(+), 27 deletions(-) diff --git a/internal/controller/state/graph/policies.go b/internal/controller/state/graph/policies.go index b5a1251aee..ed12b5c7ef 100644 --- a/internal/controller/state/graph/policies.go +++ b/internal/controller/state/graph/policies.go @@ -325,12 +325,18 @@ func checkTargetRoutesForOverlap( func checkForRouteOverlap(route *L7Route, hostPortPaths map[string]string) *conditions.Condition { for _, parentRef := range route.ParentRefs { if parentRef.Attachment != nil { + // fmt.Printf("This is the parentRef attachment: %#v\n", parentRef.Attachment) port := parentRef.Attachment.ListenerPort for _, hostname := range parentRef.Attachment.AcceptedHostnames { + // fmt.Println("Here is the keyName: " + keyName) + // fmt.Printf("This is the parentref hostnames: %v\n", hostname) for _, rule := range route.Spec.Rules { + // fmt.Printf("This is the routeRule: %v\n", rule) for _, match := range rule.Matches { + // fmt.Printf("This is the match: %v\n", match) if match.Path != nil && match.Path.Value != nil { key := fmt.Sprintf("%s:%d%s", hostname, port, *match.Path.Value) + // fmt.Println("This is the key: " + key) if val, ok := hostPortPaths[key]; !ok { hostPortPaths[key] = fmt.Sprintf("%s/%s", route.Source.GetNamespace(), route.Source.GetName()) } else { diff --git a/internal/controller/state/graph/policies_test.go b/internal/controller/state/graph/policies_test.go index 6d477ea809..7df4a9d432 100644 --- a/internal/controller/state/graph/policies_test.go +++ b/internal/controller/state/graph/policies_test.go @@ -1126,6 +1126,7 @@ func TestProcessPolicies_RouteOverlap(t *testing.T) { policyGVK := schema.GroupVersionKind{Group: "Group", Version: "Version", Kind: "MyPolicy"} pol1, pol1Key := createTestPolicyAndKey(policyGVK, "pol1", hrRefCoffee) pol2, pol2Key := createTestPolicyAndKey(policyGVK, "pol2", hrRefCoffee, hrRefCoffeeTea) + pol3, pol3Key := createTestPolicyAndKey(policyGVK, "pol3", hrRefCoffeeTea) tests := []struct { validator validation.PolicyValidator @@ -1153,6 +1154,25 @@ func TestProcessPolicies_RouteOverlap(t *testing.T) { }, valid: true, }, + { + name: "no overlap two policies", + validator: &policiesfakes.FakeValidator{}, + policies: map[PolicyKey]policies.Policy{ + pol1Key: pol1, + pol3Key: pol3, + }, + routes: map[RouteKey]*L7Route{ + { + RouteType: RouteTypeHTTP, + NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr-coffee"}, + }: createTestRouteWithPaths("hr-coffee", "/coffee"), + { + RouteType: RouteTypeHTTP, + NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr-coffee-tea"}, + }: createTestRouteWithPaths("hr-coffee-tea", "/coffee-tea"), + }, + valid: true, + }, { name: "policy references route that overlaps a non-referenced route", validator: &policiesfakes.FakeValidator{}, @@ -1256,7 +1276,7 @@ func TestProcessPolicies_RouteOverlap(t *testing.T) { g := NewWithT(t) processed := processPolicies(test.policies, test.validator, test.routes, nil, gateways) - g.Expect(processed).To(HaveLen(1)) + g.Expect(processed).To(HaveLen(len(test.policies))) for _, pol := range processed { g.Expect(pol.Valid).To(Equal(test.valid)) diff --git a/internal/controller/state/graph/route_common.go b/internal/controller/state/graph/route_common.go index c702f1d46a..977be82d4b 100644 --- a/internal/controller/state/graph/route_common.go +++ b/internal/controller/state/graph/route_common.go @@ -285,34 +285,57 @@ func buildSectionNameRefs( } uniqueSectionsPerGateway := make(map[key]struct{}) - for i, p := range parentRefs { + parentRefIndex := 0 + + for _, p := range parentRefs { gw := findGatewayForParentRef(p, routeNamespace, gws) if gw == nil { continue } - var sectionName string - if p.SectionName != nil { - sectionName = string(*p.SectionName) - } - gwNsName := client.ObjectKeyFromObject(gw.Source) k := key{ - gwNsName: gwNsName, - sectionName: sectionName, + gwNsName: gwNsName, + } + + // If there is no section name, we create ParentRefs for each listener in the gateway + if p.SectionName == nil { + for _, l := range gw.Listeners { + sectionName := string(l.Source.Name) + k.sectionName = sectionName + + if _, exist := uniqueSectionsPerGateway[k]; exist { + return nil, fmt.Errorf("duplicate section name %q for Gateway %s", sectionName, gwNsName.String()) + } + uniqueSectionsPerGateway[k] = struct{}{} + + sectionNameRefs = append(sectionNameRefs, ParentRef{ + Idx: parentRefIndex, + Gateway: CreateParentRefGateway(gw), + SectionName: &l.Source.Name, + Port: p.Port, + }) + parentRefIndex++ + } + continue } + sectionName := string(*p.SectionName) + + k.sectionName = sectionName + if _, exist := uniqueSectionsPerGateway[k]; exist { return nil, fmt.Errorf("duplicate section name %q for Gateway %s", sectionName, gwNsName.String()) } uniqueSectionsPerGateway[k] = struct{}{} sectionNameRefs = append(sectionNameRefs, ParentRef{ - Idx: i, + Idx: parentRefIndex, Gateway: CreateParentRefGateway(gw), SectionName: p.SectionName, Port: p.Port, }) + parentRefIndex++ } return sectionNameRefs, nil diff --git a/internal/controller/state/graph/route_common_test.go b/internal/controller/state/graph/route_common_test.go index 8acf8c005b..86dbfb2565 100644 --- a/internal/controller/state/graph/route_common_test.go +++ b/internal/controller/state/graph/route_common_test.go @@ -25,6 +25,7 @@ func TestBuildSectionNameRefs(t *testing.T) { gwNsName1 := types.NamespacedName{Namespace: routeNamespace, Name: "gateway-1"} gwNsName2 := types.NamespacedName{Namespace: routeNamespace, Name: "gateway-2"} + gwNsName3 := types.NamespacedName{Namespace: routeNamespace, Name: "gateway-3"} parentRefs := []gatewayv1.ParentReference{ { @@ -51,6 +52,10 @@ func TestBuildSectionNameRefs(t *testing.T) { Name: gatewayv1.ObjectName("some-other-gateway"), SectionName: helpers.GetPointer[gatewayv1.SectionName]("same-name"), }, + { + Name: gatewayv1.ObjectName(gwNsName3.Name), + SectionName: nil, + }, } gws := map[types.NamespacedName]*Gateway{ @@ -70,6 +75,26 @@ func TestBuildSectionNameRefs(t *testing.T) { }, }, }, + gwNsName3: { + Listeners: []*Listener{ + { + Source: gatewayv1.Listener{ + Name: "http", + }, + }, + { + Source: gatewayv1.Listener{ + Name: "https", + }, + }, + }, + Source: &gatewayv1.Gateway{ + ObjectMeta: metav1.ObjectMeta{ + Name: gwNsName3.Name, + Namespace: gwNsName3.Namespace, + }, + }, + }, } expected := []ParentRef{ @@ -79,20 +104,30 @@ func TestBuildSectionNameRefs(t *testing.T) { SectionName: parentRefs[0].SectionName, }, { - Idx: 2, + Idx: 1, Gateway: CreateParentRefGateway(gws[gwNsName2]), SectionName: parentRefs[2].SectionName, }, { - Idx: 3, + Idx: 2, Gateway: CreateParentRefGateway(gws[gwNsName1]), SectionName: parentRefs[3].SectionName, }, { - Idx: 4, + Idx: 3, Gateway: CreateParentRefGateway(gws[gwNsName2]), SectionName: parentRefs[4].SectionName, }, + { + Idx: 4, + Gateway: CreateParentRefGateway(gws[gwNsName3]), + SectionName: helpers.GetPointer[gatewayv1.SectionName]("http"), + }, + { + Idx: 5, + Gateway: CreateParentRefGateway(gws[gwNsName3]), + SectionName: helpers.GetPointer[gatewayv1.SectionName]("https"), + }, } tests := []struct { @@ -121,20 +156,6 @@ func TestBuildSectionNameRefs(t *testing.T) { name: "duplicate sectionNames", expectedError: errors.New("duplicate section name \"http\" for Gateway test/gateway-1"), }, - { - parentRefs: []gatewayv1.ParentReference{ - { - Name: gatewayv1.ObjectName(gwNsName1.Name), - SectionName: nil, - }, - { - Name: gatewayv1.ObjectName(gwNsName1.Name), - SectionName: nil, - }, - }, - name: "nil sectionNames", - expectedError: errors.New("duplicate section name \"\" for Gateway test/gateway-1"), - }, } for _, test := range tests { From ba8f9dd177c8876a20921ab4fc174ae90081c39b Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 29 May 2025 16:14:38 -0700 Subject: [PATCH 2/8] Update build graph test --- internal/controller/state/graph/graph_test.go | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/internal/controller/state/graph/graph_test.go b/internal/controller/state/graph/graph_test.go index 9006e5bd46..04e60c33c8 100644 --- a/internal/controller/state/graph/graph_test.go +++ b/internal/controller/state/graph/graph_test.go @@ -778,6 +778,32 @@ func TestBuildGraph(t *testing.T) { NamespacedName: client.ObjectKeyFromObject(gw1.Source), EffectiveNginxProxy: np1Effective, }, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{}, + Attached: false, + FailedConditions: []conditions.Condition{conditions.NewRouteNotAllowedByListeners()}, + }, + SectionName: &gw1.Source.Spec.Listeners[0].Name, + }, + { + Idx: 1, + Gateway: &ParentRefGateway{ + NamespacedName: client.ObjectKeyFromObject(gw1.Source), + EffectiveNginxProxy: np1Effective, + }, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{}, + Attached: false, + FailedConditions: []conditions.Condition{conditions.NewRouteNotAllowedByListeners()}, + }, + SectionName: &gw1.Source.Spec.Listeners[1].Name, + }, + { + Idx: 2, + Gateway: &ParentRefGateway{ + NamespacedName: client.ObjectKeyFromObject(gw1.Source), + EffectiveNginxProxy: np1Effective, + }, Attachment: &ParentRefAttachmentStatus{ Attached: true, AcceptedHostnames: map[string][]string{ @@ -785,12 +811,26 @@ func TestBuildGraph(t *testing.T) { client.ObjectKeyFromObject(gw1.Source), "listener-443-2", ): {"fizz.example.org"}, + }, + }, + SectionName: &gw1.Source.Spec.Listeners[2].Name, + }, + { + Idx: 3, + Gateway: &ParentRefGateway{ + NamespacedName: client.ObjectKeyFromObject(gw1.Source), + EffectiveNginxProxy: np1Effective, + }, + Attachment: &ParentRefAttachmentStatus{ + Attached: true, + AcceptedHostnames: map[string][]string{ CreateGatewayListenerKey( client.ObjectKeyFromObject(gw1.Source), "listener-8443", ): {"fizz.example.org"}, }, }, + SectionName: &gw1.Source.Spec.Listeners[3].Name, }, }, Spec: L4RouteSpec{ @@ -820,11 +860,51 @@ func TestBuildGraph(t *testing.T) { NamespacedName: client.ObjectKeyFromObject(gw1.Source), EffectiveNginxProxy: np1Effective, }, + Attachment: &ParentRefAttachmentStatus{ + Attached: false, + AcceptedHostnames: map[string][]string{}, + FailedConditions: []conditions.Condition{conditions.NewRouteNotAllowedByListeners()}, + }, + SectionName: &gw1.Source.Spec.Listeners[0].Name, + }, + { + Idx: 1, + Gateway: &ParentRefGateway{ + NamespacedName: client.ObjectKeyFromObject(gw1.Source), + EffectiveNginxProxy: np1Effective, + }, + Attachment: &ParentRefAttachmentStatus{ + AcceptedHostnames: map[string][]string{}, + Attached: false, + FailedConditions: []conditions.Condition{conditions.NewRouteNotAllowedByListeners()}, + }, + SectionName: &gw1.Source.Spec.Listeners[1].Name, + }, + { + Idx: 2, + Gateway: &ParentRefGateway{ + NamespacedName: client.ObjectKeyFromObject(gw1.Source), + EffectiveNginxProxy: np1Effective, + }, + Attachment: &ParentRefAttachmentStatus{ + Attached: false, + AcceptedHostnames: map[string][]string{}, + FailedConditions: []conditions.Condition{conditions.NewRouteHostnameConflict()}, + }, + SectionName: &gw1.Source.Spec.Listeners[2].Name, + }, + { + Idx: 3, + Gateway: &ParentRefGateway{ + NamespacedName: client.ObjectKeyFromObject(gw1.Source), + EffectiveNginxProxy: np1Effective, + }, Attachment: &ParentRefAttachmentStatus{ Attached: false, AcceptedHostnames: map[string][]string{}, FailedConditions: []conditions.Condition{conditions.NewRouteHostnameConflict()}, }, + SectionName: &gw1.Source.Spec.Listeners[3].Name, }, }, Spec: L4RouteSpec{ From 49615d6e66ecebab6adff6e3ba68227b2930711b Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 29 May 2025 17:34:51 -0700 Subject: [PATCH 3/8] Refactor code to use closure function --- .../controller/state/graph/route_common.go | 31 ++++++++++--------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/internal/controller/state/graph/route_common.go b/internal/controller/state/graph/route_common.go index 977be82d4b..93f91448d4 100644 --- a/internal/controller/state/graph/route_common.go +++ b/internal/controller/state/graph/route_common.go @@ -277,14 +277,23 @@ func buildSectionNameRefs( routeNamespace string, gws map[types.NamespacedName]*Gateway, ) ([]ParentRef, error) { - sectionNameRefs := make([]ParentRef, 0, len(parentRefs)) - type key struct { gwNsName types.NamespacedName sectionName string } uniqueSectionsPerGateway := make(map[key]struct{}) + sectionNameRefs := make([]ParentRef, 0, len(parentRefs)) + + checkUniqueSections := func(key key) error { + if _, exist := uniqueSectionsPerGateway[key]; exist { + return fmt.Errorf("duplicate section name %q for Gateway %s", key.sectionName, key.gwNsName.String()) + } + + uniqueSectionsPerGateway[key] = struct{}{} + return nil + } + parentRefIndex := 0 for _, p := range parentRefs { @@ -301,13 +310,11 @@ func buildSectionNameRefs( // If there is no section name, we create ParentRefs for each listener in the gateway if p.SectionName == nil { for _, l := range gw.Listeners { - sectionName := string(l.Source.Name) - k.sectionName = sectionName + k.sectionName = string(l.Source.Name) - if _, exist := uniqueSectionsPerGateway[k]; exist { - return nil, fmt.Errorf("duplicate section name %q for Gateway %s", sectionName, gwNsName.String()) + if err := checkUniqueSections(k); err != nil { + return nil, err } - uniqueSectionsPerGateway[k] = struct{}{} sectionNameRefs = append(sectionNameRefs, ParentRef{ Idx: parentRefIndex, @@ -320,14 +327,10 @@ func buildSectionNameRefs( continue } - sectionName := string(*p.SectionName) - - k.sectionName = sectionName - - if _, exist := uniqueSectionsPerGateway[k]; exist { - return nil, fmt.Errorf("duplicate section name %q for Gateway %s", sectionName, gwNsName.String()) + k.sectionName = string(*p.SectionName) + if err := checkUniqueSections(k); err != nil { + return nil, err } - uniqueSectionsPerGateway[k] = struct{}{} sectionNameRefs = append(sectionNameRefs, ParentRef{ Idx: parentRefIndex, From 5308f8ca8d27d9962c89579a881ebfc76d549b9a Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 29 May 2025 17:49:43 -0700 Subject: [PATCH 4/8] Add another test for coverage --- .../controller/state/graph/route_common_test.go | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/internal/controller/state/graph/route_common_test.go b/internal/controller/state/graph/route_common_test.go index 86dbfb2565..4c6b4b660c 100644 --- a/internal/controller/state/graph/route_common_test.go +++ b/internal/controller/state/graph/route_common_test.go @@ -156,6 +156,20 @@ func TestBuildSectionNameRefs(t *testing.T) { name: "duplicate sectionNames", expectedError: errors.New("duplicate section name \"http\" for Gateway test/gateway-1"), }, + { + parentRefs: []gatewayv1.ParentReference{ + { + Name: gatewayv1.ObjectName(gwNsName3.Name), + SectionName: helpers.GetPointer[gatewayv1.SectionName]("http"), + }, + { + Name: gatewayv1.ObjectName(gwNsName3.Name), + SectionName: nil, + }, + }, + name: "duplicate sectionNames when one parentRef has no sectionName", + expectedError: errors.New("duplicate section name \"http\" for Gateway test/gateway-3"), + }, } for _, test := range tests { From 366624f44e4baaa138db5af68562255a2a633ac0 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 30 May 2025 00:12:52 -0700 Subject: [PATCH 5/8] Remove duplicate parentRefs from httproute status --- internal/controller/state/graph/graph_test.go | 12 ++--- .../controller/state/graph/route_common.go | 6 ++- .../state/graph/route_common_test.go | 2 +- .../controller/status/prepare_requests.go | 32 ++++++++++++- .../status/prepare_requests_test.go | 47 +++++++++++++++++++ 5 files changed, 89 insertions(+), 10 deletions(-) diff --git a/internal/controller/state/graph/graph_test.go b/internal/controller/state/graph/graph_test.go index 04e60c33c8..748a3aa20c 100644 --- a/internal/controller/state/graph/graph_test.go +++ b/internal/controller/state/graph/graph_test.go @@ -786,7 +786,7 @@ func TestBuildGraph(t *testing.T) { SectionName: &gw1.Source.Spec.Listeners[0].Name, }, { - Idx: 1, + Idx: 0, Gateway: &ParentRefGateway{ NamespacedName: client.ObjectKeyFromObject(gw1.Source), EffectiveNginxProxy: np1Effective, @@ -799,7 +799,7 @@ func TestBuildGraph(t *testing.T) { SectionName: &gw1.Source.Spec.Listeners[1].Name, }, { - Idx: 2, + Idx: 0, Gateway: &ParentRefGateway{ NamespacedName: client.ObjectKeyFromObject(gw1.Source), EffectiveNginxProxy: np1Effective, @@ -816,7 +816,7 @@ func TestBuildGraph(t *testing.T) { SectionName: &gw1.Source.Spec.Listeners[2].Name, }, { - Idx: 3, + Idx: 0, Gateway: &ParentRefGateway{ NamespacedName: client.ObjectKeyFromObject(gw1.Source), EffectiveNginxProxy: np1Effective, @@ -868,7 +868,7 @@ func TestBuildGraph(t *testing.T) { SectionName: &gw1.Source.Spec.Listeners[0].Name, }, { - Idx: 1, + Idx: 0, Gateway: &ParentRefGateway{ NamespacedName: client.ObjectKeyFromObject(gw1.Source), EffectiveNginxProxy: np1Effective, @@ -881,7 +881,7 @@ func TestBuildGraph(t *testing.T) { SectionName: &gw1.Source.Spec.Listeners[1].Name, }, { - Idx: 2, + Idx: 0, Gateway: &ParentRefGateway{ NamespacedName: client.ObjectKeyFromObject(gw1.Source), EffectiveNginxProxy: np1Effective, @@ -894,7 +894,7 @@ func TestBuildGraph(t *testing.T) { SectionName: &gw1.Source.Spec.Listeners[2].Name, }, { - Idx: 3, + Idx: 0, Gateway: &ParentRefGateway{ NamespacedName: client.ObjectKeyFromObject(gw1.Source), EffectiveNginxProxy: np1Effective, diff --git a/internal/controller/state/graph/route_common.go b/internal/controller/state/graph/route_common.go index 93f91448d4..235fd75bea 100644 --- a/internal/controller/state/graph/route_common.go +++ b/internal/controller/state/graph/route_common.go @@ -322,8 +322,12 @@ func buildSectionNameRefs( SectionName: &l.Source.Name, Port: p.Port, }) - parentRefIndex++ } + + // if the ParentRefs we create are for each listener in the same gateway, we keep the + // parentRefIndex the same so when we look at a route's parentRef's we can see + // if the parentRef is a unique parentRef or one we created internally + parentRefIndex++ continue } diff --git a/internal/controller/state/graph/route_common_test.go b/internal/controller/state/graph/route_common_test.go index 4c6b4b660c..fba7be33cf 100644 --- a/internal/controller/state/graph/route_common_test.go +++ b/internal/controller/state/graph/route_common_test.go @@ -124,7 +124,7 @@ func TestBuildSectionNameRefs(t *testing.T) { SectionName: helpers.GetPointer[gatewayv1.SectionName]("http"), }, { - Idx: 5, + Idx: 4, Gateway: CreateParentRefGateway(gws[gwNsName3]), SectionName: helpers.GetPointer[gatewayv1.SectionName]("https"), }, diff --git a/internal/controller/status/prepare_requests.go b/internal/controller/status/prepare_requests.go index 1da7f8e348..d24a1e4030 100644 --- a/internal/controller/status/prepare_requests.go +++ b/internal/controller/status/prepare_requests.go @@ -95,6 +95,32 @@ func PrepareRouteRequests( return reqs } +func removeDuplicateIndexParentRefs(parentRefs []graph.ParentRef) []graph.ParentRef { + idxCount := make(map[int]int) + for _, ref := range parentRefs { + idxCount[ref.Idx]++ + } + + seen := make(map[int]bool) + result := make([]graph.ParentRef, 0, len(parentRefs)) + + for _, ref := range parentRefs { + if seen[ref.Idx] { + continue // skip duplicates + } + seen[ref.Idx] = true + + // If this Idx was duplicated, clear SectionName + if idxCount[ref.Idx] > 1 { + ref.SectionName = nil + } + + result = append(result, ref) + } + + return result +} + func prepareRouteStatus( gatewayCtlrName string, parentRefs []graph.ParentRef, @@ -103,11 +129,13 @@ func prepareRouteStatus( transitionTime metav1.Time, srcGeneration int64, ) v1.RouteStatus { - parents := make([]v1.RouteParentStatus, 0, len(parentRefs)) + processedParentRefs := removeDuplicateIndexParentRefs(parentRefs) + + parents := make([]v1.RouteParentStatus, 0, len(processedParentRefs)) defaultConds := conditions.NewDefaultRouteConditions() - for _, ref := range parentRefs { + for _, ref := range processedParentRefs { failedAttachmentCondCount := 0 if ref.Attachment != nil { failedAttachmentCondCount = len(ref.Attachment.FailedConditions) diff --git a/internal/controller/status/prepare_requests_test.go b/internal/controller/status/prepare_requests_test.go index 81f2929481..764a76c863 100644 --- a/internal/controller/status/prepare_requests_test.go +++ b/internal/controller/status/prepare_requests_test.go @@ -72,6 +72,12 @@ var ( { SectionName: helpers.GetPointer[v1.SectionName]("listener-80-3"), }, + { + SectionName: helpers.GetPointer[v1.SectionName]("listener-443-1"), + }, + { + SectionName: helpers.GetPointer[v1.SectionName]("listener-443-2"), + }, }, } @@ -110,6 +116,22 @@ var ( FailedConditions: []conditions.Condition{invalidAttachmentCondition}, }, }, + { + Idx: 3, + Gateway: &graph.ParentRefGateway{NamespacedName: gwNsName}, + SectionName: commonRouteSpecValid.ParentRefs[3].SectionName, + Attachment: &graph.ParentRefAttachmentStatus{ + Attached: true, + }, + }, + { + Idx: 3, + Gateway: &graph.ParentRefGateway{NamespacedName: gwNsName}, + SectionName: commonRouteSpecValid.ParentRefs[4].SectionName, + Attachment: &graph.ParentRefAttachmentStatus{ + Attached: true, + }, + }, } parentRefsInvalid = []graph.ParentRef{ @@ -213,6 +235,31 @@ var ( }, }, }, + { + ParentRef: v1.ParentReference{ + Namespace: helpers.GetPointer(v1.Namespace(gwNsName.Namespace)), + Name: v1.ObjectName(gwNsName.Name), + }, + ControllerName: gatewayCtlrName, + Conditions: []metav1.Condition{ + { + Type: string(v1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + ObservedGeneration: 3, + LastTransitionTime: transitionTime, + Reason: string(v1.RouteReasonAccepted), + Message: "The route is accepted", + }, + { + Type: string(v1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + ObservedGeneration: 3, + LastTransitionTime: transitionTime, + Reason: string(v1.RouteReasonResolvedRefs), + Message: "All references are resolved", + }, + }, + }, }, } From be78ad38a2b8639ab96361c59b7f9204fb69fc09 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 30 May 2025 00:19:24 -0700 Subject: [PATCH 6/8] Add some function comments --- internal/controller/state/graph/policies.go | 6 ------ internal/controller/status/prepare_requests.go | 6 ++++++ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/controller/state/graph/policies.go b/internal/controller/state/graph/policies.go index ed12b5c7ef..b5a1251aee 100644 --- a/internal/controller/state/graph/policies.go +++ b/internal/controller/state/graph/policies.go @@ -325,18 +325,12 @@ func checkTargetRoutesForOverlap( func checkForRouteOverlap(route *L7Route, hostPortPaths map[string]string) *conditions.Condition { for _, parentRef := range route.ParentRefs { if parentRef.Attachment != nil { - // fmt.Printf("This is the parentRef attachment: %#v\n", parentRef.Attachment) port := parentRef.Attachment.ListenerPort for _, hostname := range parentRef.Attachment.AcceptedHostnames { - // fmt.Println("Here is the keyName: " + keyName) - // fmt.Printf("This is the parentref hostnames: %v\n", hostname) for _, rule := range route.Spec.Rules { - // fmt.Printf("This is the routeRule: %v\n", rule) for _, match := range rule.Matches { - // fmt.Printf("This is the match: %v\n", match) if match.Path != nil && match.Path.Value != nil { key := fmt.Sprintf("%s:%d%s", hostname, port, *match.Path.Value) - // fmt.Println("This is the key: " + key) if val, ok := hostPortPaths[key]; !ok { hostPortPaths[key] = fmt.Sprintf("%s/%s", route.Source.GetNamespace(), route.Source.GetName()) } else { diff --git a/internal/controller/status/prepare_requests.go b/internal/controller/status/prepare_requests.go index d24a1e4030..c1952c8143 100644 --- a/internal/controller/status/prepare_requests.go +++ b/internal/controller/status/prepare_requests.go @@ -95,6 +95,8 @@ func PrepareRouteRequests( return reqs } +// removeDuplicateIndexParentRefs removes duplicate ParentRefs by Idx, keeping the first occurrence. +// If an Idx is duplicated, the SectionName for the stored ParentRef is nil. func removeDuplicateIndexParentRefs(parentRefs []graph.ParentRef) []graph.ParentRef { idxCount := make(map[int]int) for _, ref := range parentRefs { @@ -129,6 +131,10 @@ func prepareRouteStatus( transitionTime metav1.Time, srcGeneration int64, ) v1.RouteStatus { + // If a route did not specify a sectionName in its parentRefs section, it will attempt to attach to all available + // listeners. In this case, parentRefs will be created and attached to the route for each attachable listener. + // These parentRefs will all have the same Idx, and in order to not duplicate route statuses for the same Gateway, + // we need to remove these duplicates. Additionally, we remove the sectionName. processedParentRefs := removeDuplicateIndexParentRefs(parentRefs) parents := make([]v1.RouteParentStatus, 0, len(processedParentRefs)) From 527c85bb77a3b2857d8aaef8b80025b239f5a430 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 30 May 2025 02:32:08 -0700 Subject: [PATCH 7/8] Fix conformance test --- .../controller/status/prepare_requests.go | 33 +++++---- .../status/prepare_requests_test.go | 70 ++++++++++++++++++- 2 files changed, 89 insertions(+), 14 deletions(-) diff --git a/internal/controller/status/prepare_requests.go b/internal/controller/status/prepare_requests.go index c1952c8143..4cb8fd5eab 100644 --- a/internal/controller/status/prepare_requests.go +++ b/internal/controller/status/prepare_requests.go @@ -98,29 +98,36 @@ func PrepareRouteRequests( // removeDuplicateIndexParentRefs removes duplicate ParentRefs by Idx, keeping the first occurrence. // If an Idx is duplicated, the SectionName for the stored ParentRef is nil. func removeDuplicateIndexParentRefs(parentRefs []graph.ParentRef) []graph.ParentRef { - idxCount := make(map[int]int) + idxToParentRef := make(map[int][]graph.ParentRef) for _, ref := range parentRefs { - idxCount[ref.Idx]++ + idxToParentRef[ref.Idx] = append(idxToParentRef[ref.Idx], ref) } - seen := make(map[int]bool) - result := make([]graph.ParentRef, 0, len(parentRefs)) + results := make([]graph.ParentRef, len(idxToParentRef)) - for _, ref := range parentRefs { - if seen[ref.Idx] { - continue // skip duplicates + for idx, refs := range idxToParentRef { + if len(refs) == 1 { + results[idx] = refs[0] + continue } - seen[ref.Idx] = true - // If this Idx was duplicated, clear SectionName - if idxCount[ref.Idx] > 1 { - ref.SectionName = nil + winningParentRef := graph.ParentRef{ + Idx: idx, + Gateway: refs[0].Gateway, + Attachment: refs[0].Attachment, } - result = append(result, ref) + for _, ref := range refs { + if ref.Attachment.Attached { + if len(ref.Attachment.FailedConditions) == 0 || winningParentRef.Attachment == nil { + winningParentRef.Attachment = ref.Attachment + } + } + } + results[idx] = winningParentRef } - return result + return results } func prepareRouteStatus( diff --git a/internal/controller/status/prepare_requests_test.go b/internal/controller/status/prepare_requests_test.go index 764a76c863..2480423d1e 100644 --- a/internal/controller/status/prepare_requests_test.go +++ b/internal/controller/status/prepare_requests_test.go @@ -78,6 +78,15 @@ var ( { SectionName: helpers.GetPointer[v1.SectionName]("listener-443-2"), }, + { + SectionName: helpers.GetPointer[v1.SectionName]("listener-443-3"), + }, + { + SectionName: helpers.GetPointer[v1.SectionName]("listener-8080-1"), + }, + { + SectionName: helpers.GetPointer[v1.SectionName]("listener-8080-2"), + }, }, } @@ -121,7 +130,8 @@ var ( Gateway: &graph.ParentRefGateway{NamespacedName: gwNsName}, SectionName: commonRouteSpecValid.ParentRefs[3].SectionName, Attachment: &graph.ParentRefAttachmentStatus{ - Attached: true, + Attached: false, + FailedConditions: []conditions.Condition{invalidAttachmentCondition}, }, }, { @@ -132,6 +142,33 @@ var ( Attached: true, }, }, + { + Idx: 3, + Gateway: &graph.ParentRefGateway{NamespacedName: gwNsName}, + SectionName: commonRouteSpecValid.ParentRefs[5].SectionName, + Attachment: &graph.ParentRefAttachmentStatus{ + Attached: true, + FailedConditions: []conditions.Condition{invalidAttachmentCondition}, + }, + }, + { + Idx: 4, + Gateway: &graph.ParentRefGateway{NamespacedName: gwNsName}, + SectionName: commonRouteSpecValid.ParentRefs[6].SectionName, + Attachment: &graph.ParentRefAttachmentStatus{ + Attached: false, + FailedConditions: []conditions.Condition{invalidAttachmentCondition}, + }, + }, + { + Idx: 4, + Gateway: &graph.ParentRefGateway{NamespacedName: gwNsName}, + SectionName: commonRouteSpecValid.ParentRefs[7].SectionName, + Attachment: &graph.ParentRefAttachmentStatus{ + Attached: false, + FailedConditions: []conditions.Condition{invalidAttachmentCondition}, + }, + }, } parentRefsInvalid = []graph.ParentRef{ @@ -260,6 +297,37 @@ var ( }, }, }, + { + ParentRef: v1.ParentReference{ + Namespace: helpers.GetPointer(v1.Namespace(gwNsName.Namespace)), + Name: v1.ObjectName(gwNsName.Name), + }, + ControllerName: gatewayCtlrName, + Conditions: []metav1.Condition{ + { + Type: string(v1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + ObservedGeneration: 3, + LastTransitionTime: transitionTime, + Reason: string(v1.RouteReasonAccepted), + Message: "The route is accepted", + }, + { + Type: string(v1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + ObservedGeneration: 3, + LastTransitionTime: transitionTime, + Reason: string(v1.RouteReasonResolvedRefs), + Message: "All references are resolved", + }, + { + Type: invalidAttachmentCondition.Type, + Status: metav1.ConditionTrue, + ObservedGeneration: 3, + LastTransitionTime: transitionTime, + }, + }, + }, }, } From c84872f5852a68ee8be4ceac87a77547a9179968 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 30 May 2025 09:44:45 -0700 Subject: [PATCH 8/8] Revert parentRefIndex --- internal/controller/state/graph/route_common.go | 16 ++++++---------- .../controller/state/graph/route_common_test.go | 10 +++++----- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/internal/controller/state/graph/route_common.go b/internal/controller/state/graph/route_common.go index 235fd75bea..1cbb2fc480 100644 --- a/internal/controller/state/graph/route_common.go +++ b/internal/controller/state/graph/route_common.go @@ -294,9 +294,7 @@ func buildSectionNameRefs( return nil } - parentRefIndex := 0 - - for _, p := range parentRefs { + for i, p := range parentRefs { gw := findGatewayForParentRef(p, routeNamespace, gws) if gw == nil { continue @@ -317,17 +315,16 @@ func buildSectionNameRefs( } sectionNameRefs = append(sectionNameRefs, ParentRef{ - Idx: parentRefIndex, + // if the ParentRefs we create are for each listener in the same gateway, we keep the + // parentRefIndex the same so when we look at a route's parentRef's we can see + // if the parentRef is a unique parentRef or one we created internally + Idx: i, Gateway: CreateParentRefGateway(gw), SectionName: &l.Source.Name, Port: p.Port, }) } - // if the ParentRefs we create are for each listener in the same gateway, we keep the - // parentRefIndex the same so when we look at a route's parentRef's we can see - // if the parentRef is a unique parentRef or one we created internally - parentRefIndex++ continue } @@ -337,12 +334,11 @@ func buildSectionNameRefs( } sectionNameRefs = append(sectionNameRefs, ParentRef{ - Idx: parentRefIndex, + Idx: i, Gateway: CreateParentRefGateway(gw), SectionName: p.SectionName, Port: p.Port, }) - parentRefIndex++ } return sectionNameRefs, nil diff --git a/internal/controller/state/graph/route_common_test.go b/internal/controller/state/graph/route_common_test.go index fba7be33cf..4b4fd1c2fe 100644 --- a/internal/controller/state/graph/route_common_test.go +++ b/internal/controller/state/graph/route_common_test.go @@ -104,27 +104,27 @@ func TestBuildSectionNameRefs(t *testing.T) { SectionName: parentRefs[0].SectionName, }, { - Idx: 1, + Idx: 2, Gateway: CreateParentRefGateway(gws[gwNsName2]), SectionName: parentRefs[2].SectionName, }, { - Idx: 2, + Idx: 3, Gateway: CreateParentRefGateway(gws[gwNsName1]), SectionName: parentRefs[3].SectionName, }, { - Idx: 3, + Idx: 4, Gateway: CreateParentRefGateway(gws[gwNsName2]), SectionName: parentRefs[4].SectionName, }, { - Idx: 4, + Idx: 6, Gateway: CreateParentRefGateway(gws[gwNsName3]), SectionName: helpers.GetPointer[gatewayv1.SectionName]("http"), }, { - Idx: 4, + Idx: 6, Gateway: CreateParentRefGateway(gws[gwNsName3]), SectionName: helpers.GetPointer[gatewayv1.SectionName]("https"), },