Node Drain behaving unexpectedly

iamkirkbater commented 3 months ago

We have a requirement where we want to allow certain users the ability to cordon and drain certain nodes. We do not want these users to be able to cordon and/or drain control plane nodes, or certain other nodes defined by node role labels. We have set up a webhook that denies these users patch operations on nodes with these labels.

If a user with the correct permissions attempts to drain a node with a label they are not supposed to be able to drain, an error is spit out by CLI, and then the drain continues anyway.

What happened:

It seems that the cordon operation is the only patch operation to the API. Even when the cordon fails, the drain continues if the user has permission to evict the pods in the namespaces running on those nodes.

What you expected to happen:

I want to acknowledge that I understand that the user could run these same operations manually without any issue. I would expect, however, if the drain command is supposed to cordon the node first and it cannot cordon the node due to permissions issues on the cordon operation that the subsequent drain for that node would also error out.

How to reproduce it (as minimally and precisely as possible):

Create a user group that has admin permissions
Create an admission webhook that disallows this user group from editing nodes (similar to https://github.com/openshift/managed-cluster-validating-webhooks/blob/426092ea6601ee92219248513dbf032cca6bcc7e/pkg/webhooks/node/node.go#L175-L181)
create a user in this user group, and log in as this user
run kubectl drain [some node with control plane label]
See the following output:

error: unable to cordon node "ip-10-0-2-209.ec2.internal": admission webhook "node-validation-osd.managed.openshift.io" denied the request: Prevented from modifying Red Hat managed control plane nodes. .... rest of webhook error message
evicting pod namespace1/pod-aaabbbcc-1
evicting pod namespace2/another-pod-5v5q4
...
pod/pod-aaabbbcc-1 evicted
pod/another-pod-5v5q4 evicted

Anything else we need to know?: I'm not certain, but I wonder if this line without the continue has anything to do with it, or if there just needs to be some error handling in general added here: https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/drain/drain.go#L434. Though, I do wonder if the lack of any error handling here is how the CLI is handling a scenario where the node is already cordoned, ie:

$ kubectl drain ip-10-0-2-209.ec2.internal
node/ip-10-0-2-209.ec2.internal already cordoned

Environment:

Kubernetes client and server versions (use kubectl version):
```
Client Version: v1.27.4
Kustomize Version: v5.0.1
```
Cloud provider or hardware configuration: OpenShift on AWS
OS (e.g: cat /etc/os-release): RHEL 9.3 (in a container) and MacOS on ARM64

This bug/functionality was identified by @aliceh and then we tracked down the exact issue together.

k8s-ci-robot commented 3 months ago

This issue is currently awaiting triage.

SIG CLI takes a lead on issue triage for this repo, but any Kubernetes member can accept issues by applying the triage/accepted label.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

ardaguclu commented 3 months ago

@iamkirkbater if PATCH gets an error, it tries to perform UPDATE operation (https://github.com/kubernetes/kubernetes/blob/017eaa519de5926fd75a9ddc61bedf2398b69653/staging/src/k8s.io/kubectl/pkg/drain/cordon.go#L96-L109) and is it possible that your user has UPDATE permissions?.

Additionally, did you check that evicting pod namespace1/pod-aaabbbcc-1 is indeed evicted or not (just a printer error that is caused by lack of continue as you pointed out https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/drain/drain.go#L434)

iamkirkbater commented 3 months ago

So in our case the user would have both PATCH and UPDATE permissions as they're administrators - however they're blocked by a webhook which I think would be catching both the PATCH and UPDATE API calls and rejecting at that level. I'll admit my deep understanding of how the model of RBAC patch vs Update operations on webhooks works is limited - but after the command is run the node is NOT cordoned so I think the webhook would be blocking either call.

I'll have to circle back around to double check if the pods are actually being evicted or not. I'm hoping to have some time to look into this by the end of this week.

iamkirkbater commented 3 months ago

Circling back to this - I can confirm that the pod evictions are actually happening after the cordon action is blocked by the webhook.

To test I opened two terminal windows to my cluster. In the first I ran:

watch -n 1 kubectl get pods -A --field-selector spec.nodeName=ip-10-0-34-127.ec2.internal

In the second I ran kubectl drain ip-10-0-34-127.ec2.internal --ignore-daemonsets --delete-emptydir-data --force and watched the pods all terminate.

It's worth noting that they were immediately recreated because of the lack of a cordon on the node itself, so the impact here isn't massive because we also run our control planes with a minimum of three instances - I just wonder if this is expected behavior.

ardaguclu commented 1 month ago

This PR https://github.com/kubernetes/kubernetes/pull/52917 brings with some logic that may explain the expected behavior with regards to historical context.

If at least one node fails to be cordoned/uncordoned, the command will
continue to operate on any remaining nodes, uninterrupted, reporting
any errors it encounters along the way.

If at least one node fails to be drained, the command will halt, printing
the immediate error encountered, and a list of nodes yet to be drained
(including the node that just failed to be drained).

To sum up, cordon failures don't block the process to continue to other nodes but any failure in draining short cuts the process and errors out.

I think we can close this issue as not a bug.

ardaguclu commented 1 month ago

/close

k8s-ci-robot commented 1 month ago

@ardaguclu: Closing this issue.

In response to [this](https://github.com/kubernetes/kubectl/issues/1568#issuecomment-2144980982): >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.

kubernetes / kubectl

Node Drain behaving unexpectedly #1568