[NET-6305] xds: Ensure v2 route match and protocol are populated for …

…gRPC (#19343) * xds: Ensure v2 route match is populated for gRPC Similar to HTTP, ensure that route match config (which is required by Envoy) is populated when default values are used. Because the default matches generated for gRPC contain a single empty `GRPCRouteMatch`, and that proto does not directly support prefix-based config, an interpretation of the empty struct is needed to generate the same output that the `HTTPRouteMatch` is explicitly configured to provide in internal/mesh/internal/controllers/routes/generate.go. * xds: Ensure protocol set for gRPC resources Add explicit protocol in `ProxyStateTemplate` builders and validate it is always set on clusters. This ensures that HTTP filters and `http2_protocol_options` are populated in all the necessary places for gRPC traffic and prevents future unintended omissions of non-TCP protocols. Co-authored-by: John Murret <john.murret@hashicorp.com> --------- Co-authored-by: John Murret <john.murret@hashicorp.com>
hashicorp · Oct 25, 2023 · a7803bd · a7803bd
1 parent e414cbe
commit a7803bd
Show file tree

Hide file tree

Showing 70 changed files with 1,631 additions and 1,141 deletions.
diff --git a/agent/xds/proxystateconverter/clusters.go b/agent/xds/proxystateconverter/clusters.go
@@ -462,7 +462,7 @@ func (s *Converter) makeAppCluster(cfgSnap *proxycfg.ConfigSnapshot, name, pathP
 	if protocol == "" {
 		protocol = cfg.Protocol
 	}
-	namedCluster.cluster.Protocol = protocol
+	namedCluster.cluster.Protocol = protocolMap[protocol]
 	if cfg.MaxInboundConnections > 0 {
 		namedCluster.cluster.GetEndpointGroup().GetStatic().GetConfig().
 			CircuitBreakers = &pbproxystate.CircuitBreakers{
@@ -646,7 +646,7 @@ func (s *Converter) makeUpstreamClusterForPreparedQuery(upstream structs.Upstrea
 
 	if c == nil {
 		c = &pbproxystate.Cluster{
-			Protocol: cfg.Protocol,
+			Protocol: protocolMap[cfg.Protocol],
 			Group: &pbproxystate.Cluster_EndpointGroup{
 				EndpointGroup: &pbproxystate.EndpointGroup{
 					Group: &pbproxystate.EndpointGroup_Dynamic{
@@ -932,7 +932,7 @@ func (s *Converter) makeUpstreamClustersForDiscoveryChain(
 			} else {
 				cluster := &pbproxystate.Cluster{
 					AltStatName: mappedTargets.baseClusterName,
-					Protocol:    upstreamConfig.Protocol,
+					Protocol:    protocolMap[upstreamConfig.Protocol],
 					Group: &pbproxystate.Cluster_EndpointGroup{
 						EndpointGroup: &pbproxystate.EndpointGroup{
 							Group: &pbproxystate.EndpointGroup_Dynamic{
@@ -952,7 +952,7 @@ func (s *Converter) makeUpstreamClustersForDiscoveryChain(
 			failoverGroup.EndpointGroups = endpointGroups
 			cluster := &pbproxystate.Cluster{
 				AltStatName: mappedTargets.baseClusterName,
-				Protocol:    upstreamConfig.Protocol,
+				Protocol:    protocolMap[upstreamConfig.Protocol],
 				Group: &pbproxystate.Cluster_FailoverGroup{
 					FailoverGroup: failoverGroup,
 				},
@@ -1251,3 +1251,13 @@ func makeOutlierDetection(p *structs.PassiveHealthCheck, override *structs.Passi
 
 	return od
 }
+
+// protocolMap converts config entry protocols to proxystate protocol values.
+// As documented on config entry protos, the valid values are "tcp", "http",
+// "http2" and "grpc". Anything else is treated as tcp.
+var protocolMap = map[string]pbproxystate.Protocol{
+	"http":  pbproxystate.Protocol_PROTOCOL_HTTP,
+	"http2": pbproxystate.Protocol_PROTOCOL_HTTP2,
+	"grpc":  pbproxystate.Protocol_PROTOCOL_GRPC,
+	"tcp":   pbproxystate.Protocol_PROTOCOL_TCP,
+}
diff --git a/agent/xdsv2/cluster_resources.go b/agent/xdsv2/cluster_resources.go
@@ -81,7 +81,7 @@ func (pr *ProxyResources) makeClusters(name string) ([]proto.Message, error) {
 	return clusters, nil
 }
 
-func (pr *ProxyResources) makeEnvoyCluster(name string, protocol string, eg *pbproxystate.EndpointGroup) (*envoy_cluster_v3.Cluster, error) {
+func (pr *ProxyResources) makeEnvoyCluster(name string, protocol pbproxystate.Protocol, eg *pbproxystate.EndpointGroup) (*envoy_cluster_v3.Cluster, error) {
 	if eg != nil {
 		switch t := eg.Group.(type) {
 		case *pbproxystate.EndpointGroup_Dynamic:
@@ -103,7 +103,7 @@ func (pr *ProxyResources) makeEnvoyCluster(name string, protocol string, eg *pbp
 	return nil, fmt.Errorf("no endpoint group")
 }
 
-func (pr *ProxyResources) makeEnvoyDynamicCluster(name string, protocol string, dynamic *pbproxystate.DynamicEndpointGroup) (*envoy_cluster_v3.Cluster, error) {
+func (pr *ProxyResources) makeEnvoyDynamicCluster(name string, protocol pbproxystate.Protocol, dynamic *pbproxystate.DynamicEndpointGroup) (*envoy_cluster_v3.Cluster, error) {
 	cluster := &envoy_cluster_v3.Cluster{
 		Name:                 name,
 		ClusterDiscoveryType: &envoy_cluster_v3.Cluster_Type{Type: envoy_cluster_v3.Cluster_EDS},
@@ -153,7 +153,7 @@ func (pr *ProxyResources) makeEnvoyDynamicCluster(name string, protocol string,
 
 }
 
-func (pr *ProxyResources) makeEnvoyStaticCluster(name string, protocol string, static *pbproxystate.StaticEndpointGroup) (*envoy_cluster_v3.Cluster, error) {
+func (pr *ProxyResources) makeEnvoyStaticCluster(name string, protocol pbproxystate.Protocol, static *pbproxystate.StaticEndpointGroup) (*envoy_cluster_v3.Cluster, error) {
 	cluster := &envoy_cluster_v3.Cluster{
 		Name:                 name,
 		ClusterDiscoveryType: &envoy_cluster_v3.Cluster_Type{Type: envoy_cluster_v3.Cluster_STATIC},
@@ -182,11 +182,11 @@ func (pr *ProxyResources) makeEnvoyStaticCluster(name string, protocol string, s
 	return cluster, nil
 }
 
-func (pr *ProxyResources) makeEnvoyDnsCluster(name string, protocol string, dns *pbproxystate.DNSEndpointGroup) (*envoy_cluster_v3.Cluster, error) {
+func (pr *ProxyResources) makeEnvoyDnsCluster(name string, protocol pbproxystate.Protocol, dns *pbproxystate.DNSEndpointGroup) (*envoy_cluster_v3.Cluster, error) {
 	return nil, nil
 }
 
-func (pr *ProxyResources) makeEnvoyPassthroughCluster(name string, protocol string, passthrough *pbproxystate.PassthroughEndpointGroup) (*envoy_cluster_v3.Cluster, error) {
+func (pr *ProxyResources) makeEnvoyPassthroughCluster(name string, protocol pbproxystate.Protocol, passthrough *pbproxystate.PassthroughEndpointGroup) (*envoy_cluster_v3.Cluster, error) {
 	cluster := &envoy_cluster_v3.Cluster{
 		Name:                 name,
 		ConnectTimeout:       passthrough.Config.ConnectTimeout,
@@ -207,7 +207,7 @@ func (pr *ProxyResources) makeEnvoyPassthroughCluster(name string, protocol stri
 	return cluster, nil
 }
 
-func (pr *ProxyResources) makeEnvoyAggregateCluster(name string, protocol string, fg *pbproxystate.FailoverGroup) ([]*envoy_cluster_v3.Cluster, error) {
+func (pr *ProxyResources) makeEnvoyAggregateCluster(name string, protocol pbproxystate.Protocol, fg *pbproxystate.FailoverGroup) ([]*envoy_cluster_v3.Cluster, error) {
 	var clusters []*envoy_cluster_v3.Cluster
 	if fg != nil {
 		var egNames []string
@@ -250,8 +250,8 @@ func (pr *ProxyResources) makeEnvoyAggregateCluster(name string, protocol string
 	return clusters, nil
 }
 
-func addLocalAppHttpProtocolOptions(protocol string, c *envoy_cluster_v3.Cluster) error {
-	if !(protocol == "http2" || protocol == "grpc") {
+func addLocalAppHttpProtocolOptions(protocol pbproxystate.Protocol, c *envoy_cluster_v3.Cluster) error {
+	if !(protocol == pbproxystate.Protocol_PROTOCOL_HTTP2 || protocol == pbproxystate.Protocol_PROTOCOL_GRPC) {
 		// do not error.  returning nil means it won't get set.
 		return nil
 	}
@@ -274,8 +274,8 @@ func addLocalAppHttpProtocolOptions(protocol string, c *envoy_cluster_v3.Cluster
 	return nil
 }
 
-func addHttpProtocolOptions(protocol string, c *envoy_cluster_v3.Cluster) error {
-	if !(protocol == "http2" || protocol == "grpc") {
+func addHttpProtocolOptions(protocol pbproxystate.Protocol, c *envoy_cluster_v3.Cluster) error {
+	if !(protocol == pbproxystate.Protocol_PROTOCOL_HTTP2 || protocol == pbproxystate.Protocol_PROTOCOL_GRPC) {
 		// do not error.  returning nil means it won't get set.
 		return nil
 	}

diff --git a/agent/xdsv2/testdata/clusters/source/l7-expose-paths.golden b/agent/xdsv2/testdata/clusters/source/l7-expose-paths.golden
@@ -47,6 +47,14 @@
             ]
           }
         ]
+      },
+      "typedExtensionProtocolOptions":  {
+        "envoy.extensions.upstreams.http.v3.HttpProtocolOptions":  {
+          "@type":  "type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions",
+          "explicitHttpConfig":  {
+            "http2ProtocolOptions":  {}
+          }
+        }
       }
     },
     {

diff --git a/agent/xdsv2/testdata/clusters/source/local-and-inbound-connections.golden b/agent/xdsv2/testdata/clusters/source/local-and-inbound-connections.golden
@@ -47,6 +47,14 @@
             ]
           }
         ]
+      },
+      "typedExtensionProtocolOptions":  {
+        "envoy.extensions.upstreams.http.v3.HttpProtocolOptions":  {
+          "@type":  "type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions",
+          "explicitHttpConfig":  {
+            "http2ProtocolOptions":  {}
+          }
+        }
       }
     },
     {

diff --git a/...tdata/clusters/source/multiport-l7-multiple-workload-addresses-with-specific-ports.golden b/...tdata/clusters/source/multiport-l7-multiple-workload-addresses-with-specific-ports.golden
@@ -47,6 +47,14 @@
             ]
           }
         ]
+      },
+      "typedExtensionProtocolOptions":  {
+        "envoy.extensions.upstreams.http.v3.HttpProtocolOptions":  {
+          "@type":  "type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions",
+          "explicitHttpConfig":  {
+            "http2ProtocolOptions":  {}
+          }
+        }
       }
     }
   ],

diff --git a/...v2/testdata/clusters/source/multiport-l7-multiple-workload-addresses-without-ports.golden b/...v2/testdata/clusters/source/multiport-l7-multiple-workload-addresses-without-ports.golden
@@ -47,6 +47,14 @@
             ]
           }
         ]
+      },
+      "typedExtensionProtocolOptions":  {
+        "envoy.extensions.upstreams.http.v3.HttpProtocolOptions":  {
+          "@type":  "type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions",
+          "explicitHttpConfig":  {
+            "http2ProtocolOptions":  {}
+          }
+        }
       }
     },
     {
@@ -71,6 +79,14 @@
             ]
           }
         ]
+      },
+      "typedExtensionProtocolOptions":  {
+        "envoy.extensions.upstreams.http.v3.HttpProtocolOptions":  {
+          "@type":  "type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions",
+          "explicitHttpConfig":  {
+            "http2ProtocolOptions":  {}
+          }
+        }
       }
     }
   ],

diff --git a/agent/xdsv2/testdata/routes/destination/mixed-multi-destination.golden b/agent/xdsv2/testdata/routes/destination/mixed-multi-destination.golden
@@ -6,8 +6,10 @@
       "name":  "default/local/default/api-1:http:1.1.1.1:1234",
       "virtualHosts":  [
         {
-          "domains": ["*"],
           "name":  "default/local/default/api-1:http:1.1.1.1:1234",
+          "domains":  [
+            "*"
+          ],
           "routes":  [
             {
               "match":  {

diff --git a/...tdata/routes/destination/multiport-l4-and-l7-multiple-implicit-destinations-tproxy.golden b/...tdata/routes/destination/multiport-l4-and-l7-multiple-implicit-destinations-tproxy.golden
@@ -6,8 +6,10 @@
       "name":  "default/local/default/api-app2:http",
       "virtualHosts":  [
         {
-          "domains": ["*"],
           "name":  "default/local/default/api-app2:http",
+          "domains":  [
+            "*"
+          ],
           "routes":  [
             {
               "match":  {
@@ -27,8 +29,10 @@
       "name":  "default/local/default/api-app:http",
       "virtualHosts":  [
         {
-          "domains": ["*"],
           "name":  "default/local/default/api-app:http",
+          "domains":  [
+            "*"
+          ],
           "routes":  [
             {
               "match":  {

diff --git a/...testdata/routes/destination/multiport-l4-and-l7-single-implicit-destination-tproxy.golden b/...testdata/routes/destination/multiport-l4-and-l7-single-implicit-destination-tproxy.golden
@@ -6,8 +6,10 @@
       "name":  "default/local/default/api-app:http",
       "virtualHosts":  [
         {
-          "domains": ["*"],
           "name":  "default/local/default/api-app:http",
+          "domains":  [
+            "*"
+          ],
           "routes":  [
             {
               "match":  {

diff --git a/...ion/multiport-l4-and-l7-single-implicit-destination-with-multiple-workloads-tproxy.golden b/...ion/multiport-l4-and-l7-single-implicit-destination-with-multiple-workloads-tproxy.golden
@@ -6,8 +6,10 @@
       "name":  "default/local/default/api-app:http",
       "virtualHosts":  [
         {
-          "domains": ["*"],
           "name":  "default/local/default/api-app:http",
+          "domains":  [
+            "*"
+          ],
           "routes":  [
             {
               "match":  {

diff --git a/internal/catalog/exports.go b/internal/catalog/exports.go
@@ -124,3 +124,7 @@ func ValidatePortName(name string) error {
 func IsValidUnixSocketPath(host string) bool {
 	return types.IsValidUnixSocketPath(host)
 }
+
+func ValidateProtocol(protocol pbcatalog.Protocol) error {
+	return types.ValidateProtocol(protocol)
+}
diff --git a/internal/catalog/internal/types/service.go b/internal/catalog/internal/types/service.go
@@ -101,7 +101,7 @@ func ValidateService(res *pbresource.Resource) error {
 			})
 		}
 
-		if protoErr := validateProtocol(port.Protocol); protoErr != nil {
+		if protoErr := ValidateProtocol(port.Protocol); protoErr != nil {
 			err = multierror.Append(err, resource.ErrInvalidListElement{
 				Name:  "ports",
 				Index: idx,

diff --git a/internal/catalog/internal/types/service_endpoints.go b/internal/catalog/internal/types/service_endpoints.go
@@ -144,7 +144,7 @@ func validateEndpoint(endpoint *pbcatalog.Endpoint, res *pbresource.Resource) er
 			})
 		}
 
-		if protoErr := validateProtocol(port.Protocol); protoErr != nil {
+		if protoErr := ValidateProtocol(port.Protocol); protoErr != nil {
 			err = multierror.Append(err, resource.ErrInvalidMapValue{
 				Map: "ports",
 				Key: portName,

diff --git a/internal/catalog/internal/types/validators.go b/internal/catalog/internal/types/validators.go
@@ -151,7 +151,8 @@ func ValidatePortName(name string) error {
 	return nil
 }
 
-func validateProtocol(protocol pbcatalog.Protocol) error {
+func ValidateProtocol(protocol pbcatalog.Protocol) error {
+	// enumcover:pbcatalog.Protocol
 	switch protocol {
 	case pbcatalog.Protocol_PROTOCOL_UNSPECIFIED,
 		// means pbcatalog.FailoverMode_FAILOVER_MODE_TCP
@@ -240,6 +241,7 @@ func validateReference(allowedType *pbresource.Type, allowedTenancy *pbresource.
 }
 
 func validateHealth(health pbcatalog.Health) error {
+	// enumcover:pbcatalog.Health
 	switch health {
 	case pbcatalog.Health_HEALTH_ANY,
 		pbcatalog.Health_HEALTH_PASSING,

diff --git a/internal/catalog/internal/types/validators_test.go b/internal/catalog/internal/types/validators_test.go
@@ -373,26 +373,6 @@ func TestValidatePortName(t *testing.T) {
 	})
 }
 
-func TestValidateProtocol(t *testing.T) {
-	// this test simply verifies that we accept all enum values specified in our proto
-	// in order to avoid validator drift.
-	for name, value := range pbcatalog.Protocol_value {
-		t.Run(name, func(t *testing.T) {
-			require.NoError(t, validateProtocol(pbcatalog.Protocol(value)))
-		})
-	}
-}
-
-func TestValidateHealth(t *testing.T) {
-	// this test simply verifies that we accept all enum values specified in our proto
-	// in order to avoid validator drift.
-	for name, value := range pbcatalog.Health_value {
-		t.Run(name, func(t *testing.T) {
-			require.NoError(t, validateHealth(pbcatalog.Health(value)))
-		})
-	}
-}
-
 func TestValidateWorkloadAddress(t *testing.T) {
 	type testCase struct {
 		addr        *pbcatalog.WorkloadAddress

diff --git a/internal/catalog/internal/types/workload.go b/internal/catalog/internal/types/workload.go
@@ -70,7 +70,7 @@ func ValidateWorkload(res *pbresource.Resource) error {
 			})
 		}
 
-		if protoErr := validateProtocol(port.Protocol); protoErr != nil {
+		if protoErr := ValidateProtocol(port.Protocol); protoErr != nil {
 			err = multierror.Append(err, resource.ErrInvalidMapValue{
 				Map: "ports",
 				Key: portName,

diff --git a/internal/mesh/internal/controllers/sidecarproxy/builder/destination_multiport_test.go b/internal/mesh/internal/controllers/sidecarproxy/builder/destination_multiport_test.go
@@ -41,6 +41,31 @@ func TestBuildMultiportImplicitDestinations(t *testing.T) {
 		},
 	}
 
+	multiportServiceData := &pbcatalog.Service{
+		Ports: []*pbcatalog.ServicePort{
+			{
+				TargetPort:  "tcp",
+				VirtualPort: 7070,
+				Protocol:    pbcatalog.Protocol_PROTOCOL_TCP,
+			},
+			{
+				TargetPort:  "tcp2",
+				VirtualPort: 8081,
+				Protocol:    pbcatalog.Protocol_PROTOCOL_TCP,
+			},
+			{
+				TargetPort:  "http",
+				VirtualPort: 8080,
+				Protocol:    pbcatalog.Protocol_PROTOCOL_HTTP,
+			},
+			{
+				TargetPort:  "mesh",
+				VirtualPort: 20000,
+				Protocol:    pbcatalog.Protocol_PROTOCOL_MESH,
+			},
+		},
+	}
+
 	multiportEndpointsData := &pbcatalog.ServiceEndpoints{
 		Endpoints: []*pbcatalog.Endpoint{
 			{
@@ -57,12 +82,12 @@ func TestBuildMultiportImplicitDestinations(t *testing.T) {
 	}
 	apiAppService := resourcetest.Resource(pbcatalog.ServiceType, apiApp).
 		WithTenancy(resource.DefaultNamespacedTenancy()).
-		WithData(t, serviceData).
+		WithData(t, multiportServiceData).
 		Build()
 
 	apiApp2Service := resourcetest.Resource(pbcatalog.ServiceType, apiApp2).
 		WithTenancy(resource.DefaultNamespacedTenancy()).
-		WithData(t, serviceData).
+		WithData(t, multiportServiceData).
 		Build()
 
 	apiAppEndpoints := resourcetest.Resource(pbcatalog.ServiceEndpointsType, apiApp).