From a5f65cb75d0aa548eaf89414c3cf67885861b560 Mon Sep 17 00:00:00 2001 From: Nadia Pinaeva Date: Mon, 6 Mar 2023 18:11:16 +0100 Subject: [PATCH] Add egress firewall external id to make name+externalIDs unique even when the acl name is cropped. That happens when namespace name is longer than 43 symbols. Signed-off-by: Nadia Pinaeva (cherry picked from commit dafe82b98c33a66bdb1e97ea26a4fa1cd146a141) (cherry picked from commit d74d6ead21d0cd509779848af33dfcf540e38879) Conflicts: go-controller/pkg/ovn/egressfirewall_test.go - use hardcoded address set hash names --- go-controller/pkg/ovn/egressfirewall.go | 8 +- go-controller/pkg/ovn/egressfirewall_test.go | 158 +++++++++++++++++-- 2 files changed, 151 insertions(+), 15 deletions(-) diff --git a/go-controller/pkg/ovn/egressfirewall.go b/go-controller/pkg/ovn/egressfirewall.go index 90c538d3ae..2389c4f787 100644 --- a/go-controller/pkg/ovn/egressfirewall.go +++ b/go-controller/pkg/ovn/egressfirewall.go @@ -25,7 +25,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 { @@ -347,7 +348,10 @@ func (oc *Controller) createEgressFirewallRules(priority int, match, action, ext 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 60f9ec246f..e07dfa2c94 100644 --- a/go-controller/pkg/ovn/egressfirewall_test.go +++ b/go-controller/pkg/ovn/egressfirewall_test.go @@ -195,8 +195,25 @@ 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 + newKeepACL := libovsdbops.BuildACL( + buildEgressFwAclName(namespace1.Name, t.EgressFirewallStartPriority), + nbdb.ACLDirectionToLport, + t.EgressFirewallStartPriority, + "(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102", + 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 @@ -204,9 +221,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 - keepACL.Match = "(ip4.dst == 1.2.3.4/23) && ip4.src == $a10481622940199974102" - // 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 @@ -217,6 +231,7 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() { expectedDatabaseState := []libovsdb.TestData{ otherACL, purgeACL, + newKeepACL, keepACL, nodeSwitch, joinSwitch, @@ -282,7 +297,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" @@ -350,7 +368,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" @@ -427,7 +448,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" @@ -501,7 +525,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" @@ -583,7 +610,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" @@ -660,7 +690,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" @@ -767,7 +800,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" @@ -869,7 +905,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" @@ -954,7 +993,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" @@ -969,6 +1011,96 @@ 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()) + + ipv4ACL1 := libovsdbops.BuildACL( + buildEgressFwAclName(namespace1.Name, t.EgressFirewallStartPriority), + nbdb.ACLDirectionToLport, + t.EgressFirewallStartPriority, + "(ip4.dst == 1.2.3.5/23) && ip4.src == $a9275935520880020708", + 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 == $a9275935520880020708", + 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)) + + 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()) + }) } }) })