fix(rulebinding): match namespaces by exact name, not String() substring - #402
Conversation
Signed-off-by: manu <kiratcodes99@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe rule-binding cache now logs selector parsing errors and skips invalid bindings. Namespace matching checks exact namespace names instead of substring matches. Tests cover exact, missing, substring, nil, and empty namespace lists. ChangesRule-binding selector fixes
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Reviewed this against main in a local checkout of the branch: go build ./... and go vet ./admission/... are clean, and go test ./... is green (including admission/rulebinding/cache), so the unchecked "New and existing unit tests pass locally" box can be ticked.
The fix is correct. I reproduced #401 through ListRulesForObject (not just the helper) with a fake clientset: on main a pod in dev matches a binding whose namespaceSelector selects only devel; on this branch it does not. namespaceListHasName itself is right, including the nil and empty-list guards.
I found no merge-blocking defect in the change itself. Two things I'd still want before merge, and two follow-ups:
Must-fix
- Add a regression test at the
ListRulesForObjectlevel.TestNamespaceListHasNameonly tests the new helper in isolation — it would still pass if the call site oncache.go:122were reverted tostrings.Contains. The bug reported in #401 lives in the wiring, and the existingk8sinterface.NewKubernetesApiMock()already gives you a fake clientset whoseNamespaces().Listhonours the label selector, so the test is cheap. Code inline below (verified: fails onmain, passes here). - The description undersells the error handling. Those two
_→errchanges fix an unrecovered nil-pointer panic that crashes the operator, not just a missing log line. Details inline oncache.go:100. Please call it out in the PR body / release notes, and cover it with a test — it is the higher-severity half of this PR.
Follow-ups (not blocking)
- The identical bug is still live in
node-agent—pkg/rulebindingmanager/cache/cache.gohas bothstrings.Contains(namespaces.String(), pod.GetNamespace())and the same two discardedLabelSelectorAsSelectorerrors (checkedv0.3.38, the version pinned in this repo'sgo.mod, and it is still there on newer tags). The operator only imports node-agent's types and interfaces, so this PR is complete for this repo — but a user hitting #401 through the runtime (non-admission) path is not fixed until node-agent is. Worth an issue there. - The namespace check LISTs every matching namespace from the API server, once per rule binding, on every admission request — in the webhook latency path. A single
Namespaces().Get(object.GetNamespace())(ideally from an informer cache) plusnsSelector.Matches(labels.Set(ns.Labels))is one cheap call, removes the fan-out, and removes the need fornamespaceListHasNamealtogether. Detail inline onhelpers.go.
Nits inline. Nice, tight fix — thanks for the clear comment explaining why String() must not be used.
| podSelector, _ := metav1.LabelSelectorAsSelector(&rb.Spec.PodSelector) | ||
| podSelector, err := metav1.LabelSelectorAsSelector(&rb.Spec.PodSelector) | ||
| if err != nil { | ||
| logger.L().Error("failed to parse pod selector", helpers.String("ruleBiding", uniqueName(&rb)), helpers.Error(err)) |
There was a problem hiding this comment.
This is the most valuable change in the PR and the description doesn't mention it: on main these discarded errors are a crash vector, not a silent mismatch.
metav1.LabelSelectorAsSelector returns (nil, err) on a bad selector — it does not fall back to labels.Nothing(). So podSelector.Matches(...) on the old line dereferences a nil interface, and so does nsSelector.String() below. Verified on main with a RuntimeAlertRuleBinding whose podSelector uses an unknown operator:
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation]
github.com/kubescape/operator/admission/rulebinding/cache.(*RBCache).ListRulesForObject(...)
admission/rulebinding/cache/cache.go:100
There is no recover() anywhere under admission/ or in main.go (only mainhandler and restapihandler have one), and ListRulesForObject is called from the admission worker in admission/webhook/validator.go:282 — so one malformed CRD, which the API server will happily accept since the CRD schema doesn't validate operator values, takes down the operator process. With this branch the same input logs failed to parse pod selector ... "Bogus" is not a valid label selector operator and skips the binding (fail-closed, which is the right call for an admission matcher).
Two asks:
- Mention this in the PR body / release notes — "fixes a panic that crashes the operator on an invalid rule binding selector" is a much bigger headline than "reports conversion errors".
- Add a test for it. It's ~20 lines and needs no fixtures:
func TestListRulesForObjectInvalidSelectorDoesNotPanic(t *testing.T) {
c := NewCacheMock()
c.addRuleBinding(&typesv1.RuntimeAlertRuleBinding{
ObjectMeta: metav1.ObjectMeta{Name: "badRB", Namespace: "kubescape"},
Spec: typesv1.RuntimeAlertRuleBindingSpec{
PodSelector: metav1.LabelSelector{
MatchExpressions: []metav1.LabelSelectorRequirement{
{Key: "app", Operator: "Bogus", Values: []string{"x"}},
},
},
Rules: []typesv1.RuntimeAlertRuleBindingRule{{RuleID: "R2000"}},
},
})
obj := &unstructured.Unstructured{Object: map[string]interface{}{
"metadata": map[string]interface{}{
"name": "p", "namespace": "ns",
"labels": map[string]interface{}{"app": "x"},
},
}}
assert.NotPanics(t, func() {
assert.Empty(t, c.ListRulesForObject(context.Background(), obj))
})
}(Panics on main, passes on this branch.)
| nsSelector, _ := metav1.LabelSelectorAsSelector(&rb.Spec.NamespaceSelector) | ||
| nsSelector, err := metav1.LabelSelectorAsSelector(&rb.Spec.NamespaceSelector) | ||
| if err != nil { | ||
| logger.L().Error("failed to parse namespace selector", helpers.String("ruleBiding", uniqueName(&rb)), helpers.Error(err)) |
There was a problem hiding this comment.
Two nits on both new log lines (this one and the pod-selector one above):
rbNameis already computed fromuniqueName(&rb)at the top of the loop (cache.go:81) — reuse it instead of recomputing."ruleBiding"is a typo. It's pre-existing on thefailed to list namespacesline below, so it's consistent — but if it's being copied into two new lines, worth fixing all three to"ruleBinding"in the same commit. Anyone grepping logs or building a dashboard on this field will thank you.
logger.L().Error("failed to parse namespace selector", helpers.String("ruleBinding", rbName), helpers.Error(err))| continue | ||
| } | ||
| if !strings.Contains(namespaces.String(), object.GetNamespace()) { | ||
| if !namespaceListHasName(namespaces, object.GetNamespace()) { |
There was a problem hiding this comment.
Must-fix: this call site — the one #401 is actually about — has no test.
TestNamespaceListHasName tests the helper in isolation, so it would stay green if this line were reverted to strings.Contains(namespaces.String(), object.GetNamespace()). Nothing in the suite exercises ListRulesForObject with a non-empty namespaceSelector at all, which is exactly why the bug survived this long.
The existing k8sinterface.NewKubernetesApiMock() backs KubernetesClient with a real kubernetesfake.NewSimpleClientset(), and the generated fake's Namespaces().List does apply the label selector — so this reproduces the reported scenario end to end with no new test infrastructure:
func TestListRulesForObjectNamespaceSelectorExactMatch(t *testing.T) {
k8sAPI := k8sinterface.NewKubernetesApiMock()
for _, ns := range []*corev1.Namespace{
{ObjectMeta: metav1.ObjectMeta{Name: "devel", Labels: map[string]string{"repro": "selected"}}},
{ObjectMeta: metav1.ObjectMeta{Name: "dev"}},
} {
_, err := k8sAPI.KubernetesClient.CoreV1().Namespaces().Create(context.Background(), ns, metav1.CreateOptions{})
assert.NoError(t, err)
}
c := NewCacheMock()
c.k8sClient = k8sAPI
c.addRuleBinding(&typesv1.RuntimeAlertRuleBinding{
ObjectMeta: metav1.ObjectMeta{Name: "testRB", Namespace: "kubescape"},
Spec: typesv1.RuntimeAlertRuleBindingSpec{
PodSelector: metav1.LabelSelector{MatchLabels: map[string]string{"app": "testPod"}},
NamespaceSelector: metav1.LabelSelector{MatchLabels: map[string]string{"repro": "selected"}},
Rules: []typesv1.RuntimeAlertRuleBindingRule{{RuleID: "R2000"}},
},
})
pod := func(ns string) *unstructured.Unstructured {
return &unstructured.Unstructured{Object: map[string]interface{}{
"metadata": map[string]interface{}{
"name": "testPod", "namespace": ns,
"labels": map[string]interface{}{"app": "testPod"},
},
}}
}
assert.Len(t, c.ListRulesForObject(context.Background(), pod("devel")), 1, "selected namespace must match")
assert.Len(t, c.ListRulesForObject(context.Background(), pod("dev")), 0, "substring namespace must NOT match")
}I ran this both ways:
- on
main:Error: "[0xc000664810]" should have 0 item(s), but has 1 — substring namespace must NOT match - on this branch:
PASS
Keep TestNamespaceListHasName too — it's good — but this is the one that pins the regression.
| // namespaceListHasName reports whether name is an exact member of list.Items. | ||
| // Do not use strings.Contains(list.String(), name): NamespaceList.String() is a | ||
| // debug dump, so substrings (e.g. "dev" in "devel") falsely match. | ||
| func namespaceListHasName(list *corev1.NamespaceList, name string) bool { |
There was a problem hiding this comment.
Follow-up, not blocking this PR — the helper is the right fix for the bug as written.
But it's worth noting why this helper has to exist: the caller LISTs every namespace in the cluster matching the selector, then linearly scans the result for one name. That LIST is issued once per rule binding, on every admission request, in the webhook's latency path. With N bindings carrying a namespaceSelector, a single pod create costs N cluster-wide namespace LISTs against the API server.
The question being asked is "does this one namespace match the selector", so it can be answered without the fan-out:
ns, err := c.k8sClient.GetKubernetesClient().CoreV1().Namespaces().Get(ctx, object.GetNamespace(), metav1.GetOptions{})
if err != nil {
logger.L().Error("failed to get namespace", helpers.String("ruleBinding", rbName), helpers.Error(err))
continue
}
if !nsSelector.Matches(labels.Set(ns.GetLabels())) {
continue
}One GET instead of a LIST (and ideally served from an informer/lister cache rather than the API server at all), no nsSelector.String() round-trip through ListOptions, and namespaceListHasName disappears — along with the whole class of bug it's guarding against. Same treatment would apply to node-agent's copy of this loop.
2 participants
Overview
fixes #401
This PR fixes false-positive rulebinding matches when a namespace name is a substring of another (e.g.
devmatchingdevel).Namespace membership now checks exact names instead of
strings.Contains(namespaces.String(), …). Invalid label selectors are skipped.Signed Commits
How to Test
develwithrepro=selected; leavedevunlabeled.R2002with that namespace selector.develshould match.Checklist before requesting a review
Summary by CodeRabbit