Skip to content
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

Modify IPSet Setfilter to make it suitable for filtering ipsets dynamically #9669

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

Modify IPSet Setfilter to make it suitable for filtering ipsets dynamically #9669

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
12435fc
Modify setfilter to make it suitable for filtering ipsets dynamically
sridhartigera Jan 2, 2025
44b74d0
Filter BPF ipsets
sridhartigera Jan 3, 2025
1154860
Address review comments
sridhartigera Jan 3, 2025
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
18 changes: 15 additions & 3 deletions felix/bpf/ipsets/ipsets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ type bpfIPSets struct {
opRecorder logutils.OpRecorder

lg *log.Entry

filterIPSet func(string) bool
Copy link
Member

Choose a reason for hiding this comment

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

Worth a comment saying what this does. In particular:

  • What does nil mean?
  • Is true "filter in" or "filter out"

}

func NewBPFIPSets(
Expand Down Expand Up @@ -133,6 +135,14 @@ func (m *bpfIPSets) deleteIPSetAndReleaseID(ipSet *bpfIPSet) {
// to ApplyUpdates(), the IP sets will be replaced with the new contents and the set's metadata
// will be updated as appropriate.
func (m *bpfIPSets) AddOrReplaceIPSet(setMetadata ipsets.IPSetMetadata, members []string) {
if m.filterIPSet != nil && !m.filterIPSet(setMetadata.SetID) {
ipSet := m.getExistingIPSetString(setMetadata.SetID)
if ipSet != nil {
ipSet.Deleted = true
m.markIPSetDirty(ipSet)
}
return
}
ipSet := m.getOrCreateIPSet(setMetadata.SetID)
ipSet.Type = setMetadata.Type
m.lg.WithFields(log.Fields{"stringID": setMetadata.SetID, "uint64ID": ipSet.ID, "members": members}).Info("IP set added")
Expand Down Expand Up @@ -370,9 +380,11 @@ func (m *bpfIPSets) markIPSetDirty(data *bpfIPSet) {
m.dirtyIPSetIDs.Add(data.ID)
}

func (m *bpfIPSets) SetFilter(ipSetNames set.Set[string]) {
// Not needed for this IP set dataplane. All known IP sets
// are written into the corresponding BPF map.
func (m *bpfIPSets) SetFilter(fn func(ipSetName string) bool) {
m.filterIPSet = fn
}

func (m *bpfIPSets) MarkDirty(ipsetNames set.Set[string]) {
Copy link
Member

Choose a reason for hiding this comment

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

Feel a bit uneasy that we have a flexible filter function but we're not implementing the rescan here. Someone could come along later and use the filter function without realising this isn't implemented.

Should at least comment "Not implemented because the only filter we use with this IP set is 100% static, based on the IP set name."

}

type bpfIPSet struct {
Expand Down
3 changes: 2 additions & 1 deletion felix/dataplane/ipsets/ipsets_mgr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ type IPSetsDataplane interface {
QueueResync()
ApplyUpdates()
ApplyDeletions() (reschedule bool)
SetFilter(neededIPSets set.Set[string])
SetFilter(fn func(string) bool)
MarkDirty(set.Set[string])
}

// Except for domain IP sets, IPSetsManager simply passes through IP set updates from the datastore
Expand Down
6 changes: 5 additions & 1 deletion felix/dataplane/ipsets/mock_ipsets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,10 @@ func (s *MockIPSets) ApplyDeletions() bool {
return false
}

func (s *MockIPSets) SetFilter(ipSetNames set.Set[string]) {
func (s *MockIPSets) SetFilter(fn func(string) bool) {
// Not implemented for UT.
}

func (s *MockIPSets) MarkDirty(ipSetNames set.Set[string]) {
// Not implemented for UT.
}
14 changes: 12 additions & 2 deletions felix/dataplane/linux/int_dataplane.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -770,7 +770,12 @@ func NewIntDataplaneDriver(config Config) *InternalDataplane {
bpfutils.RemoveBPFSpecialDevices()
} else {
// In BPF mode we still use iptables for raw egress policy.
dp.RegisterManager(newRawEgressPolicyManager(rawTableV4, ruleRenderer, 4, ipSetsV4.SetFilter, config.RulesConfig.NFTables))
mgr := newRawEgressPolicyManager(rawTableV4, ruleRenderer, 4, ipSetsV4.MarkDirty, config.RulesConfig.NFTables)
ipSetsV4.SetFilter(func(ipSetName string) bool {
neededIPSets := mgr.GetNeededIPSets()
return neededIPSets.Contains(ipSetName)
})
dp.RegisterManager(mgr)
}

interfaceRegexes := make([]string, len(config.RulesConfig.WorkloadIfacePrefixes))
Expand Down Expand Up @@ -1035,7 +1040,12 @@ func NewIntDataplaneDriver(config Config) *InternalDataplane {
config.MaxIPSetSize))
dp.RegisterManager(newPolicyManager(rawTableV6, mangleTableV6, filterTableV6, ruleRenderer, 6, config.RulesConfig.NFTables))
} else {
dp.RegisterManager(newRawEgressPolicyManager(rawTableV6, ruleRenderer, 6, ipSetsV6.SetFilter, config.RulesConfig.NFTables))
mgr := newRawEgressPolicyManager(rawTableV6, ruleRenderer, 6, ipSetsV6.MarkDirty, config.RulesConfig.NFTables)
ipSetsV6.SetFilter(func(ipSetName string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Worth a comment here saying what this filter stuff is about.

Suggested change
ipSetsV6.SetFilter(func(ipSetName string) bool {
// When in BPF mode, we still program egress do-not-track rules into
// (ip|nf)tables; set a filter on the IP sets we program so that only the
// IP sets needed for do-not-track are programmed.
ipSetsV6.SetFilter(func(ipSetName string) bool {

neededIPSets := mgr.GetNeededIPSets()
Copy link
Member

Choose a reason for hiding this comment

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

This is a very inefficient way to do this check. Every call to the filter will build the full "needed sets" Set and then throw it away again. That is O(n^2) in the number of IP sets, which is not acceptable.

However, now you've switched to a "pull" model for this, you should be able to do a lot better. What if you made the API mgr.IPSetNeeded(name)? Then you could do a trivial map lookup inside the manager instead of building the unneeded set.

return neededIPSets.Contains(ipSetName)
})
dp.RegisterManager(mgr)
}

dp.RegisterManager(newEndpointManager(
Expand Down
11 changes: 11 additions & 0 deletions felix/dataplane/linux/policy_mgr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,3 +173,14 @@ func (m *policyManager) CompleteDeferredWork() error {
m.ipSetsCallback(merged)
return nil
}

func (m *policyManager) GetNeededIPSets() set.Set[string] {
merged := set.New[string]()
for _, ipSets := range m.neededIPSets {
ipSets.Iter(func(item string) error {
merged.Add(item)
return nil
})
}
return merged
}
6 changes: 4 additions & 2 deletions felix/dataplane/windows/ipsets/ipsets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,8 @@ func (m *IPSets) ApplyDeletions() bool {
return false
}

func (s *IPSets) SetFilter(ipSetNames set.Set[string]) {
// Not needed for Windows.
func (m *IPSets) SetFilter(fn func(ipSetName string) bool) {
Copy link
Member

Choose a reason for hiding this comment

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

Worth commenting that these are deliberately not implemented

}

func (m *IPSets) MarkDirty(ipsetNames set.Set[string]) {
}
34 changes: 16 additions & 18 deletions felix/ipsets/ipsets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ type IPSets struct {

// Optional filter. When non-nil, only these IP set IDs will be rendered into the dataplane
// as Linux IP sets.
neededIPSetNames set.Set[string]
filterIPSet func(string) bool
}

func NewIPSets(ipVersionConfig *IPVersionConfig, recorder logutils.OpRecorder) *IPSets {
Expand Down Expand Up @@ -1092,31 +1092,29 @@ func (s *IPSets) updateDirtiness(name string) {
}
}

func (s *IPSets) SetFilter(ipSetNames set.Set[string]) {
oldSetNames := s.neededIPSetNames
if oldSetNames == nil && ipSetNames == nil {
return
}
s.logCxt.Debugf("Filtering to needed IP set names: %v", ipSetNames)
s.neededIPSetNames = ipSetNames
for name, meta := range s.setNameToAllMetadata {
if s.ipSetNeeded(name) {
s.setNameToProgrammedMetadata.Desired().Set(name, meta)
} else {
s.setNameToProgrammedMetadata.Desired().Delete(name)
}
s.updateDirtiness(name)
}
func (s *IPSets) SetFilter(fn func(string) bool) {
s.filterIPSet = fn
}

func (s *IPSets) ipSetNeeded(name string) bool {
if s.neededIPSetNames == nil {
if s.filterIPSet == nil {
// We're not filtering down to a "needed" set, so all IP sets are needed.
return true
}

// We are filtering down, so compare against the needed set.
return s.neededIPSetNames.Contains(name)
return s.filterIPSet(name)
}

func (s *IPSets) MarkDirty(ipsetNames set.Set[string]) {
for name, meta := range s.setNameToAllMetadata {
if ipsetNames.Contains(name) {
Copy link
Member

Choose a reason for hiding this comment

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

This is now the wrong check because s.filterIPSet should govern whether the IP set is programmed. You should remove the ipsetNames parameter and use the filter. Then this method can be called OnFilterUpdated or something like that and it just triggers a recheck of the filter for all IP sets.

s.setNameToProgrammedMetadata.Desired().Set(name, meta)
} else {
s.setNameToProgrammedMetadata.Desired().Delete(name)
}
s.updateDirtiness(name)
}
}

// CanonicaliseMember converts the string representation of an IP set member to a canonical
Expand Down
18 changes: 15 additions & 3 deletions felix/ipsets/ipsets_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -598,7 +598,10 @@ var _ = Describe("IP sets dataplane", func() {

Context("with filtering to two IP sets", func() {
BeforeEach(func() {
ipsets.SetFilter(set.From(v4MainIPSetName2, v4MainIPSetName))
ipsets.SetFilter(func(ipSetName string) bool {
neededIPSets := set.From(v4MainIPSetName2, v4MainIPSetName)
return neededIPSets.Contains(ipSetName)
})
ipsets.QueueResync()
apply()
})
Expand Down Expand Up @@ -699,7 +702,11 @@ var _ = Describe("IP sets dataplane", func() {

Context("with filtering to single IP set", func() {
BeforeEach(func() {
ipsets.SetFilter(set.From(v4MainIPSetName2))
ipsets.SetFilter(func(ipSetName string) bool {
neededIPSets := set.From(v4MainIPSetName2)
return neededIPSets.Contains(ipSetName)
})
ipsets.MarkDirty(set.From(v4MainIPSetName2))
apply()
})

Expand All @@ -714,7 +721,12 @@ var _ = Describe("IP sets dataplane", func() {

Context("with filtering to both known IP sets", func() {
BeforeEach(func() {
ipsets.SetFilter(set.From(v4MainIPSetName2, v4MainIPSetName))
ipsets.SetFilter(func(ipSetName string) bool {
neededIPSets := set.From(v4MainIPSetName2, v4MainIPSetName)
return neededIPSets.Contains(ipSetName)
})
ipsets.MarkDirty(set.From(v4MainIPSetName2, v4MainIPSetName))

apply()
})

Expand Down
20 changes: 9 additions & 11 deletions felix/nftables/ipsets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ type IPSets struct {

// Optional filter. When non-nil, only these IP set IDs will be rendered into the dataplane
Copy link
Member

Choose a reason for hiding this comment

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

Out of date comment

// as Linux IP sets.
neededIPSetNames set.Set[string]
filterIPSet func(string) bool

nft knftables.Interface
}
Expand Down Expand Up @@ -781,15 +781,13 @@ func (s *IPSets) updateDirtiness(name string) {
}
}

func (s *IPSets) SetFilter(ipSetNames set.Set[string]) {
oldSetNames := s.neededIPSetNames
if oldSetNames == nil && ipSetNames == nil {
return
}
s.logCxt.Debugf("Filtering to needed IP set names: %v", ipSetNames)
s.neededIPSetNames = ipSetNames
func (s *IPSets) SetFilter(fn func(ipSetName string) bool) {
s.filterIPSet = fn
}

func (s *IPSets) MarkDirty(ipsetNames set.Set[string]) {
for name, meta := range s.setNameToAllMetadata {
if s.ipSetNeeded(name) {
if ipsetNames.Contains(name) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here, new filter func should govern the check.

s.setNameToProgrammedMetadata.Desired().Set(name, meta)
} else {
s.setNameToProgrammedMetadata.Desired().Delete(name)
Expand All @@ -799,13 +797,13 @@ func (s *IPSets) SetFilter(ipSetNames set.Set[string]) {
}

func (s *IPSets) ipSetNeeded(name string) bool {
if s.neededIPSetNames == nil {
if s.filterIPSet == nil {
// We're not filtering down to a "needed" set, so all IP sets are needed.
return true
}

// We are filtering down, so compare against the needed set.
return s.neededIPSetNames.Contains(name)
return s.filterIPSet(name)
}

// CanonicaliseMember converts the string representation of an nftables set member to a canonical
Expand Down
Loading