From f40895b6d9fef3eb1d704ee0274e3ce475be8a6d Mon Sep 17 00:00:00 2001 From: Quan Tian Date: Thu, 17 Jun 2021 18:07:45 +0800 Subject: [PATCH] Address comments Signed-off-by: Quan Tian --- pkg/controller/egress/controller.go | 20 ++++++++-------- pkg/controller/egress/controller_test.go | 30 +++++++++++++++++++----- 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/pkg/controller/egress/controller.go b/pkg/controller/egress/controller.go index 046f6a8f306..efb28cd60c5 100644 --- a/pkg/controller/egress/controller.go +++ b/pkg/controller/egress/controller.go @@ -206,8 +206,8 @@ func (c *EgressController) updateIPAllocation(egress *egressv1alpha2.Egress) { // TODO: Use validation webhook to ensure it. func (c *EgressController) createOrUpdateIPAllocator(ipPool *egressv1alpha2.ExternalIPPool) bool { changed := false - c.ipAllocationMutex.Lock() - defer c.ipAllocationMutex.Unlock() + c.ipAllocatorMutex.Lock() + defer c.ipAllocatorMutex.Unlock() existingIPRanges := sets.NewString() multiIPAllocator, exists := c.ipAllocatorMap[ipPool.Name] @@ -247,15 +247,15 @@ func (c *EgressController) createOrUpdateIPAllocator(ipPool *egressv1alpha2.Exte // deleteIPAllocator deletes the IP allocator of the given IP pool. func (c *EgressController) deleteIPAllocator(ipPoolName string) { - c.ipAllocationMutex.Lock() - defer c.ipAllocationMutex.Unlock() + c.ipAllocatorMutex.Lock() + defer c.ipAllocatorMutex.Unlock() delete(c.ipAllocatorMap, ipPoolName) } // getIPAllocator gets the IP allocator of the given IP pool. func (c *EgressController) getIPAllocator(ipPoolName string) (ipallocator.MultiIPAllocator, bool) { - c.ipAllocationMutex.RLock() - defer c.ipAllocationMutex.RUnlock() + c.ipAllocatorMutex.RLock() + defer c.ipAllocatorMutex.RUnlock() ipAllocator, exists := c.ipAllocatorMap[ipPoolName] return ipAllocator, exists } @@ -315,7 +315,7 @@ func (c *EgressController) syncEgressIP(egress *egressv1alpha2.Egress) (net.IP, prevIP, prevIPPool, exists := c.getIPAllocation(egress.Name) if exists { _, ipAllocatorExists := c.getIPAllocator(prevIPPool) - // The EgressIP and the ExternalIPPool doesn't change, do nothing. + // The EgressIP and the ExternalIPPool don't change, do nothing. if prevIP.String() == egress.Spec.EgressIP && prevIPPool == egress.Spec.ExternalIPPool && ipAllocatorExists { return prevIP, nil } @@ -323,14 +323,14 @@ func (c *EgressController) syncEgressIP(egress *egressv1alpha2.Egress) (net.IP, c.releaseEgressIP(egress.Name, prevIP, prevIPPool) } - // Skip allocating EgressIP if ExternalIPPool is not specified and returns whatever user specifies. + // Skip allocating EgressIP if ExternalIPPool is not specified and return whatever user specifies. if egress.Spec.ExternalIPPool == "" { return net.ParseIP(egress.Spec.EgressIP), nil } ipAllocator, exists := c.ipAllocatorMap[egress.Spec.ExternalIPPool] if !exists { - // The IP pool had been deleted, reclaim the IP from the Egress API. + // The IP pool has been deleted, reclaim the IP from the Egress API. if egress.Spec.EgressIP != "" { if err := c.updateEgressIP(egress, ""); err != nil { return nil, err @@ -406,7 +406,7 @@ func (c *EgressController) syncEgress(key string) error { egress, err := c.egressLister.Get(key) if err != nil { - // The Egress had been deleted, release its EgressIP if there was one. + // The Egress has been deleted, release its EgressIP if there was one. if prevIP, prevIPPool, exists := c.getIPAllocation(key); exists { c.releaseEgressIP(key, prevIP, prevIPPool) } diff --git a/pkg/controller/egress/controller_test.go b/pkg/controller/egress/controller_test.go index e9203e198b7..c7b55da6d4e 100644 --- a/pkg/controller/egress/controller_test.go +++ b/pkg/controller/egress/controller_test.go @@ -470,17 +470,35 @@ func TestSyncEgressIP(t *testing.T) { expectErr bool }{ { - name: "Egress with empty EgressIP and existing ExternalIPPool", - existingExternalIPPool: newExternalIPPool("ipPoolA", "1.1.1.0/24", "", ""), + name: "Egress with empty EgressIP and existing ExternalIPPool", + existingEgresses: []*v1alpha2.Egress{ + { + ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA"}, + Spec: v1alpha2.EgressSpec{ + EgressIP: "1.1.1.1", + ExternalIPPool: "ipPoolA", + }, + }, + { + ObjectMeta: metav1.ObjectMeta{Name: "egressB", UID: "uidB"}, + Spec: v1alpha2.EgressSpec{ + EgressIP: "1.1.1.2", + ExternalIPPool: "ipPoolA", + }, + }, + }, + // The first IPRange 1.1.1.0/30 should be occupied by the existing Egresses. The input Egress's IP should + // be allocated from the second IPRange 1.1.2.10-1.1.2.20. + existingExternalIPPool: newExternalIPPool("ipPoolA", "1.1.1.0/30", "1.1.2.10", "1.1.2.20"), inputEgress: &v1alpha2.Egress{ - ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA"}, + ObjectMeta: metav1.ObjectMeta{Name: "egressC", UID: "uidC"}, Spec: v1alpha2.EgressSpec{ EgressIP: "", ExternalIPPool: "ipPoolA", }, }, - expectedEgressIP: "1.1.1.1", - expectedExternalIPPoolUsed: 1, + expectedEgressIP: "1.1.2.10", + expectedExternalIPPoolUsed: 3, expectErr: false, }, { @@ -572,7 +590,7 @@ func TestSyncEgressIP(t *testing.T) { expectErr: false, }, { - name: "Egress with conflicted EgressIP", + name: "Egress with conflicting EgressIP", existingEgresses: []*v1alpha2.Egress{ { ObjectMeta: metav1.ObjectMeta{Name: "egressA", UID: "uidA"},