distinguish portSecurity with security group (#3862)

* distinguish portSecurity with security group



---------

Signed-off-by: bobz965 <[email protected]>

pkg/controller/pod.go

@@ -714,7 +714,6 @@ func (c *Controller) reconcileAllocateSubnets(cachedPod, pod *v1.Pod, needAlloca
 				portSecurity = true
 			}
 
-			securityGroupAnnotation := pod.Annotations[fmt.Sprintf(util.SecurityGroupAnnotationTemplate, podNet.ProviderName)]
 			vips := vipsMap[fmt.Sprintf("%s.%s", podNet.Subnet.Name, podNet.ProviderName)]
 			for _, ip := range strings.Split(vips, ",") {
 				if ip != "" && net.ParseIP(ip) == nil {
@@ -729,6 +728,9 @@ func (c *Controller) reconcileAllocateSubnets(cachedPod, pod *v1.Pod, needAlloca
 				DHCPv4OptionsUUID: subnet.Status.DHCPv4OptionsUUID,
 				DHCPv6OptionsUUID: subnet.Status.DHCPv6OptionsUUID,
 			}
+
+			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())
@@ -762,13 +764,12 @@ func (c *Controller) reconcileAllocateSubnets(cachedPod, pod *v1.Pod, needAlloca
 				}
 			}
 
-			if portSecurity {
-				sgNames := strings.Split(securityGroupAnnotation, ",")
+			if securityGroupAnnotation != "" {
+				sgNames := strings.Split(securityGroups, ",")
 				for _, sgName := range sgNames {
-					if sgName == "" {
-						continue
+					if sgName != "" {
+						c.syncSgPortsQueue.Add(sgName)
 					}
-					c.syncSgPortsQueue.Add(sgName)
 				}
 			}
 
@@ -1171,13 +1172,11 @@ func (c *Controller) handleDeletePod(key string) error {
 	}
 	for _, podNet := range podNets {
 		c.syncVirtualPortsQueue.Add(podNet.Subnet.Name)
-		if pod.Annotations[fmt.Sprintf(util.PortSecurityAnnotationTemplate, podNet.ProviderName)] == "true" {
-			if securityGroupAnnotation, ok := pod.Annotations[fmt.Sprintf(util.SecurityGroupAnnotationTemplate, podNet.ProviderName)]; ok {
-				sgNames := strings.Split(securityGroupAnnotation, ",")
-				for _, sgName := range sgNames {
-					if sgName == "" {
-						continue
-					}
+		securityGroupAnnotation := pod.Annotations[fmt.Sprintf(util.SecurityGroupAnnotationTemplate, podNet.ProviderName)]
+		if securityGroupAnnotation != "" {
+			securityGroups := strings.ReplaceAll(securityGroupAnnotation, " ", "")
+			for _, sgName := range strings.Split(securityGroups, ",") {
+				if sgName != "" {
 					c.syncSgPortsQueue.Add(sgName)
 				}
 			}
@@ -1233,13 +1232,14 @@ func (c *Controller) handleUpdatePodSecurity(key string) error {
 		}
 
 		c.syncVirtualPortsQueue.Add(podNet.Subnet.Name)
-
+		securityGroupAnnotation := pod.Annotations[fmt.Sprintf(util.SecurityGroupAnnotationTemplate, podNet.ProviderName)]
 		var securityGroups string
-		if portSecurity {
-			securityGroups = pod.Annotations[fmt.Sprintf(util.SecurityGroupAnnotationTemplate, podNet.ProviderName)]
-			securityGroups = strings.ReplaceAll(securityGroups, " ", "")
-			for _, sg := range strings.Split(securityGroups, ",") {
-				c.syncSgPortsQueue.Add(sg)
+		if securityGroupAnnotation != "" {
+			securityGroups = strings.ReplaceAll(securityGroupAnnotation, " ", "")
+			for _, sgName := range strings.Split(securityGroups, ",") {
+				if sgName != "" {
+					c.syncSgPortsQueue.Add(sgName)
+				}
 			}
 		}
 		if err = c.reconcilePortSg(ovs.PodNameToPortName(podName, namespace, podNet.ProviderName), securityGroups); err != nil {

pkg/controller/security_group.go

@@ -423,10 +423,10 @@ func (c *Controller) syncSgLogicalPort(key string) error {
 
 	var ports, v4s, v6s []string
 	for _, lsp := range sgPorts {
+		ports = append(ports, lsp.Name)
 		if len(lsp.PortSecurity) == 0 {
 			continue
 		}
-		ports = append(ports, lsp.Name)
 		for _, ps := range lsp.PortSecurity {
 			fields := strings.Fields(ps)
 			if len(fields) < 2 {
@@ -445,10 +445,10 @@ func (c *Controller) syncSgLogicalPort(key string) error {
 	sg, err := c.sgsLister.Get(key)
 	if err != nil {
 		if k8serrors.IsNotFound(err) {
-			klog.Errorf("sg '%s' not found.", key)
+			klog.Warningf("no security group %s ", key)
 			return nil
 		}
-		klog.Errorf("failed to get sg '%s'. %v", key, err)
+		klog.Errorf("failed to get security group %s: %v", key, err)
 		return err
 	}
 

pkg/controller/service.go

@@ -88,7 +88,7 @@ func (c *Controller) enqueueDeleteService(obj interface{}) {
 			for _, ip := range ips {
 				vpcSvc.Vips = append(vpcSvc.Vips, util.JoinHostPort(ip, port.Port))
 			}
-			klog.Infof("delete vpc service %v", vpcSvc)
+			klog.V(3).Infof("delete vpc service: %v", vpcSvc)
 			c.deleteServiceQueue.Add(vpcSvc)
 		}
 	}

pkg/ovs/ovn-nb-logical_switch_port.go

@@ -35,21 +35,23 @@ func buildLogicalSwitchPort(lspName, lsName, ip, mac, podName, namespace string,
 	// addresses is the first element of addresses
 	lsp.Addresses = []string{strings.TrimSpace(strings.Join(addresses, " "))}
 	lsp.ExternalIDs["vendor"] = util.CniTypeName
+
+	lsp.PortSecurity = nil
 	if portSecurity {
 		if len(vips) != 0 {
 			addresses = append(addresses, vipList...)
 		}
 		// addresses is the first element of port_security
 		lsp.PortSecurity = []string{strings.TrimSpace(strings.Join(addresses, " "))}
+	}
 
-		// set security groups
-		if len(securityGroups) != 0 {
-			lsp.ExternalIDs[sgsKey] = strings.ReplaceAll(securityGroups, ",", "/")
+	// set security groups
+	if len(securityGroups) != 0 {
+		lsp.ExternalIDs[sgsKey] = strings.ReplaceAll(securityGroups, ",", "/")
 
-			sgList := strings.Split(securityGroups, ",")
-			for _, sg := range sgList {
-				lsp.ExternalIDs[associatedSgKeyPrefix+sg] = "true"
-			}
+		sgList := strings.Split(securityGroups, ",")
+		for _, sg := range sgList {
+			lsp.ExternalIDs[associatedSgKeyPrefix+sg] = "true"
 		}
 	}
 

pkg/ovs/ovn-nb-logical_switch_port_test.go

@@ -120,8 +120,8 @@ func (suite *OvnClientTestSuite) testCreateLogicalSwitchPort() {
 		require.Equal(t, dhcpUUIDs.DHCPv6OptionsUUID, *lsp.Dhcpv6Options)
 	})
 
-	t.Run("create logical switch port with default vpc", func(t *testing.T) {
-		lspName := "test-create-lsp-lsp-default-vpc"
+	t.Run("create logical switch port in default vpc with sg", func(t *testing.T) {
+		lspName := "test-create-lsp-lsp-in-default-vpc-with-sg"
 		sgs := "sg,sg1"
 		vpcName := "ovn-cluster"
 
@@ -146,7 +146,7 @@ func (suite *OvnClientTestSuite) testCreateLogicalSwitchPort() {
 		require.Equal(t, dhcpUUIDs.DHCPv6OptionsUUID, *lsp.Dhcpv6Options)
 	})
 
-	t.Run("create logical switch port with portSecurity=false", func(t *testing.T) {
+	t.Run("create logical switch port with portSecurity=false and sg", func(t *testing.T) {
 		lspName := "test-create-lsp-lsp-no-port-security"
 		sgs := "sg,sg1"
 		vpcName := "test-vpc"
@@ -159,11 +159,14 @@ func (suite *OvnClientTestSuite) testCreateLogicalSwitchPort() {
 		require.ElementsMatch(t, []string{"00:00:00:AB:B4:65 10.244.0.37 fc00::af4:25"}, lsp.Addresses)
 		require.Equal(t, map[string]string{
 			"associated_sg_" + util.DefaultSecurityGroupName: "false",
-			"pod":         fmt.Sprintf("%s/%s", podNamespace, podName),
-			"ls":          lsName,
-			"vendor":      util.CniTypeName,
-			"vips":        vips,
-			"attach-vips": "true",
+			"associated_sg_sg":  "true",
+			"associated_sg_sg1": "true",
+			"pod":               fmt.Sprintf("%s/%s", podNamespace, podName),
+			"security_groups":   "sg/sg1",
+			"ls":                lsName,
+			"vendor":            util.CniTypeName,
+			"vips":              vips,
+			"attach-vips":       "true",
 		}, lsp.ExternalIDs)
 		require.Equal(t, dhcpUUIDs.DHCPv4OptionsUUID, *lsp.Dhcpv4Options)
 		require.Equal(t, dhcpUUIDs.DHCPv6OptionsUUID, *lsp.Dhcpv6Options)

pkg/ovs/ovn-nb-port_group.go

@@ -53,6 +53,10 @@ func (c *OVNNbClient) PortGroupRemovePorts(pgName string, lspNames ...string) er
 }
 
 func (c *OVNNbClient) PortGroupSetPorts(pgName string, ports []string) error {
+	if pgName == "" {
+		return fmt.Errorf("port group name is empty")
+	}
+
 	pg, err := c.GetPortGroup(pgName, false)
 	if err != nil {
 		return fmt.Errorf("get port group %s: %v", pgName, err)