Fix nil pointer dereference when nodeAffinity is empty

Previously, we expected a PV with remote storage to have an empty set of selector but actually all the NodeAffinity section is missing. Now, we check if the pointer is nil Co-authored-by: Thomas Langé <t.lange@criteo.com>
criteo · Nov 23, 2023 · 619b055 · 619b055
1 parent 2f26ef5
commit 619b055
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 7 deletions.
diff --git a/internal/controller/applicationdisruptionbudget_controller_test.go b/internal/controller/applicationdisruptionbudget_controller_test.go
@@ -26,6 +26,7 @@ import (
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 )
@@ -67,10 +68,6 @@ var _ = Describe("ApplicationDisruptionBudget controller", func() {
 				},
 			}
 			Expect(k8sClient.Create(ctx, ns)).Should(Succeed())
-
-			By("Adding Pods")
-			pod1 := newPod("podadb1", ADBNamespace, "node1", podLabels)
-			Expect(k8sClient.Create(ctx, &pod1)).Should(Succeed())
 		})
 
 		AfterEach(func() {
@@ -90,6 +87,10 @@ var _ = Describe("ApplicationDisruptionBudget controller", func() {
 
 		When("Pods or PVC changes", func() {
 			It("updates the budget status", func() {
+				By("Adding Pod")
+				pod1 := newPod("podadb1", ADBNamespace, "node1", podLabels)
+				Expect(k8sClient.Create(ctx, &pod1)).Should(Succeed())
+
 				By("creating a budget that accepts one disruption")
 				ndb := &nodedisruptionv1alpha1.ApplicationDisruptionBudget{
 					TypeMeta: metav1.TypeMeta{
@@ -144,6 +145,8 @@ var _ = Describe("ApplicationDisruptionBudget controller", func() {
 		When("Node disruption is created", func() {
 			It("updates the budget status", func() {
 				By("Adding pods and PVCs")
+				pod1 := newPod("podadb1", ADBNamespace, "node1", podLabels)
+				Expect(k8sClient.Create(ctx, &pod1)).Should(Succeed())
 				pod2 := newPod("podadb2", ADBNamespace, "node2", podLabels)
 				Expect(k8sClient.Create(ctx, &pod2)).Should(Succeed())
 				pvc3 := newPVC("pvc3", ADBNamespace, "node3-pv-local", podLabels)
@@ -205,6 +208,60 @@ var _ = Describe("ApplicationDisruptionBudget controller", func() {
 			})
 		})
 
+		When("PV doesn't have an affinity", func() {
+			It("is ignored by ADB", func() {
+				By("Creating PV without NodeAffinity")
+				ressources := make(corev1.ResourceList, 1)
+				ressources[corev1.ResourceStorage] = *resource.NewQuantity(100, ressources.Storage().Format)
+
+				PVWithoutAffinity := &corev1.PersistentVolume{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "remote-pv",
+					},
+					Spec: corev1.PersistentVolumeSpec{
+						Capacity:               ressources,
+						AccessModes:            []corev1.PersistentVolumeAccessMode{corev1.ReadWriteOnce},
+						PersistentVolumeSource: corev1.PersistentVolumeSource{CSI: &corev1.CSIPersistentVolumeSource{Driver: "test", VolumeHandle: "test"}},
+						NodeAffinity:           nil,
+					},
+				}
+				Expect(k8sClient.Create(ctx, PVWithoutAffinity)).Should(Succeed())
+
+				By("Adding PVC")
+				pvc := newPVC("pvc", ADBNamespace, "remote-pv", podLabels)
+				Expect(k8sClient.Create(ctx, &pvc)).Should(Succeed())
+
+				By("creating a budget that accepts one disruption")
+				adb := &nodedisruptionv1alpha1.ApplicationDisruptionBudget{
+					TypeMeta: metav1.TypeMeta{
+						APIVersion: "nodedisruption.criteo.com/v1alpha1",
+						Kind:       "ApplicationDisruptionBudget",
+					},
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      ADBname,
+						Namespace: ADBNamespace,
+					},
+					Spec: nodedisruptionv1alpha1.ApplicationDisruptionBudgetSpec{
+						PodSelector:    metav1.LabelSelector{MatchLabels: podLabels},
+						PVCSelector:    metav1.LabelSelector{MatchLabels: podLabels},
+						MaxDisruptions: 1,
+					},
+				}
+				Expect(k8sClient.Create(ctx, adb)).Should(Succeed())
+
+				By("checking the ApplicationDisruptionBudget updated the status and has seen no nodes")
+				Eventually(func() int {
+					err := k8sClient.Get(ctx, types.NamespacedName{
+						Namespace: ADBNamespace,
+						Name:      ADBname,
+					}, adb)
+					Expect(err).Should(Succeed())
+					return adb.Status.DisruptionsAllowed
+				}, timeout, interval).Should(Equal(1))
+
+				Expect(adb.Status.WatchedNodes).Should(BeEmpty())
+			})
+		})
 	})
 
 })
diff --git a/pkg/resolver/resolver.go b/pkg/resolver/resolver.go
@@ -13,6 +13,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/log"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -140,6 +141,8 @@ func NodeSelectorAsSelector(nodeSelector *corev1.NodeSelector) (labels.Selector,
 }
 
 func (r *Resolver) GetNodesFromNamespacedPVCSelector(ctx context.Context, pvcSelector metav1.LabelSelector, namespace string) (NodeSet, error) {
+	logger := log.FromContext(ctx)
+
 	nodeNames := set.New()
 	nodeSet := NodeSet{Nodes: nodeNames}
 
@@ -172,11 +175,13 @@ func (r *Resolver) GetNodesFromNamespacedPVCSelector(ctx context.Context, pvcSel
 			return nodeSet, err
 		}
 
-		nodeSelector := pv.Spec.NodeAffinity.Required
-		if nodeSelector == nil {
+		if pv.Spec.NodeAffinity == nil || pv.Spec.NodeAffinity.Required == nil {
+			logger.Info("Skipping PV because NodeAffinity or Required is not defined", "pv_name", pv.Name)
 			continue
 		}
 
+		nodeSelector := pv.Spec.NodeAffinity.Required
+
 		opts := []client.ListOption{}
 		labelSelector, fieldSelector, err := NodeSelectorAsSelector(nodeSelector)
 		if err != nil {
@@ -185,7 +190,7 @@ func (r *Resolver) GetNodesFromNamespacedPVCSelector(ctx context.Context, pvcSel
 
 		if labelSelector.Empty() && fieldSelector.Empty() {
 			// Ignore this PV
-			fmt.Printf("skipping %s, no affinity", PVName)
+			logger.Info("Skipping PV because there are no selector defined", "pv_name", pv.Name)
 			continue
 		}