xds: Ensure protocol set for gRPC resources

Add expliciti 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.
hashicorp · Oct 25, 2023 · a9e4a67 · a9e4a67
1 parent 0a8fe0d
commit a9e4a67
Show file tree

Hide file tree

Showing 69 changed files with 1,621 additions and 1,143 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).