Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a Kubernetes RBAC extractor and integrates it into the Kubernetes scraper; includes fixtures, unit and end-to-end tests, and Makefile updates for envtest setup. Changes
Sequence Diagram(s)sequenceDiagram
participant Scraper as Kubernetes Scraper
participant RBAC as RBAC Extractor
participant RoleIdx as Role Processor
participant BindingCol as Binding Collector
participant Results as Results Aggregator
Scraper->>RBAC: Initialize extractor (cluster, scraperID)
Scraper->>RBAC: Index all objects
RBAC->>RoleIdx: Process Role / ClusterRole objects
RoleIdx-->>RBAC: Store ExternalRole entries
Scraper->>BindingCol: Collect RoleBinding / ClusterRoleBinding (deferred)
BindingCol->>RBAC: Process bindings -> resolve subjects, create ExternalUser/ExternalGroup, create ConfigAccess
RBAC->>Results: Aggregate ExternalRoles, ExternalUsers, ExternalGroups, ConfigAccess
Results-->>Scraper: Append RBAC results to scrape output
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
| } | ||
|
|
||
| // builtinResourceKinds maps plural resource names to their Kind for core Kubernetes resources. | ||
| var builtinResourceKinds = map[string]string{ |
There was a problem hiding this comment.
Shouldn't this be more dynamic ?
There was a problem hiding this comment.
these are the builtin ones, we scrape crds for getting other resources
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@scrapers/kubernetes/rbac_test.go`:
- Around line 96-103: The test for ConfigAccess (using extractor.getAccess() in
rbac_test.go) is missing assertions for ExternalRoleAliases; update the unit
test to assert that each returned ConfigAccess has ExternalRoleAliases set to
the expected slice (matching the fixture k8s_rbac_access_test.json), and if
failures occur, fix the extractor code in rbac.go (the logic that builds
ConfigAccess entries) so it copies the external_role_aliases field into the
ConfigAccess.ExternalRoleAliases before returning; locate code that constructs
ConfigAccess objects and ensure ExternalRoleAliases is populated from the parsed
fixture data, then add assertions in the loop in rbac_test.go to validate the
expected role alias values.
In `@scrapers/kubernetes/rbac.go`:
- Around line 386-407: The ExternalConfigAccess entries created in the loop are
missing ExternalRoleAliases and the embedded ConfigAccess.ID, which breaks
dedup/upsert; compute the role alias once for the binding (the same value used
elsewhere for roles, e.g., the pattern produced for Kubernetes/<cluster>/Role...
or using the same helper used for role aliases), call generateRBACID(subject,
target, roleAlias) to produce a stable ID, and set access.ID to that generated
ID and access.ExternalRoleAliases = []string{roleAlias} when building the
v1.ExternalConfigAccess; locate this change around the block that constructs
v1.ExternalConfigAccess (use symbols KubernetesAlias, GetConfigTypeForKind,
generateRBACID, ExternalConfigAccess) and ensure the roleAlias is computed
before iterating subjects so every access entry gets the same role alias and
deterministic ID.
🧹 Nitpick comments (5)
scrapers/kubernetes/rbac_test.go (1)
362-398:makeRoleBindingandmakeClusterRoleBindingshare duplicated subject-building logic.The subject map construction (lines 363–378 and 401–416) is identical. Consider extracting a shared helper.
scrapers/kubernetes/kubernetes.go (1)
568-572: An emptyScrapeResultis always appended when RBAC produces no entities.When
rbacis nil,results()returnsv1.ScrapeResult{}. Whenrbacis non-nil but no RBAC objects exist, it returns aScrapeResultwith empty slices and zero-value fields (no Name, Type, etc.). Either way, an effectively empty result is appended. Consider guarding against this.Proposed fix
// Process role bindings after all roles have been processed for _, binding := range roleBindings { rbac.processRoleBinding(binding) } - results = append(results, rbac.results(ctx.config.BaseScraper)) + if rbacResult := rbac.results(ctx.config.BaseScraper); rbacResult.HasRBACData() { + results = append(results, rbacResult) + }Alternatively, perform a simpler check:
- results = append(results, rbac.results(ctx.config.BaseScraper)) + rbacResult := rbac.results(ctx.config.BaseScraper) + if len(rbacResult.ExternalRoles) > 0 || len(rbacResult.ExternalUsers) > 0 || len(rbacResult.ExternalGroups) > 0 || len(rbacResult.ConfigAccess) > 0 { + results = append(results, rbacResult) + }scrapers/kubernetes/rbac.go (3)
414-436: Wildcard resource"*"is silently skipped — consider documenting this trade-off.When an RBAC rule grants access to
resources: ["*"],findTargetResourcesskips it entirely (line 416-418). This is a reasonable choice to avoid an explosion of access entries for overly permissive roles likecluster-admin, but the behavior should be documented so future maintainers understand the intentional gap.Add a comment explaining the rationale
for _, resourceType := range rule.Resources { if resourceType == "*" { + // Wildcard resource access is intentionally skipped to avoid + // generating an access entry for every object in the cluster. + // Roles like cluster-admin would otherwise produce O(N) entries. continue }
80-92: Global mutable cache — consider documenting thread-safety guarantees.
crdResourceKindCacheis a package-level mutable with a mutex. The current usage infetchCRDResourceKindsis correct (lock/unlock protects reads and writes), and callers only read the returned map, so there's no data race. However, the returnedmap[string]stringis a reference to the cached data — if any future caller were to mutate it, the cache would be corrupted.newRBACExtractorcorrectly copies entries into a new map, but this invariant is fragile.
96-148:fetchCRDResourceKindsholds the mutex during the entire CRD API call.The lock is acquired at line 97 and held through the network call to list CRDs (line 128) and cache update (line 142). If the API call is slow or times out, all other goroutines calling this function for any cluster will block. Consider a lock-per-cluster approach or releasing the lock before the API call and re-acquiring it for the cache update (double-check pattern).
| access := extractor.getAccess() | ||
| require.Len(t, access, 2, "expected 2 config access entries (one per pod in namespace)") | ||
|
|
||
| // Check that access entries point to pods, not the role | ||
| for _, a := range access { | ||
| assert.Equal(t, ConfigTypePrefix+"Pod", a.ConfigExternalID.ConfigType) | ||
| assert.Equal(t, []string{expectedUserAlias}, a.ExternalUserAliases) | ||
| } |
There was a problem hiding this comment.
Tests don't verify ExternalRoleAliases on ConfigAccess entries.
None of the unit tests assert that ExternalRoleAliases is set on the generated ConfigAccess entries. Given that the test fixture (k8s_rbac_access_test.json) includes external_role_aliases on every config_access entry, this seems like intended behavior that isn't being tested or produced by the extractor. See the related comment on rbac.go for the root cause.
🤖 Prompt for AI Agents
In `@scrapers/kubernetes/rbac_test.go` around lines 96 - 103, The test for
ConfigAccess (using extractor.getAccess() in rbac_test.go) is missing assertions
for ExternalRoleAliases; update the unit test to assert that each returned
ConfigAccess has ExternalRoleAliases set to the expected slice (matching the
fixture k8s_rbac_access_test.json), and if failures occur, fix the extractor
code in rbac.go (the logic that builds ConfigAccess entries) so it copies the
external_role_aliases field into the ConfigAccess.ExternalRoleAliases before
returning; locate code that constructs ConfigAccess objects and ensure
ExternalRoleAliases is populated from the parsed fixture data, then add
assertions in the loop in rbac_test.go to validate the expected role alias
values.
yashmehrotra
force-pushed
the
k8s-config-access
branch
from
4a624e5 to
5fab1a9
Compare
2 participants
Fixes: #1844
Summary by CodeRabbit
New Features
Tests
Chores