Improve codes according to review comments

Signed-off-by: Masaki Kimura <masaki.kimura@hitachivantara.com>

pkg/discovery/network/canal.go

@@ -54,7 +54,11 @@ func discoverCanalFlannelNetwork(clientSet kubernetes.Interface) (*ClusterNetwor
 
 	// Try to detect the service CIDRs using the generic functions
 	clusterIPRange, err := findClusterIPRange(clientSet)
-	if err == nil && clusterIPRange != "" {
+	if err != nil {
+		return nil, err
+	}
+
+	if clusterIPRange != "" {
 		clusterNetwork.ServiceCIDRs = []string{clusterIPRange}
 	}
 

pkg/discovery/network/generic.go

@@ -33,12 +33,20 @@ func discoverGenericNetwork(clientSet kubernetes.Interface) (*ClusterNetwork, er
 	}
 
 	podIPRange, err := findPodIPRange(clientSet)
-	if err == nil && podIPRange != "" {
+	if err != nil {
+		return nil, err
+	}
+
+	if podIPRange != "" {
 		clusterNetwork.PodCIDRs = []string{podIPRange}
 	}
 
 	clusterIPRange, err := findClusterIPRange(clientSet)
-	if err == nil && clusterIPRange != "" {
+	if err != nil {
+		return nil, err
+	}
+
+	if clusterIPRange != "" {
 		clusterNetwork.ServiceCIDRs = []string{clusterIPRange}
 	}
 
@@ -51,16 +59,16 @@ func discoverGenericNetwork(clientSet kubernetes.Interface) (*ClusterNetwork, er
 
 func findClusterIPRange(clientSet kubernetes.Interface) (string, error) {
 	clusterIPRange, err := findClusterIPRangeApiserver(clientSet)
-	if err == nil && clusterIPRange != "" {
-		return clusterIPRange, nil
+	if err != nil || clusterIPRange != "" {
+		return clusterIPRange, err
 	}
 
 	clusterIPRange, err = findClusterIPRangeSvcCreation(clientSet)
-	if err == nil && clusterIPRange != "" {
-		return clusterIPRange, nil
+	if err != nil || clusterIPRange != "" {
+		return clusterIPRange, err
 	}
 
-	return "", fmt.Errorf("clusterIPRange can't be found by any methods")
+	return "", nil
 }
 
 func findClusterIPRangeApiserver(clientSet kubernetes.Interface) (string, error) {
@@ -89,7 +97,7 @@ func findClusterIPRangeSvcCreation(clientSet kubernetes.Interface) (string, erro
 
 	// creating invalid svc didn't fail as expected
 	if err == nil {
-		return "", fmt.Errorf("creating invalid service(%v) didn't fail", invalidSvcSpec)
+		return "", nil
 	}
 
 	return parseToServiceCidr(err.Error())
@@ -115,21 +123,21 @@ func parseToServiceCidr(msg string) (string, error) {
 
 func findPodIPRange(clientSet kubernetes.Interface) (string, error) {
 	podIPRange, err := findPodIPRangeKubeController(clientSet)
-	if err == nil && podIPRange != "" {
-		return podIPRange, nil
+	if err != nil || podIPRange != "" {
+		return podIPRange, err
 	}
 
 	podIPRange, err = findPodIPRangeKubeProxy(clientSet)
-	if err == nil && podIPRange != "" {
-		return podIPRange, nil
+	if err != nil || podIPRange != "" {
+		return podIPRange, err
 	}
 
 	podIPRange, err = findPodIPRangeNodeSpec(clientSet)
-	if err == nil && podIPRange != "" {
-		return podIPRange, nil
+	if err != nil || podIPRange != "" {
+		return podIPRange, err
 	}
 
-	return "", fmt.Errorf("podIPRange can't be found by any methods")
+	return "", nil
 }
 
 func findPodIPRangeKubeController(clientSet kubernetes.Interface) (string, error) {
@@ -157,5 +165,5 @@ func parseToPodCidr(nodes []v1.Node) (string, error) {
 		}
 	}
 
-	return "", fmt.Errorf("no node with Spec.PodCIDR found")
+	return "", nil
 }

pkg/discovery/network/generic_test.go

@@ -84,7 +84,7 @@ var _ = Describe("discoverGenericNetwork", func() {
 			Expect(clusterNet).NotTo(BeNil())
 		})
 
-		It("Should return the ClusterNetwork structure with PodCIDR", func() {
+		It("Should return the ClusterNetwork structure with pod CIDR", func() {
 			Expect(clusterNet.PodCIDRs).To(Equal([]string{testPodCIDR}))
 		})
 
@@ -168,7 +168,7 @@ var _ = Describe("discoverGenericNetwork", func() {
 		})
 	})
 
-	When("There is no pod cidr information on node", func() {
+	When("No pod CIDR information exists on any node", func() {
 		It("Should return nil cluster network", func() {
 			clusterNet := testDiscoverGenericWith(
 				fakeNode("node1", ""),
@@ -178,7 +178,7 @@ var _ = Describe("discoverGenericNetwork", func() {
 		})
 	})
 
-	When("There is pod cidr information on any of nodes", func() {
+	When("Pod CIDR information exists on a node", func() {
 		var clusterNet *ClusterNetwork
 
 		BeforeEach(func() {
@@ -201,7 +201,7 @@ var _ = Describe("discoverGenericNetwork", func() {
 		})
 	})
 
-	When("There is pod cidr information on any of nodes and there is kubeapi pod", func() {
+	When("Both pod and service CIDR information exists", func() {
 		var clusterNet *ClusterNetwork
 
 		BeforeEach(func() {
@@ -221,16 +221,10 @@ var _ = Describe("discoverGenericNetwork", func() {
 		})
 	})
 
-	When("No resource and creating invalid svc doesn't return error", func() {
-		It("Should return nil cluster network", func() {
-			clusterNet := testDiscoverGenericEmulatedErrWith(nil)
-			Expect(clusterNet).To(BeNil())
-		})
-	})
-
 	When("No resource and creating invalid svc returns unexpected error", func() {
 		It("Should return nil cluster network", func() {
-			clusterNet := testDiscoverGenericEmulatedErrWith(errUnexpected)
+			clusterNet, err := doTestDiscoverGenericEmulatedErrWith(errUnexpected)
+			Expect(err).To(HaveOccurred())
 			Expect(clusterNet).To(BeNil())
 		})
 	})
@@ -256,26 +250,6 @@ var _ = Describe("discoverGenericNetwork", func() {
 
 	})
 
-	When("Node with cidr information and creating invalid svc returns expected error", func() {
-		var clusterNet *ClusterNetwork
-
-		BeforeEach(func() {
-			clusterNet = testDiscoverGenericEmulatedErrWith(
-				errExpected,
-				fakeNode("node1", testPodCIDR),
-			)
-		})
-
-		It("Should return ClusterNetwork with all CIDRs", func() {
-			Expect(clusterNet.ServiceCIDRs).To(Equal([]string{testServiceCIDR}))
-			Expect(clusterNet.PodCIDRs).To(Equal([]string{testPodCIDR}))
-		})
-
-		It("Should identify the networkplugin as generic", func() {
-			Expect(clusterNet.NetworkPlugin).To(BeIdenticalTo("generic"))
-		})
-	})
-
 })
 
 func testDiscoverGenericWith(objects ...runtime.Object) *ClusterNetwork {
@@ -285,22 +259,19 @@ func testDiscoverGenericWith(objects ...runtime.Object) *ClusterNetwork {
 	return clusterNet
 }
 
-func testDiscoverGenericEmulatedErrWith(expectedErr error, objects ...runtime.Object) *ClusterNetwork {
+func doTestDiscoverGenericEmulatedErrWith(expectedErr error, objects ...runtime.Object) (*ClusterNetwork, error) {
 	clientSet := fake.NewSimpleClientset(objects...)
 	// Inject error for create services to return expectedErr
-	if expectedErr != nil {
-		clientSet.CoreV1().(*fakecorev1.FakeCoreV1).PrependReactor("create", "services",
-			func(action testing.Action) (handled bool, ret runtime.Object, err error) {
-				return true, nil, expectedErr
-			})
-	} else {
-		clientSet.CoreV1().(*fakecorev1.FakeCoreV1).PrependReactor("create", "services",
-			func(action testing.Action) (handled bool, ret runtime.Object, err error) {
-				return true, &v1.Service{}, nil
-			})
-	}
+	clientSet.CoreV1().(*fakecorev1.FakeCoreV1).PrependReactor("create", "services",
+		func(action testing.Action) (handled bool, ret runtime.Object, err error) {
+			return true, nil, expectedErr
+		})
 
-	clusterNet, err := discoverGenericNetwork(clientSet)
+	return discoverGenericNetwork(clientSet)
+}
+
+func testDiscoverGenericEmulatedErrWith(expectedErr error, objects ...runtime.Object) *ClusterNetwork {
+	clusterNet, err := doTestDiscoverGenericEmulatedErrWith(expectedErr, objects...)
 	Expect(err).NotTo(HaveOccurred())
 	return clusterNet
 }

pkg/discovery/network/network_suite_test.go

@@ -69,14 +69,12 @@ func fakeService(namespace, name, component string) *v1.Service {
 }
 
 func fakeNode(name, podCIDR string) *v1.Node {
-	spec := v1.NodeSpec{}
-	if podCIDR != "" {
-		spec.PodCIDR = podCIDR
-	}
 	return &v1.Node{
 		ObjectMeta: v1meta.ObjectMeta{
 			Name: name,
 		},
-		Spec: spec,
+		Spec: v1.NodeSpec{
+			PodCIDR: podCIDR,
+		},
 	}
 }