From 619b05576842f25c919e96670d7d751adb870033 Mon Sep 17 00:00:00 2001 From: Geoffrey Beausire Date: Thu, 23 Nov 2023 18:20:39 +0100 Subject: [PATCH] Fix nil pointer dereference when nodeAffinity is empty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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é --- ...icationdisruptionbudget_controller_test.go | 65 +++++++++++++++++-- pkg/resolver/resolver.go | 11 +++- 2 files changed, 69 insertions(+), 7 deletions(-) diff --git a/internal/controller/applicationdisruptionbudget_controller_test.go b/internal/controller/applicationdisruptionbudget_controller_test.go index 8194748..157281c 100644 --- 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 index a475591..7e53ec2 100644 --- 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 }