Possible undefined behavior in balancer.lua

[zuzzas]

BUG REPORT

NGINX Ingress controller version: nginx-0.25

Kubernetes version (use kubectl version):

Server Version: version.Info{Major:"1", Minor:"15", GitVersion:"v1.15.0", GitCommit:"e8462b5b5dc2584fdcd18e6bcfe9f1e4d970a529", GitTreeState:"clean", BuildDate:"2019-06-19T16:32:14Z", GoVersion:"go1.12.5", Compiler:"gc", Platform:"linux/amd64"}

Environment:

• Cloud provider or hardware configuration: baremetal
• OS (e.g. from /etc/os-release): Ubuntu 16.04.6 LTS
• Kernel (e.g. uname -a): Linux kube-a-1 4.18.0-20-generic #21~18.04.1 SMP Thu May 16 11:54:52 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
• Install tools: kubeadm
• Others:

What happened:
It seems possible to get different balancers in different stages (balance, rewrite, log, etc.) in the same request.

In this function there is a randomness in play which might return different backend name (ergo, balancer) on consecutive invocations: https://github.com/kubernetes/ingress-nginx/blob/master/rootfs/etc/nginx/lua/balancer.lua#L185

get_balancer() is used 3 times in rewrite, balance, and log stages.

What you expected to happen:
Stable way to get balancer that should be used for a single request.

[dmgtn]

Extra information:

1. The problem happens when canary is enabled.
2. For now, this affects only ewma logic.

[ElvinEfendi]

Thanks for the report - this seems like a bug. I'll address it as soon as possible.

Out of curiosity how did you discover this? I haven't dug deep into this yet, but from my understanding this should affect only EWMA stats in a way that we collect less data about right endpoints.

[zuzzas]

@ElvinEfendi
It did not impact us in any way, but we've got curious about getting a balancer from a function. Based on further examination, we've found out that function's return might not be stable across a single request. Thanks to @distol's keen eye. :)

[dmgtn]

Out of curiosity how did you discover this? I haven't dug deep into this yet, but from my understanding this should affect only EWMA stats in a way that we collect less data about right endpoints.

By reading random parts of code... And because it would be really hard to debug not properly working ewma, we decided that it should be reported.

[ElvinEfendi]

Thanks to @distol's keen eye. :)

very keen indeed! Thanks again.

The implication of this bug is very subtle since under enough load EWMA will eventually get stats about all the endpoints anyway. But say you send 5% of traffic to canary and 95% to production. That means it is very likely that EWMA stores the stats about canary endpoints in the production EWMA instance - which means when later on making balancing decision for canary backend it will have less statistics about the endpoints.

[ElvinEfendi]

#4365 fixes this bug. Reviews are welcome!

[dmgtn]

Thank you for the fix. For me, it looks completely as it should be.

[xliu1996-shelly]

Hi, I am encountering this recently as well. But what I want to do is actually want to make the balancer change across different region as I want to add a header dynamically during rewrite phase, and it should goes to canary balancer if the header is set, but it does not work because we will not get balancer if we have already, so I want to make a trick like:

local ngx = ngx
local _M = {}
function _M.rewrite()
  ngx.req.set_header("Region", "cd")
  ngx.ctx.balancer = nil
end
return _M

with set the balancer to nil manually, it will force in balance phase to get a new balancer based on my added header. But I am not sure if there any risks to leverage this trick and is this a best practice to do it?

Thanks very much if you can help! @ElvinEfendi