✨ Enable configuring the kube config timeout

[cnmcavoy]

What this PR does / why we need it:

Allows configuring the kubernetes client configuration timeout. Also moves the other related configuration into a standard place and reduces duplicated logic in various main functions.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #8864

[k8s-ci-robot]

Welcome @cnmcavoy!

It looks like this is your first PR to kubernetes-sigs/cluster-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @cnmcavoy. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign cecilerobertmichon for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[nojnhuh]

Is there a reason Timeout is not also defined here?

[cnmcavoy]

The kubernetes clients used for the management clusters never had a timeout assigned in any of the various main.go and I didn't want to introduce a unintended change in behavior.

If we prefer consistency, I can add the timeout here as well.

[nojnhuh]

This helper is very obviously pulling its weight! 🙌

[nojnhuh]

/ok-to-test

[fabriziopandini]

/hold
If I'm not wrong the solution proposed is not addressing the problem being discussed (making the timeout for draining workload clusters configurable), but instead it configures the timeout for management cluster clients.
see also #8864 (comment)

[killianmuldoon]

If I'm not wrong the solution proposed is not addressing the problem being discussed (making the timeout for draining workload clusters configurable), but instead it configures the timeout for management cluster clients.

Agreed this doesn't address the issue with drain, but this does configure both workload and management cluster clients AFAIK - just not for drain.

[fabriziopandini]

Agreed this doesn't address the issue with drain, but this does configure both workload and management cluster clients AFAIK - just not for drain.

I'm not sure this applies to workload clusters, could you kindly point me to where this is happening

[killianmuldoon]

I'm not sure this applies to workload clusters, could you kindly point me to where this is happening

You can see this on the change in #8882. The defaultClientTimeout is set in the remote.RESTConfig function. This is called in a number of places to create clients for workload clusters, including in the ClusterCacheTracker's createAccessor here:

cluster-api/controllers/remote/cluster_cache_tracker.go

Line 275 in ed31880

config, err := RESTConfig(ctx, t.controllerName, t.client, cluster)

[sbueringer]

This feels like a very implicit way of configuring the ClusterCacheTracker

[cnmcavoy]

This feels like a very implicit way of configuring the ClusterCacheTracker

Can you clarify? I'm not really sure what you mean.

If I'm not wrong the solution proposed is not addressing the problem being discussed (making the timeout for draining workload clusters configurable), but instead it configures the timeout for management cluster clients.

I commented over in the issue, but the HTTP timeout is very much what we are interested in. If we feel that making the QPS flags consistent is an unwanted change, I can revert that part and make this PR only the timeout.

[sbueringer]

This feels like a very implicit way of configuring the ClusterCacheTracker

Can you clarify? I'm not really sure what you mean.

Sorry that was not very actionable and clear feedback :). What I meant is I don't like to introduce flags, which are then writing package global variables and thus affect the behavior of the ClusterCacheTracker and the RESTConfig func.

There are folks using CAPI as a library and use ClusterCacheTracker directly. I don't want to force them to have to use the flags to be able to configure the timeouts.

I would prefer something like what we did with flags.TLSOptions. Provide a util to define the flags and then explicitly hand them over (in the TLSOptions case to the webhookserver, in our case here to the ClusterCacheTracker).

If I'm not wrong the solution proposed is not addressing the problem being discussed (making the timeout for draining workload clusters configurable), but instead it configures the timeout for management cluster clients.

I commented over in the issue, but the HTTP timeout is very much what we are interested in. If we feel that making the QPS flags consistent is an unwanted change, I can revert that part and make this PR only the timeout.

I think you're right that this would change the timeout used for draining (but also more). Let's continue the discussion on the issue and once we have consensus come back to the PR.

[cnmcavoy]

Closed in preference to #8917