Repository: argoproj/argo-cd
Pull request #8404 (2022-02-07 12:58:49)

feat: Allow fine-tuning of K8s rest client connection properties
This adds some under-the-hood options for fine tuning certain connection parameters of the Kubernetes REST client used by the application controller and API server, using environment variables.

The main motivation behind this is the ability to stabilize operations to clusters at the far edge, which may be on very high latency and very low bandwidth links, and the default timeouts may not be sufficient for proper operations.

It introduces the following environment variables to change the properties from their defaults:

* `ARGOCD_K8S_TCP_TIMEOUT` - (duration) allows to set the general timeout for TCP connections to the K8s API, and also sets the server-side timeout to the same value
* `ARGOCD_K8S_TCP_KEEPALIVE` - (duration) allows to set the interval at which TCP keep alive packets will be sent to the K8s API
* `ARGOCD_K8S_TLS_HANDSHAKE_TIMEOUT` - (duration) allows to set the timeout for waiting on the TLS handshake to complete with the K8s API 
* `ARGOCD_K8S_TCP_IDLE_TIMEOUT` - (duration) allows to set the timeout for closing idle TCP connections to the K8s API server

Signed-off-by: jannfis <jann@mistrust.net>

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the *Details* link next to the DCO action for instructions on how to resolve this.

Checklist:

* [ ] Either (a) I've created an [enhancement proposal](https://github.com/argoproj/argo-cd/issues/new/choose) and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
* [ ] The title of the PR states what changed and the related issues number (used for the release note).
* [ ] I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
* [ ] I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
* [ ] Does this PR require documentation updates?
* [ ] I've updated documentation as required by this PR.
* [ ] Optional. My organization is added to USERS.md.
* [ ] I have signed off all my commits as required by [DCO](https://github.com/argoproj/argoproj/tree/master/community#contributing-to-argo)
* [ ] I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
* [ ] My build is green ([troubleshooting builds](https://argo-cd.readthedocs.io/en/latest/developer-guide/ci/)). 



codecov[bot]: # [Codecov](https://codecov.io/gh/argoproj/argo-cd/pull/8404?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj) Report
> Merging [#8404](https://codecov.io/gh/argoproj/argo-cd/pull/8404?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj) (1d9f964) into [master](https://codecov.io/gh/argoproj/argo-cd/commit/d0cbb1597ca8b93cae081e01c07c5c2e74f580d4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj) (d0cbb15) will **increase** coverage by `0.12%`.
> The diff coverage is `78.57%`.

[![Impacted file tree graph](https://codecov.io/gh/argoproj/argo-cd/pull/8404/graphs/tree.svg?width=650&height=150&src=pr&token=kf5kwuegwE&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj)](https://codecov.io/gh/argoproj/argo-cd/pull/8404?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj)

```diff
@@            Coverage Diff             @@
##           master    #8404      +/-   ##
==========================================
+ Coverage   41.99%   42.12%   +0.12%     
==========================================
  Files         178      178              
  Lines       22905    22919      +14     
==========================================
+ Hits         9620     9654      +34     
+ Misses      11906    11889      -17     
+ Partials     1379     1376       -3     
```


| [Impacted Files](https://codecov.io/gh/argoproj/argo-cd/pull/8404?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj) | Coverage Δ | |
|---|---|---|
| [pkg/apis/application/v1alpha1/types.go](https://codecov.io/gh/argoproj/argo-cd/pull/8404/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj#diff-cGtnL2FwaXMvYXBwbGljYXRpb24vdjFhbHBoYTEvdHlwZXMuZ28=) | `55.08% <0.00%> (-0.25%)` | :arrow_down: |
| [pkg/apis/application/v1alpha1/cluster\_constants.go](https://codecov.io/gh/argoproj/argo-cd/pull/8404/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj#diff-cGtnL2FwaXMvYXBwbGljYXRpb24vdjFhbHBoYTEvY2x1c3Rlcl9jb25zdGFudHMuZ28=) | `100.00% <100.00%> (+81.81%)` | :arrow_up: |
| [util/env/env.go](https://codecov.io/gh/argoproj/argo-cd/pull/8404/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj#diff-dXRpbC9lbnYvZW52Lmdv) | `90.90% <100.00%> (+29.37%)` | :arrow_up: |
| [util/settings/settings.go](https://codecov.io/gh/argoproj/argo-cd/pull/8404/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj#diff-dXRpbC9zZXR0aW5ncy9zZXR0aW5ncy5nbw==) | `47.08% <0.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/argoproj/argo-cd/pull/8404?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/argoproj/argo-cd/pull/8404?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj). Last update [d0cbb15...1d9f964](https://codecov.io/gh/argoproj/argo-cd/pull/8404?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=argoproj).


Patch:
diff --git a/docs/operator-manual/cluster-bootstrapping.md b/docs/operator-manual/cluster-bootstrapping.md
index 962b0d1e6..d7f7138d6 100644
--- a/docs/operator-manual/cluster-bootstrapping.md
+++ b/docs/operator-manual/cluster-bootstrapping.md
@@ -98,7 +98,7 @@ View [the example on GitHub](https://github.com/argoproj/argocd-example-apps/tre
 
 ### Cascading deletion
 
-If you want to ensure that child-apps and all of their resources are deleted when the parent-app is deleted make sure to add the appropriate [finalizer](https://argo-cd.readthedocs.io/en/latest/user-guide/app_deletion/#about-the-deletion-finalizer) to your `Application` definition
+If you want to ensure that child-apps and all of their resources are deleted when the parent-app is deleted make sure to add the appropriate [finalizer](../user-guide/app_deletion.md#about-the-deletion-finalizer) to your `Application` definition
 
 ```yaml
 apiVersion: argoproj.io/v1alpha1
diff --git a/pkg/apis/application/v1alpha1/cluster_constants.go b/pkg/apis/application/v1alpha1/cluster_constants.go
index 8502df712..13895eaf6 100644
--- a/pkg/apis/application/v1alpha1/cluster_constants.go
+++ b/pkg/apis/application/v1alpha1/cluster_constants.go
@@ -1,8 +1,10 @@
 package v1alpha1
 
 import (
-	"os"
-	"strconv"
+	"math"
+	"time"
+
+	"github.com/argoproj/argo-cd/v2/util/env"
 )
 
 const (
@@ -19,39 +21,44 @@ const (
 
 	// EnvK8sClientMaxIdleConnections is the number of max idle connections in K8s REST client HTTP transport (default: 500)
 	EnvK8sClientMaxIdleConnections = "ARGOCD_K8S_CLIENT_MAX_IDLE_CONNECTIONS"
+
+	// EnvK8sTCPTimeout is the duration for TCP timeouts when communicating with K8s API servers
+	EnvK8sTCPTimeout = "ARGOCD_K8S_TCP_TIMEOUT"
+
+	// EnvK8sTCPKeepalive is the interval for TCP keep alive probes to be sent when communicating with K8s API servers
+	EnvK8sTCPKeepAlive = "ARGOCD_K8S_TCP_KEEPALIVE"
+
+	// EnvK8sTLSHandshakeTimeout is the duration for TLS handshake timeouts when establishing connections to K8s API servers
+	EnvK8sTLSHandshakeTimeout = "ARGOCD_K8S_TLS_HANDSHAKE_TIMEOUT"
+
+	// EnvK8sTCPIdleConnTimeout is the duration when idle TCP connection to the K8s API servers should timeout
+	EnvK8sTCPIdleConnTimeout = "ARGOCD_K8S_TCP_IDLE_TIMEOUT"
 )
 
-// Constants associated with the Cluster API
+// Configuration variables associated with the Cluster API
 var (
 
 	// K8sClientConfigQPS controls the QPS to be used in K8s REST client configs
-	K8sClientConfigQPS float32 = 50
+	K8sClientConfigQPS float32 = env.ParseFloatFromEnv(EnvK8sClientQPS, 50, 0, math.MaxFloat32)
 
 	// K8sClientConfigBurst controls the burst to be used in K8s REST client configs
-	K8sClientConfigBurst int = 100
+	K8sClientConfigBurst int = env.ParseNumFromEnv(EnvK8sClientBurst, int(2*K8sClientConfigQPS), 0, math.MaxInt32)
 
 	// K8sMaxIdleConnections controls the number of max idle connections in K8s REST client HTTP transport
-	K8sMaxIdleConnections = 500
-)
+	K8sMaxIdleConnections = env.ParseNumFromEnv(EnvK8sClientMaxIdleConnections, 500, 0, math.MaxInt32)
 
-func init() {
-	if envQPS := os.Getenv(EnvK8sClientQPS); envQPS != "" {
-		if qps, err := strconv.ParseFloat(envQPS, 32); err == nil {
-			K8sClientConfigQPS = float32(qps)
-		}
-	}
-	if envBurst := os.Getenv(EnvK8sClientBurst); envBurst != "" {
-		if burst, err := strconv.Atoi(envBurst); err == nil {
-			K8sClientConfigBurst = burst
-		}
-	} else {
-		K8sClientConfigBurst = 2 * int(K8sClientConfigQPS)
-	}
-
-	if envMaxConn := os.Getenv(EnvK8sClientMaxIdleConnections); envMaxConn != "" {
-		if maxConn, err := strconv.Atoi(envMaxConn); err == nil {
-			K8sMaxIdleConnections = maxConn
-		}
-	}
-
-}
+	// K8sTLSHandshakeTimeout defines the maximum duration to wait for a TLS handshake to complete
+	K8sTLSHandshakeTimeout = env.ParseDurationFromEnv(EnvK8sTLSHandshakeTimeout, 10*time.Second, 0, math.MaxInt32)
+
+	// K8sTCPTimeout defines the TCP timeout to use when performing K8s API requests
+	K8sTCPTimeout = env.ParseDurationFromEnv(EnvK8sTCPTimeout, 30*time.Second, 0, math.MaxInt32)
+
+	// K8sTCPKeepAlive defines the interval for sending TCP keep alive to K8s API server
+	K8sTCPKeepAlive = env.ParseDurationFromEnv(EnvK8sTCPKeepAlive, 30*time.Second, 0, math.MaxInt32)
+
+	// K8sTCPIdleConnTimeout defines the duration for keeping idle TCP connections to the K8s API server
+	K8sTCPIdleConnTimeout = env.ParseDurationFromEnv(EnvK8sTCPIdleConnTimeout, 5*time.Minute, 0, math.MaxInt32)
+
+	// K8sServerSideTimeout defines which server side timeout to send with each API request
+	K8sServerSideTimeout = env.ParseDurationFromEnv(EnvK8sTCPTimeout, 32*time.Second, 0, math.MaxInt32)
+)
diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go
index 304a8cefd..3ae40f032 100644
--- a/pkg/apis/application/v1alpha1/types.go
+++ b/pkg/apis/application/v1alpha1/types.go
@@ -2353,18 +2353,19 @@ func SetK8SConfigDefaults(config *rest.Config) error {
 	}
 
 	dial := (&net.Dialer{
-		Timeout:   30 * time.Second,
-		KeepAlive: 30 * time.Second,
+		Timeout:   K8sTCPTimeout,
+		KeepAlive: K8sTCPKeepAlive,
 	}).DialContext
 	transport := utilnet.SetTransportDefaults(&http.Transport{
 		Proxy:               http.ProxyFromEnvironment,
-		TLSHandshakeTimeout: 10 * time.Second,
+		TLSHandshakeTimeout: K8sTLSHandshakeTimeout,
 		TLSClientConfig:     tlsConfig,
 		MaxIdleConns:        K8sMaxIdleConnections,
 		MaxIdleConnsPerHost: K8sMaxIdleConnections,
 		MaxConnsPerHost:     K8sMaxIdleConnections,
 		DialContext:         dial,
 		DisableCompression:  config.DisableCompression,
+		IdleConnTimeout:     K8sTCPIdleConnTimeout,
 	})
 	tr, err := rest.HTTPWrappersForConfig(config, transport)
 	if err != nil {
@@ -2376,6 +2377,9 @@ func SetK8SConfigDefaults(config *rest.Config) error {
 	config.AuthProvider = nil
 	config.ExecProvider = nil
 
+	// Set server-side timeout
+	config.Timeout = K8sServerSideTimeout
+
 	config.Transport = tr
 	return nil
 }
@@ -2459,6 +2463,7 @@ func (c *Cluster) RawRestConfig() *rest.Config {
 	if err != nil {
 		panic(fmt.Sprintf("Unable to create K8s REST config: %v", err))
 	}
+	config.Timeout = K8sServerSideTimeout
 	return config
 }
 
diff --git a/ui/src/app/applications/components/application-details/application-details.scss b/ui/src/app/applications/components/application-details/application-details.scss
index c73fd9537..36afb5c00 100644
--- a/ui/src/app/applications/components/application-details/application-details.scss
+++ b/ui/src/app/applications/components/application-details/application-details.scss
@@ -8,7 +8,7 @@ $header: 120px;
         position: fixed;
         left: 60px;
         right: 0;
-        z-index: 2;
+        z-index: 3;
         @media screen and (max-width: map-get($breakpoints, xlarge)) {
             top: 150px;
         }
@@ -209,7 +209,7 @@ $header: 120px;
 
     .graph-options-panel {
         margin-left: 10px;
-        z-index: 10;
+        z-index: 1;
         padding: 5px;
         display: inline-block;
         background-color: $argo-color-gray-1;
diff --git a/ui/src/app/applications/components/filter/filter.scss b/ui/src/app/applications/components/filter/filter.scss
index 1cdc939af..4651ca381 100644
--- a/ui/src/app/applications/components/filter/filter.scss
+++ b/ui/src/app/applications/components/filter/filter.scss
@@ -92,7 +92,7 @@
         padding: 10px;
         overflow-y: hidden;
         overflow-x: hidden;
-        z-index: 1;
+        z-index: 2;
 
         position: fixed;
         width: 240px;
diff --git a/util/env/env.go b/util/env/env.go
index 19243e882..c86a5a736 100644
--- a/util/env/env.go
+++ b/util/env/env.go
@@ -64,9 +64,38 @@ func ParseInt64FromEnv(env string, defaultValue, min, max int64) int64 {
 	return num
 }
 
+// Helper function to parse a float32 from an environment variable. Returns a
+// default if env is not set, is not parseable to a number, exceeds max (if
+// max is greater than 0) or is less than min (and min is greater than 0).
+//
+// nolint:unparam
+func ParseFloatFromEnv(env string, defaultValue, min, max float32) float32 {
+	str := os.Getenv(env)
+	if str == "" {
+		return defaultValue
+	}
+
+	num, err := strconv.ParseFloat(str, 32)
+	if err != nil {
+		log.Warnf("Could not parse '%s' as a float32 from environment %s", str, env)
+		return defaultValue
+	}
+	if float32(num) < min {
+		log.Warnf("Value in %s is %f, which is less than minimum %f allowed", env, num, min)
+		return defaultValue
+	}
+	if float32(num) > max {
+		log.Warnf("Value in %s is %f, which is greater than maximum %f allowed", env, num, max)
+		return defaultValue
+	}
+	return float32(num)
+}
+
 // Helper function to parse a time duration from an environment variable. Returns a
 // default if env is not set, is not parseable to a duration, exceeds max (if
 // max is greater than 0) or is less than min.
+//
+// nolinit:unparam
 func ParseDurationFromEnv(env string, defaultValue, min, max time.Duration) time.Duration {
 	str := os.Getenv(env)
 	if str == "" {
@@ -99,6 +128,8 @@ func StringFromEnv(env string, defaultValue string) string {
 
 // ParseBoolFromEnv retrieves a boolean value from given environment envVar.
 // Returns default value if envVar is not set.
+//
+// nolinit:unparam
 func ParseBoolFromEnv(envVar string, defaultValue bool) bool {
 	if val := os.Getenv(envVar); val != "" {
 		if strings.ToLower(val) == "true" {


Test patch:
diff --git a/util/env/env_test.go b/util/env/env_test.go
index 1db9ebe0f..10145dca6 100644
--- a/util/env/env_test.go
+++ b/util/env/env_test.go
@@ -1,7 +1,9 @@
 package env
 
 import (
+	"fmt"
 	"io"
+	"math"
 	"os"
 	"testing"
 	"time"
@@ -62,6 +64,84 @@ func TestParseNumFromEnv_OutOfRangeValueSet(t *testing.T) {
 	assert.Equal(t, 10, num)
 }
 
+func TestParseFloatFromEnv(t *testing.T) {
+	t.Run("Env not set", func(t *testing.T) {
+		closer := setEnv(t, "test", "")
+		defer util.Close(closer)
+		f := ParseFloatFromEnv("test", 1, 0, math.MaxFloat32)
+		assert.Equal(t, float32(1.0), f)
+	})
+	t.Run("Parse valid float", func(t *testing.T) {
+		closer := setEnv(t, "test", "2.5")
+		defer util.Close(closer)
+		f := ParseFloatFromEnv("test", 1, 0, math.MaxFloat32)
+		assert.Equal(t, float32(2.5), f)
+	})
+	t.Run("Parse valid integer as float", func(t *testing.T) {
+		closer := setEnv(t, "test", "2")
+		defer util.Close(closer)
+		f := ParseFloatFromEnv("test", 1, 0, math.MaxFloat32)
+		assert.Equal(t, float32(2.0), f)
+	})
+	t.Run("Parse invalid value", func(t *testing.T) {
+		closer := setEnv(t, "test", "foo")
+		defer util.Close(closer)
+		f := ParseFloatFromEnv("test", 1, 0, math.MaxFloat32)
+		assert.Equal(t, float32(1.0), f)
+	})
+	t.Run("Float lesser than allowed", func(t *testing.T) {
+		closer := setEnv(t, "test", "-2.0")
+		defer util.Close(closer)
+		f := ParseFloatFromEnv("test", 1, 0, math.MaxFloat32)
+		assert.Equal(t, float32(1.0), f)
+	})
+	t.Run("Float greater than allowed", func(t *testing.T) {
+		closer := setEnv(t, "test", "5.0")
+		defer util.Close(closer)
+		f := ParseFloatFromEnv("test", 1, 0, 4)
+		assert.Equal(t, float32(1.0), f)
+	})
+	t.Run("Check float overflow returning default value", func(t *testing.T) {
+		closer := setEnv(t, "test", fmt.Sprintf("%f", math.MaxFloat32*2))
+		defer util.Close(closer)
+		f := ParseFloatFromEnv("test", 1, 0, math.MaxFloat32)
+		assert.Equal(t, float32(1.0), f)
+	})
+}
+
+func TestParseInt64FromEnv(t *testing.T) {
+	t.Run("Env not set", func(t *testing.T) {
+		closer := setEnv(t, "test", "")
+		defer util.Close(closer)
+		i := ParseInt64FromEnv("test", 1, 0, math.MaxInt64)
+		assert.Equal(t, int64(1), i)
+	})
+	t.Run("Parse valid int64", func(t *testing.T) {
+		closer := setEnv(t, "test", "3")
+		defer util.Close(closer)
+		i := ParseInt64FromEnv("test", 1, 0, math.MaxInt64)
+		assert.Equal(t, int64(3), i)
+	})
+	t.Run("Parse invalid value", func(t *testing.T) {
+		closer := setEnv(t, "test", "foo")
+		defer util.Close(closer)
+		i := ParseInt64FromEnv("test", 1, 0, math.MaxInt64)
+		assert.Equal(t, int64(1), i)
+	})
+	t.Run("Int64 lesser than allowed", func(t *testing.T) {
+		closer := setEnv(t, "test", "-2")
+		defer util.Close(closer)
+		i := ParseInt64FromEnv("test", 1, 0, math.MaxInt64)
+		assert.Equal(t, int64(1), i)
+	})
+	t.Run("Int64 greater than allowed", func(t *testing.T) {
+		closer := setEnv(t, "test", "5")
+		defer util.Close(closer)
+		i := ParseInt64FromEnv("test", 1, 0, 4)
+		assert.Equal(t, int64(1), i)
+	})
+}
+
 func TestParseDurationFromEnv(t *testing.T) {
 	testKey := "key"
 	defaultVal := 2 * time.Second