Skip to content

Commit

Permalink
do not need to delete pg when update networkpolicy (#1959)
Browse files Browse the repository at this point in the history
  • Loading branch information
@hongzhen-ma
hongzhen-ma committed Nov 3, 2022
1 parent 5231059 commit d41c467
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 56 deletions.
91 changes: 54 additions & 37 deletions pkg/controller/network_policy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down
45 changes: 26 additions & 19 deletions pkg/ovs/ovn-nbctl-legacy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1484,7 +1484,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"
Expand All @@ -1493,63 +1493,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) {
Expand Down

0 comments on commit d41c467

Please sign in to comment.