Merge pull request #636 from hzxuzhonghu/fix-test-flake

Fix TestPodSidecarLabelChangeTriggersAddIptablesAction flake
kmesh-net · Jul 31, 2024 · 75a7312 · 75a7312
2 parents e4143c2 + 7fc23fb
commit 75a7312
Show file tree

Hide file tree

Showing 2 changed files with 36 additions and 31 deletions.
diff --git a/pkg/controller/bypass/bypass_controller.go b/pkg/controller/bypass/bypass_controller.go
@@ -74,7 +74,7 @@ func NewByPassController(client kubernetes.Interface) *Controller {
 				return
 			}
 
-			log.Debugf("%s/%s: enable bypass control", pod.GetNamespace(), pod.GetName())
+			log.Infof("%s/%s: bypass sidecar control", pod.GetNamespace(), pod.GetName())
 			nspath, _ := ns.GetPodNSpath(pod)
 			if err := addIptables(nspath); err != nil {
 				log.Errorf("failed to add iptables rules for %s: %v", nspath, err)
@@ -100,15 +100,15 @@ func NewByPassController(client kubernetes.Interface) *Controller {
 			}
 
 			if shouldBypass(oldPod) && !shouldBypass(newPod) {
-				log.Debugf("%s/%s: restore sidecar control", newPod.GetNamespace(), newPod.GetName())
+				log.Infof("%s/%s: restore sidecar control", newPod.GetNamespace(), newPod.GetName())
 				nspath, _ := ns.GetPodNSpath(newPod)
 				if err := deleteIptables(nspath); err != nil {
 					log.Errorf("failed to delete iptables rules for %s: %v", nspath, err)
 					return
 				}
 			}
 			if !shouldBypass(oldPod) && shouldBypass(newPod) {
-				log.Debugf("%s/%s: enable bypass control", newPod.GetNamespace(), newPod.GetName())
+				log.Infof("%s/%s: bypass sidecar control", newPod.GetNamespace(), newPod.GetName())
 				nspath, _ := ns.GetPodNSpath(newPod)
 				if err := addIptables(nspath); err != nil {
 					log.Errorf("failed to add iptables rules for %s: %v", nspath, err)

diff --git a/pkg/controller/bypass/bypass_test.go b/pkg/controller/bypass/bypass_test.go
@@ -25,24 +25,38 @@ import (
 
 	"github.com/agiledragon/gomonkey/v2"
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 	"istio.io/api/annotation"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes/fake"
 )
 
-func TestPodSidecarLabelChangeTriggersAddIptablesAction(t *testing.T) {
-	client := fake.NewSimpleClientset()
-
-	err := os.Setenv("NODE_NAME", "test_node")
-	require.NoError(t, err)
+func TestBypassController(t *testing.T) {
+	nodeName := "test_node"
+	err := os.Setenv("NODE_NAME", nodeName)
+	assert.NoError(t, err)
 	t.Cleanup(func() {
 		os.Unsetenv("NODE_NAME")
 	})
-	stopCh := make(<-chan struct{})
+	stopCh := make(chan struct{})
+	defer close(stopCh)
+	namespaceName := "default"
+	namespace := &corev1.Namespace{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name: namespaceName,
+			Labels: map[string]string{
+				"istio-injection": "enabled",
+			},
+		},
+	}
+	client := fake.NewSimpleClientset(namespace)
 	c := NewByPassController(client)
-	go c.Run(stopCh)
+	c.Run(stopCh)
+
 	enabled := atomic.Bool{}
 	disabled := atomic.Bool{}
 
@@ -57,30 +71,18 @@ func TestPodSidecarLabelChangeTriggersAddIptablesAction(t *testing.T) {
 		wg.Done()
 		return nil
 	})
-
-	patches2 := gomonkey.NewPatches()
-	defer patches2.Reset()
-
-	patches2.ApplyFunc(deleteIptables, func(ns string) error {
+	patches1.ApplyFunc(deleteIptables, func(ns string) error {
 		disabled.Store(true)
 		// Signal that addIptables has been called
 		wg.Done()
 		return nil
 	})
 
-	namespaceName := "default"
-	namespace := &corev1.Namespace{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: namespaceName,
-			Labels: map[string]string{
-				"istio-injection": "enabled",
-			},
-		},
-	}
-	_, err = client.CoreV1().Namespaces().Create(context.TODO(), namespace, metav1.CreateOptions{})
-	require.NoError(t, err)
-
 	podWithBypassButNoSidecar := &corev1.Pod{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       "Pod",
+			APIVersion: "v1",
+		},
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      "test-pod-no-sidecar",
 			Namespace: namespaceName,
@@ -89,10 +91,11 @@ func TestPodSidecarLabelChangeTriggersAddIptablesAction(t *testing.T) {
 			},
 		},
 		Spec: corev1.PodSpec{
-			NodeName: "test-node",
+			NodeName: nodeName,
 		},
 	}
 
+	// case 1: pod with bypass label but no sidecar
 	_, err = client.CoreV1().Pods(namespaceName).Create(context.TODO(), podWithBypassButNoSidecar, metav1.CreateOptions{})
 	assert.NoError(t, err)
 	assert.Equal(t, false, enabled.Load(), "unexpected value for enabled flag")
@@ -110,10 +113,11 @@ func TestPodSidecarLabelChangeTriggersAddIptablesAction(t *testing.T) {
 			},
 		},
 		Spec: corev1.PodSpec{
-			NodeName: "test-node",
+			NodeName: nodeName,
 		},
 	}
 
+	// case 2: pod with bypass label and sidecar
 	wg.Add(1)
 	_, err = client.CoreV1().Pods(namespaceName).Create(context.TODO(), podWithBypass, metav1.CreateOptions{})
 	assert.NoError(t, err)
@@ -124,6 +128,7 @@ func TestPodSidecarLabelChangeTriggersAddIptablesAction(t *testing.T) {
 	enabled.Store(false)
 	disabled.Store(false)
 
+	// case 3: pod update by removing bypass label
 	// Update pod by removing the bypass label
 	newPod := podWithBypass.DeepCopy()
 	delete(newPod.Labels, ByPassLabel)
@@ -136,7 +141,7 @@ func TestPodSidecarLabelChangeTriggersAddIptablesAction(t *testing.T) {
 
 	enabled.Store(false)
 	disabled.Store(false)
-	// Update pod by adding the bypass label
+	// case 4: Update pod by adding the bypass label
 	newPod = podWithBypass.DeepCopy()
 	newPod.Labels[ByPassLabel] = ByPassValue
 	wg.Add(1)