⚠️ Set user agent and timeout for remote cluster client #4060

nader-ziada · 2021-01-08T15:34:48Z

What this PR does / why we need it:

set a UserAgent and Timeout for remote cluster client

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 #2993

nader-ziada · 2021-01-08T15:34:58Z

/cc @fabriziopandini

vincepri · 2021-01-08T15:40:23Z

controllers/remote/cluster.go

@@ -26,6 +27,11 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )

+const (
+	defaultClientTimeout   = 10 * time.Second
+	remoteClusterUserAgent = "remote-cluster"


We might want to build something like https://github.com/kubernetes/client-go/blob/9761a13537eb881bb91c13819ba223eda84ac0af/rest/config.go#L463-L477

but specific to Cluster API?

So instead of something like kubernetes/ we could have cluster.x-k8s.io/ for example?

ok cool, I can do that

+1, it might be good to also have a component that is defined at initialization time. That way we could differentiate which component is using the remote client, which could potentially be different controllers or even different controller managers

nader-ziada · 2021-01-08T20:08:38Z

/test pull-cluster-api-test-main

nader-ziada · 2021-01-08T22:31:11Z

/test pull-cluster-api-test-main

is there any chance this is a flake?

vincepri

/lgtm
/assign @CecileRobertMichon @detiber

vincepri · 2021-01-11T20:07:41Z

/milestone v0.4.0

controllers/remote/restconfig.go

vincepri · 2021-01-12T18:08:57Z

controllers/remote/cluster.go

+func NewClusterClient(ctx context.Context, sourceName string, c client.Client, cluster client.ObjectKey) (client.Client, error) {
+	restConfig, err := RESTConfig(ctx, sourceName, c, cluster)


These changes make the PR a breaking change ⚠️

so are you saying we should not do that? or document it? I didn't how else to identity the base component calling

I was waiting if see if reviewers think this approach is good before updating the PR description to say its breaking

@detiber is that more in the direction you had in mind?

Mostly document it

This also means this PR should not be backported to 0.3 (not sure if there were plans to)

I don't think there are any plans to back port this one

nader-ziada · 2021-01-12T18:10:24Z

/test pull-cluster-api-test-main

nader-ziada · 2021-01-12T18:47:01Z

/test pull-cluster-api-apidiff-main

nader-ziada · 2021-01-12T18:47:13Z

/test pull-cluster-api-test-main

controlplane/kubeadm/internal/cluster.go

nader-ziada · 2021-01-13T14:11:06Z

/test pull-cluster-api-test-main

nader-ziada · 2021-01-13T15:32:45Z

/test pull-cluster-api-test-main

nader-ziada · 2021-01-13T18:56:23Z

/test pull-cluster-api-test-main

vincepri · 2021-01-14T17:31:18Z

/test pull-cluster-api-test-main

fabriziopandini · 2021-01-15T10:58:44Z

Is this a flake?

I'm not sure, in my analysis of recent failures, I don't remember other cases of errors in the path helper tests.

/test pull-cluster-api-test-main

nader-ziada · 2021-01-15T13:53:26Z

Is this a flake?

I'm not sure, in my analysis of recent failures, I don't remember other cases of errors in the path helper tests.

/test pull-cluster-api-test-main

its been occasionally passing with me changing anything, but will take another look

vincepri

/lgtm
/assign @detiber

nader-ziada · 2021-01-20T14:48:53Z

@detiber any comments? Thanks

CecileRobertMichon · 2021-01-28T23:06:28Z

bootstrap/kubeadm/controllers/kubeadmconfig_controller.go

@@ -266,7 +266,7 @@ func (r *KubeadmConfigReconciler) refreshBootstrapToken(ctx context.Context, con
 	log := ctrl.LoggerFrom(ctx)
 	token := config.Spec.JoinConfiguration.Discovery.BootstrapToken.Token

-	remoteClient, err := r.remoteClientGetter(ctx, r.Client, util.ObjectKey(cluster))
+	remoteClient, err := r.remoteClientGetter(ctx, "kubeadmconfig-controller", r.Client, util.ObjectKey(cluster))


can this be a const?

same comment for all other source names

updated all of the names to const

nader-ziada · 2021-01-29T00:42:58Z

/test pull-cluster-api-e2e-main

nader-ziada · 2021-01-29T00:54:00Z

/retest

k8s-ci-robot · 2021-01-29T00:56:20Z

@nader-ziada: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-apidiff-main	`3bcf832`	link	`/test pull-cluster-api-apidiff-main`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

nader-ziada · 2021-01-29T13:49:42Z

/test pull-cluster-api-e2e-main

nader-ziada · 2021-01-29T15:22:45Z

@CecileRobertMichon I think its ready now, there was some flakes with the tests last night, but now seems fine, PTAL. Thanks

detiber · 2021-01-29T16:04:56Z

Apologies for the delay
/lgtm

vincepri

/approve

k8s-ci-robot · 2021-01-29T16:52:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vincepri]

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

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 8, 2021

k8s-ci-robot requested review from JoelSpeed and vincepri January 8, 2021 15:34

k8s-ci-robot requested a review from fabriziopandini January 8, 2021 15:34

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 8, 2021

vincepri reviewed Jan 8, 2021

View reviewed changes

nader-ziada force-pushed the remote-user-agent branch from 6b7ab1d to ec6570f Compare January 8, 2021 19:14

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 8, 2021

nader-ziada force-pushed the remote-user-agent branch from ec6570f to e127b3a Compare January 8, 2021 19:25

nader-ziada force-pushed the remote-user-agent branch 2 times, most recently from 894f33c to 8258eec Compare January 9, 2021 00:24

vincepri approved these changes Jan 11, 2021

View reviewed changes

k8s-ci-robot assigned CecileRobertMichon, detiber and vincepri Jan 11, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 11, 2021

k8s-ci-robot added this to the v0.4.0 milestone Jan 11, 2021

detiber reviewed Jan 11, 2021

View reviewed changes

controllers/remote/restconfig.go Show resolved Hide resolved

nader-ziada force-pushed the remote-user-agent branch from 8258eec to f1eb9cf Compare January 12, 2021 17:42

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 12, 2021

vincepri reviewed Jan 12, 2021

View reviewed changes

fabriziopandini reviewed Jan 13, 2021

View reviewed changes

controlplane/kubeadm/internal/cluster.go Outdated Show resolved Hide resolved

nader-ziada force-pushed the remote-user-agent branch from f1eb9cf to b6036cf Compare January 13, 2021 14:01

nader-ziada changed the title ~~🌱 Set user agent and timeout for remote cluster client~~ ⚠️ Set user agent and timeout for remote cluster client Jan 13, 2021

vincepri approved these changes Jan 15, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 15, 2021

CecileRobertMichon reviewed Jan 28, 2021

View reviewed changes

nader-ziada force-pushed the remote-user-agent branch from b6036cf to b6fa994 Compare January 29, 2021 00:26

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 29, 2021

set user agent and timeout for remote cluster client

3bcf832

nader-ziada force-pushed the remote-user-agent branch from b6fa994 to 3bcf832 Compare January 29, 2021 00:33

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 29, 2021

vincepri approved these changes Jan 29, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 29, 2021

k8s-ci-robot merged commit 5815384 into kubernetes-sigs:master Jan 29, 2021

This pull request was closed.

		func NewClusterClient(ctx context.Context, sourceName string, c client.Client, cluster client.ObjectKey) (client.Client, error) {
		restConfig, err := RESTConfig(ctx, sourceName, c, cluster)

⚠️ Set user agent and timeout for remote cluster client #4060

⚠️ Set user agent and timeout for remote cluster client #4060

Conversation

nader-ziada commented Jan 8, 2021

nader-ziada commented Jan 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nader-ziada commented Jan 8, 2021

nader-ziada commented Jan 8, 2021

vincepri left a comment

Choose a reason for hiding this comment

vincepri commented Jan 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nader-ziada commented Jan 12, 2021

nader-ziada commented Jan 12, 2021

nader-ziada commented Jan 12, 2021

nader-ziada commented Jan 13, 2021

nader-ziada commented Jan 13, 2021

nader-ziada commented Jan 13, 2021

vincepri commented Jan 14, 2021

fabriziopandini commented Jan 15, 2021

nader-ziada commented Jan 15, 2021

vincepri left a comment

Choose a reason for hiding this comment

nader-ziada commented Jan 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nader-ziada commented Jan 29, 2021

nader-ziada commented Jan 29, 2021

k8s-ci-robot commented Jan 29, 2021 • edited Loading

nader-ziada commented Jan 29, 2021

nader-ziada commented Jan 29, 2021

detiber commented Jan 29, 2021

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 29, 2021

k8s-ci-robot commented Jan 29, 2021 •

edited

Loading