Flawed logic in ingress controller pod discovery

[r0bobo]

NGINX Ingress controller version: 0.45.0

Kubernetes version (use kubectl version): 1.18.9

Environment:

• Cloud provider or hardware configuration: AWS EKS
• OS (e.g. from /etc/os-release): Bottlerocket OS 1.0.5
• Kernel (e.g. uname -a): Linux 5.4.80
• Install tools: Ingress Nginx Helm Chart deployed with ArgoCD
• Others: Ingress is running behind NLB with TLS Termination in NLB

What happened:

When we do a restart of the ingress controller (with kubectl rollout restart deployment) the controller incorrectly removes the address value (.status.loadBalancer.ingress[].hostname). The address is eventually added to the manifest again after ~30s.

This appeared after we upgraded from 0.41.2 -> 0.45.0.

What you expected to happen:

That the .status.loadBalancer.ingress[].hostname field would not be removed by the ingress controller on a restart.

How to reproduce it:

Install kind

• Kind https://kind.sigs.k8s.io/docs/user/quick-start/

Install the ingress controller

kubectl apply -f https://raw.githubusercontent.com/kubernetes/ingress-nginx/controller-v0.45.0/deploy/static/provider/baremetal/deploy.yaml

Install an application that will act as default backend (is just an echo app)

kubectl apply -f https://raw.githubusercontent.com/kubernetes/ingress-nginx/master/docs/examples/http-svc.yaml

Create an ingress (please add any additional annotation required)

echo "
  apiVersion: networking.k8s.io/v1beta1
  kind: Ingress
  metadata:
    name: foo-bar
  spec:
    rules:
    - host: foo.bar
      http:
        paths:
        - backend:
            serviceName: http-svc
            servicePort: 80
          path: /
" | kubectl apply -f -

Restart the ingress controller

Watch the ingress to see the address disappear and reappear:

watch -n 1 kubectl get ingress -A

Restart the controller

kubectl -n ingress-nginx rollout restart deployment ingress-nginx-controller

The following message also appears in the log of the current leader when it shuts down:

ingress-nginx/ingress-nginx-controller-8544f6fcc9-9jjgp[controller]: I0413 13:13:35.582145 7 status.go:132] "removing value from ingress status" address=[172.40.1.2]

Anything else we need to know:

We looked through the code and saw that there was a significant refactoring with the code that figures out if there are any other controller pod instances.
Before the refactor it listed pods based on the hard-coded labels app.kubernetes.io/component, app.kubernetes.io/instance, app.kubernetes.io/name to find other controller pods, but this changed to listing with a selector that is based on all labels assigned to the current pod. This means that the pod-template-hash label is also included in the selector so that the controller does not see the newly created pods and incorrectly assumes that there are no other replicas.
Then I guess it takes ~30s for them to fixed because we need to wait for a new leader to be elected.

/kind bug

[kundan2707]

/assign

[longwuyuan]

For what it is worth the install url is https://raw.githubusercontent.com/kubernetes/ingress-nginx/controller-v0.45.0/deploy/static/provider/baremetal/deploy.yaml And url I see in first message is not the documented one. Can you repost data after using this url and maybe use minikube with a vm driver Thanks, ; Long

…

On Thu, 22 Apr, 2021, 5:41 PM Kundan Kumar, ***@***.***> wrote: /assign — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#7047 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGZVWUHLH3YSS5R7UQUOBLTKAHARANCNFSM423KOQQA> .

[r0bobo]

Ah, right. I re-ran it with the correct install manifests and got exactly the same error:

ingress-nginx/ingress-nginx-controller-6b55b9f44-crz6d[controller]: I0423 06:19:52.569867       7 status.go:132] "removing value from ingress status" address=[172.40.1.2]

We also initially discovered this issue in our production cluster where the ingress was installed from the official Helm chart version 3.27.0.

[longwuyuan]

Hmm...... now I am doubting my reasoning. Guess to study code. Why should the address continue to be in the object's state without the parent depending controller pod existing (in that small window between stopped and running state)?

[r0bobo]

When we do a rolling restart (or an update) there is a new pod which can recieve the traffic so the address should not be removed (and it wasn't before 0.45.0). This code also suggest that that is the intended behavior.

In our case the issue is that the AWS Load Balancer controller removes the ingress-nginx service endpoint from Load Balancers whenever that address disappears because the current leader is restarted, which causes an outage even though the non-leader pods are still live and running.

[longwuyuan]

Rolling restasrt of ingress controller is what you mentioned in the first ever message here. Now are you saying that its your workload that you are doing a restart/rolling-update on etc and NOT the ingress controller ?

[longwuyuan]

/remove-kind bug
/triage needs-information

[Jell]

@longwuyuan in his reply @r0bobo means a rolling restart of the controller, as written in the report:

kubectl -n ingress-nginx rollout restart deployment ingress-nginx-controller

Here is my understanding:

What happens then is that a new replicaset is created for the restart, that gets a new template-hash label. The controller is still deployed and running, and a new leader will be elected (just from the new replicaset instead), and there are pods that can be elected leader (just in the new, restarted replicaset).

So this code:

ingress-nginx/internal/ingress/status/status.go

Lines 127 to 130 in b1c8e30

	if s.isRunningMultiplePods() {
	klog.V(2).InfoS("skipping Ingress status update (multiple pods running - another one will be elected as master)")
	return
	}

Used to be "no another pod running in the deployment", and is now "no other pod running in the replicaset", which can break during rolling restarts or updates if a new leader is not elected in the new replicaset before the last one elected in the previous one is terminated.

[Jell]

I think it is actually easier to see the issue if running a deployment of the controller with a single replica and rolling restart strategy with 100% surge, then the bug is triggered every time. Perhaps @r0bobo you could update the example to reflect that?

[r0bobo]

@Jell I was about to update it but we get a 100% surge by default with one replica. So the error happens every time you do a restart as it is now.

[longwuyuan]

Just to get explicit, request you to kindly put in a note as to the reason for doing this rolling restart with the command kubectl rollout restart deployment .

Also to be explicit, please put in a note that you use helm upgrade to update the ingress-nginx-controller, when a new release is available.