Make logging requests configurable

Greg Dubicki · Greg Dubicki · commit ff4ca9b9b963 · 2020-06-03T13:02:28.000+02:00
not requiring the app logging level to be set to TRACE
and with configurable log address (stdout/stderr/syslog etc.)
diff --git a/README.md b/README.md
@@ -22,31 +22,9 @@ To handle intentions, HAProxy Connect, sets up a SPOE filter on the application
 
 ## How to use
 
+Please see this for the current app parameters:
 ```
 ./haproxy-consul-connect --help
-Usage of ./haproxy-consul-connect:
-  -dataplane string
-    	Dataplane binary path (default "dataplane-api")
-  -enable-intentions
-    	Enable Connect intentions
-  -haproxy string
-    	Haproxy binary path (default "haproxy")
-  -haproxy-cfg-base-path string
-    	Haproxy binary path (default "/tmp")
-  -http-addr string
-    	Consul agent address (default "127.0.0.1:8500")
-  -log-level string
-    	Log level (default "INFO")
-  -sidecar-for string
-    	The consul service id to proxy
-  -sidecar-for-tag string
-    	The consul service id to proxy
-  -stats-addr string
-    	Listen addr for stats server
-  -stats-service-register
-    	Register a consul service for connect stats
-  -token string
-    	Consul ACL token./haproxy-consul-connect --help
 ```
 
 ## Minimal working example
diff --git a/go.mod b/go.mod
@@ -3,7 +3,7 @@ module github.com/haproxytech/haproxy-consul-connect
 go 1.13
 
 require (
-	github.com/asaskevich/govalidator v0.0.0-20190424111038-f61b66f89f4a // indirect
+	github.com/asaskevich/govalidator v0.0.0-20190424111038-f61b66f89f4a
 	github.com/criteo/haproxy-spoe-go v0.0.0-20190925130734-97891c13d324
 	github.com/d4l3k/messagediff v1.2.1 // indirect
 	github.com/facebookgo/freeport v0.0.0-20150612182905-d4adf43b75b9
diff --git a/haproxy/config.go b/haproxy/config.go
@@ -28,6 +28,7 @@ global
 	tune.ssl.default-dh-param 1024
 	nbproc 1
 	nbthread {{.NbThread}}
+	log-tag haproxy_sidecar
 
 userlist controller
 	user {{.DataplaneUser}} insecure-password {{.DataplanePass}}
@@ -57,7 +58,6 @@ type baseParams struct {
 	SocketPath    string
 	DataplaneUser string
 	DataplanePass string
-	LogsPath      string
 }
 
 type haConfig struct {
@@ -113,7 +113,6 @@ func newHaConfig(baseDir string, sd *lib.Shutdown) (*haConfig, error) {
 	err = tmpl.Execute(cfgFile, baseParams{
 		NbThread:      runtime.GOMAXPROCS(0),
 		SocketPath:    cfg.StatsSock,
-		LogsPath:      cfg.LogsSock,
 		DataplaneUser: dataplaneUser,
 		DataplanePass: dataplanePass,
 	})
diff --git a/haproxy/haproxy.go b/haproxy/haproxy.go
@@ -60,11 +60,13 @@ func (h *HAProxy) start(sd *lib.Shutdown) error {
 	}
 	h.haConfig = hc
 
-	if h.opts.LogRequests {
+	if h.opts.HAProxyLogAddress == "" {
 		err := h.startLogger()
 		if err != nil {
 			return err
 		}
+	} else {
+		log.Infof("not starting built-in syslog - using %s as syslog address", h.opts.HAProxyLogAddress)
 	}
 
 	if h.opts.EnableIntentions {
@@ -77,6 +79,7 @@ func (h *HAProxy) start(sd *lib.Shutdown) error {
 	dpc, err := haproxy_cmd.Start(sd, haproxy_cmd.Config{
 		HAProxyPath:             h.opts.HAProxyBin,
 		HAProxyConfigPath:       hc.HAProxy,
+		HAProxyLogWithThisApp:   h.opts.HAProxyLogAddress == "",
 		DataplanePath:           h.opts.DataplaneBin,
 		DataplaneTransactionDir: hc.DataplaneTransactionDir,
 		DataplaneSock:           hc.DataplaneSock,
@@ -112,6 +115,8 @@ func (h *HAProxy) startLogger() error {
 		}
 	}(channel)
 
+	log.Infof("starting built-in syslog server at %s", h.haConfig.LogsSock)
+
 	return nil
 }
 
diff --git a/haproxy/haproxy_cmd/cmd.go b/haproxy/haproxy_cmd/cmd.go
@@ -13,10 +13,12 @@ import (
 	log "github.com/sirupsen/logrus"
 )
 
-func runCommand(sd *lib.Shutdown, cmdPath string, args ...string) (*exec.Cmd, error) {
+func runCommand(sd *lib.Shutdown, cmdPath string, logPrefix string, logWithThisApp bool, args ...string) (*exec.Cmd, error) {
 	_, file := path.Split(cmdPath)
 	cmd := exec.Command(cmdPath, args...)
-	halog.Cmd("haproxy", cmd)
+	if logWithThisApp {
+		halog.Cmd(logPrefix, cmd)
+	}
 
 	sd.Add(1)
 	err := cmd.Start()
diff --git a/haproxy/haproxy_cmd/cmd_test.go b/haproxy/haproxy_cmd/cmd_test.go
@@ -10,15 +10,15 @@ import (
 func Test_runCommand_ok(t *testing.T) {
 	t.Parallel()
 	sd := lib.NewShutdown()
-	cmd, err := runCommand(sd, "ls", ".")
+	cmd, err := runCommand(sd, "ls", "foobar", true, ".")
 	require.NoError(t, err)
 	cmd.Wait()
 }
 
 func Test_runCommand_nok_wrong_path(t *testing.T) {
 	t.Parallel()
 	sd := lib.NewShutdown()
-	cmd, err := runCommand(sd, "/path/to/nowhere/that/can/be/found/myExec", "--help")
+	cmd, err := runCommand(sd, "/path/to/nowhere/that/can/be/found/myExec", "foobar", true, "--help")
 	require.NotNil(t, err)
 	require.Contains(t, err.Error(), "no such file or directory")
 	require.Nil(t, cmd)
diff --git a/haproxy/haproxy_cmd/run.go b/haproxy/haproxy_cmd/run.go
@@ -25,6 +25,7 @@ const (
 type Config struct {
 	HAProxyPath             string
 	HAProxyConfigPath       string
+	HAProxyLogWithThisApp   bool
 	DataplanePath           string
 	DataplaneTransactionDir string
 	DataplaneSock           string
@@ -35,6 +36,8 @@ type Config struct {
 func Start(sd *lib.Shutdown, cfg Config) (*dataplane.Dataplane, error) {
 	haCmd, err := runCommand(sd,
 		cfg.HAProxyPath,
+		"haproxy",
+		cfg.HAProxyLogWithThisApp,
 		"-f",
 		cfg.HAProxyConfigPath,
 	)
@@ -47,6 +50,8 @@ func Start(sd *lib.Shutdown, cfg Config) (*dataplane.Dataplane, error) {
 
 	cmd, err := runCommand(sd,
 		cfg.DataplanePath,
+		"dataplaneapi",
+		true,
 		"--scheme", "unix",
 		"--socket-path", cfg.DataplaneSock,
 		"--haproxy-bin", cfg.HAProxyPath,
diff --git a/haproxy/options.go b/haproxy/options.go
@@ -8,5 +8,6 @@ type Options struct {
 	EnableIntentions     bool
 	StatsListenAddr      string
 	StatsRegisterService bool
-	LogRequests          bool
+	HAProxyLogRequests   bool
+	HAProxyLogAddress    string
 }
diff --git a/haproxy/state.go b/haproxy/state.go
@@ -95,10 +95,15 @@ func (h *HAProxy) watch(sd *lib.Shutdown) error {
 
 			newConsulCfg := nextState.Load().(consul.Config)
 
+			logAddress := h.haConfig.LogsSock
+			if h.opts.HAProxyLogAddress != "" {
+				logAddress = h.opts.HAProxyLogAddress
+			}
+
 			newState, err := state.Generate(state.Options{
 				EnableIntentions: h.opts.EnableIntentions,
-				LogRequests:      h.opts.LogRequests,
-				LogSocket:        h.haConfig.LogsSock,
+				LogRequests:      h.opts.HAProxyLogRequests,
+				LogAddress:       logAddress,
 				SPOEConfigPath:   h.haConfig.SPOE,
 				SPOESocket:       h.haConfig.SPOESock,
 			}, h.haConfig, currentState, newConsulCfg)
diff --git a/haproxy/state/downstream.go b/haproxy/state/downstream.go
@@ -44,10 +44,10 @@ func generateDownstream(opts Options, certStore CertificateStore, cfg consul.Dow
 	}
 
 	// Logging
-	if opts.LogRequests && opts.LogSocket != "" {
+	if opts.LogRequests {
 		fe.LogTarget = &models.LogTarget{
 			ID:       int64p(0),
-			Address:  opts.LogSocket,
+			Address:  opts.LogAddress,
 			Facility: models.LogTargetFacilityLocal0,
 			Format:   models.LogTargetFormatRfc5424,
 		}
@@ -100,10 +100,10 @@ func generateDownstream(opts Options, certStore CertificateStore, cfg consul.Dow
 	}
 
 	// Logging
-	if opts.LogRequests && opts.LogSocket != "" {
+	if opts.LogRequests {
 		be.LogTarget = &models.LogTarget{
 			ID:       int64p(0),
-			Address:  opts.LogSocket,
+			Address:  opts.LogAddress,
 			Facility: models.LogTargetFacilityLocal0,
 			Format:   models.LogTargetFormatRfc5424,
 		}
diff --git a/haproxy/state/snapshot_test.go b/haproxy/state/snapshot_test.go
@@ -217,7 +217,7 @@ func GetTestHAConfig(baseCfg string) State {
 var TestOpts = Options{
 	EnableIntentions: true,
 	LogRequests:      true,
-	LogSocket:        "//logs.sock",
+	LogAddress:       "//logs.sock",
 	SPOEConfigPath:   "//spoe",
 }
 
diff --git a/haproxy/state/states.go b/haproxy/state/states.go
@@ -12,7 +12,7 @@ import (
 type Options struct {
 	EnableIntentions bool
 	LogRequests      bool
-	LogSocket        string
+	LogAddress       string
 	SPOEConfigPath   string
 	SPOESocket       string
 }
diff --git a/haproxy/state/upstream.go b/haproxy/state/upstream.go
@@ -34,10 +34,10 @@ func generateUpstream(opts Options, certStore CertificateStore, cfg consul.Upstr
 			Port:    &fePort64,
 		},
 	}
-	if opts.LogRequests && opts.LogSocket != "" {
+	if opts.LogRequests && opts.LogAddress != "" {
 		fe.LogTarget = &models.LogTarget{
 			ID:       int64p(0),
-			Address:  opts.LogSocket,
+			Address:  opts.LogAddress,
 			Facility: models.LogTargetFacilityLocal0,
 			Format:   models.LogTargetFormatRfc5424,
 		}
@@ -56,10 +56,10 @@ func generateUpstream(opts Options, certStore CertificateStore, cfg consul.Upstr
 			Mode: beMode,
 		},
 	}
-	if opts.LogRequests && opts.LogSocket != "" {
+	if opts.LogRequests && opts.LogAddress != "" {
 		be.LogTarget = &models.LogTarget{
 			ID:       int64p(0),
-			Address:  opts.LogSocket,
+			Address:  opts.LogAddress,
 			Facility: models.LogTargetFacilityLocal0,
 			Format:   models.LogTargetFormatRfc5424,
 		}
diff --git a/main.go b/main.go
@@ -1,14 +1,17 @@
 package main
 
 import (
+	"errors"
 	"flag"
 	"fmt"
 	"os"
+	"regexp"
 	"strings"
 
+	"github.com/asaskevich/govalidator"
 	log "github.com/sirupsen/logrus"
 
-	haproxy "github.com/haproxytech/haproxy-consul-connect/haproxy"
+	"github.com/haproxytech/haproxy-consul-connect/haproxy"
 	"github.com/haproxytech/haproxy-consul-connect/haproxy/haproxy_cmd"
 	"github.com/haproxytech/haproxy-consul-connect/lib"
 
@@ -40,7 +43,7 @@ func (consulLogger) Infof(format string, args ...interface{}) {
 
 // Warnf Display warning message
 func (consulLogger) Warnf(format string, args ...interface{}) {
-	log.Infof(format, args...)
+	log.Warnf(format, args...)
 }
 
 // Errorf Display error message
@@ -59,15 +62,37 @@ func validateRequirements(dataplaneBin, haproxyBin string) error {
 	return nil
 }
 
+// HAproxy doesn't exit immediately when you pass incorrect log address, so we try to do it on our own to fail fast
+func validateHaproxyLogAddress(logAddress string) error {
+	// allowed values taken from https://cbonte.github.io/haproxy-dconv/2.0/configuration.html#4.2-log
+	fi, err := os.Stat(logAddress)
+	if err != nil {
+		match, err := regexp.Match(`(fd@<[0-9]+>|stdout|stderr)`, []byte(logAddress))
+		if err != nil && match {
+			return nil
+		}
+		if !govalidator.IsHost(logAddress) && !govalidator.IsDialString(logAddress) {
+			return errors.New(fmt.Sprintf("%s should be either syslog host[:port] or a socket", logAddress))
+		}
+	} else {
+		if fi.Mode()&os.ModeSocket == 0 {
+			return errors.New(fmt.Sprintf("%s is a file but not a socket", logAddress))
+		}
+	}
+	return nil
+}
+
 func main() {
 	versionFlag := flag.Bool("version", false, "Show version and exit")
-	logLevel := flag.String("log-level", "INFO", "Log level")
 	consulAddr := flag.String("http-addr", "127.0.0.1:8500", "Consul agent address")
 	service := flag.String("sidecar-for", "", "The consul service id to proxy")
 	serviceTag := flag.String("sidecar-for-tag", "", "The consul service id to proxy")
 	haproxyBin := flag.String("haproxy", haproxy_cmd.DefaultHAProxyBin, "Haproxy binary path")
 	dataplaneBin := flag.String("dataplane", haproxy_cmd.DefaultDataplaneBin, "Dataplane binary path")
-	haproxyCfgBasePath := flag.String("haproxy-cfg-base-path", "/tmp", "Haproxy binary path")
+	haproxyCfgBasePath := flag.String("haproxy-cfg-base-path", "/tmp", "Generated Haproxy configs path")
+	haproxyLogRequests := flag.Bool("haproxy-log-requests", false, "Enable logging requests by Haproxy")
+	haproxyLogAddress := flag.String("haproxy-log-address", "", "Address for Haproxy logs (default stderr with this app logs)")
+	logLevel := flag.String("log-level", "INFO", "This app log level")
 	statsListenAddr := flag.String("stats-addr", "", "Listen addr for stats server")
 	statsServiceRegister := flag.Bool("stats-service-register", false, "Register a consul service for connect stats")
 	enableIntentions := flag.Bool("enable-intentions", false, "Enable Connect intentions")
@@ -89,6 +114,12 @@ func main() {
 	}
 	log.SetLevel(ll)
 
+	if *haproxyLogAddress != "" {
+		if err := validateHaproxyLogAddress(*haproxyLogAddress); err != nil {
+			log.Fatal(err)
+		}
+	}
+
 	sd := lib.NewShutdown()
 
 	consulConfig := &api.Config{
@@ -144,7 +175,8 @@ func main() {
 		EnableIntentions:     *enableIntentions,
 		StatsListenAddr:      *statsListenAddr,
 		StatsRegisterService: *statsServiceRegister,
-		LogRequests:          ll == log.TraceLevel,
+		HAProxyLogRequests:   *haproxyLogRequests,
+		HAProxyLogAddress:    *haproxyLogAddress,
 	})
 	sd.Add(1)
 	go func() {

Original file line number	Diff line number	Diff line change
`@@ -60,11 +60,13 @@ func (h HAProxy) start(sd lib.Shutdown) error {`
`60`	`60`	`}`
`61`	`61`	`h.haConfig = hc`
`62`	`62`
`63`		`- if h.opts.LogRequests {`
	`63`	`+ if h.opts.HAProxyLogAddress == "" {`
`64`	`64`	`err := h.startLogger()`
`65`	`65`	`if err != nil {`
`66`	`66`	`return err`
`67`	`67`	`}`
	`68`	`+ } else {`
	`69`	`+ log.Infof("not starting built-in syslog - using %s as syslog address", h.opts.HAProxyLogAddress)`
`68`	`70`	`}`
`69`	`71`
`70`	`72`	`if h.opts.EnableIntentions {`
`@@ -77,6 +79,7 @@ func (h HAProxy) start(sd lib.Shutdown) error {`
`77`	`79`	`dpc, err := haproxy_cmd.Start(sd, haproxy_cmd.Config{`
`78`	`80`	`HAProxyPath: h.opts.HAProxyBin,`
`79`	`81`	`HAProxyConfigPath: hc.HAProxy,`
	`82`	`+ HAProxyLogWithThisApp: h.opts.HAProxyLogAddress == "",`
`80`	`83`	`DataplanePath: h.opts.DataplaneBin,`
`81`	`84`	`DataplaneTransactionDir: hc.DataplaneTransactionDir,`
`82`	`85`	`DataplaneSock: hc.DataplaneSock,`
`@@ -112,6 +115,8 @@ func (h *HAProxy) startLogger() error {`
`112`	`115`	`}`
`113`	`116`	`}(channel)`
`114`	`117`
	`118`	`+ log.Infof("starting built-in syslog server at %s", h.haConfig.LogsSock)`
	`119`	`+`
`115`	`120`	`return nil`
`116`	`121`	`}`
`117`	`122`
Original file line number	Diff line number	Diff line change
`@@ -8,5 +8,6 @@ type Options struct {`
`8`	`8`	`EnableIntentions bool`
`9`	`9`	`StatsListenAddr string`
`10`	`10`	`StatsRegisterService bool`
`11`		`- LogRequests bool`
	`11`	`+ HAProxyLogRequests bool`
	`12`	`+ HAProxyLogAddress string`
`12`	`13`	`}`
Original file line number	Diff line number	Diff line change
`@@ -44,10 +44,10 @@ func generateDownstream(opts Options, certStore CertificateStore, cfg consul.Dow`
`44`	`44`	`}`
`45`	`45`
`46`	`46`	`// Logging`
`47`		`- if opts.LogRequests && opts.LogSocket != "" {`
	`47`	`+ if opts.LogRequests {`
`48`	`48`	`fe.LogTarget = &models.LogTarget{`
`49`	`49`	`ID: int64p(0),`
`50`		`- Address: opts.LogSocket,`
	`50`	`+ Address: opts.LogAddress,`
`51`	`51`	`Facility: models.LogTargetFacilityLocal0,`
`52`	`52`	`Format: models.LogTargetFormatRfc5424,`
`53`	`53`	`}`
`@@ -100,10 +100,10 @@ func generateDownstream(opts Options, certStore CertificateStore, cfg consul.Dow`
`100`	`100`	`}`
`101`	`101`
`102`	`102`	`// Logging`
`103`		`- if opts.LogRequests && opts.LogSocket != "" {`
	`103`	`+ if opts.LogRequests {`
`104`	`104`	`be.LogTarget = &models.LogTarget{`
`105`	`105`	`ID: int64p(0),`
`106`		`- Address: opts.LogSocket,`
	`106`	`+ Address: opts.LogAddress,`
`107`	`107`	`Facility: models.LogTargetFacilityLocal0,`
`108`	`108`	`Format: models.LogTargetFormatRfc5424,`
`109`	`109`	`}`