fix: do not return 500 for all requests when part of BackendRefs are invalid (envoyproxy#7488)

* do not return 500 for all requests when part of BackendRefs are invalid

Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: Huabing (Robin) Zhao <[email protected]>
(cherry picked from commit 2899416)
Signed-off-by: Huabing Zhao <[email protected]>

Author: zhaohuabing

internal/gatewayapi/route.go

@@ -233,6 +233,7 @@ func (t *Translator) processHTTPRouteRules(httpRoute *HTTPRouteContext, parentRe
 				BackendRef: &rule.BackendRefs[i].BackendRef,
 				Filters:    rule.BackendRefs[i].Filters,
 			}
+			// ds will never be nil here because processDestination returns an empty DestinationSetting for invalid backendRefs.
 			ds, unstructuredRef, err := t.processDestination(settingName, backendRefCtx, parentRef, httpRoute, resources)
 			if err != nil {
 				// Gateway API conformance: When backendRef Service exists but has no endpoints,
@@ -250,13 +251,12 @@ func (t *Translator) processHTTPRouteRules(httpRoute *HTTPRouteContext, parentRe
 					))
 					processDestinationError = err
 				}
-				continue
 			}
 			if unstructuredRef != nil {
 				backendCustomRefs = append(backendCustomRefs, unstructuredRef)
 			}
-			// ds can be nil if the backendRef weight is 0
-			if ds == nil {
+			// skip backendRefs with weight 0 as they do not affect the traffic distribution
+			if ds.Weight != nil && *ds.Weight == 0 {
 				continue
 			}
 			allDs = append(allDs, ds)
@@ -275,9 +275,9 @@ func (t *Translator) processHTTPRouteRules(httpRoute *HTTPRouteContext, parentRe
 			Metadata: routeRuleMetadata,
 		}
 		switch {
-		// return 500 if any destination setting is invalid
+		// return 500 if no valid destination settings exist
 		// the error is already added to the error list when processing the destination
-		case processDestinationError != nil:
+		case processDestinationError != nil && destination.ToBackendWeights().Valid == 0:
 			routesWithDirectResponse := sets.New[string]()
 			for _, irRoute := range ruleRoutes {
 				// If the route already has a direct response or redirect configured, then it was from a filter so skip
@@ -297,9 +297,9 @@ func (t *Translator) processHTTPRouteRules(httpRoute *HTTPRouteContext, parentRe
 					"error", processDestinationError,
 				)
 			}
-		// return 503 if endpoints does not exist
+		// return 503 if no ready endpoints exist
 		// the error is already added to the error list when processing the destination
-		case failedNoReadyEndpoints && len(allDs) == 0:
+		case failedNoReadyEndpoints && destination.ToBackendWeights().Valid == 0:
 			routesWithDirectResponse := sets.New[string]()
 			for _, irRoute := range ruleRoutes {
 				// If the route already has a direct response or redirect configured, then it was from a filter so skip
@@ -334,8 +334,8 @@ func (t *Translator) processHTTPRouteRules(httpRoute *HTTPRouteContext, parentRe
 				t.Logger.Info("setting 500 direct response in routes due to all valid destinations having 0 weight",
 					"routes", sets.List(routesWithDirectResponse))
 			}
-			// A route can only have one destination if this destination is a dynamic resolver, because the behavior of
-			// multiple destinations with one being a dynamic resolver just doesn't make sense.
+		// A route can only have one destination if this destination is a dynamic resolver, because the behavior of
+		// multiple destinations with one being a dynamic resolver just doesn't make sense.
 		case hasDynamicResolver && len(rule.BackendRefs) > 1:
 			routesWithDirectResponse := sets.New[string]()
 			for _, irRoute := range ruleRoutes {
@@ -388,10 +388,6 @@ func (t *Translator) processHTTPRouteRules(httpRoute *HTTPRouteContext, parentRe
 			}
 		}
 
-		// TODO handle:
-		//	- sum of weights for valid backend refs is 0
-		//	- etc.
-
 		irRoutes = append(irRoutes, ruleRoutes...)
 	}
 	if errorCollector.Empty() {
@@ -826,6 +822,7 @@ func (t *Translator) processGRPCRouteRules(grpcRoute *GRPCRouteContext, parentRe
 				BackendRef: &rule.BackendRefs[i].BackendRef,
 				Filters:    rule.BackendRefs[i].Filters,
 			}
+			// ds will never be nil here because processDestination returns an empty DestinationSetting for invalid backendRefs.
 			ds, _, err := t.processDestination(settingName, backendRefCtx, parentRef, grpcRoute, resources)
 			if err != nil {
 				// Gateway API conformance: When backendRef Service exists but has no endpoints,
@@ -843,10 +840,10 @@ func (t *Translator) processGRPCRouteRules(grpcRoute *GRPCRouteContext, parentRe
 					))
 					processDestinationError = err
 				}
-				continue
 			}
 
-			if ds == nil {
+			// skip backendRefs with weight 0 as they do not affect the traffic distribution
+			if ds.Weight != nil && *ds.Weight == 0 {
 				continue
 			}
 			allDs = append(allDs, ds)
@@ -862,7 +859,7 @@ func (t *Translator) processGRPCRouteRules(grpcRoute *GRPCRouteContext, parentRe
 		switch {
 		// return 500 if any destination setting is invalid
 		// the error is already added to the error list when processing the destination
-		case processDestinationError != nil:
+		case processDestinationError != nil && destination.ToBackendWeights().Valid == 0:
 			routesWithDirectResponse := sets.New[string]()
 			for _, irRoute := range ruleRoutes {
 				// If the route already has a direct response or redirect configured, then it was from a filter so skip
@@ -883,7 +880,7 @@ func (t *Translator) processGRPCRouteRules(grpcRoute *GRPCRouteContext, parentRe
 			}
 		// return 503 if endpoints does not exist
 		// the error is already added to the error list when processing the destination
-		case failedNoReadyEndpoints && len(allDs) == 0:
+		case failedNoReadyEndpoints && destination.ToBackendWeights().Valid == 0:
 			routesWithDirectResponse := sets.New[string]()
 			for _, irRoute := range ruleRoutes {
 				// If the route already has a direct response or redirect configured, then it was from a filter so skip
@@ -945,10 +942,6 @@ func (t *Translator) processGRPCRouteRules(grpcRoute *GRPCRouteContext, parentRe
 			}
 		}
 
-		// TODO handle:
-		//	- sum of weights for valid backend refs is 0
-		//	- etc.
-
 		irRoutes = append(irRoutes, ruleRoutes...)
 	}
 
@@ -1195,12 +1188,14 @@ func (t *Translator) processTLSRouteParentRefs(tlsRoute *TLSRouteContext, resour
 			for i := range rule.BackendRefs {
 				settingName := irDestinationSettingName(destName, i)
 				backendRefCtx := DirectBackendRef{BackendRef: &rule.BackendRefs[i]}
+				// ds will never be nil here because processDestination returns an empty DestinationSetting for invalid backendRefs.
 				ds, _, err := t.processDestination(settingName, backendRefCtx, parentRef, tlsRoute, resources)
 				if err != nil {
 					resolveErrs.Add(err)
 					continue
 				}
-				if ds != nil {
+				// skip backendRefs with weight 0 as they do not affect the traffic distribution
+				if ds.Weight != nil && *ds.Weight > 0 {
 					destSettings = append(destSettings, ds)
 				}
 			}
@@ -1351,14 +1346,15 @@ func (t *Translator) processUDPRouteParentRefs(udpRoute *UDPRouteContext, resour
 		for i := range udpRoute.Spec.Rules[0].BackendRefs {
 			settingName := irDestinationSettingName(destName, i)
 			backendRefCtx := DirectBackendRef{BackendRef: &udpRoute.Spec.Rules[0].BackendRefs[i]}
+			// ds will never be nil here because processDestination returns an empty DestinationSetting for invalid backendRefs.
 			ds, _, err := t.processDestination(settingName, backendRefCtx, parentRef, udpRoute, resources)
 			if err != nil {
 				resolveErrs.Add(err)
 				continue
 			}
 
-			// Skip nil destination settings
-			if ds != nil {
+			// skip backendRefs with weight 0 as they do not affect the traffic distribution
+			if ds.Weight != nil && *ds.Weight > 0 {
 				destSettings = append(destSettings, ds)
 			}
 		}
@@ -1507,8 +1503,8 @@ func (t *Translator) processTCPRouteParentRefs(tcpRoute *TCPRouteContext, resour
 				resolveErrs.Add(err)
 				continue
 			}
-			// Skip nil destination settings
-			if ds != nil {
+			// skip backendRefs with weight 0 as they do not affect the traffic distribution
+			if ds.Weight != nil && *ds.Weight > 0 {
 				destSettings = append(destSettings, ds)
 			}
 		}
@@ -1613,27 +1609,33 @@ func (t *Translator) processTCPRouteParentRefs(tcpRoute *TCPRouteContext, resour
 func (t *Translator) processDestination(name string, backendRefContext BackendRefContext,
 	parentRef *RouteParentContext, route RouteContext, resources *resource.Resources,
 ) (ds *ir.DestinationSetting, unstructuredRef *ir.UnstructuredRef, err status.Error) {
-	routeType := route.GetRouteType()
-	weight := uint32(1)
-	backendRef := backendRefContext.GetBackendRef()
-	if backendRef.Weight != nil {
-		weight = uint32(*backendRef.Weight)
+	var (
+		routeType  = route.GetRouteType()
+		weight     = (uint32(ptr.Deref(backendRefContext.GetBackendRef().Weight, int32(1))))
+		backendRef = backendRefContext.GetBackendRef()
+	)
+
+	// Create an empty DS without endpoints
+	// This represents an invalid DS.
+	emptyDS := &ir.DestinationSetting{
+		Name:   name,
+		Weight: &weight,
 	}
 
 	backendNamespace := NamespaceDerefOr(backendRef.Namespace, route.GetNamespace())
 	if !t.isCustomBackendResource(backendRef.Group, KindDerefOr(backendRef.Kind, resource.KindService)) {
 		err = t.validateBackendRef(backendRefContext, route, resources, backendNamespace, routeType)
 		{
-			// return with empty endpoint means the backend is invalid and an error to fail the associated route.
+			// Empty DS means the backend is invalid and an error to fail the associated route.
 			if err != nil {
-				return nil, nil, err
+				return emptyDS, nil, err
 			}
 		}
 	}
 
 	// Skip processing backends with 0 weight
 	if weight == 0 {
-		return nil, nil, nil
+		return emptyDS, nil, nil
 	}
 
 	var envoyProxy *egv1a1.EnvoyProxy
@@ -1660,19 +1662,19 @@ func (t *Translator) processDestination(name string, backendRefContext BackendRe
 		envoyProxy,
 	)
 	if tlsErr != nil {
-		return nil, nil, status.NewRouteStatusError(tlsErr, status.RouteReasonInvalidBackendTLS)
+		return emptyDS, nil, status.NewRouteStatusError(tlsErr, status.RouteReasonInvalidBackendTLS)
 	}
 
 	switch KindDerefOr(backendRef.Kind, resource.KindService) {
 	case resource.KindServiceImport:
 		ds, err = t.processServiceImportDestinationSetting(name, backendRef.BackendObjectReference, backendNamespace, protocol, resources, envoyProxy)
 		if err != nil {
-			return nil, nil, err
+			return emptyDS, nil, err
 		}
 	case resource.KindService:
 		ds, err = t.processServiceDestinationSetting(name, backendRef.BackendObjectReference, backendNamespace, protocol, resources, envoyProxy)
 		if err != nil {
-			return nil, nil, err
+			return emptyDS, nil, err
 		}
 		svc := resources.GetService(backendNamespace, string(backendRef.Name))
 		ds.IPFamily = getServiceIPFamily(svc)
@@ -1687,7 +1689,7 @@ func (t *Translator) processDestination(name string, backendRefContext BackendRe
 
 			// Check if the custom backend resource was found
 			if unstructuredRef == nil {
-				return nil, nil, status.NewRouteStatusError(
+				return emptyDS, nil, status.NewRouteStatusError(
 					fmt.Errorf("custom backend %s %s/%s not found",
 						KindDerefOr(backendRef.Kind, resource.KindService),
 						backendNamespace,
@@ -1709,11 +1711,11 @@ func (t *Translator) processDestination(name string, backendRefContext BackendRe
 	var filtersErr error
 	ds.Filters, filtersErr = t.processDestinationFilters(routeType, backendRefContext, parentRef, route, resources)
 	if filtersErr != nil {
-		return nil, nil, status.NewRouteStatusError(filtersErr, status.RouteReasonInvalidBackendFilters)
+		return emptyDS, nil, status.NewRouteStatusError(filtersErr, status.RouteReasonInvalidBackendFilters)
 	}
 
 	if err := validateDestinationSettings(ds, t.IsEnvoyServiceRouting(envoyProxy), backendRef.Kind); err != nil {
-		return nil, nil, err
+		return emptyDS, nil, err
 	}
 
 	ds.Weight = &weight

internal/gatewayapi/testdata/grpcroute-with-mixed-invalid-and-valid-backend-refs.in.yaml

@@ -0,0 +1,45 @@
+gateways:
+- apiVersion: gateway.networking.k8s.io/v1
+  kind: Gateway
+  metadata:
+    namespace: envoy-gateway
+    name: gateway-1
+  spec:
+    gatewayClassName: envoy-gateway-class
+    listeners:
+    - name: http
+      protocol: HTTP
+      port: 80
+      allowedRoutes:
+        namespaces:
+          from: All
+grpcRoutes:
+- apiVersion: gateway.networking.k8s.io/v1alpha2
+  kind: GRPCRoute
+  metadata:
+    namespace: default
+    name: grpcroute-1
+  spec:
+    parentRefs:
+    - namespace: envoy-gateway
+      name: gateway-1
+      sectionName: http
+    rules:
+    - matches:
+      - method:
+          method: ExampleExact
+          type: Exact
+      backendRefs:
+      - name: service-1
+        port: 8080
+      - name: service-not-exist
+        port: 8080
+services:
+- apiVersion: v1
+  kind: Service
+  metadata:
+    name: service-1
+  spec:
+    clusterIP: 7.7.7.7
+    ports:
+    - port: 8080