diff --git a/.pipelines/npm/npm-conformance-tests.yaml b/.pipelines/npm/npm-conformance-tests.yaml index 580d7b0ad4..e30d625fed 100644 --- a/.pipelines/npm/npm-conformance-tests.yaml +++ b/.pipelines/npm/npm-conformance-tests.yaml @@ -187,9 +187,10 @@ jobs: curl -LO https://dl.k8s.io/release/v1.20.0/bin/linux/amd64/kubectl chmod +x kubectl npmPodList=`kubectl get pods -n kube-system | grep npm | awk '{print $1}'` - mkdir -p $(System.DefaultWorkingDirectory)/npmLogs_$(PROFILE) - for npm in $npmPodList; do ./kubectl logs -n kube-system $npm --kubeconfig=./kubeconfig > $(System.DefaultWorkingDirectory)/npmLogs/$npm ;done - mv ./kubeconfig $(System.DefaultWorkingDirectory)/npmLogs_$(PROFILE)/kubeconfig + npmLogsFolder=$(System.DefaultWorkingDirectory)/npmLogs_$(PROFILE) + mkdir -p $npmLogsFolder + for npm in $npmPodList; do ./kubectl logs -n kube-system $npm --kubeconfig=./kubeconfig > $npmLogsFolder/$npm ;done + mv ./kubeconfig $npmLogsFolder/kubeconfig displayName: "Gather NPM Logs" condition: always() diff --git a/npm/metrics/prometheus-metrics.go b/npm/metrics/prometheus-metrics.go index cca3ea165d..6d7daa2acc 100644 --- a/npm/metrics/prometheus-metrics.go +++ b/npm/metrics/prometheus-metrics.go @@ -77,6 +77,14 @@ func InitializeAll() { } } +// ReinitializeAll creates/replaces Prometheus metrics. This function is intended for UTs. +// Be sure to reset helper variables e.g. ipsetInventoryMap. +func ReinitializeAll() { + haveInitialized = false + InitializeAll() + ipsetInventoryMap = make(map[string]int) +} + // GetHandler returns the HTTP handler for the metrics endpoint func GetHandler(isNodeLevel bool) http.Handler { return promhttp.HandlerFor(getRegistry(isNodeLevel), promhttp.HandlerOpts{}) diff --git a/npm/pkg/dataplane/dataplane_test.go b/npm/pkg/dataplane/dataplane_test.go index b44c5559b1..f5892b8a81 100644 --- a/npm/pkg/dataplane/dataplane_test.go +++ b/npm/pkg/dataplane/dataplane_test.go @@ -8,7 +8,6 @@ import ( "github.com/Azure/azure-container-networking/npm/metrics" "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets" "github.com/Azure/azure-container-networking/npm/pkg/dataplane/policies" - "github.com/Azure/azure-container-networking/npm/util" testutils "github.com/Azure/azure-container-networking/test/utils" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -27,11 +26,6 @@ var ( }, } - fakeIPSetRestoreSuccess = testutils.TestCmd{ - Cmd: []string{util.Ipset, util.IpsetRestoreFlag}, - ExitCode: 0, - } - setPodKey1 = &ipsets.TranslatedIPSet{ Metadata: ipsets.NewIPSetMetadata("setpodkey1", ipsets.KeyLabelOfPod), } diff --git a/npm/pkg/dataplane/ipsets/ipset_test.go b/npm/pkg/dataplane/ipsets/ipset_test.go new file mode 100644 index 0000000000..2a30e1bb7f --- /dev/null +++ b/npm/pkg/dataplane/ipsets/ipset_test.go @@ -0,0 +1,134 @@ +package ipsets + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestShouldBeInKernelAndCanDelete(t *testing.T) { + s := &IPSetMetadata{"test-set", Namespace} + l := &IPSetMetadata{"test-list", KeyLabelOfNamespace} + tests := []struct { + name string + set *IPSet + wantInKernel bool + wantDeletable bool + }{ + { + name: "only has selector reference", + set: &IPSet{ + Name: s.GetPrefixName(), + SetProperties: SetProperties{ + Type: s.Type, + Kind: s.GetSetKind(), + }, + SelectorReference: map[string]struct{}{ + "ref-1": {}, + }, + }, + wantInKernel: true, + wantDeletable: false, + }, + { + name: "only has netpol reference", + set: &IPSet{ + Name: s.GetPrefixName(), + SetProperties: SetProperties{ + Type: s.Type, + Kind: s.GetSetKind(), + }, + NetPolReference: map[string]struct{}{ + "ref-1": {}, + }, + }, + wantInKernel: true, + wantDeletable: false, + }, + { + name: "only referenced in list (in kernel)", + set: &IPSet{ + Name: s.GetPrefixName(), + SetProperties: SetProperties{ + Type: s.Type, + Kind: s.GetSetKind(), + }, + ipsetReferCount: 1, + kernelReferCount: 1, + }, + wantInKernel: true, + wantDeletable: false, + }, + { + name: "only referenced in list (not in kernel)", + set: &IPSet{ + Name: s.GetPrefixName(), + SetProperties: SetProperties{ + Type: s.Type, + Kind: s.GetSetKind(), + }, + ipsetReferCount: 1, + }, + wantInKernel: false, + wantDeletable: false, + }, + { + name: "only has set members", + set: &IPSet{ + Name: l.GetPrefixName(), + SetProperties: SetProperties{ + Type: l.Type, + Kind: l.GetSetKind(), + }, + MemberIPSets: map[string]*IPSet{ + s.GetPrefixName(): NewIPSet(s), + }, + }, + wantInKernel: false, + wantDeletable: false, + }, + { + name: "only has ip members", + set: &IPSet{ + Name: s.GetPrefixName(), + SetProperties: SetProperties{ + Type: s.Type, + Kind: s.GetSetKind(), + }, + IPPodKey: map[string]string{ + "1.2.3.4": "pod-a", + }, + }, + wantInKernel: false, + wantDeletable: false, + }, + { + name: "deletable", + set: &IPSet{ + Name: s.GetPrefixName(), + SetProperties: SetProperties{ + Type: Namespace, + Kind: s.GetSetKind(), + }, + }, + wantInKernel: false, + wantDeletable: true, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + if tt.wantInKernel { + require.True(t, tt.set.shouldBeInKernel()) + } else { + require.False(t, tt.set.shouldBeInKernel()) + } + + if tt.wantDeletable { + require.True(t, tt.set.canBeDeleted()) + } else { + require.False(t, tt.set.canBeDeleted()) + } + }) + } +} diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager.go b/npm/pkg/dataplane/ipsets/ipsetmanager.go index d815838cfe..cc7575fd5f 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager.go @@ -37,8 +37,6 @@ type IPSetManagerCfg struct { NetworkName string } -// TODO delegate prometheus metrics logic to OS-specific ones? - func NewIPSetManager(iMgrCfg *IPSetManagerCfg, ioShim *common.IOShim) *IPSetManager { return &IPSetManager{ iMgrCfg: iMgrCfg, @@ -56,6 +54,8 @@ func (iMgr *IPSetManager) ResetIPSets() error { if err != nil { return fmt.Errorf("error while resetting ipsetmanager: %w", err) } + // TODO update prometheus metrics here instead of in OS-specific functions (done in Linux right now) + // metrics.ResetNumIPSets() and metrics.ResetIPSetEntries() return nil } @@ -97,7 +97,8 @@ func (iMgr *IPSetManager) DeleteIPSet(name string) { delete(iMgr.setMap, name) metrics.DecNumIPSets() if iMgr.iMgrCfg.IPSetMode == ApplyAllIPSets { - iMgr.modifyCacheForKernelRemoval(name) // FIXME this mode would try to delete an ipset from the kernel if it's never been created in the kernel + // 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 } @@ -132,7 +133,7 @@ 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(set.Name) + iMgr.modifyCacheForKernelCreation(setName) // if set.Kind == HashSet, then this for loop will do nothing for _, member := range set.MemberIPSets { @@ -170,101 +171,201 @@ func (iMgr *IPSetManager) DeleteReference(setName, referenceName string, referen } func (iMgr *IPSetManager) AddToSets(addToSets []*IPSetMetadata, ip, podKey string) error { - // check if the IP is IPV4 family in controller + if len(addToSets) == 0 { + return nil + } + // TODO check if the IP is IPV4 family in controller iMgr.Lock() defer iMgr.Unlock() - if err := iMgr.checkForIPUpdateErrors(addToSets, npmerrors.AppendIPSet); err != nil { - return err - } + 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] + } + if set.Kind != HashSet { + return npmerrors.Errorf(npmerrors.AppendIPSet, false, fmt.Sprintf("ipset %s is not a hash set", prefixedName)) + } - for _, setMetadata := range addToSets { - prefixedName := setMetadata.GetPrefixName() - set := iMgr.setMap[prefixedName] - cachedPodKey, ok := set.IPPodKey[ip] + // 2. add ip to the set, and update the pod key + _, ok := set.IPPodKey[ip] set.IPPodKey[ip] = podKey - if ok && cachedPodKey != podKey { - klog.Infof("AddToSet: PodOwner has changed for Ip: %s, setName:%s, Old podKey: %s, new podKey: %s. Replace context with new PodOwner.", - ip, set.Name, cachedPodKey, podKey) + if ok { continue } - iMgr.modifyCacheForKernelMemberUpdate(prefixedName) + iMgr.modifyCacheForKernelMemberUpdate(set) metrics.AddEntryToIPSet(prefixedName) } return nil } func (iMgr *IPSetManager) RemoveFromSets(removeFromSets []*IPSetMetadata, ip, podKey string) error { + if len(removeFromSets) == 0 { + return nil + } iMgr.Lock() defer iMgr.Unlock() - if err := iMgr.checkForIPUpdateErrors(removeFromSets, npmerrors.DeleteIPSet); err != nil { - return err - } - - for _, setMetadata := range removeFromSets { - prefixedName := setMetadata.GetPrefixName() - set := iMgr.setMap[prefixedName] + // 1. check for errors (ignore missing sets) + for _, metadata := range removeFromSets { + prefixedName := metadata.GetPrefixName() + set, exists := iMgr.setMap[prefixedName] + if !exists { + continue + } + if set.Kind != HashSet { + return npmerrors.Errorf(npmerrors.DeleteIPSet, false, fmt.Sprintf("ipset %s is not a hash set", prefixedName)) + } - // in case the IP belongs to a new Pod, then ignore this Delete call as this might be stale + // 2. remove ip from the set cachedPodKey, exists := set.IPPodKey[ip] if !exists { continue } + // 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", - ip, prefixedName, 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", + ip, prefixedName, cachedPodKey, podKey, + ) continue } // update the IP ownership with podkey delete(set.IPPodKey, ip) - iMgr.modifyCacheForKernelMemberUpdate(prefixedName) + iMgr.modifyCacheForKernelMemberUpdate(set) metrics.RemoveEntryFromIPSet(prefixedName) } return nil } func (iMgr *IPSetManager) AddToLists(listMetadatas, setMetadatas []*IPSetMetadata) error { + if len(listMetadatas) == 0 || len(setMetadatas) == 0 { + return nil + } iMgr.Lock() defer iMgr.Unlock() - if err := iMgr.checkForListMemberUpdateErrors(listMetadatas, setMetadatas, npmerrors.AppendIPSet); err != nil { - return err + // 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] + } + + // Nested IPSets are only supported for windows + // Check if we want to actually use that support + if set.Kind != HashSet { + return npmerrors.Errorf(npmerrors.AppendIPSet, false, fmt.Sprintf("ipset %s is not a hash set and nested list sets are not supported", setName)) + } } for _, listMetadata := range listMetadatas { + // 2. create the list if it's missing and check for list errors listName := listMetadata.GetPrefixName() - for _, setMetadata := range setMetadatas { - setName := setMetadata.GetPrefixName() - iMgr.addMemberIPSet(listName, setName) + 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] + } + + if list.Kind != ListSet { + return npmerrors.Errorf(npmerrors.AppendIPSet, false, fmt.Sprintf("ipset %s is not a list set", listName)) + } + + modified := false + // 3. add all members to the list + for _, memberMetadata := range setMetadatas { + memberName := memberMetadata.GetPrefixName() + // the member shouldn't be the list itself, but this is satisfied since we already asserted that the member is a HashSet + if list.hasMember(memberName) { + continue + } + member := iMgr.setMap[memberName] + + list.MemberIPSets[memberName] = member + member.incIPSetReferCount() + metrics.AddEntryToIPSet(listName) + listIsInKernel := iMgr.shouldBeInKernel(list) + if listIsInKernel { + iMgr.incKernelReferCountAndModifyCache(member) + } + modified = true + } + if modified { + iMgr.modifyCacheForKernelMemberUpdate(list) } - iMgr.modifyCacheForKernelMemberUpdate(listName) - metrics.AddEntryToIPSet(listName) } return nil } func (iMgr *IPSetManager) RemoveFromList(listMetadata *IPSetMetadata, setMetadatas []*IPSetMetadata) error { + if len(setMetadatas) == 0 { + return nil + } iMgr.Lock() defer iMgr.Unlock() - if err := iMgr.checkForListMemberUpdateErrors([]*IPSetMetadata{listMetadata}, setMetadatas, npmerrors.DeleteIPSet); err != nil { - return err + // 1. check for errors (ignore missing sets) + listName := listMetadata.GetPrefixName() + list, exists := iMgr.setMap[listName] + if !exists { + return nil } - listName := listMetadata.GetPrefixName() + if list.Kind != ListSet { + return npmerrors.Errorf(npmerrors.DeleteIPSet, false, fmt.Sprintf("ipset %s is not a list set", listName)) + } + + modified := false for _, setMetadata := range setMetadatas { - setName := setMetadata.GetPrefixName() - iMgr.removeMemberIPSet(listName, setName) + memberName := setMetadata.GetPrefixName() + member, exists := iMgr.setMap[memberName] + if !exists { + continue + } + + // Nested IPSets are only supported for windows + // Check if we want to actually use that support + if member.Kind != HashSet { + if modified { + iMgr.modifyCacheForKernelMemberUpdate(list) + } + return npmerrors.Errorf(npmerrors.DeleteIPSet, false, fmt.Sprintf("ipset %s is not a hash set and nested list sets are not supported", memberName)) + } + + // 2. remove member from the list + if !list.hasMember(memberName) { + continue + } + + delete(list.MemberIPSets, memberName) + member.decIPSetReferCount() + metrics.RemoveEntryFromIPSet(list.Name) + listIsInKernel := iMgr.shouldBeInKernel(list) + if listIsInKernel { + iMgr.decKernelReferCountAndModifyCache(member) + } + modified = true + } + if modified { + iMgr.modifyCacheForKernelMemberUpdate(list) } - iMgr.modifyCacheForKernelMemberUpdate(listName) - metrics.RemoveEntryFromIPSet(listName) return nil } func (iMgr *IPSetManager) ApplyIPSets() error { + prometheusTimer := metrics.StartNewTimer() + iMgr.Lock() defer iMgr.Unlock() @@ -272,6 +373,7 @@ func (iMgr *IPSetManager) ApplyIPSets() error { klog.Info("[IPSetManager] No IPSets to apply") return nil } + defer metrics.RecordIPSetExecTime(prometheusTimer) // record execution time regardless of failure klog.Infof("[IPSetManager] toAddUpdateCache %+v \n ", iMgr.toAddOrUpdateCache) klog.Infof("[IPSetManager] toDeleteCache %+v \n ", iMgr.toDeleteCache) @@ -383,102 +485,9 @@ func (iMgr *IPSetManager) decKernelReferCountAndModifyCache(member *IPSet) { } } -func (iMgr *IPSetManager) checkForIPUpdateErrors(setNames []*IPSetMetadata, npmErrorString string) error { - for _, set := range setNames { - prefixedSetName := set.GetPrefixName() - if !iMgr.exists(prefixedSetName) { - iMgr.createIPSet(set) - } - - set := iMgr.setMap[prefixedSetName] - if set.Kind != HashSet { - return npmerrors.Errorf(npmErrorString, false, fmt.Sprintf("ipset %s is not a hash set", prefixedSetName)) - } - } - return nil -} - -func (iMgr *IPSetManager) modifyCacheForKernelMemberUpdate(setName string) { - set := iMgr.setMap[setName] +func (iMgr *IPSetManager) modifyCacheForKernelMemberUpdate(set *IPSet) { if iMgr.shouldBeInKernel(set) { - iMgr.toAddOrUpdateCache[setName] = struct{}{} - /* - TODO kernel-based prometheus metrics - - if isAdd { - metrics.AddEntryToKernelIPSet(setName) - } else { - metrics.RemoveEntryFromKernelIPSet(setName) - } - */ - } -} - -func (iMgr *IPSetManager) checkForListMemberUpdateErrors(listMetadata, memberMetadatas []*IPSetMetadata, npmErrorString string) error { - for _, listMetadata := range listMetadata { - prefixedListName := listMetadata.GetPrefixName() - if !iMgr.exists(prefixedListName) { - iMgr.createIPSet(listMetadata) - } - - list := iMgr.setMap[prefixedListName] - if list.Kind != ListSet { - return npmerrors.Errorf(npmErrorString, false, fmt.Sprintf("ipset %s is not a list set", prefixedListName)) - } - for _, memberMetadata := range memberMetadatas { - memberName := memberMetadata.GetPrefixName() - if prefixedListName == memberName { - return npmerrors.Errorf(npmErrorString, false, fmt.Sprintf("ipset %s cannot be added to itself", prefixedListName)) - } - } - } - - for _, memberMetadata := range memberMetadatas { - memberName := memberMetadata.GetPrefixName() - if !iMgr.exists(memberName) { - iMgr.createIPSet(memberMetadata) - } - member := iMgr.setMap[memberName] - - // Nested IPSets are only supported for windows - // Check if we want to actually use that support - if member.Kind != HashSet { - return npmerrors.Errorf(npmErrorString, false, fmt.Sprintf("ipset %s is not a hash set and nested list sets are not supported", memberName)) - } - } - return nil -} - -func (iMgr *IPSetManager) addMemberIPSet(listName, memberName string) { - list := iMgr.setMap[listName] - if list.hasMember(memberName) { - return - } - - member := iMgr.setMap[memberName] - - list.MemberIPSets[memberName] = member - member.incIPSetReferCount() - metrics.AddEntryToIPSet(list.Name) - listIsInKernel := iMgr.shouldBeInKernel(list) - if listIsInKernel { - iMgr.incKernelReferCountAndModifyCache(member) - } -} - -func (iMgr *IPSetManager) removeMemberIPSet(listName, memberName string) { - list := iMgr.setMap[listName] - if !list.hasMember(memberName) { - return - } - - member := iMgr.setMap[memberName] - delete(list.MemberIPSets, member.Name) - member.decIPSetReferCount() - metrics.RemoveEntryFromIPSet(list.Name) - listIsInKernel := iMgr.shouldBeInKernel(list) - if listIsInKernel { - iMgr.decKernelReferCountAndModifyCache(member) + iMgr.toAddOrUpdateCache[set.Name] = struct{}{} } } @@ -488,11 +497,7 @@ func (iMgr *IPSetManager) sanitizeDirtyCache() { for setName := range iMgr.toDeleteCache { _, ok := iMgr.toAddOrUpdateCache[setName] if ok { - // delete(iMgr.toDeleteCache, setName) - // We have decided not proactively clean up the cache - // instead will be logging a log message as below - - klog.Infof("[IPSetManager] Unexpected state in dirty cache %s set is part of both update and delete caches \n ", setName) + klog.Errorf("[IPSetManager] Unexpected state in dirty cache %s set is part of both update and delete caches \n ", setName) } } } diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go index 34d77a13f0..7e8a8afdc9 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go @@ -1,18 +1,51 @@ package ipsets import ( + "fmt" "os" "testing" "github.com/Azure/azure-container-networking/common" "github.com/Azure/azure-container-networking/npm/metrics" + "github.com/Azure/azure-container-networking/npm/metrics/promutil" "github.com/Azure/azure-container-networking/npm/util" testutils "github.com/Azure/azure-container-networking/test/utils" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +type expectedInfo struct { + mainCache []setMembers + toAddUpdateCache []*IPSetMetadata + toDeleteCache []string + setsForKernel []*IPSetMetadata + // TODO add expected failure metric values too + + /* + ipset metrics can be inferred from the above values: + - num ipsets in cache/kernel + - num entries (in kernel) + - ipset inventory for kernel (num entries per set) + */ +} + +type setMembers struct { + metadata *IPSetMetadata + members []member +} + +type member struct { + value string + // either an IP/IP,PORT/CIDR or set name + kind memberKind +} + +type memberKind bool + const ( + isHashMember = memberKind(true) + // TODO uncomment and use for list add/delete UTs + // isSetMember = memberKind(false) + testSetName = "test-set" testListName = "test-list" testPodKey = "test-pod-key" @@ -29,64 +62,599 @@ var ( IPSetMode: ApplyAllIPSets, NetworkName: "azure", } -) -func TestCreateIPSet(t *testing.T) { - iMgr := NewIPSetManager(applyOnNeedCfg, common.NewMockIOShim([]testutils.TestCmd{})) - - setMetadata := NewIPSetMetadata(testSetName, Namespace) - iMgr.CreateIPSets([]*IPSetMetadata{setMetadata}) - // creating twice - iMgr.CreateIPSets([]*IPSetMetadata{setMetadata}) - - assert.True(t, iMgr.exists(setMetadata.GetPrefixName())) + namespaceSet = NewIPSetMetadata("test-set1", Namespace) + keyLabelOfPodSet = NewIPSetMetadata("test-set2", KeyLabelOfPod) + list = NewIPSetMetadata("test-list1", KeyLabelOfNamespace) +) - set := iMgr.GetIPSet(setMetadata.GetPrefixName()) - require.NotNil(t, set) - assert.Equal(t, setMetadata.GetPrefixName(), set.Name) - assert.Equal(t, util.GetHashedName(setMetadata.GetPrefixName()), set.HashedName) +func TestApplyIPSets(t *testing.T) { + type args struct { + toAddUpdateSets []*IPSetMetadata + toDeleteSets []*IPSetMetadata + commandError bool + } + tests := []struct { + name string + args args + expectedExecCount int + wantErr bool + }{ + { + name: "nothing to update", + args: args{ + toAddUpdateSets: nil, + toDeleteSets: nil, + commandError: false, + }, + expectedExecCount: 0, + wantErr: false, + }, + { + name: "success with just add", + args: args{ + toAddUpdateSets: []*IPSetMetadata{namespaceSet}, + toDeleteSets: nil, + commandError: false, + }, + expectedExecCount: 1, + wantErr: false, + }, + { + name: "success with just delete", + args: args{ + toAddUpdateSets: []*IPSetMetadata{namespaceSet}, + toDeleteSets: nil, + commandError: false, + }, + expectedExecCount: 1, + wantErr: false, + }, + { + name: "success with add and delete", + args: args{ + toAddUpdateSets: []*IPSetMetadata{namespaceSet}, + toDeleteSets: []*IPSetMetadata{keyLabelOfPodSet}, + commandError: false, + }, + expectedExecCount: 1, + wantErr: false, + }, + { + name: "set is in both delete and add/update cache", + args: args{ + toAddUpdateSets: []*IPSetMetadata{namespaceSet}, + toDeleteSets: []*IPSetMetadata{namespaceSet}, + commandError: false, + }, + expectedExecCount: 1, + wantErr: false, + }, + { + name: "apply error", + args: args{ + toAddUpdateSets: []*IPSetMetadata{namespaceSet}, + toDeleteSets: []*IPSetMetadata{keyLabelOfPodSet}, + commandError: true, + }, + expectedExecCount: 1, + wantErr: true, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + metrics.ReinitializeAll() + calls := GetApplyIPSetsTestCalls(tt.args.toAddUpdateSets, tt.args.toDeleteSets) + if tt.args.commandError { + // add an error to the last call (the ipset-restore call) + // this would potentially cause problems if we used pointers to the TestCmds + require.Greater(t, len(calls), 0) + calls[len(calls)-1].ExitCode = 1 + // then add errors as many times as we retry + for i := 1; i < maxTryCount; i++ { + calls = append(calls, testutils.TestCmd{Cmd: ipsetRestoreStringSlice, ExitCode: 1}) + } + } + ioShim := common.NewMockIOShim(calls) + defer ioShim.VerifyCalls(t, calls) + iMgr := NewIPSetManager(applyAlwaysCfg, ioShim) + iMgr.CreateIPSets(tt.args.toAddUpdateSets) + for _, set := range tt.args.toDeleteSets { + iMgr.toDeleteCache[set.GetPrefixName()] = struct{}{} + } + err := iMgr.ApplyIPSets() + + // cache behavior is currently undefined if there's an apply error + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + cache := make([]setMembers, 0) + for _, set := range tt.args.toAddUpdateSets { + cache = append(cache, setMembers{metadata: set, members: nil}) + } + assertExpectedInfo(t, iMgr, &expectedInfo{ + mainCache: cache, + toAddUpdateCache: nil, + toDeleteCache: nil, + setsForKernel: nil, + }) + } + + execCount, err := metrics.GetIPSetExecCount() + promutil.NotifyIfErrors(t, err) + require.Equal(t, tt.expectedExecCount, execCount) + }) + } } -func TestCreateIPSetApplyAlways(t *testing.T) { - iMgr := NewIPSetManager(applyAlwaysCfg, common.NewMockIOShim([]testutils.TestCmd{})) - - setMetadata := NewIPSetMetadata(testSetName, Namespace) - iMgr.CreateIPSets([]*IPSetMetadata{setMetadata}) - // creating twice - iMgr.CreateIPSets([]*IPSetMetadata{setMetadata}) - - assert.True(t, iMgr.exists(setMetadata.GetPrefixName())) +func TestCreateIPSet(t *testing.T) { + type args struct { + cfg *IPSetManagerCfg + metadatas []*IPSetMetadata + } + tests := []struct { + name string + args args + expectedInfo + }{ + { + name: "Apply Always: create two new sets", + args: args{ + cfg: applyAlwaysCfg, + metadatas: []*IPSetMetadata{namespaceSet, list}, + }, + expectedInfo: expectedInfo{ + mainCache: []setMembers{ + {metadata: namespaceSet, members: nil}, + {metadata: list, members: nil}, + }, + toAddUpdateCache: []*IPSetMetadata{namespaceSet, list}, + toDeleteCache: nil, + setsForKernel: []*IPSetMetadata{namespaceSet, list}, + }, + }, + { + name: "Apply On Need: create two new sets", + args: args{ + cfg: applyOnNeedCfg, + metadatas: []*IPSetMetadata{namespaceSet, list}, + }, + expectedInfo: expectedInfo{ + mainCache: []setMembers{ + {metadata: namespaceSet, members: nil}, + {metadata: list, members: nil}, + }, + toAddUpdateCache: nil, + toDeleteCache: nil, + setsForKernel: nil, + }, + }, + { + name: "Apply Always: no-op for set that exists", + args: args{ + cfg: applyAlwaysCfg, + metadatas: []*IPSetMetadata{namespaceSet, namespaceSet}, + }, + expectedInfo: expectedInfo{ + mainCache: []setMembers{ + {metadata: namespaceSet, members: nil}, + }, + toAddUpdateCache: []*IPSetMetadata{namespaceSet}, + toDeleteCache: nil, + setsForKernel: []*IPSetMetadata{namespaceSet}, + }, + }, + { + name: "Apply On Need: no-op for set that exists", + args: args{ + cfg: applyOnNeedCfg, + metadatas: []*IPSetMetadata{namespaceSet, namespaceSet}, + }, + expectedInfo: expectedInfo{ + mainCache: []setMembers{ + {metadata: namespaceSet, members: nil}, + }, + toAddUpdateCache: nil, + toDeleteCache: nil, + setsForKernel: nil, + }, + }, + } - set := iMgr.GetIPSet(setMetadata.GetPrefixName()) - require.NotNil(t, set) - assert.Equal(t, setMetadata.GetPrefixName(), set.Name) - assert.Equal(t, util.GetHashedName(setMetadata.GetPrefixName()), set.HashedName) + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + metrics.ReinitializeAll() + ioShim := common.NewMockIOShim(nil) + defer ioShim.VerifyCalls(t, nil) + iMgr := NewIPSetManager(tt.args.cfg, ioShim) + iMgr.CreateIPSets(tt.args.metadatas) + assertExpectedInfo(t, iMgr, &tt.expectedInfo) + }) + } } -func TestAddToSet(t *testing.T) { - iMgr := NewIPSetManager(applyOnNeedCfg, common.NewMockIOShim([]testutils.TestCmd{})) - - setMetadata := NewIPSetMetadata(testSetName, Namespace) - iMgr.CreateIPSets([]*IPSetMetadata{setMetadata}) +func TestDeleteIPSet(t *testing.T) { + type args struct { + cfg *IPSetManagerCfg + toCreateMetadatas []*IPSetMetadata + toDeleteName string + } + tests := []struct { + name string + args args + expectedInfo expectedInfo + }{ + { + name: "Apply Always: delete set", + args: args{ + cfg: applyAlwaysCfg, + toCreateMetadatas: []*IPSetMetadata{namespaceSet}, + toDeleteName: namespaceSet.GetPrefixName(), + }, + expectedInfo: expectedInfo{ + mainCache: nil, + toAddUpdateCache: nil, + toDeleteCache: []string{namespaceSet.GetPrefixName()}, + setsForKernel: nil, + }, + }, + { + name: "Apply On Need: delete set", + args: args{ + cfg: applyOnNeedCfg, + toCreateMetadatas: []*IPSetMetadata{namespaceSet}, + toDeleteName: namespaceSet.GetPrefixName(), + }, + expectedInfo: expectedInfo{ + mainCache: nil, + toAddUpdateCache: nil, + toDeleteCache: nil, + setsForKernel: nil, + }, + }, + { + name: "Apply Always: set doesn't exist", + args: args{ + cfg: applyAlwaysCfg, + toCreateMetadatas: []*IPSetMetadata{namespaceSet}, + toDeleteName: keyLabelOfPodSet.GetPrefixName(), + }, + expectedInfo: expectedInfo{ + mainCache: []setMembers{ + {metadata: namespaceSet, members: nil}, + }, + toAddUpdateCache: nil, + toDeleteCache: nil, + setsForKernel: nil, + }, + }, + { + name: "Apply On Need: set doesn't exist", + args: args{ + cfg: applyOnNeedCfg, + toCreateMetadatas: []*IPSetMetadata{namespaceSet}, + toDeleteName: keyLabelOfPodSet.GetPrefixName(), + }, + expectedInfo: expectedInfo{ + mainCache: []setMembers{ + {metadata: namespaceSet, members: nil}, + }, + toAddUpdateCache: nil, + toDeleteCache: nil, + setsForKernel: nil, + }, + }, + } - err := iMgr.AddToSets([]*IPSetMetadata{setMetadata}, testPodIP, testPodKey) - require.NoError(t, err) + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + metrics.ReinitializeAll() + var calls []testutils.TestCmd + if tt.args.cfg == applyAlwaysCfg { + calls = GetApplyIPSetsTestCalls(tt.args.toCreateMetadatas, nil) + } + ioShim := common.NewMockIOShim(calls) + defer ioShim.VerifyCalls(t, calls) + iMgr := NewIPSetManager(tt.args.cfg, ioShim) + iMgr.CreateIPSets(tt.args.toCreateMetadatas) + require.NoError(t, iMgr.ApplyIPSets()) + iMgr.DeleteIPSet(tt.args.toDeleteName) + assertExpectedInfo(t, iMgr, &tt.expectedInfo) + }) + } +} - err = iMgr.AddToSets([]*IPSetMetadata{setMetadata}, "2001:db8:0:0:0:0:2:1", "newpod") - require.NoError(t, err) +func TestDeleteIPSetNotAllowed(t *testing.T) { + // try to delete a list with a member and a set referenced in kernel (by a list) + // must use applyAlwaysCfg for set to be in kernel + // logic for ipset.canBeDeleted is tested elsewhere (in ipset_test.go) + metrics.ReinitializeAll() + calls := GetApplyIPSetsTestCalls([]*IPSetMetadata{list, namespaceSet}, nil) + ioShim := common.NewMockIOShim(calls) + defer ioShim.VerifyCalls(t, calls) + iMgr := NewIPSetManager(applyAlwaysCfg, ioShim) + require.NoError(t, iMgr.AddToLists([]*IPSetMetadata{list}, []*IPSetMetadata{namespaceSet})) + require.NoError(t, iMgr.ApplyIPSets()) + + iMgr.DeleteIPSet(namespaceSet.GetPrefixName()) + iMgr.DeleteIPSet(list.GetPrefixName()) + + assertExpectedInfo(t, iMgr, &expectedInfo{ + mainCache: []setMembers{ + {metadata: list, members: nil}, + {metadata: namespaceSet, members: nil}, + }, + toAddUpdateCache: nil, + toDeleteCache: nil, + setsForKernel: nil, + }) +} - // same IP changed podkey - err = iMgr.AddToSets([]*IPSetMetadata{setMetadata}, testPodIP, "newpod") - require.NoError(t, err) +func TestAddToSets(t *testing.T) { + // TODO test ip,port members, cidr members, and (if not done in controller) error throwing on invalid members + ipv4 := "1.2.3.4" + ipv6 := "2001:db8:0:0:0:0:2:1" + + type args struct { + cfg *IPSetManagerCfg + toCreateMetadatas []*IPSetMetadata + toAddMetadatas []*IPSetMetadata + member string + memberExistedPrior bool + hasDiffPodKey bool + } + tests := []struct { + name string + args args + expectedInfo expectedInfo + wantErr bool + }{ + { + name: "Apply Always: add new IP to 1 existing set and 1 non-existing set", + args: args{ + cfg: applyAlwaysCfg, + toCreateMetadatas: []*IPSetMetadata{namespaceSet}, + toAddMetadatas: []*IPSetMetadata{namespaceSet, keyLabelOfPodSet}, + member: ipv4, + }, + expectedInfo: expectedInfo{ + mainCache: []setMembers{ + {metadata: namespaceSet, members: []member{{ipv4, isHashMember}}}, + {metadata: keyLabelOfPodSet, members: []member{{ipv4, isHashMember}}}, + }, + toAddUpdateCache: []*IPSetMetadata{namespaceSet, keyLabelOfPodSet}, + toDeleteCache: nil, + setsForKernel: []*IPSetMetadata{namespaceSet, keyLabelOfPodSet}, + }, + wantErr: false, + }, + { + name: "Apply On Need: add to new set", + args: args{ + cfg: applyOnNeedCfg, + toCreateMetadatas: nil, + toAddMetadatas: []*IPSetMetadata{namespaceSet}, + member: ipv4, + }, + expectedInfo: expectedInfo{ + mainCache: []setMembers{ + {metadata: namespaceSet, members: []member{{ipv4, isHashMember}}}, + }, + toAddUpdateCache: nil, + toDeleteCache: nil, + setsForKernel: nil, + }, + wantErr: false, + }, + { + name: "add IPv6", + args: args{ + cfg: applyAlwaysCfg, + toCreateMetadatas: []*IPSetMetadata{namespaceSet}, + toAddMetadatas: []*IPSetMetadata{namespaceSet}, + member: ipv6, + }, + expectedInfo: expectedInfo{ + mainCache: []setMembers{ + {metadata: namespaceSet, members: []member{{ipv6, isHashMember}}}, + }, + toAddUpdateCache: []*IPSetMetadata{namespaceSet}, + toDeleteCache: nil, + setsForKernel: []*IPSetMetadata{namespaceSet}, + }, + wantErr: false, + }, + { + name: "add existing IP to set (same pod key)", + args: args{ + cfg: applyAlwaysCfg, + toCreateMetadatas: []*IPSetMetadata{namespaceSet}, + toAddMetadatas: []*IPSetMetadata{namespaceSet}, + member: ipv4, + memberExistedPrior: true, + hasDiffPodKey: false, + }, + expectedInfo: expectedInfo{ + mainCache: []setMembers{ + {metadata: namespaceSet, members: []member{{ipv4, isHashMember}}}, + }, + toAddUpdateCache: nil, + toDeleteCache: nil, + setsForKernel: []*IPSetMetadata{namespaceSet}, + }, + wantErr: false, + }, + { + name: "add existing IP to set (diff pod key)", + args: args{ + cfg: applyAlwaysCfg, + toCreateMetadatas: []*IPSetMetadata{namespaceSet}, + toAddMetadatas: []*IPSetMetadata{namespaceSet}, + member: ipv4, + memberExistedPrior: true, + hasDiffPodKey: false, + }, + expectedInfo: expectedInfo{ + mainCache: []setMembers{ + {metadata: namespaceSet, members: []member{{ipv4, isHashMember}}}, + }, + toAddUpdateCache: nil, + toDeleteCache: nil, + setsForKernel: []*IPSetMetadata{namespaceSet}, + }, + wantErr: false, + }, + { + name: "no-op for empty toAddMetadatas", + args: args{ + cfg: applyAlwaysCfg, + toCreateMetadatas: nil, + toAddMetadatas: nil, + }, + expectedInfo: expectedInfo{}, + }, + { + // NOTE: we create the list and consider it dirty (because of the creation) even though an "add" error occurs + name: "Apply Always: error on add to list", + args: args{ + cfg: applyAlwaysCfg, + toCreateMetadatas: nil, + toAddMetadatas: []*IPSetMetadata{list}, + member: ipv4, + }, + expectedInfo: expectedInfo{ + mainCache: []setMembers{ + {metadata: list, members: nil}, + }, + toAddUpdateCache: []*IPSetMetadata{list}, + toDeleteCache: nil, + setsForKernel: []*IPSetMetadata{list}, + }, + wantErr: true, + }, + { + // NOTE: we create the list even though an "add" error occurs + name: "Apply On need: error on add to list", + args: args{ + cfg: applyOnNeedCfg, + toCreateMetadatas: nil, + toAddMetadatas: []*IPSetMetadata{list}, + member: ipv4, + }, + expectedInfo: expectedInfo{ + mainCache: []setMembers{ + {metadata: list, members: nil}, + }, + toAddUpdateCache: nil, + toDeleteCache: nil, + setsForKernel: nil, + }, + wantErr: true, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + metrics.ReinitializeAll() + var calls []testutils.TestCmd + if tt.args.cfg == applyAlwaysCfg { + calls = GetApplyIPSetsTestCalls(tt.args.toCreateMetadatas, nil) + } + ioShim := common.NewMockIOShim(calls) + defer ioShim.VerifyCalls(t, calls) + iMgr := NewIPSetManager(tt.args.cfg, ioShim) + iMgr.CreateIPSets(tt.args.toCreateMetadatas) + + podKey := "pod-a" + otherPodKey := "pod-b" + if tt.args.memberExistedPrior { + require.NoError(t, iMgr.AddToSets(tt.args.toAddMetadatas, tt.args.member, podKey)) + } + require.NoError(t, iMgr.ApplyIPSets()) + k := podKey + if tt.args.hasDiffPodKey { + k = otherPodKey + } + err := iMgr.AddToSets(tt.args.toAddMetadatas, tt.args.member, k) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + assertExpectedInfo(t, iMgr, &tt.expectedInfo) + }) + } +} - listMetadata := NewIPSetMetadata("testipsetlist", KeyLabelOfNamespace) - iMgr.CreateIPSets([]*IPSetMetadata{listMetadata}) - err = iMgr.AddToSets([]*IPSetMetadata{listMetadata}, testPodIP, testPodKey) - require.Error(t, err) +func TestAddToSetInKernelApplyOnNeed(t *testing.T) { + tests := []struct { + name string + metadata *IPSetMetadata + wantDirty bool + wantErr bool + }{ + { + name: "success", + metadata: namespaceSet, + wantDirty: true, + wantErr: false, + }, + { + name: "failure", + metadata: list, + wantDirty: false, + wantErr: true, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + podKey := "pod-a" + ipv4 := "1.2.3.4" + metadatas := []*IPSetMetadata{tt.metadata} + + metrics.ReinitializeAll() + calls := GetApplyIPSetsTestCalls(metadatas, nil) + ioShim := common.NewMockIOShim(calls) + 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.ApplyIPSets()) + + err := iMgr.AddToSets(metadatas, ipv4, podKey) + var members []member + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + members = []member{{ipv4, isHashMember}} + } + var dirtySets []*IPSetMetadata + if tt.wantDirty { + dirtySets = []*IPSetMetadata{namespaceSet} + } + assertExpectedInfo(t, iMgr, &expectedInfo{ + mainCache: []setMembers{ + {metadata: tt.metadata, members: members}, + }, + toAddUpdateCache: dirtySets, + toDeleteCache: nil, + setsForKernel: []*IPSetMetadata{tt.metadata}, + }) + }) + } } -func TestRemoveFromSet(t *testing.T) { - iMgr := NewIPSetManager(applyOnNeedCfg, common.NewMockIOShim([]testutils.TestCmd{})) +func TestRemoveFromSets(t *testing.T) { + calls := []testutils.TestCmd{} + ioShim := common.NewMockIOShim(calls) + defer ioShim.VerifyCalls(t, calls) + iMgr := NewIPSetManager(applyOnNeedCfg, ioShim) setMetadata := NewIPSetMetadata(testSetName, Namespace) iMgr.CreateIPSets([]*IPSetMetadata{setMetadata}) @@ -121,11 +689,11 @@ func TestAddToList(t *testing.T) { require.NoError(t, err) set := iMgr.GetIPSet(listMetadata.GetPrefixName()) - assert.NotNil(t, set) - assert.Equal(t, listMetadata.GetPrefixName(), set.Name) - assert.Equal(t, util.GetHashedName(listMetadata.GetPrefixName()), set.HashedName) - assert.Equal(t, 1, len(set.MemberIPSets)) - assert.Equal(t, setMetadata.GetPrefixName(), set.MemberIPSets[setMetadata.GetPrefixName()].Name) + require.NotNil(t, set) + require.Equal(t, listMetadata.GetPrefixName(), set.Name) + require.Equal(t, util.GetHashedName(listMetadata.GetPrefixName()), set.HashedName) + require.Equal(t, 1, len(set.MemberIPSets)) + require.Equal(t, setMetadata.GetPrefixName(), set.MemberIPSets[setMetadata.GetPrefixName()].Name) } func TestRemoveFromList(t *testing.T) { @@ -138,18 +706,18 @@ func TestRemoveFromList(t *testing.T) { require.NoError(t, err) set := iMgr.GetIPSet(listMetadata.GetPrefixName()) - assert.NotNil(t, set) - assert.Equal(t, listMetadata.GetPrefixName(), set.Name) - assert.Equal(t, util.GetHashedName(listMetadata.GetPrefixName()), set.HashedName) - assert.Equal(t, 1, len(set.MemberIPSets)) - assert.Equal(t, setMetadata.GetPrefixName(), set.MemberIPSets[setMetadata.GetPrefixName()].Name) + require.NotNil(t, set) + require.Equal(t, listMetadata.GetPrefixName(), set.Name) + require.Equal(t, util.GetHashedName(listMetadata.GetPrefixName()), set.HashedName) + require.Equal(t, 1, len(set.MemberIPSets)) + require.Equal(t, setMetadata.GetPrefixName(), set.MemberIPSets[setMetadata.GetPrefixName()].Name) err = iMgr.RemoveFromList(listMetadata, []*IPSetMetadata{setMetadata}) require.NoError(t, err) set = iMgr.GetIPSet(listMetadata.GetPrefixName()) - assert.NotNil(t, set) - assert.Equal(t, 0, len(set.MemberIPSets)) + require.NotNil(t, set) + require.Equal(t, 0, len(set.MemberIPSets)) } func TestRemoveFromListMissing(t *testing.T) { @@ -162,16 +730,6 @@ func TestRemoveFromListMissing(t *testing.T) { err := iMgr.RemoveFromList(listMetadata, []*IPSetMetadata{setMetadata}) require.NoError(t, err) } - -func TestDeleteIPSet(t *testing.T) { - iMgr := NewIPSetManager(applyOnNeedCfg, common.NewMockIOShim([]testutils.TestCmd{})) - setMetadata := NewIPSetMetadata(testSetName, Namespace) - iMgr.CreateIPSets([]*IPSetMetadata{setMetadata}) - - iMgr.DeleteIPSet(setMetadata.GetPrefixName()) - // TODO add cache check -} - func TestGetIPsFromSelectorIPSets(t *testing.T) { iMgr := NewIPSetManager(applyOnNeedCfg, common.NewMockIOShim([]testutils.TestCmd{})) setsTocreate := []*IPSetMetadata{ @@ -211,14 +769,14 @@ func TestGetIPsFromSelectorIPSets(t *testing.T) { ips, err := iMgr.GetIPsFromSelectorIPSets(ipsetList) require.NoError(t, err) - assert.Equal(t, 2, len(ips)) + require.Equal(t, 2, len(ips)) expectedintersection := map[string]struct{}{ "10.0.0.1": {}, "10.0.0.2": {}, } - assert.Equal(t, ips, expectedintersection) + require.Equal(t, ips, expectedintersection) } func TestAddDeleteSelectorReferences(t *testing.T) { @@ -263,8 +821,8 @@ func TestAddDeleteSelectorReferences(t *testing.T) { require.NoError(t, err) } - assert.Equal(t, 5, len(iMgr.toAddOrUpdateCache)) - assert.Equal(t, 0, len(iMgr.toDeleteCache)) + 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) @@ -273,8 +831,8 @@ func TestAddDeleteSelectorReferences(t *testing.T) { } } - assert.Equal(t, 0, len(iMgr.toAddOrUpdateCache)) - assert.Equal(t, 5, len(iMgr.toDeleteCache)) + require.Equal(t, 0, len(iMgr.toAddOrUpdateCache)) + require.Equal(t, 5, len(iMgr.toDeleteCache)) for _, v := range setsTocreate { iMgr.DeleteIPSet(v.GetPrefixName()) @@ -282,7 +840,7 @@ func TestAddDeleteSelectorReferences(t *testing.T) { // Above delete will not remove setpod3 and setpod4 // because they are referencing each other - assert.Equal(t, 2, len(iMgr.setMap)) + require.Equal(t, 2, len(iMgr.setMap)) err = iMgr.RemoveFromList(setsTocreate[3], []*IPSetMetadata{setsTocreate[4]}) require.NoError(t, err) @@ -293,7 +851,7 @@ func TestAddDeleteSelectorReferences(t *testing.T) { for _, v := range setsTocreate { set := iMgr.GetIPSet(v.GetPrefixName()) - assert.Nil(t, set) + require.Nil(t, set) } } @@ -332,15 +890,15 @@ func TestAddDeleteNetPolReferences(t *testing.T) { require.NoError(t, err) } - assert.Equal(t, 5, len(iMgr.toAddOrUpdateCache)) - assert.Equal(t, 0, len(iMgr.toDeleteCache)) + 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) } - assert.Equal(t, 0, len(iMgr.toAddOrUpdateCache)) - assert.Equal(t, 5, len(iMgr.toDeleteCache)) + require.Equal(t, 0, len(iMgr.toAddOrUpdateCache)) + require.Equal(t, 5, len(iMgr.toDeleteCache)) for _, v := range setsTocreate { iMgr.DeleteIPSet(v.GetPrefixName()) @@ -348,7 +906,7 @@ func TestAddDeleteNetPolReferences(t *testing.T) { // Above delete will not remove setpod3 and setpod4 // because they are referencing each other - assert.Equal(t, 2, len(iMgr.setMap)) + require.Equal(t, 2, len(iMgr.setMap)) err = iMgr.RemoveFromList(setsTocreate[3], []*IPSetMetadata{setsTocreate[4]}) require.NoError(t, err) @@ -359,7 +917,7 @@ func TestAddDeleteNetPolReferences(t *testing.T) { for _, v := range setsTocreate { set := iMgr.GetIPSet(v.GetPrefixName()) - assert.Nil(t, set) + require.Nil(t, set) } for _, v := range setsTocreate { @@ -375,3 +933,92 @@ func TestMain(m *testing.M) { os.Exit(exitCode) } + +func assertExpectedInfo(t *testing.T, iMgr *IPSetManager, info *expectedInfo) { + // 1. assert cache contents + require.Equal(t, len(info.mainCache), len(iMgr.setMap), "main cache size mismatch") + for _, setMembers := range info.mainCache { + setName := setMembers.metadata.GetPrefixName() + require.True(t, iMgr.exists(setName), "set %s not found in main cache", setName) + 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") + 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) + } else { + _, ok := set.MemberIPSets[member.value] + require.True(t, ok, "set member %s not found in list %s", member.value, setName) + } + } + } + + require.Equal(t, len(info.toAddUpdateCache), len(iMgr.toAddOrUpdateCache), "toAddUpdateCache size mismatch") + for _, setMetadata := range info.toAddUpdateCache { + setName := setMetadata.GetPrefixName() + _, ok := iMgr.toAddOrUpdateCache[setName] + require.True(t, ok, "set %s not in the toAddUpdateCache") + require.True(t, iMgr.exists(setName), "set %s not in the main cache but is in the toAddUpdateCache", setName) + } + + 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) + } + + for _, setMetadata := range info.setsForKernel { + setName := setMetadata.GetPrefixName() + require.True(t, iMgr.exists(setName), "kernel set %s not found", setName) + set := iMgr.setMap[setName] + require.True(t, iMgr.shouldBeInKernel(set), "set %s should be in kernel", setName) + } + + // 2. assert prometheus metrics + // at this point, the expected cache/kernel is the same as the actual cache/kernel + numIPSetsInKernel, err := metrics.GetNumIPSets() + promutil.NotifyIfErrors(t, err) + fmt.Println(numIPSetsInKernel) + // TODO uncomment and remove print statement when we have prometheus metric for in kernel + // require.Equal(t, len(info.setsInKernel), numIPSetsInKernel, "numIPSetsInKernel mismatch") + + // TODO update get function when we have prometheus metric for in kernel + numIPSetsInCache, err := metrics.GetNumIPSets() + promutil.NotifyIfErrors(t, err) + require.Equal(t, len(iMgr.setMap), numIPSetsInCache, "numIPSetsInCache mismatch") + + // the setMap is equal + expectedNumEntriesInCache := 0 + expectedNumEntriesInKernel := 0 + for _, set := range iMgr.setMap { + // one of IPPodKey or MemberIPSets should be nil + expectedNumEntriesInCache += len(set.IPPodKey) + len(set.MemberIPSets) + if iMgr.shouldBeInKernel(set) { + expectedNumEntriesInKernel += len(set.IPPodKey) + len(set.MemberIPSets) + } + } + + numEntriesInKernel, err := metrics.GetNumIPSetEntries() + promutil.NotifyIfErrors(t, err) + fmt.Println(numEntriesInKernel) + // TODO uncomment and remove print statement when we have prometheus metric for in kernel + // require.Equal(t, expectedNumEntriesInKernel, numEntriesInKernel, "numEntriesInKernel mismatch") + + // TODO update get func when we have prometheus metric for in kernel + numEntriesInCache, err := metrics.GetNumIPSetEntries() + promutil.NotifyIfErrors(t, err) + require.Equal(t, expectedNumEntriesInCache, numEntriesInCache) + for _, set := range iMgr.setMap { + expectedNumEntries := 0 + // TODO replace bool with iMgr.shouldBeInKernel(set) when we have prometheus metric for in kernel + if set.Name != "pizza" { + // one of IPPodKey or MemberIPSets should be nil + expectedNumEntries = len(set.IPPodKey) + len(set.MemberIPSets) + } + numEntries, err := metrics.GetNumEntriesForIPSet(set.Name) + promutil.NotifyIfErrors(t, err) + require.Equal(t, expectedNumEntries, numEntries, "numEntries mismatch for set %s", set.Name) + } +} diff --git a/npm/pkg/dataplane/ipsets/testutils_linux.go b/npm/pkg/dataplane/ipsets/testutils_linux.go index 0b063fb215..2189a2d0fb 100644 --- a/npm/pkg/dataplane/ipsets/testutils_linux.go +++ b/npm/pkg/dataplane/ipsets/testutils_linux.go @@ -20,6 +20,8 @@ func GetApplyIPSetsTestCalls(toAddOrUpdateIPSets, toDeleteIPSets []*IPSetMetadat {Cmd: []string{"grep", "azure-npm-"}, ExitCode: 1}, // grep didn't find anything {Cmd: ipsetRestoreStringSlice}, } + } else if len(toDeleteIPSets) == 0 { + return []testutils.TestCmd{} } return []testutils.TestCmd{fakeRestoreSuccessCommand} } diff --git a/npm/pkg/dataplane/policies/policymanager.go b/npm/pkg/dataplane/policies/policymanager.go index 13e0e90b3a..d132615dd9 100644 --- a/npm/pkg/dataplane/policies/policymanager.go +++ b/npm/pkg/dataplane/policies/policymanager.go @@ -6,6 +6,7 @@ import ( "time" "github.com/Azure/azure-container-networking/common" + "github.com/Azure/azure-container-networking/npm/metrics" npmerrors "github.com/Azure/azure-container-networking/npm/util/errors" "k8s.io/klog" ) @@ -93,10 +94,12 @@ func (pMgr *PolicyManager) GetPolicy(policyKey string) (*NPMNetworkPolicy, bool) } func (pMgr *PolicyManager) AddPolicy(policy *NPMNetworkPolicy, endpointList map[string]string) error { + prometheusTimer := metrics.StartNewTimer() if len(policy.ACLs) == 0 { klog.Infof("[DataPlane] No ACLs in policy %s to apply", policy.PolicyKey) return nil } + defer metrics.RecordACLRuleExecTime(prometheusTimer) // record execution time regardless of failure normalizePolicy(policy) if err := validatePolicy(policy); err != nil { return npmerrors.Errorf(npmerrors.AddPolicy, false, fmt.Sprintf("couldn't add malformed policy: %s", err.Error())) diff --git a/npm/pkg/dataplane/policies/policymanager_test.go b/npm/pkg/dataplane/policies/policymanager_test.go index 6950422de3..8f8ea31978 100644 --- a/npm/pkg/dataplane/policies/policymanager_test.go +++ b/npm/pkg/dataplane/policies/policymanager_test.go @@ -1,9 +1,11 @@ package policies import ( + "os" "testing" "github.com/Azure/azure-container-networking/common" + "github.com/Azure/azure-container-networking/npm/metrics" "github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets" "github.com/stretchr/testify/require" ) @@ -167,3 +169,11 @@ func TestNormalizeAndValidatePolicy(t *testing.T) { }) } } + +func TestMain(m *testing.M) { + metrics.InitializeAll() + + exitCode := m.Run() + + os.Exit(exitCode) +}