Merge pull request #41763 from dhilipkumars/ImproveSrvCodeCov

Automatic merge from submit-queue

Improve Service controller's code coverage a little bit

**What this PR does / why we need it**:
Improves the code coverage for Service Controller
Before
```
go test --cover ./pkg/controller/service
ok      k8s.io/kubernetes/pkg/controller/service        0.101s  coverage: 23.4% of statements
```
After
```
go test --cover ./pkg/controller/service/
ok      k8s.io/kubernetes/pkg/controller/service        0.094s  coverage: 62.0% of statements
```

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #
More unit testing

**Special notes for your reviewer**:

**Release note**: 

```release-note
NONE
```

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,91 +307,56 @@ func TestGetNodeConditionPredicate(t *testing.T) {
 
 // TODO(a-robinson): Add tests for update/sync/delete.
 
-func TestSyncService(t *testing.T) {
-	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.syncService(keyExpected)
-	if err != nil {
-		t.Fatalf("sync service error: %v", err)
-	}
-
-	_, exist := controller.cache.get(keyExpected)
-	if exist {
-		t.Fatalf("sync service error, workingQueue should not contain service: %s any more", keyExpected)
-	}
-
-}
-
-func TestProcessServiceDeletion(t *testing.T) {
-	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)
-	}
-}
-
 func TestProcessServiceUpdate(t *testing.T) {
-	controller, _, _ := newController()
 
-	testServiceName := "sync-test-name"
+	var controller *ServiceController
-	testServiceUID := types.UID("sync-test-uid")
+	var cloud *fakecloud.FakeCloud
-	lbIP := "192.168.1.1"
-	testService := newService(testServiceName, testServiceUID, v1.ServiceTypeLoadBalancer)
-	testService.Spec.LoadBalancerIP = lbIP
 
-	keyExpected := testService.GetObjectMeta().GetNamespace() + "/" + testService.GetObjectMeta().GetName()
+	//A pair of old and new loadbalancer IP address
+	oldLBIP := "192.168.1.1"
+	newLBIP := "192.168.1.11"
 
-	controller.enqueueService(testService)
+	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 = testService
+				cachedServiceTest.state = svc
 				controller.cache.set(keyExpected, cachedServiceTest)
 
 				keyGot, quit := controller.workingQueue.Get()
@@ -393,24 +367,444 @@ func TestProcessServiceUpdate(t *testing.T) {
 					t.Fatalf("get service key error, expected: %s, got: %s", keyExpected, keyGot.(string))
 				}
 
-	copy, err := api.Scheme.DeepCopy(testService)
+				copy, err := api.Scheme.DeepCopy(svc)
 				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)
+				return newService
+
+			},
+			expectedFn: func(svc *v1.Service, err error, retryDuration time.Duration) error {
+
 				if err != nil {
-		t.Fatalf("update service error: %v", err)
+					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 {
-		t.Fatalf("update service error, workingQueue should contain service: %s", keyExpected)
+					return fmt.Errorf("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)
+					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) {
+
+	var controller *ServiceController
+	var cloud *fakecloud.FakeCloud
+
+	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()
+
+			},
+			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."),
+		},
+		*/
+
+		//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
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+
+		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()
+	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)
+		}
+	}
+}
+
+//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) {
+
+	//ServiceCache a common service cache for all the test cases
+	sc := &serviceCache{serviceMap: make(map[string]*cachedService)}
+
+	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")
+
+			},
+			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
+			},
+		},
+	}
+
+	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")
 	}
 }