diff --git a/go-controller/pkg/libovsdbops/acl.go b/go-controller/pkg/libovsdbops/acl.go index 8a8b2de7a5f..ce32fd78d5e 100644 --- a/go-controller/pkg/libovsdbops/acl.go +++ b/go-controller/pkg/libovsdbops/acl.go @@ -102,6 +102,11 @@ func CreateOrUpdateACLsOps(nbClient libovsdbclient.Client, ops []libovsdb.Operat for i := range acls { // can't use i in the predicate, for loop replaces it in-memory acl := acls[i] + // ensure names are truncated (let's cover our bases from snippets that don't call BuildACL and call this directly) + if acl.Name != nil { + // node ACLs won't have names set + *acl.Name = fmt.Sprintf("%.63s", *acl.Name) + } opModel := operationModel{ Model: acl, ModelPredicate: func(item *nbdb.ACL) bool { return isEquivalentACL(item, acl) }, diff --git a/go-controller/pkg/ovn/egressfirewall.go b/go-controller/pkg/ovn/egressfirewall.go index 3c001861b66..9fd532e360f 100644 --- a/go-controller/pkg/ovn/egressfirewall.go +++ b/go-controller/pkg/ovn/egressfirewall.go @@ -28,7 +28,8 @@ const ( egressFirewallAppliedCorrectly = "EgressFirewall Rules applied" egressFirewallAddError = "EgressFirewall Rules not correctly added" // egressFirewallACLExtIdKey external ID key for egress firewall ACLs - egressFirewallACLExtIdKey = "egressFirewall" + egressFirewallACLExtIdKey = "egressFirewall" + egressFirewallACLPriorityKey = "priority" ) type egressFirewall struct { @@ -403,7 +404,10 @@ func (oc *DefaultNetworkController) createEgressFirewallRules(priority int, matc aclLogging, // since egressFirewall has direction to-lport, set type to ingress lportIngress, - map[string]string{egressFirewallACLExtIdKey: externalID}, + map[string]string{ + egressFirewallACLExtIdKey: externalID, + egressFirewallACLPriorityKey: fmt.Sprintf("%d", priority), + }, ) ops, err := libovsdbops.CreateOrUpdateACLsOps(oc.nbClient, nil, egressFirewallACL) if err != nil { diff --git a/go-controller/pkg/ovn/egressfirewall_test.go b/go-controller/pkg/ovn/egressfirewall_test.go index 503eac974a5..f61cd04ed79 100644 --- a/go-controller/pkg/ovn/egressfirewall_test.go +++ b/go-controller/pkg/ovn/egressfirewall_test.go @@ -202,8 +202,26 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() { // Both ACLs will be removed from the node switch nodeSwitch.ACLs = nil + // keepACL will be re-created with the new external ID + asHash, _ := getNsAddrSetHashNames(namespace1.Name) + newKeepACL := libovsdbops.BuildACL( + buildEgressFwAclName(namespace1.Name, t.EgressFirewallStartPriority), + nbdb.ACLDirectionToLport, + t.EgressFirewallStartPriority, + "(ip4.dst == 1.2.3.4/23) && ip4.src == $"+asHash, + nbdb.ACLActionAllow, + t.OvnACLLoggingMeter, + "", + false, + map[string]string{ + egressFirewallACLExtIdKey: "namespace1", + egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority), + }, + nil, + ) + newKeepACL.UUID = "newKeepACL-UUID" // keepACL will be added to the clusterPortGroup - clusterPortGroup.ACLs = []string{keepACL.UUID} + clusterPortGroup.ACLs = []string{newKeepACL.UUID} // Direction of both ACLs will be converted to keepACL.Direction = nbdb.ACLDirectionToLport @@ -211,10 +229,6 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() { keepACL.Name = &newName // check severity was reset from default to nil keepACL.Severity = nil - // subnet exclusion will be deleted - asHash, _ := getNsAddrSetHashNames(namespace1.Name) - keepACL.Match = "(ip4.dst == 1.2.3.4/23) && ip4.src == $" + asHash - // purgeACL ACL will be deleted when test server starts deleting dereferenced ACLs // for now we need to update its fields, since it is present in the db purgeACL.Direction = nbdb.ACLDirectionToLport @@ -225,6 +239,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() { expectedDatabaseState := []libovsdb.TestData{ otherACL, purgeACL, + newKeepACL, keepACL, nodeSwitch, joinSwitch, @@ -290,7 +305,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() { t.OvnACLLoggingMeter, "", false, - map[string]string{egressFirewallACLExtIdKey: "namespace1"}, + map[string]string{ + egressFirewallACLExtIdKey: "namespace1", + egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority), + }, nil, ) ipv4ACL.UUID = "ipv4ACL-UUID" @@ -358,7 +376,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() { t.OvnACLLoggingMeter, "", false, - map[string]string{egressFirewallACLExtIdKey: "namespace1"}, + map[string]string{ + egressFirewallACLExtIdKey: "namespace1", + egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority), + }, nil, ) ipv6ACL.UUID = "ipv6ACL-UUID" @@ -435,7 +456,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() { t.OvnACLLoggingMeter, "", false, - map[string]string{egressFirewallACLExtIdKey: "namespace1"}, + map[string]string{ + egressFirewallACLExtIdKey: "namespace1", + egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority), + }, nil, ) udpACL.UUID = "udpACL-UUID" @@ -510,7 +534,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() { t.OvnACLLoggingMeter, "", false, - map[string]string{egressFirewallACLExtIdKey: "namespace1"}, + map[string]string{ + egressFirewallACLExtIdKey: "namespace1", + egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority), + }, nil, ) ipv4ACL.UUID = "ipv4ACL-UUID" @@ -593,7 +620,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() { t.OvnACLLoggingMeter, "", false, - map[string]string{egressFirewallACLExtIdKey: "namespace1"}, + map[string]string{ + egressFirewallACLExtIdKey: "namespace1", + egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority), + }, nil, ) ipv4ACL.UUID = "ipv4ACL-UUID" @@ -819,7 +849,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() { t.OvnACLLoggingMeter, "", false, - map[string]string{"egressFirewall": "namespace1"}, + map[string]string{ + egressFirewallACLExtIdKey: "namespace1", + egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority), + }, nil, ) ipv4ACL.UUID = "ipv4ACL-UUID" @@ -898,7 +931,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() { t.OvnACLLoggingMeter, "", false, - map[string]string{egressFirewallACLExtIdKey: "namespace1"}, + map[string]string{ + egressFirewallACLExtIdKey: "namespace1", + egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority), + }, nil, ) ipv4ACL.UUID = "ipv4ACL-UUID" @@ -1006,7 +1042,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() { t.OvnACLLoggingMeter, "", false, - map[string]string{egressFirewallACLExtIdKey: "namespace1"}, + map[string]string{ + egressFirewallACLExtIdKey: "namespace1", + egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority), + }, nil, ) ipv4ACL.UUID = "ipv4ACL-UUID" @@ -1109,7 +1148,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() { t.OvnACLLoggingMeter, "", false, - map[string]string{egressFirewallACLExtIdKey: "namespace1"}, + map[string]string{ + egressFirewallACLExtIdKey: "namespace1", + egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority), + }, nil, ) ipv4ACL.UUID = "ipv4ACL-UUID" @@ -1197,7 +1239,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() { t.OvnACLLoggingMeter, "", false, - map[string]string{"egressFirewall": namespace1.Name}, + map[string]string{ + egressFirewallACLExtIdKey: "namespace1", + egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority), + }, nil, ) ipv4ACL.UUID = "ipv4ACL-UUID" @@ -1267,7 +1312,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() { t.OvnACLLoggingMeter, "", false, - map[string]string{egressFirewallACLExtIdKey: "namespace1"}, + map[string]string{ + egressFirewallACLExtIdKey: "namespace1", + egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority), + }, nil, ) acl.UUID = "acl-UUID" @@ -1282,6 +1330,103 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() { err := app.Run([]string{app.Name}) gomega.Expect(err).NotTo(gomega.HaveOccurred()) }) + ginkgo.It(fmt.Sprintf("correctly creates an egressfirewall for namespace name > 43 symbols, gateway mode %s", gwMode), func() { + app.Action = func(ctx *cli.Context) error { + // 52 characters namespace + namespace1 := *newNamespace("abcdefghigklmnopqrstuvwxyzabcdefghigklmnopqrstuvwxyz") + egressFirewall := newEgressFirewallObject("default", namespace1.Name, []egressfirewallapi.EgressFirewallRule{ + { + Type: "Allow", + To: egressfirewallapi.EgressFirewallDestination{ + CIDRSelector: "1.2.3.5/23", + }, + }, + { + Type: "Allow", + To: egressfirewallapi.EgressFirewallDestination{ + CIDRSelector: "2.2.3.5/23", + }, + }, + }) + + fakeOVN.startWithDBSetup(dbSetup, + &egressfirewallapi.EgressFirewallList{ + Items: []egressfirewallapi.EgressFirewall{ + *egressFirewall, + }, + }, + &v1.NamespaceList{ + Items: []v1.Namespace{ + namespace1, + }, + }) + + err := fakeOVN.controller.WatchNamespaces() + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + err = fakeOVN.controller.WatchEgressFirewall() + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + + asHash, _ := getNsAddrSetHashNames(namespace1.Name) + ipv4ACL1 := libovsdbops.BuildACL( + buildEgressFwAclName(namespace1.Name, t.EgressFirewallStartPriority), + nbdb.ACLDirectionToLport, + t.EgressFirewallStartPriority, + "(ip4.dst == 1.2.3.5/23) && ip4.src == $"+asHash, + nbdb.ACLActionAllow, + t.OvnACLLoggingMeter, + "", + false, + map[string]string{ + egressFirewallACLExtIdKey: namespace1.Name, + egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority), + }, + nil, + ) + ipv4ACL1.UUID = "ipv4ACL1-UUID" + ipv4ACL2 := libovsdbops.BuildACL( + buildEgressFwAclName(namespace1.Name, t.EgressFirewallStartPriority-1), + nbdb.ACLDirectionToLport, + t.EgressFirewallStartPriority-1, + "(ip4.dst == 2.2.3.5/23) && ip4.src == $"+asHash, + nbdb.ACLActionAllow, + t.OvnACLLoggingMeter, + "", + false, + map[string]string{ + egressFirewallACLExtIdKey: namespace1.Name, + egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority-1), + }, + nil, + ) + ipv4ACL2.UUID = "ipv4ACL2-UUID" + + // new ACL will be added to the port group + clusterPortGroup.ACLs = []string{ipv4ACL1.UUID, ipv4ACL2.UUID} + expectedDatabaseState := append(initialData, ipv4ACL1, ipv4ACL2) + + gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState)) + + // NOTE: syncEgressFirewall is not calling libovsdbops.BuildACL and directly calls CreateOrUpdateACLs + // This part of test ensures syncEgressFirewall code path is tested well and that we truncate the ACL names correctly + err = fakeOVN.controller.syncEgressFirewall([]interface{}{*egressFirewall}) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + gomega.Eventually(fakeOVN.nbClient).Should(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()) + + // ACL should be removed from the port group egfw is deleted + clusterPortGroup.ACLs = []string{} + // this ACL will be deleted when test server starts deleting dereferenced ACLs + expectedDatabaseState = append(initialData, ipv4ACL1, ipv4ACL2) + gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState)) + + return nil + } + + err := app.Run([]string{app.Name}) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + }) } }) }) diff --git a/go-controller/pkg/ovn/master.go b/go-controller/pkg/ovn/master.go index 248fed6b186..b37833f6a3e 100644 --- a/go-controller/pkg/ovn/master.go +++ b/go-controller/pkg/ovn/master.go @@ -14,6 +14,7 @@ import ( "k8s.io/klog/v2" utilnet "k8s.io/utils/net" + 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/kube" "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/libovsdbops" @@ -118,23 +119,43 @@ func (oc *DefaultNetworkController) SetupMaster(existingNodeNames []string) erro } oc.defaultCOPPUUID = *(logicalRouter.Copp) - // Create a cluster-wide port group that all logical switch ports are part of - pg := libovsdbops.BuildPortGroup(types.ClusterPortGroupName, types.ClusterPortGroupName, nil, nil) - err = libovsdbops.CreateOrUpdatePortGroups(oc.nbClient, pg) - if err != nil { - klog.Errorf("Failed to create cluster port group: %v", err) + pg := &nbdb.PortGroup{ + Name: types.ClusterPortGroupName, + } + pg, err = libovsdbops.GetPortGroup(oc.nbClient, pg) + if err != nil && err != libovsdbclient.ErrNotFound { return err } + if pg == nil { + // we didn't find an existing clusterPG, let's create a new empty PG (fresh cluster install) + // Create a cluster-wide port group that all logical switch ports are part of + pg := libovsdbops.BuildPortGroup(types.ClusterPortGroupName, types.ClusterPortGroupName, nil, nil) + err = libovsdbops.CreateOrUpdatePortGroups(oc.nbClient, pg) + if err != nil { + klog.Errorf("Failed to create cluster port group: %v", err) + return err + } + } - // Create a cluster-wide port group with all node-to-cluster router - // logical switch ports. Currently the only user is multicast but it might - // be used for other features in the future. - pg = libovsdbops.BuildPortGroup(types.ClusterRtrPortGroupName, types.ClusterRtrPortGroupName, nil, nil) - err = libovsdbops.CreateOrUpdatePortGroups(oc.nbClient, pg) - if err != nil { - klog.Errorf("Failed to create cluster port group: %v", err) + pg = &nbdb.PortGroup{ + Name: types.ClusterRtrPortGroupName, + } + pg, err = libovsdbops.GetPortGroup(oc.nbClient, pg) + if err != nil && err != libovsdbclient.ErrNotFound { return err } + if pg == nil { + // we didn't find an existing clusterRtrPG, let's create a new empty PG (fresh cluster install) + // Create a cluster-wide port group with all node-to-cluster router + // logical switch ports. Currently the only user is multicast but it might + // be used for other features in the future. + pg = libovsdbops.BuildPortGroup(types.ClusterRtrPortGroupName, types.ClusterRtrPortGroupName, nil, nil) + err = libovsdbops.CreateOrUpdatePortGroups(oc.nbClient, pg) + if err != nil { + klog.Errorf("Failed to create cluster port group: %v", err) + return err + } + } // If supported, enable IGMP relay on the router to forward multicast // traffic between nodes.