fix: ensure NillableDuration is round-trippable (kubernetes-sigs#1545)

Co-authored-by: Nick Tran <10810510+njtran@users.noreply.github.com>

pkg/apis/v1/duration.go

@@ -16,7 +16,11 @@ package v1
 
 import (
 	"encoding/json"
+	"fmt"
+	"slices"
 	"time"
+
+	"github.com/samber/lo"
 )
 
 const Never = "Never"
@@ -26,6 +30,17 @@ const Never = "Never"
 // that the duration is disabled and sets the inner duration as nil
 type NillableDuration struct {
 	*time.Duration
+
+	// Raw is used to ensure we remarshal the NillableDuration in the same format it was specified.
+	// This ensures tools like Flux and ArgoCD don't mistakenly detect drift due to our conversion webhooks.
+	Raw []byte `hash:"ignore"`
+}
+
+func MustParseNillableDuration(val string) NillableDuration {
+	nd := NillableDuration{}
+	// Use %q instead of %s to ensure that we unmarshal the value as a string and not an int
+	lo.Must0(json.Unmarshal([]byte(fmt.Sprintf("%q", val)), &nd))
+	return nd
 }
 
 // UnmarshalJSON implements the json.Unmarshaller interface.
@@ -42,22 +57,29 @@ func (d *NillableDuration) UnmarshalJSON(b []byte) error {
 	if err != nil {
 		return err
 	}
+	d.Raw = slices.Clone(b)
 	d.Duration = &pd
 	return nil
 }
 
 // MarshalJSON implements the json.Marshaler interface.
 func (d NillableDuration) MarshalJSON() ([]byte, error) {
-	if d.Duration == nil {
-		return json.Marshal(Never)
+	if d.Raw != nil {
+		return d.Raw, nil
+	}
+	if d.Duration != nil {
+		return json.Marshal(d.Duration.String())
 	}
-	return json.Marshal(d.Duration.String())
+	return json.Marshal(Never)
 }
 
 // ToUnstructured implements the value.UnstructuredConverter interface.
 func (d NillableDuration) ToUnstructured() interface{} {
-	if d.Duration == nil {
-		return Never
+	if d.Raw != nil {
+		return d.Raw
+	}
+	if d.Duration != nil {
+		return d.Duration.String()
 	}
-	return d.Duration.String()
+	return Never
 }

pkg/apis/v1/nodeclaim_conversion_test.go

@@ -274,22 +274,8 @@ var _ = Describe("Convert V1beta1 to V1 NodeClaim API", func() {
 	)
 
 	BeforeEach(func() {
-		v1nodePool = &NodePool{
-			ObjectMeta: test.ObjectMeta(),
-			Spec: NodePoolSpec{
-				Template: NodeClaimTemplate{
-					Spec: NodeClaimTemplateSpec{
-						NodeClassRef: &NodeClassReference{
-							Kind:  "test-kind",
-							Name:  "default",
-							Group: "test-group",
-						},
-						Requirements: []NodeSelectorRequirementWithMinValues{},
-					},
-				},
-			},
-		}
-		v1nodePool.Spec.Template.Spec.ExpireAfter = NillableDuration{Duration: lo.ToPtr(30 * time.Minute)}
+		v1nodePool = test.NodePool()
+		v1nodePool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("30m")
 		v1nodeclaim = &NodeClaim{
 			ObjectMeta: metav1.ObjectMeta{
 				Labels: map[string]string{
@@ -327,13 +313,13 @@ var _ = Describe("Convert V1beta1 to V1 NodeClaim API", func() {
 	Context("ExpireAfter", func() {
 		It("should default the v1beta1 expireAfter to v1 when the nodepool doesn't exist", func() {
 			Expect(env.Client.Delete(ctx, v1nodePool)).To(Succeed())
-			v1nodePool.Spec.Template.Spec.ExpireAfter = NillableDuration{Duration: lo.ToPtr(30 * time.Minute)}
+			v1nodePool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("30m")
 			Expect(v1nodeclaim.ConvertFrom(ctx, v1beta1nodeclaim)).To(Succeed())
 			Expect(v1nodeclaim.Spec.ExpireAfter.Duration).To(BeNil())
 		})
 		It("should default the v1beta1 expireAfter to v1 when the nodepool label doesn't exist", func() {
 			delete(v1beta1nodeclaim.Labels, v1beta1.NodePoolLabelKey)
-			v1nodePool.Spec.Template.Spec.ExpireAfter = NillableDuration{Duration: lo.ToPtr(30 * time.Minute)}
+			v1nodePool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("30m")
 			Expect(env.Client.Update(ctx, v1nodePool)).To(Succeed())
 			Expect(v1nodeclaim.ConvertFrom(ctx, v1beta1nodeclaim)).To(Succeed())
 			Expect(v1nodeclaim.Spec.ExpireAfter.Duration).To(BeNil())

pkg/apis/v1/nodepool_conversion_test.go

@@ -211,13 +211,13 @@ var _ = Describe("Convert V1 to V1beta1 NodePool API", func() {
 		Context("Disruption", func() {
 			It("should convert v1 nodepool consolidateAfter to nil with WhenEmptyOrUnderutilized", func() {
 				v1nodepool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmptyOrUnderutilized
-				v1nodepool.Spec.Disruption.ConsolidateAfter = NillableDuration{Duration: lo.ToPtr(time.Second * 2121)}
+				v1nodepool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("2121s")
 				Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
 				Expect(v1beta1nodepool.Spec.Disruption.ConsolidateAfter).To(BeNil())
 			})
 			It("should convert v1 nodepool consolidateAfter with WhenEmpty", func() {
 				v1nodepool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmpty
-				v1nodepool.Spec.Disruption.ConsolidateAfter = NillableDuration{Duration: lo.ToPtr(time.Second * 2121)}
+				v1nodepool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("2121s")
 				Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
 				Expect(lo.FromPtr(v1beta1nodepool.Spec.Disruption.ConsolidateAfter.Duration)).To(Equal(lo.FromPtr(v1nodepool.Spec.Disruption.ConsolidateAfter.Duration)))
 			})
@@ -227,7 +227,7 @@ var _ = Describe("Convert V1 to V1beta1 NodePool API", func() {
 				Expect(string(v1beta1nodepool.Spec.Disruption.ConsolidationPolicy)).To(Equal(string(v1nodepool.Spec.Disruption.ConsolidationPolicy)))
 			})
 			It("should convert v1 nodepool ExpireAfter", func() {
-				v1nodepool.Spec.Template.Spec.ExpireAfter = NillableDuration{Duration: lo.ToPtr(time.Second * 2121)}
+				v1nodepool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("2121s")
 				Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
 				Expect(v1beta1nodepool.Spec.Disruption.ExpireAfter.Duration).To(Equal(v1nodepool.Spec.Template.Spec.ExpireAfter.Duration))
 			})
@@ -243,6 +243,40 @@ var _ = Describe("Convert V1 to V1beta1 NodePool API", func() {
 			Expect(v1beta1nodepool.Status.Resources[resource]).To(Equal(v1nodepool.Status.Resources[resource]))
 		}
 	})
+	Context("Round Trip", func() {
+		It("spec.template.spec.expireAfter", func() {
+			v1nodepool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("10h")
+			Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
+			Expect(v1nodepool.ConvertFrom(ctx, v1beta1nodepool)).To(Succeed())
+			result, err := json.Marshal(v1nodepool.Spec.Template.Spec.ExpireAfter)
+			Expect(err).To(BeNil())
+			Expect(string(result)).To(Equal(`"10h"`))
+		})
+		It("spec.template.spec.expireAfter (Never)", func() {
+			v1nodepool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("Never")
+			Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
+			Expect(v1nodepool.ConvertFrom(ctx, v1beta1nodepool)).To(Succeed())
+			result, err := json.Marshal(v1nodepool.Spec.Template.Spec.ExpireAfter)
+			Expect(err).To(BeNil())
+			Expect(string(result)).To(Equal(`"Never"`))
+		})
+		It("spec.disruption.consolidateAfter", func() {
+			v1nodepool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("10h")
+			Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
+			Expect(v1nodepool.ConvertFrom(ctx, v1beta1nodepool)).To(Succeed())
+			result, err := json.Marshal(v1nodepool.Spec.Disruption.ConsolidateAfter)
+			Expect(err).To(BeNil())
+			Expect(string(result)).To(Equal(`"10h"`))
+		})
+		It("spec.disruption.consolidateAfter (Never)", func() {
+			v1nodepool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("Never")
+			Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
+			Expect(v1nodepool.ConvertFrom(ctx, v1beta1nodepool)).To(Succeed())
+			result, err := json.Marshal(v1nodepool.Spec.Disruption.ConsolidateAfter)
+			Expect(err).To(BeNil())
+			Expect(string(result)).To(Equal(`"Never"`))
+		})
+	})
 })
 
 var _ = Describe("Convert V1beta1 to V1 NodePool API", func() {
@@ -484,4 +518,38 @@ var _ = Describe("Convert V1beta1 to V1 NodePool API", func() {
 			Expect(v1beta1nodepool.Status.Resources[resource]).To(Equal(v1nodepool.Status.Resources[resource]))
 		}
 	})
+	Context("Round Trip", func() {
+		It("spec.disruption.expireAfter", func() {
+			v1beta1nodepool.Spec.Disruption.ExpireAfter = v1beta1.MustParseNillableDuration("10h")
+			Expect(v1nodepool.ConvertFrom(ctx, v1beta1nodepool)).To(Succeed())
+			Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
+			result, err := json.Marshal(v1beta1nodepool.Spec.Disruption.ExpireAfter)
+			Expect(err).To(BeNil())
+			Expect(string(result)).To(Equal(`"10h"`))
+		})
+		It("spec.disruption.expireAfter (Never)", func() {
+			v1beta1nodepool.Spec.Disruption.ExpireAfter = v1beta1.MustParseNillableDuration("Never")
+			Expect(v1nodepool.ConvertFrom(ctx, v1beta1nodepool)).To(Succeed())
+			Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
+			result, err := json.Marshal(v1beta1nodepool.Spec.Disruption.ExpireAfter)
+			Expect(err).To(BeNil())
+			Expect(string(result)).To(Equal(`"Never"`))
+		})
+		It("spec.disruption.consolidateAfter", func() {
+			v1beta1nodepool.Spec.Disruption.ConsolidateAfter = lo.ToPtr(v1beta1.MustParseNillableDuration("10h"))
+			Expect(v1nodepool.ConvertFrom(ctx, v1beta1nodepool)).To(Succeed())
+			Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
+			result, err := json.Marshal(lo.FromPtr(v1beta1nodepool.Spec.Disruption.ConsolidateAfter))
+			Expect(err).To(BeNil())
+			Expect(string(result)).To(Equal(`"10h"`))
+		})
+		It("spec.disruption.consolidateAfter (Never)", func() {
+			v1beta1nodepool.Spec.Disruption.ConsolidateAfter = lo.ToPtr(v1beta1.MustParseNillableDuration("Never"))
+			Expect(v1nodepool.ConvertFrom(ctx, v1beta1nodepool)).To(Succeed())
+			Expect(v1nodepool.ConvertTo(ctx, v1beta1nodepool)).To(Succeed())
+			result, err := json.Marshal(lo.FromPtr(v1beta1nodepool.Spec.Disruption.ConsolidateAfter))
+			Expect(err).To(BeNil())
+			Expect(string(result)).To(Equal(`"Never"`))
+		})
+	})
 })

pkg/apis/v1/nodepool_validation_cel_test.go

@@ -62,47 +62,47 @@ var _ = Describe("CEL/Validation", func() {
 	})
 	Context("Disruption", func() {
 		It("should fail on negative expireAfter", func() {
-			nodePool.Spec.Template.Spec.ExpireAfter.Duration = lo.ToPtr(lo.Must(time.ParseDuration("-1s")))
+			nodePool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("-1s")
 			Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed())
 		})
 		It("should succeed on a disabled expireAfter", func() {
-			nodePool.Spec.Template.Spec.ExpireAfter.Duration = nil
+			nodePool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("Never")
 			Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
 		})
 		It("should succeed on a valid expireAfter", func() {
-			nodePool.Spec.Template.Spec.ExpireAfter.Duration = lo.ToPtr(lo.Must(time.ParseDuration("30s")))
+			nodePool.Spec.Template.Spec.ExpireAfter = MustParseNillableDuration("30s")
 			Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
 		})
 		It("should fail on negative consolidateAfter", func() {
-			nodePool.Spec.Disruption.ConsolidateAfter = NillableDuration{Duration: lo.ToPtr(lo.Must(time.ParseDuration("-1s")))}
+			nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("-1s")
 			Expect(env.Client.Create(ctx, nodePool)).ToNot(Succeed())
 		})
 		It("should succeed on a disabled consolidateAfter", func() {
-			nodePool.Spec.Disruption.ConsolidateAfter = NillableDuration{Duration: nil}
+			nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("Never")
 			Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
 		})
 		It("should succeed on a valid consolidateAfter", func() {
-			nodePool.Spec.Disruption.ConsolidateAfter = NillableDuration{Duration: lo.ToPtr(lo.Must(time.ParseDuration("30s")))}
+			nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("30s")
 			nodePool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmpty
 			Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
 		})
 		It("should succeed when setting consolidateAfter with consolidationPolicy=WhenEmpty", func() {
-			nodePool.Spec.Disruption.ConsolidateAfter = NillableDuration{Duration: lo.ToPtr(lo.Must(time.ParseDuration("30s")))}
+			nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("30s")
 			nodePool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmpty
 			Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
 		})
-		It("should succeed  when setting consolidateAfter with consolidationPolicy=WhenEmptyOrUnderutilized", func() {
-			nodePool.Spec.Disruption.ConsolidateAfter = NillableDuration{Duration: lo.ToPtr(lo.Must(time.ParseDuration("30s")))}
+		It("should succeed when setting consolidateAfter with consolidationPolicy=WhenUnderutilized", func() {
+			nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("30s")
 			nodePool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmptyOrUnderutilized
 			Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
 		})
-		It("should succeed when setting consolidateAfter to 'Never' with consolidationPolicy=WhenEmptyOrUnderutilized", func() {
-			nodePool.Spec.Disruption.ConsolidateAfter = NillableDuration{Duration: nil}
+		It("should succeed when setting consolidateAfter to 'Never' with consolidationPolicy=WhenUnderutilized", func() {
+			nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("Never")
 			nodePool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmptyOrUnderutilized
 			Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
 		})
 		It("should succeed when setting consolidateAfter to 'Never' with consolidationPolicy=WhenEmpty", func() {
-			nodePool.Spec.Disruption.ConsolidateAfter = NillableDuration{Duration: nil}
+			nodePool.Spec.Disruption.ConsolidateAfter = MustParseNillableDuration("Never")
 			nodePool.Spec.Disruption.ConsolidationPolicy = ConsolidationPolicyWhenEmpty
 			Expect(env.Client.Create(ctx, nodePool)).To(Succeed())
 		})

pkg/apis/v1beta1/duration.go

@@ -16,7 +16,11 @@ package v1beta1
 
 import (
 	"encoding/json"
+	"fmt"
+	"slices"
 	"time"
+
+	"github.com/samber/lo"
 )
 
 const Never = "Never"
@@ -26,6 +30,17 @@ const Never = "Never"
 // that the duration is disabled and sets the inner duration as nil
 type NillableDuration struct {
 	*time.Duration
+
+	// Raw is used to ensure we remarshal the NillableDuration in the same format it was specified.
+	// This ensures tools like Flux and ArgoCD don't mistakenly detect drift due to our conversion webhooks.
+	Raw []byte `hash:"ignore"`
+}
+
+func MustParseNillableDuration(val string) NillableDuration {
+	nd := NillableDuration{}
+	// Use %q instead of %s to ensure that we unmarshal the value as a string and not an int
+	lo.Must0(json.Unmarshal([]byte(fmt.Sprintf("%q", val)), &nd))
+	return nd
 }
 
 // UnmarshalJSON implements the json.Unmarshaller interface.
@@ -42,22 +57,29 @@ func (d *NillableDuration) UnmarshalJSON(b []byte) error {
 	if err != nil {
 		return err
 	}
+	d.Raw = slices.Clone(b)
 	d.Duration = &pd
 	return nil
 }
 
 // MarshalJSON implements the json.Marshaler interface.
 func (d NillableDuration) MarshalJSON() ([]byte, error) {
-	if d.Duration == nil {
-		return json.Marshal(Never)
+	if d.Raw != nil {
+		return d.Raw, nil
+	}
+	if d.Duration != nil {
+		return json.Marshal(d.Duration.String())
 	}
-	return json.Marshal(d.Duration.String())
+	return json.Marshal(Never)
 }
 
 // ToUnstructured implements the value.UnstructuredConverter interface.
 func (d NillableDuration) ToUnstructured() interface{} {
-	if d.Duration == nil {
-		return Never
+	if d.Raw != nil {
+		return d.Raw
+	}
+	if d.Duration != nil {
+		return d.Duration.String()
 	}
-	return d.Duration.String()
+	return Never
 }

pkg/controllers/disruption/consolidation_test.go

@@ -3162,7 +3162,7 @@ var _ = Describe("Consolidation", func() {
 		var node1, node2 *v1.Node
 
 		BeforeEach(func() {
-			nodePool.Spec.Disruption.ExpireAfter = v1beta1.NillableDuration{Duration: lo.ToPtr(3 * time.Second)}
+			nodePool.Spec.Disruption.ExpireAfter = lo.ToPtr(v1beta1.MustParseNillableDuration("3s"))
 			nodeClaim1, node1 = test.NodeClaimAndNode(v1beta1.NodeClaim{
 				ObjectMeta: metav1.ObjectMeta{
 					Labels: map[string]string{

pkg/controllers/disruption/drift_test.go

@@ -47,8 +47,8 @@ var _ = Describe("Drift", func() {
 		nodePool = test.NodePool(v1beta1.NodePool{
 			Spec: v1beta1.NodePoolSpec{
 				Disruption: v1beta1.Disruption{
-					ConsolidateAfter: &v1beta1.NillableDuration{Duration: nil},
-					ExpireAfter:      v1beta1.NillableDuration{Duration: nil},
+					ConsolidateAfter: lo.ToPtr(v1beta1.MustParseNillableDuration("Never")),
+					ExpireAfter:      v1beta1.MustParseNillableDuration("Never"),
 				},
 			},
 		})

pkg/controllers/disruption/emptiness_test.go

@@ -42,9 +42,9 @@ var _ = Describe("Emptiness", func() {
 		nodePool = test.NodePool(v1beta1.NodePool{
 			Spec: v1beta1.NodePoolSpec{
 				Disruption: v1beta1.Disruption{
-					ConsolidateAfter:    &v1beta1.NillableDuration{Duration: lo.ToPtr(time.Second * 30)},
+					ConsolidateAfter:    lo.ToPtr(v1beta1.MustParseNillableDuration("30s")),
 					ConsolidationPolicy: v1beta1.ConsolidationPolicyWhenEmpty,
-					ExpireAfter:         v1beta1.NillableDuration{Duration: nil},
+					ExpireAfter:         v1beta1.MustParseNillableDuration("Never"),
 				},
 			},
 		})