From 0777fac33f852769db33e50674b3e70028306613 Mon Sep 17 00:00:00 2001 From: Steven Landow Date: Fri, 15 Nov 2024 14:25:52 -0800 Subject: [PATCH 1/6] fix route reports for custom backends --- .../custom-be-route-report.yaml | 6 +++ projects/gateway2/reports/status.go | 14 +++++++ .../gateway_http_route_translator.go | 39 ++++++++++--------- 3 files changed, 41 insertions(+), 18 deletions(-) create mode 100644 changelog/v1.18.0-beta35/custom-be-route-report.yaml diff --git a/changelog/v1.18.0-beta35/custom-be-route-report.yaml b/changelog/v1.18.0-beta35/custom-be-route-report.yaml new file mode 100644 index 00000000000..1b2caea6b63 --- /dev/null +++ b/changelog/v1.18.0-beta35/custom-be-route-report.yaml @@ -0,0 +1,6 @@ +changelog: + - type: NON_USER_FACING + description: >- + Fix route report issues when we write multiple values + for a status, or when we're reporting for a custom backend + type. diff --git a/projects/gateway2/reports/status.go b/projects/gateway2/reports/status.go index b241413b69b..1fa4f1ae204 100644 --- a/projects/gateway2/reports/status.go +++ b/projects/gateway2/reports/status.go @@ -7,6 +7,7 @@ import ( "slices" "github.com/solo-io/go-utils/contextutils" + "istio.io/istio/pkg/ptr" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -126,6 +127,19 @@ func (r *ReportMap) BuildRouteStatus(ctx context.Context, obj client.Object, cNa for _, pCondition := range parentStatusReport.Conditions { pCondition.ObservedGeneration = routeReport.observedGeneration + if existing := meta.FindStatusCondition(finalConditions, pCondition.Type); existing != nil { + contextutils.LoggerFrom(ctx).Debugw( + "duplicate condiition", + "routeName", obj.GetName(), + "routeNamespace", obj.GetNamespace(), + "parentRef", string(ptr.OrDefault(parentRef.Namespace, gwv1.Namespace(obj.GetNamespace())))+"/"+string(parentRef.Name), + "conditionType", pCondition.Type, + "conditionStatus", existing.Status, + "conditionConflictingStatus", pCondition.Status, + ) + continue + } + // Copy old condition to preserve LastTransitionTime, if it exists if cond := meta.FindStatusCondition(currentParentRefConditions, pCondition.Type); cond != nil { finalConditions = append(finalConditions, *cond) diff --git a/projects/gateway2/translator/httproute/gateway_http_route_translator.go b/projects/gateway2/translator/httproute/gateway_http_route_translator.go index a2323dafd45..05599506306 100644 --- a/projects/gateway2/translator/httproute/gateway_http_route_translator.go +++ b/projects/gateway2/translator/httproute/gateway_http_route_translator.go @@ -353,13 +353,6 @@ func setRouteAction( clusterName := "blackhole_cluster" ns := "blackhole_ns" - obj, err := gwroute.GetBackendForRef(backendRef.BackendObjectReference) - ptrClusterName := query.ProcessBackendRef(obj, err, reporter, backendRef.BackendObjectReference) - if ptrClusterName != nil { - clusterName = *ptrClusterName - ns = obj.GetNamespace() - } - var weight *wrappers.UInt32Value if backendRef.Weight != nil { weight = &wrappers.UInt32Value{ @@ -372,20 +365,30 @@ func setRouteAction( } } + obj, err := gwroute.GetBackendForRef(backendRef.BackendObjectReference) fromPlugin := false - for _, bp := range pluginRegistry.GetBackendPlugins() { - if dest, ok := bp.ApplyBackendPlugin(obj, backendRef.BackendObjectReference); ok { - fromPlugin = true - weightedDestinations = append(weightedDestinations, &v1.WeightedDestination{ - Destination: dest, - Weight: weight, - }) - break + if err == nil { + for _, bp := range pluginRegistry.GetBackendPlugins() { + if dest, ok := bp.ApplyBackendPlugin(obj, backendRef.BackendObjectReference); ok { + fromPlugin = true + weightedDestinations = append(weightedDestinations, &v1.WeightedDestination{ + Destination: dest, + Weight: weight, + }) + break + } + } + // TODO break out a buildDestination func to avoid this awkwardness + if fromPlugin { + continue } } - // TODO break out a buildDestination func to avoid this awkwardness - if fromPlugin { - continue + + // only call ProcessBackendRef when the plugin didn't handle it + ptrClusterName := query.ProcessBackendRef(obj, err, reporter, backendRef.BackendObjectReference) + if ptrClusterName != nil { + clusterName = *ptrClusterName + ns = obj.GetNamespace() } var port uint32 From e32f344ea6e6f4ae44f0ca9990c91a12c4e317f0 Mon Sep 17 00:00:00 2001 From: Steven Landow Date: Sat, 16 Nov 2024 13:36:25 -0800 Subject: [PATCH 2/6] Update projects/gateway2/reports/status.go Co-authored-by: Omar Hammami <58956785+puertomontt@users.noreply.github.com> --- projects/gateway2/reports/status.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/projects/gateway2/reports/status.go b/projects/gateway2/reports/status.go index 1fa4f1ae204..419342affee 100644 --- a/projects/gateway2/reports/status.go +++ b/projects/gateway2/reports/status.go @@ -129,7 +129,7 @@ func (r *ReportMap) BuildRouteStatus(ctx context.Context, obj client.Object, cNa if existing := meta.FindStatusCondition(finalConditions, pCondition.Type); existing != nil { contextutils.LoggerFrom(ctx).Debugw( - "duplicate condiition", + "duplicate condition", "routeName", obj.GetName(), "routeNamespace", obj.GetNamespace(), "parentRef", string(ptr.OrDefault(parentRef.Namespace, gwv1.Namespace(obj.GetNamespace())))+"/"+string(parentRef.Name), From 0aaebda48af65d809910db49dd8c44df2991c36d Mon Sep 17 00:00:00 2001 From: Steven Landow Date: Tue, 19 Nov 2024 09:28:36 -0800 Subject: [PATCH 3/6] test coverage for backend plugins --- projects/gateway2/reports/status.go | 14 ------- .../translator/gateway_translator_test.go | 32 +++++++++++---- .../gateway_http_route_translator.go | 2 +- .../inputs/backend-plugin/gateway.yaml | 39 +++++++++++++++++++ .../outputs/backend-plugin-proxy.yaml | 31 +++++++++++++++ .../translator/testutils/test_queries.go | 4 +- .../translator/translator_case_test.go | 38 +++++++++++++++++- 7 files changed, 133 insertions(+), 27 deletions(-) create mode 100644 projects/gateway2/translator/testutils/inputs/backend-plugin/gateway.yaml create mode 100644 projects/gateway2/translator/testutils/outputs/backend-plugin-proxy.yaml diff --git a/projects/gateway2/reports/status.go b/projects/gateway2/reports/status.go index 419342affee..b241413b69b 100644 --- a/projects/gateway2/reports/status.go +++ b/projects/gateway2/reports/status.go @@ -7,7 +7,6 @@ import ( "slices" "github.com/solo-io/go-utils/contextutils" - "istio.io/istio/pkg/ptr" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -127,19 +126,6 @@ func (r *ReportMap) BuildRouteStatus(ctx context.Context, obj client.Object, cNa for _, pCondition := range parentStatusReport.Conditions { pCondition.ObservedGeneration = routeReport.observedGeneration - if existing := meta.FindStatusCondition(finalConditions, pCondition.Type); existing != nil { - contextutils.LoggerFrom(ctx).Debugw( - "duplicate condition", - "routeName", obj.GetName(), - "routeNamespace", obj.GetNamespace(), - "parentRef", string(ptr.OrDefault(parentRef.Namespace, gwv1.Namespace(obj.GetNamespace())))+"/"+string(parentRef.Name), - "conditionType", pCondition.Type, - "conditionStatus", existing.Status, - "conditionConflictingStatus", pCondition.Status, - ) - continue - } - // Copy old condition to preserve LastTransitionTime, if it exists if cond := meta.FindStatusCondition(currentParentRefConditions, pCondition.Type); cond != nil { finalConditions = append(finalConditions, *cond) diff --git a/projects/gateway2/translator/gateway_translator_test.go b/projects/gateway2/translator/gateway_translator_test.go index d678654fc89..3ac2aa9e517 100644 --- a/projects/gateway2/translator/gateway_translator_test.go +++ b/projects/gateway2/translator/gateway_translator_test.go @@ -50,7 +50,8 @@ var _ = DescribeTable("Basic GatewayTranslator Tests", gwNN: types.NamespacedName{ Namespace: "default", Name: "example-gateway", - }}), + }, + }), Entry( "https gateway with basic routing", translatorTestCase{ @@ -59,7 +60,8 @@ var _ = DescribeTable("Basic GatewayTranslator Tests", gwNN: types.NamespacedName{ Namespace: "default", Name: "example-gateway", - }}), + }, + }), Entry( "http gateway with multiple listeners on the same port", translatorTestCase{ @@ -68,7 +70,8 @@ var _ = DescribeTable("Basic GatewayTranslator Tests", gwNN: types.NamespacedName{ Namespace: "default", Name: "http", - }}), + }, + }), Entry( "https gateway with multiple listeners on the same port", translatorTestCase{ @@ -77,7 +80,8 @@ var _ = DescribeTable("Basic GatewayTranslator Tests", gwNN: types.NamespacedName{ Namespace: "default", Name: "http", - }}), + }, + }), Entry( "http gateway with multiple routing rules and HeaderModifier filter", translatorTestCase{ @@ -86,7 +90,8 @@ var _ = DescribeTable("Basic GatewayTranslator Tests", gwNN: types.NamespacedName{ Namespace: "default", Name: "gw", - }}), + }, + }), Entry( "http gateway with lambda destination", translatorTestCase{ @@ -95,7 +100,8 @@ var _ = DescribeTable("Basic GatewayTranslator Tests", gwNN: types.NamespacedName{ Namespace: "default", Name: "gw", - }}), + }, + }), Entry( "http gateway with azure destination", translatorTestCase{ @@ -104,7 +110,8 @@ var _ = DescribeTable("Basic GatewayTranslator Tests", gwNN: types.NamespacedName{ Namespace: "default", Name: "gw", - }}), + }, + }), Entry( "gateway with correctly sorted routes", translatorTestCase{ @@ -113,7 +120,8 @@ var _ = DescribeTable("Basic GatewayTranslator Tests", gwNN: types.NamespacedName{ Namespace: "infra", Name: "example-gateway", - }}), + }, + }), Entry( "route with missing backend reports correctly", translatorTestCase{ @@ -192,6 +200,14 @@ var _ = DescribeTable("Basic GatewayTranslator Tests", Name: "example-tcp-gateway", }, }), + Entry("Plugin Backend", translatorTestCase{ + inputFile: "backend-plugin/gateway.yaml", + outputFile: "backend-plugin-proxy.yaml", + gwNN: types.NamespacedName{ + Namespace: "default", + Name: "example-gateway", + }, + }), ) var _ = DescribeTable("Route Delegation translator", diff --git a/projects/gateway2/translator/httproute/gateway_http_route_translator.go b/projects/gateway2/translator/httproute/gateway_http_route_translator.go index 05599506306..c2648bf6488 100644 --- a/projects/gateway2/translator/httproute/gateway_http_route_translator.go +++ b/projects/gateway2/translator/httproute/gateway_http_route_translator.go @@ -448,7 +448,7 @@ func setRouteAction( }) default: - contextutils.LoggerFrom(ctx).Errorf("unsupported backend type for kind: %v and type: %v", *backendRef.BackendObjectReference.Kind, *backendRef.BackendObjectReference.Group) + contextutils.LoggerFrom(ctx).Errorf("unsupported backend type for kind: %v and type: %v", backendRef.BackendObjectReference.Kind, backendRef.BackendObjectReference.Group) } } diff --git a/projects/gateway2/translator/testutils/inputs/backend-plugin/gateway.yaml b/projects/gateway2/translator/testutils/inputs/backend-plugin/gateway.yaml new file mode 100644 index 00000000000..08806aeda37 --- /dev/null +++ b/projects/gateway2/translator/testutils/inputs/backend-plugin/gateway.yaml @@ -0,0 +1,39 @@ +#$ Used in: +#$ - site-src/guides/http-routing.md +apiVersion: gateway.networking.k8s.io/v1 +kind: Gateway +metadata: + name: example-gateway +spec: + gatewayClassName: example-gateway-class + listeners: + - name: http + protocol: HTTP + port: 80 +--- +apiVersion: gateway.networking.k8s.io/v1 +kind: HTTPRoute +metadata: + name: example-route +spec: + parentRefs: + - name: example-gateway + hostnames: + - "example.com" + rules: + - backendRefs: + - name: example-svc + kind: test-backend-plugin + port: 80 +--- +apiVersion: v1 +kind: Service +metadata: + name: example-svc +spec: + selector: + test: test + ports: + - protocol: TCP + port: 80 + targetPort: test diff --git a/projects/gateway2/translator/testutils/outputs/backend-plugin-proxy.yaml b/projects/gateway2/translator/testutils/outputs/backend-plugin-proxy.yaml new file mode 100644 index 00000000000..b367e7c1bcf --- /dev/null +++ b/projects/gateway2/translator/testutils/outputs/backend-plugin-proxy.yaml @@ -0,0 +1,31 @@ +--- +listeners: +- aggregateListener: + httpFilterChains: + - matcher: {} + virtualHostRefs: + - http~example_com + httpResources: + virtualHosts: + http~example_com: + domains: + - example.com + name: http~example_com + routes: + - matchers: + - prefix: / + name: httproute-example-route-default-0-0 + options: {} + routeAction: + single: + upstream: + name: test-backend-plugin-us + bindAddress: '::' + bindPort: 8080 + name: http +metadata: + labels: + created_by: gloo-kube-gateway-api + gateway_namespace: default + name: default-example-gateway + namespace: gloo-system diff --git a/projects/gateway2/translator/testutils/test_queries.go b/projects/gateway2/translator/testutils/test_queries.go index 1e495fc1989..de94e7d7cbe 100644 --- a/projects/gateway2/translator/testutils/test_queries.go +++ b/projects/gateway2/translator/testutils/test_queries.go @@ -21,8 +21,8 @@ func BuildIndexedFakeClient(objs []client.Object, funcs ...IndexIteratorFunc) cl return builder.WithObjects(objs...).Build() } -func BuildGatewayQueriesWithClient(fakeClient client.Client) query.GatewayQueries { - return query.NewData(fakeClient, schemes.TestingScheme()) +func BuildGatewayQueriesWithClient(fakeClient client.Client, opts ...query.Option) query.GatewayQueries { + return query.NewData(fakeClient, schemes.TestingScheme(), opts...) } func BuildGatewayQueries( diff --git a/projects/gateway2/translator/translator_case_test.go b/projects/gateway2/translator/translator_case_test.go index 61533f4d01e..db9839f6cc2 100644 --- a/projects/gateway2/translator/translator_case_test.go +++ b/projects/gateway2/translator/translator_case_test.go @@ -22,9 +22,11 @@ import ( "github.com/solo-io/gloo/pkg/utils/statusutils" sologatewayv1 "github.com/solo-io/gloo/projects/gateway/pkg/api/v1" solokubev1 "github.com/solo-io/gloo/projects/gateway/pkg/api/v1/kube/apis/gateway.solo.io/v1" + "github.com/solo-io/gloo/projects/gateway2/query" gwquery "github.com/solo-io/gloo/projects/gateway2/query" "github.com/solo-io/gloo/projects/gateway2/reports" . "github.com/solo-io/gloo/projects/gateway2/translator" + "github.com/solo-io/gloo/projects/gateway2/translator/plugins" httplisquery "github.com/solo-io/gloo/projects/gateway2/translator/plugins/httplisteneroptions/query" lisquery "github.com/solo-io/gloo/projects/gateway2/translator/plugins/listeneroptions/query" "github.com/solo-io/gloo/projects/gateway2/translator/plugins/registry" @@ -52,6 +54,36 @@ func CompareProxy(expectedFile string, actualProxy *v1.Proxy) (string, error) { return cmp.Diff(expectedProxy, actualProxy, protocmp.Transform(), cmpopts.EquateNaNs()), nil } +var ( + _ plugins.BackendPlugin = &testBackendPlugin{} + _ query.BackendRefResolver = &testBackendPlugin{} +) + +type testBackendPlugin struct{} + +// GetBackendForRef implements query.BackendRefResolver. +func (tp *testBackendPlugin) GetBackendForRef(ctx context.Context, obj gwquery.From, ref *gwv1.BackendObjectReference) (client.Object, error, bool) { + if ref.Kind == nil || *ref.Kind != "test-backend-plugin" { + return nil, nil, false + } + // doesn't matter as long as its not nil + return &gwv1.HTTPRoute{}, nil, true +} + +func (tp *testBackendPlugin) ApplyBackendPlugin( + resolvedBackend client.Object, + ref gwv1.BackendObjectReference, +) (*v1.Destination, bool) { + if ref.Kind == nil || *ref.Kind != "test-backend-plugin" { + return nil, false + } + return &v1.Destination{ + DestinationType: &v1.Destination_Upstream{ + Upstream: &core.ResourceRef{Name: "test-backend-plugin-us"}, + }, + }, true +} + func (tc TestCase) Run(ctx context.Context) (map[types.NamespacedName]ActualTestResult, error) { var ( gateways []*gwv1.Gateway @@ -94,7 +126,7 @@ func (tc TestCase) Run(ctx context.Context) (map[types.NamespacedName]ActualTest lisquery.IterateIndices, httplisquery.IterateIndices, ) - queries := testutils.BuildGatewayQueriesWithClient(fakeClient) + queries := testutils.BuildGatewayQueriesWithClient(fakeClient, query.WithBackendRefResolvers(&testBackendPlugin{})) resourceClientFactory := &factory.MemoryResourceClientFactory{ Cache: memory.NewInMemoryResourceCache(), @@ -109,7 +141,9 @@ func (tc TestCase) Run(ctx context.Context) (map[types.NamespacedName]ActualTest routeOptionCollection := krt.NewStaticCollection(routeOptions) vhOptionCollection := krt.NewStatic[*solokubev1.VirtualHostOption](nil, true).AsCollection() - pluginRegistry := registry.NewPluginRegistry(registry.BuildPlugins(queries, fakeClient, routeOptionCollection, vhOptionCollection, statusReporter)) + allPlugins := registry.BuildPlugins(queries, fakeClient, routeOptionCollection, vhOptionCollection, statusReporter) + allPlugins = append(allPlugins, &testBackendPlugin{}) + pluginRegistry := registry.NewPluginRegistry(allPlugins) results := make(map[types.NamespacedName]ActualTestResult) From 651ecffd00809ae246820b19228dd0feca384a9b Mon Sep 17 00:00:00 2001 From: Steven Landow Date: Tue, 19 Nov 2024 09:43:50 -0800 Subject: [PATCH 4/6] move changelog --- .../{v1.18.0-beta35 => v1.18.0-rc2}/custom-be-route-report.yaml | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog/{v1.18.0-beta35 => v1.18.0-rc2}/custom-be-route-report.yaml (100%) diff --git a/changelog/v1.18.0-beta35/custom-be-route-report.yaml b/changelog/v1.18.0-rc2/custom-be-route-report.yaml similarity index 100% rename from changelog/v1.18.0-beta35/custom-be-route-report.yaml rename to changelog/v1.18.0-rc2/custom-be-route-report.yaml From a66da6a60f4e0dfea2a55329e2fea81a28cd760c Mon Sep 17 00:00:00 2001 From: Steven Landow Date: Fri, 22 Nov 2024 09:25:49 -0800 Subject: [PATCH 5/6] address comments --- .../gateway_http_route_translator.go | 35 ++++++++++++------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/projects/gateway2/translator/httproute/gateway_http_route_translator.go b/projects/gateway2/translator/httproute/gateway_http_route_translator.go index c2648bf6488..cb38e2a4e68 100644 --- a/projects/gateway2/translator/httproute/gateway_http_route_translator.go +++ b/projects/gateway2/translator/httproute/gateway_http_route_translator.go @@ -16,6 +16,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" + "sigs.k8s.io/controller-runtime/pkg/client" gwv1 "sigs.k8s.io/gateway-api/apis/v1" "github.com/solo-io/gloo/projects/gateway2/query" @@ -366,20 +367,15 @@ func setRouteAction( } obj, err := gwroute.GetBackendForRef(backendRef.BackendObjectReference) - fromPlugin := false if err == nil { - for _, bp := range pluginRegistry.GetBackendPlugins() { - if dest, ok := bp.ApplyBackendPlugin(obj, backendRef.BackendObjectReference); ok { - fromPlugin = true - weightedDestinations = append(weightedDestinations, &v1.WeightedDestination{ - Destination: dest, - Weight: weight, - }) - break - } - } - // TODO break out a buildDestination func to avoid this awkwardness - if fromPlugin { + // Only apply backend plugin when the backend is resolved. + // If any backend plugin matches this ref, we don't need the standard + // reports or validation path. + if dest, ok := applyBackendPlugins(obj, backendRef.BackendObjectReference, pluginRegistry); ok { + weightedDestinations = append(weightedDestinations, &v1.WeightedDestination{ + Destination: dest, + Weight: weight, + }) continue } } @@ -523,3 +519,16 @@ func makeDestinationSpec(upstream *gloov1.Upstream, filters []gwv1.HTTPRouteFilt } return nil, nil } + +func applyBackendPlugins( + obj client.Object, + backendRef gwv1.BackendObjectReference, + plugins registry.PluginRegistry, +) (*v1.Destination, bool) { + for _, bp := range plugins.GetBackendPlugins() { + if dest, ok := bp.ApplyBackendPlugin(obj, backendRef); ok { + return dest, true + } + } + return nil, false +} From 56dc20b8069f60b1a45e09039d0a3e077c412539 Mon Sep 17 00:00:00 2001 From: Steven Landow Date: Mon, 25 Nov 2024 11:49:36 -0800 Subject: [PATCH 6/6] changelogs --- .../{v1.18.0-rc2 => v1.18.0-rc3}/custom-be-route-report.yaml | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog/{v1.18.0-rc2 => v1.18.0-rc3}/custom-be-route-report.yaml (100%) diff --git a/changelog/v1.18.0-rc2/custom-be-route-report.yaml b/changelog/v1.18.0-rc3/custom-be-route-report.yaml similarity index 100% rename from changelog/v1.18.0-rc2/custom-be-route-report.yaml rename to changelog/v1.18.0-rc3/custom-be-route-report.yaml