Add namespace label selector support for reflection#620
Add namespace label selector support for reflection#620winromulus merged 11 commits intoemberstack:mainfrom
Conversation
Add two new annotations that allow selecting target namespaces by Kubernetes label selectors instead of only name regex patterns: - reflection-allowed-namespaces-selector - reflection-auto-namespaces-selector When both a name-pattern and a label selector annotation are set, a namespace matches if it satisfies either condition (OR logic). The label selector supports standard Kubernetes syntax: equality (=, ==, !=), existence, and set-based (in, notin) expressions. Closes emberstack#409
a75fa1c to
a704f21
Compare
|
@davidswimbird while I review this, please update the documentation as well (readme) |
There was a problem hiding this comment.
Pull request overview
Adds namespace label selector support to the reflector’s “allowed namespaces” and “auto namespaces” logic, enabling selection of target namespaces via standard Kubernetes label selector syntax in addition to existing name/regex matching.
Changes:
- Introduces two new reflection annotations for label selectors (allowed/auto namespaces selectors) and wires them into
MirroringProperties. - Implements label-selector matching (with OR logic vs name-pattern matching) and updates mirroring flows to use
V1Namespacewhen available. - Adds unit tests and test helpers for selector behavior; exposes internals to the test project.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/ES.Kubernetes.Reflector.Tests/LabelSelectorMatchTests.cs | Adds unit tests for label selector parsing/matching and OR logic with name patterns. |
| tests/ES.Kubernetes.Reflector.Tests/Additions/ReflectorAnnotationsBuilder.cs | Adds builder helpers for the new selector annotations in tests. |
| src/ES.Kubernetes.Reflector/Mirroring/Core/ResourceMirror.cs | Caches namespaces for selector evaluation and uses V1Namespace overloads for matching. |
| src/ES.Kubernetes.Reflector/Mirroring/Core/MirroringPropertiesExtensions.cs | Parses new annotations, adds selector-aware matching, and implements label selector evaluation. |
| src/ES.Kubernetes.Reflector/Mirroring/Core/MirroringProperties.cs | Adds properties for allowed/auto namespace label selectors. |
| src/ES.Kubernetes.Reflector/Mirroring/Core/Annotations.cs | Adds constants for the new selector annotations. |
| src/ES.Kubernetes.Reflector/ES.Kubernetes.Reflector.csproj | Adds InternalsVisibleTo to allow unit tests to access LabelSelectorMatch. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Document the new reflection-allowed-namespaces-selector and reflection-auto-namespaces-selector annotations from emberstack#620.
Invalid or degenerate selectors (e.g. ",", "!", "!=value") previously produced no valid requirements or empty keys, causing LabelSelectorMatch to return true and unintentionally allow reflection to all namespaces. Now returns false when no requirements are parsed or a key is empty.
The namespace cache was only populated on Added events, so label changes (Modified) left stale entries and deleted namespaces were never evicted. - Handle Modified: update cache and reconcile auto-reflections (create if now matching, delete if no longer matching) - Handle Deleted: evict cache entry and clean up auto-reflection tracking
Cover unbalanced parentheses, empty set-based expressions, and bare operators to document that the parser fails closed on all of these.
|
Automatically marked as stale due to no recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
Hey @winromulus. How's the review coming along? :) |
|
@davidswimbird a couple of comments left as pending. Can you check those please? |
|
@winromulus resolved all comments. They had already been adressed, just didn't want to resolve comments without you having a look :) |
winromulus
left a comment
There was a problem hiding this comment.
Hey — thanks for your patience with this one, sorry it sat longer than it should have. Circling back now with a proper pass.
First, the Copilot round was handled really well. I checked each one against current code: the fail-closed path in LabelSelectorMatch, the Modified + Deleted handlers, the negative-case tests, and the updated XML doc are all solid. The negative test coverage in particular (the InvalidSelector_* theories) is exactly the right shape.
I had a couple of items from an earlier review pass that didn't come through on the PR conversation — apologies for that, they're on me. I've restated them as inline comments below along with a few smaller things I spotted on this pass.
On tests: the unit coverage for the parser is thorough, but there's no end-to-end coverage for the feature itself. Given the existing Testcontainers.K3s pattern (see MirroringIntegrationTests.AutoReflect_To_AllowedNamespaces), it would be worth one or two integration tests — particularly one that exercises the new Modified-path logic (label changes creating/removing reflections), since that's easy to silently break with a future refactor.
Thanks again for the contribution — the core approach is sound and the implementation is genuinely considerate (the XML docs, the overload preservation, the InternalsVisibleTo pattern, all good calls). Happy to merge once the items below are resolved.
…gured
When a source configures only a label selector (no name pattern) and the
target namespace isn't in _namespaceCache (e.g. after a watcher reconnect
or at startup before namespace events arrive), the string overload of
CanBeReflectedToNamespace fell through to PatternListMatch("", ns),
which returns true for empty patterns — silently allowing reflection to
every namespace.
Deny reflection on cache miss when a label selector is configured so the
selector cannot be bypassed by missing cache state.
Adds two Testcontainers.K3s integration tests covering the new reflection-allowed-namespaces-selector annotation: - AutoReflect_To_NamespacesMatchingLabelSelector: source with a label selector reflects only to namespaces whose labels match. - AutoReflect_UpdatesReflections_WhenNamespaceLabelsChange: exercises the namespace Modified-path so that relabeling an existing namespace creates or removes the auto-reflection accordingly. This covers logic that is easy to silently break with a future refactor. Extends BaseIntegrationTest with a labels argument on CreateNamespaceAsync and a resilience pipeline that waits for a resource to become absent.
Previously the label selector parser accepted any characters matching a loose regex as a key, and malformed selectors silently produced no match with no feedback to the operator — a typo in an annotation looked like "nothing reflected". This change: - Refactors the parser to produce a V1LabelSelector and a list of parse errors, separating parsing from evaluation. Matching now uses the standard Kubernetes operator semantics (In, NotIn, Exists, DoesNotExist). - Adds strict validation for label keys (optional DNS-subdomain prefix up to 253 chars, name segment up to 63 chars, start/end alphanumeric) and label values, matching the constraints Kubernetes itself enforces. - Exposes MirroringProperties.GetLabelSelectorErrors() so callers can surface parse errors. - In ResourceMirror.HandleUpsert, logs a warning for each parse error the first time a given signature is observed for a source (and when it changes), so operators get signal for misconfigured annotations without log spam on repeated watch events. LabelSelectorMatch still fails closed on invalid input; existing behavior is preserved for all previously-supported valid selectors.
Mirror auto-reflection's namespace event handling for direct reflections so selector changes take effect immediately rather than waiting for the next source upsert to prune the cache. Also clear direct cache entries on namespace deletion to match the existing auto cleanup.
Namespace Modified events fire on every status, annotation, or resourceVersion bump. Reflection eligibility only depends on the namespace name and labels, so short-circuit the auto-source loop and direct-reflection rebalance when the cached namespace's labels match the incoming ones.
ba60e77 to
baf0a2d
Compare
|
Thanks for the review @winromulus! Appreciate that you took the time and dug deep, you found things I would never have caught myself. I've adressed your inline comments, and added integration tests for the full feature as per the root review comment. Let me know if there's anything else I should adress :) |
winromulus
left a comment
There was a problem hiding this comment.
Really thorough response — appreciate the care on every point. I walked through each of the new commits against the earlier asks and they all hold up.
- Security fallback (983e507):
CanBeReflectedToNamespaceCached/CanBeAutoReflectedToNamespaceCachednow fail closed when a selector is configured and the namespace isn't cached, with a debug log for visibility. Clean, preserves existing behavior for regex-only sources. - Parser refactor (551bdc9):
V1LabelSelector/V1LabelSelectorRequirementas the internal representation, K8s-spec label key/value validation (prefix + name, length limits, regexes), and warning logs deduped per source-signature — exactly the surface area that was missing. The split betweenTryParseSetBased/TryParseInequality/TryParseEquality/TryParseExistencereads cleanly and each path validates independently.GetLabelSelectorErrorsas a public diagnostic is a nice touch. - Direct reflection rebalance (25ab8ff): both Modified and Deleted now walk
_directReflectionCache, and theDirectReflect_StopsUpdating_WhenNamespaceLabelsNoLongerMatchSelectorintegration test exercises exactly the shift-label-mid-flight scenario. - Labels-equal short-circuit (baf0a2d):
NamespaceLabelsEqualwith empty-dict coalescing, dedicated unit tests covering null/empty/add/remove/rename/same, applied before both the auto-source loop and the direct-rebalance loop. - Integration tests:
AutoReflect_To_NamespacesMatchingLabelSelectorandAutoReflect_UpdatesReflections_WhenNamespaceLabelsChangeplus the direct-reflection test give end-to-end protection for the new behavior. Pattern matches the existingAutoReflect_To_AllowedNamespacesshape, which is exactly right.
Two very small observations I won't block on:
NamespaceLabelsEqualusesSequenceEqual, which is order-sensitive — two semantically-equal dicts with different insertion order would compare unequal and fall through to reconciliation. Correctness is preserved (more work, not less), so it's fine as-is; aCount + All(TryGetValue...)check would be order-independent if you want to tune it.TryParseEqualitysilently overwrites duplicate keys with conflicting values — e.g.env=prod,env=stagingparses asmatchLabels["env"] = "staging"rather than being rejected. Real-world operators won't write that, and kubectl would reject it, but a "duplicate key" check would make the parser a touch more faithful. Up to you — I'd leave it.
Really strong engagement through the review. Approving — happy to merge.
… (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
Add two new annotations that allow selecting target namespaces by
Kubernetes label selectors instead of only name regex patterns:
When both a name-pattern and a label selector annotation are set, a
namespace matches if it satisfies either condition (OR logic). The label
selector supports standard Kubernetes syntax: equality (=, ==, !=),
existence, and set-based (in, notin) expressions.
This is useful when you want to reflect a secret to a subset of non-deterministic namespaces, that can't be represented with a regex.
For my use case, it makes the manual overhead of managing reflector a lot smaller, as I can automate the label configuration for my namespaces ahead of time, to know that they will be receiving the secrets when the namespace is created with a specified label.
This is based on the suggestion in: Discussion #409