From c2c5c5cd70f8b311e2e124ca3b5cc1d71c601b7b Mon Sep 17 00:00:00 2001
From: Joona Hoikkala <joohoi@users.noreply.github.com>
Date: Wed, 31 Oct 2018 00:54:51 +0200
Subject: [PATCH] Better error handling in goroutines (#122)

* More robust goroutine error handling using channels

* Fix tests and make startup log msg saner

* Clarification to README and config file
---
 .travis.yml  |  1 +
 README.md    |  4 +++-
 config.cfg   |  4 +++-
 main.go      | 43 ++++++++++++++++++++++++++++++++++++-------
 main_test.go | 13 ++++++++++++-
 util.go      |  9 ---------
 6 files changed, 55 insertions(+), 19 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 2a63da5..e2211a7 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -1,6 +1,7 @@
 language: go
 go:
   - 1.9
+  - 1.11
 env:
   - "PATH=/home/travis/gopath/bin:$PATH"
 before_install:
diff --git a/README.md b/README.md
index 5efbfd5..862c190 100644
--- a/README.md
+++ b/README.md
@@ -212,7 +212,9 @@ $ dig @auth.example.org d420c923-bbd7-4056-ab64-c3ca54c9b3cf.auth.example.org
 
 ```bash
 [general]
-# dns interface
+# DNS interface. Note that systemd-resolved may reserve port 53 on 127.0.0.53
+# In this case acme-dns will error out and you will need to define the listening interface
+# for example: listen = "127.0.0.1:53"
 listen = ":53"
 # protocol, "udp", "udp4", "udp6" or "tcp", "tcp4", "tcp6"
 protocol = "udp"
diff --git a/config.cfg b/config.cfg
index 531c964..d93d3fb 100644
--- a/config.cfg
+++ b/config.cfg
@@ -1,5 +1,7 @@
 [general]
-# dns interface
+# DNS interface. Note that systemd-resolved may reserve port 53 on 127.0.0.53
+# In this case acme-dns will error out and you will need to define the listening interface
+# for example: listen = "127.0.0.1:53"
 listen = ":53"
 # protocol, "udp", "udp4", "udp6" or "tcp", "tcp4", "tcp6"
 protocol = "udp"
diff --git a/main.go b/main.go
index 67b6cd8..063f7b2 100644
--- a/main.go
+++ b/main.go
@@ -11,6 +11,7 @@ import (
 	"syscall"
 
 	"github.com/julienschmidt/httprouter"
+	"github.com/miekg/dns"
 	"github.com/rs/cors"
 	log "github.com/sirupsen/logrus"
 	"golang.org/x/crypto/acme/autocert"
@@ -55,16 +56,41 @@ func main() {
 	DB = newDB
 	defer DB.Close()
 
+	// Error channel for servers
+	errChan := make(chan error, 1)
+
 	// DNS server
-	startDNS(Config.General.Listen, Config.General.Proto)
+	dnsServer := setupDNSServer()
+	go startDNS(dnsServer, errChan)
 
 	// HTTP API
-	startHTTPAPI()
+	go startHTTPAPI(errChan)
 
+	// block waiting for error
+	select {
+	case err = <-errChan:
+		if err != nil {
+			log.Fatal(err)
+		}
+	}
 	log.Debugf("Shutting down...")
 }
 
-func startHTTPAPI() {
+func startDNS(server *dns.Server, errChan chan error) {
+	// DNS server part
+	dns.HandleFunc(".", handleRequest)
+	log.WithFields(log.Fields{"addr": Config.General.Listen}).Info("Listening DNS")
+	err := server.ListenAndServe()
+	if err != nil {
+		errChan <- err
+	}
+}
+
+func setupDNSServer() *dns.Server {
+	return &dns.Server{Addr: Config.General.Listen, Net: Config.General.Proto}
+}
+
+func startHTTPAPI(errChan chan error) {
 	// Setup http logger
 	logger := log.New()
 	logwriter := logger.Writer()
@@ -90,7 +116,7 @@ func startHTTPAPI() {
 	cfg := &tls.Config{
 		MinVersion: tls.VersionTLS12,
 	}
-
+	var err error
 	switch Config.API.TLS {
 	case "letsencrypt":
 		m := autocert.Manager{
@@ -109,7 +135,7 @@ func startHTTPAPI() {
 			ErrorLog:  stdlog.New(logwriter, "", 0),
 		}
 		log.WithFields(log.Fields{"host": host, "domain": Config.API.Domain}).Info("Listening HTTPS, using certificate from autocert")
-		log.Fatal(srv.ListenAndServeTLS("", ""))
+		err = srv.ListenAndServeTLS("", "")
 	case "cert":
 		srv := &http.Server{
 			Addr:      host,
@@ -118,9 +144,12 @@ func startHTTPAPI() {
 			ErrorLog:  stdlog.New(logwriter, "", 0),
 		}
 		log.WithFields(log.Fields{"host": host}).Info("Listening HTTPS")
-		log.Fatal(srv.ListenAndServeTLS(Config.API.TLSCertFullchain, Config.API.TLSCertPrivkey))
+		err = srv.ListenAndServeTLS(Config.API.TLSCertFullchain, Config.API.TLSCertPrivkey)
 	default:
 		log.WithFields(log.Fields{"host": host}).Info("Listening HTTP")
-		log.Fatal(http.ListenAndServe(host, c.Handler(api)))
+		err = http.ListenAndServe(host, c.Handler(api))
+	}
+	if err != nil {
+		errChan <- err
 	}
 }
diff --git a/main_test.go b/main_test.go
index 54348db..23771ad 100644
--- a/main_test.go
+++ b/main_test.go
@@ -7,6 +7,7 @@ import (
 	logrustest "github.com/sirupsen/logrus/hooks/test"
 	"io/ioutil"
 	"os"
+	"sync"
 	"testing"
 )
 
@@ -42,7 +43,15 @@ func TestMain(m *testing.M) {
 		_ = newDb.Init("sqlite3", ":memory:")
 	}
 	DB = newDb
-	server := startDNS("0.0.0.0:15353", "udp")
+	server := setupDNSServer()
+	// Make sure that we're not creating a race condition in tests
+	var wg sync.WaitGroup
+	wg.Add(1)
+	server.NotifyStartedFunc = func() {
+		wg.Done()
+	}
+	go startDNS(server, make(chan error, 1))
+	wg.Wait()
 	exitval := m.Run()
 	server.Shutdown()
 	DB.Close()
@@ -57,6 +66,8 @@ func setupConfig() {
 
 	var generalcfg = general{
 		Domain:        "auth.example.org",
+		Listen:        "127.0.0.1:15353",
+		Proto:         "udp",
 		Nsname:        "ns1.auth.example.org",
 		Nsadmin:       "admin.example.org",
 		StaticRecords: records,
diff --git a/util.go b/util.go
index 695c6c4..c835a0d 100644
--- a/util.go
+++ b/util.go
@@ -10,7 +10,6 @@ import (
 	"strings"
 
 	"github.com/BurntSushi/toml"
-	"github.com/miekg/dns"
 	log "github.com/sirupsen/logrus"
 )
 
@@ -108,14 +107,6 @@ func setupLogging(format string, level string) {
 	// TODO: file logging
 }
 
-func startDNS(listen string, proto string) *dns.Server {
-	// DNS server part
-	dns.HandleFunc(".", handleRequest)
-	server := &dns.Server{Addr: listen, Net: proto}
-	go server.ListenAndServe()
-	return server
-}
-
 func getIPListFromHeader(header string) []string {
 	iplist := []string{}
 	for _, v := range strings.Split(header, ",") {
-- 
GitLab