Ipsetmanager-update (#1034)

* add new npm errors

* add logic for adding/removing sets to kernel in ipsetmanager, update usage of prometheus metrics, and update dataplane API to not use IPSets

* update to a pointer return value for NewPolicyManager

* fix go lints

* renamed count to kernelReferCount

* fix a bug with kernel logic, rearrange code, rename things, and update comments

* rearrange functions

* removed checkIfExists, consolidated AddReference and DeleteReference with a special type, and fixed go lints

* moved logic for different reference types to ipset.go

* remove file with just notes on it

* remove redundant boolean calculation that is always true

* add clarifying comment

* fix ipset  metrics to be for all of NPM (not necessarily in kernel), and write TODOs for kernel-based metrics

* update based on code review

* var name change

* update unit test to use DeleteIPSet

* moved internal functions to the bottom

Author: huntergregory

npm/pkg/dataplane/dataplane.go

@@ -9,8 +9,8 @@ import (
 )
 
 type DataPlane struct {
-	policyMgr policies.PolicyManager
-	ipsetMgr  ipsets.IPSetManager
+	policyMgr *policies.PolicyManager
+	ipsetMgr  *ipsets.IPSetManager
 	networkID string
 	// key is PodKey
 	endpointCache map[string]*NPMEndpoint
@@ -44,8 +44,8 @@ func (dp *DataPlane) ResetDataPlane() error {
 }
 
 // CreateIPSet takes in a set object and updates local cache with this set
-func (dp *DataPlane) CreateIPSet(set *ipsets.IPSet) error {
-	err := dp.ipsetMgr.CreateIPSet(set)
+func (dp *DataPlane) CreateIPSet(setName string, setType ipsets.SetType) error {
+	err := dp.ipsetMgr.CreateIPSet(setName, setType)
 	if err != nil {
 		return fmt.Errorf("[DataPlane] error while creating set: %w", err)
 	}
@@ -54,26 +54,17 @@ func (dp *DataPlane) CreateIPSet(set *ipsets.IPSet) error {
 
 // DeleteSet checks for members and references of the given "set" type ipset
 // if not used then will delete it from cache
-func (dp *DataPlane) DeleteSet(name string) error {
-	err := dp.ipsetMgr.DeleteSet(name)
+func (dp *DataPlane) DeleteIPSet(name string) error {
+	err := dp.ipsetMgr.DeleteIPSet(name)
 	if err != nil {
 		return fmt.Errorf("[DataPlane] error while deleting set: %w", err)
 	}
 	return nil
 }
 
-// DeleteList sanity checks and deletes a list ipset
-func (dp *DataPlane) DeleteList(name string) error {
-	err := dp.ipsetMgr.DeleteList(name)
-	if err != nil {
-		return fmt.Errorf("[DataPlane] error while deleting list: %w", err)
-	}
-	return nil
-}
-
-// AddToSet takes in a list of IPset objects along with IP member
+// AddToSet takes in a list of IPSet names along with IP member
 // and then updates it local cache
-func (dp *DataPlane) AddToSet(setNames []*ipsets.IPSet, ip, podKey string) error {
+func (dp *DataPlane) AddToSet(setNames []string, ip, podKey string) error {
 	err := dp.ipsetMgr.AddToSet(setNames, ip, podKey)
 	if err != nil {
 		return fmt.Errorf("[DataPlane] error while adding to set: %w", err)

npm/pkg/dataplane/dataplane_test.go

@@ -14,9 +14,8 @@ func TestNewDataPlane(t *testing.T) {
 	if dp == nil {
 		t.Error("NewDataPlane() returned nil")
 	}
-	set := ipsets.NewIPSet("test", ipsets.NameSpace)
 
-	err := dp.CreateIPSet(set)
+	err := dp.CreateIPSet("test", ipsets.NameSpace)
 	if err != nil {
 		t.Error("CreateIPSet() returned error")
 	}

npm/pkg/dataplane/ipsets/ipset.go

@@ -3,6 +3,7 @@ package ipsets
 import (
 	"errors"
 
+	"github.com/Azure/azure-container-networking/log"
 	"github.com/Azure/azure-container-networking/npm/util"
 )
 
@@ -24,9 +25,10 @@ type IPSet struct {
 	// NetPolReference holds networkpolicy names where this IPSet
 	// is being referred as part of rules
 	NetPolReference map[string]struct{}
-	// IpsetReferCount keeps count of 2nd level Nested IPSets
-	// with member as this IPSet
-	IpsetReferCount int
+	// ipsetReferCount keeps track of how many lists in the cache refer to this ipset
+	ipsetReferCount int
+	// kernelReferCount keeps track of how many lists in the kernel refer to this ipset
+	kernelReferCount int
 }
 
 type SetProperties struct {
@@ -75,7 +77,7 @@ var (
 		CIDRBlocks:               "CIDRBlocks",
 	}
 	// ErrIPSetInvalidKind is returned when IPSet kind is invalid
-	ErrIPSetInvalidKind = errors.New("Invalid IPSet Kind")
+	ErrIPSetInvalidKind = errors.New("invalid IPSet Kind")
 )
 
 func (x SetType) String() string {
@@ -91,6 +93,15 @@ const (
 	HashSet SetKind = "set"
 )
 
+// ReferenceType specifies the kind of reference for an IPSet
+type ReferenceType string
+
+// Possible ReferenceTypes
+const (
+	SelectorType ReferenceType = "Selector"
+	NetPolType   ReferenceType = "NetPol"
+)
+
 func NewIPSet(name string, setType SetType) *IPSet {
 	set := &IPSet{
 		Name:       name,
@@ -104,8 +115,9 @@ func NewIPSet(name string, setType SetType) *IPSet {
 		SelectorReference: make(map[string]struct{}),
 		// Map with Key as Network Policy name to to emulate set
 		// and value as struct{} for minimal memory consumption
-		NetPolReference: make(map[string]struct{}),
-		IpsetReferCount: 0,
+		NetPolReference:  make(map[string]struct{}),
+		ipsetReferCount:  0,
+		kernelReferCount: 0,
 	}
 	if set.Kind == HashSet {
 		set.IPPodKey = make(map[string]string)
@@ -163,47 +175,77 @@ func getSetKind(setType SetType) SetKind {
 	}
 }
 
-func (set *IPSet) AddMemberIPSet(memberIPSet *IPSet) {
-	set.MemberIPSets[memberIPSet.Name] = memberIPSet
-}
-
-func (set *IPSet) IncIpsetReferCount() {
-	set.IpsetReferCount++
+func (set *IPSet) incIPSetReferCount() {
+	set.ipsetReferCount++
 }
 
-func (set *IPSet) DecIpsetReferCount() {
-	if set.IpsetReferCount == 0 {
+func (set *IPSet) decIPSetReferCount() {
+	if set.ipsetReferCount == 0 {
 		return
 	}
-	set.IpsetReferCount--
+	set.ipsetReferCount--
+}
+
+func (set *IPSet) incKernelReferCount() {
+	set.kernelReferCount++
 }
 
-func (set *IPSet) AddSelectorReference(netPolName string) {
-	set.SelectorReference[netPolName] = struct{}{}
+func (set *IPSet) decKernelReferCount() {
+	if set.kernelReferCount == 0 {
+		return
+	}
+	set.kernelReferCount--
 }
 
-func (set *IPSet) DeleteSelectorReference(netPolName string) {
-	delete(set.SelectorReference, netPolName)
+func (set *IPSet) addReference(referenceName string, referenceType ReferenceType) {
+	switch referenceType {
+	case SelectorType:
+		set.SelectorReference[referenceName] = struct{}{}
+	case NetPolType:
+		set.NetPolReference[referenceName] = struct{}{}
+	default:
+		log.Logf("IPSet_addReference: encountered unknown ReferenceType")
+	}
 }
 
-func (set *IPSet) AddNetPolReference(netPolName string) {
-	set.NetPolReference[netPolName] = struct{}{}
+func (set *IPSet) deleteReference(referenceName string, referenceType ReferenceType) {
+	switch referenceType {
+	case SelectorType:
+		delete(set.SelectorReference, referenceName)
+	case NetPolType:
+		delete(set.NetPolReference, referenceName)
+	default:
+		log.Logf("IPSet_deleteReference: encountered unknown ReferenceType")
+	}
 }
 
-func (set *IPSet) DeleteNetPolReference(netPolName string) {
-	delete(set.NetPolReference, netPolName)
+func (set *IPSet) shouldBeInKernel() bool {
+	return set.usedByNetPol() || set.referencedInKernel()
 }
 
-func (set *IPSet) CanBeDeleted() bool {
-	return len(set.SelectorReference) == 0 &&
-		len(set.NetPolReference) == 0 &&
-		set.IpsetReferCount == 0 &&
+func (set *IPSet) canBeDeleted() bool {
+	return !set.usedByNetPol() &&
+		!set.referencedInList() &&
 		len(set.MemberIPSets) == 0 &&
 		len(set.IPPodKey) == 0
 }
 
-// UsedByNetPol check if an IPSet is referred in network policies.
-func (set *IPSet) UsedByNetPol() bool {
+// usedByNetPol check if an IPSet is referred in network policies.
+func (set *IPSet) usedByNetPol() bool {
 	return len(set.SelectorReference) > 0 &&
 		len(set.NetPolReference) > 0
 }
+
+func (set *IPSet) referencedInList() bool {
+	return set.ipsetReferCount > 0
+}
+
+func (set *IPSet) referencedInKernel() bool {
+	return set.kernelReferCount > 0
+}
+
+// panics if set is not a list set
+func (set *IPSet) hasMember(memberName string) bool {
+	_, isMember := set.MemberIPSets[memberName]
+	return isMember
+}