Use ip address from CNI output #68876

AlexeyPerevalov · 2018-09-20T07:24:36Z

Before this patch Kubernetes obtains ip address from container's eth
device. But in case when containers work only with loopback device and
with vhostuser (it's not device it's UNIX domain socket file) Kubernetes
can't rely on this approach, it should rely on CNI output. Most of plugins
provide IP address of created port.

This patch also touches unit test, CNI spec declares ip field as mandatory
field of ADD/GET command output. But some users may relay on current behaviour
so code where ip is empty and obtained from container's eth is preserved,
but it's not covered in unit test.

Signed-off-by: Alexey Perevalov a.perevalov@samsung.com
Fixes #65756

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

Before this patch Kubernetes obtains ip address from container's eth device. But in case when containers work only with loopback device and with vhostuser (it's not device it's UNIX domain socket file) Kubernetes can't rely on this approach, it should rely on CNI output. Most of plugins provide IP address of created port. This patch also touches unit test, CNI spec declares ip field as mandatory field of ADD/GET command output. But some users may relay on current behaviour so code where ip is empty and obtained from container's eth is preserved, but it's not covered in unit test. Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com> Fixes kubernetes#65756

k8s-ci-robot · 2018-09-20T07:24:38Z

@AlexeyPerevalov: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

AlexeyPerevalov · 2018-09-20T07:30:13Z

/assign @dchen1107

AlexeyPerevalov · 2018-09-20T07:30:26Z

/assign @dcbw

dcbw · 2018-09-20T16:49:16Z

/ok-to-test

dcbw · 2018-09-20T16:50:10Z

/sig network

dcbw · 2018-09-25T15:22:25Z

pkg/kubelet/dockershim/network/cni/cni.go

+		return nil, nil
+	}
+
+	curRes, err := cnicurrent.NewResultFromResult(res)


Should also log result conversion errors here? Otherwise the pod IP cache will be empty for the pod even though the CNI call succeeded.

dcbw · 2018-09-25T15:39:57Z

pkg/kubelet/dockershim/network/cni/cni.go

+	curRes, err := cnicurrent.NewResultFromResult(res)
+	if curRes != nil && len(curRes.IPs) != 0 {
+		glog.V(4).Infof("IP address for CNI network was set from plugin's output", netConf.Name, netConf.Plugins[0].Network.Type)
+		plugin.SetPodIPs(podSandboxID, curRes.IPs[0].Address.IP.To4().String())


A couple of issues with this block, which is too simple:

The first IP address in the list might be for any interface, not just the pod's interface. CNI 0.3.0 and later plugins report multiple interfaces and IP addresses, and you need to make sure the IP is actually for the pod interface and not for say the host's bridge interface.

The code isn't IPv6 compatible due to the To4()

It should probably warn if the pod interface it finds is not called "eth0", which is what dockershim explicitly requests and which plugins are required to honor by the CNI spec

Instead, something like this:

// Find the pod's IP (ignoring any non-sandbox interfaces) var podIP string for _, ip := range curRes.IPs { if ip.Interface != nil { if len(curRes.Interfaces) > *ip.Interface { intf := curRes.Interfaces[*ip.Interface] if intf.Sandbox != "" && intf.Name == network.DefaultInterfaceName { // Found our sandbox interface, use the IP podIP = ip.Address.IP.String() break } } } else if podIP == "" { // Fall back to first un-associated address for compatibility // with older CNI plugins podIP = ip.Address.IP.String() } }

dcbw · 2018-09-25T15:40:44Z

pkg/kubelet/dockershim/network/cni/cni.go

@@ -333,6 +347,22 @@ func (plugin *cniNetworkPlugin) deleteFromNetwork(network *cniNetwork, podName s
 	return nil
 }

+func (plugin *cniNetworkPlugin) SetPodIPs(podSandboxID kubecontainer.ContainerID, ipAddress string) {


This function only sets one pod IP address, so it should probably be SetPodIP() (eg, not plural)

dcbw · 2018-09-25T15:41:23Z

pkg/kubelet/dockershim/network/cni/cni.go

+	}
+}
+
+func (plugin *cniNetworkPlugin) GetPodIPs(podSandboxID kubecontainer.ContainerID) string {


Same here, function name probably shouldn't be plural.

dcbw · 2018-09-25T15:49:03Z

pkg/kubelet/dockershim/network/cni/cni_others.go

@@ -60,6 +61,11 @@ func (plugin *cniNetworkPlugin) platformInit() error {
 // TODO: Use the addToNetwork function to obtain the IP of the Pod. That will assume idempotent ADD call to the plugin.
 // Also fix the runtime's call to Status function to be done only in the case that the IP is lost, no need to do periodic calls
 func (plugin *cniNetworkPlugin) GetPodNetworkStatus(namespace string, name string, id kubecontainer.ContainerID) (*network.PodNetworkStatus, error) {
+


Should have a comment here about the ordering of how to get the pod's IP address.

But really, we should simply stop jumping into the namespace to check the pod's IP, and just rely on the CNI plugin's result since that result is authoritative. Then we can get rid of the code in docker_sandbox.go in getIP() that checks Docker's NetworkSettings too. Some of the Windows code could also be consolidated perhaps.

I didn't remove code related to nsenter, because I hope to keep compatibility with CNI plugins which relies on it. But frankly saying I don't know such plugins, I see workarounds in kuryr-kubernetes and intel's vhostuser can work only in couple with multus - it's the same workaround.
So lets remove nsenter code ;)
Do you expect that in separate patch?

I found, just removing nsenter approach it's not good idea, due to GetPodNetworkStatus is called after kubelet restart, and IP in plugin is empty. I think it's better to store that ip in etcd for that case.

k8s-ci-robot · 2018-10-05T12:55:03Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AlexeyPerevalov
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: dcbw

If they are not already assigned, you can assign the PR to them by writing /assign @dcbw in a comment when ready.

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/dockershim/network/OWNERS

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

Before this patch Kubernetes obtained ip address from container's eth device. But in case when containers work only with loopback device and with vhostuser (it's not device it's UNIX domain socket file) Kubernetes can't rely on this approach, it should rely on CNI output. Most of plugins provide IP address of created port. This patch also touches unit test, CNI spec declares ip field as mandatory field of ADD/GET command output. But some users may relay on current behaviour so code where ip is empty and obtained from container's eth is preserved, but it's not covered in unit test. Signed-off-by: Alexey Perevalov <a.perevalov@samsung.com> Fixes kubernetes#65756

k8s-ci-robot · 2018-10-05T15:25:58Z

@AlexeyPerevalov: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-verify	`36e20ac`	link	`/test pull-kubernetes-verify`
pull-kubernetes-e2e-kops-aws	`36e20ac`	link	`/test pull-kubernetes-e2e-kops-aws`
pull-kubernetes-local-e2e-containerized	`36e20ac`	link	`/test pull-kubernetes-local-e2e-containerized`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

AlexeyPerevalov · 2018-10-12T15:29:10Z

/assign @dcbw

k8s-ci-robot requested review from bowei and caseydavenport September 20, 2018 07:24

k8s-ci-robot assigned dchen1107 Sep 20, 2018

k8s-ci-robot assigned dcbw Sep 20, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 20, 2018

k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Sep 20, 2018

dcbw suggested changes Sep 25, 2018

View reviewed changes

Merge branch 'master' into GetIPFromCNIOutput

9546ccb

AlexeyPerevalov closed this Oct 15, 2018

AlexeyPerevalov mentioned this pull request Oct 16, 2018

Use ip address from CNI output #69821

Closed

AlexeyPerevalov deleted the GetIPFromCNIOutput branch August 21, 2020 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ip address from CNI output #68876

Use ip address from CNI output #68876

AlexeyPerevalov commented Sep 20, 2018

k8s-ci-robot commented Sep 20, 2018

AlexeyPerevalov commented Sep 20, 2018

AlexeyPerevalov commented Sep 20, 2018

dcbw commented Sep 20, 2018

dcbw commented Sep 20, 2018

dcbw Sep 25, 2018

dcbw Sep 25, 2018

dcbw Sep 25, 2018

dcbw Sep 25, 2018

dcbw Sep 25, 2018

AlexeyPerevalov Sep 26, 2018

AlexeyPerevalov Oct 5, 2018

k8s-ci-robot commented Oct 5, 2018

k8s-ci-robot commented Oct 5, 2018

AlexeyPerevalov commented Oct 12, 2018

Use ip address from CNI output #68876

Use ip address from CNI output #68876

Conversation

AlexeyPerevalov commented Sep 20, 2018

k8s-ci-robot commented Sep 20, 2018

AlexeyPerevalov commented Sep 20, 2018

AlexeyPerevalov commented Sep 20, 2018

dcbw commented Sep 20, 2018

dcbw commented Sep 20, 2018

dcbw Sep 25, 2018

Choose a reason for hiding this comment

dcbw Sep 25, 2018

Choose a reason for hiding this comment

dcbw Sep 25, 2018

Choose a reason for hiding this comment

dcbw Sep 25, 2018

Choose a reason for hiding this comment

dcbw Sep 25, 2018

Choose a reason for hiding this comment

AlexeyPerevalov Sep 26, 2018

Choose a reason for hiding this comment

AlexeyPerevalov Oct 5, 2018

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 5, 2018

k8s-ci-robot commented Oct 5, 2018

AlexeyPerevalov commented Oct 12, 2018