Make matchers part of the Policy interface #2514
Conversation
hscontrol/mapper/mapper.go
Outdated
| // access each-other at all and remove them from the peers. | ||
| if len(filter) > 0 { | ||
| changed = policy.FilterNodesByACL(node, changed, filter) | ||
| matchers := polMan.Matchers() |
There was a problem hiding this comment.
It occurred to me after the implementation that this could be subject to race conditions -- the ACLs could be updated between the call polMan.Filter() and the call to polMan.Matchers().
Would it maybe make more sense if Policy.Filter() returned a tuple ([]tailcfg.FilterRule, []matcher.Match) so that the returned filter rules and matches are always in sync?
There was a problem hiding this comment.
That is a good point, I think that is fair, particularly since it will just be returned and not computed. can probably just underscore the matchers when we dont need them.
There was a problem hiding this comment.
Done (and also added matchers to tests where the Policy function is tested).
|
Given that there is already a changelog section on the policy rework, do you think that a separate entry is necessary for this change? |
| filterChanged := filterHash == pm.filterHash | ||
| pm.filter = filter | ||
| pm.filterHash = filterHash | ||
| if filterChanged { |
There was a problem hiding this comment.
There is a couple of failing tests, not sure why, I wonder if this somehow could be the cause if it prevents the matchers from being generated in some conditions, like empty policies? I really do not think it should be related, but I cant see anything else special about v2.
There was a problem hiding this comment.
Isn't the filterChanged actually computed incorrectly a few lines above? Wouldn't filterChanged := filterHash != pm.filterHash make more sense? Should I change that?
There was a problem hiding this comment.
oh dear, you are right... yes please.
Pull Request Revisions
✅ AI review completed for r3 HelpReact with emojis to give feedback on AI-generated reviews:
We'd love to hear from you—reach out anytime at [email protected]. |
|
|
||
| filterHash := deephash.Hash(&filter) | ||
| filterChanged := filterHash == pm.filterHash | ||
| filterChanged := filterHash != pm.filterHash |
There was a problem hiding this comment.
I see that only SSH tests in v2 fail now, I suspect that it is because there was a bug here that never returned "early" on line 102, causing 110 (clear ssh) to always run. Now that it does return early, I suspect that my logic for when the ssh needs to be clear is wrong, and it wasnt discovered because of this.
There was a problem hiding this comment.
I don't know if this occurs in the integration tests, but here is an example where I think updateLocked behaves incorrectly with the early return: If only the SSHs field of the policy changes, the call to updateLocked in SetPolicy would AFAICT return false, nil, but the SSH policy needs to be recompiled nevertheless in this case.
Would it maybe make sense to just always clear the SSH policy map or add an additional guard checking whether the SSHs field of the policy has changed?
There was a problem hiding this comment.
That a good point, I moved the clear up to a defer so it always runs and rebased. If that makes the tests happy, then I will merge it!
Signed-off-by: Kristoffer Dalby <[email protected]>
7de444f to
dc79941
Compare
| filterHash := deephash.Hash(&filter) | ||
| filterChanged := filterHash == pm.filterHash | ||
| filterChanged := filterHash != pm.filterHash | ||
| pm.filter = filter | ||
| pm.filterHash = filterHash | ||
| if filterChanged { | ||
| pm.matchers = matcher.MatchesFromFilterRules(pm.filter) | ||
| } |
There was a problem hiding this comment.
In the updateLocked() method, you're using filterChanged to determine whether to update matchers, but the condition to set filterChanged is incorrect. On line 80, it's defined as filterChanged := filterHash != pm.filterHash, indicating that matchers should be updated when the filter has changed. However, if filterChanged is true, then we should always update the matchers. This logic seems correct, but it differs from the code that was removed (where it was incorrectly checking filterHash == pm.filterHash).
| // Filter returns the current filter rules for the entire tailnet and the associated matchers. | ||
| func (pm *PolicyManager) Filter() ([]tailcfg.FilterRule, []matcher.Match) { | ||
| pm.mu.Lock() | ||
| defer pm.mu.Unlock() | ||
| return pm.filter | ||
| return pm.filter, pm.matchers | ||
| } |
There was a problem hiding this comment.
In hscontrol/policy/v2/policy.go, there's a potential race condition in the Filter() method. You're returning the slice of matchers, but if another goroutine calls updateLocked() between when you get the lock and return, the matchers might become out of sync with the filter rules. Consider making a defensive copy of both slices or ensuring that any modification of these fields is atomic.
This PR makes the
Matches that are computed fromFilterRules part of thePolicyinterface, so that they don't have to be recomputed at each call toNode.CanAccess. There have already been some discussion on this approach in #2416.