From 8579c5d20142f31420dfba439c23242553d373d2 Mon Sep 17 00:00:00 2001 From: Abhinandan Prativadi Date: Thu, 10 May 2018 12:14:44 -0700 Subject: [PATCH 1/2] Add check for overlapping subnets Signed-off-by: Abhinandan Prativadi --- ipam/structures.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ipam/structures.go b/ipam/structures.go index 09a77695dd..455a16ca65 100644 --- a/ipam/structures.go +++ b/ipam/structures.go @@ -262,12 +262,13 @@ func (aSpace *addrSpace) updatePoolDBOnAdd(k SubnetKey, nw *net.IPNet, ipr *Addr defer aSpace.Unlock() // Check if already allocated - if p, ok := aSpace.subnets[k]; ok { + if _, ok := aSpace.subnets[k]; ok { if pdf { return nil, types.InternalMaskableErrorf("predefined pool %s is already reserved", nw) } - aSpace.incRefCount(p, 1) - return func() error { return nil }, nil + // This means the same pool is already allocated. updatePoolDBOnAdd is called when there + // is request for a pool/subpool. It should ensure there is no overlap with existing pools + return nil, ipamapi.ErrPoolOverlap } // If master pool, check for overlap From e65fabdea551be2ff9bd10386f7fb3af0997d578 Mon Sep 17 00:00:00 2001 From: Abhinandan Prativadi Date: Thu, 10 May 2018 14:47:52 -0700 Subject: [PATCH 2/2] fixing test cases Signed-off-by: Abhinandan Prativadi --- ipam/allocator_test.go | 69 +++++++++---------------------------- libnetwork_internal_test.go | 4 +-- 2 files changed, 19 insertions(+), 54 deletions(-) diff --git a/ipam/allocator_test.go b/ipam/allocator_test.go index de97461992..70ee992983 100644 --- a/ipam/allocator_test.go +++ b/ipam/allocator_test.go @@ -241,12 +241,9 @@ func TestAddSubnets(t *testing.T) { t.Fatal("returned same pool id for same subnets in different namespaces") } - pid, _, _, err := a.RequestPool("abc", "10.0.0.0/8", "", nil, false) - if err != nil { - t.Fatalf("Unexpected failure requesting existing subnet: %v", err) - } - if pid != pid1 { - t.Fatal("returned different pool id for same subnet requests") + _, _, _, err = a.RequestPool("abc", "10.0.0.0/8", "", nil, false) + if err == nil { + t.Fatalf("Expected failure requesting existing subnet") } _, _, _, err = a.RequestPool("abc", "10.128.0.0/9", "", nil, false) @@ -254,16 +251,13 @@ func TestAddSubnets(t *testing.T) { t.Fatal("Expected failure on adding overlapping base subnet") } - pid2, _, _, err := a.RequestPool("abc", "10.0.0.0/8", "10.128.0.0/9", nil, false) + _, _, _, err = a.RequestPool("abc", "10.0.0.0/8", "10.128.0.0/9", nil, false) if err != nil { t.Fatalf("Unexpected failure on adding sub pool: %v", err) } - pid3, _, _, err := a.RequestPool("abc", "10.0.0.0/8", "10.128.0.0/9", nil, false) - if err != nil { - t.Fatalf("Unexpected failure on adding overlapping sub pool: %v", err) - } - if pid2 != pid3 { - t.Fatal("returned different pool id for same sub pool requests") + _, _, _, err = a.RequestPool("abc", "10.0.0.0/8", "10.128.0.0/9", nil, false) + if err == nil { + t.Fatalf("Expected failure on adding overlapping sub pool") } _, _, _, err = a.RequestPool(localAddressSpace, "10.20.2.0/24", "", nil, false) @@ -293,7 +287,7 @@ func TestAddReleasePoolID(t *testing.T) { a, err := getAllocator(store) assert.NoError(t, err) - var k0, k1, k2 SubnetKey + var k0, k1 SubnetKey aSpace, err := a.getAddrSpace(localAddressSpace) if err != nil { t.Fatal(err) @@ -326,6 +320,10 @@ func TestAddReleasePoolID(t *testing.T) { t.Fatal(err) } + if pid0 == pid1 { + t.Fatalf("Incorrect poolIDs returned %s, %s", pid0, pid1) + } + aSpace, err = a.getAddrSpace(localAddressSpace) if err != nil { t.Fatal(err) @@ -336,15 +334,9 @@ func TestAddReleasePoolID(t *testing.T) { t.Fatalf("Unexpected ref count for %s: %d", k1, subnets[k1].RefCount) } - pid2, _, _, err := a.RequestPool(localAddressSpace, "10.0.0.0/8", "10.0.0.0/16", nil, false) - if err != nil { - t.Fatal("Unexpected failure in adding sub pool") - } - if pid0 == pid1 || pid0 == pid2 || pid1 != pid2 { - t.Fatalf("Incorrect poolIDs returned %s, %s, %s", pid0, pid1, pid2) - } - if err := k2.FromString(pid2); err != nil { - t.Fatal(err) + _, _, _, err = a.RequestPool(localAddressSpace, "10.0.0.0/8", "10.0.0.0/16", nil, false) + if err == nil { + t.Fatal("Expected failure in adding sub pool") } aSpace, err = a.getAddrSpace(localAddressSpace) @@ -353,11 +345,8 @@ func TestAddReleasePoolID(t *testing.T) { } subnets = aSpace.subnets - if subnets[k2].RefCount != 2 { - t.Fatalf("Unexpected ref count for %s: %d", k2, subnets[k2].RefCount) - } - if subnets[k0].RefCount != 3 { + if subnets[k0].RefCount != 2 { t.Fatalf("Unexpected ref count for %s: %d", k0, subnets[k0].RefCount) } @@ -371,23 +360,13 @@ func TestAddReleasePoolID(t *testing.T) { } subnets = aSpace.subnets - if subnets[k0].RefCount != 2 { + if subnets[k0].RefCount != 1 { t.Fatalf("Unexpected ref count for %s: %d", k0, subnets[k0].RefCount) } if err := a.ReleasePool(pid0); err != nil { t.Fatal(err) } - aSpace, err = a.getAddrSpace(localAddressSpace) - if err != nil { - t.Fatal(err) - } - - subnets = aSpace.subnets - if subnets[k0].RefCount != 1 { - t.Fatalf("Unexpected ref count for %s: %d", k0, subnets[k0].RefCount) - } - pid00, _, _, err := a.RequestPool(localAddressSpace, "10.0.0.0/8", "", nil, false) if err != nil { t.Fatal("Unexpected failure in adding pool") @@ -401,20 +380,6 @@ func TestAddReleasePoolID(t *testing.T) { t.Fatal(err) } - subnets = aSpace.subnets - if subnets[k0].RefCount != 2 { - t.Fatalf("Unexpected ref count for %s: %d", k0, subnets[k0].RefCount) - } - - if err := a.ReleasePool(pid2); err != nil { - t.Fatal(err) - } - - aSpace, err = a.getAddrSpace(localAddressSpace) - if err != nil { - t.Fatal(err) - } - subnets = aSpace.subnets if subnets[k0].RefCount != 1 { t.Fatalf("Unexpected ref count for %s: %d", k0, subnets[k0].RefCount) diff --git a/libnetwork_internal_test.go b/libnetwork_internal_test.go index 58742cf5e1..172f535b4d 100644 --- a/libnetwork_internal_test.go +++ b/libnetwork_internal_test.go @@ -601,7 +601,7 @@ func TestIpamReleaseOnNetDriverFailures(t *testing.T) { } // Now create good bridge network with different gateway - ipamOpt2 := NetworkOptionIpam(ipamapi.DefaultIPAM, "", []*IpamConf{{PreferredPool: "10.34.0.0/16", Gateway: "10.34.255.253"}}, nil, nil) + ipamOpt2 := NetworkOptionIpam(ipamapi.DefaultIPAM, "", []*IpamConf{{PreferredPool: "10.35.0.0/16", Gateway: "10.35.255.253"}}, nil, nil) gnw, err = c.NewNetwork("bridge", "goodnet2", "", ipamOpt2) if err != nil { t.Fatal(err) @@ -614,7 +614,7 @@ func TestIpamReleaseOnNetDriverFailures(t *testing.T) { } defer ep.Delete(false) - expectedIP, _ := types.ParseCIDR("10.34.0.1/16") + expectedIP, _ := types.ParseCIDR("10.35.0.1/16") if !types.CompareIPNet(ep.Info().Iface().Address(), expectedIP) { t.Fatalf("Ipam release must have failed, endpoint has unexpected address: %v", ep.Info().Iface().Address()) }