From 17768563948cbe1b12dff957b9b61bc3aeda92c3 Mon Sep 17 00:00:00 2001 From: Amanuel Engeda <74629455+engedaam@users.noreply.github.com> Date: Mon, 23 Sep 2024 08:32:00 -0700 Subject: [PATCH] (backport v0.36.x) fix: Panic on nodeClassRef `nil` dereference (#1694) --- pkg/apis/v1/nodeclaim_conversion.go | 27 ++++++++++++++---------- pkg/apis/v1/nodeclaim_conversion_test.go | 14 ++++++++++++ pkg/apis/v1/nodepool_conversion.go | 27 ++++++++++++++---------- pkg/apis/v1/nodepool_conversion_test.go | 14 ++++++++++++ 4 files changed, 60 insertions(+), 22 deletions(-) diff --git a/pkg/apis/v1/nodeclaim_conversion.go b/pkg/apis/v1/nodeclaim_conversion.go index 4b96b41248..bc0c626efe 100644 --- a/pkg/apis/v1/nodeclaim_conversion.go +++ b/pkg/apis/v1/nodeclaim_conversion.go @@ -66,13 +66,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{} @@ -162,11 +164,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 index 8c272ef45a..a759af07e0 100644 --- a/pkg/apis/v1/nodeclaim_conversion_test.go +++ b/pkg/apis/v1/nodeclaim_conversion_test.go @@ -210,6 +210,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() { @@ -493,6 +500,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 index cc8b1fe4c7..d2fed0f021 100644 --- a/pkg/apis/v1/nodepool_conversion.go +++ b/pkg/apis/v1/nodepool_conversion.go @@ -88,13 +88,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{} @@ -173,11 +175,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 index 7e885f6cec..8a1990dc46 100644 --- a/pkg/apis/v1/nodepool_conversion_test.go +++ b/pkg/apis/v1/nodepool_conversion_test.go @@ -208,6 +208,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() { @@ -515,6 +522,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() {