caddyfile: Introduce basic linting and fmt check (#3923)

* caddyfile: Introduce basic linting and fmt check This will help encourage people to keep their Caddyfiles tidy. * Remove unrelated tests I am not sure that testing the output of warnings here is quite the right idea; these tests are just for syntax and parsing success.
caddyserver · Jan 4, 2021 · c8557dc · c8557dc
1 parent 1b453dd
commit c8557dc
Show file tree

Hide file tree

Showing 7 changed files with 40 additions and 54 deletions.
diff --git a/caddyconfig/caddyfile/adapter.go b/caddyconfig/caddyfile/adapter.go
@@ -15,6 +15,7 @@
 package caddyfile
 
 import (
+	"bytes"
 	"encoding/json"
 	"fmt"
 
@@ -51,11 +52,17 @@ func (a Adapter) Adapt(body []byte, options map[string]interface{}) ([]byte, []c
 		return nil, warnings, err
 	}
 
-	marshalFunc := json.Marshal
-	if options["pretty"] == "true" {
-		marshalFunc = caddyconfig.JSONIndent
+	// lint check: see if input was properly formatted; sometimes messy files files parse
+	// successfully but result in logical errors because the Caddyfile is a bad format
+	// TODO: also perform this check on imported files
+	if !bytes.Equal(Format(body), body) {
+		warnings = append(warnings, caddyconfig.Warning{
+			File:    filename,
+			Message: "file is not formatted with 'caddy fmt'",
+		})
 	}
-	result, err := marshalFunc(cfg)
+
+	result, err := json.Marshal(cfg)
 
 	return result, warnings, err
 }

diff --git a/caddyconfig/configadapters.go b/caddyconfig/configadapters.go
@@ -93,12 +93,6 @@ func JSONModuleObject(val interface{}, fieldName, fieldVal string, warnings *[]W
 	return result
 }
 
-// JSONIndent is used to JSON-marshal the final resulting Caddy
-// configuration in a consistent, human-readable way.
-func JSONIndent(val interface{}) ([]byte, error) {
-	return json.MarshalIndent(val, "", "\t")
-}
-
 // RegisterAdapter registers a config adapter with the given name.
 // This should usually be done at init-time. It panics if the
 // adapter cannot be registered successfully.

diff --git a/caddyconfig/httpcaddyfile/builtins_test.go b/caddyconfig/httpcaddyfile/builtins_test.go
@@ -10,15 +10,13 @@ import (
 func TestLogDirectiveSyntax(t *testing.T) {
 	for i, tc := range []struct {
 		input       string
-		expectWarn  bool
 		expectError bool
 	}{
 		{
 			input: `:8080 {
 				log
 			}
 			`,
-			expectWarn:  false,
 			expectError: false,
 		},
 		{
@@ -28,7 +26,6 @@ func TestLogDirectiveSyntax(t *testing.T) {
 				}
 			}
 			`,
-			expectWarn:  false,
 			expectError: false,
 		},
 		{
@@ -38,7 +35,6 @@ func TestLogDirectiveSyntax(t *testing.T) {
 				}
 			}
 			`,
-			expectWarn:  false,
 			expectError: true,
 		},
 	} {
@@ -47,12 +43,7 @@ func TestLogDirectiveSyntax(t *testing.T) {
 			ServerType: ServerType{},
 		}
 
-		_, warnings, err := adapter.Adapt([]byte(tc.input), nil)
-
-		if len(warnings) > 0 != tc.expectWarn {
-			t.Errorf("Test %d warning expectation failed Expected: %v, got %v", i, tc.expectWarn, warnings)
-			continue
-		}
+		_, _, err := adapter.Adapt([]byte(tc.input), nil)
 
 		if err != nil != tc.expectError {
 			t.Errorf("Test %d error expectation failed Expected: %v, got %s", i, tc.expectError, err)

diff --git a/caddyconfig/httpcaddyfile/httptype_test.go b/caddyconfig/httpcaddyfile/httptype_test.go
@@ -9,7 +9,6 @@ import (
 func TestMatcherSyntax(t *testing.T) {
 	for i, tc := range []struct {
 		input       string
-		expectWarn  bool
 		expectError bool
 	}{
 		{
@@ -18,7 +17,6 @@ func TestMatcherSyntax(t *testing.T) {
 				query showdebug=1
 			}
 			`,
-			expectWarn:  false,
 			expectError: false,
 		},
 		{
@@ -27,7 +25,6 @@ func TestMatcherSyntax(t *testing.T) {
 				query bad format
 			}
 			`,
-			expectWarn:  false,
 			expectError: true,
 		},
 		{
@@ -38,7 +35,6 @@ func TestMatcherSyntax(t *testing.T) {
 				}
 			}
 			`,
-			expectWarn:  false,
 			expectError: false,
 		},
 		{
@@ -47,14 +43,12 @@ func TestMatcherSyntax(t *testing.T) {
 				not path /somepath*
 			}
 			`,
-			expectWarn:  false,
 			expectError: false,
 		},
 		{
 			input: `http://localhost
 			@debug not path /somepath*
 			`,
-			expectWarn:  false,
 			expectError: false,
 		},
 		{
@@ -63,7 +57,6 @@ func TestMatcherSyntax(t *testing.T) {
 			}
 			http://localhost
 			`,
-			expectWarn:  false,
 			expectError: true,
 		},
 	} {
@@ -72,12 +65,7 @@ func TestMatcherSyntax(t *testing.T) {
 			ServerType: ServerType{},
 		}
 
-		_, warnings, err := adapter.Adapt([]byte(tc.input), nil)
-
-		if len(warnings) > 0 != tc.expectWarn {
-			t.Errorf("Test %d warning expectation failed Expected: %v, got %v", i, tc.expectWarn, warnings)
-			continue
-		}
+		_, _, err := adapter.Adapt([]byte(tc.input), nil)
 
 		if err != nil != tc.expectError {
 			t.Errorf("Test %d error expectation failed Expected: %v, got %s", i, tc.expectError, err)
@@ -119,7 +107,6 @@ func TestSpecificity(t *testing.T) {
 func TestGlobalOptions(t *testing.T) {
 	for i, tc := range []struct {
 		input       string
-		expectWarn  bool
 		expectError bool
 	}{
 		{
@@ -129,7 +116,6 @@ func TestGlobalOptions(t *testing.T) {
 				}
 				:80
 			`,
-			expectWarn:  false,
 			expectError: false,
 		},
 		{
@@ -139,7 +125,6 @@ func TestGlobalOptions(t *testing.T) {
 				}
 				:80
 			`,
-			expectWarn:  false,
 			expectError: false,
 		},
 		{
@@ -149,7 +134,6 @@ func TestGlobalOptions(t *testing.T) {
 				}
 				:80
 			`,
-			expectWarn:  false,
 			expectError: false,
 		},
 		{
@@ -161,7 +145,6 @@ func TestGlobalOptions(t *testing.T) {
 				}
 				:80
 			`,
-			expectWarn:  false,
 			expectError: true,
 		},
 		{
@@ -174,7 +157,6 @@ func TestGlobalOptions(t *testing.T) {
 				}
 				:80
 			`,
-			expectWarn:  false,
 			expectError: false,
 		},
 		{
@@ -187,7 +169,6 @@ func TestGlobalOptions(t *testing.T) {
 				}
 				:80
 			`,
-			expectWarn:  false,
 			expectError: false,
 		},
 		{
@@ -200,7 +181,6 @@ func TestGlobalOptions(t *testing.T) {
 				}
 				:80
 			`,
-			expectWarn:  false,
 			expectError: true,
 		},
 		{
@@ -213,7 +193,6 @@ func TestGlobalOptions(t *testing.T) {
 				}
 				:80
 			`,
-			expectWarn:  false,
 			expectError: true,
 		},
 	} {
@@ -222,12 +201,7 @@ func TestGlobalOptions(t *testing.T) {
 			ServerType: ServerType{},
 		}
 
-		_, warnings, err := adapter.Adapt([]byte(tc.input), nil)
-
-		if len(warnings) > 0 != tc.expectWarn {
-			t.Errorf("Test %d warning expectation failed Expected: %v, got %v", i, tc.expectWarn, warnings)
-			continue
-		}
+		_, _, err := adapter.Adapt([]byte(tc.input), nil)
 
 		if err != nil != tc.expectError {
 			t.Errorf("Test %d error expectation failed Expected: %v, got %s", i, tc.expectError, err)

diff --git a/caddytest/caddytest.go b/caddytest/caddytest.go
@@ -336,14 +336,21 @@ func CompareAdapt(t *testing.T, rawConfig string, adapterName string, expectedRe
 	}
 
 	options := make(map[string]interface{})
-	options["pretty"] = "true"
 
 	result, warnings, err := cfgAdapter.Adapt([]byte(rawConfig), options)
 	if err != nil {
 		t.Logf("adapting config using %s adapter: %v", adapterName, err)
 		return false
 	}
 
+	// prettify results to keep tests human-manageable
+	var prettyBuf bytes.Buffer
+	err = json.Indent(&prettyBuf, result, "", "\t")
+	if err != nil {
+		return false
+	}
+	result = prettyBuf.Bytes()
+
 	if len(warnings) > 0 {
 		for _, w := range warnings {
 			t.Logf("warning: directive: %s : %s", w.Directive, w.Message)

diff --git a/cmd/commandfuncs.go b/cmd/commandfuncs.go
@@ -463,17 +463,22 @@ func cmdAdaptConfig(fl Flags) (int, error) {
 			fmt.Errorf("reading input file: %v", err)
 	}
 
-	opts := make(map[string]interface{})
-	if adaptCmdPrettyFlag {
-		opts["pretty"] = "true"
-	}
-	opts["filename"] = adaptCmdInputFlag
+	opts := map[string]interface{}{"filename": adaptCmdInputFlag}
 
 	adaptedConfig, warnings, err := cfgAdapter.Adapt(input, opts)
 	if err != nil {
 		return caddy.ExitCodeFailedStartup, err
 	}
 
+	if adaptCmdPrettyFlag {
+		var prettyBuf bytes.Buffer
+		err = json.Indent(&prettyBuf, adaptedConfig, "", "\t")
+		if err != nil {
+			return caddy.ExitCodeFailedStartup, err
+		}
+		adaptedConfig = prettyBuf.Bytes()
+	}
+
 	// print warnings to stderr
 	for _, warn := range warnings {
 		msg := warn.Message

diff --git a/modules/caddyhttp/reverseproxy/caddyfile.go b/modules/caddyhttp/reverseproxy/caddyfile.go
@@ -15,6 +15,7 @@
 package reverseproxy
 
 import (
+	"log"
 	"net"
 	"net/http"
 	"net/url"
@@ -497,6 +498,13 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 				case 1:
 					err = headers.CaddyfileHeaderOp(h.Headers.Request, args[0], "", "")
 				case 2:
+					// some lint checks, I guess
+					if strings.EqualFold(args[0], "host") && (args[1] == "{hostport}" || args[1] == "{http.request.hostport}") {
+						log.Printf("[WARNING] Unnecessary header_up ('Host' field): the reverse proxy's default behavior is to pass headers to the upstream")
+					}
+					if strings.EqualFold(args[0], "x-forwarded-proto") && (args[1] == "{scheme}" || args[1] == "{http.request.scheme}") {
+						log.Printf("[WARNING] Unnecessary header_up ('X-Forwarded-Proto' field): the reverse proxy's default behavior is to pass headers to the upstream")
+					}
 					err = headers.CaddyfileHeaderOp(h.Headers.Request, args[0], args[1], "")
 				case 3:
 					err = headers.CaddyfileHeaderOp(h.Headers.Request, args[0], args[1], args[2])