Skip to content

Commit

Permalink
[NET-5455] Allow disabling request and idle timeouts with negative va…
Browse files Browse the repository at this point in the history
…lues in service router and service resolver (#19992)

* add coverage for testing these timeouts
  • Loading branch information
ndhanushkodi authored Dec 19, 2023
1 parent 013bcef commit 9975b8b
Show file tree
Hide file tree
Showing 10 changed files with 347 additions and 14 deletions.
3 changes: 3 additions & 0 deletions .changelog/19992.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:breaking-change
config-entries: Allow disabling request and idle timeouts with negative values in service router and service resolver config entries.
```
50 changes: 47 additions & 3 deletions agent/proxycfg/testing_upstreams.go
Original file line number Diff line number Diff line change
Expand Up @@ -535,8 +535,7 @@ func setupTestVariationDiscoveryChain(
Kind: structs.ServiceResolver,
Name: "db",
EnterpriseMeta: entMeta,
ConnectTimeout: 33 * time.Second,
RequestTimeout: 33 * time.Second,
ConnectTimeout: 25 * time.Second,
},
&structs.ProxyConfigEntry{
Kind: structs.ProxyDefaults,
Expand All @@ -547,11 +546,56 @@ func setupTestVariationDiscoveryChain(
"protocol": "http",
},
},
// Adding a ServiceRouter in this case allows testing ServiceRoute.Destination timeouts.
&structs.ServiceRouterConfigEntry{
Kind: structs.ServiceRouter,
Name: "db",
EnterpriseMeta: entMeta,
Routes: []structs.ServiceRoute{
{
Match: &structs.ServiceRouteMatch{
HTTP: &structs.ServiceRouteHTTPMatch{
PathPrefix: "/big-side",
},
},
Destination: &structs.ServiceRouteDestination{
Service: "big-side",
// Test disabling idle timeout.
IdleTimeout: -1 * time.Second,
// Test a positive value for request timeout.
RequestTimeout: 10 * time.Second,
},
},
{
Match: &structs.ServiceRouteMatch{
HTTP: &structs.ServiceRouteHTTPMatch{
PathPrefix: "/lil-bit-side",
},
},
Destination: &structs.ServiceRouteDestination{
Service: "lil-bit-side",
// Test zero values for these timeouts.
IdleTimeout: 0 * time.Second,
RequestTimeout: 0 * time.Second,
},
},
},
},
&structs.ServiceSplitterConfigEntry{
Kind: structs.ServiceSplitter,
Name: "db",
EnterpriseMeta: entMeta,
Splits: []structs.ServiceSplit{
{
Weight: 1,
Service: "db",
RequestHeaders: &structs.HTTPHeaderModifiers{
Set: map[string]string{"x-split-leg": "db"},
},
ResponseHeaders: &structs.HTTPHeaderModifiers{
Set: map[string]string{"x-split-leg": "db"},
},
},
{
Weight: 95.5,
Service: "big-side",
Expand All @@ -563,7 +607,7 @@ func setupTestVariationDiscoveryChain(
},
},
{
Weight: 4,
Weight: 3,
Service: "goldilocks-side",
RequestHeaders: &structs.HTTPHeaderModifiers{
Set: map[string]string{"x-split-leg": "goldilocks"},
Expand Down
41 changes: 32 additions & 9 deletions agent/xds/proxystateconverter/routes.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,18 @@ package proxystateconverter
import (
"errors"
"fmt"
"github.com/hashicorp/consul/proto-public/pbmesh/v2beta1/pbproxystate"
"sort"
"strings"
"time"

"github.com/hashicorp/consul/proto-public/pbmesh/v2beta1/pbproxystate"

"github.com/hashicorp/consul/agent/xds/response"

"google.golang.org/protobuf/types/known/durationpb"

"github.com/hashicorp/consul/agent/proxycfg"
"github.com/hashicorp/consul/agent/structs"
"google.golang.org/protobuf/types/known/durationpb"
)

// routesFromSnapshot returns the xDS API representation of the "routes" in the
Expand Down Expand Up @@ -255,23 +257,32 @@ func (s *Converter) makeUpstreamHostForDiscoveryChain(
routeRule := &pbproxystate.RouteRule{}

if destination != nil {
configHandle := routeDestination.DestinationConfiguration
routeDestinationConfiguration := routeDestination.DestinationConfiguration
if destination.PrefixRewrite != "" {
configHandle.PrefixRewrite = destination.PrefixRewrite
routeDestinationConfiguration.PrefixRewrite = destination.PrefixRewrite
}

if destination.RequestTimeout > 0 || destination.IdleTimeout > 0 {
configHandle.TimeoutConfig = &pbproxystate.TimeoutConfig{}
if destination.RequestTimeout != 0 || destination.IdleTimeout != 0 {
routeDestinationConfiguration.TimeoutConfig = &pbproxystate.TimeoutConfig{}
}
if destination.RequestTimeout > 0 {
configHandle.TimeoutConfig.Timeout = durationpb.New(destination.RequestTimeout)
routeDestinationConfiguration.TimeoutConfig.Timeout = durationpb.New(destination.RequestTimeout)
}
// Disable the timeout if user specifies negative value. Setting 0 disables the timeout in Envoy.
if destination.RequestTimeout < 0 {
routeDestinationConfiguration.TimeoutConfig.Timeout = durationpb.New(0 * time.Second)
}

if destination.IdleTimeout > 0 {
configHandle.TimeoutConfig.IdleTimeout = durationpb.New(destination.IdleTimeout)
routeDestinationConfiguration.TimeoutConfig.IdleTimeout = durationpb.New(destination.IdleTimeout)
}
// Disable the timeout if user specifies negative value. Setting 0 disables the timeout in Envoy.
if destination.IdleTimeout < 0 {
routeDestinationConfiguration.TimeoutConfig.IdleTimeout = durationpb.New(0 * time.Second)
}

if destination.HasRetryFeatures() {
configHandle.RetryPolicy = getRetryPolicyForDestination(destination)
routeDestinationConfiguration.RetryPolicy = getRetryPolicyForDestination(destination)
}

if err := injectHeaderManipToRoute(destination, routeRule); err != nil {
Expand Down Expand Up @@ -320,12 +331,24 @@ func (s *Converter) makeUpstreamHostForDiscoveryChain(
return nil, fmt.Errorf("failed to apply load balancer configuration to route action: %v", err)
}

// A request timeout can be configured on a resolver or router. If configured on a resolver, the timeout will
// only apply if the start node is a resolver. This is because the timeout is attached to an (Envoy
// RouteAction)[https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-msg-config-route-v3-routeaction]
// If there is a splitter before this resolver, the branches of the split are configured within the same
// RouteAction, and the timeout cannot be shared between branches of a split.
if startNode.Resolver.RequestTimeout > 0 {
to := &pbproxystate.TimeoutConfig{
Timeout: durationpb.New(startNode.Resolver.RequestTimeout),
}
routeDestination.DestinationConfiguration.TimeoutConfig = to
}
// Disable the timeout if user specifies negative value. Setting 0 disables the timeout in Envoy.
if startNode.Resolver.RequestTimeout < 0 {
to := &pbproxystate.TimeoutConfig{
Timeout: durationpb.New(0 * time.Second),
}
routeDestination.DestinationConfiguration.TimeoutConfig = to
}
defaultRoute := &pbproxystate.RouteRule{
Match: makeDefaultRouteMatch(),
Destination: routeDestination,
Expand Down
17 changes: 17 additions & 0 deletions agent/xds/routes.go
Original file line number Diff line number Diff line change
Expand Up @@ -696,10 +696,18 @@ func (s *ResourceGenerator) makeUpstreamRouteForDiscoveryChain(
if destination.RequestTimeout > 0 {
routeAction.Route.Timeout = durationpb.New(destination.RequestTimeout)
}
// Disable the timeout if user specifies negative value. Setting 0 disables the timeout in Envoy.
if destination.RequestTimeout < 0 {
routeAction.Route.Timeout = durationpb.New(0 * time.Second)
}

if destination.IdleTimeout > 0 {
routeAction.Route.IdleTimeout = durationpb.New(destination.IdleTimeout)
}
// Disable the timeout if user specifies negative value. Setting 0 disables the timeout in Envoy.
if destination.IdleTimeout < 0 {
routeAction.Route.IdleTimeout = durationpb.New(0 * time.Second)
}

if destination.HasRetryFeatures() {
routeAction.Route.RetryPolicy = getRetryPolicyForDestination(destination)
Expand Down Expand Up @@ -754,9 +762,18 @@ func (s *ResourceGenerator) makeUpstreamRouteForDiscoveryChain(
return nil, fmt.Errorf("failed to apply load balancer configuration to route action: %v", err)
}

// A request timeout can be configured on a resolver or router. If configured on a resolver, the timeout will
// only apply if the start node is a resolver. This is because the timeout is attached to an (Envoy
// RouteAction)[https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-msg-config-route-v3-routeaction]
// If there is a splitter before this resolver, the branches of the split are configured within the same
// RouteAction, and the timeout cannot be shared between branches of a split.
if startNode.Resolver.RequestTimeout > 0 {
routeAction.Route.Timeout = durationpb.New(startNode.Resolver.RequestTimeout)
}
// Disable the timeout if user specifies negative value. Setting 0 disables the timeout in Envoy.
if startNode.Resolver.RequestTimeout < 0 {
routeAction.Route.Timeout = durationpb.New(0 * time.Second)
}
defaultRoute := &envoy_route_v3.Route{
Match: makeDefaultRouteMatch(),
Action: routeAction,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,54 @@
},
"type": "EDS"
},
{
"@type": "type.googleapis.com/envoy.config.cluster.v3.Cluster",
"altStatName": "db.default.dc1.internal.11111111-2222-3333-4444-555555555555.consul",
"circuitBreakers": {},
"commonLbConfig": {
"healthyPanicThreshold": {}
},
"connectTimeout": "25s",
"edsClusterConfig": {
"edsConfig": {
"ads": {},
"resourceApiVersion": "V3"
}
},
"name": "db.default.dc1.internal.11111111-2222-3333-4444-555555555555.consul",
"outlierDetection": {},
"transportSocket": {
"name": "tls",
"typedConfig": {
"@type": "type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext",
"commonTlsContext": {
"tlsCertificates": [
{
"certificateChain": {
"inlineString": "-----BEGIN CERTIFICATE-----\nMIICjDCCAjKgAwIBAgIIC5llxGV1gB8wCgYIKoZIzj0EAwIwFDESMBAGA1UEAxMJ\nVGVzdCBDQSAyMB4XDTE5MDMyMjEzNTgyNloXDTI5MDMyMjEzNTgyNlowDjEMMAoG\nA1UEAxMDd2ViMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEADPv1RHVNRfa2VKR\nAB16b6rZnEt7tuhaxCFpQXPj7M2omb0B9Favq5E0ivpNtv1QnFhxtPd7d5k4e+T7\nSkW1TaOCAXIwggFuMA4GA1UdDwEB/wQEAwIDuDAdBgNVHSUEFjAUBggrBgEFBQcD\nAgYIKwYBBQUHAwEwDAYDVR0TAQH/BAIwADBoBgNVHQ4EYQRfN2Q6MDc6ODc6M2E6\nNDA6MTk6NDc6YzM6NWE6YzA6YmE6NjI6ZGY6YWY6NGI6ZDQ6MDU6MjU6NzY6M2Q6\nNWE6OGQ6MTY6OGQ6Njc6NWU6MmU6YTA6MzQ6N2Q6ZGM6ZmYwagYDVR0jBGMwYYBf\nZDE6MTE6MTE6YWM6MmE6YmE6OTc6YjI6M2Y6YWM6N2I6YmQ6ZGE6YmU6YjE6OGE6\nZmM6OWE6YmE6YjU6YmM6ODM6ZTc6NWU6NDE6NmY6ZjI6NzM6OTU6NTg6MGM6ZGIw\nWQYDVR0RBFIwUIZOc3BpZmZlOi8vMTExMTExMTEtMjIyMi0zMzMzLTQ0NDQtNTU1\nNTU1NTU1NTU1LmNvbnN1bC9ucy9kZWZhdWx0L2RjL2RjMS9zdmMvd2ViMAoGCCqG\nSM49BAMCA0gAMEUCIGC3TTvvjj76KMrguVyFf4tjOqaSCRie3nmHMRNNRav7AiEA\npY0heYeK9A6iOLrzqxSerkXXQyj5e9bE4VgUnxgPU6g=\n-----END CERTIFICATE-----\n"
},
"privateKey": {
"inlineString": "-----BEGIN EC PRIVATE KEY-----\nMHcCAQEEIMoTkpRggp3fqZzFKh82yS4LjtJI+XY+qX/7DefHFrtdoAoGCCqGSM49\nAwEHoUQDQgAEADPv1RHVNRfa2VKRAB16b6rZnEt7tuhaxCFpQXPj7M2omb0B9Fav\nq5E0ivpNtv1QnFhxtPd7d5k4e+T7SkW1TQ==\n-----END EC PRIVATE KEY-----\n"
}
}
],
"tlsParams": {},
"validationContext": {
"matchSubjectAltNames": [
{
"exact": "spiffe://11111111-2222-3333-4444-555555555555.consul/ns/default/dc/dc1/svc/db"
}
],
"trustedCa": {
"inlineString": "-----BEGIN CERTIFICATE-----\nMIICXDCCAgKgAwIBAgIICpZq70Z9LyUwCgYIKoZIzj0EAwIwFDESMBAGA1UEAxMJ\nVGVzdCBDQSAyMB4XDTE5MDMyMjEzNTgyNloXDTI5MDMyMjEzNTgyNlowFDESMBAG\nA1UEAxMJVGVzdCBDQSAyMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEIhywH1gx\nAsMwuF3ukAI5YL2jFxH6Usnma1HFSfVyxbXX1/uoZEYrj8yCAtdU2yoHETyd+Zx2\nThhRLP79pYegCaOCATwwggE4MA4GA1UdDwEB/wQEAwIBhjAPBgNVHRMBAf8EBTAD\nAQH/MGgGA1UdDgRhBF9kMToxMToxMTphYzoyYTpiYTo5NzpiMjozZjphYzo3Yjpi\nZDpkYTpiZTpiMTo4YTpmYzo5YTpiYTpiNTpiYzo4MzplNzo1ZTo0MTo2ZjpmMjo3\nMzo5NTo1ODowYzpkYjBqBgNVHSMEYzBhgF9kMToxMToxMTphYzoyYTpiYTo5Nzpi\nMjozZjphYzo3YjpiZDpkYTpiZTpiMTo4YTpmYzo5YTpiYTpiNTpiYzo4MzplNzo1\nZTo0MTo2ZjpmMjo3Mzo5NTo1ODowYzpkYjA/BgNVHREEODA2hjRzcGlmZmU6Ly8x\nMTExMTExMS0yMjIyLTMzMzMtNDQ0NC01NTU1NTU1NTU1NTUuY29uc3VsMAoGCCqG\nSM49BAMCA0gAMEUCICOY0i246rQHJt8o8Oya0D5PLL1FnmsQmQqIGCi31RwnAiEA\noR5f6Ku+cig2Il8T8LJujOp2/2A72QcHZA57B13y+8o=\n-----END CERTIFICATE-----\n"
}
}
},
"sni": "db.default.dc1.internal.11111111-2222-3333-4444-555555555555.consul"
}
},
"type": "EDS"
},
{
"@type": "type.googleapis.com/envoy.config.cluster.v3.Cluster",
"circuitBreakers": {},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,54 @@
},
"type": "EDS"
},
{
"@type": "type.googleapis.com/envoy.config.cluster.v3.Cluster",
"altStatName": "db.default.dc1.internal.11111111-2222-3333-4444-555555555555.consul",
"circuitBreakers": {},
"commonLbConfig": {
"healthyPanicThreshold": {}
},
"connectTimeout": "25s",
"edsClusterConfig": {
"edsConfig": {
"ads": {},
"resourceApiVersion": "V3"
}
},
"name": "db.default.dc1.internal.11111111-2222-3333-4444-555555555555.consul",
"outlierDetection": {},
"transportSocket": {
"name": "tls",
"typedConfig": {
"@type": "type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext",
"commonTlsContext": {
"tlsCertificates": [
{
"certificateChain": {
"inlineString": "-----BEGIN CERTIFICATE-----\nMIICjDCCAjKgAwIBAgIIC5llxGV1gB8wCgYIKoZIzj0EAwIwFDESMBAGA1UEAxMJ\nVGVzdCBDQSAyMB4XDTE5MDMyMjEzNTgyNloXDTI5MDMyMjEzNTgyNlowDjEMMAoG\nA1UEAxMDd2ViMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEADPv1RHVNRfa2VKR\nAB16b6rZnEt7tuhaxCFpQXPj7M2omb0B9Favq5E0ivpNtv1QnFhxtPd7d5k4e+T7\nSkW1TaOCAXIwggFuMA4GA1UdDwEB/wQEAwIDuDAdBgNVHSUEFjAUBggrBgEFBQcD\nAgYIKwYBBQUHAwEwDAYDVR0TAQH/BAIwADBoBgNVHQ4EYQRfN2Q6MDc6ODc6M2E6\nNDA6MTk6NDc6YzM6NWE6YzA6YmE6NjI6ZGY6YWY6NGI6ZDQ6MDU6MjU6NzY6M2Q6\nNWE6OGQ6MTY6OGQ6Njc6NWU6MmU6YTA6MzQ6N2Q6ZGM6ZmYwagYDVR0jBGMwYYBf\nZDE6MTE6MTE6YWM6MmE6YmE6OTc6YjI6M2Y6YWM6N2I6YmQ6ZGE6YmU6YjE6OGE6\nZmM6OWE6YmE6YjU6YmM6ODM6ZTc6NWU6NDE6NmY6ZjI6NzM6OTU6NTg6MGM6ZGIw\nWQYDVR0RBFIwUIZOc3BpZmZlOi8vMTExMTExMTEtMjIyMi0zMzMzLTQ0NDQtNTU1\nNTU1NTU1NTU1LmNvbnN1bC9ucy9kZWZhdWx0L2RjL2RjMS9zdmMvd2ViMAoGCCqG\nSM49BAMCA0gAMEUCIGC3TTvvjj76KMrguVyFf4tjOqaSCRie3nmHMRNNRav7AiEA\npY0heYeK9A6iOLrzqxSerkXXQyj5e9bE4VgUnxgPU6g=\n-----END CERTIFICATE-----\n"
},
"privateKey": {
"inlineString": "-----BEGIN EC PRIVATE KEY-----\nMHcCAQEEIMoTkpRggp3fqZzFKh82yS4LjtJI+XY+qX/7DefHFrtdoAoGCCqGSM49\nAwEHoUQDQgAEADPv1RHVNRfa2VKRAB16b6rZnEt7tuhaxCFpQXPj7M2omb0B9Fav\nq5E0ivpNtv1QnFhxtPd7d5k4e+T7SkW1TQ==\n-----END EC PRIVATE KEY-----\n"
}
}
],
"tlsParams": {},
"validationContext": {
"matchSubjectAltNames": [
{
"exact": "spiffe://11111111-2222-3333-4444-555555555555.consul/ns/default/dc/dc1/svc/db"
}
],
"trustedCa": {
"inlineString": "-----BEGIN CERTIFICATE-----\nMIICXDCCAgKgAwIBAgIICpZq70Z9LyUwCgYIKoZIzj0EAwIwFDESMBAGA1UEAxMJ\nVGVzdCBDQSAyMB4XDTE5MDMyMjEzNTgyNloXDTI5MDMyMjEzNTgyNlowFDESMBAG\nA1UEAxMJVGVzdCBDQSAyMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEIhywH1gx\nAsMwuF3ukAI5YL2jFxH6Usnma1HFSfVyxbXX1/uoZEYrj8yCAtdU2yoHETyd+Zx2\nThhRLP79pYegCaOCATwwggE4MA4GA1UdDwEB/wQEAwIBhjAPBgNVHRMBAf8EBTAD\nAQH/MGgGA1UdDgRhBF9kMToxMToxMTphYzoyYTpiYTo5NzpiMjozZjphYzo3Yjpi\nZDpkYTpiZTpiMTo4YTpmYzo5YTpiYTpiNTpiYzo4MzplNzo1ZTo0MTo2ZjpmMjo3\nMzo5NTo1ODowYzpkYjBqBgNVHSMEYzBhgF9kMToxMToxMTphYzoyYTpiYTo5Nzpi\nMjozZjphYzo3YjpiZDpkYTpiZTpiMTo4YTpmYzo5YTpiYTpiNTpiYzo4MzplNzo1\nZTo0MTo2ZjpmMjo3Mzo5NTo1ODowYzpkYjA/BgNVHREEODA2hjRzcGlmZmU6Ly8x\nMTExMTExMS0yMjIyLTMzMzMtNDQ0NC01NTU1NTU1NTU1NTUuY29uc3VsMAoGCCqG\nSM49BAMCA0gAMEUCICOY0i246rQHJt8o8Oya0D5PLL1FnmsQmQqIGCi31RwnAiEA\noR5f6Ku+cig2Il8T8LJujOp2/2A72QcHZA57B13y+8o=\n-----END CERTIFICATE-----\n"
}
}
},
"sni": "db.default.dc1.internal.11111111-2222-3333-4444-555555555555.consul"
}
},
"type": "EDS"
},
{
"@type": "type.googleapis.com/envoy.config.cluster.v3.Cluster",
"altStatName": "goldilocks-side.default.dc1.internal.11111111-2222-3333-4444-555555555555.consul",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,40 @@
{
"nonce": "00000001",
"resources": [
{
"@type": "type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment",
"clusterName": "db.default.dc1.internal.11111111-2222-3333-4444-555555555555.consul",
"endpoints": [
{
"lbEndpoints": [
{
"endpoint": {
"address": {
"socketAddress": {
"address": "10.10.1.1",
"portValue": 8080
}
}
},
"healthStatus": "HEALTHY",
"loadBalancingWeight": 1
},
{
"endpoint": {
"address": {
"socketAddress": {
"address": "10.10.1.2",
"portValue": 8080
}
}
},
"healthStatus": "HEALTHY",
"loadBalancingWeight": 1
}
]
}
]
},
{
"@type": "type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment",
"clusterName": "geo-cache.default.dc1.query.11111111-2222-3333-4444-555555555555.consul",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,41 @@
{
"nonce": "00000001",
"resources": [
{
"@type": "type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment",
"clusterName": "db.default.dc1.internal.11111111-2222-3333-4444-555555555555.consul",
"endpoints": [
{
"lbEndpoints": [
{
"endpoint": {
"address": {
"socketAddress": {
"address": "10.10.1.1",
"portValue": 8080
}
}
},
"healthStatus": "HEALTHY",
"loadBalancingWeight": 1
},
{
"endpoint": {
"address": {
"socketAddress": {
"address": "10.10.1.2",
"portValue": 8080
}
}
},
"healthStatus": "HEALTHY",
"loadBalancingWeight": 1
}
]
}
]
}
],
"typeUrl": "type.googleapis.com/envoy.config.endpoint.v3.ClusterLoadAssignment",
"versionInfo": "00000001"
}
Loading

0 comments on commit 9975b8b

Please sign in to comment.