Add option to prevent events from being emitted in default namespace#6733
Conversation
|
Closed and reopened to unstuck CI. |
|
Looks like most of the CI checks are failing with errors similar to: Does this PR perhaps now need a rebase? Let us know if any help with that or debugging is needed @frigaut-orange 🙏 |
|
Hi @jbw976, the PR currently fails because crossplane/crossplane-runtime#868 is still open, and I did not update the dependency in crossplane's PR. Let me know if there is something I can do to temporarily point to my PR on crossplane-runtime (or do something less dirty 😬) Maybe I should add a little more context to this PR: I originally published crossplane/crossplane-runtime#829 which was merged (thanks again btw), adding the possibility to use a filter function while emitting events. However, I did not find a better way than what is suggested in this PR in order to pass this filter function as a parameter, so that controllers can use it (hence the addition of an attribute in the |
|
Just approved and merged the runtime PR./\ |
5332fd5 to
4ddaf8b
Compare
📝 WalkthroughWalkthroughAdds an optional CLI flag to restrict recorded events to namespaced (non-empty, non-"default") resources and threads variadic EventFilterFunctions through event.NewAPIRecorder invocations across controller setup code so recorders can apply those filters. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Flag Parser
participant Core as Core Init
participant Filter as Event Filter Builder
participant RecorderFactory as event.NewAPIRecorder
participant Reconciler as Controller Setup/Reconciler
CLI->>Core: parse RestrictNamespacedEvents flag
Core->>Filter: build EventFilterFunctions (if enabled)
alt enabled
Filter-->>Core: [namespace non-empty && namespace != "default"]
else disabled
Filter-->>Core: []
end
Core->>RecorderFactory: NewAPIRecorder(mgr.GetEventRecorderFor(name), EventFilterFunctions...)
RecorderFactory-->>Core: configured recorder
Core->>Reconciler: pass recorder via WithRecorder(...)
Reconciler->>RecorderFactory: RecordEvent(...)
RecorderFactory->>Filter: apply filters -> accept/reject
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Thank you — let me know if you want the predicate adjusted (e.g., configurable default namespaces) or a short test plan to validate behavior. Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
contributing/guide-adding-external-secret-stores.md (1)
116-117: Consider adding brief explanation of EventFilterFunctions.The guide now shows
o.EventFilterFunctions...being passed toNewAPIRecorder, but doesn't explain what event filter functions are or when developers might want to use them when adding secret store support.Would it be helpful to add a brief note explaining that EventFilterFunctions can be used to filter which events are recorded, and mentioning the
--restrict-namespaced-eventsflag as an example?cmd/crossplane/core/core.go (2)
137-137: Clarify the flag help text to match implementation behavior.The current help text says "Prevent events from being produced on resources that are not namespaced," which is a bit confusing. The implementation actually prevents events for:
- Cluster-scoped resources (which have empty namespace)
- Resources in the "default" namespace
Based on issue #6348 and the use case, would this be clearer?
-RestrictNamespacedEvents bool `default:"false" help:"Prevent events from being produced on resources that are not namespaced. Useful when crossplane does not have permissions in the default namespace."` +RestrictNamespacedEvents bool `default:"false" help:"Prevent events from being emitted to the default namespace. Filters out cluster-scoped resources and resources in the default namespace. Useful when Crossplane lacks permissions in the default namespace."`This makes it clearer that:
- Events that would go to the default namespace are filtered
- Both cluster-scoped resources (which emit to default) and explicit default namespace resources are affected
234-242: Consider adding explanatory comment for filter logic clarity.The filter logic is correct, but a brief comment would help future maintainers understand why both empty namespace and "default" namespace are filtered:
eventFilterFns := []event.FilterFn{} -// If the option to restrict event emission to namespaced resources if active, -// we create a filter function accordingly +// Filter events that would be emitted to the default namespace. +// Cluster-scoped resources (empty namespace) emit events to "default", +// so we filter both empty namespace and explicit "default" namespace. if c.RestrictNamespacedEvents { eventFilterFns = append(eventFilterFns, func(obj runtime.Object, e event.Event) bool { m, err := kmeta.Accessor(obj) return (err == nil && m.GetNamespace() != "" && m.GetNamespace() != "default") }) }This clarifies the "why" behind filtering both conditions, especially the non-obvious fact that cluster-scoped resources emit events to the default namespace.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
cmd/crossplane/core/core.go(4 hunks)contributing/guide-adding-external-secret-stores.md(1 hunks)internal/controller/apiextensions/activationpolicy/setup.go(1 hunks)internal/controller/apiextensions/composition/reconciler.go(1 hunks)internal/controller/apiextensions/definition/reconciler.go(1 hunks)internal/controller/apiextensions/managed/setup.go(1 hunks)internal/controller/apiextensions/offered/reconciler.go(1 hunks)internal/controller/apiextensions/revision/reconciler.go(1 hunks)internal/controller/ops/cronoperation/constructor.go(1 hunks)internal/controller/ops/operation/constructor.go(1 hunks)internal/controller/ops/watchoperation/constructor.go(1 hunks)internal/controller/pkg/manager/reconciler.go(3 hunks)internal/controller/pkg/revision/reconciler.go(3 hunks)internal/controller/pkg/runtime/reconciler.go(2 hunks)internal/controller/protection/usage/reconciler.go(3 hunks)internal/controller/rbac/definition/reconciler.go(1 hunks)internal/controller/rbac/provider/binding/reconciler.go(1 hunks)internal/controller/rbac/provider/roles/reconciler.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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:
internal/controller/apiextensions/activationpolicy/setup.gointernal/controller/protection/usage/reconciler.gointernal/controller/apiextensions/offered/reconciler.gointernal/controller/rbac/provider/binding/reconciler.gointernal/controller/ops/operation/constructor.gointernal/controller/ops/cronoperation/constructor.gointernal/controller/pkg/manager/reconciler.gointernal/controller/ops/watchoperation/constructor.gointernal/controller/pkg/runtime/reconciler.gointernal/controller/rbac/definition/reconciler.gointernal/controller/apiextensions/composition/reconciler.gointernal/controller/rbac/provider/roles/reconciler.gointernal/controller/pkg/revision/reconciler.gointernal/controller/apiextensions/definition/reconciler.gocmd/crossplane/core/core.gointernal/controller/apiextensions/managed/setup.gointernal/controller/apiextensions/revision/reconciler.go
**/internal/controller/**
⚙️ CodeRabbit configuration file
**/internal/controller/**: Review controller logic for proper reconciliation patterns, error
handling, and resource management. Pay special attention to conditions
and events: Conditions must be actionable for users (not developers),
stable/deterministic, with proper Type/Reason/Message format. Events
only when something actually happens, with specific details about what
changed. No transient errors in conditions/events. All error messages
must be meaningful to end users - include context about what
resource/operation failed and why.
Files:
internal/controller/apiextensions/activationpolicy/setup.gointernal/controller/protection/usage/reconciler.gointernal/controller/apiextensions/offered/reconciler.gointernal/controller/rbac/provider/binding/reconciler.gointernal/controller/ops/operation/constructor.gointernal/controller/ops/cronoperation/constructor.gointernal/controller/pkg/manager/reconciler.gointernal/controller/ops/watchoperation/constructor.gointernal/controller/pkg/runtime/reconciler.gointernal/controller/rbac/definition/reconciler.gointernal/controller/apiextensions/composition/reconciler.gointernal/controller/rbac/provider/roles/reconciler.gointernal/controller/pkg/revision/reconciler.gointernal/controller/apiextensions/definition/reconciler.gointernal/controller/apiextensions/managed/setup.gointernal/controller/apiextensions/revision/reconciler.go
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Ensure Markdown files are wrapped at 100 columns for consistency and
readability. Lines can be longer if it makes links more readable, but
otherwise should wrap at 100 characters. Check for proper heading
structure, clear language, and that documentation is helpful for users.
Files:
contributing/guide-adding-external-secret-stores.md
**/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
🧬 Code graph analysis (10)
internal/controller/apiextensions/offered/reconciler.go (1)
internal/controller/apiextensions/activationpolicy/setup.go (1)
WithRecorder(63-67)
internal/controller/ops/cronoperation/constructor.go (1)
internal/controller/ops/operation/constructor.go (1)
WithRecorder(67-71)
internal/controller/pkg/manager/reconciler.go (1)
internal/controller/apiextensions/activationpolicy/setup.go (1)
WithRecorder(63-67)
internal/controller/pkg/runtime/reconciler.go (1)
internal/controller/pkg/revision/reconciler.go (1)
WithRecorder(152-156)
internal/controller/rbac/definition/reconciler.go (1)
internal/controller/rbac/provider/binding/reconciler.go (1)
WithRecorder(94-98)
internal/controller/apiextensions/composition/reconciler.go (1)
internal/controller/apiextensions/composite/reconciler.go (1)
WithRecorder(287-291)
internal/controller/rbac/provider/roles/reconciler.go (1)
internal/controller/rbac/provider/binding/reconciler.go (1)
WithRecorder(94-98)
internal/controller/pkg/revision/reconciler.go (1)
internal/controller/pkg/manager/reconciler.go (1)
WithRecorder(146-150)
internal/controller/apiextensions/definition/reconciler.go (1)
internal/controller/apiextensions/activationpolicy/setup.go (1)
WithRecorder(63-67)
cmd/crossplane/core/core.go (4)
internal/controller/ops/controller/options.go (1)
Options(28-36)internal/controller/pkg/controller/options.go (1)
Options(27-49)internal/controller/rbac/controller/options.go (1)
Options(40-47)internal/controller/apiextensions/controller/options.go (1)
Options(28-36)
🔍 Remote MCP
Based on the search results, I can provide you with relevant context about the event filtering mechanism and the linked issue. Let me gather a bit more specific information about the runtime PR dependency.## Summary of Additional Context Found
Event Filtering Mechanism Foundation
The foundational event filtering capability was introduced by frigaut-orange in crossplane-runtime PR #829 titled "Add filter functions to event recorder". This PR established the infrastructure allowing event filters to be applied at the recorder level across the Crossplane control plane.
Core Problem Addressed (Issue #6348)
This PR attempts to solve a legitimate security and operational challenge. Crossplane emits events for cluster scoped resources to the 'default' namespace. The underlying issue #6348 identifies that Kubernetes namespaces events, while most Crossplane resources (XRs, etc) are cluster scoped, creating a permission and security concern. The original issue describes problems where the controller requests cluster-wide list/watch permissions and logs forbidden errors when these are denied, leading to potential CrashLoopBackOff scenarios.
Implementation Approach: Event-Level Filtering
The PR takes an event filtering approach rather than a full namespaced controller mode. The implementation:
- CLI Option: Adds
--restrict-namespaced-eventsflag to filter events at emission time - Filter Logic: Creates event filters that exclude objects with empty namespaces or those in the "default" namespace
- Systematic Application: Applies these filters across 15+ controller setup files by passing
o.EventFilterFunctions...toevent.NewAPIRecorder
Dependency Status - Critical Issue
The PR currently cannot build because it depends on crossplane-runtime PR #868, which has not yet been merged. CI failures indicate that o.EventFilterFunctions is undefined in the controller options, as the runtime changes introducing this field have not been released. The PR author acknowledged this dependency issue and the implementation is flagged as backward compatible.
Architectural Context for Assessment
The approach uses a variadic event filter function pattern similar to other controller-runtime patterns in Crossplane, suggesting a design that supports multiple composable filters. However, as noted in the PR comments, Crossplane emits events for cluster scoped resources to the 'default' namespace, meaning this solution specifically addresses event emission concerns rather than the broader namespace-scoped controller capability mentioned in issue #6348.
[::web_search::]
🔇 Additional comments (10)
internal/controller/apiextensions/offered/reconciler.go (1)
149-149: LGTM - consistent event filter wiring.The addition of
o.EventFilterFunctions...to the recorder setup follows the established pattern across the codebase.internal/controller/apiextensions/activationpolicy/setup.go (1)
40-40: LGTM - consistent event filter wiring.The addition of
o.EventFilterFunctions...follows the established pattern.internal/controller/protection/usage/reconciler.go (1)
108-108: LGTM - consistent event filter wiring across all Usage controllers.All three Setup functions (Usage, ClusterUsage, and LegacyUsage) correctly wire
o.EventFilterFunctions...to the API recorder.Also applies to: 123-123, 139-139
internal/controller/apiextensions/managed/setup.go (1)
43-43: LGTM - consistent event filter wiring.The addition of
o.EventFilterFunctions...follows the established pattern.internal/controller/apiextensions/definition/reconciler.go (1)
180-180: LGTM - consistent event filter wiring.The addition of
o.EventFilterFunctions...follows the established pattern.cmd/crossplane/core/core.go (1)
32-32: LGTM - clean integration of event filtering infrastructure.The imports and wiring of
EventFilterFunctionsinto the controller options is well-structured. The feature is properly gated behind the CLI flag with a safe default offalse.Also applies to: 48-48, 250-250
internal/controller/ops/operation/constructor.go (1)
46-46: LGTM - consistent event filter wiring.The addition of
o.EventFilterFunctions...follows the established pattern across all controller setups.internal/controller/rbac/definition/reconciler.go (1)
79-79: Event filtering for cluster-scoped resources requires verification after crossplane-runtime dependency is available.Thank you for raising this concern—your structural analysis is sound. I've confirmed that all the affected controllers reconcile cluster-scoped resources (CompositeResourceDefinition, Composition, CompositionRevision, ProviderRevision, Provider, Configuration, and Function), and as you noted, Kubernetes emits events for these to the
defaultnamespace by design.However, I cannot fully verify the filtering behavior here because
EventFilterFunctionsis not defined in this codebase—it comes from the crossplane-runtime dependency (PR #868, which you've already flagged as having a build issue). Without access to that implementation, I cannot confirm:
- Whether the filter distinguishes between cluster-scoped and truly namespaced resources
- How it treats events in the
defaultnamespace- Whether suppressing cluster-scoped resource events is the intended outcome
Recommendation: Once crossplane-runtime PR #868 is merged and integrated, verify that:
- The filter logic is namespace-aware and can distinguish cluster-scoped resources from namespaced ones
- Events for cluster-scoped resources are handled intentionally (either recorded or filtered as designed)
- The behavior aligns with user expectations for observability of cluster-scoped resources like XRDs and Providers
internal/controller/rbac/provider/roles/reconciler.go (1)
87-87: Clarify event filtering behavior for cluster-scoped resources.The addition of
o.EventFilterFunctions...is mechanically consistent across both setup paths, which is good. However, I'd like to understand the intended behavior better:This controller manages ClusterRoles (cluster-scoped resources) and emits events such as:
- Line 251: Warning events when applying roles fails
- Line 302: Warning events for individual role apply errors
- Line 315: Normal events listing successfully applied ClusterRoles
According to the PR context, Crossplane emits events for cluster-scoped resources to the 'default' namespace. If
--restrict-namespaced-eventsfilters out events in the default namespace, would all RBAC ClusterRole events be suppressed?This could impact users' ability to:
- Troubleshoot RBAC permission issues
- Audit which ClusterRoles were created or updated
- Diagnose provider installation problems
Could you clarify whether filtering is intended for these cluster-scoped resource events, or if the filter should distinguish between namespaced and cluster-scoped resources?
Also applies to: 103-103
internal/controller/ops/cronoperation/constructor.go (1)
47-47: Let me search for information about crossplane-runtime PR #868 and verify the status of the v2.1.0-rc.0 release.Let me search more specifically for PR #868 in crossplane-runtime.
Based on my verification, I found that the code implementation is mechanically correct and follows the established pattern used across 23+ controller files in the codebase. However, the reference to PR #868 appears to be incorrect—PR #838 ("Update Event recorder to not parse % format characters in error message") was the event-related improvement in the v2.0.0 release.
The legitimate concern remains about test coverage: the CronOperation reconciler tests contain no tests verifying that events are properly filtered when the
--restrict-namespaced-eventsflag is enabled. This gap deserves attention since event filtering behavior could inadvertently suppress important diagnostic information.Please verify the specific crossplane-runtime PR reference and add tests for CronOperation event filtering behavior.
Key findings:
- ✅
o.EventFilterFunctionsusage is correct and consistent across the codebase- ✅ go.mod shows v2.1.0-rc.0, which should include necessary event filtering support
⚠️ PR #868 reference cannot be verified; the actual event recorder fix was in PR #838- ❌ No specific event filtering tests exist for CronOperation
Signed-off-by: François Rigaut <francois.rigaut@orange.com>
4ddaf8b to
e1cd80c
Compare
|
Thanks @negz ! Rebased + fixed linting. I thought about adding unit and/or e2e tests, but unit tests seem out of topic for this PR, and e2e test are more related to testing updates on k8s resources, so I don't know if it can apply to this PR. Let me know if there is something I can do. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/crossplane/core/core.go (1)
234-242: Consider adding a comment to clarify filter behavior.The filter logic correctly implements the intended behavior, but a more descriptive comment would help future maintainers understand what gets filtered and why.
Consider enhancing the comment:
eventFilterFns := []event.FilterFn{} -// If the option to restrict event emission to namespaced resources if active, -// we create a filter function accordingly +// If the option to restrict event emission is active, create a filter that: +// - Excludes cluster-scoped resources (no namespace) +// - Excludes resources in the "default" namespace +// - Allows events for resources in other namespaces +// This is useful when Crossplane lacks permissions in default namespace or for cluster-scoped resources. if c.RestrictNamespacedEvents { eventFilterFns = append(eventFilterFns, func(obj runtime.Object, _ event.Event) bool { m, err := kmeta.Accessor(obj) return (err == nil && m.GetNamespace() != "" && m.GetNamespace() != "default") }) }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
cmd/crossplane/core/core.go(4 hunks)contributing/guide-adding-external-secret-stores.md(1 hunks)internal/controller/apiextensions/activationpolicy/setup.go(1 hunks)internal/controller/apiextensions/composition/reconciler.go(1 hunks)internal/controller/apiextensions/definition/reconciler.go(1 hunks)internal/controller/apiextensions/managed/setup.go(1 hunks)internal/controller/apiextensions/offered/reconciler.go(1 hunks)internal/controller/apiextensions/revision/reconciler.go(1 hunks)internal/controller/ops/cronoperation/constructor.go(1 hunks)internal/controller/ops/operation/constructor.go(1 hunks)internal/controller/ops/watchoperation/constructor.go(1 hunks)internal/controller/pkg/manager/reconciler.go(3 hunks)internal/controller/pkg/revision/reconciler.go(3 hunks)internal/controller/pkg/runtime/reconciler.go(2 hunks)internal/controller/protection/usage/reconciler.go(3 hunks)internal/controller/rbac/definition/reconciler.go(1 hunks)internal/controller/rbac/provider/binding/reconciler.go(1 hunks)internal/controller/rbac/provider/roles/reconciler.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/controller/pkg/manager/reconciler.go
- internal/controller/rbac/provider/binding/reconciler.go
- internal/controller/pkg/revision/reconciler.go
- internal/controller/apiextensions/definition/reconciler.go
- internal/controller/rbac/definition/reconciler.go
- internal/controller/ops/cronoperation/constructor.go
- contributing/guide-adding-external-secret-stores.md
- internal/controller/pkg/runtime/reconciler.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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:
internal/controller/apiextensions/offered/reconciler.gointernal/controller/rbac/provider/roles/reconciler.gointernal/controller/ops/operation/constructor.gointernal/controller/apiextensions/managed/setup.gointernal/controller/apiextensions/composition/reconciler.gointernal/controller/ops/watchoperation/constructor.gointernal/controller/apiextensions/revision/reconciler.gointernal/controller/protection/usage/reconciler.gointernal/controller/apiextensions/activationpolicy/setup.gocmd/crossplane/core/core.go
**/internal/controller/**
⚙️ CodeRabbit configuration file
**/internal/controller/**: Review controller logic for proper reconciliation patterns, error
handling, and resource management. Pay special attention to conditions
and events: Conditions must be actionable for users (not developers),
stable/deterministic, with proper Type/Reason/Message format. Events
only when something actually happens, with specific details about what
changed. No transient errors in conditions/events. All error messages
must be meaningful to end users - include context about what
resource/operation failed and why.
Files:
internal/controller/apiextensions/offered/reconciler.gointernal/controller/rbac/provider/roles/reconciler.gointernal/controller/ops/operation/constructor.gointernal/controller/apiextensions/managed/setup.gointernal/controller/apiextensions/composition/reconciler.gointernal/controller/ops/watchoperation/constructor.gointernal/controller/apiextensions/revision/reconciler.gointernal/controller/protection/usage/reconciler.gointernal/controller/apiextensions/activationpolicy/setup.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
🧬 Code graph analysis (1)
internal/controller/protection/usage/reconciler.go (1)
internal/controller/apiextensions/activationpolicy/setup.go (1)
WithRecorder(63-67)
🔇 Additional comments (11)
internal/controller/ops/operation/constructor.go (1)
46-46: LGTM - Event filter functions correctly propagated.The variadic
o.EventFilterFunctions...parameter is properly passed toNewAPIRecorder, enabling event filtering for the Operation controller when configured.internal/controller/protection/usage/reconciler.go (1)
108-108: LGTM - Consistent event filter propagation across all Usage controller variants.All three setup functions (
SetupUsage,SetupClusterUsage,SetupLegacyUsage) correctly passo.EventFilterFunctions...to the API recorder, ensuring consistent event filtering behavior across usage-related controllers.Also applies to: 123-123, 139-139
internal/controller/ops/watchoperation/constructor.go (1)
102-102: LGTM - Event filtering enabled for WatchOperation controller.The variadic
o.EventFilterFunctions...parameter is correctly passed through to the recorder.cmd/crossplane/core/core.go (2)
137-137: Verify the intended behavior for cluster-scoped resources.The flag description states it prevents events from being emitted for resources "that are not namespaced." This might be ambiguous - does it mean:
- Resources without a namespace (cluster-scoped), OR
- Resources not in a specific namespace?
The implementation (lines 238-241) filters out events for cluster-scoped resources (empty namespace) AND resources in the "default" namespace. Could you clarify whether filtering cluster-scoped resources is intentional? Based on issue #6348, this might be desired to reduce permission requirements, but it would be helpful to make this explicit in the flag description.
Consider updating the help text to be more explicit:
-RestrictNamespacedEvents bool `default:"false" help:"Prevent events from being produced on resources that are not namespaced. Useful when crossplane does not have permissions in the default namespace."` +RestrictNamespacedEvents bool `default:"false" help:"Prevent events from being emitted for cluster-scoped resources (no namespace) and resources in the 'default' namespace. Useful when Crossplane does not have full cluster admin permissions."`
238-241: Event filtering logic looks correct but could benefit from verification.The filter returns
true(allow event) when:
- No error accessing metadata AND
- Namespace is not empty (not cluster-scoped) AND
- Namespace is not "default"
This means events are filtered out (not emitted) for:
- Objects where metadata access fails
- Cluster-scoped resources (empty namespace)
- Resources in the "default" namespace
This aligns with the PR objectives to avoid emitting events when Crossplane lacks permissions. However, it would be valuable to verify that filtering cluster-scoped resource events doesn't break any expected functionality.
Would you like me to generate a script to search for any code that might rely on events being emitted for cluster-scoped resources?
internal/controller/rbac/provider/roles/reconciler.go (1)
87-87: LGTM - Event filtering consistently applied to both RBAC reconciler paths.Both reconciler initialization paths (with and without
AllowClusterRole) correctly passo.EventFilterFunctions...to the API recorder.Also applies to: 103-103
internal/controller/apiextensions/composition/reconciler.go (1)
71-71: LGTM - Event filtering enabled for Composition revision controller.The change correctly propagates event filter functions to the API recorder.
internal/controller/apiextensions/revision/reconciler.go (1)
62-62: LGTM - Event filtering correctly wired for CompositionRevision controller.The variadic
o.EventFilterFunctions...parameter is properly passed through.internal/controller/apiextensions/offered/reconciler.go (1)
149-149: LGTM - Event filtering enabled for composite resource claim controller.The change correctly passes event filter functions to the API recorder, completing the consistent propagation across all controllers.
internal/controller/apiextensions/activationpolicy/setup.go (1)
40-40: Let me search for information about crossplane-runtime v2.1.0-rc.0 and whether it includes the event filter functions support:Based on comprehensive verification, the original review comment is accurate and grounded in reality.
Code addition requires crossplane-runtime PR #868 to be merged first.
The current
NewAPIRecorderin crossplane-runtime accepts only a singlerecord.EventRecorderparameter, but the code at line 40 passeso.EventFilterFunctions...as an additional argument. This will not compile.The go.mod specifies
github.com/crossplane/crossplane-runtime/v2 v2.1.0-rc.0, which does not include the multi-parameterNewAPIRecordersignature needed to support event filter functions. The changes correctly follow the pattern used across 22+ controller files, but all depend on the runtime PR being merged and the dependency updated.Clarify the dependency resolution plan before merging:
- Will this PR wait for crossplane-runtime PR #868 to be merged and a new version released?
- Or will go.mod temporarily reference a fork/branch containing the runtime changes?
internal/controller/apiextensions/managed/setup.go (1)
43-43: Confirm the pattern is consistent and working as intended; verify event filtering has adequate test coverage and documentation.The code change in managed/setup.go line 43 is correct—it properly integrates the event filtering pattern used across other controllers (activationpolicy, revision, pkg runtime, etc.). The variadic parameter usage is sound.
However, your concerns about testing and user impact are valid and worth addressing at the feature level:
Testing gap confirmed: The event filtering logic in
core.go(which enablesRestrictNamespacedEventsto filter out cluster-scoped and default namespace events) has no corresponding tests in the managed controller tests or elsewhere. ThetestRecorderin reconciler tests only records events but doesn't validate the filtering mechanism.User visibility concern confirmed: When events are filtered, they silently disappear from
kubectl describeandkubectl get eventsoutputs. There's no logging to help users understand why. WhileRestrictNamespacedEventshas CLI help text, end users won't see documentation about filtered events when troubleshooting.The change itself is sound and consistent. Before merging, it would be valuable to verify that the
EventFilterFunctionsfeature across all controllers has adequate test coverage for the filtering behavior and considers adding logging/documentation to help users understand when and why events are suppressed.
Description of your changes
Add a
--restrict-namespaced-eventsoption to prevent events from being emitted in thedefaultnamespace.Can be useful when crossplane does not have full admin permissions on the cluster.
Fixes #6348
Requires crossplane-runtime#868
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.