Fix init sg (#3890)

* fix custom sg not recover as ip --------- Signed-off-by: bobz965 <[email protected]>
kubeovn · Apr 9, 2024 · 859e109 · 859e109
1 parent 2fa5df2
commit 859e109
Show file tree

Hide file tree

Showing 6 changed files with 193 additions and 36 deletions.
diff --git a/mocks/pkg/ovs/interface.go b/mocks/pkg/ovs/interface.go
diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go
@@ -1119,9 +1119,12 @@ func (c *Controller) initResourceOnce() {
 		util.LogFatalAndExit(err, "failed to initialize node chassis")
 	}
 
-	if err := c.initDenyAllSecurityGroup(); err != nil {
+	if err := c.initDefaultDenyAllSecurityGroup(); err != nil {
 		util.LogFatalAndExit(err, "failed to initialize 'deny_all' security group")
 	}
+	if err := c.syncSecurityGroup(); err != nil {
+		util.LogFatalAndExit(err, "failed to sync security group")
+	}
 
 	if err := c.syncVpcNatGatewayCR(); err != nil {
 		util.LogFatalAndExit(err, "failed to sync crd vpc nat gateways")

diff --git a/pkg/controller/security_group.go b/pkg/controller/security_group.go
@@ -11,6 +11,7 @@ import (
 	"github.com/cnf/structhash"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/client-go/tools/cache"
@@ -119,7 +120,7 @@ func (c *Controller) processNextAddOrUpdateSgWorkItem() bool {
 			utilruntime.HandleError(fmt.Errorf("expected string in workqueue but got %#v", obj))
 			return nil
 		}
-		if err := c.handleAddOrUpdateSg(key); err != nil {
+		if err := c.handleAddOrUpdateSg(key, false); err != nil {
 			c.addOrUpdateSgQueue.AddRateLimited(key)
 			return fmt.Errorf("error syncing '%s': %s, requeuing", key, err.Error())
 		}
@@ -163,7 +164,7 @@ func (c *Controller) processNextDeleteSgWorkItem() bool {
 	return true
 }
 
-func (c *Controller) initDenyAllSecurityGroup() error {
+func (c *Controller) initDefaultDenyAllSecurityGroup() error {
 	pgName := ovs.GetSgPortGroupName(util.DenyAllSecurityGroup)
 	if err := c.OVNNbClient.CreatePortGroup(pgName, map[string]string{
 		"type": "security_group",
@@ -182,6 +183,28 @@ func (c *Controller) initDenyAllSecurityGroup() error {
 	return nil
 }
 
+func (c *Controller) syncSecurityGroup() error {
+	sgs, err := c.sgsLister.List(labels.Everything())
+	if err != nil {
+		klog.Errorf("failed to list security groups: %v", err)
+		return err
+	}
+	for _, sg := range sgs {
+		lost, err := c.OVNNbClient.SGLostACL(sg)
+		if err != nil {
+			err = fmt.Errorf("failed to check if security group %s lost acl: %v", sg.Name, err)
+			klog.Error(err)
+			return err
+		}
+		if lost {
+			if err := c.handleAddOrUpdateSg(sg.Name, true); err != nil {
+				klog.Errorf("failed to sync security group %s: %v", sg.Name, err)
+			}
+		}
+	}
+	return nil
+}
+
 // updateDenyAllSgPorts set lsp to deny which security_groups is not empty
 func (c *Controller) updateDenyAllSgPorts() error {
 	// list all lsp which security_groups is not empty
@@ -224,7 +247,7 @@ func (c *Controller) updateDenyAllSgPorts() error {
 	return nil
 }
 
-func (c *Controller) handleAddOrUpdateSg(key string) error {
+func (c *Controller) handleAddOrUpdateSg(key string, force bool) error {
 	c.sgKeyMutex.LockKey(key)
 	defer func() { _ = c.sgKeyMutex.UnlockKey(key) }()
 	klog.Infof("handle add/update security group %s", key)
@@ -277,26 +300,31 @@ func (c *Controller) handleAddOrUpdateSg(key string) error {
 		return err
 	}
 
-	ingressNeedUpdate := false
-	egressNeedUpdate := false
-
-	// check md5
-	newIngressMd5 := fmt.Sprintf("%x", structhash.Md5(sg.Spec.IngressRules, 1))
-	if !sg.Status.IngressLastSyncSuccess || newIngressMd5 != sg.Status.IngressMd5 {
-		klog.Infof("ingress need update, sg:%s", sg.Name)
+	var ingressNeedUpdate, egressNeedUpdate bool
+	var newIngressMd5, newEgressMd5 string
+	if force {
+		klog.Infof("force update sg %s", sg.Name)
 		ingressNeedUpdate = true
-	}
-	newEgressMd5 := fmt.Sprintf("%x", structhash.Md5(sg.Spec.EgressRules, 1))
-	if !sg.Status.EgressLastSyncSuccess || newEgressMd5 != sg.Status.EgressMd5 {
-		klog.Infof("egress need update, sg:%s", sg.Name)
 		egressNeedUpdate = true
-	}
+	} else {
+		// check md5
+		newIngressMd5 = fmt.Sprintf("%x", structhash.Md5(sg.Spec.IngressRules, 1))
+		if !sg.Status.IngressLastSyncSuccess || newIngressMd5 != sg.Status.IngressMd5 {
+			klog.Infof("ingress need update, sg:%s", sg.Name)
+			ingressNeedUpdate = true
+		}
+		newEgressMd5 = fmt.Sprintf("%x", structhash.Md5(sg.Spec.EgressRules, 1))
+		if !sg.Status.EgressLastSyncSuccess || newEgressMd5 != sg.Status.EgressMd5 {
+			klog.Infof("egress need update, sg:%s", sg.Name)
+			egressNeedUpdate = true
+		}
 
-	// check allowSameGroupTraffic switch
-	if sg.Status.AllowSameGroupTraffic != sg.Spec.AllowSameGroupTraffic {
-		klog.Infof("both ingress && egress need update, sg:%s", sg.Name)
-		ingressNeedUpdate = true
-		egressNeedUpdate = true
+		// check allowSameGroupTraffic switch
+		if sg.Status.AllowSameGroupTraffic != sg.Spec.AllowSameGroupTraffic {
+			klog.Infof("both ingress && egress need update, sg:%s", sg.Name)
+			ingressNeedUpdate = true
+			egressNeedUpdate = true
+		}
 	}
 
 	// update sg rule

diff --git a/pkg/controller/subnet.go b/pkg/controller/subnet.go
@@ -2045,7 +2045,7 @@ func (c *Controller) calcDualSubnetStatusIP(subnet *kubeovnv1.Subnet) (*kubeovnv
 			klog.Error(err)
 			return nil, err
 		}
-		lenOvnEip := len(ovnEips)
+		lenOvnEip = len(ovnEips)
 		usingIPs += float64(lenOvnEip)
 	}
 
@@ -2071,13 +2071,6 @@ func (c *Controller) calcDualSubnetStatusIP(subnet *kubeovnv1.Subnet) (*kubeovnv
 		return subnet, nil
 	}
 
-	if v4UsingIPStr == "" && v6UsingIPStr == "" && usingIPs != 0 {
-		// in case of subnet deletion, v4 v6 using ip should be 0
-		err = fmt.Errorf("ipam subnet %s has no ip in using, but some ip cr left: ip %d, vip %d, iptable eip %d, ovn eip %d", subnet.Name, lenIP, lenVip, lenIptablesEip, lenOvnEip)
-		klog.Error(err)
-		return nil, err
-	}
-
 	subnet.Status.V4AvailableIPs = v4availableIPs
 	subnet.Status.V6AvailableIPs = v6availableIPs
 	subnet.Status.V4UsingIPs = usingIPs
@@ -2217,13 +2210,6 @@ func (c *Controller) calcSubnetStatusIP(subnet *kubeovnv1.Subnet) (*kubeovnv1.Su
 		return subnet, nil
 	}
 
-	if v4UsingIPStr == "" && v6UsingIPStr == "" && usingIPs != 0 {
-		// in case of subnet deletion, v4 v6 using ip should be 0
-		err = fmt.Errorf("ipam subnet %s has no ip in using, but some ip cr left: ip %d, vip %d, iptable eip %d, ovn eip %d", subnet.Name, lenIP, lenVip, lenIptablesEip, lenOvnEip)
-		klog.Error(err)
-		return nil, err
-	}
-
 	bytes, err := subnet.Status.Bytes()
 	if err != nil {
 		klog.Error(err)

diff --git a/pkg/ovs/interface.go b/pkg/ovs/interface.go
@@ -146,6 +146,7 @@ type ACL interface {
 	UpdateLogicalSwitchACL(lsName, cidrBlock string, subnetAcls []kubeovnv1.ACL, allowEWTraffic bool) error
 	SetACLLog(pgName, protocol string, logEnable, isIngress bool) error
 	SetLogicalSwitchPrivate(lsName, cidrBlock, nodeSwitchCIDR string, allowSubnets []string) error
+	SGLostACL(sg *kubeovnv1.SecurityGroup) (bool, error)
 	DeleteAcls(parentName, parentType, direction string, externalIDs map[string]string) error
 	DeleteAclsOps(parentName, parentType, direction string, externalIDs map[string]string) ([]ovsdb.Operation, error)
 }

diff --git a/pkg/ovs/ovn-nb-acl.go b/pkg/ovs/ovn-nb-acl.go
@@ -1175,3 +1175,112 @@ func (c *OVNNbClient) DeleteAclsOps(parentName, parentType, direction string, ex
 
 	return removeACLOp, nil
 }
+
+// sgRuleNoACL check if security group rule has acl
+func (c *OVNNbClient) sgRuleNoACL(sgName, direction string, rule *kubeovnv1.SgRule) (bool, error) {
+	ipSuffix := "ip4"
+	if rule.IPVersion == "ipv6" {
+		ipSuffix = "ip6"
+	}
+
+	pgName := GetSgPortGroupName(sgName)
+
+	// ingress rule
+	srcOrDst, portDirection := "src", "outport"
+	if direction == ovnnb.ACLDirectionFromLport { // egress rule
+		srcOrDst = "dst"
+		portDirection = "inport"
+	}
+
+	ipKey := ipSuffix + "." + srcOrDst
+
+	/* match all traffic to or from pgName */
+	allIPMatch := NewAndACLMatch(
+		NewACLMatch(portDirection, "==", "@"+pgName, ""),
+		NewACLMatch(ipSuffix, "", "", ""),
+	)
+
+	/* allow allowed ip traffic */
+	// type address
+	allowedIPMatch := NewAndACLMatch(
+		allIPMatch,
+		NewACLMatch(ipKey, "==", rule.RemoteAddress, ""),
+	)
+
+	// type securityGroup
+	remotePgName := GetSgV4AssociatedName(rule.RemoteSecurityGroup)
+	if rule.IPVersion == "ipv6" {
+		remotePgName = GetSgV6AssociatedName(rule.RemoteSecurityGroup)
+	}
+	if rule.RemoteType == kubeovnv1.SgRemoteTypeSg {
+		allowedIPMatch = NewAndACLMatch(
+			allIPMatch,
+			NewACLMatch(ipKey, "==", "$"+remotePgName, ""),
+		)
+	}
+
+	/* allow layer 4 traffic */
+	// allow all layer 4 traffic
+	match := allowedIPMatch
+
+	switch rule.Protocol {
+	case kubeovnv1.ProtocolICMP:
+		match = NewAndACLMatch(
+			allowedIPMatch,
+			NewACLMatch("icmp4", "", "", ""),
+		)
+		if ipSuffix == "ip6" {
+			match = NewAndACLMatch(
+				allowedIPMatch,
+				NewACLMatch("icmp6", "", "", ""),
+			)
+		}
+	case kubeovnv1.ProtocolTCP, kubeovnv1.ProtocolUDP:
+		match = NewAndACLMatch(
+			allowedIPMatch,
+			NewACLMatch(string(rule.Protocol)+".dst", "<=", strconv.Itoa(rule.PortRangeMin), strconv.Itoa(rule.PortRangeMax)),
+		)
+	}
+
+	exists, err := c.ACLExists(pgName, direction, strconv.Itoa(rule.Priority), match.String())
+	if err != nil {
+		err = fmt.Errorf("failed to check acl rule for security group %s: %v", sgName, err)
+		klog.Error(err)
+		return false, err
+	}
+
+	// sg rule no acl, need to sync
+	if !exists {
+		return true, nil
+	}
+	return false, nil
+}
+
+// SGLostACL check if security group lost an acl
+func (c *OVNNbClient) SGLostACL(sg *kubeovnv1.SecurityGroup) (bool, error) {
+	ingressRules := sg.Spec.IngressRules
+	for _, rule := range ingressRules {
+		no, err := c.sgRuleNoACL(sg.Name, ovnnb.ACLDirectionToLport, rule)
+		if err != nil {
+			klog.Error(err)
+			return false, err
+		}
+		if no {
+			klog.Infof("security group %s lost ingress rule: %v", sg.Name, rule)
+			return true, nil
+		}
+	}
+	egressRules := sg.Spec.EgressRules
+	for _, rule := range egressRules {
+		no, err := c.sgRuleNoACL(sg.Name, ovnnb.ACLDirectionFromLport, rule)
+		if err != nil {
+			klog.Error(err)
+			return false, err
+		}
+		if no {
+			klog.Infof("security group %s lost egress rule: %v", sg.Name, rule)
+			return true, nil
+		}
+	}
+	return false, nil
+}