Add namespace exclusion for watchers#623
Conversation
Pull latest upstream changes
|
Automatically marked as stale due to no recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@winromulus would love to get this reviewed/merged if possible |
winromulus
left a comment
There was a problem hiding this comment.
Thanks for tackling this — the bounded-channel pressure in large clusters is a real operational pain point, and filtering at the producer side before the channel is the right layer to catch it.
A few observations from reading through, mostly small refinements plus one larger ask around test coverage.
What works well
- Good choice of filter location — dropping events before
WriteAsyncmeans the channel never sees the noise. - Logging is thoughtful: patterns at session start, excluded count at session close when non-zero. Useful operational signal.
IOptionsMonitor.CurrentValuecaptured at session start matches howWatcherTimeoutalready behaves — consistent with the existing pattern.- Helm wiring is complete across both
deployment.yamlandcron.yaml,values.yamlhas an explanatory comment, README table updated. Regex.Escapebefore replacing\*/\?is the correct glob-to-regex translation;RegexOptions.Compiledis appropriate for a per-event matcher.
Main ask: tests
The feature currently has no coverage. Given the integration suite already uses Testcontainers.K3s with a clean builder pattern (see MirroringIntegrationTests.AutoReflect_To_AllowedNamespaces), this feels like a good candidate for:
- A unit-level test of the glob parser/matcher covering edge cases like empty strings, whitespace,
*alone, multiple patterns,?wildcards, and namespace names containing regex metacharacters.ParseGlobPatternsandIsNamespaceExcludedareprivate staticin the base class today — extracting them to a smallGlobMatcherhelper would make them trivially testable and slightly more reusable. - An integration test mirroring the shape of
AutoReflect_To_AllowedNamespaces: setES_Reflector__Watcher__ExcludedNamespaces=excluded-*, create a source in an excluded namespace and verify no mirrors appear; create one in an allowed namespace and verify they do.
Without coverage, a future well-meaning simplification of the escape sequence could silently change ? matching or similar.
Scope clarification worth adding to the docs
One thing an operator might reasonably misread: this setting drops watch events originating in the listed namespaces, but it doesn't stop auto-reflection from creating mirrors into them — target-side filtering still runs off the source's reflection-auto-namespaces annotation. A sentence in the README covering that distinction would head off confusion.
On NamespaceWatcher
Since V1Namespace is cluster-scoped, item.Metadata.NamespaceProperty is null for namespace events and the filter short-circuits. That's actually correct — you want NamespaceWatcher to keep seeing events so auto-reflection fires on new namespace creation — just flagging it so the interaction is explicit in the review record.
Smaller notes left inline. Overall a focused, well-scoped change solving a concrete problem.
Thanks again for the contribution — this is clearly solving a real-world pain point, and I appreciate the care taken with the Helm wiring and observability. Happy to merge once the items above are addressed.
Extract namespace glob matching into a shared internal helper, expose internals to the test assembly, and add unit and integration coverage for skipping reflection from excluded namespaces.
|
Thank you so much for the detailed review and directions. I have to admit, I used quite a lot of AI for the unit/integration tests so please take a closer look there: |
winromulus
left a comment
There was a problem hiding this comment.
Really appreciate the engagement here — this is a clean, well-tested response. Walked through each of the earlier asks against the current code:
- Drop redundant
.Where→GlobMatcher.ParseGlobPatternsuses onlySplit(RemoveEmptyEntries | TrimEntries).Select(...). ✓ - README wording → now "exclude from reflection processing." ✓
- Case sensitivity → patterns normalized to lowercase at parse time. ✓
- NamespaceWatcher no-op comment → clear 3-line explanation inline. ✓
- Extract to helper class →
GlobMatcherwith a clean static surface. ✓ - Unit tests → 19 tests in
GlobMatcherTestswith good coverage; the metacharacter cases (ns.specialas literal,ns[1]with brackets) are the ones that would catch a future refactor breaking the escape-before-substitute order. Nice. - Integration test →
ExcludedNamespacesIntegrationTestswith its own fixture wiring. ✓
On the AI-assisted tests: honest take — the unit tests are genuinely solid, especially the metacharacter edge cases. The integration test is functionally correct with one structural observation left inline.
Three small things, none blocking — all covered inline. Otherwise this is in really good shape.
|
Heads up — the channel-buffer bump ( |
Update excludedNamespaces docs to describe reflection processing rather than watching, and expand integration coverage to verify excluded source namespaces do not auto-reflect while excluded target namespaces still receive reflections.
I believe all the latest requests have been addressed now. 🙇🏼 |
winromulus
left a comment
There was a problem hiding this comment.
Thanks for sticking with this through the back-and-forth — really appreciate the responsiveness on every piece of feedback.
The expanded integration test landed exactly where I was hoping, and I particularly like that the second test reads like inline documentation for the source-vs-target distinction. The unit tests on the regex metacharacter cases are great too — exactly the kind of thing that'll catch a well-meaning future refactor breaking the escape order.
Approving. Thanks again for solving a real operational pain point for folks running this at scale.
… (10.0.35 → 10.0.39) (#293) This PR contains the following updates: | Package | Update | Change | |---|---|---| | [ghcr.io/emberstack/helm-charts/reflector](https://github.com/emberstack/kubernetes-reflector) | patch | `10.0.35` → `10.0.39` | --- ### Release Notes <details> <summary>emberstack/kubernetes-reflector (ghcr.io/emberstack/helm-charts/reflector)</summary> ### [`v10.0.39`](https://github.com/emberstack/kubernetes-reflector/releases/tag/v10.0.39) [Compare Source](emberstack/kubernetes-reflector@v10.0.38...v10.0.39) The release process is automated. #### What's Changed - Add namespace exclusion for watchers by [@​komapa](https://github.com/komapa) in [#​623](emberstack/kubernetes-reflector#623) #### New Contributors - [@​komapa](https://github.com/komapa) made their first contribution in [#​623](emberstack/kubernetes-reflector#623) **Full Changelog**: <emberstack/kubernetes-reflector@v10.0.38...v10.0.39> ### [`v10.0.38`](https://github.com/emberstack/kubernetes-reflector/releases/tag/v10.0.38) [Compare Source](emberstack/kubernetes-reflector@v10.0.37...v10.0.38) The release process is automated. #### What's Changed - Add namespace label selector support for reflection by [@​davidswimbird](https://github.com/davidswimbird) in [#​620](emberstack/kubernetes-reflector#620) #### New Contributors - [@​davidswimbird](https://github.com/davidswimbird) made their first contribution in [#​620](emberstack/kubernetes-reflector#620) **Full Changelog**: <emberstack/kubernetes-reflector@v10.0.37...v10.0.38> ### [`v10.0.37`](https://github.com/emberstack/kubernetes-reflector/releases/tag/v10.0.37) [Compare Source](emberstack/kubernetes-reflector@v10.0.36...v10.0.37) The release process is automated. #### What's Changed - Bump the all-dependencies group with 8 updates by [@​dependabot](https://github.com/dependabot)\[bot] in [#​642](emberstack/kubernetes-reflector#642) **Full Changelog**: <emberstack/kubernetes-reflector@v10.0.36...v10.0.37> ### [`v10.0.36`](https://github.com/emberstack/kubernetes-reflector/releases/tag/v10.0.36) [Compare Source](emberstack/kubernetes-reflector@v10.0.35...v10.0.36) The release process is automated. #### What's Changed - Bump the all-dependencies group with 6 updates by [@​dependabot](https://github.com/dependabot)\[bot] in [#​641](emberstack/kubernetes-reflector#641) **Full Changelog**: <emberstack/kubernetes-reflector@v10.0.35...v10.0.36> </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 6am on Monday" in timezone Europe/Paris, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled because a matching PR was automerged previously. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xMDEuMSIsInVwZGF0ZWRJblZlciI6IjQzLjEwMS4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJyZW5vdmF0ZS9jb250YWluZXIiLCJ0eXBlL3BhdGNoIl19--> Reviewed-on: https://git.erwanleboucher.dev/eleboucher/homelab/pulls/293
The reflector watches all ConfigMaps and Secrets across every namespace in the cluster. In large clusters with thousands of ConfigMaps, this creates massive event volume — the vast majority of which are for resources with no reflector annotations. This unnecessary traffic saturates the internal bounded channel (256 slots) and can contribute to silent stalls.
This PR adds a new
excludedNamespacesconfiguration option that accepts comma-separated glob patterns. Events from matching namespaces are dropped before entering the bounded channel, eliminating noise at the earliest possible point.