From 21b2104b9901316c17efd251c2e79b99bbd06b46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=A9=AC=E6=B4=AA=E8=B4=9E?= Date: Fri, 28 Oct 2022 15:49:16 +0800 Subject: [PATCH] do not need to delete pg when update networkpolicy --- pkg/controller/network_policy.go | 91 +++++++++++++++++++------------- pkg/ovs/ovn-nbctl-legacy.go | 45 +++++++++------- 2 files changed, 80 insertions(+), 56 deletions(-) diff --git a/pkg/controller/network_policy.go b/pkg/controller/network_policy.go index 60e4a7e5778..030910793d5 100644 --- a/pkg/controller/network_policy.go +++ b/pkg/controller/network_policy.go @@ -192,11 +192,6 @@ func (c *Controller) handleUpdateNp(key string) error { egressAllowAsNamePrefix := strings.Replace(fmt.Sprintf("%s.%s.egress.allow", np.Name, np.Namespace), "-", ".", -1) egressExceptAsNamePrefix := strings.Replace(fmt.Sprintf("%s.%s.egress.except", np.Name, np.Namespace), "-", ".", -1) - // delete existing pg to update acl - if err = c.ovnLegacyClient.DeletePortGroup(pgName); err != nil { - klog.Errorf("failed to delete port group %s before networkpolicy update process, %v", pgName, err) - } - if err = c.ovnLegacyClient.CreateNpPortGroup(pgName, np.Namespace, np.Name); err != nil { klog.Errorf("failed to create port group for np %s, %v", key, err) return err @@ -240,12 +235,6 @@ func (c *Controller) handleUpdateNp(key string) error { } } - // before update or add ingress info,we should first delete acl and address_set - if err = c.ovnLegacyClient.DeleteACL(pgName, "to-lport"); err != nil { - klog.Errorf("failed to delete np %s ingress acls, %v", key, err) - return err - } - ingressAsNames, err := c.ovnLegacyClient.ListNpAddressSet(np.Namespace, np.Name, "ingress") if err != nil { klog.Errorf("failed to list ingress address_set, %v", err) @@ -258,6 +247,15 @@ func (c *Controller) handleUpdateNp(key string) error { } } + var ingressAclCmd []string + exist, err := c.ovnLegacyClient.PortGroupExists(pgName) + if err != nil { + klog.Errorf("failed to query np %s port group, %v", key, err) + return err + } + if exist { + ingressAclCmd = []string{"--type=port-group", "acl-del", pgName, "to-lport"} + } if hasIngressRule(np) { for _, cidrBlock := range strings.Split(subnet.Spec.CIDRBlock, ",") { protocol := util.CheckProtocol(cidrBlock) @@ -309,15 +307,9 @@ func (c *Controller) handleUpdateNp(key string) error { } if len(allows) != 0 || len(excepts) != 0 { - if err = c.ovnLegacyClient.CreateIngressACL(pgName, ingressAllowAsName, ingressExceptAsName, svcAsName, protocol, npr.Ports, logEnable); err != nil { - klog.Errorf("failed to create ingress acls for np %s, %v", key, err) - return err - } + ingressAclCmd = c.ovnLegacyClient.CombineIngressACLCmd(pgName, ingressAllowAsName, ingressExceptAsName, svcAsName, protocol, npr.Ports, logEnable, ingressAclCmd, idx) } else { - if err = c.ovnLegacyClient.CreateIngressACL(pgName, ingressAllowAsName, ingressExceptAsName, svcAsName, protocol, []netv1.NetworkPolicyPort{}, logEnable); err != nil { - klog.Errorf("failed to create default deny all ingress acls for np %s, %v", key, err) - return err - } + ingressAclCmd = c.ovnLegacyClient.CombineIngressACLCmd(pgName, ingressAllowAsName, ingressExceptAsName, svcAsName, protocol, []netv1.NetworkPolicyPort{}, logEnable, ingressAclCmd, idx) } } if len(np.Spec.Ingress) == 0 { @@ -333,10 +325,13 @@ func (c *Controller) handleUpdateNp(key string) error { return err } ingressPorts := []netv1.NetworkPolicyPort{} - if err = c.ovnLegacyClient.CreateIngressACL(pgName, ingressAllowAsName, ingressExceptAsName, svcAsName, protocol, ingressPorts, logEnable); err != nil { - klog.Errorf("failed to create ingress acls for np %s, %v", key, err) - return err - } + ingressAclCmd = c.ovnLegacyClient.CombineIngressACLCmd(pgName, ingressAllowAsName, ingressExceptAsName, svcAsName, protocol, ingressPorts, logEnable, ingressAclCmd, 0) + } + + klog.Infof("create ingress acl cmd is: %v", ingressAclCmd) + if err = c.ovnLegacyClient.CreateACL(ingressAclCmd); err != nil { + klog.Errorf("failed to create ingress acls for np %s, %v", key, err) + return err } if err = c.ovnLegacyClient.SetAclLog(pgName, logEnable, true); err != nil { @@ -387,12 +382,6 @@ func (c *Controller) handleUpdateNp(key string) error { } } - // before update or add egress info, we should first delete acl and address_set - if err = c.ovnLegacyClient.DeleteACL(pgName, "from-lport"); err != nil { - klog.Errorf("failed to delete np %s egress acls, %v", key, err) - return err - } - egressAsNames, err := c.ovnLegacyClient.ListNpAddressSet(np.Namespace, np.Name, "egress") if err != nil { klog.Errorf("failed to list egress address_set, %v", err) @@ -404,6 +393,16 @@ func (c *Controller) handleUpdateNp(key string) error { return err } } + + var egressAclCmd []string + exist, err = c.ovnLegacyClient.PortGroupExists(pgName) + if err != nil { + klog.Errorf("failed to query np %s port group, %v", key, err) + return err + } + if exist { + egressAclCmd = []string{"--type=port-group", "acl-del", pgName, "from-lport"} + } if hasEgressRule(np) { for _, cidrBlock := range strings.Split(subnet.Spec.CIDRBlock, ",") { protocol := util.CheckProtocol(cidrBlock) @@ -455,10 +454,7 @@ func (c *Controller) handleUpdateNp(key string) error { } if len(allows) != 0 || len(excepts) != 0 { - if err = c.ovnLegacyClient.CreateEgressACL(pgName, egressAllowAsName, egressExceptAsName, protocol, npr.Ports, svcAsName, logEnable); err != nil { - klog.Errorf("failed to create egress acls for np %s, %v", key, err) - return err - } + egressAclCmd = c.ovnLegacyClient.CombineEgressACLCmd(pgName, egressAllowAsName, egressExceptAsName, protocol, npr.Ports, svcAsName, logEnable, egressAclCmd, idx) } } if len(np.Spec.Egress) == 0 { @@ -474,11 +470,15 @@ func (c *Controller) handleUpdateNp(key string) error { return err } egressPorts := []netv1.NetworkPolicyPort{} - if err = c.ovnLegacyClient.CreateEgressACL(pgName, egressAllowAsName, egressExceptAsName, protocol, egressPorts, svcAsName, logEnable); err != nil { - klog.Errorf("failed to create egress acls for np %s, %v", key, err) - return err - } + egressAclCmd = c.ovnLegacyClient.CombineEgressACLCmd(pgName, egressAllowAsName, egressExceptAsName, protocol, egressPorts, svcAsName, logEnable, egressAclCmd, 0) } + + klog.Infof("create egress acl cmd is: %v", egressAclCmd) + if err = c.ovnLegacyClient.CreateACL(egressAclCmd); err != nil { + klog.Errorf("failed to create egress acls for np %s, %v", key, err) + return err + } + if err = c.ovnLegacyClient.SetAclLog(pgName, logEnable, false); err != nil { // just log and do not return err here klog.Errorf("failed to set egress acl log for np %s, %v", key, err) @@ -509,6 +509,23 @@ func (c *Controller) handleUpdateNp(key string) error { } } } + } else { + if err = c.ovnLegacyClient.DeleteACL(pgName, "from-lport"); err != nil { + klog.Errorf("failed to delete np %s egress acls, %v", key, err) + return err + } + + asNames, err := c.ovnLegacyClient.ListNpAddressSet(np.Namespace, np.Name, "egress") + if err != nil { + klog.Errorf("failed to list egress address_set, %v", err) + return err + } + for _, asName := range asNames { + if err = c.ovnLegacyClient.DeleteAddressSet(asName); err != nil { + klog.Errorf("failed to delete np %s address set, %v", key, err) + return err + } + } } if err = c.ovnLegacyClient.CreateGatewayACL(pgName, subnet.Spec.Gateway, subnet.Spec.CIDRBlock); err != nil { diff --git a/pkg/ovs/ovn-nbctl-legacy.go b/pkg/ovs/ovn-nbctl-legacy.go index 2653a5fdafd..27f7a6cf88a 100644 --- a/pkg/ovs/ovn-nbctl-legacy.go +++ b/pkg/ovs/ovn-nbctl-legacy.go @@ -1611,7 +1611,7 @@ func (c LegacyClient) CreateNpAddressSet(asName, npNamespace, npName, direction return err } -func (c LegacyClient) CreateIngressACL(pgName, asIngressName, asExceptName, svcAsName, protocol string, npp []netv1.NetworkPolicyPort, logEnable bool) error { +func (c LegacyClient) CombineIngressACLCmd(pgName, asIngressName, asExceptName, svcAsName, protocol string, npp []netv1.NetworkPolicyPort, logEnable bool, aclCmds []string, index int) []string { var allowArgs, ovnArgs []string ipSuffix := "ip4" @@ -1620,63 +1620,70 @@ func (c LegacyClient) CreateIngressACL(pgName, asIngressName, asExceptName, svcA } if logEnable { - ovnArgs = []string{MayExist, "--type=port-group", "--log", fmt.Sprintf("--severity=%s", "warning"), "acl-add", pgName, "to-lport", util.IngressDefaultDrop, fmt.Sprintf("outport==@%s && ip", pgName), "drop"} + ovnArgs = []string{"--", fmt.Sprintf("--id=@%s.drop.%d", pgName, index), "create", "acl", "action=drop", "direction=to-lport", "log=true", "severity=warning", fmt.Sprintf("priority=%s", util.IngressDefaultDrop), fmt.Sprintf("match=\"%s\"", fmt.Sprintf("outport==@%s && ip", pgName)), "--", "add", "port-group", pgName, "acls", fmt.Sprintf("@%s.drop.%d", pgName, index)} } else { - ovnArgs = []string{MayExist, "--type=port-group", "acl-add", pgName, "to-lport", util.IngressDefaultDrop, fmt.Sprintf("outport==@%s && ip", pgName), "drop"} + ovnArgs = []string{"--", fmt.Sprintf("--id=@%s.drop.%d", pgName, index), "create", "acl", "action=drop", "direction=to-lport", "log=false", fmt.Sprintf("priority=%s", util.IngressDefaultDrop), fmt.Sprintf("match=\"%s\"", fmt.Sprintf("outport==@%s && ip", pgName)), "--", "add", "port-group", pgName, "acls", fmt.Sprintf("@%s.drop.%d", pgName, index)} } if len(npp) == 0 { - allowArgs = []string{"--", MayExist, "--type=port-group", "acl-add", pgName, "to-lport", util.IngressAllowPriority, fmt.Sprintf("%s.src == $%s && %s.src != $%s && outport==@%s && ip", ipSuffix, asIngressName, ipSuffix, asExceptName, pgName), "allow-related"} + allowArgs = []string{"--", fmt.Sprintf("--id=@%s.noport.%d", pgName, index), "create", "acl", "action=allow-related", "direction=to-lport", fmt.Sprintf("priority=%s", util.IngressAllowPriority), fmt.Sprintf("match=\"%s\"", fmt.Sprintf("%s.src == $%s && %s.src != $%s && outport==@%s && ip", ipSuffix, asIngressName, ipSuffix, asExceptName, pgName)), "--", "add", "port-group", pgName, "acls", fmt.Sprintf("@%s.noport.%d", pgName, index)} ovnArgs = append(ovnArgs, allowArgs...) } else { - for _, port := range npp { + for pidx, port := range npp { if port.Port != nil { if port.EndPort != nil { - allowArgs = []string{"--", MayExist, "--type=port-group", "acl-add", pgName, "to-lport", util.IngressAllowPriority, fmt.Sprintf("%s.src == $%s && %s.src != $%s && %d <= %s.dst <= %d && outport==@%s && ip", ipSuffix, asIngressName, ipSuffix, asExceptName, port.Port.IntVal, strings.ToLower(string(*port.Protocol)), *port.EndPort, pgName), "allow-related"} + allowArgs = []string{"--", fmt.Sprintf("--id=@%s.%d.port.%d", pgName, index, pidx), "create", "acl", "action=allow-related", "direction=to-lport", fmt.Sprintf("priority=%s", util.IngressAllowPriority), fmt.Sprintf("match=\"%s\"", fmt.Sprintf("%s.src == $%s && %s.src != $%s && %d <= %s.dst <= %d && outport==@%s && ip", ipSuffix, asIngressName, ipSuffix, asExceptName, port.Port.IntVal, strings.ToLower(string(*port.Protocol)), *port.EndPort, pgName)), "--", "add", "port-group", pgName, "acls", fmt.Sprintf("@%s.%d.port.%d", pgName, index, pidx)} } else { - allowArgs = []string{"--", MayExist, "--type=port-group", "acl-add", pgName, "to-lport", util.IngressAllowPriority, fmt.Sprintf("%s.src == $%s && %s.src != $%s && %s.dst == %d && outport==@%s && ip", ipSuffix, asIngressName, ipSuffix, asExceptName, strings.ToLower(string(*port.Protocol)), port.Port.IntVal, pgName), "allow-related"} + allowArgs = []string{"--", fmt.Sprintf("--id=@%s.%d.port.%d", pgName, index, pidx), "create", "acl", "action=allow-related", "direction=to-lport", fmt.Sprintf("priority=%s", util.IngressAllowPriority), fmt.Sprintf("match=\"%s\"", fmt.Sprintf("%s.src == $%s && %s.src != $%s && %s.dst == %d && outport==@%s && ip", ipSuffix, asIngressName, ipSuffix, asExceptName, strings.ToLower(string(*port.Protocol)), port.Port.IntVal, pgName)), "--", "add", "port-group", pgName, "acls", fmt.Sprintf("@%s.%d.port.%d", pgName, index, pidx)} } } else { - allowArgs = []string{"--", MayExist, "--type=port-group", "acl-add", pgName, "to-lport", util.IngressAllowPriority, fmt.Sprintf("%s.src == $%s && %s.src != $%s && %s && outport==@%s && ip", ipSuffix, asIngressName, ipSuffix, asExceptName, strings.ToLower(string(*port.Protocol)), pgName), "allow-related"} + allowArgs = []string{"--", fmt.Sprintf("--id=@%s.%d.port.%d", pgName, index, pidx), "create", "acl", "action=allow-related", "direction=to-lport", fmt.Sprintf("priority=%s", util.IngressAllowPriority), fmt.Sprintf("match=\"%s\"", fmt.Sprintf("%s.src == $%s && %s.src != $%s && %s && outport==@%s && ip", ipSuffix, asIngressName, ipSuffix, asExceptName, strings.ToLower(string(*port.Protocol)), pgName)), "--", "add", "port-group", pgName, "acls", fmt.Sprintf("@%s.%d.port.%d", pgName, index, pidx)} } ovnArgs = append(ovnArgs, allowArgs...) } } - _, err := c.ovnNbCommand(ovnArgs...) + aclCmds = append(aclCmds, ovnArgs...) + return aclCmds +} + +func (c LegacyClient) CreateACL(aclCmds []string) error { + _, err := c.ovnNbCommand(aclCmds...) return err } -func (c LegacyClient) CreateEgressACL(pgName, asEgressName, asExceptName, protocol string, npp []netv1.NetworkPolicyPort, portSvcName string, logEnable bool) error { +func (c LegacyClient) CombineEgressACLCmd(pgName, asEgressName, asExceptName, protocol string, npp []netv1.NetworkPolicyPort, portSvcName string, logEnable bool, aclCmds []string, index int) []string { var allowArgs, ovnArgs []string ipSuffix := "ip4" if protocol == kubeovnv1.ProtocolIPv6 { ipSuffix = "ip6" } + if logEnable { - ovnArgs = []string{"--", MayExist, "--type=port-group", "--log", fmt.Sprintf("--severity=%s", "warning"), "acl-add", pgName, "from-lport", util.EgressDefaultDrop, fmt.Sprintf("inport==@%s && ip", pgName), "drop"} + ovnArgs = []string{"--", fmt.Sprintf("--id=@%s.drop.%d", pgName, index), "create", "acl", "action=drop", "direction=from-lport", "log=true", "severity=warning", fmt.Sprintf("priority=%s", util.EgressDefaultDrop), fmt.Sprintf("match=\"%s\"", fmt.Sprintf("inport==@%s && ip", pgName)), "--", "add", "port-group", pgName, "acls", fmt.Sprintf("@%s.drop.%d", pgName, index)} } else { - ovnArgs = []string{"--", MayExist, "--type=port-group", "acl-add", pgName, "from-lport", util.EgressDefaultDrop, fmt.Sprintf("inport==@%s && ip", pgName), "drop"} + ovnArgs = []string{"--", fmt.Sprintf("--id=@%s.drop.%d", pgName, index), "create", "acl", "action=drop", "direction=from-lport", "log=false", fmt.Sprintf("priority=%s", util.EgressDefaultDrop), fmt.Sprintf("match=\"%s\"", fmt.Sprintf("inport==@%s && ip", pgName)), "--", "add", "port-group", pgName, "acls", fmt.Sprintf("@%s.drop.%d", pgName, index)} } + if len(npp) == 0 { - allowArgs = []string{"--", MayExist, "--type=port-group", "acl-add", pgName, "from-lport", util.EgressAllowPriority, fmt.Sprintf("%s.dst == $%s && %s.dst != $%s && inport==@%s && ip", ipSuffix, asEgressName, ipSuffix, asExceptName, pgName), "allow-related"} + allowArgs = []string{"--", fmt.Sprintf("--id=@%s.noport.%d", pgName, index), "create", "acl", "action=allow-related", "direction=from-lport", fmt.Sprintf("priority=%s", util.EgressAllowPriority), fmt.Sprintf("match=\"%s\"", fmt.Sprintf("%s.dst == $%s && %s.dst != $%s && inport==@%s && ip", ipSuffix, asEgressName, ipSuffix, asExceptName, pgName)), "--", "add", "port-group", pgName, "acls", fmt.Sprintf("@%s.noport.%d", pgName, index)} ovnArgs = append(ovnArgs, allowArgs...) } else { - for _, port := range npp { + for pidx, port := range npp { if port.Port != nil { if port.EndPort != nil { - allowArgs = []string{"--", MayExist, "--type=port-group", "acl-add", pgName, "from-lport", util.EgressAllowPriority, fmt.Sprintf("%s.dst == $%s && %s.dst != $%s && %d <= %s.dst <= %d && inport==@%s && ip", ipSuffix, asEgressName, ipSuffix, asExceptName, port.Port.IntVal, strings.ToLower(string(*port.Protocol)), *port.EndPort, pgName), "allow-related"} + allowArgs = []string{"--", fmt.Sprintf("--id=@%s.%d.port.%d", pgName, index, pidx), "create", "acl", "action=allow-related", "direction=from-lport", fmt.Sprintf("priority=%s", util.EgressAllowPriority), fmt.Sprintf("match=\"%s\"", fmt.Sprintf("%s.dst == $%s && %s.dst != $%s && %d <= %s.dst <= %d && inport==@%s && ip", ipSuffix, asEgressName, ipSuffix, asExceptName, port.Port.IntVal, strings.ToLower(string(*port.Protocol)), *port.EndPort, pgName)), "--", "add", "port-group", pgName, "acls", fmt.Sprintf("@%s.%d.port.%d", pgName, index, pidx)} } else { - allowArgs = []string{"--", MayExist, "--type=port-group", "acl-add", pgName, "from-lport", util.EgressAllowPriority, fmt.Sprintf("%s.dst == $%s && %s.dst != $%s && %s.dst == %d && inport==@%s && ip", ipSuffix, asEgressName, ipSuffix, asExceptName, strings.ToLower(string(*port.Protocol)), port.Port.IntVal, pgName), "allow-related"} + allowArgs = []string{"--", fmt.Sprintf("--id=@%s.%d.port.%d", pgName, index, pidx), "create", "acl", "action=allow-related", "direction=from-lport", fmt.Sprintf("priority=%s", util.EgressAllowPriority), fmt.Sprintf("match=\"%s\"", fmt.Sprintf("%s.dst == $%s && %s.dst != $%s && %s.dst == %d && inport==@%s && ip", ipSuffix, asEgressName, ipSuffix, asExceptName, strings.ToLower(string(*port.Protocol)), port.Port.IntVal, pgName)), "--", "add", "port-group", pgName, "acls", fmt.Sprintf("@%s.%d.port.%d", pgName, index, pidx)} } } else { - allowArgs = []string{"--", MayExist, "--type=port-group", "acl-add", pgName, "from-lport", util.EgressAllowPriority, fmt.Sprintf("%s.dst == $%s && %s.dst != $%s && %s && inport==@%s && ip", ipSuffix, asEgressName, ipSuffix, asExceptName, strings.ToLower(string(*port.Protocol)), pgName), "allow-related"} + allowArgs = []string{"--", fmt.Sprintf("--id=@%s.%d.port.%d", pgName, index, pidx), "create", "acl", "action=allow-related", "direction=from-lport", fmt.Sprintf("priority=%s", util.EgressAllowPriority), fmt.Sprintf("match=\"%s\"", fmt.Sprintf("%s.dst == $%s && %s.dst != $%s && %s && inport==@%s && ip", ipSuffix, asEgressName, ipSuffix, asExceptName, strings.ToLower(string(*port.Protocol)), pgName)), "--", "add", "port-group", pgName, "acls", fmt.Sprintf("@%s.%d.port.%d", pgName, index, pidx)} } ovnArgs = append(ovnArgs, allowArgs...) } } - _, err := c.ovnNbCommand(ovnArgs...) - return err + aclCmds = append(aclCmds, ovnArgs...) + return aclCmds } func (c LegacyClient) DeleteACL(pgName, direction string) (err error) {