From f3596f734d9c3241577c57b62fc3c1b277bc4c43 Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Wed, 14 Oct 2015 23:45:28 -0600 Subject: [PATCH] Epic revert of 0ac8bf5 and adding OncePerServerBlock Turns out having each server block share a single server.Config during initialization when the Setup functions are being called was a bad idea. Sure, startup and shutdown functions were only executed once, but they had no idea what their hostname or port was. So here we revert to the old way of doing things where Setup may be called multiple times per server block (once per host associated with the block, to be precise), but the Setup functions now know their host and port since the config belongs to exactly one virtualHost. To have something happen just once per server block, use OncePerServerBlock, a new function available on each Controller. --- config/config.go | 113 ++++++++++++----------------- config/parse/parse.go | 2 +- config/parse/parsing.go | 21 +++--- config/parse/parsing_test.go | 135 +++++++++++++++++++++-------------- config/setup/controller.go | 1 + config/setup/fastcgi.go | 2 +- main.go | 7 +- server/server.go | 2 +- 8 files changed, 145 insertions(+), 138 deletions(-) diff --git a/config/config.go b/config/config.go index cc7c5b11c..93f5146bf 100644 --- a/config/config.go +++ b/config/config.go @@ -5,6 +5,7 @@ import ( "io" "log" "net" + "sync" "github.com/mholt/caddy/app" "github.com/mholt/caddy/config/parse" @@ -40,32 +41,51 @@ func Load(filename string, input io.Reader) (Group, error) { return Default() } - // Each server block represents one or more servers/addresses. + // Each server block represents similar hosts/addresses. // Iterate each server block and make a config for each one, // executing the directives that were parsed. for _, sb := range serverBlocks { - sharedConfig, err := serverBlockToConfig(filename, sb) - if err != nil { - return nil, err - } + var once sync.Once + + for _, addr := range sb.Addresses { + config := server.Config{ + Host: addr.Host, + Port: addr.Port, + Root: Root, + Middleware: make(map[string][]middleware.Middleware), + ConfigFile: filename, + AppName: app.Name, + AppVersion: app.Version, + } + + // It is crucial that directives are executed in the proper order. + for _, dir := range directiveOrder { + // Execute directive if it is in the server block + if tokens, ok := sb.Tokens[dir.name]; ok { + // Each setup function gets a controller, which is the + // server config and the dispenser containing only + // this directive's tokens. + controller := &setup.Controller{ + Config: &config, + Dispenser: parse.NewDispenserTokens(filename, tokens), + OncePerServerBlock: func(f func()) { once.Do(f) }, + } + + midware, err := dir.setup(controller) + if err != nil { + return nil, err + } + if midware != nil { + // TODO: For now, we only support the default path scope / + config.Middleware["/"] = append(config.Middleware["/"], midware) + } + } + } - // Now share the config with as many hosts as share the server block - for i, addr := range sb.Addresses { - config := sharedConfig - config.Host = addr.Host - config.Port = addr.Port if config.Port == "" { config.Port = Port } - if config.Port == "http" { - config.TLS.Enabled = false - log.Printf("Warning: TLS disabled for %s://%s. To force TLS over the plaintext HTTP port, "+ - "specify port 80 explicitly (https://%s:80).", config.Port, config.Host, config.Host) - } - if i == 0 { - sharedConfig.Startup = []func() error{} - sharedConfig.Shutdown = []func() error{} - } + configs = append(configs, config) } } @@ -73,50 +93,9 @@ func Load(filename string, input io.Reader) (Group, error) { // restore logging settings log.SetFlags(flags) - // Group by address/virtualhosts return arrangeBindings(configs) } -// serverBlockToConfig makes a config for the server block -// by executing the tokens that were parsed. The returned -// config is shared among all hosts/addresses for the server -// block, so Host and Port information is not filled out -// here. -func serverBlockToConfig(filename string, sb parse.ServerBlock) (server.Config, error) { - sharedConfig := server.Config{ - Root: Root, - Middleware: make(map[string][]middleware.Middleware), - ConfigFile: filename, - AppName: app.Name, - AppVersion: app.Version, - } - - // It is crucial that directives are executed in the proper order. - for _, dir := range directiveOrder { - // Execute directive if it is in the server block - if tokens, ok := sb.Tokens[dir.name]; ok { - // Each setup function gets a controller, which is the - // server config and the dispenser containing only - // this directive's tokens. - controller := &setup.Controller{ - Config: &sharedConfig, - Dispenser: parse.NewDispenserTokens(filename, tokens), - } - - midware, err := dir.setup(controller) - if err != nil { - return sharedConfig, err - } - if midware != nil { - // TODO: For now, we only support the default path scope / - sharedConfig.Middleware["/"] = append(sharedConfig.Middleware["/"], midware) - } - } - } - - return sharedConfig, nil -} - // arrangeBindings groups configurations by their bind address. For example, // a server that should listen on localhost and another on 127.0.0.1 will // be grouped into the same address: 127.0.0.1. It will return an error @@ -125,8 +104,8 @@ func serverBlockToConfig(filename string, sb parse.ServerBlock) (server.Config, // bind address to list of configs that would become VirtualHosts on that // server. Use the keys of the returned map to create listeners, and use // the associated values to set up the virtualhosts. -func arrangeBindings(allConfigs []server.Config) (Group, error) { - addresses := make(Group) +func arrangeBindings(allConfigs []server.Config) (map[*net.TCPAddr][]server.Config, error) { + addresses := make(map[*net.TCPAddr][]server.Config) // Group configs by bind address for _, conf := range allConfigs { @@ -234,8 +213,9 @@ func validDirective(d string) bool { return false } -// NewDefault creates a default configuration using the default -// root, host, and port. +// NewDefault makes a default configuration, which +// is empty except for root, host, and port, +// which are essentials for serving the cwd. func NewDefault() server.Config { return server.Config{ Root: Root, @@ -244,9 +224,8 @@ func NewDefault() server.Config { } } -// Default makes a default configuration which -// is empty except for root, host, and port, -// which are essentials for serving the cwd. +// Default obtains a default config and arranges +// bindings so it's ready to use. func Default() (Group, error) { return arrangeBindings([]server.Config{NewDefault()}) } diff --git a/config/parse/parse.go b/config/parse/parse.go index dbb62a360..b44041d4f 100644 --- a/config/parse/parse.go +++ b/config/parse/parse.go @@ -6,7 +6,7 @@ import "io" // ServerBlocks parses the input just enough to organize tokens, // in order, by server block. No further parsing is performed. // Server blocks are returned in the order in which they appear. -func ServerBlocks(filename string, input io.Reader) ([]ServerBlock, error) { +func ServerBlocks(filename string, input io.Reader) ([]serverBlock, error) { p := parser{Dispenser: NewDispenser(filename, input)} blocks, err := p.parseAll() return blocks, err diff --git a/config/parse/parsing.go b/config/parse/parsing.go index 4fb1f3dfd..6ec04dced 100644 --- a/config/parse/parsing.go +++ b/config/parse/parsing.go @@ -9,12 +9,12 @@ import ( type parser struct { Dispenser - block ServerBlock // current server block being parsed + block serverBlock // current server block being parsed eof bool // if we encounter a valid EOF in a hard place } -func (p *parser) parseAll() ([]ServerBlock, error) { - var blocks []ServerBlock +func (p *parser) parseAll() ([]serverBlock, error) { + var blocks []serverBlock for p.Next() { err := p.parseOne() @@ -30,7 +30,7 @@ func (p *parser) parseAll() ([]ServerBlock, error) { } func (p *parser) parseOne() error { - p.block = ServerBlock{Tokens: make(map[string][]token)} + p.block = serverBlock{Tokens: make(map[string][]token)} err := p.begin() if err != nil { @@ -87,7 +87,7 @@ func (p *parser) addresses() error { break } - if tkn != "" { + if tkn != "" { // empty token possible if user typed "" in Caddyfile // Trailing comma indicates another address will follow, which // may possibly be on the next line if tkn[len(tkn)-1] == ',' { @@ -102,7 +102,7 @@ func (p *parser) addresses() error { if err != nil { return err } - p.block.Addresses = append(p.block.Addresses, Address{host, port}) + p.block.Addresses = append(p.block.Addresses, address{host, port}) } // Advance token and possibly break out of loop or return error @@ -301,15 +301,14 @@ func standardAddress(str string) (host, port string, err error) { } type ( - // ServerBlock associates tokens with a list of addresses + // serverBlock associates tokens with a list of addresses // and groups tokens by directive name. - ServerBlock struct { - Addresses []Address + serverBlock struct { + Addresses []address Tokens map[string][]token } - // Address represents a host and port. - Address struct { + address struct { Host, Port string } ) diff --git a/config/parse/parsing_test.go b/config/parse/parsing_test.go index 197ec0da2..c8a7ef0be 100644 --- a/config/parse/parsing_test.go +++ b/config/parse/parsing_test.go @@ -59,7 +59,7 @@ func TestStandardAddress(t *testing.T) { func TestParseOneAndImport(t *testing.T) { setupParseTests() - testParseOne := func(input string) (ServerBlock, error) { + testParseOne := func(input string) (serverBlock, error) { p := testParser(input) p.Next() // parseOne doesn't call Next() to start, so we must err := p.parseOne() @@ -69,22 +69,22 @@ func TestParseOneAndImport(t *testing.T) { for i, test := range []struct { input string shouldErr bool - addresses []Address + addresses []address tokens map[string]int // map of directive name to number of tokens expected }{ - {`localhost`, false, []Address{ + {`localhost`, false, []address{ {"localhost", ""}, }, map[string]int{}}, {`localhost - dir1`, false, []Address{ + dir1`, false, []address{ {"localhost", ""}, }, map[string]int{ "dir1": 1, }}, {`localhost:1234 - dir1 foo bar`, false, []Address{ + dir1 foo bar`, false, []address{ {"localhost", "1234"}, }, map[string]int{ "dir1": 3, @@ -92,7 +92,7 @@ func TestParseOneAndImport(t *testing.T) { {`localhost { dir1 - }`, false, []Address{ + }`, false, []address{ {"localhost", ""}, }, map[string]int{ "dir1": 1, @@ -101,7 +101,7 @@ func TestParseOneAndImport(t *testing.T) { {`localhost:1234 { dir1 foo bar dir2 - }`, false, []Address{ + }`, false, []address{ {"localhost", "1234"}, }, map[string]int{ "dir1": 3, @@ -109,7 +109,7 @@ func TestParseOneAndImport(t *testing.T) { }}, {`http://localhost https://localhost - dir1 foo bar`, false, []Address{ + dir1 foo bar`, false, []address{ {"localhost", "http"}, {"localhost", "https"}, }, map[string]int{ @@ -118,7 +118,7 @@ func TestParseOneAndImport(t *testing.T) { {`http://localhost https://localhost { dir1 foo bar - }`, false, []Address{ + }`, false, []address{ {"localhost", "http"}, {"localhost", "https"}, }, map[string]int{ @@ -127,7 +127,7 @@ func TestParseOneAndImport(t *testing.T) { {`http://localhost, https://localhost { dir1 foo bar - }`, false, []Address{ + }`, false, []address{ {"localhost", "http"}, {"localhost", "https"}, }, map[string]int{ @@ -135,13 +135,13 @@ func TestParseOneAndImport(t *testing.T) { }}, {`http://localhost, { - }`, true, []Address{ + }`, true, []address{ {"localhost", "http"}, }, map[string]int{}}, {`host1:80, http://host2.com dir1 foo bar - dir2 baz`, false, []Address{ + dir2 baz`, false, []address{ {"host1", "80"}, {"host2.com", "http"}, }, map[string]int{ @@ -151,7 +151,7 @@ func TestParseOneAndImport(t *testing.T) { {`http://host1.com, http://host2.com, - https://host3.com`, false, []Address{ + https://host3.com`, false, []address{ {"host1.com", "http"}, {"host2.com", "http"}, {"host3.com", "https"}, @@ -161,7 +161,7 @@ func TestParseOneAndImport(t *testing.T) { dir1 foo { bar baz } - dir2`, false, []Address{ + dir2`, false, []address{ {"host1.com", "1234"}, {"host2.com", "https"}, }, map[string]int{ @@ -175,7 +175,7 @@ func TestParseOneAndImport(t *testing.T) { } dir2 { foo bar - }`, false, []Address{ + }`, false, []address{ {"127.0.0.1", ""}, }, map[string]int{ "dir1": 5, @@ -183,13 +183,13 @@ func TestParseOneAndImport(t *testing.T) { }}, {`127.0.0.1 - unknown_directive`, true, []Address{ + unknown_directive`, true, []address{ {"127.0.0.1", ""}, }, map[string]int{}}, {`localhost dir1 { - foo`, true, []Address{ + foo`, true, []address{ {"localhost", ""}, }, map[string]int{ "dir1": 3, @@ -197,7 +197,15 @@ func TestParseOneAndImport(t *testing.T) { {`localhost dir1 { - }`, false, []Address{ + }`, false, []address{ + {"localhost", ""}, + }, map[string]int{ + "dir1": 3, + }}, + + {`localhost + dir1 { + } }`, true, []address{ {"localhost", ""}, }, map[string]int{ "dir1": 3, @@ -209,18 +217,18 @@ func TestParseOneAndImport(t *testing.T) { foo } } - dir2 foo bar`, false, []Address{ + dir2 foo bar`, false, []address{ {"localhost", ""}, }, map[string]int{ "dir1": 7, "dir2": 3, }}, - {``, false, []Address{}, map[string]int{}}, + {``, false, []address{}, map[string]int{}}, {`localhost dir1 arg1 - import import_test1.txt`, false, []Address{ + import import_test1.txt`, false, []address{ {"localhost", ""}, }, map[string]int{ "dir1": 2, @@ -228,16 +236,20 @@ func TestParseOneAndImport(t *testing.T) { "dir3": 1, }}, - {`import import_test2.txt`, false, []Address{ + {`import import_test2.txt`, false, []address{ {"host1", ""}, }, map[string]int{ "dir1": 1, "dir2": 2, }}, - {``, false, []Address{}, map[string]int{}}, + {`import import_test1.txt import_test2.txt`, true, []address{}, map[string]int{}}, - {`""`, false, []Address{}, map[string]int{}}, + {`import not_found.txt`, true, []address{}, map[string]int{}}, + + {`""`, false, []address{}, map[string]int{}}, + + {``, false, []address{}, map[string]int{}}, } { result, err := testParseOne(test.input) @@ -282,43 +294,43 @@ func TestParseOneAndImport(t *testing.T) { func TestParseAll(t *testing.T) { setupParseTests() - testParseAll := func(input string) ([]ServerBlock, error) { - p := testParser(input) - return p.parseAll() - } - for i, test := range []struct { input string shouldErr bool - numBlocks int + addresses [][]address // addresses per server block, in order }{ - {`localhost`, false, 1}, + {`localhost`, false, [][]address{ + {{"localhost", ""}}, + }}, - {`localhost { - dir1 - }`, false, 1}, + {`localhost:1234`, false, [][]address{ + []address{{"localhost", "1234"}}, + }}, - {`http://localhost https://localhost - dir1 foo bar`, false, 1}, + {`localhost:1234 { + } + localhost:2015 { + }`, false, [][]address{ + []address{{"localhost", "1234"}}, + []address{{"localhost", "2015"}}, + }}, - {`http://localhost, https://localhost { - dir1 foo bar - }`, false, 1}, + {`localhost:1234, http://host2`, false, [][]address{ + []address{{"localhost", "1234"}, {"host2", "http"}}, + }}, - {`http://host1.com, - http://host2.com, - https://host3.com`, false, 1}, + {`localhost:1234, http://host2,`, true, [][]address{}}, - {`host1 { - } - host2 { - }`, false, 2}, - - {`""`, false, 0}, - - {``, false, 0}, + {`http://host1.com, http://host2.com { + } + https://host3.com, https://host4.com { + }`, false, [][]address{ + []address{{"host1.com", "http"}, {"host2.com", "http"}}, + []address{{"host3.com", "https"}, {"host4.com", "https"}}, + }}, } { - results, err := testParseAll(test.input) + p := testParser(test.input) + blocks, err := p.parseAll() if test.shouldErr && err == nil { t.Errorf("Test %d: Expected an error, but didn't get one", i) @@ -327,11 +339,28 @@ func TestParseAll(t *testing.T) { t.Errorf("Test %d: Expected no error, but got: %v", i, err) } - if len(results) != test.numBlocks { + if len(blocks) != len(test.addresses) { t.Errorf("Test %d: Expected %d server blocks, got %d", - i, test.numBlocks, len(results)) + i, len(test.addresses), len(blocks)) continue } + for j, block := range blocks { + if len(block.Addresses) != len(test.addresses[j]) { + t.Errorf("Test %d: Expected %d addresses in block %d, got %d", + i, len(test.addresses[j]), j, len(block.Addresses)) + continue + } + for k, addr := range block.Addresses { + if addr.Host != test.addresses[j][k].Host { + t.Errorf("Test %d, block %d, address %d: Expected host to be '%s', but was '%s'", + i, j, k, test.addresses[j][k].Host, addr.Host) + } + if addr.Port != test.addresses[j][k].Port { + t.Errorf("Test %d, block %d, address %d: Expected port to be '%s', but was '%s'", + i, j, k, test.addresses[j][k].Port, addr.Port) + } + } + } } } diff --git a/config/setup/controller.go b/config/setup/controller.go index c917a148f..e1e511897 100644 --- a/config/setup/controller.go +++ b/config/setup/controller.go @@ -15,6 +15,7 @@ import ( type Controller struct { *server.Config parse.Dispenser + OncePerServerBlock func(f func()) } // NewTestController creates a new *Controller for diff --git a/config/setup/fastcgi.go b/config/setup/fastcgi.go index ab21ef1f2..a2a7e8794 100644 --- a/config/setup/fastcgi.go +++ b/config/setup/fastcgi.go @@ -31,7 +31,7 @@ func FastCGI(c *Controller) (middleware.Middleware, error) { SoftwareName: c.AppName, SoftwareVersion: c.AppVersion, ServerName: c.Host, - ServerPort: c.Port, // BUG: This is not known until the server blocks are split up... + ServerPort: c.Port, } }, nil } diff --git a/main.go b/main.go index 0740346cf..272cb8fce 100644 --- a/main.go +++ b/main.go @@ -49,7 +49,7 @@ func main() { log.Fatal(err) } - // Load address configurations from highest priority input + // Load config from file addresses, err := loadConfigs() if err != nil { log.Fatal(err) @@ -123,10 +123,9 @@ func isLocalhost(s string) bool { // loadConfigs loads configuration from a file or stdin (piped). // The configurations are grouped by bind address. -// Configuration is obtained from one of three sources, tried +// Configuration is obtained from one of four sources, tried // in this order: 1. -conf flag, 2. stdin, 3. command line argument 4. Caddyfile. -// If none of those are available, a default configuration is -// loaded. +// If none of those are available, a default configuration is loaded. func loadConfigs() (config.Group, error) { // -conf flag if conf != "" { diff --git a/server/server.go b/server/server.go index 8e93c2941..24aa92eb5 100644 --- a/server/server.go +++ b/server/server.go @@ -86,7 +86,7 @@ func (s *Server) Serve() error { go func(vh virtualHost) { // Wait for signal interrupt := make(chan os.Signal, 1) - signal.Notify(interrupt, os.Interrupt, os.Kill) + signal.Notify(interrupt, os.Interrupt, os.Kill) // TODO: syscall.SIGQUIT? (Ctrl+\, Unix-only) <-interrupt // Run callbacks