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

lzhecheng · 2023-03-23T02:46:23Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

[Dual-stack] Fix generateAPIPodStatus() of kubelet handling Secondary IP. hostIPs order may not be be consistent. If secondary IP is before primary one, current logic adds primary IP twice into PodIPs, which leads to error: "may specify no more than one IP for each IP family".

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

[Dual-stack] Fix generateAPIPodStatus() of kubelet handling Secondary IP. hostIPs order may not be be consistent. If secondary IP is before primary one, current logic adds primary IP twice into PodIPs, which leads to error: "may specify no more than one IP for each IP family".

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

jackfrancis · 2023-03-23T16:50:23Z

/assign

pkg/kubelet/kubelet_pods.go

k8s-ci-robot · 2023-03-23T17:09:15Z

@aojea: GitHub didn't allow me to request PR reviews from the following users: he, has, a, to, try, organize, this, keep.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

I see, if the first hostIP returns IPA, and the subsequent request returms IPB,IPA in different order, it will add { IPA, IPA}

However, there are a lot of places in the code that depend on the order of the returned address, that is why we have
func (kl *Kubelet) sortPodIPs(podIPs []string) []string {
that is called from
convertStatusToAPIStatus
/cc @danwinship he has a keep to try to organize this

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

aojea · 2023-03-27T10:13:02Z

/assign

lzhecheng · 2023-03-30T01:46:01Z

/test pull-kubernetes-e2e-kind

pkg/kubelet/kubelet_pods.go

bart0sh · 2023-03-30T21:23:48Z

/triage accepted
/priority important-soon

danwinship · 2023-04-10T12:46:07Z

pkg/kubelet/kubelet_pods.go

-					s.PodIPs = append(s.PodIPs, v1.PodIP{IP: hostIPs[1].String()})
+					ok, err := utilnet.IsDualStackIPStrings([]string{s.PodIPs[0].IP, hostIPs[1].String()})
+					if err != nil {
+						kl.recorder.Eventf(pod, v1.EventTypeWarning, "InvalidHostIPs", "host IP is invalid %q", hostIPs[1].String())


You can't get an error here. s.PodIPs is validated and hostIPs comes from a function that only returns valid IPs.

Undetstood, but I think we'd better still handle the error?

kubernetes/pkg/apis/core/validation/validation.go

Lines 7118 to 7119 in 9e571c0

dualStack, err := netutils.IsDualStackIPStrings(service.Spec.ClusterIPs)

if err != nil { // though we check for that earlier. safe > sorry

yeh, I think there is also a linter now or is going to be added to enforce that

danwinship · 2023-04-10T13:36:55Z

pkg/kubelet/kubelet_pods.go

+							s.PodIPs = append(s.PodIPs, v1.PodIP{IP: hostIPs[1].String()})
+						} else {
+							kl.recorder.Eventf(pod, v1.EventTypeWarning, "InvalidHostIPs",
+								"Inconsistent addresses order received from the cloud-provider, IP family of the second host IP %q is the same as the first one configured in the kubelet %q",


The wording here is not great in terms of helping the admin understand what to do.

Also, if we are going to notice the "you requested IPv4 but the cloud provider wants dual-stack IPv6-primary" case, then we should really also notice the "you requested IPv4 but the cloud provider wants single-stack IPv6" case too. (It is likely that the original reporter was also running into that case but just didn't notice yet because it doesn't cause any explicit errors. But it may cause problems later with Services.)

So, starting at line 1627:

if utilnet.IPFamilyOfString(s.HostIP) != utilnet.IPFamilyOf(hostIPs[0]) then we need to log a warning:

if s.HostIP is IPv4 then the problem is that kubelet auto-detected an IPv4 node IP, but the cloud provider wants this to be a single-stack IPv6 or dual-stack IPv6-primary node, and so the fix is for the admin to either (a) pass --node-ip :: to kubelet to tell it to prefer IPv6 like the cloud provider wants, (b) pass --node-ip <EXPLICIT-IPV4-ADDRESS> to request a single-stack IPv4 node with a specific IP. (Once CloudDualStackNodeIPs is beta we can also suggest (c) pass --node-ip <EXPLICIT-IPV4-ADDRESS>,<EXPLICIT-IPV6-ADDRESS>.)

if s.HostIP is IPv6 then the problem is that the admin passed --node-ip :: to request an IPv6 node IP, but the cloud provider wants this to be a single-stack IPv4 or dual-stack IPv4-primary node, and so the fix is for the admin to either (a) not pass --node-ip :: to kubelet and let it default to IPv4-primary like the cloud provider wants, (b)/(c) pass an explicit IPv6 IP as above

regardless of whether it logged a warning, it should do s.HostIP = hostIPs[0].String(), as it does now

if the pod is host-network then:

it should set s.PodIPs[0] to hostIPs[0] if s.PodIPs is unset, as it does now

it should set s.PodIPs[1] to hostIPs[1] if len(hostIPs) == 2 && len(s.PodIPs) == 1 and utilnet.IPFamilyOfString(s.PodIPs[0].IP) != utilnet.IPFamilyOf(hostIPs[1])

(the family mismatch can only happen if we have already logged an error above, so we don't need to warn again here)

if s.HostIP is IPv4 then the problem is that kubelet auto-detected an IPv4 node IP, but the cloud provider

I think you are talking about hostIPs[0]?

Thanks for the detailed explanation. Updated.

lzhecheng · 2023-04-11T09:28:19Z

/retest

aojea · 2023-04-11T14:26:21Z

I think is ok, but defer to Dan, he is driving all this node-ip effort and the person more familiar with it
/assign @danwinship @thockin

lzhecheng · 2023-06-13T02:52:19Z

@danwinship hello, do you have any other comments? Thank you.

danwinship · 2023-06-28T13:06:54Z

pkg/kubelet/kubelet_pods.go

+				ok, err := utilnet.IsDualStackIPStrings([]string{s.HostIP, hostIPs[0].String()})
+				if err != nil {
+					kl.recorder.Eventf(pod, v1.EventTypeWarning, "InvalidHostIPs", "host IPs are invalid %q or %q", s.HostIP, hostIPs[0].String())
+				}
+				if ok {


I think

if utilnet.IPFamilyOfString(s.HostIP) != utilnet.IPFamilyOf(hostIPs[0]) {

is clearer: the error case can't actually happen anyway, and we're comparing exactly two IPs, not scanning a possibly-long list of IPs.

I also prefer clearer code. Updated.

danwinship · 2023-06-28T13:07:49Z

pkg/kubelet/kubelet_pods.go

+				}
+				if ok {
+					// Once CloudDualStackNodeIPs is beta, <EXPLICIT-IPV4-ADDRESS>,<EXPLICIT-IPV6-ADDRESS> can be set to --node-ip.
+					if utilnet.IPFamilyOf(hostIPs[0]) == utilnet.IPv4 {


if utilnet.IsIPv4(hostIPs[0]) {

actually, wait, this is backwards: hostIPs is what the cloud provider chose, s.HostIP is what kubelet detected.

danwinship · 2023-06-28T13:11:48Z

pkg/kubelet/kubelet_pods.go

+						// kubelet auto-detected an IPv4 node IP (hostIPs[0]), but the cloud provider wants this to be
+						// a single-stack IPv6 or dual-stack IPv6-primary node
+						kl.recorder.Eventf(pod, v1.EventTypeWarning, "HostIPsIPFamilyMismatch",
+							"set [::] or an explicit IPv4 address to --node-ip")


The information that is in the comment needs to be in the Event text instead to make it understandable (after which you no longer need the comment).

"Kubelet auto-detected an IPv4 node IP, but the cloud provider selected an IPv6 node IP; pass an explicit `--node-ip` to kubelet to fix this."

danwinship · 2023-06-28T13:16:41Z

pkg/kubelet/kubelet_pods.go

+						// kubelet auto-detected an IPv6 node IP (hostIPs[0]), but the cloud provider wants this to be
+						// a single-stack IPv4 or dual-stack IPv4-primary node
+						kl.recorder.Eventf(pod, v1.EventTypeWarning, "HostIPsIPFamilyMismatch",
+							"do not set --node-ip or set an explicit IPv4 address to --node-ip")


likewise. Also, the fix here is to pass an explicit IPv6 address, not IPv4.

actually, we don't really even need separate errors for the v4 and v6 case. Just replace this all (1627-1646) with:

if s.HostIP != "" && utilnet.IPFamilyOfString(s.HostIP) != utilnet.IPFamilyOf(hostIPs[0]) { kl.recorder.Eventf(pod, v1.EventTypeWarning, "HostIPsIPFamilyMismatch", "Kubelet detected an IPv%s node IP (%s), but the cloud provider selected an IPv%s node IP (%s); pass an explicit `--node-ip` to kubelet to fix this.", utilnet.IPFamilyOfString(s.HostIP), s.HostIP, utilnet.IPFamilyOf(hostIPs[0])), hostIPs[0].String()) }

danwinship · 2023-06-28T13:23:26Z

pkg/kubelet/kubelet_pods.go

@@ -1635,7 +1655,12 @@ func (kl *Kubelet) generateAPIPodStatus(pod *v1.Pod, podStatus *kubecontainer.Po
 				}
 				// Secondary IP is not set #105320
 				if len(hostIPs) == 2 && len(s.PodIPs) == 1 {
-					s.PodIPs = append(s.PodIPs, v1.PodIP{IP: hostIPs[1].String()})
+					ok, err := utilnet.IsDualStackIPStrings([]string{s.PodIPs[0].IP, hostIPs[1].String()})


again, I think using IPFamilyOf != IPFamilyOf would be clearer here, and get rid of the unnecessary error check

hostIPs order may not be be consistent. If secondary IP is before primary one, current logic adds primary IP twice into PodIPs, which leads to error: "may specify no more than one IP for each IP family". In this case, the second IP shouldn't be added. Co-authored-by: Antonio Ojea <antonio.ojea.garcia@gmail.com>

danwinship · 2023-07-03T16:11:05Z

/lgtm
but needs a node approver

k8s-ci-robot · 2023-07-03T16:11:12Z

LGTM label has been added.

Git tree hash: fe173fc6f04c8dc9bb1440ee78c5caa1aca2d19d

lzhecheng · 2023-07-04T01:27:32Z

/assign @Random-Liu
@Random-Liu @thockin could you please help review the PR? It has lgtm now.

thockin

Thanks!

/lgtm
/approve

k8s-ci-robot · 2023-07-07T21:13:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lzhecheng, thockin

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~~ [thockin]

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

k8s-triage-robot · 2023-07-07T23:52:46Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-triage-robot · 2023-07-08T02:33:46Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-ci-robot requested review from odinuge and wzshiming March 23, 2023 02:46

lzhecheng mentioned this pull request Mar 23, 2023

ipv6 + Calico + out of tree cloud-provider is broken kubernetes-sigs/cloud-provider-azure#3401

Closed

lzhecheng force-pushed the fix-generateAPIPodStatus-dualstack branch from 568868e to f35d0e7 Compare March 23, 2023 04:56

k8s-ci-robot assigned jackfrancis Mar 23, 2023

aojea reviewed Mar 23, 2023

View reviewed changes

pkg/kubelet/kubelet_pods.go Outdated Show resolved Hide resolved

k8s-ci-robot requested a review from danwinship March 23, 2023 17:09

k8s-ci-robot assigned aojea Mar 27, 2023

lzhecheng force-pushed the fix-generateAPIPodStatus-dualstack branch 3 times, most recently from 8b36f12 to bc7ee64 Compare March 28, 2023 05:22

SergeyKanzhelev added this to Triage in SIG Node PR Triage Mar 29, 2023

aojea reviewed Mar 30, 2023

View reviewed changes

pkg/kubelet/kubelet_pods.go Outdated Show resolved Hide resolved

aojea reviewed Mar 30, 2023

View reviewed changes

pkg/kubelet/kubelet_pods.go Outdated Show resolved Hide resolved

k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Mar 30, 2023

danwinship suggested changes Apr 10, 2023

View reviewed changes

lzhecheng force-pushed the fix-generateAPIPodStatus-dualstack branch from 7f43437 to c297ee8 Compare April 11, 2023 06:33

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 11, 2023

lzhecheng force-pushed the fix-generateAPIPodStatus-dualstack branch from c297ee8 to c8c6ff8 Compare April 11, 2023 07:40

k8s-ci-robot assigned danwinship and thockin Apr 11, 2023

danwinship reviewed Jun 28, 2023

View reviewed changes

lzhecheng force-pushed the fix-generateAPIPodStatus-dualstack branch from c8c6ff8 to 985cf71 Compare July 3, 2023 06:47

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 3, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2023

k8s-ci-robot assigned Random-Liu Jul 4, 2023

thockin reviewed Jul 7, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2023

k8s-ci-robot merged commit 1e0b4c8 into kubernetes:master Jul 8, 2023
12 checks passed

SIG Node PR Triage automation moved this from Needs Reviewer to Done Jul 8, 2023

k8s-ci-robot added this to the v1.28 milestone Jul 8, 2023

lzhecheng deleted the fix-generateAPIPodStatus-dualstack branch July 8, 2023 08:46

aojea mentioned this pull request Sep 21, 2023

kubelet: lookup node address logic for external provider assign wrong PodIPs to hostNetwork pods #120720

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lzhecheng commented Mar 23, 2023

jackfrancis commented Mar 23, 2023

k8s-ci-robot commented Mar 23, 2023

aojea commented Mar 27, 2023

lzhecheng commented Mar 30, 2023

bart0sh commented Mar 30, 2023

danwinship Apr 10, 2023

lzhecheng Apr 11, 2023

aojea Apr 11, 2023

danwinship Apr 10, 2023

lzhecheng Apr 11, 2023

lzhecheng commented Apr 11, 2023

aojea commented Apr 11, 2023

lzhecheng commented Jun 13, 2023

danwinship Jun 28, 2023

lzhecheng Jul 3, 2023

danwinship Jun 28, 2023

danwinship Jun 28, 2023

danwinship Jun 28, 2023

lzhecheng Jul 3, 2023

danwinship Jun 28, 2023

lzhecheng Jul 3, 2023

danwinship commented Jul 3, 2023

k8s-ci-robot commented Jul 3, 2023

lzhecheng commented Jul 4, 2023

thockin left a comment

k8s-ci-robot commented Jul 7, 2023

k8s-triage-robot commented Jul 7, 2023

k8s-triage-robot commented Jul 8, 2023

	dualStack, err := netutils.IsDualStackIPStrings(service.Spec.ClusterIPs)
	if err != nil { // though we check for that earlier. safe > sorry

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

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

Conversation

lzhecheng commented Mar 23, 2023

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

jackfrancis commented Mar 23, 2023

k8s-ci-robot commented Mar 23, 2023

aojea commented Mar 27, 2023

lzhecheng commented Mar 30, 2023

bart0sh commented Mar 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzhecheng commented Apr 11, 2023

aojea commented Apr 11, 2023

lzhecheng commented Jun 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwinship commented Jul 3, 2023

k8s-ci-robot commented Jul 3, 2023

lzhecheng commented Jul 4, 2023

thockin left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 7, 2023

k8s-triage-robot commented Jul 7, 2023

k8s-triage-robot commented Jul 8, 2023