logging: Fix default access logger (#6251)

* logging: Fix default access logger

* Simplify logic, remove retry without port, reject config with port, docs

* Nil check

Author: francislavoie

modules/caddyhttp/app.go

@@ -329,9 +329,10 @@ func (app *App) Provision(ctx caddy.Context) error {
 
 // Validate ensures the app's configuration is valid.
 func (app *App) Validate() error {
-	// each server must use distinct listener addresses
 	lnAddrs := make(map[string]string)
+
 	for srvName, srv := range app.Servers {
+		// each server must use distinct listener addresses
 		for _, addr := range srv.Listen {
 			listenAddr, err := caddy.ParseNetworkAddress(addr)
 			if err != nil {
@@ -347,6 +348,15 @@ func (app *App) Validate() error {
 				lnAddrs[addr] = srvName
 			}
 		}
+
+		// logger names must not have ports
+		if srv.Logs != nil {
+			for host := range srv.Logs.LoggerNames {
+				if _, _, err := net.SplitHostPort(host); err == nil {
+					return fmt.Errorf("server %s: logger name must not have a port: %s", srvName, host)
+				}
+			}
+		}
 	}
 	return nil
 }

modules/caddyhttp/logging.go

@@ -37,10 +37,16 @@ type ServerLogConfig struct {
 	DefaultLoggerName string `json:"default_logger_name,omitempty"`
 
 	// LoggerNames maps request hostnames to one or more custom logger
-	// names. For example, a mapping of "example.com" to "example" would
+	// names. For example, a mapping of `"example.com": ["example"]` would
 	// cause access logs from requests with a Host of example.com to be
 	// emitted by a logger named "http.log.access.example". If there are
 	// multiple logger names, then the log will be emitted to all of them.
+	// If the logger name is an empty, the default logger is used, i.e.
+	// the logger "http.log.access".
+	//
+	// Keys must be hostnames (without ports), and may contain wildcards
+	// to match subdomains. The value is an array of logger names.
+	//
 	// For backwards compatibility, if the value is a string, it is treated
 	// as a single-element array.
 	LoggerNames map[string]StringArray `json:"logger_names,omitempty"`
@@ -64,10 +70,19 @@ type ServerLogConfig struct {
 // wrapLogger wraps logger in one or more logger named
 // according to user preferences for the given host.
 func (slc ServerLogConfig) wrapLogger(logger *zap.Logger, host string) []*zap.Logger {
-	hosts := slc.getLoggerHosts(host)
+	// logger config should always be only
+	// the hostname, without the port
+	hostWithoutPort, _, err := net.SplitHostPort(host)
+	if err != nil {
+		hostWithoutPort = host
+	}
+
+	hosts := slc.getLoggerHosts(hostWithoutPort)
 	loggers := make([]*zap.Logger, 0, len(hosts))
 	for _, loggerName := range hosts {
+		// no name, use the default logger
 		if loggerName == "" {
+			loggers = append(loggers, logger)
 			continue
 		}
 		loggers = append(loggers, logger.Named(loggerName))
@@ -76,23 +91,8 @@ func (slc ServerLogConfig) wrapLogger(logger *zap.Logger, host string) []*zap.Lo
 }
 
 func (slc ServerLogConfig) getLoggerHosts(host string) []string {
-	tryHost := func(key string) ([]string, bool) {
-		// first try exact match
-		if hosts, ok := slc.LoggerNames[key]; ok {
-			return hosts, ok
-		}
-		// strip port and try again (i.e. Host header of "example.com:1234" should
-		// match "example.com" if there is no "example.com:1234" in the map)
-		hostOnly, _, err := net.SplitHostPort(key)
-		if err != nil {
-			return []string{}, false
-		}
-		hosts, ok := slc.LoggerNames[hostOnly]
-		return hosts, ok
-	}
-
 	// try the exact hostname first
-	if hosts, ok := tryHost(host); ok {
+	if hosts, ok := slc.LoggerNames[host]; ok {
 		return hosts
 	}
 
@@ -104,7 +104,7 @@ func (slc ServerLogConfig) getLoggerHosts(host string) []string {
 		}
 		labels[i] = "*"
 		wildcardHost := strings.Join(labels, ".")
-		if hosts, ok := tryHost(wildcardHost); ok {
+		if hosts, ok := slc.LoggerNames[wildcardHost]; ok {
 			return hosts
 		}
 	}

modules/caddyhttp/server.go

@@ -717,13 +717,20 @@ func (s *Server) shouldLogRequest(r *http.Request) bool {
 		// logging is disabled
 		return false
 	}
-	if _, ok := s.Logs.LoggerNames[r.Host]; ok {
+
+	// strip off the port if any, logger names are host only
+	hostWithoutPort, _, err := net.SplitHostPort(r.Host)
+	if err != nil {
+		hostWithoutPort = r.Host
+	}
+
+	if _, ok := s.Logs.LoggerNames[hostWithoutPort]; ok {
 		// this host is mapped to a particular logger name
 		return true
 	}
 	for _, dh := range s.Logs.SkipHosts {
 		// logging for this particular host is disabled
-		if certmagic.MatchWildcard(r.Host, dh) {
+		if certmagic.MatchWildcard(hostWithoutPort, dh) {
 			return false
 		}
 	}