From 3d5ab9e479946b4fa9258b1be3198d10d39d12cb Mon Sep 17 00:00:00 2001 From: Amanuel Engeda Date: Mon, 16 Sep 2024 11:20:04 -0700 Subject: [PATCH] Add nil check for nodeclassref --- .../karpenter.kwok.sh_kwoknodeclasses.yaml | 2 +- kwok/charts/crds/karpenter.sh_nodeclaims.yaml | 2 +- kwok/charts/crds/karpenter.sh_nodepools.yaml | 2 +- pkg/apis/crds/karpenter.sh_nodeclaims.yaml | 2 +- pkg/apis/crds/karpenter.sh_nodepools.yaml | 2 +- pkg/apis/v1/nodeclaim_conversion.go | 28 ++++++++++------- pkg/apis/v1/nodeclaim_conversion_test.go | 14 +++++++++ pkg/apis/v1/nodepool_conversion.go | 31 ++++++++++++------- pkg/apis/v1/nodepool_conversion_test.go | 14 +++++++++ .../karpenter.test.sh_testnodeclasses.yaml | 2 +- 10 files changed, 70 insertions(+), 29 deletions(-) diff --git a/kwok/apis/crds/karpenter.kwok.sh_kwoknodeclasses.yaml b/kwok/apis/crds/karpenter.kwok.sh_kwoknodeclasses.yaml index 79b9db76fc..c1657ab40a 100644 --- a/kwok/apis/crds/karpenter.kwok.sh_kwoknodeclasses.yaml +++ b/kwok/apis/crds/karpenter.kwok.sh_kwoknodeclasses.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.16.2 + controller-gen.kubebuilder.io/version: v0.16.3 name: kwoknodeclasses.karpenter.kwok.sh spec: group: karpenter.kwok.sh diff --git a/kwok/charts/crds/karpenter.sh_nodeclaims.yaml b/kwok/charts/crds/karpenter.sh_nodeclaims.yaml index 6e43e5640d..e6fee113cb 100644 --- a/kwok/charts/crds/karpenter.sh_nodeclaims.yaml +++ b/kwok/charts/crds/karpenter.sh_nodeclaims.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.16.2 + controller-gen.kubebuilder.io/version: v0.16.3 name: nodeclaims.karpenter.sh spec: group: karpenter.sh diff --git a/kwok/charts/crds/karpenter.sh_nodepools.yaml b/kwok/charts/crds/karpenter.sh_nodepools.yaml index 53e868df8f..96aa1a3a6d 100644 --- a/kwok/charts/crds/karpenter.sh_nodepools.yaml +++ b/kwok/charts/crds/karpenter.sh_nodepools.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.16.2 + controller-gen.kubebuilder.io/version: v0.16.3 name: nodepools.karpenter.sh spec: group: karpenter.sh diff --git a/pkg/apis/crds/karpenter.sh_nodeclaims.yaml b/pkg/apis/crds/karpenter.sh_nodeclaims.yaml index 4e48290c73..0384f3140b 100644 --- a/pkg/apis/crds/karpenter.sh_nodeclaims.yaml +++ b/pkg/apis/crds/karpenter.sh_nodeclaims.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.16.2 + controller-gen.kubebuilder.io/version: v0.16.3 name: nodeclaims.karpenter.sh spec: group: karpenter.sh diff --git a/pkg/apis/crds/karpenter.sh_nodepools.yaml b/pkg/apis/crds/karpenter.sh_nodepools.yaml index f186c4c5f7..3a10307d63 100644 --- a/pkg/apis/crds/karpenter.sh_nodepools.yaml +++ b/pkg/apis/crds/karpenter.sh_nodepools.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.16.2 + controller-gen.kubebuilder.io/version: v0.16.3 name: nodepools.karpenter.sh spec: group: karpenter.sh diff --git a/pkg/apis/v1/nodeclaim_conversion.go b/pkg/apis/v1/nodeclaim_conversion.go index ef18e1f80a..3b408ad71c 100644 --- 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 index a6d18d79b9..cb8debf45f 100644 --- 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 index 97b0258aa9..4e32d5d627 100644 --- 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 index c9289968ce..7c5b362836 100644 --- 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() { diff --git a/pkg/test/v1alpha1/crds/karpenter.test.sh_testnodeclasses.yaml b/pkg/test/v1alpha1/crds/karpenter.test.sh_testnodeclasses.yaml index fb3c15c292..b8684d22e0 100644 --- a/pkg/test/v1alpha1/crds/karpenter.test.sh_testnodeclasses.yaml +++ b/pkg/test/v1alpha1/crds/karpenter.test.sh_testnodeclasses.yaml @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.16.2 + controller-gen.kubebuilder.io/version: v0.16.3 name: testnodeclasses.karpenter.test.sh spec: group: karpenter.test.sh