fix: Panic on nodeClassRef nil dereference (#1669)

kubernetes-sigs · Sep 19, 2024 · c34b2b4 · c34b2b4
1 parent 8761b2d
commit c34b2b4
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 23 deletions.
diff --git a/pkg/apis/v1/nodeclaim_conversion.go b/pkg/apis/v1/nodeclaim_conversion.go
@@ -23,6 +23,7 @@ import (
 	"strings"
 
 	"github.com/samber/lo"
+
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
@@ -63,13 +64,15 @@ func (in *NodeClaimSpec) convertTo(v1beta1nc *v1beta1.NodeClaimSpec, kubeletAnno
 	})
 	// Convert the NodeClassReference depending on whether the annotation exists
 	v1beta1nc.NodeClassRef = &v1beta1.NodeClassReference{}
-	if nodeClassReferenceAnnotation != "" {
-		if err := json.Unmarshal([]byte(nodeClassReferenceAnnotation), v1beta1nc.NodeClassRef); err != nil {
-			return fmt.Errorf("unmarshaling nodeClassRef annotation, %w", err)
+	if in.NodeClassRef != nil {
+		if nodeClassReferenceAnnotation != "" {
+			if err := json.Unmarshal([]byte(nodeClassReferenceAnnotation), v1beta1nc.NodeClassRef); err != nil {
+				return fmt.Errorf("unmarshaling nodeClassRef annotation, %w", err)
+			}
+		} else {
+			v1beta1nc.NodeClassRef.Name = in.NodeClassRef.Name
+			v1beta1nc.NodeClassRef.Kind = in.NodeClassRef.Kind
 		}
-	} else {
-		v1beta1nc.NodeClassRef.Name = in.NodeClassRef.Name
-		v1beta1nc.NodeClassRef.Kind = in.NodeClassRef.Kind
 	}
 	if kubeletAnnotation != "" {
 		v1beta1kubelet := &v1beta1.KubeletConfiguration{}
@@ -151,11 +154,14 @@ func (in *NodeClaimSpec) convertFrom(ctx context.Context, v1beta1nc *v1beta1.Nod
 		}
 	})
 
-	defaultNodeClassGVK := injection.GetNodeClasses(ctx)[0]
-	in.NodeClassRef = &NodeClassReference{
-		Name:  v1beta1nc.NodeClassRef.Name,
-		Kind:  lo.Ternary(v1beta1nc.NodeClassRef.Kind == "", defaultNodeClassGVK.Kind, v1beta1nc.NodeClassRef.Kind),
-		Group: lo.Ternary(v1beta1nc.NodeClassRef.APIVersion == "", defaultNodeClassGVK.Group, strings.Split(v1beta1nc.NodeClassRef.APIVersion, "/")[0]),
+	in.NodeClassRef = &NodeClassReference{}
+	if v1beta1nc.NodeClassRef != nil {
+		defaultNodeClassGVK := injection.GetNodeClasses(ctx)[0]
+		in.NodeClassRef = &NodeClassReference{
+			Name:  v1beta1nc.NodeClassRef.Name,
+			Kind:  lo.Ternary(v1beta1nc.NodeClassRef.Kind == "", defaultNodeClassGVK.Kind, v1beta1nc.NodeClassRef.Kind),
+			Group: lo.Ternary(v1beta1nc.NodeClassRef.APIVersion == "", defaultNodeClassGVK.Group, strings.Split(v1beta1nc.NodeClassRef.APIVersion, "/")[0]),
+		}
 	}
 
 	if v1beta1nc.Kubelet != nil {

diff --git a/pkg/apis/v1/nodeclaim_conversion_test.go b/pkg/apis/v1/nodeclaim_conversion_test.go
@@ -201,6 +201,13 @@ var _ = Describe("Convert v1 to v1beta1 NodeClaim API", func() {
 				Expect(v1beta1nodeclaim.Spec.NodeClassRef.Kind).To(Equal(nodeClassReference.Kind))
 				Expect(v1beta1nodeclaim.Spec.NodeClassRef.APIVersion).To(Equal(nodeClassReference.APIVersion))
 			})
+			It("should not panic when the v1 nodeclassRef is not defined", func() {
+				v1nodeclaim.Spec.NodeClassRef = nil
+				Expect(v1nodeclaim.ConvertTo(ctx, v1beta1nodeclaim)).To(Succeed())
+				Expect(v1beta1nodeclaim.Spec.NodeClassRef.Kind).To(Equal(""))
+				Expect(v1beta1nodeclaim.Spec.NodeClassRef.Name).To(Equal(""))
+				Expect(v1beta1nodeclaim.Spec.NodeClassRef.APIVersion).To(Equal(""))
+			})
 		})
 	})
 	Context("NodeClaim Status", func() {
@@ -473,6 +480,13 @@ var _ = Describe("Convert V1beta1 to V1 NodeClaim API", func() {
 				Expect(v1nodeclaim.Spec.NodeClassRef.Group).To(Equal(cloudProvider.NodeClassGroupVersionKind[0].Group))
 				Expect(v1nodeclaim.Annotations).To(HaveKeyWithValue(NodeClassReferenceAnnotationKey, string(nodeClassReferenceAnnotation)))
 			})
+			It("should not panic when the v1beta1 nodeclassRef is not defined", func() {
+				v1beta1nodeclaim.Spec.NodeClassRef = nil
+				Expect(v1nodeclaim.ConvertFrom(ctx, v1beta1nodeclaim)).To(Succeed())
+				Expect(v1nodeclaim.Spec.NodeClassRef.Kind).To(Equal(""))
+				Expect(v1nodeclaim.Spec.NodeClassRef.Name).To(Equal(""))
+				Expect(v1nodeclaim.Spec.NodeClassRef.Group).To(Equal(""))
+			})
 		})
 	})
 	Context("NodeClaim Status", func() {

diff --git a/pkg/apis/v1/nodepool_conversion.go b/pkg/apis/v1/nodepool_conversion.go
@@ -23,12 +23,13 @@ import (
 	"strings"
 	"time"
 
+	"sigs.k8s.io/karpenter/pkg/operator/injection"
+
 	"github.com/samber/lo"
 	v1 "k8s.io/api/core/v1"
 	"knative.dev/pkg/apis"
 
 	"sigs.k8s.io/karpenter/pkg/apis/v1beta1"
-	"sigs.k8s.io/karpenter/pkg/operator/injection"
 )
 
 // Convert v1 NodePool to v1beta1 NodePool
@@ -89,13 +90,15 @@ func (in *NodeClaimTemplate) convertTo(v1beta1np *v1beta1.NodeClaimTemplate, kub
 	})
 	// Convert the NodeClassReference depending on whether the annotation exists
 	v1beta1np.Spec.NodeClassRef = &v1beta1.NodeClassReference{}
-	if nodeClassReferenceAnnotation != "" {
-		if err := json.Unmarshal([]byte(nodeClassReferenceAnnotation), v1beta1np.Spec.NodeClassRef); err != nil {
-			return fmt.Errorf("unmarshaling nodeClassRef annotation, %w", err)
+	if in.Spec.NodeClassRef != nil {
+		if nodeClassReferenceAnnotation != "" {
+			if err := json.Unmarshal([]byte(nodeClassReferenceAnnotation), v1beta1np.Spec.NodeClassRef); err != nil {
+				return fmt.Errorf("unmarshaling nodeClassRef annotation, %w", err)
+			}
+		} else {
+			v1beta1np.Spec.NodeClassRef.Name = in.Spec.NodeClassRef.Name
+			v1beta1np.Spec.NodeClassRef.Kind = in.Spec.NodeClassRef.Kind
 		}
-	} else {
-		v1beta1np.Spec.NodeClassRef.Name = in.Spec.NodeClassRef.Name
-		v1beta1np.Spec.NodeClassRef.Kind = in.Spec.NodeClassRef.Kind
 	}
 	if kubeletAnnotation != "" {
 		v1beta1kubelet := &v1beta1.KubeletConfiguration{}
@@ -127,6 +130,7 @@ func (in *NodePool) ConvertFrom(ctx context.Context, v1beta1np apis.Convertible)
 	} else {
 		in.Annotations = lo.Assign(in.Annotations, map[string]string{KubeletCompatibilityAnnotationKey: kubeletAnnotation})
 	}
+
 	nodeClassRefAnnotation, err := json.Marshal(v1beta1NP.Spec.Template.Spec.NodeClassRef)
 	if err != nil {
 		return fmt.Errorf("marshaling nodeClassRef annotation, %w", err)
@@ -175,11 +179,14 @@ func (in *NodeClaimTemplate) convertFrom(ctx context.Context, v1beta1np *v1beta1
 		}
 	})
 
-	defaultNodeClassGVK := injection.GetNodeClasses(ctx)[0]
-	in.Spec.NodeClassRef = &NodeClassReference{
-		Name:  v1beta1np.Spec.NodeClassRef.Name,
-		Kind:  lo.Ternary(v1beta1np.Spec.NodeClassRef.Kind == "", defaultNodeClassGVK.Kind, v1beta1np.Spec.NodeClassRef.Kind),
-		Group: lo.Ternary(v1beta1np.Spec.NodeClassRef.APIVersion == "", defaultNodeClassGVK.Group, strings.Split(v1beta1np.Spec.NodeClassRef.APIVersion, "/")[0]),
+	in.Spec.NodeClassRef = &NodeClassReference{}
+	if v1beta1np.Spec.NodeClassRef != nil {
+		defaultNodeClassGVK := injection.GetNodeClasses(ctx)[0]
+		in.Spec.NodeClassRef = &NodeClassReference{
+			Name:  v1beta1np.Spec.NodeClassRef.Name,
+			Kind:  lo.Ternary(v1beta1np.Spec.NodeClassRef.Kind == "", defaultNodeClassGVK.Kind, v1beta1np.Spec.NodeClassRef.Kind),
+			Group: lo.Ternary(v1beta1np.Spec.NodeClassRef.APIVersion == "", defaultNodeClassGVK.Group, strings.Split(v1beta1np.Spec.NodeClassRef.APIVersion, "/")[0]),
+		}
 	}
 	if v1beta1np.Spec.Kubelet != nil {
 		kubelet, err := json.Marshal(v1beta1np.Spec.Kubelet)

diff --git a/pkg/apis/v1/nodepool_conversion_test.go b/pkg/apis/v1/nodepool_conversion_test.go
@@ -213,6 +213,13 @@ var _ = Describe("Convert V1 to V1beta1 NodePool API", func() {
 					Expect(v1beta1nodepool.Spec.Template.Spec.NodeClassRef.Kind).To(Equal(nodeClassReference.Kind))
 					Expect(v1beta1nodepool.Spec.Template.Spec.NodeClassRef.APIVersion).To(Equal(nodeClassReference.APIVersion))
 				})
+				It("should not panic when the v1 nodeclassRef is not defined", func() {
+					v1nodepool.Spec.Template.Spec.NodeClassRef = nil
+					Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
+					Expect(v1beta1nodepool.Spec.Template.Spec.NodeClassRef.Kind).To(Equal(""))
+					Expect(v1beta1nodepool.Spec.Template.Spec.NodeClassRef.Name).To(Equal(""))
+					Expect(v1beta1nodepool.Spec.Template.Spec.NodeClassRef.APIVersion).To(Equal(""))
+				})
 			})
 		})
 		Context("Disruption", func() {
@@ -523,6 +530,13 @@ var _ = Describe("Convert V1beta1 to V1 NodePool API", func() {
 					Expect(v1nodepool.Spec.Template.Spec.NodeClassRef.Group).To(Equal(cloudProvider.NodeClassGroupVersionKind[0].Group))
 					Expect(v1nodepool.Annotations).To(HaveKeyWithValue(NodeClassReferenceAnnotationKey, string(nodeClassReferenceAnnotation)))
 				})
+				It("should not panic when the v1beta1 nodeclassRef is not defined", func() {
+					v1beta1nodepool.Spec.Template.Spec.NodeClassRef = nil
+					Expect(v1nodepool.ConvertFrom(ctx, v1beta1nodepool)).To(Succeed())
+					Expect(v1nodepool.Spec.Template.Spec.NodeClassRef.Kind).To(Equal(""))
+					Expect(v1nodepool.Spec.Template.Spec.NodeClassRef.Name).To(Equal(""))
+					Expect(v1nodepool.Spec.Template.Spec.NodeClassRef.Group).To(Equal(""))
+				})
 			})
 		})
 		Context("Disruption", func() {