Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jun 17, 2022
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 6 additions & 15 deletions test/infrastructure/docker/internal/docker/machine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,9 +388,6 @@ func (m *Machine) SetNodeProviderID(ctx context.Context) error {
if err != nil {
return errors.Wrapf(err, "unable to set NodeProviderID. error getting a kubectl node")
}
if kubectlNode == nil {
return errors.New("unable to set NodeProviderID. there are no kubectl node available")
}
if !kubectlNode.IsRunning() {
return errors.Wrapf(ContainerNotRunningError{Name: kubectlNode.Name}, "unable to set NodeProviderID")
}
Expand All @@ -399,7 +396,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",
Copy link
Member

@sbueringer sbueringer Jun 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup. That's what I expected

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"patch",
"node", m.ContainerName(),
"--patch", patch,
Expand All @@ -416,28 +413,22 @@ func (m *Machine) SetNodeProviderID(ctx context.Context) error {
}

func (m *Machine) getKubectlNode(ctx context.Context) (*types.Node, error) {
// collect info about the existing controlplane nodes
// collect info about the existing nodes
filters := container.FilterBuilder{}
filters.AddKeyNameValue(filterLabel, clusterLabelKey, m.cluster)
filters.AddKeyNameValue(filterLabel, nodeRoleLabelKey, constants.ControlPlaneNodeRoleValue)

kubectlNodes, err := listContainers(ctx, filters)
if err != nil {
return nil, errors.WithStack(err)
}
if len(kubectlNodes) == 0 {
return nil, nil
}
// Return the first node that is not the current machine.
// The assumption being made is that the existing control planes will already be ready.
// This is true when we are using kubeadm control plane.
// Return the node matching the current machine, required to patch itself using its kubelet config
for _, node := range kubectlNodes {
if node.Name != m.container.Name {
if node.Name == m.container.Name {
return node, nil
}
}
// This will happen when the current machine is the only machine.
return kubectlNodes[0], nil

return nil, fmt.Errorf("there are no Kubernetes nodes matching the container name")
}

// Delete deletes a docker container hosting a Kubernetes node.
Expand Down