diff --git a/pkg/ovs/ovn-nb-acl.go b/pkg/ovs/ovn-nb-acl.go index 226bda4e61a..5f30de3311e 100644 --- a/pkg/ovs/ovn-nb-acl.go +++ b/pkg/ovs/ovn-nb-acl.go @@ -1154,6 +1154,10 @@ func (c *OVNNbClient) CreateAclsOps(parentName, parentType string, acls ...*ovnn // delete to-lport and from-lport direction acl when direction is empty, otherwise one-way // parentType is 'ls' or 'pg' func (c *OVNNbClient) DeleteAclsOps(parentName, parentType, direction string, externalIDs map[string]string) ([]ovsdb.Operation, error) { + if parentName == "" { + return nil, errors.New("the port group name or logical switch name is required") + } + if externalIDs == nil { externalIDs = make(map[string]string) } diff --git a/pkg/ovs/ovn-nb-logical_switch_port_test.go b/pkg/ovs/ovn-nb-logical_switch_port_test.go index 7bd7d0c2151..94c8a92b569 100644 --- a/pkg/ovs/ovn-nb-logical_switch_port_test.go +++ b/pkg/ovs/ovn-nb-logical_switch_port_test.go @@ -1549,6 +1549,7 @@ func (suite *OvnClientTestSuite) testListLogicalSwitchPorts() { t.Parallel() nbClient := suite.ovnNBClient + failedNbClient := suite.faiedOvnNBClient lsName := "test-list-lsp-ls" err := nbClient.CreateBareLogicalSwitch(lsName) @@ -1570,7 +1571,19 @@ func (suite *OvnClientTestSuite) testListLogicalSwitchPorts() { require.Equal(t, lspName, out[0].Name) }) - t.Run("patch lsp", func(t *testing.T) { + t.Run("failed client create patch lsp", func(t *testing.T) { + t.Parallel() + + // patch lsp + lrName := "test-list-patch-lsp-lr" + lspName := "test-list-patch-lsp-lsp" + lrpName := "test-list-patch-lsp-lrp" + + err = failedNbClient.CreateLogicalPatchPort(lsName, lrName, lspName, lrpName, "10.19.100.1/24", "") + require.Error(t, err) + }) + + t.Run("failed client create patch lsp", func(t *testing.T) { t.Parallel() // patch lsp diff --git a/pkg/ovs/ovn-nb-nat_test.go b/pkg/ovs/ovn-nb-nat_test.go index bbaa0e84406..b52d047e2b3 100644 --- a/pkg/ovs/ovn-nb-nat_test.go +++ b/pkg/ovs/ovn-nb-nat_test.go @@ -569,7 +569,7 @@ func (suite *OvnClientTestSuite) testGetNat() { }) t.Run("logical ip is different", func(t *testing.T) { - _, err := nbClient.GetNat(lrName, natType, externalIP, "10.250.0.10", false) + _, err := nbClient.GetNat(lrName, natType, externalIP, "10.250.0.171", false) require.ErrorContains(t, err, "not found") }) diff --git a/pkg/ovs/ovn-nb-port_group.go b/pkg/ovs/ovn-nb-port_group.go index d6bbdb29b6a..32d51b9ae52 100644 --- a/pkg/ovs/ovn-nb-port_group.go +++ b/pkg/ovs/ovn-nb-port_group.go @@ -188,6 +188,9 @@ func (c *OVNNbClient) DeletePortGroup(pgName ...string) error { // GetPortGroup get port group by name func (c *OVNNbClient) GetPortGroup(pgName string, ignoreNotFound bool) (*ovnnb.PortGroup, error) { + if pgName == "" { + return nil, errors.New("port group name is empty") + } ctx, cancel := context.WithTimeout(context.Background(), c.Timeout) defer cancel() diff --git a/pkg/ovs/ovn-nb-suite_test.go b/pkg/ovs/ovn-nb-suite_test.go index 0005fb9c4c3..6181c31be85 100644 --- a/pkg/ovs/ovn-nb-suite_test.go +++ b/pkg/ovs/ovn-nb-suite_test.go @@ -1056,6 +1056,10 @@ func (suite *OvnClientTestSuite) Test_OvsFind() { suite.testOvsFind() } +func (suite *OvnClientTestSuite) Test_ParseOvsFindOutput() { + suite.testParseOvsFindOutput() +} + func (suite *OvnClientTestSuite) Test_OvsClear() { suite.testOvsClear() } diff --git a/pkg/ovs/ovn-nb_global.go b/pkg/ovs/ovn-nb_global.go index 6201c9982be..eba49126a0d 100644 --- a/pkg/ovs/ovn-nb_global.go +++ b/pkg/ovs/ovn-nb_global.go @@ -13,6 +13,12 @@ import ( ) func (c *OVNNbClient) CreateNbGlobal(nbGlobal *ovnnb.NBGlobal) error { + if nbGlobal == nil { + err := errors.New("nb global is nil") + klog.Error(err) + return err + } + op, err := c.ovsDbClient.Create(nbGlobal) if err != nil { klog.Error(err) @@ -61,6 +67,12 @@ func (c *OVNNbClient) GetNbGlobal() (*ovnnb.NBGlobal, error) { } func (c *OVNNbClient) UpdateNbGlobal(nbGlobal *ovnnb.NBGlobal, fields ...interface{}) error { + if nbGlobal == nil { + err := errors.New("nb global is nil") + klog.Error(err) + return err + } + op, err := c.Where(nbGlobal).Update(nbGlobal, fields...) if err != nil { klog.Error(err) diff --git a/pkg/ovs/ovn-nb_global_test.go b/pkg/ovs/ovn-nb_global_test.go index 8f059287ad6..c13b76340e9 100644 --- a/pkg/ovs/ovn-nb_global_test.go +++ b/pkg/ovs/ovn-nb_global_test.go @@ -22,9 +22,13 @@ func (suite *OvnClientTestSuite) testGetNbGlobal() { t := suite.T() nbClient := suite.ovnNBClient + failedNbClient := suite.faiedOvnNBClient t.Cleanup(func() { - err := nbClient.DeleteNbGlobal() + err := failedNbClient.DeleteNbGlobal() + require.Error(t, err) + + err = nbClient.DeleteNbGlobal() require.NoError(t, err) _, err = nbClient.GetNbGlobal() @@ -45,12 +49,18 @@ func (suite *OvnClientTestSuite) testGetNbGlobal() { require.NoError(t, err) require.NotEmpty(t, out.UUID) }) + + t.Run("failed client create nb_global", func(t *testing.T) { + err := failedNbClient.CreateNbGlobal(nil) + require.Error(t, err) + }) } func (suite *OvnClientTestSuite) testUpdateNbGlobal() { t := suite.T() nbClient := suite.ovnNBClient + failedNbClient := suite.faiedOvnNBClient t.Cleanup(func() { err := nbClient.DeleteNbGlobal() @@ -79,6 +89,11 @@ func (suite *OvnClientTestSuite) testUpdateNbGlobal() { require.Equal(t, "16711680", out.Options["max_tunid"]) }) + t.Run("failed client update", func(t *testing.T) { + err = failedNbClient.UpdateNbGlobal(nil) + require.Error(t, err) + }) + t.Run("create options", func(t *testing.T) { nbGlobal := &ovnnb.NBGlobal{ UUID: nbGlobal.UUID, diff --git a/pkg/ovs/ovn-nb_test.go b/pkg/ovs/ovn-nb_test.go index 2ec6a1634d2..7b651ce2476 100644 --- a/pkg/ovs/ovn-nb_test.go +++ b/pkg/ovs/ovn-nb_test.go @@ -15,6 +15,7 @@ func (suite *OvnClientTestSuite) testCreateGatewayLogicalSwitch() { t.Parallel() nbClient := suite.ovnNBClient + failedNbClient := suite.faiedOvnNBClient lsName := "test-create-gw-ls" lrName := "test-create-gw-lr" lspName := fmt.Sprintf("%s-%s", lsName, lrName) @@ -25,6 +26,11 @@ func (suite *OvnClientTestSuite) testCreateGatewayLogicalSwitch() { err := nbClient.CreateLogicalRouter(lrName) require.NoError(t, err) + // create with failed client + err = failedNbClient.CreateGatewayLogicalSwitch(lsName, lrName, "test-external", "192.168.230.1/24,fc00::0af4:01/112", util.GenerateMac(), 210, chassises...) + require.Error(t, err) + + // create with normal client err = nbClient.CreateGatewayLogicalSwitch(lsName, lrName, "test-external", "192.168.230.1/24,fc00::0af4:01/112", util.GenerateMac(), 210, chassises...) require.NoError(t, err) @@ -52,6 +58,8 @@ func (suite *OvnClientTestSuite) testCreateLogicalPatchPort() { t.Parallel() nbClient := suite.ovnNBClient + failedNbClient := suite.faiedOvnNBClient + lsName := "test-create-router-ls" lrName := "test-create-router-lr" lspName := fmt.Sprintf("%s-%s", lsName, lrName) @@ -101,6 +109,33 @@ func (suite *OvnClientTestSuite) testCreateLogicalPatchPort() { require.NoError(t, err) require.Empty(t, lrp.GatewayChassis) }) + + t.Run("failed client to create router port with chassises", func(t *testing.T) { + t.Parallel() + // failed to create with failed client + err := failedNbClient.CreateLogicalPatchPort(lsName, lrName, lspName, lrpName, "192.168.230.1/24,fc00::0af4:01/112", util.GenerateMac(), chassises...) + require.Error(t, err) + + // failed to create with invalid cidr + err = failedNbClient.CreateLogicalPatchPort(lsName, lrName, lspName, lrpName, "192.168.230.1/33,fc00::0af4:01/129", util.GenerateMac(), chassises...) + require.Error(t, err) + + err = failedNbClient.CreateLogicalPatchPort(lsName, lrName, lspName, lrpName, "0.0.0.0/0,fc00::0af4:01/112", util.GenerateMac(), chassises...) + require.Error(t, err) + + err = failedNbClient.CreateLogicalPatchPort(lsName, lrName, lspName, lrpName, "192.168.230.1/24,00::00:00/0", util.GenerateMac(), chassises...) + require.Error(t, err) + + err = failedNbClient.CreateLogicalPatchPort(lsName, lrName, lspName, lrpName, "192.168.230.1/31,fc00::0af4:01/127", util.GenerateMac(), chassises...) + require.Error(t, err) + + err = failedNbClient.CreateLogicalPatchPort(lsName, lrName, lspName, lrpName, "192.168.230.1/32fc00::0af4:01/128", util.GenerateMac(), chassises...) + require.Error(t, err) + + lrp, err := nbClient.GetLogicalRouterPort(lrpName, false) + require.Error(t, err) + require.Nil(t, lrp) + }) } func (suite *OvnClientTestSuite) testRemoveRouterPort() { @@ -108,6 +143,8 @@ func (suite *OvnClientTestSuite) testRemoveRouterPort() { t.Parallel() nbClient := suite.ovnNBClient + failedNbClient := suite.faiedOvnNBClient + lsName := "test-remove-router-type-ls" lrName := "test-remove-router-type-lr" lspName := fmt.Sprintf("%s-%s", lsName, lrName) @@ -131,6 +168,15 @@ func (suite *OvnClientTestSuite) testRemoveRouterPort() { err = nbClient.RemoveLogicalPatchPort(lspName, lrpName) require.NoError(t, err) }) + + t.Run("failed client del router type port", func(t *testing.T) { + err = failedNbClient.RemoveLogicalPatchPort(lspName, lrpName) + require.Nil(t, err) + err = failedNbClient.RemoveLogicalPatchPort("", lrpName) + require.Error(t, err) + err = failedNbClient.RemoveLogicalPatchPort(lspName, "") + require.Nil(t, err) + }) } func (suite *OvnClientTestSuite) testDeleteLogicalGatewaySwitch() { @@ -138,6 +184,8 @@ func (suite *OvnClientTestSuite) testDeleteLogicalGatewaySwitch() { t.Parallel() nbClient := suite.ovnNBClient + failedNbClient := suite.faiedOvnNBClient + lsName := "test-del-gw-ls" lrName := "test-del-gw-lr" @@ -147,6 +195,15 @@ func (suite *OvnClientTestSuite) testDeleteLogicalGatewaySwitch() { err = nbClient.CreateGatewayLogicalSwitch(lsName, lrName, "test-external", "192.168.230.1/24,fc00::0af4:01/112", util.GenerateMac(), 210) require.NoError(t, err) + err = failedNbClient.DeleteLogicalGatewaySwitch("", lrName) + require.Error(t, err) + + err = failedNbClient.DeleteLogicalGatewaySwitch(lsName, "") + require.Nil(t, err) + + err = failedNbClient.DeleteLogicalGatewaySwitch(lsName, lrName) + require.Nil(t, err) + // localnet port and lsp will be deleted when delete logical switch in real ovsdb, // it's different from the mock memory ovsdb, // so no need to check localnet port and lsp existence @@ -162,14 +219,27 @@ func (suite *OvnClientTestSuite) testDeleteSecurityGroup() { t.Parallel() nbClient := suite.ovnNBClient + failedNbClient := suite.faiedOvnNBClient sgName := "test_del_sg" asName := "test_del_sg_as" pgName := GetSgPortGroupName(sgName) priority := "5111" match := "outport == @ovn.sg.test_del_sg && ip" - /* prepate test */ - err := nbClient.CreatePortGroup(pgName, map[string]string{ + // create with empty pg + err := nbClient.CreatePortGroup("", map[string]string{ + "type": "security_group", + sgKey: sgName, + }) + require.Error(t, err) + // create with failed client + err = failedNbClient.CreatePortGroup(pgName, map[string]string{ + "type": "security_group", + sgKey: sgName, + }) + require.Error(t, err) + // create with normal pg + err = nbClient.CreatePortGroup(pgName, map[string]string{ "type": "security_group", sgKey: sgName, }) @@ -186,7 +256,14 @@ func (suite *OvnClientTestSuite) testDeleteSecurityGroup() { }) require.NoError(t, err) - /* run test */ + // failed client delete sg + err = failedNbClient.DeleteSecurityGroup("") + require.Error(t, err) + + err = failedNbClient.DeleteSecurityGroup(sgName) + require.Nil(t, err) + + // normal delete sg err = nbClient.DeleteSecurityGroup(sgName) require.NoError(t, err) diff --git a/pkg/ovs/ovn-sb-chassis_test.go b/pkg/ovs/ovn-sb-chassis_test.go index cb67a606cfc..013726253d9 100644 --- a/pkg/ovs/ovn-sb-chassis_test.go +++ b/pkg/ovs/ovn-sb-chassis_test.go @@ -491,7 +491,14 @@ func (suite *OvnClientTestSuite) testGetKubeOvnChassisses() { require.NoError(t, err) chassisList, err := sbClient.GetKubeOvnChassisses() require.NoError(t, err) - require.Empty(t, *chassisList) + names := make(map[string]bool) + for _, chassis := range *chassisList { + names[chassis.Name] = true + } + require.False(t, names["kube-ovn-chassis-1"]) + require.False(t, names["kube-ovn-chassis-2"]) + require.False(t, names["non-kube-ovn-chassis"]) + require.False(t, names["mixed-chassis"]) }) kubeOvnChassis1 := newChassis(0, "host-1", "kube-ovn-chassis-1", nil, nil, nil, map[string]string{"vendor": util.CniTypeName}, nil) diff --git a/pkg/ovs/ovs-vsctl.go b/pkg/ovs/ovs-vsctl.go index 11e341f6923..1af9ec8f801 100644 --- a/pkg/ovs/ovs-vsctl.go +++ b/pkg/ovs/ovs-vsctl.go @@ -112,6 +112,11 @@ func ovsFind(table, column string, conditions ...string) ([]string, error) { klog.Error(err) return nil, err } + ret := parseOvsFindOutput(output) + return ret, nil +} + +func parseOvsFindOutput(output string) []string { values := strings.Split(output, "\n\n") // We want "bare" values for strings, but we can't pass --bare to ovs-vsctl because // it breaks more complicated types. So try passing each value through Unquote(); @@ -127,7 +132,7 @@ func ovsFind(table, column string, conditions ...string) ([]string, error) { ret = append(ret, strings.Trim(strings.TrimSpace(val), "\"")) } } - return ret, nil + return ret } func ovsClear(table, record string, columns ...string) error { diff --git a/pkg/ovs/ovs-vsctl_test.go b/pkg/ovs/ovs-vsctl_test.go index bbc8ecd5c97..a396517ad1f 100644 --- a/pkg/ovs/ovs-vsctl_test.go +++ b/pkg/ovs/ovs-vsctl_test.go @@ -75,6 +75,17 @@ func (suite *OvnClientTestSuite) testOvsFind() { require.Empty(t, ret) } +func (suite *OvnClientTestSuite) testParseOvsFindOutput() { + t := suite.T() + t.Parallel() + input := `br-int + +br-businessnet +` + ret := parseOvsFindOutput(input) + require.Len(t, ret, 2) +} + func (suite *OvnClientTestSuite) testOvsClear() { t := suite.T() t.Parallel() diff --git a/pkg/ovs/util.go b/pkg/ovs/util.go index a00410d792d..84ceccecab2 100644 --- a/pkg/ovs/util.go +++ b/pkg/ovs/util.go @@ -43,6 +43,9 @@ func LogicalSwitchPortName(lr, ls string) string { } func GetSgPortGroupName(sgName string) string { + if sgName == "" { + return "" + } return strings.ReplaceAll(fmt.Sprintf("ovn.sg.%s", sgName), "-", ".") } diff --git a/pkg/util/net_test.go b/pkg/util/net_test.go index aa0cccd08c0..d4662912aed 100644 --- a/pkg/util/net_test.go +++ b/pkg/util/net_test.go @@ -1422,3 +1422,52 @@ func TestInvalidCIDR(t *testing.T) { err = InvalidSpecialCIDR(invalidCIDR) require.Error(t, err) } + +func TestInvalidNetworkMask(t *testing.T) { + validNet := net.IPNet{ + IP: net.ParseIP("10.10.10.0"), + Mask: net.CIDRMask(0, 32), + } + err := InvalidNetworkMask(&validNet) + require.Nil(t, err) + + validNet = net.IPNet{ + IP: net.ParseIP("10.10.10.0"), + Mask: net.CIDRMask(0, 33), + } + err = InvalidNetworkMask(&validNet) + require.Error(t, err) + + validNet = net.IPNet{ + IP: net.ParseIP("0.0.0.0"), + Mask: net.CIDRMask(0, 0), + } + err = InvalidNetworkMask(&validNet) + require.Error(t, err) + + // Test for IPv6 + validNet = net.IPNet{ + IP: net.ParseIP("2001:db8::1"), + Mask: net.CIDRMask(0, 128), + } + err = InvalidNetworkMask(&validNet) + require.Nil(t, err) + validNet = net.IPNet{ + IP: net.ParseIP("2001:db8::1"), + Mask: net.CIDRMask(0, 129), + } + err = InvalidNetworkMask(&validNet) + require.Error(t, err) + validNet = net.IPNet{ + IP: net.ParseIP("2001:db8::1"), + Mask: net.CIDRMask(0, 0), + } + err = InvalidNetworkMask(&validNet) + require.Error(t, err) + validNet = net.IPNet{ + IP: net.ParseIP("0:0::0"), + Mask: net.CIDRMask(0, 0), + } + err = InvalidNetworkMask(&validNet) + require.Error(t, err) +}