Dualstack downward api #83123

aramase · 2019-09-25T17:14:16Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:

https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/20180612-ipv4-ipv6-dual-stack.md#container-environment-variables

adds downward api support for podIPs
updates the host file with v4 and v6 pod IPs for dualstack

        env:
        - name: MY_POD_IPS
          valueFrom:
            fieldRef:
              fieldPath: status.podIPs

$ kubectl exec -ti nginx-8bff7fdb7-xdzj6 -- env | grep POD_IPS
MY_POD_IPS=10.244.0.6,fd00::6

$ kubectl exec -ti nginx-8bff7fdb7-xdzj6 -- cat /etc/hosts
# Kubernetes-managed hosts file.
127.0.0.1    localhost
::1    localhost ip6-localhost ip6-loopback
fe00::0    ip6-localnet
fe00::0    ip6-mcastprefix
fe00::1    ip6-allnodes
fe00::2    ip6-allrouters
10.244.0.6    nginx-8bff7fdb7-xdzj6
fd00::6    nginx-8bff7fdb7-xdzj6

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

The following information is available through environment variables:

status.podIPs - the pod's IP addresses

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

KEP - https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/20180612-ipv4-ipv6-dual-stack.md

fejta-bot · 2019-09-26T05:26:53Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

aramase · 2019-10-17T03:44:10Z

/retest

pkg/kubelet/container/helpers.go

pkg/kubelet/kuberuntime/kuberuntime_container_test.go

aramase · 2019-10-17T19:25:20Z

/test pull-kubernetes-e2e-gce

khenidak

Another review, fewer set of comments. Getting close.

add host file write for podIPs update tests remove import alias update type check update type check remove import alias update open api spec add tests update test add tests address review comments update imports remove todo and import alias

aramase · 2019-10-25T19:31:36Z

/test pull-kubernetes-e2e-gce-100-performance

khenidak · 2019-10-29T17:47:34Z

/lgtm

aojea · 2019-10-29T18:17:30Z

/test pull-kubernetes-conformance-kind-ipv6

aramase · 2019-11-06T17:48:19Z

/assign @thockin

andrewsykim · 2019-11-06T21:40:59Z

pkg/kubelet/kubelet_pods.go

 	// Kubernetes only mounts on /etc/hosts if:
 	// - container is not an infrastructure (pause) container
 	// - container is not already mounting on /etc/hosts
 	// - OS is not Windows
 	// Kubernetes will not mount /etc/hosts if:
 	// - when the Pod sandbox is being created, its IP is still unknown. Hence, PodIP will not have been set.
-	mountEtcHostsFile := len(podIP) > 0 && runtime.GOOS != "windows"
-	klog.V(3).Infof("container: %v/%v/%v podIP: %q creating hosts mount: %v", pod.Namespace, pod.Name, container.Name, podIP, mountEtcHostsFile)
+	mountEtcHostsFile := len(podIPs) > 0 && runtime.GOOS != "windows"


This seems scary, is there a reason we can't keep this as len(podIP) > 0? It ensures existing behavior and has the same result since primary pod IP is guaranteed to be set here anyways.

The podIP and podIPs are passed from caller SyncPod. Here podIP is set explicitly to the first value of podIPs - https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kuberuntime/kuberuntime_manager.go#L744-L747. So podIPs is guaranteed to be set right? If podIPs is empty, then podIP is going to be empty here.

This is changing the meaning of the test. Before it was checking whether the podIP was valid (kind of - does it have any bytes). Now it is testing whether the list of podIPs has and values listed.

That is meaningfully different, though I think it will come out the same, since we should never set podIPs[0] if there is not a valid IP. I think. I checked the conversion code and it seems safe.

lachie83 · 2019-11-14T22:38:25Z

/milestone v1.17

thockin

Thanks! Fixes in followups is OK.

/lgtm
/approve

thockin · 2019-11-15T00:41:54Z

pkg/kubelet/container/helpers.go

@@ -45,7 +45,7 @@ type HandlerRunner interface {
 // RuntimeHelper wraps kubelet to make container runtime
 // able to get necessary informations like the RunContainerOptions, DNS settings, Host IP.
 type RuntimeHelper interface {
-	GenerateRunContainerOptions(pod *v1.Pod, container *v1.Container, podIP string) (contOpts *RunContainerOptions, cleanupAction func(), err error)
+	GenerateRunContainerOptions(pod *v1.Pod, container *v1.Container, podIP string, podIPs []string) (contOpts *RunContainerOptions, cleanupAction func(), err error)


nit: isn't podIP always podIPs[0] ? We should not need both. Ok for followup.

@thockin correct, podIP is always podsIPs[0]. Will do a followup PR. Thank you for the review.

thockin · 2019-11-15T00:47:04Z

pkg/kubelet/kubelet_pods.go

 	// Kubernetes only mounts on /etc/hosts if:
 	// - container is not an infrastructure (pause) container
 	// - container is not already mounting on /etc/hosts
 	// - OS is not Windows
 	// Kubernetes will not mount /etc/hosts if:
 	// - when the Pod sandbox is being created, its IP is still unknown. Hence, PodIP will not have been set.
-	mountEtcHostsFile := len(podIP) > 0 && runtime.GOOS != "windows"
-	klog.V(3).Infof("container: %v/%v/%v podIP: %q creating hosts mount: %v", pod.Namespace, pod.Name, container.Name, podIP, mountEtcHostsFile)
+	mountEtcHostsFile := len(podIPs) > 0 && runtime.GOOS != "windows"


This is changing the meaning of the test. Before it was checking whether the podIP was valid (kind of - does it have any bytes). Now it is testing whether the list of podIPs has and values listed.

That is meaningfully different, though I think it will come out the same, since we should never set podIPs[0] if there is not a valid IP. I think. I checked the conversion code and it seems safe.

k8s-ci-robot · 2019-11-15T01:13:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aramase, 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:

~~api/OWNERS~~ [thockin]
~~pkg/apis/core/OWNERS~~ [thockin]
~~pkg/kubelet/OWNERS~~ [thockin]
~~staging/src/k8s.io/api/OWNERS~~ [thockin]

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

aramase · 2019-11-15T03:41:27Z

/test pull-kubernetes-kubemark-e2e-gce-big

k8s-ci-robot requested review from deads2k and derekwaynecarr September 25, 2019 17:15

aramase force-pushed the dualstack-downward-api branch from 3b19ab7 to 7d09498 Compare September 25, 2019 17:47

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 25, 2019

aramase force-pushed the dualstack-downward-api branch from ceb3bf0 to 9fe9d4d Compare September 25, 2019 18:03

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 25, 2019

aramase force-pushed the dualstack-downward-api branch 5 times, most recently from fff4bb3 to b0e439f Compare September 25, 2019 23:01

aramase changed the title ~~[WIP] Dualstack downward api~~ Dualstack downward api Sep 26, 2019

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 26, 2019

khenidak reviewed Oct 17, 2019

View reviewed changes

pkg/kubelet/container/helpers.go Show resolved Hide resolved

khenidak reviewed Oct 17, 2019

View reviewed changes

pkg/kubelet/container/helpers.go Outdated Show resolved Hide resolved

khenidak reviewed Oct 17, 2019

View reviewed changes

pkg/kubelet/kuberuntime/kuberuntime_container_test.go Outdated Show resolved Hide resolved

aramase force-pushed the dualstack-downward-api branch from 2e4391b to 0b451ef Compare October 17, 2019 18:14

khenidak suggested changes Oct 17, 2019

View reviewed changes

add status.podIPs in downward api

af4d18c

add host file write for podIPs update tests remove import alias update type check update type check remove import alias update open api spec add tests update test add tests address review comments update imports remove todo and import alias

aramase force-pushed the dualstack-downward-api branch from d414f48 to af4d18c Compare October 25, 2019 16:19

k8s-ci-robot assigned khenidak Oct 29, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2019

k8s-ci-robot assigned thockin Nov 6, 2019

andrewsykim reviewed Nov 6, 2019

View reviewed changes

k8s-ci-robot added this to the v1.17 milestone Nov 14, 2019

thockin reviewed Nov 15, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2019

k8s-ci-robot merged commit 4e45328 into kubernetes:master Nov 15, 2019

aramase deleted the dualstack-downward-api branch November 15, 2019 06:14

lachie83 referenced this pull request in kubernetes/enhancements Feb 5, 2020

address review comments

8d22ac2

aojea mentioned this pull request Aug 9, 2020

error converting fieldPath: field label not supported: status.podIPs. #93741

Closed

zhouya0 mentioned this pull request Sep 11, 2020

Field selector podIPs not supported but enabled #94719

Closed

aojea mentioned this pull request Feb 4, 2021

dualstack kep updates kubernetes/enhancements#2325

Merged

pacoxu mentioned this pull request Sep 23, 2021

Downward API doesn't support status.podIPs to be used #105203

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dualstack downward api #83123

Dualstack downward api #83123

aramase commented Sep 25, 2019 •

edited

fejta-bot commented Sep 26, 2019

aramase commented Oct 17, 2019

aramase commented Oct 17, 2019

khenidak left a comment

aramase commented Oct 25, 2019

khenidak commented Oct 29, 2019

aojea commented Oct 29, 2019

aramase commented Nov 6, 2019

andrewsykim Nov 6, 2019

aramase Nov 6, 2019

thockin Nov 15, 2019

lachie83 commented Nov 14, 2019

thockin left a comment

thockin Nov 15, 2019

aramase Nov 15, 2019

thockin Nov 15, 2019

k8s-ci-robot commented Nov 15, 2019

aramase commented Nov 15, 2019

Dualstack downward api #83123

Dualstack downward api #83123

Conversation

aramase commented Sep 25, 2019 • edited

fejta-bot commented Sep 26, 2019

aramase commented Oct 17, 2019

aramase commented Oct 17, 2019

khenidak left a comment

Choose a reason for hiding this comment

aramase commented Oct 25, 2019

khenidak commented Oct 29, 2019

aojea commented Oct 29, 2019

aramase commented Nov 6, 2019

andrewsykim Nov 6, 2019

Choose a reason for hiding this comment

aramase Nov 6, 2019

Choose a reason for hiding this comment

thockin Nov 15, 2019

Choose a reason for hiding this comment

lachie83 commented Nov 14, 2019

thockin left a comment

Choose a reason for hiding this comment

thockin Nov 15, 2019

Choose a reason for hiding this comment

aramase Nov 15, 2019

Choose a reason for hiding this comment

thockin Nov 15, 2019

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 15, 2019

aramase commented Nov 15, 2019

aramase commented Sep 25, 2019 •

edited