From f98f79d9f5e2c6d57b5776a8bcfb2db8fc5c9d05 Mon Sep 17 00:00:00 2001 From: Shashank Ram Date: Mon, 6 Jan 2025 14:17:30 -0800 Subject: [PATCH] gateway2: allow child policies to always set fields unset on the parent 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. Testing done: Adds tests that otherwise fail without the code change to the plugin. - Unit test - Translator test --- changelog/v1.18.4/policy-prefixrewrite.yaml | 14 +++++++ .../routeoptions/route_options_plugin_test.go | 38 ++++++++++++++++++- ...ti_level_inheritance_override_partial.yaml | 6 +++ ...ti_level_inheritance_override_partial.yaml | 1 + projects/gloo/pkg/utils/merge.go | 5 ++- 5 files changed, 62 insertions(+), 2 deletions(-) create mode 100644 changelog/v1.18.4/policy-prefixrewrite.yaml diff --git a/changelog/v1.18.4/policy-prefixrewrite.yaml b/changelog/v1.18.4/policy-prefixrewrite.yaml new file mode 100644 index 00000000000..66757a2d04f --- /dev/null +++ b/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. diff --git a/projects/gateway2/translator/plugins/routeoptions/route_options_plugin_test.go b/projects/gateway2/translator/plugins/routeoptions/route_options_plugin_test.go index a0497cb53e4..85172b5ccf2 100644 --- a/projects/gateway2/translator/plugins/routeoptions/route_options_plugin_test.go +++ b/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 { diff --git a/projects/gateway2/translator/testutils/inputs/delegation/route_options_multi_level_inheritance_override_partial.yaml b/projects/gateway2/translator/testutils/inputs/delegation/route_options_multi_level_inheritance_override_partial.yaml index badfc3480a2..8070a110334 100644 --- a/projects/gateway2/translator/testutils/inputs/delegation/route_options_multi_level_inheritance_override_partial.yaml +++ b/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 diff --git a/projects/gateway2/translator/testutils/outputs/delegation/route_options_multi_level_inheritance_override_partial.yaml b/projects/gateway2/translator/testutils/outputs/delegation/route_options_multi_level_inheritance_override_partial.yaml index ff5dd730ac6..e68e2599148 100644 --- a/projects/gateway2/translator/testutils/outputs/delegation/route_options_multi_level_inheritance_override_partial.yaml +++ b/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: diff --git a/projects/gloo/pkg/utils/merge.go b/projects/gloo/pkg/utils/merge.go index 6cbedab218d..132ff400e03 100644 --- a/projects/gloo/pkg/utils/merge.go +++ b/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 }