feat(proxyBuffers): Add router-level and app-level proxy buffer config options

[krancour]

Fixes #293

Alternative to #307

Some credit goes to @Anichale for getting the ball rolling on this. I'm just taking it over the finish line.

cc @jdumars you also have mentioned needing this feature somewhat urgently, so if I can ask you to verify this meets your needs, that would be great!

[codecov-io]

Codecov Report

Merging #323 into master will increase coverage by 0.77%.
The diff coverage is 64.44%.

@@            Coverage Diff             @@
##           master     #323      +/-   ##
==========================================
+ Coverage   55.69%   56.47%   +0.77%     
==========================================
  Files           6        6              
  Lines         386      425      +39     
==========================================
+ Hits          215      240      +25     
- Misses        151      159       +8     
- Partials       20       26       +6

Impacted Files	Coverage Δ
nginx/config.go	56.86% <ø> (ø)	✅
model/model.go	49.18% <64.44%> (+2.83%)	✅

---

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 8ecb82a...2b9074f. Read the comment docs.

[mboersma]

I've been trying to test this with the commented-out service_annotations key in router's values.yaml, but I don't see my annotations being templated correctly into the deployed service. Did we break this, or am I testing wrong?

[mboersma]

It appears that the annotations will only work in this case if put into the top-level values.yaml in the overall Workflow chart. Scratch that; reverse it.

[vdice]

I set values via fetching down the latest dev workflow chart (resolved to v2.11.1-dev-20170301202706-sha.70ccd31) and modified workflow/charts/router/values.yaml with the following relevant section:

service_annotations:
  router.deis.io/nginx.proxyBuffers.number: "10"
  router.deis.io/nginx.proxyBuffers.size: "5k"
  router.deis.io/nginx.proxyBuffers.busySize: "10k"
  router.deis.io/connectTimeout: "35s"

Chart installation was successful. Created and deployed a test app hi (from deis/example-go) and see the following values:

$ kd exec -it deis-router-4173122539-0lhhh cat /opt/router/conf/nginx.conf
...
	server {
		listen 8080;
		server_name ~^hi\.(?<domain>.+)$;
		server_name_in_redirect off;
		port_in_redirect off;
		set $app_name "hi";

		vhost_traffic_status_filter_by_set_key hi application::*;

		location / {
			proxy_buffering off;
			proxy_buffers 8 4k;
			proxy_busy_buffers_size 8k;
			proxy_set_header Host $host;
			proxy_set_header X-Forwarded-For $remote_addr;
			proxy_set_header X-Forwarded-Proto $access_scheme;
			proxy_set_header X-Forwarded-Port $forwarded_port;
			proxy_redirect off;
			proxy_connect_timeout 30s;
			proxy_send_timeout 1300s;
			proxy_read_timeout 1300s;
			proxy_http_version 1.1;
			proxy_set_header Upgrade $http_upgrade;
			proxy_set_header Connection $connection_upgrade;
			proxy_pass http://10.131.246.200:80;
		}

	}

Note how the values above don't match the customized values intended from values.yaml. Also, if helpful:

$ k get svc -n hi -o yaml
apiVersion: v1
items:
- apiVersion: v1
  kind: Service
  metadata:
    annotations:
      router.deis.io/domains: hi
      router.deis.io/maintenance: "False"
      router.deis.io/ssl.enforce: "False"
    creationTimestamp: 2017-03-03T18:29:50Z
    labels:
      app: hi
      heritage: deis
      router.deis.io/routable: "true"
    name: hi
    namespace: hi
    resourceVersion: "1610125"
    selfLink: /api/v1/namespaces/hi/services/hi
    uid: 63681d6b-003f-11e7-bd36-42010a800074
  spec:
    clusterIP: 10.131.246.200
    ports:
    - name: http
      port: 80
      protocol: TCP
      targetPort: 5000
    selector:
      app: hi
      heritage: deis
      type: web
    sessionAffinity: None
    type: ClusterIP
  status:
    loadBalancer: {}
kind: List
metadata: {}
resourceVersion: ""
selfLink: ""

[mboersma]

Current problem for me: using helm v2.2.1 and the deis-dev Workflow chart, I edit workflow/values.yaml router section to contain this:

  service_annotations:
    router.deis.io/nginx.proxyBuffers.number: "10"

This leads to an error:

$ helm install --namespace=deis ./workflow/
Error: release eloping-molly failed: Service in version "v1" cannot be handled as a Service: [pos 107]: json: expect char '"' but got char '1'

Seems ok when editing the router/values.yaml file.

[mboersma]

If I set all the values in the router/values.yaml, I can install the chart. But the nginx.conf doesn't reflect those values, even after I rebuilt from the updated commit.

[mboersma]

This appears to be because the root-level values.yaml in the Workflow "umbrella" chart has an empty service_annotations entry. This is respected and the corresponding value in router/values.yaml is ignored.

If I comment out service_annotations completely in the root-level values.yaml, I get template parsing errors from my changes to router/values.yaml, as noted above:

$ helm upgrade edgy-goat ./workflow/
Error: UPGRADE FAILED: [pos 196]: json: expect char '"' but got char '1'

[krancour]

@mboersma @vdice let me clarify what's been going on in terms of why you guys have had some difficulty...

Prior to this morning, this PR was titled "Add app-level proxy buffer config options." "app level" were the operative words. Adding annotations to the router's own deployment or service doesn't effect the addition of those annotations to the "routable services" (i.e. services for individual applications).

An aside: Adding annotations to the router's deployment effects "global" / router-level configuration changes. Adding annotations to the router's service mainly effects whatever load balancer might have been provisioned for the router (depends on what cloud provider, if any, is in play). See #327 for an example of that being used to change an ELB setting if on AWS.

Ok... back to the story at hand... this PR was aimed only at app-level configuration before this morning. Since the deis CLI and the controller have no specific support for managing these settings, the only way to apply them to any given application would have been to manually apply these annotations to a given application's service using kubectl edit.

In a separate thread with @jdumars, I've learned that an important customer wants to set proxy number, size, etc. globally (i.e. at the router-level) and have that apply to all applications. That struck me as heavy-handed, because I think the conditions that trigger someone to tweak those settings probably don't occur often.

So what I've done now is spent most of today updating this PR so that all of these settings can be managed globally, but also overridden on an app-by-app basis if so desired... hopefully that makes everyone happy. 😄 (I've changed the PR's name accordingly to reflect this, btw.)

So... just to ensure we're all on the same page now. To set router-level configuration, add annotations to the router's deployment. This can allegedly be done by editing values.yaml, although I have never tried it personally. To override any such settings at the application level, you need to apply the annotations directly to the application service(s) in question using kubectl edit.

Slack me with any follow-up questions.

[krancour]

And based on my previous comment, I'm wondering if #325 is perhaps not actually an issue, but rather a symptom of this misunderstanding.

[mboersma]

I'm wondering if #325 is perhaps not actually an issue

It is an issue, in that there doesn't appear to be a way to set deployment_annotations or service_annotations successfully. It's not an issue created by this PR though--looks to have been broken before that.

PR #328 seems to fix it, but I'm not sure if it might cause other subtle issues by blindly quoting all annotation values.

[krancour]

@mboersma

I'm not sure if it might cause other subtle issues by blindly quoting all annotation values

See my comment on that: #328 (comment)

[mboersma]

In conjunction with #328, I rebuilt and re-tested this and see the deployment annotations making it all the way through to the main nginx.conf section. I'll test with app service-specific annotations next.

[bacongobbler]

^^ double-confirming that deployment annotations work for me as well.

><> cat workflow/values.yaml | grep proxyBuffers
    router.deis.io/nginx.proxyBuffers.enabled: "true"
><> kd exec deis-router-2009517868-1f1pv cat /opt/router/conf/nginx.conf | grep proxy_buffering
                        proxy_buffering on;
                        proxy_buffering on;
                        proxy_buffering on;
                        proxy_buffering on;

[mboersma]

I made annotations to app services and they do appear to override the more global defaults correctly inside nginx.conf.