✨ making SetNodeProviderID no more blocking although Control Plane is externally managed #6640

prometherion · 2022-06-13T16:40:24Z

What this PR does / why we need it:

The current code-base gave for granted that in a Docker-based CAPI installation, the Control Plane is managed in the same infrastructure. However, this assumption couldn't be entirely correct, and due to this, the reconciliation is blocking the operations required to assign the Provider IDs to the machines.

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

k8s-ci-robot · 2022-06-13T16:40:31Z

Welcome @prometherion!

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 · 2022-06-13T16:40:32Z

Hi @prometherion. 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.

killianmuldoon

/ok-to-test

sbueringer · 2022-06-14T08:13:37Z

test/infrastructure/docker/internal/docker/machine.go

@@ -399,7 +399,7 @@ func (m *Machine) SetNodeProviderID(ctx context.Context) error {
 	patch := fmt.Sprintf(`{"spec": {"providerID": %q}}`, m.ProviderID())
 	cmd := kubectlNode.Commander.Command(
 		"kubectl",
-		"--kubeconfig", "/etc/kubernetes/admin.conf",
+		"--kubeconfig", "/etc/kubernetes/kubelet.conf",


Does the kubelet.conf on any machine have the rights to patch any other node?

I wonder why we are doing this via kubectl. Is there something which prevents us from simply using a Client configured against the workload cluster API server in the controller?
(maybe it doesn't work for the first control plane node and we get stuck?)

(cc @fabriziopandini)

Any kubelet.conf seems not work, I think the kubelet.conf of a node is only able to patch himself:

Trying to patch itself: allowed (still throws an error due to invalid patch, could be ignored in this case)

root@capi-quickstart-md-0-bj6ch-79956b7999-dvpx9:/# kubectl --kubeconfig /etc/kubernetes/kubelet.conf patch no capi-quickstart-md-0-bj6ch-79956b7999-dvpx9 -p '{"spec": {"providerID": "docker:////capi-quickstart-md-0-bj6ch-79956b7999-dvpx9-foo"}}' The Node "capi-quickstart-md-0-bj6ch-79956b7999-dvpx9" is invalid: spec.providerID: Forbidden: node updates may not change providerID except from "" to valid

Trying to patch other node: fails because forbidden

root@capi-quickstart-md-0-bj6ch-79956b7999-dvpx9:/# kubectl --kubeconfig /etc/kubernetes/kubelet.conf patch no capi-quickstart-md-0-bj6ch-79956b7999-7hpkk -p '{"spec": {"providerID": "docker:////capi-quickstart-md-0-bj6ch-79956b7999-7hpkk-foo"}}' Error from server (Forbidden): nodes "capi-quickstart-md-0-bj6ch-79956b7999-7hpkk" is forbidden: node "capi-quickstart-md-0-bj6ch-79956b7999-dvpx9" is not allowed to modify node "capi-quickstart-md-0-bj6ch-79956b7999-7hpkk"

This is due to the Node authorization mode in kube-apiserver: --authorization-mode=Node,RBAC

Yup. That's what I expected

I changed the code to launch the patch command from the node itself. CI is green now.

However, I don't see a real benefit here on launching the command from the node, besides reducing the number of lines of code. If we all agree on using the controller-runtime client, I can rework the PR.

As I'm not really sure about why we use kubectl I would say let's just keep it that way (at least for this PR) as the current PR will definitely already fix your issue and is an improvement

sbueringer · 2022-06-14T08:16:18Z

/retest

sbueringer · 2022-06-14T08:16:24Z

/test pull-cluster-api-e2e-full-main

sbueringer · 2022-06-15T05:14:06Z

/test pull-cluster-api-e2e-full-main

sbueringer · 2022-06-15T05:16:11Z

/lgtm
(assuming e2e-full-main will succeed)

/assign @fabriziopandini

chrischdi · 2022-06-15T07:19:10Z

/lgtm

vincepri · 2022-06-17T14:52:34Z

test/infrastructure/docker/internal/docker/machine.go

-	// This will happen when the current machine is the only machine.
-	return kubectlNodes[0], nil
+
+	return nil, nil


We should avoid returning nil, nil, shouldn't we have an error if we haven't found the node?

Good point. We also did this before, but let's fix / improve it

Address with d4b2a03, let me know if it should be squashed in a single commit.

Looks good. Yup, please squash

Thanks, squashed.

…nfiguration

sbueringer · 2022-06-17T18:01:22Z

/lgtm

vincepri

/approve

k8s-ci-robot · 2022-06-17T18:08:09Z

[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 Jun 13, 2022

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 13, 2022

k8s-ci-robot requested review from sbueringer and stmcginnis June 13, 2022 16:40

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 13, 2022

killianmuldoon reviewed Jun 13, 2022

View reviewed changes

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 Jun 13, 2022

sbueringer reviewed Jun 14, 2022

View reviewed changes

prometherion force-pushed the issues/6639 branch from 2eddc5e to a8b832f Compare June 14, 2022 18:20

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 14, 2022

prometherion requested review from killianmuldoon and sbueringer June 14, 2022 18:42

k8s-ci-robot assigned fabriziopandini and sbueringer Jun 15, 2022

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

k8s-ci-robot assigned chrischdi Jun 15, 2022

vincepri reviewed Jun 17, 2022

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 17, 2022

feat: retrieving first available docker-based node for provider id co…

3ce889f

…nfiguration

prometherion force-pushed the issues/6639 branch from d4b2a03 to 3ce889f Compare June 17, 2022 15:26

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

vincepri approved these changes Jun 17, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 17, 2022

k8s-ci-robot merged commit 41664d9 into kubernetes-sigs:main Jun 17, 2022

k8s-ci-robot added this to the v1.2 milestone Jun 17, 2022

prometherion deleted the issues/6639 branch June 17, 2022 18:24

prometherion mentioned this pull request Jun 20, 2022

🌱 patching Docker-based nodes provider ID using client-runtime #6684

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ making SetNodeProviderID no more blocking although Control Plane is externally managed #6640

✨ making SetNodeProviderID no more blocking although Control Plane is externally managed #6640

prometherion commented Jun 13, 2022

k8s-ci-robot commented Jun 13, 2022

k8s-ci-robot commented Jun 13, 2022

killianmuldoon left a comment

sbueringer Jun 14, 2022 •

edited

Loading

sbueringer Jun 14, 2022

chrischdi Jun 14, 2022

sbueringer Jun 14, 2022

prometherion Jun 14, 2022

sbueringer Jun 15, 2022

sbueringer commented Jun 14, 2022

sbueringer commented Jun 14, 2022

sbueringer commented Jun 15, 2022

sbueringer commented Jun 15, 2022

chrischdi commented Jun 15, 2022

vincepri Jun 17, 2022

sbueringer Jun 17, 2022

prometherion Jun 17, 2022

sbueringer Jun 17, 2022

prometherion Jun 17, 2022

sbueringer commented Jun 17, 2022

vincepri left a comment

k8s-ci-robot commented Jun 17, 2022

✨ making SetNodeProviderID no more blocking although Control Plane is externally managed #6640

✨ making SetNodeProviderID no more blocking although Control Plane is externally managed #6640

Conversation

prometherion commented Jun 13, 2022

k8s-ci-robot commented Jun 13, 2022

k8s-ci-robot commented Jun 13, 2022

killianmuldoon left a comment

Choose a reason for hiding this comment

sbueringer Jun 14, 2022 • edited Loading

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

sbueringer commented Jun 14, 2022

sbueringer commented Jun 14, 2022

sbueringer commented Jun 15, 2022

sbueringer commented Jun 15, 2022

chrischdi commented Jun 15, 2022

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

sbueringer commented Jun 17, 2022

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 17, 2022

sbueringer Jun 14, 2022 •

edited

Loading