From dc21ec2fff7d65bc5e0dc2bb28d6c98119bbfc8c Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Tue, 18 Jan 2022 14:13:21 -0800 Subject: [PATCH 01/10] wip --- npm/metrics/prometheus-metrics.go | 26 +- npm/pkg/dataplane/ipsets/ipset_test.go | 134 +++++++++ npm/pkg/dataplane/ipsets/ipsetmanager.go | 173 ++++++----- npm/pkg/dataplane/ipsets/ipsetmanager_test.go | 279 +++++++++++++++--- 4 files changed, 491 insertions(+), 121 deletions(-) create mode 100644 npm/pkg/dataplane/ipsets/ipset_test.go diff --git a/npm/metrics/prometheus-metrics.go b/npm/metrics/prometheus-metrics.go index cca3ea165d..d623bb48a6 100644 --- a/npm/metrics/prometheus-metrics.go +++ b/npm/metrics/prometheus-metrics.go @@ -62,21 +62,27 @@ var ( ) // InitializeAll creates all the Prometheus Metrics. The metrics will be nil before this method is called. +// This function will only initialize the metrics once no matter how many times it is called. func InitializeAll() { if !haveInitialized { - numPolicies = createGauge(numPoliciesName, numPoliciesHelp, false) - addPolicyExecTime = createSummary(addPolicyExecTimeName, addPolicyExecTimeHelp, true) - numACLRules = createGauge(numACLRulesName, numACLRulesHelp, false) - addACLRuleExecTime = createSummary(addACLRuleExecTimeName, addACLRuleExecTimeHelp, true) - numIPSets = createGauge(numIPSetsName, numIPSetsHelp, false) - addIPSetExecTime = createSummary(addIPSetExecTimeName, addIPSetExecTimeHelp, true) - numIPSetEntries = createGauge(numIPSetEntriesName, numIPSetEntriesHelp, false) - ipsetInventory = createGaugeVec(ipsetInventoryName, ipsetInventoryHelp, false, setNameLabel, setHashLabel) - log.Logf("Finished initializing all Prometheus metrics") - haveInitialized = true + ReinitializeAll() } } +// ReinitializeAll creates new Prometheus metrics. +func ReinitializeAll() { + numPolicies = createGauge(numPoliciesName, numPoliciesHelp, false) + addPolicyExecTime = createSummary(addPolicyExecTimeName, addPolicyExecTimeHelp, true) + numACLRules = createGauge(numACLRulesName, numACLRulesHelp, false) + addACLRuleExecTime = createSummary(addACLRuleExecTimeName, addACLRuleExecTimeHelp, true) + numIPSets = createGauge(numIPSetsName, numIPSetsHelp, false) + addIPSetExecTime = createSummary(addIPSetExecTimeName, addIPSetExecTimeHelp, true) + numIPSetEntries = createGauge(numIPSetEntriesName, numIPSetEntriesHelp, false) + ipsetInventory = createGaugeVec(ipsetInventoryName, ipsetInventoryHelp, false, setNameLabel, setHashLabel) + log.Logf("Finished initializing all Prometheus metrics") + haveInitialized = true +} + // 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/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 f2d5b10bf9..adc52b3fb8 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,7 @@ 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 return nil } @@ -97,7 +96,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 } @@ -174,12 +174,23 @@ func (iMgr *IPSetManager) AddToSets(addToSets []*IPSetMetadata, ip, podKey strin iMgr.Lock() defer iMgr.Unlock() - if err := iMgr.checkForIPUpdateErrors(addToSets, npmerrors.AppendIPSet); err != nil { - return err + // 1. check for errors and create any missing sets + for _, metadata := range addToSets { + 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() + // 2. add ip to to all sets + for _, metadata := range addToSets { + prefixedName := metadata.GetPrefixName() set := iMgr.setMap[prefixedName] cachedPodKey, ok := set.IPPodKey[ip] set.IPPodKey[ip] = podKey @@ -199,13 +210,25 @@ func (iMgr *IPSetManager) RemoveFromSets(removeFromSets []*IPSetMetadata, ip, po iMgr.Lock() defer iMgr.Unlock() - if err := iMgr.checkForIPUpdateErrors(removeFromSets, npmerrors.DeleteIPSet); err != nil { - return err + // 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)) + } } - for _, setMetadata := range removeFromSets { - prefixedName := setMetadata.GetPrefixName() - set := iMgr.setMap[prefixedName] + // 2. remove ip from all existing sets + for _, metadata := range removeFromSets { + prefixedName := metadata.GetPrefixName() + set, exists := iMgr.setMap[prefixedName] + if !exists { + continue + } // in case the IP belongs to a new Pod, then ignore this Delete call as this might be stale cachedPodKey, exists := set.IPPodKey[ip] @@ -230,10 +253,46 @@ func (iMgr *IPSetManager) AddToLists(listMetadatas, setMetadatas []*IPSetMetadat iMgr.Lock() defer iMgr.Unlock() - if err := iMgr.checkForListMemberUpdateErrors(listMetadatas, setMetadatas, npmerrors.AppendIPSet); err != nil { - return err + // 1. check for errors in lists and create any missing lists + for _, listMetadata := range listMetadatas { + 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] + } + + if list.Kind != ListSet { + return npmerrors.Errorf(npmerrors.AppendIPSet, false, fmt.Sprintf("ipset %s is not a list set", listName)) + } + + for _, memberMetadata := range setMetadatas { + memberName := memberMetadata.GetPrefixName() + if listName == memberName { + return npmerrors.Errorf(npmerrors.AppendIPSet, false, fmt.Sprintf("ipset %s cannot be added to itself", listName)) + } + } + } + + // 2. 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)) + } } + // 3. add sets to all lists for _, listMetadata := range listMetadatas { listName := listMetadata.GetPrefixName() for _, setMetadata := range setMetadatas { @@ -241,7 +300,6 @@ func (iMgr *IPSetManager) AddToLists(listMetadatas, setMetadatas []*IPSetMetadat iMgr.addMemberIPSet(listName, setName) } iMgr.modifyCacheForKernelMemberUpdate(listName) - metrics.AddEntryToIPSet(listName) } return nil } @@ -250,17 +308,35 @@ func (iMgr *IPSetManager) RemoveFromList(listMetadata *IPSetMetadata, setMetadat iMgr.Lock() defer iMgr.Unlock() - if err := iMgr.checkForListMemberUpdateErrors([]*IPSetMetadata{listMetadata}, setMetadatas, npmerrors.DeleteIPSet); err != nil { - return err + listName := listMetadata.GetPrefixName() + list, exists := iMgr.setMap[listName] + if !exists { + return nil + } + + if list.Kind != ListSet { + return npmerrors.Errorf(npmerrors.DeleteIPSet, false, fmt.Sprintf("ipset %s is not a list set", listName)) + } + + for _, setMetadata := range setMetadatas { + setName := setMetadata.GetPrefixName() + set, exists := iMgr.setMap[setName] + if !exists { + continue + } + + // Nested IPSets are only supported for windows + // Check if we want to actually use that support + if set.Kind != HashSet { + return npmerrors.Errorf(npmerrors.DeleteIPSet, false, fmt.Sprintf("ipset %s is not a hash set and nested list sets are not supported", setName)) + } } - listName := listMetadata.GetPrefixName() for _, setMetadata := range setMetadatas { setName := setMetadata.GetPrefixName() iMgr.removeMemberIPSet(listName, setName) } iMgr.modifyCacheForKernelMemberUpdate(listName) - metrics.RemoveEntryFromIPSet(listName) return nil } @@ -383,72 +459,13 @@ 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] 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) { diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go index 34d77a13f0..4f6ea3201a 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go @@ -31,36 +31,228 @@ var ( } ) -func TestCreateIPSet(t *testing.T) { - iMgr := NewIPSetManager(applyOnNeedCfg, common.NewMockIOShim([]testutils.TestCmd{})) +type cacheValues struct { + mainCache []string + addUpdateCacheMembers map[string][]member + toDeleteCache []string +} - setMetadata := NewIPSetMetadata(testSetName, Namespace) - iMgr.CreateIPSets([]*IPSetMetadata{setMetadata}) - // creating twice - iMgr.CreateIPSets([]*IPSetMetadata{setMetadata}) +type member struct { + // either an IP or set name + value string + kind memberKind +} - assert.True(t, iMgr.exists(setMetadata.GetPrefixName())) +type memberKind bool - 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) -} +const ( + isIP = memberKind(true) + isSet = memberKind(false) +) -func TestCreateIPSetApplyAlways(t *testing.T) { - iMgr := NewIPSetManager(applyAlwaysCfg, common.NewMockIOShim([]testutils.TestCmd{})) +func TestCreateIPSet(t *testing.T) { + m1 := NewIPSetMetadata(testSetName, Namespace) + m2 := NewIPSetMetadata(testListName, KeyLabelOfNamespace) + tests := []struct { + name string + cfg *IPSetManagerCfg + metadatas []*IPSetMetadata + expectedCache cacheValues + expectedNumIPSets int + expectedNumIPSetsInKernel int + }{ + { + name: "Apply Always: create two new sets", + cfg: applyAlwaysCfg, + metadatas: []*IPSetMetadata{m1, m2}, + expectedCache: cacheValues{ + mainCache: []string{m1.GetPrefixName(), m2.GetPrefixName()}, + addUpdateCacheMembers: map[string][]member{ + m1.GetPrefixName(): {}, + m2.GetPrefixName(): {}, + }, + toDeleteCache: []string{}, + }, + expectedNumIPSets: 2, + expectedNumIPSetsInKernel: 2, + }, + { + name: "Apply On Need: create two new sets", + cfg: applyOnNeedCfg, + metadatas: []*IPSetMetadata{m1, m2}, + expectedCache: cacheValues{ + mainCache: []string{m1.GetPrefixName(), m2.GetPrefixName()}, + addUpdateCacheMembers: map[string][]member{}, + toDeleteCache: []string{}, + }, + expectedNumIPSets: 2, + expectedNumIPSetsInKernel: 0, + }, + { + name: "Apply Always: no-op for set that exists", + cfg: applyAlwaysCfg, + metadatas: []*IPSetMetadata{m1, m1}, + expectedCache: cacheValues{ + mainCache: []string{m1.GetPrefixName()}, + addUpdateCacheMembers: map[string][]member{ + m1.GetPrefixName(): {}, + }, + toDeleteCache: []string{}, + }, + expectedNumIPSets: 1, + expectedNumIPSetsInKernel: 1, + }, + { + name: "Apply On Need: no-op for set that exists", + cfg: applyOnNeedCfg, + metadatas: []*IPSetMetadata{m1, m1}, + expectedCache: cacheValues{ + mainCache: []string{m1.GetPrefixName()}, + addUpdateCacheMembers: map[string][]member{}, + toDeleteCache: []string{}, + }, + expectedNumIPSets: 1, + expectedNumIPSetsInKernel: 0, + }, + } - setMetadata := NewIPSetMetadata(testSetName, Namespace) - iMgr.CreateIPSets([]*IPSetMetadata{setMetadata}) - // creating twice - iMgr.CreateIPSets([]*IPSetMetadata{setMetadata}) + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + metrics.ReinitializeAll() + iMgr := NewIPSetManager(tt.cfg, common.NewMockIOShim(nil)) + iMgr.CreateIPSets(tt.metadatas) + + assertEqualCache(t, iMgr, tt.expectedCache) + + numIPSets, _ := metrics.GetNumIPSets() + assert.Equal(t, tt.expectedNumIPSets, numIPSets) + // TODO update when we have prometheus metric for in kernel + numIPSetsInKernel := tt.expectedNumIPSetsInKernel + assert.Equal(t, tt.expectedNumIPSetsInKernel, numIPSetsInKernel) + numEntries, _ := metrics.GetNumIPSetEntries() + assert.Equal(t, 0, numEntries) + for _, setName := range tt.expectedCache.mainCache { + numEntries, _ := metrics.GetNumEntriesForIPSet(setName) + assert.Equal(t, 0, numEntries) + } + }) + } +} + +func TestDeleteIPSet(t *testing.T) { + m1 := NewIPSetMetadata(testSetName, Namespace) + m2 := NewIPSetMetadata(testListName, KeyLabelOfNamespace) + tests := []struct { + name string + cfg *IPSetManagerCfg + toCreateMetadatas []*IPSetMetadata + toDeleteName string + expectedCache cacheValues + expectedNumIPSets int + }{ + { + name: "Apply Always: delete set", + cfg: applyAlwaysCfg, + toCreateMetadatas: []*IPSetMetadata{m1}, + toDeleteName: m1.GetPrefixName(), + expectedCache: cacheValues{ + mainCache: []string{}, + addUpdateCacheMembers: map[string][]member{}, + toDeleteCache: []string{m1.GetPrefixName()}, + }, + expectedNumIPSets: 0, + }, + { + name: "Apply On Need: delete set", + cfg: applyOnNeedCfg, + toCreateMetadatas: []*IPSetMetadata{m1}, + toDeleteName: m1.GetPrefixName(), + expectedCache: cacheValues{ + mainCache: []string{}, + addUpdateCacheMembers: map[string][]member{}, + toDeleteCache: []string{}, + }, + expectedNumIPSets: 0, + }, + { + name: "Apply Always: set doesn't exist", + cfg: applyAlwaysCfg, + toCreateMetadatas: []*IPSetMetadata{m1}, + toDeleteName: m2.GetPrefixName(), + expectedCache: cacheValues{ + mainCache: []string{m1.GetPrefixName()}, + addUpdateCacheMembers: map[string][]member{}, + toDeleteCache: []string{}, + }, + expectedNumIPSets: 1, + }, + { + name: "Apply On Need: set doesn't exist", + cfg: applyOnNeedCfg, + toCreateMetadatas: []*IPSetMetadata{m1}, + toDeleteName: m2.GetPrefixName(), + expectedCache: cacheValues{ + mainCache: []string{m1.GetPrefixName()}, + addUpdateCacheMembers: map[string][]member{}, + toDeleteCache: []string{}, + }, + expectedNumIPSets: 1, + }, + } - assert.True(t, iMgr.exists(setMetadata.GetPrefixName())) + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + metrics.ReinitializeAll() + calls := GetApplyIPSetsTestCalls(tt.toCreateMetadatas, nil) + iMgr := NewIPSetManager(tt.cfg, common.NewMockIOShim(calls)) + iMgr.CreateIPSets(tt.toCreateMetadatas) + iMgr.ApplyIPSets() + iMgr.DeleteIPSet(tt.toDeleteName) + + assertEqualCache(t, iMgr, tt.expectedCache) + + numIPSets, _ := metrics.GetNumIPSets() + assert.Equal(t, tt.expectedNumIPSets, numIPSets) + // TODO update when we have prometheus metric for in kernel + numIPSetsInKernel := 0 + assert.Equal(t, 0, numIPSetsInKernel) + numEntries, _ := metrics.GetNumIPSetEntries() + assert.Equal(t, 0, numEntries) + numEntries, _ = metrics.GetNumEntriesForIPSet(tt.toDeleteName) + assert.Equal(t, 0, numEntries) + }) + } +} - 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 TestDeleteIPSetNotAllowed(t *testing.T) { + metrics.ReinitializeAll() + m := NewIPSetMetadata(testSetName, Namespace) + l := NewIPSetMetadata(testListName, KeyLabelOfNamespace) + calls := GetApplyIPSetsTestCalls([]*IPSetMetadata{l, m}, nil) + iMgr := NewIPSetManager(applyOnNeedCfg, common.NewMockIOShim(calls)) + iMgr.AddToLists([]*IPSetMetadata{l}, []*IPSetMetadata{m}) + iMgr.ApplyIPSets() + iMgr.DeleteIPSet(m.GetPrefixName()) + + assertEqualCache(t, iMgr, cacheValues{ + mainCache: []string{l.GetPrefixName(), m.GetPrefixName()}, + addUpdateCacheMembers: map[string][]member{}, + toDeleteCache: []string{}, + }) + + numIPSets, _ := metrics.GetNumIPSets() + assert.Equal(t, 2, numIPSets) + // TODO update when we have prometheus metric for in kernel + numIPSetsInKernel := 0 + assert.Equal(t, 0, numIPSetsInKernel) + numEntries, _ := metrics.GetNumIPSetEntries() + assert.Equal(t, 1, numEntries) + numEntries, _ = metrics.GetNumEntriesForIPSet(m.GetPrefixName()) + assert.Equal(t, 0, numEntries) + numEntries, _ = metrics.GetNumEntriesForIPSet(l.GetPrefixName()) + assert.Equal(t, 1, numEntries) } func TestAddToSet(t *testing.T) { @@ -162,16 +354,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{ @@ -375,3 +557,34 @@ func TestMain(m *testing.M) { os.Exit(exitCode) } + +func assertEqualCache(t *testing.T, iMgr *IPSetManager, cache cacheValues) { + require.Equal(t, len(cache.mainCache), len(iMgr.setMap)) + for _, setName := range cache.mainCache { + require.True(t, iMgr.exists(setName)) + set := iMgr.GetIPSet(setName) + require.NotNil(t, set) + assert.Equal(t, util.GetHashedName(setName), set.HashedName) + } + + require.Equal(t, len(cache.addUpdateCacheMembers), len(iMgr.toAddOrUpdateCache)) + for setName, members := range cache.addUpdateCacheMembers { + require.True(t, iMgr.exists(setName)) + for _, member := range members { + set := iMgr.setMap[setName] + if member.kind == isIP { + _, ok := set.IPPodKey[member.value] + require.True(t, ok) + } else { + _, ok := set.MemberIPSets[member.value] + require.True(t, ok) + } + } + } + + require.Equal(t, len(cache.toDeleteCache), len(iMgr.toDeleteCache)) + for _, setName := range cache.toDeleteCache { + _, ok := iMgr.toDeleteCache[setName] + require.True(t, ok) + } +} From fe81c7946dc9edd2be487885cb8f2648be1be662 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Tue, 25 Jan 2022 17:54:29 -0800 Subject: [PATCH 02/10] dont touch v1 metrics code and fix lints --- npm/metrics/prometheus-metrics.go | 26 +++++++++---------- npm/pkg/dataplane/ipsets/ipsetmanager_test.go | 6 ++--- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/npm/metrics/prometheus-metrics.go b/npm/metrics/prometheus-metrics.go index d623bb48a6..2a5bc04fc8 100644 --- a/npm/metrics/prometheus-metrics.go +++ b/npm/metrics/prometheus-metrics.go @@ -62,25 +62,25 @@ var ( ) // InitializeAll creates all the Prometheus Metrics. The metrics will be nil before this method is called. -// This function will only initialize the metrics once no matter how many times it is called. func InitializeAll() { if !haveInitialized { - ReinitializeAll() + numPolicies = createGauge(numPoliciesName, numPoliciesHelp, false) + addPolicyExecTime = createSummary(addPolicyExecTimeName, addPolicyExecTimeHelp, true) + numACLRules = createGauge(numACLRulesName, numACLRulesHelp, false) + addACLRuleExecTime = createSummary(addACLRuleExecTimeName, addACLRuleExecTimeHelp, true) + numIPSets = createGauge(numIPSetsName, numIPSetsHelp, false) + addIPSetExecTime = createSummary(addIPSetExecTimeName, addIPSetExecTimeHelp, true) + numIPSetEntries = createGauge(numIPSetEntriesName, numIPSetEntriesHelp, false) + ipsetInventory = createGaugeVec(ipsetInventoryName, ipsetInventoryHelp, false, setNameLabel, setHashLabel) + log.Logf("Finished initializing all Prometheus metrics") + haveInitialized = true } } -// ReinitializeAll creates new Prometheus metrics. +// ReinitializeAll creates/replaces Prometheus metrics. This function is intended for UTs. func ReinitializeAll() { - numPolicies = createGauge(numPoliciesName, numPoliciesHelp, false) - addPolicyExecTime = createSummary(addPolicyExecTimeName, addPolicyExecTimeHelp, true) - numACLRules = createGauge(numACLRulesName, numACLRulesHelp, false) - addACLRuleExecTime = createSummary(addACLRuleExecTimeName, addACLRuleExecTimeHelp, true) - numIPSets = createGauge(numIPSetsName, numIPSetsHelp, false) - addIPSetExecTime = createSummary(addIPSetExecTimeName, addIPSetExecTimeHelp, true) - numIPSetEntries = createGauge(numIPSetEntriesName, numIPSetEntriesHelp, false) - ipsetInventory = createGaugeVec(ipsetInventoryName, ipsetInventoryHelp, false, setNameLabel, setHashLabel) - log.Logf("Finished initializing all Prometheus metrics") - haveInitialized = true + haveInitialized = false + InitializeAll() } // GetHandler returns the HTTP handler for the metrics endpoint diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go index 4f6ea3201a..7acb414b25 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go @@ -208,7 +208,7 @@ func TestDeleteIPSet(t *testing.T) { calls := GetApplyIPSetsTestCalls(tt.toCreateMetadatas, nil) iMgr := NewIPSetManager(tt.cfg, common.NewMockIOShim(calls)) iMgr.CreateIPSets(tt.toCreateMetadatas) - iMgr.ApplyIPSets() + require.NoError(t, iMgr.ApplyIPSets()) iMgr.DeleteIPSet(tt.toDeleteName) assertEqualCache(t, iMgr, tt.expectedCache) @@ -232,8 +232,8 @@ func TestDeleteIPSetNotAllowed(t *testing.T) { l := NewIPSetMetadata(testListName, KeyLabelOfNamespace) calls := GetApplyIPSetsTestCalls([]*IPSetMetadata{l, m}, nil) iMgr := NewIPSetManager(applyOnNeedCfg, common.NewMockIOShim(calls)) - iMgr.AddToLists([]*IPSetMetadata{l}, []*IPSetMetadata{m}) - iMgr.ApplyIPSets() + require.NoError(t, iMgr.AddToLists([]*IPSetMetadata{l}, []*IPSetMetadata{m})) + require.NoError(t, iMgr.ApplyIPSets()) iMgr.DeleteIPSet(m.GetPrefixName()) assertEqualCache(t, iMgr, cacheValues{ From aff520c86212bde4e0145cb52309065496fff3cd Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Tue, 25 Jan 2022 18:08:00 -0800 Subject: [PATCH 03/10] add comment and comment code to resolve lint --- npm/pkg/dataplane/ipsets/ipsetmanager_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go index 7acb414b25..f6756d09f6 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go @@ -46,8 +46,9 @@ type member struct { type memberKind bool const ( - isIP = memberKind(true) - isSet = memberKind(false) + isIP = memberKind(true) + // TODO uncomment and write UTs that check cache contents for more add/delete members + // isSet = memberKind(false) ) func TestCreateIPSet(t *testing.T) { From 4d6894901f5c8ff3d89c7eea130ff2e27f826b57 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Thu, 27 Jan 2022 13:46:42 -0800 Subject: [PATCH 04/10] optimize looping and dirty cache updates --- npm/pkg/dataplane/ipsets/ipsetmanager.go | 125 ++++++++++++----------- 1 file changed, 64 insertions(+), 61 deletions(-) diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager.go b/npm/pkg/dataplane/ipsets/ipsetmanager.go index adc52b3fb8..0bd26db1bb 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager.go @@ -170,12 +170,15 @@ 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() - // 1. check for errors and create any missing sets for _, metadata := range addToSets { + // 1. check for errors and create a missing set prefixedName := metadata.GetPrefixName() set, exists := iMgr.setMap[prefixedName] if !exists { @@ -186,17 +189,17 @@ func (iMgr *IPSetManager) AddToSets(addToSets []*IPSetMetadata, ip, podKey strin if set.Kind != HashSet { return npmerrors.Errorf(npmerrors.AppendIPSet, false, fmt.Sprintf("ipset %s is not a hash set", prefixedName)) } - } - // 2. add ip to to all sets - for _, metadata := range addToSets { - prefixedName := metadata.GetPrefixName() - set := iMgr.setMap[prefixedName] + // 2. add ip to the set cachedPodKey, 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 { + if 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, + ) + } continue } @@ -207,6 +210,9 @@ func (iMgr *IPSetManager) AddToSets(addToSets []*IPSetMetadata, ip, podKey strin } func (iMgr *IPSetManager) RemoveFromSets(removeFromSets []*IPSetMetadata, ip, podKey string) error { + if len(removeFromSets) == 0 { + return nil + } iMgr.Lock() defer iMgr.Unlock() @@ -220,24 +226,18 @@ func (iMgr *IPSetManager) RemoveFromSets(removeFromSets []*IPSetMetadata, ip, po if set.Kind != HashSet { return npmerrors.Errorf(npmerrors.DeleteIPSet, false, fmt.Sprintf("ipset %s is not a hash set", prefixedName)) } - } - - // 2. remove ip from all existing sets - for _, metadata := range removeFromSets { - prefixedName := metadata.GetPrefixName() - set, exists := iMgr.setMap[prefixedName] - if !exists { - continue - } - // 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 } @@ -250,32 +250,15 @@ func (iMgr *IPSetManager) RemoveFromSets(removeFromSets []*IPSetMetadata, ip, po } func (iMgr *IPSetManager) AddToLists(listMetadatas, setMetadatas []*IPSetMetadata) error { + if len(listMetadatas) == 0 || len(setMetadatas) == 0 { + return nil + } iMgr.Lock() defer iMgr.Unlock() - // 1. check for errors in lists and create any missing lists - for _, listMetadata := range listMetadatas { - 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] - } - - if list.Kind != ListSet { - return npmerrors.Errorf(npmerrors.AppendIPSet, false, fmt.Sprintf("ipset %s is not a list set", listName)) - } - - for _, memberMetadata := range setMetadatas { - memberName := memberMetadata.GetPrefixName() - if listName == memberName { - return npmerrors.Errorf(npmerrors.AppendIPSet, false, fmt.Sprintf("ipset %s cannot be added to itself", listName)) - } - } - } - - // 2. check for errors in members and create any missing sets + // use this for efficient error checking later + memberNames := make(map[string]struct{}, len(setMetadatas)) + // 1. check for errors in members and create any missing sets for _, setMetadata := range setMetadatas { setName := setMetadata.GetPrefixName() set, exists := iMgr.setMap[setName] @@ -290,14 +273,32 @@ func (iMgr *IPSetManager) AddToLists(listMetadatas, setMetadatas []*IPSetMetadat 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)) } + + memberNames[setName] = struct{}{} } - // 3. add sets to all lists 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)) + } + + if _, exists := memberNames[listName]; exists { + return npmerrors.Errorf(npmerrors.AppendIPSet, false, fmt.Sprintf("ipset %s cannot be added to itself", listName)) + } + + // 3. add all members to the list + for _, memberMetadata := range setMetadatas { + memberName := memberMetadata.GetPrefixName() + iMgr.addMemberIPSet(listName, memberName) } iMgr.modifyCacheForKernelMemberUpdate(listName) } @@ -305,9 +306,13 @@ func (iMgr *IPSetManager) AddToLists(listMetadatas, setMetadatas []*IPSetMetadat } func (iMgr *IPSetManager) RemoveFromList(listMetadata *IPSetMetadata, setMetadatas []*IPSetMetadata) error { + if len(setMetadatas) == 0 { + return nil + } iMgr.Lock() defer iMgr.Unlock() + // 1. check for errors (ignore missing sets) listName := listMetadata.GetPrefixName() list, exists := iMgr.setMap[listName] if !exists { @@ -318,9 +323,10 @@ func (iMgr *IPSetManager) RemoveFromList(listMetadata *IPSetMetadata, setMetadat 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() - set, exists := iMgr.setMap[setName] + memberName := setMetadata.GetPrefixName() + set, exists := iMgr.setMap[memberName] if !exists { continue } @@ -328,15 +334,16 @@ func (iMgr *IPSetManager) RemoveFromList(listMetadata *IPSetMetadata, setMetadat // Nested IPSets are only supported for windows // Check if we want to actually use that support if set.Kind != HashSet { - return npmerrors.Errorf(npmerrors.DeleteIPSet, false, fmt.Sprintf("ipset %s is not a hash set and nested list sets are not supported", setName)) + return npmerrors.Errorf(npmerrors.DeleteIPSet, false, fmt.Sprintf("ipset %s is not a hash set and nested list sets are not supported", memberName)) } - } - for _, setMetadata := range setMetadatas { - setName := setMetadata.GetPrefixName() - iMgr.removeMemberIPSet(listName, setName) + // 2. remove member from the list + iMgr.removeMemberIPSet(listName, memberName) + modified = true + } + if modified { + iMgr.modifyCacheForKernelMemberUpdate(listName) } - iMgr.modifyCacheForKernelMemberUpdate(listName) return nil } @@ -505,10 +512,6 @@ 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) } } From f9bc5e8160c03590dead6d0159a0e3e6a622399a Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Thu, 27 Jan 2022 15:25:10 -0800 Subject: [PATCH 05/10] address comments and change param type of modifyCacheForKernelMemberUpdate to reduce map lookups --- npm/pkg/dataplane/ipsets/ipsetmanager.go | 104 ++++++++++------------- 1 file changed, 43 insertions(+), 61 deletions(-) diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager.go b/npm/pkg/dataplane/ipsets/ipsetmanager.go index 0bd26db1bb..e35a8c937d 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager.go @@ -132,7 +132,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 { @@ -190,20 +190,14 @@ func (iMgr *IPSetManager) AddToSets(addToSets []*IPSetMetadata, ip, podKey strin return npmerrors.Errorf(npmerrors.AppendIPSet, false, fmt.Sprintf("ipset %s is not a hash set", prefixedName)) } - // 2. add ip to the set - 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 { - if 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, - ) - } continue } - iMgr.modifyCacheForKernelMemberUpdate(prefixedName) + iMgr.modifyCacheForKernelMemberUpdate(set) metrics.AddEntryToIPSet(prefixedName) } return nil @@ -243,7 +237,7 @@ func (iMgr *IPSetManager) RemoveFromSets(removeFromSets []*IPSetMetadata, ip, po // update the IP ownership with podkey delete(set.IPPodKey, ip) - iMgr.modifyCacheForKernelMemberUpdate(prefixedName) + iMgr.modifyCacheForKernelMemberUpdate(set) metrics.RemoveEntryFromIPSet(prefixedName) } return nil @@ -256,8 +250,6 @@ func (iMgr *IPSetManager) AddToLists(listMetadatas, setMetadatas []*IPSetMetadat iMgr.Lock() defer iMgr.Unlock() - // use this for efficient error checking later - memberNames := make(map[string]struct{}, len(setMetadatas)) // 1. check for errors in members and create any missing sets for _, setMetadata := range setMetadatas { setName := setMetadata.GetPrefixName() @@ -273,8 +265,6 @@ func (iMgr *IPSetManager) AddToLists(listMetadatas, setMetadatas []*IPSetMetadat 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)) } - - memberNames[setName] = struct{}{} } for _, listMetadata := range listMetadatas { @@ -291,16 +281,28 @@ func (iMgr *IPSetManager) AddToLists(listMetadatas, setMetadatas []*IPSetMetadat return npmerrors.Errorf(npmerrors.AppendIPSet, false, fmt.Sprintf("ipset %s is not a list set", listName)) } - if _, exists := memberNames[listName]; exists { - return npmerrors.Errorf(npmerrors.AppendIPSet, false, fmt.Sprintf("ipset %s cannot be added to itself", listName)) - } - + modified := false // 3. add all members to the list for _, memberMetadata := range setMetadatas { memberName := memberMetadata.GetPrefixName() - iMgr.addMemberIPSet(listName, memberName) + // 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) } return nil } @@ -326,23 +328,37 @@ func (iMgr *IPSetManager) RemoveFromList(listMetadata *IPSetMetadata, setMetadat modified := false for _, setMetadata := range setMetadatas { memberName := setMetadata.GetPrefixName() - set, exists := iMgr.setMap[memberName] + 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 set.Kind != HashSet { + 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 - iMgr.removeMemberIPSet(listName, memberName) + list := iMgr.setMap[listName] + 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(listName) + iMgr.modifyCacheForKernelMemberUpdate(list) } return nil } @@ -466,43 +482,9 @@ func (iMgr *IPSetManager) decKernelReferCountAndModifyCache(member *IPSet) { } } -func (iMgr *IPSetManager) modifyCacheForKernelMemberUpdate(setName string) { - set := iMgr.setMap[setName] +func (iMgr *IPSetManager) modifyCacheForKernelMemberUpdate(set *IPSet) { if iMgr.shouldBeInKernel(set) { - iMgr.toAddOrUpdateCache[setName] = struct{}{} - } -} - -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{}{} } } From 6e71db9d9b6bcca9bdb28e10fb006e2e93ea2bf1 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Fri, 28 Jan 2022 13:54:38 -0800 Subject: [PATCH 06/10] add exec time metrics --- npm/pkg/dataplane/ipsets/ipsetmanager.go | 3 +++ npm/pkg/dataplane/policies/policymanager.go | 3 +++ 2 files changed, 6 insertions(+) diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager.go b/npm/pkg/dataplane/ipsets/ipsetmanager.go index e14e183bda..fb254f5ba6 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager.go @@ -364,6 +364,8 @@ func (iMgr *IPSetManager) RemoveFromList(listMetadata *IPSetMetadata, setMetadat } func (iMgr *IPSetManager) ApplyIPSets() error { + prometheusTimer := metrics.StartNewTimer() + iMgr.Lock() defer iMgr.Unlock() @@ -371,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) 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())) From 00fc5898b09f1125098d6457480b2bab84188ef8 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Mon, 31 Jan 2022 15:37:22 -0800 Subject: [PATCH 07/10] UTs --- npm/metrics/prometheus-metrics.go | 2 + npm/pkg/dataplane/dataplane_test.go | 6 - npm/pkg/dataplane/ipsets/ipsetmanager.go | 5 +- npm/pkg/dataplane/ipsets/ipsetmanager_test.go | 881 +++++++++++++----- npm/pkg/dataplane/ipsets/testutils_linux.go | 2 + 5 files changed, 664 insertions(+), 232 deletions(-) diff --git a/npm/metrics/prometheus-metrics.go b/npm/metrics/prometheus-metrics.go index 2a5bc04fc8..6d7daa2acc 100644 --- a/npm/metrics/prometheus-metrics.go +++ b/npm/metrics/prometheus-metrics.go @@ -78,9 +78,11 @@ 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 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/ipsetmanager.go b/npm/pkg/dataplane/ipsets/ipsetmanager.go index fb254f5ba6..fa80370cd5 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager.go @@ -54,7 +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 + // TODO update prometheus metrics here instead of in OS-specific functions (done in Linux right now) + // metrics.ResetNumIPSets() and metrics.ResetIPSetEntries() return nil } @@ -497,7 +498,7 @@ func (iMgr *IPSetManager) sanitizeDirtyCache() { for setName := range iMgr.toDeleteCache { _, ok := iMgr.toAddOrUpdateCache[setName] if ok { - 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 f6756d09f6..77cf675080 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,91 +62,204 @@ var ( IPSetMode: ApplyAllIPSets, NetworkName: "azure", } -) -type cacheValues struct { - mainCache []string - addUpdateCacheMembers map[string][]member - toDeleteCache []string -} + namespaceSet = NewIPSetMetadata("test-set1", Namespace) + keyLabelOfPodSet = NewIPSetMetadata("test-set2", KeyLabelOfPod) + list = NewIPSetMetadata("test-list1", KeyLabelOfNamespace) +) -type member struct { - // either an IP or set name - value string - kind memberKind -} +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() -type memberKind bool + // 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, + }) + } -const ( - isIP = memberKind(true) - // TODO uncomment and write UTs that check cache contents for more add/delete members - // isSet = memberKind(false) -) + execCount, err := metrics.GetIPSetExecCount() + promutil.NotifyIfErrors(t, err) + require.Equal(t, tt.expectedExecCount, execCount) + }) + } +} func TestCreateIPSet(t *testing.T) { - m1 := NewIPSetMetadata(testSetName, Namespace) - m2 := NewIPSetMetadata(testListName, KeyLabelOfNamespace) + type args struct { + cfg *IPSetManagerCfg + metadatas []*IPSetMetadata + } tests := []struct { - name string - cfg *IPSetManagerCfg - metadatas []*IPSetMetadata - expectedCache cacheValues - expectedNumIPSets int - expectedNumIPSetsInKernel int + name string + args args + expectedInfo }{ { - name: "Apply Always: create two new sets", - cfg: applyAlwaysCfg, - metadatas: []*IPSetMetadata{m1, m2}, - expectedCache: cacheValues{ - mainCache: []string{m1.GetPrefixName(), m2.GetPrefixName()}, - addUpdateCacheMembers: map[string][]member{ - m1.GetPrefixName(): {}, - m2.GetPrefixName(): {}, + 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}, }, - toDeleteCache: []string{}, + toAddUpdateCache: []*IPSetMetadata{namespaceSet, list}, + toDeleteCache: nil, + setsForKernel: []*IPSetMetadata{namespaceSet, list}, }, - expectedNumIPSets: 2, - expectedNumIPSetsInKernel: 2, }, { - name: "Apply On Need: create two new sets", - cfg: applyOnNeedCfg, - metadatas: []*IPSetMetadata{m1, m2}, - expectedCache: cacheValues{ - mainCache: []string{m1.GetPrefixName(), m2.GetPrefixName()}, - addUpdateCacheMembers: map[string][]member{}, - toDeleteCache: []string{}, + 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, }, - expectedNumIPSets: 2, - expectedNumIPSetsInKernel: 0, }, { - name: "Apply Always: no-op for set that exists", - cfg: applyAlwaysCfg, - metadatas: []*IPSetMetadata{m1, m1}, - expectedCache: cacheValues{ - mainCache: []string{m1.GetPrefixName()}, - addUpdateCacheMembers: map[string][]member{ - m1.GetPrefixName(): {}, + 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}, }, - toDeleteCache: []string{}, + toAddUpdateCache: []*IPSetMetadata{namespaceSet}, + toDeleteCache: nil, + setsForKernel: []*IPSetMetadata{namespaceSet}, }, - expectedNumIPSets: 1, - expectedNumIPSetsInKernel: 1, }, { - name: "Apply On Need: no-op for set that exists", - cfg: applyOnNeedCfg, - metadatas: []*IPSetMetadata{m1, m1}, - expectedCache: cacheValues{ - mainCache: []string{m1.GetPrefixName()}, - addUpdateCacheMembers: map[string][]member{}, - toDeleteCache: []string{}, + 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, }, - expectedNumIPSets: 1, - expectedNumIPSetsInKernel: 0, }, } @@ -121,84 +267,85 @@ func TestCreateIPSet(t *testing.T) { tt := tt t.Run(tt.name, func(t *testing.T) { metrics.ReinitializeAll() - iMgr := NewIPSetManager(tt.cfg, common.NewMockIOShim(nil)) - iMgr.CreateIPSets(tt.metadatas) - - assertEqualCache(t, iMgr, tt.expectedCache) - - numIPSets, _ := metrics.GetNumIPSets() - assert.Equal(t, tt.expectedNumIPSets, numIPSets) - // TODO update when we have prometheus metric for in kernel - numIPSetsInKernel := tt.expectedNumIPSetsInKernel - assert.Equal(t, tt.expectedNumIPSetsInKernel, numIPSetsInKernel) - numEntries, _ := metrics.GetNumIPSetEntries() - assert.Equal(t, 0, numEntries) - for _, setName := range tt.expectedCache.mainCache { - numEntries, _ := metrics.GetNumEntriesForIPSet(setName) - assert.Equal(t, 0, numEntries) - } + 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 TestDeleteIPSet(t *testing.T) { - m1 := NewIPSetMetadata(testSetName, Namespace) - m2 := NewIPSetMetadata(testListName, KeyLabelOfNamespace) - tests := []struct { - name string + type args struct { cfg *IPSetManagerCfg toCreateMetadatas []*IPSetMetadata toDeleteName string - expectedCache cacheValues - expectedNumIPSets int + } + tests := []struct { + name string + args args + expectedInfo expectedInfo }{ { - name: "Apply Always: delete set", - cfg: applyAlwaysCfg, - toCreateMetadatas: []*IPSetMetadata{m1}, - toDeleteName: m1.GetPrefixName(), - expectedCache: cacheValues{ - mainCache: []string{}, - addUpdateCacheMembers: map[string][]member{}, - toDeleteCache: []string{m1.GetPrefixName()}, + 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, }, - expectedNumIPSets: 0, }, { - name: "Apply On Need: delete set", - cfg: applyOnNeedCfg, - toCreateMetadatas: []*IPSetMetadata{m1}, - toDeleteName: m1.GetPrefixName(), - expectedCache: cacheValues{ - mainCache: []string{}, - addUpdateCacheMembers: map[string][]member{}, - toDeleteCache: []string{}, + 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, }, - expectedNumIPSets: 0, }, { - name: "Apply Always: set doesn't exist", - cfg: applyAlwaysCfg, - toCreateMetadatas: []*IPSetMetadata{m1}, - toDeleteName: m2.GetPrefixName(), - expectedCache: cacheValues{ - mainCache: []string{m1.GetPrefixName()}, - addUpdateCacheMembers: map[string][]member{}, - toDeleteCache: []string{}, + 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, }, - expectedNumIPSets: 1, }, { - name: "Apply On Need: set doesn't exist", - cfg: applyOnNeedCfg, - toCreateMetadatas: []*IPSetMetadata{m1}, - toDeleteName: m2.GetPrefixName(), - expectedCache: cacheValues{ - mainCache: []string{m1.GetPrefixName()}, - addUpdateCacheMembers: map[string][]member{}, - toDeleteCache: []string{}, + 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, }, - expectedNumIPSets: 1, }, } @@ -206,80 +353,308 @@ func TestDeleteIPSet(t *testing.T) { tt := tt t.Run(tt.name, func(t *testing.T) { metrics.ReinitializeAll() - calls := GetApplyIPSetsTestCalls(tt.toCreateMetadatas, nil) - iMgr := NewIPSetManager(tt.cfg, common.NewMockIOShim(calls)) - iMgr.CreateIPSets(tt.toCreateMetadatas) + 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.toDeleteName) - - assertEqualCache(t, iMgr, tt.expectedCache) - - numIPSets, _ := metrics.GetNumIPSets() - assert.Equal(t, tt.expectedNumIPSets, numIPSets) - // TODO update when we have prometheus metric for in kernel - numIPSetsInKernel := 0 - assert.Equal(t, 0, numIPSetsInKernel) - numEntries, _ := metrics.GetNumIPSetEntries() - assert.Equal(t, 0, numEntries) - numEntries, _ = metrics.GetNumEntriesForIPSet(tt.toDeleteName) - assert.Equal(t, 0, numEntries) + iMgr.DeleteIPSet(tt.args.toDeleteName) + assertExpectedInfo(t, iMgr, tt.expectedInfo) }) } } 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() - m := NewIPSetMetadata(testSetName, Namespace) - l := NewIPSetMetadata(testListName, KeyLabelOfNamespace) - calls := GetApplyIPSetsTestCalls([]*IPSetMetadata{l, m}, nil) - iMgr := NewIPSetManager(applyOnNeedCfg, common.NewMockIOShim(calls)) - require.NoError(t, iMgr.AddToLists([]*IPSetMetadata{l}, []*IPSetMetadata{m})) + 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(m.GetPrefixName()) - assertEqualCache(t, iMgr, cacheValues{ - mainCache: []string{l.GetPrefixName(), m.GetPrefixName()}, - addUpdateCacheMembers: map[string][]member{}, - toDeleteCache: []string{}, - }) + iMgr.DeleteIPSet(namespaceSet.GetPrefixName()) + iMgr.DeleteIPSet(list.GetPrefixName()) - numIPSets, _ := metrics.GetNumIPSets() - assert.Equal(t, 2, numIPSets) - // TODO update when we have prometheus metric for in kernel - numIPSetsInKernel := 0 - assert.Equal(t, 0, numIPSetsInKernel) - numEntries, _ := metrics.GetNumIPSetEntries() - assert.Equal(t, 1, numEntries) - numEntries, _ = metrics.GetNumEntriesForIPSet(m.GetPrefixName()) - assert.Equal(t, 0, numEntries) - numEntries, _ = metrics.GetNumEntriesForIPSet(l.GetPrefixName()) - assert.Equal(t, 1, numEntries) + assertExpectedInfo(t, iMgr, expectedInfo{ + mainCache: []setMembers{ + {metadata: list, members: nil}, + {metadata: namespaceSet, members: nil}, + }, + toAddUpdateCache: nil, + toDeleteCache: nil, + setsForKernel: nil, + }) } -func TestAddToSet(t *testing.T) { - iMgr := NewIPSetManager(applyOnNeedCfg, common.NewMockIOShim([]testutils.TestCmd{})) - - setMetadata := NewIPSetMetadata(testSetName, Namespace) - iMgr.CreateIPSets([]*IPSetMetadata{setMetadata}) - - err := iMgr.AddToSets([]*IPSetMetadata{setMetadata}, testPodIP, testPodKey) - 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) + }) + } +} - err = iMgr.AddToSets([]*IPSetMetadata{setMetadata}, "2001:db8:0:0:0:0:2:1", "newpod") - require.NoError(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} - // same IP changed podkey - err = iMgr.AddToSets([]*IPSetMetadata{setMetadata}, testPodIP, "newpod") - require.NoError(t, err) + 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()) - listMetadata := NewIPSetMetadata("testipsetlist", KeyLabelOfNamespace) - iMgr.CreateIPSets([]*IPSetMetadata{listMetadata}) - err = iMgr.AddToSets([]*IPSetMetadata{listMetadata}, testPodIP, testPodKey) - require.Error(t, err) + 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}) @@ -314,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) { @@ -331,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) { @@ -394,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) { @@ -446,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) @@ -456,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()) @@ -465,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) @@ -476,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) } } @@ -515,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()) @@ -531,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) @@ -542,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 { @@ -559,33 +934,91 @@ func TestMain(m *testing.M) { os.Exit(exitCode) } -func assertEqualCache(t *testing.T, iMgr *IPSetManager, cache cacheValues) { - require.Equal(t, len(cache.mainCache), len(iMgr.setMap)) - for _, setName := range cache.mainCache { - require.True(t, iMgr.exists(setName)) +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) - assert.Equal(t, util.GetHashedName(setName), set.HashedName) - } - - require.Equal(t, len(cache.addUpdateCacheMembers), len(iMgr.toAddOrUpdateCache)) - for setName, members := range cache.addUpdateCacheMembers { - require.True(t, iMgr.exists(setName)) - for _, member := range members { + 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 == isIP { + if member.kind == isHashMember { _, ok := set.IPPodKey[member.value] - require.True(t, ok) + 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) + require.True(t, ok, "set member %s not found in list %s", member.value, setName) } } } - require.Equal(t, len(cache.toDeleteCache), len(iMgr.toDeleteCache)) - for _, setName := range cache.toDeleteCache { + 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) + 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} } From 4ae8e86abf4c5d9db2175af5772a7941442f3b6e Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Mon, 31 Jan 2022 15:55:39 -0800 Subject: [PATCH 08/10] fix lints --- npm/pkg/dataplane/ipsets/ipsetmanager.go | 1 - npm/pkg/dataplane/ipsets/ipsetmanager_test.go | 14 +++++++------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager.go b/npm/pkg/dataplane/ipsets/ipsetmanager.go index fa80370cd5..cc7575fd5f 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager.go @@ -344,7 +344,6 @@ func (iMgr *IPSetManager) RemoveFromList(listMetadata *IPSetMetadata, setMetadat } // 2. remove member from the list - list := iMgr.setMap[listName] if !list.hasMember(memberName) { continue } diff --git a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go index 77cf675080..7e8a8afdc9 100644 --- a/npm/pkg/dataplane/ipsets/ipsetmanager_test.go +++ b/npm/pkg/dataplane/ipsets/ipsetmanager_test.go @@ -174,7 +174,7 @@ func TestApplyIPSets(t *testing.T) { for _, set := range tt.args.toAddUpdateSets { cache = append(cache, setMembers{metadata: set, members: nil}) } - assertExpectedInfo(t, iMgr, expectedInfo{ + assertExpectedInfo(t, iMgr, &expectedInfo{ mainCache: cache, toAddUpdateCache: nil, toDeleteCache: nil, @@ -271,7 +271,7 @@ func TestCreateIPSet(t *testing.T) { defer ioShim.VerifyCalls(t, nil) iMgr := NewIPSetManager(tt.args.cfg, ioShim) iMgr.CreateIPSets(tt.args.metadatas) - assertExpectedInfo(t, iMgr, tt.expectedInfo) + assertExpectedInfo(t, iMgr, &tt.expectedInfo) }) } } @@ -363,7 +363,7 @@ func TestDeleteIPSet(t *testing.T) { iMgr.CreateIPSets(tt.args.toCreateMetadatas) require.NoError(t, iMgr.ApplyIPSets()) iMgr.DeleteIPSet(tt.args.toDeleteName) - assertExpectedInfo(t, iMgr, tt.expectedInfo) + assertExpectedInfo(t, iMgr, &tt.expectedInfo) }) } } @@ -383,7 +383,7 @@ func TestDeleteIPSetNotAllowed(t *testing.T) { iMgr.DeleteIPSet(namespaceSet.GetPrefixName()) iMgr.DeleteIPSet(list.GetPrefixName()) - assertExpectedInfo(t, iMgr, expectedInfo{ + assertExpectedInfo(t, iMgr, &expectedInfo{ mainCache: []setMembers{ {metadata: list, members: nil}, {metadata: namespaceSet, members: nil}, @@ -585,7 +585,7 @@ func TestAddToSets(t *testing.T) { } else { require.NoError(t, err) } - assertExpectedInfo(t, iMgr, tt.expectedInfo) + assertExpectedInfo(t, iMgr, &tt.expectedInfo) }) } } @@ -638,7 +638,7 @@ func TestAddToSetInKernelApplyOnNeed(t *testing.T) { if tt.wantDirty { dirtySets = []*IPSetMetadata{namespaceSet} } - assertExpectedInfo(t, iMgr, expectedInfo{ + assertExpectedInfo(t, iMgr, &expectedInfo{ mainCache: []setMembers{ {metadata: tt.metadata, members: members}, }, @@ -934,7 +934,7 @@ func TestMain(m *testing.M) { os.Exit(exitCode) } -func assertExpectedInfo(t *testing.T, iMgr *IPSetManager, info expectedInfo) { +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 { From ef03e4071673c3448c66f3f4598b21e78cce3140 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Mon, 31 Jan 2022 16:38:27 -0800 Subject: [PATCH 09/10] initialize metrics in policymanager tests --- npm/pkg/dataplane/policies/policymanager_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) 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) +} From 7d2be895986d674d6bdfb4399711d214f2ccb525 Mon Sep 17 00:00:00 2001 From: Hunter Gregory Date: Tue, 1 Feb 2022 11:17:30 -0800 Subject: [PATCH 10/10] fix bug in publishing npm logs --- .pipelines/npm/npm-conformance-tests.yaml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) 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()