From 702818e17646835de0589eb366e74933eac2bad7 Mon Sep 17 00:00:00 2001 From: Balraj Singh Date: Wed, 6 Dec 2017 02:16:12 +0000 Subject: [PATCH 1/2] Addressing duplicate IP issue on ingress network On leader change or leader reboot the restore logic in the allocator was allocating overlapping IP address for VIPs and Endpoints in the ingress network. The fix added as part of this commit ensures that during restore we allocate the existing VIP and endpoint. Signed-off-by: Balraj Singh (cherry picked from commit 5fd25d2bf65d543f0a36c0202a305d063bf443e5) --- manager/allocator/cnmallocator/networkallocator.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/manager/allocator/cnmallocator/networkallocator.go b/manager/allocator/cnmallocator/networkallocator.go index 53f9ffbeee..b89e72ed6e 100644 --- a/manager/allocator/cnmallocator/networkallocator.go +++ b/manager/allocator/cnmallocator/networkallocator.go @@ -404,6 +404,11 @@ func (na *cnmNetworkAllocator) IsServiceAllocated(s *api.Service, flags ...func( vipLoop: for _, vip := range s.Endpoint.VirtualIPs { if na.IsVIPOnIngressNetwork(vip) && networkallocator.IsIngressNetworkNeeded(s) { + // This checks the condition when ingress network is needed + // but allocation has not been done. + if _, ok := na.services[s.ID]; !ok { + return false + } continue vipLoop } for _, net := range specNetworks { From da24fae7067a35fa2ae7a156d240b5c8b01f16a2 Mon Sep 17 00:00:00 2001 From: Balraj Singh Date: Wed, 13 Dec 2017 23:23:57 +0000 Subject: [PATCH 2/2] Added unit test TestAllocatorRestoreForDuplicateIPs Signed-off-by: Balraj Singh (cherry picked from commit 2397ddf3c1da3a2265f7cd2a56fe22ea66b6e374) --- manager/allocator/allocator_test.go | 130 ++++++++++++++++++++++++++++ 1 file changed, 130 insertions(+) diff --git a/manager/allocator/allocator_test.go b/manager/allocator/allocator_test.go index e50e8c3664..ac3f74bc30 100644 --- a/manager/allocator/allocator_test.go +++ b/manager/allocator/allocator_test.go @@ -666,6 +666,136 @@ func TestNoDuplicateIPs(t *testing.T) { } } +func TestAllocatorRestoreForDuplicateIPs(t *testing.T) { + s := store.NewMemoryStore(nil) + assert.NotNil(t, s) + defer s.Close() + // Create 3 services with 1 task each + numsvcstsks := 3 + assert.NoError(t, s.Update(func(tx store.Tx) error { + // populate ingress network + in := &api.Network{ + ID: "ingress-nw-id", + Spec: api.NetworkSpec{ + Annotations: api.Annotations{ + Name: "default-ingress", + }, + Ingress: true, + }, + } + assert.NoError(t, store.CreateNetwork(tx, in)) + + for i := 0; i != numsvcstsks; i++ { + svc := &api.Service{ + ID: "testServiceID" + strconv.Itoa(i), + Spec: api.ServiceSpec{ + Annotations: api.Annotations{ + Name: "service" + strconv.Itoa(i), + }, + Endpoint: &api.EndpointSpec{ + Mode: api.ResolutionModeVirtualIP, + + Ports: []*api.PortConfig{ + { + Name: "", + Protocol: api.ProtocolTCP, + TargetPort: 8000, + PublishedPort: uint32(8001 + i), + }, + }, + }, + }, + Endpoint: &api.Endpoint{ + Ports: []*api.PortConfig{ + { + Name: "", + Protocol: api.ProtocolTCP, + TargetPort: 8000, + PublishedPort: uint32(8001 + i), + }, + }, + VirtualIPs: []*api.Endpoint_VirtualIP{ + { + NetworkID: "ingress-nw-id", + Addr: "10.0.0." + strconv.Itoa(2+i) + "/24", + }, + }, + }, + } + assert.NoError(t, store.CreateService(tx, svc)) + } + return nil + })) + + for i := 0; i != numsvcstsks; i++ { + assert.NoError(t, s.Update(func(tx store.Tx) error { + tsk := &api.Task{ + ID: "testTaskID" + strconv.Itoa(i), + Status: api.TaskStatus{ + State: api.TaskStateNew, + }, + ServiceID: "testServiceID" + strconv.Itoa(i), + DesiredState: api.TaskStateRunning, + } + assert.NoError(t, store.CreateTask(tx, tsk)) + return nil + })) + } + + assignedVIPs := make(map[string]bool) + assignedIPs := make(map[string]bool) + hasNoIPOverlapServices := func(fakeT assert.TestingT, service *api.Service) bool { + assert.NotEqual(fakeT, len(service.Endpoint.VirtualIPs), 0) + assert.NotEqual(fakeT, len(service.Endpoint.VirtualIPs[0].Addr), 0) + + assignedVIP := service.Endpoint.VirtualIPs[0].Addr + if assignedVIPs[assignedVIP] { + t.Fatalf("service %s assigned duplicate IP %s", service.ID, assignedVIP) + } + assignedVIPs[assignedVIP] = true + if assignedIPs[assignedVIP] { + t.Fatalf("a task and service %s have the same IP %s", service.ID, assignedVIP) + } + return true + } + + hasNoIPOverlapTasks := func(fakeT assert.TestingT, s *store.MemoryStore, task *api.Task) bool { + assert.NotEqual(fakeT, len(task.Networks), 0) + assert.NotEqual(fakeT, len(task.Networks[0].Addresses), 0) + + assignedIP := task.Networks[0].Addresses[0] + if assignedIPs[assignedIP] { + t.Fatalf("task %s assigned duplicate IP %s", task.ID, assignedIP) + } + assignedIPs[assignedIP] = true + if assignedVIPs[assignedIP] { + t.Fatalf("a service and task %s have the same IP %s", task.ID, assignedIP) + } + return true + } + + a, err := New(s, nil) + assert.NoError(t, err) + assert.NotNil(t, a) + // Start allocator + go func() { + assert.NoError(t, a.Run(context.Background())) + }() + defer a.Stop() + + taskWatch, cancel := state.Watch(s.WatchQueue(), api.EventUpdateTask{}, api.EventDeleteTask{}) + defer cancel() + + serviceWatch, cancel := state.Watch(s.WatchQueue(), api.EventUpdateService{}, api.EventDeleteService{}) + defer cancel() + + // Confirm tasks have no IPs that overlap with the services VIPs on restart + for i := 0; i != numsvcstsks; i++ { + watchTask(t, s, taskWatch, false, hasNoIPOverlapTasks) + watchService(t, serviceWatch, false, hasNoIPOverlapServices) + } +} + func TestNodeAllocator(t *testing.T) { s := store.NewMemoryStore(nil) assert.NotNil(t, s)