(backport v0.34.x) fix: Panic on nodeClassRef nil dereference (#1696)

kubernetes-sigs · Sep 23, 2024 · 5a45b25 · 5a45b25
1 parent 87c3f16
commit 5a45b25
Show file tree

Hide file tree

Showing 4 changed files with 60 additions and 22 deletions.
diff --git a/pkg/apis/v1/nodeclaim_conversion.go b/pkg/apis/v1/nodeclaim_conversion.go
@@ -63,13 +63,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{}
@@ -158,11 +160,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
@@ -209,6 +209,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() {
@@ -489,6 +496,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
@@ -85,13 +85,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{}
@@ -168,11 +170,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
@@ -207,6 +207,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() {
@@ -511,6 +518,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() {