Merge pull request #2169 from JacobTanenbaum/BZ1947917

fix deadlock in EgressFirewall DNS code
ovn-org · Apr 15, 2021 · a99cc89 · a99cc89
2 parents bba0191 + c1b58c4
commit a99cc89
Show file tree

Hide file tree

Showing 2 changed files with 117 additions and 19 deletions.
diff --git a/go-controller/pkg/ovn/egressfirewall_dns.go b/go-controller/pkg/ovn/egressfirewall_dns.go
@@ -24,7 +24,7 @@ type EgressDNS struct {
 	addressSetFactory addressset.AddressSetFactory
 
 	// Report change when Add operation is done
-	added          chan string
+	added          chan struct{}
 	stopChan       chan struct{}
 	controllerStop <-chan struct{}
 }
@@ -50,7 +50,7 @@ func NewEgressDNS(addressSetFactory addressset.AddressSetFactory, controllerStop
 		dnsEntries:        make(map[string]*dnsEntry),
 		addressSetFactory: addressSetFactory,
 
-		added:          make(chan string, 1),
+		added:          make(chan struct{}),
 		stopChan:       make(chan struct{}),
 		controllerStop: controllerStop,
 	}
@@ -75,7 +75,7 @@ func (e *EgressDNS) Add(namespace, dnsName string) (addressset.AddressSet, error
 			return nil, fmt.Errorf("cannot create addressSet for %s: %v", dnsName, err)
 		}
 		e.dnsEntries[dnsName] = &dnsEntry
-		e.signalAdded(dnsName)
+		go e.addToDNS(dnsName)
 	}
 	e.dnsEntries[dnsName].namespaces[namespace] = struct{}{}
 	return e.dnsEntries[dnsName].dnsAddressSet, nil
@@ -124,12 +124,31 @@ func (e *EgressDNS) updateEntryForName(dnsName string) error {
 	return nil
 }
 
+// addToDNS takes the dnsName adds it to the underlying dns resolver and
+// performs the first update. After completing that signals the
+// thread performing periodic updates that a new DNS name has been added and
+// so that it can updates GetNextQueryTime() if needed
+func (e *EgressDNS) addToDNS(dnsName string) {
+	if err := e.dns.Add(dnsName); err != nil {
+		utilruntime.HandleError(err)
+	}
+	if err := e.updateEntryForName(dnsName); err != nil {
+		utilruntime.HandleError(err)
+	}
+	// No need to block waiting to signal the add.
+	select {
+	case e.added <- struct{}{}:
+		klog.V(5).Infof("Recalculation of next query time requested")
+	default:
+		klog.V(5).Infof("Recalculation of next query time already requested")
+	}
+}
+
 // Run spawns a goroutine that handles updates to the dns entries for dnsNames used in
 // EgressFirewalls. The loop runs after receiving one of two signals
-// 1. a new dnsName has been added and a signal is sent to add the new DNS name, if an
-//    EgressFirewall uses a DNS name already added by another egressFirewall the previous
-//    entry is used
-// 2. If the defaultInterval has run (30 min) without updating the DNS server is manually queried
+// 1. time.After(durationTillNextQuery) times out and the dnsName with the lowest ttl is checked
+//    and the durationTillNextQuery is updated
+// 2. e.added is recived and durationTillNextQuery is recomputed
 func (e *EgressDNS) Run(defaultInterval time.Duration) {
 	var dnsName string
 	var ttl time.Time
@@ -138,15 +157,12 @@ func (e *EgressDNS) Run(defaultInterval time.Duration) {
 	durationTillNextQuery := defaultInterval
 	go func() {
 		for {
-			// Wait for the given duration or until something gets added
+			// perform periodic updates on dnsNames as each ttl runs out, checking for updates at
+			// least every defaultInterval. Update durationTillNextQuery everytime a new DNS name gets
+			// added
 			select {
-			case dnsName := <-e.added:
-				if err := e.dns.Add(dnsName); err != nil {
-					utilruntime.HandleError(err)
-				}
-				if err := e.updateEntryForName(dnsName); err != nil {
-					utilruntime.HandleError(err)
-				}
+			case <-e.added:
+				//on update need to check if the GetNextQueryTime has changed
 			case <-time.After(durationTillNextQuery):
 				if len(dnsName) > 0 {
 					if _, err := e.Update(dnsName); err != nil {
@@ -177,7 +193,3 @@ func (e *EgressDNS) Run(defaultInterval time.Duration) {
 func (e *EgressDNS) Shutdown() {
 	close(e.stopChan)
 }
-
-func (e *EgressDNS) signalAdded(dnsName string) {
-	e.added <- dnsName
-}
diff --git a/test/e2e/e2e.go b/test/e2e/e2e.go
@@ -15,6 +15,7 @@ import (
 
 	"github.com/onsi/ginkgo"
 	"github.com/onsi/ginkgo/extensions/table"
+	"github.com/onsi/gomega"
 	"github.com/pkg/errors"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -1391,6 +1392,91 @@ spec:
 			framework.Failf("Failed to curl the remote host %s from container %s on node %s: %v", exFWPermitTcpWwwDest, ovnContainer, serverNodeInfo.name, err)
 		}
 	})
+	ginkgo.It("Should validate the egress firewall DNS does not deadlock when adding many dnsNames", func() {
+		frameworkNsFlag := fmt.Sprintf("--namespace=%s", f.Namespace.Name)
+		var egressFirewallConfig = fmt.Sprintf(`kind: EgressFirewall
+apiVersion: k8s.ovn.org/v1
+metadata:
+  name: default
+  namespace: %s
+spec:
+  egress:
+  - type: Allow
+    to:
+      cidrSelector: 8.8.8.8/32
+  - type: Allow
+    to:
+      dnsName: www.test1.com
+  - type: Allow
+    to:
+      dnsName: www.test2.com
+  - type: Allow
+    to:
+      dnsName: www.test3.com
+  - type: Allow
+    to:
+      dnsName: www.test4.com
+  - type: Allow
+    to:
+      dnsName: www.test5.com
+  - type: Allow
+    to:
+      dnsName: www.test6.com
+  - type: Allow
+    to:
+      dnsName: www.test7.com
+  - type: Allow
+    to:
+      dnsName: www.test8.com
+  - type: Allow
+    to:
+      dnsName: www.test9.com
+  - type: Allow
+    to:
+      dnsName: www.test10.com
+  - type: Allow
+    to:
+      dnsName: www.test11.com
+  - type: Allow
+    to:
+      dnsName: www.test12.com
+  - type: Allow
+    to:
+      cidrSelector: 1.1.1.0/24
+    ports:
+      - protocol: TCP
+        port: 80
+  - type: Deny
+    to:
+      cidrSelector: 0.0.0.0/0
+`, f.Namespace.Name)
+		// write the config to a file for application and defer the removal
+		if err := ioutil.WriteFile(egressFirewallYamlFile, []byte(egressFirewallConfig), 0644); err != nil {
+			framework.Failf("Unable to write CRD config to disk: %v", err)
+		}
+		defer func() {
+			if err := os.Remove(egressFirewallYamlFile); err != nil {
+				framework.Logf("Unable to remove the CRD config from disk: %v", err)
+			}
+		}()
+		// create the CRD config parameters
+		applyArgs := []string{
+			"apply",
+			frameworkNsFlag,
+			"-f",
+			egressFirewallYamlFile,
+		}
+		framework.Logf("Applying EgressFirewall configuration: %s ", applyArgs)
+		// apply the egress firewall configuration
+		framework.RunKubectlOrDie(f.Namespace.Name, applyArgs...)
+		gomega.Eventually(func() bool {
+			output, err := framework.RunKubectl(f.Namespace.Name, "get", "egressfirewall", "default")
+			if err != nil {
+				framework.Failf("could not get the egressfirewall default in namespace: %s", f.Namespace.Name)
+			}
+			return strings.Contains(output, "EgressFirewall Rules applied")
+		}, 30*time.Second).Should(gomega.BeTrue())
+	})
 })
 
 // Validate pods can reach a network running in a container's looback address via