diff --git a/mocks/pkg/ovs/interface.go b/mocks/pkg/ovs/interface.go index bcfe04e24439..998bcec1bf1c 100644 --- a/mocks/pkg/ovs/interface.go +++ b/mocks/pkg/ovs/interface.go @@ -772,20 +772,6 @@ func (mr *MockLogicalSwitchPortMockRecorder) SetLogicalSwitchPortsSecurityGroup( return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetLogicalSwitchPortsSecurityGroup", reflect.TypeOf((*MockLogicalSwitchPort)(nil).SetLogicalSwitchPortsSecurityGroup), sgName, op) } -// UpdateLogicalSwitchAcl mocks base method. -func (m *MockLogicalSwitchPort) UpdateLogicalSwitchAcl(lsName string, subnetAcls []v1.Acl) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "UpdateLogicalSwitchAcl", lsName, subnetAcls) - ret0, _ := ret[0].(error) - return ret0 -} - -// UpdateLogicalSwitchAcl indicates an expected call of UpdateLogicalSwitchAcl. -func (mr *MockLogicalSwitchPortMockRecorder) UpdateLogicalSwitchAcl(lsName, subnetAcls interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateLogicalSwitchAcl", reflect.TypeOf((*MockLogicalSwitchPort)(nil).UpdateLogicalSwitchAcl), lsName, subnetAcls) -} - // MockLoadBalancer is a mock of LoadBalancer interface. type MockLoadBalancer struct { ctrl *gomock.Controller @@ -1185,17 +1171,17 @@ func (mr *MockACLMockRecorder) CreateSgDenyAllAcl(sgName interface{}) *gomock.Ca } // DeleteAcls mocks base method. -func (m *MockACL) DeleteAcls(parentName, parentType, direction string) error { +func (m *MockACL) DeleteAcls(parentName, parentType, direction string, externalIDs map[string]string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteAcls", parentName, parentType, direction) + ret := m.ctrl.Call(m, "DeleteAcls", parentName, parentType, direction, externalIDs) ret0, _ := ret[0].(error) return ret0 } // DeleteAcls indicates an expected call of DeleteAcls. -func (mr *MockACLMockRecorder) DeleteAcls(parentName, parentType, direction interface{}) *gomock.Call { +func (mr *MockACLMockRecorder) DeleteAcls(parentName, parentType, direction, externalIDs interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAcls", reflect.TypeOf((*MockACL)(nil).DeleteAcls), parentName, parentType, direction) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAcls", reflect.TypeOf((*MockACL)(nil).DeleteAcls), parentName, parentType, direction, externalIDs) } // SetAclLog mocks base method. @@ -1226,6 +1212,20 @@ func (mr *MockACLMockRecorder) SetLogicalSwitchPrivate(lsName, cidrBlock, allowS return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetLogicalSwitchPrivate", reflect.TypeOf((*MockACL)(nil).SetLogicalSwitchPrivate), lsName, cidrBlock, allowSubnets) } +// UpdateLogicalSwitchAcl mocks base method. +func (m *MockACL) UpdateLogicalSwitchAcl(lsName string, subnetAcls []v1.Acl) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "UpdateLogicalSwitchAcl", lsName, subnetAcls) + ret0, _ := ret[0].(error) + return ret0 +} + +// UpdateLogicalSwitchAcl indicates an expected call of UpdateLogicalSwitchAcl. +func (mr *MockACLMockRecorder) UpdateLogicalSwitchAcl(lsName, subnetAcls interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateLogicalSwitchAcl", reflect.TypeOf((*MockACL)(nil).UpdateLogicalSwitchAcl), lsName, subnetAcls) +} + // UpdateSgAcl mocks base method. func (m *MockACL) UpdateSgAcl(sg *v1.SecurityGroup, direction string) error { m.ctrl.T.Helper() @@ -2160,17 +2160,17 @@ func (mr *MockOvnClientMockRecorder) CreateVirtualLogicalSwitchPorts(lsName inte } // DeleteAcls mocks base method. -func (m *MockOvnClient) DeleteAcls(parentName, parentType, direction string) error { +func (m *MockOvnClient) DeleteAcls(parentName, parentType, direction string, externalIDs map[string]string) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteAcls", parentName, parentType, direction) + ret := m.ctrl.Call(m, "DeleteAcls", parentName, parentType, direction, externalIDs) ret0, _ := ret[0].(error) return ret0 } // DeleteAcls indicates an expected call of DeleteAcls. -func (mr *MockOvnClientMockRecorder) DeleteAcls(parentName, parentType, direction interface{}) *gomock.Call { +func (mr *MockOvnClientMockRecorder) DeleteAcls(parentName, parentType, direction, externalIDs interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAcls", reflect.TypeOf((*MockOvnClient)(nil).DeleteAcls), parentName, parentType, direction) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteAcls", reflect.TypeOf((*MockOvnClient)(nil).DeleteAcls), parentName, parentType, direction, externalIDs) } // DeleteAddressSet mocks base method. diff --git a/pkg/controller/network_policy.go b/pkg/controller/network_policy.go index af6360a6b873..f5f2e364fa50 100644 --- a/pkg/controller/network_policy.go +++ b/pkg/controller/network_policy.go @@ -242,6 +242,12 @@ 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 + } + if err := c.ovnClient.DeleteAddressSets(map[string]string{ networkPolicyKey: fmt.Sprintf("%s/%s/%s", np.Namespace, np.Name, "ingress"), }); err != nil { @@ -377,7 +383,7 @@ func (c *Controller) handleUpdateNp(key string) error { } } } else { - if err = c.ovnClient.DeleteAcls(key, portGroupKey, "to-lport"); err != nil { + if err = c.ovnClient.DeleteAcls(key, portGroupKey, "to-lport", nil); err != nil { klog.Errorf("delete np %s ingress acls: %v", key, err) return err } @@ -509,7 +515,7 @@ func (c *Controller) handleUpdateNp(key string) error { } } } else { - if err = c.ovnClient.DeleteAcls(key, portGroupKey, "from-lport"); err != nil { + if err = c.ovnClient.DeleteAcls(key, portGroupKey, "from-lport", nil); err != nil { klog.Errorf("delete np %s egress acls: %v", key, err) return err } diff --git a/pkg/controller/node.go b/pkg/controller/node.go index 6bd8cffbb09d..8e1af33e31d6 100644 --- a/pkg/controller/node.go +++ b/pkg/controller/node.go @@ -977,7 +977,7 @@ func (c *Controller) checkAndUpdateNodePortGroup() error { } } else { // clear all acl - if err = c.ovnClient.DeleteAcls(pgName, portGroupKey, ""); err != nil { + if err = c.ovnClient.DeleteAcls(pgName, portGroupKey, "", nil); err != nil { klog.Errorf("delete node acl for node pg %s: %v", pgName, err) } } diff --git a/pkg/controller/subnet.go b/pkg/controller/subnet.go index 27669277d1eb..b6ab11fcb3fd 100644 --- a/pkg/controller/subnet.go +++ b/pkg/controller/subnet.go @@ -757,7 +757,7 @@ func (c *Controller) handleAddOrUpdateSubnet(key string) error { c.patchSubnetStatus(subnet, "SetPrivateLogicalSwitchSuccess", "") } else { // clear acl when direction is "" - if err = c.ovnClient.DeleteAcls(key, logicalSwitchKey, ""); err != nil { + if err = c.ovnClient.DeleteAcls(key, logicalSwitchKey, "", nil); err != nil { c.patchSubnetStatus(subnet, "ResetLogicalSwitchAclFailed", err.Error()) return err } @@ -824,7 +824,7 @@ func (c *Controller) handleDeleteLogicalSwitch(key string) (err error) { } // clear acl when direction is "" - if err = c.ovnClient.DeleteAcls(key, logicalSwitchKey, ""); err != nil { + if err = c.ovnClient.DeleteAcls(key, logicalSwitchKey, "", nil); err != nil { klog.Errorf("clear logical switch %s acls: %v", key, err) return err } diff --git a/pkg/ovs/interface.go b/pkg/ovs/interface.go index aa8488c940ea..3701b708427e 100644 --- a/pkg/ovs/interface.go +++ b/pkg/ovs/interface.go @@ -59,7 +59,6 @@ type LogicalSwitchPort interface { SetLogicalSwitchPortExternalIds(lspName string, externalIds map[string]string) error SetLogicalSwitchPortVlanTag(lspName string, vlanID int) error SetLogicalSwitchPortsSecurityGroup(sgName string, op string) error - UpdateLogicalSwitchAcl(lsName string, subnetAcls []kubeovnv1.Acl) error EnablePortLayer2forward(lspName string) error DeleteLogicalSwitchPort(lspName string) error ListLogicalSwitchPorts(needVendorFilter bool, externalIDs map[string]string, filter func(lsp *ovnnb.LogicalSwitchPort) bool) ([]ovnnb.LogicalSwitchPort, error) @@ -99,9 +98,10 @@ type ACL interface { CreateSgDenyAllAcl(sgName string) error CreateSgBaseACL(sgName string, direction string) error UpdateSgAcl(sg *kubeovnv1.SecurityGroup, direction string) error + UpdateLogicalSwitchAcl(lsName string, subnetAcls []kubeovnv1.Acl) error SetAclLog(pgName string, logEnable, isIngress bool) error SetLogicalSwitchPrivate(lsName, cidrBlock string, allowSubnets []string) error - DeleteAcls(parentName, parentType string, direction string) error + DeleteAcls(parentName, parentType string, direction string, externalIDs map[string]string) error } type AddressSet interface { diff --git a/pkg/ovs/ovn-nb-acl.go b/pkg/ovs/ovn-nb-acl.go index 057d9cd1ea1d..010bbaa8c7b6 100644 --- a/pkg/ovs/ovn-nb-acl.go +++ b/pkg/ovs/ovn-nb-acl.go @@ -136,7 +136,14 @@ func (c *ovnClient) CreateGatewayAcl(lsName, pgName, gateway string) error { return fmt.Errorf("new allow ingress acl for %s: %v", parentName, err) } - allowEgressAcl, err := c.newAcl(parentName, ovnnb.ACLDirectionFromLport, util.EgressAllowPriority, fmt.Sprintf("%s.dst == %s", ipSuffix, gw), ovnnb.ACLActionAllowRelated) + options := func(acl *ovnnb.ACL) { + if acl.Options == nil { + acl.Options = make(map[string]string) + } + acl.Options["apply-after-lb"] = "true" + } + + allowEgressAcl, err := c.newAcl(parentName, ovnnb.ACLDirectionFromLport, util.EgressAllowPriority, fmt.Sprintf("%s.dst == %s", ipSuffix, gw), ovnnb.ACLActionAllowRelated, options) if err != nil { return fmt.Errorf("new allow egress acl for %s: %v", parentName, err) } @@ -144,19 +151,12 @@ func (c *ovnClient) CreateGatewayAcl(lsName, pgName, gateway string) error { acls = append(acls, allowIngressAcl, allowEgressAcl) if ipSuffix == "ip6" { - options := func(acl *ovnnb.ACL) { - if acl.Options == nil { - acl.Options = make(map[string]string) - } - acl.Options["apply-after-lb"] = "true" - } - ndAcl, err := c.newAcl(parentName, ovnnb.ACLDirectionFromLport, util.EgressAllowPriority, "nd || nd_ra || nd_rs", ovnnb.ACLActionAllowRelated, options) - acls = append(acls, ndAcl) - if err != nil { return fmt.Errorf("new nd acl for %s: %v", parentName, err) } + + acls = append(acls, ndAcl) } } @@ -320,7 +320,7 @@ func (c *ovnClient) UpdateSgAcl(sg *kubeovnv1.SecurityGroup, direction string) e pgName := GetSgPortGroupName(sg.Name) // clear acl - if err := c.DeleteAcls(pgName, portGroupKey, direction); err != nil { + if err := c.DeleteAcls(pgName, portGroupKey, direction, nil); err != nil { return fmt.Errorf("delete direction '%s' acls from port group %s: %v", direction, pgName, err) } @@ -373,8 +373,8 @@ func (c *ovnClient) UpdateSgAcl(sg *kubeovnv1.SecurityGroup, direction string) e } func (c *ovnClient) UpdateLogicalSwitchAcl(lsName string, subnetAcls []kubeovnv1.Acl) error { - if err := c.DeleteAcls(lsName, logicalSwitchKey, ""); err != nil { - return fmt.Errorf("clear logical switch %s acls: %v", lsName, err) + if err := c.DeleteAcls(lsName, logicalSwitchKey, "", map[string]string{"subnet": lsName}); err != nil { + return fmt.Errorf("delete subnet acls from %s: %v", lsName, err) } if len(subnetAcls) == 0 { @@ -382,9 +382,16 @@ func (c *ovnClient) UpdateLogicalSwitchAcl(lsName string, subnetAcls []kubeovnv1 } acls := make([]*ovnnb.ACL, 0, len(subnetAcls)) + options := func(acl *ovnnb.ACL) { + if acl.ExternalIDs == nil { + acl.ExternalIDs = make(map[string]string) + } + acl.ExternalIDs["subnet"] = lsName + } + /* recreate logical switch acl */ for _, subnetAcl := range subnetAcls { - acl, err := c.newAcl(lsName, subnetAcl.Direction, strconv.Itoa(subnetAcl.Priority), subnetAcl.Match, subnetAcl.Action) + acl, err := c.newAcl(lsName, subnetAcl.Direction, strconv.Itoa(subnetAcl.Priority), subnetAcl.Match, subnetAcl.Action, options) if err != nil { return fmt.Errorf("new acl for logical switch %s: %v", lsName, err) } @@ -419,7 +426,7 @@ func (c *ovnClient) UpdateAcl(acl *ovnnb.ACL, fields ...interface{}) error { // SetLogicalSwitchPrivate will drop all ingress traffic except allow subnets, same subnet and node subnet func (c *ovnClient) SetLogicalSwitchPrivate(lsName, cidrBlock string, allowSubnets []string) error { // clear acls - if err := c.DeleteAcls(lsName, logicalSwitchKey, ""); err != nil { + if err := c.DeleteAcls(lsName, logicalSwitchKey, "", nil); err != nil { return fmt.Errorf("clear logical switch %s acls: %v", lsName, err) } @@ -633,8 +640,12 @@ func (c *ovnClient) CreateBareAcl(parentName, direction, priority, match, action // DeleteAcls 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) DeleteAcls(parentName, parentType string, direction string) error { - externalIDs := map[string]string{aclParentKey: parentName} +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) diff --git a/pkg/ovs/ovn-nb-acl_test.go b/pkg/ovs/ovn-nb-acl_test.go index 00fe8ada68cf..7fe7551e0f80 100644 --- a/pkg/ovs/ovn-nb-acl_test.go +++ b/pkg/ovs/ovn-nb-acl_test.go @@ -346,7 +346,9 @@ func (suite *OvnClientTestSuite) testCreateGatewayAcl() { checkAcl(parent, ovnnb.ACLDirectionToLport, util.IngressAllowPriority, match, nil) match = fmt.Sprintf("%s.dst == %s", ipSuffix, gw) - checkAcl(parent, ovnnb.ACLDirectionFromLport, util.EgressAllowPriority, match, nil) + checkAcl(parent, ovnnb.ACLDirectionFromLport, util.EgressAllowPriority, match, map[string]string{ + "apply-after-lb": "true", + }) if ipSuffix == "ip6" { match = "nd || nd_ra || nd_rs" @@ -777,6 +779,7 @@ func (suite *OvnClientTestSuite) testUpdateLogicalSwitchAcl() { require.NoError(t, err) expect := newAcl(lsName, subnetAcl.Direction, strconv.Itoa(subnetAcl.Priority), subnetAcl.Match, subnetAcl.Action) expect.UUID = acl.UUID + expect.ExternalIDs["subnet"] = lsName require.Equal(t, expect, acl) require.Contains(t, ls.ACLs, acl.UUID) } @@ -1242,7 +1245,7 @@ func (suite *OvnClientTestSuite) testDeleteAcls() { require.NoError(t, err) require.Len(t, pg.ACLs, 5) - err = ovnClient.DeleteAcls(pgName, portGroupKey, "") + err = ovnClient.DeleteAcls(pgName, portGroupKey, "", nil) require.NoError(t, err) pg, err = ovnClient.GetPortGroup(pgName, false) @@ -1279,7 +1282,7 @@ func (suite *OvnClientTestSuite) testDeleteAcls() { require.Len(t, pg.ACLs, 5) /* delete to-lport direction acl */ - err = ovnClient.DeleteAcls(pgName, portGroupKey, ovnnb.ACLDirectionToLport) + err = ovnClient.DeleteAcls(pgName, portGroupKey, ovnnb.ACLDirectionToLport, nil) require.NoError(t, err) pg, err = ovnClient.GetPortGroup(pgName, false) @@ -1287,7 +1290,7 @@ func (suite *OvnClientTestSuite) testDeleteAcls() { require.Len(t, pg.ACLs, 3) /* delete from-lport direction acl */ - err = ovnClient.DeleteAcls(pgName, portGroupKey, ovnnb.ACLDirectionFromLport) + err = ovnClient.DeleteAcls(pgName, portGroupKey, ovnnb.ACLDirectionFromLport, nil) require.NoError(t, err) pg, err = ovnClient.GetPortGroup(pgName, false) @@ -1323,7 +1326,7 @@ func (suite *OvnClientTestSuite) testDeleteAcls() { require.NoError(t, err) require.Len(t, ls.ACLs, 5) - err = ovnClient.DeleteAcls(lsName, logicalSwitchKey, "") + err = ovnClient.DeleteAcls(lsName, logicalSwitchKey, "", nil) require.NoError(t, err) ls, err = ovnClient.GetLogicalSwitch(lsName, false) @@ -1360,7 +1363,7 @@ func (suite *OvnClientTestSuite) testDeleteAcls() { require.Len(t, ls.ACLs, 5) /* delete to-lport direction acl */ - err = ovnClient.DeleteAcls(lsName, logicalSwitchKey, ovnnb.ACLDirectionToLport) + err = ovnClient.DeleteAcls(lsName, logicalSwitchKey, ovnnb.ACLDirectionToLport, nil) require.NoError(t, err) ls, err = ovnClient.GetLogicalSwitch(lsName, false) @@ -1368,7 +1371,44 @@ func (suite *OvnClientTestSuite) testDeleteAcls() { require.Len(t, ls.ACLs, 3) /* delete from-lport direction acl */ - err = ovnClient.DeleteAcls(lsName, logicalSwitchKey, ovnnb.ACLDirectionFromLport) + err = ovnClient.DeleteAcls(lsName, logicalSwitchKey, ovnnb.ACLDirectionFromLport, nil) + require.NoError(t, err) + + ls, err = ovnClient.GetLogicalSwitch(lsName, false) + require.NoError(t, err) + require.Empty(t, ls.ACLs) + }) + + t.Run("delete acls with additional external ids", func(t *testing.T) { + priority := "5801" + basePort := 5801 + acls := make([]*ovnnb.ACL, 0, 5) + + // to-lport + + match := fmt.Sprintf("%s && udp.dst == %d", matchPrefix, basePort) + acl, err := ovnClient.newAcl(lsName, ovnnb.ACLDirectionToLport, priority, match, ovnnb.ACLActionAllowRelated, func(acl *ovnnb.ACL) { + if acl.ExternalIDs == nil { + acl.ExternalIDs = make(map[string]string) + } + acl.ExternalIDs["subnet"] = lsName + }) + require.NoError(t, err) + acls = append(acls, acl) + + err = ovnClient.CreateAcls(lsName, logicalSwitchKey, acls...) + require.NoError(t, err) + + ls, err := ovnClient.GetLogicalSwitch(lsName, false) + require.NoError(t, err) + require.Len(t, ls.ACLs, 1) + + newAcl := &ovnnb.ACL{UUID: ls.ACLs[0]} + err = ovnClient.GetEntityInfo(newAcl) + require.NoError(t, err) + + /* delete to-lport direction acl */ + err = ovnClient.DeleteAcls(lsName, logicalSwitchKey, ovnnb.ACLDirectionToLport, map[string]string{"subnet": lsName}) require.NoError(t, err) ls, err = ovnClient.GetLogicalSwitch(lsName, false) diff --git a/pkg/ovs/ovn-nb-suite_test.go b/pkg/ovs/ovn-nb-suite_test.go index 47ca5bf40699..cd6804668075 100644 --- a/pkg/ovs/ovn-nb-suite_test.go +++ b/pkg/ovs/ovn-nb-suite_test.go @@ -632,8 +632,7 @@ func Test_scratch(t *testing.T) { ovnClient, err := newOvnClient(t, endpoint, 10, "") require.NoError(t, err) - addresses := []string{"10.104.25.159", "10.244.0.78"} - err = ovnClient.AddressSetUpdateAddress("test.as", addresses...) + err = ovnClient.CreateGatewayAcl("ovn-default", "", "10.16.0.1") require.NoError(t, err) } diff --git a/pkg/ovs/ovn-nb.go b/pkg/ovs/ovn-nb.go index 77e6c1758ce7..f2cdf189b6e3 100644 --- a/pkg/ovs/ovn-nb.go +++ b/pkg/ovs/ovn-nb.go @@ -101,7 +101,7 @@ func (c *ovnClient) DeleteSecurityGroup(sgName string) error { pgName := GetSgPortGroupName(sgName) // clear acl - if err := c.DeleteAcls(pgName, portGroupKey, ""); err != nil { + if err := c.DeleteAcls(pgName, portGroupKey, "", nil); err != nil { return fmt.Errorf("delete acls from port group %s: %v", pgName, err) }