fix: [NPM-WIN] add an empty set to lists when theyre created (#1431)

Author: huntergregory

npm/pkg/dataplane/dataplane.go

@@ -42,6 +42,10 @@ type DataPlane struct {
 
 func NewDataPlane(nodeName string, ioShim *common.IOShim, cfg *Config, stopChannel <-chan struct{}) (*DataPlane, error) {
 	metrics.InitializeAll()
+	if util.IsWindowsDP() {
+		klog.Infof("[DataPlane] enabling AddEmptySetToLists for Windows")
+		cfg.IPSetManagerCfg.AddEmptySetToLists = true
+	}
 	dp := &DataPlane{
 		Config:          cfg,
 		policyMgr:       policies.NewPolicyManager(ioShim, cfg.PolicyManagerCfg),

npm/pkg/dataplane/ipsets/ipset.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/Azure/azure-container-networking/log"
 	"github.com/Azure/azure-container-networking/npm/util"
+	"k8s.io/klog"
 )
 
 type IPSetMetadata struct {
@@ -60,9 +61,13 @@ func (setMetadata *IPSetMetadata) GetPrefixName() string {
 		return fmt.Sprintf("%s%s", util.NamespaceLabelPrefix, setMetadata.Name)
 	case NestedLabelOfPod:
 		return fmt.Sprintf("%s%s", util.NestedLabelPrefix, setMetadata.Name)
+	case EmptyHashSet:
+		return fmt.Sprintf("%s%s", util.EmptySetPrefix, setMetadata.Name)
 	case UnknownType: // adding this to appease golint
+		klog.Errorf("experienced unknown type in set metadata: %+v", setMetadata)
 		return Unknown
 	default:
+		klog.Errorf("experienced unexpected type %d in set metadata: %+v", setMetadata.Type, setMetadata)
 		return Unknown
 	}
 }
@@ -83,6 +88,8 @@ func (setType SetType) getSetKind() SetKind {
 		return HashSet
 	case KeyValueLabelOfPod:
 		return HashSet
+	case EmptyHashSet:
+		return HashSet
 	case KeyLabelOfNamespace:
 		return ListSet
 	case KeyValueLabelOfNamespace:
@@ -132,6 +139,7 @@ type SetProperties struct {
 
 type SetType int8
 
+// Possble values for SetType
 const (
 	// Unknown SetType
 	UnknownType SetType = 0
@@ -154,6 +162,9 @@ const (
 	NestedLabelOfPod SetType = 7
 	// CIDRBlocks holds CIDR blocks
 	CIDRBlocks SetType = 8
+	// EmptyHashSet is a set meant to have no members
+	EmptyHashSet SetType = 9
+
 	// Unknown const for unknown string
 	Unknown string = "unknown"
 )
@@ -162,13 +173,14 @@ var (
 	setTypeName = map[SetType]string{
 		UnknownType:              Unknown,
 		Namespace:                "Namespace",
-		KeyLabelOfNamespace:      "KeyLabelOfNameSpace",
-		KeyValueLabelOfNamespace: "KeyValueLabelOfNameSpace",
+		KeyLabelOfNamespace:      "KeyLabelOfNamespace",
+		KeyValueLabelOfNamespace: "KeyValueLabelOfNamespace",
 		KeyLabelOfPod:            "KeyLabelOfPod",
 		KeyValueLabelOfPod:       "KeyValueLabelOfPod",
 		NamedPorts:               "NamedPorts",
 		NestedLabelOfPod:         "NestedLabelOfPod",
 		CIDRBlocks:               "CIDRBlocks",
+		EmptyHashSet:             "EmptySet",
 	}
 	// ErrIPSetInvalidKind is returned when IPSet kind is invalid
 	ErrIPSetInvalidKind = errors.New("invalid IPSet Kind")
@@ -342,11 +354,18 @@ func (set *IPSet) canBeForceDeleted() bool {
 		!set.referencedInList()
 }
 
-func (set *IPSet) canBeDeleted() bool {
+func (set *IPSet) canBeDeleted(ignorableMember *IPSet) bool {
+	var firstMember *IPSet
+	for _, member := range set.MemberIPSets {
+		firstMember = member
+		break
+	}
+	listMembersOk := len(set.MemberIPSets) == 0 || (len(set.MemberIPSets) == 1 && firstMember == ignorableMember)
+
 	return !set.usedByNetPol() &&
 		!set.referencedInList() &&
-		len(set.MemberIPSets) == 0 &&
-		len(set.IPPodKey) == 0
+		len(set.IPPodKey) == 0 &&
+		listMembersOk
 }
 
 // usedByNetPol check if an IPSet is referred in network policies.

npm/pkg/dataplane/ipsets/ipset_test.go

@@ -7,8 +7,12 @@ import (
 )
 
 func TestShouldBeInKernelAndCanDelete(t *testing.T) {
+	ignorableSetMetadata := &IPSetMetadata{"ignorableSet", EmptyHashSet}
+	ignorableSet := NewIPSet(ignorableSetMetadata)
+
 	s := &IPSetMetadata{"test-set", Namespace}
 	l := &IPSetMetadata{"test-list", KeyLabelOfNamespace}
+
 	tests := []struct {
 		name          string
 		set           *IPSet
@@ -87,6 +91,21 @@ func TestShouldBeInKernelAndCanDelete(t *testing.T) {
 			wantInKernel:  false,
 			wantDeletable: false,
 		},
+		{
+			name: "only has ignorable member",
+			set: &IPSet{
+				Name: l.GetPrefixName(),
+				SetProperties: SetProperties{
+					Type: l.Type,
+					Kind: l.GetSetKind(),
+				},
+				MemberIPSets: map[string]*IPSet{
+					ignorableSet.Name: ignorableSet,
+				},
+			},
+			wantInKernel:  false,
+			wantDeletable: true,
+		},
 		{
 			name: "only has ip members",
 			set: &IPSet{
@@ -125,9 +144,9 @@ func TestShouldBeInKernelAndCanDelete(t *testing.T) {
 			}
 
 			if tt.wantDeletable {
-				require.True(t, tt.set.canBeDeleted())
+				require.True(t, tt.set.canBeDeleted(ignorableSet))
 			} else {
-				require.False(t, tt.set.canBeDeleted())
+				require.False(t, tt.set.canBeDeleted(ignorableSet))
 			}
 		})
 	}

npm/pkg/dataplane/ipsets/ipsetmanager.go

@@ -33,8 +33,21 @@ const (
 	ApplyOnNeed    IPSetMode = "on-need"
 )
 
+var (
+	emptySetMetadata = &IPSetMetadata{
+		Name: "emptyhashset",
+		Type: EmptyHashSet,
+	}
+	emptySetPrefixName = emptySetMetadata.GetPrefixName()
+)
+
 type IPSetManager struct {
-	iMgrCfg    *IPSetManagerCfg
+	iMgrCfg *IPSetManagerCfg
+	// emptySet is a direct reference to the empty ipset that should always be in the kernel.
+	// This set is used based on the AddEmptySetToLists flag.
+	// If emptySet is non-nil, it should be in the kernel or ready to be created in the dirtyCache.
+	// Its reference counts are currently unaccounted for and may be incorrect.
+	emptySet   *IPSet
 	setMap     map[string]*IPSet
 	dirtyCache dirtyCacheInterface
 	ioShim     *common.IOShim
@@ -45,11 +58,16 @@ type IPSetManagerCfg struct {
 	IPSetMode IPSetMode
 	// NetworkName can be left empty or set to 'azure' (the only supported network)
 	NetworkName string
+	// AddEmptySetToLists determines whether all lists should have an empty set as a member.
+	// This is necessary for HNS (Windows); otherwise, an allow ACL with a list condition
+	// allows all IPs if the list has no members.
+	AddEmptySetToLists bool
 }
 
 func NewIPSetManager(iMgrCfg *IPSetManagerCfg, ioShim *common.IOShim) *IPSetManager {
 	return &IPSetManager{
 		iMgrCfg:    iMgrCfg,
+		emptySet:   nil, // will be set if needed in calls to AddToLists
 		setMap:     make(map[string]*IPSet),
 		dirtyCache: newDirtyCache(),
 		ioShim:     ioShim,
@@ -60,11 +78,6 @@ func NewIPSetManager(iMgrCfg *IPSetManagerCfg, ioShim *common.IOShim) *IPSetMana
 	Reconcile removes empty/unreferenced sets from the cache.
 	For ApplyAllIPSets mode, those sets are added to the toDeleteCache.
 	We can't delete from kernel immediately unless we lock iMgr during policy CRUD.
-	If this call adds a set to the toDeleteCache, and then that set is created before
-	ApplyIPSets is called, then the set may be unnecessarily added to the toAddOrUpdateCache,
-	meaning:
-		- for Linux, an unnecessary "-N set-name --exists" call would be made in the restore file
-		- for Windows, ...
 */
 func (iMgr *IPSetManager) Reconcile() {
 	iMgr.Lock()
@@ -86,6 +99,7 @@ func (iMgr *IPSetManager) ResetIPSets() error {
 	metrics.ResetIPSetEntries()
 	err := iMgr.resetIPSets()
 	iMgr.setMap = make(map[string]*IPSet)
+	iMgr.emptySet = nil
 	iMgr.clearDirtyCache()
 	if err != nil {
 		metrics.SendErrorLogAndMetric(util.IpsmID, "error: failed to reset ipsetmanager: %s", err.Error())
@@ -109,12 +123,28 @@ func (iMgr *IPSetManager) createAndGetIPSet(setMetadata *IPSetMetadata) *IPSet {
 	if exists {
 		return set
 	}
+
 	set = NewIPSet(setMetadata)
 	iMgr.setMap[prefixedName] = set
 	metrics.IncNumIPSets()
 	if iMgr.iMgrCfg.IPSetMode == ApplyAllIPSets {
 		iMgr.modifyCacheForKernelCreation(set)
 	}
+
+	// if configured, add the empty set to lists of type KeyLabelOfNamespace and KeyValueLabelOfNamespace.
+	// The NestedLabelOfPod list ipset type is assumed to always have a member (it is created specifically for network policy pod selectors).
+	if iMgr.iMgrCfg.AddEmptySetToLists && (set.Type == KeyLabelOfNamespace || set.Type == KeyValueLabelOfNamespace) {
+		if iMgr.emptySet == nil {
+			// duplicate of code chunk above
+			iMgr.emptySet = NewIPSet(emptySetMetadata)
+			iMgr.setMap[emptySetPrefixName] = iMgr.emptySet
+			metrics.IncNumIPSets()
+			iMgr.modifyCacheForKernelCreation(iMgr.emptySet)
+		}
+
+		iMgr.addMemberToList(set, iMgr.emptySet)
+	}
+
 	return set
 }
 
@@ -214,7 +244,6 @@ func (iMgr *IPSetManager) AddToSets(addToSets []*IPSetMetadata, ip, podKey strin
 		return npmerrors.Errorf(npmerrors.AppendIPSet, true, msg)
 	}
 
-	// TODO check if the IP is IPV4 family in controller
 	iMgr.Lock()
 	defer iMgr.Unlock()
 
@@ -334,19 +363,24 @@ func (iMgr *IPSetManager) AddToLists(listMetadatas, setMetadatas []*IPSetMetadat
 			}
 			member := iMgr.setMap[memberName]
 
-			iMgr.modifyCacheForKernelMemberAdd(list, member.HashedName)
-			list.MemberIPSets[memberName] = member
-			member.incIPSetReferCount()
-			metrics.AddEntryToIPSet(list.Name)
+			iMgr.addMemberToList(list, member)
 			listIsInKernel := iMgr.shouldBeInKernel(list)
 			if listIsInKernel {
 				iMgr.incKernelReferCountAndModifyCache(member)
 			}
 		}
 	}
+
 	return nil
 }
 
+func (iMgr *IPSetManager) addMemberToList(list, member *IPSet) {
+	iMgr.modifyCacheForKernelMemberAdd(list, member.HashedName)
+	list.MemberIPSets[member.Name] = member
+	member.incIPSetReferCount()
+	metrics.AddEntryToIPSet(list.Name)
+}
+
 func (iMgr *IPSetManager) RemoveFromList(listMetadata *IPSetMetadata, setMetadatas []*IPSetMetadata) error {
 	if len(setMetadatas) == 0 {
 		return nil
@@ -370,9 +404,15 @@ func (iMgr *IPSetManager) RemoveFromList(listMetadata *IPSetMetadata, setMetadat
 	for _, setMetadata := range setMetadatas {
 		memberName := setMetadata.GetPrefixName()
 		if memberName == "" {
-			metrics.SendErrorLogAndMetric(util.IpsmID, "[RemoveFromList] warning: removing empty member name from list %s", list.Name)
+			metrics.SendErrorLogAndMetric(util.IpsmID, "[RemoveFromList] warning: tried to remove empty member name from list %s", list.Name)
+			continue
+		}
+
+		if iMgr.iMgrCfg.AddEmptySetToLists && memberName == emptySetPrefixName {
+			metrics.SendErrorLogAndMetric(util.IpsmID, "[RemoveFromList] warning: tried to remove empty set from list %s", list.Name)
 			continue
 		}
+
 		member, exists := iMgr.setMap[memberName]
 		if !exists {
 			continue
@@ -449,21 +489,23 @@ func (iMgr *IPSetManager) exists(name string) bool {
 
 // the metric for number of ipsets in the kernel will be lower than in reality until the next applyIPSet call
 func (iMgr *IPSetManager) modifyCacheForCacheDeletion(set *IPSet, deleteOption util.DeleteOption) {
+	if set == iMgr.emptySet {
+		return
+	}
+
 	if deleteOption == util.ForceDelete {
 		// If force delete, then check if Set is used by other set or network policy
 		// else delete the set even if it has members
 		if !set.canBeForceDeleted() {
 			return
 		}
-	} else if !set.canBeDeleted() {
+	} else if !set.canBeDeleted(iMgr.emptySet) {
 		return
 	}
 
 	delete(iMgr.setMap, set.Name)
 	metrics.DeleteIPSet(set.Name)
 	if iMgr.iMgrCfg.IPSetMode == ApplyAllIPSets {
-		// NOTE: in ApplyAllIPSets mode, if this ipset has never been created in the kernel,
-		// it would be added to the deleteCache, and then the OS would fail to delete it
 		iMgr.modifyCacheForKernelRemoval(set)
 	}
 	// if mode is ApplyOnNeed, the set will not be in the kernel (or will be in the delete cache already) since there are no references
@@ -489,7 +531,7 @@ func (iMgr *IPSetManager) incKernelReferCountAndModifyCache(member *IPSet) {
 }
 
 func (iMgr *IPSetManager) shouldBeInKernel(set *IPSet) bool {
-	return set.shouldBeInKernel() || iMgr.iMgrCfg.IPSetMode == ApplyAllIPSets
+	return set.shouldBeInKernel() || iMgr.iMgrCfg.IPSetMode == ApplyAllIPSets || set == iMgr.emptySet
 }
 
 func (iMgr *IPSetManager) modifyCacheForKernelRemoval(set *IPSet) {