Merge branch 'main' into devel-backport-changelog

changelog/v1.19.0-beta3/fix-sp-7315.yaml

@@ -0,0 +1,20 @@
+changelog:
+  - type: FIX
+    issueLink: https://github.com/solo-io/solo-projects/issues/7315
+    resolvesIssue: false
+    description: |
+      gateway2/delegation: enable inherited policy overrides
+
+      Adds the ability to override inherited policy fields when
+      explicitly permitted by a parent route using the annotation
+      delegation.gateway.solo.io/enable-policy-overrides.
+      It supports a wildcard value "*" or a comma separated list
+      of field names such as "faults,timeouts,retries,headermanipulation".
+
+      Functionally, a child RouteOption may only override the RouteOptions
+      derived from its parent if the above annotation exists on the parent
+      route. This is required to make the override behavior safe to use.
+
+      Testing done:
+      - Translator tests for the new scenarios.
+

projects/gateway2/translator/gateway_translator_test.go

@@ -326,4 +326,7 @@ var _ = DescribeTable("Route Delegation translator",
 	Entry("Child route matcher does not match parent", "bug-6621.yaml"),
 	// https://github.com/k8sgateway/k8sgateway/issues/10379
 	Entry("Multi-level multiple parents delegation", "bug-10379.yaml"),
+	Entry("RouteOptions prefer child override when allowed", "route_options_inheritance_child_override_allow.yaml"),
+	Entry("RouteOptions multi level inheritance with child override when allowed", "route_options_multi_level_inheritance_override_allow.yaml"),
+	Entry("RouteOptions multi level inheritance with partial child override", "route_options_multi_level_inheritance_override_partial.yaml"),
 )

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

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"strings"
 
 	"github.com/hashicorp/go-multierror"
 	"github.com/rotisserie/eris"
@@ -15,6 +16,7 @@ import (
 	"istio.io/istio/pkg/kube/krt"
 	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"
 
@@ -31,6 +33,15 @@ import (
 	glooutils "github.com/solo-io/gloo/projects/gloo/pkg/utils"
 )
 
+const (
+	// policyOverrideAnnotation can be set by parent routes to allow child routes to override
+	// all (wildcard *) or specific fields (comma separated field names) in RouteOptions inherited from the parent route.
+	policyOverrideAnnotation = "delegation.gateway.solo.io/enable-policy-overrides"
+
+	// wildcardField is used to enable overriding all fields in RouteOptions inherited from the parent route.
+	wildcardField = "*"
+)
+
 var (
 	_ plugins.RoutePlugin  = &plugin{}
 	_ plugins.StatusPlugin = &plugin{}
@@ -83,7 +94,7 @@ func (p *plugin) ApplyRoutePlugin(
 	routeCtx *plugins.RouteContext,
 	outputRoute *gloov1.Route,
 ) error {
-	// check for RouteOptions applied to full Route
+	// check for RouteOptions applied to the given routeCtx
 	routeOptions, _, sources, err := p.handleAttachment(ctx, routeCtx)
 	if err != nil {
 		return err
@@ -92,22 +103,59 @@ func (p *plugin) ApplyRoutePlugin(
 		return nil
 	}
 
-	// If the route already has options set, we should override them.
-	// This is important because for delegated routes, the plugin will
-	// be invoked on the child routes multiple times for each parent route
-	// that may override them.
-	merged, usedExistingSources := glooutils.ShallowMergeRouteOptions(routeOptions, outputRoute.GetOptions())
+	merged, OptionsMergeResult := mergeOptionsForRoute(ctx, routeCtx.HTTPRoute, routeOptions, outputRoute.GetOptions())
+	if OptionsMergeResult == glooutils.OptionsMergedNone {
+		// No existing options merged into 'sources', so set the 'sources' on the outputRoute
+		routeutils.SetRouteSources(outputRoute, sources)
+	} else if OptionsMergeResult == glooutils.OptionsMergedPartial {
+		// Some existing options merged into 'sources', so append the 'sources' on the outputRoute
+		routeutils.AppendRouteSources(outputRoute, sources)
+	} // In case OptionsMergedFull, the correct sources are already set on the outputRoute
+
+	// Set the merged RouteOptions on the outputRoute
 	outputRoute.Options = merged
 
-	// Track the RouteOption policy sources that are used so we can report status on it
-	routeutils.AppendSourceToRoute(outputRoute, sources, usedExistingSources)
 	// Track that we used this RouteOption is our status cache
 	// we do this so we can persist status later for all attached RouteOptions
-	p.trackAcceptedRouteOptions(sources)
+	p.trackAcceptedRouteOptions(outputRoute.GetMetadataStatic().GetSources())
 
 	return nil
 }
 
+func mergeOptionsForRoute(
+	ctx context.Context,
+	route *gwv1.HTTPRoute,
+	dst, src *gloov1.RouteOptions,
+) (*gloov1.RouteOptions, glooutils.OptionsMergeResult) {
+	// By default, lower priority options cannot override higher priority ones
+	// and can only augment them during a merge such that fields unset in the higher
+	// priority options can be merged in from the lower priority options.
+	// In the case of delegated routes, a parent route can enable child routes to override
+	// all (wildcard *) or specific fields using the policyOverrideAnnotation.
+	fieldsAllowedToOverride := sets.New[string]()
+
+	// If the route already has options set, we should override/augment them.
+	// This is important because for delegated routes, the plugin will
+	// be invoked on the child routes multiple times for each parent route
+	// that may override/augment them.
+	//
+	// By default, parent options (routeOptions) are preferred, unless the parent explicitly
+	// enabled child routes (outputRoute.Options) to override parent options.
+	fieldsStr, delegatedPolicyOverride := route.Annotations[policyOverrideAnnotation]
+	if delegatedPolicyOverride {
+		delegatedFieldsToOverride := parseDelegationFieldOverrides(fieldsStr)
+		if delegatedFieldsToOverride.Len() == 0 {
+			// Invalid annotation value, so log an error but enforce the default behavior of preferring the parent options.
+			contextutils.LoggerFrom(ctx).Errorf("invalid value %q for annotation %s on route %s; must be %s or a comma-separated list of field names",
+				fieldsStr, policyOverrideAnnotation, client.ObjectKeyFromObject(route), wildcardField)
+		} else {
+			fieldsAllowedToOverride = delegatedFieldsToOverride
+		}
+	}
+
+	return glooutils.MergeRouteOptionsWithOverrides(dst, src, fieldsAllowedToOverride)
+}
+
 func (p *plugin) InitStatusPlugin(ctx context.Context, statusCtx *plugins.StatusContext) error {
 	for _, proxyWithReport := range statusCtx.ProxiesWithReports {
 		// now that we translate proxies one by one, we can't assume ApplyRoutePlugin is called before ApplyStatusPlugin for all proxies
@@ -300,3 +348,16 @@ func extractRouteOptionSourceKeys(routeErr *validation.RouteReport_Error) (types
 
 	return types.NamespacedName{}, false
 }
+
+func parseDelegationFieldOverrides(val string) sets.Set[string] {
+	if val == wildcardField {
+		return sets.New(wildcardField)
+	}
+
+	set := sets.New[string]()
+	parts := strings.Split(val, ",")
+	for _, part := range parts {
+		set.Insert(strings.ToLower(strings.TrimSpace(part)))
+	}
+	return set
+}

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

@@ -27,6 +27,7 @@ import (
 	v1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1"
 	"github.com/solo-io/gloo/projects/gloo/pkg/api/v1/options/faultinjection"
 	"github.com/solo-io/gloo/projects/gloo/pkg/defaults"
+	glooutils "github.com/solo-io/gloo/projects/gloo/pkg/utils"
 	corev1 "github.com/solo-io/skv2/pkg/api/core.skv2.solo.io/v1"
 	"github.com/solo-io/solo-kit/pkg/api/v1/clients"
 	"github.com/solo-io/solo-kit/pkg/api/v1/clients/factory"
@@ -354,7 +355,6 @@ var _ = Describe("RouteOptionsPlugin", func() {
 					err := plugin.ApplyStatusPlugin(ctx, statusCtx)
 					Expect(err).To(MatchError(ContainSubstring(ReadingRouteOptionErrStr)))
 				})
-
 			})
 		})
 
@@ -734,6 +734,164 @@ 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(result).To(Equal(expectedResult))
+	},
+	Entry("prefer dst options by default",
+		&gwv1.HTTPRoute{},
+		&v1.RouteOptions{
+			Faults: &faultinjection.RouteFaults{
+				Abort: &faultinjection.RouteAbort{
+					HttpStatus: 500,
+				},
+			},
+		},
+		&v1.RouteOptions{
+			PrefixRewrite: &wrapperspb.StringValue{Value: "/prefix"},
+			// Faults will be ignored because it is set in dst
+			Faults: &faultinjection.RouteFaults{
+				Abort: &faultinjection.RouteAbort{
+					HttpStatus: 100,
+				},
+			},
+		},
+		&v1.RouteOptions{
+			Faults: &faultinjection.RouteFaults{
+				Abort: &faultinjection.RouteAbort{
+					HttpStatus: 500,
+				},
+			},
+			PrefixRewrite: &wrapperspb.StringValue{Value: "/prefix"},
+		},
+		glooutils.OptionsMergedPartial,
+	),
+	Entry("override dst options with annotation: full override",
+		&gwv1.HTTPRoute{
+			ObjectMeta: metav1.ObjectMeta{
+				Annotations: map[string]string{policyOverrideAnnotation: "*"},
+			},
+		},
+		&v1.RouteOptions{
+			Faults: &faultinjection.RouteFaults{
+				Abort: &faultinjection.RouteAbort{
+					HttpStatus: 500,
+				},
+			},
+		},
+		&v1.RouteOptions{
+			PrefixRewrite: &wrapperspb.StringValue{Value: "/prefix"},
+			Faults: &faultinjection.RouteFaults{
+				Abort: &faultinjection.RouteAbort{
+					HttpStatus: 100,
+				},
+			},
+		},
+		&v1.RouteOptions{
+			Faults: &faultinjection.RouteFaults{
+				Abort: &faultinjection.RouteAbort{
+					HttpStatus: 100,
+				},
+			},
+			PrefixRewrite: &wrapperspb.StringValue{Value: "/prefix"},
+		},
+		glooutils.OptionsMergedFull,
+	),
+	Entry("override dst options with annotation: partial override",
+		&gwv1.HTTPRoute{
+			ObjectMeta: metav1.ObjectMeta{
+				Annotations: map[string]string{policyOverrideAnnotation: "*"},
+			},
+		},
+		&v1.RouteOptions{
+			Faults: &faultinjection.RouteFaults{
+				Abort: &faultinjection.RouteAbort{
+					HttpStatus: 500,
+				},
+			},
+			Timeout: durationpb.New(5 * time.Second),
+		},
+		&v1.RouteOptions{
+			PrefixRewrite: &wrapperspb.StringValue{Value: "/prefix"},
+			Faults: &faultinjection.RouteFaults{
+				Abort: &faultinjection.RouteAbort{
+					HttpStatus: 100,
+				},
+			},
+		},
+		&v1.RouteOptions{
+			Faults: &faultinjection.RouteFaults{
+				Abort: &faultinjection.RouteAbort{
+					HttpStatus: 100,
+				},
+			},
+			PrefixRewrite: &wrapperspb.StringValue{Value: "/prefix"},
+			Timeout:       durationpb.New(5 * time.Second),
+		},
+		glooutils.OptionsMergedPartial,
+	),
+	Entry("override dst options with annotation: no override",
+		&gwv1.HTTPRoute{
+			ObjectMeta: metav1.ObjectMeta{
+				Annotations: map[string]string{policyOverrideAnnotation: "*"},
+			},
+		},
+		&v1.RouteOptions{
+			Faults: &faultinjection.RouteFaults{
+				Abort: &faultinjection.RouteAbort{
+					HttpStatus: 500,
+				},
+			},
+		},
+		&v1.RouteOptions{},
+		&v1.RouteOptions{
+			Faults: &faultinjection.RouteFaults{
+				Abort: &faultinjection.RouteAbort{
+					HttpStatus: 500,
+				},
+			},
+		},
+		glooutils.OptionsMergedNone,
+	),
+	Entry("override 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),
+			PrefixRewrite: &wrapperspb.StringValue{Value: "/dst"},
+		},
+		&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: "/dst"},
+			Timeout:       durationpb.New(10 * time.Second),
+		},
+		glooutils.OptionsMergedFull,
+	),
+)
+
 func routeOption() *solokubev1.RouteOption {
 	return &solokubev1.RouteOption{
 		ObjectMeta: metav1.ObjectMeta{

projects/gateway2/translator/routeutils/utils.go

@@ -5,19 +5,28 @@ import (
 	v1 "github.com/solo-io/gloo/projects/gloo/pkg/api/v1"
 )
 
-func AppendSourceToRoute(route *v1.Route, newSources []*gloov1.SourceMetadata_SourceRef, preserveExisting bool) {
+func AppendRouteSources(route *v1.Route, newSources []*gloov1.SourceMetadata_SourceRef) {
 	meta := route.GetMetadataStatic()
 	if meta == nil {
 		meta = &gloov1.SourceMetadata{}
 	}
 	sources := meta.GetSources()
-	if !preserveExisting {
-		sources = nil
-	}
 	sources = append(sources, newSources...)
 	route.OpaqueMetadata = &gloov1.Route_MetadataStatic{
 		MetadataStatic: &gloov1.SourceMetadata{
 			Sources: sources,
 		},
 	}
 }
+
+func SetRouteSources(route *v1.Route, sources []*gloov1.SourceMetadata_SourceRef) {
+	meta := route.GetMetadataStatic()
+	if meta == nil {
+		meta = &gloov1.SourceMetadata{}
+	}
+	route.OpaqueMetadata = &gloov1.Route_MetadataStatic{
+		MetadataStatic: &gloov1.SourceMetadata{
+			Sources: sources,
+		},
+	}
+}