feat: auto tolerate daemonsets with MAP#117
feat: auto tolerate daemonsets with MAP#117pehlicd wants to merge 3 commits intokubernetes-sigs:mainfrom
Conversation
k8s-ci-robot
added
the
kind/feature
✅ Deploy Preview for node-readiness-controller canceled.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pehlicd The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
Signed-off-by: pehlicd <furkanpehlivan34@gmail.com>
pehlicd
force-pushed
the
feat-auto-tolerate-daemonSets-with-map
branch
from
9676992 to
24fde7b
Compare
k8s-ci-robot
removed
the
needs-rebase
|
Thank you so much for the PR @pehlicd. I have left a few in-line comments. In addition to those, how do you suggest handling RBAC for the controller to be able to update For my local testing, I manually created the required RBAC. (updated later) Adding logs from my local testing, if you need to reproduce: |
|
Also adding, I was able to test all the scenarios listed here: (with the changes I suggested in the comments above) In addition, I also tested deleting a node-readiness-rule -> which removes the corresponding taint from the which I believe is how it should be (we should not be removing any existing tolerations, only inject new ones - the MAP rule suggested in the PR achievss that) |
|
/retest |
Signed-off-by: pehlicd <furkanpehlivan34@gmail.com>
| // +kubebuilder:rbac:groups=readiness.node.x-k8s.io,resources=nodereadinessrules/status,verbs=get;update;patch | ||
| // +kubebuilder:rbac:groups=readiness.node.x-k8s.io,resources=nodereadinessrules/finalizers,verbs=update | ||
| // +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch;create;update;patch;delete | ||
| // +kubebuilder:rbac:groups="",resources=nodes,verbs=get;list;watch;create;update;patch;delete |
There was a problem hiding this comment.
Although we had nodes rbac roles, it was missing in the inline comments so I added them.
There was a problem hiding this comment.
@pehlicd, I looked into it further.
Firstly, the node resource RBAC added above - I don't think we need them at all for this PR's purpose. (correction - they're already added in the node controller here -
node-readiness-controller/internal/controller/node_controller.go
Lines 84 to 86 in 354b151
So, Let's remove the redundant markers from here.
What I realised is we don't need the above cluster-scoped configmap RBAC as well -
because, we already have a namespace-scoped RBAC role for configmaps (leader-election-role) here which is better IMO in terms of surface area of permissions and serves our purpose -
node-readiness-controller/config/rbac/leader_election_role.yaml
Lines 9 to 21 in 354b151
The only problem is that currently the (controller-runtime's informer) cache is trying to watch configmaps at the cluster scope (see the error below again) but our namespaced role only provide permissions for nrr-system namespace, so I suggest we configure the cache to only watch configmaps in our required namespace.
❯ kubectl logs -n nrr-system
deployment/nrr-controller-manager --tail=30 | grep -i configmap
2026-02-09T09:46:35Z ERROR controller-runtime.cache.UnhandledError Failed
to watch{"reflector":
"pkg/mod/k8s.io/client-go@v0.34.0/tools/cache/reflector.go:290", "type":
"*v1.ConfigMap", "error": "failed to list *v1.ConfigMap: configmaps is forbidden:
User \"system:serviceaccount:nrr-system:nrr-controller-manager\" cannot list
resource \"configmaps\" in API group \"\" at the cluster scope"}
I tried like following, it worked -
diff --git a/cmd/main.go b/cmd/main.go
index 34652e9..1527845 100644
--- a/cmd/main.go
+++ b/cmd/main.go
@@ -28,11 +28,14 @@ import (
_ "k8s.io/client-go/plugin/pkg/client/auth"
"k8s.io/client-go/rest"
+ corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/runtime"
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/client-go/kubernetes"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
ctrl "sigs.k8s.io/controller-runtime"
+ "sigs.k8s.io/controller-runtime/pkg/cache"
+ "sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/healthz"
"sigs.k8s.io/controller-runtime/pkg/log/zap"
"sigs.k8s.io/controller-runtime/pkg/metrics/filters"
@@ -107,6 +110,15 @@ func main() {
HealthProbeBindAddress: probeAddr,
LeaderElection: enableLeaderElection,
LeaderElectionID: "ba65f13e.readiness.node.x-k8s.io",
+ Cache: cache.Options{
+ ByObject: map[client.Object]cache.ByObject{
+ &corev1.ConfigMap{}: {
+ Namespaces: map[string]cache.Config{
+ "nrr-system": {},
+ },
+ },
+ },
+ },
})
if err != nil {
setupLog.Error(err, "unable to start manager")| // +kubebuilder:rbac:groups=readiness.node.x-k8s.io,resources=nodereadinessrules,verbs=get;list;watch;create;update;patch;delete | ||
| // +kubebuilder:rbac:groups=readiness.node.x-k8s.io,resources=nodereadinessrules/status,verbs=get;update;patch | ||
| // +kubebuilder:rbac:groups=readiness.node.x-k8s.io,resources=nodereadinessrules/finalizers,verbs=update | ||
| // +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch;create;update;patch;delete |
There was a problem hiding this comment.
In addition to those, how do you suggest handling RBAC for the controller to be able to update taints-key data in the readiness-taints configmap?
The PR currently miss that piece.
@Priyankasaggu11929 regarding to your comment above I added this inline kubebuilder comments and regenerated manifests.
|
Thanks for the detailed review, @Priyankasaggu11929. I believe I have addressed all the feedback. Please let me know if anything else is needed. |
|
/retest |
|
@pehlicd: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
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-sigs/prow repository. |
|
/retest |
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
Signed-off-by: pehlicd <furkanpehlivan34@gmail.com>
|
/retest |
| - name: taintKeys | ||
| expression: | | ||
| ("taint-keys" in params.data) && params.data["taint-keys"] != "" ? | ||
| params.data["taint-keys"].split(",") : [] |
There was a problem hiding this comment.
| params.data["taint-keys"].split(",") : [] | |
| params.data["taint-keys"].split(",").filter(key, key != "") : [] |
currently, if someone intentionally add a empty taint key to the configmap
our MAP policy will create a matching empty-key wildcard toleration
For example
❯ kubectl patch configmap readiness-taints -n nrr-system --type=merge -p '{"data":{"taint-keys":"readiness.k8s.io/NetworkReady,,readiness.k8s.io/StorageReady"}}'
configmap/readiness-taints patched
// notice the two commas which basically mean an empty taint key
❯ kubectl get configmap -n nrr-system readiness-taints -o jsonpath='{.data.taint-keys}'
readiness.k8s.io/NetworkReady,,readiness.k8s.io/StorageReady
// the above empty taint, injected an empty-key wildcard toleration on the ds
❯ kubectl get ds test-ds -o jsonpath='{.spec.template.spec.tolerations[*]}' | jq
{
"effect": "NoSchedule",
"key": "readiness.k8s.io/NetworkReady",
"operator": "Exists"
}
{
"effect": "NoSchedule",
"operator": "Exists"
}
{
"effect": "NoSchedule",
"key": "readiness.k8s.io/StorageReady",
"operator": "Exists"
}
| paramRef: | ||
| name: readiness-taints | ||
| namespace: nrr-system | ||
| parameterNotFoundAction: Deny |
There was a problem hiding this comment.
Does parameterNotFoundAction:Deny blocks daemonset creations in the absence of this config-map param?
There was a problem hiding this comment.
yes, in my local testing with the PR changes, it is blocking creation of new daemonsets.
It works if a user creates node-readiness-rules first and then daemonsets, but the other way around blocks the creation of new daemonsets.
I tested the behaviour by disabling the empty configmap file in the kustomization.yaml, because I was thinking why do we need to have an empty configmap file, if the configmap creation is taken care of by the controller itself - https://github.com/kubernetes-sigs/node-readiness-controller/pull/117/changes#diff-b857080def3b2fecd3967ca35b1bdc29564dda0b064fa902fa24efcd55471899R721-R727
But the thing is - the controller logic only kicks in when a user create a new node-readiness-rule.
So if a user created a daemonset first before creating the node-readiness-rules, that daemonset object creation is blocked.
The presence of the empty configmap file at the same time of creating MAP Policy/Policy-Binding fixes it.
❯ kubectl get node nrr-test-worker -o jsonpath={.spec.taints} | jq .
[
{
"effect": "NoSchedule",
"key": "readiness.k8s.io/NetworkReady",
"value": "pending"
}
]
// applying MAP policy/policy-binding without empty configmap
❯ kubectl apply -k config/admission-policy
mutatingadmissionpolicy.admissionregistration.k8s.io/inject-daemonset-readiness-tolerations created
mutatingadmissionpolicybinding.admissionregistration.k8s.io/inject-daemonset-readiness-tolerations-binding created
❯ kubectl get cm -n nrr-system
NAME DATA AGE
kube-root-ca.crt 1 41s
// daemonset creation is blocked
❯ kubectl apply -f test-daemonset.yaml
Error from server (Forbidden): error when creating "test-daemonset.yaml": policy 'inject-daemonset-readiness-tolerations' with binding 'inject-daemonset-readiness-tolerations-binding' denied request: failed to configure binding: no params found for policy binding with `Deny` parameterNotFoundAction
// creating a new nrr rule, which will trigger in turn configmap creation/patch per PR changes
❯ kubectl apply -f examples/cni-readiness/network-readiness-rule.yaml
nodereadinessrule.readiness.node.x-k8s.io/network-readiness-rule created
❯ kubectl get cm readiness-taints -n nrr-system -o jsonpath={.data}
{"taint-keys":"readiness.k8s.io/NetworkReady"}
// now daemonset creation works
❯ kubectl apply -f test-daemonset.yaml
daemonset.apps/test-ds created
| - create | ||
| - delete |
There was a problem hiding this comment.
not from your changes, but dont think manager needs create or delete for any objects in core.
|
|
||
| ### Option 2: Direct kubectl apply | ||
|
|
||
| ```bash | ||
| # Install CRDs first | ||
| make install | ||
|
|
||
| # Deploy policy and binding | ||
| kubectl apply -f config/admission-policy/policy.yaml | ||
| kubectl apply -f config/admission-policy/binding.yaml | ||
| ``` |
There was a problem hiding this comment.
hmm. this looks redundant. also, dont we need to show updating the config-map and how we maintain it with readiness-taints introduced later?
| ↓ | ||
| Fetches Tolerations ConfigMap which contains the tolerations to be injected | ||
| ↓ | ||
| Injects tolerations (if applicable) |
There was a problem hiding this comment.
This doesnt tell what is applicable. could we also add details on the annotation requirement?
4 participants
Description
This PR implements automatic DaemonSet toleration injection for
readiness.k8s.io/*taints using MutatingAdmissionPolicy with ConfigMap parameter resource and automated sync via the existing controller.Key features:
Related Issue
Fixes #7
Type of Change
/kind feature
Testing
Tested in local kind cluster.
Checklist
make testpassesmake lintpassesDoes this PR introduce a user-facing change?
Doc #7