Fixes zone/region labels setup and kubelet stucking on startup if credentials stored in secret for legacy vSphere cloudprovider. #101028

lobziik · 2021-04-12T12:38:40Z

What this PR does / why we need it:

Disables attempts of obtaining zones within kubelet during initial node registration by vSphere provider if credentials stored in secret.
Setting zone and region labels for node moved to KCM in such case.

If credentials stored in cloud-provider config file as plaintext current behaviour does not change.

Which issue(s) this PR fixes:

Fixes #75175. Kubelet does not stucking on startup with this patch. Zone labels populates for nodes during KCM startup.

Notes:

For proper functioning ClusterRole + ClusterRoleBinding need to be created if RBAC is in use. This should be documented somewhere, would be awesome if anybody will point me a good place for this. In case of lack of permissions labels will not be set.

What type of PR is this?

/kind bug
/sig cloud-provider

/assing @andrewsykim

Does this PR introduce a user-facing change?

Fixed bug with leads to Node goes "Not-ready" state when credentials for vCenter stored in a secret and Zones feature is in use.
Zone labels setup moved to KCM component, kubelet skips this step during startup in such case. If credentials stored in cloud-provider config file as plaintext current behaviour does not change and no action required.

Action required: For proper functioning "kube-system:vsphere-legacy-cloud-provider" should be allowed to update node object if vCenter credentials stored in secret and Zone feature used.

k8s-ci-robot · 2021-04-12T12:38:49Z

Hi @lobziik. Thanks for your PR.

I'm waiting for a kubernetes 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.

Danil-Grigorev · 2021-04-12T12:44:56Z

/ok-to-test

staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere.go

yangjunmyfm192085 · 2021-04-12T15:23:42Z

staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere.go

+func (vs *VSphere) GetZone(ctx context.Context) (cloudprovider.Zone, error) {
+	nodeName, err := vs.CurrentNodeName(ctx, vs.hostName)
+	if err != nil {
+		klog.Errorf("Cannot get node name.")


Suggested change

klog.Errorf("Cannot get node name.")

klog.ErrorS(nil, "Cannot get node name.")

yangjunmyfm192085 · 2021-04-12T15:28:00Z

Is it necessary to structured log migration?

lobziik · 2021-04-13T11:01:47Z

@yangjunmyfm192085 Dunno, in my opinion logs format migration should be done for entire module. Would prefer to keep it consistent with other provider's code for now.

lobziik · 2021-04-13T13:13:57Z

/retest

staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere_test.go

staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere.go

lobziik · 2021-05-20T12:24:49Z

@andrewsykim @cheftako, Hello! Could you please take a brief look on this please? Is this approach reasonable? :)

cheftako · 2021-05-21T16:07:04Z

staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere.go

@@ -894,7 +901,16 @@ func (vs *VSphere) LoadBalancer() (cloudprovider.LoadBalancer, bool) {
 }

 func (vs *VSphere) isZoneEnabled() bool {
-	return vs.cfg != nil && vs.cfg.Labels.Zone != "" && vs.cfg.Labels.Region != ""
+	isEnabled := vs.cfg != nil && vs.cfg.Labels.Zone != "" && vs.cfg.Labels.Region != ""


Is it really both zone and region need to be set or just one of zone and region needs to be set? (I see that was the previous logic, so no problems going forward like that, just checking)

https://vmware.github.io/vsphere-storage-for-kubernetes/documentation/zones.html this doc tells that both need to be set. Case with one parameter does not described at all.

cheftako · 2021-05-25T23:39:24Z

Happy to approve but would like to see one of the primary vsphere contributors lgtm first.

lobziik · 2021-06-22T09:08:14Z

@divyenpatel @andrewsykim @SandeepPissay Hello! Could you please take a look on this please? Would be awesome to get a feedback from vsphere maintainers :)

SandeepPissay · 2021-06-22T17:23:33Z

@lobziik unfortunately I do not have bandwidth to review code changes in legacy vSphere cloud provider. Its also been a while I looked at that code.

jcpowermac · 2021-08-19T12:47:33Z

@lobziik unfortunately I do not have bandwidth to review code changes in legacy vSphere cloud provider. Its also been a while I looked at that code.

@SandeepPissay our mutal customers would like to be able to support zones. If you are unable to review is there someone else in the reviewer/approver list that can?

gnufied · 2021-10-21T14:29:07Z

/assign

lobziik · 2022-01-05T15:11:17Z

Updated pr a bit, what was done:

rebased on top of current master
fixes according @lubronzhan review

I noticed this in the release notes, is kube-syste:vsphere-legacy-cloud-provider an official role or just an example? We should make this clear in the release notes

@andrewsykim, official, i think. I introduced client instantiation here: https://github.com/kubernetes/kubernetes/pull/101028/files#diff-5000d45019379218cc35bc5f967f2731b4dbff230399361c8f6b9a8af3e7e1f0R277
Name is just hardcoded as in other legacy providers. I fixed typo in PR description, thanks for catching that!

lobziik · 2022-01-05T16:00:10Z

/test pull-kubernetes-integration

andrewsykim · 2022-01-05T17:36:52Z

@andrewsykim, official, i think. I introduced client instantiation here: https://github.com/kubernetes/kubernetes/pull/101028/files#diff-5000d45019379218cc35bc5f967f2731b4dbff230399361c8f6b9a8af3e7e1f0R277
Name is just hardcoded as in other legacy providers. I fixed typo in PR description, thanks for catching that!

Fwiw I think this is just used as the user-agent in the client and not for RBAC

lubronzhan · 2022-01-05T23:17:09Z

Thanks for reminding Andrew!
Just realized that. If we don't automatically add the role binding and role before node registration, the code here will try send 3 requests to APIserver and get error of not enough permission, failing to add the labels and won't ever add it again.
Unless customer add those rolebinding and role, then recreate nodes again.
Should we consider adding the rolebinding and role as default?

@andrewsykim, official, i think. I introduced client instantiation here: https://github.com/kubernetes/kubernetes/pull/101028/files#diff-5000d45019379218cc35bc5f967f2731b4dbff230399361c8f6b9a8af3e7e1f0R277
Name is just hardcoded as in other legacy providers. I fixed typo in PR description, thanks for catching that!

Fwiw I think this is just used as the user-agent in the client and not for RBAC

Ok so it means the client will still use the kubelet's service account? Let me check again

lubronzhan · 2022-01-05T23:43:07Z

Looking at the code of ClientOrDie here, looks like the input will be used as service account name to create config

kubernetes/staging/src/k8s.io/controller-manager/pkg/clientbuilder/client_builder_dynamic.go

Lines 121 to 129 in bfdbfa2

    
           cachedTokenSource := transport.NewCachedTokenSource(&tokenSourceImpl{ 
        
           	namespace:          t.Namespace, 
        
           	serviceAccountName: saName, 
        
           	coreClient:         t.CoreClient, 
        
           	expirationSeconds:  t.expirationSeconds, 
        
           	leewayPercent:      t.leewayPercent, 
        
           }) 
        
           configCopy.Wrap(transport.ResettableTokenSourceWrapTransport(cachedTokenSource)) 
        
           t.roundTripperFuncMap[saName] = configCopy.WrapTransport

andrewsykim · 2022-01-06T00:41:24Z

Ah you're right @lubronzhan, I was thinking of the simple client builder:

kubernetes/staging/src/k8s.io/controller-manager/pkg/clientbuilder/client_builder.go

Lines 44 to 48 in bfdbfa2

    
           // Config returns a client config for a fixed client 
        
           func (b SimpleControllerClientBuilder) Config(name string) (*restclient.Config, error) { 
        
           	clientConfig := *b.ClientConfig 
        
           	return restclient.AddUserAgent(&clientConfig, name), nil 
        
           }

andrewsykim · 2022-01-06T00:43:57Z

Should we consider adding the rolebinding and role as default?

We no longer support bootstrapping role/rolebindings specific to cloud providers, these should be handled externally

lubronzhan · 2022-01-10T18:23:03Z

If the role/rolebinding is handled externally, could we run into this issue?

If we don't automatically add the role binding and role before node registration, the code here will try send 3 requests to APIserver and get error of not enough permission, failing to add the labels and won't ever add it again.
Unless customer add those rolebinding and role, then recreate nodes again.

lobziik · 2022-01-11T16:36:49Z

@lubronzhan, I went through the code again and yes, seems this will engage only once - during new Node handling within KCM...

I will need check this again from kubelet perspective, poked around quite a while ago and don't exactly remember how it behaves with secret there.

lobziik · 2022-01-12T13:27:14Z

I dug into kubelet code, and, as far as I can say there is only one place where zone labels populates:

kubernetes/pkg/kubelet/kubelet_node_status.go

Line 406 in bad4faf

zones, ok := kl.cloud.Zones()

In other words, yes, in case of not enough permissions this what would happen exactly.

If we don't automatically add the role binding and role before node registration, the code here will try send 3 requests to APIserver and get error of not enough permission, failing to add the labels and won't ever add it again.
Unless customer add those rolebinding and role, then recreate nodes again.

lobziik · 2022-01-13T13:33:15Z

I attempted to fix the case with not sufficient permissions in 3dfc011
Doubting about this solution though - this handler might be a quite hot piece of code (calls on every node update), but didn't figure out anything better yet...
@lubronzhan could you please take a look?

lubronzhan · 2022-01-22T00:21:15Z

Hi @lobziik Right now the node label will be set every time when node is updated.
Does that mean after the customer add the role and rolebinding he needs to somehow trigger the node updates?

lobziik · 2022-01-25T09:57:06Z

Hi @lobziik Right now the node label will be set every time when node is updated.
Does that mean after the customer add the role and rolebinding he needs to somehow trigger the node updates?

Hi! For some reason i thought that this handler would be triggered time to time, during status update/heartbeat, but seems not. So, yes, in current variant node update need to be to be triggered somehow for reconcile zone labels.

p.s. made this handler run every 5 minutes in cd1d530 for avoid necessity to trigger node update.
Additionaly, this function might be used for reconcile changes in vSphere tags, since topology labels based on that

cc WDYT @lubronzhan ?

In the case of `vsphere-legacy-cloud-provider` client has insufficient permissions to update Node after its addition, this handler will attempt to populate Nodes topology labels later.

lobziik · 2022-02-07T11:17:33Z

Gentle ping @lubronzhan, @andrewsykim :)

lubronzhan · 2022-02-07T22:12:41Z

Sorry I missed the email notification.
Yeah now it makes more sense!
/approve
/lgtm

k8s-ci-robot · 2022-02-07T22:13:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cheftako, lobziik, lubronzhan

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:

~~staging/src/k8s.io/legacy-cloud-providers/OWNERS~~ [cheftako]

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

Starting kube 1.24, legacy vsphere cloud-provider requires permission to update nodes in order to set topology related labels. For more info see kubernetes/kubernetes#101028

k8s-ci-robot added the area/cloudprovider label Apr 12, 2021

k8s-ci-robot requested review from BaluDontu and dougm April 12, 2021 12:39

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 12, 2021

yangjunmyfm192085 reviewed Apr 12, 2021

View reviewed changes

pacoxu reviewed Apr 14, 2021

View reviewed changes

staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere_test.go Show resolved Hide resolved

pacoxu reviewed Apr 14, 2021

View reviewed changes

staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere_test.go Show resolved Hide resolved

Danil-Grigorev reviewed Apr 15, 2021

View reviewed changes

staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere.go Show resolved Hide resolved

staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere.go Show resolved Hide resolved

cheftako reviewed May 21, 2021

View reviewed changes

k8s-ci-robot assigned gnufied Oct 21, 2021

lobziik added 2 commits January 5, 2022 15:54

reordered imports

273099b

add description for elaborate changes in isZonesEnabled method

bfdbfa2

lobziik force-pushed the vsphere-zones-with-secret-creds branch from 2030d91 to bfdbfa2 Compare January 5, 2022 14:58

lobziik requested a review from lubronzhan January 5, 2022 15:17

Add zone/region labels sync function

cd1d530

In the case of `vsphere-legacy-cloud-provider` client has insufficient permissions to update Node after its addition, this handler will attempt to populate Nodes topology labels later.

lobziik force-pushed the vsphere-zones-with-secret-creds branch from 3dfc011 to cd1d530 Compare January 25, 2022 13:38

k8s-ci-robot assigned lubronzhan Feb 7, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2022

k8s-ci-robot merged commit c2ba0a4 into kubernetes:master Feb 7, 2022

k8s-ci-robot added this to the v1.24 milestone Feb 7, 2022

lobziik mentioned this pull request Jun 1, 2022

Bug 1902307: Let vsphere-legacy-cloud-provider update node objects openshift/cluster-kube-controller-manager-operator#631

Merged

lobziik mentioned this pull request Oct 17, 2022

REQUEST: New membership for @lobziik kubernetes/org#3773

Closed

9 tasks

Fixes zone/region labels setup and kubelet stucking on startup if credentials stored in secret for legacy vSphere cloudprovider. #101028

Fixes zone/region labels setup and kubelet stucking on startup if credentials stored in secret for legacy vSphere cloudprovider. #101028

lobziik commented Apr 12, 2021 •

edited

k8s-ci-robot commented Apr 12, 2021

Danil-Grigorev commented Apr 12, 2021

yangjunmyfm192085 Apr 12, 2021

yangjunmyfm192085 commented Apr 12, 2021

lobziik commented Apr 13, 2021

lobziik commented Apr 13, 2021

lobziik commented May 20, 2021 •

edited

cheftako May 21, 2021 •

edited

lobziik May 25, 2021

cheftako commented May 25, 2021

lobziik commented Jun 22, 2021

SandeepPissay commented Jun 22, 2021

jcpowermac commented Aug 19, 2021

gnufied commented Oct 21, 2021

lobziik commented Jan 5, 2022

lobziik commented Jan 5, 2022

andrewsykim commented Jan 5, 2022

lubronzhan commented Jan 5, 2022 •

edited

lubronzhan commented Jan 5, 2022

andrewsykim commented Jan 6, 2022

andrewsykim commented Jan 6, 2022

lubronzhan commented Jan 10, 2022

lobziik commented Jan 11, 2022

lobziik commented Jan 12, 2022

lobziik commented Jan 13, 2022 •

edited

lubronzhan commented Jan 22, 2022

lobziik commented Jan 25, 2022 •

edited

lobziik commented Feb 7, 2022

lubronzhan commented Feb 7, 2022

k8s-ci-robot commented Feb 7, 2022

	klog.Errorf("Cannot get node name.")
	klog.ErrorS(nil, "Cannot get node name.")

Fixes zone/region labels setup and kubelet stucking on startup if credentials stored in secret for legacy vSphere cloudprovider. #101028

Fixes zone/region labels setup and kubelet stucking on startup if credentials stored in secret for legacy vSphere cloudprovider. #101028

Conversation

lobziik commented Apr 12, 2021 • edited

What this PR does / why we need it:

Which issue(s) this PR fixes:

Notes:

What type of PR is this?

Does this PR introduce a user-facing change?

k8s-ci-robot commented Apr 12, 2021

Danil-Grigorev commented Apr 12, 2021

yangjunmyfm192085 Apr 12, 2021

Choose a reason for hiding this comment

yangjunmyfm192085 commented Apr 12, 2021

lobziik commented Apr 13, 2021

lobziik commented Apr 13, 2021

lobziik commented May 20, 2021 • edited

cheftako May 21, 2021 • edited

Choose a reason for hiding this comment

lobziik May 25, 2021

Choose a reason for hiding this comment

cheftako commented May 25, 2021

lobziik commented Jun 22, 2021

SandeepPissay commented Jun 22, 2021

jcpowermac commented Aug 19, 2021

gnufied commented Oct 21, 2021

lobziik commented Jan 5, 2022

lobziik commented Jan 5, 2022

andrewsykim commented Jan 5, 2022

lubronzhan commented Jan 5, 2022 • edited

lubronzhan commented Jan 5, 2022

andrewsykim commented Jan 6, 2022

andrewsykim commented Jan 6, 2022

lubronzhan commented Jan 10, 2022

lobziik commented Jan 11, 2022

lobziik commented Jan 12, 2022

lobziik commented Jan 13, 2022 • edited

lubronzhan commented Jan 22, 2022

lobziik commented Jan 25, 2022 • edited

lobziik commented Feb 7, 2022

lubronzhan commented Feb 7, 2022

k8s-ci-robot commented Feb 7, 2022

lobziik commented Apr 12, 2021 •

edited

lobziik commented May 20, 2021 •

edited

cheftako May 21, 2021 •

edited

lubronzhan commented Jan 5, 2022 •

edited

lobziik commented Jan 13, 2022 •

edited

lobziik commented Jan 25, 2022 •

edited