diff --git a/internal/controller/state/graph/graph_test.go b/internal/controller/state/graph/graph_test.go index 9006e5bd46..748a3aa20c 100644 --- a/internal/controller/state/graph/graph_test.go +++ b/internal/controller/state/graph/graph_test.go @@ -772,6 +772,32 @@ func TestBuildGraph(t *testing.T) { Attachable: true, Source: tr, ParentRefs: []ParentRef{ + { + Idx: 0, + 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[0].Name, + }, + { + Idx: 0, + 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: 0, Gateway: &ParentRefGateway{ @@ -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: 0, + 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{ @@ -814,6 +854,45 @@ func TestBuildGraph(t *testing.T) { Attachable: true, Source: tr2, ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: &ParentRefGateway{ + 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: 0, + 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: 0, + 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: 0, Gateway: &ParentRefGateway{ @@ -825,6 +904,7 @@ func TestBuildGraph(t *testing.T) { AcceptedHostnames: map[string][]string{}, FailedConditions: []conditions.Condition{conditions.NewRouteHostnameConflict()}, }, + SectionName: &gw1.Source.Spec.Listeners[3].Name, }, }, Spec: L4RouteSpec{ 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..1cbb2fc480 100644 --- a/internal/controller/state/graph/route_common.go +++ b/internal/controller/state/graph/route_common.go @@ -277,35 +277,61 @@ 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 + } + for i, 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 { + k.sectionName = string(l.Source.Name) + + if err := checkUniqueSections(k); err != nil { + return nil, err + } + + sectionNameRefs = append(sectionNameRefs, ParentRef{ + // 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, + }) + } + + continue } - 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: i, diff --git a/internal/controller/state/graph/route_common_test.go b/internal/controller/state/graph/route_common_test.go index 8acf8c005b..4b4fd1c2fe 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{ @@ -93,6 +118,16 @@ func TestBuildSectionNameRefs(t *testing.T) { Gateway: CreateParentRefGateway(gws[gwNsName2]), SectionName: parentRefs[4].SectionName, }, + { + Idx: 6, + Gateway: CreateParentRefGateway(gws[gwNsName3]), + SectionName: helpers.GetPointer[gatewayv1.SectionName]("http"), + }, + { + Idx: 6, + Gateway: CreateParentRefGateway(gws[gwNsName3]), + SectionName: helpers.GetPointer[gatewayv1.SectionName]("https"), + }, } tests := []struct { @@ -124,16 +159,16 @@ func TestBuildSectionNameRefs(t *testing.T) { { parentRefs: []gatewayv1.ParentReference{ { - Name: gatewayv1.ObjectName(gwNsName1.Name), - SectionName: nil, + Name: gatewayv1.ObjectName(gwNsName3.Name), + SectionName: helpers.GetPointer[gatewayv1.SectionName]("http"), }, { - Name: gatewayv1.ObjectName(gwNsName1.Name), + Name: gatewayv1.ObjectName(gwNsName3.Name), SectionName: nil, }, }, - name: "nil sectionNames", - expectedError: errors.New("duplicate section name \"\" for Gateway test/gateway-1"), + name: "duplicate sectionNames when one parentRef has no sectionName", + expectedError: errors.New("duplicate section name \"http\" for Gateway test/gateway-3"), }, } diff --git a/internal/controller/status/prepare_requests.go b/internal/controller/status/prepare_requests.go index 1da7f8e348..4cb8fd5eab 100644 --- a/internal/controller/status/prepare_requests.go +++ b/internal/controller/status/prepare_requests.go @@ -95,6 +95,41 @@ 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 { + idxToParentRef := make(map[int][]graph.ParentRef) + for _, ref := range parentRefs { + idxToParentRef[ref.Idx] = append(idxToParentRef[ref.Idx], ref) + } + + results := make([]graph.ParentRef, len(idxToParentRef)) + + for idx, refs := range idxToParentRef { + if len(refs) == 1 { + results[idx] = refs[0] + continue + } + + winningParentRef := graph.ParentRef{ + Idx: idx, + Gateway: refs[0].Gateway, + Attachment: refs[0].Attachment, + } + + 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 results +} + func prepareRouteStatus( gatewayCtlrName string, parentRefs []graph.ParentRef, @@ -103,11 +138,17 @@ func prepareRouteStatus( transitionTime metav1.Time, srcGeneration int64, ) v1.RouteStatus { - parents := make([]v1.RouteParentStatus, 0, len(parentRefs)) + // 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)) 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..2480423d1e 100644 --- a/internal/controller/status/prepare_requests_test.go +++ b/internal/controller/status/prepare_requests_test.go @@ -72,6 +72,21 @@ 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"), + }, + { + SectionName: helpers.GetPointer[v1.SectionName]("listener-443-3"), + }, + { + SectionName: helpers.GetPointer[v1.SectionName]("listener-8080-1"), + }, + { + SectionName: helpers.GetPointer[v1.SectionName]("listener-8080-2"), + }, }, } @@ -110,6 +125,50 @@ var ( FailedConditions: []conditions.Condition{invalidAttachmentCondition}, }, }, + { + Idx: 3, + Gateway: &graph.ParentRefGateway{NamespacedName: gwNsName}, + SectionName: commonRouteSpecValid.ParentRefs[3].SectionName, + Attachment: &graph.ParentRefAttachmentStatus{ + Attached: false, + FailedConditions: []conditions.Condition{invalidAttachmentCondition}, + }, + }, + { + Idx: 3, + Gateway: &graph.ParentRefGateway{NamespacedName: gwNsName}, + SectionName: commonRouteSpecValid.ParentRefs[4].SectionName, + Attachment: &graph.ParentRefAttachmentStatus{ + 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{ @@ -213,6 +272,62 @@ 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", + }, + }, + }, + { + 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, + }, + }, + }, }, }