Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dualstack downward api #83123

Merged
merged 1 commit into from Nov 15, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

Some generated files are not rendered by default. Learn more.

@@ -88,7 +88,8 @@ func ConvertDownwardAPIFieldLabel(version, label, value string) (string, string,
"spec.schedulerName",
"status.phase",
"status.hostIP",
"status.podIP":
"status.podIP",
"status.podIPs":
This conversation was marked as resolved by aramase

This comment has been minimized.

Copy link
@khenidak

khenidak Oct 1, 2019

Contributor

Can we please add a unit test for this?

This comment has been minimized.

Copy link
@aramase

aramase Oct 16, 2019

Author Member

done!

return label, value, nil
// This is for backwards compatibility with old v1 clients which send spec.host
case "spec.host":
@@ -168,6 +168,20 @@ func TestConvertDownwardAPIFieldLabel(t *testing.T) {
expectedLabel: "spec.nodeName",
expectedValue: "127.0.0.1",
},
{
version: "v1",
label: "status.podIPs",
value: "10.244.0.6,fd00::6",
expectedLabel: "status.podIPs",
expectedValue: "10.244.0.6,fd00::6",
},
{
version: "v1",
label: "status.podIPs",
value: "10.244.0.6",
expectedLabel: "status.podIPs",
expectedValue: "10.244.0.6",
},
}
for _, tc := range testCases {
label, value, err := ConvertDownwardAPIFieldLabel(tc.version, tc.label, tc.value)
@@ -1753,7 +1753,7 @@ type EnvVar struct {
// Only one of its fields may be set.
type EnvVarSource struct {
// Selects a field of the pod: supports metadata.name, metadata.namespace, metadata.labels, metadata.annotations,
// metadata.uid, spec.nodeName, spec.serviceAccountName, status.hostIP, status.podIP.
// metadata.uid, spec.nodeName, spec.serviceAccountName, status.hostIP, status.podIP, status.podIPs.
// +optional
FieldRef *ObjectFieldSelector
// Selects a resource of the container: only resources limits and requests
@@ -70,6 +70,7 @@ func addConversionFuncs(scheme *runtime.Scheme) error {
"spec.serviceAccountName",
"status.phase",
"status.podIP",
"status.podIPs",
This conversation was marked as resolved by aramase

This comment has been minimized.

Copy link
@khenidak

khenidak Oct 1, 2019

Contributor

unit test :-)

"status.nominatedNodeName":
return label, value, nil
// This is for backwards compatibility with old v1 clients which send spec.host
@@ -2097,7 +2097,10 @@ var validEnvDownwardAPIFieldPathExpressions = sets.NewString(
"spec.nodeName",
"spec.serviceAccountName",
"status.hostIP",
"status.podIP")
"status.podIP",
// status.podIPs is populated even if IPv6DualStack feature gate
// is not enabled. This will work for single stack and dual stack.
"status.podIPs")
var validContainerResourceFieldPathExpressions = sets.NewString("limits.cpu", "limits.memory", "limits.ephemeral-storage", "requests.cpu", "requests.memory", "requests.ephemeral-storage")

func validateEnvVarValueFrom(ev core.EnvVar, fldPath *field.Path) field.ErrorList {
@@ -4431,6 +4431,15 @@ func TestValidateEnv(t *testing.T) {
},
},
},
{
Name: "abc",
ValueFrom: &core.EnvVarSource{
FieldRef: &core.ObjectFieldSelector{
APIVersion: "v1",
FieldPath: "status.podIPs",
},
},
},
{
Name: "secret_value",
ValueFrom: &core.EnvVarSource{
@@ -4662,7 +4671,7 @@ func TestValidateEnv(t *testing.T) {
},
},
}},
expectedError: `[0].valueFrom.fieldRef.fieldPath: Unsupported value: "metadata.labels": supported values: "metadata.name", "metadata.namespace", "metadata.uid", "spec.nodeName", "spec.serviceAccountName", "status.hostIP", "status.podIP"`,
expectedError: `[0].valueFrom.fieldRef.fieldPath: Unsupported value: "metadata.labels": supported values: "metadata.name", "metadata.namespace", "metadata.uid", "spec.nodeName", "spec.serviceAccountName", "status.hostIP", "status.podIP", "status.podIPs"`,
},
{
name: "metadata.annotations without subscript",
@@ -4675,7 +4684,7 @@ func TestValidateEnv(t *testing.T) {
},
},
}},
expectedError: `[0].valueFrom.fieldRef.fieldPath: Unsupported value: "metadata.annotations": supported values: "metadata.name", "metadata.namespace", "metadata.uid", "spec.nodeName", "spec.serviceAccountName", "status.hostIP", "status.podIP"`,
expectedError: `[0].valueFrom.fieldRef.fieldPath: Unsupported value: "metadata.annotations": supported values: "metadata.name", "metadata.namespace", "metadata.uid", "spec.nodeName", "spec.serviceAccountName", "status.hostIP", "status.podIP", "status.podIPs"`,
},
{
name: "metadata.annotations with invalid key",
@@ -4714,7 +4723,7 @@ func TestValidateEnv(t *testing.T) {
},
},
}},
expectedError: `valueFrom.fieldRef.fieldPath: Unsupported value: "status.phase": supported values: "metadata.name", "metadata.namespace", "metadata.uid", "spec.nodeName", "spec.serviceAccountName", "status.hostIP", "status.podIP"`,
expectedError: `valueFrom.fieldRef.fieldPath: Unsupported value: "status.phase": supported values: "metadata.name", "metadata.namespace", "metadata.uid", "spec.nodeName", "spec.serviceAccountName", "status.hostIP", "status.podIP", "status.podIPs"`,
},
}
for _, tc := range errorCases {
@@ -24,7 +24,7 @@ import (

"k8s.io/klog"

"k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
This conversation was marked as resolved by aramase

This comment has been minimized.

Copy link
@khenidak

khenidak Oct 17, 2019

Contributor

please restore to original value.

This comment has been minimized.

Copy link
@aramase

aramase Oct 17, 2019

Author Member

done!

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
@@ -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)

This comment has been minimized.

Copy link
@thockin

thockin Nov 15, 2019

Member

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

This comment has been minimized.

Copy link
@aramase

aramase Nov 15, 2019

Author Member

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

GetPodDNS(pod *v1.Pod) (dnsConfig *runtimeapi.DNSConfig, err error)
// GetPodCgroupParent returns the CgroupName identifier, and its literal cgroupfs form on the host
// of a pod.
@@ -34,7 +34,7 @@ type FakeRuntimeHelper struct {
Err error
}

func (f *FakeRuntimeHelper) GenerateRunContainerOptions(pod *v1.Pod, container *v1.Container, podIP string) (*kubecontainer.RunContainerOptions, func(), error) {
func (f *FakeRuntimeHelper) GenerateRunContainerOptions(pod *v1.Pod, container *v1.Container, podIP string, podIPs []string) (*kubecontainer.RunContainerOptions, func(), error) {
var opts kubecontainer.RunContainerOptions
if len(container.TerminationMessagePath) != 0 {
opts.PodContainerDir = f.PodContainerDir
@@ -128,15 +128,15 @@ func (kl *Kubelet) makeBlockVolumes(pod *v1.Pod, container *v1.Container, podVol
}

// makeMounts determines the mount points for the given container.
func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, hostDomain, podIP string, podVolumes kubecontainer.VolumeMap, hu hostutil.HostUtils, subpather subpath.Interface, expandEnvs []kubecontainer.EnvVar) ([]kubecontainer.Mount, func(), error) {
func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, hostDomain string, podIPs []string, podVolumes kubecontainer.VolumeMap, hu hostutil.HostUtils, subpather subpath.Interface, expandEnvs []kubecontainer.EnvVar) ([]kubecontainer.Mount, func(), error) {
// 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 comment has been minimized.

Copy link
@andrewsykim

andrewsykim Nov 6, 2019

Member

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.

This comment has been minimized.

Copy link
@aramase

aramase Nov 6, 2019

Author Member

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 comment has been minimized.

Copy link
@thockin

thockin Nov 15, 2019

Member

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.

klog.V(3).Infof("container: %v/%v/%v podIPs: %q creating hosts mount: %v", pod.Namespace, pod.Name, container.Name, podIPs, mountEtcHostsFile)
mounts := []kubecontainer.Mount{}
var cleanupAction func()
for i, mount := range container.VolumeMounts {
@@ -258,7 +258,7 @@ func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, h
}
if mountEtcHostsFile {
hostAliases := pod.Spec.HostAliases
hostsMount, err := makeHostsMount(podDir, podIP, hostName, hostDomain, hostAliases, pod.Spec.HostNetwork)
hostsMount, err := makeHostsMount(podDir, podIPs, hostName, hostDomain, hostAliases, pod.Spec.HostNetwork)
if err != nil {
return nil, cleanupAction, err
}
@@ -292,10 +292,11 @@ func translateMountPropagation(mountMode *v1.MountPropagationMode) (runtimeapi.M
}

// makeHostsMount makes the mountpoint for the hosts file that the containers
// in a pod are injected with.
func makeHostsMount(podDir, podIP, hostName, hostDomainName string, hostAliases []v1.HostAlias, useHostNetwork bool) (*kubecontainer.Mount, error) {
// in a pod are injected with. podIPs is provided instead of podIP as podIPs
// are present even if dual-stack feature flag is not enabled.
func makeHostsMount(podDir string, podIPs []string, hostName, hostDomainName string, hostAliases []v1.HostAlias, useHostNetwork bool) (*kubecontainer.Mount, error) {
hostsFilePath := path.Join(podDir, "etc-hosts")
if err := ensureHostsFile(hostsFilePath, podIP, hostName, hostDomainName, hostAliases, useHostNetwork); err != nil {
if err := ensureHostsFile(hostsFilePath, podIPs, hostName, hostDomainName, hostAliases, useHostNetwork); err != nil {
return nil, err
}
return &kubecontainer.Mount{
@@ -309,7 +310,7 @@ func makeHostsMount(podDir, podIP, hostName, hostDomainName string, hostAliases

// ensureHostsFile ensures that the given host file has an up-to-date ip, host
// name, and domain name.
func ensureHostsFile(fileName, hostIP, hostName, hostDomainName string, hostAliases []v1.HostAlias, useHostNetwork bool) error {
func ensureHostsFile(fileName string, hostIPs []string, hostName, hostDomainName string, hostAliases []v1.HostAlias, useHostNetwork bool) error {
var hostsFileContent []byte
var err error

@@ -323,7 +324,7 @@ func ensureHostsFile(fileName, hostIP, hostName, hostDomainName string, hostAlia
}
} else {
// if Pod is not using host network, create a managed hosts file with Pod IP and other information.
hostsFileContent = managedHostsFileContent(hostIP, hostName, hostDomainName, hostAliases)
hostsFileContent = managedHostsFileContent(hostIPs, hostName, hostDomainName, hostAliases)
}

return ioutil.WriteFile(fileName, hostsFileContent, 0644)
@@ -342,9 +343,9 @@ func nodeHostsFileContent(hostsFilePath string, hostAliases []v1.HostAlias) ([]b
return buffer.Bytes(), nil
}

// managedHostsFileContent generates the content of the managed etc hosts based on Pod IP and other
// managedHostsFileContent generates the content of the managed etc hosts based on Pod IPs and other
// information.
func managedHostsFileContent(hostIP, hostName, hostDomainName string, hostAliases []v1.HostAlias) []byte {
func managedHostsFileContent(hostIPs []string, hostName, hostDomainName string, hostAliases []v1.HostAlias) []byte {
var buffer bytes.Buffer
buffer.WriteString(managedHostsHeader)
buffer.WriteString("127.0.0.1\tlocalhost\n") // ipv4 localhost
@@ -354,9 +355,16 @@ func managedHostsFileContent(hostIP, hostName, hostDomainName string, hostAliase
buffer.WriteString("fe00::1\tip6-allnodes\n")
buffer.WriteString("fe00::2\tip6-allrouters\n")
if len(hostDomainName) > 0 {
buffer.WriteString(fmt.Sprintf("%s\t%s.%s\t%s\n", hostIP, hostName, hostDomainName, hostName))
// host entry generated for all IPs in podIPs
// podIPs field is populated for clusters even
// dual-stack feature flag is not enabled.
for _, hostIP := range hostIPs {
buffer.WriteString(fmt.Sprintf("%s\t%s.%s\t%s\n", hostIP, hostName, hostDomainName, hostName))
}
} else {
buffer.WriteString(fmt.Sprintf("%s\t%s\n", hostIP, hostName))
for _, hostIP := range hostIPs {
buffer.WriteString(fmt.Sprintf("%s\t%s\n", hostIP, hostName))
}
}
buffer.Write(hostsEntriesFromHostAliases(hostAliases))
return buffer.Bytes()
@@ -433,7 +441,7 @@ func (kl *Kubelet) GetPodCgroupParent(pod *v1.Pod) string {

// GenerateRunContainerOptions generates the RunContainerOptions, which can be used by
// the container runtime to set parameters for launching a container.
func (kl *Kubelet) GenerateRunContainerOptions(pod *v1.Pod, container *v1.Container, podIP string) (*kubecontainer.RunContainerOptions, func(), error) {
func (kl *Kubelet) GenerateRunContainerOptions(pod *v1.Pod, container *v1.Container, podIP string, podIPs []string) (*kubecontainer.RunContainerOptions, func(), error) {
opts, err := kl.containerManager.GetResources(pod, container)
if err != nil {
return nil, nil, err
@@ -459,13 +467,14 @@ func (kl *Kubelet) GenerateRunContainerOptions(pod *v1.Pod, container *v1.Contai
opts.Devices = append(opts.Devices, blkVolumes...)
}

envs, err := kl.makeEnvironmentVariables(pod, container, podIP)
envs, err := kl.makeEnvironmentVariables(pod, container, podIP, podIPs)
if err != nil {
return nil, nil, err
}
opts.Envs = append(opts.Envs, envs...)

mounts, cleanupAction, err := makeMounts(pod, kl.getPodDir(pod.UID), container, hostname, hostDomainName, podIP, volumes, kl.hostutil, kl.subpather, opts.Envs)
// only podIPs is sent to makeMounts, as podIPs is populated even if dual-stack feature flag is not enabled.
mounts, cleanupAction, err := makeMounts(pod, kl.getPodDir(pod.UID), container, hostname, hostDomainName, podIPs, volumes, kl.hostutil, kl.subpather, opts.Envs)
if err != nil {
return nil, cleanupAction, err
}
@@ -545,7 +554,7 @@ func (kl *Kubelet) getServiceEnvVarMap(ns string, enableServiceLinks bool) (map[
}

// Make the environment variables for a pod in the given namespace.
func (kl *Kubelet) makeEnvironmentVariables(pod *v1.Pod, container *v1.Container, podIP string) ([]kubecontainer.EnvVar, error) {
func (kl *Kubelet) makeEnvironmentVariables(pod *v1.Pod, container *v1.Container, podIP string, podIPs []string) ([]kubecontainer.EnvVar, error) {
if pod.Spec.EnableServiceLinks == nil {
return nil, fmt.Errorf("nil pod.spec.enableServiceLinks encountered, cannot construct envvars")
}
@@ -669,7 +678,7 @@ func (kl *Kubelet) makeEnvironmentVariables(pod *v1.Pod, container *v1.Container
// Step 1b: resolve alternate env var sources
switch {
case envVar.ValueFrom.FieldRef != nil:
runtimeVal, err = kl.podFieldSelectorRuntimeValue(envVar.ValueFrom.FieldRef, pod, podIP)
runtimeVal, err = kl.podFieldSelectorRuntimeValue(envVar.ValueFrom.FieldRef, pod, podIP, podIPs)
if err != nil {
return result, err
}
@@ -770,7 +779,7 @@ func (kl *Kubelet) makeEnvironmentVariables(pod *v1.Pod, container *v1.Container

// podFieldSelectorRuntimeValue returns the runtime value of the given
// selector for a pod.
func (kl *Kubelet) podFieldSelectorRuntimeValue(fs *v1.ObjectFieldSelector, pod *v1.Pod, podIP string) (string, error) {
func (kl *Kubelet) podFieldSelectorRuntimeValue(fs *v1.ObjectFieldSelector, pod *v1.Pod, podIP string, podIPs []string) (string, error) {
internalFieldPath, _, err := podshelper.ConvertDownwardAPIFieldLabel(fs.APIVersion, fs.FieldPath, "")
if err != nil {
return "", err
@@ -788,6 +797,8 @@ func (kl *Kubelet) podFieldSelectorRuntimeValue(fs *v1.ObjectFieldSelector, pod
return hostIP.String(), nil
case "status.podIP":
return podIP, nil
case "status.podIPs":
return strings.Join(podIPs, ","), nil
}
return fieldpath.ExtractFieldPathAsString(pod, internalFieldPath)
}
@@ -22,7 +22,7 @@ import (
"testing"

"github.com/stretchr/testify/assert"
v1 "k8s.io/api/core/v1"
"k8s.io/api/core/v1"

runtimeapi "k8s.io/cri-api/pkg/apis/runtime/v1alpha2"
_ "k8s.io/kubernetes/pkg/apis/core/install"
@@ -249,7 +249,7 @@ func TestMakeMounts(t *testing.T) {
},
}

mounts, _, err := makeMounts(&pod, "/pod", &tc.container, "fakepodname", "", "", tc.podVolumes, fhu, fsp, nil)
mounts, _, err := makeMounts(&pod, "/pod", &tc.container, "fakepodname", "", []string{""}, tc.podVolumes, fhu, fsp, nil)

// validate only the error if we expect an error
if tc.expectErr {
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.