Repository: caddyserver/caddy (language: go)

Issue:
[BUG?] Access logs for sub-domains are not written after Apr 26 update 
Caddy version:

```
v2.5.1 h1:bAWwslD1jNeCzDa+jDCNwb8M3UJ2tPa8UZFFzPVmGKs=
```

running as: `systemctl start caddy` on `Ubuntu 20.04.4 LTS`


Caddy file:

```
(logging) {
	log {
		format json {
			message_key msg
			level_key level
			time_key ts
			name_key name
			time_format wall_milli
		}
		output file /var/log/caddy/{args.0}.access.log {
			roll_size 10mb
			roll_keep 20
			roll_keep_for 30d
		}
		level INFO
	}
}

*.domain.com {
	redir https://domain.com{uri}
}

domain.com {
	import logging domain.com
	respond "ok" 200
}

sub1.domain.com {
	import logging sub1.domain.com
	respond "ok" 200
}

sub2.domain.com {
	import logging sub2.domain.com
	respond "ok" 200
}
```

Expected behavior:
---

in the folder `/var/log/caddy` we should see the following files when each sub-domain is accessed:
 * domain.com.access.log
 * sub1.domain.com.access.log
 * sub2.domain.com.access.log

Issue
---
 * Currently there is only one log file created for the root domain; access logs for sub-domains are not created
 
 
Tried already:
 ---
  - replace `import logging sub1.domain.com` with:
    ```
            log {
                output file /var/log/caddy/sub1.domain.com
        }
    ```
 - replacing `.` with `_` in the domain name, e.g. `import logging sub1_domain_com`
 - permissions to access `/var/log/caddy` are correct
 
Notes:
---

prior Apr 26 everything was working fine.


Discussion / hints:
Isn't that the expected behaviour? You catch all subdomain requests in the first block without logging. After the redirect only domain.com would receive requests and therefore yield log entries.
Adapting the config to JSON, the issue becomes obvious:

```json
					"logs": {
						"logger_names": {
							"domain.com": "log0",
							"sub1.domain.com": "log1",
							"sub2.domain.com": "log2"
						},
						"skip_hosts": [
							"*.domain.com"
						]
					}
```

The `skip_hosts` is preventing the `sub1.domain.com` loggers from working, I think. We might need to flip this so if there's an explicitly configured logger, wildcard skips should not skip those.

I think this behaviour changed in https://github.com/caddyserver/caddy/pull/4606 which is where `skip_hosts` started honoring wildcard domains.
Just wanted to chime in and say that the current behaviour definitely is not intuitive with how the rest of Caddy works. From the docs:

> If a request matches multiple site blocks, the site block with the most specific matching address is chosen. Requests don't cascade into to other site blocks.

In my case I have two blocks: `www.domain.com` and `*.domain.com`. 

- `www.domain.com` has logging enabled
- `*.domain.com` reverse proxies to `www.domain.com`

Because of that setup (and this issue) requests directly to `www.domain.com` were not being logged. 

Flipping the logic like @francislavoie mentioned above would be ideal 👍 

Fix patch:
diff --git a/caddyconfig/httpcaddyfile/builtins.go b/caddyconfig/httpcaddyfile/builtins.go
index 7f23fd59dd9..f4c9ce01bec 100644
--- a/caddyconfig/httpcaddyfile/builtins.go
+++ b/caddyconfig/httpcaddyfile/builtins.go
@@ -735,7 +735,7 @@ func parseLogHelper(h Helper, globalLogNames map[string]struct{}) ([]ConfigValue
 				// reference the default logger. See the
 				// setupNewDefault function in the logging
 				// package for where this is configured.
-				globalLogName = "default"
+				globalLogName = caddy.DefaultLoggerName
 			}
 
 			// Verify this name is unused.
diff --git a/caddyconfig/httpcaddyfile/httptype.go b/caddyconfig/httpcaddyfile/httptype.go
index 7159454b031..c220c066d87 100644
--- a/caddyconfig/httpcaddyfile/httptype.go
+++ b/caddyconfig/httpcaddyfile/httptype.go
@@ -219,7 +219,7 @@ func (st ServerType) Setup(inputServerBlocks []caddyfile.ServerBlock,
 		if ncl.name == "" {
 			return
 		}
-		if ncl.name == "default" {
+		if ncl.name == caddy.DefaultLoggerName {
 			hasDefaultLog = true
 		}
 		if _, ok := options["debug"]; ok && ncl.log.Level == "" {
@@ -240,7 +240,7 @@ func (st ServerType) Setup(inputServerBlocks []caddyfile.ServerBlock,
 		// configure it with any applicable options
 		if _, ok := options["debug"]; ok {
 			customLogs = append(customLogs, namedCustomLog{
-				name: "default",
+				name: caddy.DefaultLoggerName,
 				log:  &caddy.CustomLog{Level: zap.DebugLevel.CapitalString()},
 			})
 		}
@@ -299,11 +299,11 @@ func (st ServerType) Setup(inputServerBlocks []caddyfile.ServerBlock,
 			// most users seem to prefer not writing access logs
 			// to the default log when they are directed to a
 			// file or have any other special customization
-			if ncl.name != "default" && len(ncl.log.Include) > 0 {
-				defaultLog, ok := cfg.Logging.Logs["default"]
+			if ncl.name != caddy.DefaultLoggerName && len(ncl.log.Include) > 0 {
+				defaultLog, ok := cfg.Logging.Logs[caddy.DefaultLoggerName]
 				if !ok {
 					defaultLog = new(caddy.CustomLog)
-					cfg.Logging.Logs["default"] = defaultLog
+					cfg.Logging.Logs[caddy.DefaultLoggerName] = defaultLog
 				}
 				defaultLog.Exclude = append(defaultLog.Exclude, ncl.log.Include...)
 			}
@@ -518,15 +518,6 @@ func (st *ServerType) serversFromPairings(
 		var hasCatchAllTLSConnPolicy, addressQualifiesForTLS bool
 		autoHTTPSWillAddConnPolicy := autoHTTPS != "off"
 
-		// if a catch-all server block (one which accepts all hostnames) exists in this pairing,
-		// we need to know that so that we can configure logs properly (see #3878)
-		var catchAllSblockExists bool
-		for _, sblock := range p.serverBlocks {
-			if len(sblock.hostsFromKeys(false)) == 0 {
-				catchAllSblockExists = true
-			}
-		}
-
 		// if needed, the ServerLogConfig is initialized beforehand so
 		// that all server blocks can populate it with data, even when not
 		// coming with a log directive
@@ -658,18 +649,10 @@ func (st *ServerType) serversFromPairings(
 				} else {
 					// map each host to the user's desired logger name
 					for _, h := range sblockLogHosts {
-						// if the custom logger name is non-empty, add it to the map;
-						// otherwise, only map to an empty logger name if this or
-						// another site block on this server has a catch-all host (in
-						// which case only requests with mapped hostnames will be
-						// access-logged, so it'll be necessary to add them to the
-						// map even if they use default logger)
-						if ncl.name != "" || catchAllSblockExists {
-							if srv.Logs.LoggerNames == nil {
-								srv.Logs.LoggerNames = make(map[string]string)
-							}
-							srv.Logs.LoggerNames[h] = ncl.name
+						if srv.Logs.LoggerNames == nil {
+							srv.Logs.LoggerNames = make(map[string]string)
 						}
+						srv.Logs.LoggerNames[h] = ncl.name
 					}
 				}
 			}
diff --git a/logging.go b/logging.go
index 5f8ea06a908..c00c8315d67 100644
--- a/logging.go
+++ b/logging.go
@@ -105,7 +105,7 @@ func (logging *Logging) openLogs(ctx Context) error {
 	// then set up any other custom logs
 	for name, l := range logging.Logs {
 		// the default log is already set up
-		if name == "default" {
+		if name == DefaultLoggerName {
 			continue
 		}
 
@@ -138,7 +138,7 @@ func (logging *Logging) setupNewDefault(ctx Context) error {
 
 	// extract the user-defined default log, if any
 	newDefault := new(defaultCustomLog)
-	if userDefault, ok := logging.Logs["default"]; ok {
+	if userDefault, ok := logging.Logs[DefaultLoggerName]; ok {
 		newDefault.CustomLog = userDefault
 	} else {
 		// if none, make one with our own default settings
@@ -147,7 +147,7 @@ func (logging *Logging) setupNewDefault(ctx Context) error {
 		if err != nil {
 			return fmt.Errorf("setting up default Caddy log: %v", err)
 		}
-		logging.Logs["default"] = newDefault.CustomLog
+		logging.Logs[DefaultLoggerName] = newDefault.CustomLog
 	}
 
 	// set up this new log
@@ -702,6 +702,8 @@ var (
 
 var writers = NewUsagePool()
 
+const DefaultLoggerName = "default"
+
 // Interface guards
 var (
 	_ io.WriteCloser = (*notClosable)(nil)
diff --git a/modules/caddyhttp/server.go b/modules/caddyhttp/server.go
index 4d47d266e7d..60fc3c3877f 100644
--- a/modules/caddyhttp/server.go
+++ b/modules/caddyhttp/server.go
@@ -639,21 +639,18 @@ func (s *Server) shouldLogRequest(r *http.Request) bool {
 		// logging is disabled
 		return false
 	}
+	if _, ok := s.Logs.LoggerNames[r.Host]; 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) {
 			return false
 		}
 	}
-	if _, ok := s.Logs.LoggerNames[r.Host]; ok {
-		// this host is mapped to a particular logger name
-		return true
-	}
-	if s.Logs.SkipUnmappedHosts {
-		// this host is not mapped and thus must not be logged
-		return false
-	}
-	return true
+	// if configured, this host is not mapped and thus must not be logged
+	return !s.Logs.SkipUnmappedHosts
 }
 
 // protocol returns true if the protocol proto is configured/enabled.


Test patch:
diff --git a/caddytest/integration/caddyfile_adapt/log_skip_hosts.txt b/caddytest/integration/caddyfile_adapt/log_skip_hosts.txt
index 6fbd85a8701..25e4bd1c206 100644
--- a/caddytest/integration/caddyfile_adapt/log_skip_hosts.txt
+++ b/caddytest/integration/caddyfile_adapt/log_skip_hosts.txt
@@ -62,6 +62,9 @@ example.com {
 						}
 					],
 					"logs": {
+						"logger_names": {
+							"one.example.com": ""
+						},
 						"skip_hosts": [
 							"three.example.com",
 							"two.example.com",