diff --git a/npm/pkg/controlplane/controllers/v2/namespaceController.go b/npm/pkg/controlplane/controllers/v2/namespaceController.go index 0c9930ba2e..4bed108743 100644 --- a/npm/pkg/controlplane/controllers/v2/namespaceController.go +++ b/npm/pkg/controlplane/controllers/v2/namespaceController.go @@ -470,9 +470,6 @@ func (nsc *NamespaceController) cleanDeletedNamespace(cachedNsKey string) error return fmt.Errorf("failed to remove from list during clean deleted namespace %w", err) } - // Delete ipset for the namespace. - nsc.dp.DeleteIPSet(ipsets.NewIPSetMetadata(cachedNsKey, ipsets.Namespace), util.SoftDelete) - delete(nsc.npmNamespaceCache.NsMap, cachedNsKey) return nil diff --git a/npm/pkg/controlplane/controllers/v2/namespaceController_test.go b/npm/pkg/controlplane/controllers/v2/namespaceController_test.go index 2fc91b0140..0d21ab7a51 100644 --- a/npm/pkg/controlplane/controllers/v2/namespaceController_test.go +++ b/npm/pkg/controlplane/controllers/v2/namespaceController_test.go @@ -623,7 +623,6 @@ func TestDeleteNamespace(t *testing.T) { for i := 1; i < len(setsToAddNamespaceTo); i++ { dp.EXPECT().RemoveFromList(setsToAddNamespaceTo[i], setsToAddNamespaceTo[:1]).Return(nil).Times(1) } - dp.EXPECT().DeleteIPSet(setsToAddNamespaceTo[0], util.SoftDelete).Return().Times(1) deleteNamespace(t, f, nsObj, DeletedFinalStateknownObject) @@ -702,7 +701,6 @@ func TestDeleteNamespaceWithTombstoneAfterAddingNameSpace(t *testing.T) { for i := 1; i < len(setsToAddNamespaceTo); i++ { dp.EXPECT().RemoveFromList(setsToAddNamespaceTo[i], setsToAddNamespaceTo[:1]).Return(nil).Times(1) } - dp.EXPECT().DeleteIPSet(setsToAddNamespaceTo[0], util.SoftDelete).Return().Times(1) deleteNamespace(t, f, nsObj, DeletedFinalStateUnknownObject) testCases := []expectedNsValues{ diff --git a/npm/pkg/dataplane/dataplane.go b/npm/pkg/dataplane/dataplane.go index 21a3ff68df..84c029a0ad 100644 --- a/npm/pkg/dataplane/dataplane.go +++ b/npm/pkg/dataplane/dataplane.go @@ -2,6 +2,7 @@ package dataplane import ( "fmt" + "time" "github.com/Azure/azure-container-networking/common" "github.com/Azure/azure-container-networking/npm/metrics" @@ -15,6 +16,8 @@ import ( const ( // AzureNetworkName is default network Azure CNI creates AzureNetworkName = "azure" + + reconcileTimeInMinutes = 5 ) type PolicyMode string @@ -78,7 +81,27 @@ func (dp *DataPlane) BootupDataplane() error { // RunPeriodicTasks runs periodic tasks. Should only be called once. func (dp *DataPlane) RunPeriodicTasks() { - dp.policyMgr.Reconcile(dp.stopChannel) + go func() { + ticker := time.NewTicker(time.Minute * time.Duration(reconcileTimeInMinutes)) + defer ticker.Stop() + + for { + select { + case <-dp.stopChannel: + return + case <-ticker.C: + // send the heartbeat log in another go routine in case it takes a while + go metrics.SendHeartbeatLog() + + // locks ipset manager + dp.ipsetMgr.Reconcile() + + // in Windows, does nothing + // in Linux, locks policy manager but can be interrupted + dp.policyMgr.Reconcile() + } + } + }() } func (dp *DataPlane) GetIPSet(setName string) *ipsets.IPSet { @@ -292,7 +315,7 @@ func (dp *DataPlane) createIPSetsAndReferences(sets []*ipsets.TranslatedIPSet, n } for _, set := range sets { dp.ipsetMgr.CreateIPSets([]*ipsets.IPSetMetadata{set.Metadata}) - err := dp.ipsetMgr.AddReference(set.Metadata.GetPrefixName(), netpolName, referenceType) + err := dp.ipsetMgr.AddReference(set.Metadata, netpolName, referenceType) if err != nil { return npmerrors.Errorf(npmErrorString, false, fmt.Sprintf("[DataPlane] failed to add reference with err: %s", err.Error())) } @@ -337,7 +360,6 @@ func (dp *DataPlane) deleteIPSetsAndReferences(sets []*ipsets.TranslatedIPSet, n } for _, set := range sets { // TODO ignore set does not exist error - // TODO add delete ipset after removing members err := dp.ipsetMgr.DeleteReference(set.Metadata.GetPrefixName(), netpolName, referenceType) if err != nil { return npmerrors.Errorf(npmErrorString, false, fmt.Sprintf("[DataPlane] failed to deleteIPSetReferences with err: %s", err.Error())) @@ -371,7 +393,6 @@ func (dp *DataPlane) deleteIPSetsAndReferences(sets []*ipsets.TranslatedIPSet, n if err != nil { return npmerrors.Errorf(npmErrorString, false, fmt.Sprintf("[DataPlane] failed to RemoveFromList in deleteIPSetReferences with err: %s", err.Error())) } - } // Try to delete these IPSets diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager.go b/npm/pkg/dataplane/ipsets/ipsetmanager.go index f399cbbb56..0703638718 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager.go @@ -13,12 +13,23 @@ import ( type IPSetMode string +/* + IPSet Modes + + - ApplyAllIPSets: + - all ipsets are added to the kernel + - ipsets are removed from the kernel when they are deleted from the cache + - creates empty ipsets + - adds empty/unreferenced ipsets to the toDelete cache periodically + + - ApplyOnNeed: + - ipsets are added to the kernel when they are referenced by network policies or lists in the kernel + - ipsets are removed from the kernel when they no longer have a reference + - removes empty/unreferenced ipsets from the cache periodically +*/ const ( - // ApplyAllIPSets will change dataplane behavior to apply all ipsets ApplyAllIPSets IPSetMode = "all" - // ApplyOnNeed will change dataplane behavior to apply - // only ipsets that are referenced by network policies - ApplyOnNeed IPSetMode = "on-need" + ApplyOnNeed IPSetMode = "on-need" ) type IPSetManager struct { @@ -48,6 +59,29 @@ 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() + defer iMgr.Unlock() + originalNumSets := len(iMgr.setMap) + for _, set := range iMgr.setMap { + iMgr.modifyCacheForCacheDeletion(set, util.SoftDelete) + } + numRemovedSets := originalNumSets - len(iMgr.setMap) + if numRemovedSets > 0 { + klog.Infof("[IPSetManager] removed %d empty/unreferenced ipsets, updating toDeleteCache to: %+v", numRemovedSets, iMgr.toDeleteCache) + } +} + func (iMgr *IPSetManager) ResetIPSets() error { iMgr.Lock() defer iMgr.Unlock() @@ -68,51 +102,34 @@ func (iMgr *IPSetManager) CreateIPSets(setMetadatas []*IPSetMetadata) { defer iMgr.Unlock() for _, set := range setMetadatas { - iMgr.createIPSet(set) + _ = iMgr.createAndGetIPSet(set) } } -func (iMgr *IPSetManager) createIPSet(setMetadata *IPSetMetadata) { - // TODO (vamsi) check for os specific restrictions on ipsets +func (iMgr *IPSetManager) createAndGetIPSet(setMetadata *IPSetMetadata) *IPSet { prefixedName := setMetadata.GetPrefixName() - if iMgr.exists(prefixedName) { - return + set, exists := iMgr.setMap[prefixedName] + if exists { + return set } - iMgr.setMap[prefixedName] = NewIPSet(setMetadata) + set = NewIPSet(setMetadata) + iMgr.setMap[prefixedName] = set metrics.IncNumIPSets() if iMgr.iMgrCfg.IPSetMode == ApplyAllIPSets { iMgr.modifyCacheForKernelCreation(prefixedName) } + return set } // DeleteIPSet expects the prefixed ipset name -func (iMgr *IPSetManager) DeleteIPSet(name string, force util.DeleteOption) { +func (iMgr *IPSetManager) DeleteIPSet(name string, deleteOption util.DeleteOption) { iMgr.Lock() defer iMgr.Unlock() - if !iMgr.exists(name) { + set, exists := iMgr.setMap[name] + if !exists { return } - - set := iMgr.setMap[name] - if force { - // 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() { - return - } - } - - delete(iMgr.setMap, name) - metrics.DecNumIPSets() - 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(name) - } - // 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 + iMgr.modifyCacheForCacheDeletion(set, deleteOption) } // GetIPSet needs the prefixed ipset name @@ -125,23 +142,15 @@ func (iMgr *IPSetManager) GetIPSet(name string) *IPSet { return iMgr.setMap[name] } -// AddReference takes in the prefixed setname and adds relevant reference -func (iMgr *IPSetManager) AddReference(setName, referenceName string, referenceType ReferenceType) error { +// AddReference creates the set if necessary and adds relevant reference +// it throws an error if the set and reference type are an invalid combination +func (iMgr *IPSetManager) AddReference(setMetadata *IPSetMetadata, referenceName string, referenceType ReferenceType) error { iMgr.Lock() defer iMgr.Unlock() - if !iMgr.exists(setName) { - npmErrorString := npmerrors.AddSelectorReference - if referenceType == NetPolType { - npmErrorString = npmerrors.AddNetPolReference - } - msg := fmt.Sprintf("ipset %s does not exist", setName) - metrics.SendErrorLogAndMetric(util.IpsmID, "error: failed to add reference: %s", msg) - return npmerrors.Errorf(npmErrorString, false, msg) - } - - set := iMgr.setMap[setName] + // NOTE: any newly created IPSet will still be in the cache if an error is returned later + set := iMgr.createAndGetIPSet(setMetadata) if referenceType == SelectorType && !set.canSetBeSelectorIPSet() { - msg := fmt.Sprintf("ipset %s is not a selector ipset it is of type %s", setName, set.Type.String()) + msg := fmt.Sprintf("ipset %s is not a selector ipset it is of type %s", set.Name, set.Type.String()) metrics.SendErrorLogAndMetric(util.IpsmID, "error: failed to add reference: %s", msg) return npmerrors.Errorf(npmerrors.AddSelectorReference, false, msg) } @@ -149,7 +158,12 @@ func (iMgr *IPSetManager) AddReference(setName, referenceName string, referenceT set.addReference(referenceName, referenceType) if !wasInKernel { // the set should be in the kernel, so add it to the kernel if it wasn't beforehand - iMgr.modifyCacheForKernelCreation(setName) + // this branch can only be taken for ApplyOnNeed mode + iMgr.modifyCacheForKernelCreation(set.Name) + + // for ApplyAllIPSets mode, the set either: + // a) existed already and doesn't need to be added to toAddOrUpdateCache + // b) was created in createAndGetIPSet, where it was added to toAddOrUpdateCache // if set.Kind == HashSet, then this for loop will do nothing for _, member := range set.MemberIPSets { @@ -159,7 +173,8 @@ func (iMgr *IPSetManager) AddReference(setName, referenceName string, referenceT return nil } -// DeleteReference takes in the prefixed setname and removes relevant reference +// DeleteReference removes relevant reference +// it throws an error if the set doesn't exist (since a set should exist in the cache & kernel if it has a reference) func (iMgr *IPSetManager) DeleteReference(setName, referenceName string, referenceType ReferenceType) error { iMgr.Lock() defer iMgr.Unlock() @@ -178,8 +193,11 @@ func (iMgr *IPSetManager) DeleteReference(setName, referenceName string, referen set.deleteReference(referenceName, referenceType) if wasInKernel && !iMgr.shouldBeInKernel(set) { // remove from kernel if it was in the kernel before and shouldn't be now + // this branch can only be taken for ApplyOnNeed mode iMgr.modifyCacheForKernelRemoval(set.Name) + // for ApplyAllIPSets mode, we don't want to make the set dirty + // if set.Kind == HashSet, then this for loop will do nothing for _, member := range set.MemberIPSets { iMgr.decKernelReferCountAndModifyCache(member) @@ -199,12 +217,8 @@ func (iMgr *IPSetManager) AddToSets(addToSets []*IPSetMetadata, ip, podKey strin for _, metadata := range addToSets { // 1. check for errors and create a missing set prefixedName := metadata.GetPrefixName() - set, exists := iMgr.setMap[prefixedName] - if !exists { - // NOTE: any newly created IPSet will still be in the cache if an error is returned later - iMgr.createIPSet(metadata) - set = iMgr.setMap[prefixedName] - } + // NOTE: any newly created IPSet will still be in the cache if an error is returned later + set := iMgr.createAndGetIPSet(metadata) if set.Kind != HashSet { msg := fmt.Sprintf("ipset %s is not a hash set", prefixedName) metrics.SendErrorLogAndMetric(util.IpsmID, "error: failed to add to sets: %s", msg) @@ -252,7 +266,7 @@ func (iMgr *IPSetManager) RemoveFromSets(removeFromSets []*IPSetMetadata, ip, po // in case the IP belongs to a new Pod, then ignore this Delete call as this might be stale if cachedPodKey != podKey { klog.Infof( - "DeleteFromSet: PodOwner has changed for Ip: %s, setName:%s, Old podKey: %s, new podKey: %s. Ignore the delete as this is stale update", + "[IPSetManager] DeleteFromSet: PodOwner has changed for Ip: %s, setName:%s, Old podKey: %s, new podKey: %s. Ignore the delete as this is stale update", ip, prefixedName, cachedPodKey, podKey, ) continue @@ -275,18 +289,13 @@ func (iMgr *IPSetManager) AddToLists(listMetadatas, setMetadatas []*IPSetMetadat // 1. check for errors in members and create any missing sets for _, setMetadata := range setMetadatas { - setName := setMetadata.GetPrefixName() - set, exists := iMgr.setMap[setName] - if !exists { - // NOTE: any newly created IPSet will still be in the cache if an error is returned later - iMgr.createIPSet(setMetadata) - set = iMgr.setMap[setName] - } + // NOTE: any newly created IPSet will still be in the cache if an error is returned later + set := iMgr.createAndGetIPSet(setMetadata) // Nested IPSets are only supported for windows // Check if we want to actually use that support if set.Kind != HashSet { - msg := fmt.Sprintf("ipset %s is not a hash set and nested list sets are not supported", setName) + msg := fmt.Sprintf("ipset %s is not a hash set and nested list sets are not supported", set.Name) metrics.SendErrorLogAndMetric(util.IpsmID, "error: failed to add to lists: %s", msg) return npmerrors.Errorf(npmerrors.AppendIPSet, false, msg) } @@ -294,16 +303,11 @@ func (iMgr *IPSetManager) AddToLists(listMetadatas, setMetadatas []*IPSetMetadat for _, listMetadata := range listMetadatas { // 2. create the list if it's missing and check for list errors - listName := listMetadata.GetPrefixName() - list, exists := iMgr.setMap[listName] - if !exists { - // NOTE: any newly created IPSet will still be in the cache if an error is returned later - iMgr.createIPSet(listMetadata) - list = iMgr.setMap[listName] - } + // NOTE: any newly created IPSet will still be in the cache if an error is returned later + list := iMgr.createAndGetIPSet(listMetadata) if list.Kind != ListSet { - msg := fmt.Sprintf("ipset %s is not a list set", listName) + msg := fmt.Sprintf("ipset %s is not a list set", list.Name) metrics.SendErrorLogAndMetric(util.IpsmID, "error: failed to add to lists: %s", msg) return npmerrors.Errorf(npmerrors.AppendIPSet, false, msg) } @@ -320,7 +324,7 @@ func (iMgr *IPSetManager) AddToLists(listMetadatas, setMetadatas []*IPSetMetadat list.MemberIPSets[memberName] = member member.incIPSetReferCount() - metrics.AddEntryToIPSet(listName) + metrics.AddEntryToIPSet(list.Name) listIsInKernel := iMgr.shouldBeInKernel(list) if listIsInKernel { iMgr.incKernelReferCountAndModifyCache(member) @@ -402,8 +406,7 @@ func (iMgr *IPSetManager) ApplyIPSets() error { return nil } - klog.Infof("[IPSetManager] toAddUpdateCache %+v \n ", iMgr.toAddOrUpdateCache) - klog.Infof("[IPSetManager] toDeleteCache %+v \n ", iMgr.toDeleteCache) + klog.Infof("[IPSetManager] toAddUpdateCache: %+v \ntoDeleteCache: %+v", iMgr.toAddOrUpdateCache, iMgr.toDeleteCache) iMgr.sanitizeDirtyCache() // Call the appropriate apply ipsets @@ -420,53 +423,6 @@ func (iMgr *IPSetManager) ApplyIPSets() error { return nil } -// GetIPsFromSelectorIPSets will take in a map of prefixedSetNames and return an intersection of IPs -func (iMgr *IPSetManager) GetIPsFromSelectorIPSets(setList map[string]struct{}) (map[string]struct{}, error) { - if len(setList) == 0 { - return map[string]struct{}{}, nil - } - iMgr.Lock() - defer iMgr.Unlock() - - setintersections := make(map[string]struct{}) - var err error - firstLoop := true - for setName := range setList { - if !iMgr.exists(setName) { - return nil, npmerrors.Errorf( - npmerrors.GetSelectorReference, - false, - fmt.Sprintf("[ipset manager] selector ipset %s does not exist", setName)) - } - set := iMgr.setMap[setName] - if firstLoop { - intialSetIPs := set.IPPodKey - for k := range intialSetIPs { - setintersections[k] = struct{}{} - } - firstLoop = false - } - setintersections, err = set.getSetIntersection(setintersections) - if err != nil { - return nil, err - } - } - return setintersections, err -} - -func (iMgr *IPSetManager) GetSelectorReferencesBySet(setName string) (map[string]struct{}, error) { - iMgr.Lock() - defer iMgr.Unlock() - if !iMgr.exists(setName) { - return nil, npmerrors.Errorf( - npmerrors.GetSelectorReference, - false, - fmt.Sprintf("[ipset manager] selector ipset %s does not exist", setName)) - } - set := iMgr.setMap[setName] - return set.SelectorReference, nil -} - func (iMgr *IPSetManager) GetAllIPSets() []string { iMgr.Lock() defer iMgr.Unlock() @@ -484,6 +440,28 @@ func (iMgr *IPSetManager) exists(name string) bool { return ok } +// 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 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() { + return + } + + delete(iMgr.setMap, set.Name) + metrics.DecNumIPSets() + 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.Name) + } + // 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 +} + func (iMgr *IPSetManager) modifyCacheForKernelCreation(setName string) { iMgr.toAddOrUpdateCache[setName] = struct{}{} delete(iMgr.toDeleteCache, setName) diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go index b80bd7b731..3af773af77 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go @@ -17,8 +17,8 @@ type expectedInfo struct { mainCache []setMembers toAddUpdateCache []*IPSetMetadata toDeleteCache []string - setsForKernel []*IPSetMetadata - // TODO add expected failure metric values too + // setsForKernel represents the sets in toAddUpdateCache that should be in the kernel + setsForKernel []*IPSetMetadata /* ipset metrics can be inferred from the above values: @@ -29,8 +29,10 @@ type expectedInfo struct { } type setMembers struct { - metadata *IPSetMetadata - members []member + metadata *IPSetMetadata + members []member + selectorReferences []string + netPolReferences []string } type member struct { @@ -43,13 +45,13 @@ type memberKind bool const ( isHashMember = memberKind(true) - // TODO uncomment and use for list add/delete UTs - // isSetMember = memberKind(false) + isSetMember = memberKind(false) - testSetName = "test-set" - testListName = "test-list" - testPodKey = "test-pod-key" - testPodIP = "10.0.0.0" + testSetName = "test-set" + testListName = "test-list" + testPodKey = "test-pod-key" + testPodIP = "10.0.0.0" + testNetPolKey = "test-ns/test-netpol" ) var ( @@ -68,6 +70,134 @@ var ( list = NewIPSetMetadata("test-list1", KeyLabelOfNamespace) ) +func TestReconcileCache(t *testing.T) { + type args struct { + cfg *IPSetManagerCfg + setsInKernel []*IPSetMetadata + } + deletableSet := keyLabelOfPodSet + otherSet := namespaceSet + bothMetadatas := []*IPSetMetadata{deletableSet, otherSet} + tests := []struct { + name string + args args + toDeleteCache []string + }{ + { + name: "Apply Always", + args: args{cfg: applyAlwaysCfg, setsInKernel: bothMetadatas}, + toDeleteCache: []string{deletableSet.GetPrefixName()}, + }, + { + name: "Apply On Need", + args: args{cfg: applyOnNeedCfg, setsInKernel: nil}, + toDeleteCache: nil, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + metrics.ReinitializeAll() + calls := GetApplyIPSetsTestCalls(tt.args.setsInKernel, nil) + ioShim := common.NewMockIOShim(calls) + defer ioShim.VerifyCalls(t, calls) + iMgr := NewIPSetManager(tt.args.cfg, ioShim) + + // create two sets, one which can be deleted + iMgr.CreateIPSets(bothMetadatas) + require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{otherSet}, testPodIP, testPodKey)) + require.NoError(t, iMgr.ApplyIPSets()) + + iMgr.Reconcile() + assertExpectedInfo(t, iMgr, &expectedInfo{ + mainCache: []setMembers{ + {metadata: otherSet, members: []member{{testPodIP, isHashMember}}}, + }, + toAddUpdateCache: nil, + toDeleteCache: tt.toDeleteCache, + setsForKernel: nil, + }) + }) + } +} + +// only care about ApplyAllIPSets mode since ApplyOnNeed mode doesn't update the toDeleteCache +func TestReconcileAndLaterDelete(t *testing.T) { + deletableSet := keyLabelOfPodSet + otherSet := namespaceSet + thirdSet := list + tests := []struct { + name string + setsToAdd []*IPSetMetadata + shouldDeleteLater bool + *expectedInfo + }{ + { + name: "apply the delete only", + setsToAdd: nil, + shouldDeleteLater: true, + expectedInfo: &expectedInfo{ + mainCache: []setMembers{ + {metadata: otherSet, members: []member{{testPodIP, isHashMember}}}, + }, + toAddUpdateCache: nil, + toDeleteCache: []string{deletableSet.GetPrefixName()}, + }, + }, + { + name: "delete the set and add a different one", + setsToAdd: []*IPSetMetadata{thirdSet}, + shouldDeleteLater: true, + expectedInfo: &expectedInfo{ + mainCache: []setMembers{ + {metadata: otherSet, members: []member{{testPodIP, isHashMember}}}, + {metadata: thirdSet}, + }, + toAddUpdateCache: []*IPSetMetadata{thirdSet}, + toDeleteCache: []string{deletableSet.GetPrefixName()}, + }, + }, + { + name: "add the set back", + setsToAdd: []*IPSetMetadata{deletableSet}, + shouldDeleteLater: false, + expectedInfo: &expectedInfo{ + mainCache: []setMembers{ + {metadata: deletableSet}, + {metadata: otherSet, members: []member{{testPodIP, isHashMember}}}, + }, + toAddUpdateCache: []*IPSetMetadata{deletableSet}, + toDeleteCache: nil, + }, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + metrics.ReinitializeAll() + originalMetadatas := []*IPSetMetadata{deletableSet, otherSet} + var toDeleteMetadatas []*IPSetMetadata + if tt.shouldDeleteLater { + toDeleteMetadatas = []*IPSetMetadata{deletableSet} + } + calls := GetApplyIPSetsTestCalls(originalMetadatas, nil) + calls = append(calls, GetApplyIPSetsTestCalls(tt.setsToAdd, toDeleteMetadatas)...) + ioShim := common.NewMockIOShim(calls) + defer ioShim.VerifyCalls(t, calls) + iMgr := NewIPSetManager(applyAlwaysCfg, ioShim) + // create two sets, one which can be deleted + iMgr.CreateIPSets(originalMetadatas) + require.NoError(t, iMgr.AddToSets([]*IPSetMetadata{namespaceSet}, testPodIP, testPodKey)) + require.NoError(t, iMgr.ApplyIPSets()) + iMgr.Reconcile() + + iMgr.CreateIPSets(tt.setsToAdd) + assertExpectedInfo(t, iMgr, tt.expectedInfo) + require.NoError(t, iMgr.ApplyIPSets()) + }) + } +} + // see ipsetmanager_linux_test.go for testing when an error occurs func TestResetIPSets(t *testing.T) { metrics.ReinitializeAll() @@ -112,8 +242,8 @@ func TestCreateIPSet(t *testing.T) { }, expectedInfo: expectedInfo{ mainCache: []setMembers{ - {metadata: namespaceSet, members: nil}, - {metadata: list, members: nil}, + {metadata: namespaceSet}, + {metadata: list}, }, toAddUpdateCache: []*IPSetMetadata{namespaceSet, list}, toDeleteCache: nil, @@ -128,8 +258,8 @@ func TestCreateIPSet(t *testing.T) { }, expectedInfo: expectedInfo{ mainCache: []setMembers{ - {metadata: namespaceSet, members: nil}, - {metadata: list, members: nil}, + {metadata: namespaceSet}, + {metadata: list}, }, toAddUpdateCache: nil, toDeleteCache: nil, @@ -144,7 +274,7 @@ func TestCreateIPSet(t *testing.T) { }, expectedInfo: expectedInfo{ mainCache: []setMembers{ - {metadata: namespaceSet, members: nil}, + {metadata: namespaceSet}, }, toAddUpdateCache: []*IPSetMetadata{namespaceSet}, toDeleteCache: nil, @@ -159,7 +289,7 @@ func TestCreateIPSet(t *testing.T) { }, expectedInfo: expectedInfo{ mainCache: []setMembers{ - {metadata: namespaceSet, members: nil}, + {metadata: namespaceSet}, }, toAddUpdateCache: nil, toDeleteCache: nil, @@ -229,7 +359,7 @@ func TestDeleteIPSet(t *testing.T) { }, expectedInfo: expectedInfo{ mainCache: []setMembers{ - {metadata: namespaceSet, members: nil}, + {metadata: namespaceSet}, }, toAddUpdateCache: nil, toDeleteCache: nil, @@ -245,7 +375,7 @@ func TestDeleteIPSet(t *testing.T) { }, expectedInfo: expectedInfo{ mainCache: []setMembers{ - {metadata: namespaceSet, members: nil}, + {metadata: namespaceSet}, }, toAddUpdateCache: nil, toDeleteCache: nil, @@ -259,7 +389,7 @@ func TestDeleteIPSet(t *testing.T) { t.Run(tt.name, func(t *testing.T) { metrics.ReinitializeAll() var calls []testutils.TestCmd - if tt.args.cfg == applyAlwaysCfg { + if tt.args.cfg.IPSetMode == ApplyAllIPSets { calls = GetApplyIPSetsTestCalls(tt.args.toCreateMetadatas, nil) } ioShim := common.NewMockIOShim(calls) @@ -290,8 +420,8 @@ func TestDeleteIPSetNotAllowed(t *testing.T) { assertExpectedInfo(t, iMgr, &expectedInfo{ mainCache: []setMembers{ - {metadata: list, members: nil}, - {metadata: namespaceSet, members: nil}, + {metadata: list, members: []member{{namespaceSet.GetPrefixName(), isSetMember}}}, + {metadata: namespaceSet}, }, toAddUpdateCache: nil, toDeleteCache: nil, @@ -389,7 +519,7 @@ func TestAddToSets(t *testing.T) { }, toAddUpdateCache: nil, toDeleteCache: nil, - setsForKernel: []*IPSetMetadata{namespaceSet}, + setsForKernel: nil, }, wantErr: false, }, @@ -409,7 +539,7 @@ func TestAddToSets(t *testing.T) { }, toAddUpdateCache: nil, toDeleteCache: nil, - setsForKernel: []*IPSetMetadata{namespaceSet}, + setsForKernel: nil, }, wantErr: false, }, @@ -433,7 +563,7 @@ func TestAddToSets(t *testing.T) { }, expectedInfo: expectedInfo{ mainCache: []setMembers{ - {metadata: list, members: nil}, + {metadata: list}, }, toAddUpdateCache: []*IPSetMetadata{list}, toDeleteCache: nil, @@ -452,7 +582,7 @@ func TestAddToSets(t *testing.T) { }, expectedInfo: expectedInfo{ mainCache: []setMembers{ - {metadata: list, members: nil}, + {metadata: list}, }, toAddUpdateCache: nil, toDeleteCache: nil, @@ -466,7 +596,7 @@ func TestAddToSets(t *testing.T) { t.Run(tt.name, func(t *testing.T) { metrics.ReinitializeAll() var calls []testutils.TestCmd - if tt.args.cfg == applyAlwaysCfg { + if tt.args.cfg.IPSetMode == ApplyAllIPSets { calls = GetApplyIPSetsTestCalls(tt.args.toCreateMetadatas, nil) } ioShim := common.NewMockIOShim(calls) @@ -528,7 +658,7 @@ func TestAddToSetInKernelApplyOnNeed(t *testing.T) { defer ioShim.VerifyCalls(t, calls) iMgr := NewIPSetManager(applyOnNeedCfg, ioShim) iMgr.CreateIPSets(metadatas) - require.NoError(t, iMgr.AddReference(tt.metadata.GetPrefixName(), "ref", NetPolType)) + require.NoError(t, iMgr.AddReference(tt.metadata, testNetPolKey, NetPolType)) require.NoError(t, iMgr.ApplyIPSets()) err := iMgr.AddToSets(metadatas, ipv4, podKey) @@ -545,11 +675,11 @@ func TestAddToSetInKernelApplyOnNeed(t *testing.T) { } assertExpectedInfo(t, iMgr, &expectedInfo{ mainCache: []setMembers{ - {metadata: tt.metadata, members: members}, + {metadata: tt.metadata, members: members, netPolReferences: []string{testNetPolKey}}, }, toAddUpdateCache: dirtySets, toDeleteCache: nil, - setsForKernel: []*IPSetMetadata{tt.metadata}, + setsForKernel: dirtySets, }) }) } @@ -635,199 +765,351 @@ func TestRemoveFromListMissing(t *testing.T) { err := iMgr.RemoveFromList(listMetadata, []*IPSetMetadata{setMetadata}) require.NoError(t, err) } -func TestGetIPsFromSelectorIPSets(t *testing.T) { - iMgr := NewIPSetManager(applyOnNeedCfg, common.NewMockIOShim([]testutils.TestCmd{})) - setsTocreate := []*IPSetMetadata{ + +func TestAddReference(t *testing.T) { + ref0 := "ref0" // for alreadyReferenced + ref1 := "ref1" + type args struct { + cfg *IPSetManagerCfg + metadata *IPSetMetadata + refType ReferenceType + alreadyExisted bool + alreadyReferenced bool + } + tests := []struct { + name string + args args + wantErr bool + *expectedInfo + }{ { - Name: "setNs1", - Type: Namespace, + name: "Apply Always: successfully add selector reference (set did not exist)", + args: args{ + cfg: applyAlwaysCfg, + metadata: namespaceSet, + refType: SelectorType, + }, + wantErr: false, + expectedInfo: &expectedInfo{ + mainCache: []setMembers{ + {metadata: namespaceSet, selectorReferences: []string{ref1}}, + }, + toAddUpdateCache: []*IPSetMetadata{namespaceSet}, + setsForKernel: []*IPSetMetadata{namespaceSet}, + }, }, { - Name: "setpod1", - Type: KeyLabelOfPod, + name: "Apply Always: successfully add selector reference (set existed)", + args: args{ + cfg: applyAlwaysCfg, + metadata: namespaceSet, + refType: SelectorType, + alreadyExisted: true, + }, + wantErr: false, + expectedInfo: &expectedInfo{ + mainCache: []setMembers{ + {metadata: namespaceSet, selectorReferences: []string{ref1}}, + }, + }, }, { - Name: "setpod2", - Type: KeyLabelOfPod, + name: "Apply Always: not a selector set (set did not exist)", + args: args{ + cfg: applyAlwaysCfg, + metadata: list, + refType: SelectorType, + }, + wantErr: true, + expectedInfo: &expectedInfo{ + mainCache: []setMembers{ + {metadata: list}, + }, + toAddUpdateCache: []*IPSetMetadata{list}, + setsForKernel: []*IPSetMetadata{list}, + }, }, { - Name: "setpod3", - Type: KeyValueLabelOfPod, + name: "Apply Always: not a selector set (set existed)", + args: args{ + cfg: applyAlwaysCfg, + metadata: list, + refType: SelectorType, + alreadyExisted: true, + }, + wantErr: true, + expectedInfo: &expectedInfo{ + mainCache: []setMembers{ + {metadata: list}, + }, + }, }, - } - - iMgr.CreateIPSets(setsTocreate) - - err := iMgr.AddToSets(setsTocreate, "10.0.0.1", "test") - require.NoError(t, err) - - err = iMgr.AddToSets(setsTocreate, "10.0.0.2", "test1") - require.NoError(t, err) - - err = iMgr.AddToSets([]*IPSetMetadata{setsTocreate[0], setsTocreate[2], setsTocreate[3]}, "10.0.0.3", "test3") - require.NoError(t, err) - - ipsetList := map[string]struct{}{} - for _, v := range setsTocreate { - ipsetList[v.GetPrefixName()] = struct{}{} - } - ips, err := iMgr.GetIPsFromSelectorIPSets(ipsetList) - require.NoError(t, err) - - require.Equal(t, 2, len(ips)) - - expectedintersection := map[string]struct{}{ - "10.0.0.1": {}, - "10.0.0.2": {}, - } - - require.Equal(t, ips, expectedintersection) -} - -func TestAddDeleteSelectorReferences(t *testing.T) { - iMgr := NewIPSetManager(applyOnNeedCfg, common.NewMockIOShim([]testutils.TestCmd{})) - setsTocreate := []*IPSetMetadata{ { - Name: "setNs1", - Type: Namespace, + // already tested set (not) existing + name: "Apply Always: successfully add netpol reference", + args: args{ + cfg: applyAlwaysCfg, + metadata: namespaceSet, + refType: NetPolType, + alreadyExisted: true, + }, + wantErr: false, + expectedInfo: &expectedInfo{ + mainCache: []setMembers{ + {metadata: namespaceSet, netPolReferences: []string{ref1}}, + }, + }, }, { - Name: "setpod1", - Type: KeyLabelOfPod, + name: "Apply On Need: successfully add reference (set did not exist)", + args: args{ + cfg: applyOnNeedCfg, + metadata: namespaceSet, + refType: SelectorType, + }, + wantErr: false, + expectedInfo: &expectedInfo{ + mainCache: []setMembers{ + {metadata: namespaceSet, selectorReferences: []string{ref1}}, + }, + toAddUpdateCache: []*IPSetMetadata{namespaceSet}, + setsForKernel: []*IPSetMetadata{namespaceSet}, + }, }, { - Name: "setpod2", - Type: KeyLabelOfPod, + name: "Apply On Need: successfully add reference (set existed but not already referenced)", + args: args{ + cfg: applyOnNeedCfg, + metadata: namespaceSet, + refType: SelectorType, + alreadyExisted: true, + }, + wantErr: false, + expectedInfo: &expectedInfo{ + mainCache: []setMembers{ + {metadata: namespaceSet, selectorReferences: []string{ref1}}, + }, + toAddUpdateCache: []*IPSetMetadata{namespaceSet}, + setsForKernel: []*IPSetMetadata{namespaceSet}, + }, }, { - Name: "setpod3", - Type: NestedLabelOfPod, + name: "Apply On Need: successfully add reference (set already referenced)", + args: args{ + cfg: applyOnNeedCfg, + metadata: namespaceSet, + refType: SelectorType, + alreadyExisted: true, + alreadyReferenced: true, + }, + wantErr: false, + expectedInfo: &expectedInfo{ + mainCache: []setMembers{ + {metadata: namespaceSet, selectorReferences: []string{ref0, ref1}}, + }, + }, }, { - Name: "setpod4", - Type: KeyLabelOfPod, + // best to test an unreferenced set to make sure it doesn't get added to the update cache + name: "Apply On Need: not a selector set", + args: args{ + cfg: applyOnNeedCfg, + metadata: list, + refType: SelectorType, + }, + wantErr: true, + expectedInfo: &expectedInfo{ + mainCache: []setMembers{ + {metadata: list}, + }, + }, }, } - networkPolicName := "testNetworkPolicy" - - for _, k := range setsTocreate { - err := iMgr.AddReference(k.GetPrefixName(), networkPolicName, SelectorType) - require.Error(t, err) - } - - iMgr.CreateIPSets(setsTocreate) - - // Add setpod4 to setpod3 - err := iMgr.AddToLists([]*IPSetMetadata{setsTocreate[3]}, []*IPSetMetadata{setsTocreate[4]}) - require.NoError(t, err) - - for _, v := range setsTocreate { - err = iMgr.AddReference(v.GetPrefixName(), networkPolicName, SelectorType) - require.NoError(t, err) - } - - require.Equal(t, 5, len(iMgr.toAddOrUpdateCache)) - require.Equal(t, 0, len(iMgr.toDeleteCache)) - - for _, v := range setsTocreate { - err = iMgr.DeleteReference(v.GetPrefixName(), networkPolicName, SelectorType) - if err != nil { - t.Errorf("DeleteReference failed with error %s", err.Error()) - } - } - - require.Equal(t, 0, len(iMgr.toAddOrUpdateCache)) - require.Equal(t, 5, len(iMgr.toDeleteCache)) + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + metrics.ReinitializeAll() - for _, v := range setsTocreate { - iMgr.DeleteIPSet(v.GetPrefixName(), util.SoftDelete) - } + metadatas := []*IPSetMetadata{tt.args.metadata} - // Above delete will not remove setpod3 and setpod4 - // because they are referencing each other - require.Equal(t, 2, len(iMgr.setMap)) + var calls []testutils.TestCmd + if tt.args.alreadyExisted && (tt.args.cfg.IPSetMode == ApplyAllIPSets || tt.args.alreadyReferenced) { + calls = GetApplyIPSetsTestCalls(metadatas, nil) + } + ioShim := common.NewMockIOShim(calls) + defer ioShim.VerifyCalls(t, calls) + iMgr := NewIPSetManager(tt.args.cfg, ioShim) - err = iMgr.RemoveFromList(setsTocreate[3], []*IPSetMetadata{setsTocreate[4]}) - require.NoError(t, err) + if tt.args.alreadyExisted { + iMgr.CreateIPSets(metadatas) + if tt.args.alreadyReferenced { + require.NoError(t, iMgr.AddReference(tt.args.metadata, ref0, tt.args.refType), "alreadyReferenced and wantErr is not supported") + } + require.NoError(t, iMgr.ApplyIPSets()) + } - for _, v := range setsTocreate { - iMgr.DeleteIPSet(v.GetPrefixName(), util.SoftDelete) - } + err := iMgr.AddReference(tt.args.metadata, ref1, tt.args.refType) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } - for _, v := range setsTocreate { - set := iMgr.GetIPSet(v.GetPrefixName()) - require.Nil(t, set) + assertExpectedInfo(t, iMgr, tt.expectedInfo) + }) } } -func TestAddDeleteNetPolReferences(t *testing.T) { - iMgr := NewIPSetManager(applyOnNeedCfg, common.NewMockIOShim([]testutils.TestCmd{})) - setsTocreate := []*IPSetMetadata{ +func TestDeleteReferenceApplyAlways(t *testing.T) { + metadata := namespaceSet + type args struct { + refType ReferenceType + setExists bool + hadReference bool + } + tests := []struct { + name string + args args + wantErr bool + }{ { - Name: "setNs1", - Type: Namespace, + name: "successfully delete selector reference", + args: args{ + refType: SelectorType, + setExists: true, + hadReference: true, + }, + wantErr: false, }, { - Name: "setpod1", - Type: KeyLabelOfPod, + name: "successfully delete selector reference (wasn't referenced)", + args: args{ + refType: SelectorType, + setExists: true, + hadReference: false, + }, + wantErr: false, }, { - Name: "setpod2", - Type: KeyLabelOfPod, + name: "successfully delete netpol reference", + args: args{ + refType: NetPolType, + setExists: true, + hadReference: true, + }, + wantErr: false, }, { - Name: "setpod3", - Type: NestedLabelOfPod, + name: "successfully delete netpol reference (wasn't referenced)", + args: args{ + refType: NetPolType, + setExists: true, + hadReference: false, + }, + wantErr: false, }, { - Name: "setpod4", - Type: KeyLabelOfPod, + name: "failure when set doesn't exist", + args: args{ + refType: SelectorType, + hadReference: false, + }, + wantErr: true, }, } - networkPolicName := "testNetworkPolicy" + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + // check semantics + require.False(t, tt.args.setExists && tt.wantErr, "setExists and wantErr is not supported") - iMgr.CreateIPSets(setsTocreate) - err := iMgr.AddToLists([]*IPSetMetadata{setsTocreate[3]}, []*IPSetMetadata{setsTocreate[4]}) - require.NoError(t, err) + ref := "ref" + metrics.ReinitializeAll() - for _, v := range setsTocreate { - err = iMgr.AddReference(v.GetPrefixName(), networkPolicName, NetPolType) - require.NoError(t, err) - } + var calls []testutils.TestCmd + if tt.args.setExists { + calls = GetApplyIPSetsTestCalls([]*IPSetMetadata{metadata}, nil) + } + ioShim := common.NewMockIOShim(calls) + defer ioShim.VerifyCalls(t, calls) + iMgr := NewIPSetManager(applyAlwaysCfg, ioShim) - require.Equal(t, 5, len(iMgr.toAddOrUpdateCache)) - require.Equal(t, 0, len(iMgr.toDeleteCache)) - for _, v := range setsTocreate { - err = iMgr.DeleteReference(v.GetPrefixName(), networkPolicName, NetPolType) - require.NoError(t, err) - } + if tt.args.setExists { + iMgr.CreateIPSets([]*IPSetMetadata{metadata}) + require.NoError(t, iMgr.AddReference(metadata, ref, tt.args.refType)) + require.NoError(t, iMgr.ApplyIPSets()) + } - require.Equal(t, 0, len(iMgr.toAddOrUpdateCache)) - require.Equal(t, 5, len(iMgr.toDeleteCache)) + err := iMgr.DeleteReference(metadata.GetPrefixName(), ref, tt.args.refType) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } - for _, v := range setsTocreate { - iMgr.DeleteIPSet(v.GetPrefixName(), util.SoftDelete) + info := &expectedInfo{} + if tt.args.setExists { + setMember := setMembers{metadata: metadata} + info.mainCache = []setMembers{setMember} + } + assertExpectedInfo(t, iMgr, info) + }) } +} - // Above delete will not remove setpod3 and setpod4 - // because they are referencing each other - require.Equal(t, 2, len(iMgr.setMap)) +func TestDeleteReferenceApplyOnNeed(t *testing.T) { + type referenceCount bool + oneReference := referenceCount(false) + twoReferences := referenceCount(true) + tests := []struct { + name string + numReferences referenceCount + shouldDeleteSet bool + }{ + { + name: "Apply On Need: delete last reference", + numReferences: oneReference, + shouldDeleteSet: true, + }, + { + name: "Apply On Need: delete a reference (set still referenced after)", + numReferences: twoReferences, + shouldDeleteSet: false, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + metadata := namespaceSet + ref0 := "ref0" + ref1 := "ref1" - err = iMgr.RemoveFromList(setsTocreate[3], []*IPSetMetadata{setsTocreate[4]}) - require.NoError(t, err) + metrics.ReinitializeAll() - for _, v := range setsTocreate { - iMgr.DeleteIPSet(v.GetPrefixName(), util.SoftDelete) - } + calls := GetApplyIPSetsTestCalls([]*IPSetMetadata{metadata}, nil) + ioShim := common.NewMockIOShim(calls) + defer ioShim.VerifyCalls(t, calls) + iMgr := NewIPSetManager(applyOnNeedCfg, ioShim) - for _, v := range setsTocreate { - set := iMgr.GetIPSet(v.GetPrefixName()) - require.Nil(t, set) - } + iMgr.CreateIPSets([]*IPSetMetadata{metadata}) + if tt.numReferences == twoReferences { + require.NoError(t, iMgr.AddReference(metadata, ref0, SelectorType)) + } + require.NoError(t, iMgr.AddReference(metadata, ref1, SelectorType)) + require.NoError(t, iMgr.ApplyIPSets()) + require.NoError(t, iMgr.DeleteReference(metadata.GetPrefixName(), ref1, SelectorType)) - for _, v := range setsTocreate { - err = iMgr.DeleteReference(v.GetPrefixName(), networkPolicName, NetPolType) - require.Error(t, err) + info := &expectedInfo{} + s := setMembers{metadata: metadata} + if tt.numReferences == twoReferences { + s.selectorReferences = []string{ref0} + } + info.mainCache = []setMembers{s} + if tt.shouldDeleteSet { + info.toDeleteCache = []string{metadata.GetPrefixName()} + } + assertExpectedInfo(t, iMgr, info) + }) } } @@ -887,6 +1169,7 @@ func TestValidateIPBlock(t *testing.T) { func assertExpectedInfo(t *testing.T, iMgr *IPSetManager, info *expectedInfo) { // 1. assert cache contents + // 1.1. make sure the main cache is equal, including members and references require.Equal(t, len(info.mainCache), len(iMgr.setMap), "main cache size mismatch") for _, setMembers := range info.mainCache { setName := setMembers.metadata.GetPrefixName() @@ -894,8 +1177,9 @@ func assertExpectedInfo(t *testing.T, iMgr *IPSetManager, info *expectedInfo) { set := iMgr.GetIPSet(setName) require.NotNil(t, set, "set %s should be non-nil", setName) require.Equal(t, util.GetHashedName(setName), set.HashedName, "HashedName mismatch") + + require.Equal(t, len(setMembers.members), len(set.IPPodKey)+len(set.MemberIPSets), "set %s member size mismatch", setName) for _, member := range setMembers.members { - set := iMgr.setMap[setName] if member.kind == isHashMember { _, ok := set.IPPodKey[member.value] require.True(t, ok, "ip member %s not found in set %s", member.value, setName) @@ -904,8 +1188,21 @@ func assertExpectedInfo(t *testing.T, iMgr *IPSetManager, info *expectedInfo) { require.True(t, ok, "set member %s not found in list %s", member.value, setName) } } + + require.Equal(t, len(setMembers.selectorReferences), len(set.SelectorReference), "set %s selector reference size mismatch", setName) + for _, ref := range setMembers.selectorReferences { + _, ok := set.SelectorReference[ref] + require.True(t, ok, "selector reference %s not found in set %s", ref, setName) + } + + require.Equal(t, len(setMembers.netPolReferences), len(set.NetPolReference), "set %s netpol reference size mismatch", setName) + for _, ref := range setMembers.netPolReferences { + _, ok := set.NetPolReference[ref] + require.True(t, ok, "netpol reference %s not found in set %s", ref, setName) + } } + // 1.2. make sure the toAddOrUpdateCache is equal require.Equal(t, len(info.toAddUpdateCache), len(iMgr.toAddOrUpdateCache), "toAddUpdateCache size mismatch") for _, setMetadata := range info.toAddUpdateCache { setName := setMetadata.GetPrefixName() @@ -914,13 +1211,19 @@ func assertExpectedInfo(t *testing.T, iMgr *IPSetManager, info *expectedInfo) { require.True(t, iMgr.exists(setName), "set %s not in the main cache but is in the toAddUpdateCache", setName) } + // 1.3. make sure the toDeleteCache is equal require.Equal(t, len(info.toDeleteCache), len(iMgr.toDeleteCache), "toDeleteCache size mismatch") for _, setName := range info.toDeleteCache { _, ok := iMgr.toDeleteCache[setName] require.True(t, ok, "set %s not found in toDeleteCache", setName) } + // 1.4. assert kernel status of sets in the toAddOrUpdateCache for _, setMetadata := range info.setsForKernel { + // check semantics + _, ok := iMgr.toAddOrUpdateCache[setMetadata.GetPrefixName()] + require.True(t, ok, "setsForKernel should be a subset of toAddUpdateCache") + setName := setMetadata.GetPrefixName() require.True(t, iMgr.exists(setName), "kernel set %s not found", setName) set := iMgr.setMap[setName] diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_windows.go b/npm/pkg/dataplane/ipsets/ipsetmanager_windows.go index ea601e5002..2c3072d9c2 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_windows.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_windows.go @@ -25,6 +25,53 @@ type networkPolicyBuilder struct { toDeleteSets map[string]*hcn.SetPolicySetting } +// GetIPsFromSelectorIPSets will take in a map of prefixedSetNames and return an intersection of IPs +func (iMgr *IPSetManager) GetIPsFromSelectorIPSets(setList map[string]struct{}) (map[string]struct{}, error) { + if len(setList) == 0 { + return map[string]struct{}{}, nil + } + iMgr.Lock() + defer iMgr.Unlock() + + setintersections := make(map[string]struct{}) + var err error + firstLoop := true + for setName := range setList { + if !iMgr.exists(setName) { + return nil, errors.Errorf( + errors.GetSelectorReference, + false, + fmt.Sprintf("[ipset manager] selector ipset %s does not exist", setName)) + } + set := iMgr.setMap[setName] + if firstLoop { + intialSetIPs := set.IPPodKey + for k := range intialSetIPs { + setintersections[k] = struct{}{} + } + firstLoop = false + } + setintersections, err = set.getSetIntersection(setintersections) + if err != nil { + return nil, err + } + } + return setintersections, err +} + +func (iMgr *IPSetManager) GetSelectorReferencesBySet(setName string) (map[string]struct{}, error) { + iMgr.Lock() + defer iMgr.Unlock() + if !iMgr.exists(setName) { + return nil, errors.Errorf( + errors.GetSelectorReference, + false, + fmt.Sprintf("[ipset manager] selector ipset %s does not exist", setName)) + } + set := iMgr.setMap[setName] + return set.SelectorReference, nil +} + func (iMgr *IPSetManager) resetIPSets() error { klog.Infof("[IPSetManager Windows] Resetting Dataplane") network, err := iMgr.getHCnNetwork() diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_windows_test.go b/npm/pkg/dataplane/ipsets/ipsetmanager_windows_test.go index c0f2c52ca2..425e6d8bde 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_windows_test.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_windows_test.go @@ -11,6 +11,55 @@ import ( "github.com/stretchr/testify/require" ) +func TestGetIPsFromSelectorIPSets(t *testing.T) { + iMgr := NewIPSetManager(applyOnNeedCfg, common.NewMockIOShim([]testutils.TestCmd{})) + setsTocreate := []*IPSetMetadata{ + { + Name: "setNs1", + Type: Namespace, + }, + { + Name: "setpod1", + Type: KeyLabelOfPod, + }, + { + Name: "setpod2", + Type: KeyLabelOfPod, + }, + { + Name: "setpod3", + Type: KeyValueLabelOfPod, + }, + } + + iMgr.CreateIPSets(setsTocreate) + + err := iMgr.AddToSets(setsTocreate, "10.0.0.1", "test") + require.NoError(t, err) + + err = iMgr.AddToSets(setsTocreate, "10.0.0.2", "test1") + require.NoError(t, err) + + err = iMgr.AddToSets([]*IPSetMetadata{setsTocreate[0], setsTocreate[2], setsTocreate[3]}, "10.0.0.3", "test3") + require.NoError(t, err) + + ipsetList := map[string]struct{}{} + for _, v := range setsTocreate { + ipsetList[v.GetPrefixName()] = struct{}{} + } + ips, err := iMgr.GetIPsFromSelectorIPSets(ipsetList) + require.NoError(t, err) + + require.Equal(t, 2, len(ips)) + + expectedintersection := map[string]struct{}{ + "10.0.0.1": {}, + "10.0.0.2": {}, + } + + require.Equal(t, ips, expectedintersection) +} + func TestAddToSetWindows(t *testing.T) { hns := GetHNSFake(t) io := common.NewMockIOShimWithFakeHNS(hns) diff --git a/npm/pkg/dataplane/policies/policymanager.go b/npm/pkg/dataplane/policies/policymanager.go index e5501dd8ea..8c578dce59 100644 --- a/npm/pkg/dataplane/policies/policymanager.go +++ b/npm/pkg/dataplane/policies/policymanager.go @@ -3,7 +3,6 @@ package policies import ( "fmt" "sync" - "time" "github.com/Azure/azure-container-networking/common" "github.com/Azure/azure-container-networking/npm/metrics" @@ -22,8 +21,6 @@ const ( // IPPolicyMode will replace ipset names with their value IPs in policies IPPolicyMode PolicyManagerMode = "IP" - reconcileTimeInMinutes = 5 - // this number is based on the implementation in chain-management_linux.go // it represents the number of rules unrelated to policies // it's technically 3 off when there are no policies since we flush the AZURE-NPM chain then @@ -83,21 +80,8 @@ func (pMgr *PolicyManager) Bootup(epIDs []string) error { return nil } -func (pMgr *PolicyManager) Reconcile(stopChannel <-chan struct{}) { - go func() { - ticker := time.NewTicker(time.Minute * time.Duration(reconcileTimeInMinutes)) - defer ticker.Stop() - - for { - select { - case <-stopChannel: - return - case <-ticker.C: - pMgr.reconcile() - metrics.SendHeartbeatLog() - } - } - }() +func (pMgr *PolicyManager) Reconcile() { + pMgr.reconcile() } func (pMgr *PolicyManager) GetAllPolicies() []string { diff --git a/npm/pkg/dataplane/policies/testutils.go b/npm/pkg/dataplane/policies/testutils.go index 8e13578474..f35b83af50 100644 --- a/npm/pkg/dataplane/policies/testutils.go +++ b/npm/pkg/dataplane/policies/testutils.go @@ -20,6 +20,11 @@ var ( MatchType: EitherMatch, }, }, + // derived from testACLs + RuleIPSets: []*ipsets.TranslatedIPSet{ + {Metadata: ipsets.TestCIDRSet.Metadata, Members: nil}, + {Metadata: ipsets.TestKeyPodSet.Metadata, Members: nil}, + }, ACLs: testACLs, }, { @@ -42,6 +47,9 @@ var ( MatchType: EitherMatch, }, }, + RuleIPSets: []*ipsets.TranslatedIPSet{ + {Metadata: ipsets.TestCIDRSet.Metadata, Members: nil}, + }, ACLs: []*ACLPolicy{ testACLs[0], }, @@ -50,6 +58,9 @@ var ( Name: "test3", NameSpace: "z", PolicyKey: "z/test3", + RuleIPSets: []*ipsets.TranslatedIPSet{ + {Metadata: ipsets.TestCIDRSet.Metadata, Members: nil}, + }, ACLs: []*ACLPolicy{ testACLs[3], }, diff --git a/test/integration/npm/main.go b/test/integration/npm/main.go index dd6f8f23fd..f94c2007bb 100644 --- a/test/integration/npm/main.go +++ b/test/integration/npm/main.go @@ -12,12 +12,14 @@ import ( ) const ( - MaxSleepTime = 2 - finalWaitTimeInMinutes = 10 - includeLists = false + MaxSleepTime = 1 + finalSleepTimeInSeconds = 10 + includeLists = false ) var ( + counter = 0 + dpCfg = &dataplane.Config{ IPSetManagerCfg: &ipsets.IPSetManagerCfg{ IPSetMode: ipsets.ApplyAllIPSets, @@ -31,7 +33,7 @@ var ( nodeName = "testNode" testNetPol = &policies.NPMNetworkPolicy{ - Name: "test/test-netpol", + PolicyKey: "test/test-netpol", PodSelectorIPSets: []*ipsets.TranslatedIPSet{ { Metadata: ipsets.TestNSSet.Metadata, @@ -77,8 +79,8 @@ var ( func main() { dp, err := dataplane.NewDataPlane(nodeName, common.NewIOShim(), dpCfg, make(chan struct{}, 1)) - dp.RunPeriodicTasks() panicOnError(err) + dp.RunPeriodicTasks() printAndWait(true) podMetadata := &dataplane.PodMetadata{ @@ -170,9 +172,21 @@ func main() { printAndWait(true) panicOnError(dp.AddPolicy(policies.TestNetworkPolicies[0])) fmt.Println("AZURE-NPM should have rules now") + printAndWait(true) + + unusedSet1 := ipsets.NewIPSetMetadata("unused-set1", ipsets.CIDRBlocks) + fmt.Printf("\ncreating an empty set, it should be deleted by reconcile: %s\n", unusedSet1.GetHashedName()) + dp.CreateIPSets([]*ipsets.IPSetMetadata{unusedSet1}) + panicOnError(dp.ApplyDataPlane()) + + fmt.Printf("sleeping %d seconds to allow reconcile (update the reconcile time in dataplane.go to be less than %d seconds)\n", finalSleepTimeInSeconds, finalSleepTimeInSeconds) + time.Sleep(time.Duration(finalSleepTimeInSeconds) * time.Second) + + unusedSet2 := ipsets.NewIPSetMetadata("unused-set2", ipsets.CIDRBlocks) + fmt.Printf("\ncreating an unused set %s. The prior empty set %s should be deleted on this apply\n", unusedSet2.GetHashedName(), unusedSet1.GetHashedName()) + dp.CreateIPSets([]*ipsets.IPSetMetadata{unusedSet2}) + panicOnError(dp.ApplyDataPlane()) - fmt.Println("waiting for reconcile to finish (will be a while if you don't update the reconcile time in policymanager.go") - time.Sleep(finalWaitTimeInMinutes * time.Minute) } func panicOnError(err error) { @@ -182,7 +196,8 @@ func panicOnError(err error) { } func printAndWait(wait bool) { - fmt.Printf("#####################\nCompleted running, please check relevant commands, script will resume in %d secs\n#############\n", MaxSleepTime) + counter++ + fmt.Printf("#####################\nCompleted running step %d, please check relevant commands, script will resume in %d secs\n#############\n", counter, MaxSleepTime) if wait { for i := 0; i < MaxSleepTime; i++ { fmt.Print(".")