Change default agent health check port to avoid conflicts #19830

tklauser · 2022-05-16T08:27:08Z

The default value 9876 for the agent health port introduced in commit
efffbdb ("daemon: expose HTTP endpoint on localhost for health
checks") conflicts with Istio's ControlZ port. The latter has been in
use for longer. Because the port is only exposed on localhost for use by
liveness/readiness probe, we can change the default value without
breaking users. Thus, change it to 9879 for which there doesn't seem to
be any documented use.

Fixes #19817

Ref. https://github.com/cilium/cilium/blob/dfa6b157e8e9484c65fd938b2b45a4f5f50c61f9/daemon/cmd/agenthealth.go#L25-L31 Signed-off-by: Tobias Klauser <tobias@cilium.io>

The default value 9876 for the agent health port introduced in commit efffbdb ("daemon: expose HTTP endpoint on localhost for health checks") conflicts with Istio's ControlZ port. The latter has been in use for longer. Because the port is only exposed on localhost for use by liveness/readiness probe, we can change the default value without breaking users. Thus, change it to 9879 for which there doesn't seem to be any documented use. Suggested-by: Joe Stringer <joe@cilium.io> Signed-off-by: Tobias Klauser <tobias@cilium.io>

tklauser · 2022-05-16T08:33:45Z

/test

gandro

Looks pretty straightforward. I assume this does not have any upgrade impact, as this is only used on the pod spec, which is always updated together with the cilium-agent image, correct?

tklauser · 2022-05-16T09:25:05Z

Looks pretty straightforward. I assume this does not have any upgrade impact, as this is only used on the pod spec, which is always updated together with the cilium-agent image, correct?

That's my understanding as well. As far as I can tell it's only used in the startupProbe, livenessProbe and readinessProbe of the cilium-agent DaemonSet:

cilium/install/kubernetes/cilium/templates/cilium-agent/daemonset.yaml

Lines 101 to 166 in dfa6b15

    
           startupProbe: 
        
             httpGet: 
        
               host: {{ .Values.ipv4.enabled | ternary "127.0.0.1" "::1" | quote }} 
        
               path: /healthz 
        
               port: {{ .Values.healthPort }} 
        
               scheme: HTTP 
        
               httpHeaders: 
        
               - name: "brief" 
        
                 value: "true" 
        
             failureThreshold: {{ .Values.startupProbe.failureThreshold }} 
        
             periodSeconds: {{ .Values.startupProbe.periodSeconds }} 
        
             successThreshold: 1 
        
           {{- end }} 
        
           livenessProbe: 
        
             {{- if or .Values.keepDeprecatedProbes $defaultKeepDeprecatedProbes }} 
        
             exec: 
        
               command: 
        
               - cilium 
        
               - status 
        
               - --brief 
        
             {{- else }} 
        
             httpGet: 
        
               host: {{ .Values.ipv4.enabled | ternary "127.0.0.1" "::1" | quote }} 
        
               path: /healthz 
        
               port: {{ .Values.healthPort }} 
        
               scheme: HTTP 
        
               httpHeaders: 
        
               - name: "brief" 
        
                 value: "true" 
        
             {{- end }} 
        
             {{- if semverCompare "<1.20-0" .Capabilities.KubeVersion.Version }} 
        
             # The initial delay for the liveness probe is intentionally large to 
        
             # avoid an endless kill & restart cycle if in the event that the initial 
        
             # bootstrapping takes longer than expected. 
        
             # Starting from Kubernetes 1.20, we are using startupProbe instead 
        
             # of this field. 
        
             initialDelaySeconds: 120 
        
             {{- end }} 
        
             periodSeconds: {{ .Values.livenessProbe.periodSeconds }} 
        
             successThreshold: 1 
        
             failureThreshold: {{ .Values.livenessProbe.failureThreshold }} 
        
             timeoutSeconds: 5 
        
           readinessProbe: 
        
             {{- if or .Values.keepDeprecatedProbes $defaultKeepDeprecatedProbes }} 
        
             exec: 
        
               command: 
        
               - cilium 
        
               - status 
        
               - --brief 
        
             {{- else }} 
        
             httpGet: 
        
               host: {{ .Values.ipv4.enabled | ternary "127.0.0.1" "::1" | quote }} 
        
               path: /healthz 
        
               port: {{ .Values.healthPort }} 
        
               scheme: HTTP 
        
               httpHeaders: 
        
               - name: "brief" 
        
                 value: "true" 
        
             {{- end }} 
        
             {{- if semverCompare "<1.20-0" .Capabilities.KubeVersion.Version }} 
        
             initialDelaySeconds: 5 
        
             {{- end }} 
        
             periodSeconds: {{ .Values.readinessProbe.periodSeconds }} 
        
             successThreshold: 1 
        
             failureThreshold: {{ .Values.readinessProbe.failureThreshold }} 
        
             timeoutSeconds: 5

Otherwise e.g. helm charts are checkout out from master instead of the PR's ref. Signed-off-by: Tobias Klauser <tobias@cilium.io>

tklauser · 2022-05-16T09:54:29Z

L4LB test is failing (https://github.com/cilium/cilium/actions/runs/2330685480) because helm charts for installation are checked out from master instead of the PR branch and thus the wrong port is used for the startup/liveness/readiness probes in that test. I've pushed an additional commit fixing that.

tklauser · 2022-05-16T10:13:35Z

/test

tklauser · 2022-05-16T11:23:00Z

/ci-l4lb

tklauser · 2022-05-16T11:50:04Z

L4LB test passed with DO NOT MERGE commit: https://github.com/cilium/cilium/actions/runs/2331557522

Removing that commit and re-triggering CI.

tklauser · 2022-05-16T11:52:42Z

/test

joestringer

I'm not sure how the l4lb piece relates to the rest, but all of the individual changes LGTM.

tklauser · 2022-05-17T06:20:43Z

I'm not sure how the l4lb piece relates to the rest, but all of the individual changes LGTM.

The change to the l4lb workflow is necessary because before it was using the helm charts from master rather than the PR branch. This lead to a mismatch in the agent health check port numbers between cilium-agent (using the new port 9879) vs. the helm charts from master still using port 9876 for the {startup,liveness,readiness}Probe.

tklauser · 2022-05-17T06:22:00Z

Reviews are in and all required tests apart from L4LB XDP passed. That one was successful with the commit that's part of this PR, see #19830 (comment) and the explanation in #19830 (comment)

Marking as ready to merge.

This should fix issues occuring after the change of the agent health check port in #19830 Signed-off-by: Tobias Klauser <tobias@cilium.io>

tklauser added 2 commits May 16, 2022 10:14

docs: agent health port is only listening on localhost

e7e0397

Ref. https://github.com/cilium/cilium/blob/dfa6b157e8e9484c65fd938b2b45a4f5f50c61f9/daemon/cmd/agenthealth.go#L25-L31 Signed-off-by: Tobias Klauser <tobias@cilium.io>

tklauser added area/daemon Impacts operation of the Cilium daemon. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. needs-backport/1.9 labels May 16, 2022

tklauser requested review from joestringer and a team May 16, 2022 08:27

tklauser requested a review from a team as a code owner May 16, 2022 08:27

tklauser requested a review from a team May 16, 2022 08:27

tklauser requested a review from a team as a code owner May 16, 2022 08:27

tklauser requested review from ldelossa, christarazi and gandro May 16, 2022 08:27

gandro approved these changes May 16, 2022

View reviewed changes

ci: checkout correct ref in l4lb workflow

c6c4fd9

Otherwise e.g. helm charts are checkout out from master instead of the PR's ref. Signed-off-by: Tobias Klauser <tobias@cilium.io>

tklauser requested review from a team as code owners May 16, 2022 09:53

sayboras approved these changes May 16, 2022

View reviewed changes

tklauser force-pushed the pr/agent-health-default-port branch from 4ebe1e1 to c6c4fd9 Compare May 16, 2022 11:50

christarazi approved these changes May 16, 2022

View reviewed changes

joestringer approved these changes May 16, 2022

View reviewed changes

tklauser added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 17, 2022

christarazi merged commit c3b2948 into master May 17, 2022

christarazi deleted the pr/agent-health-default-port branch May 17, 2022 17:41

This was referenced May 17, 2022

v1.11 backports 2022-05-17 #19858

Merged

v1.10 backports 2022-05-17 #19859

Merged

christarazi added backport-pending/1.10 and removed needs-backport/1.10 labels May 18, 2022

jibi added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels May 23, 2022

tklauser added a commit that referenced this pull request May 24, 2022

.github/workflows: bump kind workflow to cilium-cli v0.10.6

61b2bfe

This should fix issues occuring after the change of the agent health check port in #19830 Signed-off-by: Tobias Klauser <tobias@cilium.io>

tklauser mentioned this pull request May 24, 2022

v1.10: .github/workflows: bump kind workflow to cilium-cli v0.10.6 #19934

Merged

tklauser added a commit that referenced this pull request May 24, 2022

.github/workflows: bump v1.10 workflows to cilium-cli v0.10.6

201db79

This should fix issues occuring after the change of the agent health check port in #19830 Signed-off-by: Tobias Klauser <tobias@cilium.io>

tklauser mentioned this pull request May 24, 2022

.github/workflows: bump v1.10 workflows to cilium-cli v0.10.6 #19935

Merged

jibi pushed a commit that referenced this pull request May 24, 2022

.github/workflows: bump kind workflow to cilium-cli v0.10.6

2199c7b

This should fix issues occuring after the change of the agent health check port in #19830 Signed-off-by: Tobias Klauser <tobias@cilium.io>

jibi pushed a commit that referenced this pull request May 24, 2022

.github/workflows: bump v1.10 workflows to cilium-cli v0.10.6

0d0e1b5

This should fix issues occuring after the change of the agent health check port in #19830 Signed-off-by: Tobias Klauser <tobias@cilium.io>

jibi added backport-done/1.10 and removed backport-pending/1.10 labels May 31, 2022

This was referenced Jun 10, 2022

Prepare for release v1.10.12 #20168

Merged

Prepare for release v1.11.6 #20190

Merged

aanm mentioned this pull request Jun 22, 2022

Prepare for release v1.12.0-rc3 #20279

Merged

This was referenced Jun 24, 2022

Add support for the updated (startup|liveness|readiness)Probe.Port numbers in Cilium kubernetes-sigs/kubespray#9031

Merged

Add support for the new Cilium *Probe ports from v1.11.6 kubernetes-sigs/kubespray#9030

Closed

ReillyBrogan mentioned this pull request Jun 29, 2022

Update Cilium to 1.11.6 kubernetes/kops#13917

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Change default agent health check port to avoid conflicts #19830

Change default agent health check port to avoid conflicts #19830

Uh oh!

tklauser commented May 16, 2022

Uh oh!

tklauser commented May 16, 2022

Uh oh!

gandro left a comment

Uh oh!

tklauser commented May 16, 2022

Uh oh!

tklauser commented May 16, 2022

Uh oh!

tklauser commented May 16, 2022

Uh oh!

tklauser commented May 16, 2022

Uh oh!

tklauser commented May 16, 2022

Uh oh!

tklauser commented May 16, 2022

Uh oh!

joestringer left a comment

Uh oh!

tklauser commented May 17, 2022

Uh oh!

tklauser commented May 17, 2022

Uh oh!

Uh oh!

Change default agent health check port to avoid conflicts #19830

Change default agent health check port to avoid conflicts #19830

Uh oh!

Conversation

tklauser commented May 16, 2022

Uh oh!

tklauser commented May 16, 2022

Uh oh!

gandro left a comment

Choose a reason for hiding this comment

Uh oh!

tklauser commented May 16, 2022

Uh oh!

tklauser commented May 16, 2022

Uh oh!

tklauser commented May 16, 2022

Uh oh!

tklauser commented May 16, 2022

Uh oh!

tklauser commented May 16, 2022

Uh oh!

tklauser commented May 16, 2022

Uh oh!

joestringer left a comment

Choose a reason for hiding this comment

Uh oh!

tklauser commented May 17, 2022

Uh oh!

tklauser commented May 17, 2022

Uh oh!

Uh oh!