gateway2: merge multiple targetRef based Route & VirtualHost options (#…

…9614)

Implements merging of targetRef based RouteOptions and
VirtualHostOptions in a specific order of precedence from
oldest to newest created resource.

The merging uses shallow merging such that for an option
A that is higher priority than option B, merge(A,B) merges
the top-level options of B that have not already been set on A.
This allows options later in the precedence chain to augment
the existing options during a merge but not overwrite them.

Signed-off-by: Shashank Ram <shashank.ram@solo.io>

changelog/v1.18.0-beta1/options-merge.yaml

@@ -0,0 +1,16 @@
+changelog:
+  - type: NEW_FEATURE
+    issueLink: https://github.com/solo-io/solo-projects/issues/6313
+    resolvesIssue: false
+    description: |
+      gateway2: merge multiple targetRef based Route & VirtualHost options
+
+      Implements merging of targetRef based RouteOptions and
+      VirtualHostOptions in a specific order of precedence from
+      oldest to newest created resource.
+
+      The merging uses shallow merging such that for an option
+      A that is higher priority than option B, merge(A,B) merges
+      the top-level options of B that have not already been set on A.
+      This allows options later in the precedence chain to augment
+      the existing options during a merge but not overwrite them.

projects/gateway/pkg/translator/merge.go

@@ -7,34 +7,11 @@ import (
 	"github.com/golang/protobuf/ptypes/duration"
 	"github.com/golang/protobuf/ptypes/wrappers"
 	"github.com/imdario/mergo"
-	v1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1"
 	"github.com/solo-io/gloo/projects/gloo/pkg/api/v1/options/hcm"
 	"github.com/solo-io/gloo/projects/gloo/pkg/api/v1/ssl"
-	"github.com/solo-io/gloo/projects/gloo/pkg/utils"
 	"github.com/solo-io/solo-kit/pkg/api/v1/resources/core"
 )
 
-// Merges the fields of src into dst.
-// The fields in dst that have non-zero values will not be overwritten.
-func mergeVirtualHostOptions(dst, src *v1.VirtualHostOptions) *v1.VirtualHostOptions {
-	if src == nil {
-		return dst
-	}
-
-	if dst == nil {
-		return proto.Clone(src).(*v1.VirtualHostOptions)
-	}
-
-	dstValue, srcValue := reflect.ValueOf(dst).Elem(), reflect.ValueOf(src).Elem()
-
-	for i := range dstValue.NumField() {
-		dstField, srcField := dstValue.Field(i), srcValue.Field(i)
-		utils.ShallowMerge(dstField, srcField, false)
-	}
-
-	return dst
-}
-
 func mergeSslConfig(parent, child *ssl.SslConfig, preventChildOverrides bool) *ssl.SslConfig {
 	if child == nil {
 		// use parent exactly as-is

projects/gateway/pkg/translator/merge_test.go

@@ -3,6 +3,7 @@ package translator
 import (
 	"github.com/golang/protobuf/ptypes/duration"
 	"github.com/solo-io/gloo/projects/gloo/pkg/api/v1/options/headers"
+	"github.com/solo-io/gloo/projects/gloo/pkg/utils"
 
 	"github.com/solo-io/gloo/projects/gloo/pkg/api/v1/enterprise/options/ratelimit"
 	rltypes "github.com/solo-io/solo-apis/pkg/api/ratelimit.solo.io/v1alpha1"
@@ -59,7 +60,7 @@ var _ = Describe("Merge", func() {
 			},
 		}
 
-		actual := mergeVirtualHostOptions(dst, src)
+		actual, _ := utils.ShallowMergeVirtualHostOptions(dst, src)
 		Expect(actual).To(Equal(expected))
 	})
 })

projects/gateway/pkg/translator/virtual_service_translator.go

@@ -10,15 +10,17 @@ import (
 
 	"github.com/golang/protobuf/ptypes/wrappers"
 	errors "github.com/rotisserie/eris"
+
+	"github.com/solo-io/go-utils/hashutils"
+	"github.com/solo-io/solo-kit/pkg/api/v2/reporter"
+	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/selection"
+
 	"github.com/solo-io/gloo/pkg/utils/settingsutil"
 	v1 "github.com/solo-io/gloo/projects/gateway/pkg/api/v1"
 	gloov1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1"
 	"github.com/solo-io/gloo/projects/gloo/pkg/api/v1/core/matchers"
 	"github.com/solo-io/gloo/projects/gloo/pkg/utils"
-	"github.com/solo-io/go-utils/hashutils"
-	"github.com/solo-io/solo-kit/pkg/api/v2/reporter"
-	"k8s.io/apimachinery/pkg/labels"
-	"k8s.io/apimachinery/pkg/selection"
 )
 
 var (
@@ -106,7 +108,6 @@ func applyGlobalVirtualServiceSettings(ctx context.Context, virtualServices v1.V
 
 // Errors will be added to the report object.
 func validateVirtualServiceDomains(gateway *v1.Gateway, virtualServices v1.VirtualServiceList, reports reporter.ResourceReports) {
-
 	// Index the virtual services for this gateway by the domain
 	vsByDomain := map[string]v1.VirtualServiceList{}
 	for _, vs := range virtualServices {
@@ -213,8 +214,8 @@ func virtualServiceMatchesLabels(virtualService *v1.VirtualService, validLabels
 	labelSelector = labels.SelectorFromSet(validLabels)
 	return labelSelector.Matches(vsLabels) && virtualServiceNamespaceValidForGateway(virtualServiceNamespaces, virtualService)
 }
-func virtualServiceValidForSelectorExpressions(virtualService *v1.VirtualService, selector *v1.VirtualServiceSelectorExpressions, virtualServiceNamespaces []string) (bool, error) {
 
+func virtualServiceValidForSelectorExpressions(virtualService *v1.VirtualService, selector *v1.VirtualServiceSelectorExpressions, virtualServiceNamespaces []string) (bool, error) {
 	vsLabels := labels.Set(virtualService.GetMetadata().GetLabels())
 	// Check whether labels match (expression requirements)
 	if len(selector.GetExpressions()) > 0 {
@@ -237,6 +238,7 @@ func virtualServiceValidForSelectorExpressions(virtualService *v1.VirtualService
 	nsMatches := virtualServiceNamespaceValidForGateway(virtualServiceNamespaces, virtualService)
 	return nsMatches, nil
 }
+
 func virtualServiceNamespaceValidForGateway(virtualServiceNamespaces []string, virtualService *v1.VirtualService) bool {
 	if len(virtualServiceNamespaces) > 0 {
 		for _, ns := range virtualServiceNamespaces {
@@ -260,6 +262,7 @@ func virtualServiceLabelsMatchesExpressionRequirements(requirements labels.Requi
 	}
 	return true
 }
+
 func hasSsl(vs *v1.VirtualService) bool {
 	return vs.GetSslConfig() != nil
 }
@@ -322,7 +325,8 @@ func (v *VirtualServiceTranslator) mergeDelegatedVirtualHostOptions(vs *v1.Virtu
 			vs.GetVirtualHost().Options = vhOption.GetOptions()
 			continue
 		}
-		vs.GetVirtualHost().Options = mergeVirtualHostOptions(vs.GetVirtualHost().GetOptions(), vhOption.GetOptions())
+		merged, _ := utils.ShallowMergeVirtualHostOptions(vs.GetVirtualHost().GetOptions(), vhOption.GetOptions())
+		vs.GetVirtualHost().Options = merged
 	}
 }
 
@@ -447,7 +451,6 @@ func regexShortCircuits(laterMatcher, earlierMatcher *matchers.Matcher) bool {
 // As future matcher APIs get added, this validation will need to be updated as well.
 // If it gets too complex, consider modeling as a constraint satisfaction problem.
 func nonPathEarlyMatcherShortCircuitsLateMatcher(laterMatcher, earlierMatcher *matchers.Matcher) bool {
-
 	// we play a trick here to validate the methods by writing them as header
 	// matchers and just reusing the header matcher logic
 	earlyMatcher := *earlierMatcher
@@ -606,7 +609,6 @@ func earlyHeaderMatchersShortCircuitLaterOnes(laterMatcher, earlyMatcher matcher
 //     routeAction:
 //     ....
 func laterOrRegexPartiallyShortCircuited(laterHeaderMatcher, earlyHeaderMatcher *matchers.HeaderMatcher) bool {
-
 	// regex matches simple OR regex, e.g. (GET|POST|...)
 	re, err := regexp.Compile("^\\([\\w]+([|[\\w]+)+\\)$")
 	if err != nil {

projects/gateway2/translator/plugins/routeoptions/query/query.go

@@ -94,24 +94,25 @@ func (r *routeOptionQueries) GetRouteOptionForRouteRule(
 		}
 	}
 
+	merged := override
+	if merged == nil {
+		merged = &solokubev1.RouteOption{}
+	}
+
 	out := make([]*solokubev1.RouteOption, len(list.Items))
 	for i := range list.Items {
 		out[i] = &list.Items[i]
 	}
 	utils.SortByCreationTime(out)
-	attached := out[0]
-
-	if override == nil {
-		sources = append(sources, routeOptionToSourceRef(attached))
-		return attached, sources, nil
-	}
-
-	_, usedAttached := glooutils.ShallowMergeRouteOptions(override.Spec.GetOptions(), attached.Spec.GetOptions())
-	if usedAttached {
-		sources = append(sources, routeOptionToSourceRef(attached))
+	for _, opt := range out {
+		optionUsed := false
+		merged.Spec.Options, optionUsed = glooutils.ShallowMergeRouteOptions(merged.Spec.GetOptions(), opt.Spec.GetOptions())
+		if optionUsed {
+			sources = append(sources, routeOptionToSourceRef(opt))
+		}
 	}
 
-	return override, sources, nil
+	return merged, sources, nil
 }
 
 func lookupFilterOverride(

projects/gateway2/translator/plugins/routeoptions/query/query_test.go

@@ -39,10 +39,11 @@ var _ = Describe("Query", func() {
 			ctx := context.Background()
 
 			hr := httpRoute()
+			attachedOpt := attachedRouteOption()
 			hrNsName := types.NamespacedName{Namespace: hr.GetNamespace(), Name: hr.GetName()}
 			deps := []client.Object{
 				hr,
-				attachedRouteOption(),
+				attachedOpt,
 				diffNamespaceRouteOption(),
 			}
 			fakeClient := builder.WithObjects(deps...).Build()
@@ -54,8 +55,8 @@ var _ = Describe("Query", func() {
 
 			Expect(err).NotTo(HaveOccurred())
 			Expect(rtOpt).ToNot(BeNil())
-			Expect(rtOpt.GetName()).To(Equal("good-policy"))
-			Expect(rtOpt.GetNamespace()).To(Equal("default"))
+
+			Expect(rtOpt.Spec.GetOptions().GetFaults().GetAbort().GetPercentage()).To(BeNumerically("==", attachedOpt.Spec.GetOptions().GetFaults().GetAbort().GetPercentage()))
 			Expect(sources).To(HaveLen(1))
 		})
 
@@ -87,9 +88,10 @@ var _ = Describe("Query", func() {
 			hr := httpRoute()
 			hrNsName := types.NamespacedName{Namespace: hr.GetNamespace(), Name: hr.GetName()}
 
+			attachedOpt := attachedRouteOptionOmitNamespace()
 			deps := []client.Object{
 				hr,
-				attachedRouteOptionOmitNamespace(),
+				attachedOpt,
 				diffNamespaceRouteOption(),
 			}
 			fakeClient := builder.WithObjects(deps...).Build()
@@ -101,8 +103,8 @@ var _ = Describe("Query", func() {
 
 			Expect(err).NotTo(HaveOccurred())
 			Expect(rtOpt).ToNot(BeNil())
-			Expect(rtOpt.GetName()).To(Equal("good-policy-no-ns"))
-			Expect(rtOpt.GetNamespace()).To(Equal("default"))
+
+			Expect(rtOpt.Spec.GetOptions().GetFaults().GetAbort().GetPercentage()).To(BeNumerically("==", attachedOpt.Spec.GetOptions().GetFaults().GetAbort().GetPercentage()))
 			Expect(sources).To(HaveLen(1))
 		})
 

projects/gateway2/translator/plugins/routeoptions/route_options_plugin_test.go

@@ -8,6 +8,7 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/types/known/durationpb"
 	"google.golang.org/protobuf/types/known/wrapperspb"
 
 	"github.com/solo-io/gloo/pkg/utils/statusutils"
@@ -324,6 +325,87 @@ var _ = Describe("RouteOptionsPlugin", func() {
 			})
 		})
 
+		When("Multiple RouteOptions using targetRef attach to the same route", func() {
+			It("correctly merges in priority order from oldest to newest", func() {
+				first := newBaseRouteOption("first")
+
+				second := first.Clone().(*sologatewayv1.RouteOption)
+				second.Metadata.Name = "second"
+				second.Options.Faults.Abort.Percentage = 5
+				second.Options.PrefixRewrite = &wrapperspb.StringValue{Value: "/prefix2"}
+
+				third := second.Clone().(*sologatewayv1.RouteOption)
+				third.Metadata.Name = "third"
+				third.Options.PrefixRewrite = &wrapperspb.StringValue{Value: "/prefix3"}
+				third.Options.IdleTimeout = &durationpb.Duration{Seconds: 5}
+
+				routeOptionClient.Write(first, clients.WriteOpts{})
+				routeOptionClient.Write(second, clients.WriteOpts{})
+				routeOptionClient.Write(third, clients.WriteOpts{})
+
+				firstOpt := attachedRouteOptionAfterT("first", 0, first)
+				secondOpt := attachedRouteOptionAfterT("second", 1*time.Hour, second)
+				thirdOpt := attachedRouteOptionAfterT("third", 2*time.Hour, third)
+
+				deps := []client.Object{firstOpt, secondOpt, thirdOpt}
+				fakeClient := testutils.BuildIndexedFakeClient(deps, gwquery.IterateIndices, rtoptquery.IterateIndices)
+				gwQueries := testutils.BuildGatewayQueriesWithClient(fakeClient)
+				plugin := NewPlugin(gwQueries, fakeClient, routeOptionClient, statusReporter)
+
+				ctx := context.Background()
+				routeCtx := &plugins.RouteContext{
+					Route: &gwv1.HTTPRoute{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "route",
+							Namespace: "default",
+						},
+					},
+				}
+
+				outputRoute := &v1.Route{
+					Options: &v1.RouteOptions{},
+				}
+				plugin.ApplyRoutePlugin(ctx, routeCtx, outputRoute)
+
+				// Confirm that the merged values are as expected
+				Expect(outputRoute.GetOptions().GetFaults().GetAbort().GetPercentage()).To(BeNumerically("==", first.GetOptions().GetFaults().GetAbort().GetPercentage()))
+				Expect(outputRoute.GetOptions().GetPrefixRewrite().GetValue()).To(Equal(second.GetOptions().GetPrefixRewrite().GetValue()))
+				Expect(outputRoute.GetOptions().GetIdleTimeout()).To(Equal(third.GetOptions().GetIdleTimeout()))
+
+				Expect(outputRoute.GetMetadataStatic().GetSources()).To(HaveLen(3))
+				Expect(outputRoute.GetMetadataStatic().GetSources()[0].GetResourceRef().GetName()).To(Equal("first"))
+				Expect(outputRoute.GetMetadataStatic().GetSources()[1].GetResourceRef().GetName()).To(Equal("second"))
+				Expect(outputRoute.GetMetadataStatic().GetSources()[2].GetResourceRef().GetName()).To(Equal("third"))
+
+				px := &v1.Proxy{}
+				statusCtx := plugins.StatusContext{
+					ProxiesWithReports: []translatorutils.ProxyWithReports{
+						{
+							Proxy: px,
+							Reports: translatorutils.TranslationReports{
+								ProxyReport:     &validation.ProxyReport{},
+								ResourceReports: reporter.ResourceReports{},
+							},
+						},
+					},
+				}
+
+				plugin.ApplyStatusPlugin(ctx, &statusCtx)
+
+				firstObj, _ := routeOptionClient.Read("default", "first", clients.ReadOpts{Ctx: ctx})
+				status := firstObj.GetNamespacedStatuses().Statuses["gloo-system"]
+				Expect(status.State).To(Equal(core.Status_Accepted))
+
+				secondObj, _ := routeOptionClient.Read("default", "second", clients.ReadOpts{Ctx: ctx})
+				status = secondObj.GetNamespacedStatuses().Statuses["gloo-system"]
+				Expect(status.State).To(Equal(core.Status_Accepted))
+
+				thirdObj, _ := routeOptionClient.Read("default", "third", clients.ReadOpts{Ctx: ctx})
+				status = thirdObj.GetNamespacedStatuses().Statuses["gloo-system"]
+				Expect(status.State).To(Equal(core.Status_Accepted))
+			})
+		})
+
 		When("RouteOptions exist in the same namespace but are not attached correctly", func() {
 			It("does not add faultinjection", func() {
 				routeOptionClient.Write(nonAttachedInternal(), clients.WriteOpts{})
@@ -418,7 +500,7 @@ var _ = Describe("RouteOptionsPlugin", func() {
 			})
 		})
 
-		When("RouteOptions exist in the same namespace and are attached correctly but hvae processing errors during xds translation", func() {
+		When("RouteOptions exist in the same namespace and are attached correctly but have processing errors during xds translation", func() {
 			It("propagates faultinjection config but reports the processing error on resource status", func() {
 				routeOptionClient.Write(attachedInvalidInternal(), clients.WriteOpts{})
 				deps := []client.Object{attachedInvalidRouteOption()}
@@ -620,6 +702,31 @@ func routeWithFilter() *gwv1.HTTPRoute {
 	return rwf
 }
 
+func newBaseRouteOption(name string) *sologatewayv1.RouteOption {
+	return &sologatewayv1.RouteOption{
+		Metadata: &core.Metadata{
+			Name:      name,
+			Namespace: "default",
+		},
+		TargetRefs: []*corev1.PolicyTargetReference{
+			{
+				Group:     gwv1.GroupVersion.Group,
+				Kind:      wellknown.HTTPRouteKind,
+				Name:      "route",
+				Namespace: wrapperspb.String("default"),
+			},
+		},
+		Options: &v1.RouteOptions{
+			Faults: &faultinjection.RouteFaults{
+				Abort: &faultinjection.RouteAbort{
+					Percentage: 4.19,
+					HttpStatus: 500,
+				},
+			},
+		},
+	}
+}
+
 func attachedInternal() *sologatewayv1.RouteOption {
 	return &sologatewayv1.RouteOption{
 		Metadata: &core.Metadata{
@@ -700,6 +807,20 @@ func attachedRouteOptionBefore() *solokubev1.RouteOption {
 	}
 }
 
+func attachedRouteOptionAfterT(name string, d time.Duration, spec *sologatewayv1.RouteOption) *solokubev1.RouteOption {
+	return &solokubev1.RouteOption{
+		TypeMeta: metav1.TypeMeta{
+			Kind: sologatewayv1.RouteOptionGVK.Kind,
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:              name,
+			Namespace:         "default",
+			CreationTimestamp: metav1.NewTime(time.Now().Add(d)),
+		},
+		Spec: *spec,
+	}
+}
+
 func attachedOmitNamespaceInternal() *sologatewayv1.RouteOption {
 	return &sologatewayv1.RouteOption{
 		Metadata: &core.Metadata{