feat(*): add configurable server_tokens nginx setting, tests refactoring

[gerred]

There are two significant changes to this PR. I'm marking this as in progress and to start a conversation:

1) Add a server_tokens configurable field to for the nginx conf. This enables users to disable the Server response header field as documented here.

2) Begin adding tests to the nginx module. This will involve a refactoring of the module to allow dependency injection and converting to use writers/buffers rather than directly writing files. This'll also harden the codebase and allow testability. Right now, these changes are encapsulated in the test file, but the intent is to refactor both model and nginx packages to support testability.

Changes I'd like to make:

• [ ] Convert functions to take io.Writer/io.Reader instead of filenames
• [ ] 100% coverage on template options. Still debating the right way to get coverage on this (after template compilation)
• [ ] Make router models more easily exposed or isolatable in tests to reduce test dependencies
• [ ] Add defaults to the template to allow the uninstantiated configs
• [ ] Somehow sanely test commands.go

[deis-bot]

@krancour and @helgi are potential reviewers of this pull request based on my analysis of git blame information. Thanks @gerred!

[arschles]

@gerred IMO this is a great start. A few ideas, inline with each TODO:

• Convert functions to take io.Writer/io.Reader instead of filenames - I'm 100% :+1: on this change. Small nit: can you put it in a separate, tightly scoped PR?
• 100% coverage on template options. Still debating the right way to get coverage on this (after template compilation) - My only idea so far is to hand-build a slice of the options and then test all combinations - if you render each template to a buffer, you could do each combo in parallel
• Make router models more easily exposed or isolatable in tests to reduce test dependencies - Not 100% sure what you mean here. The RouterConfig seems to be the top level model, and its dependencies seem to be all inside the model package. Maybe I'm misunderstanding what dependencies you're referring to?
• Add defaults to the template to allow the uninstantiated configs - :100:, but again maybe in a more tightly scoped PR
• Somehow sanely test commands.go - maybe could be in an integration test, toggled by build tags, which runs in a container that has nginx in the right place

Also, one final note about the template - if you're willing to take on a larger refactor, you could represent the template in Go code. There are 2 options I can think of to do so:

• You could represent the template in Go code as a set of custom builder functions. For example: tmplBuilder := NewRouterTemplate().AddSection("events").AddKeyValPair("worker_connections", routerConfig.MaxWorkerConnections).... This kind of logic resembles a parser, but in reverse - instead of tokens coming into your logic, your logic is outputting them
• I've seen at least one Go templating engine that allows you to write your template code in real Go. One such option that could help is: https://github.com/tkrajina/ftmpl

Either one of those options could potentially help you get to 100% coverage on template options.

[krancour]

I'm not opposed to any of what's been proposed, but also think that any refactor should be separate from the PR adding support for an additional option (server_tokens).

[gerred]

Agreed. I'm happy to leave this PR as is and then open a new working PR to start the other items.

[krancour]

If the proposal can be separated from this feature, then, for me, only https://github.com/deis/router/pull/209#discussion_r68299813 is blocking this feature from being merged my LGTM.

[gerred]

Working on that today! I'll create it as a defaultRouterConfig.

[mboersma]

@gerred is this still in progress or should we close it?

[bacongobbler]

going to close this as inactive. @gerred please feel free to re-open if you feel like taking a crack at this again. Thanks!