Create egress firewall with one db transaction.

Update cleanup test to have dns-based address set as a
leftover, since all acls are commited in one transaction now.

Signed-off-by: Nadia Pinaeva <npinaeva@redhat.com>

go-controller/pkg/ovn/address_set/address_set.go

@@ -321,10 +321,12 @@ func GetDbObjsForAS(dbIDs *libovsdbops.DbObjectIDs, ips []net.IP) (*nbdb.Address
 	v4set = buildAddressSet(dbIDs, ipv4InternalID)
 	uniqIPs := ipsToStringUnique(v4IPs)
 	v4set.Addresses = uniqIPs
+	v4set.UUID = v4set.Name + "-UUID"
 	// v6 address set
 	v6set = buildAddressSet(dbIDs, ipv6InternalID)
 	uniqIPs = ipsToStringUnique(v6IPs)
 	v6set.Addresses = uniqIPs
+	v6set.UUID = v6set.Name + "-UUID"
 	return v4set, v6set
 }
 

go-controller/pkg/ovn/egressfirewall.go

@@ -7,6 +7,7 @@ import (
 	"strings"
 	"sync"
 
+	libovsdb "github.com/ovn-org/libovsdb/ovsdb"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/config"
 	egressfirewallapi "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/egressfirewall/v1"
 	libovsdbops "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/libovsdb/ops"
@@ -305,6 +306,8 @@ func (oc *DefaultNetworkController) updateEgressFirewallStatusWithRetry(egressFi
 
 func (oc *DefaultNetworkController) addEgressFirewallRules(ef *egressFirewall, hashedAddressSetNameIPv4,
 	hashedAddressSetNameIPv6 string, aclLogging *libovsdbutil.ACLLoggingLevels, ruleIDs ...int) error {
+	var ops []libovsdb.Operation
+	var err error
 	for _, rule := range ef.egressRules {
 		// check if only specific rule ids are requested to be added
 		if len(ruleIDs) > 0 {
@@ -365,17 +368,21 @@ func (oc *DefaultNetworkController) addEgressFirewallRules(ef *egressFirewall, h
 		}
 
 		match := generateMatch(hashedAddressSetNameIPv4, hashedAddressSetNameIPv6, matchTargets, rule.ports)
-		err := oc.createEgressFirewallRules(rule.id, match, action, ef.namespace, aclLogging)
+		ops, err = oc.createEgressFirewallACLOps(ops, rule.id, match, action, ef.namespace, aclLogging)
 		if err != nil {
 			return err
 		}
 	}
+	_, err = libovsdbops.TransactAndCheck(oc.nbClient, ops)
+	if err != nil {
+		return fmt.Errorf("failed to transact egressFirewall ACL: %v", err)
+	}
 	return nil
 }
 
-// createEgressFirewallRules uses the previously generated elements and creates the
+// createEgressFirewallACLOps uses the previously generated elements and creates the
 // acls for all node switches
-func (oc *DefaultNetworkController) createEgressFirewallRules(ruleIdx int, match, action, namespace string, aclLogging *libovsdbutil.ACLLoggingLevels) error {
+func (oc *DefaultNetworkController) createEgressFirewallACLOps(ops []libovsdb.Operation, ruleIdx int, match, action, namespace string, aclLogging *libovsdbutil.ACLLoggingLevels) ([]libovsdb.Operation, error) {
 	aclIDs := oc.getEgressFirewallACLDbIDs(namespace, ruleIdx)
 	priority := types.EgressFirewallStartPriority - ruleIdx
 	egressFirewallACL := libovsdbutil.BuildACL(
@@ -387,24 +394,21 @@ func (oc *DefaultNetworkController) createEgressFirewallRules(ruleIdx int, match
 		// since egressFirewall has direction to-lport, set type to ingress
 		libovsdbutil.LportIngress,
 	)
-	ops, err := libovsdbops.CreateOrUpdateACLsOps(oc.nbClient, nil, egressFirewallACL)
+	var err error
+	ops, err = libovsdbops.CreateOrUpdateACLsOps(oc.nbClient, ops, egressFirewallACL)
 	if err != nil {
-		return fmt.Errorf("failed to create egressFirewall ACL %v: %v", egressFirewallACL, err)
+		return ops, fmt.Errorf("failed to create egressFirewall ACL %v: %v", egressFirewallACL, err)
 	}
 
 	// Applying ACLs on types.ClusterPortGroupName is equivalent to applying on every node switch, since
 	// types.ClusterPortGroupName contains management port from every switch.
 	ops, err = libovsdbops.AddACLsToPortGroupOps(oc.nbClient, ops, types.ClusterPortGroupNameBase, egressFirewallACL)
 	if err != nil {
-		return fmt.Errorf("failed to add egressFirewall ACL %v to port group %s: %v",
+		return ops, fmt.Errorf("failed to add egressFirewall ACL %v to port group %s: %v",
 			egressFirewallACL, types.ClusterPortGroupNameBase, err)
 	}
-	_, err = libovsdbops.TransactAndCheck(oc.nbClient, ops)
-	if err != nil {
-		return fmt.Errorf("failed to transact egressFirewall ACL: %v", err)
-	}
 
-	return nil
+	return ops, nil
 }
 
 func (oc *DefaultNetworkController) deleteEgressFirewallRule(namespace string, ruleIdx int) error {

go-controller/pkg/ovn/egressfirewall_dns_test.go

@@ -85,6 +85,29 @@ func generateRR(dnsName, ip, nextQueryTime string) dns.RR {
 	return rr
 }
 
+func setDNSOpsMock(dnsName, retIP string) {
+	mockDnsOps := new(util_mocks.DNSOps)
+	util.SetDNSLibOpsMockInst(mockDnsOps)
+	methods := []ovntest.TestifyMockHelper{
+		{"ClientConfigFromFile", []string{"string"}, []interface{}{}, []interface{}{&dns.ClientConfig{
+			Servers: []string{"1.1.1.1"},
+			Port:    "1234"}, nil}, 0, 1},
+		{"Fqdn", []string{"string"}, []interface{}{}, []interface{}{dnsName}, 0, 1},
+		{"SetQuestion", []string{"*dns.Msg", "string", "uint16"}, []interface{}{}, []interface{}{&dns.Msg{}}, 0, 1},
+		{"Exchange", []string{"*dns.Client", "*dns.Msg", "string"}, []interface{}{}, []interface{}{&dns.Msg{Answer: []dns.RR{generateRR(dnsName, retIP, "300")}}, 500 * time.Second, nil}, 0, 1},
+	}
+	for _, item := range methods {
+		call := mockDnsOps.On(item.OnCallMethodName)
+		for _, arg := range item.OnCallMethodArgType {
+			call.Arguments = append(call.Arguments, mock.AnythingOfType(arg))
+		}
+		for _, ret := range item.RetArgList {
+			call.ReturnArguments = append(call.ReturnArguments, ret)
+		}
+		call.Once()
+	}
+}
+
 func TestAdd(t *testing.T) {
 	mockAddressSetFactoryOps := new(mocks.AddressSetFactory)
 	mockAddressSetOps := new(mocks.AddressSet)

go-controller/pkg/ovn/egressfirewall_test.go

@@ -1184,33 +1184,33 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 				config.Gateway.Mode = gwMode
 				app.Action = func(ctx *cli.Context) error {
 					namespace1 := *newNamespace("namespace1")
+					dnsName := "a.b.c"
+					resolvedIP := "2.2.2.2"
 					egressFirewall := newEgressFirewallObject("default", namespace1.Name, []egressfirewallapi.EgressFirewallRule{
 						{
 							Type: "Deny",
 							To: egressfirewallapi.EgressFirewallDestination{
-								CIDRSelector: "1.2.3.4/23",
-							},
-						},
-						{
-							Type: "Deny",
-							To: egressfirewallapi.EgressFirewallDestination{
-								DNSName: "a.b.c",
+								DNSName: dnsName,
 							},
 						},
 					})
-					startOvn(dbSetup, []v1.Namespace{namespace1}, nil)
-
-					// dns-based rule creation will fail, because addressset factory is nil
-					fakeOVN.controller.egressFirewallDNS = &EgressDNS{
-						dnsEntries:        make(map[string]*dnsEntry),
-						addressSetFactory: nil,
-
-						added:    make(chan struct{}, 1),
-						deleted:  make(chan string, 1),
-						stopChan: make(chan struct{}),
+					initialData = []libovsdbtest.TestData{
+						nodeSwitch,
+						joinSwitch,
+						// delete clusterPortGroup to fail db transaction
+						//clusterPortGroup,
+						clusterRouter,
 					}
+					startOvn(libovsdbtest.TestSetup{
+						NBData: initialData}, []v1.Namespace{namespace1}, nil)
 
-					_, err := fakeOVN.fakeClient.EgressFirewallClient.K8sV1().EgressFirewalls(egressFirewall.Namespace).
+					var err error
+					setDNSOpsMock(dnsName, resolvedIP)
+					fakeOVN.controller.egressFirewallDNS, err = NewEgressDNS(fakeOVN.controller.addressSetFactory,
+						fakeOVN.controller.controllerName, fakeOVN.controller.stopChan)
+					gomega.Expect(err).NotTo(gomega.HaveOccurred())
+
+					_, err = fakeOVN.fakeClient.EgressFirewallClient.K8sV1().EgressFirewalls(egressFirewall.Namespace).
 						Create(context.TODO(), egressFirewall, metav1.CreateOptions{})
 					gomega.Expect(err).NotTo(gomega.HaveOccurred())
 
@@ -1219,28 +1219,14 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 					gomega.Expect(err).NotTo(gomega.HaveOccurred())
 					retry.CheckRetryObjectEventually(efKey, true, fakeOVN.controller.retryEgressFirewalls)
 
-					// check first acl was successfully created
-					asHash, _ := getNsAddrSetHashNames(namespace1.Name)
-					dbIDs := fakeOVN.controller.getEgressFirewallACLDbIDs(egressFirewall.Namespace, 0)
-					acl := libovsdbops.BuildACL(
-						libovsdbutil.GetACLName(dbIDs),
-						nbdb.ACLDirectionToLport,
-						t.EgressFirewallStartPriority,
-						"(ip4.dst == 1.2.3.4/23) && ip4.src == $"+asHash,
-						nbdb.ACLActionDrop,
-						t.OvnACLLoggingMeter,
-						"",
-						false,
-						dbIDs.GetExternalIDs(),
-						nil,
-						t.DefaultACLTier,
-					)
-					acl.UUID = "acl-UUID"
-					clusterPortGroup.ACLs = []string{acl.UUID}
-					expectedDatabaseState := append(initialData, acl)
+					// check dns address set was created
+					addrSet, _ := addressset.GetDbObjsForAS(
+						getEgressFirewallDNSAddrSetDbIDs(dnsName, fakeOVN.controller.controllerName),
+						[]net.IP{net.ParseIP(resolvedIP)})
+					expectedDatabaseState := append(initialData, addrSet)
 					gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState))
 
-					// delete wrong object
+					// delete failed object
 					err = fakeOVN.fakeClient.EgressFirewallClient.K8sV1().EgressFirewalls(egressFirewall.Namespace).
 						Delete(context.TODO(), egressFirewall.Name, metav1.DeleteOptions{})
 					gomega.Expect(err).NotTo(gomega.HaveOccurred())
@@ -1249,11 +1235,8 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 						return retry.CheckRetryObj(efKey, fakeOVN.controller.retryEgressFirewalls)
 					}, time.Second).Should(gomega.BeFalse())
 
-					// check created acl will be cleaned up on delete
-					// acl will be dereferenced, but not deleted by the test server
-					clusterPortGroup.ACLs = []string{}
-					expectedDatabaseState = append(initialData, acl)
-					gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState))
+					// check dns address set is cleaned up on delete
+					gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(initialData))
 					return nil
 				}
 				err := app.Run([]string{app.Name})
@@ -1698,17 +1681,3 @@ var _ = ginkgo.Describe("OVN test basic functions", func() {
 		}
 	})
 })
-
-//helper functions to help test egressfirewallDNS
-
-// Create an EgressDNS object without the Sync function
-// To make it easier to mock EgressFirewall functionality create an egressFirewall
-// without the go routine of the sync function
-
-// GetDNSEntryForTest Gets a dnsEntry from a EgressDNS object for testing
-func (e *EgressDNS) GetDNSEntryForTest(dnsName string) (map[string]struct{}, []net.IP, addressset.AddressSet, error) {
-	if e.dnsEntries[dnsName] == nil {
-		return nil, nil, nil, fmt.Errorf("there is no dnsEntry for dnsName: %s", dnsName)
-	}
-	return e.dnsEntries[dnsName].namespaces, e.dnsEntries[dnsName].dnsResolves, e.dnsEntries[dnsName].dnsAddressSet, nil
-}