diff --git a/internal/gatewayapi/backendtrafficpolicy.go b/internal/gatewayapi/backendtrafficpolicy.go index a520dbc212..c4a2e55a99 100644 --- a/internal/gatewayapi/backendtrafficpolicy.go +++ b/internal/gatewayapi/backendtrafficpolicy.go @@ -823,7 +823,7 @@ func (t *Translator) translateBackendTrafficPolicyForRoute( policyTargetGatewayNN *types.NamespacedName, policyTargetListener *gwapiv1.SectionName, ) error { - tf, errs := t.buildTrafficFeatures(policy) + tf, errs := t.buildTrafficFeatures(policy, nil) if tf == nil { // should not happen return nil @@ -848,13 +848,13 @@ func (t *Translator) translateBackendTrafficPolicyForRouteWithMerge( policyTargetGatewayNN types.NamespacedName, policyTargetListener *gwapiv1.SectionName, route RouteContext, xdsIR resource.XdsIRMap, ) error { - mergedPolicy, err := t.mergeBackendTrafficPolicy(policy, parentPolicy) + mergedPolicy, owners, err := t.mergeBackendTrafficPolicy(policy, parentPolicy) if err != nil { return fmt.Errorf("error merging policies: %w", err) } // Build traffic features from the merged policy - tf, errs := t.buildTrafficFeatures(mergedPolicy) + tf, errs := t.buildTrafficFeatures(mergedPolicy, owners) if tf == nil { // should not happen return nil @@ -869,8 +869,8 @@ func (t *Translator) translateBackendTrafficPolicyForRouteWithMerge( // 2. Only gateway policy has rate limits - preserve gateway policy's rule names // 3. Only route policy has rate limits - use route policy's rule names (default behavior) if policy.Spec.RateLimit != nil && parentPolicy.Spec.RateLimit != nil { - tfGW, _ := t.buildTrafficFeatures(parentPolicy) - tfRoute, _ := t.buildTrafficFeatures(policy) + tfGW, _ := t.buildTrafficFeatures(parentPolicy, nil) + tfRoute, _ := t.buildTrafficFeatures(policy, nil) if tfGW != nil && tfRoute != nil && tfGW.RateLimit != nil && tfRoute.RateLimit != nil { @@ -884,7 +884,7 @@ func (t *Translator) translateBackendTrafficPolicyForRouteWithMerge( } } else if policy.Spec.RateLimit == nil && parentPolicy.Spec.RateLimit != nil { // Case 2: Only gateway policy has rate limits - preserve gateway policy's rule names - tfGW, _ := t.buildTrafficFeatures(parentPolicy) + tfGW, _ := t.buildTrafficFeatures(parentPolicy, nil) if tfGW != nil && tfGW.RateLimit != nil { // Use the gateway policy's rate limit with its original rule names tf.RateLimit = tfGW.RateLimit @@ -899,7 +899,7 @@ func (t *Translator) translateBackendTrafficPolicyForRouteWithMerge( } t.applyTrafficFeatureToRoute(route, tf, errs, mergedPolicy, target, x, policyTargetListener) - return nil + return errs } func (t *Translator) applyTrafficFeatureToRoute(route RouteContext, @@ -1034,25 +1034,25 @@ func (t *Translator) applyTrafficFeatureToRoute(route RouteContext, } } -// mergeBackendTrafficPolicy merges route policy into gateway policy. -func (t *Translator) mergeBackendTrafficPolicy(routePolicy, gwPolicy *egv1a1.BackendTrafficPolicy) (*egv1a1.BackendTrafficPolicy, error) { +// mergeBackendTrafficPolicy merges route policy into gateway policy, returning the merged +// policy and the per-field owners used to resolve references against the contributing +// policy's namespace. +func (t *Translator) mergeBackendTrafficPolicy(routePolicy, gwPolicy *egv1a1.BackendTrafficPolicy) (*egv1a1.BackendTrafficPolicy, *backendTrafficPolicyOwners, error) { if routePolicy.Spec.MergeType == nil || gwPolicy == nil { - return routePolicy, nil + return routePolicy, nil, nil } - // Resolve LocalObjectReferences to inline content in the policies before merge so the merge operates on concrete values. - if err := t.resolveLocalObjectRefsInPolicy(gwPolicy); err != nil { - return nil, err - } - if err := t.resolveLocalObjectRefsInPolicy(routePolicy); err != nil { - return nil, err + mergedPolicy, err := utils.Merge(gwPolicy, routePolicy, *routePolicy.Spec.MergeType) + if err != nil { + return nil, nil, err } - - return utils.Merge(gwPolicy, routePolicy, *routePolicy.Spec.MergeType) + return mergedPolicy, buildBackendTrafficPolicyOwners(routePolicy, gwPolicy), nil } -// buildTrafficFeatures builds IR traffic features from a BackendTrafficPolicy. -func (t *Translator) buildTrafficFeatures(policy *egv1a1.BackendTrafficPolicy) (*ir.TrafficFeatures, error) { +// buildTrafficFeatures builds IR traffic features from a BackendTrafficPolicy. owners is +// the per-field owners for a merged policy, or nil to resolve references against the +// policy's own namespace. +func (t *Translator) buildTrafficFeatures(policy *egv1a1.BackendTrafficPolicy, owners *backendTrafficPolicyOwners) (*ir.TrafficFeatures, error) { var ( rl *ir.RateLimit bl *ir.BandwidthLimit @@ -1128,7 +1128,7 @@ func (t *Translator) buildTrafficFeatures(policy *egv1a1.BackendTrafficPolicy) ( errs = errors.Join(errs, err) } - if ro, err = t.buildResponseOverride(policy); err != nil { + if ro, err = t.buildResponseOverride(policy, owners); err != nil { err = perr.WithMessage(err, "ResponseOverride") errs = errors.Join(errs, err) } @@ -1211,7 +1211,7 @@ func (t *Translator) translateBackendTrafficPolicyForGateway( policy *egv1a1.BackendTrafficPolicy, target policyTargetReferenceWithSectionName, gateway *GatewayContext, xdsIR resource.XdsIRMap, ) error { - tf, errs := t.buildTrafficFeatures(policy) + tf, errs := t.buildTrafficFeatures(policy, nil) if tf == nil { // should not happen return errs @@ -1904,11 +1904,17 @@ func buildRequestBuffer(spec *egv1a1.RequestBuffer) (*ir.RequestBuffer, error) { }, nil } -func (t *Translator) buildResponseOverride(policy *egv1a1.BackendTrafficPolicy) (*ir.ResponseOverride, error) { +func (t *Translator) buildResponseOverride(policy *egv1a1.BackendTrafficPolicy, owners *backendTrafficPolicyOwners) (*ir.ResponseOverride, error) { if len(policy.Spec.ResponseOverride) == 0 { return nil, nil } + // Resolve body ValueRefs against the owner's namespace, falling back to the policy's own. + responseOverrideNs := policy.Namespace + if owners != nil && owners.responseOverride != nil { + responseOverrideNs = owners.responseOverride.Namespace + } + rules := make([]ir.ResponseOverrideRule, 0, len(policy.Spec.ResponseOverride)) for index, ro := range policy.Spec.ResponseOverride { match := ir.CustomResponseMatch{ @@ -1966,7 +1972,7 @@ func (t *Translator) buildResponseOverride(policy *egv1a1.BackendTrafficPolicy) } var err error - response.Body, err = t.getCustomResponseBody(ro.Response.Body, policy.Namespace) + response.Body, err = t.getCustomResponseBody(ro.Response.Body, responseOverrideNs) if err != nil { return nil, err } @@ -2042,45 +2048,23 @@ func (t *Translator) getCustomResponseBody( return nil, nil } -// resolveCustomResponseBodyRefToInline resolves a ValueRef in body to inline content using the given namespace. -// It mutates body in place: replaces Type and ValueRef with Inline content. No-op if body is nil or already Inline. -func (t *Translator) resolveCustomResponseBodyRefToInline(body *egv1a1.CustomResponseBody, policyNs string) error { - if body == nil { - return nil - } - if body.Type == nil || *body.Type != egv1a1.ResponseValueTypeValueRef || body.ValueRef == nil { - return nil - } - data, err := t.getCustomResponseBody(body, policyNs) - if err != nil { - return err - } - inlineStr := string(data) - t.Logger.Info("resolved custom response body ref to inline before merge", - "namespace", policyNs, - "ref", body.ValueRef.Name, - ) - body.Type = new(egv1a1.ResponseValueTypeInline) - body.Inline = &inlineStr - body.ValueRef = nil - return nil +// backendTrafficPolicyOwners records which policy (route or parent) contributed each +// merged field that references other objects, so references resolve against the owner's +// namespace. Mirrors the field-owner pattern used for SecurityPolicy. +type backendTrafficPolicyOwners struct { + responseOverride *egv1a1.BackendTrafficPolicy } -// resolveLocalObjectRefsInPolicy resolves LocalObjectReferences to inline content in the given policy (mutates in place). -// Currently handles ResponseOverride body ValueRefs; may be extended for other refs BackendTrafficPolicy supports. -func (t *Translator) resolveLocalObjectRefsInPolicy(policy *egv1a1.BackendTrafficPolicy) error { - if policy == nil || len(policy.Spec.ResponseOverride) == 0 { - return nil +// buildBackendTrafficPolicyOwners picks the owner of each merged field: the route policy +// when it sets the field, otherwise the parent. +func buildBackendTrafficPolicyOwners(route, parent *egv1a1.BackendTrafficPolicy) *backendTrafficPolicyOwners { + responseOverrideOwner := parent + if len(route.Spec.ResponseOverride) > 0 { + responseOverrideOwner = route } - policyNs := policy.Namespace - for _, ro := range policy.Spec.ResponseOverride { - if ro != nil && ro.Response != nil && ro.Response.Body != nil { - if err := t.resolveCustomResponseBodyRefToInline(ro.Response.Body, policyNs); err != nil { - return err - } - } + return &backendTrafficPolicyOwners{ + responseOverride: responseOverrideOwner, } - return nil } func sourceFromAPI(s *egv1a1.ResponseOverrideSource) egv1a1.ResponseOverrideSource { diff --git a/internal/gatewayapi/backendtrafficpolicy_test.go b/internal/gatewayapi/backendtrafficpolicy_test.go index 56c0cd9bfd..cb4c8e6cac 100644 --- a/internal/gatewayapi/backendtrafficpolicy_test.go +++ b/internal/gatewayapi/backendtrafficpolicy_test.go @@ -215,7 +215,7 @@ func TestBuildTrafficFeaturesRejectsRequestBufferWithHTTPUpgrade(t *testing.T) { }, } - tf, err := tr.buildTrafficFeatures(policy) + tf, err := tr.buildTrafficFeatures(policy, nil) require.ErrorContains(t, err, "RequestBuffer: requestBuffer cannot be used together with httpUpgrade") require.NotNil(t, tf) }) @@ -238,10 +238,10 @@ func TestBuildTrafficFeaturesRejectsRequestBufferWithHTTPUpgrade(t *testing.T) { }, } - mergedPolicy, err := tr.mergeBackendTrafficPolicy(routePolicy, parentPolicy) + mergedPolicy, owners, err := tr.mergeBackendTrafficPolicy(routePolicy, parentPolicy) require.NoError(t, err) - tf, err := tr.buildTrafficFeatures(mergedPolicy) + tf, err := tr.buildTrafficFeatures(mergedPolicy, owners) require.ErrorContains(t, err, "RequestBuffer: requestBuffer cannot be used together with httpUpgrade") require.NotNil(t, tf) }) diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-with-response-override-merged-diff-namespace.out.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-with-response-override-merged-diff-namespace.out.yaml index 5e85ff359b..c55b0c1cea 100644 --- a/internal/gatewayapi/testdata/backendtrafficpolicy-with-response-override-merged-diff-namespace.out.yaml +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-with-response-override-merged-diff-namespace.out.yaml @@ -62,9 +62,11 @@ backendTrafficPolicies: type: Range response: body: - inline: | - error page contents here - type: Inline + type: ValueRef + valueRef: + group: "" + kind: ConfigMap + name: error-page contentType: text/html targetRefs: - group: gateway.networking.k8s.io diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-with-response-override-merged-invalid-valueref.in.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-with-response-override-merged-invalid-valueref.in.yaml new file mode 100644 index 0000000000..91d0c2ef75 --- /dev/null +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-with-response-override-merged-invalid-valueref.in.yaml @@ -0,0 +1,91 @@ +# Tests merged BackendTrafficPolicies when the ResponseOverride ValueRef inherited from +# the gateway (parent) policy points at a ConfigMap that does not exist. The route BTP +# has mergeType and inherits the parent responseOverride; resolution must fail and the +# route policy must report an Invalid condition (not Merged). +gateways: + - apiVersion: gateway.networking.k8s.io/v1 + kind: Gateway + metadata: + namespace: envoy-gateway-system + name: my-gateway + spec: + gatewayClassName: envoy-gateway-class + listeners: + - name: http + protocol: HTTP + port: 80 + allowedRoutes: + namespaces: + from: All +backendTrafficPolicies: + - apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: BackendTrafficPolicy + metadata: + name: my-gateway-error-response + namespace: envoy-gateway-system + spec: + targetRefs: + - group: gateway.networking.k8s.io + kind: Gateway + name: my-gateway + responseOverride: + - match: + statusCodes: + - type: Range + range: + start: 502 + end: 504 + response: + contentType: "text/html" + body: + type: ValueRef + valueRef: + group: "" + kind: ConfigMap + name: missing-error-page + - apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: BackendTrafficPolicy + metadata: + name: my-app-rate-limit + namespace: default + spec: + targetRefs: + - group: gateway.networking.k8s.io + kind: HTTPRoute + name: my-app + mergeType: StrategicMerge + rateLimit: + local: + rules: + - clientSelectors: + - sourceCIDR: + type: Distinct + value: 0.0.0.0/0 + limit: + requests: 200 + unit: Minute +httpRoutes: + - apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + name: my-app + namespace: default + spec: + hostnames: + - myapp.example.com + parentRefs: + - group: gateway.networking.k8s.io + kind: Gateway + name: my-gateway + namespace: envoy-gateway-system + rules: + - backendRefs: + - group: "" + kind: Service + name: service-1 + port: 8080 + weight: 1 + matches: + - path: + type: PathPrefix + value: / diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-with-response-override-merged-invalid-valueref.out.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-with-response-override-merged-invalid-valueref.out.yaml new file mode 100644 index 0000000000..7c96720b0a --- /dev/null +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-with-response-override-merged-invalid-valueref.out.yaml @@ -0,0 +1,276 @@ +backendTrafficPolicies: +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: BackendTrafficPolicy + metadata: + name: my-app-rate-limit + namespace: default + spec: + mergeType: StrategicMerge + rateLimit: + local: + rules: + - clientSelectors: + - sourceCIDR: + type: Distinct + value: 0.0.0.0/0 + limit: + requests: 200 + unit: Minute + targetRefs: + - group: gateway.networking.k8s.io + kind: HTTPRoute + name: my-app + status: + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: my-gateway + namespace: envoy-gateway-system + sectionName: http + conditions: + - lastTransitionTime: null + message: 'ResponseOverride: can''t find the referenced configmap envoy-gateway-system/missing-error-page.' + reason: Invalid + status: "False" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: my-gateway + namespace: envoy-gateway-system + conditions: + - lastTransitionTime: null + message: Policy has been accepted. + reason: Accepted + status: "True" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: BackendTrafficPolicy + metadata: + name: my-gateway-error-response + namespace: envoy-gateway-system + spec: + responseOverride: + - match: + statusCodes: + - range: + end: 504 + start: 502 + type: Range + response: + body: + type: ValueRef + valueRef: + group: "" + kind: ConfigMap + name: missing-error-page + contentType: text/html + targetRefs: + - group: gateway.networking.k8s.io + kind: Gateway + name: my-gateway + status: + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: my-gateway + namespace: envoy-gateway-system + conditions: + - lastTransitionTime: null + message: 'ResponseOverride: can''t find the referenced configmap envoy-gateway-system/missing-error-page.' + reason: Invalid + status: "False" + type: Accepted + - lastTransitionTime: null + message: 'This policy is being overridden by other backendTrafficPolicies + for these routes: [default/my-app]' + reason: Overridden + status: "True" + type: Overridden + controllerName: gateway.envoyproxy.io/gatewayclass-controller +gateways: +- apiVersion: gateway.networking.k8s.io/v1 + kind: Gateway + metadata: + name: my-gateway + namespace: envoy-gateway-system + spec: + gatewayClassName: envoy-gateway-class + listeners: + - allowedRoutes: + namespaces: + from: All + name: http + port: 80 + protocol: HTTP + status: + listeners: + - attachedRoutes: 1 + conditions: + - lastTransitionTime: null + message: Sending translated listener configuration to the data plane + reason: Programmed + status: "True" + type: Programmed + - lastTransitionTime: null + message: Listener has been successfully translated + reason: Accepted + status: "True" + type: Accepted + - lastTransitionTime: null + message: Listener references have been resolved + reason: ResolvedRefs + status: "True" + type: ResolvedRefs + name: http + supportedKinds: + - group: gateway.networking.k8s.io + kind: HTTPRoute + - group: gateway.networking.k8s.io + kind: GRPCRoute +httpRoutes: +- apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + name: my-app + namespace: default + spec: + hostnames: + - myapp.example.com + parentRefs: + - group: gateway.networking.k8s.io + kind: Gateway + name: my-gateway + namespace: envoy-gateway-system + rules: + - backendRefs: + - group: "" + kind: Service + name: service-1 + port: 8080 + weight: 1 + matches: + - path: + type: PathPrefix + value: / + status: + parents: + - conditions: + - lastTransitionTime: null + message: Route is accepted + reason: Accepted + status: "True" + type: Accepted + - lastTransitionTime: null + message: Resolved all the Object references for the Route + reason: ResolvedRefs + status: "True" + type: ResolvedRefs + controllerName: gateway.envoyproxy.io/gatewayclass-controller + parentRef: + group: gateway.networking.k8s.io + kind: Gateway + name: my-gateway + namespace: envoy-gateway-system +infraIR: + envoy-gateway-system/my-gateway: + proxy: + listeners: + - name: envoy-gateway-system/my-gateway/http + ports: + - containerPort: 10080 + name: http-80 + protocol: HTTP + servicePort: 80 + metadata: + labels: + gateway.envoyproxy.io/owning-gateway-name: my-gateway + gateway.envoyproxy.io/owning-gateway-namespace: envoy-gateway-system + ownerReference: + kind: GatewayClass + name: envoy-gateway-class + name: envoy-gateway-system/my-gateway + namespace: envoy-gateway-system +xdsIR: + envoy-gateway-system/my-gateway: + accessLog: + json: + - path: /dev/stdout + globalResources: + proxyServiceCluster: + metadata: + kind: Service + name: envoy-envoy-gateway-system-my-gateway-1d35c6f5 + namespace: envoy-gateway-system + sectionName: "8080" + name: envoy-gateway-system/my-gateway + settings: + - addressType: IP + endpoints: + - host: 7.6.5.4 + port: 8080 + zone: zone1 + metadata: + kind: Service + name: envoy-envoy-gateway-system-my-gateway-1d35c6f5 + namespace: envoy-gateway-system + sectionName: "8080" + name: envoy-gateway-system/my-gateway + protocol: TCP + http: + - address: 0.0.0.0 + externalPort: 80 + hostnames: + - '*' + metadata: + kind: Gateway + name: my-gateway + namespace: envoy-gateway-system + sectionName: http + name: envoy-gateway-system/my-gateway/http + path: + escapedSlashesAction: UnescapeAndRedirect + mergeSlashes: true + port: 10080 + routes: + - destination: + metadata: + kind: HTTPRoute + name: my-app + namespace: default + name: httproute/default/my-app/rule/0 + settings: + - addressType: IP + endpoints: + - host: 7.7.7.7 + port: 8080 + metadata: + kind: Service + name: service-1 + namespace: default + sectionName: "8080" + name: httproute/default/my-app/rule/0/backend/0 + protocol: HTTP + weight: 1 + directResponse: + statusCode: 500 + hostname: myapp.example.com + isHTTP2: false + metadata: + kind: HTTPRoute + name: my-app + namespace: default + name: httproute/default/my-app/rule/0/match/0/myapp_example_com + pathMatch: + distinct: false + name: "" + prefix: / + readyListener: + address: 0.0.0.0 + ipFamily: IPv4 + path: /ready + port: 19003 diff --git a/release-notes/current/other_changes/9445-backendtrafficpolicy-response-override-owner-pattern.md b/release-notes/current/other_changes/9445-backendtrafficpolicy-response-override-owner-pattern.md new file mode 100644 index 0000000000..39bf5cfc0c --- /dev/null +++ b/release-notes/current/other_changes/9445-backendtrafficpolicy-response-override-owner-pattern.md @@ -0,0 +1 @@ +Refactored BackendTrafficPolicy merging to use the field-owner pattern already used for SecurityPolicy, resolving the response override body reference against the owning policy's namespace instead of mutating the shared policy's `Spec` before merge.