[backport] gateway2: allow child policies to always set fields unset …

…on the parent (#10550)

changelog/v1.18.4/policy-prefixrewrite.yaml

@@ -0,0 +1,14 @@
+changelog:
+  - type: FIX
+    issueLink: https://github.com/solo-io/solo-projects/issues/7601
+    resolvesIssue: false
+    description: |
+      When merging parent-child policies, the merging should allow child
+      policies to augment parent policies such that fields unset on the
+      parent can be set by the child. There is a bug when using policy
+      override capability with route delegation that disallows this when
+      the annotation specifies non-wildcard fields, such that even if
+      a field is unset by the parent only the fields specified in the
+      override annotation are merged in - which is incorrect because
+      the annotation only applies to fields that are being overriden
+      (set by the parent). This change fixes the bug.

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

@@ -5,9 +5,11 @@ import (
 	"errors"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/testing/protocmp"
 	"google.golang.org/protobuf/types/known/durationpb"
 	"google.golang.org/protobuf/types/known/wrapperspb"
 	"istio.io/istio/pkg/kube/krt"
@@ -737,7 +739,7 @@ var _ = Describe("RouteOptionsPlugin", func() {
 var _ = DescribeTable("mergeOptionsForRoute",
 	func(route *gwv1.HTTPRoute, dst, src *v1.RouteOptions, expectedOptions *v1.RouteOptions, expectedResult glooutils.OptionsMergeResult) {
 		mergedOptions, result := mergeOptionsForRoute(context.TODO(), route, dst, src)
-		Expect(proto.Equal(mergedOptions, expectedOptions)).To(BeTrue())
+		Expect(cmp.Diff(mergedOptions, expectedOptions, protocmp.Transform())).To(BeEmpty())
 		Expect(result).To(Equal(expectedResult))
 	},
 	Entry("prefer dst options by default",
@@ -890,6 +892,40 @@ var _ = DescribeTable("mergeOptionsForRoute",
 		},
 		glooutils.OptionsMergedFull,
 	),
+	Entry("override and augment dst options with annotation: specific fields",
+		&gwv1.HTTPRoute{
+			ObjectMeta: metav1.ObjectMeta{
+				Annotations: map[string]string{policyOverrideAnnotation: "faults,timeout"},
+			},
+		},
+		&v1.RouteOptions{
+			Faults: &faultinjection.RouteFaults{
+				Abort: &faultinjection.RouteAbort{
+					HttpStatus: 500,
+				},
+			},
+			Timeout: durationpb.New(5 * time.Second),
+		},
+		&v1.RouteOptions{
+			PrefixRewrite: &wrapperspb.StringValue{Value: "/src"},
+			Faults: &faultinjection.RouteFaults{
+				Abort: &faultinjection.RouteAbort{
+					HttpStatus: 100,
+				},
+			},
+			Timeout: durationpb.New(10 * time.Second),
+		},
+		&v1.RouteOptions{
+			Faults: &faultinjection.RouteFaults{
+				Abort: &faultinjection.RouteAbort{
+					HttpStatus: 100,
+				},
+			},
+			PrefixRewrite: &wrapperspb.StringValue{Value: "/src"},
+			Timeout:       durationpb.New(10 * time.Second),
+		},
+		glooutils.OptionsMergedFull,
+	),
 )
 
 func routeOption() *solokubev1.RouteOption {

projects/gateway2/translator/testutils/inputs/delegation/route_options_multi_level_inheritance_override_partial.yaml

@@ -112,6 +112,12 @@ spec:
     - path:
         type: PathPrefix
         value: /a/1
+    filters:
+      - type: URLRewrite
+        urlRewrite:
+          path:
+            replacePrefixMatch: /rewrite/path
+            type: ReplacePrefixMatch
     backendRefs:
     - name: svc-a
       port: 8080

projects/gateway2/translator/testutils/outputs/delegation/route_options_multi_level_inheritance_override_partial.yaml

@@ -37,6 +37,7 @@ listeners:
               headerManipulation:
                 requestHeadersToRemove:
                 - override
+              prefixRewrite: /rewrite/path
             name: httproute-route-a-a-0-0
             routeAction:
               single:

projects/gloo/pkg/utils/merge.go

@@ -147,7 +147,10 @@ func MergeRouteOptionsWithOverrides(dst, src *v1.RouteOptions, allowedOverrides
 	var dstFieldsOverwritten int
 	for i := range dstValue.NumField() {
 		dstField, srcField := dstValue.Field(i), srcValue.Field(i)
-		if overwriteByDefault && !(allowedOverrides.Has(wildcardField) ||
+		// Allow overrides if the field in dst is unset as merging from src into dst by default
+		// allows src to augment dst by setting fields unset in dst, hence the override check only
+		// applies when the field in dst is set: !isEmptyValue(dstField).
+		if !isEmptyValue(dstField) && overwriteByDefault && !(allowedOverrides.Has(wildcardField) ||
 			allowedOverrides.Has(strings.ToLower(dstValue.Type().Field(i).Name))) {
 			continue
 		}