From c8557dc00bd93ce8ecf0bb724856a320e129c71b Mon Sep 17 00:00:00 2001 From: Matt Holt Date: Mon, 4 Jan 2021 11:11:36 -0700 Subject: [PATCH] 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. --- caddyconfig/caddyfile/adapter.go | 15 ++++++++--- caddyconfig/configadapters.go | 6 ----- caddyconfig/httpcaddyfile/builtins_test.go | 11 +------- caddyconfig/httpcaddyfile/httptype_test.go | 30 ++------------------- caddytest/caddytest.go | 9 ++++++- cmd/commandfuncs.go | 15 +++++++---- modules/caddyhttp/reverseproxy/caddyfile.go | 8 ++++++ 7 files changed, 40 insertions(+), 54 deletions(-) diff --git a/caddyconfig/caddyfile/adapter.go b/caddyconfig/caddyfile/adapter.go index 5b4495eb7..8d624f1d8 100644 --- 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 index 1918fa757..1665fa03d 100644 --- 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 index a255538ce..c06dd230c 100644 --- a/caddyconfig/httpcaddyfile/builtins_test.go +++ b/caddyconfig/httpcaddyfile/builtins_test.go @@ -10,7 +10,6 @@ import ( func TestLogDirectiveSyntax(t *testing.T) { for i, tc := range []struct { input string - expectWarn bool expectError bool }{ { @@ -18,7 +17,6 @@ func TestLogDirectiveSyntax(t *testing.T) { 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 index 233e37495..69f55501c 100644 --- 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 index 5387da788..4076bd5aa 100644 --- a/caddytest/caddytest.go +++ b/caddytest/caddytest.go @@ -336,7 +336,6 @@ 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 { @@ -344,6 +343,14 @@ func CompareAdapt(t *testing.T, rawConfig string, adapterName string, expectedRe 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 index 28fa26ed9..20c662b0d 100644 --- 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 index f4556ea22..4a6928748 100644 --- 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])