Skip to content

Commit 92572c7

Browse files
committed
fix(ipset): ignore non-kube-router ipsets
Attempt to filter out sets that we are not authoritative for to avoid race conditions with other operators (like Istio) that might be attempting to modify ipsets at the same time.
1 parent 8bf2e56 commit 92572c7

File tree

7 files changed

+620
-8
lines changed

7 files changed

+620
-8
lines changed

pkg/controllers/netpol/network_policy_controller_test.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -741,6 +741,10 @@ func (ips *fakeIPSet) Restore() error {
741741
return nil
742742
}
743743

744+
func (ips *fakeIPSet) RestoreSets(_ []string) error {
745+
return nil
746+
}
747+
744748
func (ips *fakeIPSet) Flush() error {
745749
return nil
746750
}

pkg/controllers/netpol/policy.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,9 +152,11 @@ func (npc *NetworkPolicyController) syncNetworkPolicyChains(networkPoliciesInfo
152152
}
153153
}
154154

155+
knownIPSets := utils.ConvertMapKeysToSlice[string](activePolicyIPSets)
156+
155157
for ipFamily, ipset := range npc.ipSetHandlers {
156158
restoreStart := time.Now()
157-
err := ipset.Restore()
159+
err := ipset.RestoreSets(knownIPSets)
158160
restoreEndTime := time.Since(restoreStart)
159161

160162
if npc.MetricsEnabled {

pkg/controllers/proxy/network_services_controller.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -710,7 +710,7 @@ func (nsc *NetworkServicesController) syncIpvsFirewall() error {
710710

711711
setHandler.RefreshSet(serviceIPPortsSetName, serviceIPPortsIPSets[family], utils.TypeHashIPPort)
712712

713-
err := setHandler.Restore()
713+
err := setHandler.RestoreSets([]string{serviceIPsIPSetName, serviceIPPortsSetName})
714714
if err != nil {
715715
return fmt.Errorf("could not save ipset for service firewall: %v", err)
716716
}

pkg/controllers/routing/network_routes_controller.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -864,7 +864,7 @@ func (nrc *NetworkRoutingController) syncNodeIPSets() error {
864864

865865
ipSetHandler.RefreshSet(nodeAddrsIPSetName, currentNodeIPs[family], utils.TypeHashIP)
866866

867-
err = ipSetHandler.Restore()
867+
err = ipSetHandler.RestoreSets([]string{podSubnetsIPSetName, nodeAddrsIPSetName})
868868
if err != nil {
869869
return fmt.Errorf("failed to sync pod subnets / node addresses ipsets: %v", err)
870870
}

pkg/utils/ipset.go

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"errors"
99
"fmt"
1010
"os/exec"
11+
"slices"
1112
"sort"
1213
"strings"
1314

@@ -163,6 +164,7 @@ type IPSetHandler interface {
163164
DestroyAllWithin() error
164165
Save() error
165166
Restore() error
167+
RestoreSets([]string) error
166168
Flush() error
167169
Get(setName string) *Set
168170
Sets() map[string]*Set
@@ -510,13 +512,35 @@ func scrubInitValFromOptions(options []string) []string {
510512
return options
511513
}
512514

515+
// buildIPSetRestore creates a set of ipset rules that can be fed into ipset restore. ipset contains a list of "sets"
516+
// that we will act on. If setIncludeNames is not null, then the list of sets will be dynamically filtered to ensure
517+
// that the string for ipset restore only includes sets with those names.
518+
//
519+
// In order to ensure that our changes are atomic, we create a temporary set per unique ipset options, then we load
520+
// that temporary set with the new list of entries, then we swap them to the final name of the set, and then flush the
521+
// temporary set before potentially reusing it in a future load.
522+
//
523+
// The temporary set is created only as needed to ensure that we don't needlessly create temporary sets. Temporary sets
524+
// have to be unique to the "Type" of the set. This is things like: hash:ip and others.
525+
//
513526
// Build ipset restore input
514527
// ex:
515528
// create KUBE-DST-3YNVZWWGX3UQQ4VQ hash:ip family inet hashsize 1024 maxelem 65536 timeout 0
516529
// add KUBE-DST-3YNVZWWGX3UQQ4VQ 100.96.1.6 timeout 0
517-
func buildIPSetRestore(ipset *IPSet) string {
530+
func buildIPSetRestore(ipset *IPSet, setIncludeNames []string) string {
518531
setNames := make([]string, 0, len(ipset.sets))
519-
for setName := range ipset.sets {
532+
for setName, set := range ipset.sets {
533+
// If we've been passed a set of filter names, check to see if this set is contained within that set before
534+
// adding it to the restore to ensure that we don't impact other unrelated sets
535+
if setIncludeNames != nil {
536+
origName := setName
537+
if set.Parent.isIpv6 {
538+
origName = strings.Replace(setName, fmt.Sprintf("%s:", IPv6SetPrefix), "", 1)
539+
}
540+
if !slices.Contains(setIncludeNames, origName) {
541+
continue
542+
}
543+
}
520544
// we need setNames in some consistent order so that we can unit-test this method has a predictable output:
521545
setNames = append(setNames, setName)
522546
}
@@ -591,7 +615,14 @@ func (ipset *IPSet) Save() error {
591615
// mode except list, help, version, interactive mode and restore itself.
592616
// Send formatted ipset.sets into stdin of "ipset restore" command.
593617
func (ipset *IPSet) Restore() error {
594-
restoreString := buildIPSetRestore(ipset)
618+
return ipset.RestoreSets(nil)
619+
}
620+
621+
// RestoreSets is very similar to Restore, except that it filters by set names that are passed in a string array
622+
// so that we don't disrumpt other things that might be using ipsets on the host. In general, this function should be
623+
// preferred over the Restore() function.
624+
func (ipset *IPSet) RestoreSets(setNames []string) error {
625+
restoreString := buildIPSetRestore(ipset, setNames)
595626
klog.V(3).Infof("ipset (ipv6? %t) restore looks like:\n%s", ipset.isIpv6, restoreString)
596627
stdin := bytes.NewBufferString(restoreString)
597628
err := ipset.runWithStdin(stdin, "restore", "-exist")

0 commit comments

Comments
 (0)