always compare client.Object

Author: chrischdi

pkg/controllers/machinesetsync/machineset_sync_controller.go

@@ -21,7 +21,6 @@ import (
 	"errors"
 	"fmt"
 	"slices"
-	"sort"
 
 	"github.com/go-logr/logr"
 	configv1 "github.com/openshift/api/config/v1"
@@ -740,7 +739,7 @@ func (r *MachineSetSyncReconciler) createOrUpdateCAPIInfraMachineTemplate(ctx co
 			return updateErr
 		}
 
-		if len(capiInfraMachineTemplatesDiff) == 0 {
+		if capiInfraMachineTemplatesDiff.HasChanges() {
 			logger.Info("No changes detected for CAPI infra machine template")
 			return nil
 		}
@@ -841,11 +840,11 @@ func (r *MachineSetSyncReconciler) ensureCAPIMachineSet(ctx context.Context, sou
 }
 
 // ensureCAPIMachineSetSpecUpdated updates the CAPI machine set if changes are detected.
-func (r *MachineSetSyncReconciler) ensureCAPIMachineSetSpecUpdated(ctx context.Context, mapiMachineSet *mapiv1beta1.MachineSet, capiMachineSetsDiff map[string]any, updatedOrCreatedCAPIMachineSet *clusterv1.MachineSet) (bool, error) {
+func (r *MachineSetSyncReconciler) ensureCAPIMachineSetSpecUpdated(ctx context.Context, mapiMachineSet *mapiv1beta1.MachineSet, capiMachineSetsDiff util.DiffResult, updatedOrCreatedCAPIMachineSet *clusterv1.MachineSet) (bool, error) {
 	logger := logf.FromContext(ctx)
 
 	// If there are no spec changes, return early.
-	if !hasSpecOrMetadataOrProviderSpecChanges(capiMachineSetsDiff) {
+	if !(capiMachineSetsDiff.HasMetadataChanges() || capiMachineSetsDiff.HasSpecChanges() || capiMachineSetsDiff.HasProviderSpecChanges()) {
 		return false, nil
 	}
 
@@ -867,11 +866,11 @@ func (r *MachineSetSyncReconciler) ensureCAPIMachineSetSpecUpdated(ctx context.C
 }
 
 // ensureCAPIMachineSetStatusUpdated updates the CAPI machine set status if changes are detected and conditions are met.
-func (r *MachineSetSyncReconciler) ensureCAPIMachineSetStatusUpdated(ctx context.Context, mapiMachineSet *mapiv1beta1.MachineSet, existingCAPIMachineSet *clusterv1.MachineSet, convertedCAPIMachineSet *clusterv1.MachineSet, updatedOrCreatedCAPIMachineSet *clusterv1.MachineSet, capiMachineSetsDiff map[string]any, specUpdated bool) (bool, error) {
+func (r *MachineSetSyncReconciler) ensureCAPIMachineSetStatusUpdated(ctx context.Context, mapiMachineSet *mapiv1beta1.MachineSet, existingCAPIMachineSet *clusterv1.MachineSet, convertedCAPIMachineSet *clusterv1.MachineSet, updatedOrCreatedCAPIMachineSet *clusterv1.MachineSet, capiMachineSetsDiff util.DiffResult, specUpdated bool) (bool, error) {
 	logger := logf.FromContext(ctx)
 
 	// If there are no status changes and the spec has not been updated, return early.
-	if !hasStatusChanges(capiMachineSetsDiff) && !specUpdated {
+	if !capiMachineSetsDiff.HasStatusChanges() && !specUpdated {
 		return false, nil
 	}
 
@@ -918,11 +917,11 @@ func (r *MachineSetSyncReconciler) ensureCAPIMachineSetStatusUpdated(ctx context
 }
 
 // ensureMAPIMachineSetSpecUpdated updates the MAPI machine set if changes are detected.
-func (r *MachineSetSyncReconciler) ensureMAPIMachineSetSpecUpdated(ctx context.Context, mapiMachineSet *mapiv1beta1.MachineSet, mapiMachineSetsDiff map[string]any, updatedMAPIMachineSet *mapiv1beta1.MachineSet) (bool, error) {
+func (r *MachineSetSyncReconciler) ensureMAPIMachineSetSpecUpdated(ctx context.Context, mapiMachineSet *mapiv1beta1.MachineSet, mapiMachineSetsDiff util.DiffResult, updatedMAPIMachineSet *mapiv1beta1.MachineSet) (bool, error) {
 	logger := logf.FromContext(ctx)
 
 	// If there are no spec changes, return early.
-	if !hasSpecOrMetadataOrProviderSpecChanges(mapiMachineSetsDiff) {
+	if !(mapiMachineSetsDiff.HasMetadataChanges() || mapiMachineSetsDiff.HasSpecChanges() || mapiMachineSetsDiff.HasProviderSpecChanges()) {
 		return false, nil
 	}
 
@@ -945,11 +944,11 @@ func (r *MachineSetSyncReconciler) ensureMAPIMachineSetSpecUpdated(ctx context.C
 }
 
 // ensureMAPIMachineSetStatusUpdated updates the MAPI machine set status if changes are detected and conditions are met.
-func (r *MachineSetSyncReconciler) ensureMAPIMachineSetStatusUpdated(ctx context.Context, existingMAPIMachineSet *mapiv1beta1.MachineSet, convertedMAPIMachineSet *mapiv1beta1.MachineSet, updatedMAPIMachineSet *mapiv1beta1.MachineSet, sourceCAPIMachineSet *clusterv1.MachineSet, mapiMachineSetsDiff map[string]any, specUpdated bool) (bool, error) {
+func (r *MachineSetSyncReconciler) ensureMAPIMachineSetStatusUpdated(ctx context.Context, existingMAPIMachineSet *mapiv1beta1.MachineSet, convertedMAPIMachineSet *mapiv1beta1.MachineSet, updatedMAPIMachineSet *mapiv1beta1.MachineSet, sourceCAPIMachineSet *clusterv1.MachineSet, mapiMachineSetsDiff util.DiffResult, specUpdated bool) (bool, error) {
 	logger := logf.FromContext(ctx)
 
 	// If there are no status changes and the spec has not been updated, return early.
-	if !hasStatusChanges(mapiMachineSetsDiff) && !specUpdated {
+	if !mapiMachineSetsDiff.HasStatusChanges() && !specUpdated {
 		return false, nil
 	}
 
@@ -1019,7 +1018,7 @@ func (r *MachineSetSyncReconciler) updateMAPIMachineSet(ctx context.Context, exi
 
 	// Here we always assume the existingMAPIMachineSet already exists, so we don't need to create it.
 
-	mapiMachineSetsDiff, err := compareMAPIMachineSets(existingMAPIMachineSet, convertedMAPIMachineSet)
+	mapiMachineSetsDiff, err := compareMAPIMachineSets(r.Platform, existingMAPIMachineSet, convertedMAPIMachineSet)
 	if err != nil {
 		return fmt.Errorf("unable to compare MAPI machine sets: %w", err)
 	}
@@ -1315,14 +1314,8 @@ func initInfraMachineTemplateListAndInfraClusterListFromProvider(platform config
 }
 
 // compareCAPIInfraMachineTemplates compares CAPI infra machine templates a and b, and returns a list of differences, or none if there are none.
-//
-//nolint:funlen
-func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachineTemplate1, infraMachineTemplate2 client.Object) (map[string]any, error) {
-	diff := make(map[string]any)
-
-	var metadata1, metadata2 metav1.ObjectMeta
-
-	var spec1, spec2, status1, status2 any
+func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachineTemplate1, infraMachineTemplate2 client.Object) (util.DiffResult, error) {
+	var obj1, obj2 client.Object
 
 	switch platform {
 	case configv1.AWSPlatformType:
@@ -1336,12 +1329,8 @@ func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachi
 			return nil, errAssertingCAPIAWSMachineTemplate
 		}
 
-		spec1 = typedInfraMachineTemplate1.Spec
-		spec2 = typedinfraMachineTemplate2.Spec
-		metadata1 = typedInfraMachineTemplate1.ObjectMeta
-		metadata2 = typedinfraMachineTemplate2.ObjectMeta
-		status1 = typedInfraMachineTemplate1.Status
-		status2 = typedinfraMachineTemplate2.Status
+		obj1 = typedInfraMachineTemplate1
+		obj2 = typedinfraMachineTemplate2
 	case configv1.OpenStackPlatformType:
 		typedInfraMachineTemplate1, ok := infraMachineTemplate1.(*openstackv1.OpenStackMachineTemplate)
 		if !ok {
@@ -1353,10 +1342,8 @@ func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachi
 			return nil, errAssertingCAPIOpenStackMachineTemplate
 		}
 
-		spec1 = typedInfraMachineTemplate1.Spec
-		spec2 = typedinfraMachineTemplate2.Spec
-		metadata1 = typedInfraMachineTemplate1.ObjectMeta
-		metadata2 = typedinfraMachineTemplate2.ObjectMeta
+		obj1 = typedInfraMachineTemplate1
+		obj2 = typedinfraMachineTemplate2
 	case configv1.PowerVSPlatformType:
 		typedInfraMachineTemplate1, ok := infraMachineTemplate1.(*ibmpowervsv1.IBMPowerVSMachineTemplate)
 		if !ok {
@@ -1368,124 +1355,44 @@ func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachi
 			return nil, errAssertingCAPIIBMPowerVSMachineTemplate
 		}
 
-		spec1 = typedInfraMachineTemplate1.Spec
-		spec2 = typedinfraMachineTemplate2.Spec
-		metadata1 = typedInfraMachineTemplate1.ObjectMeta
-		metadata2 = typedinfraMachineTemplate2.ObjectMeta
-		status1 = typedInfraMachineTemplate1.Status
-		status2 = typedinfraMachineTemplate2.Status
+		obj1 = typedInfraMachineTemplate1
+		obj2 = typedinfraMachineTemplate2
 	default:
 		return nil, fmt.Errorf("%w: %s", errPlatformNotSupported, platform)
 	}
 
-	// Compare metadata
-	if diffMetadata, err := util.ObjectMetaEqual(metadata1, metadata2); err != nil {
-		return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine metadata: %w", err)
-	} else if diffMetadata.Changed() {
-		diff[".metadata"] = diffMetadata.String()
-	}
-
-	// Compare spec
-	if diffSpec, err := util.NewDiffer().Diff(spec1, spec2); err != nil {
-		return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine spec: %w", err)
-	} else if diffSpec.Changed() {
-		diff[".spec"] = diffSpec.String()
-	}
-
-	// Compare status
-	if diffStatus, err := util.NewDiffer(util.WithIgnoreConditionsLastTransitionTime()).Diff(status1, status2); err != nil {
-		return nil, fmt.Errorf("failed to compare Cluster API Infrastructure machine status: %w", err)
-	} else if diffStatus.Changed() {
-		diff[".status"] = diffStatus.String()
+	diff, err := util.NewDefaultDiffer().Diff(obj1, obj2)
+	if err != nil {
+		return nil, fmt.Errorf("failed to compare Cluster API infrastructure machine templates: %w", err)
 	}
 
 	return diff, nil
 }
 
 // compareCAPIMachineSets compares CAPI machineSets a and b, and returns a list of differences, or none if there are none.
-func compareCAPIMachineSets(capiMachineSet1, capiMachineSet2 *clusterv1.MachineSet) (map[string]any, error) {
-	diff := make(map[string]any)
-
-	// Compare metadata
-	if diffMetadata, err := util.ObjectMetaEqual(capiMachineSet1.ObjectMeta, capiMachineSet2.ObjectMeta); err != nil {
-		return nil, fmt.Errorf("failed to compare Cluster sAPI machine set metadata: %w", err)
-	} else if diffMetadata.Changed() {
-		diff[".metadata"] = diffMetadata.String()
-	}
-
-	// Compare spec
-	if diffSpec, err := util.NewDiffer().Diff(capiMachineSet1.Spec, capiMachineSet2.Spec); err != nil {
-		return nil, fmt.Errorf("failed to compare Cluster API machine spec: %w", err)
-	} else if diffSpec.Changed() {
-		diff[".spec"] = diffSpec.String()
-	}
-
-	// Compare status
-	if diffStatus, err := util.CAPIMachineSetStatusEqual(capiMachineSet1.Status, capiMachineSet2.Status); err != nil {
-		return nil, fmt.Errorf("failed to compare Cluster API machine set status: %w", err)
-	} else if diffStatus.Changed() {
-		diff[".status"] = diffStatus.String()
+func compareCAPIMachineSets(capiMachineSet1, capiMachineSet2 *clusterv1.MachineSet) (util.DiffResult, error) {
+	diff, err := util.NewDefaultDiffer().Diff(capiMachineSet1, capiMachineSet2)
+	if err != nil {
+		return nil, fmt.Errorf("failed to compare Cluster API machinesets: %w", err)
 	}
 
 	return diff, nil
 }
 
 // compareMAPIMachineSets compares MAPI machineSets a and b, and returns a list of differences, or none if there are none.
-func compareMAPIMachineSets(a, b *mapiv1beta1.MachineSet) (map[string]any, error) {
-	diff := make(map[string]any)
+func compareMAPIMachineSets(platform configv1.PlatformType, a, b *mapiv1beta1.MachineSet) (util.DiffResult, error) {
+	diff, err := util.NewDefaultDiffer(
+		util.WithProviderSpec(platform, []string{"spec", "template", "providerSpec", "value"}, mapi2capi.ProviderSpecFromRawExtension),
 
-	ps1, err := mapi2capi.AWSProviderSpecFromRawExtension(a.Spec.Template.Spec.ProviderSpec.Value)
-	if err != nil {
-		return nil, fmt.Errorf("unable to parse first MAPI machine set providerSpec: %w", err)
-	}
+		// Other status fields to ignore
+		util.WithIgnoreField("status", "replicas"),
+		util.WithIgnoreField("status", "observedGeneration"),
+		util.WithIgnoreField("status", "authoritativeAPI"),
+		util.WithIgnoreField("status", "synchronizedGeneration"),
+	).Diff(a, b)
 
-	ps2, err := mapi2capi.AWSProviderSpecFromRawExtension(b.Spec.Template.Spec.ProviderSpec.Value)
 	if err != nil {
-		return nil, fmt.Errorf("unable to parse second MAPI machine set providerSpec: %w", err)
-	}
-
-	// Sort the tags by name to ensure consistent ordering.
-	// On the CAPI side these tags are in a map,
-	// so the order is not guaranteed when converting back from a CAPI map to a MAPI slice.
-	sort.Slice(ps1.Tags, func(i, j int) bool {
-		return ps1.Tags[i].Name < ps1.Tags[j].Name
-	})
-
-	// Sort the tags by name to ensure consistent ordering.
-	// On the CAPI side these tags are in a map,
-	// so the order is not guaranteed when converting back from a CAPI map to a MAPI slice.
-	sort.Slice(ps2.Tags, func(i, j int) bool {
-		return ps2.Tags[i].Name < ps2.Tags[j].Name
-	})
-
-	// Compare providerSpec
-	if diffProviderSpec, err := util.NewDiffer().Diff(ps1, ps2); err != nil {
-		return nil, fmt.Errorf("failed to compare Machine API machine spec: %w", err)
-	} else if diffProviderSpec.Changed() {
-		diff[".providerSpec"] = diffProviderSpec.String()
-	}
-
-	// Compare metadata
-	if diffMetadata, err := util.ObjectMetaEqual(a.ObjectMeta, b.ObjectMeta); err != nil {
-		return nil, fmt.Errorf("failed to compare Machine API machine set metadata: %w", err)
-	} else if diffMetadata.Changed() {
-		diff[".metadata"] = diffMetadata.String()
-	}
-
-	// Compare spec
-	if diffSpec, err := util.NewDiffer(
-		util.WithIgnoreField("template", "providerSpec", "value"),
-	).Diff(a.Spec, b.Spec); err != nil {
-		return nil, fmt.Errorf("failed to compare Machine API machine spec: %w", err)
-	} else if diffSpec.Changed() {
-		diff[".spec"] = diffSpec.String()
-	}
-
-	// Compare status
-	if diffStatus, err := util.MAPIMachineSetStatusEqual(a.Status, b.Status); err != nil {
-		return nil, fmt.Errorf("failed to compare Machine API machine set status: %w", err)
-	} else if diffStatus.Changed() {
-		diff[".status"] = diffStatus.String()
+		return nil, fmt.Errorf("failed to compare Machine API machinesets: %w", err)
 	}
 
 	return diff, nil
@@ -1542,19 +1449,3 @@ func restoreMAPIFields(existingMAPIMachineSet, convertedMAPIMachineSet *mapiv1be
 	// Restore finalizers.
 	convertedMAPIMachineSet.SetFinalizers(existingMAPIMachineSet.GetFinalizers())
 }
-
-// hasStatusChanges returns true if there are changes to the status.
-func hasStatusChanges(diff map[string]any) bool {
-	_, ok := diff[".status"]
-
-	return ok
-}
-
-// hasSpecOrMetadataOrProviderSpecChanges returns true if there are changes to the spec, metadata, or providerSpec.
-func hasSpecOrMetadataOrProviderSpecChanges(diff map[string]any) bool {
-	_, ok1 := diff[".spec"]
-	_, ok2 := diff[".metadata"]
-	_, ok3 := diff[".providerSpec"]
-
-	return ok1 || ok2 || ok3
-}

pkg/controllers/machinesetsync/machineset_sync_controller_test.go

@@ -1174,18 +1174,17 @@ var _ = Describe("compareMAPIMachineSets", func() {
 
 	Context("when comparing MachineSets with different instance types", func() {
 		It("should detect differences in providerSpec", func() {
-			diff, err := compareMAPIMachineSets(mapiMachineSet1, mapiMachineSet2)
+			diff, err := compareMAPIMachineSets(configv1.AWSPlatformType, mapiMachineSet1, mapiMachineSet2)
 			Expect(err).NotTo(HaveOccurred())
-			Expect(diff).To(HaveKey(".providerSpec"))
-			Expect(diff[".providerSpec"]).NotTo(BeEmpty())
+			Expect(diff.HasProviderSpecChanges()).To(BeTrue())
 		})
 	})
 
 	Context("when comparing identical MachineSets", func() {
 		It("should detect no differences", func() {
-			diff, err := compareMAPIMachineSets(mapiMachineSet1, mapiMachineSet1)
+			diff, err := compareMAPIMachineSets(configv1.AWSPlatformType, mapiMachineSet1, mapiMachineSet1)
 			Expect(err).NotTo(HaveOccurred())
-			Expect(diff).To(BeEmpty())
+			Expect(diff.HasChanges()).To(BeFalse())
 		})
 	})
 })