Make podIPs order match node IP family preference #97979

danwinship · 2021-01-12T18:57:48Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Kubelet currently sets pod.status.podIPs directly from the array of IPs returned from the runtime. But there are two problems with this:

pod.status.podIPs is restricted to be at most one IPv4 and at most one IPv6 IP, but the list of IPs from CRI/CNI has no such restriction; if the runtime returned, eg, 3 IPv4 addresses, then kubelet would try to set podIPs to that and then fail.
kubelet is assuming that the IPs are returned in preference order, but CRI/CNI does not actually specify that, and the consensus of the CNI maintainers in DualStack: Ip family of status.podIP can be wrong depending on CNI-plugin #95251 seems to be that we should not be assuming that; kubelet should treat the list as unordered, and impose its own logic on them. (As a concrete example of how this causes problems; Cilium always returns the IPv6 pod IP first if it is configured to do dual-stack, which currently means any Cilium dual-stack kubernetes cluster ends up having IPv6-primary pod IPs, whether the administrator wants that or not.) Although this technically doesn't break anything, it's considered ugly and confusing. (Maybe a bigger problem is that if the CNI plugin returns dual-stack pod IPs in a single-stack cluster, kubelet might end up grabbing a pod IP that is incompatible with the cluster configuration.)

This PR fixes kubelet to always pick an IP of the same family as its --node-ip for the first pod IP, and an IP of the opposite family (if any) for the second pod IP, and ignore any other IPs from the runtime.

Which issue(s) this PR fixes:
Fixes #95251

Does this PR introduce a user-facing change?:

When a CNI plugin returns dual-stack pod IPs, kubelet will now try to respect the
"primary IP family" of the cluster by picking a primary pod IP of the same family
as the (primary) node IP, rather than assuming that the CNI plugin returned the IPs
in the order the administrator wanted (since some CNI plugins don't allow
configuring this).

/sig network
/priority important-longterm
/cc @uablrek @aojea @khenidak
/auto-cc

krmayankk · 2021-01-13T07:40:19Z

So this issue should only be observed in dual stack configurations ?

uablrek · 2021-01-13T08:19:14Z

No, as @danwinship points out;

(Maybe a bigger problem is that if the CNI plugin returns dual-stack pod IPs in a single-stack cluster, kubelet might end up grabbing a pod IP that is incompatible with the cluster configuration.)

So in a single stack cluster with primary family IPv4 where the CNI-plugin returns two addresses but the IPv6 address first you get;

  09:02:35 Server Version: v1.20.1
  09:02:35 CNI-plugin; bridge
  09:02:36 Single-stack cluster. BaseFamily=IPv4, phase.3 dual-stack API
$ kubectl get pod kahttp-6wqm8 -o wide
NAME           READY   STATUS    RESTARTS   AGE     IP          NODE     NOMINATED NODE   READINESS GATES
kahttp-6wqm8   1/1     Running   0          3m15s   1100::402   vm-004   <none>           <none>

NOTE this is a single stack IPv4 cluster with; --feature-gates=IPv6DualStack=false,SCTPSupport=true.

I use bridge CNI-plugin since it allows control of the order of addresses.

This is not as bad for K8s as it looks since endpointslices saves the day;

$ kubectl get endpoints kahttp
NAME     ENDPOINTS                                                     AGE
kahttp   [1100::104]:443,[1100::202]:443,[1100::303]:443 + 5 more...   4m42s
$ kubectl get endpointslices | grep kahttp
kahttp-lt8hr               IPv4          443,80           11.0.2.2,11.0.3.3,11.0.4.2 + 1 more...            5m

It is probably a disaster for other programs that looks at podIP and endpoints, for instance a service mesh.

uablrek · 2021-01-13T08:28:47Z

Same thing with this PR applied;

  09:23:36 Server Version: v1.21.0-alpha.0.786+a098b5b16c9a7f-dirty
  09:23:36 CNI-plugin; bridge
  09:23:36 Single-stack cluster. BaseFamily=IPv4, phase.3 dual-stack API
$ kubectl get pod kahttp-88cdx -o wide
NAME           READY   STATUS    RESTARTS   AGE    IP         NODE     NOMINATED NODE   READINESS GATES
kahttp-88cdx   1/1     Running   0          2m3s   11.0.2.3   vm-002   <none>           <none>
$ kubectl get endpoints kahttp
NAME     ENDPOINTS                                            AGE
kahttp   11.0.1.3:443,11.0.2.3:443,11.0.3.3:443 + 5 more...   2m18s
$ kubectl get endpointslices | grep kahttp
kahttp-48l5j               IPv4          443,80           11.0.2.3,11.0.4.2,11.0.3.3 + 1 more...            2m26s

uablrek

LGTM

aojea · 2021-01-13T09:44:28Z

pull-kubernetes-verify — Job failed.

./hack/update-bazel.sh
./hack/update-gofmt.sh

it LGTM, should we document this somewhere else?

kubelet is assuming that the IPs are returned in preference order, but CRI/CNI does not actually specify that, and the consensus of the CNI maintainers in #95251 seems to be that we should not be assuming that; kubelet should treat the list as unordered, and impose its own logic on them.

danwinship · 2021-01-14T13:06:32Z

pull-kubernetes-verify — Job failed.

sigh, I always forget that

it LGTM, should we document this somewhere else?

it's really a CNI thing, not a kubernetes thing...

aojea · 2021-01-14T13:38:27Z

it's really a CNI thing, not a kubernetes thing...

I mean the kubelet behaviour

In dual-stack clusters, kubelet will now try to respect the "primary IP family" of the
cluster when setting pod.status.podIPs, by picking a primary pod IP of the same family
as the node IP rather than assuming that the CNI plugin returned the IPs in the order
the administrator wanted.

ehashman · 2021-01-19T00:50:06Z

/retest
flake

I'd be +1 to documenting that the order output here matters and why.

ehashman

/lgtm

ehashman · 2021-01-19T00:50:21Z

pkg/kubelet/kubelet_pods_test.go

@@ -2613,3 +2614,137 @@ func TestGenerateAPIPodStatusHostNetworkPodIPs(t *testing.T) {
 		})
 	}
 }
+
+func TestGenerateAPIPodStatusPodIPs(t *testing.T) {


Great tests \o/

+1 great coverage

ehashman · 2021-01-19T00:50:46Z

/triage accepted

aojea · 2021-02-06T11:48:28Z

IMO this must go into K8s v1.21 for dual-stack to go "beta". Enhancement freeze at Feb 9.

totally agree

aojea · 2021-02-13T12:12:21Z

/assign @thockin
for final approval

derekwaynecarr · 2021-03-04T19:56:30Z

the kubelet changes look good to me for sig-node. ordering makes sense here.

/approve

k8s-ci-robot · 2021-03-04T19:56:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aanm, danwinship, derekwaynecarr, uablrek

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/kubelet/OWNERS~~ [derekwaynecarr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

thockin

Need to rebase and nuke BUILD

aojea · 2021-03-04T23:05:42Z

/lgtm
it was already lgtmed before the rebase and there is a wide consensus

uablrek · 2021-03-05T07:38:26Z

Great 👍

This works around rook/rook#7522 by making podIP use the IPv6 address instead of the IPv4 address. As of kubernetes 1.21 with kubernetes/kubernetes#97979, it looks like the podIP family will match the node IP family. The node IP still defaults to IPv4, but specifying it manually works.

Jean-Daniel · 2021-06-29T07:25:54Z

For the record, this fix is only a partial fix, as it misses pod creation case: #103263

Add missing comment about node addresses and podIPs order: kubernetes/kubernetes#95239 kubernetes/kubernetes#97979

k8s-ci-robot requested review from aojea, khenidak and uablrek January 12, 2021 18:57

k8s-ci-robot requested review from derekwaynecarr and feiskyer January 12, 2021 18:59

ehashman added this to Needs Reviewer in SIG Node PR Triage Jan 12, 2021

uablrek approved these changes Jan 13, 2021

View reviewed changes

danwinship force-pushed the pod-ip-family branch from 9b86f65 to 6c7b716 Compare January 14, 2021 13:04

ehashman reviewed Jan 19, 2021

View reviewed changes

k8s-ci-robot assigned ehashman Jan 19, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 19, 2021

k8s-ci-robot assigned thockin Feb 13, 2021

derekwaynecarr moved this from Needs Approver to Done in SIG Node PR Triage Mar 4, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 4, 2021

thockin reviewed Mar 4, 2021

View reviewed changes

Make podIPs order match node IP family preference

5fd1651

danwinship force-pushed the pod-ip-family branch from f1dfb04 to 5fd1651 Compare March 4, 2021 20:46

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2021

k8s-ci-robot assigned aojea Mar 4, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2021

k8s-ci-robot merged commit 9f451c0 into kubernetes:master Mar 5, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 5, 2021

dseomn mentioned this pull request Apr 9, 2021

ceph-mon uses IPv4 address for --public-bind-addr in ipFamily: IPv6 cluster rook/rook#7522

Open

danwinship deleted the pod-ip-family branch May 3, 2021 22:08

VivekThrivikraman-est mentioned this pull request Jun 22, 2021

Dual stack KEP: correcting primary pod IP assignment criteria kubernetes/enhancements#2791

Merged

Jean-Daniel mentioned this pull request Jun 29, 2021

"fieldPath: status.podIP" may use wrong IP family in dual stack cluster. #103263

Closed

aojea mentioned this pull request Jun 29, 2021

podIPs order match node IP family preference (Downward API) #103307

Merged

aojea pushed a commit to aojea/website that referenced this pull request Aug 25, 2021

Update dual-stack node.status

4001b9c

Add missing comment about node addresses and podIPs order: kubernetes/kubernetes#95239 kubernetes/kubernetes#97979

aojea mentioned this pull request Aug 25, 2021

Update dual-stack node.status kubernetes/website#29532

Merged

danwinship mentioned this pull request May 5, 2022

As a user I want to be able to restrict IP families to some pods #109814

Open

chandanD4 mentioned this pull request Aug 9, 2022

For dual stack deployments, node addresses show only one IP if node-ip parameter is passed #111695

Closed

aojea mentioned this pull request Mar 27, 2023

[Dual-stack] Fix generateAPIPodStatus() of kubelet handling Secondary IP #116879

Merged

danielmellado mentioned this pull request Nov 25, 2023

OCPBUGS-21610: Change config to allow ipv6/4 openshift/cluster-monitoring-operator#2166

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make podIPs order match node IP family preference #97979

Make podIPs order match node IP family preference #97979

danwinship commented Jan 12, 2021 •

edited

krmayankk commented Jan 13, 2021

uablrek commented Jan 13, 2021

uablrek commented Jan 13, 2021

uablrek left a comment

aojea commented Jan 13, 2021

danwinship commented Jan 14, 2021

aojea commented Jan 14, 2021

ehashman commented Jan 19, 2021

ehashman left a comment

ehashman Jan 19, 2021

krmayankk Jan 19, 2021

ehashman commented Jan 19, 2021

aojea commented Feb 6, 2021

aojea commented Feb 13, 2021

derekwaynecarr commented Mar 4, 2021

k8s-ci-robot commented Mar 4, 2021

thockin left a comment

aojea commented Mar 4, 2021

uablrek commented Mar 5, 2021

Jean-Daniel commented Jun 29, 2021

Make podIPs order match node IP family preference #97979

Make podIPs order match node IP family preference #97979

Conversation

danwinship commented Jan 12, 2021 • edited

krmayankk commented Jan 13, 2021

uablrek commented Jan 13, 2021

uablrek commented Jan 13, 2021

uablrek left a comment

Choose a reason for hiding this comment

aojea commented Jan 13, 2021

danwinship commented Jan 14, 2021

aojea commented Jan 14, 2021

ehashman commented Jan 19, 2021

ehashman left a comment

Choose a reason for hiding this comment

ehashman Jan 19, 2021

Choose a reason for hiding this comment

krmayankk Jan 19, 2021

Choose a reason for hiding this comment

ehashman commented Jan 19, 2021

aojea commented Feb 6, 2021

aojea commented Feb 13, 2021

derekwaynecarr commented Mar 4, 2021

k8s-ci-robot commented Mar 4, 2021

thockin left a comment

Choose a reason for hiding this comment

aojea commented Mar 4, 2021

uablrek commented Mar 5, 2021

Jean-Daniel commented Jun 29, 2021

danwinship commented Jan 12, 2021 •

edited