fix(clusterroles): don't ParseBool aggregation label values#609
Conversation
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.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #609 +/- ##
==========================================
+ Coverage 46.57% 46.59% +0.02%
==========================================
Files 69 69
Lines 3957 3955 -2
==========================================
Hits 1843 1843
+ Misses 1806 1805 -1
+ Partials 308 307 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@RaphaelKorPro Can you fix this in |
| if err != nil { | ||
| return nil, fmt.Errorf("couldn't convert string to bool %v", err) | ||
| } | ||
| // A ClusterRole whose label matches an aggregation selector is |
|
Done — trimmed the comment to a single line. On ClusterRoleBinding/RoleBinding: I don't think there's an equivalent change to make here. This bug was specific to the aggregation path — Happy to add something if you had a specific case in mind — let me know. |
Problem
retrieveUsedClusterRolesaborts the entire ClusterRole scan on any cluster running a controller that ships aggregated ClusterRoles with non-boolean aggregation label values — notably Emissary / Ambassador:In
pkg/kor/clusterroles.go, when a ClusterRole's label matches an aggregation selector, the code did:But
valueis the aggregation label value — an arbitrary string (e.g.emissary-emissary-ingress-agent), not a boolean.ParseBoolerrors and the whole non-namespaced ClusterRole scan bails out. The existing test only happened to use"true"as the value, so it never caught this.Fix
A ClusterRole whose label matches an aggregation selector is pulled into the aggregated (used) ClusterRole, so it is itself used — set it to
truerather than parsing the value.strconvis no longer needed.Test
Adds
TestRetrieveUsedClusterRoles_NonBooleanAggregationLabel, which reproduces the Emissary case (a non-boolean aggregation label value). It fails before the fix with the exact error above and passes after; existing clusterroles tests are unchanged.