feat(appset): make K8s client configurable #16945

[alexymantha]

There is currently no way to tweak the Kubernetes client configuration for the applicationset-controller, it defaults to the values set in controller-runtime:

https://github.com/kubernetes-sigs/controller-runtime/blob/1f5b39fa59d15fae78e521c9c9f2acabbbb3ea17/pkg/client/config/config.go#L101-L106

which are pretty low.

We use the ApplicationSet controller with the cluster generators pretty heavily which means we call ListClusters very frequently and we started to get throttled by the client. It was causing the ApplicationSet controller to not be able to process the ApplicationSets fast enough due to the limit of default limit of 30 QPS which caused the queue to slowly build up over time and the ApplicationSet controller was never able to catch up.

Using this PR, I was able to increase the rate limits and fix the performance issues we were having:

For consistency, I think it makes sense to apply the same default client config to the ApplicationSet controller that is applied to the Application controller.

Here is the diff in the client config when applying the same default as the Application Controller:

&rest.Config{
        ... // 9 identical fields
        AuthConfigPersister: nil,
        ExecProvider:        nil,
        TLSClientConfig: rest.TLSClientConfig{
                ... // 2 identical fields
                CertFile: "",
                KeyFile:  "",
-               CAFile:   "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt",
+               CAFile:   "",
                CertData: nil,
                KeyData:  nil,
                ... // 2 identical fields
        },
        UserAgent:          "",
        DisableCompression: false,
-       Transport:          nil,
+       Transport: &transport.bearerAuthRoundTripper{
+               bearer: "--REDACTED--"...,
+               source: &transport.cachingTokenSource{
+                       base:   &transport.fileTokenSource{path: "/var/run/secrets/kubernetes.io/s"..., period: s"1m0s"},
+                       leeway: s"10s",
+                       now:    ⟪0x4957e0⟫,
+               },
+               rt: &http.Transport{
+                       altProto:            atomic.Value{v: map[string]http.RoundTripper{...}},
+                       Proxy:               ⟪0x8a6880⟫,
+                       DialContext:         ⟪0x8bfbc0⟫,
+                       TLSClientConfig:     &tls.Config{RootCAs: &x509.CertPool{...}, NextProtos: []string{...}, MinVersion: 771},
+                       TLSHandshakeTimeout: s"10s",
+                       MaxIdleConns:        500,
+                       MaxIdleConnsPerHost: 500,
+                       MaxConnsPerHost:     500,
+                       ...
+               },
+       },
        WrapTransport:  ⟪0x00⟫,
-       QPS:            20,
+       QPS:            50,
-       Burst:          30,
+       Burst:          100,
        RateLimiter:    nil,
        WarningHandler: nil,
        ... // 3 identical fields
  }

This has been running on our ArgoCD instance for almost a week without any issues.

Checklist:

• Either (a) I've created an enhancement proposal 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).
• The title of the PR conforms to the Toolchain Guide
• 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.
• I have signed off all my commits as required by DCO
• 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).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.
• Optional. My organization is added to USERS.md.
• Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

Closes #16945

[alexymantha]

Looking at the Application controller, these settings do not seem to be documented and are not exposed other than by the environment variable. I'm not sure if it should stay like that for the ApplicationSet as well.

I'll be happy to add some documentation and expose these options correctly if you think it should be done, but in that case it should also be done for the Application controller IMO

[agaudreault]

@jannfis Seems like you worked on that part in the past with #8404 so maybe you can think of an issue with this, but so far LGTM.

@alexymantha see issue #8527 for documentation. If you want to tackle it, it could be in this one or another PR.

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 50.30%. Comparing base (e98d3b2) to head (fff55d5).
Report is 519 commits behind head on master.

Files with missing lines	Patch %	Lines
...t-controller/commands/applicationset_controller.go	50.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #18623      +/-   ##
==========================================
+ Coverage   50.26%   50.30%   +0.04%     
==========================================
  Files         315      315              
  Lines       43125    43130       +5     
==========================================
+ Hits        21677    21698      +21     
+ Misses      18963    18944      -19     
- Partials     2485     2488       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ishitasequeira]

Thanks @alexymantha for the changes. LGTM!