diff --git a/internal/nginx/config/generator_test.go b/internal/nginx/config/generator_test.go index 6f39b3366..50f916701 100644 --- a/internal/nginx/config/generator_test.go +++ b/internal/nginx/config/generator_test.go @@ -8,18 +8,17 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/config" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/dataplane" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/graph" ) // Note: this test only verifies that Generate() returns a byte array with upstream, server, and split_client blocks. // It does not test the correctness of those blocks. That functionality is covered by other tests in this package. func TestGenerate(t *testing.T) { - bg := graph.BackendGroup{ + bg := dataplane.BackendGroup{ Source: types.NamespacedName{Namespace: "test", Name: "hr"}, RuleIdx: 0, - Backends: []graph.BackendRef{ - {Name: "test", Valid: true, Weight: 1}, - {Name: "test2", Valid: true, Weight: 1}, + Backends: []dataplane.Backend{ + {UpstreamName: "test", Valid: true, Weight: 1}, + {UpstreamName: "test2", Valid: true, Weight: 1}, }, } @@ -49,7 +48,7 @@ func TestGenerate(t *testing.T) { Endpoints: nil, }, }, - BackendGroups: []graph.BackendGroup{bg}, + BackendGroups: []dataplane.BackendGroup{bg}, } generator := config.NewGeneratorImpl() cfg := string(generator.Generate(conf)) diff --git a/internal/nginx/config/servers_test.go b/internal/nginx/config/servers_test.go index bbd7c5e79..5a5a8ba71 100644 --- a/internal/nginx/config/servers_test.go +++ b/internal/nginx/config/servers_test.go @@ -15,7 +15,6 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/helpers" "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/config/http" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/dataplane" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/graph" ) func TestExecuteServers(t *testing.T) { @@ -318,54 +317,54 @@ func TestCreateServers(t *testing.T) { hrNsName := types.NamespacedName{Namespace: hr.Namespace, Name: hr.Name} - fooGroup := graph.BackendGroup{ + fooGroup := dataplane.BackendGroup{ Source: hrNsName, RuleIdx: 0, - Backends: []graph.BackendRef{ + Backends: []dataplane.Backend{ { - Name: "test_foo_80", - Valid: true, - Weight: 1, + UpstreamName: "test_foo_80", + Valid: true, + Weight: 1, }, }, } // barGroup has two backends, which should generate a proxy pass with a variable. - barGroup := graph.BackendGroup{ + barGroup := dataplane.BackendGroup{ Source: hrNsName, RuleIdx: 1, - Backends: []graph.BackendRef{ + Backends: []dataplane.Backend{ { - Name: "test_bar_80", - Valid: true, - Weight: 50, + UpstreamName: "test_bar_80", + Valid: true, + Weight: 50, }, { - Name: "test_bar2_80", - Valid: true, - Weight: 50, + UpstreamName: "test_bar2_80", + Valid: true, + Weight: 50, }, }, } // baz group has an invalid backend, which should generate a proxy pass to the invalid ref backend. - bazGroup := graph.BackendGroup{ + bazGroup := dataplane.BackendGroup{ Source: hrNsName, RuleIdx: 2, - Backends: []graph.BackendRef{ + Backends: []dataplane.Backend{ { - Name: "test_baz_80", - Valid: false, - Weight: 1, + UpstreamName: "test_baz_80", + Valid: false, + Weight: 1, }, }, } - filterGroup1 := graph.BackendGroup{Source: hrNsName, RuleIdx: 3} + filterGroup1 := dataplane.BackendGroup{Source: hrNsName, RuleIdx: 3} - filterGroup2 := graph.BackendGroup{Source: hrNsName, RuleIdx: 4} + filterGroup2 := dataplane.BackendGroup{Source: hrNsName, RuleIdx: 4} - invalidFilterGroup := graph.BackendGroup{Source: hrNsName, RuleIdx: 5} + invalidFilterGroup := dataplane.BackendGroup{Source: hrNsName, RuleIdx: 5} cafePathRules := []dataplane.PathRule{ { @@ -694,14 +693,14 @@ func TestCreateLocationsRootPath(t *testing.T) { hrNsName := types.NamespacedName{Namespace: "test", Name: "route1"} - fooGroup := graph.BackendGroup{ + fooGroup := dataplane.BackendGroup{ Source: hrNsName, RuleIdx: 0, - Backends: []graph.BackendRef{ + Backends: []dataplane.Backend{ { - Name: "test_foo_80", - Valid: true, - Weight: 1, + UpstreamName: "test_foo_80", + Valid: true, + Weight: 1, }, }, } diff --git a/internal/nginx/config/split_clients.go b/internal/nginx/config/split_clients.go index 28d39030d..a77e176f1 100644 --- a/internal/nginx/config/split_clients.go +++ b/internal/nginx/config/split_clients.go @@ -7,7 +7,6 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/config/http" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/dataplane" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/graph" ) var splitClientsTemplate = gotemplate.Must(gotemplate.New("split_clients").Parse(splitClientsTemplateText)) @@ -18,7 +17,7 @@ func executeSplitClients(conf dataplane.Configuration) []byte { return execute(splitClientsTemplate, splitClients) } -func createSplitClients(backendGroups []graph.BackendGroup) []http.SplitClient { +func createSplitClients(backendGroups []dataplane.BackendGroup) []http.SplitClient { numSplits := 0 for _, group := range backendGroups { if backendGroupNeedsSplit(group) { @@ -40,7 +39,7 @@ func createSplitClients(backendGroups []graph.BackendGroup) []http.SplitClient { } splitClients = append(splitClients, http.SplitClient{ - VariableName: convertStringToSafeVariableName(group.GroupName()), + VariableName: convertStringToSafeVariableName(group.Name()), Distributions: distributions, }) @@ -49,7 +48,7 @@ func createSplitClients(backendGroups []graph.BackendGroup) []http.SplitClient { return splitClients } -func createSplitClientDistributions(group graph.BackendGroup) []http.SplitClientDistribution { +func createSplitClientDistributions(group dataplane.BackendGroup) []http.SplitClientDistribution { if !backendGroupNeedsSplit(group) { return nil } @@ -101,9 +100,9 @@ func createSplitClientDistributions(group graph.BackendGroup) []http.SplitClient return distributions } -func getSplitClientValue(b graph.BackendRef) string { +func getSplitClientValue(b dataplane.Backend) string { if b.Valid { - return b.Name + return b.UpstreamName } return invalidBackendRef } @@ -118,7 +117,7 @@ func percentOf(weight, totalWeight int32) float64 { return math.Floor(p*100) / 100 } -func backendGroupNeedsSplit(group graph.BackendGroup) bool { +func backendGroupNeedsSplit(group dataplane.BackendGroup) bool { return len(group.Backends) > 1 } @@ -126,17 +125,17 @@ func backendGroupNeedsSplit(group graph.BackendGroup) bool { // If the group needs to be split, the name returned is the group name. // If the group doesn't need to be split, the name returned is the name of the backend if it is valid. // If the name cannot be determined, it returns the name of the invalid backend upstream. -func backendGroupName(group graph.BackendGroup) string { +func backendGroupName(group dataplane.BackendGroup) string { switch len(group.Backends) { case 0: return invalidBackendRef case 1: b := group.Backends[0] - if b.Weight <= 0 || !b.Valid { + if b.Weight == 0 || !b.Valid { return invalidBackendRef } - return b.Name + return b.UpstreamName default: - return group.GroupName() + return group.Name() } } diff --git a/internal/nginx/config/split_clients_test.go b/internal/nginx/config/split_clients_test.go index 91b82f807..ac190448e 100644 --- a/internal/nginx/config/split_clients_test.go +++ b/internal/nginx/config/split_clients_test.go @@ -9,45 +9,44 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/nginx/config/http" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/dataplane" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/graph" ) func TestExecuteSplitClients(t *testing.T) { - bg1 := graph.BackendGroup{ + bg1 := dataplane.BackendGroup{ Source: types.NamespacedName{Namespace: "test", Name: "hr"}, RuleIdx: 0, - Backends: []graph.BackendRef{ - {Name: "test1", Valid: true, Weight: 1}, - {Name: "test2", Valid: true, Weight: 1}, + Backends: []dataplane.Backend{ + {UpstreamName: "test1", Valid: true, Weight: 1}, + {UpstreamName: "test2", Valid: true, Weight: 1}, }, } - bg2 := graph.BackendGroup{ + bg2 := dataplane.BackendGroup{ Source: types.NamespacedName{Namespace: "test", Name: "no-split"}, RuleIdx: 1, - Backends: []graph.BackendRef{ - {Name: "no-split", Valid: true, Weight: 1}, + Backends: []dataplane.Backend{ + {UpstreamName: "no-split", Valid: true, Weight: 1}, }, } - bg3 := graph.BackendGroup{ + bg3 := dataplane.BackendGroup{ Source: types.NamespacedName{Namespace: "test", Name: "hr"}, RuleIdx: 1, - Backends: []graph.BackendRef{ - {Name: "test3", Valid: true, Weight: 1}, - {Name: "test4", Valid: true, Weight: 1}, + Backends: []dataplane.Backend{ + {UpstreamName: "test3", Valid: true, Weight: 1}, + {UpstreamName: "test4", Valid: true, Weight: 1}, }, } tests := []struct { msg string - backendGroups []graph.BackendGroup + backendGroups []dataplane.BackendGroup expStrings []string notExpStrings []string }{ { msg: "non-zero weights", - backendGroups: []graph.BackendGroup{ + backendGroups: []dataplane.BackendGroup{ bg1, bg2, bg3, @@ -64,13 +63,13 @@ func TestExecuteSplitClients(t *testing.T) { }, { msg: "zero weight", - backendGroups: []graph.BackendGroup{ + backendGroups: []dataplane.BackendGroup{ { Source: types.NamespacedName{Namespace: "test", Name: "zero-percent"}, RuleIdx: 0, - Backends: []graph.BackendRef{ - {Name: "non-zero", Valid: true, Weight: 1}, - {Name: "zero", Valid: true, Weight: 0}, + Backends: []dataplane.Backend{ + {UpstreamName: "non-zero", Valid: true, Weight: 1}, + {UpstreamName: "zero", Valid: true, Weight: 0}, }, }, }, @@ -83,12 +82,12 @@ func TestExecuteSplitClients(t *testing.T) { }, { msg: "no split clients", - backendGroups: []graph.BackendGroup{ + backendGroups: []dataplane.BackendGroup{ { Source: types.NamespacedName{Namespace: "test", Name: "single-backend-route"}, RuleIdx: 0, - Backends: []graph.BackendRef{ - {Name: "single-backend", Valid: true, Weight: 1}, + Backends: []dataplane.Backend{ + {UpstreamName: "single-backend", Valid: true, Weight: 1}, }, }, }, @@ -132,9 +131,9 @@ func TestCreateSplitClients(t *testing.T) { createBackendGroup := func( sourceNsName types.NamespacedName, ruleIdx int, - backends ...graph.BackendRef, - ) graph.BackendGroup { - return graph.BackendGroup{ + backends ...dataplane.Backend, + ) dataplane.BackendGroup { + return dataplane.BackendGroup{ Source: sourceNsName, RuleIdx: ruleIdx, Backends: backends, @@ -146,46 +145,46 @@ func TestCreateSplitClients(t *testing.T) { oneBackend := createBackendGroup( hrNoSplit, 0, - graph.BackendRef{Name: "one-backend", Valid: true, Weight: 1}, + dataplane.Backend{UpstreamName: "one-backend", Valid: true, Weight: 1}, ) invalidBackend := createBackendGroup( hrNoSplit, 0, - graph.BackendRef{Name: "invalid-backend", Valid: false, Weight: 1}, + dataplane.Backend{UpstreamName: "invalid-backend", Valid: false, Weight: 1}, ) // the following backends need splits oneSplit := createBackendGroup( hrOneSplit, 0, - graph.BackendRef{Name: "one-split-1", Valid: true, Weight: 50}, - graph.BackendRef{Name: "one-split-2", Valid: true, Weight: 50}, + dataplane.Backend{UpstreamName: "one-split-1", Valid: true, Weight: 50}, + dataplane.Backend{UpstreamName: "one-split-2", Valid: true, Weight: 50}, ) twoSplitGroup0 := createBackendGroup( hrTwoSplits, 0, - graph.BackendRef{Name: "two-split-1", Valid: true, Weight: 50}, - graph.BackendRef{Name: "two-split-2", Valid: true, Weight: 50}, + dataplane.Backend{UpstreamName: "two-split-1", Valid: true, Weight: 50}, + dataplane.Backend{UpstreamName: "two-split-2", Valid: true, Weight: 50}, ) twoSplitGroup1 := createBackendGroup( hrTwoSplits, 1, - graph.BackendRef{Name: "two-split-3", Valid: true, Weight: 50}, - graph.BackendRef{Name: "two-split-4", Valid: true, Weight: 50}, - graph.BackendRef{Name: "two-split-5", Valid: true, Weight: 50}, + dataplane.Backend{UpstreamName: "two-split-3", Valid: true, Weight: 50}, + dataplane.Backend{UpstreamName: "two-split-4", Valid: true, Weight: 50}, + dataplane.Backend{UpstreamName: "two-split-5", Valid: true, Weight: 50}, ) tests := []struct { msg string - backendGroups []graph.BackendGroup + backendGroups []dataplane.BackendGroup expSplitClients []http.SplitClient }{ { msg: "normal case", - backendGroups: []graph.BackendGroup{ + backendGroups: []dataplane.BackendGroup{ noBackends, oneBackend, invalidBackend, @@ -241,7 +240,7 @@ func TestCreateSplitClients(t *testing.T) { }, { msg: "no split clients are needed", - backendGroups: []graph.BackendGroup{ + backendGroups: []dataplane.BackendGroup{ noBackends, oneBackend, }, @@ -260,7 +259,7 @@ func TestCreateSplitClients(t *testing.T) { func TestCreateSplitClientDistributions(t *testing.T) { tests := []struct { msg string - backends []graph.BackendRef + backends []dataplane.Backend expDistributions []http.SplitClientDistribution }{ { @@ -270,27 +269,27 @@ func TestCreateSplitClientDistributions(t *testing.T) { }, { msg: "one backend", - backends: []graph.BackendRef{ + backends: []dataplane.Backend{ { - Name: "one", - Valid: true, - Weight: 1, + UpstreamName: "one", + Valid: true, + Weight: 1, }, }, expDistributions: nil, }, { msg: "total weight 0", - backends: []graph.BackendRef{ + backends: []dataplane.Backend{ { - Name: "one", - Valid: true, - Weight: 0, + UpstreamName: "one", + Valid: true, + Weight: 0, }, { - Name: "two", - Valid: true, - Weight: 0, + UpstreamName: "two", + Valid: true, + Weight: 0, }, }, expDistributions: []http.SplitClientDistribution{ @@ -302,16 +301,16 @@ func TestCreateSplitClientDistributions(t *testing.T) { }, { msg: "two backends; equal weights that sum to 100", - backends: []graph.BackendRef{ + backends: []dataplane.Backend{ { - Name: "one", - Valid: true, - Weight: 1, + UpstreamName: "one", + Valid: true, + Weight: 1, }, { - Name: "two", - Valid: true, - Weight: 1, + UpstreamName: "two", + Valid: true, + Weight: 1, }, }, expDistributions: []http.SplitClientDistribution{ @@ -327,21 +326,21 @@ func TestCreateSplitClientDistributions(t *testing.T) { }, { msg: "three backends; whole percentages that sum to 100", - backends: []graph.BackendRef{ + backends: []dataplane.Backend{ { - Name: "one", - Valid: true, - Weight: 20, + UpstreamName: "one", + Valid: true, + Weight: 20, }, { - Name: "two", - Valid: true, - Weight: 30, + UpstreamName: "two", + Valid: true, + Weight: 30, }, { - Name: "three", - Valid: true, - Weight: 50, + UpstreamName: "three", + Valid: true, + Weight: 50, }, }, expDistributions: []http.SplitClientDistribution{ @@ -361,21 +360,21 @@ func TestCreateSplitClientDistributions(t *testing.T) { }, { msg: "three backends; whole percentages that sum to less than 100", - backends: []graph.BackendRef{ + backends: []dataplane.Backend{ { - Name: "one", - Valid: true, - Weight: 3, + UpstreamName: "one", + Valid: true, + Weight: 3, }, { - Name: "two", - Valid: true, - Weight: 3, + UpstreamName: "two", + Valid: true, + Weight: 3, }, { - Name: "three", - Valid: true, - Weight: 3, + UpstreamName: "three", + Valid: true, + Weight: 3, }, }, expDistributions: []http.SplitClientDistribution{ @@ -396,7 +395,7 @@ func TestCreateSplitClientDistributions(t *testing.T) { } for _, test := range tests { - result := createSplitClientDistributions(graph.BackendGroup{Backends: test.backends}) + result := createSplitClientDistributions(dataplane.BackendGroup{Backends: test.backends}) if diff := cmp.Diff(test.expDistributions, result); diff != "" { t.Errorf("createSplitClientDistributions() mismatch for %q (-want +got):\n%s", test.msg, diff) } @@ -407,21 +406,21 @@ func TestGetSplitClientValue(t *testing.T) { tests := []struct { msg string expValue string - backend graph.BackendRef + backend dataplane.Backend }{ { msg: "valid backend", - backend: graph.BackendRef{ - Name: "valid", - Valid: true, + backend: dataplane.Backend{ + UpstreamName: "valid", + Valid: true, }, expValue: "valid", }, { msg: "invalid backend", - backend: graph.BackendRef{ - Name: "invalid", - Valid: false, + backend: dataplane.Backend{ + UpstreamName: "invalid", + Valid: false, }, expValue: invalidBackendRef, }, @@ -515,12 +514,12 @@ func TestPercentOf(t *testing.T) { func TestBackendGroupNeedsSplit(t *testing.T) { tests := []struct { msg string - backends []graph.BackendRef + backends []dataplane.Backend expSplit bool }{ { msg: "empty backends", - backends: []graph.BackendRef{}, + backends: []dataplane.Backend{}, expSplit: false, }, { @@ -530,54 +529,54 @@ func TestBackendGroupNeedsSplit(t *testing.T) { }, { msg: "one valid backend", - backends: []graph.BackendRef{ + backends: []dataplane.Backend{ { - Name: "backend1", - Valid: true, - Weight: 1, + UpstreamName: "backend1", + Valid: true, + Weight: 1, }, }, expSplit: false, }, { msg: "one invalid backend", - backends: []graph.BackendRef{ + backends: []dataplane.Backend{ { - Name: "backend1", - Valid: false, - Weight: 1, + UpstreamName: "backend1", + Valid: false, + Weight: 1, }, }, expSplit: false, }, { msg: "multiple valid backends", - backends: []graph.BackendRef{ + backends: []dataplane.Backend{ { - Name: "backend1", - Valid: true, - Weight: 1, + UpstreamName: "backend1", + Valid: true, + Weight: 1, }, { - Name: "backend2", - Valid: true, - Weight: 1, + UpstreamName: "backend2", + Valid: true, + Weight: 1, }, }, expSplit: true, }, { msg: "multiple backends - one invalid", - backends: []graph.BackendRef{ + backends: []dataplane.Backend{ { - Name: "backend1", - Valid: true, - Weight: 1, + UpstreamName: "backend1", + Valid: true, + Weight: 1, }, { - Name: "backend2", - Valid: false, - Weight: 1, + UpstreamName: "backend2", + Valid: false, + Weight: 1, }, }, expSplit: true, @@ -585,7 +584,7 @@ func TestBackendGroupNeedsSplit(t *testing.T) { } for _, test := range tests { - bg := graph.BackendGroup{ + bg := dataplane.BackendGroup{ Source: types.NamespacedName{Namespace: "test", Name: "hr"}, Backends: test.backends, } @@ -600,11 +599,11 @@ func TestBackendGroupName(t *testing.T) { tests := []struct { msg string expName string - backends []graph.BackendRef + backends []dataplane.Backend }{ { msg: "empty backends", - backends: []graph.BackendRef{}, + backends: []dataplane.Backend{}, expName: invalidBackendRef, }, { @@ -614,65 +613,65 @@ func TestBackendGroupName(t *testing.T) { }, { msg: "one valid backend with non-zero weight", - backends: []graph.BackendRef{ + backends: []dataplane.Backend{ { - Name: "backend1", - Valid: true, - Weight: 1, + UpstreamName: "backend1", + Valid: true, + Weight: 1, }, }, expName: "backend1", }, { msg: "one valid backend with zero weight", - backends: []graph.BackendRef{ + backends: []dataplane.Backend{ { - Name: "backend1", - Valid: true, - Weight: 0, + UpstreamName: "backend1", + Valid: true, + Weight: 0, }, }, expName: invalidBackendRef, }, { msg: "one invalid backend", - backends: []graph.BackendRef{ + backends: []dataplane.Backend{ { - Name: "backend1", - Valid: false, - Weight: 1, + UpstreamName: "backend1", + Valid: false, + Weight: 1, }, }, expName: invalidBackendRef, }, { msg: "multiple valid backends", - backends: []graph.BackendRef{ + backends: []dataplane.Backend{ { - Name: "backend1", - Valid: true, - Weight: 1, + UpstreamName: "backend1", + Valid: true, + Weight: 1, }, { - Name: "backend2", - Valid: true, - Weight: 1, + UpstreamName: "backend2", + Valid: true, + Weight: 1, }, }, expName: "test__hr_rule0", }, { msg: "multiple invalid backends", - backends: []graph.BackendRef{ + backends: []dataplane.Backend{ { - Name: "backend1", - Valid: false, - Weight: 1, + UpstreamName: "backend1", + Valid: false, + Weight: 1, }, { - Name: "backend2", - Valid: false, - Weight: 1, + UpstreamName: "backend2", + Valid: false, + Weight: 1, }, }, expName: "test__hr_rule0", @@ -680,7 +679,7 @@ func TestBackendGroupName(t *testing.T) { } for _, test := range tests { - bg := graph.BackendGroup{ + bg := dataplane.BackendGroup{ Source: types.NamespacedName{Namespace: "test", Name: "hr"}, RuleIdx: 0, Backends: test.backends, diff --git a/internal/state/change_processor_test.go b/internal/state/change_processor_test.go index c9ad15ec8..ab313190f 100644 --- a/internal/state/change_processor_test.go +++ b/internal/state/change_processor_test.go @@ -23,7 +23,6 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/state" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/dataplane" - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/graph" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/relationship" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/relationship/relationshipfakes" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/secrets/secretsfakes" @@ -53,17 +52,17 @@ func createRoute( CommonRouteSpec: v1beta1.CommonRouteSpec{ ParentRefs: []v1beta1.ParentReference{ { - Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer("test")), + Namespace: (*v1beta1.Namespace)(helpers.GetPointer("test")), Name: v1beta1.ObjectName(gateway), SectionName: (*v1beta1.SectionName)( - helpers.GetStringPointer("listener-80-1"), + helpers.GetPointer("listener-80-1"), ), }, { - Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer("test")), + Namespace: (*v1beta1.Namespace)(helpers.GetPointer("test")), Name: v1beta1.ObjectName(gateway), SectionName: (*v1beta1.SectionName)( - helpers.GetStringPointer("listener-443-1"), + helpers.GetPointer("listener-443-1"), ), }, }, @@ -77,7 +76,7 @@ func createRoute( { Path: &v1beta1.HTTPPathMatch{ Type: helpers.GetPointer(v1beta1.PathMatchPathPrefix), - Value: helpers.GetStringPointer("/"), + Value: helpers.GetPointer("/"), }, }, }, @@ -118,12 +117,12 @@ func createGatewayWithTLSListener(name string) *v1beta1.Gateway { Port: 443, Protocol: v1beta1.HTTPSProtocolType, TLS: &v1beta1.GatewayTLSConfig{ - Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate), + Mode: helpers.GetPointer(v1beta1.TLSModeTerminate), CertificateRefs: []v1beta1.SecretObjectReference{ { - Kind: (*v1beta1.Kind)(helpers.GetStringPointer("Secret")), + Kind: (*v1beta1.Kind)(helpers.GetPointer("Secret")), Name: "secret", - Namespace: (*v1beta1.Namespace)(helpers.GetStringPointer("test")), + Namespace: (*v1beta1.Namespace)(helpers.GetPointer("test")), }, }, }, @@ -246,7 +245,7 @@ var _ = Describe("ChangeProcessor", func() { var ( gcUpdated *v1beta1.GatewayClass hr1, hr1Updated, hr2 *v1beta1.HTTPRoute - hr1Group, hr2Group graph.BackendGroup + hr1Group, hr2Group dataplane.BackendGroup gw1, gw1Updated, gw2 *v1beta1.Gateway ) BeforeAll(func() { @@ -255,7 +254,7 @@ var _ = Describe("ChangeProcessor", func() { hr1 = createRoute("hr-1", "gateway-1", "foo.example.com") - hr1Group = graph.BackendGroup{ + hr1Group = dataplane.BackendGroup{ Source: types.NamespacedName{Namespace: hr1.Namespace, Name: hr1.Name}, RuleIdx: 0, } @@ -265,7 +264,7 @@ var _ = Describe("ChangeProcessor", func() { hr2 = createRoute("hr-2", "gateway-2", "bar.example.com") - hr2Group = graph.BackendGroup{ + hr2Group = dataplane.BackendGroup{ Source: types.NamespacedName{Namespace: hr2.Namespace, Name: hr2.Name}, RuleIdx: 0, } @@ -418,9 +417,7 @@ var _ = Describe("ChangeProcessor", func() { SSL: &dataplane.SSL{CertificatePath: certificatePath}, }, }, - BackendGroups: []graph.BackendGroup{ - hr1Group, - }, + BackendGroups: []dataplane.BackendGroup{hr1Group}, } expectedStatuses := state.Statuses{ @@ -534,9 +531,7 @@ var _ = Describe("ChangeProcessor", func() { SSL: &dataplane.SSL{CertificatePath: certificatePath}, }, }, - BackendGroups: []graph.BackendGroup{ - hr1Group, - }, + BackendGroups: []dataplane.BackendGroup{hr1Group}, } expectedStatuses := state.Statuses{ GatewayClassStatus: &state.GatewayClassStatus{ @@ -650,9 +645,7 @@ var _ = Describe("ChangeProcessor", func() { SSL: &dataplane.SSL{CertificatePath: certificatePath}, }, }, - BackendGroups: []graph.BackendGroup{ - hr1Group, - }, + BackendGroups: []dataplane.BackendGroup{hr1Group}, } expectedStatuses := state.Statuses{ GatewayClassStatus: &state.GatewayClassStatus{ @@ -765,9 +758,7 @@ var _ = Describe("ChangeProcessor", func() { SSL: &dataplane.SSL{CertificatePath: certificatePath}, }, }, - BackendGroups: []graph.BackendGroup{ - hr1Group, - }, + BackendGroups: []dataplane.BackendGroup{hr1Group}, } expectedStatuses := state.Statuses{ GatewayClassStatus: &state.GatewayClassStatus{ @@ -879,9 +870,7 @@ var _ = Describe("ChangeProcessor", func() { SSL: &dataplane.SSL{CertificatePath: certificatePath}, }, }, - BackendGroups: []graph.BackendGroup{ - hr1Group, - }, + BackendGroups: []dataplane.BackendGroup{hr1Group}, } expectedStatuses := state.Statuses{ GatewayClassStatus: &state.GatewayClassStatus{ @@ -986,9 +975,7 @@ var _ = Describe("ChangeProcessor", func() { SSL: &dataplane.SSL{CertificatePath: certificatePath}, }, }, - BackendGroups: []graph.BackendGroup{ - hr1Group, - }, + BackendGroups: []dataplane.BackendGroup{hr1Group}, } expectedStatuses := state.Statuses{ GatewayClassStatus: &state.GatewayClassStatus{ @@ -1117,9 +1104,7 @@ var _ = Describe("ChangeProcessor", func() { SSL: &dataplane.SSL{CertificatePath: certificatePath}, }, }, - BackendGroups: []graph.BackendGroup{ - hr2Group, - }, + BackendGroups: []dataplane.BackendGroup{hr2Group}, } expectedStatuses := state.Statuses{ GatewayClassStatus: &state.GatewayClassStatus{ @@ -2019,7 +2004,7 @@ var _ = Describe("ChangeProcessor", func() { Namespace: (*v1beta1.Namespace)(&gw.Namespace), Name: v1beta1.ObjectName(gw.Name), SectionName: (*v1beta1.SectionName)( - helpers.GetStringPointer("listener-80-1"), + helpers.GetPointer("listener-80-1"), ), }, }, @@ -2033,7 +2018,7 @@ var _ = Describe("ChangeProcessor", func() { { Path: &v1beta1.HTTPPathMatch{ Type: helpers.GetPointer(v1beta1.PathMatchPathPrefix), - Value: helpers.GetStringPointer("/"), + Value: helpers.GetPointer("/"), }, }, }, @@ -2105,7 +2090,7 @@ var _ = Describe("ChangeProcessor", func() { processor.CaptureUpsertChange(gw) processor.CaptureUpsertChange(hr) - bg := graph.BackendGroup{ + bg := dataplane.BackendGroup{ Source: types.NamespacedName{Namespace: hr.Namespace, Name: hr.Name}, RuleIdx: 0, } @@ -2134,7 +2119,7 @@ var _ = Describe("ChangeProcessor", func() { }, }, SSLServers: []dataplane.VirtualServer{}, - BackendGroups: []graph.BackendGroup{bg}, + BackendGroups: []dataplane.BackendGroup{bg}, } expectedStatuses := state.Statuses{ GatewayClassStatus: &state.GatewayClassStatus{ diff --git a/internal/state/dataplane/configuration.go b/internal/state/dataplane/configuration.go index 81f559da2..02b17b9a8 100644 --- a/internal/state/dataplane/configuration.go +++ b/internal/state/dataplane/configuration.go @@ -5,6 +5,7 @@ import ( "fmt" "sort" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/graph" @@ -30,8 +31,7 @@ type Configuration struct { // Upstreams holds all unique Upstreams. Upstreams []Upstream // BackendGroups holds all unique BackendGroups. - // FIXME(pleshakov): Ensure Configuration doesn't include types from the graph package. - BackendGroups []graph.BackendGroup + BackendGroups []BackendGroup } // VirtualServer is a virtual server. @@ -91,13 +91,44 @@ type MatchRule struct { // the entire resource. Source *v1beta1.HTTPRoute // BackendGroup is the group of Backends that the rule routes to. - BackendGroup graph.BackendGroup + BackendGroup BackendGroup // MatchIdx is the index of the rule in the Rule.Matches. MatchIdx int // RuleIdx is the index of the corresponding rule in the HTTPRoute. RuleIdx int } +// BackendGroup represents a group of Backends for a routing rule in an HTTPRoute. +type BackendGroup struct { + // Source is the NamespacedName of the HTTPRoute the group belongs to. + Source types.NamespacedName + // Backends is a list of Backends in the Group. + Backends []Backend + // RuleIdx is the index of the corresponding rule in the HTTPRoute. + RuleIdx int +} + +// Name returns the name of the backend group. +// This name must be unique across all HTTPRoutes and all rules within the same HTTPRoute. +// The RuleIdx is used to make the name unique across all rules within the same HTTPRoute. +// The RuleIdx may change for a given rule if an update is made to the HTTPRoute, but it will always match the index +// of the rule in the stored HTTPRoute. +func (bg *BackendGroup) Name() string { + return fmt.Sprintf("%s__%s_rule%d", bg.Source.Namespace, bg.Source.Name, bg.RuleIdx) +} + +// Backend represents a Backend for a routing rule. +type Backend struct { + // UpstreamName is the name of the upstream for this backend. + UpstreamName string + // Weight is the weight of the BackendRef. + // The possible values of weight are 0-1,000,000. + // If weight is 0, no traffic should be forwarded for this entry. + Weight int32 + // Valid indicates whether the Backend is valid. + Valid bool +} + // GetMatch returns the HTTPRouteMatch of the Route . func (r *MatchRule) GetMatch() v1beta1.HTTPRouteMatch { return r.Source.Spec.Rules[r.RuleIdx].Matches[r.MatchIdx] @@ -114,49 +145,41 @@ func BuildConfiguration(ctx context.Context, g *graph.Graph, resolver resolver.S return Configuration{} } - upstreamsMap := buildUpstreamsMap(ctx, g.Gateway.Listeners, resolver) + upstreams := buildUpstreams(ctx, g.Gateway.Listeners, resolver) httpServers, sslServers := buildServers(g.Gateway.Listeners) - backendGroups := buildBackendGroups(g.Gateway.Listeners) + backendGroups := buildBackendGroups(append(httpServers, sslServers...)) config := Configuration{ HTTPServers: httpServers, SSLServers: sslServers, - Upstreams: upstreamsMapToSlice(upstreamsMap), + Upstreams: upstreams, BackendGroups: backendGroups, } return config } -func upstreamsMapToSlice(upstreamsMap map[string]Upstream) []Upstream { - if len(upstreamsMap) == 0 { - return nil - } - - upstreams := make([]Upstream, 0, len(upstreamsMap)) - for _, upstream := range upstreamsMap { - upstreams = append(upstreams, upstream) +func buildBackendGroups(servers []VirtualServer) []BackendGroup { + type key struct { + nsname types.NamespacedName + ruleIdx int } - return upstreams -} - -func buildBackendGroups(listeners map[string]*graph.Listener) []graph.BackendGroup { // There can be duplicate backend groups if a route is attached to multiple listeners. // We use a map to deduplicate them. - uniqueGroups := make(map[string]graph.BackendGroup) + uniqueGroups := make(map[key]BackendGroup) - for _, l := range listeners { - - if !l.Valid { - continue - } + for _, s := range servers { + for _, pr := range s.PathRules { + for _, mr := range pr.MatchRules { + group := mr.BackendGroup - for _, r := range l.Routes { - for _, group := range r.GetAllBackendGroups() { - if _, ok := uniqueGroups[group.GroupName()]; !ok { - uniqueGroups[group.GroupName()] = group + key := key{ + nsname: group.Source, + ruleIdx: group.RuleIdx, } + + uniqueGroups[key] = group } } } @@ -166,7 +189,7 @@ func buildBackendGroups(listeners map[string]*graph.Listener) []graph.BackendGro return nil } - groups := make([]graph.BackendGroup, 0, numGroups) + groups := make([]BackendGroup, 0, numGroups) for _, group := range uniqueGroups { groups = append(groups, group) } @@ -174,6 +197,28 @@ func buildBackendGroups(listeners map[string]*graph.Listener) []graph.BackendGro return groups } +func newBackendGroup(refs []graph.BackendRef, sourceNsName types.NamespacedName, ruleIdx int) BackendGroup { + var backends []Backend + + if len(refs) > 0 { + backends = make([]Backend, 0, len(refs)) + } + + for _, ref := range refs { + backends = append(backends, Backend{ + UpstreamName: ref.ServicePortReference(), + Weight: ref.Weight, + Valid: ref.Valid, + }) + } + + return BackendGroup{ + Backends: backends, + Source: sourceNsName, + RuleIdx: ruleIdx, + } +} + func buildServers(listeners map[string]*graph.Listener) (http, ssl []VirtualServer) { rulesForProtocol := map[v1beta1.ProtocolType]*hostPathRules{ v1beta1.HTTPProtocolType: newHostPathRules(), @@ -220,7 +265,7 @@ func (hpr *hostPathRules) upsertListener(l *graph.Listener) { hpr.httpsListeners = append(hpr.httpsListeners, l) } - for _, r := range l.Routes { + for routeNsName, r := range l.Routes { var hostnames []string for _, h := range r.Source.Spec.Hostnames { @@ -277,7 +322,7 @@ func (hpr *hostPathRules) upsertListener(l *graph.Listener) { MatchIdx: j, RuleIdx: i, Source: r.Source, - BackendGroup: r.Rules[i].BackendGroup, + BackendGroup: newBackendGroup(r.Rules[i].BackendRefs, routeNsName, i), Filters: filters, }) @@ -356,11 +401,11 @@ func (hpr *hostPathRules) buildServers() []VirtualServer { return servers } -func buildUpstreamsMap( +func buildUpstreams( ctx context.Context, listeners map[string]*graph.Listener, resolver resolver.ServiceResolver, -) map[string]Upstream { +) []Upstream { // There can be duplicate upstreams if multiple routes reference the same upstream. // We use a map to deduplicate them. uniqueUpstreams := make(map[string]Upstream) @@ -372,10 +417,15 @@ func buildUpstreamsMap( } for _, route := range l.Routes { - for _, group := range route.GetAllBackendGroups() { - for _, backend := range group.Backends { - if name := backend.Name; name != "" { - _, exist := uniqueUpstreams[name] + for _, rule := range route.Rules { + if !rule.ValidMatches || !rule.ValidFilters { + // don't generate upstreams for rules that have invalid matches or filters + continue + } + for _, br := range rule.BackendRefs { + if br.Valid { + upstreamName := br.ServicePortReference() + _, exist := uniqueUpstreams[upstreamName] if exist { continue @@ -383,13 +433,13 @@ func buildUpstreamsMap( var errMsg string - eps, err := resolver.Resolve(ctx, backend.Svc, backend.Port) + eps, err := resolver.Resolve(ctx, br.Svc, br.Port) if err != nil { errMsg = err.Error() } - uniqueUpstreams[name] = Upstream{ - Name: name, + uniqueUpstreams[upstreamName] = Upstream{ + Name: upstreamName, Endpoints: eps, ErrorMsg: errMsg, } @@ -399,7 +449,16 @@ func buildUpstreamsMap( } } - return uniqueUpstreams + if len(uniqueUpstreams) == 0 { + return nil + } + + upstreams := make([]Upstream, 0, len(uniqueUpstreams)) + + for _, up := range uniqueUpstreams { + upstreams = append(upstreams, up) + } + return upstreams } func getListenerHostname(h *v1beta1.Hostname) string { diff --git a/internal/state/dataplane/configuration_test.go b/internal/state/dataplane/configuration_test.go index 4b854c5c2..0e5029666 100644 --- a/internal/state/dataplane/configuration_test.go +++ b/internal/state/dataplane/configuration_test.go @@ -12,6 +12,7 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "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/helpers" @@ -88,24 +89,25 @@ func TestBuildConfiguration(t *testing.T) { fakeResolver := &resolverfakes.FakeServiceResolver{} fakeResolver.ResolveReturns(fooEndpoints, nil) - createBackendGroup := func(nsname types.NamespacedName, idx int, validRule bool) graph.BackendGroup { + validBackendRef := graph.BackendRef{ + Svc: fooSvc, + Port: 80, + Valid: true, + Weight: 1, + } + + expValidBackend := Backend{ + UpstreamName: fooUpstreamName, + Weight: 1, + Valid: true, + } + + createBackendRefs := func(validRule bool) []graph.BackendRef { if !validRule { - return graph.BackendGroup{} + return nil } - return graph.BackendGroup{ - Source: nsname, - RuleIdx: idx, - Backends: []graph.BackendRef{ - { - Name: fooUpstreamName, - Svc: fooSvc, - Port: 80, - Valid: true, - Weight: 1, - }, - }, - } + return []graph.BackendRef{validBackendRef} } createRules := func(hr *v1beta1.HTTPRoute, paths []pathAndType) []graph.Rule { @@ -119,7 +121,7 @@ func TestBuildConfiguration(t *testing.T) { rules[i] = graph.Rule{ ValidMatches: validMatches, ValidFilters: validFilters, - BackendGroup: createBackendGroup(types.NamespacedName{Namespace: "test", Name: hr.Name}, i, validRule), + BackendRefs: createBackendRefs(validRule), } } @@ -137,29 +139,69 @@ func TestBuildConfiguration(t *testing.T) { return r } + createExpBackendGroupsForRoute := func(route *graph.Route) []BackendGroup { + groups := make([]BackendGroup, 0) + + for idx, r := range route.Rules { + var backends []Backend + if r.ValidFilters && r.ValidMatches { + backends = []Backend{expValidBackend} + } + + groups = append(groups, BackendGroup{ + Backends: backends, + Source: client.ObjectKeyFromObject(route.Source), + RuleIdx: idx, + }) + } + + return groups + } + createTestResources := func(name, hostname, listenerName string, paths ...pathAndType) ( - *v1beta1.HTTPRoute, []graph.BackendGroup, *graph.Route, + *v1beta1.HTTPRoute, []BackendGroup, *graph.Route, ) { hr := createRoute(name, hostname, listenerName, paths...) route := createInternalRoute(hr, paths) - return hr, route.GetAllBackendGroups(), route + groups := createExpBackendGroupsForRoute(route) + return hr, groups, route } prefix := v1beta1.PathMatchPathPrefix - hr1, hr1Groups, routeHR1 := createTestResources( - "hr-1", "foo.example.com", "listener-80-1", pathAndType{path: "/", pathType: prefix}) - hr2, hr2Groups, routeHR2 := createTestResources( - "hr-2", "bar.example.com", "listener-80-1", pathAndType{path: "/", pathType: prefix}) - hr3, hr3Groups, routeHR3 := createTestResources( - "hr-3", "foo.example.com", "listener-80-1", - pathAndType{path: "/", pathType: prefix}, pathAndType{path: "/third", pathType: prefix}) - hr4, hr4Groups, routeHR4 := createTestResources( - "hr-4", "foo.example.com", "listener-80-1", - pathAndType{path: "/fourth", pathType: prefix}, pathAndType{path: "/", pathType: prefix}) - - hr5, hr5Groups, routeHR5 := createTestResources( - "hr-5", "foo.example.com", "listener-80-1", - pathAndType{path: "/", pathType: prefix}, pathAndType{path: invalidFiltersPath, pathType: prefix}) + + hr1, expHR1Groups, routeHR1 := createTestResources( + "hr-1", + "foo.example.com", + "listener-80-1", + pathAndType{path: "/", pathType: prefix}, + ) + hr2, expHR2Groups, routeHR2 := createTestResources( + "hr-2", + "bar.example.com", + "listener-80-1", + pathAndType{path: "/", pathType: prefix}, + ) + hr3, expHR3Groups, routeHR3 := createTestResources( + "hr-3", + "foo.example.com", + "listener-80-1", + pathAndType{path: "/", pathType: prefix}, + pathAndType{path: "/third", pathType: prefix}, + ) + hr4, expHR4Groups, routeHR4 := createTestResources( + "hr-4", + "foo.example.com", + "listener-80-1", + pathAndType{path: "/fourth", pathType: prefix}, + pathAndType{path: "/", pathType: prefix}, + ) + hr5, expHR5Groups, routeHR5 := createTestResources( + "hr-5", + "foo.example.com", + "listener-80-1", + pathAndType{path: "/", pathType: prefix}, pathAndType{path: invalidFiltersPath, pathType: prefix}, + ) + redirect := v1beta1.HTTPRouteFilter{ Type: v1beta1.HTTPRouteFilterRequestRedirect, RequestRedirect: &v1beta1.HTTPRequestRedirectFilter{ @@ -168,7 +210,7 @@ func TestBuildConfiguration(t *testing.T) { } addFilters(hr5, []v1beta1.HTTPRouteFilter{redirect}) - hr6, hr6Groups, routeHR6 := createTestResources( + hr6, expHR6Groups, routeHR6 := createTestResources( "hr-6", "foo.example.com", "listener-80-1", @@ -182,42 +224,42 @@ func TestBuildConfiguration(t *testing.T) { pathAndType{path: "/valid", pathType: prefix}, pathAndType{path: "/valid", pathType: v1beta1.PathMatchExact}, ) - httpsHR1, httpsHR1Groups, httpsRouteHR1 := createTestResources( + httpsHR1, expHTTPSHR1Groups, httpsRouteHR1 := createTestResources( "https-hr-1", "foo.example.com", "listener-443-1", pathAndType{path: "/", pathType: prefix}, ) - httpsHR2, httpsHR2Groups, httpsRouteHR2 := createTestResources( + httpsHR2, expHTTPSHR2Groups, httpsRouteHR2 := createTestResources( "https-hr-2", "bar.example.com", "listener-443-1", pathAndType{path: "/", pathType: prefix}, ) - httpsHR3, httpsHR3Groups, httpsRouteHR3 := createTestResources( + httpsHR3, expHTTPSHR3Groups, httpsRouteHR3 := createTestResources( "https-hr-3", "foo.example.com", "listener-443-1", pathAndType{path: "/", pathType: prefix}, pathAndType{path: "/third", pathType: prefix}, ) - httpsHR4, httpsHR4Groups, httpsRouteHR4 := createTestResources( + httpsHR4, expHTTPSHR4Groups, httpsRouteHR4 := createTestResources( "https-hr-4", "foo.example.com", "listener-443-1", pathAndType{path: "/fourth", pathType: prefix}, pathAndType{path: "/", pathType: prefix}, ) - httpsHR5, httpsHR5Groups, httpsRouteHR5 := createTestResources( + httpsHR5, expHTTPSHR5Groups, httpsRouteHR5 := createTestResources( "https-hr-5", "example.com", "listener-443-with-hostname", pathAndType{path: "/", pathType: prefix}, ) - httpsHR6, httpsHR6Groups, httpsRouteHR6 := createTestResources( + httpsHR6, expHTTPSHR6Groups, httpsRouteHR6 := createTestResources( "https-hr-6", "foo.example.com", "listener-443-1", @@ -443,7 +485,7 @@ func TestBuildConfiguration(t *testing.T) { { MatchIdx: 0, RuleIdx: 0, - BackendGroup: hr2Groups[0], + BackendGroup: expHR2Groups[0], Source: hr2, }, }, @@ -460,7 +502,7 @@ func TestBuildConfiguration(t *testing.T) { { MatchIdx: 0, RuleIdx: 0, - BackendGroup: hr1Groups[0], + BackendGroup: expHR1Groups[0], Source: hr1, }, }, @@ -470,7 +512,7 @@ func TestBuildConfiguration(t *testing.T) { }, SSLServers: []VirtualServer{}, Upstreams: []Upstream{fooUpstream}, - BackendGroups: []graph.BackendGroup{hr1Groups[0], hr2Groups[0]}, + BackendGroups: []BackendGroup{expHR1Groups[0], expHR2Groups[0]}, }, msg: "one http listener with two routes for different hostnames", }, @@ -531,7 +573,7 @@ func TestBuildConfiguration(t *testing.T) { { MatchIdx: 0, RuleIdx: 0, - BackendGroup: httpsHR2Groups[0], + BackendGroup: expHTTPSHR2Groups[0], Source: httpsHR2, }, }, @@ -551,7 +593,7 @@ func TestBuildConfiguration(t *testing.T) { { MatchIdx: 0, RuleIdx: 0, - BackendGroup: httpsHR5Groups[0], + BackendGroup: expHTTPSHR5Groups[0], Source: httpsHR5, }, }, @@ -571,7 +613,7 @@ func TestBuildConfiguration(t *testing.T) { { MatchIdx: 0, RuleIdx: 0, - BackendGroup: httpsHR1Groups[0], + BackendGroup: expHTTPSHR1Groups[0], Source: httpsHR1, }, }, @@ -587,7 +629,7 @@ func TestBuildConfiguration(t *testing.T) { }, }, Upstreams: []Upstream{fooUpstream}, - BackendGroups: []graph.BackendGroup{httpsHR1Groups[0], httpsHR2Groups[0], httpsHR5Groups[0]}, + BackendGroups: []BackendGroup{expHTTPSHR1Groups[0], expHTTPSHR2Groups[0], expHTTPSHR5Groups[0]}, }, msg: "two https listeners each with routes for different hostnames", }, @@ -647,13 +689,13 @@ func TestBuildConfiguration(t *testing.T) { { MatchIdx: 0, RuleIdx: 0, - BackendGroup: hr3Groups[0], + BackendGroup: expHR3Groups[0], Source: hr3, }, { MatchIdx: 0, RuleIdx: 1, - BackendGroup: hr4Groups[1], + BackendGroup: expHR4Groups[1], Source: hr4, }, }, @@ -665,7 +707,7 @@ func TestBuildConfiguration(t *testing.T) { { MatchIdx: 0, RuleIdx: 0, - BackendGroup: hr4Groups[0], + BackendGroup: expHR4Groups[0], Source: hr4, }, }, @@ -677,7 +719,7 @@ func TestBuildConfiguration(t *testing.T) { { MatchIdx: 0, RuleIdx: 1, - BackendGroup: hr3Groups[1], + BackendGroup: expHR3Groups[1], Source: hr3, }, }, @@ -702,13 +744,13 @@ func TestBuildConfiguration(t *testing.T) { { MatchIdx: 0, RuleIdx: 0, - BackendGroup: httpsHR3Groups[0], + BackendGroup: expHTTPSHR3Groups[0], Source: httpsHR3, }, { MatchIdx: 0, RuleIdx: 1, - BackendGroup: httpsHR4Groups[1], + BackendGroup: expHTTPSHR4Groups[1], Source: httpsHR4, }, }, @@ -720,7 +762,7 @@ func TestBuildConfiguration(t *testing.T) { { MatchIdx: 0, RuleIdx: 0, - BackendGroup: httpsHR4Groups[0], + BackendGroup: expHTTPSHR4Groups[0], Source: httpsHR4, }, }, @@ -732,7 +774,7 @@ func TestBuildConfiguration(t *testing.T) { { MatchIdx: 0, RuleIdx: 1, - BackendGroup: httpsHR3Groups[1], + BackendGroup: expHTTPSHR3Groups[1], Source: httpsHR3, }, }, @@ -745,15 +787,15 @@ func TestBuildConfiguration(t *testing.T) { }, }, Upstreams: []Upstream{fooUpstream}, - BackendGroups: []graph.BackendGroup{ - hr3Groups[0], - hr3Groups[1], - hr4Groups[0], - hr4Groups[1], - httpsHR3Groups[0], - httpsHR3Groups[1], - httpsHR4Groups[0], - httpsHR4Groups[1], + BackendGroups: []BackendGroup{ + expHR3Groups[0], + expHR3Groups[1], + expHR4Groups[0], + expHR4Groups[1], + expHTTPSHR3Groups[0], + expHTTPSHR3Groups[1], + expHTTPSHR4Groups[0], + expHTTPSHR4Groups[1], }, }, msg: "one http and one https listener with two routes with the same hostname with and without collisions", @@ -864,7 +906,7 @@ func TestBuildConfiguration(t *testing.T) { MatchIdx: 0, RuleIdx: 0, Source: hr5, - BackendGroup: hr5Groups[0], + BackendGroup: expHR5Groups[0], Filters: Filters{ RequestRedirect: redirect.RequestRedirect, }, @@ -876,9 +918,10 @@ func TestBuildConfiguration(t *testing.T) { PathType: PathTypePrefix, MatchRules: []MatchRule{ { - MatchIdx: 0, - RuleIdx: 1, - Source: hr5, + MatchIdx: 0, + RuleIdx: 1, + Source: hr5, + BackendGroup: expHR5Groups[1], Filters: Filters{ InvalidFilter: &InvalidFilter{}, }, @@ -890,7 +933,7 @@ func TestBuildConfiguration(t *testing.T) { }, SSLServers: []VirtualServer{}, Upstreams: []Upstream{fooUpstream}, - BackendGroups: hr5Groups, + BackendGroups: []BackendGroup{expHR5Groups[0], expHR5Groups[1]}, }, msg: "one http listener with one route with filters", }, @@ -946,7 +989,7 @@ func TestBuildConfiguration(t *testing.T) { { MatchIdx: 0, RuleIdx: 0, - BackendGroup: hr6Groups[0], + BackendGroup: expHR6Groups[0], Source: hr6, }, }, @@ -971,7 +1014,7 @@ func TestBuildConfiguration(t *testing.T) { { MatchIdx: 0, RuleIdx: 0, - BackendGroup: httpsHR6Groups[0], + BackendGroup: expHTTPSHR6Groups[0], Source: httpsHR6, }, }, @@ -984,9 +1027,9 @@ func TestBuildConfiguration(t *testing.T) { }, }, Upstreams: []Upstream{fooUpstream}, - BackendGroups: []graph.BackendGroup{ - hr6Groups[0], - httpsHR6Groups[0], + BackendGroups: []BackendGroup{ + expHR6Groups[0], + expHTTPSHR6Groups[0], }, }, msg: "one http and one https listener with routes with valid and invalid rules", @@ -1053,7 +1096,7 @@ func TestBuildConfiguration(t *testing.T) { }, SSLServers: []VirtualServer{}, Upstreams: []Upstream{fooUpstream}, - BackendGroups: []graph.BackendGroup{hr7Groups[0], hr7Groups[1]}, + BackendGroups: []BackendGroup{hr7Groups[0], hr7Groups[1]}, }, msg: "duplicate paths with different types", }, @@ -1111,13 +1154,13 @@ func TestBuildConfiguration(t *testing.T) { { MatchIdx: 0, RuleIdx: 0, - BackendGroup: httpsHR5Groups[0], + BackendGroup: expHTTPSHR5Groups[0], Source: httpsHR5, }, { MatchIdx: 0, RuleIdx: 0, - BackendGroup: httpsHR5Groups[0], + BackendGroup: expHTTPSHR5Groups[0], Source: httpsHR5, }, }, @@ -1133,7 +1176,7 @@ func TestBuildConfiguration(t *testing.T) { }, }, Upstreams: []Upstream{fooUpstream}, - BackendGroups: []graph.BackendGroup{httpsHR5Groups[0]}, + BackendGroups: []BackendGroup{expHTTPSHR5Groups[0]}, }, msg: "two https listeners with different hostnames but same route; chooses listener with more specific hostname", }, @@ -1144,7 +1187,7 @@ func TestBuildConfiguration(t *testing.T) { result := BuildConfiguration(context.TODO(), test.graph, fakeResolver) sort.Slice(result.BackendGroups, func(i, j int) bool { - return result.BackendGroups[i].GroupName() < result.BackendGroups[j].GroupName() + return result.BackendGroups[i].Name() < result.BackendGroups[j].Name() }) sort.Slice(result.Upstreams, func(i, j int) bool { @@ -1357,14 +1400,14 @@ func TestGetListenerHostname(t *testing.T) { } } -func groupsToValidRules(groups ...graph.BackendGroup) []graph.Rule { - rules := make([]graph.Rule, 0, len(groups)) +func refsToValidRules(refs ...[]graph.BackendRef) []graph.Rule { + rules := make([]graph.Rule, 0, len(refs)) - for _, group := range groups { + for _, ref := range refs { rules = append(rules, graph.Rule{ ValidMatches: true, ValidFilters: true, - BackendGroup: group, + BackendRefs: ref, }) } @@ -1420,56 +1463,55 @@ func TestBuildUpstreams(t *testing.T) { }, } - createBackendGroup := func(serviceNames ...string) graph.BackendGroup { + createBackendRefs := func(serviceNames ...string) []graph.BackendRef { var backends []graph.BackendRef for _, name := range serviceNames { backends = append(backends, graph.BackendRef{ - Name: name, - Svc: &v1.Service{ObjectMeta: metav1.ObjectMeta{Namespace: "test", Name: name}}, + Svc: &v1.Service{ObjectMeta: metav1.ObjectMeta{Namespace: "test", Name: name}}, + Port: 80, + Valid: name != "", }) } - return graph.BackendGroup{ - Backends: backends, - } + return backends } - hr1Group0 := createBackendGroup("foo", "bar") + hr1Refs0 := createBackendRefs("foo", "bar") - hr1Group1 := createBackendGroup("baz", "", "") // empty service names should be ignored + hr1Refs1 := createBackendRefs("baz", "", "") // empty service names should be ignored - hr2Group0 := createBackendGroup("foo", "baz") // shouldn't duplicate foo and baz upstream + hr2Refs0 := createBackendRefs("foo", "baz") // shouldn't duplicate foo and baz upstream - hr2Group1 := createBackendGroup("nil-endpoints") + hr2Refs1 := createBackendRefs("nil-endpoints") - hr3Group0 := createBackendGroup("baz") // shouldn't duplicate baz upstream + hr3Refs0 := createBackendRefs("baz") // shouldn't duplicate baz upstream - hr4Group0 := createBackendGroup("empty-endpoints", "") + hr4Refs0 := createBackendRefs("empty-endpoints", "") - hr4Group1 := createBackendGroup("baz2") + hr4Refs1 := createBackendRefs("baz2") - invalidGroup := createBackendGroup("invalid") + invalidRefs := createBackendRefs("invalid") routes := map[types.NamespacedName]*graph.Route{ {Name: "hr1", Namespace: "test"}: { - Rules: groupsToValidRules(hr1Group0, hr1Group1), + Rules: refsToValidRules(hr1Refs0, hr1Refs1), }, {Name: "hr2", Namespace: "test"}: { - Rules: groupsToValidRules(hr2Group0, hr2Group1), + Rules: refsToValidRules(hr2Refs0, hr2Refs1), }, {Name: "hr3", Namespace: "test"}: { - Rules: groupsToValidRules(hr3Group0), + Rules: refsToValidRules(hr3Refs0), }, } routes2 := map[types.NamespacedName]*graph.Route{ {Name: "hr4", Namespace: "test"}: { - Rules: groupsToValidRules(hr4Group0, hr4Group1), + Rules: refsToValidRules(hr4Refs0, hr4Refs1), }, } invalidRoutes := map[types.NamespacedName]*graph.Route{ {Name: "invalid", Namespace: "test"}: { - Rules: groupsToValidRules(invalidGroup), + Rules: refsToValidRules(invalidRefs), }, } @@ -1491,34 +1533,35 @@ func TestBuildUpstreams(t *testing.T) { emptyEndpointsErrMsg := "empty endpoints error" nilEndpointsErrMsg := "nil endpoints error" - expUpstreams := map[string]Upstream{ - "bar": { - Name: "bar", + expUpstreams := []Upstream{ + { + Name: "test_bar_80", Endpoints: barEndpoints, }, - "baz": { - Name: "baz", - Endpoints: bazEndpoints, - }, - "baz2": { - Name: "baz2", + { + Name: "test_baz2_80", Endpoints: baz2Endpoints, }, - "empty-endpoints": { - Name: "empty-endpoints", + { + Name: "test_baz_80", + Endpoints: bazEndpoints, + }, + { + Name: "test_empty-endpoints_80", Endpoints: []resolver.Endpoint{}, ErrorMsg: emptyEndpointsErrMsg, }, - "foo": { - Name: "foo", + { + Name: "test_foo_80", Endpoints: fooEndpoints, }, - "nil-endpoints": { - Name: "nil-endpoints", + { + Name: "test_nil-endpoints_80", Endpoints: nil, ErrorMsg: nilEndpointsErrMsg, }, } + fakeResolver := &resolverfakes.FakeServiceResolver{} fakeResolver.ResolveCalls(func(ctx context.Context, svc *v1.Service, port int32) ([]resolver.Endpoint, error) { switch svc.Name { @@ -1541,140 +1584,82 @@ func TestBuildUpstreams(t *testing.T) { g := NewGomegaWithT(t) - upstreams := buildUpstreamsMap(context.TODO(), listeners, fakeResolver) - - g.Expect(helpers.Diff(upstreams, expUpstreams)).To(BeEmpty()) + upstreams := buildUpstreams(context.TODO(), listeners, fakeResolver) + g.Expect(upstreams).To(ConsistOf(expUpstreams)) } func TestBuildBackendGroups(t *testing.T) { - createBackendGroup := func(name string, ruleIdx int, backendNames ...string) graph.BackendGroup { - backends := make([]graph.BackendRef, len(backendNames)) + createBackendGroup := func(name string, ruleIdx int, backendNames ...string) BackendGroup { + backends := make([]Backend, len(backendNames)) for i, name := range backendNames { - backends[i] = graph.BackendRef{Name: name} + backends[i] = Backend{UpstreamName: name} } - return graph.BackendGroup{ + return BackendGroup{ Source: types.NamespacedName{Namespace: "test", Name: name}, RuleIdx: ruleIdx, Backends: backends, } } - hr1Rule0 := createBackendGroup("hr1", 0, "foo", "bar") + hr1Group0 := createBackendGroup("hr1", 0, "foo", "bar") - hr1Rule1 := createBackendGroup("hr1", 1, "foo") + hr1Group1 := createBackendGroup("hr1", 1, "foo") - hr2Rule0 := createBackendGroup("hr2", 0, "foo", "bar") + hr2Group0 := createBackendGroup("hr2", 0, "foo", "bar") - hr2Rule1 := createBackendGroup("hr2", 1, "foo") + hr2Group1 := createBackendGroup("hr2", 1, "foo") - hr3Rule0 := createBackendGroup("hr3", 0, "foo", "bar") + hr3Group0 := createBackendGroup("hr3", 0, "foo", "bar") - hr3Rule1 := createBackendGroup("hr3", 1, "foo") + hr3Group1 := createBackendGroup("hr3", 1, "foo") - hrInvalid := createBackendGroup("hr-invalid", 0, "invalid") + // groups with no backends should still be included + hrNoBackends := createBackendGroup("no-backends", 0) - invalidRoutes := map[types.NamespacedName]*graph.Route{ - {Name: "invalid", Namespace: "test"}: { - Rules: groupsToValidRules(hrInvalid), - }, - } + createServer := func(groups ...BackendGroup) VirtualServer { + matchRules := make([]MatchRule, 0, len(groups)) + for _, g := range groups { + matchRules = append(matchRules, MatchRule{BackendGroup: g}) + } - routes := map[types.NamespacedName]*graph.Route{ - {Name: "hr1", Namespace: "test"}: { - Rules: groupsToValidRules(hr1Rule0, hr1Rule1), - }, - {Name: "hr2", Namespace: "test"}: { - Rules: groupsToValidRules(hr2Rule0, hr2Rule1), - }, - } + server := VirtualServer{ + PathRules: []PathRule{ + { + MatchRules: matchRules, + }, + }, + } - routes2 := map[types.NamespacedName]*graph.Route{ - // this backend group is a dupe and should be ignored. - {Name: "hr1", Namespace: "test"}: { - Rules: groupsToValidRules(hr1Rule0, hr1Rule1), - }, - {Name: "hr3", Namespace: "test"}: { - Rules: groupsToValidRules(hr3Rule0, hr3Rule1), - }, + return server } - - listeners := map[string]*graph.Listener{ - "invalid-listener": { - Valid: false, - Routes: invalidRoutes, // routes on invalid listener should be ignored. - }, - "listener-1": { - Valid: true, - Routes: routes, - }, - "listener-2": { - Valid: true, - Routes: routes2, - }, + servers := []VirtualServer{ + createServer(hr1Group0, hr1Group1), + createServer(hr2Group0, hr2Group1), + createServer(hr3Group0, hr3Group1), + createServer(hr1Group0, hr1Group1), // next three are duplicates + createServer(hr2Group0, hr2Group1), + createServer(hr3Group0, hr3Group1), + createServer(hrNoBackends), } - expGroups := []graph.BackendGroup{ - hr1Rule0, - hr1Rule1, - hr2Rule0, - hr2Rule1, - hr3Rule0, - hr3Rule1, + expGroups := []BackendGroup{ + hr1Group0, + hr1Group1, + hr2Group0, + hr2Group1, + hr3Group0, + hr3Group1, + hrNoBackends, } g := NewGomegaWithT(t) - result := buildBackendGroups(listeners) + result := buildBackendGroups(servers) g.Expect(result).To(ConsistOf(expGroups)) } -func TestUpstreamsMapToSlice(t *testing.T) { - fooUpstream := Upstream{ - Name: "foo", - Endpoints: []resolver.Endpoint{ - {Address: "10.0.0.0", Port: 80}, - {Address: "10.0.0.0", Port: 81}, - }, - } - - barUpstream := Upstream{ - Name: "bar", - ErrorMsg: "error", - Endpoints: nil, - } - - bazUpstream := Upstream{ - Name: "baz", - Endpoints: []resolver.Endpoint{ - {Address: "11.0.0.0", Port: 80}, - }, - } - - upstreamMap := map[string]Upstream{ - "foo": fooUpstream, - "bar": barUpstream, - "baz": bazUpstream, - } - - expUpstreams := []Upstream{ - barUpstream, - bazUpstream, - fooUpstream, - } - - upstreams := upstreamsMapToSlice(upstreamMap) - - sort.Slice(upstreams, func(i, j int) bool { - return upstreams[i].Name < upstreams[j].Name - }) - - if diff := cmp.Diff(expUpstreams, upstreams); diff != "" { - t.Errorf("upstreamMapToSlice() mismatch (-want +got):\n%s", diff) - } -} - func TestConvertPathType(t *testing.T) { g := NewGomegaWithT(t) diff --git a/internal/state/graph/backend_group.go b/internal/state/graph/backend_group.go deleted file mode 100644 index 9c36cc2ff..000000000 --- a/internal/state/graph/backend_group.go +++ /dev/null @@ -1,33 +0,0 @@ -package graph - -import ( - "fmt" - - v1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/types" -) - -// BackendGroup represents a group of backends for a rule in an HTTPRoute. -type BackendGroup struct { - Source types.NamespacedName - Backends []BackendRef - RuleIdx int -} - -// BackendRef is an internal representation of a backendRef in an HTTPRoute. -type BackendRef struct { - Svc *v1.Service - Name string - Port int32 - Weight int32 - Valid bool -} - -// GroupName returns the name of the backend group. -// This name must be unique across all HTTPRoutes and all rules within the same HTTPRoute. -// The RuleIdx is used to make the name unique across all rules within the same HTTPRoute. -// The RuleIdx may change for a given rule if an update is made to the HTTPRoute, but it will always match the index -// of the rule in the stored HTTPRoute. -func (bg *BackendGroup) GroupName() string { - return fmt.Sprintf("%s__%s_rule%d", bg.Source.Namespace, bg.Source.Name, bg.RuleIdx) -} diff --git a/internal/state/graph/backend_group_test.go b/internal/state/graph/backend_group_test.go deleted file mode 100644 index 4d29f1f85..000000000 --- a/internal/state/graph/backend_group_test.go +++ /dev/null @@ -1,21 +0,0 @@ -package graph_test - -import ( - "testing" - - "k8s.io/apimachinery/pkg/types" - - "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/graph" -) - -func TestBackendGroup_GroupName(t *testing.T) { - bg := graph.BackendGroup{ - Source: types.NamespacedName{Namespace: "test", Name: "hr"}, - RuleIdx: 20, - } - expected := "test__hr_rule20" - result := bg.GroupName() - if result != expected { - t.Errorf("BackendGroup.GroupName() mismatch; expected %s, got %s", expected, result) - } -} diff --git a/internal/state/graph/backend_refs.go b/internal/state/graph/backend_refs.go index 6ba2c0337..b16c7e081 100644 --- a/internal/state/graph/backend_refs.go +++ b/internal/state/graph/backend_refs.go @@ -11,19 +11,36 @@ import ( "github.com/nginxinc/nginx-kubernetes-gateway/internal/state/conditions" ) -func addBackendGroupsToRoutes( +// BackendRef is an internal representation of a backendRef in an HTTPRoute. +type BackendRef struct { + // Svc is the service referenced by the backendRef. + Svc *v1.Service + // Port is the port of the backendRef. + Port int32 + // Weight is the weight of the backendRef. + Weight int32 + // Valid indicates whether the backendRef is valid. + Valid bool +} + +// ServicePortReference returns a string representation for the service and port that is referenced by the BackendRef. +func (b BackendRef) ServicePortReference() string { + return fmt.Sprintf("%s_%s_%d", b.Svc.Namespace, b.Svc.Name, b.Port) +} + +func addBackendRefsToRouteRules( routes map[types.NamespacedName]*Route, services map[types.NamespacedName]*v1.Service, ) { for _, r := range routes { - addBackendGroupsToRoute(r, services) + addBackendRefsToRules(r, services) } } -// addBackendGroupsToRoute iterates over the rules of a route and adds BackendGroups to the rules. +// addBackendRefsToRules iterates over the rules of a route and adds a list of BackendRef to each rule. // The route is modified in place. // If a reference in a rule is invalid, the function will add a condition to the rule. -func addBackendGroupsToRoute(route *Route, services map[types.NamespacedName]*v1.Service) { +func addBackendRefsToRules(route *Route, services map[types.NamespacedName]*v1.Service) { if !route.Valid { return } @@ -41,24 +58,24 @@ func addBackendGroupsToRoute(route *Route, services map[types.NamespacedName]*v1 continue } - group := &route.Rules[idx].BackendGroup - - group.Backends = make([]BackendRef, 0, len(rule.BackendRefs)) + backendRefs := make([]BackendRef, 0, len(rule.BackendRefs)) for refIdx, ref := range rule.BackendRefs { refPath := field.NewPath("spec").Child("rules").Index(idx).Child("backendRefs").Index(refIdx) - backend, cond := createBackend(ref, route.Source.Namespace, services, refPath) + ref, cond := createBackendRef(ref, route.Source.Namespace, services, refPath) - group.Backends = append(group.Backends, backend) + backendRefs = append(backendRefs, ref) if cond != nil { route.Conditions = append(route.Conditions, *cond) } } + + route.Rules[idx].BackendRefs = backendRefs } } -func createBackend( +func createBackendRef( ref v1beta1.HTTPBackendRef, sourceNamespace string, services map[types.NamespacedName]*v1.Service, @@ -77,38 +94,37 @@ func createBackend( } } - var backend BackendRef + var backendRef BackendRef valid, cond := validateHTTPBackendRef(ref, sourceNamespace, refPath) if !valid { - backend = BackendRef{ + backendRef = BackendRef{ Weight: weight, Valid: false, } - return backend, &cond + return backendRef, &cond } svc, port, err := getServiceAndPortFromRef(ref.BackendRef, sourceNamespace, services, refPath) if err != nil { - backend = BackendRef{ + backendRef = BackendRef{ Weight: weight, Valid: false, } cond := conditions.NewRouteBackendRefRefBackendNotFound(err.Error()) - return backend, &cond + return backendRef, &cond } - backend = BackendRef{ - Name: fmt.Sprintf("%s_%s_%d", svc.Namespace, svc.Name, port), + backendRef = BackendRef{ Svc: svc, Port: port, Valid: true, Weight: weight, } - return backend, nil + return backendRef, nil } func getServiceAndPortFromRef( diff --git a/internal/state/graph/backend_refs_test.go b/internal/state/graph/backend_refs_test.go index 92f0cd820..0a6663a40 100644 --- a/internal/state/graph/backend_refs_test.go +++ b/internal/state/graph/backend_refs_test.go @@ -248,7 +248,7 @@ func TestGetServiceAndPortFromRef(t *testing.T) { } } -func TestAddBackendGroupsToRouteTest(t *testing.T) { +func TestAddBackendRefsToRulesTest(t *testing.T) { createRoute := func(name string, kind v1beta1.Kind, refsPerBackend int, serviceNames ...string) *v1beta1.HTTPRoute { hr := &v1beta1.HTTPRoute{ ObjectMeta: metav1.ObjectMeta{ @@ -301,10 +301,6 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { for i := range rules { rules[i].ValidMatches = validMatches rules[i].ValidFilters = validFilters - rules[i].BackendGroup = BackendGroup{ - Source: client.ObjectKeyFromObject(hr), - RuleIdx: i, - } } return rules } @@ -332,10 +328,10 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { } tests := []struct { - name string - route *Route - expectedBackendGroups []BackendGroup - expectedConditions []conditions.Condition + name string + route *Route + expectedBackendRefs []BackendRef + expectedConditions []conditions.Condition }{ { route: &Route{ @@ -344,19 +340,12 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { Valid: true, Rules: createRules(hrWithOneBackend, allValid, allValid), }, - expectedBackendGroups: []BackendGroup{ + expectedBackendRefs: []BackendRef{ { - Source: client.ObjectKeyFromObject(hrWithOneBackend), - RuleIdx: 0, - Backends: []BackendRef{ - { - Name: "test_svc1_80", - Svc: svc1, - Port: 80, - Valid: true, - Weight: 1, - }, - }, + Svc: svc1, + Port: 80, + Valid: true, + Weight: 1, }, }, expectedConditions: nil, @@ -369,26 +358,18 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { Valid: true, Rules: createRules(hrWithTwoBackends, allValid, allValid), }, - expectedBackendGroups: []BackendGroup{ + expectedBackendRefs: []BackendRef{ { - Source: client.ObjectKeyFromObject(hrWithTwoBackends), - RuleIdx: 0, - Backends: []BackendRef{ - { - Name: "test_svc1_80", - Svc: svc1, - Port: 80, - Valid: true, - Weight: 1, - }, - { - Name: "test_svc1_81", - Svc: svc1, - Port: 81, - Valid: true, - Weight: 5, - }, - }, + Svc: svc1, + Port: 80, + Valid: true, + Weight: 1, + }, + { + Svc: svc1, + Port: 81, + Valid: true, + Weight: 5, }, }, expectedConditions: nil, @@ -400,9 +381,9 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { ParentRefs: sectionNameRefs, Valid: false, }, - expectedBackendGroups: nil, - expectedConditions: nil, - name: "invalid route", + expectedBackendRefs: nil, + expectedConditions: nil, + name: "invalid route", }, { route: &Route{ @@ -411,9 +392,9 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { Valid: true, Rules: createRules(hrWithOneBackend, allInvalid, allValid), }, - expectedBackendGroups: nil, - expectedConditions: nil, - name: "invalid matches", + expectedBackendRefs: nil, + expectedConditions: nil, + name: "invalid matches", }, { route: &Route{ @@ -422,9 +403,9 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { Valid: true, Rules: createRules(hrWithOneBackend, allValid, allInvalid), }, - expectedBackendGroups: nil, - expectedConditions: nil, - name: "invalid filters", + expectedBackendRefs: nil, + expectedConditions: nil, + name: "invalid filters", }, { route: &Route{ @@ -433,15 +414,9 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { Valid: true, Rules: createRules(hrWithInvalidRule, allValid, allValid), }, - expectedBackendGroups: []BackendGroup{ + expectedBackendRefs: []BackendRef{ { - Source: client.ObjectKeyFromObject(hrWithInvalidRule), - RuleIdx: 0, - Backends: []BackendRef{ - { - Weight: 1, - }, - }, + Weight: 1, }, }, expectedConditions: []conditions.Condition{ @@ -458,14 +433,9 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { Valid: true, Rules: createRules(hrWithZeroBackendRefs, allValid, allValid), }, - expectedBackendGroups: []BackendGroup{ - { - Source: client.ObjectKeyFromObject(hrWithZeroBackendRefs), - RuleIdx: 0, - }, - }, - expectedConditions: nil, - name: "zero backendRefs", + expectedBackendRefs: nil, + expectedConditions: nil, + name: "zero backendRefs", }, } @@ -473,9 +443,14 @@ func TestAddBackendGroupsToRouteTest(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewGomegaWithT(t) - addBackendGroupsToRoute(test.route, services) + addBackendRefsToRules(test.route, services) + + var actual []BackendRef + if test.route.Rules != nil { + actual = test.route.Rules[0].BackendRefs + } - g.Expect(helpers.Diff(test.expectedBackendGroups, test.route.GetAllBackendGroups())).To(BeEmpty()) + g.Expect(helpers.Diff(test.expectedBackendRefs, actual)).To(BeEmpty()) g.Expect(test.route.Conditions).To(Equal(test.expectedConditions)) }) } @@ -496,7 +471,6 @@ func TestCreateBackend(t *testing.T) { }, expectedBackend: BackendRef{ Svc: svc1, - Name: "test_service1_80", Port: 80, Weight: 5, Valid: true, @@ -513,7 +487,6 @@ func TestCreateBackend(t *testing.T) { }, expectedBackend: BackendRef{ Svc: svc1, - Name: "test_service1_80", Port: 80, Weight: 1, Valid: true, @@ -530,7 +503,6 @@ func TestCreateBackend(t *testing.T) { }, expectedBackend: BackendRef{ Svc: nil, - Name: "", Port: 0, Weight: 0, Valid: false, @@ -551,7 +523,6 @@ func TestCreateBackend(t *testing.T) { }, expectedBackend: BackendRef{ Svc: nil, - Name: "", Port: 0, Weight: 5, Valid: false, @@ -572,7 +543,6 @@ func TestCreateBackend(t *testing.T) { }, expectedBackend: BackendRef{ Svc: nil, - Name: "", Port: 0, Weight: 5, Valid: false, @@ -595,7 +565,7 @@ func TestCreateBackend(t *testing.T) { t.Run(test.name, func(t *testing.T) { g := NewGomegaWithT(t) - backend, cond := createBackend(test.ref, sourceNamespace, services, refPath) + backend, cond := createBackendRef(test.ref, sourceNamespace, services, refPath) g.Expect(helpers.Diff(test.expectedBackend, backend)).To(BeEmpty()) g.Expect(cond).To(Equal(test.expectedCondition)) diff --git a/internal/state/graph/graph.go b/internal/state/graph/graph.go index 300916b32..042c00228 100644 --- a/internal/state/graph/graph.go +++ b/internal/state/graph/graph.go @@ -53,7 +53,7 @@ func BuildGraph( routes := buildRoutesForGateways(validators.HTTPFieldsValidator, state.HTTPRoutes, processedGws.GetAllNsNames()) bindRoutesToListeners(routes, gw) - addBackendGroupsToRoutes(routes, state.Services) + addBackendRefsToRouteRules(routes, state.Services) g := &Graph{ GatewayClass: gc, diff --git a/internal/state/graph/graph_test.go b/internal/state/graph/graph_test.go index c7a4a93cc..ed51f38c0 100644 --- a/internal/state/graph/graph_test.go +++ b/internal/state/graph/graph_test.go @@ -24,11 +24,11 @@ func TestBuildGraph(t *testing.T) { secretPath = "/etc/nginx/secrets/test_secret" ) - createValidRuleWithBackendGroup := func(group BackendGroup) Rule { + createValidRuleWithBackendRefs := func(refs []BackendRef) Rule { return Rule{ ValidMatches: true, ValidFilters: true, - BackendGroup: group, + BackendRefs: refs, } } @@ -85,31 +85,21 @@ func TestBuildGraph(t *testing.T) { fooSvc := &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: "test"}} - hr1Group := BackendGroup{ - Source: types.NamespacedName{Namespace: hr1.Namespace, Name: hr1.Name}, - RuleIdx: 0, - Backends: []BackendRef{ - { - Name: "test_foo_80", - Svc: fooSvc, - Port: 80, - Valid: true, - Weight: 1, - }, + hr1Refs := []BackendRef{ + { + Svc: fooSvc, + Port: 80, + Valid: true, + Weight: 1, }, } - hr3Group := BackendGroup{ - Source: types.NamespacedName{Namespace: hr3.Namespace, Name: hr3.Name}, - RuleIdx: 0, - Backends: []BackendRef{ - { - Name: "test_foo_80", - Svc: fooSvc, - Port: 80, - Valid: true, - Weight: 1, - }, + hr3Refs := []BackendRef{ + { + Svc: fooSvc, + Port: 80, + Valid: true, + Weight: 1, }, } @@ -187,7 +177,7 @@ func TestBuildGraph(t *testing.T) { }, }, }, - Rules: []Rule{createValidRuleWithBackendGroup(hr1Group)}, + Rules: []Rule{createValidRuleWithBackendRefs(hr1Refs)}, } routeHR3 := &Route{ @@ -202,7 +192,7 @@ func TestBuildGraph(t *testing.T) { }, }, }, - Rules: []Rule{createValidRuleWithBackendGroup(hr3Group)}, + Rules: []Rule{createValidRuleWithBackendRefs(hr3Refs)}, } secretMemoryMgr := &secretsfakes.FakeSecretDiskMemoryManager{} diff --git a/internal/state/graph/httproute.go b/internal/state/graph/httproute.go index 25156f200..08249882e 100644 --- a/internal/state/graph/httproute.go +++ b/internal/state/graph/httproute.go @@ -15,8 +15,8 @@ import ( // Rule represents a rule of an HTTPRoute. type Rule struct { - // BackendGroup is the BackendGroup of the rule. - BackendGroup BackendGroup + // BackendRefs is a list of BackendRefs for the rule. + BackendRefs []BackendRef // ValidMatches indicates whether the matches of the rule are valid. // If the matches are invalid, NGK should not generate any configuration for the rule. ValidMatches bool @@ -64,36 +64,6 @@ type Route struct { Valid bool } -// GetAllBackendGroups returns BackendGroups for all rules with valid matches and filters in the Route. -// -// FIXME(pleshakov) Improve the name once https://github.com/nginxinc/nginx-kubernetes-gateway/issues/468 is -// implemented. The current name doesn't reflect the filtering of rules inside the loops. -// See also the discussion below for more context: -// https://github.com/nginxinc/nginx-kubernetes-gateway/pull/455#discussion_r1136277589 -func (r *Route) GetAllBackendGroups() []BackendGroup { - count := 0 - - for _, rule := range r.Rules { - if rule.ValidMatches && rule.ValidFilters { - count++ - } - } - - if count == 0 { - return nil - } - - groups := make([]BackendGroup, 0, count) - - for _, rule := range r.Rules { - if rule.ValidMatches && rule.ValidFilters { - groups = append(groups, rule.BackendGroup) - } - } - - return groups -} - // buildRoutesForGateways builds routes from HTTPRoutes that reference any of the specified Gateways. func buildRoutesForGateways( validator validation.HTTPFieldsValidator, @@ -146,7 +116,9 @@ func buildSectionNameRefs( } if _, exist := uniqueSectionsPerGateway[k]; exist { - panicForBrokenWebhookAssumption(fmt.Errorf("duplicate section name %q for Gateway %s", sectionName, gw.String())) + panicForBrokenWebhookAssumption( + fmt.Errorf("duplicate section name %q for Gateway %s", sectionName, gw.String()), + ) } uniqueSectionsPerGateway[k] = struct{}{} @@ -246,10 +218,6 @@ func buildRoute( r.Rules[i] = Rule{ ValidMatches: len(matchesErrs) == 0, ValidFilters: len(filtersErrs) == 0, - BackendGroup: BackendGroup{ - Source: client.ObjectKeyFromObject(r.Source), - RuleIdx: i, - }, } } diff --git a/internal/state/graph/httproute_test.go b/internal/state/graph/httproute_test.go index c52fc2946..48300041e 100644 --- a/internal/state/graph/httproute_test.go +++ b/internal/state/graph/httproute_test.go @@ -76,69 +76,6 @@ func addFilterToPath(hr *v1beta1.HTTPRoute, path string, filter v1beta1.HTTPRout } } -func TestRouteGetAllBackendGroups(t *testing.T) { - group0 := BackendGroup{ - RuleIdx: 0, - } - group1 := BackendGroup{ - RuleIdx: 1, - } - group2 := BackendGroup{ - RuleIdx: 2, - } - group3 := BackendGroup{ - RuleIdx: 3, - } - - tests := []struct { - route *Route - name string - expected []BackendGroup - }{ - { - route: &Route{}, - expected: nil, - name: "no rules", - }, - { - route: &Route{ - Rules: []Rule{ - { - ValidMatches: true, - ValidFilters: true, - BackendGroup: group0, - }, - { - ValidMatches: false, - ValidFilters: true, - BackendGroup: group1, - }, - { - ValidMatches: true, - ValidFilters: false, - BackendGroup: group2, - }, - { - ValidMatches: false, - ValidFilters: false, - BackendGroup: group3, - }, - }, - }, - expected: []BackendGroup{group0}, - name: "mix of valid and invalid rules", - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - g := NewGomegaWithT(t) - result := test.route.GetAllBackendGroups() - g.Expect(result).To(Equal(test.expected)) - }) - } -} - func TestBuildRoutes(t *testing.T) { gwNsName := types.NamespacedName{Namespace: "test", Name: "gateway"} @@ -171,10 +108,6 @@ func TestBuildRoutes(t *testing.T) { { ValidMatches: true, ValidFilters: true, - BackendGroup: BackendGroup{ - Source: client.ObjectKeyFromObject(hr), - RuleIdx: 0, - }, }, }, }, @@ -459,24 +392,10 @@ func TestBuildRoute(t *testing.T) { { ValidMatches: true, ValidFilters: true, - BackendGroup: BackendGroup{ - Source: types.NamespacedName{ - Namespace: hr.Namespace, - Name: hr.Name, - }, - RuleIdx: 0, - }, }, { ValidMatches: true, ValidFilters: true, - BackendGroup: BackendGroup{ - Source: types.NamespacedName{ - Namespace: hr.Namespace, - Name: hr.Name, - }, - RuleIdx: 1, - }, }, }, }, @@ -527,10 +446,6 @@ func TestBuildRoute(t *testing.T) { { ValidMatches: false, ValidFilters: true, - BackendGroup: BackendGroup{ - Source: client.ObjectKeyFromObject(hr), - RuleIdx: 0, - }, }, }, }, @@ -557,10 +472,6 @@ func TestBuildRoute(t *testing.T) { { ValidMatches: true, ValidFilters: false, - BackendGroup: BackendGroup{ - Source: client.ObjectKeyFromObject(hr), - RuleIdx: 0, - }, }, }, }, @@ -590,26 +501,14 @@ func TestBuildRoute(t *testing.T) { { ValidMatches: false, ValidFilters: true, - BackendGroup: BackendGroup{ - Source: client.ObjectKeyFromObject(hrInvalidValidRules), - RuleIdx: 0, - }, }, { ValidMatches: true, ValidFilters: false, - BackendGroup: BackendGroup{ - Source: client.ObjectKeyFromObject(hrInvalidValidRules), - RuleIdx: 1, - }, }, { ValidMatches: true, ValidFilters: true, - BackendGroup: BackendGroup{ - Source: client.ObjectKeyFromObject(hrInvalidValidRules), - RuleIdx: 2, - }, }, }, }, @@ -1486,7 +1385,9 @@ func TestValidateFilter(t *testing.T) { filter: v1beta1.HTTPRouteFilter{ Type: v1beta1.HTTPRouteFilterRequestRedirect, RequestRedirect: &v1beta1.HTTPRequestRedirectFilter{ - Hostname: helpers.GetPointer[v1beta1.PreciseHostname]("example.com"), // any value is invalid by the validator + Hostname: helpers.GetPointer[v1beta1.PreciseHostname]( + "example.com", + ), // any value is invalid by the validator }, }, expectErrCount: 1, @@ -1543,8 +1444,12 @@ func TestValidateFilter(t *testing.T) { filter: v1beta1.HTTPRouteFilter{ Type: v1beta1.HTTPRouteFilterRequestRedirect, RequestRedirect: &v1beta1.HTTPRequestRedirectFilter{ - Hostname: helpers.GetPointer[v1beta1.PreciseHostname]("example.com"), // any value is invalid by the validator - Port: helpers.GetPointer[v1beta1.PortNumber](80), // any value is invalid by the validator + Hostname: helpers.GetPointer[v1beta1.PreciseHostname]( + "example.com", + ), // any value is invalid by the validator + Port: helpers.GetPointer[v1beta1.PortNumber]( + 80, + ), // any value is invalid by the validator }, }, expectErrCount: 2,