diff --git a/internal/gatewayapi/backendtrafficpolicy.go b/internal/gatewayapi/backendtrafficpolicy.go index 933ba491fb..aef76ed05d 100644 --- a/internal/gatewayapi/backendtrafficpolicy.go +++ b/internal/gatewayapi/backendtrafficpolicy.go @@ -261,6 +261,14 @@ func (t *Translator) ProcessBackendTrafficPolicies( handledPolicies := make(map[types.NamespacedName]*egv1a1.BackendTrafficPolicy, policyMapSize) + // Tracks policies that were successfully attached to at least one real target + // during this pass. Any policy that ends up in res without attaching to a + // target has carried-over (deep-copied) status from a previous generation that + // is now stale, so we drop that status below. Cleaning up the resulting + // detached status on the actual object is handled by the provider status + // updater when it observes the status delete (see internal/provider/kubernetes/status.go). + attachedPolicies := make(map[types.NamespacedName]bool, policyMapSize) + // Translate // 1. First translate Policies targeting RouteRules // 2. Next translate Policies targeting xRoutes @@ -284,8 +292,10 @@ func (t *Translator) ProcessBackendTrafficPolicies( res = append(res, policy) } - t.processBackendTrafficPolicyForRoute(xdsIR, - routeMap, gatewayRouteMap, gatewayPolicyMerged, gatewayPolicyMap, policy, currTarget) + if t.processBackendTrafficPolicyForRoute(xdsIR, + routeMap, gatewayRouteMap, gatewayPolicyMerged, gatewayPolicyMap, policy, currTarget) { + attachedPolicies[policyName] = true + } } } } @@ -310,8 +320,10 @@ func (t *Translator) ProcessBackendTrafficPolicies( res = append(res, policy) } - t.processBackendTrafficPolicyForRoute(xdsIR, - routeMap, gatewayRouteMap, gatewayPolicyMerged, gatewayPolicyMap, policy, currTarget) + if t.processBackendTrafficPolicyForRoute(xdsIR, + routeMap, gatewayRouteMap, gatewayPolicyMerged, gatewayPolicyMap, policy, currTarget) { + attachedPolicies[policyName] = true + } } } } @@ -329,8 +341,10 @@ func (t *Translator) ProcessBackendTrafficPolicies( handledPolicies[policyName] = policy res = append(res, policy) } - t.processBackendTrafficPolicyForGateway(xdsIR, - gatewayMap, gatewayRouteMap, gatewayPolicyMerged, policy, currTarget) + if t.processBackendTrafficPolicyForGateway(xdsIR, + gatewayMap, gatewayRouteMap, gatewayPolicyMerged, policy, currTarget) { + attachedPolicies[policyName] = true + } } } } @@ -354,12 +368,30 @@ func (t *Translator) ProcessBackendTrafficPolicies( handledPolicies[policyName] = policy res = append(res, policy) } - t.processBackendTrafficPolicyForGateway(xdsIR, - gatewayMap, gatewayRouteMap, gatewayPolicyMerged, policy, currTarget) + if t.processBackendTrafficPolicyForGateway(xdsIR, + gatewayMap, gatewayRouteMap, gatewayPolicyMerged, policy, currTarget) { + attachedPolicies[policyName] = true + } } } } + // Drop stale carried-over status for policies that were added to res (their + // targetRef/targetRefs resolved to an object reference) but did not actually + // attach to any target this pass because the referenced object does not exist + // (#8926). Without this, the deep-copied Accepted=True condition with an + // out-of-date observedGeneration would be republished as-is. Emptying the + // ancestor list makes the gatewayapi runner treat the policy as having no + // status, which deletes it from the status store and lets the provider status + // updater clean up the stale status on the object. Policies whose selectors + // matched nothing (#8927) are never added to res, so they are already handled + // by that same delete path. + for _, policy := range res { + if !attachedPolicies[utils.NamespacedName(policy)] { + policy.Status.Ancestors = nil + } + } + for _, policy := range res { // Truncate Ancestor list of longer than 16 if len(policy.Status.Ancestors) > 16 { @@ -429,7 +461,7 @@ func (t *Translator) processBackendTrafficPolicyForRoute( gatewayPolicyMap map[NamespacedNameWithSection]*egv1a1.BackendTrafficPolicy, policy *egv1a1.BackendTrafficPolicy, currTarget policyTargetReferenceWithSectionName, -) { +) bool { var ( targetedRoute RouteContext resolveErr *status.PolicyResolveError @@ -441,7 +473,7 @@ func (t *Translator) processBackendTrafficPolicyForRoute( // reconciled by multiple controllers. And the other controller may // have the target route. if targetedRoute == nil { - return + return false } // Find the Gateway that the route belongs to and add it to the @@ -491,7 +523,7 @@ func (t *Translator) processBackendTrafficPolicyForRoute( policy.Generation, resolveErr, ) - return + return true } if policy.Spec.MergeType == nil { @@ -593,7 +625,7 @@ func (t *Translator) processBackendTrafficPolicyForRoute( // Check if this policy is overridden by other policies targeting at route rule levels // If policy target is route rule, we can skip the check if currTarget.SectionName != nil { - return + return true } key := policyTargetRouteKey{ @@ -613,6 +645,8 @@ func (t *Translator) processBackendTrafficPolicyForRoute( policy.Generation, ) } + + return true } func (t *Translator) processBackendTrafficPolicyForGateway( @@ -622,7 +656,7 @@ func (t *Translator) processBackendTrafficPolicyForGateway( gatewayPolicyMergedMap *GatewayPolicyRouteMap, policy *egv1a1.BackendTrafficPolicy, currTarget policyTargetReferenceWithSectionName, -) { +) bool { var ( targetedGateway *GatewayContext resolveErr *status.PolicyResolveError @@ -631,7 +665,7 @@ func (t *Translator) processBackendTrafficPolicyForGateway( // Negative statuses have already been assigned so it's safe to skip targetedGateway, resolveErr = resolveBackendTrafficPolicyGatewayTargetRef(currTarget, gatewayMap) if targetedGateway == nil { - return + return false } // Find its ancestor reference by resolved gateway, even with resolve error @@ -646,7 +680,7 @@ func (t *Translator) processBackendTrafficPolicyForGateway( policy.Generation, resolveErr, ) - return + return true } // Set conditions for translation error if it got any @@ -692,6 +726,8 @@ func (t *Translator) processBackendTrafficPolicyForGateway( policy.Generation, ) } + + return true } func resolveBackendTrafficPolicyGatewayTargetRef( diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-selector-no-match.in.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-selector-no-match.in.yaml new file mode 100644 index 0000000000..60d4ca7fa7 --- /dev/null +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-selector-no-match.in.yaml @@ -0,0 +1,50 @@ +gateways: +- apiVersion: gateway.networking.k8s.io/v1 + kind: Gateway + metadata: + namespace: envoy-gateway + name: gateway-1 + spec: + gatewayClassName: envoy-gateway-class + listeners: + - name: listener-1 + protocol: HTTP + port: 8081 + allowedRoutes: + namespaces: + from: All +httpRoutes: +- apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + namespace: default + name: httproute-1 + labels: + app: real-label + spec: + parentRefs: + - namespace: envoy-gateway + name: gateway-1 + sectionName: listener-1 + rules: + - matches: + - path: + value: "/foo" + backendRefs: + - name: service-1 + port: 8080 +backendTrafficPolicies: +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: BackendTrafficPolicy + metadata: + namespace: default + name: policy-with-unmatched-selector + spec: + targetSelectors: + - group: gateway.networking.k8s.io + kind: HTTPRoute + matchLabels: + app: no-such-label + timeout: + tcp: + connectTimeout: 15s diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-selector-no-match.out.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-selector-no-match.out.yaml new file mode 100644 index 0000000000..1d49f4ee6f --- /dev/null +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-selector-no-match.out.yaml @@ -0,0 +1,174 @@ +gateways: +- apiVersion: gateway.networking.k8s.io/v1 + kind: Gateway + metadata: + name: gateway-1 + namespace: envoy-gateway + spec: + gatewayClassName: envoy-gateway-class + listeners: + - allowedRoutes: + namespaces: + from: All + name: listener-1 + port: 8081 + 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: listener-1 + 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: + labels: + app: real-label + name: httproute-1 + namespace: default + spec: + parentRefs: + - name: gateway-1 + namespace: envoy-gateway + sectionName: listener-1 + rules: + - backendRefs: + - name: service-1 + port: 8080 + matches: + - path: + value: /foo + 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: + name: gateway-1 + namespace: envoy-gateway + sectionName: listener-1 +infraIR: + envoy-gateway/gateway-1: + proxy: + listeners: + - name: envoy-gateway/gateway-1/listener-1 + ports: + - containerPort: 8081 + name: http-8081 + protocol: HTTP + servicePort: 8081 + metadata: + labels: + gateway.envoyproxy.io/owning-gateway-name: gateway-1 + gateway.envoyproxy.io/owning-gateway-namespace: envoy-gateway + ownerReference: + kind: GatewayClass + name: envoy-gateway-class + name: envoy-gateway/gateway-1 + namespace: envoy-gateway-system +xdsIR: + envoy-gateway/gateway-1: + accessLog: + json: + - path: /dev/stdout + globalResources: + proxyServiceCluster: + metadata: + kind: Service + name: envoy-envoy-gateway-gateway-1-196ae069 + namespace: envoy-gateway-system + sectionName: "8080" + name: envoy-gateway/gateway-1 + settings: + - addressType: IP + endpoints: + - host: 7.6.5.4 + port: 8080 + zone: zone1 + metadata: + kind: Service + name: envoy-envoy-gateway-gateway-1-196ae069 + namespace: envoy-gateway-system + sectionName: "8080" + name: envoy-gateway/gateway-1 + protocol: TCP + http: + - address: 0.0.0.0 + externalPort: 8081 + hostnames: + - '*' + metadata: + kind: Gateway + name: gateway-1 + namespace: envoy-gateway + sectionName: listener-1 + name: envoy-gateway/gateway-1/listener-1 + path: + escapedSlashesAction: UnescapeAndRedirect + mergeSlashes: true + port: 8081 + routes: + - destination: + metadata: + kind: HTTPRoute + name: httproute-1 + namespace: default + name: httproute/default/httproute-1/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/httproute-1/rule/0/backend/0 + protocol: HTTP + weight: 1 + hostname: '*' + isHTTP2: false + metadata: + kind: HTTPRoute + name: httproute-1 + namespace: default + name: httproute/default/httproute-1/rule/0/match/0/* + pathMatch: + distinct: false + name: "" + prefix: /foo + readyListener: + address: 0.0.0.0 + ipFamily: IPv4 + path: /ready + port: 19003 diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-targetref-not-found-stale-status.in.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-targetref-not-found-stale-status.in.yaml new file mode 100644 index 0000000000..8e9424ae7c --- /dev/null +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-targetref-not-found-stale-status.in.yaml @@ -0,0 +1,63 @@ +gateways: +- apiVersion: gateway.networking.k8s.io/v1 + kind: Gateway + metadata: + namespace: envoy-gateway + name: gateway-1 + spec: + gatewayClassName: envoy-gateway-class + listeners: + - name: listener-1 + protocol: HTTP + port: 8081 + allowedRoutes: + namespaces: + from: All +httpRoutes: +- apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + namespace: default + name: httproute-1 + spec: + parentRefs: + - namespace: envoy-gateway + name: gateway-1 + sectionName: listener-1 + rules: + - matches: + - path: + value: "/foo" + backendRefs: + - name: service-1 + port: 8080 +backendTrafficPolicies: +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: BackendTrafficPolicy + metadata: + namespace: default + name: policy-was-attached-now-missing + generation: 2 + spec: + targetRef: + group: gateway.networking.k8s.io + kind: HTTPRoute + name: does-not-exist + timeout: + tcp: + connectTimeout: 15s + status: + ancestors: + - ancestorRef: + group: gateway.networking.k8s.io + kind: Gateway + name: gateway-1 + namespace: envoy-gateway + conditions: + - lastTransitionTime: null + message: Policy has been accepted. + observedGeneration: 1 + reason: Accepted + status: "True" + type: Accepted + controllerName: gateway.envoyproxy.io/gatewayclass-controller diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-targetref-not-found-stale-status.out.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-targetref-not-found-stale-status.out.yaml new file mode 100644 index 0000000000..b4be5b066d --- /dev/null +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-targetref-not-found-stale-status.out.yaml @@ -0,0 +1,189 @@ +backendTrafficPolicies: +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: BackendTrafficPolicy + metadata: + generation: 2 + name: policy-was-attached-now-missing + namespace: default + spec: + targetRef: + group: gateway.networking.k8s.io + kind: HTTPRoute + name: does-not-exist + timeout: + tcp: + connectTimeout: 15s + status: + ancestors: null +gateways: +- apiVersion: gateway.networking.k8s.io/v1 + kind: Gateway + metadata: + name: gateway-1 + namespace: envoy-gateway + spec: + gatewayClassName: envoy-gateway-class + listeners: + - allowedRoutes: + namespaces: + from: All + name: listener-1 + port: 8081 + 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: listener-1 + 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: httproute-1 + namespace: default + spec: + parentRefs: + - name: gateway-1 + namespace: envoy-gateway + sectionName: listener-1 + rules: + - backendRefs: + - name: service-1 + port: 8080 + matches: + - path: + value: /foo + 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: + name: gateway-1 + namespace: envoy-gateway + sectionName: listener-1 +infraIR: + envoy-gateway/gateway-1: + proxy: + listeners: + - name: envoy-gateway/gateway-1/listener-1 + ports: + - containerPort: 8081 + name: http-8081 + protocol: HTTP + servicePort: 8081 + metadata: + labels: + gateway.envoyproxy.io/owning-gateway-name: gateway-1 + gateway.envoyproxy.io/owning-gateway-namespace: envoy-gateway + ownerReference: + kind: GatewayClass + name: envoy-gateway-class + name: envoy-gateway/gateway-1 + namespace: envoy-gateway-system +xdsIR: + envoy-gateway/gateway-1: + accessLog: + json: + - path: /dev/stdout + globalResources: + proxyServiceCluster: + metadata: + kind: Service + name: envoy-envoy-gateway-gateway-1-196ae069 + namespace: envoy-gateway-system + sectionName: "8080" + name: envoy-gateway/gateway-1 + settings: + - addressType: IP + endpoints: + - host: 7.6.5.4 + port: 8080 + zone: zone1 + metadata: + kind: Service + name: envoy-envoy-gateway-gateway-1-196ae069 + namespace: envoy-gateway-system + sectionName: "8080" + name: envoy-gateway/gateway-1 + protocol: TCP + http: + - address: 0.0.0.0 + externalPort: 8081 + hostnames: + - '*' + metadata: + kind: Gateway + name: gateway-1 + namespace: envoy-gateway + sectionName: listener-1 + name: envoy-gateway/gateway-1/listener-1 + path: + escapedSlashesAction: UnescapeAndRedirect + mergeSlashes: true + port: 8081 + routes: + - destination: + metadata: + kind: HTTPRoute + name: httproute-1 + namespace: default + name: httproute/default/httproute-1/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/httproute-1/rule/0/backend/0 + protocol: HTTP + weight: 1 + hostname: '*' + isHTTP2: false + metadata: + kind: HTTPRoute + name: httproute-1 + namespace: default + name: httproute/default/httproute-1/rule/0/match/0/* + pathMatch: + distinct: false + name: "" + prefix: /foo + readyListener: + address: 0.0.0.0 + ipFamily: IPv4 + path: /ready + port: 19003 diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-targetref-not-found.in.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-targetref-not-found.in.yaml new file mode 100644 index 0000000000..341ccd2e12 --- /dev/null +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-targetref-not-found.in.yaml @@ -0,0 +1,47 @@ +gateways: +- apiVersion: gateway.networking.k8s.io/v1 + kind: Gateway + metadata: + namespace: envoy-gateway + name: gateway-1 + spec: + gatewayClassName: envoy-gateway-class + listeners: + - name: listener-1 + protocol: HTTP + port: 8081 + allowedRoutes: + namespaces: + from: All +httpRoutes: +- apiVersion: gateway.networking.k8s.io/v1 + kind: HTTPRoute + metadata: + namespace: default + name: httproute-1 + spec: + parentRefs: + - namespace: envoy-gateway + name: gateway-1 + sectionName: listener-1 + rules: + - matches: + - path: + value: "/foo" + backendRefs: + - name: service-1 + port: 8080 +backendTrafficPolicies: +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: BackendTrafficPolicy + metadata: + namespace: default + name: policy-for-nonexistent-route + spec: + targetRef: + group: gateway.networking.k8s.io + kind: HTTPRoute + name: does-not-exist + timeout: + tcp: + connectTimeout: 15s diff --git a/internal/gatewayapi/testdata/backendtrafficpolicy-targetref-not-found.out.yaml b/internal/gatewayapi/testdata/backendtrafficpolicy-targetref-not-found.out.yaml new file mode 100644 index 0000000000..1e30ca215a --- /dev/null +++ b/internal/gatewayapi/testdata/backendtrafficpolicy-targetref-not-found.out.yaml @@ -0,0 +1,188 @@ +backendTrafficPolicies: +- apiVersion: gateway.envoyproxy.io/v1alpha1 + kind: BackendTrafficPolicy + metadata: + name: policy-for-nonexistent-route + namespace: default + spec: + targetRef: + group: gateway.networking.k8s.io + kind: HTTPRoute + name: does-not-exist + timeout: + tcp: + connectTimeout: 15s + status: + ancestors: null +gateways: +- apiVersion: gateway.networking.k8s.io/v1 + kind: Gateway + metadata: + name: gateway-1 + namespace: envoy-gateway + spec: + gatewayClassName: envoy-gateway-class + listeners: + - allowedRoutes: + namespaces: + from: All + name: listener-1 + port: 8081 + 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: listener-1 + 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: httproute-1 + namespace: default + spec: + parentRefs: + - name: gateway-1 + namespace: envoy-gateway + sectionName: listener-1 + rules: + - backendRefs: + - name: service-1 + port: 8080 + matches: + - path: + value: /foo + 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: + name: gateway-1 + namespace: envoy-gateway + sectionName: listener-1 +infraIR: + envoy-gateway/gateway-1: + proxy: + listeners: + - name: envoy-gateway/gateway-1/listener-1 + ports: + - containerPort: 8081 + name: http-8081 + protocol: HTTP + servicePort: 8081 + metadata: + labels: + gateway.envoyproxy.io/owning-gateway-name: gateway-1 + gateway.envoyproxy.io/owning-gateway-namespace: envoy-gateway + ownerReference: + kind: GatewayClass + name: envoy-gateway-class + name: envoy-gateway/gateway-1 + namespace: envoy-gateway-system +xdsIR: + envoy-gateway/gateway-1: + accessLog: + json: + - path: /dev/stdout + globalResources: + proxyServiceCluster: + metadata: + kind: Service + name: envoy-envoy-gateway-gateway-1-196ae069 + namespace: envoy-gateway-system + sectionName: "8080" + name: envoy-gateway/gateway-1 + settings: + - addressType: IP + endpoints: + - host: 7.6.5.4 + port: 8080 + zone: zone1 + metadata: + kind: Service + name: envoy-envoy-gateway-gateway-1-196ae069 + namespace: envoy-gateway-system + sectionName: "8080" + name: envoy-gateway/gateway-1 + protocol: TCP + http: + - address: 0.0.0.0 + externalPort: 8081 + hostnames: + - '*' + metadata: + kind: Gateway + name: gateway-1 + namespace: envoy-gateway + sectionName: listener-1 + name: envoy-gateway/gateway-1/listener-1 + path: + escapedSlashesAction: UnescapeAndRedirect + mergeSlashes: true + port: 8081 + routes: + - destination: + metadata: + kind: HTTPRoute + name: httproute-1 + namespace: default + name: httproute/default/httproute-1/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/httproute-1/rule/0/backend/0 + protocol: HTTP + weight: 1 + hostname: '*' + isHTTP2: false + metadata: + kind: HTTPRoute + name: httproute-1 + namespace: default + name: httproute/default/httproute-1/rule/0/match/0/* + pathMatch: + distinct: false + name: "" + prefix: /foo + readyListener: + address: 0.0.0.0 + ipFamily: IPv4 + path: /ready + port: 19003 diff --git a/internal/provider/kubernetes/status.go b/internal/provider/kubernetes/status.go index 438c917be2..011a32a429 100644 --- a/internal/provider/kubernetes/status.go +++ b/internal/provider/kubernetes/status.go @@ -413,12 +413,18 @@ func (r *gatewayAPIReconciler) updateStatusFromSubscriptions(ctx context.Context message.Metadata{Runner: string(egv1a1.LogComponentProviderRunner), Message: message.BackendTrafficPolicyStatusMessageName}, r.subscriptions.backendTrafficPolicyStatuses, func(update message.Update[types.NamespacedName, *gwapiv1.PolicyStatus], errChan chan error) { - // skip delete updates. + key := update.Key + val := update.Value + + // A delete means the translator no longer produces status for this + // policy (e.g. its targetRef points at a nonexistent object, or its + // targetSelectors stopped matching anything). Clean up any stale + // status this controller previously wrote so it is not left behind. if update.Delete { + r.statusUpdater.Send(r.backendTrafficPolicyStatusCleanupUpdate(key, errChan)) return } - key := update.Key - val := update.Value + r.statusUpdater.Send(Update{ NamespacedName: key, Resource: new(egv1a1.BackendTrafficPolicy), @@ -732,6 +738,48 @@ func mergeRouteParentStatus(ns string, old, new []gwapiv1.RouteParentStatus) []g return merged } +// removePolicyStatusForController returns the ancestor statuses with all entries +// owned by the given controller removed, while preserving ancestors owned by other +// controllers. It is used to clean up stale status for a policy that this controller +// no longer attaches to any target. A nil slice is returned when no ancestors remain +// so the resulting status has an empty (rather than [] with residual) ancestor list. +func removePolicyStatusForController(ancestors []gwapiv1.PolicyAncestorStatus, controller gwapiv1.GatewayController) []gwapiv1.PolicyAncestorStatus { + var remaining []gwapiv1.PolicyAncestorStatus + for _, ancestor := range ancestors { + if ancestor.ControllerName != controller { + remaining = append(remaining, ancestor) + } + } + return remaining +} + +// backendTrafficPolicyStatusCleanupUpdate builds a status Update that strips the +// ancestor conditions owned by this controller from a BackendTrafficPolicy, while +// preserving ancestors owned by other controllers. It is used when the translator +// stops producing status for a policy (its targetRef points at a nonexistent object, +// or its targetSelectors stopped matching anything) so a stale Accepted=True condition +// with an out-of-date observedGeneration is not left behind. If the object itself was +// deleted, the status updater's Get returns NotFound and applying this is a no-op. +func (r *gatewayAPIReconciler) backendTrafficPolicyStatusCleanupUpdate(key types.NamespacedName, errChan chan error) Update { + return Update{ + NamespacedName: key, + Resource: new(egv1a1.BackendTrafficPolicy), + Mutator: MutatorFunc(func(obj client.Object) client.Object { + t, ok := obj.(*egv1a1.BackendTrafficPolicy) + if !ok { + err := fmt.Errorf("unsupported object type %T", obj) + if errChan != nil { + errChan <- err + } + panic(err) + } + tCopy := t.DeepCopy() + tCopy.Status.Ancestors = removePolicyStatusForController(tCopy.Status.Ancestors, r.classController) + return tCopy + }), + } +} + func (r *gatewayAPIReconciler) updateStatusForGateway(ctx context.Context, gtw *gwapiv1.Gateway) { // nil check for unit tests. if r.statusUpdater == nil { diff --git a/internal/provider/kubernetes/status_test.go b/internal/provider/kubernetes/status_test.go index 8c619fbe8e..56477b168e 100644 --- a/internal/provider/kubernetes/status_test.go +++ b/internal/provider/kubernetes/status_test.go @@ -6,11 +6,20 @@ package kubernetes import ( + "context" + "os" "reflect" "testing" + "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" gwapiv1 "sigs.k8s.io/gateway-api/apis/v1" + + egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1" + "github.com/envoyproxy/gateway/internal/envoygateway" + "github.com/envoyproxy/gateway/internal/logging" ) func Test_mergeRouteParentStatus(t *testing.T) { @@ -887,3 +896,159 @@ func Test_mergeRouteParentStatus(t *testing.T) { }) } } + +func Test_removePolicyStatusForController(t *testing.T) { + const ours = gwapiv1.GatewayController("gateway.envoyproxy.io/gatewayclass-controller") + const theirs = gwapiv1.GatewayController("istio.io/gateway-controller") + + ancestor := func(controller gwapiv1.GatewayController, name string) gwapiv1.PolicyAncestorStatus { + return gwapiv1.PolicyAncestorStatus{ + ControllerName: controller, + AncestorRef: gwapiv1.ParentReference{Name: gwapiv1.ObjectName(name)}, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.PolicyConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(gwapiv1.PolicyReasonAccepted), + ObservedGeneration: 1, + }, + }, + } + } + + tests := []struct { + name string + ancestors []gwapiv1.PolicyAncestorStatus + controller gwapiv1.GatewayController + want []gwapiv1.PolicyAncestorStatus + }{ + { + // #8926 / #8927: policy attached to nothing this pass, its only ancestor + // is ours and stale -> it gets cleared so the stale Accepted=True is gone. + name: "single stale ancestor of ours is cleared", + ancestors: []gwapiv1.PolicyAncestorStatus{ancestor(ours, "gateway1")}, + controller: ours, + want: nil, + }, + { + // Multi-controller: only our ancestor is removed, the other controller's + // status is preserved untouched. + name: "only our ancestor removed, other controller preserved", + ancestors: []gwapiv1.PolicyAncestorStatus{ancestor(theirs, "gateway1"), ancestor(ours, "gateway2")}, + controller: ours, + want: []gwapiv1.PolicyAncestorStatus{ancestor(theirs, "gateway1")}, + }, + { + name: "no ancestors of ours leaves status untouched", + ancestors: []gwapiv1.PolicyAncestorStatus{ancestor(theirs, "gateway1")}, + controller: ours, + want: []gwapiv1.PolicyAncestorStatus{ancestor(theirs, "gateway1")}, + }, + { + name: "empty input returns empty", + ancestors: nil, + controller: ours, + want: nil, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := removePolicyStatusForController(tt.ancestors, tt.controller); !reflect.DeepEqual(got, tt.want) { + t.Errorf("removePolicyStatusForController() = %v, want %v", got, tt.want) + } + }) + } +} + +// TestBackendTrafficPolicyStatusCleanupOnDelete exercises the delete-path cleanup +// end-to-end through the real UpdateHandler.apply against a fake client. It covers +// both filed issues (a BackendTrafficPolicy whose target went missing, #8926, and one +// whose selectors stopped matching, #8927) since both surface identically: the object +// carries a stale Accepted=True condition with an out-of-date observedGeneration that +// must be removed once the translator stops producing status for it. +func TestBackendTrafficPolicyStatusCleanupOnDelete(t *testing.T) { + const ours = gwapiv1.GatewayController("gateway.envoyproxy.io/gatewayclass-controller") + const theirs = gwapiv1.GatewayController("istio.io/gateway-controller") + + staleAncestor := func(controller gwapiv1.GatewayController, name string) gwapiv1.PolicyAncestorStatus { + return gwapiv1.PolicyAncestorStatus{ + ControllerName: controller, + AncestorRef: gwapiv1.ParentReference{Name: gwapiv1.ObjectName(name)}, + Conditions: []metav1.Condition{{ + Type: string(gwapiv1.PolicyConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(gwapiv1.PolicyReasonAccepted), + ObservedGeneration: 1, // stale: object generation below is 2 + }}, + } + } + + tests := []struct { + name string + existing *egv1a1.BackendTrafficPolicy + key types.NamespacedName + wantAncestors []gwapiv1.PolicyAncestorStatus + }{ + { + name: "stale status owned solely by our controller is cleared", + existing: &egv1a1.BackendTrafficPolicy{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "btp-1", Generation: 2}, + Status: gwapiv1.PolicyStatus{Ancestors: []gwapiv1.PolicyAncestorStatus{staleAncestor(ours, "gateway-1")}}, + }, + key: types.NamespacedName{Namespace: "default", Name: "btp-1"}, + wantAncestors: nil, + }, + { + name: "only our ancestor is cleared, other controller's status preserved", + existing: &egv1a1.BackendTrafficPolicy{ + ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "btp-2", Generation: 2}, + Status: gwapiv1.PolicyStatus{Ancestors: []gwapiv1.PolicyAncestorStatus{ + staleAncestor(theirs, "gateway-1"), + staleAncestor(ours, "gateway-2"), + }}, + }, + key: types.NamespacedName{Namespace: "default", Name: "btp-2"}, + wantAncestors: []gwapiv1.PolicyAncestorStatus{staleAncestor(theirs, "gateway-1")}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cli := fakeclient.NewClientBuilder(). + WithScheme(envoygateway.GetScheme()). + WithObjects(tt.existing). + WithStatusSubresource(tt.existing). + Build() + + logger := logging.DefaultLogger(os.Stdout, egv1a1.LogLevelInfo) + u := NewUpdateHandler(logger.Logger, cli) + r := &gatewayAPIReconciler{ + log: logger, + classController: ours, + } + + u.apply(r.backendTrafficPolicyStatusCleanupUpdate(tt.key, nil)) + + got := &egv1a1.BackendTrafficPolicy{} + require.NoError(t, cli.Get(context.Background(), tt.key, got)) + require.Equal(t, tt.wantAncestors, got.Status.Ancestors) + }) + } + + t.Run("delete for an already-removed object is a no-op", func(t *testing.T) { + cli := fakeclient.NewClientBuilder().WithScheme(envoygateway.GetScheme()).Build() + logger := logging.DefaultLogger(os.Stdout, egv1a1.LogLevelInfo) + u := NewUpdateHandler(logger.Logger, cli) + r := &gatewayAPIReconciler{ + log: logger, + classController: ours, + } + + // Must not panic or error when the object no longer exists. + u.apply(r.backendTrafficPolicyStatusCleanupUpdate(types.NamespacedName{Namespace: "default", Name: "gone"}, nil)) + + got := &egv1a1.BackendTrafficPolicy{} + err := cli.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: "gone"}, got) + require.Error(t, err) // still not found + }) +} diff --git a/release-notes/current/bug_fixes/8926-btp-status-target-not-found.md b/release-notes/current/bug_fixes/8926-btp-status-target-not-found.md new file mode 100644 index 0000000000..ac84273a7d --- /dev/null +++ b/release-notes/current/bug_fixes/8926-btp-status-target-not-found.md @@ -0,0 +1 @@ +Fixed BackendTrafficPolicy status going stale when the policy stops attaching to any target. A policy whose `targetRef`/`targetRefs` points at a nonexistent object or whose `targetSelectors` no longer match anything now has its stale ancestor status cleaned up instead of retaining an outdated `Accepted=True` condition with an out-of-date `observedGeneration`.