From 4c6e19f2e1167500df9722c7da1c7986e262d909 Mon Sep 17 00:00:00 2001 From: Aaron U'Ren Date: Thu, 14 Sep 2023 17:11:22 -0500 Subject: [PATCH] feat(ipset): consolidate ipset usage across controllers Before this, we had 2 different ways to interact with ipsets, through the handler interface which had the best handling for IPv6 because NPC heavily utilizes it, and through the ipset struct which mostly repeated the handler logic, but didn't handle some key things. NPC utilized the handler functions and NSC / NRC mostly utilized the old ipset struct functions. This caused a lot of duplication between the two groups of functions and also caused issues with proper IPv6 handling. This commit consolidates the two sets of usage into just the handler interface. This greatly simplifies how the controllers interact with ipsets and it also reduces the logic complexity on the ipset side. This also fixes up some inconsistency with how we handled IPv6 ipset names. ipset likes them to be prefixed with inet6:, but we weren't always doing this in a way that made sense and was consistent across all functions in the ipset struct. --- .../netpol/network_policy_controller_test.go | 4 + .../proxy/network_services_controller.go | 62 ++---- .../routing/network_routes_controller.go | 54 ++--- pkg/utils/ipset.go | 206 +++++------------- 4 files changed, 94 insertions(+), 232 deletions(-) diff --git a/pkg/controllers/netpol/network_policy_controller_test.go b/pkg/controllers/netpol/network_policy_controller_test.go index ac9b5884a..fd3ef84ec 100644 --- a/pkg/controllers/netpol/network_policy_controller_test.go +++ b/pkg/controllers/netpol/network_policy_controller_test.go @@ -749,6 +749,10 @@ func (ips *fakeIPSet) Sets() map[string]*utils.Set { return nil } +func (ips *fakeIPSet) Name(name string) string { + return name +} + func TestNetworkPolicyController(t *testing.T) { testCases := []tNetPolConfigTestCase{ { diff --git a/pkg/controllers/proxy/network_services_controller.go b/pkg/controllers/proxy/network_services_controller.go index 1395ab80a..fa05dd9b1 100644 --- a/pkg/controllers/proxy/network_services_controller.go +++ b/pkg/controllers/proxy/network_services_controller.go @@ -71,8 +71,9 @@ const ( svcSchedFlagsAnnotation = "kube-router.io/service.schedflags" // All IPSET names need to be less than 31 characters in order for the Kernel to accept them. Keep in mind that the - // actual formulation for this may be inet6: depending on ip family so that means that these base names - // actually need to be less than 25 characters + // actual formulation for this may be inet6: depending on ip family, plus when we change ipsets we use + // a swap operation that adds a hyphen to the end, so that means that these base names actually need to be less than + // 24 characters localIPsIPSetName = "kube-router-local-ips" serviceIPPortsSetName = "kube-router-svip-prt" serviceIPsIPSetName = "kube-router-svip" @@ -120,11 +121,6 @@ type NetworkServicesController struct { ipsetMutex *sync.Mutex fwMarkMap map[uint32]string - // Map of ipsets that we use. - localIPsIPSets map[v1.IPFamily]*utils.Set - serviceIPPortsIPSet map[v1.IPFamily]*utils.Set - serviceIPsIPSet map[v1.IPFamily]*utils.Set - svcLister cache.Indexer epSliceLister cache.Indexer podLister cache.Indexer @@ -399,34 +395,27 @@ func (nsc *NetworkServicesController) setupIpvsFirewall() error { - create ipsets - create firewall rules */ - var err error - var ipset *utils.Set - // Remember ipsets for use in syncIpvsFirewall - nsc.localIPsIPSets = make(map[v1.IPFamily]*utils.Set) - nsc.serviceIPPortsIPSet = make(map[v1.IPFamily]*utils.Set) - nsc.serviceIPsIPSet = make(map[v1.IPFamily]*utils.Set) - for family, ipSetHandler := range nsc.ipSetHandlers { + // Initialize some blank ipsets with the correct names in order to use them in the iptables below. We don't need + // to retain references to them, because we'll use the handler to refresh them later in syncIpvsFirewall + for _, ipSetHandler := range nsc.ipSetHandlers { // Create ipset for local addresses. - ipset, err = ipSetHandler.Create(localIPsIPSetName, utils.TypeHashIP, utils.OptionTimeout, "0") + _, err = ipSetHandler.Create(localIPsIPSetName, utils.TypeHashIP, utils.OptionTimeout, "0") if err != nil { return fmt.Errorf("failed to create ipset: %s - %v", localIPsIPSetName, err) } - nsc.localIPsIPSets[family] = ipset // Create 2 ipsets for services. One for 'ip' and one for 'ip,port' - ipset, err = ipSetHandler.Create(serviceIPsIPSetName, utils.TypeHashIP, utils.OptionTimeout, "0") + _, err = ipSetHandler.Create(serviceIPsIPSetName, utils.TypeHashIP, utils.OptionTimeout, "0") if err != nil { return fmt.Errorf("failed to create ipset: %s - %v", serviceIPsIPSetName, err) } - nsc.serviceIPsIPSet[family] = ipset - ipset, err = ipSetHandler.Create(serviceIPPortsSetName, utils.TypeHashIPPort, utils.OptionTimeout, "0") + _, err = ipSetHandler.Create(serviceIPPortsSetName, utils.TypeHashIPPort, utils.OptionTimeout, "0") if err != nil { return fmt.Errorf("failed to create ipset: %s - %v", serviceIPPortsSetName, err) } - nsc.serviceIPPortsIPSet[family] = ipset } // Setup a custom iptables chain to explicitly allow input traffic to ipvs services only. @@ -612,16 +601,13 @@ func (nsc *NetworkServicesController) syncIpvsFirewall() error { for family, addrs := range addrsMap { // Convert addrs from a slice of net.IP to a slice of string - localIPsSets := make([]string, 0, len(addrs)) + localIPsSets := make([][]string, 0, len(addrs)) for _, addr := range addrs { - localIPsSets = append(localIPsSets, addr.String()) + localIPsSets = append(localIPsSets, []string{addr.String(), utils.OptionTimeout, "0"}) } // Refresh the family specific IPSet with the slice of strings - err = nsc.localIPsIPSets[family].Refresh(localIPsSets) - if err != nil { - return fmt.Errorf("failed to sync ipset: %s", err.Error()) - } + nsc.ipSetHandlers[family].RefreshSet(localIPsIPSetName, localIPsSets, utils.TypeHashIP) } // Populate service ipsets. @@ -630,8 +616,8 @@ func (nsc *NetworkServicesController) syncIpvsFirewall() error { return errors.New("Failed to list IPVS services: " + err.Error()) } - serviceIPsSets := make(map[v1.IPFamily][]string) - serviceIPPortsIPSets := make(map[v1.IPFamily][]string) + serviceIPsSets := make(map[v1.IPFamily][][]string) + serviceIPPortsIPSets := make(map[v1.IPFamily][][]string) for _, ipvsService := range ipvsServices { var address net.IP @@ -667,24 +653,22 @@ func (nsc *NetworkServicesController) syncIpvsFirewall() error { family = v1.IPv6Protocol } - serviceIPsSets[family] = append(serviceIPsSets[family], address.String()) + serviceIPsSets[family] = append(serviceIPsSets[family], []string{address.String(), utils.OptionTimeout, "0"}) ipvsAddressWithPort := fmt.Sprintf("%s,%s:%d", address, protocol, port) - serviceIPPortsIPSets[family] = append(serviceIPPortsIPSets[family], ipvsAddressWithPort) + serviceIPPortsIPSets[family] = append(serviceIPPortsIPSets[family], + []string{ipvsAddressWithPort, utils.OptionTimeout, "0"}) } - for family := range nsc.ipSetHandlers { - serviceIPsIPSet := nsc.serviceIPsIPSet[family] - err = serviceIPsIPSet.Refresh(serviceIPsSets[family]) - if err != nil { - return fmt.Errorf("failed to sync ipset: %v", err) - } + for family, setHandler := range nsc.ipSetHandlers { + setHandler.RefreshSet(serviceIPsIPSetName, serviceIPsSets[family], utils.TypeHashIP) + + setHandler.RefreshSet(serviceIPPortsSetName, serviceIPPortsIPSets[family], utils.TypeHashIPPort) - serviceIPPortsIPSet := nsc.serviceIPPortsIPSet[family] - err = serviceIPPortsIPSet.Refresh(serviceIPPortsIPSets[family]) + err := setHandler.Restore() if err != nil { - return fmt.Errorf("failed to sync ipset: %v", err) + return fmt.Errorf("could not save ipset for service firewall: %v", err) } } diff --git a/pkg/controllers/routing/network_routes_controller.go b/pkg/controllers/routing/network_routes_controller.go index 38fbbd881..4f14d5564 100644 --- a/pkg/controllers/routing/network_routes_controller.go +++ b/pkg/controllers/routing/network_routes_controller.go @@ -963,8 +963,8 @@ func (nrc *NetworkRoutingController) syncNodeIPSets() error { nodes := nrc.nodeLister.List() // Collect active PodCIDR(s) and NodeIPs from nodes - currentPodCidrs := make(map[v1core.IPFamily][]string) - currentNodeIPs := make(map[v1core.IPFamily][]string) + currentPodCidrs := make(map[v1core.IPFamily][][]string) + currentNodeIPs := make(map[v1core.IPFamily][][]string) for _, obj := range nodes { node := obj.(*v1core.Node) podCIDRs := getPodCIDRsFromAllNodeSources(node) @@ -978,13 +978,15 @@ func (nrc *NetworkRoutingController) syncNodeIPSets() error { klog.Warningf("Wasn't able to parse pod CIDR %s for node %s, skipping", cidr, node.Name) } if ip.To4() != nil { - currentPodCidrs[v1core.IPv4Protocol] = append(currentPodCidrs[v1core.IPv4Protocol], cidr) + currentPodCidrs[v1core.IPv4Protocol] = append(currentPodCidrs[v1core.IPv4Protocol], + []string{cidr, utils.OptionTimeout, "0"}) } else { - currentPodCidrs[v1core.IPv6Protocol] = append(currentPodCidrs[v1core.IPv6Protocol], cidr) + currentPodCidrs[v1core.IPv6Protocol] = append(currentPodCidrs[v1core.IPv6Protocol], + []string{cidr, utils.OptionTimeout, "0"}) } } - var ipv4Addrs, ipv6Addrs []string + var ipv4Addrs, ipv6Addrs [][]string intExtNodeIPsv4, intExtNodeIPsv6 := utils.GetAllNodeIPs(node) if err != nil { klog.Errorf("Failed to find a node IP, cannot add to node ipset which could affect routing: %v", err) @@ -992,12 +994,12 @@ func (nrc *NetworkRoutingController) syncNodeIPSets() error { } for _, nodeIPv4s := range intExtNodeIPsv4 { for _, nodeIPv4 := range nodeIPv4s { - ipv4Addrs = append(ipv4Addrs, nodeIPv4.String()) + ipv4Addrs = append(ipv4Addrs, []string{nodeIPv4.String(), utils.OptionTimeout, "0"}) } } for _, nodeIPv6s := range intExtNodeIPsv6 { for _, nodeIPv6 := range nodeIPv6s { - ipv6Addrs = append(ipv6Addrs, nodeIPv6.String()) + ipv6Addrs = append(ipv6Addrs, []string{nodeIPv6.String(), utils.OptionTimeout, "0"}) } } currentNodeIPs[v1core.IPv4Protocol] = append(currentNodeIPs[v1core.IPv4Protocol], ipv4Addrs...) @@ -1006,41 +1008,13 @@ func (nrc *NetworkRoutingController) syncNodeIPSets() error { // Syncing Pod subnet ipset entries for family, ipSetHandler := range nrc.ipSetHandlers { - psSet := ipSetHandler.Get(podSubnetsIPSetName) - if psSet == nil { - klog.Infof("Creating missing ipset \"%s\"", podSubnetsIPSetName) - _, err = ipSetHandler.Create(podSubnetsIPSetName, utils.OptionTimeout, "0") - if err != nil { - return fmt.Errorf("ipset \"%s\" not found in controller instance", - podSubnetsIPSetName) - } - psSet = ipSetHandler.Get(podSubnetsIPSetName) - if nil == psSet { - return fmt.Errorf("failed to get ipsethandler for ipset \"%s\"", podSubnetsIPSetName) - } - } - err = psSet.Refresh(currentPodCidrs[family]) - if err != nil { - return fmt.Errorf("failed to sync Pod Subnets ipset: %s", err) - } + ipSetHandler.RefreshSet(podSubnetsIPSetName, currentPodCidrs[family], utils.TypeHashNet) - // Syncing Node Addresses ipset entries - naSet := ipSetHandler.Get(nodeAddrsIPSetName) - if naSet == nil { - klog.Infof("Creating missing ipset \"%s\"", nodeAddrsIPSetName) - _, err = ipSetHandler.Create(nodeAddrsIPSetName, utils.OptionTimeout, "0") - if err != nil { - return fmt.Errorf("ipset \"%s\" not found in controller instance", - nodeAddrsIPSetName) - } - naSet = ipSetHandler.Get(nodeAddrsIPSetName) - if nil == naSet { - return fmt.Errorf("failed to get ipsethandler for ipset \"%s\"", nodeAddrsIPSetName) - } - } - err = naSet.Refresh(currentNodeIPs[family]) + ipSetHandler.RefreshSet(nodeAddrsIPSetName, currentNodeIPs[family], utils.TypeHashIP) + + err = ipSetHandler.Restore() if err != nil { - return fmt.Errorf("failed to sync Node Addresses ipset: %s", err) + return fmt.Errorf("failed to sync pod subnets / node addresses ipsets: %v", err) } } return nil diff --git a/pkg/utils/ipset.go b/pkg/utils/ipset.go index 462bc0552..0a00ef68a 100644 --- a/pkg/utils/ipset.go +++ b/pkg/utils/ipset.go @@ -150,6 +150,9 @@ const ( // tmpIPSetPrefix Is the prefix added to temporary ipset names used in the atomic swap operations during ipset // restore. You should never see these on your system because they only exist during the restore. tmpIPSetPrefix = "TMP-" + + // IPv6SetPrefix is the prefix that ipset requires on IPv6 ipsets in order to distinguish them from IPv4 sets + IPv6SetPrefix = "inet6" ) type IPSetHandler interface { @@ -163,6 +166,7 @@ type IPSetHandler interface { Flush() error Get(setName string) *Set Sets() map[string]*Set + Name(ipSet string) string } // IPSet represent ipset sets managed by. @@ -253,17 +257,35 @@ func NewIPSet(isIpv6 bool) (*IPSet, error) { // require type specific options. Does not create set on the system if it // already exists by the same name. func (ipset *IPSet) Create(setName string, createOptions ...string) (*Set, error) { + ipsetName := ipset.Name(setName) // Populate Set map if needed - if ipset.Get(setName) == nil { - ipset.sets[setName] = &Set{ - Name: setName, + if ipset.Get(ipsetName) == nil { + // check that caller didn't leave out ipv6 options, fill it in for them if they did + ipv6OptionFound := false + if ipset.isIpv6 { + for idx, option := range createOptions { + if option == "family" { + if createOptions[idx+1] != "inet6" { + return nil, fmt.Errorf("family option passed with a parameter that was not inet6 to an IPv6 "+ + "only ipset handler, detected family type passed: %s", createOptions[idx+1]) + } + ipv6OptionFound = true + break + } + } + if !ipv6OptionFound { + createOptions = append(createOptions, "family", "inet6") + } + } + ipset.sets[ipsetName] = &Set{ + Name: ipsetName, Options: createOptions, Parent: ipset, } } // Determine if set with the same name is already active on the system - setIsActive, err := ipset.sets[setName].IsActive() + setIsActive, err := ipset.sets[ipsetName].IsActive() if err != nil { return nil, fmt.Errorf("failed to determine if ipset set %s exists: %s", setName, err) @@ -271,28 +293,20 @@ func (ipset *IPSet) Create(setName string, createOptions ...string) (*Set, error // Create set if missing from the system if !setIsActive { - if ipset.isIpv6 { - // Add "family inet6" option and a "inet6:" prefix for IPv6 sets. - args := []string{"create", "-exist", ipset.sets[setName].name()} - args = append(args, createOptions...) - args = append(args, "family", "inet6") - if _, err := ipset.run(args...); err != nil { - return nil, fmt.Errorf("failed to create ipset set on system: %s", err) - } - } else { - _, err := ipset.run(append([]string{"create", "-exist", setName}, - createOptions...)...) - if err != nil { - return nil, fmt.Errorf("failed to create ipset set on system: %s", err) - } + args := []string{"create", "-exist", ipset.sets[ipsetName].name()} + args = append(args, createOptions...) + klog.V(2).Infof("running ipset command: %s", args) + _, err := ipset.run(args...) + if err != nil { + return nil, fmt.Errorf("failed to create ipset set on system: %s", err) } } - return ipset.sets[setName], nil + return ipset.sets[ipsetName], nil } // Add a given Set to an IPSet func (ipset *IPSet) Add(set *Set) error { - _, err := ipset.Create(set.Name, set.Options...) + _, err := ipset.Create(set.name(), set.Options...) if err != nil { return err } @@ -302,7 +316,7 @@ func (ipset *IPSet) Add(set *Set) error { options[index] = entry.Options } - err = ipset.Get(set.Name).BatchAdd(options) + err = ipset.Get(set.name()).BatchAdd(options) if err != nil { return err } @@ -312,39 +326,23 @@ func (ipset *IPSet) Add(set *Set) error { // RefreshSet add/update internal Sets with a Set of entries but does not run restore command func (ipset *IPSet) RefreshSet(setName string, entriesWithOptions [][]string, setType string) { - if ipset.Get(setName) == nil { + ipsetName := ipset.Name(setName) + if ipset.Get(ipsetName) == nil { options := []string{setType, OptionTimeout, "0"} if ipset.isIpv6 { options = append(options, "family", "inet6") } - ipset.sets[setName] = &Set{ - Name: setName, + ipset.sets[ipsetName] = &Set{ + Name: ipsetName, Options: options, Parent: ipset, } } entries := make([]*Entry, len(entriesWithOptions)) for i, entry := range entriesWithOptions { - entries[i] = &Entry{Set: ipset.sets[setName], Options: entry} - } - ipset.Get(setName).Entries = entries -} - -// Add a given entry to the set. If the -exist option is specified, ipset -// ignores if the entry already added to the set. -// Note: if you need to add multiple entries (e.g., in a loop), use BatchAdd instead, -// as it’s much more performant. -func (set *Set) Add(addOptions ...string) (*Entry, error) { - entry := &Entry{ - Set: set, - Options: addOptions, - } - set.Entries = append(set.Entries, entry) - _, err := set.Parent.run(append([]string{"add", "-exist", entry.Set.name()}, addOptions...)...) - if err != nil { - return nil, err + entries[i] = &Entry{Set: ipset.sets[ipsetName], Options: entry} } - return entry, nil + ipset.Get(ipsetName).Entries = entries } // BatchAdd given entries (with their options) to the set. @@ -376,30 +374,6 @@ func (set *Set) BatchAdd(addOptions [][]string) error { return nil } -// Del an entry from a set. If the -exist option is specified and the entry is -// not in the set (maybe already expired), then the command is ignored. -func (entry *Entry) Del() error { - _, err := entry.Set.Parent.run(append([]string{"del", entry.Set.name()}, entry.Options...)...) - if err != nil { - return err - } - err = entry.Set.Parent.Save() - if err != nil { - return err - } - return nil -} - -// Test whether an entry is in a set or not. Exit status number is zero if the -// tested entry is in the set and nonzero if it is missing from the set. -func (set *Set) Test(testOptions ...string) (bool, error) { - _, err := set.Parent.run(append([]string{"test", set.name()}, testOptions...)...) - if err != nil { - return false, err - } - return true, nil -} - // Destroy the specified set or all the sets if none is given. If the set has // got reference(s), nothing is done and no set destroyed. func (set *Set) Destroy() error { @@ -416,7 +390,7 @@ func (set *Set) Destroy() error { // is done and no set destroyed. If the IPSet does not contain the named set // then Destroy is a no-op. func (ipset *IPSet) Destroy(setName string) error { - set := ipset.Get(setName) + set := ipset.Get(ipset.Name(setName)) if set == nil { return nil } @@ -453,11 +427,15 @@ func (set *Set) IsActive() (bool, error) { return true, nil } -func (set *Set) name() string { - if set.Parent.isIpv6 { - return "inet6:" + set.Name +func (ipset *IPSet) Name(setName string) string { + if ipset.isIpv6 && !strings.HasPrefix(setName, IPv6SetPrefix+":") { + return fmt.Sprintf("%s:%s", IPv6SetPrefix, setName) } - return set.Name + return setName +} + +func (set *Set) name() string { + return set.Parent.Name(set.Name) } // Parse ipset save stdout. @@ -582,7 +560,8 @@ func (ipset *IPSet) Save() error { // mode except list, help, version, interactive mode and restore itself. // Send formatted ipset.sets into stdin of "ipset restore" command. func (ipset *IPSet) Restore() error { - stdin := bytes.NewBufferString(buildIPSetRestore(ipset)) + restoreString := buildIPSetRestore(ipset) + stdin := bytes.NewBufferString(restoreString) err := ipset.runWithStdin(stdin, "restore", "-exist") if err != nil { return err @@ -590,16 +569,7 @@ func (ipset *IPSet) Restore() error { return nil } -// Flush all entries from the specified set or flush all sets if none is given. -func (set *Set) Flush() error { - _, err := set.Parent.run("flush", set.Name) - if err != nil { - return err - } - return nil -} - -// Flush all entries from the specified set or flush all sets if none is given. +// Flush all entries from all sets func (ipset *IPSet) Flush() error { _, err := ipset.run("flush") if err != nil { @@ -622,73 +592,3 @@ func (ipset *IPSet) Get(setName string) *Set { func (ipset *IPSet) Sets() map[string]*Set { return ipset.sets } - -// Rename a set. Set identified by SETNAME-TO must not exist. -func (set *Set) Rename(newName string) error { - if set.Parent.isIpv6 { - newName = "ipv6:" + newName - } - _, err := set.Parent.run("rename", set.name(), newName) - if err != nil { - return err - } - return nil -} - -// Swap the content of two sets, or in another words, exchange the name of two -// sets. The referred sets must exist and compatible type of sets can be -// swapped only. -func (set *Set) Swap(setTo *Set) error { - _, err := set.Parent.run("swap", set.name(), setTo.name()) - if err != nil { - return err - } - return nil -} - -// Refresh a Set with new entries. -func (set *Set) Refresh(entries []string, extraOptions ...string) error { - entriesWithOptions := make([][]string, len(entries)) - - for index, entry := range entries { - entriesWithOptions[index] = append([]string{entry}, extraOptions...) - } - - return set.RefreshWithBuiltinOptions(entriesWithOptions) -} - -// RefreshWithBuiltinOptions refresh a Set with new entries with built-in options. -func (set *Set) RefreshWithBuiltinOptions(entries [][]string) error { - var err error - - // The set-name must be < 32 characters! - tempName := set.Name + "-" - - newSet := &Set{ - Parent: set.Parent, - Name: tempName, - Options: set.Options, - } - - err = set.Parent.Add(newSet) - if err != nil { - return err - } - - err = newSet.BatchAdd(entries) - if err != nil { - return err - } - - err = set.Swap(newSet) - if err != nil { - return err - } - - err = set.Parent.Destroy(tempName) - if err != nil { - return err - } - - return nil -}