ApplicationController sync operations should respect HTTP transport settings

jMarkP commented 3 months ago

Checklist:

[ ] I've searched in the docs and FAQ for my answer: https://bit.ly/argocd-faq.
[ ] I've included steps to reproduce the bug.
[ ] I've pasted the output of argocd version.

Describe the bug

While digging into a separate issue on our installation, I noticed that https://github.com/argoproj/argo-cd/blob/master/pkg/apis/application/v1alpha1/types.go provides two functions to get K8s REST config for a cluster:

RawRestConfig() - fills in a client-go Config object with the settings from the cluster secret
RESTConfig() - wraps the above with some config defaults like timeouts, retries, QPS etc

The controller code uses the former for sync operations, and so settings made by the user like ARGOCD_K8SCLIENT_RETRY_MAX won't be applied as far as I can see. A naive reading of the docs would imply this is a global setting.

I realise this may be intentional, given the complexity around running a sync operation - and given the fact that RawRestConfig() is a public function and so presumably intended for use outside. I've not fully walked through all the code to see if these settings are replicated in a different way. I thought it was worth raising though in case this is an oversight.

Expected behavior

Settings which apply to K8s client operations in one part of the system should affect all K8s client calls.

Version

HEAD of git

agaudreault commented 3 months ago

@jMarkP Can you share the reason you would want to apply these settings? are you currently having performance issues?

@jannfis might be interested to this one since he made the initial implemention iirc

jMarkP commented 3 months ago

@agaudreault thanks! I've been investigating an intermittent authentication error on sync in our setup, and one thing that occurred to me was to look into retries to at least make the errors have less of an impact, and that's when I found the difference is usage.

In the end I think I've found the underlying source of the issue to fix it for good, so I don't think I'll need to change any of these settings. I just raised it in case it's a surprising thing that these config defaults (things like timeouts and backoffs) aren't applied universally in ArgoCD. In other words, I'm not blocked in any way on this. It's just an FYI more than anything

argoproj / argo-cd

ApplicationController sync operations should respect HTTP transport settings #18825