From 650eba6904fa3f6d826e0450709cdd2700ec31ff Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Tue, 31 Aug 2021 08:39:55 +0200 Subject: [PATCH 1/2] generic ephemeral volumes: helper functions The name concatenation and ownership check were originally considered small enough to not warrant dedicated functions, but the intent of the code is more readable with them. --- .../storage/ephemeral/ephemeral.go | 57 ++++++++ .../storage/ephemeral/ephemeral_test.go | 126 ++++++++++++++++++ vendor/modules.txt | 1 + 3 files changed, 184 insertions(+) create mode 100644 staging/src/k8s.io/component-helpers/storage/ephemeral/ephemeral.go create mode 100644 staging/src/k8s.io/component-helpers/storage/ephemeral/ephemeral_test.go diff --git a/staging/src/k8s.io/component-helpers/storage/ephemeral/ephemeral.go b/staging/src/k8s.io/component-helpers/storage/ephemeral/ephemeral.go new file mode 100644 index 00000000000..67dddaf5c31 --- /dev/null +++ b/staging/src/k8s.io/component-helpers/storage/ephemeral/ephemeral.go @@ -0,0 +1,57 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// Package ephemeral provides code that supports the usual pattern +// for accessing the PVC that provides a generic ephemeral inline volume: +// +// - determine the PVC name that corresponds to the inline volume source +// - retrieve the PVC +// - verify that the PVC is owned by the pod +// - use the PVC +package ephemeral + +import ( + "fmt" + + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// VolumeClaimName returns the name of the PersistentVolumeClaim +// object that gets created for the generic ephemeral inline volume. The +// name is deterministic and therefore this function does not need any +// additional information besides the Pod name and volume name and it +// will never fail. +// +// Before using the PVC for the Pod, the caller must check that it is +// indeed the PVC that was created for the Pod by calling IsUsable. +func VolumeClaimName(pod *v1.Pod, volume *v1.Volume) string { + return pod.Name + "-" + volume.Name +} + +// VolumeIsForPod checks that the PVC is the ephemeral volume that +// was created for the Pod. It returns an error that is informative +// enough to be returned by the caller without adding further details +// about the Pod or PVC. +func VolumeIsForPod(pod *v1.Pod, pvc *v1.PersistentVolumeClaim) error { + // Checking the namespaces is just a precaution. The caller should + // never pass in a PVC that isn't from the same namespace as the + // Pod. + if pvc.Namespace != pod.Namespace || !metav1.IsControlledBy(pvc, pod) { + return fmt.Errorf("PVC %s/%s was not created for pod %s/%s (pod is not owner)", pvc.Namespace, pvc.Name, pod.Namespace, pod.Name) + } + return nil +} diff --git a/staging/src/k8s.io/component-helpers/storage/ephemeral/ephemeral_test.go b/staging/src/k8s.io/component-helpers/storage/ephemeral/ephemeral_test.go new file mode 100644 index 00000000000..09f1d5844eb --- /dev/null +++ b/staging/src/k8s.io/component-helpers/storage/ephemeral/ephemeral_test.go @@ -0,0 +1,126 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package ephemeral + +import ( + "fmt" + "testing" + + v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" +) + +func TestVolumeIsForPod(t *testing.T) { + uid := 0 + newUID := func() types.UID { + uid++ + return types.UID(fmt.Sprintf("%d", uid)) + } + isController := true + + podNotOwner := &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "kube-system", + Name: "podNotOwner", + UID: newUID(), + }, + } + podOwner := &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "kube-system", + Name: "podOwner", + UID: newUID(), + }, + } + pvcNoOwner := &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "kube-system", + Name: "pvcNoOwner", + UID: newUID(), + }, + } + pvcWithOwner := &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "kube-system", + Name: "pvcNoOwner", + UID: newUID(), + OwnerReferences: []metav1.OwnerReference{ + { + UID: podOwner.UID, + Controller: &isController, + }, + }, + }, + } + userPVCWithOwner := &v1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "user-namespace", + Name: "userPVCWithOwner", + UID: newUID(), + OwnerReferences: []metav1.OwnerReference{ + { + UID: podOwner.UID, + Controller: &isController, + }, + }, + }, + } + + testcases := map[string]struct { + pod *v1.Pod + pvc *v1.PersistentVolumeClaim + expectedError string + }{ + "owned": { + pod: podOwner, + pvc: pvcWithOwner, + }, + "other-pod": { + pod: podNotOwner, + pvc: pvcWithOwner, + expectedError: `PVC kube-system/pvcNoOwner was not created for pod kube-system/podNotOwner (pod is not owner)`, + }, + "no-owner": { + pod: podOwner, + pvc: pvcNoOwner, + expectedError: `PVC kube-system/pvcNoOwner was not created for pod kube-system/podOwner (pod is not owner)`, + }, + "different-namespace": { + pod: podOwner, + pvc: userPVCWithOwner, + expectedError: `PVC user-namespace/userPVCWithOwner was not created for pod kube-system/podOwner (pod is not owner)`, + }, + } + + for name, tc := range testcases { + t.Run(name, func(t *testing.T) { + err := VolumeIsForPod(tc.pod, tc.pvc) + if tc.expectedError == "" { + if err != nil { + t.Errorf("expected no error, got %v", err) + } + } else { + if err == nil { + t.Errorf("expected error %q, got nil", tc.expectedError) + } else if tc.expectedError != err.Error() { + t.Errorf("expected error %q, got %v", tc.expectedError, err) + } + } + }) + } +} diff --git a/vendor/modules.txt b/vendor/modules.txt index 18b3f39ef90..9b51749c9ee 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -1976,6 +1976,7 @@ k8s.io/component-helpers/node/utils/sysctl k8s.io/component-helpers/node/utils/sysctl/testing k8s.io/component-helpers/scheduling/corev1 k8s.io/component-helpers/scheduling/corev1/nodeaffinity +k8s.io/component-helpers/storage/ephemeral k8s.io/component-helpers/storage/volume # k8s.io/controller-manager v0.0.0 => ./staging/src/k8s.io/controller-manager ## explicit From 4ae0eecb343a13137ed0aabe075f462d66512fed Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Tue, 31 Aug 2021 08:39:55 +0200 Subject: [PATCH 2/2] controller: use generic ephemeral volume helper functions The name concatenation and ownership check were originally considered small enough to not warrant dedicated functions, but the intent of the code is more readable with them. There also was a missing owner check in the attach controller. --- .../volume/attachdetach/util/util.go | 27 ++++++++++++------- pkg/controller/volume/ephemeral/controller.go | 24 ++++++++--------- .../pvc_protection_controller.go | 5 ++-- 3 files changed, 31 insertions(+), 25 deletions(-) diff --git a/pkg/controller/volume/attachdetach/util/util.go b/pkg/controller/volume/attachdetach/util/util.go index 034bce8fc33..c811e94b4cd 100644 --- a/pkg/controller/volume/attachdetach/util/util.go +++ b/pkg/controller/volume/attachdetach/util/util.go @@ -26,6 +26,7 @@ import ( "k8s.io/apimachinery/pkg/util/sets" utilfeature "k8s.io/apiserver/pkg/util/feature" corelisters "k8s.io/client-go/listers/core/v1" + "k8s.io/component-helpers/storage/ephemeral" "k8s.io/klog/v2" "k8s.io/kubernetes/pkg/controller/volume/attachdetach/cache" "k8s.io/kubernetes/pkg/features" @@ -46,8 +47,10 @@ func CreateVolumeSpec(podVolume v1.Volume, pod *v1.Pod, nodeName types.NodeName, claimName = pvcSource.ClaimName readOnly = pvcSource.ReadOnly } + isEphemeral := false if ephemeralSource := podVolume.VolumeSource.Ephemeral; ephemeralSource != nil && utilfeature.DefaultFeatureGate.Enabled(features.GenericEphemeralVolume) { - claimName = pod.Name + "-" + podVolume.Name + claimName = ephemeral.VolumeClaimName(pod, &podVolume) + isEphemeral = true } if claimName != "" { klog.V(10).Infof( @@ -56,8 +59,7 @@ func CreateVolumeSpec(podVolume v1.Volume, pod *v1.Pod, nodeName types.NodeName, claimName) // If podVolume is a PVC, fetch the real PV behind the claim - pvName, pvcUID, err := getPVCFromCacheExtractPV( - pod.Namespace, claimName, pvcLister) + pvc, err := getPVCFromCache(pod.Namespace, claimName, pvcLister) if err != nil { return nil, fmt.Errorf( "error processing PVC %q/%q: %v", @@ -65,7 +67,13 @@ func CreateVolumeSpec(podVolume v1.Volume, pod *v1.Pod, nodeName types.NodeName, claimName, err) } + if isEphemeral { + if err := ephemeral.VolumeIsForPod(pod, pvc); err != nil { + return nil, err + } + } + pvName, pvcUID := pvc.Spec.VolumeName, pvc.UID klog.V(10).Infof( "Found bound PV for PVC (ClaimName %q/%q pvcUID %v): pvName=%q", pod.Namespace, @@ -119,20 +127,19 @@ func CreateVolumeSpec(podVolume v1.Volume, pod *v1.Pod, nodeName types.NodeName, return spec, nil } -// getPVCFromCacheExtractPV fetches the PVC object with the given namespace and -// name from the shared internal PVC store extracts the name of the PV it is -// pointing to and returns it. +// getPVCFromCache fetches the PVC object with the given namespace and +// name from the shared internal PVC store. // This method returns an error if a PVC object does not exist in the cache // with the given namespace/name. // This method returns an error if the PVC object's phase is not "Bound". -func getPVCFromCacheExtractPV(namespace string, name string, pvcLister corelisters.PersistentVolumeClaimLister) (string, types.UID, error) { +func getPVCFromCache(namespace string, name string, pvcLister corelisters.PersistentVolumeClaimLister) (*v1.PersistentVolumeClaim, error) { pvc, err := pvcLister.PersistentVolumeClaims(namespace).Get(name) if err != nil { - return "", "", fmt.Errorf("failed to find PVC %s/%s in PVCInformer cache: %v", namespace, name, err) + return nil, fmt.Errorf("failed to find PVC %s/%s in PVCInformer cache: %v", namespace, name, err) } if pvc.Status.Phase != v1.ClaimBound || pvc.Spec.VolumeName == "" { - return "", "", fmt.Errorf( + return nil, fmt.Errorf( "PVC %s/%s has non-bound phase (%q) or empty pvc.Spec.VolumeName (%q)", namespace, name, @@ -140,7 +147,7 @@ func getPVCFromCacheExtractPV(namespace string, name string, pvcLister coreliste pvc.Spec.VolumeName) } - return pvc.Spec.VolumeName, pvc.UID, nil + return pvc, nil } // getPVSpecFromCache fetches the PV object with the given name from the shared diff --git a/pkg/controller/volume/ephemeral/controller.go b/pkg/controller/volume/ephemeral/controller.go index e4fb9749fdf..b5ad0cf685e 100644 --- a/pkg/controller/volume/ephemeral/controller.go +++ b/pkg/controller/volume/ephemeral/controller.go @@ -37,10 +37,10 @@ import ( kcache "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/record" "k8s.io/client-go/util/workqueue" + "k8s.io/component-helpers/storage/ephemeral" "k8s.io/kubernetes/pkg/controller/volume/common" ephemeralvolumemetrics "k8s.io/kubernetes/pkg/controller/volume/ephemeral/metrics" "k8s.io/kubernetes/pkg/controller/volume/events" - "k8s.io/kubernetes/pkg/volume/util" ) // Controller creates PVCs for ephemeral inline volumes in a pod spec. @@ -241,24 +241,22 @@ func (ec *ephemeralController) syncHandler(key string) error { // handleEphemeralVolume is invoked for each volume of a pod. func (ec *ephemeralController) handleVolume(pod *v1.Pod, vol v1.Volume) error { klog.V(5).Infof("ephemeral: checking volume %s", vol.Name) - ephemeral := vol.Ephemeral - if ephemeral == nil { + if vol.Ephemeral == nil { return nil } - pvcName := pod.Name + "-" + vol.Name + pvcName := ephemeral.VolumeClaimName(pod, &vol) pvc, err := ec.pvcLister.PersistentVolumeClaims(pod.Namespace).Get(pvcName) if err != nil && !errors.IsNotFound(err) { return err } if pvc != nil { - if metav1.IsControlledBy(pvc, pod) { - // Already created, nothing more to do. - klog.V(5).Infof("ephemeral: volume %s: PVC %s already created", vol.Name, pvcName) - return nil + if err := ephemeral.VolumeIsForPod(pod, pvc); err != nil { + return err } - return fmt.Errorf("PVC %q (uid: %q) was not created for the pod", - util.GetPersistentVolumeClaimQualifiedName(pvc), pvc.UID) + // Already created, nothing more to do. + klog.V(5).Infof("ephemeral: volume %s: PVC %s already created", vol.Name, pvcName) + return nil } // Create the PVC with pod as owner. @@ -276,10 +274,10 @@ func (ec *ephemeralController) handleVolume(pod *v1.Pod, vol v1.Volume) error { BlockOwnerDeletion: &isTrue, }, }, - Annotations: ephemeral.VolumeClaimTemplate.Annotations, - Labels: ephemeral.VolumeClaimTemplate.Labels, + Annotations: vol.Ephemeral.VolumeClaimTemplate.Annotations, + Labels: vol.Ephemeral.VolumeClaimTemplate.Labels, }, - Spec: ephemeral.VolumeClaimTemplate.Spec, + Spec: vol.Ephemeral.VolumeClaimTemplate.Spec, } ephemeralvolumemetrics.EphemeralVolumeCreateAttempts.Inc() _, err = ec.kubeClient.CoreV1().PersistentVolumeClaims(pod.Namespace).Create(context.TODO(), pvc, metav1.CreateOptions{}) diff --git a/pkg/controller/volume/pvcprotection/pvc_protection_controller.go b/pkg/controller/volume/pvcprotection/pvc_protection_controller.go index cd971c395f9..b85961383fe 100644 --- a/pkg/controller/volume/pvcprotection/pvc_protection_controller.go +++ b/pkg/controller/volume/pvcprotection/pvc_protection_controller.go @@ -32,6 +32,7 @@ import ( "k8s.io/client-go/tools/cache" "k8s.io/client-go/util/workqueue" "k8s.io/component-base/metrics/prometheus/ratelimiter" + "k8s.io/component-helpers/storage/ephemeral" "k8s.io/klog/v2" "k8s.io/kubernetes/pkg/controller/volume/common" "k8s.io/kubernetes/pkg/controller/volume/protectionutil" @@ -299,7 +300,7 @@ func (c *Controller) podUsesPVC(pod *v1.Pod, pvc *v1.PersistentVolumeClaim) bool if pod.Spec.NodeName != "" { for _, volume := range pod.Spec.Volumes { if volume.PersistentVolumeClaim != nil && volume.PersistentVolumeClaim.ClaimName == pvc.Name || - c.genericEphemeralVolumeFeatureEnabled && !podIsShutDown(pod) && volume.Ephemeral != nil && pod.Name+"-"+volume.Name == pvc.Name && metav1.IsControlledBy(pvc, pod) { + c.genericEphemeralVolumeFeatureEnabled && !podIsShutDown(pod) && volume.Ephemeral != nil && ephemeral.VolumeClaimName(pod, &volume) == pvc.Name && ephemeral.VolumeIsForPod(pod, pvc) == nil { klog.V(2).InfoS("Pod uses PVC", "pod", klog.KObj(pod), "PVC", klog.KObj(pvc)) return true } @@ -407,7 +408,7 @@ func (c *Controller) enqueuePVCs(pod *v1.Pod, deleted bool) { case volume.PersistentVolumeClaim != nil: c.queue.Add(pod.Namespace + "/" + volume.PersistentVolumeClaim.ClaimName) case c.genericEphemeralVolumeFeatureEnabled && volume.Ephemeral != nil: - c.queue.Add(pod.Namespace + "/" + pod.Name + "-" + volume.Name) + c.queue.Add(pod.Namespace + "/" + ephemeral.VolumeClaimName(pod, &volume)) } } }