diff --git a/pkg/controller/service/servicecontroller_test.go b/pkg/controller/service/servicecontroller_test.go index 168c4355590..0a7329f1d90 100644 --- a/pkg/controller/service/servicecontroller_test.go +++ b/pkg/controller/service/servicecontroller_test.go @@ -17,8 +17,10 @@ limitations under the License. package service import ( + "fmt" "reflect" "testing" + "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -35,7 +37,14 @@ import ( const region = "us-central" func newService(name string, uid types.UID, serviceType v1.ServiceType) *v1.Service { - return &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "namespace", UID: uid, SelfLink: testapi.Default.SelfLink("services", name)}, Spec: v1.ServiceSpec{Type: serviceType}} + return &v1.Service{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "default", UID: uid, SelfLink: testapi.Default.SelfLink("services", name)}, Spec: v1.ServiceSpec{Type: serviceType}} +} + +//Wrap newService so that you dont have to call default argumetns again and again. +func defaultExternalService() *v1.Service { + + return newService("external-balancer", types.UID("123"), v1.ServiceTypeLoadBalancer) + } func alwaysReady() bool { return true } @@ -298,119 +307,504 @@ func TestGetNodeConditionPredicate(t *testing.T) { // TODO(a-robinson): Add tests for update/sync/delete. +func TestProcessServiceUpdate(t *testing.T) { + + var controller *ServiceController + var cloud *fakecloud.FakeCloud + + //A pair of old and new loadbalancer IP address + oldLBIP := "192.168.1.1" + newLBIP := "192.168.1.11" + + testCases := []struct { + testName string + key string + updateFn func(*v1.Service) *v1.Service //Manipulate the structure + svc *v1.Service + expectedFn func(*v1.Service, error, time.Duration) error //Error comparision function + }{ + { + testName: "If updating a valid service", + key: "validKey", + svc: defaultExternalService(), + updateFn: func(svc *v1.Service) *v1.Service { + + controller, cloud, _ = newController() + controller.cache.getOrCreate("validKey") + return svc + + }, + expectedFn: func(svc *v1.Service, err error, retryDuration time.Duration) error { + + if err != nil { + return err + } + if retryDuration != doNotRetry { + return fmt.Errorf("retryDuration Expected=%v Obtained=%v", doNotRetry, retryDuration) + } + return nil + }, + }, + { + testName: "If Updating Loadbalancer IP", + key: "default/sync-test-name", + svc: newService("sync-test-name", types.UID("sync-test-uid"), v1.ServiceTypeLoadBalancer), + updateFn: func(svc *v1.Service) *v1.Service { + + svc.Spec.LoadBalancerIP = oldLBIP + + keyExpected := svc.GetObjectMeta().GetNamespace() + "/" + svc.GetObjectMeta().GetName() + controller.enqueueService(svc) + cachedServiceTest := controller.cache.getOrCreate(keyExpected) + cachedServiceTest.state = svc + controller.cache.set(keyExpected, cachedServiceTest) + + keyGot, quit := controller.workingQueue.Get() + if quit { + t.Fatalf("get no workingQueue element") + } + if keyExpected != keyGot.(string) { + t.Fatalf("get service key error, expected: %s, got: %s", keyExpected, keyGot.(string)) + } + + copy, err := api.Scheme.DeepCopy(svc) + if err != nil { + t.Fatalf("copy service error: %v", err) + } + newService := copy.(*v1.Service) + + newService.Spec.LoadBalancerIP = newLBIP + return newService + + }, + expectedFn: func(svc *v1.Service, err error, retryDuration time.Duration) error { + + if err != nil { + return err + } + if retryDuration != doNotRetry { + return fmt.Errorf("retryDuration Expected=%v Obtained=%v", doNotRetry, retryDuration) + } + + keyExpected := svc.GetObjectMeta().GetNamespace() + "/" + svc.GetObjectMeta().GetName() + + cachedServiceGot, exist := controller.cache.get(keyExpected) + if !exist { + return fmt.Errorf("update service error, workingQueue should contain service: %s", keyExpected) + } + if cachedServiceGot.state.Spec.LoadBalancerIP != newLBIP { + return fmt.Errorf("update LoadBalancerIP error, expected: %s, got: %s", newLBIP, cachedServiceGot.state.Spec.LoadBalancerIP) + } + return nil + }, + }, + } + + for _, tc := range testCases { + newSvc := tc.updateFn(tc.svc) + svcCache := controller.cache.getOrCreate(tc.key) + obtErr, retryDuration := controller.processServiceUpdate(svcCache, newSvc, tc.key) + if err := tc.expectedFn(newSvc, obtErr, retryDuration); err != nil { + t.Errorf("%v processServiceUpdate() %v", tc.testName, err) + } + } + +} + func TestSyncService(t *testing.T) { - controller, _, _ := newController() - testServiceName := "sync-test-name" - testServiceUID := types.UID("sync-test-uid") + var controller *ServiceController + var cloud *fakecloud.FakeCloud - testService := newService(testServiceName, testServiceUID, v1.ServiceTypeLoadBalancer) + testCases := []struct { + testName string + key string + updateFn func() //Function to manipulate the controller element to simulate error + expectedFn func(error) error //Expected function if returns nil then test passed, failed otherwise + }{ + { + testName: "if an invalid service name is synced", + key: "invalid/key/string", + updateFn: func() { + controller, cloud, _ = newController() - keyExpected := testService.GetObjectMeta().GetNamespace() + "/" + testService.GetObjectMeta().GetName() + }, + expectedFn: func(e error) error { + //TODO: Expected error is of the format fmt.Errorf("unexpected key format: %q", "invalid/key/string"), + //TODO: should find a way to test for dependent package errors in such a way that it wont break + //TODO: our tests, currently we only test if there is an error. + //Error should be non-nil + if e == nil { + return fmt.Errorf("Expected=unexpected key format: %q, Obtained=nil", "invalid/key/string") + } + return nil + }, + }, + /* We cannot open this test case as syncService(key) currently runtime.HandleError(err) and suppresses frequently occurring errors + { + testName: "if an invalid service is synced", + key: "somethingelse", + updateFn: func() { + controller, cloud, _ = newController() + srv := controller.cache.getOrCreate("external-balancer") + srv.state = defaultExternalService() + }, + expectedErr: fmt.Errorf("Service somethingelse not in cache even though the watcher thought it was. Ignoring the deletion."), + }, + */ - controller.enqueueService(testService) - cachedServiceTest := controller.cache.getOrCreate(keyExpected) - cachedServiceTest.state = testService - - controller.cache.set(keyExpected, cachedServiceTest) - - keyGot, quit := controller.workingQueue.Get() - - if quit { - t.Fatalf("get no workingQueue element") - } - if keyExpected != keyGot.(string) { - t.Fatalf("get service key error, expected: %s, got: %s", keyExpected, keyGot.(string)) + //TODO: see if we can add a test for valid but error throwing service, its difficult right now because synCService() currently runtime.HandleError + { + testName: "if valid service", + key: "external-balancer", + updateFn: func() { + testSvc := defaultExternalService() + controller, cloud, _ = newController() + controller.enqueueService(testSvc) + svc := controller.cache.getOrCreate("external-balancer") + svc.state = testSvc + }, + expectedFn: func(e error) error { + //error should be nil + if e != nil { + return fmt.Errorf("Expected=nil, Obtained=%v", e) + } + return nil + }, + }, } - err := controller.syncService(keyExpected) - if err != nil { - t.Fatalf("sync service error: %v", err) - } + for _, tc := range testCases { - _, exist := controller.cache.get(keyExpected) - if exist { - t.Fatalf("sync service error, workingQueue should not contain service: %s any more", keyExpected) - } + tc.updateFn() + obtainedErr := controller.syncService(tc.key) + //expected matches obtained ??. + if exp := tc.expectedFn(obtainedErr); exp != nil { + t.Errorf("%v Error:%v", tc.testName, exp) + } + + //Post processing, the element should not be in the sync queue. + _, exist := controller.cache.get(tc.key) + if exist { + t.Fatalf("%v working Queue should be empty, but contains %s", tc.testName, tc.key) + } + } } func TestProcessServiceDeletion(t *testing.T) { + + var controller *ServiceController + var cloud *fakecloud.FakeCloud + //Add a global svcKey name + svcKey := "external-balancer" + + testCases := []struct { + testName string + updateFn func(*ServiceController) //Update function used to manupulate srv and controller values + expectedFn func(svcErr error, retryDuration time.Duration) error //Function to check if the returned value is expected + }{ + { + testName: "If an non-existant service is deleted", + updateFn: func(controller *ServiceController) { + //Does not do anything + }, + expectedFn: func(svcErr error, retryDuration time.Duration) error { + + expectedError := "Service external-balancer not in cache even though the watcher thought it was. Ignoring the deletion." + if svcErr == nil || svcErr.Error() != expectedError { + //cannot be nil or Wrong error message + return fmt.Errorf("Expected=%v Obtained=%v", expectedError, svcErr) + } + + if retryDuration != doNotRetry { + //Retry duration should match + return fmt.Errorf("RetryDuration Expected=%v Obtained=%v", doNotRetry, retryDuration) + } + + return nil + }, + }, + { + testName: "If cloudprovided failed to delete the service", + updateFn: func(controller *ServiceController) { + + svc := controller.cache.getOrCreate(svcKey) + svc.state = defaultExternalService() + cloud.Err = fmt.Errorf("Error Deleting the Loadbalancer") + + }, + expectedFn: func(svcErr error, retryDuration time.Duration) error { + + expectedError := "Error Deleting the Loadbalancer" + + if svcErr == nil || svcErr.Error() != expectedError { + return fmt.Errorf("Expected=%v Obtained=%v", expectedError, svcErr) + } + + if retryDuration != minRetryDelay { + return fmt.Errorf("RetryDuration Expected=%v Obtained=%v", minRetryDelay, retryDuration) + } + return nil + }, + }, + { + testName: "If delete was successful", + updateFn: func(controller *ServiceController) { + + testSvc := defaultExternalService() + controller.enqueueService(testSvc) + svc := controller.cache.getOrCreate(svcKey) + svc.state = testSvc + controller.cache.set(svcKey, svc) + + }, + expectedFn: func(svcErr error, retryDuration time.Duration) error { + + if svcErr != nil { + return fmt.Errorf("Expected=nil Obtained=%v", svcErr) + } + + if retryDuration != doNotRetry { + //Retry duration should match + return fmt.Errorf("RetryDuration Expected=%v Obtained=%v", doNotRetry, retryDuration) + } + + //It should no longer be in the workqueue. + _, exist := controller.cache.get(svcKey) + if exist { + return fmt.Errorf("delete service error, workingQueue should not contain service: %s any more", svcKey) + } + + return nil + }, + }, + } + + for _, tc := range testCases { + //Create a new controller. + controller, cloud, _ = newController() + tc.updateFn(controller) + obtainedErr, retryDuration := controller.processServiceDeletion(svcKey) + if err := tc.expectedFn(obtainedErr, retryDuration); err != nil { + t.Errorf("%v processServiceDeletion() %v", tc.testName, err) + } + } + +} + +func TestDoesExternalLoadBalancerNeedsUpdate(t *testing.T) { + + var oldSvc, newSvc *v1.Service + + testCases := []struct { + testName string //Name of the test case + updateFn func() //Function to update the service object + expectedNeedsUpdate bool //needsupdate always returns bool + + }{ + { + testName: "If the service type is changed from LoadBalancer to ClusterIP", + updateFn: func() { + oldSvc = defaultExternalService() + newSvc = defaultExternalService() + newSvc.Spec.Type = v1.ServiceTypeClusterIP + }, + expectedNeedsUpdate: true, + }, + { + testName: "If the Ports are different", + updateFn: func() { + oldSvc = defaultExternalService() + newSvc = defaultExternalService() + oldSvc.Spec.Ports = []v1.ServicePort{ + { + Port: 8000, + }, + { + Port: 9000, + }, + { + Port: 10000, + }, + } + newSvc.Spec.Ports = []v1.ServicePort{ + { + Port: 8001, + }, + { + Port: 9001, + }, + { + Port: 10001, + }, + } + + }, + expectedNeedsUpdate: true, + }, + { + testName: "If externel ip counts are different", + updateFn: func() { + oldSvc = defaultExternalService() + newSvc = defaultExternalService() + oldSvc.Spec.ExternalIPs = []string{"old.IP.1"} + newSvc.Spec.ExternalIPs = []string{"new.IP.1", "new.IP.2"} + }, + expectedNeedsUpdate: true, + }, + { + testName: "If externel ips are different", + updateFn: func() { + oldSvc = defaultExternalService() + newSvc = defaultExternalService() + oldSvc.Spec.ExternalIPs = []string{"old.IP.1", "old.IP.2"} + newSvc.Spec.ExternalIPs = []string{"new.IP.1", "new.IP.2"} + }, + expectedNeedsUpdate: true, + }, + { + testName: "If UID is different", + updateFn: func() { + oldSvc = defaultExternalService() + newSvc = defaultExternalService() + oldSvc.UID = types.UID("UID old") + newSvc.UID = types.UID("UID new") + }, + expectedNeedsUpdate: true, + }, + } + controller, _, _ := newController() - - testServiceName := "sync-test-name" - testServiceUID := types.UID("sync-test-uid") - testService := newService(testServiceName, testServiceUID, v1.ServiceTypeLoadBalancer) - - keyExpected := testService.GetObjectMeta().GetNamespace() + "/" + testService.GetObjectMeta().GetName() - - controller.enqueueService(testService) - cachedServiceTest := controller.cache.getOrCreate(keyExpected) - cachedServiceTest.state = testService - controller.cache.set(keyExpected, cachedServiceTest) - - keyGot, quit := controller.workingQueue.Get() - - if quit { - t.Fatalf("get no workingQueue element") - } - if keyExpected != keyGot.(string) { - t.Fatalf("get service key error, expected: %s, got: %s", keyExpected, keyGot.(string)) - } - - err, _ := controller.processServiceDeletion(keyExpected) - if err != nil { - t.Fatalf("delete service error: %v", err) - } - - _, exist := controller.cache.get(keyExpected) - if exist { - t.Fatalf("delete service error, workingQueue should not contain service: %s any more", keyExpected) + for _, tc := range testCases { + tc.updateFn() + obtainedResult := controller.needsUpdate(oldSvc, newSvc) + if obtainedResult != tc.expectedNeedsUpdate { + t.Errorf("%v needsUpdate() should have returned %v but returned %v", tc.testName, tc.expectedNeedsUpdate, obtainedResult) + } } } -func TestProcessServiceUpdate(t *testing.T) { - controller, _, _ := newController() +//All the testcases for ServiceCache uses a single cache, these below test cases should be run in order, +//as tc1 (addCache would add elements to the cache) +//and tc2 (delCache would remove element from the cache without it adding automatically) +//Please keep this in mind while adding new test cases. +func TestServiceCache(t *testing.T) { - testServiceName := "sync-test-name" - testServiceUID := types.UID("sync-test-uid") - lbIP := "192.168.1.1" - testService := newService(testServiceName, testServiceUID, v1.ServiceTypeLoadBalancer) - testService.Spec.LoadBalancerIP = lbIP + //ServiceCache a common service cache for all the test cases + sc := &serviceCache{serviceMap: make(map[string]*cachedService)} - keyExpected := testService.GetObjectMeta().GetNamespace() + "/" + testService.GetObjectMeta().GetName() + testCases := []struct { + testName string + setCacheFn func() + checkCacheFn func() error + }{ + { + testName: "Add", + setCacheFn: func() { + cS := sc.getOrCreate("addTest") + cS.state = defaultExternalService() + }, + checkCacheFn: func() error { + //There must be exactly one element + if len(sc.serviceMap) != 1 { + return fmt.Errorf("Expected=1 Obtained=%d", len(sc.serviceMap)) + } + return nil + }, + }, + { + testName: "Del", + setCacheFn: func() { + sc.delete("addTest") - controller.enqueueService(testService) - cachedServiceTest := controller.cache.getOrCreate(keyExpected) - cachedServiceTest.state = testService - controller.cache.set(keyExpected, cachedServiceTest) - - keyGot, quit := controller.workingQueue.Get() - if quit { - t.Fatalf("get no workingQueue element") - } - if keyExpected != keyGot.(string) { - t.Fatalf("get service key error, expected: %s, got: %s", keyExpected, keyGot.(string)) + }, + checkCacheFn: func() error { + //Now it should have no element + if len(sc.serviceMap) != 0 { + return fmt.Errorf("Expected=0 Obtained=%d", len(sc.serviceMap)) + } + return nil + }, + }, + { + testName: "Set and Get", + setCacheFn: func() { + sc.set("addTest", &cachedService{state: defaultExternalService()}) + }, + checkCacheFn: func() error { + //Now it should have one element + Cs, bool := sc.get("addTest") + if !bool { + return fmt.Errorf("is Available Expected=true Obtained=%v", bool) + } + if Cs == nil { + return fmt.Errorf("CachedService expected:non-nil Obtained=nil") + } + return nil + }, + }, + { + testName: "ListKeys", + setCacheFn: func() { + //Add one more entry here + sc.set("addTest1", &cachedService{state: defaultExternalService()}) + }, + checkCacheFn: func() error { + //It should have two elements + keys := sc.ListKeys() + if len(keys) != 2 { + return fmt.Errorf("Elementes Expected=2 Obtained=%v", len(keys)) + } + return nil + }, + }, + { + testName: "GetbyKeys", + setCacheFn: nil, //Nothing to set + checkCacheFn: func() error { + //It should have two elements + svc, isKey, err := sc.GetByKey("addTest") + if svc == nil || isKey == false || err != nil { + return fmt.Errorf("Expected(non-nil, true, nil) Obtained(%v,%v,%v)", svc, isKey, err) + } + return nil + }, + }, + { + testName: "allServices", + setCacheFn: nil, //Nothing to set + checkCacheFn: func() error { + //It should return two elements + svcArray := sc.allServices() + if len(svcArray) != 2 { + return fmt.Errorf("Expected(2) Obtained(%v)", len(svcArray)) + } + return nil + }, + }, } - copy, err := api.Scheme.DeepCopy(testService) - if err != nil { - t.Fatalf("copy service error: %v", err) - } - newService := copy.(*v1.Service) - - newLBIP := "192.168.1.11" - newService.Spec.LoadBalancerIP = newLBIP - err, _ = controller.processServiceUpdate(cachedServiceTest, newService, keyExpected) - if err != nil { - t.Fatalf("update service error: %v", err) - } - - cachedServiceGot, exist := controller.cache.get(keyExpected) - if !exist { - t.Fatalf("update service error, workingQueue should contain service: %s", keyExpected) - } - if cachedServiceGot.state.Spec.LoadBalancerIP != newLBIP { - t.Fatalf("update LoadBalancerIP error, expected: %s, got: %s", newLBIP, cachedServiceGot.state.Spec.LoadBalancerIP) + for _, tc := range testCases { + if tc.setCacheFn != nil { + tc.setCacheFn() + } + if err := tc.checkCacheFn(); err != nil { + t.Errorf("%v returned %v", tc.testName, err) + } + } +} + +//Test a utility functions as its not easy to unit test nodeSyncLoop directly +func TestNodeSlicesEqualForLB(t *testing.T) { + numNodes := 10 + nArray := make([]*v1.Node, 10) + + for i := 0; i < numNodes; i++ { + nArray[i] = &v1.Node{} + nArray[i].Name = fmt.Sprintf("node1") + } + if !nodeSlicesEqualForLB(nArray, nArray) { + t.Errorf("nodeSlicesEqualForLB() Expected=true Obtained=false") } }