From e5c82217a94f56e44b245e68f53bf583ce3a3481 Mon Sep 17 00:00:00 2001 From: liguo <404200721@qq.com> Date: Tue, 25 Apr 2023 19:32:29 +0800 Subject: [PATCH] fix network policy err --- pkg/controller/network_policy.go | 22 ++-- pkg/ovs/ovn-nb-acl.go | 207 +++++++++++++++++++------------ 2 files changed, 139 insertions(+), 90 deletions(-) diff --git a/pkg/controller/network_policy.go b/pkg/controller/network_policy.go index f5f2e364fa5..6eb1808309a 100644 --- a/pkg/controller/network_policy.go +++ b/pkg/controller/network_policy.go @@ -242,12 +242,7 @@ func (c *Controller) handleUpdateNp(key string) error { } } - // we should first delete acl and address_set before update - if err = c.ovnClient.DeleteAcls(pgName, portGroupKey, "", nil); err != nil { - klog.Errorf("delete np %s acls: %v", key, err) - return err - } - + // we should first delete address_set before update if err := c.ovnClient.DeleteAddressSets(map[string]string{ networkPolicyKey: fmt.Sprintf("%s/%s/%s", np.Namespace, np.Name, "ingress"), }); err != nil { @@ -255,13 +250,6 @@ func (c *Controller) handleUpdateNp(key string) error { return err } - if err := c.ovnClient.DeleteAddressSets(map[string]string{ - networkPolicyKey: fmt.Sprintf("%s/%s/%s", np.Namespace, np.Name, "egress"), - }); err != nil { - klog.Errorf("delete np %s egress address set: %v", key, err) - return err - } - if hasIngressRule(np) { for _, cidrBlock := range strings.Split(subnet.Spec.CIDRBlock, ",") { protocol := util.CheckProtocol(cidrBlock) @@ -396,6 +384,14 @@ func (c *Controller) handleUpdateNp(key string) error { } } + // we should first delete address_set before update + if err := c.ovnClient.DeleteAddressSets(map[string]string{ + networkPolicyKey: fmt.Sprintf("%s/%s/%s", np.Namespace, np.Name, "egress"), + }); err != nil { + klog.Errorf("delete np %s egress address set: %v", key, err) + return err + } + if hasEgressRule(np) { for _, cidrBlock := range strings.Split(subnet.Spec.CIDRBlock, ",") { protocol := util.CheckProtocol(cidrBlock) diff --git a/pkg/ovs/ovn-nb-acl.go b/pkg/ovs/ovn-nb-acl.go index 010bbaa8c7b..0351fa06d1c 100644 --- a/pkg/ovs/ovn-nb-acl.go +++ b/pkg/ovs/ovn-nb-acl.go @@ -53,8 +53,23 @@ func (c *ovnClient) CreateIngressAcl(pgName, asIngressName, asExceptName, protoc acls = append(acls, allowAcl) } - if err := c.CreateAcls(pgName, portGroupKey, acls...); err != nil { - return fmt.Errorf("add ingress acls to port group %s: %v", pgName, err) + // let clear acl and add acl in one transaction to prevent some corner case err + AclsDeleteOp, err := c.DeleteAclsOps(pgName, portGroupKey, ovnnb.ACLDirectionToLport, nil) + if err != nil { + return err + } + + aclsAddOp, err := c.CreateAclsOps(pgName, portGroupKey, acls...) + if err != nil { + return err + } + + ops := make([]ovsdb.Operation, 0, len(AclsDeleteOp)+len(aclsAddOp)) + ops = append(ops, AclsDeleteOp...) + ops = append(ops, aclsAddOp...) + + if err = c.Transact("acls-add", ops); err != nil { + return fmt.Errorf("add ingress acls to %s: %v", pgName, err) } return nil @@ -104,8 +119,23 @@ func (c *ovnClient) CreateEgressAcl(pgName, asEgressName, asExceptName, protocol acls = append(acls, allowAcl) } - if err := c.CreateAcls(pgName, portGroupKey, acls...); err != nil { - return fmt.Errorf("add egress acls to port group %s: %v", pgName, err) + // let clear acl and add acl in one transaction to prevent some corner case err + AclsDeleteOp, err := c.DeleteAclsOps(pgName, portGroupKey, ovnnb.ACLDirectionFromLport, nil) + if err != nil { + return err + } + + aclsAddOp, err := c.CreateAclsOps(pgName, portGroupKey, acls...) + if err != nil { + return err + } + + ops := make([]ovsdb.Operation, 0, len(AclsDeleteOp)+len(aclsAddOp)) + ops = append(ops, AclsDeleteOp...) + ops = append(ops, aclsAddOp...) + + if err = c.Transact("acls-add", ops); err != nil { + return fmt.Errorf("add ingress acls to %s: %v", pgName, err) } return nil @@ -573,45 +603,11 @@ func (c *ovnClient) SetAclLog(pgName string, logEnable, isIngress bool) error { // CreateAcls create several acl once // parentType is 'ls' or 'pg' func (c *ovnClient) CreateAcls(parentName, parentType string, acls ...*ovnnb.ACL) error { - if parentType != portGroupKey && parentType != logicalSwitchKey { - return fmt.Errorf("acl parent type must be '%s' or '%s'", portGroupKey, logicalSwitchKey) - } - - if len(acls) == 0 { - return nil - } - - models := make([]model.Model, 0, len(acls)) - aclUUIDs := make([]string, 0, len(acls)) - for _, acl := range acls { - if acl != nil { - models = append(models, model.Model(acl)) - aclUUIDs = append(aclUUIDs, acl.UUID) - } - } - - createAclsOp, err := c.ovnNbClient.Create(models...) + ops, err := c.CreateAclsOps(parentName, parentType, acls...) if err != nil { - return fmt.Errorf("generate operations for creating acls: %v", err) - } - - var aclAddOp []ovsdb.Operation - if parentType == portGroupKey { // acl attach to port group - aclAddOp, err = c.portGroupUpdateAclOp(parentName, aclUUIDs, ovsdb.MutateOperationInsert) - if err != nil { - return fmt.Errorf("generate operations for adding acls to port group %s: %v", parentName, err) - } - } else { // acl attach to logical switch - aclAddOp, err = c.logicalSwitchUpdateAclOp(parentName, aclUUIDs, ovsdb.MutateOperationInsert) - if err != nil { - return fmt.Errorf("generate operations for adding acls to logical switch %s: %v", parentName, err) - } + return err } - ops := make([]ovsdb.Operation, 0, len(createAclsOp)+len(aclAddOp)) - ops = append(ops, createAclsOp...) - ops = append(ops, aclAddOp...) - if err = c.Transact("acls-add", ops); err != nil { return fmt.Errorf("add acls to type %s %s: %v", parentType, parentName, err) } @@ -641,46 +637,11 @@ func (c *ovnClient) CreateBareAcl(parentName, direction, priority, match, action // delete to-lport and from-lport direction acl when direction is empty, otherwise one-way // parentType is 'ls' or 'pg' func (c *ovnClient) DeleteAcls(parentName, parentType string, direction string, externalIDs map[string]string) error { - if externalIDs == nil { - externalIDs = make(map[string]string) - } - - externalIDs[aclParentKey] = parentName - - /* delete acls from port group or logical switch */ - acls, err := c.ListAcls(direction, externalIDs) - if err != nil { - return fmt.Errorf("list type %s %s acls: %v", parentType, parentName, err) - } - - aclUUIDs := make([]string, 0, len(acls)) - for _, acl := range acls { - aclUUIDs = append(aclUUIDs, acl.UUID) - } - - var removeAclOp []ovsdb.Operation - if parentType == portGroupKey { // remove acl from port group - removeAclOp, err = c.portGroupUpdateAclOp(parentName, aclUUIDs, ovsdb.MutateOperationDelete) - if err != nil { - return fmt.Errorf("generate operations for deleting acls from port group %s: %v", parentName, err) - } - } else { // remove acl from logical switch - removeAclOp, err = c.logicalSwitchUpdateAclOp(parentName, aclUUIDs, ovsdb.MutateOperationDelete) - if err != nil { - return fmt.Errorf("generate operations for deleting acls from logical switch %s: %v", parentName, err) - } - } - - // delete acls - delAclsOp, err := c.WhereCache(aclFilter(direction, externalIDs)).Delete() + ops, err := c.DeleteAclsOps(parentName, parentType, direction, externalIDs) if err != nil { - return fmt.Errorf("generate operation for deleting acls: %v", err) + return err } - ops := make([]ovsdb.Operation, 0, len(removeAclOp)+len(delAclsOp)) - ops = append(ops, removeAclOp...) - ops = append(ops, delAclsOp...) - if err = c.Transact("acls-del", ops); err != nil { return fmt.Errorf("del acls from type %s %s: %v", parentType, parentName, err) } @@ -1032,3 +993,95 @@ func aclFilter(direction string, externalIDs map[string]string) func(acl *ovnnb. return true } } + +// CreateAcls return operations which create several acl once +// parentType is 'ls' or 'pg' +func (c *ovnClient) CreateAclsOps(parentName, parentType string, acls ...*ovnnb.ACL) ([]ovsdb.Operation, error) { + if parentType != portGroupKey && parentType != logicalSwitchKey { + return nil, fmt.Errorf("acl parent type must be '%s' or '%s'", portGroupKey, logicalSwitchKey) + } + + if len(acls) == 0 { + return nil, nil + } + + models := make([]model.Model, 0, len(acls)) + aclUUIDs := make([]string, 0, len(acls)) + for _, acl := range acls { + if acl != nil { + models = append(models, model.Model(acl)) + aclUUIDs = append(aclUUIDs, acl.UUID) + } + } + + createAclsOp, err := c.ovnNbClient.Create(models...) + if err != nil { + return nil, fmt.Errorf("generate operations for creating acls: %v", err) + } + + var aclAddOp []ovsdb.Operation + if parentType == portGroupKey { // acl attach to port group + aclAddOp, err = c.portGroupUpdateAclOp(parentName, aclUUIDs, ovsdb.MutateOperationInsert) + if err != nil { + return nil, fmt.Errorf("generate operations for adding acls to port group %s: %v", parentName, err) + } + } else { // acl attach to logical switch + aclAddOp, err = c.logicalSwitchUpdateAclOp(parentName, aclUUIDs, ovsdb.MutateOperationInsert) + if err != nil { + return nil, fmt.Errorf("generate operations for adding acls to logical switch %s: %v", parentName, err) + } + } + + ops := make([]ovsdb.Operation, 0, len(createAclsOp)+len(aclAddOp)) + ops = append(ops, createAclsOp...) + ops = append(ops, aclAddOp...) + + return ops, nil +} + +// DeleteAcls return operation which delete several acl once, +// delete to-lport and from-lport direction acl when direction is empty, otherwise one-way +// parentType is 'ls' or 'pg' +func (c *ovnClient) DeleteAclsOps(parentName, parentType string, direction string, externalIDs map[string]string) ([]ovsdb.Operation, error) { + if externalIDs == nil { + externalIDs = make(map[string]string) + } + + externalIDs[aclParentKey] = parentName + + /* delete acls from port group or logical switch */ + acls, err := c.ListAcls(direction, externalIDs) + if err != nil { + return nil, fmt.Errorf("list type %s %s acls: %v", parentType, parentName, err) + } + + aclUUIDs := make([]string, 0, len(acls)) + for _, acl := range acls { + aclUUIDs = append(aclUUIDs, acl.UUID) + } + + var removeAclOp []ovsdb.Operation + if parentType == portGroupKey { // remove acl from port group + removeAclOp, err = c.portGroupUpdateAclOp(parentName, aclUUIDs, ovsdb.MutateOperationDelete) + if err != nil { + return nil, fmt.Errorf("generate operations for deleting acls from port group %s: %v", parentName, err) + } + } else { // remove acl from logical switch + removeAclOp, err = c.logicalSwitchUpdateAclOp(parentName, aclUUIDs, ovsdb.MutateOperationDelete) + if err != nil { + return nil, fmt.Errorf("generate operations for deleting acls from logical switch %s: %v", parentName, err) + } + } + + // delete acls + delAclsOp, err := c.WhereCache(aclFilter(direction, externalIDs)).Delete() + if err != nil { + return nil, fmt.Errorf("generate operation for deleting acls: %v", err) + } + + ops := make([]ovsdb.Operation, 0, len(removeAclOp)+len(delAclsOp)) + ops = append(ops, removeAclOp...) + ops = append(ops, delAclsOp...) + + return ops, nil +}