Fix HTTPRoute section name related bugs (#568)

- Previously, NKG assumed section names in parentRefs of HTTPRoute were unique, even when they referenced different Gateways. This commit fixes that. Fixes #484 - Previously, when NKG reported status for a parentRef with an empty section name, it would give the ref the name "unattached" in the HTTPRoute status. This behavior is not Gateway API spec compliant -- the API prescribe to use empty section name in that case. Now NKG will use empty section name. See the corresponding FIXME in the code https://github.com/nginxinc/nginx-kubernetes-gateway/blob/b594695bb42a79653eb8217a6dfd00c6d9594d5b/internal/state/statuses.go#L151 Note: the FIXME advice is wrong. - Previously, in case of multiple parentRefs in an HTTPRoute, when reporting their statuses in the HTTPRoute status, NKG could change the order of the refs in the status. This commit restores the order. See the corresponding FIXME in the code https://github.com/nginxinc/nginx-kubernetes-gateway/blob/b594695bb42a79653eb8217a6dfd00c6d9594d5b/internal/status/httproute.go#L25
nginxinc · Apr 13, 2023 · 918d650 · 918d650
1 parent 0f472ec
commit 918d650
Show file tree

Hide file tree

Showing 11 changed files with 462 additions and 435 deletions.
diff --git 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
@@ -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,42 +394,42 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) {
 		},
 		{
 			route: &Route{
-				Source:          hrWithOneBackend,
-				SectionNameRefs: sectionNameRefs,
-				Valid:           false,
+				Source:     hrWithOneBackend,
+				ParentRefs: sectionNameRefs,
+				Valid:      false,
 			},
 			expectedBackendGroups: nil,
 			expectedConditions:    nil,
 			name:                  "invalid route",
 		},
 		{
 			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,
 			name:                  "invalid matches",
 		},
 		{
 			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,
 			name:                  "invalid filters",
 		},
 		{
 			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
@@ -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{}