From 465d95a541e963b31ee013f5cdf4ba5e6188bd03 Mon Sep 17 00:00:00 2001 From: Manish Kumar Date: Wed, 12 May 2021 15:04:27 +0530 Subject: [PATCH 1/6] Fixed missing required in cluster config for connect_timeout Signed-off-by: Manish Kumar --- api/envoy/config/cluster/v3/cluster.proto | 5 ++++- api/envoy/config/cluster/v4alpha/cluster.proto | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/api/envoy/config/cluster/v3/cluster.proto b/api/envoy/config/cluster/v3/cluster.proto index 7012f4ef5c435..13f800e56517b 100644 --- a/api/envoy/config/cluster/v3/cluster.proto +++ b/api/envoy/config/cluster/v3/cluster.proto @@ -709,7 +709,10 @@ message Cluster { EdsClusterConfig eds_cluster_config = 3; // The timeout for new network connections to hosts in the cluster. - google.protobuf.Duration connect_timeout = 4 [(validate.rules).duration = {gt {}}]; + google.protobuf.Duration connect_timeout = 4 [(validate.rules).duration = { + required: true + gt {} + }]; // Soft limit on size of the cluster’s connections read and write buffers. If // unspecified, an implementation defined default is applied (1MiB). diff --git a/api/envoy/config/cluster/v4alpha/cluster.proto b/api/envoy/config/cluster/v4alpha/cluster.proto index 807b70a0ce8be..b462a246e472a 100644 --- a/api/envoy/config/cluster/v4alpha/cluster.proto +++ b/api/envoy/config/cluster/v4alpha/cluster.proto @@ -718,7 +718,10 @@ message Cluster { EdsClusterConfig eds_cluster_config = 3; // The timeout for new network connections to hosts in the cluster. - google.protobuf.Duration connect_timeout = 4 [(validate.rules).duration = {gt {}}]; + google.protobuf.Duration connect_timeout = 4 [(validate.rules).duration = { + required: true + gt {} + }]; // Soft limit on size of the cluster’s connections read and write buffers. If // unspecified, an implementation defined default is applied (1MiB). From e458506f8ac84a8cd98f49fc16013bf3126b9aa7 Mon Sep 17 00:00:00 2001 From: Manish Kumar Date: Wed, 12 May 2021 17:16:58 +0530 Subject: [PATCH 2/6] fix format. Signed-off-by: Manish Kumar --- generated_api_shadow/envoy/config/cluster/v3/cluster.proto | 5 ++++- .../envoy/config/cluster/v4alpha/cluster.proto | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/generated_api_shadow/envoy/config/cluster/v3/cluster.proto b/generated_api_shadow/envoy/config/cluster/v3/cluster.proto index 0e8a711900dd7..68306bfd98924 100644 --- a/generated_api_shadow/envoy/config/cluster/v3/cluster.proto +++ b/generated_api_shadow/envoy/config/cluster/v3/cluster.proto @@ -710,7 +710,10 @@ message Cluster { EdsClusterConfig eds_cluster_config = 3; // The timeout for new network connections to hosts in the cluster. - google.protobuf.Duration connect_timeout = 4 [(validate.rules).duration = {gt {}}]; + google.protobuf.Duration connect_timeout = 4 [(validate.rules).duration = { + required: true + gt {} + }]; // Soft limit on size of the cluster’s connections read and write buffers. If // unspecified, an implementation defined default is applied (1MiB). diff --git a/generated_api_shadow/envoy/config/cluster/v4alpha/cluster.proto b/generated_api_shadow/envoy/config/cluster/v4alpha/cluster.proto index 6cbc477cee886..eca60331a64ed 100644 --- a/generated_api_shadow/envoy/config/cluster/v4alpha/cluster.proto +++ b/generated_api_shadow/envoy/config/cluster/v4alpha/cluster.proto @@ -718,7 +718,10 @@ message Cluster { EdsClusterConfig eds_cluster_config = 3; // The timeout for new network connections to hosts in the cluster. - google.protobuf.Duration connect_timeout = 4 [(validate.rules).duration = {gt {}}]; + google.protobuf.Duration connect_timeout = 4 [(validate.rules).duration = { + required: true + gt {} + }]; // Soft limit on size of the cluster’s connections read and write buffers. If // unspecified, an implementation defined default is applied (1MiB). From 0a722b1fe39c1b479e7da482017c89e2a481bcbf Mon Sep 17 00:00:00 2001 From: Manish Kumar Date: Thu, 13 May 2021 00:22:36 +0530 Subject: [PATCH 3/6] fix Signed-off-by: Manish Kumar --- test/common/upstream/cds_api_impl_test.cc | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/common/upstream/cds_api_impl_test.cc b/test/common/upstream/cds_api_impl_test.cc index 9fd535fa72041..d2c37a3ad9518 100644 --- a/test/common/upstream/cds_api_impl_test.cc +++ b/test/common/upstream/cds_api_impl_test.cc @@ -88,6 +88,7 @@ version_info: '0' resources: - "@type": type.googleapis.com/envoy.config.cluster.v3.Cluster name: cluster1 + connect_timeout: 5s type: EDS eds_cluster_config: eds_config: @@ -183,6 +184,7 @@ TEST_F(CdsApiImplTest, DeltaConfigUpdate) { { envoy::config::cluster::v3::Cluster cluster; cluster.set_name("cluster_1"); + cluster.mutable_connect_timeout()->set_seconds(5); expectAdd("cluster_1", "v1"); auto* resource = resources.Add(); resource->mutable_resource()->PackFrom(cluster); @@ -192,6 +194,7 @@ TEST_F(CdsApiImplTest, DeltaConfigUpdate) { { envoy::config::cluster::v3::Cluster cluster; cluster.set_name("cluster_2"); + cluster.mutable_connect_timeout()->set_seconds(5); expectAdd("cluster_2", "v1"); auto* resource = resources.Add(); resource->mutable_resource()->PackFrom(cluster); @@ -208,6 +211,7 @@ TEST_F(CdsApiImplTest, DeltaConfigUpdate) { { envoy::config::cluster::v3::Cluster cluster; cluster.set_name("cluster_3"); + cluster.mutable_connect_timeout()->set_seconds(5); expectAdd("cluster_3", "v2"); auto* resource = resources.Add(); resource->mutable_resource()->PackFrom(cluster); @@ -260,6 +264,7 @@ version_info: '0' resources: - "@type": type.googleapis.com/envoy.config.cluster.v3.Cluster name: cluster1 + connect_timeout: 5s type: EDS eds_cluster_config: eds_config: @@ -267,6 +272,7 @@ version_info: '0' path: eds path - "@type": type.googleapis.com/envoy.config.cluster.v3.Cluster name: cluster2 + connect_timeout: 5s type: EDS eds_cluster_config: eds_config: @@ -291,6 +297,7 @@ version_info: '1' resources: - "@type": type.googleapis.com/envoy.config.cluster.v3.Cluster name: cluster1 + connect_timeout: 5s type: EDS eds_cluster_config: eds_config: @@ -298,6 +305,7 @@ version_info: '1' path: eds path - "@type": type.googleapis.com/envoy.config.cluster.v3.Cluster name: cluster3 + connect_timeout: 5s type: EDS eds_cluster_config: eds_config: @@ -329,6 +337,7 @@ version_info: '0' resources: - "@type": type.googleapis.com/envoy.config.cluster.v3.Cluster name: cluster1 + connect_timeout: 5s type: EDS eds_cluster_config: eds_config: @@ -336,6 +345,7 @@ version_info: '0' path: eds path - "@type": type.googleapis.com/envoy.config.cluster.v3.Cluster name: cluster1 + connect_timeout: 5s type: EDS eds_cluster_config: eds_config: From 92376b62e82bf05d8c26de4bd7031ddfc1037a9f Mon Sep 17 00:00:00 2001 From: Manish Kumar Date: Thu, 13 May 2021 00:47:09 +0530 Subject: [PATCH 4/6] Added default value 5s to connect_timeout Signed-off-by: Manish Kumar --- api/envoy/config/cluster/v3/cluster.proto | 1 + api/envoy/config/cluster/v4alpha/cluster.proto | 1 + generated_api_shadow/envoy/config/cluster/v3/cluster.proto | 1 + generated_api_shadow/envoy/config/cluster/v4alpha/cluster.proto | 1 + source/common/upstream/upstream_impl.cc | 2 +- 5 files changed, 5 insertions(+), 1 deletion(-) diff --git a/api/envoy/config/cluster/v3/cluster.proto b/api/envoy/config/cluster/v3/cluster.proto index 7012f4ef5c435..957c2b3341e3a 100644 --- a/api/envoy/config/cluster/v3/cluster.proto +++ b/api/envoy/config/cluster/v3/cluster.proto @@ -709,6 +709,7 @@ message Cluster { EdsClusterConfig eds_cluster_config = 3; // The timeout for new network connections to hosts in the cluster. + // If not set, a default value of 5s will be used. google.protobuf.Duration connect_timeout = 4 [(validate.rules).duration = {gt {}}]; // Soft limit on size of the cluster’s connections read and write buffers. If diff --git a/api/envoy/config/cluster/v4alpha/cluster.proto b/api/envoy/config/cluster/v4alpha/cluster.proto index 807b70a0ce8be..8adf5ea460e4e 100644 --- a/api/envoy/config/cluster/v4alpha/cluster.proto +++ b/api/envoy/config/cluster/v4alpha/cluster.proto @@ -718,6 +718,7 @@ message Cluster { EdsClusterConfig eds_cluster_config = 3; // The timeout for new network connections to hosts in the cluster. + // If not set, a default value of 5s will be used. google.protobuf.Duration connect_timeout = 4 [(validate.rules).duration = {gt {}}]; // Soft limit on size of the cluster’s connections read and write buffers. If diff --git a/generated_api_shadow/envoy/config/cluster/v3/cluster.proto b/generated_api_shadow/envoy/config/cluster/v3/cluster.proto index 0e8a711900dd7..d9e64b44ce88a 100644 --- a/generated_api_shadow/envoy/config/cluster/v3/cluster.proto +++ b/generated_api_shadow/envoy/config/cluster/v3/cluster.proto @@ -710,6 +710,7 @@ message Cluster { EdsClusterConfig eds_cluster_config = 3; // The timeout for new network connections to hosts in the cluster. + // If not set, a default value of 5s will be used. google.protobuf.Duration connect_timeout = 4 [(validate.rules).duration = {gt {}}]; // Soft limit on size of the cluster’s connections read and write buffers. If diff --git a/generated_api_shadow/envoy/config/cluster/v4alpha/cluster.proto b/generated_api_shadow/envoy/config/cluster/v4alpha/cluster.proto index 6cbc477cee886..d637591a2251c 100644 --- a/generated_api_shadow/envoy/config/cluster/v4alpha/cluster.proto +++ b/generated_api_shadow/envoy/config/cluster/v4alpha/cluster.proto @@ -718,6 +718,7 @@ message Cluster { EdsClusterConfig eds_cluster_config = 3; // The timeout for new network connections to hosts in the cluster. + // If not set, a default value of 5s will be used. google.protobuf.Duration connect_timeout = 4 [(validate.rules).duration = {gt {}}]; // Soft limit on size of the cluster’s connections read and write buffers. If diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 07db4f18f2ff6..0d28ca37c5f96 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -742,7 +742,7 @@ ClusterInfoImpl::ClusterInfoImpl( runtime_.snapshot().getInteger(Http::MaxResponseHeadersCountOverrideKey, Http::DEFAULT_MAX_HEADERS_COUNT))), connect_timeout_( - std::chrono::milliseconds(PROTOBUF_GET_MS_REQUIRED(config, connect_timeout))), + std::chrono::milliseconds(PROTOBUF_GET_MS_OR_DEFAULT(config, connect_timeout, 5000))), per_upstream_preconnect_ratio_(PROTOBUF_GET_WRAPPED_OR_DEFAULT( config.preconnect_policy(), per_upstream_preconnect_ratio, 1.0)), peekahead_ratio_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config.preconnect_policy(), From b0b58880cc530d7f75e6e6e59d50b4037c7a6f02 Mon Sep 17 00:00:00 2001 From: Manish Kumar Date: Thu, 13 May 2021 05:20:58 +0530 Subject: [PATCH 5/6] Added unit test for checking default value. Signed-off-by: Manish Kumar --- test/common/upstream/upstream_impl_test.cc | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/common/upstream/upstream_impl_test.cc b/test/common/upstream/upstream_impl_test.cc index a0ce86ae68ac1..38d72ce7b535c 100644 --- a/test/common/upstream/upstream_impl_test.cc +++ b/test/common/upstream/upstream_impl_test.cc @@ -2607,6 +2607,20 @@ TEST_F(ClusterInfoImplTest, TestTrackRemainingResourcesGauges) { EXPECT_EQ(4U, high_remaining_retries.value()); } +TEST_F(ClusterInfoImplTest, DefaultConnectTimeout) { + const std::string yaml = R"EOF( + name: cluster1 + type: STRICT_DNS + lb_policy: ROUND_ROBIN +)EOF"; + + auto cluster = makeCluster(yaml); + envoy::config::cluster::v3::Cluster cluster_config = parseClusterFromV3Yaml(yaml); + + EXPECT_FALSE(cluster_config.has_connect_timeout()); + EXPECT_EQ(std::chrono::seconds(5), cluster->info()->connectTimeout()); +} + TEST_F(ClusterInfoImplTest, Timeouts) { const std::string yaml = R"EOF( name: name From f0928f21fe4de1f6e8746a814c5f98ad3081ed9f Mon Sep 17 00:00:00 2001 From: Manish Kumar Date: Thu, 20 May 2021 15:59:19 +0530 Subject: [PATCH 6/6] Added docs in faq and version_history. Signed-off-by: Manish Kumar --- docs/root/faq/configuration/timeouts.rst | 4 ++-- docs/root/version_history/current.rst | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/root/faq/configuration/timeouts.rst b/docs/root/faq/configuration/timeouts.rst index fd530474ea344..43a228e0c27a5 100644 --- a/docs/root/faq/configuration/timeouts.rst +++ b/docs/root/faq/configuration/timeouts.rst @@ -104,8 +104,8 @@ TCP --- * The cluster :ref:`connect_timeout ` specifies the amount - of time Envoy will wait for an upstream TCP connection to be established. This timeout has no - default, but is required in the configuration. + of time Envoy will wait for an upstream TCP connection to be established. If this value is not set, + a default value of 5 seconds will be used. .. attention:: diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index bd265aaccff6b..332df94449503 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -20,6 +20,7 @@ Minor Behavior Changes requests to S3, ES or Glacier, which used the literal string ``UNSIGNED-PAYLOAD``. Buffering can be now be disabled in favor of using unsigned payloads with compatible services via the new `use_unsigned_payload` filter option (default false). +* cluster: added default value of 5 seconds for :ref:`connect_timeout `. * http: disable the integration between :ref:`ExtensionWithMatcher ` and HTTP filters by default to reflects its experimental status. This feature can be enabled by seting ``envoy.reloadable_features.experimental_matching_api`` to true.