Add an option to prevent caching of resources in other namespaces#6732
Conversation
|
(Closed and re-opened to trigger CI run.) |
729327a to
04d6a49
Compare
📝 WalkthroughWalkthroughAdded a public Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller as Crossplane Controller (startCommand.Run)
participant Manager as controller-runtime Manager
participant Cache as controller-runtime Cache
User->>Controller: start (flag: --watch-cache-namespaced true|false)
activate Controller
Controller->>Controller: build cacheOptions (SyncPeriod)
alt WatchCacheNamespaced == true
Controller->>Controller: set cacheOptions.DefaultNamespaces = [controllerNamespace]
Controller->>Controller: set cacheOptionsAPIExt.DefaultNamespaces = [controllerNamespace]
end
Controller->>Manager: New(manager.Options{Cache: cacheOptions})
Manager->>Cache: initialize with cacheOptions / cacheOptionsAPIExt
Cache-->>Manager: cache started (namespaced or cluster-wide)
Manager-->>Controller: manager ready
deactivate Controller
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Would you like me to also suggest the CLI flag name and a Helm values change to expose this option, or keep that in a follow-up PR? Pre-merge checks✅ Passed checks (6 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3){apis,cmd}/**📄 CodeRabbit inference engine (Custom checks)
Files:
**/*.go⚙️ CodeRabbit configuration file
Files:
**/cmd/**⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-11-20T10:28:53.064ZApplied to files:
📚 Learning: 2025-11-25T21:57:11.518ZApplied to files:
🔍 Remote MCPLet me search the web for relevant Crossplane documentation and context about the caching mechanism and RBAC:Let me search for more specific information about Kubernetes controller manager caching and the implementation patterns this PR follows: Summary of Additional Context for PR #6732 ReviewBased on the search results, here is relevant context for reviewing this PR: Problem ContextThe RBAC manager runs as a distinct pod responsible for creating and binding RBAC roles, allowing Crossplane and its providers to run as service accounts limited to a specific set of resources and verbs (e.g., create, watch, etc.) However, the Crossplane Helm Chart deployments have excessive RBAC permissions beyond what they need. Related Ecosystem TrendsCrossplane v2 makes all managed resources (MRs) namespaced, with maintainers actively working to update managed resources for other systems to support namespaced MRs. This PR aligns with the broader v2 direction toward namespace-scoped resources. Technical Implementation PatternThe DefaultNamespaces cache option creates a multiNamespaceCache, which internally maintains a map of namespace name to Cache (informerCache), allowing specification of Informer behavior for particular namespaces. The Manager can restrict the namespace that all controllers will watch for resources by configuring the Cache Options DefaultNamespaces. RBAC Permissions ImpactControllers need appropriate RBAC permissions for object kinds they retrieve via cached clients (i.e., list and watch), and by default, watches started by a controller-runtime cache are cluster-scoped, meaning it watches and caches objects across all namespaces. This PR addresses the permission issue by restricting watches to the controller's own namespace when the flag is enabled. Cache Configuration ImplicationsSyncPeriod determines the minimum frequency at which watched resources are reconciled—a lower period will correct entropy more quickly, but reduce responsiveness to change if there are many watched resources. The PR's approach of configuring separate cache options with SyncPeriod allows fine-grained control over both the core controller and API extension caches. Backward CompatibilityAs noted in the PR description, the change is inert unless the new flag is explicitly specified, maintaining default behavior for existing deployments. Sources used: Crossplane documentation and design files, Kubernetes controller-runtime documentation, and related GitHub issues and discussions discussing RBAC permissions, caching strategies, and namespace scoping patterns. 🔇 Additional comments (3)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
04d6a49 to
2a7a999
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/crossplane/core/core.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Enforce Crossplane-specific patterns: Use crossplane-runtime/pkg/errors
for wrapping. Check variable naming (short for local scope, descriptive
for wider scope). Ensure 'return early' pattern. Verify error scoping
(declare in conditionals when possible). For nolint directives, require
specific linter names and explanations. CRITICAL: Ensure all error
messages are meaningful to end users, not just developers - avoid
technical jargon, include context about what the user was trying to do,
and suggest next steps when possible.
Files:
cmd/crossplane/core/core.go
**/cmd/**
⚙️ CodeRabbit configuration file
**/cmd/**: Review CLI commands for proper flag handling, help text, and error
messages. Ensure commands follow Crossplane CLI conventions. Ask about
backward compatibility and user experience. CLI error messages must be
especially user-friendly - avoid internal error details, provide
actionable guidance.
Files:
cmd/crossplane/core/core.go
🔍 Remote MCP
I'll search for additional context about Crossplane's caching architecture and the namespaced cache feature to help with your PR review.Based on my research, I've gathered comprehensive context about Crossplane's architecture, caching mechanisms, and RBAC model. Here's the relevant context for reviewing PR #6732:
Context for PR #6732 Review
Problem Context: Cache and Permissions Issues
The Crossplane controller faces caching challenges where resource leaks occur when the local cache is not updated quickly enough after large numbers of resources are created, causing subsequent reconciliations to not find created resources in the cache and create new resources instead. This PR addresses a more specific issue: disabling caching of Kubernetes-native resources when the controller lacks cluster-wide permissions.
Architectural Context: RBAC and Cluster-Scoped Resources
The base Crossplane installation consists of two pods—the crossplane pod and the crossplane-rbac-manager pod—both installed in the crossplane-system namespace by default. The Crossplane RBAC manager pod automates required Kubernetes RBAC permissions for Crossplane and Providers; Crossplane installs and enables the RBAC manager by default, and disabling it requires manual Kubernetes permissions definitions for proper Crossplane operations.
Design Direction: Future Namespacing
The PR's approach aligns with Crossplane's roadmap. In Crossplane v2, all Managed Resources are proposed to become namespaced; someday all MRs are expected to be namespaced, with cluster-scoped MRs becoming legacy for v1 backward compatibility, though cluster-scoped resource support is needed for a long migration period.
Related Caching Precedents
A related PR disabled caching of Unstructured resources for claims and XRs to address similar resource leaks, suggesting precedent for selective caching configuration in Crossplane.
Test Coverage Note
The PR checklist indicates no unit or e2e tests were added, which is notable given the medium complexity of the change and the critical nature of caching behavior to controller stability.
[::web_search::]
🔇 Additional comments (2)
cmd/crossplane/core/core.go (2)
179-193: Nice implementation of namespace-scoped caching!The logic correctly restricts cache watches to Crossplane's namespace when the flag is enabled, which should prevent the permission-related crashes described in issue #6348. The backward-compatible default ensures existing deployments aren't affected.
344-362: No action required – the separate cache design is correct.The separate cache created for the ControllerEngine (lines 344-362) doesn't need the
WatchCacheNamespacedrestriction. Here's why:
Different caches, different concerns: The main manager cache (lines 182-189) watches native Kubernetes resources like ServiceAccounts, Services, and Secrets—which is where permission errors occur. The separate
cacache watches only Crossplane-specific CRDs: Claims, CompositeResources, ComposedResources, and CompositionRevisions.Intentional cluster-wide scope: The ControllerEngine cache needs exclusive ownership and cluster-wide visibility to dynamically track which controllers are using which informers. XRs are cluster-scoped resources, and the cache must discover all composed resources across all namespaces to manage them correctly.
Correct configuration: The cache explicitly caches unstructured resources (XRs and MRs) and excludes Secrets—exactly what it should do for API extension management.
The PR correctly applies the namespace restriction to the main manager cache (which watches native K8s resources) while leaving the ControllerEngine cache unrestricted (which watches Crossplane CRDs). This design is sound.
|
Ah, looks like merging the other PR caused a conflict for this one. |
Signed-off-by: François Rigaut <francois.rigaut@orange.com>
2a7a999 to
564046c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/crossplane/core/core.go (2)
142-142: Clear and helpful flag addition!The field declaration and help text look great. The help text is user-friendly and clearly explains when to use this option, which aligns well with the coding guidelines for CLI commands.
One optional suggestion: Consider mentioning in the help text what functionality might be limited when this flag is enabled (e.g., "Resources in other namespaces will not be watched or cached"). This would help users make informed decisions about enabling it.
Optional improvement to help text
- WatchCacheNamespaced bool `default:"false" help:"Restrict resource caching to Crossplane's namespace only. Use this when Crossplane lacks cluster-wide permissions."` + WatchCacheNamespaced bool `default:"false" help:"Restrict resource caching to Crossplane's namespace only; resources in other namespaces will not be watched or cached. Use this when Crossplane lacks cluster-wide permissions."`
186-196: Consider adding tests for this crash-prevention featureSince this feature addresses a crash scenario when Crossplane lacks cluster-wide permissions (as mentioned in issue #6348), it would be valuable to add tests verifying that:
- The cache correctly restricts to the specified namespace when the flag is enabled
- The controller doesn't crash when permissions are limited
I understand tests weren't included in this PR, but they would increase confidence that this feature works as intended and prevent regressions.
Would you like to add tests in this PR, or would you prefer to track them separately?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/crossplane/core/core.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Enforce Crossplane-specific patterns: Use crossplane-runtime/pkg/errors
for wrapping. Check variable naming (short for local scope, descriptive
for wider scope). Ensure 'return early' pattern. Verify error scoping
(declare in conditionals when possible). For nolint directives, require
specific linter names and explanations. CRITICAL: Ensure all error
messages are meaningful to end users, not just developers - avoid
technical jargon, include context about what the user was trying to do,
and suggest next steps when possible.
Files:
cmd/crossplane/core/core.go
**/cmd/**
⚙️ CodeRabbit configuration file
**/cmd/**: Review CLI commands for proper flag handling, help text, and error
messages. Ensure commands follow Crossplane CLI conventions. Ask about
backward compatibility and user experience. CLI error messages must be
especially user-friendly - avoid internal error details, provide
actionable guidance.
Files:
cmd/crossplane/core/core.go
🔇 Additional comments (1)
cmd/crossplane/core/core.go (1)
186-196: Manager cache configuration looks good!The cache options configuration is well-structured and the conditional logic correctly restricts the cache to Crossplane's namespace when the flag is enabled. The comment clearly explains the reasoning.
|
@negz I rebased and fixed conflicts. |
|
There is a failure in e2e tests, however I cannot replicate it locally. Since my code supposedly does not change Crossplane's behavior unless specified with the added argument, I'd say this is a timeout shenanigan in Github actions, but feel free to correct me if I'm wrong. |
Signed-off-by: François Rigaut <francois.rigaut@orange.com>
|
@negz Up - could you please tell me if this PR is ok ? |
|
Hi, thanks for the great work, this feature is almost exactly what we're looking for as well. I've got just one additional request: Would it be possible to also configure which namespaces are being watched? I had something like this in mind: with WatchCacheNamespaces a list of namespaces, instead of / alternative to your I am not experienced with the implications this has, thus any feedback / input to this suggestion is welcome! |
|
Hello, I'm all for it, we also suggested something very similar in external-secrets (external-secrets/external-secrets#4721) Here the goal is to restrict Crossplane's cache in a namespace-restricted way according to the philosophy of this design doc (which is a little outdaded regarding the name of the options, etc.). Also I would have to test if it keeps reconciling resources correctly on all namespaces. And before doing that, having the insight of a maintainer may be useful |
jbw976
left a comment
There was a problem hiding this comment.
Thank you @frigaut-orange for continuing to drive these namespace restriction changes. This change looks focused and clean to do what it advertises ✅
I haven't been keeping up with the broader effort, but I know you would like to get this PR merged, so I'm catching up a bit now and trying to help with that.
It sounds like from #6425 (comment) that this is one step towards the broader picture outlined in the namespace restricted mode design doc.
Merging this doesn't invalidate #6425, it's more of a step along the way and would need to be absorbed into that effort if I understand correctly.
I'm hesitant to update this specific PR at this point to start handling multiple namespaces of the users choosing (instead of just the core crossplane namespace), because I think that has knock on effects in other places and should be thought of more completely in the context of the general namespace restriction design.
Let's get this merged and revisit the effort in its entirety to figure out clear next steps 🙇♂️
Description of your changes
Add a
--restrict-cache-optionsconfig flag in core cmd to avoid caching resources from other namespaces (and prevent crashes when crossplane does not have permissions on other namespaces)Fixes #6348
I have:
earthly +reviewableto ensure this PR is ready for review.[ ] Added or updated unit tests.[ ] Added or updated e2e tests.[ ] Addedbackport release-x.ylabels to auto-backport this PR.[ ] Followed the API promotion workflow if this PR introduces, removes, or promotes an API.Need help with this checklist? See the cheat sheet.