Merge pull request #1834 from npinaeva/ocpbugs-18057

OCPBUGS-18057: [release-4.13] Create egress firewall with one db transaction

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

@@ -291,10 +291,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

@@ -6,6 +6,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"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/libovsdbops"
@@ -334,6 +335,8 @@ func (oc *DefaultNetworkController) updateEgressFirewallStatusWithRetry(egressfi
 
 func (oc *DefaultNetworkController) addEgressFirewallRules(ef *egressFirewall, hashedAddressSetNameIPv4,
 	hashedAddressSetNameIPv6 string, efStartPriority int, aclLogging *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 {
@@ -394,17 +397,21 @@ func (oc *DefaultNetworkController) addEgressFirewallRules(ef *egressFirewall, h
 		}
 
 		match := generateMatch(hashedAddressSetNameIPv4, hashedAddressSetNameIPv6, matchTargets, rule.ports)
-		err := oc.createEgressFirewallRules(efStartPriority-rule.id, match, action, ef.namespace, aclLogging)
+		ops, err = oc.createEgressFirewallACLOps(ops, efStartPriority-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(priority int, match, action, externalID string, aclLogging *ACLLoggingLevels) error {
+func (oc *DefaultNetworkController) createEgressFirewallACLOps(ops []libovsdb.Operation, priority int, match, action, externalID string, aclLogging *ACLLoggingLevels) ([]libovsdb.Operation, error) {
 	// a name is needed for logging purposes - the name must be unique, so make it
 	// egressFirewall_<namespace name>_<priority>
 	aclName := buildEgressFwAclName(externalID, priority)
@@ -422,24 +429,21 @@ func (oc *DefaultNetworkController) createEgressFirewallRules(priority int, matc
 			egressFirewallACLPriorityKey: fmt.Sprintf("%d", priority),
 		},
 	)
-	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.ClusterPortGroupName, 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.ClusterPortGroupName, 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(name string) 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

@@ -15,7 +15,6 @@ import (
 	egressfirewallapi "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/crd/egressfirewall/v1"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/libovsdbops"
 	"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/retry"
 	"github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing/libovsdb"
 	libovsdbtest "github.com/ovn-org/ovn-kubernetes/go-controller/pkg/testing/libovsdb"
@@ -1392,21 +1391,24 @@ 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,
 							},
 						},
 					})
-					fakeOVN.startWithDBSetup(dbSetup,
+					initialData = []libovsdbtest.TestData{
+						nodeSwitch,
+						joinSwitch,
+						// delete clusterPortGroup to fail db transaction
+						//clusterPortGroup,
+						clusterRouter,
+					}
+					fakeOVN.startWithDBSetup(libovsdbtest.TestSetup{NBData: initialData},
 						&v1.NamespaceList{
 							Items: []v1.Namespace{
 								namespace1,
@@ -1417,15 +1419,10 @@ var _ = ginkgo.Describe("OVN EgressFirewall Operations", func() {
 					err = fakeOVN.controller.WatchEgressFirewall()
 					gomega.Expect(err).NotTo(gomega.HaveOccurred())
 
-					// 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{}),
-						deleted:  make(chan string, 1),
-						stopChan: make(chan struct{}),
-					}
+					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{})
@@ -1436,30 +1433,12 @@ 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)
-					acl := libovsdbops.BuildACL(
-						buildEgressFwAclName(namespace1.Name, t.EgressFirewallStartPriority),
-						nbdb.ACLDirectionToLport,
-						t.EgressFirewallStartPriority,
-						"(ip4.dst == 1.2.3.4/23) && ip4.src == $"+asHash,
-						nbdb.ACLActionDrop,
-						t.OvnACLLoggingMeter,
-						"",
-						false,
-						map[string]string{
-							egressFirewallACLExtIdKey:    namespace1.Name,
-							egressFirewallACLPriorityKey: fmt.Sprintf("%d", t.EgressFirewallStartPriority),
-						},
-						nil,
-					)
+					// check dns address set was created, db didn't change
+					addrSetIDs := getEgressFirewallDNSAddrSetDbIDs(dnsName, fakeOVN.controller.controllerName)
+					fakeOVN.asf.EventuallyExpectAddressSetWithIPs(addrSetIDs, []string{resolvedIP})
+					gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(initialData))
 
-					acl.UUID = "acl-UUID"
-					clusterPortGroup.ACLs = []string{acl.UUID}
-					expectedDatabaseState := append(initialData, acl)
-					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())
@@ -1468,11 +1447,9 @@ 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
+					fakeOVN.asf.EventuallyExpectNoAddressSet(addrSetIDs)
+					gomega.Eventually(fakeOVN.nbClient).Should(libovsdbtest.HaveData(initialData))
 					return nil
 				}
 				err := app.Run([]string{app.Name})
@@ -1917,17 +1894,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
-}