From 2fa5df222b927fcc6f0a6a1319818db5cd364e30 Mon Sep 17 00:00:00 2001 From: bobz965 Date: Mon, 8 Apr 2024 09:29:41 +0800 Subject: [PATCH] distinguish portSecurity with security group (#3862) * distinguish portSecurity with security group --------- Signed-off-by: bobz965 --- pkg/controller/pod.go | 38 +++++++++++----------- pkg/controller/security_group.go | 6 ++-- pkg/controller/service.go | 2 +- pkg/ovs/ovn-nb-logical_switch_port.go | 16 +++++---- pkg/ovs/ovn-nb-logical_switch_port_test.go | 19 ++++++----- pkg/ovs/ovn-nb-port_group.go | 4 +++ 6 files changed, 47 insertions(+), 38 deletions(-) diff --git a/pkg/controller/pod.go b/pkg/controller/pod.go index dc27bc629f5..48589c9295e 100644 --- a/pkg/controller/pod.go +++ b/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 { diff --git a/pkg/controller/security_group.go b/pkg/controller/security_group.go index d5d979e4b54..233c9dfb308 100644 --- a/pkg/controller/security_group.go +++ b/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 } diff --git a/pkg/controller/service.go b/pkg/controller/service.go index 60777b8aea8..3c48dbc52b6 100644 --- a/pkg/controller/service.go +++ b/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) } } diff --git a/pkg/ovs/ovn-nb-logical_switch_port.go b/pkg/ovs/ovn-nb-logical_switch_port.go index 66ad24ef207..e3e1b0ea5df 100644 --- a/pkg/ovs/ovn-nb-logical_switch_port.go +++ b/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" } } diff --git a/pkg/ovs/ovn-nb-logical_switch_port_test.go b/pkg/ovs/ovn-nb-logical_switch_port_test.go index 4d15482e206..3fff4abaa38 100644 --- a/pkg/ovs/ovn-nb-logical_switch_port_test.go +++ b/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) diff --git a/pkg/ovs/ovn-nb-port_group.go b/pkg/ovs/ovn-nb-port_group.go index ce14104c75a..73b1af6d60a 100644 --- a/pkg/ovs/ovn-nb-port_group.go +++ b/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)