From 3866d7a9f4c5f04ea652fa18be1469a0d3d78a28 Mon Sep 17 00:00:00 2001 From: Masaki Kimura Date: Wed, 3 Feb 2021 21:15:36 +0000 Subject: [PATCH] Improve codes according to review comments Signed-off-by: Masaki Kimura --- pkg/discovery/network/canal.go | 6 +- pkg/discovery/network/generic.go | 40 ++++++++------ pkg/discovery/network/generic_test.go | 61 ++++++--------------- pkg/discovery/network/network_suite_test.go | 8 +-- 4 files changed, 48 insertions(+), 67 deletions(-) diff --git a/pkg/discovery/network/canal.go b/pkg/discovery/network/canal.go index 2b2330a588..71d39ac6e2 100644 --- a/pkg/discovery/network/canal.go +++ b/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} } diff --git a/pkg/discovery/network/generic.go b/pkg/discovery/network/generic.go index e39a0b70e9..230da32c52 100644 --- a/pkg/discovery/network/generic.go +++ b/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 } diff --git a/pkg/discovery/network/generic_test.go b/pkg/discovery/network/generic_test.go index 1f34992021..3b4c98cf59 100644 --- a/pkg/discovery/network/generic_test.go +++ b/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 } diff --git a/pkg/discovery/network/network_suite_test.go b/pkg/discovery/network/network_suite_test.go index cc62634d4a..d7af38cb57 100644 --- a/pkg/discovery/network/network_suite_test.go +++ b/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, + }, } }