feat(nginx): Allow proxy buffer size/number configuration.

[warlokkz]

addresses: #293

Adds proxy_buffers, proxy_buffer_size, proxy_busy_buffers_size lines to the nginx config in config.go along with matching Model amendments to AppConfig and default nginx values.

Edit: Only added configuration for the routable-application component, as I'm not sure if you guys want the deis-router and deis-builder proxy_buffers configurable. Also, have yet to add anything to the list of Annotations in the readme, but I will if you give the ok.

[codecov-io]

Codecov Report

Merging #307 into master will increase coverage by 0.34%.

@@            Coverage Diff             @@
##           master     #307      +/-   ##
==========================================
+ Coverage   55.69%   56.04%   +0.34%     
==========================================
  Files           6        6              
  Lines         386      389       +3     
==========================================
+ Hits          215      218       +3     
  Misses        151      151              
  Partials       20       20

Impacted Files	Coverage Δ
nginx/config.go	56.86% <ø> (ø)	:white_check_mark:
model/model.go	47.11% <100%> (+0.77%)	:white_check_mark:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 55a38f6...8fa0f3a. Read the comment docs.

[bacongobbler]

It looks like there's some tabs/spaces conflicting in some of the files causing some indentation errors. Would you mind fixing those up? Thanks!

[bacongobbler]

Jenkins, test this please.

[vdice]

Jenkins, test this please.

[vdice]

@Anichale seeing the following router pod logs on the latest CI run for this/these changes:

2017/01/30 22:00:03 INFO: Starting nginx...
2017/01/30 22:00:03 INFO: nginx started.
2017/01/30 22:00:03 INFO: Router configuration has changed in k8s.
2017/01/30 22:00:03 INFO: Reloading nginx...
2017/01/30 22:00:03 INFO: nginx reloaded.
nginx: [emerg] "proxy_buffers" directive invalid value in /opt/router/conf/nginx.conf:163

(as a result, the router pod never enters a running state)

Artifact used: quay.io/deisci/router:git-615a8ea if needed to debug/replicate issue...

[warlokkz]

Ah, my mistake. I'll pick this up later.

Edit: I think it has to do with enabling the ax|bx format in the byte sizes. The nginx docs showed 4 4k|8k as the default value, but I think I read it wrong. They're just showing the differences between default values that nginx determines from the platform.

[vdice]

@Anichale Do you have access to the Travis build results? (https://travis-ci.org/deis/router/builds/198899466) Seeing the following from make test-cover:

--- FAIL: TestValidAppProxyBufferSize (0.00s)
    model_validation_test.go:387: Using value "1|2", received an unexpected error: Field "proxyBufferSize" value "1|2" does not satisfy constraint /^[1-9]\d*[kKmM]?$/
FAIL
coverage: 29.2% of statements
FAIL    github.com/deis/router/model    0.039s
make: *** [test-cover] Error 1

[vdice]

Jenkins, test this please.

[krancour]

I understand the impetus for adding this feature, but there are some implementation specifics here that need to be addressed.

First, and entirely independent of the rest of my feedback, it's not clear to me from my reading of Nginx docs, but are proxy_buffers and proxy_buffer_size mutually exclusive options? Or should they be? It seems proxy_buffers allows you to configure number and size of buffers, while proxy_buffer_size allows you to configure just size. Let's make sure we're crystal clear on whether it's meaningful to use both of these options together or whether we can get by with just the proxy_buffers directive in the config template and maybe two separate annotations like router.deis.io/nginx.proxyBuffers.number and router.deis.io/nginx.proxyBuffers.size.

tl;dr on the rest: Rename annotations to:

• router.deis.io/nginx.proxyBuffers.number
• router.deis.io/nginx.proxyBuffers.size
• router.deis.io/nginx.proxyBuffers.busySize

Here's why:

1. I would recommend that as with ssl, gzip, and a few other things that are general configuration categories, with each possessing a number of more specific options within, the annotations should look more like router.deis.io/proxyBuffers.<option>. Likewise, the relevant parts of the model should be compartmentalized in a ProxyBuffersConfig type. That should all be fairly easy to address.

2. My second bit of feedback is more nuanced. The configuration options (currently) supported by the router have been very carefully segregated into options that are nginx-specific (or more accurately, specific to our nginx-based router implementation) and options that are not implementation-specific. \ Currently, all the annotations that are applicable to the router's deployment object are implementation-specific (you will notice all such annotations are prefixed with router.deis.io/nginx.) and all the annotations on the routable services are not nginx-specific. (To be fair, the documentation for some of those options mentions Nginx, but it shouldn't.) This latter category of options all refer to general concepts that any theoretical router implementation should be able to work with: domains, certificates, timeouts, etc. i.e. These annotations are the basis of a de-facto spec for other router implementations. (Some do exist in the wild.) \ The options you're adding, however, are nginx-specific-- and not because of the similarity between their names and corresponding Nginx directives, but because the semantics of how a proxy buffer is configured in Nginx (defined in terms of the three very specific options you are exposing) is bleeding through. Think about an alternative router implementation based on Apache or Vulcan, for instance. While general concepts like domain, certificate, or timeout all survive the leap from one implementation to another intact, do specific options like proxyBusyBuffersSize mean anything in those other contexts? \ I see three possible paths forward.

   1. The first is appealing, but seems prohibitively difficult. It would require somehow distilling proxy buffer configuration options into more generic or universal terms that could be interpreted and applied by any router implementation.

   2. The second is that these options move up to the router's deployment resource and join the ranks of all the existing nginx-specific configuration options. This is probably unappealing because those are all global options and wouldn't give you fine-grained control over proxy buffer size for individual applications-- which, I think, is something you want.

   3. The middle ground is that we start allowing implementation-specific options on the routable services. There isn't any existing precedent for this, but I see no problem with establishing that as long as the annotations are renamed appropriately-- i.e. prefixed with router.deis.io/nginx.. (Don't forget all other renaming guidance given earlier in this comment.) I think it would make it clear that these are not part of the de-facto spec that alternative router implementations should follow. The caveat to this, however, is that being implementation-specific, do not expect that the deis CLI or Workflow controller will ever evolve to manage this particular set of new options. These annotations (like a few others) would have to be applied to the routable service manually by an operator.

Does all of that make sense? I know this all sounds very academic, but I think it's important to carefully preserve the option to implement alternative routers.

[warlokkz]

Makes sense, and thanks for taking the time to write out explanations.

I'll go ahead and rewrite the annotations/nginx config in lines with the third path you specified (with nginx specific details like these being opt-in).

As proxy_buffers and proxy_buffer_size are not mutually exclusive - size input from proxy_buffers overrides proxy_buffer_size - I'll only include proxy_buffers as a configurable field with separate size/number annotations.

[krancour]

@Anichale did you happen to have the changes we discuss queued up? Internally, this is something we want to move along sooner rather than later. I stand ready to finish up what you've started, but just want to avoid stepping on toes. I'll be getting to work on it Tues. morning (EST) unless I hear that I shouldn't.

[krancour]

323 handled this and has been merged.