diff --git a/pkg/agent/controller/networkpolicy/priority.go b/pkg/agent/controller/networkpolicy/priority.go index ddf617a154c..435750cda65 100644 --- a/pkg/agent/controller/networkpolicy/priority.go +++ b/pkg/agent/controller/networkpolicy/priority.go @@ -253,21 +253,25 @@ func (pa *priorityAssigner) GetOFPriority(p types.Priority) (uint16, bool) { return of, registered } -// RegisterPriorities registers a list of Priorities with the priorityAssigner. It allocates ofPriorities for -// input Priorities that are not yet registered. It also returns the ofPriority updates if there are reassignments, +// RegisterPriorities registers a list of types.Priority with the priorityAssigner. It allocates ofPriorities for +// input priorities that are not yet registered. It also returns the ofPriority updates if there are reassignments, // as well as a revert function that can undo the registration if any error occurred in data plane. // Note that this function modifies the priorities slice in the parameter, as it only keeps the Priorities which -// this priorityAssigner has not yet registered. +// this priorityAssigner has not yet registered. Input priorities are not assumed to be unique or consecutive. func (pa *priorityAssigner) RegisterPriorities(priorities []types.Priority) (map[uint16]uint16, func(), error) { // create a zero-length slice with the same underlying array to save memory usage. prioritiesToRegister := priorities[:0] + priorityDedup := map[types.Priority]struct{}{} for _, p := range priorities { - if _, exists := pa.priorityMap[p]; !exists { - prioritiesToRegister = append(prioritiesToRegister, p) + if _, dup := priorityDedup[p]; !dup { + priorityDedup[p] = struct{}{} + if _, exists := pa.priorityMap[p]; !exists { + prioritiesToRegister = append(prioritiesToRegister, p) + } } } numPriorityToRegister := len(prioritiesToRegister) - klog.V(2).Infof("%v new Priorities need to be registered", numPriorityToRegister) + klog.V(2).Infof("%v new priorities need to be registered", numPriorityToRegister) if numPriorityToRegister == 0 { return nil, nil, nil } else if uint16(numPriorityToRegister+len(pa.sortedPriorities)) > pa.policyTopPriority-pa.policyBottomPriority+1 { @@ -275,7 +279,7 @@ func (pa *priorityAssigner) RegisterPriorities(priorities []types.Priority) (map } sort.Sort(types.ByPriority(prioritiesToRegister)) var consecutivePriorities [][]types.Priority - // break the Priorities into lists of consecutive Priorities. + // break the list of Priority into lists of consecutive Priority. for i, j := 0, 1; j <= numPriorityToRegister; j++ { if j == numPriorityToRegister || !prioritiesToRegister[j].IsConsecutive(prioritiesToRegister[j-1]) { consecutivePriorities = append(consecutivePriorities, prioritiesToRegister[i:j]) diff --git a/pkg/agent/controller/networkpolicy/priority_test.go b/pkg/agent/controller/networkpolicy/priority_test.go index b34bbe71236..2a2766018dc 100644 --- a/pkg/agent/controller/networkpolicy/priority_test.go +++ b/pkg/agent/controller/networkpolicy/priority_test.go @@ -162,7 +162,7 @@ func TestReassignBoundaryPriorities(t *testing.T) { } priorityUpdates := map[types.Priority]*PriorityUpdate{} err := pa.reassignBoundaryPriorities(tt.lowerBound, tt.upperBound, prioritiesToRegister, priorityUpdates) - assert.Equalf(t, nil, err, "Error occurred in reassignment") + assert.NoError(t, err, "Error occurred in reassignment") assert.Equalf(t, tt.expectedPriorityMap, pa.priorityMap, "priorityMap unexpected after reassignment") assert.Equalf(t, tt.expectedUpdates, priorityUpdates, "priority updates unexpected after reassignment") }) @@ -265,7 +265,7 @@ func TestInsertConsecutivePriorities(t *testing.T) { } priorityUpdates := map[types.Priority]*PriorityUpdate{} err := pa.insertConsecutivePriorities(prioritiesToRegister, priorityUpdates) - assert.Equalf(t, nil, err, "Error occurred in priority insertion") + assert.NoError(t, err, "Error occurred in priority insertion") assert.Equalf(t, tt.expectedOFPriorityMap, pa.ofPriorityMap, "priorityMap unexpected after insertion") }) } @@ -287,7 +287,7 @@ func TestRegisterPrioritiesAndRevert(t *testing.T) { insertionPoint191: p191, insertionPoint191 + 1: p190, } _, revertFunc, err := pa.RegisterPriorities(prioritiesToRegister) - assert.Equalf(t, nil, err, "Error occurred in priority registration") + assert.NoError(t, err, "Error occurred in priority registration") assert.Equalf(t, expectedOFMapAfterRegister, pa.ofPriorityMap, "priorityMap unexpected after registration") expectedOFMapAfterRevert := map[uint16]types.Priority{ @@ -297,6 +297,24 @@ func TestRegisterPrioritiesAndRevert(t *testing.T) { assert.Equalf(t, expectedOFMapAfterRevert, pa.ofPriorityMap, "priorityMap unexpected after revert") } +func TestRegisterDuplicatePriorities(t *testing.T) { + pa1 := newPriorityAssigner(false) + pa2 := newPriorityAssigner(false) + prioritiesToRegister := []types.Priority{p1131, p1130} + prioritiesToRegisterDuplicate := []types.Priority{p1130, p1131, p1130, p1130, p1130, p1131, p1130} + + _, _, err := pa1.RegisterPriorities(prioritiesToRegister) + assert.NoError(t, err, "Error occurred in priority registration") + _, _, err2 := pa2.RegisterPriorities(prioritiesToRegisterDuplicate) + assert.NoError(t, err2, "Error occurred in priority registration") + ofPriority1130, _ := pa1.GetOFPriority(p1130) + ofPriority1130Dup, _ := pa2.GetOFPriority(p1130) + assert.Equal(t, ofPriority1130, ofPriority1130Dup) + ofPriority1131, _ := pa1.GetOFPriority(p1131) + ofPriority1131Dup, _ := pa2.GetOFPriority(p1131) + assert.Equal(t, ofPriority1131, ofPriority1131Dup) +} + func generatePriorities(tierPriority, start, end int32, policyPriority float64) []types.Priority { priorities := make([]types.Priority, end-start+1) for i := start; i <= end; i++ { @@ -309,7 +327,7 @@ func TestRegisterAllOFPriorities(t *testing.T) { pa := newPriorityAssigner(true) maxPriorities := generatePriorities(253, int32(BaselinePolicyBottomPriority), int32(BaselinePolicyTopPriority), 5) _, _, err := pa.RegisterPriorities(maxPriorities) - assert.Equalf(t, nil, err, "Error occurred in registering max number of allowed priorities in baseline tier") + assert.NoError(t, err, "Error occurred in registering max number of allowed priorities in baseline tier") extraPriority := types.Priority{ TierPriority: 253, @@ -323,10 +341,10 @@ func TestRegisterAllOFPriorities(t *testing.T) { consecPriorities1 := generatePriorities(5, int32(PolicyBottomPriority), 10000, 5) _, _, err = pa.RegisterPriorities(consecPriorities1) - assert.Equalf(t, nil, err, "Error occurred before registering max number of allowed priorities") + assert.NoError(t, err, "Error occurred before registering max number of allowed priorities") consecPriorities2 := generatePriorities(10, 10001, int32(PolicyTopPriority), 5) _, _, err = pa.RegisterPriorities(consecPriorities2) - assert.Equalf(t, nil, err, "Error occurred in registering max number of allowed priorities") + assert.NoError(t, err, "Error occurred in registering max number of allowed priorities") _, _, err = pa.RegisterPriorities([]types.Priority{extraPriority}) assert.Errorf(t, err, "Error should be raised after max number of priorities are registered")