Backport of fix: use Envoy's default for validate_clusters to fix bre…

…aking routes when some backend clusters don't exist into release/1.19.x (#21621) --------- Co-authored-by: Nitya Dhanushkodi <nitya@hashicorp.com>
hashicorp · Aug 20, 2024 · b996f99 · b996f99
1 parent abefc02
commit b996f99
Show file tree

Hide file tree

Showing 94 changed files with 2,248 additions and 1,671 deletions.
diff --git a/.changelog/21587.txt b/.changelog/21587.txt
@@ -0,0 +1,3 @@
+```release-note:improvement
+Use Envoy's default for a route's validate_clusters option, which is false. This fixes a case where non-existent clusters could cause a route to no longer route to any of its backends, including existing ones.
+```
diff --git a/agent/structs/config_entry_mesh.go b/agent/structs/config_entry_mesh.go
@@ -20,6 +20,14 @@ type MeshConfigEntry struct {
 	// MutualTLSMode=permissive in either service-defaults or proxy-defaults.
 	AllowEnablingPermissiveMutualTLS bool `json:",omitempty" alias:"allow_enabling_permissive_mutual_tls"`
 
+	// ValidateClusters controls whether the clusters the route table refers to are validated. The default value is
+	// false. When set to false and a route refers to a cluster that does not exist, the route table loads and routing
+	// to a non-existent cluster results in a 404. When set to true and the route is set to a cluster that do not exist,
+	// the route table will not load. For more information, refer to
+	// [HTTP route configuration in the Envoy docs](https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route.proto#envoy-v3-api-field-config-route-v3-routeconfiguration-validate-clusters)
+	// for more details.
+	ValidateClusters bool `json:",omitempty" alias:"validate_clusters"`
+
 	TLS *MeshTLSConfig `json:",omitempty"`
 
 	HTTP *MeshHTTPConfig `json:",omitempty"`

diff --git a/agent/xds/resources_test.go b/agent/xds/resources_test.go
@@ -625,6 +625,16 @@ func getConnectProxyDiscoChainTests(enterprise bool) []goldenTestCase {
 			},
 			alsoRunTestForV2: true,
 		},
+		{
+			name: "connect-proxy-with-chain-and-splitter-and-mesh-validate-clusters",
+			create: func(t testinf.T) *proxycfg.ConfigSnapshot {
+				cfgSnap := proxycfg.TestConfigSnapshotDiscoveryChain(t, "chain-and-splitter", enterprise, nil, nil)
+				cfgSnap.ConnectProxy.MeshConfig = &structs.MeshConfigEntry{
+					ValidateClusters: true,
+				}
+				return cfgSnap
+			},
+		},
 		{
 			name: "connect-proxy-with-grpc-router",
 			create: func(t testinf.T) *proxycfg.ConfigSnapshot {

diff --git a/agent/xds/routes.go b/agent/xds/routes.go
@@ -51,10 +51,19 @@ func (s *ResourceGenerator) routesFromSnapshot(cfgSnap *proxycfg.ConfigSnapshot)
 	}
 }
 
+func meshValidateClusters(cfgSnap *proxycfg.ConfigSnapshot) bool {
+	validate := false
+	if mesh := cfgSnap.MeshConfig(); mesh != nil {
+		validate = mesh.ValidateClusters
+	}
+	return validate
+}
+
 // routesFromSnapshotConnectProxy returns the xDS API representation of the
 // "routes" in the snapshot.
 func (s *ResourceGenerator) routesForConnectProxy(cfgSnap *proxycfg.ConfigSnapshot) ([]proto.Message, error) {
 	var resources []proto.Message
+	validateClusters := meshValidateClusters(cfgSnap)
 	for uid, chain := range cfgSnap.ConnectProxy.DiscoveryChain {
 		if chain.Default {
 			continue
@@ -76,10 +85,9 @@ func (s *ResourceGenerator) routesForConnectProxy(cfgSnap *proxycfg.ConfigSnapsh
 		route := &envoy_route_v3.RouteConfiguration{
 			Name:         uid.EnvoyID(),
 			VirtualHosts: []*envoy_route_v3.VirtualHost{virtualHost},
-			// ValidateClusters defaults to true when defined statically and false
-			// when done via RDS. Re-set the reasonable value of true to prevent
-			// null-routing traffic.
-			ValidateClusters: response.MakeBoolValue(true),
+		}
+		if validateClusters {
+			route.ValidateClusters = response.MakeBoolValue(true)
 		}
 		resources = append(resources, route)
 	}
@@ -112,7 +120,7 @@ func (s *ResourceGenerator) routesForConnectProxy(cfgSnap *proxycfg.ConfigSnapsh
 	}
 
 	for routeName, clusters := range addressesMap {
-		routes, err := s.makeRoutesForAddresses(routeName, clusters)
+		routes, err := s.makeRoutesForAddresses(routeName, clusters, validateClusters)
 		if err != nil {
 			return nil, err
 		}
@@ -125,10 +133,10 @@ func (s *ResourceGenerator) routesForConnectProxy(cfgSnap *proxycfg.ConfigSnapsh
 	return resources, nil
 }
 
-func (s *ResourceGenerator) makeRoutesForAddresses(routeName string, addresses map[string]string) ([]proto.Message, error) {
+func (s *ResourceGenerator) makeRoutesForAddresses(routeName string, addresses map[string]string, validateClusters bool) ([]proto.Message, error) {
 	var resources []proto.Message
 
-	route, err := makeNamedAddressesRoute(routeName, addresses)
+	route, err := makeNamedAddressesRoute(routeName, addresses, validateClusters)
 	if err != nil {
 		s.Logger.Error("failed to make route", "cluster", "error", err)
 		return nil, err
@@ -223,7 +231,8 @@ func (s *ResourceGenerator) makeRoutes(
 	if resolver.LoadBalancer != nil {
 		lb = resolver.LoadBalancer
 	}
-	route, err := makeNamedDefaultRouteWithLB(clusterName, lb, resolver.RequestTimeout, autoHostRewrite)
+	validateClusters := meshValidateClusters(cfgSnap)
+	route, err := makeNamedDefaultRouteWithLB(clusterName, lb, resolver.RequestTimeout, autoHostRewrite, validateClusters)
 	if err != nil {
 		s.Logger.Error("failed to make route", "cluster", clusterName, "error", err)
 		return nil, err
@@ -233,7 +242,7 @@ func (s *ResourceGenerator) makeRoutes(
 	// If there is a service-resolver for this service then also setup routes for each subset
 	for name := range resolver.Subsets {
 		clusterName = connect.ServiceSNI(svc.Name, name, svc.NamespaceOrDefault(), svc.PartitionOrDefault(), cfgSnap.Datacenter, cfgSnap.Roots.TrustDomain)
-		route, err := makeNamedDefaultRouteWithLB(clusterName, lb, resolver.RequestTimeout, autoHostRewrite)
+		route, err := makeNamedDefaultRouteWithLB(clusterName, lb, resolver.RequestTimeout, autoHostRewrite, validateClusters)
 		if err != nil {
 			s.Logger.Error("failed to make route", "cluster", clusterName, "error", err)
 			return nil, err
@@ -276,18 +285,17 @@ func (s *ResourceGenerator) routesForMeshGateway(cfgSnap *proxycfg.ConfigSnapsho
 		route := &envoy_route_v3.RouteConfiguration{
 			Name:         uid.EnvoyID(),
 			VirtualHosts: []*envoy_route_v3.VirtualHost{virtualHost},
-			// ValidateClusters defaults to true when defined statically and false
-			// when done via RDS. Re-set the reasonable value of true to prevent
-			// null-routing traffic.
-			ValidateClusters: response.MakeBoolValue(true),
+		}
+		if meshValidateClusters(cfgSnap) {
+			route.ValidateClusters = response.MakeBoolValue(true)
 		}
 		resources = append(resources, route)
 	}
 
 	return resources, nil
 }
 
-func makeNamedDefaultRouteWithLB(clusterName string, lb *structs.LoadBalancer, timeout time.Duration, autoHostRewrite bool) (*envoy_route_v3.RouteConfiguration, error) {
+func makeNamedDefaultRouteWithLB(clusterName string, lb *structs.LoadBalancer, timeout time.Duration, autoHostRewrite bool, validateClusters bool) (*envoy_route_v3.RouteConfiguration, error) {
 	action := makeRouteActionFromName(clusterName)
 
 	if err := injectLBToRouteAction(lb, action.Route); err != nil {
@@ -305,7 +313,7 @@ func makeNamedDefaultRouteWithLB(clusterName string, lb *structs.LoadBalancer, t
 		action.Route.Timeout = durationpb.New(timeout)
 	}
 
-	return &envoy_route_v3.RouteConfiguration{
+	route := &envoy_route_v3.RouteConfiguration{
 		Name: clusterName,
 		VirtualHosts: []*envoy_route_v3.VirtualHost{
 			{
@@ -319,20 +327,19 @@ func makeNamedDefaultRouteWithLB(clusterName string, lb *structs.LoadBalancer, t
 				},
 			},
 		},
-		// ValidateClusters defaults to true when defined statically and false
-		// when done via RDS. Re-set the reasonable value of true to prevent
-		// null-routing traffic.
-		ValidateClusters: response.MakeBoolValue(true),
-	}, nil
+	}
+	if validateClusters {
+		route.ValidateClusters = response.MakeBoolValue(true)
+	}
+	return route, nil
 }
 
-func makeNamedAddressesRoute(routeName string, addresses map[string]string) (*envoy_route_v3.RouteConfiguration, error) {
+func makeNamedAddressesRoute(routeName string, addresses map[string]string, validateClusters bool) (*envoy_route_v3.RouteConfiguration, error) {
 	route := &envoy_route_v3.RouteConfiguration{
 		Name: routeName,
-		// ValidateClusters defaults to true when defined statically and false
-		// when done via RDS. Re-set the reasonable value of true to prevent
-		// null-routing traffic.
-		ValidateClusters: response.MakeBoolValue(true),
+	}
+	if validateClusters {
+		route.ValidateClusters = response.MakeBoolValue(true)
 	}
 	for clusterName, address := range addresses {
 		action := makeRouteActionFromName(clusterName)
@@ -371,10 +378,10 @@ func (s *ResourceGenerator) routesForIngressGateway(cfgSnap *proxycfg.ConfigSnap
 		// don't have custom filter chains and routes to this.
 		defaultRoute := &envoy_route_v3.RouteConfiguration{
 			Name: listenerKey.RouteName(),
-			// ValidateClusters defaults to true when defined statically and false
-			// when done via RDS. Re-set the reasonable value of true to prevent
-			// null-routing traffic.
-			ValidateClusters: response.MakeBoolValue(true),
+		}
+		validateClusters := meshValidateClusters(cfgSnap)
+		if validateClusters {
+			defaultRoute.ValidateClusters = response.MakeBoolValue(true)
 		}
 
 		for _, u := range upstreams {
@@ -422,9 +429,11 @@ func (s *ResourceGenerator) routesForIngressGateway(cfgSnap *proxycfg.ConfigSnap
 				defaultRoute.VirtualHosts = append(defaultRoute.VirtualHosts, virtualHost)
 			} else {
 				svcRoute := &envoy_route_v3.RouteConfiguration{
-					Name:             svcRouteName,
-					ValidateClusters: response.MakeBoolValue(true),
-					VirtualHosts:     []*envoy_route_v3.VirtualHost{virtualHost},
+					Name:         svcRouteName,
+					VirtualHosts: []*envoy_route_v3.VirtualHost{virtualHost},
+				}
+				if validateClusters {
+					svcRoute.ValidateClusters = response.MakeBoolValue(true)
 				}
 				result = append(result, svcRoute)
 			}
@@ -460,10 +469,10 @@ func (s *ResourceGenerator) routesForAPIGateway(cfgSnap *proxycfg.ConfigSnapshot
 
 		listenerRoute := &envoy_route_v3.RouteConfiguration{
 			Name: readyListener.listenerKey.RouteName(),
-			// ValidateClusters defaults to true when defined statically and false
-			// when done via RDS. Re-set the reasonable value of true to prevent
-			// null-routing traffic.
-			ValidateClusters: response.MakeBoolValue(true),
+		}
+		validateClusters := meshValidateClusters(cfgSnap)
+		if validateClusters {
+			listenerRoute.ValidateClusters = response.MakeBoolValue(true)
 		}
 
 		// Consolidate all routes for this listener into the minimum possible set based on hostname matching.

diff --git a/.../builtin_extension/routes/lambda-terminating-gateway-with-service-resolvers.latest.golden b/.../builtin_extension/routes/lambda-terminating-gateway-with-service-resolvers.latest.golden
@@ -4,7 +4,6 @@
     {
       "@type": "type.googleapis.com/envoy.config.route.v3.RouteConfiguration",
       "name": "canary1.web.default.dc1.internal.11111111-2222-3333-4444-555555555555.consul",
-      "validateClusters": true,
       "virtualHosts": [
         {
           "domains": [
@@ -27,7 +26,6 @@
     {
       "@type": "type.googleapis.com/envoy.config.route.v3.RouteConfiguration",
       "name": "canary2.web.default.dc1.internal.11111111-2222-3333-4444-555555555555.consul",
-      "validateClusters": true,
       "virtualHosts": [
         {
           "domains": [
@@ -50,7 +48,6 @@
     {
       "@type": "type.googleapis.com/envoy.config.route.v3.RouteConfiguration",
       "name": "web.default.dc1.internal.11111111-2222-3333-4444-555555555555.consul",
-      "validateClusters": true,
       "virtualHosts": [
         {
           "domains": [

diff --git a/agent/xds/testdata/builtin_extension/routes/lambda-terminating-gateway.latest.golden b/agent/xds/testdata/builtin_extension/routes/lambda-terminating-gateway.latest.golden
@@ -4,7 +4,6 @@
     {
       "@type": "type.googleapis.com/envoy.config.route.v3.RouteConfiguration",
       "name": "web.default.dc1.internal.11111111-2222-3333-4444-555555555555.consul",
-      "validateClusters": true,
       "virtualHosts": [
         {
           "domains": [

diff --git a/...ata/builtin_extension/routes/lua-outbound-applies-to-local-upstreams-tproxy.latest.golden b/...ata/builtin_extension/routes/lua-outbound-applies-to-local-upstreams-tproxy.latest.golden
@@ -4,7 +4,6 @@
     {
       "@type": "type.googleapis.com/envoy.config.route.v3.RouteConfiguration",
       "name": "destination.443.~http.default.dc1.internal.11111111-2222-3333-4444-555555555555.consul",
-      "validateClusters": true,
       "virtualHosts": [
         {
           "domains": [
@@ -27,7 +26,6 @@
     {
       "@type": "type.googleapis.com/envoy.config.route.v3.RouteConfiguration",
       "name": "destination.9093.~http.default.dc1.internal.11111111-2222-3333-4444-555555555555.consul",
-      "validateClusters": true,
       "virtualHosts": [
         {
           "domains": [

diff --git a/..._extension/routes/propertyoverride-patch-specific-upstream-service-splitter.latest.golden b/..._extension/routes/propertyoverride-patch-specific-upstream-service-splitter.latest.golden
@@ -5,7 +5,6 @@
       "@type": "type.googleapis.com/envoy.config.route.v3.RouteConfiguration",
       "mostSpecificHeaderMutationsWins": true,
       "name": "db",
-      "validateClusters": true,
       "virtualHosts": [
         {
           "domains": [

diff --git a/...a/builtin_extension/routes/propertyoverride-patch-specific-upstream-service.latest.golden b/...a/builtin_extension/routes/propertyoverride-patch-specific-upstream-service.latest.golden
@@ -22,8 +22,7 @@
             }
           ]
         }
-      ],
-      "validateClusters":  true
+      ]
     }
   ],
   "typeUrl":  "type.googleapis.com/envoy.config.route.v3.RouteConfiguration",