Skip to content

Address feedback for recent ExtraComputedSelector changes #12505

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nelljerram wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Address feedback for recent ExtraComputedSelector changes #12505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c8786ab
Revert generic usage for Add/RemoveExtraComputedSelector
nelljerram Apr 16, 2026
e32f0a7
Pass caller on to label index so that we get caller-scoped callbacks
nelljerram Apr 16, 2026
0524b8d
Send Matched and MatchStopped callbacks only to the relevant listener
nelljerram Apr 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 30 additions & 46 deletions felix/calc/active_rules_calculator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ type FelixSender interface {
type PolicyMatchListener interface {
OnPolicyMatch(policyKey model.PolicyKey, endpointKey model.EndpointKey)
OnPolicyMatchStopped(policyKey model.PolicyKey, endpointKey model.EndpointKey)
}

type ComputedSelectorListener interface {
OnComputedSelectorMatch(cs string, endpointKey model.EndpointKey)
OnComputedSelectorMatchStopped(cs string, endpointKey model.EndpointKey)
}
Expand Down Expand Up @@ -91,9 +94,6 @@ type ActiveRulesCalculator struct {
// log out those profiles at the end of the resync.
missingProfiles set.Set[string]

// Tracks components that have called AddExtraComputedSelector.
computedSelectorCallers map[string]set.Set[any]

// Callback objects.
RuleScanner ruleScanner
PolicyMatchListeners []PolicyMatchListener
Expand All @@ -102,7 +102,10 @@ type ActiveRulesCalculator struct {
OnAlive func()
}

type computedSelector string
type computedSelectorKey struct {
selector string
caller ComputedSelectorListener
}

Comment thread
nelljerram marked this conversation as resolved.
func NewActiveRulesCalculator() *ActiveRulesCalculator {
arc := &ActiveRulesCalculator{
Expand All @@ -120,8 +123,6 @@ func NewActiveRulesCalculator() *ActiveRulesCalculator {

// Cache of profile IDs by local endpoint.
endpointKeyToProfileIDs: NewEndpointKeyToProfileIDMap(),

computedSelectorCallers: make(map[string]set.Set[any]),
}
arc.labelIndex = labelindex.NewInheritIndex(arc.onMatchStarted, arc.onMatchStopped)
return arc
Expand Down Expand Up @@ -297,39 +298,24 @@ func (arc *ActiveRulesCalculator) OnUpdate(update api.Update) (_ bool) {
// OnComputedSelectorMatch and OnComputedSelectorMatchStopped callbacks when that selector
// matches/stops matching local endpoints, allowing the expensive selector index to be shared.
//
// Registration is tracked per caller. The caller identity must therefore be stable, and the
// same caller value (including the same inferred type T) must be passed to
// RemoveExtraComputedSelector to remove that caller's registration. Repeated adds from the same
// caller are deduplicated.
//
// The underlying selector is added to the label index when the first caller registers it, and it
// is only removed after the last caller removes its registration. Callbacks for matches/stops
// matching continue to be delivered to all registered PolicyMatchListeners while the selector is
// present.
func AddExtraComputedSelector[T comparable](arc *ActiveRulesCalculator, cs string, caller T) {
callers := arc.computedSelectorCallers[cs]
if callers == nil {
callers = set.New[any]()
arc.computedSelectorCallers[cs] = callers
sel, err := selector.Parse(cs)
if err != nil {
log.WithError(err).Panicf("Failed to parse computed selector %#v", cs)
}
arc.labelIndex.UpdateSelector(computedSelector(cs), sel)
// Registration is tracked per caller. The caller identity must therefore be stable, and the same
// caller value must be passed to RemoveExtraComputedSelector to remove that caller's registration.
func (arc *ActiveRulesCalculator) AddExtraComputedSelector(cs string, caller ComputedSelectorListener) {
sel, err := selector.Parse(cs)
if err != nil {
log.WithError(err).Panicf("Failed to parse computed selector %#v", cs)
}
callers.Add(any(caller))
arc.labelIndex.UpdateSelector(computedSelectorKey{
selector: cs,
caller: caller,
}, sel)
}
Comment on lines +308 to 312
Copy link

Copilot AI Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using computedSelectorKey{selector: cs, caller: caller} as the selector ID means the same selector string will be evaluated separately for every caller (scanAllLabels/scanAllSelectors run per selector ID). If multiple components register the same selector, this will multiply selector evaluation work and increase memory usage. A shared selector ID plus a separate callers set would keep evaluation cost constant while still allowing per-caller callback attribution.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

@nelljerram nelljerram Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but this is the same as the existing situation for policy selectors.


func RemoveExtraComputedSelector[T comparable](arc *ActiveRulesCalculator, cs string, caller T) {
callers := arc.computedSelectorCallers[cs]
if callers == nil {
return
}
callers.Discard(any(caller))
if callers.Len() == 0 {
arc.labelIndex.DeleteSelector(computedSelector(cs))
delete(arc.computedSelectorCallers, cs)
}
func (arc *ActiveRulesCalculator) RemoveExtraComputedSelector(cs string, caller ComputedSelectorListener) {
arc.labelIndex.DeleteSelector(computedSelectorKey{
selector: cs,
caller: caller,
})
}

func policyForceProgrammed(policy *model.Policy) bool {
Expand Down Expand Up @@ -394,11 +380,10 @@ func (arc *ActiveRulesCalculator) updateEndpointProfileIDs(key model.Key, profil
}

func (arc *ActiveRulesCalculator) onMatchStarted(selID, labelId any) {
if cs, ok := selID.(computedSelector); ok {
for _, l := range arc.PolicyMatchListeners {
if labelId, ok := labelId.(model.EndpointKey); ok {
l.OnComputedSelectorMatch(string(cs), labelId)
}
if key, ok := selID.(computedSelectorKey); ok {
// ComputedSelector callers are only interested in endpoints.
if labelId, ok := labelId.(model.EndpointKey); ok {
key.caller.OnComputedSelectorMatch(key.selector, labelId)
}
return
}
Expand Down Expand Up @@ -426,11 +411,10 @@ func (arc *ActiveRulesCalculator) onMatchStarted(selID, labelId any) {
}

func (arc *ActiveRulesCalculator) onMatchStopped(selID, labelId any) {
if cs, ok := selID.(computedSelector); ok {
for _, l := range arc.PolicyMatchListeners {
if labelId, ok := labelId.(model.EndpointKey); ok {
l.OnComputedSelectorMatchStopped(string(cs), labelId)
}
if key, ok := selID.(computedSelectorKey); ok {
// ComputedSelector callers are only interested in endpoints.
if labelId, ok := labelId.(model.EndpointKey); ok {
key.caller.OnComputedSelectorMatchStopped(key.selector, labelId)
}
return
}
Expand Down
Loading
Loading