diff --git a/go-controller/hybrid-overlay/pkg/controller/master.go b/go-controller/hybrid-overlay/pkg/controller/master.go index fed37d9395b..80e8e460ec5 100644 --- a/go-controller/hybrid-overlay/pkg/controller/master.go +++ b/go-controller/hybrid-overlay/pkg/controller/master.go @@ -236,7 +236,7 @@ func (m *MasterController) handleOverlayPort(node *kapi.Node, annotator kube.Ann ", stderr:%s: %v", node.Name, stderr, err) } for _, subnet := range subnets { - if err := util.UpdateNodeSwitchExcludeIPs(node.Name, subnet); err != nil { + if err := util.UpdateNodeSwitchExcludeIPs(m.nbClient, node.Name, subnet); err != nil { return err } } diff --git a/go-controller/hybrid-overlay/pkg/controller/master_test.go b/go-controller/hybrid-overlay/pkg/controller/master_test.go index aed9b85fa3f..4f24fcdaa69 100644 --- a/go-controller/hybrid-overlay/pkg/controller/master_test.go +++ b/go-controller/hybrid-overlay/pkg/controller/master_test.go @@ -185,9 +185,23 @@ var _ = Describe("Hybrid SDN Master Operations", func() { Name: types.OVNClusterRouter, Policies: []string{"reroute-policy-UUID"}, }, + &nbdb.LogicalSwitchPort{ + Name: types.HybridOverlayPrefix + nodeName, + UUID: types.HybridOverlayPrefix + nodeName + "-UUID", + }, + } + + // Pre-add the HO port until the ovn-nbctl lsp-add commands are converted to libovsdb + nodeSwitch := &nbdb.LogicalSwitch{ + Name: nodeName, + UUID: nodeName + "-UUID", + Ports: []string{types.HybridOverlayPrefix + nodeName + "-UUID"}, } + + initialExpectedDB := append(expectedDatabaseState, nodeSwitch) + dbSetup := libovsdbtest.TestSetup{ - NBData: expectedDatabaseState, + NBData: initialExpectedDB, } libovsdbOvnNBClient, err := libovsdbtest.NewNBTestHarness(dbSetup, stopChan) gomega.Expect(err).NotTo(gomega.HaveOccurred()) @@ -204,9 +218,19 @@ var _ = Describe("Hybrid SDN Master Operations", func() { Expect(err).NotTo(HaveOccurred()) // #1 node add - addLinuxNodeCommands(fexec, nodeHOMAC, nodeName, nodeHOIP) + fexec.AddFakeCmdsNoOutputNoError([]string{ + // Setting the mac on the lsp + "ovn-nbctl --timeout=15 -- " + + "--may-exist lsp-add node1 int-node1 -- " + + "lsp-set-addresses int-node1 " + nodeHOMAC, + }) // #2 comes because we set the ho dr gw mac annotation in #1 - addLinuxNodeCommands(fexec, nodeHOMAC, nodeName, nodeHOIP) + fexec.AddFakeCmdsNoOutputNoError([]string{ + // Setting the mac on the lsp + "ovn-nbctl --timeout=15 -- " + + "--may-exist lsp-add node1 int-node1 -- " + + "lsp-set-addresses int-node1 " + nodeHOMAC, + }) f.Start(stopChan) wg.Add(1) @@ -224,6 +248,10 @@ var _ = Describe("Hybrid SDN Master Operations", func() { return updatedNode.Annotations, nil }, 2).Should(HaveKeyWithValue(hotypes.HybridOverlayDRMAC, nodeHOMAC)) + nodeSwitch.OtherConfig = map[string]string{"exclude_ips": "10.1.2.2"} + + expectedDatabaseState = append(expectedDatabaseState, nodeSwitch) + Eventually(fexec.CalledMatchesExpected, 2).Should(BeTrue(), fexec.ErrorDesc) Eventually(libovsdbOvnNBClient).Should(libovsdbtest.HaveDataIgnoringUUIDs(expectedDatabaseState)) @@ -240,6 +268,12 @@ var _ = Describe("Hybrid SDN Master Operations", func() { &nbdb.LogicalRouter{ Name: types.OVNClusterRouter, }, + // This will be deleted once the nbctl commands for lsps are converted + &nbdb.LogicalSwitchPort{ + Name: types.HybridOverlayPrefix + nodeName, + UUID: types.HybridOverlayPrefix + nodeName + "-uuid", + }, + nodeSwitch, } Eventually(libovsdbOvnNBClient).Should(libovsdbtest.HaveDataIgnoringUUIDs(expectedDatabaseState)) return nil @@ -381,6 +415,10 @@ var _ = Describe("Hybrid SDN Master Operations", func() { Addresses: []string{nodeHOMAC, nodeHOIP}, DynamicAddresses: &dynAdd, }, + &nbdb.LogicalSwitch{ + Name: nodeName, + UUID: nodeName + "-UUID", + }, } dbSetup := libovsdbtest.TestSetup{ NBData: expectedDatabaseState, @@ -443,20 +481,3 @@ var _ = Describe("Hybrid SDN Master Operations", func() { Expect(err).NotTo(HaveOccurred()) }) }) - -func addLinuxNodeCommands(fexec *ovntest.FakeExec, nodeHOMAC, nodeName, nodeHOIP string) { - fexec.AddFakeCmdsNoOutputNoError([]string{ - // Setting the mac on the lsp - "ovn-nbctl --timeout=15 -- " + - "--may-exist lsp-add node1 int-node1 -- " + - "lsp-set-addresses int-node1 " + nodeHOMAC, - }) - - fexec.AddFakeCmd(&ovntest.ExpectedCmd{ - Cmd: "ovn-nbctl --timeout=15 lsp-list " + nodeName, - Output: "29df5ce5-2802-4ee5-891f-4fb27ca776e9 (" + types.K8sPrefix + nodeName + ")", - }) - fexec.AddFakeCmdsNoOutputNoError([]string{ - "ovn-nbctl --timeout=15 -- --if-exists set logical_switch " + nodeName + " other-config:exclude_ips=" + nodeHOIP, - }) -} diff --git a/go-controller/pkg/libovsdb/libovsdb.go b/go-controller/pkg/libovsdb/libovsdb.go index b758d5d9be0..0e105de6731 100644 --- a/go-controller/pkg/libovsdb/libovsdb.go +++ b/go-controller/pkg/libovsdb/libovsdb.go @@ -8,7 +8,6 @@ import ( "io/ioutil" "reflect" "strings" - "time" "github.com/cenkalti/backoff/v4" "github.com/ovn-org/libovsdb/client" @@ -16,22 +15,19 @@ import ( "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config" "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/nbdb" "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/sbdb" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types" "gopkg.in/fsnotify/fsnotify.v1" "k8s.io/klog/v2" "k8s.io/klog/v2/klogr" ) -const ( - OVSDBTimeout = 10 * time.Second -) - // newClient creates a new client object given the provided config // the stopCh is required to ensure the goroutine for ssl cert // update is not leaked func newClient(cfg config.OvnAuthConfig, dbModel *model.ClientDBModel, stopCh <-chan struct{}) (client.Client, error) { logger := klogr.New() options := []client.Option{ - client.WithReconnect(OVSDBTimeout, &backoff.ZeroBackOff{}), + client.WithReconnect(types.OVSDBTimeout, &backoff.ZeroBackOff{}), client.WithLeaderOnly(true), client.WithLogger(&logger), } @@ -56,7 +52,7 @@ func newClient(cfg config.OvnAuthConfig, dbModel *model.ClientDBModel, stopCh <- return nil, err } - ctx, cancel := context.WithTimeout(context.Background(), OVSDBTimeout) + ctx, cancel := context.WithTimeout(context.Background(), types.OVSDBTimeout) defer cancel() err = client.Connect(ctx) if err != nil { diff --git a/go-controller/pkg/ovn/acl/acl.go b/go-controller/pkg/ovn/acl/acl.go deleted file mode 100644 index 58e45c22746..00000000000 --- a/go-controller/pkg/ovn/acl/acl.go +++ /dev/null @@ -1,58 +0,0 @@ -package acl - -import ( - "fmt" - "strings" - - libovsdbclient "github.com/ovn-org/libovsdb/client" - "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/ovn/libovsdbops" - "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types" - "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util" - - "github.com/pkg/errors" - - "k8s.io/klog/v2" -) - -// RemoveACLFromNodeSwitches removes the ACL uuid entry from Logical Switch acl's list. -func RemoveACLFromNodeSwitches(switches []string, aclUUID string) error { - if len(switches) == 0 { - return nil - } - args := []string{} - for _, ls := range switches { - args = append(args, "--", "--if-exists", "remove", "logical_switch", ls, "acl", aclUUID) - } - _, _, err := util.RunOVNNbctl(args...) - if err != nil { - return errors.Wrapf(err, "Error while removing ACL: %s, from switches", aclUUID) - } - klog.Infof("ACL: %s, removed from switches: %s", aclUUID, switches) - return nil -} - -func PurgeRejectRules(nbClient libovsdbclient.Client) error { - acls, err := libovsdbops.FindRejectACLs(nbClient) - if err != nil { - return errors.Wrap(err, "Error while finding rejct ACLs") - } - - for _, acl := range acls { - data, stderr, err := util.RunOVNNbctl("--format=csv", "--data=bare", "--no-headings", "--columns=_uuid", "find", "logical_switch", fmt.Sprintf("acls{>=}%s", acl.UUID)) - if err != nil { - return errors.Wrapf(err, "Error while querying ACLs uuid:%s with reject action: %s", acl.UUID, stderr) - } - ls := strings.Split(data, "\n") - err = RemoveACLFromNodeSwitches(ls, acl.UUID) - if err != nil { - return errors.Wrapf(err, "Failed to remove reject acl from logical switches") - } - } - - err = libovsdbops.DeleteACLsFromPortGroup(nbClient, types.ClusterPortGroupName, acls...) - if err != nil { - klog.Errorf("Error trying to remove ACLs %+v from port group %s: %v", acls, types.ClusterPortGroupName, err) - } - - return nil -} diff --git a/go-controller/pkg/ovn/acl/acl_test.go b/go-controller/pkg/ovn/acl/acl_test.go deleted file mode 100644 index 2f2ea9b800a..00000000000 --- a/go-controller/pkg/ovn/acl/acl_test.go +++ /dev/null @@ -1,64 +0,0 @@ -package acl - -import ( - "fmt" - "testing" - - ovntest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing" - "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util" -) - -func TestRemoveACLFromNodeSwitches(t *testing.T) { - tests := []struct { - name string - switches []string - aclUUID string - ovnCmd ovntest.ExpectedCmd - wantErr bool - }{ - { - name: "remove acl on two switches", - switches: []string{"sw1", "sw2"}, - aclUUID: "a08ea426-2288-11eb-a30b-a8a1590cda29", - ovnCmd: ovntest.ExpectedCmd{ - Cmd: "ovn-nbctl --timeout=15 -- --if-exists remove logical_switch sw1 acl a08ea426-2288-11eb-a30b-a8a1590cda29 -- --if-exists remove logical_switch sw2 acl a08ea426-2288-11eb-a30b-a8a1590cda29", - Output: "", - }, - wantErr: false, - }, - { - name: "remove acl on no switches", - switches: []string{}, - aclUUID: "a08ea426-2288-11eb-a30b-a8a1590cda29", - ovnCmd: ovntest.ExpectedCmd{}, - wantErr: false, - }, - { - name: "remove acl and OVN error on first switch", - switches: []string{"sw1", "sw2"}, - aclUUID: "a08ea426-2288-11eb-a30b-a8a1590cda29", - ovnCmd: ovntest.ExpectedCmd{ - Cmd: "ovn-nbctl --timeout=15 -- --if-exists remove logical_switch sw1 acl a08ea426-2288-11eb-a30b-a8a1590cda29 -- --if-exists remove logical_switch sw2 acl a08ea426-2288-11eb-a30b-a8a1590cda29", - Output: "", - Err: fmt.Errorf("error while removing ACL: sw1, from switches"), - }, - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - fexec := ovntest.NewLooseCompareFakeExec() - fexec.AddFakeCmd(&tt.ovnCmd) - err := util.SetExec(fexec) - if err != nil { - t.Errorf("fexec error: %v", err) - } - - err = RemoveACLFromNodeSwitches(tt.switches, tt.aclUUID) - if (err != nil) != tt.wantErr { - t.Errorf("RemoveACLFromNodeSwitches() error = %v, wantErr %v", err, tt.wantErr) - return - } - }) - } -} diff --git a/go-controller/pkg/ovn/controller/services/repair.go b/go-controller/pkg/ovn/controller/services/repair.go index 3e72cf481f3..a733e618a08 100644 --- a/go-controller/pkg/ovn/controller/services/repair.go +++ b/go-controller/pkg/ovn/controller/services/repair.go @@ -8,7 +8,7 @@ import ( libovsdbclient "github.com/ovn-org/libovsdb/client" globalconfig "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config" - "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/ovn/acl" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/ovn/libovsdbops" ovnlb "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/ovn/loadbalancer" "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util" @@ -115,7 +115,12 @@ func (r *repair) runBeforeSync() { // Remove existing reject rules. They are not used anymore // given the introduction of idling loadbalancers - err = acl.PurgeRejectRules(r.nbClient) + acls, err := libovsdbops.FindRejectACLs(r.nbClient) + if err != nil { + klog.Errorf("Error while finding rejct ACLs error: %v", err) + } + + err = libovsdbops.RemoveACLsFromAllSwitches(r.nbClient, acls) if err != nil { klog.Errorf("Failed to purge existing reject rules: %v", err) } diff --git a/go-controller/pkg/ovn/controller/services/services_controller.go b/go-controller/pkg/ovn/controller/services/services_controller.go index a213500fbff..26bdb674c61 100644 --- a/go-controller/pkg/ovn/controller/services/services_controller.go +++ b/go-controller/pkg/ovn/controller/services/services_controller.go @@ -107,8 +107,7 @@ type Controller struct { client clientset.Interface // libovsdb northbound client interface - nbClient libovsdbclient.Client - + nbClient libovsdbclient.Client eventBroadcaster record.EventBroadcaster eventRecorder record.EventRecorder diff --git a/go-controller/pkg/ovn/egressfirewall.go b/go-controller/pkg/ovn/egressfirewall.go index a9291776229..744d2ce34c6 100644 --- a/go-controller/pkg/ovn/egressfirewall.go +++ b/go-controller/pkg/ovn/egressfirewall.go @@ -3,8 +3,6 @@ package ovn import ( "fmt" "net" - "sort" - "strconv" "strings" "sync" @@ -16,7 +14,6 @@ import ( addressset "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/ovn/address_set" "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/ovn/libovsdbops" "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types" - "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util" kapi "k8s.io/api/core/v1" "k8s.io/client-go/util/retry" @@ -96,100 +93,51 @@ func newEgressFirewallRule(rawEgressFirewallRule egressfirewallapi.EgressFirewal // NOTE: Utilize the fact that we know that all egress firewall related setup must have a priority: types.MinimumReservedEgressFirewallPriority <= priority <= types.EgressFirewallStartPriority func (oc *Controller) syncEgressFirewall(egressFirwalls []interface{}) { + // Lookup all ACLs used for egress Firewalls + egressFirewallACLs, err := libovsdbops.FindACLsByPriorityRange(oc.nbClient, types.EgressFirewallStartPriority, types.MinimumReservedEgressFirewallPriority) + if err != nil { + klog.Errorf("Unable to list egress firewall ACLs, cannot cleanup old stale data, err: %v", err) + return + } + if config.Gateway.Mode == config.GatewayModeShared { - // Mode is shared gateway mode, make sure to delete all ACLs on the node switches - egressFirewallACLIDs, stderr, err := util.RunOVNNbctl( - "--data=bare", - "--no-heading", - "--columns=_uuid", - "--format=table", - "find", - "acl", - fmt.Sprintf("priority<=%s", types.EgressFirewallStartPriority), - fmt.Sprintf("priority>=%s", types.MinimumReservedEgressFirewallPriority), - ) - if err != nil { - klog.Errorf("Unable to list egress firewall logical router policies, cannot cleanup old stale data, stderr: %s, err: %v", stderr, err) - return - } - if egressFirewallACLIDs != "" { - nodes, err := oc.watchFactory.GetNodes() + // Mode is shared gateway mode, make sure to delete all egfw ACLs on the node switches + if len(egressFirewallACLs) != 0 { + err = libovsdbops.RemoveACLsFromNodeSwitches(oc.nbClient, egressFirewallACLs) if err != nil { - klog.Errorf("Unable to cleanup egress firewall ACLs remaining from local gateway mode, cannot list nodes, err: %v", err) + klog.Errorf("Failed to remove reject acl from node logical switches: %v", err) return } - logicalSwitches := []string{} - for _, node := range nodes { - logicalSwitches = append(logicalSwitches, node.Name) - } - for _, logicalSwitch := range logicalSwitches { - switchACLs, stderr, err := util.RunOVNNbctl( - "--data=bare", - "--no-heading", - "--columns=acls", - "--format=table", - "list", - "logical_switch", - logicalSwitch, - ) - if err != nil { - klog.Errorf("Unable to remove egress firewall acl, cannot list ACLs on switch: %s, stderr: %s, err: %v", logicalSwitch, stderr, err) - } - for _, egressFirewallACLID := range strings.Fields(egressFirewallACLIDs) { - if strings.Contains(switchACLs, egressFirewallACLID) { - _, stderr, err := util.RunOVNNbctl( - "remove", - "logical_switch", - logicalSwitch, - "acls", - egressFirewallACLID, - ) - if err != nil { - klog.Errorf("Unable to remove egress firewall acl: %s on %s, cannot cleanup old stale data, stderr: %s, err: %v", egressFirewallACLID, logicalSwitch, stderr, err) - } - } - } - } } } - egressFirewallACLIDs, stderr, err := util.RunOVNNbctl( - "--data=bare", - "--no-heading", - "--columns=_uuid", - "--format=table", - "find", - "acl", - fmt.Sprintf("priority<=%s", types.EgressFirewallStartPriority), - fmt.Sprintf("priority>=%s", types.MinimumReservedEgressFirewallPriority), - fmt.Sprintf("direction=%s", types.DirectionFromLPort), - ) - if err != nil { - klog.Errorf("Unable to list egress firewall logical router policies, cannot convert old ACL data, stderr: %s, err: %v", stderr, err) - return - } - if egressFirewallACLIDs != "" { - for _, egressFirewallACLID := range strings.Fields(egressFirewallACLIDs) { - _, stderr, err := util.RunOVNNbctl( - "set", - "acl", - egressFirewallACLID, - fmt.Sprintf("direction=%s", types.DirectionToLPort), - ) - if err != nil { - klog.Errorf("Unable to set ACL direction on egress firewall acl: %s, cannot convert old ACL data, stderr: %s, err: %v", egressFirewallACLID, stderr, err) - } + // update the direction of each egressFirewallACL if needed + if len(egressFirewallACLs) != 0 { + opModels := []libovsdbops.OperationModel{} + for i, egressFirewallACL := range egressFirewallACLs { + aclName := egressFirewallACLs[i].Name + egressFirewallACL.Direction = types.DirectionToLPort + opModels = append(opModels, libovsdbops.OperationModel{ + Model: &egressFirewallACL, + ModelPredicate: func(acl *nbdb.ACL) bool { return acl.Name == aclName }, + OnModelUpdates: []interface{}{ + &egressFirewallACL.Direction, + }, + ErrNotFound: true, + BulkOp: true, + }) + } + if _, err := oc.modelClient.CreateOrUpdate(opModels...); err != nil { + klog.Errorf("Unable to set ACL direction on egress firewall acls, cannot convert old ACL data err: %v", err) } } // In any gateway mode, make sure to delete all LRPs on ovn_cluster_router. // This covers old local GW mode -> shared GW and old local GW mode -> new local GW mode - intStartPriority, _ := strconv.Atoi(types.EgressFirewallStartPriority) - intEndPriority, _ := strconv.Atoi(types.MinimumReservedEgressFirewallPriority) logicalRouter := nbdb.LogicalRouter{} logicalRouterPolicyRes := []nbdb.LogicalRouterPolicy{} opModels := []libovsdbops.OperationModel{ { ModelPredicate: func(lrp *nbdb.LogicalRouterPolicy) bool { - return lrp.Priority <= intStartPriority && lrp.Priority >= intEndPriority + return lrp.Priority <= types.EgressFirewallStartPriority && lrp.Priority >= types.MinimumReservedEgressFirewallPriority }, ExistingResult: &logicalRouterPolicyRes, DoAfter: func() { @@ -210,30 +158,15 @@ func (oc *Controller) syncEgressFirewall(egressFirwalls []interface{}) { } // sync the ovn and k8s egressFirewall states - ovnEgressFirewallExternalIDs, stderr, err := util.RunOVNNbctl( - "--data=bare", - "--no-heading", - "--columns=external_id", - "--format=table", - "find", - "acl", - fmt.Sprintf("priority<=%s", types.EgressFirewallStartPriority), - fmt.Sprintf("priority>=%s", types.MinimumReservedEgressFirewallPriority), - ) - if err != nil { - klog.Errorf("Cannot reconcile the state of egressfirewalls in ovn database and k8s. stderr: %s, err: %v", stderr, err) - } - splitOVNEgressFirewallExternalIDs := strings.Fields(ovnEgressFirewallExternalIDs) - // represents the namespaces that have firewalls according to ovn ovnEgressFirewalls := make(map[string]struct{}) - for _, externalID := range splitOVNEgressFirewallExternalIDs { - if strings.Contains(externalID, "egressFirewall=") { + for _, egressFirewallACL := range egressFirewallACLs { + if ns, ok := egressFirewallACL.ExternalIDs["egressFirewall"]; ok { // Most egressFirewalls will have more then one ACL but we only need to know if there is one for the namespace // so a map is fine and we will add an entry every iteration but because it is a map will overwrite the previous // entry if it already existed - ovnEgressFirewalls[strings.Split(externalID, "egressFirewall=")[1]] = struct{}{} + ovnEgressFirewalls[ns] = struct{}{} } } @@ -243,26 +176,20 @@ func (oc *Controller) syncEgressFirewall(egressFirwalls []interface{}) { klog.Errorf("Cannot reconcile the state of egressfirewalls in ovn database and k8s. err: %v", err) } // delete entries from the map that exist in k8s and ovn - txn := util.NewNBTxn() for _, egressFirewall := range egressFirewallList.Items { delete(ovnEgressFirewalls, egressFirewall.Namespace) } // any that are left are spurious and should be cleaned up for spuriousEF := range ovnEgressFirewalls { - err := oc.deleteEgressFirewallRules(spuriousEF, txn) + err := oc.deleteEgressFirewallRules(spuriousEF) if err != nil { klog.Errorf("Cannot fully reconcile the state of egressfirewalls ACLs for namespace %s still exist in ovn db: %v", spuriousEF, err) return } - _, stderr, err := txn.Commit() - if err != nil { - klog.Errorf("Cannot fully reconcile the state of egressfirewalls ACLs that still exist in ovn db: stderr: %q, err: %+v", stderr, err) - } - } } -func (oc *Controller) addEgressFirewall(egressFirewall *egressfirewallapi.EgressFirewall, txn *util.NBTxn) error { +func (oc *Controller) addEgressFirewall(egressFirewall *egressfirewallapi.EgressFirewall) error { klog.Infof("Adding egressFirewall %s in namespace %s", egressFirewall.Name, egressFirewall.Namespace) ef := cloneEgressFirewall(egressFirewall) @@ -275,17 +202,9 @@ func (oc *Controller) addEgressFirewall(egressFirewall *egressfirewallapi.Egress } var addErrors error - egressFirewallStartPriorityInt, err := strconv.Atoi(types.EgressFirewallStartPriority) - if err != nil { - return fmt.Errorf("failed to convert egressFirewallStartPriority to Integer: cannot add egressFirewall for namespace %s", egressFirewall.Namespace) - } - minimumReservedEgressFirewallPriorityInt, err := strconv.Atoi(types.MinimumReservedEgressFirewallPriority) - if err != nil { - return fmt.Errorf("failed to convert minumumReservedEgressFirewallPriority to Integer: cannot add egressFirewall for namespace %s", egressFirewall.Namespace) - } for i, egressFirewallRule := range egressFirewall.Spec.Egress { // process Rules into egressFirewallRules for egressFirewall struct - if i > egressFirewallStartPriorityInt-minimumReservedEgressFirewallPriorityInt { + if i > types.EgressFirewallStartPriority-types.MinimumReservedEgressFirewallPriority { klog.Warningf("egressFirewall for namespace %s has too many rules, the rest will be ignored", egressFirewall.Namespace) break @@ -306,12 +225,12 @@ func (oc *Controller) addEgressFirewall(egressFirewall *egressfirewallapi.Egress // EgressFirewall needs to make sure that the address_set for the namespace exists independently of the namespace object // so that OVN doesn't get unresolved references to the address_set. // TODO: This should go away once we do something like refcounting for address_sets. - _, err = oc.addressSetFactory.EnsureAddressSet(egressFirewall.Namespace) + _, err := oc.addressSetFactory.EnsureAddressSet(egressFirewall.Namespace) if err != nil { return fmt.Errorf("cannot Ensure that addressSet for namespace %s exists %v", egressFirewall.Namespace, err) } ipv4HashedAS, ipv6HashedAS := addressset.MakeAddressSetHashNames(egressFirewall.Namespace) - err = oc.addEgressFirewallRules(ef, ipv4HashedAS, ipv6HashedAS, egressFirewallStartPriorityInt, txn) + err = oc.addEgressFirewallRules(ef, ipv4HashedAS, ipv6HashedAS, types.EgressFirewallStartPriority) if err != nil { return err } @@ -319,16 +238,16 @@ func (oc *Controller) addEgressFirewall(egressFirewall *egressfirewallapi.Egress return nil } -func (oc *Controller) updateEgressFirewall(oldEgressFirewall, newEgressFirewall *egressfirewallapi.EgressFirewall, txn *util.NBTxn) error { - updateErrors := oc.deleteEgressFirewall(oldEgressFirewall, txn) +func (oc *Controller) updateEgressFirewall(oldEgressFirewall, newEgressFirewall *egressfirewallapi.EgressFirewall) error { + updateErrors := oc.deleteEgressFirewall(oldEgressFirewall) if updateErrors != nil { return updateErrors } - updateErrors = oc.addEgressFirewall(newEgressFirewall, txn) + updateErrors = oc.addEgressFirewall(newEgressFirewall) return updateErrors } -func (oc *Controller) deleteEgressFirewall(egressFirewallObj *egressfirewallapi.EgressFirewall, txn *util.NBTxn) error { +func (oc *Controller) deleteEgressFirewall(egressFirewallObj *egressfirewallapi.EgressFirewall) error { klog.Infof("Deleting egress Firewall %s in namespace %s", egressFirewallObj.Name, egressFirewallObj.Namespace) deleteDNS := false obj, loaded := oc.egressFirewalls.LoadAndDelete(egressFirewallObj.Namespace) @@ -340,7 +259,7 @@ func (oc *Controller) deleteEgressFirewall(egressFirewallObj *egressfirewallapi. ef, ok := obj.(*egressFirewall) if !ok { return fmt.Errorf("deleteEgressFirewall failed: type assertion to *egressFirewall"+ - " failed for EgressFirewall %s of type %T in namespace %s.", + " failed for EgressFirewall %s of type %T in namespace %s", egressFirewallObj.Name, obj, egressFirewallObj.Namespace) } @@ -356,7 +275,7 @@ func (oc *Controller) deleteEgressFirewall(egressFirewallObj *egressfirewallapi. oc.egressFirewallDNS.Delete(egressFirewallObj.Namespace) } - return oc.deleteEgressFirewallRules(egressFirewallObj.Namespace, txn) + return oc.deleteEgressFirewallRules(egressFirewallObj.Namespace) } func (oc *Controller) updateEgressFirewallWithRetry(egressfirewall *egressfirewallapi.EgressFirewall) error { @@ -370,7 +289,7 @@ func (oc *Controller) updateEgressFirewallWithRetry(egressfirewall *egressfirewa return nil } -func (oc *Controller) addEgressFirewallRules(ef *egressFirewall, hashedAddressSetNameIPv4, hashedAddressSetNameIPv6 string, efStartPriority int, txn *util.NBTxn) error { +func (oc *Controller) addEgressFirewallRules(ef *egressFirewall, hashedAddressSetNameIPv4, hashedAddressSetNameIPv6 string, efStartPriority int) error { for _, rule := range ef.egressRules { var action string var matchTargets []matchTarget @@ -400,7 +319,7 @@ func (oc *Controller) addEgressFirewallRules(ef *egressFirewall, hashedAddressSe } } match := generateMatch(hashedAddressSetNameIPv4, hashedAddressSetNameIPv6, matchTargets, rule.ports) - err := oc.createEgressFirewallRules(efStartPriority-rule.id, match, action, ef.namespace, txn) + err := oc.createEgressFirewallRules(efStartPriority-rule.id, match, action, ef.namespace) if err != nil { return err } @@ -410,7 +329,7 @@ func (oc *Controller) addEgressFirewallRules(ef *egressFirewall, hashedAddressSe // createEgressFirewallRules uses the previously generated elements and creates the // logical_router_policy/join_switch_acl for a specific egressFirewallRouter -func (oc *Controller) createEgressFirewallRules(priority int, match, action, externalID string, txn *util.NBTxn) error { +func (oc *Controller) createEgressFirewallRules(priority int, match, action, externalID string) error { logicalSwitches := []string{} if config.Gateway.Mode == config.GatewayModeLocal { nodes, err := oc.watchFactory.GetNodes() @@ -423,69 +342,88 @@ func (oc *Controller) createEgressFirewallRules(priority int, match, action, ext } else { logicalSwitches = append(logicalSwitches, types.OVNJoinSwitch) } - uuids, stderr, err := util.RunOVNNbctl("--data=bare", "--no-heading", - "--columns=_uuid", "--format=table", "find", "ACL", match, "action="+action, - fmt.Sprintf("external-ids:egressFirewall=%s", externalID)) - if err != nil { - return fmt.Errorf("error executing find ACL command, stderr: %q, %+v", stderr, err) - } - sort.Strings(logicalSwitches) - for _, logicalSwitch := range logicalSwitches { - if uuids == "" { - id := fmt.Sprintf("%s-%d", logicalSwitch, priority) - _, stderr, err := txn.AddOrCommit([]string{"--id=@" + id, "create", "acl", - fmt.Sprintf("priority=%d", priority), - fmt.Sprintf("direction=%s", types.DirectionToLPort), match, "action=" + action, - fmt.Sprintf("external-ids:egressFirewall=%s", externalID), - "--", "add", "logical_switch", logicalSwitch, - "acls", "@" + id}) - if err != nil { - return fmt.Errorf("failed to commit db changes for egressFirewall stderr: %q, err: %+v", stderr, err) - } + egressFirewallACL := &nbdb.ACL{ + Priority: priority, + Direction: types.DirectionToLPort, + Match: match, + Action: action, + ExternalIDs: map[string]string{"egressFirewall": externalID}, + } + + // add it's UUID to the necessary logical switches + opModels := []libovsdbops.OperationModel{} + switches := []*nbdb.LogicalSwitch{} + for _, logicalSwitchName := range logicalSwitches { + lsn := logicalSwitchName + lsw := nbdb.LogicalSwitch{Name: lsn} + switches = append(switches, &lsw) + opModels = append(opModels, []libovsdbops.OperationModel{ + { + Model: &lsw, + ModelPredicate: func(ls *nbdb.LogicalSwitch) bool { return ls.Name == lsn }, + OnModelMutations: []interface{}{ + &lsw.ACLs, + }, + ErrNotFound: true, + BulkOp: true, + }, + }...) + } - } else { - for _, uuid := range strings.Fields(uuids) { - _, stderr, err := txn.AddOrCommit([]string{"add", "logical_switch", logicalSwitch, "acls", uuid}) - if err != nil { - return fmt.Errorf("failed to commit db changes for egressFirewall stderr: %q, err: %+v", stderr, err) + foundACLs := []nbdb.ACL{} + opModels = append([]libovsdbops.OperationModel{ + { + Model: egressFirewallACL, + ModelPredicate: func(acl *nbdb.ACL) bool { return libovsdbops.IsEquivalentACL(acl, egressFirewallACL) }, + ExistingResult: &foundACLs, + DoAfter: func() { + var uuid string + if len(foundACLs) == 1 { + uuid = foundACLs[0].UUID + } else { + uuid = egressFirewallACL.UUID } - } - } + for _, lsw := range switches { + lsw.ACLs = []string{uuid} + } + }, + }, + }, opModels...) + + if _, err := oc.modelClient.CreateOrUpdate(opModels...); err != nil { + return fmt.Errorf("failed to create egressFirewall ACL in ns %s and add to logical switches %v err: %v", + externalID, logicalSwitches, err) } + return nil } // deleteEgressFirewallRules delete the specific logical router policy/join switch Acls -func (oc *Controller) deleteEgressFirewallRules(externalID string, txn *util.NBTxn) error { - logicalSwitches := []string{} - if config.Gateway.Mode == config.GatewayModeLocal { - nodes, err := oc.watchFactory.GetNodes() - if err != nil { - return fmt.Errorf("unable to setup egress firewall ACLs on cluster nodes, err: %v", err) - } - for _, node := range nodes { - logicalSwitches = append(logicalSwitches, node.Name) - } - } else { - logicalSwitches = []string{types.OVNJoinSwitch} +func (oc *Controller) deleteEgressFirewallRules(externalID string) error { + // Find ACLs for a given egressFirewall + egressFirewallACLs, err := libovsdbops.FindACLsByExernalID(oc.nbClient, map[string]string{"egressFirewall": externalID}) + if err != nil { + return fmt.Errorf("unable to list egress firewall ACLs, cannot cleanup old stale data, err: %v", err) + } + + if len(egressFirewallACLs) == 0 { + klog.Warningf("No egressFirewall ACLs to delete in ns: %s", externalID) + return nil } - sort.Strings(logicalSwitches) - stdout, stderr, err := util.RunOVNNbctl("--data=bare", "--no-heading", "--columns=_uuid", "--format=table", "find", "ACL", - fmt.Sprintf("external-ids:egressFirewall=%s", externalID)) + + // delete egress firewall acls off any logical switch which has it + err = libovsdbops.RemoveACLsFromAllSwitches(oc.nbClient, egressFirewallACLs) if err != nil { - return fmt.Errorf("error deleting egressFirewall with external-ids %s, cannot get ACL policies - %s:%s", - externalID, err, stderr) + return fmt.Errorf("failed to remove reject acl from logical switches: %v", err) } - uuids := strings.Fields(stdout) - for _, logicalSwitch := range logicalSwitches { - for _, uuid := range uuids { - _, stderr, err = txn.AddOrCommit([]string{"remove", "logical_switch", logicalSwitch, "acls", uuid}) - if err != nil { - return fmt.Errorf("failed to commit db changes for egressFirewall stderr: %q, err: %+v", stderr, err) - } - } + + // Manually remove the egressFirewall ACLs instead of relying on ovsdb garbage collection to do so + err = libovsdbops.DeleteACLs(oc.nbClient, egressFirewallACLs) + if err != nil { + return err } + return nil } @@ -556,7 +494,7 @@ func generateMatch(ipv4Source, ipv6Source string, destinations []matchTarget, ds } } - match := fmt.Sprintf("match=\"(%s) && %s", dst, src) + match := fmt.Sprintf("(%s) && %s", dst, src) if len(dstPorts) > 0 { match = fmt.Sprintf("%s && %s", match, egressGetL4Match(dstPorts)) } @@ -564,9 +502,9 @@ func generateMatch(ipv4Source, ipv6Source string, destinations []matchTarget, ds if config.Gateway.Mode == config.GatewayModeLocal { extraMatch = getClusterSubnetsExclusion() } else { - extraMatch = fmt.Sprintf("inport == \\\"%s%s\\\"", types.JoinSwitchToGWRouterPrefix, types.OVNClusterRouter) + extraMatch = fmt.Sprintf("inport == \"%s%s\"", types.JoinSwitchToGWRouterPrefix, types.OVNClusterRouter) } - return fmt.Sprintf("%s && %s\"", match, extraMatch) + return fmt.Sprintf("%s && %s", match, extraMatch) } // egressGetL4Match generates the rules for when ports are specified in an egressFirewall Rule diff --git a/go-controller/pkg/ovn/egressfirewall_test.go b/go-controller/pkg/ovn/egressfirewall_test.go index 2e74e75d85e..eb7e5b8b6b6 100644 --- a/go-controller/pkg/ovn/egressfirewall_test.go +++ b/go-controller/pkg/ovn/egressfirewall_test.go @@ -11,8 +11,12 @@ import ( "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config" egressfirewallapi "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/egressfirewall/v1" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/nbdb" addressset "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/ovn/address_set" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/ovn/libovsdbops" ovntest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing/libovsdb" + libovsdbtest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing/libovsdb" t "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types" "github.com/urfave/cli/v2" @@ -42,9 +46,10 @@ func newEgressFirewallObject(name, namespace string, egressRules []egressfirewal var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode", func() { var ( - app *cli.App - fakeOVN *FakeOVN - fExec *ovntest.FakeExec + app *cli.App + fakeOVN *FakeOVN + fExec *ovntest.FakeExec + stopChan chan struct{} ) ginkgo.BeforeEach(func() { @@ -59,10 +64,12 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode", fExec = ovntest.NewLooseCompareFakeExec() fakeOVN = NewFakeOVN(fExec) + stopChan = make(chan struct{}) }) ginkgo.AfterEach(func() { + close(stopChan) fakeOVN.shutdown() }) @@ -72,23 +79,46 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode", const ( node1Name string = "node1" ) - fExec.AddFakeCmdsNoOutputNoError([]string{ - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%s priority>=%s direction=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority, t.DirectionFromLPort), - }) - // the sync function will find two egressFirewalls in the ovn-databse - fExec.AddFakeCmd(&ovntest.ExpectedCmd{ - Cmd: fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=external_id --format=table find acl priority<=%s priority>=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - Output: fmt.Sprintf("%s\n%s\n", "egressFirewall=default", "egressFirewall=none"), - }) - // since there is no egressfirewall in the namespace "none" add the commands to delete it - fExec.AddFakeCmd(&ovntest.ExpectedCmd{ - Cmd: "ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find ACL external-ids:egressFirewall=none", - Output: fmt.Sprintf("%s", fakeUUID), - }) - fExec.AddFakeCmdsNoOutputNoError([]string{ - "ovn-nbctl --timeout=15 remove logical_switch " + node1Name + " acls " + fmt.Sprintf("%s", fakeUUID), - }) + purgeACL := libovsdbops.BuildACL( + "", + t.DirectionFromLPort, + t.EgressFirewallStartPriority, + "", + nbdb.ACLActionDrop, + "", + "", + false, + map[string]string{"egressFirewall": "none"}, + ) + purgeACL.UUID = libovsdbops.BuildNamedUUID() + + keepACL := libovsdbops.BuildACL( + "", + t.DirectionFromLPort, + t.EgressFirewallStartPriority+1, + "", + nbdb.ACLActionDrop, + "", + "", + false, + map[string]string{"egressFirewall": "default"}, + ) + keepACL.UUID = libovsdbops.BuildNamedUUID() + + InitialNodeSwitch := &nbdb.LogicalSwitch{ + UUID: libovsdbops.BuildNamedUUID(), + Name: node1Name, + ACLs: []string{purgeACL.UUID, keepACL.UUID}, + } + + fakeOVN.dbSetup = libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + purgeACL, + keepACL, + InitialNodeSwitch, + }, + } fakeOVN.start(ctx, &v1.NodeList{ @@ -108,7 +138,23 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode", fakeOVN.controller.WatchEgressFirewall() - gomega.Eventually(fExec.CalledMatchesExpected).Should(gomega.BeTrue(), fExec.ErrorDesc) + // stale ACL will be removed from the switch + finalNodeSwitch := &nbdb.LogicalSwitch{ + UUID: InitialNodeSwitch.UUID, + Name: node1Name, + ACLs: []string{keepACL.UUID}, + } + + // Direction of both ACLs will be converted to + purgeACL.Direction = t.DirectionToLPort + keepACL.Direction = t.DirectionToLPort + + expectedDatabaseState := []libovsdb.TestData{ + keepACL, + finalNodeSwitch, + } + + gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState)) return nil } @@ -122,12 +168,17 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode", const ( node1Name string = "node1" ) - fExec.AddFakeCmdsNoOutputNoError([]string{ - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=external_id --format=table find acl priority<=%s priority>=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%s priority>=%s direction=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority, t.DirectionFromLPort), - "ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find ACL match=\"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && ip4.dst != 10.128.0.0/14\" action=allow external-ids:egressFirewall=namespace1", - "ovn-nbctl --timeout=15 --id=@node1-10000 create acl priority=10000 direction=" + t.DirectionToLPort + " match=\"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && ip4.dst != 10.128.0.0/14\" action=allow external-ids:egressFirewall=namespace1 -- add logical_switch node1 acls @node1-10000", - }) + + InitialNodeSwitch := &nbdb.LogicalSwitch{ + UUID: libovsdbops.BuildNamedUUID(), + Name: node1Name, + } + + fakeOVN.dbSetup = libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + InitialNodeSwitch, + }, + } namespace1 := *newNamespace("namespace1") egressFirewall := newEgressFirewallObject("default", namespace1.Name, []egressfirewallapi.EgressFirewallRule{ @@ -161,7 +212,32 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode", _, err := fakeOVN.fakeClient.EgressFirewallClient.K8sV1().EgressFirewalls(egressFirewall.Namespace).Get(context.TODO(), egressFirewall.Name, metav1.GetOptions{}) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Eventually(fExec.CalledMatchesExpected).Should(gomega.BeTrue(), fExec.ErrorDesc) + ipv4ACL := libovsdbops.BuildACL( + "", + t.DirectionToLPort, + t.EgressFirewallStartPriority, + "(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && ip4.dst != 10.128.0.0/14", + nbdb.ACLActionAllow, + "", + "", + false, + map[string]string{"egressFirewall": "namespace1"}, + ) + ipv4ACL.UUID = libovsdbops.BuildNamedUUID() + + // new ACL will be added to the switch + finalNodeSwitch := &nbdb.LogicalSwitch{ + UUID: InitialNodeSwitch.UUID, + Name: node1Name, + ACLs: []string{ipv4ACL.UUID}, + } + + expectedDatabaseState := []libovsdb.TestData{ + ipv4ACL, + finalNodeSwitch, + } + + gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState)) return nil } @@ -175,12 +251,17 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode", const ( node1Name string = "node1" ) - fExec.AddFakeCmdsNoOutputNoError([]string{ - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=external_id --format=table find acl priority<=%s priority>=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%s priority>=%s direction=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority, t.DirectionFromLPort), - "ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find ACL match=\"(ip6.dst == 2002::1234:abcd:ffff:c0a8:101/64) && (ip4.src == $a10481622940199974102 || ip6.src == $a10481620741176717680) && ip4.dst != 10.128.0.0/14\" action=allow external-ids:egressFirewall=namespace1", - "ovn-nbctl --timeout=15 --id=@node1-10000 create acl priority=10000 direction=" + t.DirectionToLPort + " match=\"(ip6.dst == 2002::1234:abcd:ffff:c0a8:101/64) && (ip4.src == $a10481622940199974102 || ip6.src == $a10481620741176717680) && ip4.dst != 10.128.0.0/14\" action=allow external-ids:egressFirewall=namespace1 -- add logical_switch node1 acls @node1-10000", - }) + + InitialNodeSwitch := &nbdb.LogicalSwitch{ + UUID: libovsdbops.BuildNamedUUID(), + Name: node1Name, + } + + fakeOVN.dbSetup = libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + InitialNodeSwitch, + }, + } namespace1 := *newNamespace("namespace1") egressFirewall := newEgressFirewallObject("default", namespace1.Name, []egressfirewallapi.EgressFirewallRule{ @@ -219,7 +300,32 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode", _, err := fakeOVN.fakeClient.EgressFirewallClient.K8sV1().EgressFirewalls(egressFirewall.Namespace).Get(context.TODO(), egressFirewall.Name, metav1.GetOptions{}) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Eventually(fExec.CalledMatchesExpected).Should(gomega.BeTrue(), fExec.ErrorDesc) + ipv6ACL := libovsdbops.BuildACL( + "", + t.DirectionToLPort, + t.EgressFirewallStartPriority, + "(ip6.dst == 2002::1234:abcd:ffff:c0a8:101/64) && (ip4.src == $a10481622940199974102 || ip6.src == $a10481620741176717680) && ip4.dst != 10.128.0.0/14", + nbdb.ACLActionAllow, + "", + "", + false, + map[string]string{"egressFirewall": "namespace1"}, + ) + ipv6ACL.UUID = libovsdbops.BuildNamedUUID() + + // new ACL will be added to the switch + finalNodeSwitch := &nbdb.LogicalSwitch{ + UUID: InitialNodeSwitch.UUID, + Name: node1Name, + ACLs: []string{ipv6ACL.UUID}, + } + + expectedDatabaseState := []libovsdb.TestData{ + ipv6ACL, + finalNodeSwitch, + } + + gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState)) return nil } @@ -235,12 +341,25 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode", const ( node1Name string = "node1" ) - fExec.AddFakeCmdsNoOutputNoError([]string{ - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=external_id --format=table find acl priority<=%s priority>=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%s priority>=%s direction=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority, t.DirectionFromLPort), - "ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find ACL match=\"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && ((udp && ( udp.dst == 100 ))) && ip4.dst != 10.128.0.0/14\" action=drop external-ids:egressFirewall=namespace1", - "ovn-nbctl --timeout=15 --id=@node1-10000 create acl priority=10000 direction=" + t.DirectionToLPort + " match=\"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && ((udp && ( udp.dst == 100 ))) && ip4.dst != 10.128.0.0/14\" action=drop external-ids:egressFirewall=namespace1 -- add logical_switch " + node1Name + " acls @node1-10000", - }) + + InitialNodeSwitch := &nbdb.LogicalSwitch{ + UUID: libovsdbops.BuildNamedUUID(), + Name: node1Name, + } + + fakeOVN.dbSetup = libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + InitialNodeSwitch, + }, + } + + //fExec.AddFakeCmdsNoOutputNoError([]string{ + // fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=external_id --format=table find acl priority<=%d priority>=%d", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), + // fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%d priority>=%d direction=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority, t.DirectionFromLPort), + // "ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find ACL match=\"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && ((udp && ( udp.dst == 100 ))) && ip4.dst != 10.128.0.0/14\" action=drop external-ids:egressFirewall=namespace1", + // "ovn-nbctl --timeout=15 --id=@node1-10000 create acl priority=10000 direction=" + t.DirectionToLPort + " match=\"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && ((udp && ( udp.dst == 100 ))) && ip4.dst != 10.128.0.0/14\" action=drop external-ids:egressFirewall=namespace1 -- add logical_switch " + node1Name + " acls @node1-10000", + //}) + namespace1 := *newNamespace("namespace1") egressFirewall := newEgressFirewallObject("default", namespace1.Name, []egressfirewallapi.EgressFirewallRule{ { @@ -284,7 +403,33 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode", fakeOVN.controller.WatchEgressFirewall() - gomega.Eventually(fExec.CalledMatchesExpected).Should(gomega.BeTrue(), fExec.ErrorDesc) + udpACL := libovsdbops.BuildACL( + "", + t.DirectionToLPort, + t.EgressFirewallStartPriority, + "(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && ((udp && ( udp.dst == 100 ))) && ip4.dst != 10.128.0.0/14", + nbdb.ACLActionDrop, + "", + "", + false, + map[string]string{"egressFirewall": "namespace1"}, + ) + + udpACL.UUID = libovsdbops.BuildNamedUUID() + + // new ACL will be added to the switch + finalNodeSwitch := &nbdb.LogicalSwitch{ + UUID: InitialNodeSwitch.UUID, + Name: node1Name, + ACLs: []string{udpACL.UUID}, + } + + expectedDatabaseState := []libovsdb.TestData{ + udpACL, + finalNodeSwitch, + } + + gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState)) return nil } @@ -295,21 +440,25 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode", app.Action = func(ctx *cli.Context) error { const ( node1Name string = "node1" + node2Name string = "node2" ) - fExec.AddFakeCmdsNoOutputNoError([]string{ - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=external_id --format=table find acl priority<=%s priority>=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%s priority>=%s direction=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority, t.DirectionFromLPort), - "ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find ACL match=\"(ip4.dst == 1.2.3.5/23) && " + - "ip4.src == $a10481622940199974102 && ((tcp && ( tcp.dst == 100 ))) && ip4.dst != 10.128.0.0/14\" action=allow external-ids:egressFirewall=namespace1", - "ovn-nbctl --timeout=15 --id=@node1-10000 create acl priority=10000 direction=" + t.DirectionToLPort + " match=\"(ip4.dst == 1.2.3.5/23) && ip4.src == $a10481622940199974102 && ((tcp && ( tcp.dst == 100 ))) && ip4.dst != 10.128.0.0/14\" action=allow external-ids:egressFirewall=namespace1 -- add logical_switch node1 acls @node1-10000 -- --id=@node2-10000 create acl priority=10000 direction=" + t.DirectionToLPort + " match=\"(ip4.dst == 1.2.3.5/23) && ip4.src == $a10481622940199974102 && ((tcp && ( tcp.dst == 100 ))) && ip4.dst != 10.128.0.0/14\" action=allow external-ids:egressFirewall=namespace1 -- add logical_switch node2 acls @node2-10000", - }) - fExec.AddFakeCmd(&ovntest.ExpectedCmd{ - Cmd: "ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find ACL external-ids:egressFirewall=namespace1", - Output: fmt.Sprintf("%s", fakeUUID), - }) - fExec.AddFakeCmdsNoOutputNoError([]string{ - "ovn-nbctl --timeout=15 remove logical_switch node1 acls " + fmt.Sprintf("%s", fakeUUID) + " -- remove logical_switch node2 acls " + fmt.Sprintf("%s", fakeUUID), - }) + + nodeSwitch1 := &nbdb.LogicalSwitch{ + UUID: libovsdbops.BuildNamedUUID(), + Name: node1Name, + } + nodeSwitch2 := &nbdb.LogicalSwitch{ + UUID: libovsdbops.BuildNamedUUID(), + Name: node2Name, + } + + fakeOVN.dbSetup = libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + nodeSwitch1, + nodeSwitch2, + }, + } + namespace1 := *newNamespace("namespace1") egressFirewall := newEgressFirewallObject("default", namespace1.Name, []egressfirewallapi.EgressFirewallRule{ { @@ -351,10 +500,43 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode", fakeOVN.controller.WatchEgressFirewall() + ipv4ACL := libovsdbops.BuildACL( + "", + t.DirectionToLPort, + t.EgressFirewallStartPriority, + "(ip4.dst == 1.2.3.5/23) && ip4.src == $a10481622940199974102 && ((tcp && ( tcp.dst == 100 ))) && ip4.dst != 10.128.0.0/14", + nbdb.ACLActionAllow, + "", + "", + false, + map[string]string{"egressFirewall": "namespace1"}, + ) + ipv4ACL.UUID = libovsdbops.BuildNamedUUID() + + // new ACL will be added to the switches + nodeSwitch1.ACLs = []string{ipv4ACL.UUID} + nodeSwitch2.ACLs = []string{ipv4ACL.UUID} + + expectedDatabaseState := []libovsdb.TestData{ + ipv4ACL, + nodeSwitch1, + nodeSwitch2, + } + + gomega.Expect(fakeOVN.nbClient).To(libovsdbtest.HaveData(expectedDatabaseState)) + err := fakeOVN.fakeClient.EgressFirewallClient.K8sV1().EgressFirewalls(egressFirewall.Namespace).Delete(context.TODO(), egressFirewall.Name, *metav1.NewDeleteOptions(0)) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Eventually(fExec.CalledMatchesExpected).Should(gomega.BeTrue(), fExec.ErrorDesc) + // ACL should be removed from switches after egfw is deleted + nodeSwitch1.ACLs = []string{} + nodeSwitch2.ACLs = []string{} + expectedDatabaseState = []libovsdb.TestData{ + nodeSwitch1, + nodeSwitch2, + } + + gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState)) return nil } @@ -367,9 +549,21 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode", const ( node1Name string = "node1" ) + + InitialNodeSwitch := &nbdb.LogicalSwitch{ + UUID: libovsdbops.BuildNamedUUID(), + Name: node1Name, + } + + fakeOVN.dbSetup = libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + InitialNodeSwitch, + }, + } + fExec.AddFakeCmdsNoOutputNoError([]string{ - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=external_id --format=table find acl priority<=%s priority>=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%s priority>=%s direction=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority, t.DirectionFromLPort), + fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=external_id --format=table find acl priority<=%d priority>=%d", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), + fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%d priority>=%d direction=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority, t.DirectionFromLPort), "ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find ACL match=\"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && ip4.dst != 10.128.0.0/14\" action=allow external-ids:egressFirewall=namespace1", "ovn-nbctl --timeout=15 --id=@node1-10000 create acl priority=10000 direction=" + t.DirectionToLPort + " match=\"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && ip4.dst != 10.128.0.0/14\" action=allow external-ids:egressFirewall=namespace1 -- add logical_switch node1 acls @node1-10000", }) @@ -425,12 +619,42 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for local gateway mode", fakeOVN.controller.WatchNamespaces() fakeOVN.controller.WatchEgressFirewall() + ipv4ACL := libovsdbops.BuildACL( + "", + t.DirectionToLPort, + t.EgressFirewallStartPriority, + "(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && ip4.dst != 10.128.0.0/14", + nbdb.ACLActionAllow, + "", + "", + false, + map[string]string{"egressFirewall": "namespace1"}, + ) + ipv4ACL.UUID = libovsdbops.BuildNamedUUID() + + // new ACL will be added to the switch + finalNodeSwitch := &nbdb.LogicalSwitch{ + UUID: InitialNodeSwitch.UUID, + Name: node1Name, + ACLs: []string{ipv4ACL.UUID}, + } + + // new ACL will be added to the switch + expectedDatabaseState := []libovsdb.TestData{ + ipv4ACL, + finalNodeSwitch, + } + + gomega.Expect(fakeOVN.nbClient).To(libovsdbtest.HaveData(expectedDatabaseState)) + _, err := fakeOVN.fakeClient.EgressFirewallClient.K8sV1().EgressFirewalls(egressFirewall.Namespace).Get(context.TODO(), egressFirewall.Name, metav1.GetOptions{}) gomega.Expect(err).NotTo(gomega.HaveOccurred()) _, err = fakeOVN.fakeClient.EgressFirewallClient.K8sV1().EgressFirewalls(egressFirewall1.Namespace).Update(context.TODO(), egressFirewall1, metav1.UpdateOptions{}) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Eventually(fExec.CalledMatchesExpected).Should(gomega.BeTrue(), fExec.ErrorDesc) + ipv4ACL.Action = nbdb.ACLActionDrop + + gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState)) return nil } @@ -476,24 +700,53 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode", const ( node1Name string = "node1" ) - fExec.AddFakeCmdsNoOutputNoError([]string{ - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%s priority>=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%s priority>=%s direction=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority, t.DirectionFromLPort), - }) - // the sync function will find two egressFirewalls in the ovn-databse - fExec.AddFakeCmd(&ovntest.ExpectedCmd{ - Cmd: fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=external_id --format=table find acl priority<=%s priority>=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - Output: fmt.Sprintf("%s\n%s\n", "egressFirewall=default", "egressFirewall=none"), - }) - // since there is no egressfirewall in the namespace "none" add the commands to delete it - fExec.AddFakeCmd(&ovntest.ExpectedCmd{ - Cmd: "ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find ACL external-ids:egressFirewall=none", - Output: fmt.Sprintf("%s", fakeUUID), - }) - fExec.AddFakeCmdsNoOutputNoError([]string{ - "ovn-nbctl --timeout=15 remove logical_switch join acls " + fmt.Sprintf("%s", fakeUUID), - }) + purgeACL := libovsdbops.BuildACL( + "", + t.DirectionFromLPort, + t.EgressFirewallStartPriority, + "", + nbdb.ACLActionDrop, + "", + "", + false, + map[string]string{"egressFirewall": "none"}, + ) + purgeACL.UUID = libovsdbops.BuildNamedUUID() + + keepACL := libovsdbops.BuildACL( + "", + t.DirectionFromLPort, + t.EgressFirewallStartPriority+1, + "", + nbdb.ACLActionDrop, + "", + "", + false, + map[string]string{"egressFirewall": "default"}, + ) + keepACL.UUID = libovsdbops.BuildNamedUUID() + + InitialNodeSwitch := &nbdb.LogicalSwitch{ + UUID: libovsdbops.BuildNamedUUID(), + Name: node1Name, + ACLs: []string{purgeACL.UUID, keepACL.UUID}, + } + + InitialJoinSwitch := &nbdb.LogicalSwitch{ + UUID: libovsdbops.BuildNamedUUID(), + Name: "join", + ACLs: []string{purgeACL.UUID, keepACL.UUID}, + } + + fakeOVN.dbSetup = libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + purgeACL, + keepACL, + InitialNodeSwitch, + InitialJoinSwitch, + }, + } fakeOVN.start(ctx, &v1.NodeList{ @@ -513,7 +766,30 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode", fakeOVN.controller.WatchEgressFirewall() - gomega.Eventually(fExec.CalledMatchesExpected).Should(gomega.BeTrue(), fExec.ErrorDesc) + // Both ACLS will be removed from the node switch + finalNodeSwitch := &nbdb.LogicalSwitch{ + UUID: InitialNodeSwitch.UUID, + Name: node1Name, + } + + // purgeACL will be removed form the join switch + finalJoinSwitch := &nbdb.LogicalSwitch{ + UUID: libovsdbops.BuildNamedUUID(), + Name: "join", + ACLs: []string{keepACL.UUID}, + } + + // Direction of both ACLs will be converted to + purgeACL.Direction = t.DirectionToLPort + keepACL.Direction = t.DirectionToLPort + + expectedDatabaseState := []libovsdb.TestData{ + keepACL, + finalNodeSwitch, + finalJoinSwitch, + } + + gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState)) return nil } @@ -527,13 +803,17 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode", const ( node1Name string = "node1" ) - fExec.AddFakeCmdsNoOutputNoError([]string{ - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%s priority>=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=external_id --format=table find acl priority<=%s priority>=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%s priority>=%s direction=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority, t.DirectionFromLPort), - "ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find ACL match=\"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && inport == \\\"" + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + "\\\"\" action=allow external-ids:egressFirewall=namespace1", - "ovn-nbctl --timeout=15 --id=@join-10000 create acl priority=10000 direction=" + t.DirectionToLPort + " match=\"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && inport == \\\"" + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + "\\\"\" action=allow external-ids:egressFirewall=namespace1 -- add logical_switch join acls @join-10000", - }) + + InitialJoinSwitch := &nbdb.LogicalSwitch{ + UUID: libovsdbops.BuildNamedUUID(), + Name: "join", + } + + fakeOVN.dbSetup = libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + InitialJoinSwitch, + }, + } namespace1 := *newNamespace("namespace1") egressFirewall := newEgressFirewallObject("default", namespace1.Name, []egressfirewallapi.EgressFirewallRule{ @@ -567,7 +847,32 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode", _, err := fakeOVN.fakeClient.EgressFirewallClient.K8sV1().EgressFirewalls(egressFirewall.Namespace).Get(context.TODO(), egressFirewall.Name, metav1.GetOptions{}) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Eventually(fExec.CalledMatchesExpected).Should(gomega.BeTrue(), fExec.ErrorDesc) + ipv4ACL := libovsdbops.BuildACL( + "", + t.DirectionToLPort, + t.EgressFirewallStartPriority, + "(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && inport == \""+t.JoinSwitchToGWRouterPrefix+t.OVNClusterRouter+"\"", + nbdb.ACLActionAllow, + "", + "", + false, + map[string]string{"egressFirewall": "namespace1"}, + ) + ipv4ACL.UUID = libovsdbops.BuildNamedUUID() + + // new ACL will be added to the switch + finalJoinSwitch := &nbdb.LogicalSwitch{ + UUID: InitialJoinSwitch.UUID, + Name: "join", + ACLs: []string{ipv4ACL.UUID}, + } + + expectedDatabaseState := []libovsdb.TestData{ + ipv4ACL, + finalJoinSwitch, + } + + gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState)) return nil } @@ -581,13 +886,17 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode", const ( node1Name string = "node1" ) - fExec.AddFakeCmdsNoOutputNoError([]string{ - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%s priority>=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=external_id --format=table find acl priority<=%s priority>=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%s priority>=%s direction=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority, t.DirectionFromLPort), - "ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find ACL match=\"(ip6.dst == 2002::1234:abcd:ffff:c0a8:101/64) && (ip4.src == $a10481622940199974102 || ip6.src == $a10481620741176717680) && inport == \\\"" + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + "\\\"\" action=allow external-ids:egressFirewall=namespace1", - "ovn-nbctl --timeout=15 --id=@join-10000 create acl priority=10000 direction=" + t.DirectionToLPort + " match=\"(ip6.dst == 2002::1234:abcd:ffff:c0a8:101/64) && (ip4.src == $a10481622940199974102 || ip6.src == $a10481620741176717680) && inport == \\\"" + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + "\\\"\" action=allow external-ids:egressFirewall=namespace1 -- add logical_switch join acls @join-10000", - }) + + InitialJoinSwitch := &nbdb.LogicalSwitch{ + UUID: libovsdbops.BuildNamedUUID(), + Name: "join", + } + + fakeOVN.dbSetup = libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + InitialJoinSwitch, + }, + } namespace1 := *newNamespace("namespace1") egressFirewall := newEgressFirewallObject("default", namespace1.Name, []egressfirewallapi.EgressFirewallRule{ @@ -626,7 +935,32 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode", _, err := fakeOVN.fakeClient.EgressFirewallClient.K8sV1().EgressFirewalls(egressFirewall.Namespace).Get(context.TODO(), egressFirewall.Name, metav1.GetOptions{}) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Eventually(fExec.CalledMatchesExpected).Should(gomega.BeTrue(), fExec.ErrorDesc) + ipv6ACL := libovsdbops.BuildACL( + "", + t.DirectionToLPort, + t.EgressFirewallStartPriority, + "(ip6.dst == 2002::1234:abcd:ffff:c0a8:101/64) && (ip4.src == $a10481622940199974102 || ip6.src == $a10481620741176717680) && inport == \""+t.JoinSwitchToGWRouterPrefix+t.OVNClusterRouter+"\"", + nbdb.ACLActionAllow, + "", + "", + false, + map[string]string{"egressFirewall": "namespace1"}, + ) + ipv6ACL.UUID = libovsdbops.BuildNamedUUID() + + // new ACL will be added to the switch + finalJoinSwitch := &nbdb.LogicalSwitch{ + UUID: InitialJoinSwitch.UUID, + Name: "join", + ACLs: []string{ipv6ACL.UUID}, + } + + expectedDatabaseState := []libovsdb.TestData{ + ipv6ACL, + finalJoinSwitch, + } + + gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState)) return nil } @@ -642,13 +976,18 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode", const ( node1Name string = "node1" ) - fExec.AddFakeCmdsNoOutputNoError([]string{ - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%s priority>=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=external_id --format=table find acl priority<=%s priority>=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%s priority>=%s direction=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority, t.DirectionFromLPort), - "ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find ACL match=\"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && ((udp && ( udp.dst == 100 ))) && inport == \\\"" + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + "\\\"\" action=drop external-ids:egressFirewall=namespace1", - "ovn-nbctl --timeout=15 --id=@join-10000 create acl priority=10000 direction=" + t.DirectionToLPort + " match=\"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && ((udp && ( udp.dst == 100 ))) && inport == \\\"" + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + "\\\"\" action=drop external-ids:egressFirewall=namespace1 -- add logical_switch join acls @join-10000", - }) + + initialJoinSwitch := &nbdb.LogicalSwitch{ + UUID: libovsdbops.BuildNamedUUID(), + Name: "join", + } + + fakeOVN.dbSetup = libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + initialJoinSwitch, + }, + } + namespace1 := *newNamespace("namespace1") egressFirewall := newEgressFirewallObject("default", namespace1.Name, []egressfirewallapi.EgressFirewallRule{ { @@ -692,7 +1031,34 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode", fakeOVN.controller.WatchEgressFirewall() - gomega.Eventually(fExec.CalledMatchesExpected).Should(gomega.BeTrue(), fExec.ErrorDesc) + udpACL := libovsdbops.BuildACL( + "", + t.DirectionToLPort, + t.EgressFirewallStartPriority, + "(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && ((udp && ( udp.dst == 100 ))) && inport == \""+ + t.JoinSwitchToGWRouterPrefix+t.OVNClusterRouter+"\"", + nbdb.ACLActionDrop, + "", + "", + false, + map[string]string{"egressFirewall": "namespace1"}, + ) + + udpACL.UUID = libovsdbops.BuildNamedUUID() + + // new ACL will be added to the switch + finalJoinSwitch := &nbdb.LogicalSwitch{ + UUID: initialJoinSwitch.UUID, + Name: "join", + ACLs: []string{udpACL.UUID}, + } + + expectedDatabaseState := []libovsdb.TestData{ + udpACL, + finalJoinSwitch, + } + + gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState)) return nil } @@ -704,21 +1070,18 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode", const ( node1Name string = "node1" ) - fExec.AddFakeCmdsNoOutputNoError([]string{ - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%s priority>=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=external_id --format=table find acl priority<=%s priority>=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%s priority>=%s direction=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority, t.DirectionFromLPort), - "ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find ACL match=\"(ip4.dst == 1.2.3.5/23) && " + - "ip4.src == $a10481622940199974102 && ((tcp && ( tcp.dst == 100 ))) && inport == \\\"" + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + "\\\"\" action=allow external-ids:egressFirewall=namespace1", - "ovn-nbctl --timeout=15 --id=@join-10000 create acl priority=10000 direction=" + t.DirectionToLPort + " match=\"(ip4.dst == 1.2.3.5/23) && ip4.src == $a10481622940199974102 && ((tcp && ( tcp.dst == 100 ))) && inport == \\\"" + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + "\\\"\" action=allow external-ids:egressFirewall=namespace1 -- add logical_switch join acls @join-10000", - }) - fExec.AddFakeCmd(&ovntest.ExpectedCmd{ - Cmd: "ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find ACL external-ids:egressFirewall=namespace1", - Output: fmt.Sprintf("%s", fakeUUID), - }) - fExec.AddFakeCmdsNoOutputNoError([]string{ - "ovn-nbctl --timeout=15 remove logical_switch join acls " + fmt.Sprintf("%s", fakeUUID), - }) + + initialJoinSwitch := &nbdb.LogicalSwitch{ + UUID: libovsdbops.BuildNamedUUID(), + Name: "join", + } + + fakeOVN.dbSetup = libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + initialJoinSwitch, + }, + } + namespace1 := *newNamespace("namespace1") egressFirewall := newEgressFirewallObject("default", namespace1.Name, []egressfirewallapi.EgressFirewallRule{ { @@ -754,10 +1117,39 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode", fakeOVN.controller.WatchEgressFirewall() + ipv4ACL := libovsdbops.BuildACL( + "", + t.DirectionToLPort, + t.EgressFirewallStartPriority, + "(ip4.dst == 1.2.3.5/23) && "+ + "ip4.src == $a10481622940199974102 && ((tcp && ( tcp.dst == 100 ))) && inport == \""+t.JoinSwitchToGWRouterPrefix+t.OVNClusterRouter+"\"", + nbdb.ACLActionAllow, + "", + "", + false, + map[string]string{"egressFirewall": "namespace1"}, + ) + ipv4ACL.UUID = libovsdbops.BuildNamedUUID() + + // new ACL will be added to the switch + finalJoinSwitch := &nbdb.LogicalSwitch{ + UUID: initialJoinSwitch.UUID, + Name: "join", + ACLs: []string{ipv4ACL.UUID}, + } + + expectedDatabaseState := []libovsdb.TestData{ + ipv4ACL, + finalJoinSwitch, + } + + gomega.Expect(fakeOVN.nbClient).To(libovsdbtest.HaveData(expectedDatabaseState)) + err := fakeOVN.fakeClient.EgressFirewallClient.K8sV1().EgressFirewalls(egressFirewall.Namespace).Delete(context.TODO(), egressFirewall.Name, *metav1.NewDeleteOptions(0)) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Eventually(fExec.CalledMatchesExpected).Should(gomega.BeTrue(), fExec.ErrorDesc) + // join switch should return to orignal state, egfw was deleted + gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(fakeOVN.dbSetup.NBData)) return nil } @@ -770,21 +1162,17 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode", const ( node1Name string = "node1" ) - fExec.AddFakeCmdsNoOutputNoError([]string{ - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%s priority>=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=external_id --format=table find acl priority<=%s priority>=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority), - fmt.Sprintf("ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find acl priority<=%s priority>=%s direction=%s", t.EgressFirewallStartPriority, t.MinimumReservedEgressFirewallPriority, t.DirectionFromLPort), - "ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find ACL match=\"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && inport == \\\"" + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + "\\\"\" action=allow external-ids:egressFirewall=namespace1", - "ovn-nbctl --timeout=15 --id=@join-10000 create acl priority=10000 direction=" + t.DirectionToLPort + " match=\"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && inport == \\\"" + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + "\\\"\" action=allow external-ids:egressFirewall=namespace1 -- add logical_switch join acls @join-10000", - }) - fExec.AddFakeCmd(&ovntest.ExpectedCmd{ - Cmd: "ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find ACL external-ids:egressFirewall=namespace1", - Output: fmt.Sprintf("%s", fakeUUID), - }) - fExec.AddFakeCmdsNoOutputNoError([]string{ - "ovn-nbctl --timeout=15 --data=bare --no-heading --columns=_uuid --format=table find ACL match=\"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && inport == \\\"" + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + "\\\"\" action=drop external-ids:egressFirewall=namespace1", - "ovn-nbctl --timeout=15 remove logical_switch join acls 8a86f6d8-7972-4253-b0bd-ddbef66e9303 -- --id=@join-10000 create acl priority=10000 direction=" + t.DirectionToLPort + " match=\"(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && inport == \\\"" + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + "\\\"\" action=drop external-ids:egressFirewall=namespace1 -- add logical_switch join acls @join-10000", - }) + + initialJoinSwitch := &nbdb.LogicalSwitch{ + UUID: libovsdbops.BuildNamedUUID(), + Name: "join", + } + + fakeOVN.dbSetup = libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + initialJoinSwitch, + }, + } namespace1 := *newNamespace("namespace1") egressFirewall := newEgressFirewallObject("default", namespace1.Name, []egressfirewallapi.EgressFirewallRule{ @@ -823,12 +1211,41 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations for shared gateway mode", fakeOVN.controller.WatchEgressFirewall() + ipv4ACL := libovsdbops.BuildACL( + "", + t.DirectionToLPort, + t.EgressFirewallStartPriority, + "(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102 && inport == \""+t.JoinSwitchToGWRouterPrefix+t.OVNClusterRouter+"\"", + nbdb.ACLActionAllow, + "", + "", + false, + map[string]string{"egressFirewall": "namespace1"}, + ) + ipv4ACL.UUID = libovsdbops.BuildNamedUUID() + + // new ACL will be added to the switch + finalJoinSwitch := &nbdb.LogicalSwitch{ + UUID: initialJoinSwitch.UUID, + Name: "join", + ACLs: []string{ipv4ACL.UUID}, + } + + expectedDatabaseState := []libovsdb.TestData{ + ipv4ACL, + finalJoinSwitch, + } + + gomega.Expect(fakeOVN.nbClient).To(libovsdbtest.HaveData(expectedDatabaseState)) + _, err := fakeOVN.fakeClient.EgressFirewallClient.K8sV1().EgressFirewalls(egressFirewall.Namespace).Get(context.TODO(), egressFirewall.Name, metav1.GetOptions{}) gomega.Expect(err).NotTo(gomega.HaveOccurred()) _, err = fakeOVN.fakeClient.EgressFirewallClient.K8sV1().EgressFirewalls(egressFirewall1.Namespace).Update(context.TODO(), egressFirewall1, metav1.UpdateOptions{}) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Eventually(fExec.CalledMatchesExpected).Should(gomega.BeTrue(), fExec.ErrorDesc) + ipv4ACL.Action = nbdb.ACLActionDrop + + gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState)) return nil } @@ -918,7 +1335,7 @@ var _ = ginkgo.Describe("OVN test basic functions", func() { ipv6Mode: false, destinations: []matchTarget{{matchKindV4CIDR, "1.2.3.4/32"}}, ports: nil, - output: `match="(ip4.dst == 1.2.3.4/32) && ip4.src == $testv4 && inport == \"` + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + `\""`, + output: "(ip4.dst == 1.2.3.4/32) && ip4.src == $testv4 && inport == \"" + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + "\"", }, { internalCIDR: "10.128.0.0/14", @@ -928,7 +1345,7 @@ var _ = ginkgo.Describe("OVN test basic functions", func() { ipv6Mode: true, destinations: []matchTarget{{matchKindV4CIDR, "1.2.3.4/32"}}, ports: nil, - output: `match="(ip4.dst == 1.2.3.4/32) && (ip4.src == $testv4 || ip6.src == $testv6) && inport == \"` + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + `\""`, + output: "(ip4.dst == 1.2.3.4/32) && (ip4.src == $testv4 || ip6.src == $testv6) && inport == \"" + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + "\"", }, { internalCIDR: "10.128.0.0/14", @@ -938,7 +1355,7 @@ var _ = ginkgo.Describe("OVN test basic functions", func() { ipv6Mode: true, destinations: []matchTarget{{matchKindV4AddressSet, "destv4"}, {matchKindV6AddressSet, "destv6"}}, ports: nil, - output: `match="(ip4.dst == $destv4 || ip6.dst == $destv6) && (ip4.src == $testv4 || ip6.src == $testv6) && inport == \"` + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + `\""`, + output: "(ip4.dst == $destv4 || ip6.dst == $destv6) && (ip4.src == $testv4 || ip6.src == $testv6) && inport == \"" + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + "\"", }, { internalCIDR: "10.128.0.0/14", @@ -948,7 +1365,7 @@ var _ = ginkgo.Describe("OVN test basic functions", func() { ipv6Mode: false, destinations: []matchTarget{{matchKindV4AddressSet, "destv4"}, {matchKindV6AddressSet, ""}}, ports: nil, - output: `match="(ip4.dst == $destv4) && ip4.src == $testv4 && inport == \"` + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + `\""`, + output: "(ip4.dst == $destv4) && ip4.src == $testv4 && inport == \"" + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + "\"", }, { internalCIDR: "10.128.0.0/14", @@ -958,7 +1375,7 @@ var _ = ginkgo.Describe("OVN test basic functions", func() { ipv6Mode: true, destinations: []matchTarget{{matchKindV6CIDR, "2001::/64"}}, ports: nil, - output: `match="(ip6.dst == 2001::/64) && (ip4.src == $testv4 || ip6.src == $testv6) && inport == \"` + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + `\""`, + output: "(ip6.dst == 2001::/64) && (ip4.src == $testv4 || ip6.src == $testv6) && inport == \"" + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + "\"", }, { internalCIDR: "2002:0:0:1234::/64", @@ -968,7 +1385,7 @@ var _ = ginkgo.Describe("OVN test basic functions", func() { ipv6Mode: true, destinations: []matchTarget{{matchKindV6AddressSet, "destv6"}}, ports: nil, - output: `match="(ip6.dst == $destv6) && ip6.src == $testv6 && inport == \"` + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + `\""`, + output: "(ip6.dst == $destv6) && ip6.src == $testv6 && inport == \"" + t.JoinSwitchToGWRouterPrefix + t.OVNClusterRouter + "\"", }, } diff --git a/go-controller/pkg/ovn/libovsdbops/acl.go b/go-controller/pkg/ovn/libovsdbops/acl.go index e27c6d60394..84f73a62581 100644 --- a/go-controller/pkg/ovn/libovsdbops/acl.go +++ b/go-controller/pkg/ovn/libovsdbops/acl.go @@ -19,10 +19,10 @@ func getACLName(acl *nbdb.ACL) string { return "" } -// isEquivalentACL if it has same uuid, or if it has same name +// IsEquivalentACL if it has same uuid, or if it has same name // and external ids, or if it has same priority, direction, match // and action. -func isEquivalentACL(existing *nbdb.ACL, searched *nbdb.ACL) bool { +func IsEquivalentACL(existing *nbdb.ACL, searched *nbdb.ACL) bool { if searched.UUID != "" && existing.UUID == searched.UUID { return true } @@ -34,7 +34,6 @@ func isEquivalentACL(existing *nbdb.ACL, searched *nbdb.ACL) bool { if eName != "" && eName == sName && reflect.DeepEqual(existing.ExternalIDs, searched.ExternalIDs) { return true } - return existing.Priority == searched.Priority && existing.Direction == searched.Direction && existing.Match == searched.Match && @@ -49,7 +48,7 @@ func findACL(nbClient libovsdbclient.Client, acl *nbdb.ACL) error { acls := []nbdb.ACL{} err := nbClient.WhereCache(func(item *nbdb.ACL) bool { - return isEquivalentACL(item, acl) + return IsEquivalentACL(item, acl) }).List(&acls) if err != nil { @@ -76,15 +75,29 @@ func ensureACLUUID(acl *nbdb.ACL) { func BuildACL(name string, direction nbdb.ACLDirection, priority int, match string, action nbdb.ACLAction, meter string, severity nbdb.ACLSeverity, log bool, externalIds map[string]string) *nbdb.ACL { name = fmt.Sprintf("%.63s", name) + + var realName *string + var realMeter *string + var realSeverity *string + if len(name) != 0 { + realName = &name + } + if len(meter) != 0 { + realMeter = &match + } + if len(severity) != 0 { + realSeverity = &severity + } + return &nbdb.ACL{ - Name: &name, + Name: realName, Direction: direction, Match: match, Action: action, Priority: priority, - Severity: &severity, + Severity: realSeverity, Log: log, - Meter: &meter, + Meter: realMeter, ExternalIDs: externalIds, } } @@ -171,6 +184,24 @@ func UpdateACLsLoggingOps(nbClient libovsdbclient.Client, ops []libovsdb.Operati return ops, nil } +func DeleteACLs(nbClient libovsdbclient.Client, acls []nbdb.ACL) error { + opModels := []OperationModel{} + for _, acl := range acls { + opModels = append(opModels, OperationModel{ + Model: &acl, + ModelPredicate: func(item *nbdb.ACL) bool { return IsEquivalentACL(item, &acl) }, + }) + } + + m := NewModelClient(nbClient) + err := m.Delete(opModels...) + if err != nil { + return fmt.Errorf("failed to manually delete ACLs err: %v", err) + } + + return nil +} + func UpdateACLLogging(nbClient libovsdbclient.Client, acl *nbdb.ACL) error { ops, err := UpdateACLsLoggingOps(nbClient, nil, acl) if err != nil { @@ -181,19 +212,70 @@ func UpdateACLLogging(nbClient libovsdbclient.Client, acl *nbdb.ACL) error { return err } -func FindRejectACLs(nbClient libovsdbclient.Client) ([]*nbdb.ACL, error) { +// findACLsByPredicate looks up ACLs from the cache based on a given predicate +func findACLsByPredicate(nbClient libovsdbclient.Client, lookupFunction func(item *nbdb.ACL) bool) ([]nbdb.ACL, error) { acls := []nbdb.ACL{} - err := nbClient.WhereCache(func(acl *nbdb.ACL) bool { + err := nbClient.WhereCache(lookupFunction).List(&acls) + if err != nil { + return nil, err + } + + if len(acls) == 0 { + return nil, libovsdbclient.ErrNotFound + } + + return acls, nil +} + +func FindRejectACLs(nbClient libovsdbclient.Client) ([]nbdb.ACL, error) { + rejectACLLookupFcn := func(acl *nbdb.ACL) bool { return acl.Action == nbdb.ACLActionReject - }).List(&acls) + } + + acls, err := findACLsByPredicate(nbClient, rejectACLLookupFcn) if err != nil { return nil, err } - aclPtrs := []*nbdb.ACL{} - for i := range acls { - aclPtrs = append(aclPtrs, &acls[i]) + return acls, nil + +} + +// FindACLsByPriorityRange looks up the acls with priorities within to a given inclusive range +func FindACLsByPriorityRange(nbClient libovsdbclient.Client, startPriority, endPriority int) ([]nbdb.ACL, error) { + // Lookup all ACLs in the specified priority range + priorityRangeLookupFcn := func(item *nbdb.ACL) bool { + return (item.Priority <= startPriority || + item.Priority >= endPriority) + } + + acls, err := findACLsByPredicate(nbClient, priorityRangeLookupFcn) + if err != nil { + return nil, err + } + + return acls, nil +} + +// FindACLsByExternalID looks up the acls with the given externalID/s +func FindACLsByExernalID(nbClient libovsdbclient.Client, externalIDs map[string]string) ([]nbdb.ACL, error) { + // Find ACLs for with a given exernalID + ACLLookupFcn := func(item *nbdb.ACL) bool { + aclMatch := false + for k, v := range externalIDs { + if itemVal, ok := item.ExternalIDs[k]; ok { + if itemVal == v { + aclMatch = true + } + } + } + return aclMatch + } + + acls, err := findACLsByPredicate(nbClient, ACLLookupFcn) + if err != nil { + return nil, err } - return aclPtrs, nil + return acls, nil } diff --git a/go-controller/pkg/ovn/libovsdbops/model_client.go b/go-controller/pkg/ovn/libovsdbops/model_client.go index c9875acc348..91d21595aab 100644 --- a/go-controller/pkg/ovn/libovsdbops/model_client.go +++ b/go-controller/pkg/ovn/libovsdbops/model_client.go @@ -52,6 +52,25 @@ func BuildMutationsFromFields(fields []interface{}, mutator ovsdb.Mutator) []mod if v.IsNil() || v.Elem().IsNil() { continue } + + if m, ok := field.(*map[string]string); ok { + // check if all values on m are zero, if so create slice of keys of m and set that to field + allEmpty := true + keySlice := []string{} + for key, value := range *m { + keySlice = append(keySlice, key) + if len(value) > 0 { + allEmpty = false + break + } + } + if allEmpty { + v = reflect.ValueOf(&keySlice) + } + } else if v.Elem().Kind() == reflect.Map { + panic(fmt.Sprintf("map type %v is not supported", v.Elem().Kind())) + } + mutation := model.Mutation{ Field: field, Mutator: mutator, diff --git a/go-controller/pkg/ovn/libovsdbops/switch.go b/go-controller/pkg/ovn/libovsdbops/switch.go index effd3d73cd6..916d4758459 100644 --- a/go-controller/pkg/ovn/libovsdbops/switch.go +++ b/go-controller/pkg/ovn/libovsdbops/switch.go @@ -2,12 +2,14 @@ package libovsdbops import ( "fmt" + "strings" libovsdbclient "github.com/ovn-org/libovsdb/client" "github.com/ovn-org/libovsdb/model" libovsdb "github.com/ovn-org/libovsdb/ovsdb" "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/nbdb" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types" ) // findSwitch looks up the switch in the cache and sets the UUID @@ -21,7 +23,7 @@ func findSwitch(nbClient libovsdbclient.Client, lswitch *nbdb.LogicalSwitch) err return item.Name == lswitch.Name }).List(&switches) if err != nil { - return fmt.Errorf("can't find router %+v: %v", *lswitch, err) + return fmt.Errorf("can't find switch %+v: %v", *lswitch, err) } if len(switches) > 1 { @@ -36,6 +38,65 @@ func findSwitch(nbClient libovsdbclient.Client, lswitch *nbdb.LogicalSwitch) err return nil } +// findSwitches returns all the current logicalSwitches +func findSwitches(nbClient libovsdbclient.Client) ([]nbdb.LogicalSwitch, error) { + switches := []nbdb.LogicalSwitch{} + err := nbClient.List(&switches) + if err != nil { + return nil, fmt.Errorf("can't find Locial Switches err: %v", err) + } + + if len(switches) == 0 { + return nil, libovsdbclient.ErrNotFound + } + + return switches, nil +} + +// findSwitchesByPredicate Looks up switches in the cache based on the lookup function +func findSwitchesByPredicate(nbClient libovsdbclient.Client, lookupFunction func(item *nbdb.LogicalSwitch) bool) ([]nbdb.LogicalSwitch, error) { + switches := []nbdb.LogicalSwitch{} + err := nbClient.WhereCache(lookupFunction).List(&switches) + if err != nil { + return nil, fmt.Errorf("can't find switches: %v", err) + } + + if len(switches) == 0 { + return nil, libovsdbclient.ErrNotFound + } + + return switches, nil +} + +// FindSwitchesWithOtherConfig finds switches with otherconfig value/s +func FindSwitchesWithOtherConfig(nbClient libovsdbclient.Client) ([]nbdb.LogicalSwitch, error) { + // Get all logical siwtches with other-config set + otherConfigSearch := func(item *nbdb.LogicalSwitch) bool { + return item.OtherConfig != nil + } + + switches, err := findSwitchesByPredicate(nbClient, otherConfigSearch) + if err != nil { + return nil, err + } + + return switches, nil +} + +func FindPerNodeJoinSwitches(nbClient libovsdbclient.Client) ([]nbdb.LogicalSwitch, error) { + // Get the legacy node join switches -> join_ + joinSwitchSearch := func(item *nbdb.LogicalSwitch) bool { + return strings.HasPrefix(item.Name, types.JoinSwitchPrefix) + } + + switches, err := findSwitchesByPredicate(nbClient, joinSwitchSearch) + if err != nil { + return nil, err + } + + return switches, nil +} + func AddLoadBalancersToSwitchOps(nbClient libovsdbclient.Client, ops []libovsdb.Operation, lswitch *nbdb.LogicalSwitch, lbs ...*nbdb.LoadBalancer) ([]libovsdb.Operation, error) { if ops == nil { ops = []libovsdb.Operation{} @@ -104,3 +165,71 @@ func ListSwitchesWithLoadBalancers(nbClient libovsdbclient.Client) ([]nbdb.Logic }).List(switches) return *switches, err } + +// RemoveACLFromSwitches removes the ACL uuid entry from Logical Switch acl's list. +func removeACLsFromSwitches(nbClient libovsdbclient.Client, switches []nbdb.LogicalSwitch, acls []nbdb.ACL) error { + var opModels []OperationModel + var aclUUIDs []string + + for _, acl := range acls { + aclUUIDs = append(aclUUIDs, acl.UUID) + } + + for i, sw := range switches { + sw.ACLs = aclUUIDs + swName := switches[i].Name + opModels = append(opModels, OperationModel{ + Model: &sw, + ModelPredicate: func(ls *nbdb.LogicalSwitch) bool { return ls.Name == swName }, + OnModelMutations: []interface{}{ + &sw.ACLs, + }, + ErrNotFound: true, + BulkOp: true, + }) + } + + m := NewModelClient(nbClient) + if err := m.Delete(opModels...); err != nil { + return fmt.Errorf("error while removing ACLS: %v, from switches err: %v", aclUUIDs, err) + } + + return nil +} + +// RemoveACLFromSwitches removes the specified ACLs from the per node Logical Switches +func RemoveACLsFromNodeSwitches(nbClient libovsdbclient.Client, acls []nbdb.ACL) error { + // Find all node switches + nodeSwichLookupFcn := func(item *nbdb.LogicalSwitch) bool { + // Ignore external and Join switches(both legacy and current) + return !(strings.HasPrefix(item.Name, types.JoinSwitchPrefix) || item.Name == "join" || strings.HasPrefix(item.Name, types.ExternalSwitchPrefix)) + } + + switches, err := findSwitchesByPredicate(nbClient, nodeSwichLookupFcn) + if err != nil { + return err + } + + err = removeACLsFromSwitches(nbClient, switches, acls) + if err != nil { + return err + } + + return nil +} + +// RemoveACLFromSwitches removes the ACL uuid entry from Logical Switch acl's list. +func RemoveACLsFromAllSwitches(nbClient libovsdbclient.Client, acls []nbdb.ACL) error { + // Find all switches + switches, err := findSwitches(nbClient) + if err != nil { + return err + } + + err = removeACLsFromSwitches(nbClient, switches, acls) + if err != nil { + return err + } + + return nil +} diff --git a/go-controller/pkg/ovn/libovsdbops/switch_test.go b/go-controller/pkg/ovn/libovsdbops/switch_test.go new file mode 100644 index 00000000000..db1d5b72df8 --- /dev/null +++ b/go-controller/pkg/ovn/libovsdbops/switch_test.go @@ -0,0 +1,119 @@ +package libovsdbops + +import ( + "fmt" + "testing" + + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/nbdb" + libovsdbtest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing/libovsdb" +) + +func TestRemoveACLsFromSwitches(t *testing.T) { + fakeACL1 := &nbdb.ACL{ + UUID: BuildNamedUUID(), + } + + fakeACL2 := &nbdb.ACL{ + UUID: BuildNamedUUID(), + } + + fakeSwitch1 := &nbdb.LogicalSwitch{ + Name: "sw1", + UUID: BuildNamedUUID(), + ACLs: []string{fakeACL1.UUID}, + } + + fakeSwitch2 := &nbdb.LogicalSwitch{ + Name: "sw2", + UUID: BuildNamedUUID(), + ACLs: []string{fakeACL1.UUID, fakeACL2.UUID}, + } + + // Add switch without ACL to ensure the delete function + // can handle this case + fakeSwitch3 := &nbdb.LogicalSwitch{ + Name: "sw3", + UUID: BuildNamedUUID(), + } + + tests := []struct { + desc string + expectErr bool + initialNbdb libovsdbtest.TestSetup + expectedNbdb libovsdbtest.TestSetup + }{ + { + desc: "remove acl on two switches", + expectErr: false, + initialNbdb: libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + fakeSwitch1, + fakeSwitch2, + fakeSwitch3, + }, + }, + expectedNbdb: libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + &nbdb.LogicalSwitch{ + Name: "sw1", + UUID: fakeSwitch1.UUID, + }, + &nbdb.LogicalSwitch{ + Name: "sw2", + UUID: fakeSwitch2.UUID, + }, + fakeSwitch3, + }, + }, + }, + { + desc: "remove acl on no switches", + expectErr: true, + initialNbdb: libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + fakeSwitch3, + }, + }, + expectedNbdb: libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + fakeSwitch3, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.desc, func(t *testing.T) { + stopChan := make(chan struct{}) + + nbClient, _ := libovsdbtest.NewNBTestHarness(tt.initialNbdb, stopChan) + + fakeSwitches := []nbdb.LogicalSwitch{ + *fakeSwitch1, + *fakeSwitch2, + *fakeSwitch3, + } + + ACLs := []nbdb.ACL{ + *fakeACL1, + *fakeACL2, + } + + err := removeACLsFromSwitches(nbClient, fakeSwitches, ACLs) + if err != nil && !tt.expectErr { + t.Fatal(fmt.Errorf("RemoveACLFromNodeSwitches() error = %v", err)) + } + + matcher := libovsdbtest.HaveData(tt.expectedNbdb.NBData) + success, err := matcher.Match(nbClient) + + if !success { + t.Fatal(fmt.Errorf("test: \"%s\" didn't match expected with actual, err: %v", tt.desc, matcher.FailureMessage(nbClient))) + } + if err != nil { + t.Fatal(fmt.Errorf("test: \"%s\" encountered error: %v", tt.desc, err)) + } + + close(stopChan) + }) + } +} diff --git a/go-controller/pkg/ovn/libovsdbops/transact.go b/go-controller/pkg/ovn/libovsdbops/transact.go index d80d117abd5..84efedc6356 100644 --- a/go-controller/pkg/ovn/libovsdbops/transact.go +++ b/go-controller/pkg/ovn/libovsdbops/transact.go @@ -7,7 +7,7 @@ import ( "github.com/ovn-org/libovsdb/client" "github.com/ovn-org/libovsdb/model" "github.com/ovn-org/libovsdb/ovsdb" - "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/libovsdb" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types" ) func TransactAndCheck(client client.Client, ops []ovsdb.Operation) ([]ovsdb.OperationResult, error) { @@ -15,7 +15,7 @@ func TransactAndCheck(client client.Client, ops []ovsdb.Operation) ([]ovsdb.Oper return []ovsdb.OperationResult{{}}, nil } - ctx, cancel := context.WithTimeout(context.TODO(), libovsdb.OVSDBTimeout) + ctx, cancel := context.WithTimeout(context.TODO(), types.OVSDBTimeout) defer cancel() results, err := client.Transact(ctx, ops...) diff --git a/go-controller/pkg/ovn/master.go b/go-controller/pkg/ovn/master.go index 2f64a73ef4c..8216b60a19f 100644 --- a/go-controller/pkg/ovn/master.go +++ b/go-controller/pkg/ovn/master.go @@ -159,25 +159,14 @@ func (oc *Controller) upgradeToSingleSwitchOVNTopology(existingNodeList *kapi.No } } - nodeSwitches, stderr, err := util.RunOVNNbctl("--data=bare", "--no-heading", "--format=csv", - "--columns=name", "find", "logical_switch") + legacyJoinSwitches, err := libovsdbops.FindPerNodeJoinSwitches(oc.nbClient) if err != nil { - return fmt.Errorf("failed to get all logical switches for upgrade: stderr: %q, error: %v", - stderr, err) + klog.Errorf("Failed to remove any legacy per node join switches") } - logicalNodes := make(map[string]bool) - for _, switchName := range strings.Split(nodeSwitches, "\n") { - // We are interested only in the join_* switches - if !strings.HasPrefix(switchName, "join_") { - continue - } - nodeName := strings.TrimPrefix(switchName, "join_") - logicalNodes[nodeName] = true - } - - for nodeName := range logicalNodes { + for _, legacyJoinSwitch := range legacyJoinSwitches { // if the node was deleted when ovn-master was down, delete its per-node switch + nodeName := strings.TrimPrefix(legacyJoinSwitch.Name, types.JoinSwitchPrefix) upgradeOnly := true if _, ok := existingNodes[nodeName]; !ok { _ = oc.deleteNodeLogicalNetwork(nodeName) @@ -186,7 +175,7 @@ func (oc *Controller) upgradeToSingleSwitchOVNTopology(existingNodeList *kapi.No // for all nodes include the ones that were deleted, delete its gateway entities. // See comments above the multiJoinSwitchGatewayCleanup() function for details. - err = oc.multiJoinSwitchGatewayCleanup(nodeName, upgradeOnly) + err := oc.multiJoinSwitchGatewayCleanup(nodeName, upgradeOnly) if err != nil { return err } @@ -658,7 +647,7 @@ func (oc *Controller) syncNodeManagementPort(node *kapi.Node, hostSubnets []*net } if v4Subnet != nil { - if err := util.UpdateNodeSwitchExcludeIPs(node.Name, v4Subnet); err != nil { + if err := util.UpdateNodeSwitchExcludeIPs(oc.nbClient, node.Name, v4Subnet); err != nil { return err } } @@ -783,10 +772,8 @@ func (oc *Controller) ensureNodeLogicalNetwork(node *kapi.Node, hostSubnets []*n } } - lsArgs := []string{ - "--may-exist", - "ls-add", nodeName, - "--", "set", "logical_switch", nodeName, + logicalSwitch := nbdb.LogicalSwitch{ + Name: nodeName, } var v4Gateway, v6Gateway net.IP @@ -801,9 +788,9 @@ func (oc *Controller) ensureNodeLogicalNetwork(node *kapi.Node, hostSubnets []*n if utilnet.IsIPv6CIDR(hostSubnet) { v6Gateway = gwIfAddr.IP - lsArgs = append(lsArgs, - "other-config:ipv6_prefix="+hostSubnet.IP.String(), - ) + logicalSwitch.OtherConfig = map[string]string{ + "ipv6_prefix": hostSubnet.IP.String(), + } } else { v4Gateway = gwIfAddr.IP excludeIPs := mgmtIfAddr.IP.String() @@ -811,10 +798,10 @@ func (oc *Controller) ensureNodeLogicalNetwork(node *kapi.Node, hostSubnets []*n hybridOverlayIfAddr := util.GetNodeHybridOverlayIfAddr(hostSubnet) excludeIPs += ".." + hybridOverlayIfAddr.IP.String() } - lsArgs = append(lsArgs, - "other-config:subnet="+hostSubnet.String(), - "other-config:exclude_ips="+excludeIPs, - ) + logicalSwitch.OtherConfig = map[string]string{ + "subnet": hostSubnet.String(), + "exclude_ips": excludeIPs, + } } } @@ -844,18 +831,18 @@ func (oc *Controller) ensureNodeLogicalNetwork(node *kapi.Node, hostSubnets []*n }, ErrNotFound: true, }, + { + Model: &logicalSwitch, + ModelPredicate: func(ls *nbdb.LogicalSwitch) bool { return ls.Name == nodeName }, + OnModelUpdates: []interface{}{ + &logicalSwitch.OtherConfig, + }, + }, } if _, err := oc.modelClient.CreateOrUpdate(opModels...); err != nil { return fmt.Errorf("failed to add logical port to router, error: %v", err) } - // Create a logical switch and set its subnet. - stdout, stderr, err := util.RunOVNNbctl(lsArgs...) - if err != nil { - klog.Errorf("Failed to create a logical switch %v, stdout: %q, stderr: %q, error: %v", nodeName, stdout, stderr, err) - return err - } - // also add the join switch IPs for this node - needed in shared gateway mode lrpIPs, err := oc.joinSwIPManager.EnsureJoinLRPIPs(nodeName) if err != nil { @@ -886,49 +873,36 @@ func (oc *Controller) ensureNodeLogicalNetwork(node *kapi.Node, hostSubnets []*n // If supported, enable IGMP/MLD snooping and querier on the node. if oc.multicastSupport { - stdout, stderr, err = util.RunOVNNbctl("set", "logical_switch", - nodeName, "other-config:mcast_snoop=\"true\"") - if err != nil { - klog.Errorf("Failed to enable IGMP on logical switch %v, stdout: %q, stderr: %q, error: %v", - nodeName, stdout, stderr, err) - return err - } + logicalSwitch.OtherConfig["mcast_snoop"] = "true" // Configure IGMP/MLD querier if the gateway IP address is known. // Otherwise disable it. if v4Gateway != nil || v6Gateway != nil { + logicalSwitch.OtherConfig["mcast_querier"] = "true" + logicalSwitch.OtherConfig["mcast_eth_src"] = nodeLRPMAC.String() if v4Gateway != nil { - stdout, stderr, err = util.RunOVNNbctl("set", "logical_switch", - nodeName, "other-config:mcast_querier=\"true\"", - "other-config:mcast_eth_src=\""+nodeLRPMAC.String()+"\"", - "other-config:mcast_ip4_src=\""+v4Gateway.String()+"\"") - if err != nil { - klog.Errorf("Failed to enable IGMP Querier on logical switch %v, stdout: %q, stderr: %q, error: %v", - nodeName, stdout, stderr, err) - return err - } + logicalSwitch.OtherConfig["mcast_ip4_src"] = v4Gateway.String() } if v6Gateway != nil { - stdout, stderr, err = util.RunOVNNbctl("set", "logical_switch", - nodeName, "other-config:mcast_querier=\"true\"", - "other-config:mcast_eth_src=\""+nodeLRPMAC.String()+"\"", - "other-config:mcast_ip6_src=\""+util.HWAddrToIPv6LLA(nodeLRPMAC).String()+"\"") - if err != nil { - klog.Errorf("Failed to enable MLD Querier on logical switch %v, stdout: %q, stderr: %q, error: %v", - nodeName, stdout, stderr, err) - return err - } + logicalSwitch.OtherConfig["mcast_ip6_src"] = util.HWAddrToIPv6LLA(nodeLRPMAC).String() } } else { - stdout, stderr, err = util.RunOVNNbctl("set", "logical_switch", - nodeName, "other-config:mcast_querier=\"false\"") - if err != nil { - klog.Errorf("Failed to disable IGMP/MLD Querier on logical switch %v, stdout: %q, stderr: %q, error: %v", - nodeName, stdout, stderr, err) - return err - } - klog.Infof("Disabled IGMP/MLD Querier on logical switch %v (No IPv4/IPv6 Source IP available)", - nodeName) + logicalSwitch.OtherConfig["mcast_querier"] = "false" + } + + // Create the Node's Logical Switch and set it's subnet + opModels = []libovsdbops.OperationModel{ + { + Model: &logicalSwitch, + ModelPredicate: func(lr *nbdb.LogicalSwitch) bool { return lr.Name == nodeName }, + OnModelMutations: []interface{}{ + &logicalSwitch.OtherConfig, + }, + ErrNotFound: true, + }, + } + if _, err := oc.modelClient.CreateOrUpdate(opModels...); err != nil { + return fmt.Errorf("failed to configure Multicast on logical switch for node %s, error: %v", nodeName, err) } } @@ -936,7 +910,7 @@ func (oc *Controller) ensureNodeLogicalNetwork(node *kapi.Node, hostSubnets []*n nodeSwToRtrUUID, err := oc.addNodeLogicalSwitchPort(nodeName, types.SwitchToRouterPrefix+nodeName, "router", []string{"router"}, map[string]string{"router-port": types.RouterToSwitchPrefix + nodeName}) if err != nil { - klog.Errorf("Failed to add logical port to switch, stdout: %q, stderr: %q, error: %v", stdout, stderr, err) + klog.Errorf("Failed to add logical port to switch, error: %v", err) return err } @@ -1430,37 +1404,34 @@ func (oc *Controller) syncNodes(nodes []interface{}) { delete(chassisMap, nodeName) } - nodeSwitches, stderr, err := util.RunOVNNbctl("--data=bare", "--no-heading", - "--format=csv", "--columns=name,other-config", "find", "logical_switch") + nodeSwitches, err := libovsdbops.FindSwitchesWithOtherConfig(oc.nbClient) if err != nil { - klog.Errorf("Failed to get node logical switches: stderr: %q, error: %v", - stderr, err) + klog.Errorf("Failed to get node logical switches which have other-config set error: %v", err) return } // find node logical switches which have other-config value set - for _, result := range strings.Split(nodeSwitches, "\n") { - // Split result into name and other-config - items := strings.Split(result, ",") - if len(items) != 2 || len(items[0]) == 0 { - continue - } - nodeName := items[0] - if _, ok := foundNodes[nodeName]; ok { + for _, nodeSwitch := range nodeSwitches { + if _, ok := foundNodes[nodeSwitch.Name]; ok { // node still exists, no cleanup to do continue } var subnets []*net.IPNet - attrs := strings.Fields(items[1]) - for _, attr := range attrs { + for key, value := range nodeSwitch.OtherConfig { var subnet *net.IPNet - if strings.HasPrefix(attr, "subnet=") { - subnetStr := strings.TrimPrefix(attr, "subnet=") - _, subnet, _ = net.ParseCIDR(subnetStr) - } else if strings.HasPrefix(attr, "ipv6_prefix=") { - prefixStr := strings.TrimPrefix(attr, "ipv6_prefix=") - _, subnet, _ = net.ParseCIDR(prefixStr + "/64") + if key == "subnet" { + _, subnet, err = net.ParseCIDR(value) + if err != nil { + klog.Warningf("Unable to parse subnet CIDR %v", value) + continue + } + } else if key == "ipv6_prefix" { + _, subnet, err = net.ParseCIDR(value + "/64") + if err != nil { + klog.Warningf("Unable to parse ipv6_prefix CIDR %v/64", value) + continue + } } if subnet != nil { subnets = append(subnets, subnet) @@ -1470,9 +1441,9 @@ func (oc *Controller) syncNodes(nodes []interface{}) { continue } - oc.deleteNode(nodeName, subnets, nil) + oc.deleteNode(nodeSwitch.Name, subnets, nil) //remove the node from the chassis map so we don't delete it twice - delete(chassisMap, nodeName) + delete(chassisMap, nodeSwitch.Name) } deleteChassis(oc.ovnSBClient, chassisMap) diff --git a/go-controller/pkg/ovn/master_test.go b/go-controller/pkg/ovn/master_test.go index aebaf3235c3..b54ec272560 100644 --- a/go-controller/pkg/ovn/master_test.go +++ b/go-controller/pkg/ovn/master_test.go @@ -243,13 +243,6 @@ func addNodeportLBs(fexec *ovntest.FakeExec, nodeName, tcpLBUUID, udpLBUUID, sct */ func addNodeLogicalFlows(testData []libovsdb.TestData, expectedOVNClusterRouter *nbdb.LogicalRouter, expectedNodeSwitch *nbdb.LogicalSwitch, expectedClusterRouterPortGroup, expectedClusterPortGroup *nbdb.PortGroup, fexec *ovntest.FakeExec, node *tNode, clusterCIDR string, enableIPv6 bool) []libovsdb.TestData { - fexec.AddFakeCmdsNoOutputNoError([]string{ - "ovn-nbctl --timeout=15 --data=bare --no-heading --format=csv --columns=name,other-config find logical_switch", - }) - - fexec.AddFakeCmdsNoOutputNoError([]string{ - "ovn-nbctl --timeout=15 --may-exist ls-add " + node.Name + " -- set logical_switch " + node.Name + " other-config:subnet=" + node.NodeSubnet + " other-config:exclude_ips=" + node.NodeMgmtPortIP, - }) testData = append(testData, &nbdb.LogicalRouterPort{ Name: types.RouterToSwitchPrefix + node.Name, @@ -282,13 +275,7 @@ func addNodeLogicalFlows(testData []libovsdb.TestData, expectedOVNClusterRouter expectedNodeSwitch.Ports = append(expectedNodeSwitch.Ports, types.K8sPrefix+node.Name+"-UUID") expectedClusterPortGroup.Ports = []string{types.K8sPrefix + node.Name + "-UUID"} - fexec.AddFakeCmd(&ovntest.ExpectedCmd{ - Cmd: "ovn-nbctl --timeout=15 lsp-list " + node.Name, - Output: "29df5ce5-2802-4ee5-891f-4fb27ca776e9 (" + types.K8sPrefix + node.Name + ")", - }) - fexec.AddFakeCmdsNoOutputNoError([]string{ - "ovn-nbctl --timeout=15 -- --if-exists remove logical_switch " + node.Name + " other-config exclude_ips", "ovn-nbctl --timeout=15 --if-exists lrp-del " + types.RouterToSwitchPrefix + node.Name + " -- lrp-add ovn_cluster_router " + types.RouterToSwitchPrefix + node.Name + " " + node.NodeLRPMAC + " " + node.NodeGWIP + " -- lrp-set-gateway-chassis " + types.RouterToSwitchPrefix + node.Name + " " + node.SystemID + " 1", }) @@ -1140,8 +1127,9 @@ var _ = ginkgo.Describe("Gateway Init Operations", func() { Name: types.OVNClusterRouter, } expectedNodeSwitch := &nbdb.LogicalSwitch{ - UUID: node1.Name + "-UUID", - Name: node1.Name, + UUID: node1.Name + "-UUID", + Name: node1.Name, + OtherConfig: map[string]string{"subnet": node1.NodeSubnet}, } expectedClusterRouterPortGroup := &nbdb.PortGroup{ UUID: types.ClusterRtrPortGroupName + "-UUID", @@ -1163,8 +1151,11 @@ var _ = ginkgo.Describe("Gateway Init Operations", func() { UUID: types.OVNJoinSwitch + "-UUID", Name: types.OVNJoinSwitch, }, + &nbdb.LogicalSwitch{ + UUID: node1.Name + "-UUID", + Name: node1.Name, + }, expectedOVNClusterRouter, - expectedNodeSwitch, expectedClusterRouterPortGroup, expectedClusterPortGroup, }, @@ -1216,6 +1207,9 @@ var _ = ginkgo.Describe("Gateway Init Operations", func() { expectedDatabaseState = generateGatewayInitExpectedNB(expectedDatabaseState, expectedOVNClusterRouter, expectedNodeSwitch, node1.Name, clusterSubnets, []*net.IPNet{subnet}, l3Config, []*net.IPNet{joinLRPIPs}, []*net.IPNet{dLRPIPs}, skipSnat) gomega.Eventually(libovsdbOvnNBClient).Should(libovsdbtest.HaveData(expectedDatabaseState)) + // Make sure remaining fexec calls are still correct + gomega.Expect(fexec.CalledMatchesExpected()).To(gomega.BeTrue(), fexec.ErrorDesc) + return nil } diff --git a/go-controller/pkg/ovn/namespace_test.go b/go-controller/pkg/ovn/namespace_test.go index 7c710d1aee1..ecf47ebc2bf 100644 --- a/go-controller/pkg/ovn/namespace_test.go +++ b/go-controller/pkg/ovn/namespace_test.go @@ -292,6 +292,10 @@ var _ = ginkgo.Describe("OVN Namespace Operations", func() { fexec := fakeOvn.fakeExec expectedDatabaseState := []libovsdb.TestData{} + + // Add subnet to otherconfig for node + expectedNodeSwitch.OtherConfig = map[string]string{"subnet": node1.NodeSubnet} + expectedDatabaseState = addNodeLogicalFlows(expectedDatabaseState, expectedOVNClusterRouter, expectedNodeSwitch, expectedClusterRouterPortGroup, expectedClusterPortGroup, fexec, &node1, clusterCIDR, config.IPv6Mode) fakeOvn.controller.joinSwIPManager, _ = lsm.NewJoinLogicalSwitchIPManager(fakeOvn.nbClient, []string{node1.Name}) diff --git a/go-controller/pkg/ovn/ovn.go b/go-controller/pkg/ovn/ovn.go index cc118fdaef8..090b1910efd 100644 --- a/go-controller/pkg/ovn/ovn.go +++ b/go-controller/pkg/ovn/ovn.go @@ -735,7 +735,7 @@ func (oc *Controller) WatchEgressFirewall() *factory.Handler { AddFunc: func(obj interface{}) { egressFirewall := obj.(*egressfirewall.EgressFirewall).DeepCopy() txn := util.NewNBTxn() - addErrors := oc.addEgressFirewall(egressFirewall, txn) + addErrors := oc.addEgressFirewall(egressFirewall) if addErrors != nil { klog.Error(addErrors) egressFirewall.Status.Status = egressFirewallAddError @@ -760,7 +760,7 @@ func (oc *Controller) WatchEgressFirewall() *factory.Handler { oldEgressFirewall := old.(*egressfirewall.EgressFirewall) if !reflect.DeepEqual(oldEgressFirewall.Spec, newEgressFirewall.Spec) { txn := util.NewNBTxn() - errList := oc.updateEgressFirewall(oldEgressFirewall, newEgressFirewall, txn) + errList := oc.updateEgressFirewall(oldEgressFirewall, newEgressFirewall) if errList != nil { newEgressFirewall.Status.Status = egressFirewallUpdateError klog.Error(errList) @@ -784,7 +784,7 @@ func (oc *Controller) WatchEgressFirewall() *factory.Handler { DeleteFunc: func(obj interface{}) { egressFirewall := obj.(*egressfirewall.EgressFirewall) txn := util.NewNBTxn() - deleteErrors := oc.deleteEgressFirewall(egressFirewall, txn) + deleteErrors := oc.deleteEgressFirewall(egressFirewall) if deleteErrors != nil { klog.Error(deleteErrors) return diff --git a/go-controller/pkg/types/const.go b/go-controller/pkg/types/const.go index 5448e393da8..61dab5b80a6 100644 --- a/go-controller/pkg/types/const.go +++ b/go-controller/pkg/types/const.go @@ -1,7 +1,10 @@ package types +import "time" + const ( - K8sPrefix = "k8s-" + K8sPrefix = "k8s-" + HybridOverlayPrefix = "int-" // K8sMgmtIntfName name to be used as an OVS internal port on the node K8sMgmtIntfName = "ovn-k8s-mp0" @@ -57,8 +60,8 @@ const ( DefaultDenyPriority = 1000 // priority of logical router policies on the OVNClusterRouter - EgressFirewallStartPriority = "10000" - MinimumReservedEgressFirewallPriority = "2000" + EgressFirewallStartPriority = 10000 + MinimumReservedEgressFirewallPriority = 2000 MGMTPortPolicyPriority = "1005" NodeSubnetPolicyPriority = "1004" InterNodePolicyPriority = "1003" @@ -119,4 +122,6 @@ const ( ClusterPortGroupName = "clusterPortGroup" ClusterRtrPortGroupName = "clusterRtrPortGroup" + + OVSDBTimeout = 10 * time.Second ) diff --git a/go-controller/pkg/util/util.go b/go-controller/pkg/util/util.go index 89b33b14a4e..67fb33122e6 100644 --- a/go-controller/pkg/util/util.go +++ b/go-controller/pkg/util/util.go @@ -8,7 +8,11 @@ import ( "strings" "sync" + libovsdbclient "github.com/ovn-org/libovsdb/client" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/nbdb" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/ovn/libovsdbops" "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types" "github.com/urfave/cli/v2" @@ -60,73 +64,6 @@ func GetNodeChassisID() (string, error) { return chassisID, nil } -var updateNodeSwitchLock sync.Mutex - -// UpdateNodeSwitchExcludeIPs should be called after adding the management port -// and after adding the hybrid overlay port, and ensures that each port's IP -// is added to the logical switch's exclude_ips. This prevents ovn-northd log -// spam about duplicate IP addresses. -// See https://github.com/ovn-org/ovn-kubernetes/pull/779 -func UpdateNodeSwitchExcludeIPs(nodeName string, subnet *net.IPNet) error { - if utilnet.IsIPv6CIDR(subnet) { - // We don't exclude any IPs in IPv6 - return nil - } - - updateNodeSwitchLock.Lock() - defer updateNodeSwitchLock.Unlock() - - stdout, stderr, err := RunOVNNbctl("lsp-list", nodeName) - if err != nil { - return fmt.Errorf("failed to list logical switch %q ports: stderr: %q, error: %v", nodeName, stderr, err) - } - - var haveManagementPort, haveHybridOverlayPort bool - lines := strings.Split(stdout, "\n") - for _, line := range lines { - line = strings.TrimSpace(line) - if strings.Contains(line, "("+types.K8sPrefix+nodeName+")") { - haveManagementPort = true - } else if strings.Contains(line, "("+GetHybridOverlayPortName(nodeName)+")") { - // we always need to set to false because we do not reserve the IP on the LSP for HO - haveHybridOverlayPort = false - } - } - - mgmtIfAddr := GetNodeManagementIfAddr(subnet) - hybridOverlayIfAddr := GetNodeHybridOverlayIfAddr(subnet) - var excludeIPs string - if config.HybridOverlay.Enabled { - if haveHybridOverlayPort && haveManagementPort { - // no excluded IPs required - } else if !haveHybridOverlayPort && !haveManagementPort { - // exclude both - excludeIPs = mgmtIfAddr.IP.String() + ".." + hybridOverlayIfAddr.IP.String() - } else if haveHybridOverlayPort { - // exclude management port IP - excludeIPs = mgmtIfAddr.IP.String() - } else if haveManagementPort { - // exclude hybrid overlay port IP - excludeIPs = hybridOverlayIfAddr.IP.String() - } - } else if !haveManagementPort { - // exclude management port IP - excludeIPs = mgmtIfAddr.IP.String() - } - - args := []string{"--", "--if-exists", "remove", "logical_switch", nodeName, "other-config", "exclude_ips"} - if len(excludeIPs) > 0 { - args = []string{"--", "--if-exists", "set", "logical_switch", nodeName, "other-config:exclude_ips=" + excludeIPs} - } - - _, stderr, err = RunOVNNbctl(args...) - if err != nil { - return fmt.Errorf("failed to set node %q switch exclude_ips, "+ - "stderr: %q, error: %v", nodeName, stderr, err) - } - return nil -} - // GetHybridOverlayPortName returns the name of the hybrid overlay switch port // for a given node func GetHybridOverlayPortName(nodeName string) string { @@ -288,3 +225,94 @@ func SliceHasStringItem(slice []string, item string) bool { } return false } + +var updateNodeSwitchLock sync.Mutex + +// UpdateNodeSwitchExcludeIPs should be called after adding the management port +// and after adding the hybrid overlay port, and ensures that each port's IP +// is added to the logical switch's exclude_ips. This prevents ovn-northd log +// spam about duplicate IP addresses. +// See https://github.com/ovn-org/ovn-kubernetes/pull/779 +func UpdateNodeSwitchExcludeIPs(nbClient libovsdbclient.Client, nodeName string, subnet *net.IPNet) error { + if utilnet.IsIPv6CIDR(subnet) { + // We don't exclude any IPs in IPv6 + return nil + } + + updateNodeSwitchLock.Lock() + defer updateNodeSwitchLock.Unlock() + + managmentPort := &nbdb.LogicalSwitchPort{Name: types.K8sPrefix + nodeName} + HOPort := &nbdb.LogicalSwitchPort{Name: types.HybridOverlayPrefix + nodeName} + haveManagementPort := true + haveHybridOverlayPort := true + // Only Query The cache for mp0 and HO LSPs + if err := nbClient.Get(managmentPort); err != nil { + if err != libovsdbclient.ErrNotFound { + return fmt.Errorf("failed to get management port for node %s error: %v", nodeName, err) + } + klog.V(5).Infof("Management port does not exist for node %s", nodeName) + haveManagementPort = false + } + + if err := nbClient.Get(HOPort); err != nil { + if err != libovsdbclient.ErrNotFound { + return fmt.Errorf("failed to get hybrid overlay port for node %s error: %v", nodeName, err) + } + klog.V(5).Infof("Hybridoverlay port does not exist for node %s", nodeName) + haveHybridOverlayPort = false + } + + mgmtIfAddr := GetNodeManagementIfAddr(subnet) + hybridOverlayIfAddr := GetNodeHybridOverlayIfAddr(subnet) + + klog.V(5).Infof("haveMP %v haveHO %v ManagementPortAddress %v HybridOverlayAddressOA %v", haveManagementPort, haveHybridOverlayPort, mgmtIfAddr, hybridOverlayIfAddr) + var excludeIPs string + if config.HybridOverlay.Enabled { + if haveHybridOverlayPort && haveManagementPort { + // no excluded IPs required + } else if !haveHybridOverlayPort && !haveManagementPort { + // exclude both + excludeIPs = mgmtIfAddr.IP.String() + ".." + hybridOverlayIfAddr.IP.String() + } else if haveHybridOverlayPort { + // exclude management port IP + excludeIPs = mgmtIfAddr.IP.String() + } else if haveManagementPort { + // exclude hybrid overlay port IP + excludeIPs = hybridOverlayIfAddr.IP.String() + } + } else if !haveManagementPort { + // exclude management port IP + excludeIPs = mgmtIfAddr.IP.String() + } + + logicalSwitchDes := nbdb.LogicalSwitch{ + Name: nodeName, + OtherConfig: map[string]string{"exclude_ips": excludeIPs}, + } + + opModels := []libovsdbops.OperationModel{ + { + Model: &logicalSwitchDes, + ModelPredicate: func(ls *nbdb.LogicalSwitch) bool { return ls.Name == nodeName }, + OnModelMutations: []interface{}{ + &logicalSwitchDes.OtherConfig, + }, + ErrNotFound: true, + }, + } + + m := libovsdbops.NewModelClient(nbClient) + // If excludeIPs is empty ensure that we have no excludeIPs in the Switch's OtherConfig + if len(excludeIPs) == 0 { + if err := m.Delete(opModels...); err != nil { + return fmt.Errorf("failed to delete otherConfig:exclude_ips from logical switch %s, error: %v", nodeName, err) + } + } else { + if _, err := m.CreateOrUpdate(opModels...); err != nil { + return fmt.Errorf("failed to configure otherConfig:exclude_ips from logical switch %s, error: %v", nodeName, err) + } + } + + return nil +} diff --git a/go-controller/pkg/util/util_unit_test.go b/go-controller/pkg/util/util_unit_test.go index c77ba67b609..897d339ddb9 100644 --- a/go-controller/pkg/util/util_unit_test.go +++ b/go-controller/pkg/util/util_unit_test.go @@ -9,11 +9,15 @@ import ( "testing" "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/nbdb" ovntest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing" + "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/types" mock_k8s_io_utils_exec "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing/mocks/k8s.io/utils/exec" "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/util/mocks" "github.com/stretchr/testify/assert" + + libovsdbtest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing/libovsdb" ) func TestGetLegacyK8sMgmtIntfName(t *testing.T) { @@ -101,148 +105,6 @@ func TestGetNodeChassisID(t *testing.T) { } } -func TestUpdateNodeSwitchExcludeIPs(t *testing.T) { - mockKexecIface := new(mock_k8s_io_utils_exec.Interface) - mockExecRunner := new(mocks.ExecRunner) - mockCmd := new(mock_k8s_io_utils_exec.Cmd) - // below is defined in ovs.go - runCmdExecRunner = mockExecRunner - // note runner is defined in ovs.go file - runner = &execHelper{exec: mockKexecIface} - - tests := []struct { - desc string - inpNodeName string - inpSubnetStr string - errExpected bool - setCfgHybridOvlyEnabled bool - onRetArgsExecUtilsIface []ovntest.TestifyMockHelper - onRetArgsKexecIface []ovntest.TestifyMockHelper - }{ - { - desc: "IPv4 CIDR, ovn-nbctl fails to list logical switch ports", - errExpected: true, - inpNodeName: "ovn-control-plane", - inpSubnetStr: "192.168.1.0/24", - onRetArgsExecUtilsIface: []ovntest.TestifyMockHelper{ - {OnCallMethodName: "RunCmd", OnCallMethodArgType: []string{"*mocks.Cmd", "string", "[]string", "string", "string", "string"}, RetArgList: []interface{}{bytes.NewBuffer([]byte("")), bytes.NewBuffer([]byte("")), fmt.Errorf("RunOVNNbctl error")}}, - }, - onRetArgsKexecIface: []ovntest.TestifyMockHelper{ - {OnCallMethodName: "Command", OnCallMethodArgType: []string{"string", "string", "string", "string"}, RetArgList: []interface{}{mockCmd}}, - }, - }, - { - desc: "IPv6 CIDR, never excludes", - errExpected: false, - inpNodeName: "ovn-control-plane", - inpSubnetStr: "fd04:3e42:4a4e:3381::/64", - }, - { - desc: "IPv4 CIDR, config.HybridOverlayEnable=true, sets haveMangementPort=true, ovn-nbctl command excludeIPs list empty", - errExpected: false, - inpNodeName: "ovn-control-plane", - inpSubnetStr: "192.168.1.0/24", - setCfgHybridOvlyEnabled: true, - onRetArgsExecUtilsIface: []ovntest.TestifyMockHelper{ - { - OnCallMethodName: "RunCmd", - OnCallMethodArgType: []string{"*mocks.Cmd", "string", "[]string", "string", "string", "string"}, - RetArgList: []interface{}{ - // below is output from command --> ovn-nbctl lsp-list ovn-control-plane - bytes.NewBuffer([]byte("7dc3d98a-660a-477b-a6bc-d42904ed59e7 (k8s-ovn-control-plane)\nd23162b4-87b1-4ff8-b5a5-5cb731d822ed (kube-system_coredns-6955765f44-l9jxq)\n1e8cd861-c584-4e38-8c50-7a71a6ae26bb (local-path-storage_local-path-provisioner-85445b74d4-w5ghw)\n8f1b3173-aa43-4014-adcb-36eae52f7502 (stor-ovn-control-plane)")), - bytes.NewBuffer([]byte("")), - nil, - }, - }, - { - OnCallMethodName: "RunCmd", OnCallMethodArgType: []string{"*mocks.Cmd", "string", "[]string", "string", "string", "string", "string", "string", "string", "string", "string"}, RetArgList: []interface{}{bytes.NewBuffer([]byte("")), bytes.NewBuffer([]byte("")), nil}, - }, - }, - onRetArgsKexecIface: []ovntest.TestifyMockHelper{ - {OnCallMethodName: "Command", OnCallMethodArgType: []string{"string", "string", "string", "string", "string", "string", "string", "string", "string"}, RetArgList: []interface{}{mockCmd}}, - {OnCallMethodName: "Command", OnCallMethodArgType: []string{"string", "string", "string", "string", "string", "string", "string", "string", "string"}, RetArgList: []interface{}{mockCmd}}, - }, - }, - { - desc: "IPv4 CIDR, config.HybridOverlayEnable=true, sets haveHybridOverlayPort=false, ovn-nbctl command excludeIPs list populated", - errExpected: false, - inpNodeName: "ovn-control-plane", - inpSubnetStr: "192.168.1.0/24", - setCfgHybridOvlyEnabled: true, - onRetArgsExecUtilsIface: []ovntest.TestifyMockHelper{ - { - OnCallMethodName: "RunCmd", - OnCallMethodArgType: []string{"*mocks.Cmd", "string", "[]string", "string", "string", "string"}, - RetArgList: []interface{}{ - // below is output from command --> ovn-nbctl lsp-list ovn-control-plane - bytes.NewBuffer([]byte("7dc3d98a-660a-477b-a6bc-d42904ed59e7 (int-ovn-control-plane)\nd23162b4-87b1-4ff8-b5a5-5cb731d822ed (kube-system_coredns-6955765f44-l9jxq)\n1e8cd861-c584-4e38-8c50-7a71a6ae26bb (local-path-storage_local-path-provisioner-85445b74d4-w5ghw)\n8f1b3173-aa43-4014-adcb-36eae52f7502 (stor-ovn-control-plane)")), - bytes.NewBuffer([]byte("")), - nil, - }, - }, - { - OnCallMethodName: "RunCmd", OnCallMethodArgType: []string{"*mocks.Cmd", "string", "[]string", "string", "string", "string", "string", "string", "string", "string", "string"}, RetArgList: []interface{}{bytes.NewBuffer([]byte("")), bytes.NewBuffer([]byte("")), nil}, - }, - }, - onRetArgsKexecIface: []ovntest.TestifyMockHelper{ - {OnCallMethodName: "Command", OnCallMethodArgType: []string{"string", "string", "string", "string", "string", "string", "string", "string", "string"}, RetArgList: []interface{}{mockCmd}}, - {OnCallMethodName: "Command", OnCallMethodArgType: []string{"string", "string", "string", "string", "string", "string", "string", "string", "string"}, RetArgList: []interface{}{mockCmd}}, - }, - }, - { - desc: "IPv4 CIDR, haveMangementPort=false, ovn-nbctl command with excludeIPs list populated, returns error ", - errExpected: false, - inpNodeName: "ovn-control-plane", - inpSubnetStr: "192.168.1.0/24", - onRetArgsExecUtilsIface: []ovntest.TestifyMockHelper{ - { - OnCallMethodName: "RunCmd", - OnCallMethodArgType: []string{"*mocks.Cmd", "string", "[]string", "string", "string", "string"}, - RetArgList: []interface{}{ - // below is output from command --> ovn-nbctl lsp-list ovn-control-plane - bytes.NewBuffer([]byte("d23162b4-87b1-4ff8-b5a5-5cb731d822ed (kube-system_coredns-6955765f44-l9jxq)\n1e8cd861-c584-4e38-8c50-7a71a6ae26bb (local-path-storage_local-path-provisioner-85445b74d4-w5ghw)\n8f1b3173-aa43-4014-adcb-36eae52f7502 (stor-ovn-control-plane)")), - bytes.NewBuffer([]byte("")), - nil, - }, - }, - { - OnCallMethodName: "RunCmd", OnCallMethodArgType: []string{"*mocks.Cmd", "string", "[]string", "string", "string", "string", "string", "string", "string", "string", "string"}, RetArgList: []interface{}{bytes.NewBuffer([]byte("")), bytes.NewBuffer([]byte("")), fmt.Errorf("test error")}, - }, - }, - onRetArgsKexecIface: []ovntest.TestifyMockHelper{ - {OnCallMethodName: "Command", OnCallMethodArgType: []string{"string", "string", "string", "string"}, RetArgList: []interface{}{mockCmd}}, - {OnCallMethodName: "Command", OnCallMethodArgType: []string{"string", "string", "string", "string", "string", "string", "string", "string"}, RetArgList: []interface{}{mockCmd}}, - }, - }, - } - - for i, tc := range tests { - t.Run(fmt.Sprintf("%d:%s", i, tc.desc), func(t *testing.T) { - ovntest.ProcessMockFnList(&mockExecRunner.Mock, tc.onRetArgsExecUtilsIface) - ovntest.ProcessMockFnList(&mockKexecIface.Mock, tc.onRetArgsKexecIface) - - _, ipnet, err := net.ParseCIDR(tc.inpSubnetStr) - if err != nil { - t.Fail() - } - var e error - if tc.setCfgHybridOvlyEnabled { - config.HybridOverlay.Enabled = true - e = UpdateNodeSwitchExcludeIPs(tc.inpNodeName, ipnet) - config.HybridOverlay.Enabled = false - } else { - e = UpdateNodeSwitchExcludeIPs(tc.inpNodeName, ipnet) - } - - if tc.errExpected { - assert.Error(t, e) - } - mockExecRunner.AssertExpectations(t) - mockKexecIface.AssertExpectations(t) - }) - } -} - func TestUpdateIPsSlice(t *testing.T) { var tests = []struct { name string @@ -370,3 +232,224 @@ func TestFilterIPsSlice(t *testing.T) { }) } } + +func TestUpdateNodeSwitchExcludeIPs(t *testing.T) { + nodeName := "ovn-control-plane" + + fakeManagementPort := &nbdb.LogicalSwitchPort{ + Name: types.K8sPrefix + nodeName, + UUID: types.K8sPrefix + nodeName + "-uuid", + } + + fakeHoPort := &nbdb.LogicalSwitchPort{ + Name: types.HybridOverlayPrefix + nodeName, + UUID: types.HybridOverlayPrefix + nodeName + "-uuid", + } + + tests := []struct { + desc string + inpSubnetStr string + setCfgHybridOvlyEnabled bool + initialNbdb libovsdbtest.TestSetup + expectedNbdb libovsdbtest.TestSetup + }{ + { + desc: "IPv6 CIDR, never excludes", + initialNbdb: libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + &nbdb.LogicalSwitch{ + Name: nodeName, + UUID: nodeName + "-uuid", + Ports: []string{fakeManagementPort.UUID, fakeHoPort.UUID}, + }, + fakeManagementPort, + fakeHoPort, + }, + }, + expectedNbdb: libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + &nbdb.LogicalSwitch{ + Name: nodeName, + UUID: nodeName + "-uuid", + Ports: []string{fakeManagementPort.UUID, fakeHoPort.UUID}, + }, + fakeManagementPort, + fakeHoPort, + }, + }, + inpSubnetStr: "fd04:3e42:4a4e:3381::/64", + }, + { + desc: "IPv4 CIDR, config.HybridOverlayEnable=true, sets haveMangementPort=true, ovn-nbctl command excludeIPs list empty", + inpSubnetStr: "192.168.1.0/24", + setCfgHybridOvlyEnabled: true, + initialNbdb: libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + &nbdb.LogicalSwitch{ + UUID: nodeName + "-uuid", + Name: nodeName, + Ports: []string{fakeManagementPort.UUID, fakeHoPort.UUID}, + }, + fakeManagementPort, + fakeHoPort, + }, + }, + expectedNbdb: libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + &nbdb.LogicalSwitch{ + UUID: nodeName + "-uuid", + Name: nodeName, + Ports: []string{fakeManagementPort.UUID, fakeHoPort.UUID}, + }, + fakeManagementPort, + fakeHoPort, + }, + }, + }, + { + desc: "IPv4 CIDR, config.HybridOverlayEnable=true, sets haveMangementPort=true, ovn-nbctl command excludeIPs list empty leaves existing otherConfig alone", + inpSubnetStr: "192.168.1.0/24", + setCfgHybridOvlyEnabled: true, + initialNbdb: libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + &nbdb.LogicalSwitch{ + UUID: nodeName + "-uuid", + Name: nodeName, + Ports: []string{fakeManagementPort.UUID, fakeHoPort.UUID}, + OtherConfig: map[string]string{ + "exclude_ips": "192.168.1.3", + "mac_only": "false", + }, + }, + fakeManagementPort, + fakeHoPort, + }, + }, + expectedNbdb: libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + &nbdb.LogicalSwitch{ + UUID: nodeName + "-uuid", + Name: nodeName, + Ports: []string{fakeManagementPort.UUID, fakeHoPort.UUID}, + OtherConfig: map[string]string{ + "mac_only": "false", + }, + }, + fakeManagementPort, + fakeHoPort, + }, + }, + }, + { + desc: "IPv4 CIDR, config.HybridOverlayEnable=true, sets haveHybridOverlayPort=false, ovn-nbctl command excludeIPs list populated", + inpSubnetStr: "192.168.1.0/24", + setCfgHybridOvlyEnabled: true, + initialNbdb: libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + &nbdb.LogicalSwitch{ + UUID: nodeName + "-uuid", + Name: nodeName, + Ports: []string{fakeManagementPort.UUID}, + }, + fakeManagementPort, + }, + }, + expectedNbdb: libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + &nbdb.LogicalSwitch{ + UUID: nodeName + "-uuid", + Name: nodeName, + Ports: []string{fakeManagementPort.UUID}, + OtherConfig: map[string]string{"exclude_ips": "192.168.1.3"}, + }, + fakeManagementPort, + }, + }, + }, + { + desc: "IPv4 CIDR, haveMangementPort=false, ovn-nbctl command with excludeIPs list populated, returns error ", + inpSubnetStr: "192.168.1.0/24", + initialNbdb: libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + &nbdb.LogicalSwitch{ + UUID: nodeName + "-uuid", + Name: nodeName, + Ports: []string{fakeHoPort.UUID}, + }, + fakeHoPort, + }, + }, + expectedNbdb: libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + &nbdb.LogicalSwitch{ + UUID: nodeName + "-uuid", + Name: nodeName, + Ports: []string{fakeHoPort.UUID}, + OtherConfig: map[string]string{"exclude_ips": "192.168.1.2"}, + }, + fakeHoPort, + }, + }, + }, + { + desc: "IPv4 CIDR, config.HybridOverlayEnable=false, sets haveHybridOverlayPort=false and haveManagementPort=false ovn-nbctl command excludeIPs list populated", + inpSubnetStr: "192.168.1.0/24", + setCfgHybridOvlyEnabled: true, + initialNbdb: libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + &nbdb.LogicalSwitch{ + UUID: nodeName + "-uuid", + Name: nodeName, + Ports: []string{}, + }, + }, + }, + expectedNbdb: libovsdbtest.TestSetup{ + NBData: []libovsdbtest.TestData{ + &nbdb.LogicalSwitch{ + UUID: nodeName + "-uuid", + Name: nodeName, + Ports: []string{}, + OtherConfig: map[string]string{"exclude_ips": "192.168.1.2..192.168.1.3"}, + }, + }, + }, + }, + } + for i, tc := range tests { + t.Run(fmt.Sprintf("%d:%s", i, tc.desc), func(t *testing.T) { + stopChan := make(chan struct{}) + + nbClient, _ := libovsdbtest.NewNBTestHarness(tc.initialNbdb, stopChan) + + _, ipnet, err := net.ParseCIDR(tc.inpSubnetStr) + if err != nil { + t.Fail() + } + var e error + if tc.setCfgHybridOvlyEnabled { + config.HybridOverlay.Enabled = true + if e = UpdateNodeSwitchExcludeIPs(nbClient, nodeName, ipnet); e != nil { + t.Fatal(fmt.Errorf("failed to update NodeSwitchExcludeIPs with Hybrid Overlay enabled err: %v", e)) + } + config.HybridOverlay.Enabled = false + } else { + if e = UpdateNodeSwitchExcludeIPs(nbClient, nodeName, ipnet); e != nil { + t.Fatal(fmt.Errorf("failed to update NodeSwitchExcludeIPs with Hybrid Overlay disabled err: %v", e)) + } + + } + + matcher := libovsdbtest.HaveDataIgnoringUUIDs(tc.expectedNbdb.NBData) + success, err := matcher.Match(nbClient) + if !success { + t.Fatal(fmt.Errorf("test: \"%s\" didn't match expected with actual, err: %v", tc.desc, matcher.FailureMessage(nbClient))) + } + if err != nil { + t.Fatal(fmt.Errorf("test: \"%s\" encountered error: %v", tc.desc, err)) + } + + close(stopChan) + }) + } +}