From 7d7d2ddfd3fa463214f8dbd0751dcedd6fe9de81 Mon Sep 17 00:00:00 2001
From: Congqi Zhao <1229896069@qq.com>
Date: Wed, 12 Feb 2025 13:37:25 +0800
Subject: [PATCH] fix security groups changed when vm is shut down (#4976)

* fix security groups changed when vm is closed
* add unit test for UnionStringSlice

Signed-off-by: zhaocongqi <1229896069@qq.com>

---------

Signed-off-by: zhaocongqi <1229896069@qq.com>
---
 pkg/controller/pod.go  | 19 ++++++++++++++---
 pkg/util/slice.go      | 10 +++++++++
 pkg/util/slice_test.go | 48 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 74 insertions(+), 3 deletions(-)

diff --git a/pkg/controller/pod.go b/pkg/controller/pod.go
index 87868897e9c..35081c30a0c 100644
--- a/pkg/controller/pod.go
+++ b/pkg/controller/pod.go
@@ -571,8 +571,19 @@ func (c *Controller) reconcileAllocateSubnets(pod *v1.Pod, needAllocatePodNets [
 				DHCPv6OptionsUUID: subnet.Status.DHCPv6OptionsUUID,
 			}
 
+			var oldSgList []string
+			if vmKey != "" {
+				existingLsp, err := c.OVNNbClient.GetLogicalSwitchPort(portName, true)
+				if err != nil {
+					klog.Errorf("failed to get logical switch port %s: %v", portName, err)
+					return nil, err
+				}
+				if existingLsp != nil {
+					oldSgList, _ = c.getPortSg(existingLsp)
+				}
+			}
+
 			securityGroupAnnotation := pod.Annotations[fmt.Sprintf(util.SecurityGroupAnnotationTemplate, podNet.ProviderName)]
-			securityGroups := strings.ReplaceAll(securityGroupAnnotation, " ", "")
 			if err := c.OVNNbClient.CreateLogicalSwitchPort(subnet.Name, portName, ipStr, mac, podName, pod.Namespace,
 				portSecurity, securityGroupAnnotation, vips, podNet.Subnet.Spec.EnableDHCP, dhcpOptions, subnet.Spec.Vpc); err != nil {
 				c.recorder.Eventf(pod, v1.EventTypeWarning, "CreateOVNPortFailed", err.Error())
@@ -588,8 +599,10 @@ func (c *Controller) reconcileAllocateSubnets(pod *v1.Pod, needAllocatePodNets [
 				}
 			}
 
-			if securityGroupAnnotation != "" {
-				sgNames := strings.Split(securityGroups, ",")
+			if securityGroupAnnotation != "" || oldSgList != nil {
+				securityGroups := strings.ReplaceAll(securityGroupAnnotation, " ", "")
+				newSgList := strings.Split(securityGroups, ",")
+				sgNames := util.UnionStringSlice(oldSgList, newSgList)
 				for _, sgName := range sgNames {
 					if sgName != "" {
 						c.syncSgPortsQueue.Add(sgName)
diff --git a/pkg/util/slice.go b/pkg/util/slice.go
index bfa25d5d6dc..5c61f103940 100644
--- a/pkg/util/slice.go
+++ b/pkg/util/slice.go
@@ -1,5 +1,7 @@
 package util
 
+import "k8s.io/utils/set"
+
 func DiffStringSlice(slice1, slice2 []string) []string {
 	var diff []string
 
@@ -27,6 +29,14 @@ func DiffStringSlice(slice1, slice2 []string) []string {
 	return diff
 }
 
+func UnionStringSlice(slices ...[]string) []string {
+	union := set.New[string]()
+	for _, s := range slices {
+		union.Insert(s...)
+	}
+	return union.UnsortedList()
+}
+
 // IsStringsOverlap check if two string slices are overlapped
 func IsStringsOverlap(a, b []string) bool {
 	for _, sa := range a {
diff --git a/pkg/util/slice_test.go b/pkg/util/slice_test.go
index 84e019b69c9..e0e4cd58206 100644
--- a/pkg/util/slice_test.go
+++ b/pkg/util/slice_test.go
@@ -37,6 +37,54 @@ func TestDiffStringSlice(t *testing.T) {
 	}
 }
 
+func TestUnionStringSlice(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		desc   string
+		slice1 []string
+		slice2 []string
+		want   []string
+	}{
+		{
+			desc:   "both slices nil",
+			slice1: nil,
+			slice2: nil,
+			want:   []string{},
+		},
+		{
+			desc:   "first slice nil",
+			slice1: nil,
+			slice2: []string{"a", "b", "c"},
+			want:   []string{"a", "b", "c"},
+		},
+		{
+			desc:   "second slice nil",
+			slice1: []string{"x", "y", "z"},
+			slice2: nil,
+			want:   []string{"x", "y", "z"},
+		},
+		{
+			desc:   "duplicate elements",
+			slice1: []string{"a", "b", "a", "c"},
+			slice2: []string{"b", "c", "c", "d"},
+			want:   []string{"a", "b", "c", "d"},
+		},
+		{
+			desc:   "empty slices",
+			slice1: []string{},
+			slice2: []string{},
+			want:   []string{},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			result := UnionStringSlice(tt.slice1, tt.slice2)
+			require.ElementsMatch(t, tt.want, result)
+		})
+	}
+}
+
 func TestIsStringsOverlap(t *testing.T) {
 	tests := []struct {
 		name string