Repository: clusternet/clusternet
Pull request #833 (2024-11-28 07:04:52)

optimize/rewrite the logic and unit test of finding cluster service/pod ip range
#### What type of PR is this?

Fix or Optimize

#### What this PR does / why we need it:

According to the original logic, but fix some issues and optimize: 

1. Remove find pod ip range from pod's spec. The `node.spec.podCIDR` field is not the cluster pod ip range, but the node-assigned.
> `kubectl explain node.spec.podCIDR` shows:
> PodCIDR represents the pod IP range assigned to the node

2. Correct the label selector of kube-proxy from `component=kube-proxy` to `k8s-app=kube-proxy`

3. Similar to `findPodIPRange` function, add the redundant finding from kube-controller-manager.

4. rewrite the unit test for testing all logic and optimize the finding parameter of `labels.Selector`. 



Patch:
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index aed01f4..9bfd63f 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -30,7 +30,7 @@ jobs:
           fetch-depth: 1
           path: ${{ env.GOPATH }}/src/github.com/${{ github.repository }}
       - name: Cache go modules and build cache
-        uses: actions/cache@v4.1.1
+        uses: actions/cache@v4.1.2
         with:
           # In order:
           # * Module download cache
@@ -82,7 +82,7 @@ jobs:
       - name: Rebase code
         run: git rebase origin/main
       - name: Cache go modules and build cache
-        uses: actions/cache@v4.1.1
+        uses: actions/cache@v4.1.2
         with:
           # In order:
           # * Module download cache
@@ -135,7 +135,7 @@ jobs:
       - name: Rebase code
         run: git rebase origin/main
       - name: Cache go modules and build cache
-        uses: actions/cache@v4.1.1
+        uses: actions/cache@v4.1.2
         with:
           # In order:
           # * Module download cache
@@ -155,7 +155,7 @@ jobs:
         run: make test
       - name: Upload coverage to Codecov
         if: matrix.go-version == '1.21.x' && github.repository_owner == 'clusternet'
-        uses: codecov/codecov-action@v5.0.2
+        uses: codecov/codecov-action@v5.0.7
         with:
           token: ${{ secrets.CODECOV_TOKEN }} # required
           files: ${{ env.GOPATH }}/src/github.com/${{ github.repository }}/coverage.out
diff --git a/Dockerfile b/Dockerfile
index 97258c5..195fce2 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -1,7 +1,7 @@
-ARG BASEIMAGE
-ARG GOVERSION
-ARG LDFLAGS
-ARG PKGNAME
+ARG BASEIMAGE=alpine:3.18.4
+ARG GOVERSION=1.21.3
+ARG LDFLAGS=""
+ARG PKGNAME=clusternet-hub
 
 # Build the manager binary
 FROM --platform=$BUILDPLATFORM golang:${GOVERSION} as builder
diff --git a/pkg/controllers/clusters/clusterstatus/clusterstatus_controller.go b/pkg/controllers/clusters/clusterstatus/clusterstatus_controller.go
index 914bbae..36dd797 100644
--- a/pkg/controllers/clusters/clusterstatus/clusterstatus_controller.go
+++ b/pkg/controllers/clusters/clusterstatus/clusterstatus_controller.go
@@ -320,7 +320,7 @@ func (c *Controller) discoverServiceCIDR() (string, error) {
 
 // discoverClusterCIDR returns the cluster CIDR for the cluster.
 func (c *Controller) discoverClusterCIDR() (string, error) {
-	return findPodIPRange(c.nodeLister, c.podLister)
+	return findPodIPRange(c.podLister)
 }
 
 // get node capacity and allocatable resource
diff --git a/pkg/controllers/clusters/clusterstatus/network.go b/pkg/controllers/clusters/clusterstatus/network.go
index a54bbae..ca6497d 100644
--- a/pkg/controllers/clusters/clusterstatus/network.go
+++ b/pkg/controllers/clusters/clusterstatus/network.go
@@ -23,151 +23,106 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/labels"
-	"k8s.io/apimachinery/pkg/selection"
-	corev1Lister "k8s.io/client-go/listers/core/v1"
+	corelisters "k8s.io/client-go/listers/core/v1"
 	"k8s.io/klog/v2"
 )
 
-// findServiceIPRange returns the cluster IP range for the cluster service.
-// copied from submariner.io/submariner-operator/pkg/discovery/network/generic.go and modified
-func findServiceIPRange(podLister corev1Lister.PodLister) (string, error) {
-	clusterIPRange := findPodCommandParameter(podLister, "kube-apiserver", "--service-cluster-ip-range")
-	if clusterIPRange != "" {
-		return clusterIPRange, nil
+// findServiceIPRange returns the service ip range for the cluster.
+func findServiceIPRange(podLister corelisters.PodLister) (string, error) {
+	// Try to find the service ip range from the kube-apiserver first
+	// and then kube-controller-manager if failed
+	labelKeys := []string{"component", "app.kubernetes.io/component"}
+	labelValues := []string{"kube-apiserver", "kube-controller-manager"}
+	parameter := "--service-cluster-ip-range"
+	for _, labelValue := range labelValues {
+		for _, labelKey := range labelKeys {
+			labelSelector := labels.SelectorFromSet(labels.Set{labelKey: labelValue})
+			serviceIPRange := findPodCommandParameter(podLister, labelSelector, parameter)
+			if serviceIPRange != "" {
+				return serviceIPRange, nil
+			}
+		}
 	}
-	// Try to find the pod IP range from the env.
-	clusterIPRange = os.Getenv("SERVICE_CIDR")
-	if clusterIPRange != "" {
-		return clusterIPRange, nil
+
+	// Try to find the service ip range from the env.
+	serviceIPRange := os.Getenv("SERVICE_CIDR")
+	if serviceIPRange != "" {
+		return serviceIPRange, nil
 	}
-	return "", errors.New("can't get ServiceIPRange")
+	return "", errors.New("can't get service ip range")
 }
 
-// findPodIpRange returns the pod IP range for the cluster.
-// copied from submariner.io/submariner-operator/pkg/discovery/network/generic.go
-func findPodIPRange(nodeLister corev1Lister.NodeLister, podLister corev1Lister.PodLister) (string, error) {
-	// Try to find the pod IP range from the kube-controller-manager.
-	podIPRange := findPodIPRangeKubeController(podLister)
-	if podIPRange != "" {
-		return podIPRange, nil
+// findPodIPRange returns the pod ip range for the cluster.
+func findPodIPRange(podLister corelisters.PodLister) (string, error) {
+	// Try to find the pod ip range from the kube-controller-manager not including kube-apiserver
+	labelKeys := []string{"component", "app.kubernetes.io/component"}
+	labelValues := []string{"kube-controller-manager"}
+	parameter := "--cluster-cidr"
+	for _, labelValue := range labelValues {
+		for _, labelKey := range labelKeys {
+			labelSelector := labels.SelectorFromSet(labels.Set{labelKey: labelValue})
+			podIPRange := findPodCommandParameter(podLister, labelSelector, parameter)
+			if podIPRange != "" {
+				return podIPRange, nil
+			}
+		}
 	}
 
-	// Try to find the pod IP range from the kube-proxy.
-	podIPRange = findPodIPRangeKubeProxy(podLister)
+	// Try to find the pod ip range from the kube-proxy
+	labelKey, labelValue := "k8s-app", "kube-proxy"
+	labelSelector := labels.SelectorFromSet(labels.Set{labelKey: labelValue})
+	podIPRange := findPodCommandParameter(podLister, labelSelector, parameter)
 	if podIPRange != "" {
 		return podIPRange, nil
 	}
 
-	// Try to find the pod IP range from the node spec.
-	podIPRange = findPodIPRangeFromNodeSpec(nodeLister)
-	if podIPRange != "" {
-		return podIPRange, nil
-	}
-	// Try to find the pod IP range from the env.
+	// Try to find the pod ip range from the env.
 	podIPRange = os.Getenv("CLUSTER_CIDR")
 	if podIPRange != "" {
 		return podIPRange, nil
 	}
-	return "", errors.New("can't get PodIPRange")
-}
-
-// copied from submariner.io/submariner-operator/pkg/discovery/network/generic.go
-func findPodIPRangeKubeController(podLister corev1Lister.PodLister) string {
-	return findPodCommandParameter(podLister, "kube-controller-manager", "--cluster-cidr")
+	return "", errors.New("can't get pod ip range")
 }
 
-// copied from submariner.io/submariner-operator/pkg/discovery/network/generic.go
-func findPodIPRangeKubeProxy(podLister corev1Lister.PodLister) string {
-	return findPodCommandParameter(podLister, "kube-proxy", "--cluster-cidr")
-}
-
-// copied from submariner.io/submariner-operator/pkg/discovery/network/generic.go
-func findPodIPRangeFromNodeSpec(nodeLister corev1Lister.NodeLister) string {
-	nodes, err := nodeLister.List(labels.Everything())
+// findPodCommandParameter returns the pod container command parameter by the given labelSelector.
+func findPodCommandParameter(podLister corelisters.PodLister, labelSelector labels.Selector,
+	parameter string) string {
+	pods, err := findPods(podLister, labelSelector)
 	if err != nil {
-		klog.Errorf("Failed to list nodes: %v", err)
+		klog.Errorf("Failed to find pods by label selector %q: %v", labelSelector, err)
 		return ""
 	}
 
-	for _, node := range nodes {
-		if node.Spec.PodCIDR != "" {
-			return node.Spec.PodCIDR
+	for _, pod := range pods {
+		for _, container := range pod.Spec.Containers {
+			if val := getParameterValue(container.Command, parameter); val != "" {
+				return val
+			}
+			if val := getParameterValue(container.Args, parameter); val != "" {
+				return val
+			}
 		}
 	}
 
 	return ""
 }
 
-// findPodCommandParameter returns the pod container command parameter for the given pod.
-// copied from submariner.io/submariner-operator/pkg/discovery/network/pods.go
-func findPodCommandParameter(podLister corev1Lister.PodLister, labelSelectorValue, parameter string) string {
-	pod := findPodWithComponentLabel(podLister, labelSelectorValue)
-
-	if pod == nil {
-		pod = findPodWithK8sComponentLabel(podLister, labelSelectorValue)
-	}
-
-	if pod == nil {
-		return ""
-	}
-	for _, container := range pod.Spec.Containers {
-		if val := getParaValue(container.Command, parameter); val != "" {
-			return val
-		}
-
-		if val := getParaValue(container.Args, parameter); val != "" {
-			return val
-		}
-	}
-	return ""
-}
-
-func findPodWithComponentLabel(podLister corev1Lister.PodLister, labelSelectorValue string) *corev1.Pod {
-	pod, err := findPod(podLister, "component", labelSelectorValue)
-
-	if err != nil {
-		return nil
-	}
-	return pod
-}
-
-func findPodWithK8sComponentLabel(podLister corev1Lister.PodLister, labelSelectorValue string) *corev1.Pod {
-	pod, err := findPod(podLister, "app.kubernetes.io/component", labelSelectorValue)
-
-	if err != nil {
-		return nil
-	}
-	return pod
-}
-
-// findPod returns the pods filter by the given labelSelector.
-// copied from submariner.io/submariner-operator/pkg/discovery/network/pods.go
-func findPod(podLister corev1Lister.PodLister, labelSelectorKey, labelSelectorValue string) (*corev1.Pod, error) {
-	labelSelector := labels.NewSelector()
-	requirement, err := labels.NewRequirement(labelSelectorKey, selection.Equals, []string{labelSelectorValue})
-	if err != nil {
-		return nil, err
-	}
-	labelSelector = labelSelector.Add(*requirement)
-
+// findPods returns the pods filter by the given labelSelector.
+func findPods(podLister corelisters.PodLister, labelSelector labels.Selector) ([]*corev1.Pod, error) {
 	pods, err := podLister.List(labelSelector)
 	if err != nil {
-		klog.Errorf("Failed to list pods by label selector %q: %v", labelSelector, err)
 		return nil, err
 	}
 
-	if len(pods) == 0 {
-		return nil, nil
-	}
-
-	return pods[0], nil
+	return pods, nil
 }
 
-func getParaValue(lists []string, parameter string) string {
-	for _, arg := range lists {
+func getParameterValue(args []string, parameter string) string {
+	for _, arg := range args {
 		if strings.HasPrefix(arg, parameter) {
 			return strings.Split(arg, "=")[1]
 		}
+
 		// Handling the case where the command is in the form of /bin/sh -c exec ....
 		if strings.Contains(arg, " ") {
 			for _, subArg := range strings.Split(arg, " ") {


Test patch:
diff --git a/pkg/controllers/clusters/clusterstatus/network_test.go b/pkg/controllers/clusters/clusterstatus/network_test.go
index 8bb8f69..6670842 100644
--- a/pkg/controllers/clusters/clusterstatus/network_test.go
+++ b/pkg/controllers/clusters/clusterstatus/network_test.go
@@ -5,170 +5,170 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/util/uuid"
-	"k8s.io/client-go/informers"
+	kubeinformers "k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes/fake"
 	corelisters "k8s.io/client-go/listers/core/v1"
-	"k8s.io/client-go/tools/cache"
 )
 
-func TestFindPodCommandParameterFromArgs(t *testing.T) {
-	podArgs := BuildPod("kube-apiserver", "component", "kube-apiserver")
-	podArgs.Spec.Containers = []corev1.Container{
-		{
-			Args: []string{"--service-cluster-ip-range=1.2.3.4"},
-		},
-	}
-
-	kubeClient := fake.NewSimpleClientset()
-	kubeFactory := informers.NewSharedInformerFactory(kubeClient, 0)
-	podInformer := kubeFactory.Core().V1().Pods()
-
-	err := podInformer.Informer().GetIndexer().Add(podArgs)
-	if err != nil {
-		t.Errorf("Add pod err: %v", err)
-	}
-
-	podArgsLister := podInformer.Lister()
-
+func TestFindServiceIPRange(t *testing.T) {
 	tests := []struct {
 		description string
-		podLister   corelisters.PodLister
-		expectedIP  string
+		buildPod    func(string, string) (*corev1.Pod, labels.Selector)
 	}{
 		{
-			description: "service-cluster-ip-range is in args",
-			podLister:   podArgsLister,
-			expectedIP:  "1.2.3.4",
+			description: "service ip range in kube-apiserver",
+			buildPod:    buildAPIServerPod,
 		},
-	}
-
-	for _, test := range tests {
-		clusterIPRange := findPodCommandParameter(test.podLister, "kube-apiserver", "--service-cluster-ip-range")
-		if clusterIPRange != test.expectedIP {
-			t.Errorf("get clusteriprange is %v", clusterIPRange)
-		}
-	}
-
-}
-
-func TestFindPodCommandParameterFromArgsWithK8sLabel(t *testing.T) {
-	podArgs := BuildPod("kube-apiserver", "app.kubernetes.io/component", "kube-apiserver")
-	podArgs.Spec.Containers = []corev1.Container{
 		{
-			Args: []string{"--service-cluster-ip-range=1.2.3.4"},
+			description: "service ip range in kube-controller-manager",
+			buildPod:    buildControllerManagerWithK8sLabel,
 		},
 	}
 
-	kubeClient := fake.NewSimpleClientset()
-	kubeFactory := informers.NewSharedInformerFactory(kubeClient, 0)
-	podInformer := kubeFactory.Core().V1().Pods()
+	const expected = "1.0.0.0/8"
 
-	err := podInformer.Informer().GetIndexer().Add(podArgs)
-	if err != nil {
-		t.Errorf("Add pod err: %v", err)
-	}
+	for _, test := range tests {
+		pod, _ := test.buildPod(expected, "0.0.0.0/0")
+
+		podLister, err := buildPodLister(pod)
+		if err != nil {
+			t.Errorf("new pod lister error: %v", err)
+			continue
+		}
 
-	podArgsLister := podInformer.Lister()
+		result, _ := findServiceIPRange(podLister)
+		if result != expected {
+			t.Errorf("test for %s: expected %s, got %s", test.description, expected, result)
+		}
+	}
+}
 
+func TestFindPodIPRange(t *testing.T) {
 	tests := []struct {
 		description string
-		podLister   corelisters.PodLister
-		expectedIP  string
+		buildPod    func(string, string) (*corev1.Pod, labels.Selector)
 	}{
 		{
-			description: "service-cluster-ip-range is in args",
-			podLister:   podArgsLister,
-			expectedIP:  "1.2.3.4",
+			description: "pod ip range in kube-controller-manager",
+			buildPod:    buildControllerManagerWithK8sLabel,
+		},
+		{
+			description: "pod ip range in kube-proxy",
+			buildPod:    buildProxyPod,
 		},
 	}
 
-	for _, test := range tests {
-		clusterIPRange := findPodCommandParameter(test.podLister, "kube-apiserver", "--service-cluster-ip-range")
-		if clusterIPRange != test.expectedIP {
-			t.Errorf("get clusteriprange is %v", clusterIPRange)
-		}
-	}
+	const expected = "2.0.0.0/8"
 
-}
+	for _, test := range tests {
+		pod, _ := test.buildPod("0.0.0.0/0", expected)
 
-func TestFindPodCommandParameterFromCommand(t *testing.T) {
-	podCommand := BuildPod("kube-apiserver", "component", "kube-apiserver")
-	podCommand.Spec.Containers = []corev1.Container{
-		{
-			Command: []string{"--service-cluster-ip-range=1.2.3.4"},
-		},
-	}
+		podLister, err := buildPodLister(pod)
+		if err != nil {
+			t.Errorf("new pod lister error: %v", err)
+			continue
+		}
 
-	store := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{"namespace": cache.MetaNamespaceIndexFunc})
-	lister := corelisters.NewPodLister(store)
-	if err := store.Add(podCommand); err != nil {
-		t.Errorf("add pod err %v", err)
+		result, _ := findPodIPRange(podLister)
+		if result != expected {
+			t.Errorf("test for %s: expected %s, got %s", test.description, expected, result)
+		}
 	}
+}
 
+func TestFindPodCommandParameter(t *testing.T) {
 	tests := []struct {
 		description string
-		podLister   corelisters.PodLister
-		expectedIP  string
+		buildPod    func(string, string) (*corev1.Pod, labels.Selector)
 	}{
 		{
-			description: "service-cluster-ip-range is in command",
-			podLister:   lister,
-			expectedIP:  "1.2.3.4",
+			description: "parameter in command",
+			buildPod:    buildAPIServerPod,
+		},
+		{
+			description: "parameter in args",
+			buildPod:    buildAPIServerPodWithArgs,
 		},
 	}
 
+	const parameter = "--service-cluster-ip-range"
+	const expected = "1.0.0.0/8"
+
 	for _, test := range tests {
-		clusterIPRange := findPodCommandParameter(test.podLister, "kube-apiserver", "--service-cluster-ip-range")
-		if clusterIPRange != test.expectedIP {
-			t.Errorf("get clusteriprange is %v", clusterIPRange)
+		pod, labelSelector := test.buildPod(expected, "0.0.0.0/0")
+
+		podLister, err := buildPodLister(pod)
+		if err != nil {
+			t.Errorf("new pod lister error: %v", err)
+			continue
+		}
+
+		result := findPodCommandParameter(podLister, labelSelector, parameter)
+		if result != expected {
+			t.Errorf("test for %s: expected %s, got %s", test.description, expected, result)
 		}
 	}
 }
 
-func TestFindPodCommandParameterFromCommandWithK8sLabel(t *testing.T) {
-	podCommand := BuildPod("kube-apiserver", "app.kubernetes.io/component", "kube-apiserver")
-	podCommand.Spec.Containers = []corev1.Container{
-		{
-			Command: []string{"--service-cluster-ip-range=1.2.3.4"},
-		},
-	}
+func buildPodLister(pods ...*corev1.Pod) (corelisters.PodLister, error) {
+	kubeClientset := fake.NewSimpleClientset()
+	kubeInformerFactory := kubeinformers.NewSharedInformerFactory(kubeClientset, 0)
+	podInformer := kubeInformerFactory.Core().V1().Pods()
 
-	store := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{"namespace": cache.MetaNamespaceIndexFunc})
-	lister := corelisters.NewPodLister(store)
-	if err := store.Add(podCommand); err != nil {
-		t.Errorf("add pod err %v", err)
+	for _, pod := range pods {
+		err := podInformer.Informer().GetIndexer().Add(pod)
+		if err != nil {
+			return nil, err
+		}
 	}
+	return podInformer.Lister(), nil
+}
 
-	tests := []struct {
-		description string
-		podLister   corelisters.PodLister
-		expectedIP  string
-	}{
-		{
-			description: "service-cluster-ip-range is in command",
-			podLister:   lister,
-			expectedIP:  "1.2.3.4",
-		},
-	}
+func buildAPIServerPod(serviceIPRange, _ string) (*corev1.Pod, labels.Selector) { // no --cluster-cidr parameter
+	labelKey, labelValue := "component", "kube-apiserver"
+	return buildPod("kube-system", labelValue+"-xxx", labelKey, labelValue,
+		[]string{"kube-apiserver", "--service-cluster-ip-range=" + serviceIPRange},
+		[]string{})
+}
 
-	for _, test := range tests {
-		clusterIPRange := findPodCommandParameter(test.podLister, "kube-apiserver", "--service-cluster-ip-range")
-		if clusterIPRange != test.expectedIP {
-			t.Errorf("get clusteriprange is %v", clusterIPRange)
-		}
-	}
+func buildAPIServerPodWithArgs(serviceIPRange, _ string) (*corev1.Pod, labels.Selector) { // no --cluster-cidr parameter
+	labelKey, labelValue := "component", "kube-apiserver"
+	return buildPod("kube-system", labelValue+"-xxx", labelKey, labelValue,
+		[]string{"kube-apiserver"},
+		[]string{"--service-cluster-ip-range=" + serviceIPRange})
+}
+
+func buildControllerManagerWithK8sLabel(serviceIPRange, podIPRange string) (*corev1.Pod, labels.Selector) {
+	labelKey, labelValue := "app.kubernetes.io/component", "kube-controller-manager"
+	return buildPod("kube-system", "kube-controller-manager-xxx", labelKey, labelValue,
+		[]string{"kube-controller-manager", "--service-cluster-ip-range=" + serviceIPRange, "--cluster-cidr=" + podIPRange},
+		[]string{})
+}
+
+func buildProxyPod(serviceIPRange, podIPRange string) (*corev1.Pod, labels.Selector) {
+	return buildPod("kube-system", "kube-proxy-xxx", "k8s-app", "kube-proxy",
+		[]string{"kube-controller-manager", "--service-cluster-ip-range=" + serviceIPRange, "--cluster-cidr=" + podIPRange},
+		[]string{})
 }
 
-func BuildPod(name, labelKey, lableValue string) *corev1.Pod {
+//nolint:unparam
+func buildPod(namespace, name, labelKey, labelValue string, cmd, args []string) (*corev1.Pod, labels.Selector) {
 	pod := &corev1.Pod{
 		TypeMeta: metav1.TypeMeta{Kind: "Pod"},
 		ObjectMeta: metav1.ObjectMeta{
-			Namespace: "kube-system",
+			Namespace: namespace,
 			UID:       uuid.NewUUID(),
 			Name:      name,
-			Labels:    map[string]string{labelKey: lableValue},
+			Labels:    map[string]string{labelKey: labelValue},
+		},
+	}
+	pod.Spec.Containers = []corev1.Container{
+		{
+			Command: cmd,
+			Args:    args,
 		},
 	}
-	return pod
+	return pod, labels.SelectorFromSet(pod.Labels)
 }