Skip to content

Commit

Permalink
pkg: don't cache Host identity rule matches
Browse files Browse the repository at this point in the history
Unlike every other identity, the set of labels for the reserved:host
identity is mutable. That means that rules should not cache matches for
this identity.

So, clean up the code around determining matches.

Signed-off-by: Casey Callendrello <cdc@isovalent.com>
  • Loading branch information
squeed authored and julianwiedmann committed May 2, 2024
1 parent de55fd8 commit 8397e45
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 10 deletions.
20 changes: 12 additions & 8 deletions pkg/policy/rule.go
Original file line number Diff line number Diff line change
Expand Up @@ -613,22 +613,26 @@ func (r *rule) resolveIngressPolicy(
func (r *rule) matches(securityIdentity *identity.Identity) bool {
r.metadata.Mutex.Lock()
defer r.metadata.Mutex.Unlock()
var ruleMatches bool
isNode := securityIdentity.ID == identity.ReservedIdentityHost

if ruleMatches, cached := r.metadata.IdentitySelected[securityIdentity.ID]; cached {
return ruleMatches
}
isNode := securityIdentity.ID == identity.ReservedIdentityHost

// Short-circuit if the rule's selector type (node vs. endpoint) does not match the
// identity's type
if (r.NodeSelector.LabelSelector != nil) != isNode {
r.metadata.IdentitySelected[securityIdentity.ID] = false
return ruleMatches
return false
}

// Fall back to costly matching.
if ruleMatches = r.getSelector().Matches(securityIdentity.LabelArray); ruleMatches {
// Update cache so we don't have to do costly matching again.
r.metadata.IdentitySelected[securityIdentity.ID] = true
} else {
r.metadata.IdentitySelected[securityIdentity.ID] = false
ruleMatches := r.getSelector().Matches(securityIdentity.LabelArray)

// Update cache so we don't have to do costly matching again.
// the local Host identity has mutable labels, so we cannot use the cache
if !isNode {
r.metadata.IdentitySelected[securityIdentity.ID] = ruleMatches
}

return ruleMatches
Expand Down
17 changes: 15 additions & 2 deletions pkg/policy/rule_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2641,10 +2641,23 @@ func (ds *PolicyTestSuite) TestMatches(c *C) {
c.Assert(hostRule.matches(selectedIdentity), Equals, false)
c.Assert(hostRule.metadata.IdentitySelected, checker.DeepEquals, map[identity.NumericIdentity]bool{selectedIdentity.ID: false})

// host endpoint is selected by rule, so we it should be added to EndpointsSelected.
// host endpoint is selected by rule, but host labels are mutable, so don't cache them
c.Assert(hostRule.matches(hostIdentity), Equals, true)
c.Assert(hostRule.metadata.IdentitySelected, checker.DeepEquals,
map[identity.NumericIdentity]bool{selectedIdentity.ID: false, hostIdentity.ID: true})
map[identity.NumericIdentity]bool{selectedIdentity.ID: false})

// Assert that mutable host identities are handled
// First, add an additional label, ensure that match succeeds
hostLabels.MergeLabels(labels.NewLabelsFromModel([]string{"foo=bar"}))
hostIdentity = identity.NewIdentity(identity.ReservedIdentityHost, hostLabels)
c.Assert(hostRule.matches(hostIdentity), Equals, true)

// Then, change host to id=c, which is not selected, and ensure match is correct
hostIdentity = identity.NewIdentity(identity.ReservedIdentityHost, labels.NewLabelsFromModel([]string{"id=c"}))
c.Assert(hostRule.matches(hostIdentity), Equals, false)
c.Assert(hostRule.metadata.IdentitySelected, checker.DeepEquals,
map[identity.NumericIdentity]bool{selectedIdentity.ID: false})

}

func BenchmarkRuleString(b *testing.B) {
Expand Down

0 comments on commit 8397e45

Please sign in to comment.