Propagate ip

[arianvp]

This makes sure that nginx-controller has access to the source IP address of a request.

Fixes #119

[arianvp]

I haven't been able to test this yet because I accidentally removed my local wire app config and I forgot what hacks I needed to get it working. Something with content security policies that didn't work properly. will try to set up again tomorrow.

[arianvp]

This works on my installation of wire!

[tiago-loureiro]

Is it worth making this the default setting? I don't think it breaks anything, any thoughts @jschaul ?

Either way, could you add a potential reference here to make it easier for the reader? Thanks!

[jschaul]

I'm fine making that the default. Do I understand correctly that setting this to DaemonSet ensures there is one pod running on each kubernetes worker node? Or do we need to take care that the amount of replicas matches the amount of workers manually?

Another question: in the demo value, you set Daemonset, in the prod value, you don't set it. Why exactly?

[arianvp]

I was reluctant to change it there because for a "prod" environment running it as a DaemonSet probably isn't the best idea. (If you have 10 worker nodes, you'll have 10 nginx servers).

I saw in our prod version that we have an (commented) nodeSelector field, which can be used to force the pods to run on certain nodes. For example you might have 3 nodes behind your loadbalancer in your 10 node cluster. and then you can do something like a Deploymetnt together with nodeSelector will allow you to force that each of those 3 nodes have nginx-ingress running.

# this could be a line in our kubespray playbook
kubectl label nodes worker1 wire.com/role=ingress
kubectl label nodes worker2 wire.com/role=ingress
kubectl label nodes worker3 wire.com/role=ingress
# worker3-10 are not labeled this

# deployment.yaml
  nodeSelector:
    wire.com/role: ingress

Maybe I should add a comment about that. This sounded like a more sane choice than running nginx on every node.

But again, every prod environment is going to be different with regards to load balancing, and it's hard to give a "right" answer for that usecase. So we should probably instead link to https://kubernetes.github.io/ingress-nginx/deploy/baremetal/ or a more "wire-specific" doc on https://docs.wire.com/ that explains what choices a person should make whilst setting up a Loadbalancer and/or ingress for their k8s cluster

[arianvp]

I'm fine making that the default. Do I understand correctly that setting this to DaemonSet ensures there is one pod running on each kubernetes worker node? Or do we need to take care that the amount of replicas matches the amount of workers manually?

Your assumption is correct. There is one pod running on each worker node.

[jschaul]

I was reluctant to change it there because for a "prod" environment running it as a DaemonSet probably isn't the best idea. (If you have 10 worker nodes, you'll have 10 nginx servers).

I saw in our prod version that we have an (commented) nodeSelector field, which can be used to force the pods to run on certain nodes. For example you might have 3 nodes behind your loadbalancer in your 10 node cluster. and then you can do something like a Deploymetnt together with nodeSelector will allow you to force that each of those 3 nodes have nginx-ingress running.

# this could be a line in our kubespray playbook
kubectl label nodes worker1 wire.com/role=ingress
kubectl label nodes worker2 wire.com/role=ingress
kubectl label nodes worker3 wire.com/role=ingress
# worker3-10 are not labeled this

# deployment.yaml
  nodeSelector:
    wire.com/role: ingress

Maybe I should add a comment about that. This sounded like a more sane choice than running nginx on every node.

But again, every prod environment is going to be different with regards to load balancing, and it's hard to give a "right" answer for that usecase. So we should probably instead link to https://kubernetes.github.io/ingress-nginx/deploy/baremetal/ or a more "wire-specific" doc on https://docs.wire.com/ that explains what choices a person should make whilst setting up a Loadbalancer and/or ingress for their k8s cluster

We can of course link to, or provide a brief description of choices available on the docs. Is there a way to do the kubectl label part in a way that is not ad-hoc? I think at this point in time I would prefer a more automatic way of configuring (with for instance a daemon set) as a default, rather than having to manually know the names of workers, label them, and need to think of labeling another worker again 6 months later when replacing a worker instance. Unless there is a way to do that labeling automatically, what do you think of making the daemonset the default and adding docs on how to do a different setup "in case of large clusters" where the extra cpu/memory of an nginx could be a problem?

[arianvp]

of choices available on the docs. Is there a way to do the kubectl label part in a way that is not ad-hoc? I think at this point in time I would prefer a more automatic way of configuring (with for instance a daemon set) as a default, rather than having to manually know the names of workers, label them, and need to think of labeling another worker again 6 months later when replacing a worker instance.

It is exposed as a variable in kubespray so we could do something like:

# inventory.ini
[ingress]
node1
node2
node3

# group_vars.yaml
ingress:
  node_labels:
    wire.com/role:  ingress

then when replacing nodes or whatever, ansible will do the labelling for us
This is implemented by giving the list node labels as a CLI argument to kubelet:
https://github.com/kubernetes-sigs/kubespray/blob/1be788f7854dfb841a9bda4d4183e867ffb33a0d/roles/kubernetes/node/templates/kubelet.env.v1beta1.j2#L34

[jschaul]

of choices available on the docs. Is there a way to do the kubectl label part in a way that is not ad-hoc? I think at this point in time I would prefer a more automatic way of configuring (with for instance a daemon set) as a default, rather than having to manually know the names of workers, label them, and need to think of labeling another worker again 6 months later when replacing a worker instance.

It is exposed as a variable in kubespray so we could do something like:

# inventory.ini
[ingress]
node1
node2
node3

# group_vars.yaml
ingress:
  node_labels:
    wire.com/role:  ingress

then when replacing nodes or whatever, ansible will do the labelling for us
This is implemented by giving the list node labels as a CLI argument to kubelet:
https://github.com/kubernetes-sigs/kubespray/blob/1be788f7854dfb841a9bda4d4183e867ffb33a0d/roles/kubernetes/node/templates/kubelet.env.v1beta1.j2#L34

Sounds like a plan!

[arianvp]

Maybe then just do that by default? In the case that all nodes are in the ingress group then this does the same as it would if we label the deployment a DaemonSet right?

Well it sort of does. DaemonSet forces that each node runs at least one pod. Having it as a deployment could get you that one ingress node runs two pods, one node runs one pod, and one node runs zero pods (as spreading of replicas for deployments is best-effort), but I think this is fine for our usecase right?

Only problem I can think of is: We have a dumb loadbalancer on the outside, like a fixed set of DNS records that doesn't change, and then one of the nodes is not running the nginx ingress. Because the policy is Local, and we send a request to that node (Due to DNS round robin), that request will be dropped, as kube-proxy will not forward it due to this setting. With daemonset we have a slightly higher chance that there is something running on that node (but again, we cannot theoretically assume it).

[tiago-loureiro]

With daemonset we have a slightly higher chance that there is something running on that node (but again, we cannot theoretically assume it).

Why slightly higher? As said, being a daemonset forces it to run on every node unless we ran out of resources, no?

I also it's preferable to have a default which works out of the box, even if it's perhaps a little resource wasteful but nginx is pretty low on resources in general. You have documented it nicely on the examples and we can add it to our docs as well but for bare metal daemonset + nodePort seems like a sane thing to do.

[arianvp]

a deployment has a spread "attempt" but no guarantee. A daemonset has a spread guarantee but that doesn't mean the pods it has running on each node are actually functioning; it might be in a crashloop the entire time due to a wrong nginx config for example. The point I was trying to make is that you still need something "smart" in front that will not route traffic to dead pods (e.g. a loadbalancer, or a health-check aware DNS server), even if nginx is running on each node.

[jschaul]

Let's go with Tiago's suggestion: by default (in the chart) use a daemonset. In the docs, and overridable, allow to disable that and instead use a tagged-node approach. Would you be fine with that @arianvp ?

[tiago-loureiro]

spread guarantee but that doesn't mean the pods it has running on each node are actually functioning; it might be in a crashloop the entire time

Oh right I see, good point 👍

The point I was trying to make is that you still need something "smart" in front that will not route traffic to dead pods (e.g. a loadbalancer, or a health-check aware DNS server), even if nginx is running on each node.

Yup, clear :)

[arianvp]

Sounds good to me!

[arianvp]

OK I have added the new and shiney explanations to the values.yml
I should probably update https://github.com/wireapp/wire-server-deploy/blob/develop/docs/configuration.md#load-balancer-on-bare-metal-servers to reflect this newly gained knowledge? It's out of date anyway. Or should I move this file to docs.wire.com and then update it there?

[jschaul]

I should probably update https://github.com/wireapp/wire-server-deploy/blob/develop/docs/configuration.md#load-balancer-on-bare-metal-servers to reflect this newly gained knowledge? It's out of date anyway. Or should I move this file to docs.wire.com and then update it there?

Either way works - that file has not yet migrated to docs.wire.com yet. Whether you first update it, then we migrate it, or the other way round, is up to you.

[jschaul]

This file now looks great, with all the explanations. Could you make this the default? By "making it the default", I mean actually moving this to charts/nginx-ingress-controller/values.yaml. That way, we will not have any file under values/nginx-ingress-controller at all, and installing with helm install wire/nginx-ingress-controller will provide one working solution out-of-the-box.