Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Oct 25, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a8fe0d
xds: Ensure v2 route match is populated for gRPC
zalimeni Oct 23, 2023
74bf2aa
xds: Ensure protocol set for gRPC resources
zalimeni Oct 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions agent/xds/proxystateconverter/clusters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

proxystate cluster.Protocol is now an enumeration. so we need to convert the string from config entry to the protocol enum.

if cfg.MaxInboundConnections > 0 {
namedCluster.cluster.GetEndpointGroup().GetStatic().GetConfig().
CircuitBreakers = &pbproxystate.CircuitBreakers{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand All @@ -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,
},
Expand Down Expand Up @@ -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,
}
20 changes: 10 additions & 10 deletions agent/xdsv2/cluster_resources.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this protocol prevents importing pbcatalog code into xds resource code and keeps the IR as the intermediate seam that we have to keep strong ties between our data model and xds.

if eg != nil {
switch t := eg.Group.(type) {
case *pbproxystate.EndpointGroup_Dynamic:
Expand All @@ -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},
Expand Down Expand Up @@ -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},
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down
8 changes: 8 additions & 0 deletions agent/xdsv2/testdata/clusters/source/l7-expose-paths.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@
]
}
]
},
"typedExtensionProtocolOptions": {
"envoy.extensions.upstreams.http.v3.HttpProtocolOptions": {
"@type": "type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions",
"explicitHttpConfig": {
"http2ProtocolOptions": {}
}
}
}
},
{
Expand Down
8 changes: 8 additions & 0 deletions agent/xdsv2/testdata/clusters/source/local-and-inbound-connections.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@
]
}
]
},
"typedExtensionProtocolOptions": {
"envoy.extensions.upstreams.http.v3.HttpProtocolOptions": {
"@type": "type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions",
"explicitHttpConfig": {
"http2ProtocolOptions": {}
}
}
}
},
{
Expand Down
8 changes: 8 additions & 0 deletions ...tdata/clusters/source/multiport-l7-multiple-workload-addresses-with-specific-ports.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@
]
}
]
},
"typedExtensionProtocolOptions": {
"envoy.extensions.upstreams.http.v3.HttpProtocolOptions": {
"@type": "type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions",
"explicitHttpConfig": {
"http2ProtocolOptions": {}
}
}
}
}
],
Expand Down
16 changes: 16 additions & 0 deletions ...v2/testdata/clusters/source/multiport-l7-multiple-workload-addresses-without-ports.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,14 @@
]
}
]
},
"typedExtensionProtocolOptions": {
"envoy.extensions.upstreams.http.v3.HttpProtocolOptions": {
"@type": "type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions",
"explicitHttpConfig": {
"http2ProtocolOptions": {}
}
}
}
},
{
Expand All @@ -71,6 +79,14 @@
]
}
]
},
"typedExtensionProtocolOptions": {
"envoy.extensions.upstreams.http.v3.HttpProtocolOptions": {
"@type": "type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions",
"explicitHttpConfig": {
"http2ProtocolOptions": {}
}
}
}
}
],
Expand Down
4 changes: 3 additions & 1 deletion agent/xdsv2/testdata/routes/destination/mixed-multi-destination.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
8 changes: 6 additions & 2 deletions ...tdata/routes/destination/multiport-l4-and-l7-multiple-implicit-destinations-tproxy.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@
"name": "default/local/default/api-app2:http",
"virtualHosts": [
{
"domains": ["*"],
"name": "default/local/default/api-app2:http",
"domains": [
"*"
],
"routes": [
{
"match": {
Expand All @@ -27,8 +29,10 @@
"name": "default/local/default/api-app:http",
"virtualHosts": [
{
"domains": ["*"],
"name": "default/local/default/api-app:http",
"domains": [
"*"
],
"routes": [
{
"match": {
Expand Down
4 changes: 3 additions & 1 deletion ...testdata/routes/destination/multiport-l4-and-l7-single-implicit-destination-tproxy.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@
"name": "default/local/default/api-app:http",
"virtualHosts": [
{
"domains": ["*"],
"name": "default/local/default/api-app:http",
"domains": [
"*"
],
"routes": [
{
"match": {
Expand Down
4 changes: 3 additions & 1 deletion ...ion/multiport-l4-and-l7-single-implicit-destination-with-multiple-workloads-tproxy.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@
"name": "default/local/default/api-app:http",
"virtualHosts": [
{
"domains": ["*"],
"name": "default/local/default/api-app:http",
"domains": [
"*"
],
"routes": [
{
"match": {
Expand Down
4 changes: 4 additions & 0 deletions internal/catalog/exports.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
2 changes: 1 addition & 1 deletion internal/catalog/internal/types/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion internal/catalog/internal/types/service_endpoints.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 3 additions & 1 deletion internal/catalog/internal/types/validators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down
20 changes: 0 additions & 20 deletions internal/catalog/internal/types/validators_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,26 +373,6 @@ func TestValidatePortName(t *testing.T) {
})
}

func TestValidateProtocol(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these removed by accident?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I added those a while back before I found out about enumcover 😄 the added annos should cover what they did

// 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
Expand Down
2 changes: 1 addition & 1 deletion internal/catalog/internal/types/workload.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
29 changes: 27 additions & 2 deletions internal/mesh/internal/controllers/sidecarproxy/builder/destination_multiport_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,31 @@ func TestBuildMultiportImplicitDestinations(t *testing.T) {
},
}

multiportServiceData := &pbcatalog.Service{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previously this test used a shared version of this type of data with other tests and it was causing lots of clobbering and coordination. opted to make this local to the test.

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{
{
Expand All @@ -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).
Expand Down
Loading
Loading