diff --git a/internal/state/change_processor_test.go b/internal/state/change_processor_test.go index cef061248c..1788be7ae0 100644 --- a/internal/state/change_processor_test.go +++ b/internal/state/change_processor_test.go @@ -335,14 +335,18 @@ var _ = Describe("ChangeProcessor", func() { HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ {Namespace: "test", Name: "hr-1"}: { ObservedGeneration: hr1.Generation, - ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": { + ParentStatuses: []state.ParentStatus{ + { + GatewayNsName: client.ObjectKeyFromObject(gw1), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), Conditions: append( conditions.NewDefaultRouteConditions(), conditions.NewTODO("GatewayClass is invalid or doesn't exist"), ), }, - "listener-443-1": { + { + GatewayNsName: client.ObjectKeyFromObject(gw1), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-443-1"), Conditions: append( conditions.NewDefaultRouteConditions(), conditions.NewTODO("GatewayClass is invalid or doesn't exist"), @@ -440,12 +444,16 @@ var _ = Describe("ChangeProcessor", func() { HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ {Namespace: "test", Name: "hr-1"}: { ObservedGeneration: hr1.Generation, - ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": { - Conditions: conditions.NewDefaultRouteConditions(), + ParentStatuses: []state.ParentStatus{ + { + GatewayNsName: client.ObjectKeyFromObject(gw1), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), + Conditions: conditions.NewDefaultRouteConditions(), }, - "listener-443-1": { - Conditions: conditions.NewDefaultRouteConditions(), + { + GatewayNsName: client.ObjectKeyFromObject(gw1), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-443-1"), + Conditions: conditions.NewDefaultRouteConditions(), }, }, }, @@ -549,12 +557,16 @@ var _ = Describe("ChangeProcessor", func() { HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ {Namespace: "test", Name: "hr-1"}: { ObservedGeneration: hr1Updated.Generation, - ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": { - Conditions: conditions.NewDefaultRouteConditions(), + ParentStatuses: []state.ParentStatus{ + { + GatewayNsName: client.ObjectKeyFromObject(gw1), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), + Conditions: conditions.NewDefaultRouteConditions(), }, - "listener-443-1": { - Conditions: conditions.NewDefaultRouteConditions(), + { + GatewayNsName: client.ObjectKeyFromObject(gw1), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-443-1"), + Conditions: conditions.NewDefaultRouteConditions(), }, }, }, @@ -659,12 +671,16 @@ var _ = Describe("ChangeProcessor", func() { HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ {Namespace: "test", Name: "hr-1"}: { ObservedGeneration: hr1Updated.Generation, - ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": { - Conditions: conditions.NewDefaultRouteConditions(), + ParentStatuses: []state.ParentStatus{ + { + GatewayNsName: client.ObjectKeyFromObject(gw1Updated), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), + Conditions: conditions.NewDefaultRouteConditions(), }, - "listener-443-1": { - Conditions: conditions.NewDefaultRouteConditions(), + { + GatewayNsName: client.ObjectKeyFromObject(gw1Updated), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-443-1"), + Conditions: conditions.NewDefaultRouteConditions(), }, }, }, @@ -768,12 +784,16 @@ var _ = Describe("ChangeProcessor", func() { HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ {Namespace: "test", Name: "hr-1"}: { ObservedGeneration: hr1Updated.Generation, - ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": { - Conditions: conditions.NewDefaultRouteConditions(), + ParentStatuses: []state.ParentStatus{ + { + GatewayNsName: client.ObjectKeyFromObject(gw1Updated), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), + Conditions: conditions.NewDefaultRouteConditions(), }, - "listener-443-1": { - Conditions: conditions.NewDefaultRouteConditions(), + { + GatewayNsName: client.ObjectKeyFromObject(gw1Updated), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-443-1"), + Conditions: conditions.NewDefaultRouteConditions(), }, }, }, @@ -880,12 +900,16 @@ var _ = Describe("ChangeProcessor", func() { HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ {Namespace: "test", Name: "hr-1"}: { ObservedGeneration: hr1Updated.Generation, - ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": { - Conditions: conditions.NewDefaultRouteConditions(), + ParentStatuses: []state.ParentStatus{ + { + GatewayNsName: client.ObjectKeyFromObject(gw1Updated), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), + Conditions: conditions.NewDefaultRouteConditions(), }, - "listener-443-1": { - Conditions: conditions.NewDefaultRouteConditions(), + { + GatewayNsName: client.ObjectKeyFromObject(gw1Updated), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-443-1"), + Conditions: conditions.NewDefaultRouteConditions(), }, }, }, @@ -981,25 +1005,33 @@ var _ = Describe("ChangeProcessor", func() { HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ {Namespace: "test", Name: "hr-1"}: { ObservedGeneration: hr1Updated.Generation, - ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": { - Conditions: conditions.NewDefaultRouteConditions(), + ParentStatuses: []state.ParentStatus{ + { + GatewayNsName: client.ObjectKeyFromObject(gw1Updated), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), + Conditions: conditions.NewDefaultRouteConditions(), }, - "listener-443-1": { - Conditions: conditions.NewDefaultRouteConditions(), + { + GatewayNsName: client.ObjectKeyFromObject(gw1Updated), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-443-1"), + Conditions: conditions.NewDefaultRouteConditions(), }, }, }, {Namespace: "test", Name: "hr-2"}: { ObservedGeneration: hr2.Generation, - ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": { + ParentStatuses: []state.ParentStatus{ + { + GatewayNsName: client.ObjectKeyFromObject(gw2), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), Conditions: append( conditions.NewDefaultRouteConditions(), conditions.NewTODO("Gateway is ignored"), ), }, - "listener-443-1": { + { + GatewayNsName: client.ObjectKeyFromObject(gw2), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-443-1"), Conditions: append( conditions.NewDefaultRouteConditions(), conditions.NewTODO("Gateway is ignored"), @@ -1098,12 +1130,16 @@ var _ = Describe("ChangeProcessor", func() { HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ {Namespace: "test", Name: "hr-2"}: { ObservedGeneration: hr2.Generation, - ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": { - Conditions: conditions.NewDefaultRouteConditions(), + ParentStatuses: []state.ParentStatus{ + { + GatewayNsName: client.ObjectKeyFromObject(gw2), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), + Conditions: conditions.NewDefaultRouteConditions(), }, - "listener-443-1": { - Conditions: conditions.NewDefaultRouteConditions(), + { + GatewayNsName: client.ObjectKeyFromObject(gw2), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-443-1"), + Conditions: conditions.NewDefaultRouteConditions(), }, }, }, @@ -2102,9 +2138,11 @@ var _ = Describe("ChangeProcessor", func() { HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ hrNsName: { ObservedGeneration: hr.Generation, - ParentStatuses: map[string]state.ParentStatus{ - "listener-80-1": { - Conditions: conditions.NewDefaultRouteConditions(), + ParentStatuses: []state.ParentStatus{ + { + GatewayNsName: gwNsName, + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), + Conditions: conditions.NewDefaultRouteConditions(), }, }, }, diff --git a/internal/state/graph/backend_refs_test.go b/internal/state/graph/backend_refs_test.go index 17c67dfd6f..4826c9f34d 100644 --- a/internal/state/graph/backend_refs_test.go +++ b/internal/state/graph/backend_refs_test.go @@ -309,11 +309,11 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { return rules } - const sectionName = "test" - sectionNameRefs := map[string]ParentRef{ - sectionName: { - Idx: 0, - Gateway: types.NamespacedName{Namespace: "test", Name: "gateway"}, + sectionNameRefs := []ParentRef{ + { + Idx: 0, + Gateway: types.NamespacedName{Namespace: "test", Name: "gateway"}, + Attached: true, }, } @@ -337,10 +337,10 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { }{ { route: &Route{ - Source: hrWithOneBackend, - SectionNameRefs: sectionNameRefs, - Valid: true, - Rules: createRules(hrWithOneBackend, allValid, allValid), + Source: hrWithOneBackend, + ParentRefs: sectionNameRefs, + Valid: true, + Rules: createRules(hrWithOneBackend, allValid, allValid), }, expectedBackendGroups: []BackendGroup{ { @@ -362,10 +362,10 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { }, { route: &Route{ - Source: hrWithTwoBackends, - SectionNameRefs: sectionNameRefs, - Valid: true, - Rules: createRules(hrWithTwoBackends, allValid, allValid), + Source: hrWithTwoBackends, + ParentRefs: sectionNameRefs, + Valid: true, + Rules: createRules(hrWithTwoBackends, allValid, allValid), }, expectedBackendGroups: []BackendGroup{ { @@ -394,9 +394,9 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { }, { route: &Route{ - Source: hrWithOneBackend, - SectionNameRefs: sectionNameRefs, - Valid: false, + Source: hrWithOneBackend, + ParentRefs: sectionNameRefs, + Valid: false, }, expectedBackendGroups: nil, expectedConditions: nil, @@ -404,10 +404,10 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { }, { route: &Route{ - Source: hrWithOneBackend, - SectionNameRefs: sectionNameRefs, - Valid: true, - Rules: createRules(hrWithOneBackend, allInvalid, allValid), + Source: hrWithOneBackend, + ParentRefs: sectionNameRefs, + Valid: true, + Rules: createRules(hrWithOneBackend, allInvalid, allValid), }, expectedBackendGroups: nil, expectedConditions: nil, @@ -415,10 +415,10 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { }, { route: &Route{ - Source: hrWithOneBackend, - SectionNameRefs: sectionNameRefs, - Valid: true, - Rules: createRules(hrWithOneBackend, allValid, allInvalid), + Source: hrWithOneBackend, + ParentRefs: sectionNameRefs, + Valid: true, + Rules: createRules(hrWithOneBackend, allValid, allInvalid), }, expectedBackendGroups: nil, expectedConditions: nil, @@ -426,10 +426,10 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { }, { route: &Route{ - Source: hrWithInvalidRule, - SectionNameRefs: sectionNameRefs, - Valid: true, - Rules: createRules(hrWithInvalidRule, allValid, allValid), + Source: hrWithInvalidRule, + ParentRefs: sectionNameRefs, + Valid: true, + Rules: createRules(hrWithInvalidRule, allValid, allValid), }, expectedBackendGroups: []BackendGroup{ { @@ -451,10 +451,10 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { }, { route: &Route{ - Source: hrWithZeroBackendRefs, - SectionNameRefs: sectionNameRefs, - Valid: true, - Rules: createRules(hrWithZeroBackendRefs, allValid, allValid), + Source: hrWithZeroBackendRefs, + ParentRefs: sectionNameRefs, + Valid: true, + Rules: createRules(hrWithZeroBackendRefs, allValid, allValid), }, expectedBackendGroups: []BackendGroup{ { @@ -474,7 +474,7 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { addBackendGroupsToRoute(test.route, services) g.Expect(helpers.Diff(test.expectedBackendGroups, test.route.GetAllBackendGroups())).To(BeEmpty()) - g.Expect(test.route.GetAllConditionsForSectionName(sectionName)).To(Equal(test.expectedConditions)) + g.Expect(test.route.Conditions).To(Equal(test.expectedConditions)) }) } } diff --git a/internal/state/graph/graph_test.go b/internal/state/graph/graph_test.go index 277f2ea619..be5c52346c 100644 --- a/internal/state/graph/graph_test.go +++ b/internal/state/graph/graph_test.go @@ -12,7 +12,6 @@ import ( "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets/secretsfakes" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/validation" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/validation/validationfakes" @@ -179,27 +178,27 @@ func TestBuildGraph(t *testing.T) { routeHR1 := &Route{ Valid: true, Source: hr1, - SectionNameRefs: map[string]ParentRef{ - "listener-80-1": { - Idx: 0, - Gateway: client.ObjectKeyFromObject(gw1), + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw1), + Attached: true, }, }, - UnattachedSectionNameRefs: map[string]conditions.Condition{}, - Rules: []Rule{createValidRuleWithBackendGroup(hr1Group)}, + Rules: []Rule{createValidRuleWithBackendGroup(hr1Group)}, } routeHR3 := &Route{ Valid: true, Source: hr3, - SectionNameRefs: map[string]ParentRef{ - "listener-443-1": { - Idx: 0, - Gateway: client.ObjectKeyFromObject(gw1), + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw1), + Attached: true, }, }, - UnattachedSectionNameRefs: map[string]conditions.Condition{}, - Rules: []Rule{createValidRuleWithBackendGroup(hr3Group)}, + Rules: []Rule{createValidRuleWithBackendGroup(hr3Group)}, } secretMemoryMgr := &secretsfakes.FakeSecretDiskMemoryManager{} diff --git a/internal/state/graph/httproute.go b/internal/state/graph/httproute.go index ee23f26d01..c2a50ba58d 100644 --- a/internal/state/graph/httproute.go +++ b/internal/state/graph/httproute.go @@ -27,10 +27,14 @@ type Rule struct { // ParentRef describes a reference to a parent in an HTTPRoute. type ParentRef struct { + // FailedAttachmentCondition describes the failure of the attachment when Attached is false. + FailedAttachmentCondition conditions.Condition // Gateway is the NamespacedName of the referenced Gateway Gateway types.NamespacedName - // Idx is the index of the reference in the HTTPRoute. + // Idx is the index of the corresponding ParentReference in the HTTPRoute. Idx int + // Attached indicates if the ParentRef is attached to the Gateway. + Attached bool } // Route represents an HTTPRoute. @@ -40,11 +44,8 @@ type Route struct { // For now, we assume that the source is only HTTPRoute. // Later we can support more types - TLSRoute, TCPRoute and UDPRoute. Source *v1beta1.HTTPRoute - // SectionNameRefs is a map of section names to the referenced NKG Gateways - SectionNameRefs map[string]ParentRef - // UnattachedSectionNameRefs is a subset of SectionNameRefs that includes sections that could not be attached - // to the referenced Gateway. For example, because section does not exist in the Gateway. - UnattachedSectionNameRefs map[string]conditions.Condition + // ParentRefs includes ParentRefs with NKG Gateways only. + ParentRefs []ParentRef // Conditions include Conditions for the HTTPRoute. Conditions []conditions.Condition // Rules include Rules for the HTTPRoute. Each Rule[i] corresponds to the ith HTTPRouteRule. @@ -85,35 +86,6 @@ func (r *Route) GetAllBackendGroups() []BackendGroup { return groups } -// GetAllConditionsForSectionName returns all Conditions for the referenced section name. -// It panics if the section name does not exist. -func (r *Route) GetAllConditionsForSectionName(name string) []conditions.Condition { - if _, exist := r.SectionNameRefs[name]; !exist { - panic(fmt.Errorf("section name %q does not exist", name)) - } - - count := len(r.Conditions) - - unattachedCond, sectionIsUnattached := r.UnattachedSectionNameRefs[name] - if sectionIsUnattached { - count++ - } - - if count == 0 { - return nil - } - - conds := make([]conditions.Condition, 0, count) - - if sectionIsUnattached { - conds = append(conds, unattachedCond) - } - - conds = append(conds, r.Conditions...) - - return conds -} - // buildRoutesForGateways builds routes from HTTPRoutes that reference any of the specified Gateways. func buildRoutesForGateways( validator validation.HTTPFieldsValidator, @@ -140,8 +112,14 @@ func buildSectionNameRefs( parentRefs []v1beta1.ParentReference, routeNamespace string, gatewayNsNames []types.NamespacedName, -) map[string]ParentRef { - sectionNameRefs := make(map[string]ParentRef) +) []ParentRef { + sectionNameRefs := make([]ParentRef, 0, len(parentRefs)) + + type key struct { + gwNsName types.NamespacedName + sectionName string + } + uniqueSectionsPerGateway := make(map[key]struct{}) for i, p := range parentRefs { gw, found := findGatewayForParentRef(p, routeNamespace, gatewayNsNames) @@ -149,20 +127,25 @@ func buildSectionNameRefs( continue } - // FIXME(pleshakov): SectionNames across multiple Gateways might collide. Fix that. var sectionName string if p.SectionName != nil { sectionName = string(*p.SectionName) } - if _, exist := sectionNameRefs[sectionName]; exist { - panicForBrokenWebhookAssumption(fmt.Errorf("duplicate section name %q", sectionName)) + k := key{ + gwNsName: gw, + sectionName: sectionName, + } + + if _, exist := uniqueSectionsPerGateway[k]; exist { + panicForBrokenWebhookAssumption(fmt.Errorf("duplicate section name %q for Gateway %s", sectionName, gw.String())) } + uniqueSectionsPerGateway[k] = struct{}{} - sectionNameRefs[sectionName] = ParentRef{ + sectionNameRefs = append(sectionNameRefs, ParentRef{ Idx: i, Gateway: gw, - } + }) } return sectionNameRefs @@ -207,9 +190,8 @@ func buildRoute( } r := &Route{ - Source: ghr, - SectionNameRefs: sectionNameRefs, - UnattachedSectionNameRefs: map[string]conditions.Condition{}, + Source: ghr, + ParentRefs: sectionNameRefs, } err := validateHostnames(ghr.Spec.Hostnames, field.NewPath("spec").Child("hostnames")) @@ -297,7 +279,8 @@ func bindRouteToListeners(r *Route, gw *Gateway) { return } - for name, ref := range r.SectionNameRefs { + for i := 0; i < len(r.ParentRefs); i++ { + ref := &r.ParentRefs[i] routeRef := r.Source.Spec.ParentRefs[ref.Idx] path := field.NewPath("spec").Child("parentRefs").Index(ref.Idx) @@ -306,13 +289,13 @@ func bindRouteToListeners(r *Route, gw *Gateway) { if routeRef.SectionName == nil || *routeRef.SectionName == "" { valErr := field.Required(path.Child("sectionName"), "cannot be empty") - r.UnattachedSectionNameRefs[name] = conditions.NewRouteUnsupportedValue(valErr.Error()) + ref.FailedAttachmentCondition = conditions.NewRouteUnsupportedValue(valErr.Error()) continue } if routeRef.Port != nil { valErr := field.Forbidden(path.Child("port"), "cannot be set") - r.UnattachedSectionNameRefs[name] = conditions.NewRouteUnsupportedValue(valErr.Error()) + ref.FailedAttachmentCondition = conditions.NewRouteUnsupportedValue(valErr.Error()) continue } @@ -321,7 +304,7 @@ func bindRouteToListeners(r *Route, gw *Gateway) { referencesWinningGw := ref.Gateway.Namespace == gw.Source.Namespace && ref.Gateway.Name == gw.Source.Name if !referencesWinningGw { - r.UnattachedSectionNameRefs[name] = conditions.NewTODO("Gateway is ignored") + ref.FailedAttachmentCondition = conditions.NewTODO("Gateway is ignored") continue } @@ -340,26 +323,28 @@ func bindRouteToListeners(r *Route, gw *Gateway) { // - listener 2 for port 80 with hostname *.example.com; // In this case, the Route host foo.example.com should choose listener 1, as it is a more specific match. - l, exists := gw.Listeners[name] + l, exists := gw.Listeners[string(*routeRef.SectionName)] if !exists { // FIXME(pleshakov): Add a proper condition once it is available in the Gateway API. // https://github.com/nginxinc/nginx-kubernetes-gateway/issues/306 - r.UnattachedSectionNameRefs[name] = conditions.NewTODO("listener is not found") + ref.FailedAttachmentCondition = conditions.NewTODO("listener is not found") continue } if !l.Valid { - r.UnattachedSectionNameRefs[name] = conditions.NewRouteInvalidListener() + ref.FailedAttachmentCondition = conditions.NewRouteInvalidListener() continue } accepted := findAcceptedHostnames(l.Source.Hostname, r.Source.Spec.Hostnames) if len(accepted) == 0 { - r.UnattachedSectionNameRefs[name] = conditions.NewRouteNoMatchingListenerHostname() + ref.FailedAttachmentCondition = conditions.NewRouteNoMatchingListenerHostname() continue } + ref.Attached = true + for _, h := range accepted { l.AcceptedHostnames[h] = struct{}{} } diff --git a/internal/state/graph/httproute_test.go b/internal/state/graph/httproute_test.go index daa2c06ed9..0ab760555c 100644 --- a/internal/state/graph/httproute_test.go +++ b/internal/state/graph/httproute_test.go @@ -139,84 +139,6 @@ func TestRouteGetAllBackendGroups(t *testing.T) { } } -func TestGetAllConditionsForSectionName(t *testing.T) { - const ( - sectionName = "foo" - ) - - sectionNameRefs := map[string]ParentRef{ - sectionName: { - Idx: 0, - Gateway: types.NamespacedName{Namespace: "test", Name: "gateway"}, - }, - } - - tests := []struct { - route *Route - name string - expected []conditions.Condition - }{ - { - route: &Route{ - SectionNameRefs: sectionNameRefs, - Conditions: nil, - }, - expected: nil, - name: "no conditions", - }, - { - route: &Route{ - SectionNameRefs: sectionNameRefs, - UnattachedSectionNameRefs: map[string]conditions.Condition{ - sectionName: conditions.NewTODO("unattached"), - }, - }, - expected: []conditions.Condition{ - conditions.NewTODO("unattached"), - }, - name: "unattached section", - }, - { - route: &Route{ - SectionNameRefs: sectionNameRefs, - UnattachedSectionNameRefs: map[string]conditions.Condition{ - sectionName: conditions.NewTODO("unattached"), - }, - Conditions: []conditions.Condition{conditions.NewTODO("route")}, - }, - expected: []conditions.Condition{ - conditions.NewTODO("unattached"), - conditions.NewTODO("route"), - }, - name: "unattached section and route", - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - g := NewGomegaWithT(t) - result := test.route.GetAllConditionsForSectionName(sectionName) - g.Expect(result).To(Equal(test.expected)) - }) - } -} - -func TestGetAllConditionsForSectionNamePanics(t *testing.T) { - route := &Route{ - SectionNameRefs: map[string]ParentRef{ - "foo": { - Idx: 0, - Gateway: types.NamespacedName{Namespace: "test", Name: "gateway"}, - }, - }, - } - - invoke := func() { _ = route.GetAllConditionsForSectionName("bar") } - - g := NewGomegaWithT(t) - g.Expect(invoke).To(Panic()) -} - func TestBuildRoutes(t *testing.T) { gwNsName := types.NamespacedName{Namespace: "test", Name: "gateway"} @@ -238,14 +160,13 @@ func TestBuildRoutes(t *testing.T) { expected: map[types.NamespacedName]*Route{ client.ObjectKeyFromObject(hr): { Source: hr, - SectionNameRefs: map[string]ParentRef{ - sectionNameOfCreateHTTPRoute: { + ParentRefs: []ParentRef{ + { Idx: 0, Gateway: gwNsName, }, }, - UnattachedSectionNameRefs: map[string]conditions.Condition{}, - Valid: true, + Valid: true, Rules: []Rule{ { ValidMatches: true, @@ -279,8 +200,10 @@ func TestBuildRoutes(t *testing.T) { } func TestBuildSectionNameRefs(t *testing.T) { - gwNsName1 := types.NamespacedName{Namespace: "test", Name: "gateway-1"} - gwNsName2 := types.NamespacedName{Namespace: "test", Name: "gateway-2"} + const routeNamespace = "test" + + gwNsName1 := types.NamespacedName{Namespace: routeNamespace, Name: "gateway-1"} + gwNsName2 := types.NamespacedName{Namespace: routeNamespace, Name: "gateway-2"} parentRefs := []v1beta1.ParentReference{ { @@ -288,27 +211,46 @@ func TestBuildSectionNameRefs(t *testing.T) { SectionName: helpers.GetPointer[v1beta1.SectionName]("one"), }, { - Name: v1beta1.ObjectName("some-gateway"), - SectionName: helpers.GetPointer[v1beta1.SectionName]("other"), + Name: v1beta1.ObjectName("some-other-gateway"), + SectionName: helpers.GetPointer[v1beta1.SectionName]("two"), }, { Name: v1beta1.ObjectName(gwNsName2.Name), - SectionName: helpers.GetPointer[v1beta1.SectionName]("two"), + SectionName: helpers.GetPointer[v1beta1.SectionName]("three"), + }, + { + Name: v1beta1.ObjectName(gwNsName1.Name), + SectionName: helpers.GetPointer[v1beta1.SectionName]("same-name"), + }, + { + Name: v1beta1.ObjectName(gwNsName2.Name), + SectionName: helpers.GetPointer[v1beta1.SectionName]("same-name"), + }, + { + Name: v1beta1.ObjectName("some-other-gateway"), + SectionName: helpers.GetPointer[v1beta1.SectionName]("same-name"), }, } gwNsNames := []types.NamespacedName{gwNsName1, gwNsName2} - routeNamespace := "test" - expected := map[string]ParentRef{ - "one": { + expected := []ParentRef{ + { Idx: 0, Gateway: gwNsName1, }, - "two": { + { Idx: 2, Gateway: gwNsName2, }, + { + Idx: 3, + Gateway: gwNsName1, + }, + { + Idx: 4, + Gateway: gwNsName2, + }, } g := NewGomegaWithT(t) @@ -317,6 +259,52 @@ func TestBuildSectionNameRefs(t *testing.T) { g.Expect(result).To(Equal(expected)) } +func TestBuildSectionNameRefsPanicsForDuplicateParentRefs(t *testing.T) { + gwNsName := types.NamespacedName{Namespace: "test", Name: "gateway"} + + tests := []struct { + name string + parentRefs []v1beta1.ParentReference + }{ + { + parentRefs: []v1beta1.ParentReference{ + { + Name: v1beta1.ObjectName(gwNsName.Name), + SectionName: helpers.GetPointer[v1beta1.SectionName]("http"), + }, + { + Name: v1beta1.ObjectName(gwNsName.Name), + SectionName: helpers.GetPointer[v1beta1.SectionName]("http"), + }, + }, + name: "with sectionNames", + }, + { + parentRefs: []v1beta1.ParentReference{ + { + Name: v1beta1.ObjectName(gwNsName.Name), + SectionName: nil, + }, + { + Name: v1beta1.ObjectName(gwNsName.Name), + SectionName: nil, + }, + }, + name: "nil sectionNames", + }, + } + + gwNsNames := []types.NamespacedName{gwNsName} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + g := NewGomegaWithT(t) + run := func() { buildSectionNameRefs(test.parentRefs, gwNsName.Namespace, gwNsNames) } + g.Expect(run).To(Panic()) + }) + } +} + func TestFindGatewayForParentRef(t *testing.T) { gwNsName1 := types.NamespacedName{Namespace: "test-1", Name: "gateway-1"} gwNsName2 := types.NamespacedName{Namespace: "test-2", Name: "gateway-2"} @@ -460,14 +448,13 @@ func TestBuildRoute(t *testing.T) { hr: hr, expected: &Route{ Source: hr, - SectionNameRefs: map[string]ParentRef{ - sectionNameOfCreateHTTPRoute: { + ParentRefs: []ParentRef{ + { Idx: 0, Gateway: gatewayNsName, }, }, - UnattachedSectionNameRefs: map[string]conditions.Condition{}, - Valid: true, + Valid: true, Rules: []Rule{ { ValidMatches: true, @@ -507,13 +494,12 @@ func TestBuildRoute(t *testing.T) { expected: &Route{ Source: hrInvalidHostname, Valid: false, - SectionNameRefs: map[string]ParentRef{ - sectionNameOfCreateHTTPRoute: { + ParentRefs: []ParentRef{ + { Idx: 0, Gateway: gatewayNsName, }, }, - UnattachedSectionNameRefs: map[string]conditions.Condition{}, Conditions: []conditions.Condition{ conditions.NewRouteUnsupportedValue(`spec.hostnames[0]: Invalid value: "": cannot be empty string`), }, @@ -526,13 +512,12 @@ func TestBuildRoute(t *testing.T) { expected: &Route{ Source: hrInvalidMatches, Valid: false, - SectionNameRefs: map[string]ParentRef{ - sectionNameOfCreateHTTPRoute: { + ParentRefs: []ParentRef{ + { Idx: 0, Gateway: gatewayNsName, }, }, - UnattachedSectionNameRefs: map[string]conditions.Condition{}, Conditions: []conditions.Condition{ conditions.NewRouteUnsupportedValue( `All rules are invalid: spec.rules[0].matches[0].path: Invalid value: "/invalid": invalid path`, @@ -557,13 +542,12 @@ func TestBuildRoute(t *testing.T) { expected: &Route{ Source: hrInvalidFilters, Valid: false, - SectionNameRefs: map[string]ParentRef{ - sectionNameOfCreateHTTPRoute: { + ParentRefs: []ParentRef{ + { Idx: 0, Gateway: gatewayNsName, }, }, - UnattachedSectionNameRefs: map[string]conditions.Condition{}, Conditions: []conditions.Condition{ conditions.NewRouteUnsupportedValue( `All rules are invalid: spec.rules[0].filters[0].requestRedirect.hostname: ` + @@ -588,13 +572,12 @@ func TestBuildRoute(t *testing.T) { expected: &Route{ Source: hrInvalidValidRules, Valid: true, - SectionNameRefs: map[string]ParentRef{ - sectionNameOfCreateHTTPRoute: { + ParentRefs: []ParentRef{ + { Idx: 0, Gateway: gatewayNsName, }, }, - UnattachedSectionNameRefs: map[string]conditions.Condition{}, Conditions: []conditions.Condition{ conditions.NewTODO( `Some rules are invalid: ` + @@ -709,75 +692,77 @@ func TestBindRouteToListeners(t *testing.T) { nil, ) - normalRoute := &Route{ - Source: hr, - Valid: true, - SectionNameRefs: map[string]ParentRef{ - "listener-80-1": { - Idx: 0, - Gateway: client.ObjectKeyFromObject(gw), + var normalRoute *Route + createNormalRoute := func() *Route { + normalRoute = &Route{ + Source: hr, + Valid: true, + ParentRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + }, }, - }, - UnattachedSectionNameRefs: map[string]conditions.Condition{}, + } + return normalRoute + } + getLastNormalRoute := func() *Route { + return normalRoute } + routeWithMissingSectionName := &Route{ Source: hrWithNilSectionName, Valid: true, - SectionNameRefs: map[string]ParentRef{ - "": { + ParentRefs: []ParentRef{ + { Idx: 0, Gateway: client.ObjectKeyFromObject(gw), }, }, - UnattachedSectionNameRefs: map[string]conditions.Condition{}, } routeWithEmptySectionName := &Route{ Source: hrWithEmptySectionName, Valid: true, - SectionNameRefs: map[string]ParentRef{ - "": { + ParentRefs: []ParentRef{ + { Idx: 0, Gateway: client.ObjectKeyFromObject(gw), }, }, - UnattachedSectionNameRefs: map[string]conditions.Condition{}, } routeWithNonExistingListener := &Route{ Source: hrWithNonExistingListener, Valid: true, - SectionNameRefs: map[string]ParentRef{ - "listener-80-2": { + ParentRefs: []ParentRef{ + { Idx: 0, Gateway: client.ObjectKeyFromObject(gw), }, }, - UnattachedSectionNameRefs: map[string]conditions.Condition{}, } routeWithPort := &Route{ Source: hrWithPort, Valid: true, - SectionNameRefs: map[string]ParentRef{ - "listener-80-1": { + ParentRefs: []ParentRef{ + { Idx: 0, Gateway: client.ObjectKeyFromObject(gw), }, }, - UnattachedSectionNameRefs: map[string]conditions.Condition{}, } + ignoredGwNsName := types.NamespacedName{Namespace: "test", Name: "ignored-gateway"} routeWithIgnoredGateway := &Route{ Source: hr, Valid: true, - SectionNameRefs: map[string]ParentRef{ - "listener-80-1": { + ParentRefs: []ParentRef{ + { Idx: 0, - Gateway: types.NamespacedName{Namespace: "test", Name: "ignored-gateway"}, + Gateway: ignoredGwNsName, }, }, - UnattachedSectionNameRefs: map[string]conditions.Condition{}, } notValidRoute := &Route{ - Valid: false, - UnattachedSectionNameRefs: map[string]conditions.Condition{}, + Valid: false, } notValidListener := createModifiedListener(func(l *Listener) { @@ -793,20 +778,27 @@ func TestBindRouteToListeners(t *testing.T) { expectedRouteUnattachedSectionNameRefs map[string]conditions.Condition expectedGatewayListeners map[string]*Listener name string + expectedSectionNameRefs []ParentRef }{ { - route: normalRoute, + route: createNormalRoute(), gateway: &Gateway{ Source: gw, Listeners: map[string]*Listener{ "listener-80-1": createListener(), }, }, - expectedRouteUnattachedSectionNameRefs: map[string]conditions.Condition{}, + expectedSectionNameRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + Attached: true, + }, + }, expectedGatewayListeners: map[string]*Listener{ "listener-80-1": createModifiedListener(func(l *Listener) { l.Routes = map[types.NamespacedName]*Route{ - client.ObjectKeyFromObject(hr): normalRoute, + client.ObjectKeyFromObject(hr): getLastNormalRoute(), } l.AcceptedHostnames = map[string]struct{}{ "foo.example.com": {}, @@ -823,8 +815,15 @@ func TestBindRouteToListeners(t *testing.T) { "listener-80-1": createListener(), }, }, - expectedRouteUnattachedSectionNameRefs: map[string]conditions.Condition{ - "": conditions.NewRouteUnsupportedValue(`spec.parentRefs[0].sectionName: Required value: cannot be empty`), + expectedSectionNameRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + Attached: false, + FailedAttachmentCondition: conditions.NewRouteUnsupportedValue( + `spec.parentRefs[0].sectionName: Required value: cannot be empty`, + ), + }, }, expectedGatewayListeners: map[string]*Listener{ "listener-80-1": createListener(), @@ -839,8 +838,15 @@ func TestBindRouteToListeners(t *testing.T) { "listener-80-1": createListener(), }, }, - expectedRouteUnattachedSectionNameRefs: map[string]conditions.Condition{ - "": conditions.NewRouteUnsupportedValue(`spec.parentRefs[0].sectionName: Required value: cannot be empty`), + expectedSectionNameRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + Attached: false, + FailedAttachmentCondition: conditions.NewRouteUnsupportedValue( + `spec.parentRefs[0].sectionName: Required value: cannot be empty`, + ), + }, }, expectedGatewayListeners: map[string]*Listener{ "listener-80-1": createListener(), @@ -855,8 +861,15 @@ func TestBindRouteToListeners(t *testing.T) { "listener-80-1": createListener(), }, }, - expectedRouteUnattachedSectionNameRefs: map[string]conditions.Condition{ - "listener-80-1": conditions.NewRouteUnsupportedValue(`spec.parentRefs[0].port: Forbidden: cannot be set`), + expectedSectionNameRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + Attached: false, + FailedAttachmentCondition: conditions.NewRouteUnsupportedValue( + `spec.parentRefs[0].port: Forbidden: cannot be set`, + ), + }, }, expectedGatewayListeners: map[string]*Listener{ "listener-80-1": createListener(), @@ -871,8 +884,13 @@ func TestBindRouteToListeners(t *testing.T) { "listener-80-1": createListener(), }, }, - expectedRouteUnattachedSectionNameRefs: map[string]conditions.Condition{ - "listener-80-2": conditions.NewTODO("listener is not found"), + expectedSectionNameRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + Attached: false, + FailedAttachmentCondition: conditions.NewTODO("listener is not found"), + }, }, expectedGatewayListeners: map[string]*Listener{ "listener-80-1": createListener(), @@ -880,15 +898,20 @@ func TestBindRouteToListeners(t *testing.T) { name: "listener doesn't exist", }, { - route: normalRoute, + route: createNormalRoute(), gateway: &Gateway{ Source: gw, Listeners: map[string]*Listener{ "listener-80-1": notValidListener, }, }, - expectedRouteUnattachedSectionNameRefs: map[string]conditions.Condition{ - "listener-80-1": conditions.NewRouteInvalidListener(), + expectedSectionNameRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + Attached: false, + FailedAttachmentCondition: conditions.NewRouteInvalidListener(), + }, }, expectedGatewayListeners: map[string]*Listener{ "listener-80-1": notValidListener, @@ -896,15 +919,20 @@ func TestBindRouteToListeners(t *testing.T) { name: "listener isn't valid", }, { - route: normalRoute, + route: createNormalRoute(), gateway: &Gateway{ Source: gw, Listeners: map[string]*Listener{ "listener-80-1": nonMatchingHostnameListener, }, }, - expectedRouteUnattachedSectionNameRefs: map[string]conditions.Condition{ - "listener-80-1": conditions.NewRouteNoMatchingListenerHostname(), + expectedSectionNameRefs: []ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + Attached: false, + FailedAttachmentCondition: conditions.NewRouteNoMatchingListenerHostname(), + }, }, expectedGatewayListeners: map[string]*Listener{ "listener-80-1": nonMatchingHostnameListener, @@ -919,8 +947,13 @@ func TestBindRouteToListeners(t *testing.T) { "listener-80-1": createListener(), }, }, - expectedRouteUnattachedSectionNameRefs: map[string]conditions.Condition{ - "listener-80-1": conditions.NewTODO("Gateway is ignored"), + expectedSectionNameRefs: []ParentRef{ + { + Idx: 0, + Gateway: ignoredGwNsName, + Attached: false, + FailedAttachmentCondition: conditions.NewTODO("Gateway is ignored"), + }, }, expectedGatewayListeners: map[string]*Listener{ "listener-80-1": createListener(), @@ -949,7 +982,7 @@ func TestBindRouteToListeners(t *testing.T) { bindRouteToListeners(test.route, test.gateway) - g.Expect(test.route.UnattachedSectionNameRefs).To(Equal(test.expectedRouteUnattachedSectionNameRefs)) + g.Expect(test.route.ParentRefs).To(Equal(test.expectedSectionNameRefs)) g.Expect(helpers.Diff(test.gateway.Listeners, test.expectedGatewayListeners)).To(BeEmpty()) }) } diff --git a/internal/state/statuses.go b/internal/state/statuses.go index 7ce0f9ae81..a4898c833d 100644 --- a/internal/state/statuses.go +++ b/internal/state/statuses.go @@ -3,6 +3,7 @@ package state import ( "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/graph" @@ -48,19 +49,20 @@ type ListenerStatus struct { AttachedRoutes int32 } -// ParentStatuses holds the statuses of parents where the key is the section name in a parentRef. -type ParentStatuses map[string]ParentStatus - // HTTPRouteStatus holds the status-related information about an HTTPRoute resource. type HTTPRouteStatus struct { // ParentStatuses holds the statuses for parentRefs of the HTTPRoute. - ParentStatuses ParentStatuses + ParentStatuses []ParentStatus // ObservedGeneration is the generation of the resource that was processed. ObservedGeneration int64 } // ParentStatus holds status-related information related to how the HTTPRoute binds to a specific parentRef. type ParentStatus struct { + // GatewayNsName is the Namespaced name of the Gateway, which the parentRef references. + GatewayNsName types.NamespacedName + // SectionName is the SectionName of the parentRef. + SectionName *v1beta1.SectionName // Conditions is the list of conditions that are relevant to the parentRef. Conditions []conditions.Condition } @@ -102,14 +104,19 @@ func buildStatuses(graph *graph.Graph) Statuses { defaultConds := conditions.NewDefaultListenerConditions() for name, l := range graph.Gateway.Listeners { - conds := make([]conditions.Condition, 0, len(l.Conditions)+len(defaultConds)+1) // 1 is for missing GC + missingGCCondCount := 0 + if !gcValidAndExist { + missingGCCondCount = 1 + } + + conds := make([]conditions.Condition, 0, len(l.Conditions)+len(defaultConds)+missingGCCondCount) // We add default conds first, so that any additional conditions will override them, which is // ensured by DeduplicateConditions. conds = append(conds, defaultConds...) conds = append(conds, l.Conditions...) - if !gcValidAndExist { + if missingGCCondCount == 1 { // FIXME(pleshakov): Figure out appropriate conditions for the cases when: // (1) GatewayClass is invalid. // (2) GatewayClass does not exist. @@ -135,30 +142,32 @@ func buildStatuses(graph *graph.Graph) Statuses { } for nsname, r := range graph.Routes { - parentStatuses := make(map[string]ParentStatus) + parentStatuses := make([]ParentStatus, 0, len(r.ParentRefs)) baseConds := buildBaseRouteConditions(gcValidAndExist) - for ref := range r.SectionNameRefs { - conds := r.GetAllConditionsForSectionName(ref) + for _, ref := range r.ParentRefs { + failedAttachmentCondCount := 0 + if !ref.Attached { + failedAttachmentCondCount = 1 + } + allConds := make([]conditions.Condition, 0, len(r.Conditions)+len(baseConds)+failedAttachmentCondCount) - allConds := make([]conditions.Condition, 0, len(conds)+len(baseConds)) // We add baseConds first, so that any additional conditions will override them, which is // ensured by DeduplicateConditions. allConds = append(allConds, baseConds...) - allConds = append(allConds, conds...) - - if ref == "" { - // FIXME(pleshakov): Gateway API spec does allow empty section names in the status. - // However, NKG doesn't yet support the empty section names. - // Once NKG supports them, it will be able to determine which section name the HTTPRoute was - // attached to. So we won't need this workaround. - ref = "unattached" + allConds = append(allConds, r.Conditions...) + if failedAttachmentCondCount == 1 { + allConds = append(allConds, ref.FailedAttachmentCondition) } - parentStatuses[ref] = ParentStatus{ - Conditions: conditions.DeduplicateConditions(allConds), - } + routeRef := r.Source.Spec.ParentRefs[ref.Idx] + + parentStatuses = append(parentStatuses, ParentStatus{ + GatewayNsName: ref.Gateway, + SectionName: routeRef.SectionName, + Conditions: conditions.DeduplicateConditions(allConds), + }) } statuses.HTTPRouteStatuses[nsname] = HTTPRouteStatus{ diff --git a/internal/state/statuses_test.go b/internal/state/statuses_test.go index 644407dc24..299a1d9125 100644 --- a/internal/state/statuses_test.go +++ b/internal/state/statuses_test.go @@ -51,44 +51,32 @@ func TestBuildStatuses(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Generation: 3, }, - }, - SectionNameRefs: map[string]graph.ParentRef{ - "listener-80-1": { - Idx: 0, - Gateway: client.ObjectKeyFromObject(gw), - }, - "listener-80-2": { - Idx: 0, - Gateway: client.ObjectKeyFromObject(ignoredGw), - }, - }, - UnattachedSectionNameRefs: map[string]conditions.Condition{ - "listener-80-2": invalidCondition, - }, - }, - } - - routesAllRefsInvalid := map[types.NamespacedName]*graph.Route{ - {Namespace: "test", Name: "hr-1"}: { - Source: &v1beta1.HTTPRoute{ - ObjectMeta: metav1.ObjectMeta{ - Generation: 4, + Spec: v1beta1.HTTPRouteSpec{ + CommonRouteSpec: v1beta1.CommonRouteSpec{ + ParentRefs: []v1beta1.ParentReference{ + { + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), + }, + { + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-2"), + }, + }, + }, }, }, - SectionNameRefs: map[string]graph.ParentRef{ - "listener-80-1": { - Idx: 0, - Gateway: client.ObjectKeyFromObject(gw), + ParentRefs: []graph.ParentRef{ + { + Idx: 0, + Gateway: client.ObjectKeyFromObject(gw), + Attached: true, }, - "listener-80-2": { - Idx: 0, - Gateway: client.ObjectKeyFromObject(ignoredGw), + { + Idx: 1, + Gateway: client.ObjectKeyFromObject(gw), + Attached: false, + FailedAttachmentCondition: invalidCondition, }, }, - UnattachedSectionNameRefs: map[string]conditions.Condition{ - "listener-80-1": invalidCondition, - "listener-80-2": invalidCondition, - }, }, } @@ -135,11 +123,15 @@ func TestBuildStatuses(t *testing.T) { HTTPRouteStatuses: map[types.NamespacedName]HTTPRouteStatus{ {Namespace: "test", Name: "hr-1"}: { ObservedGeneration: 3, - ParentStatuses: map[string]ParentStatus{ - "listener-80-1": { - Conditions: conditions.NewDefaultRouteConditions(), + ParentStatuses: []ParentStatus{ + { + GatewayNsName: client.ObjectKeyFromObject(gw), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), + Conditions: conditions.NewDefaultRouteConditions(), }, - "listener-80-2": { + { + GatewayNsName: client.ObjectKeyFromObject(gw), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-2"), Conditions: append( conditions.NewDefaultRouteConditions(), invalidCondition, @@ -184,14 +176,18 @@ func TestBuildStatuses(t *testing.T) { HTTPRouteStatuses: map[types.NamespacedName]HTTPRouteStatus{ {Namespace: "test", Name: "hr-1"}: { ObservedGeneration: 3, - ParentStatuses: map[string]ParentStatus{ - "listener-80-1": { + ParentStatuses: []ParentStatus{ + { + GatewayNsName: client.ObjectKeyFromObject(gw), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), Conditions: append( conditions.NewDefaultRouteConditions(), conditions.NewTODO("GatewayClass is invalid or doesn't exist"), ), }, - "listener-80-2": { + { + GatewayNsName: client.ObjectKeyFromObject(gw), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-2"), Conditions: append( conditions.NewDefaultRouteConditions(), conditions.NewTODO("GatewayClass is invalid or doesn't exist"), @@ -250,14 +246,18 @@ func TestBuildStatuses(t *testing.T) { HTTPRouteStatuses: map[types.NamespacedName]HTTPRouteStatus{ {Namespace: "test", Name: "hr-1"}: { ObservedGeneration: 3, - ParentStatuses: map[string]ParentStatus{ - "listener-80-1": { + ParentStatuses: []ParentStatus{ + { + GatewayNsName: client.ObjectKeyFromObject(gw), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-1"), Conditions: append( conditions.NewDefaultRouteConditions(), conditions.NewTODO("GatewayClass is invalid or doesn't exist"), ), }, - "listener-80-2": { + { + GatewayNsName: client.ObjectKeyFromObject(gw), + SectionName: helpers.GetPointer[v1beta1.SectionName]("listener-80-2"), Conditions: append( conditions.NewDefaultRouteConditions(), conditions.NewTODO("GatewayClass is invalid or doesn't exist"), @@ -270,47 +270,6 @@ func TestBuildStatuses(t *testing.T) { }, name: "gatewayclass is not valid", }, - { - graph: &graph.Graph{ - GatewayClass: &graph.GatewayClass{ - Source: &v1beta1.GatewayClass{ - ObjectMeta: metav1.ObjectMeta{Generation: 1}, - }, - Valid: true, - }, - Gateway: nil, - IgnoredGateways: nil, - Routes: routesAllRefsInvalid, - }, - expected: Statuses{ - GatewayClassStatus: &GatewayClassStatus{ - ObservedGeneration: 1, - Conditions: conditions.NewDefaultGatewayClassConditions(), - }, - GatewayStatus: nil, - IgnoredGatewayStatuses: map[types.NamespacedName]IgnoredGatewayStatus{}, - HTTPRouteStatuses: map[types.NamespacedName]HTTPRouteStatus{ - {Namespace: "test", Name: "hr-1"}: { - ObservedGeneration: 4, - ParentStatuses: map[string]ParentStatus{ - "listener-80-1": { - Conditions: append( - conditions.NewDefaultRouteConditions(), - invalidCondition, - ), - }, - "listener-80-2": { - Conditions: append( - conditions.NewDefaultRouteConditions(), - invalidCondition, - ), - }, - }, - }, - }, - }, - name: "gateway and ignored gateways don't exist", - }, } for _, test := range tests { diff --git a/internal/status/httproute.go b/internal/status/httproute.go index d6a8369302..7c22e63f55 100644 --- a/internal/status/httproute.go +++ b/internal/status/httproute.go @@ -1,10 +1,7 @@ package status import ( - "sort" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" @@ -16,29 +13,17 @@ import ( // Extend support to cover more cases. func prepareHTTPRouteStatus( status state.HTTPRouteStatus, - gwNsName types.NamespacedName, gatewayCtlrName string, transitionTime metav1.Time, ) v1beta1.HTTPRouteStatus { parents := make([]v1beta1.RouteParentStatus, 0, len(status.ParentStatuses)) - // FIXME(pleshakov) Maintain the order from the HTTPRoute resource - names := make([]string, 0, len(status.ParentStatuses)) - for name := range status.ParentStatuses { - names = append(names, name) - } - sort.Strings(names) - - for _, name := range names { - ps := status.ParentStatuses[name] - - sectionName := name - + for _, ps := range status.ParentStatuses { p := v1beta1.RouteParentStatus{ ParentRef: v1beta1.ParentReference{ - Namespace: (*v1beta1.Namespace)(&gwNsName.Namespace), - Name: v1beta1.ObjectName(gwNsName.Name), - SectionName: (*v1beta1.SectionName)(§ionName), + Namespace: (*v1beta1.Namespace)(&ps.GatewayNsName.Namespace), + Name: v1beta1.ObjectName(ps.GatewayNsName.Name), + SectionName: ps.SectionName, }, ControllerName: v1beta1.GatewayController(gatewayCtlrName), Conditions: convertConditions(ps.Conditions, status.ObservedGeneration, transitionTime), diff --git a/internal/status/httproute_test.go b/internal/status/httproute_test.go index 1bbbcac9f8..3804eddd6c 100644 --- a/internal/status/httproute_test.go +++ b/internal/status/httproute_test.go @@ -14,20 +14,26 @@ import ( ) func TestPrepareHTTPRouteStatus(t *testing.T) { - g := NewGomegaWithT(t) + gwNsName1 := types.NamespacedName{Namespace: "test", Name: "gateway-1"} + gwNsName2 := types.NamespacedName{Namespace: "test", Name: "gateway-2"} status := state.HTTPRouteStatus{ ObservedGeneration: 1, - ParentStatuses: map[string]state.ParentStatus{ - "parent": { - Conditions: CreateTestConditions(), + ParentStatuses: []state.ParentStatus{ + { + GatewayNsName: gwNsName1, + SectionName: helpers.GetPointer[v1beta1.SectionName]("http"), + Conditions: CreateTestConditions(), + }, + { + GatewayNsName: gwNsName2, + SectionName: nil, + Conditions: CreateTestConditions(), }, }, } - gwNsName := types.NamespacedName{Namespace: "test", Name: "gateway"} gatewayCtlrName := "test.example.com" - transitionTime := metav1.NewTime(time.Now()) expected := v1beta1.HTTPRouteStatus{ @@ -35,9 +41,18 @@ func TestPrepareHTTPRouteStatus(t *testing.T) { Parents: []v1beta1.RouteParentStatus{ { ParentRef: v1beta1.ParentReference{ - Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer("test")), - Name: "gateway", - SectionName: (*v1beta1.SectionName)(helpers.GetStringPointer("parent")), + Namespace: helpers.GetPointer(v1beta1.Namespace(gwNsName1.Namespace)), + Name: v1beta1.ObjectName(gwNsName1.Name), + SectionName: helpers.GetPointer[v1beta1.SectionName]("http"), + }, + ControllerName: v1beta1.GatewayController(gatewayCtlrName), + Conditions: CreateExpectedAPIConditions(1, transitionTime), + }, + { + ParentRef: v1beta1.ParentReference{ + Namespace: helpers.GetPointer(v1beta1.Namespace(gwNsName2.Namespace)), + Name: v1beta1.ObjectName(gwNsName2.Name), + SectionName: nil, }, ControllerName: v1beta1.GatewayController(gatewayCtlrName), Conditions: CreateExpectedAPIConditions(1, transitionTime), @@ -45,6 +60,9 @@ func TestPrepareHTTPRouteStatus(t *testing.T) { }, }, } - result := prepareHTTPRouteStatus(status, gwNsName, gatewayCtlrName, transitionTime) + + g := NewGomegaWithT(t) + + result := prepareHTTPRouteStatus(status, gatewayCtlrName, transitionTime) g.Expect(helpers.Diff(expected, result)).To(BeEmpty()) } diff --git a/internal/status/updater.go b/internal/status/updater.go index 6bc5c72fae..5abc9a339a 100644 --- a/internal/status/updater.go +++ b/internal/status/updater.go @@ -130,7 +130,6 @@ func (upd *updaterImpl) Update(ctx context.Context, statuses state.Statuses) { // statuses.GatewayStatus is never nil when len(statuses.HTTPRouteStatuses) > 0 hr.Status = prepareHTTPRouteStatus( rs, - statuses.GatewayStatus.NsName, upd.cfg.GatewayCtlrName, upd.cfg.Clock.Now(), ) diff --git a/internal/status/updater_test.go b/internal/status/updater_test.go index 3a43f72c87..21ed6c147d 100644 --- a/internal/status/updater_test.go +++ b/internal/status/updater_test.go @@ -93,9 +93,11 @@ var _ = Describe("Updater", func() { HTTPRouteStatuses: map[types.NamespacedName]state.HTTPRouteStatus{ {Namespace: "test", Name: "route1"}: { ObservedGeneration: 5, - ParentStatuses: map[string]state.ParentStatus{ - "http": { - Conditions: status.CreateTestConditions(), + ParentStatuses: []state.ParentStatus{ + { + GatewayNsName: types.NamespacedName{Namespace: "test", Name: "gateway"}, + SectionName: helpers.GetPointer[v1beta1.SectionName]("http"), + Conditions: status.CreateTestConditions(), }, }, },