From 9ab575cf59c660f2e5ecb1a3d99f839ac19528c2 Mon Sep 17 00:00:00 2001 From: raphaelbenhamo-commits Date: Thu, 25 Jun 2026 11:25:28 +0300 Subject: [PATCH 1/2] fix(clusterroles): don't ParseBool aggregation label values MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit retrieveUsedClusterRoles marked an aggregated ClusterRole as used by parsing the aggregation label *value* as a boolean. Aggregation label values are arbitrary strings (e.g. Emissary/Ambassador ship ClusterRoles labelled `rbac.authorization.k8s.io/aggregate-to-...: emissary-emissary-ingress-agent`), so strconv.ParseBool returned an error and aborted the entire ClusterRole scan with "couldn't convert string to bool" on any cluster running such controllers. A ClusterRole whose label matches an aggregation selector is pulled into the aggregated (used) role, so it is simply used — set it to true instead of parsing the value. Adds a regression test with a non-boolean aggregation label. --- pkg/kor/clusterroles.go | 12 ++++++---- pkg/kor/clusterroles_test.go | 45 ++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 5 deletions(-) diff --git a/pkg/kor/clusterroles.go b/pkg/kor/clusterroles.go index 1e6a928f..d8fc8cc8 100644 --- a/pkg/kor/clusterroles.go +++ b/pkg/kor/clusterroles.go @@ -8,7 +8,6 @@ import ( "fmt" "os" "slices" - "strconv" v1 "k8s.io/api/rbac/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -89,10 +88,13 @@ func retrieveUsedClusterRoles(clientset kubernetes.Interface, filterOpts *filter for _, clusterRole := range clusterRoles.Items { for label, value := range clusterRole.Labels { if slices.Contains(aggregatedLabels, label+": "+value) { - usedClusterRoles[clusterRole.Name], err = strconv.ParseBool(value) - if err != nil { - return nil, fmt.Errorf("couldn't convert string to bool %v", err) - } + // A ClusterRole whose label matches an aggregation selector is + // pulled into the aggregated (used) ClusterRole, so it is itself + // used. Previously this parsed the label *value* as a bool — but + // aggregation label values are arbitrary strings (e.g. + // "emissary-emissary-ingress-agent"), so ParseBool errored and + // aborted the entire ClusterRole scan. + usedClusterRoles[clusterRole.Name] = true if clusterRole.AggregationRule == nil { continue } diff --git a/pkg/kor/clusterroles_test.go b/pkg/kor/clusterroles_test.go index cdf36c0e..fd1421b0 100644 --- a/pkg/kor/clusterroles_test.go +++ b/pkg/kor/clusterroles_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "reflect" + "slices" "sort" "testing" @@ -279,3 +280,47 @@ func init() { scheme.Scheme = runtime.NewScheme() _ = appsv1.AddToScheme(scheme.Scheme) } + +// TestRetrieveUsedClusterRoles_NonBooleanAggregationLabel is a regression test +// for aggregated ClusterRoles whose aggregation label value is an arbitrary +// string (e.g. Emissary's "emissary-emissary-ingress-agent") rather than a +// boolean. Previously the scan parsed the label value as a bool and errored, +// aborting ClusterRole detection entirely on such clusters. +func TestRetrieveUsedClusterRoles_NonBooleanAggregationLabel(t *testing.T) { + clientset := fake.NewClientset() + + if _, err := clientset.CoreV1().Namespaces().Create(context.TODO(), &corev1.Namespace{ + ObjectMeta: v1.ObjectMeta{Name: testNamespace}, + }, v1.CreateOptions{}); err != nil { + t.Fatalf("Error creating namespace %s: %v", testNamespace, err) + } + + // Aggregation label with a non-boolean value, as shipped by Emissary/Ambassador. + nonBoolLabel := map[string]string{"rbac.authorization.k8s.io/aggregate-to-emissary": "emissary-emissary-ingress-agent"} + selector := v1.LabelSelector{MatchLabels: nonBoolLabel} + + // Aggregator ClusterRole selects the non-boolean label; it is "used" via a binding. + aggregator := CreateTestClusterRole("emissary-aggregator", AppLabels, selector) + if _, err := clientset.RbacV1().ClusterRoles().Create(context.TODO(), aggregator, v1.CreateOptions{}); err != nil { + t.Fatalf("Error creating aggregator clusterRole: %v", err) + } + // Member ClusterRole carries the matching non-boolean label. + member := CreateTestClusterRole("emissary-member", nonBoolLabel) + if _, err := clientset.RbacV1().ClusterRoles().Create(context.TODO(), member, v1.CreateOptions{}); err != nil { + t.Fatalf("Error creating member clusterRole: %v", err) + } + // Bind the aggregator so it lands in usedClusterRoles and the aggregation loop runs. + ref := CreateTestRoleRefForClusterRole("emissary-aggregator") + crb := CreateTestClusterRoleBindingRoleRef(testNamespace, "emissary-rb", "test-sa", ref) + if _, err := clientset.RbacV1().ClusterRoleBindings().Create(context.TODO(), crb, v1.CreateOptions{}); err != nil { + t.Fatalf("Error creating clusterRoleBinding: %v", err) + } + + used, err := retrieveUsedClusterRoles(clientset, &filters.Options{}) + if err != nil { + t.Fatalf("non-boolean aggregation label must not error: %v", err) + } + if !slices.Contains(used, "emissary-member") { + t.Errorf("expected aggregated 'emissary-member' to be marked used, got %v", used) + } +} From 85530a55ff164a1c6bad6b65a96392495629b2ea Mon Sep 17 00:00:00 2001 From: raphaelbenhamo-commits Date: Sun, 28 Jun 2026 13:02:28 +0300 Subject: [PATCH 2/2] chore(clusterroles): trim aggregation-fix comment per review --- pkg/kor/clusterroles.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/pkg/kor/clusterroles.go b/pkg/kor/clusterroles.go index d8fc8cc8..4c1070b7 100644 --- a/pkg/kor/clusterroles.go +++ b/pkg/kor/clusterroles.go @@ -88,12 +88,7 @@ func retrieveUsedClusterRoles(clientset kubernetes.Interface, filterOpts *filter for _, clusterRole := range clusterRoles.Items { for label, value := range clusterRole.Labels { if slices.Contains(aggregatedLabels, label+": "+value) { - // A ClusterRole whose label matches an aggregation selector is - // pulled into the aggregated (used) ClusterRole, so it is itself - // used. Previously this parsed the label *value* as a bool — but - // aggregation label values are arbitrary strings (e.g. - // "emissary-emissary-ingress-agent"), so ParseBool errored and - // aborted the entire ClusterRole scan. + // Aggregated into a used ClusterRole, so it is itself used. usedClusterRoles[clusterRole.Name] = true if clusterRole.AggregationRule == nil { continue