Added configurable upstream connect timeout #4326

tsaarni · 2022-02-07T14:32:51Z

This change adds optional configurable timeout for upstream TCP connections. Currently Envoy is programmed with a hardcoded value of 2 seconds. With this change, user will be able to change this global timeout by setting a variable in the config file or ContourConfiguration.

Fixes #2264

Signed-off-by: Tero Saarni tero.saarni@est.tech

Signed-off-by: Tero Saarni <tero.saarni@est.tech>

codecov · 2022-02-07T15:10:52Z

Codecov Report

Merging #4326 (ade6c28) into main (15b2c6e) will increase coverage by 0.44%.
The diff coverage is 80.59%.

❗ Current head ade6c28 differs from pull request most recent head 5c7808e. Consider uploading reports for the commit 5c7808e to get more accurate results

@@            Coverage Diff             @@
##             main    #4326      +/-   ##
==========================================
+ Coverage   77.84%   78.29%   +0.44%     
==========================================
  Files         112      112              
  Lines       10029    10052      +23     
==========================================
+ Hits         7807     7870      +63     
+ Misses       2043     2001      -42     
- Partials      179      181       +2

Impacted Files	Coverage Δ
cmd/contour/ingressstatus.go	`34.78% <0.00%> (ø)`
cmd/contour/serve.go	`13.01% <16.66%> (+0.47%)`	⬆️
internal/envoy/v3/cluster.go	`98.63% <50.00%> (-1.37%)`	⬇️
internal/debug/dot.go	`54.54% <87.50%> (+54.54%)`	⬆️
cmd/contour/servecontext.go	`83.56% <100.00%> (+0.11%)`	⬆️
internal/contourconfig/contourconfiguration.go	`100.00% <100.00%> (ø)`
internal/dag/cache.go	`94.82% <100.00%> (+0.66%)`	⬆️
internal/dag/dag.go	`94.94% <100.00%> (ø)`
internal/dag/extension_processor.go	`95.18% <100.00%> (+0.05%)`	⬆️
internal/dag/gatewayapi_processor.go	`96.94% <100.00%> (+<0.01%)`	⬆️
... and 11 more

sunjayBhatia · 2022-02-07T19:09:12Z

pkg/config/parameters.go

@@ -463,6 +471,10 @@ func (t TimeoutParameters) Validate() error {
 		return fmt.Errorf("connection shutdown grace period %q: %w", t.ConnectionShutdownGracePeriod, err)
 	}

+	if err := v(t.ConnectTimeout); err != nil {


using v() here will allow infinity as a valid timeout, which we probably don't want, as you mentioned elsewhere

Missed that, thanks @sunjayBhatia! Now fixed.

Signed-off-by: Tero Saarni <tero.saarni@est.tech>

skriss

Looks reasonable to me, just a couple godoc nits

apis/projectcontour/v1alpha1/contourconfig.go

pkg/config/parameters.go

Signed-off-by: Tero Saarni <tero.saarni@est.tech>

…nfig CRD to result in default timeout Signed-off-by: Tero Saarni <tero.saarni@est.tech>

tsaarni · 2022-02-10T16:46:18Z

Looks reasonable to me, just a couple godoc nits

Thanks! I've fixed the field names.

I also realized that default 2s timeout was not taking place anymore with empty ContourConfiguration, which I fixed as well.

sunjayBhatia · 2022-02-10T20:48:41Z

Looks reasonable to me, just a couple godoc nits

Thanks! I've fixed the field names.

I also realized that default 2s timeout was not taking place anymore with empty ContourConfiguration, which I fixed as well.

I guess similar to other discussions, do we want to have the default take place via kubebuilder as well?

sunjayBhatia · 2022-02-10T20:54:39Z

internal/envoy/v3/cluster_test.go

+		"cluster with connect timeout set": {
+			cluster: &dag.Cluster{
+				Upstream:       service(s1),
+				ConnectTimeout: 2 * time.Second,


nit: since the test merges the want and default cluster config, maybe use a different value to ensure theres no chance a mistake in implementation still makes this test pass

Good catch, changed to 10 seconds.

sunjayBhatia

just one small comment on a test but otherwise lgtm

skriss

LGTM pending Sunjay's comment

youngnick · 2022-02-11T05:30:55Z

Let's bring this in without a kubebuilder default for now, and I'll come back to what we can do there next week.

Signed-off-by: Tero Saarni <tero.saarni@est.tech>

tsaarni · 2022-02-14T12:54:20Z

In addition to fixing @sunjayBhatia's comment I've also added connect timeout to contourconfiguration_test.go for coverage improvement.

* Added configurable upstream connect timeout Signed-off-by: Tero Saarni <tero.saarni@est.tech>

tsaarni requested a review from a team as a code owner February 7, 2022 14:32

tsaarni requested review from stevesloka and skriss and removed request for a team February 7, 2022 14:32

tsaarni force-pushed the tsaarni/issue2264 branch from 0a87f4d to e13a253 Compare February 7, 2022 14:48

tsaarni added the release-note/small A small change that needs one line of explanation in the release notes. label Feb 7, 2022

Added configurable upstream connect timeout

22ddb70

Signed-off-by: Tero Saarni <tero.saarni@est.tech>

tsaarni force-pushed the tsaarni/issue2264 branch from e13a253 to 22ddb70 Compare February 7, 2022 15:00

sunjayBhatia reviewed Feb 7, 2022

View reviewed changes

Do not allow infinite for connect-timeout config file parameter.

807d0e6

Signed-off-by: Tero Saarni <tero.saarni@est.tech>

tsaarni force-pushed the tsaarni/issue2264 branch from 72df8b4 to 807d0e6 Compare February 8, 2022 12:31

youngnick mentioned this pull request Feb 8, 2022

Downstream timeout errors #4330

Closed

skriss requested changes Feb 9, 2022

View reviewed changes

apis/projectcontour/v1alpha1/contourconfig.go Outdated Show resolved Hide resolved

pkg/config/parameters.go Outdated Show resolved Hide resolved

sunjayBhatia self-requested a review February 9, 2022 20:47

tsaarni added 2 commits February 10, 2022 14:57

Fixed field names to ConnectTimeout

8a4825f

Signed-off-by: Tero Saarni <tero.saarni@est.tech>

The default timeout needs to be set in cluster.go for empty ContourCo…

a264744

…nfig CRD to result in default timeout Signed-off-by: Tero Saarni <tero.saarni@est.tech>

tsaarni force-pushed the tsaarni/issue2264 branch from 36387e5 to a264744 Compare February 10, 2022 15:53

sunjayBhatia reviewed Feb 10, 2022

View reviewed changes

skriss approved these changes Feb 10, 2022

View reviewed changes

Do not use default timeout in test case

ec030f8

Signed-off-by: Tero Saarni <tero.saarni@est.tech>

tsaarni force-pushed the tsaarni/issue2264 branch from cd25844 to 4fcadcf Compare February 14, 2022 10:53

improved test coverage

5c7808e

Signed-off-by: Tero Saarni <tero.saarni@est.tech>

tsaarni force-pushed the tsaarni/issue2264 branch from 4fcadcf to 5c7808e Compare February 14, 2022 11:08

youngnick approved these changes Feb 14, 2022

View reviewed changes

sunjayBhatia approved these changes Feb 14, 2022

View reviewed changes

tsaarni merged commit aa134a2 into projectcontour:main Feb 15, 2022

tsaarni added a commit to Nordix/contour that referenced this pull request Apr 20, 2022

Added configurable upstream connect timeout (projectcontour#4326)

03117d2

* Added configurable upstream connect timeout Signed-off-by: Tero Saarni <tero.saarni@est.tech>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added configurable upstream connect timeout #4326

Added configurable upstream connect timeout #4326

tsaarni commented Feb 7, 2022 •

edited

Loading

codecov bot commented Feb 7, 2022 •

edited

Loading

sunjayBhatia Feb 7, 2022

tsaarni Feb 8, 2022

skriss left a comment

tsaarni commented Feb 10, 2022

sunjayBhatia commented Feb 10, 2022

sunjayBhatia Feb 10, 2022

tsaarni Feb 14, 2022

sunjayBhatia left a comment

skriss left a comment

youngnick commented Feb 11, 2022

tsaarni commented Feb 14, 2022

Added configurable upstream connect timeout #4326

Added configurable upstream connect timeout #4326

Conversation

tsaarni commented Feb 7, 2022 • edited Loading

codecov bot commented Feb 7, 2022 • edited Loading

Codecov Report

sunjayBhatia Feb 7, 2022

Choose a reason for hiding this comment

tsaarni Feb 8, 2022

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

tsaarni commented Feb 10, 2022

sunjayBhatia commented Feb 10, 2022

sunjayBhatia Feb 10, 2022

Choose a reason for hiding this comment

tsaarni Feb 14, 2022

Choose a reason for hiding this comment

sunjayBhatia left a comment

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

youngnick commented Feb 11, 2022

tsaarni commented Feb 14, 2022

tsaarni commented Feb 7, 2022 •

edited

Loading

codecov bot commented Feb 7, 2022 •

edited

Loading