diff --git a/pkg/controllers/machinesetsync/machineset_sync_controller.go b/pkg/controllers/machinesetsync/machineset_sync_controller.go index 1108ee77a..cfcdde227 100644 --- a/pkg/controllers/machinesetsync/machineset_sync_controller.go +++ b/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" @@ -40,7 +39,6 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" utilerrors "k8s.io/apimachinery/pkg/util/errors" - "github.com/go-test/deep" machinev1applyconfigs "github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" @@ -775,7 +773,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 } @@ -820,7 +818,10 @@ func (r *MachineSetSyncReconciler) createOrUpdateCAPIMachineSet(ctx context.Cont // If there is an existing CAPI machine set, work out if it needs updating. // Compare the existing CAPI machine set with the converted CAPI machine set to check for changes. - capiMachineSetsDiff := compareCAPIMachineSets(existingCAPIMachineSet, convertedCAPIMachineSet) + capiMachineSetsDiff, err := compareCAPIMachineSets(existingCAPIMachineSet, convertedCAPIMachineSet) + if err != nil { + return fmt.Errorf("failed to compare Cluster API machine sets: %w", err) + } // Make a deep copy of the converted CAPI machine set to avoid modifying the original. updatedOrCreatedCAPIMachineSet := convertedCAPIMachineSet.DeepCopy() @@ -873,11 +874,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 } @@ -899,11 +900,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 } @@ -950,11 +951,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 } @@ -977,11 +978,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 } @@ -1051,7 +1052,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) } @@ -1347,9 +1348,9 @@ 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) { +func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachineTemplate1, infraMachineTemplate2 client.Object) (util.DiffResult, error) { + var obj1, obj2 client.Object + switch platform { case configv1.AWSPlatformType: typedInfraMachineTemplate1, ok := infraMachineTemplate1.(*awsv1.AWSMachineTemplate) @@ -1362,19 +1363,8 @@ func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachi return nil, errAssertingCAPIAWSMachineTemplate } - diff := make(map[string]any) - - if diffSpec := deep.Equal(typedInfraMachineTemplate1.Spec, typedinfraMachineTemplate2.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec - } - - if diffObjectMeta := util.ObjectMetaEqual(typedInfraMachineTemplate1.ObjectMeta, typedinfraMachineTemplate2.ObjectMeta); len(diffObjectMeta) > 0 { - diff[".metadata"] = diffObjectMeta - } - - // TODO: Evaluate if we want to add status comparison if needed in the future (e.g. for scale from zero capacity). - - return diff, nil + obj1 = typedInfraMachineTemplate1 + obj2 = typedinfraMachineTemplate2 case configv1.OpenStackPlatformType: typedInfraMachineTemplate1, ok := infraMachineTemplate1.(*openstackv1.OpenStackMachineTemplate) if !ok { @@ -1386,17 +1376,8 @@ func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachi return nil, errAssertingCAPIOpenStackMachineTemplate } - diff := make(map[string]any) - - if diffSpec := deep.Equal(typedInfraMachineTemplate1.Spec, typedinfraMachineTemplate2.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec - } - - if diffObjectMeta := deep.Equal(typedInfraMachineTemplate1.ObjectMeta, typedinfraMachineTemplate2.ObjectMeta); len(diffObjectMeta) > 0 { - diff[".metadata"] = diffObjectMeta - } - - return diff, nil + obj1 = typedInfraMachineTemplate1 + obj2 = typedinfraMachineTemplate2 case configv1.PowerVSPlatformType: typedInfraMachineTemplate1, ok := infraMachineTemplate1.(*ibmpowervsv1.IBMPowerVSMachineTemplate) if !ok { @@ -1408,92 +1389,46 @@ func compareCAPIInfraMachineTemplates(platform configv1.PlatformType, infraMachi return nil, errAssertingCAPIIBMPowerVSMachineTemplate } - diff := make(map[string]any) - - if diffSpec := deep.Equal(typedInfraMachineTemplate1.Spec, typedinfraMachineTemplate2.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec - } - - if diffObjectMeta := deep.Equal(typedInfraMachineTemplate1.ObjectMeta, typedinfraMachineTemplate2.ObjectMeta); len(diffObjectMeta) > 0 { - diff[".metadata"] = diffObjectMeta - } - - // TODO: Evaluate if we want to add status comparison if needed in the future (e.g. for scale from zero capacity). - - return diff, nil + obj1 = typedInfraMachineTemplate1 + obj2 = typedinfraMachineTemplate2 default: return nil, fmt.Errorf("%w: %s", errPlatformNotSupported, platform) } -} -// 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 { - diff := make(map[string]any) - - if diffSpec := deep.Equal(capiMachineSet1.Spec, capiMachineSet2.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec + diff, err := util.NewDefaultDiffer().Diff(obj1, obj2) + if err != nil { + return nil, fmt.Errorf("failed to compare Cluster API infrastructure machine templates: %w", err) } - if diffObjectMeta := util.ObjectMetaEqual(capiMachineSet1.ObjectMeta, capiMachineSet2.ObjectMeta); len(diffObjectMeta) > 0 { - diff[".metadata"] = diffObjectMeta - } + return diff, nil +} - if diffStatus := util.CAPIMachineSetStatusEqual(capiMachineSet1.Status, capiMachineSet2.Status); len(diffStatus) > 0 { - diff[".status"] = diffStatus +// compareCAPIMachineSets compares Cluster API machineSets a and b, and returns a list of differences, or none if there are none. +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 + 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) - - 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) - } +func compareMAPIMachineSets(platform configv1.PlatformType, a, b *mapiv1beta1.MachineSet) (util.DiffResult, error) { + diff, err := util.NewDefaultDiffer( + util.WithProviderSpec(platform, []string{"spec", "template", "spec", "providerSpec", "value"}, mapi2capi.ProviderSpecFromRawExtension), + + // Other status fields to ignore + util.WithIgnoreField("status", "replicas"), + util.WithIgnoreField("status", "observedGeneration"), + util.WithIgnoreField("status", "authoritativeAPI"), + util.WithIgnoreField("status", "synchronizedGeneration"), + // The synchronized condition is always handled by the migration controller separately. + util.WithIgnoreConditionType(string(controllers.SynchronizedCondition)), + ).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 - }) - - if diffProviderSpec := deep.Equal(ps1, ps2); len(diffProviderSpec) > 0 { - diff[".providerSpec"] = diffProviderSpec - } - - // Remove the providerSpec from the Spec as we've already compared them. - aCopy := a.DeepCopy() - aCopy.Spec.Template.Spec.ProviderSpec.Value = nil - - bCopy := b.DeepCopy() - bCopy.Spec.Template.Spec.ProviderSpec.Value = nil - - if diffSpec := deep.Equal(aCopy.Spec, bCopy.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec - } - - if diffMetadata := util.ObjectMetaEqual(aCopy.ObjectMeta, bCopy.ObjectMeta); len(diffMetadata) > 0 { - diff[".metadata"] = diffMetadata - } - - if diffStatus := util.MAPIMachineSetStatusEqual(a.Status, b.Status); len(diffStatus) > 0 { - diff[".status"] = diffStatus + return nil, fmt.Errorf("failed to compare Machine API machinesets: %w", err) } return diff, nil @@ -1551,22 +1486,6 @@ func restoreMAPIFields(existingMAPIMachineSet, convertedMAPIMachineSet *mapiv1be 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 -} - // isTerminalConfigurationError returns true if the provided error is // errInvalidClusterInfraReference or errInvalidInfraMachineTemplateReference. func isTerminalConfigurationError(err error) bool { diff --git a/pkg/controllers/machinesetsync/machineset_sync_controller_test.go b/pkg/controllers/machinesetsync/machineset_sync_controller_test.go index 59bfc1217..943d39508 100644 --- a/pkg/controllers/machinesetsync/machineset_sync_controller_test.go +++ b/pkg/controllers/machinesetsync/machineset_sync_controller_test.go @@ -1190,18 +1190,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()) }) }) }) diff --git a/pkg/controllers/machinesetsync/machineset_sync_controller_unit_test.go b/pkg/controllers/machinesetsync/machineset_sync_controller_unit_test.go new file mode 100644 index 000000000..708700edf --- /dev/null +++ b/pkg/controllers/machinesetsync/machineset_sync_controller_unit_test.go @@ -0,0 +1,330 @@ +/* +Copyright 2025 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package machinesetsync + +import ( + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" + + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" +) + +var _ = Describe("Unit tests for CAPIMachineSetStatusEqual", func() { + now := metav1.Now() + later := metav1.NewTime(now.Add(1 * time.Hour)) + + type testInput struct { + a clusterv1.MachineSetStatus + b clusterv1.MachineSetStatus + want string + wantChanges bool + } + + DescribeTable("when comparing Cluster API MachineSets", + func(tt testInput) { + a := &clusterv1.MachineSet{ + Status: tt.a, + } + b := &clusterv1.MachineSet{ + Status: tt.b, + } + + got, err := compareCAPIMachineSets(a, b) + Expect(err).ToNot(HaveOccurred()) + + Expect(got.HasChanges()).To(Equal(tt.wantChanges)) + + if tt.wantChanges { + Expect(got.String()).To(Equal(tt.want)) + } + }, + Entry("no diff", testInput{ + a: clusterv1.MachineSetStatus{}, + b: clusterv1.MachineSetStatus{}, + want: "", + wantChanges: false, + }), + Entry("diff in ReadyReplicas", testInput{ + a: clusterv1.MachineSetStatus{ + ReadyReplicas: 3, + }, + b: clusterv1.MachineSetStatus{ + ReadyReplicas: 5, + }, + want: ".[status].[readyReplicas]: 3 != 5", + wantChanges: true, + }), + Entry("diff in AvailableReplicas", testInput{ + a: clusterv1.MachineSetStatus{ + AvailableReplicas: 2, + }, + b: clusterv1.MachineSetStatus{ + AvailableReplicas: 4, + }, + want: ".[status].[availableReplicas]: 2 != 4", + wantChanges: true, + }), + Entry("diff in ReadyReplicas and AvailableReplicas", testInput{ + a: clusterv1.MachineSetStatus{ + ReadyReplicas: 3, + AvailableReplicas: 2, + }, + b: clusterv1.MachineSetStatus{ + ReadyReplicas: 5, + AvailableReplicas: 4, + }, + want: ".[status].[availableReplicas]: 2 != 4, .[status].[readyReplicas]: 3 != 5", + wantChanges: true, + }), + Entry("same v1beta1 conditions", testInput{ + a: clusterv1.MachineSetStatus{ + Conditions: []clusterv1.Condition{ + { + Type: clusterv1.ReadyCondition, + Status: corev1.ConditionTrue, + LastTransitionTime: now, + }, + }, + }, + b: clusterv1.MachineSetStatus{ + Conditions: []clusterv1.Condition{ + { + Type: clusterv1.ReadyCondition, + Status: corev1.ConditionTrue, + LastTransitionTime: now, + }, + }, + }, + want: "", + wantChanges: false, + }), + Entry("changed v1beta1 condition Status", testInput{ + a: clusterv1.MachineSetStatus{ + Conditions: []clusterv1.Condition{ + { + Type: clusterv1.ReadyCondition, + Status: corev1.ConditionTrue, + LastTransitionTime: now, + }, + }, + }, + b: clusterv1.MachineSetStatus{ + Conditions: []clusterv1.Condition{ + { + Type: clusterv1.ReadyCondition, + Status: corev1.ConditionFalse, + LastTransitionTime: now, + }, + }, + }, + want: ".[status].[conditions].[type=Ready].[status]: True != False", + wantChanges: true, + }), + Entry("v1beta1 condition LastTransitionTime ignored", testInput{ + a: clusterv1.MachineSetStatus{ + Conditions: []clusterv1.Condition{ + { + Type: clusterv1.ReadyCondition, + Status: corev1.ConditionTrue, + LastTransitionTime: now, + }, + }, + }, + b: clusterv1.MachineSetStatus{ + Conditions: []clusterv1.Condition{ + { + Type: clusterv1.ReadyCondition, + Status: corev1.ConditionTrue, + LastTransitionTime: later, + }, + }, + }, + want: "", + wantChanges: false, + }), + Entry("multiple v1beta1 conditions with one changed", testInput{ + a: clusterv1.MachineSetStatus{ + Conditions: []clusterv1.Condition{ + { + Type: clusterv1.ReadyCondition, + Status: corev1.ConditionTrue, + LastTransitionTime: now, + }, + { + Type: clusterv1.MachinesReadyCondition, + Status: corev1.ConditionTrue, + LastTransitionTime: now, + }, + }, + }, + b: clusterv1.MachineSetStatus{ + Conditions: []clusterv1.Condition{ + { + Type: clusterv1.ReadyCondition, + Status: corev1.ConditionTrue, + LastTransitionTime: now, + }, + { + Type: clusterv1.MachinesReadyCondition, + Status: corev1.ConditionFalse, + LastTransitionTime: now, + }, + }, + }, + want: ".[status].[conditions].[type=MachinesReady].[status]: True != False", + wantChanges: true, + }), + Entry("same v1beta2 conditions", testInput{ + a: clusterv1.MachineSetStatus{ + V1Beta2: &clusterv1.MachineSetV1Beta2Status{ + Conditions: []metav1.Condition{ + { + Type: "Ready", + Status: metav1.ConditionTrue, + LastTransitionTime: now, + Reason: "AllReady", + Message: "All machines are ready", + }, + }, + }, + }, + b: clusterv1.MachineSetStatus{ + V1Beta2: &clusterv1.MachineSetV1Beta2Status{ + Conditions: []metav1.Condition{ + { + Type: "Ready", + Status: metav1.ConditionTrue, + LastTransitionTime: now, + Reason: "AllReady", + Message: "All machines are ready", + }, + }, + }, + }, + want: "", + wantChanges: false, + }), + Entry("changed v1beta2 condition Status", testInput{ + a: clusterv1.MachineSetStatus{ + V1Beta2: &clusterv1.MachineSetV1Beta2Status{ + Conditions: []metav1.Condition{ + { + Type: "Ready", + Status: metav1.ConditionTrue, + LastTransitionTime: now, + }, + }, + }, + }, + b: clusterv1.MachineSetStatus{ + V1Beta2: &clusterv1.MachineSetV1Beta2Status{ + Conditions: []metav1.Condition{ + { + Type: "Ready", + Status: metav1.ConditionFalse, + LastTransitionTime: now, + }, + }, + }, + }, + want: ".[status].[v1beta2].[conditions].[type=Ready].[status]: True != False", + wantChanges: true, + }), + Entry("v1beta2 condition LastTransitionTime ignored", testInput{ + a: clusterv1.MachineSetStatus{ + V1Beta2: &clusterv1.MachineSetV1Beta2Status{ + Conditions: []metav1.Condition{ + { + Type: "Ready", + Status: metav1.ConditionTrue, + LastTransitionTime: now, + Reason: "AllReady", + Message: "All machines are ready", + }, + }, + }, + }, + b: clusterv1.MachineSetStatus{ + V1Beta2: &clusterv1.MachineSetV1Beta2Status{ + Conditions: []metav1.Condition{ + { + Type: "Ready", + Status: metav1.ConditionTrue, + LastTransitionTime: later, + Reason: "AllReady", + Message: "All machines are ready", + }, + }, + }, + }, + want: "", + wantChanges: false, + }), + Entry("multiple v1beta2 conditions with one changed", testInput{ + a: clusterv1.MachineSetStatus{ + V1Beta2: &clusterv1.MachineSetV1Beta2Status{ + Conditions: []metav1.Condition{ + { + Type: "Ready", + Status: metav1.ConditionTrue, + LastTransitionTime: now, + }, + { + Type: "Available", + Status: metav1.ConditionTrue, + LastTransitionTime: now, + }, + }, + }, + }, + b: clusterv1.MachineSetStatus{ + V1Beta2: &clusterv1.MachineSetV1Beta2Status{ + Conditions: []metav1.Condition{ + { + Type: "Ready", + Status: metav1.ConditionTrue, + LastTransitionTime: now, + }, + { + Type: "Available", + Status: metav1.ConditionFalse, + LastTransitionTime: now, + }, + }, + }, + }, + want: ".[status].[v1beta2].[conditions].[type=Available].[status]: True != False", + wantChanges: true, + }), + Entry("v1beta2 nil vs non-nil", testInput{ + a: clusterv1.MachineSetStatus{ + V1Beta2: nil, + }, + b: clusterv1.MachineSetStatus{ + V1Beta2: &clusterv1.MachineSetV1Beta2Status{ + ReadyReplicas: ptr.To[int32](3), + }, + }, + want: ".[status].[v1beta2]: != [readyReplicas:3]", + wantChanges: true, + }), + ) +}) diff --git a/pkg/controllers/machinesync/machine_sync_controller.go b/pkg/controllers/machinesync/machine_sync_controller.go index 36e280e1e..032ad3534 100644 --- a/pkg/controllers/machinesync/machine_sync_controller.go +++ b/pkg/controllers/machinesync/machine_sync_controller.go @@ -21,7 +21,6 @@ import ( "errors" "fmt" "slices" - "sort" "strings" "time" @@ -36,7 +35,6 @@ import ( "github.com/openshift/cluster-capi-operator/pkg/conversion/mapi2capi" "github.com/openshift/cluster-capi-operator/pkg/util" - "github.com/go-test/deep" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -231,7 +229,7 @@ func (r *MachineSyncReconciler) Reconcile(ctx context.Context, req reconcile.Req capiMachineNotFound = true } else if err != nil { logger.Error(err, "Failed to get Cluster API Machine") - return ctrl.Result{}, fmt.Errorf("failed to get Cluster API machine:: %w", err) + return ctrl.Result{}, fmt.Errorf("failed to get Cluster API machine: %w", err) } if mapiMachineNotFound && capiMachineNotFound { @@ -483,7 +481,7 @@ func (r *MachineSyncReconciler) reconcileMAPIMachinetoCAPIMachine(ctx context.Co return ctrl.Result{}, fmt.Errorf("failed to convert Machine API machine owner references to Cluster API: %w", err) } - if err := r.validateMAPIToCAPIPlatfromSpecifics(ctx, sourceMAPIMachine); err != nil { + if err := r.validateMAPIToCAPIPlatformSpecifics(ctx, sourceMAPIMachine); err != nil { conversionErr := fmt.Errorf("failed to convert Machine API machine to Cluster API machine: %w", err) if condErr := r.applySynchronizedConditionWithPatch(ctx, sourceMAPIMachine, corev1.ConditionFalse, reasonFailedToConvertMAPIMachineToCAPI, conversionErr.Error(), nil); condErr != nil { return ctrl.Result{}, utilerrors.NewAggregate([]error{conversionErr, condErr}) @@ -599,8 +597,8 @@ func (r *MachineSyncReconciler) reconcileMAPIMachinetoCAPIMachine(ctx context.Co controllers.ReasonResourceSynchronized, messageSuccessfullySynchronizedMAPItoCAPI, &sourceMAPIMachine.Generation) } -// validateMAPIToCAPIPlatfromSpecifics verifies that shared CAPI resources are compatible before converting from MAPI -> CAPI. -func (r *MachineSyncReconciler) validateMAPIToCAPIPlatfromSpecifics(ctx context.Context, mapiMachine *mapiv1beta1.Machine) error { +// validateMAPIToCAPIPlatformSpecifics verifies that shared CAPI resources are compatible before converting from MAPI -> CAPI. +func (r *MachineSyncReconciler) validateMAPIToCAPIPlatformSpecifics(ctx context.Context, mapiMachine *mapiv1beta1.Machine) error { switch r.Platform { case configv1.AWSPlatformType: return r.validateMachineAWSLoadBalancers(ctx, mapiMachine) @@ -679,11 +677,11 @@ func (r *MachineSyncReconciler) ensureCAPIMachine(ctx context.Context, sourceMAP } // ensureCAPIMachineSpecUpdated updates the Cluster API machine if changes are detected to the spec, metadata or provider spec. -func (r *MachineSyncReconciler) ensureCAPIMachineSpecUpdated(ctx context.Context, mapiMachine *mapiv1beta1.Machine, capiMachinesDiff map[string]any, convertedCAPIMachine *clusterv1.Machine) (bool, *clusterv1.Machine, error) { +func (r *MachineSyncReconciler) ensureCAPIMachineSpecUpdated(ctx context.Context, mapiMachine *mapiv1beta1.Machine, capiMachinesDiff util.DiffResult, convertedCAPIMachine *clusterv1.Machine) (bool, *clusterv1.Machine, error) { logger := logf.FromContext(ctx) // If there are no spec changes, return early. - if !hasSpecOrMetadataOrProviderSpecChanges(capiMachinesDiff) { + if !(capiMachinesDiff.HasMetadataChanges() || capiMachinesDiff.HasSpecChanges()) { return false, nil, nil } @@ -727,7 +725,10 @@ func (r *MachineSyncReconciler) createOrUpdateCAPIMachine(ctx context.Context, s } // Compare the existing CAPI machine with the desired CAPI machine to check for changes. - capiMachinesDiff := compareCAPIMachines(existingCAPIMachine, convertedCAPIMachine) + capiMachinesDiff, err := compareCAPIMachines(existingCAPIMachine, convertedCAPIMachine) + if err != nil { + return nil, fmt.Errorf("failed to compare Cluster API machines: %w", err) + } // Don't try to update the spec of the machine if it was just created it. // The resourceVersion of the convertedCAPIMachine is empty and would lead to failure. @@ -778,7 +779,7 @@ func (r *MachineSyncReconciler) createOrUpdateMAPIMachine(ctx context.Context, e // There already is an existing MAPI machine, work out if it needs updating. // Compare the existing MAPI machine with the converted MAPI machine to check for changes. - mapiMachinesDiff, err := compareMAPIMachines(existingMAPIMachine, convertedMAPIMachine) + mapiMachinesDiff, err := compareMAPIMachines(r.Platform, existingMAPIMachine, convertedMAPIMachine) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to compare Machine API machines: %w", err) } @@ -1291,86 +1292,46 @@ func (r *MachineSyncReconciler) ensureSyncFinalizer(ctx context.Context, mapiMac } // compareCAPIMachines compares CAPI machines a and b, and returns a list of differences, or none if there are none. -func compareCAPIMachines(capiMachine1, capiMachine2 *clusterv1.Machine) map[string]any { - diff := make(map[string]any) - - if diffSpec := deep.Equal(capiMachine1.Spec, capiMachine2.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec - } - - if diffObjectMeta := util.ObjectMetaEqual(capiMachine1.ObjectMeta, capiMachine2.ObjectMeta); len(diffObjectMeta) > 0 { - diff[".metadata"] = diffObjectMeta - } - - if diffStatus := util.CAPIMachineStatusEqual(capiMachine1.Status, capiMachine2.Status); len(diffStatus) > 0 { - diff[".status"] = diffStatus +func compareCAPIMachines(capiMachine1, capiMachine2 *clusterv1.Machine) (util.DiffResult, error) { + diff, err := util.NewDefaultDiffer( + // The paused condition is always handled by the corresponding CAPI controller. + util.WithIgnoreConditionType(clusterv1.PausedV1Beta2Condition), + ).Diff(capiMachine1, capiMachine2) + if err != nil { + return nil, fmt.Errorf("failed to compare Cluster API machines: %w", err) } - return diff + return diff, nil } // compareMAPIMachines compares MAPI machines a and b, and returns a list of differences, or none if there are none. -func compareMAPIMachines(a, b *mapiv1beta1.Machine) (map[string]any, error) { - diff := make(map[string]any) - - ps1, err := mapi2capi.AWSProviderSpecFromRawExtension(a.Spec.ProviderSpec.Value) - if err != nil { - return nil, fmt.Errorf("unable to parse first Machine API machine set providerSpec: %w", err) - } +func compareMAPIMachines(platform configv1.PlatformType, a, b *mapiv1beta1.Machine) (util.DiffResult, error) { + diff, err := util.NewDefaultDiffer( + util.WithProviderSpec(platform, []string{"spec", "providerSpec", "value"}, mapi2capi.ProviderSpecFromRawExtension), + // Other status fields to ignore + util.WithIgnoreField("status", "providerStatus"), + util.WithIgnoreField("status", "lastOperation"), + util.WithIgnoreField("status", "authoritativeAPI"), + util.WithIgnoreField("status", "synchronizedGeneration"), + // The synchronized condition is always handled by the migration controller separately. + util.WithIgnoreConditionType(string(controllers.SynchronizedCondition)), + ).Diff(a, b) - ps2, err := mapi2capi.AWSProviderSpecFromRawExtension(b.Spec.ProviderSpec.Value) if err != nil { - return nil, fmt.Errorf("unable to parse second Machine API 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 - }) - - if diffProviderSpec := deep.Equal(ps1, ps2); len(diffProviderSpec) > 0 { - diff[".providerSpec"] = diffProviderSpec - } - - // Remove the providerSpec from the Spec as we've already compared them. - aCopy := a.DeepCopy() - aCopy.Spec.ProviderSpec.Value = nil - - bCopy := b.DeepCopy() - bCopy.Spec.ProviderSpec.Value = nil - - if diffSpec := deep.Equal(aCopy.Spec, bCopy.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec - } - - if diffObjectMeta := util.ObjectMetaEqual(aCopy.ObjectMeta, bCopy.ObjectMeta); len(diffObjectMeta) > 0 { - diff[".metadata"] = diffObjectMeta - } - - if diffStatus := util.MAPIMachineStatusEqual(a.Status, b.Status); len(diffStatus) > 0 { - diff[".status"] = diffStatus + return nil, fmt.Errorf("failed to compare Machine API machines: %w", err) } return diff, nil } // ensureCAPIMachineStatusUpdated updates the CAPI machine status if changes are detected and conditions are met. -func (r *MachineSyncReconciler) ensureCAPIMachineStatusUpdated(ctx context.Context, mapiMachine *mapiv1beta1.Machine, existingCAPIMachine, convertedCAPIMachine *clusterv1.Machine, capiMachinesDiff map[string]any, forceUpdate bool) (bool, error) { +func (r *MachineSyncReconciler) ensureCAPIMachineStatusUpdated(ctx context.Context, mapiMachine *mapiv1beta1.Machine, existingCAPIMachine, convertedCAPIMachine *clusterv1.Machine, capiMachinesDiff util.DiffResult, forceUpdate bool) (bool, error) { logger := logf.FromContext(ctx) // If there are spec changes we always want to update the status to sync the spec generation. // If there are status changes we always want to update the status. // If both the above are false, we can skip updating the status and return early. - if !forceUpdate && !hasStatusChanges(capiMachinesDiff) { + if !forceUpdate && !capiMachinesDiff.HasStatusChanges() { return false, nil } @@ -1445,11 +1406,11 @@ func (r *MachineSyncReconciler) ensureMAPIMachine(ctx context.Context, existingM } // ensureMAPIMachineStatusUpdated updates the MAPI machine status if changes are detected. -func (r *MachineSyncReconciler) ensureMAPIMachineStatusUpdated(ctx context.Context, existingMAPIMachine *mapiv1beta1.Machine, convertedMAPIMachine *mapiv1beta1.Machine, mapiMachinesDiff map[string]any, specUpdated bool) (bool, error) { +func (r *MachineSyncReconciler) ensureMAPIMachineStatusUpdated(ctx context.Context, existingMAPIMachine *mapiv1beta1.Machine, convertedMAPIMachine *mapiv1beta1.Machine, mapiMachinesDiff 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(mapiMachinesDiff) && !specUpdated { + if !mapiMachinesDiff.HasStatusChanges() && !specUpdated { return false, nil } @@ -1493,11 +1454,11 @@ func (r *MachineSyncReconciler) ensureMAPIMachineStatusUpdated(ctx context.Conte } // ensureMAPIMachineSpecUpdated updates the MAPI machine if changes are detected. -func (r *MachineSyncReconciler) ensureMAPIMachineSpecUpdated(ctx context.Context, existingMAPIMachine *mapiv1beta1.Machine, mapiMachinesDiff map[string]any, updatedMAPIMachine *mapiv1beta1.Machine) (bool, error) { +func (r *MachineSyncReconciler) ensureMAPIMachineSpecUpdated(ctx context.Context, existingMAPIMachine *mapiv1beta1.Machine, mapiMachinesDiff util.DiffResult, updatedMAPIMachine *mapiv1beta1.Machine) (bool, error) { logger := logf.FromContext(ctx) // If there are no spec changes, return early. - if !hasSpecOrMetadataOrProviderSpecChanges(mapiMachinesDiff) { + if !(mapiMachinesDiff.HasMetadataChanges() || mapiMachinesDiff.HasSpecChanges() || mapiMachinesDiff.HasProviderSpecChanges()) { return false, nil } @@ -1551,12 +1512,6 @@ func setChangedMAPIMachineStatusFields(existingMAPIMachine, convertedMAPIMachine existingMAPIMachine.Status = convertedMAPIMachine.Status } -// hasStatusChanges checks if there are status-related changes in the diff. -func hasStatusChanges(diff map[string]any) bool { - _, hasStatus := diff[".status"] - return hasStatus -} - // applySynchronizedConditionWithPatch updates the synchronized condition // using a server side apply patch. We do this to force ownership of the // 'Synchronized' condition and 'SynchronizedGeneration'. @@ -1567,24 +1522,6 @@ func (r *MachineSyncReconciler) applySynchronizedConditionWithPatch(ctx context. status, reason, message, generation) } -// hasSpecOrMetadataOrProviderSpecChanges checks if there are spec, metadata, or provider spec changes in the diff. -func hasSpecOrMetadataOrProviderSpecChanges(diff map[string]any) bool { - _, hasProviderSpec := diff[".providerSpec"] - return hasSpecChanges(diff) || hasMetadataChanges(diff) || hasProviderSpec -} - -// hasSpecChanges checks if there are spec changes in the diff. -func hasSpecChanges(diff map[string]any) bool { - _, hasSpec := diff[".spec"] - return hasSpec -} - -// hasSpecChanges checks if there are spec changes in the diff. -func hasMetadataChanges(diff map[string]any) bool { - _, hasSpec := diff[".metadata"] - return hasSpec -} - // isTerminalConfigurationError returns true if the provided error is // errInvalidClusterInfraReference or errInvalidInfraMachineReference. func isTerminalConfigurationError(err error) bool { diff --git a/pkg/controllers/machinesync/machine_sync_controller_test.go b/pkg/controllers/machinesync/machine_sync_controller_test.go index c16a6b6dd..1aed33017 100644 --- a/pkg/controllers/machinesync/machine_sync_controller_test.go +++ b/pkg/controllers/machinesync/machine_sync_controller_test.go @@ -336,6 +336,47 @@ var _ = Describe("With a running MachineSync Reconciler", func() { ) }) + Context("when the MAPI machine has MachineAuthority set to Machine API and the providerSpec has security groups", func() { + BeforeEach(func() { + By("Creating the MAPI machine") + providerSpecBuilder := machinev1resourcebuilder.AWSProviderSpec().WithLoadBalancers(nil). + WithSecurityGroups( + []mapiv1beta1.AWSResourceReference{ + { + Filters: []mapiv1beta1.Filter{{ + Name: "tag:Name", + Values: []string{"some-tag"}, + }}, + }, + { + ID: ptr.To("some-sg-id"), + }, + }, + ) + mapiMachine = mapiMachineBuilder.WithProviderSpecBuilder(providerSpecBuilder).Build() + + Eventually(k8sClient.Create(ctx, mapiMachine)).Should(Succeed(), "mapi machine should be able to be created") + + By("Setting the MAPI machine AuthoritativeAPI to MachineAPI") + Eventually(k.UpdateStatus(mapiMachine, func() { + mapiMachine.Status.AuthoritativeAPI = mapiv1beta1.MachineAuthorityMachineAPI + })).Should(Succeed()) + }) + + Context("when the CAPI machine does not exist", func() { + It("should update the synchronized condition on the MAPI machine to True", func() { + Eventually(k.Object(mapiMachine), timeout).Should( + HaveField("Status.Conditions", ContainElement( + SatisfyAll( + HaveField("Type", Equal(consts.SynchronizedCondition)), + HaveField("Status", Equal(corev1.ConditionTrue)), + HaveField("Reason", Equal("ResourceSynchronized")), + HaveField("Message", Equal("Successfully synchronized MAPI Machine to CAPI")), + ))), + ) + }) + }) + }) Context("when the MAPI machine has MachineAuthority set to Machine API", func() { BeforeEach(func() { By("Creating the MAPI machine") diff --git a/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go b/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go index 3edf96b28..948c03738 100644 --- a/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go +++ b/pkg/controllers/machinesync/machine_sync_mapi2capi_infrastructure.go @@ -20,7 +20,6 @@ import ( "context" "fmt" - "github.com/go-test/deep" configv1 "github.com/openshift/api/config/v1" mapiv1beta1 "github.com/openshift/api/machine/v1beta1" "github.com/openshift/cluster-capi-operator/pkg/util" @@ -30,6 +29,7 @@ import ( awsv1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" ibmpowervsv1 "sigs.k8s.io/cluster-api-provider-ibmcloud/api/v1beta2" openstackv1 "sigs.k8s.io/cluster-api-provider-openstack/api/v1beta1" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -64,7 +64,7 @@ func (r *MachineSyncReconciler) createOrUpdateCAPIInfraMachine(ctx context.Conte // Infrastructure machines are immutable so we delete it for spec changes. // The next reconciliation will create it with the expected changes. // Note: this could be improved to only trigger deletion on known immutable changes. - if hasSpecChanges(diff) { + if diff.HasSpecChanges() { logger.Info("Deleting the corresponding Cluster API Infrastructure machine as it is out of date, it will be recreated", "diff", fmt.Sprintf("%+v", diff)) syncronizationIsProgressing, err := r.ensureCAPIInfraMachineDeleted(ctx, sourceMAPIMachine, existingCAPIInfraMachine) @@ -85,9 +85,9 @@ func (r *MachineSyncReconciler) createOrUpdateCAPIInfraMachine(ctx context.Conte } if metadataUpdated || statusUpdated { - logger.Info("Successfully updated Cluster API machine") + logger.Info("Successfully updated Cluster API Infrastructure machine") } else { - logger.Info("No changes detected for Cluster API machine") + logger.Info("No changes detected for Cluster API Infrastructure machine") } return ctrl.Result{}, syncronizationIsProgressingFalse, nil @@ -122,11 +122,11 @@ func (r *MachineSyncReconciler) ensureCAPIInfraMachine(ctx context.Context, sour } // ensureCAPIInfraMachineMetadataUpdated updates the Cluster API Infrastructure machine if changes are detected to the metadata or spec (if possible). -func (r *MachineSyncReconciler) ensureCAPIInfraMachineMetadataUpdated(ctx context.Context, mapiMachine *mapiv1beta1.Machine, diff map[string]any, updatedOrCreatedCAPIInfraMachine client.Object) (bool, error) { +func (r *MachineSyncReconciler) ensureCAPIInfraMachineMetadataUpdated(ctx context.Context, mapiMachine *mapiv1beta1.Machine, diff util.DiffResult, updatedOrCreatedCAPIInfraMachine client.Object) (bool, error) { logger := logf.FromContext(ctx) // If there are no spec changes, return early. - if !hasMetadataChanges(diff) { + if !diff.HasMetadataChanges() { return false, nil } @@ -147,11 +147,11 @@ func (r *MachineSyncReconciler) ensureCAPIInfraMachineMetadataUpdated(ctx contex return true, nil } -func (r *MachineSyncReconciler) ensureCAPIInfraMachineStatusUpdated(ctx context.Context, mapiMachine *mapiv1beta1.Machine, existingCAPIInfraMachine, convertedCAPIInfraMachine client.Object, diff map[string]any, specUpdated bool) (bool, error) { +func (r *MachineSyncReconciler) ensureCAPIInfraMachineStatusUpdated(ctx context.Context, mapiMachine *mapiv1beta1.Machine, existingCAPIInfraMachine, convertedCAPIInfraMachine client.Object, diff 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(diff) && !specUpdated { + if !diff.HasStatusChanges() && !specUpdated { return false, nil } @@ -246,10 +246,8 @@ func (r *MachineSyncReconciler) ensureCAPIInfraMachineDeleted(ctx context.Contex } // compareCAPIInfraMachines compares Cluster API infra machines a and b, and returns a list of differences, or none if there are none. -// -//nolint:funlen,gocognit -func compareCAPIInfraMachines(platform configv1.PlatformType, infraMachine1, infraMachine2 client.Object) (map[string]any, error) { - diff := make(map[string]any) +func compareCAPIInfraMachines(platform configv1.PlatformType, infraMachine1, infraMachine2 client.Object) (util.DiffResult, error) { + var obj1, obj2 client.Object switch platform { case configv1.AWSPlatformType: @@ -263,17 +261,8 @@ func compareCAPIInfraMachines(platform configv1.PlatformType, infraMachine1, inf return nil, errAssertingCAPIAWSMachine } - if diffSpec := deep.Equal(typedInfraMachine1.Spec, typedinfraMachine2.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec - } - - if diffMetadata := util.ObjectMetaEqual(typedInfraMachine1.ObjectMeta, typedinfraMachine2.ObjectMeta); len(diffMetadata) > 0 { - diff[".metadata"] = diffMetadata - } - - if diffStatus := deep.Equal(typedInfraMachine1.Status, typedinfraMachine2.Status); len(diffStatus) > 0 { - diff[".status"] = diffStatus - } + obj1 = typedInfraMachine1 + obj2 = typedinfraMachine2 case configv1.OpenStackPlatformType: typedInfraMachine1, ok := infraMachine1.(*openstackv1.OpenStackMachine) if !ok { @@ -285,17 +274,8 @@ func compareCAPIInfraMachines(platform configv1.PlatformType, infraMachine1, inf return nil, errAssertingCAPIOpenStackMachine } - if diffSpec := deep.Equal(typedInfraMachine1.Spec, typedinfraMachine2.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec - } - - if diffMetadata := util.ObjectMetaEqual(typedInfraMachine1.ObjectMeta, typedinfraMachine2.ObjectMeta); len(diffMetadata) > 0 { - diff[".metadata"] = diffMetadata - } - - if diffStatus := deep.Equal(typedInfraMachine1.Status, typedinfraMachine2.Status); len(diffStatus) > 0 { - diff[".status"] = diffStatus - } + obj1 = typedInfraMachine1 + obj2 = typedinfraMachine2 case configv1.PowerVSPlatformType: typedInfraMachine1, ok := infraMachine1.(*ibmpowervsv1.IBMPowerVSMachine) if !ok { @@ -307,22 +287,20 @@ func compareCAPIInfraMachines(platform configv1.PlatformType, infraMachine1, inf return nil, errAssertingCAPIIBMPowerVSMachine } - if diffSpec := deep.Equal(typedInfraMachine1.Spec, typedinfraMachine2.Spec); len(diffSpec) > 0 { - diff[".spec"] = diffSpec - } - - if diffMetadata := util.ObjectMetaEqual(typedInfraMachine1.ObjectMeta, typedinfraMachine2.ObjectMeta); len(diffMetadata) > 0 { - diff[".metadata"] = diffMetadata - } - - if diffStatus := deep.Equal(typedInfraMachine1.Status, typedinfraMachine2.Status); len(diffStatus) > 0 { - diff[".status"] = diffStatus - } - + obj1 = typedInfraMachine1 + obj2 = typedinfraMachine2 default: return nil, fmt.Errorf("%w: %s", errPlatformNotSupported, platform) } + diff, err := util.NewDefaultDiffer( + // The paused condition is always handled by the corresponding CAPI controller. + util.WithIgnoreConditionType(clusterv1.PausedV1Beta2Condition), + ).Diff(obj1, obj2) + if err != nil { + return nil, fmt.Errorf("failed to compare Cluster API infrastructure machines: %w", err) + } + return diff, nil } diff --git a/pkg/conversion/mapi2capi/aws.go b/pkg/conversion/mapi2capi/aws.go index 159fcb992..fc0c30938 100644 --- a/pkg/conversion/mapi2capi/aws.go +++ b/pkg/conversion/mapi2capi/aws.go @@ -19,6 +19,7 @@ import ( "fmt" "reflect" "regexp" + "sort" "strings" configv1 "github.com/openshift/api/config/v1" @@ -436,6 +437,13 @@ func AWSProviderSpecFromRawExtension(rawExtension *runtime.RawExtension) (mapiv1 return mapiv1beta1.AWSMachineProviderConfig{}, fmt.Errorf("error unmarshalling providerSpec: %w", err) } + // Sort the tags by name to ensure consistent ordering. + // On the Cluster API side these tags are in a map, + // so the order is not guaranteed when converting back from a Cluster API map to a Machine API slice. + sort.Slice(spec.Tags, func(i, j int) bool { + return spec.Tags[i].Name < spec.Tags[j].Name + }) + return spec, nil } diff --git a/pkg/conversion/mapi2capi/openstack.go b/pkg/conversion/mapi2capi/openstack.go index b01ca2c27..819d60b0b 100644 --- a/pkg/conversion/mapi2capi/openstack.go +++ b/pkg/conversion/mapi2capi/openstack.go @@ -17,6 +17,7 @@ package mapi2capi import ( "fmt" + "sort" "strings" configv1 "github.com/openshift/api/config/v1" @@ -789,6 +790,10 @@ func convertMAPOServerMetadataToCAPO(mapoServerMetadata map[string]string) []ope capoServerMetadata = append(capoServerMetadata, openstackv1.ServerMetadata{Key: k, Value: v}) } + sort.SliceStable(capoServerMetadata, func(i, j int) bool { + return capoServerMetadata[i].Key < capoServerMetadata[j].Key + }) + return capoServerMetadata } diff --git a/pkg/conversion/mapi2capi/util.go b/pkg/conversion/mapi2capi/util.go index acf61edcd..30ceafdd9 100644 --- a/pkg/conversion/mapi2capi/util.go +++ b/pkg/conversion/mapi2capi/util.go @@ -17,17 +17,37 @@ limitations under the License. package mapi2capi import ( + "errors" "fmt" + configv1 "github.com/openshift/api/config/v1" mapiv1beta1 "github.com/openshift/api/machine/v1beta1" "github.com/openshift/cluster-capi-operator/pkg/conversion/consts" "github.com/openshift/cluster-capi-operator/pkg/util" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" ) +var errUnsupportedPlatform = errors.New("unsupported platform") + +// ProviderSpecFromRawExtension converts a Machine API providerSpec to a Cluster API providerSpec. +func ProviderSpecFromRawExtension(platform configv1.PlatformType, rawExtension *runtime.RawExtension) (any, error) { + switch platform { + case configv1.AWSPlatformType: + providerConfig, err := AWSProviderSpecFromRawExtension(rawExtension) + if err != nil { + return nil, fmt.Errorf("unable to parse AWS providerSpec: %w", err) + } + + return providerConfig, nil + default: + return nil, fmt.Errorf("%w: %s", errUnsupportedPlatform, platform) + } +} + func convertMAPIMachineSetSelectorToCAPI(mapiSelector metav1.LabelSelector) metav1.LabelSelector { capiSelector := mapiSelector.DeepCopy() capiSelector.MatchLabels = convertMAPILabelsToCAPI(mapiSelector.MatchLabels) diff --git a/pkg/util/diff.go b/pkg/util/diff.go new file mode 100644 index 000000000..8c0ab4bd9 --- /dev/null +++ b/pkg/util/diff.go @@ -0,0 +1,419 @@ +/* +Copyright 2025 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package util + +import ( + "errors" + "fmt" + "reflect" + "sort" + "strings" + + configv1 "github.com/openshift/api/config/v1" + + "github.com/go-test/deep" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +var ( + errObjectsToCompareCannotBeNil = errors.New("objects to diff cannot be nil") + errObjectsToCompareNotSameType = errors.New("objects to diff are not of the same type") + errProviderSpecNotFound = errors.New("providerSpec not found") +) + +// DiffResult is the interface that represents the result of a diff operation. +type DiffResult interface { + HasChanges() bool + String() string + HasMetadataChanges() bool + HasSpecChanges() bool + HasProviderSpecChanges() bool + HasStatusChanges() bool +} + +// diffResult is the implementation of the DiffResult interface. +type diffResult struct { + diff map[string][]string + providerSpecPath string +} + +// HasChanges returns true if the diff detected any changes. +func (d *diffResult) HasChanges() bool { + return len(d.diff) > 0 +} + +// HasMetadataChanges returns true if the diff detected any changes to the metadata. +func (d *diffResult) HasMetadataChanges() bool { + _, ok := d.diff["metadata"] + return ok +} + +// HasSpecChanges returns true if the diff detected any changes to the spec. +func (d *diffResult) HasSpecChanges() bool { + _, ok := d.diff["spec"] + return ok +} + +// HasProviderSpecChanges returns true if the diff detected any changes to the providerSpec. +// Only ever returns true if d.providerSpecPath was set. +func (d *diffResult) HasProviderSpecChanges() bool { + if d.providerSpecPath == "" { + return false + } + + _, ok := d.diff[d.providerSpecPath] + + return ok +} + +// HasStatusChanges returns true if the diff detected any changes to the status. +func (d *diffResult) HasStatusChanges() bool { + _, ok := d.diff["status"] + return ok +} + +// String returns the diff as a string. +func (d *diffResult) String() string { + if !d.HasChanges() { + return "" + } + + diff := []string{} + + for k, v := range d.diff { + for _, d := range v { + if strings.Contains(d, "]: ") { + diff = append(diff, fmt.Sprintf("[%s].%s", k, d)) + } else { + diff = append(diff, fmt.Sprintf("[%s]: %s", k, d)) + } + } + } + + sort.Strings(diff) + + out := "." + strings.Join(diff, ", .") + out = strings.ReplaceAll(out, ".slice[", "[") + out = strings.ReplaceAll(out, "map[", "[") + + return out +} + +type differ struct { + modifyFuncs map[string]func(obj map[string]interface{}) error + lateModifyFuncs map[string]func(obj map[string]interface{}) error + ignoredPath [][]string + + // providerSpecPath is a custom path to the providerSpec field which differs between + // Machine API Machines and MachineSets and is passed down to the result to enable HasProviderSpecChanges(). + // It is only used when set, so it does not make a difference for non Machine API objects. + providerSpecPath string +} + +// Diff compares the objects a and b, and returns a DiffResult. +func (d *differ) Diff(a, b client.Object) (DiffResult, error) { + if a == nil || b == nil { + return nil, errObjectsToCompareCannotBeNil + } + + if reflect.TypeOf(a) != reflect.TypeOf(b) { + return nil, fmt.Errorf("%w: %T != %T", errObjectsToCompareNotSameType, a, b) + } + + // 1. Convert the objects to unstructured. + unstructuredA, err := runtime.DefaultUnstructuredConverter.ToUnstructured(a) + if err != nil { + return nil, fmt.Errorf("failed to convert a to unstructured: %w", err) + } + + unstructuredB, err := runtime.DefaultUnstructuredConverter.ToUnstructured(b) + if err != nil { + return nil, fmt.Errorf("failed to convert b to unstructured: %w", err) + } + + // 2. Run the configured modify functions + // This allows customizing the diffing process, e.g. remove conditions last transition time to ignore them during diffing + // or separate handling for providerSpec. + if err := d.applyModifyFuncs(unstructuredA, unstructuredB, d.modifyFuncs); err != nil { + return nil, fmt.Errorf("failed to apply modify functions: %w", err) + } + + // 3. Remove fields configured to be ignored. + for _, ignorePath := range d.ignoredPath { + unstructured.RemoveNestedField(unstructuredA, ignorePath...) + unstructured.RemoveNestedField(unstructuredB, ignorePath...) + } + + // 4. Run the late modify functions. + // This allows customize the diffing process to make objects better comparable. E.g. compare conditions as maps with their type as key. + if err := d.applyModifyFuncs(unstructuredA, unstructuredB, d.lateModifyFuncs); err != nil { + return nil, fmt.Errorf("failed to apply modify functions: %w", err) + } + + // 4. Diff both resulted unstructured objects. + // Record the result for each top-level key in the maps, so it can be used later on for the `Has*Changes` functions. + + // Collect all top-level keys. + allKeys := sets.Set[string]{} + + for k := range unstructuredA { + allKeys.Insert(k) + } + + for k := range unstructuredB { + allKeys.Insert(k) + } + + diffByKey := map[string][]string{} + + // Diff each top-level key separately and record the output to the diff map. + for k := range allKeys { + diff := deep.Equal(unstructuredA[k], unstructuredB[k]) + + // Make the result deterministic. + sort.Strings(diff) + + if len(diff) > 0 { + diffByKey[k] = diff + } + } + + return &diffResult{ + diff: diffByKey, + providerSpecPath: d.providerSpecPath, + }, nil +} + +func (d *differ) applyModifyFuncs(a, b map[string]interface{}, modifyFuncs map[string]func(obj map[string]interface{}) error) error { + for funcName, modifyFunc := range modifyFuncs { + if err := modifyFunc(a); err != nil { + return fmt.Errorf("modify function %s on a failed: %w", funcName, err) + } + + if err := modifyFunc(b); err != nil { + return fmt.Errorf("modify function %s on b failed: %w", funcName, err) + } + } + + return nil +} + +// NewDefaultDiffer creates a new default differ with the default options. +func NewDefaultDiffer(opts ...diffopts) *differ { + return newDiffer(append(opts, + // Always ignore kind and apiVersion as they may not be always set. Instead the differ checks if the input objects have the same type. + WithIgnoreField("kind"), + WithIgnoreField("apiVersion"), + + // Options for handling of metadata fields. + + // Special handling for Cluster API's conversion-data label. + WithIgnoreField("metadata", "annotations", "cluster.x-k8s.io/conversion-data"), + + WithIgnoreField("metadata", "name"), + WithIgnoreField("metadata", "generateName"), + WithIgnoreField("metadata", "namespace"), + WithIgnoreField("metadata", "selfLink"), + WithIgnoreField("metadata", "uid"), + WithIgnoreField("metadata", "resourceVersion"), + WithIgnoreField("metadata", "generation"), + WithIgnoreField("metadata", "creationTimestamp"), + WithIgnoreField("metadata", "deletionTimestamp"), + WithIgnoreField("metadata", "deletionGracePeriodSeconds"), + WithIgnoreField("metadata", "finalizers"), + WithIgnoreField("metadata", "managedFields"), + + // Options for handling of status fields. + WithIgnoreConditionsLastTransitionTime(), + WithConditionsAsMap(), + )...) +} + +// WithIgnoreField adds a path to the list of paths to ignore when executing Diff. +func WithIgnoreField(path ...string) diffopts { + return func(d *differ) { + d.ignoredPath = append(d.ignoredPath, path) + } +} + +// WithIgnoreConditionsLastTransitionTime configures the differ to ignore LastTransitionTime for conditions when executing Diff. +func WithIgnoreConditionsLastTransitionTime() diffopts { + return func(d *differ) { + d.modifyFuncs["RemoveConditionsLastTransitionTime"] = func(a map[string]interface{}) error { + conditionPaths := [][]string{ + {"status", "conditions"}, + {"status", "v1beta2", "conditions"}, + {"status", "deprecated", "v1beta1", "conditions"}, + } + + for _, conditionPath := range conditionPaths { + conditions, found, err := unstructured.NestedSlice(a, conditionPath...) + if !found || err != nil { + continue + } + + for i, condition := range conditions { + conditionMap, ok := condition.(map[string]interface{}) + if !ok { + continue + } + + conditionMap["lastTransitionTime"] = "ignored" + conditions[i] = conditionMap + } + + if err := unstructured.SetNestedField(a, conditions, conditionPath...); err != nil { + return fmt.Errorf("failed to set nested field %s: %w", strings.Join(conditionPath, "."), err) + } + } + + return nil + } + } +} + +// WithConditionsAsMap ensures the conditions are converted to maps for comparison. +func WithConditionsAsMap() diffopts { + return func(d *differ) { + d.modifyFuncs["ConditionsAsMap"] = func(a map[string]interface{}) error { + conditionPaths := [][]string{ + {"status", "conditions"}, + {"status", "v1beta2", "conditions"}, + {"status", "deprecated", "v1beta1", "conditions"}, + } + + for _, conditionPath := range conditionPaths { + conditions, found, err := unstructured.NestedSlice(a, conditionPath...) + if !found || err != nil { + continue + } + + newConditions := map[string]interface{}{} + + for _, condition := range conditions { + conditionMap, ok := condition.(map[string]interface{}) + if !ok || conditionMap["type"] == nil { + continue + } + + conditionType, ok := conditionMap["type"].(string) + if !ok { + continue + } + + newConditions[fmt.Sprintf("type=%s", conditionType)] = condition + } + + if err := unstructured.SetNestedField(a, newConditions, conditionPath...); err != nil { + return fmt.Errorf("failed to set nested field %s: %w", strings.Join(conditionPath, "."), err) + } + } + + return nil + } + } +} + +// WithIgnoreConditionType conditionType configures the differ to ignore the condition of the given type when executing Diff. +func WithIgnoreConditionType(conditionType string) diffopts { + return func(d *differ) { + d.modifyFuncs[fmt.Sprintf("RemoveCondition[%s]", conditionType)] = func(a map[string]interface{}) error { + conditionPaths := [][]string{ + {"status", "conditions"}, + {"status", "v1beta2", "conditions"}, + {"status", "deprecated", "v1beta1", "conditions"}, + } + + for _, conditionPath := range conditionPaths { + conditions, found, err := unstructured.NestedSlice(a, conditionPath...) + if !found || err != nil { + continue + } + + newConditions := []interface{}{} + + for _, condition := range conditions { + conditionMap, ok := condition.(map[string]interface{}) + if !ok { + continue + } + + // Skip condition of the given type. + if conditionMap["type"] == conditionType { + continue + } + + newConditions = append(newConditions, condition) + } + + if err := unstructured.SetNestedField(a, newConditions, conditionPath...); err != nil { + return fmt.Errorf("failed to set nested field %s: %w", strings.Join(conditionPath, "."), err) + } + } + + return nil + } + } +} + +// WithProviderSpec configures the differ to separately diff .spec.providerSpec. +func WithProviderSpec(platform configv1.PlatformType, path []string, marshalProviderSpec func(platform configv1.PlatformType, rawExtension *runtime.RawExtension) (any, error)) diffopts { + return func(d *differ) { + d.providerSpecPath = strings.Join(path, ".") + + d.modifyFuncs["ProviderSpec"] = func(obj map[string]interface{}) error { + rawExtensionMap, found, err := unstructured.NestedMap(obj, path...) + if err != nil { + return fmt.Errorf("failed to get providerSpec value: %w", err) + } else if !found { + return fmt.Errorf("%w at path %s", errProviderSpecNotFound, strings.Join(path, ".")) + } + + rawExtension := &runtime.RawExtension{} + if err := runtime.DefaultUnstructuredConverter.FromUnstructured(rawExtensionMap, rawExtension); err != nil { + return fmt.Errorf("failed to convert providerSpec value map to raw extension: %w", err) + } + + providerSpec, err := marshalProviderSpec(platform, rawExtension) + if err != nil { + return fmt.Errorf("failed to marshal providerSpec: %w", err) + } + + // unset the nested field + unstructured.RemoveNestedField(obj, path...) + // add it as top-level field + obj[d.providerSpecPath] = providerSpec + + return nil + } + } +} + +type diffopts func(*differ) + +func newDiffer(opts ...diffopts) *differ { + d := &differ{ + modifyFuncs: map[string]func(obj map[string]interface{}) error{}, + } + for _, opt := range opts { + opt(d) + } + + return d +} diff --git a/pkg/util/diff_test.go b/pkg/util/diff_test.go new file mode 100644 index 000000000..9eed27b19 --- /dev/null +++ b/pkg/util/diff_test.go @@ -0,0 +1,323 @@ +/* +Copyright 2025 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package util + +import ( + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + mapiv1beta1 "github.com/openshift/api/machine/v1beta1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" +) + +var _ = Describe("Unit test Diff", func() { + + Describe("error cases", func() { + It("should return an error if the objects are not of the same type", func() { + a := &mapiv1beta1.MachineSet{} + b := &mapiv1beta1.Machine{} + _, err := NewDefaultDiffer().Diff(a, b) + Expect(err).To(HaveOccurred()) + Expect(err).To(MatchError(ContainSubstring("objects to diff are not of the same type"))) + }) + }) + + type testInput struct { + a unstructured.Unstructured + b unstructured.Unstructured + diffOpts []diffopts + wantChanged bool + want string + } + DescribeTable("basic operations", func(tt testInput) { + differ := newDiffer(tt.diffOpts...) + diff, err := differ.Diff(&tt.a, &tt.b) + Expect(err).ToNot(HaveOccurred()) + Expect(diff.HasChanges()).To(Equal(tt.wantChanged)) + Expect(diff.String()).To(Equal(tt.want)) + }, + Entry("no diff on empty objects", testInput{ + a: unstructured.Unstructured{ + Object: map[string]any{}, + }, + b: unstructured.Unstructured{ + Object: map[string]any{}, + }, + wantChanged: false, + want: "", + }), + Entry("no diff on matching objects", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "a": 1, + "b": 2, + "c": map[string]any{}, + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "a": 1, + "b": 2, + "c": map[string]any{}, + }}, + wantChanged: false, + want: "", + }), + Entry("diff when adding a field", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "a": 1, + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "a": 1, + "b": 2, + }}, + wantChanged: true, + want: ".[b]: != 2", + }), + Entry("diff when adding a field nested", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "foo": map[string]any{ + "a": 1, + "c": map[string]any{ + "d": 3, + }, + }, + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "foo": map[string]any{ + "a": 1, + "b": 2, + "c": map[string]any{ + "d": 4, + }, + }, + }}, + wantChanged: true, + want: ".[foo].[b]: != 2, .[foo].[c].[d]: 3 != 4", + }), + Entry("diff when removing a field", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "a": 1, + "b": 2, + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "a": 1, + }}, + wantChanged: true, + want: ".[b]: 2 != ", + }), + Entry("diff when changing a field", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "a": 1, + "b": 2, + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "a": 1, + "b": 3, + }}, + wantChanged: true, + want: ".[b]: 2 != 3", + }), + Entry("diff when adding a entry to a list", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "a": 1, + "b": []int{1, 2}, + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "a": 1, + "b": []int{1, 2, 3}, + }}, + wantChanged: true, + want: ".[b][2]: != 3", + }), + Entry("diff when removing a entry from a list", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "a": 1, + "b": []int{1, 2, 3}, + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "a": 1, + "b": []int{1, 2}, + }}, + wantChanged: true, + want: ".[b][2]: 3 != ", + }), + Entry("diff when changing a entry in a list", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "a": 1, + "b": []int{1, 2, 3}, + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "a": 1, + "b": []int{1, 2, 4}, + }}, + wantChanged: true, + want: ".[b][2]: 3 != 4", + }), + Entry("diff when deleting a list", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "a": 1, + "b": []int{1, 2, 3}, + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "a": 1, + }}, + wantChanged: true, + want: ".[b]: [1 2 3] != ", + }), + Entry("no diff on matching objects with ignore fields", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "someKey": "someValue", + "changed": 1, + "removed": 2, + "nil": map[string]any{}, + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "someKey": "someValue", + "changed": 2, + "nil": nil, + }}, + diffOpts: []diffopts{ + WithIgnoreField("changed"), + WithIgnoreField("removed"), + WithIgnoreField("nil"), + }, + wantChanged: false, + want: "", + }), + Entry("no diff on matching objects with ignore fields that does not exist", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "someKey": "someValue", + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "someKey": "someValue", + }}, + diffOpts: []diffopts{ + WithIgnoreField("doesnotexist"), + }, + wantChanged: false, + want: "", + }), + Entry("diff on not matching objects with ignore fields that do not exist or are properly ignored", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "someKey": "someValue", + "shouldbeignored": 1, + "someChangedKey": "a", + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "someKey": "someValue", + "shouldbeignored": 2, + "someChangedKey": "b", + }}, + diffOpts: []diffopts{ + WithIgnoreField("doesnotexist"), + WithIgnoreField("shouldbeignored"), + }, + wantChanged: true, + want: ".[someChangedKey]: a != b", + }), + Entry("no diff on matching objects with modifyFunc", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "someKey": "someValue", + "changed": 1, + "removed": 2, + "nil": map[string]any{}, + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "someKey": "someValue", + "changed": 2, + "nil": nil, + }}, + diffOpts: []diffopts{ + func(d *differ) { + d.modifyFuncs["test"] = func(obj map[string]interface{}) error { //nolint:unparam + obj["new"] = "new" + obj["changed"] = 3 + obj["removed"] = 3 + obj["nil"] = nil + + return nil + } + }, + }, + wantChanged: false, + want: "", + }), + Entry("no diff on objects having conditions in a different order", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "status": map[string]any{ + + "conditions": []any{ + map[string]any{ + "type": "Foo", + "status": "True", + }, + map[string]any{ + "type": "Bar", + "status": "True", + }, + }, + }, + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "status": map[string]any{ + "conditions": []any{ + map[string]any{ + "type": "Bar", + "status": "True", + }, + map[string]any{ + "type": "Foo", + "status": "True", + }, + }, + }, + }}, + diffOpts: []diffopts{ + WithConditionsAsMap(), + }, + wantChanged: false, + want: "", + }), + Entry("diff on objects having conditions in a different order and one is different", testInput{ + a: unstructured.Unstructured{Object: map[string]any{ + "status": map[string]any{"conditions": []any{ + map[string]any{ + "type": "Foo", + "status": "False", + }, + map[string]any{ + "type": "Bar", + "status": "True", + }, + }}, + }}, + b: unstructured.Unstructured{Object: map[string]any{ + "status": map[string]any{"conditions": []any{ + map[string]any{ + "type": "Bar", + "status": "True", + }, + map[string]any{ + "type": "Foo", + "status": "True", + }, + }}, + }}, + diffOpts: []diffopts{ + WithConditionsAsMap(), + }, + wantChanged: true, + want: ".[status].[conditions].[type=Foo].[status]: False != True", + }), + ) +}) diff --git a/pkg/util/suite_test.go b/pkg/util/suite_test.go new file mode 100644 index 000000000..a9f51f360 --- /dev/null +++ b/pkg/util/suite_test.go @@ -0,0 +1,28 @@ +/* +Copyright 2025 Red Hat, Inc. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +package util + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestUtil(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Util Suite") +} diff --git a/pkg/util/sync.go b/pkg/util/sync.go index 648c441e9..3804bf7c4 100644 --- a/pkg/util/sync.go +++ b/pkg/util/sync.go @@ -18,11 +18,9 @@ package util import ( "reflect" - "github.com/go-test/deep" mapiv1beta1 "github.com/openshift/api/machine/v1beta1" machinev1applyconfigs "github.com/openshift/client-go/machine/applyconfigurations/machine/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -53,298 +51,6 @@ func hasSameState(i *mapiv1beta1.Condition, j *machinev1applyconfigs.ConditionAp i.Status == *j.Status } -// ObjectMetaEqual compares variables a and b, -// and returns a list of differences, or nil if there are none, -// for the fields we care about when synchronising MAPI and CAPI Machines. -func ObjectMetaEqual(a, b metav1.ObjectMeta) map[string]any { - objectMetaDiff := map[string]any{} - - if diffLabels := deep.Equal(a.Labels, b.Labels); len(diffLabels) > 0 { - objectMetaDiff[".labels"] = diffLabels - } - - // Ignore conversion-data because this data is managed by Cluster API for down conversion. - aAnnotations := a.DeepCopy().GetAnnotations() - delete(aAnnotations, "cluster.x-k8s.io/conversion-data") - - bAnnotations := b.DeepCopy().GetAnnotations() - delete(bAnnotations, "cluster.x-k8s.io/conversion-data") - - if diffAnnotations := deep.Equal(aAnnotations, bAnnotations); len(diffAnnotations) > 0 { - objectMetaDiff[".annotations"] = diffAnnotations - } - - // Ignore the differences in finalizers, as CAPI always put finalizers on its resources - // even when its controllers are paused: - // https://github.com/kubernetes-sigs/cluster-api/blob/c70dca0fc387b44457c69b71a719132a0d9bed58/internal/controllers/machine/machine_controller.go#L207-L210 - - if diffOwnerReferences := deep.Equal(a.OwnerReferences, b.OwnerReferences); len(diffOwnerReferences) > 0 { - objectMetaDiff[".ownerReferences"] = diffOwnerReferences - } - - return objectMetaDiff -} - -// CAPIMachineSetStatusEqual compares variables a and b, -// and returns a list of differences, or nil if there are none, -// for the fields we care about when synchronising MAPI and CAPI Machines. -func CAPIMachineSetStatusEqual(a, b clusterv1.MachineSetStatus) map[string]any { - diff := map[string]any{} - - if diffReadyReplicas := deep.Equal(a.ReadyReplicas, b.ReadyReplicas); len(diffReadyReplicas) > 0 { - diff[".readyReplicas"] = diffReadyReplicas - } - - if diffAvailableReplicas := deep.Equal(a.AvailableReplicas, b.AvailableReplicas); len(diffAvailableReplicas) > 0 { - diff[".availableReplicas"] = diffAvailableReplicas - } - - if diffFullyLabeledReplicas := deep.Equal(a.FullyLabeledReplicas, b.FullyLabeledReplicas); len(diffFullyLabeledReplicas) > 0 { - diff[".fullyLabeledReplicas"] = diffFullyLabeledReplicas - } - - if diffFailureReason := deep.Equal(a.FailureReason, b.FailureReason); len(diffFailureReason) > 0 { - diff[".failureReason"] = diffFailureReason - } - - if diffFailureMessage := deep.Equal(a.FailureMessage, b.FailureMessage); len(diffFailureMessage) > 0 { - diff[".failureMessage"] = diffFailureMessage - } - - if diffConditions := compareCAPIV1Beta1Conditions(a.Conditions, b.Conditions); len(diffConditions) > 0 { - diff[".conditions"] = diffConditions - } - - // Compare the v1beta2 fields. - if a.V1Beta2 == nil { - a.V1Beta2 = &clusterv1.MachineSetV1Beta2Status{} - } - - if b.V1Beta2 == nil { - b.V1Beta2 = &clusterv1.MachineSetV1Beta2Status{} - } - - if diffUpToDateReplicas := deep.Equal(a.V1Beta2.UpToDateReplicas, b.V1Beta2.UpToDateReplicas); len(diffUpToDateReplicas) > 0 { - diff[".v1beta2.upToDateReplicas"] = diffUpToDateReplicas - } - - if diffAvailableReplicas := deep.Equal(a.V1Beta2.AvailableReplicas, b.V1Beta2.AvailableReplicas); len(diffAvailableReplicas) > 0 { - diff[".v1beta2.availableReplicas"] = diffAvailableReplicas - } - - if diffReadyReplicas := deep.Equal(a.V1Beta2.ReadyReplicas, b.V1Beta2.ReadyReplicas); len(diffReadyReplicas) > 0 { - diff[".v1beta2.readyReplicas"] = diffReadyReplicas - } - - if diffConditions := compareCAPIV1Beta2Conditions(a.V1Beta2.Conditions, b.V1Beta2.Conditions); len(diffConditions) > 0 { - diff[".v1beta2.conditions"] = diffConditions - } - - return diff -} - -// CAPIMachineStatusEqual compares variables a and b, -// and returns a list of differences, or nil if there are none, -// for the fields we care about when synchronising CAPI and MAPI Machines. -func CAPIMachineStatusEqual(a, b clusterv1.MachineStatus) map[string]any { - diff := map[string]any{} - - if diffFailureReason := deep.Equal(a.FailureReason, b.FailureReason); len(diffFailureReason) > 0 { - diff[".failureReason"] = diffFailureReason - } - - if diffFailureMessage := deep.Equal(a.FailureMessage, b.FailureMessage); len(diffFailureMessage) > 0 { - diff[".failureMessage"] = diffFailureMessage - } - - if diffLastUpdated := deep.Equal(a.LastUpdated, b.LastUpdated); len(diffLastUpdated) > 0 { - diff[".lastUpdated"] = diffLastUpdated - } - - if diffPhase := deep.Equal(a.Phase, b.Phase); len(diffPhase) > 0 { - diff[".phase"] = diffPhase - } - - if diffAddresses := deep.Equal(a.Addresses, b.Addresses); len(diffAddresses) > 0 { - diff[".addresses"] = diffAddresses - } - - if diffBootstrapReady := deep.Equal(a.BootstrapReady, b.BootstrapReady); len(diffBootstrapReady) > 0 { - diff[".bootstrapReady"] = diffBootstrapReady - } - - if diffInfrastructureReady := deep.Equal(a.InfrastructureReady, b.InfrastructureReady); len(diffInfrastructureReady) > 0 { - diff[".infrastructureReady"] = diffInfrastructureReady - } - - if diffNodeInfo := deep.Equal(a.NodeInfo, b.NodeInfo); len(diffNodeInfo) > 0 { - diff[".nodeInfo"] = diffNodeInfo - } - - if diffConditions := compareCAPIV1Beta1Conditions(a.Conditions, b.Conditions); len(diffConditions) > 0 { - diff[".conditions"] = diffConditions - } - - // Compare the v1beta2 fields. - if a.V1Beta2 == nil { - a.V1Beta2 = &clusterv1.MachineV1Beta2Status{} - } - - if b.V1Beta2 == nil { - b.V1Beta2 = &clusterv1.MachineV1Beta2Status{} - } - - if diffConditions := compareCAPIV1Beta2Conditions(a.V1Beta2.Conditions, b.V1Beta2.Conditions); len(diffConditions) > 0 { - diff[".v1beta2.conditions"] = diffConditions - } - - return diff -} - -// MAPIMachineStatusEqual compares variables a and b, -// and returns a list of differences, or nil if there are none, -// for the fields we care about when synchronising CAPI and MAPI Machines. -func MAPIMachineStatusEqual(a, b mapiv1beta1.MachineStatus) map[string]any { - diff := map[string]any{} - - if diffConditions := compareMAPIV1Beta1Conditions(a.Conditions, b.Conditions); len(diffConditions) > 0 { - diff[".conditions"] = diffConditions - } - - if diffErrorReason := deep.Equal(a.ErrorReason, b.ErrorReason); len(diffErrorReason) > 0 { - diff[".errorReason"] = diffErrorReason - } - - if diffErrorMessage := deep.Equal(a.ErrorMessage, b.ErrorMessage); len(diffErrorMessage) > 0 { - diff[".errorMessage"] = diffErrorMessage - } - - if diffPhase := deep.Equal(a.Phase, b.Phase); len(diffPhase) > 0 { - diff[".phase"] = diffPhase - } - - if diffAddresses := deep.Equal(a.Addresses, b.Addresses); len(diffAddresses) > 0 { - diff[".addresses"] = diffAddresses - } - - if diffNodeRef := deep.Equal(a.NodeRef, b.NodeRef); len(diffNodeRef) > 0 { - diff[".nodeRef"] = diffNodeRef - } - - if diffLastUpdated := deep.Equal(a.LastUpdated, b.LastUpdated); len(diffLastUpdated) > 0 { - diff[".lastUpdated"] = diffLastUpdated - } - - return diff -} - -// compareCAPIV1Beta1Conditions compares variables a and b, -// and returns a list of differences, or nil if there are none, -// for the fields we care about when synchronising CAPI v1beta1 and MAPI. -func compareCAPIV1Beta1Conditions(a, b []clusterv1.Condition) []string { - diff := []string{} - // Compare the conditions one by one. - // Ignore the differences in LastTransitionTime. - for _, condition := range a { - for _, conditionB := range b { - if condition.Type == conditionB.Type { - if condition.Status != conditionB.Status || - condition.Severity != conditionB.Severity || - condition.Reason != conditionB.Reason || - condition.Message != conditionB.Message { - diff = append(diff, deep.Equal(condition, conditionB)...) - } - - break // Break out of the inner loop once we have found a match. - } - } - } - - return diff -} - -// compareMAPIV1Beta1Conditions compares variables a and b, -// and returns a list of differences, or nil if there are none, -// for the fields we care about when synchronising MAPI v1beta1 and CAPI. -func compareMAPIV1Beta1Conditions(a, b []mapiv1beta1.Condition) []string { - diff := []string{} - // Compare the conditions one by one. - // Ignore the differences in LastTransitionTime. - for _, condition := range a { - for _, conditionB := range b { - if condition.Type == conditionB.Type { - if condition.Status != conditionB.Status || - condition.Severity != conditionB.Severity || - condition.Reason != conditionB.Reason || - condition.Message != conditionB.Message { - diff = append(diff, deep.Equal(condition, conditionB)...) - } - - break // Break out of the inner loop once we have found a match. - } - } - } - - return diff -} - -// compareCAPIV1Beta2Conditions compares variables a and b, -// and returns a list of differences, or nil if there are none, -// for the fields we care about when synchronising CAPI v1beta2 and MAPI. -func compareCAPIV1Beta2Conditions(a, b []metav1.Condition) []string { - diff := []string{} - // Compare the conditions one by one. - // Ignore the differences in LastTransitionTime. - for _, condition := range a { - for _, conditionB := range b { - if condition.Type == conditionB.Type { - if condition.Status != conditionB.Status || - condition.Reason != conditionB.Reason || - condition.Message != conditionB.Message { - diff = append(diff, deep.Equal(condition, conditionB)...) - } - - break // Break out of the inner loop once we have found a match. - } - } - } - - return diff -} - -// MAPIMachineSetStatusEqual compares variables a and b, -// and returns a list of differences, or nil if there are none, -// for the fields we care about when synchronising MAPI and CAPI Machines. -func MAPIMachineSetStatusEqual(a, b mapiv1beta1.MachineSetStatus) map[string]any { - diff := map[string]any{} - - if diffReadyReplicas := deep.Equal(a.ReadyReplicas, b.ReadyReplicas); len(diffReadyReplicas) > 0 { - diff[".readyReplicas"] = diffReadyReplicas - } - - if diffAvailableReplicas := deep.Equal(a.AvailableReplicas, b.AvailableReplicas); len(diffAvailableReplicas) > 0 { - diff[".availableReplicas"] = diffAvailableReplicas - } - - if diffFullyLabeledReplicas := deep.Equal(a.FullyLabeledReplicas, b.FullyLabeledReplicas); len(diffFullyLabeledReplicas) > 0 { - diff[".fullyLabeledReplicas"] = diffFullyLabeledReplicas - } - - if diffErrorReason := deep.Equal(a.ErrorReason, b.ErrorReason); len(diffErrorReason) > 0 { - diff[".errorReason"] = diffErrorReason - } - - if diffErrorMessage := deep.Equal(a.ErrorMessage, b.ErrorMessage); len(diffErrorMessage) > 0 { - diff[".errorMessage"] = diffErrorMessage - } - - if diffConditions := compareMAPIV1Beta1Conditions(a.Conditions, b.Conditions); len(diffConditions) > 0 { - diff[".conditions"] = diffConditions - } - - return diff -} - // GetResourceVersion returns the object ResourceVersion or the zero value for it. func GetResourceVersion(obj client.Object) string { if obj == nil || reflect.ValueOf(obj).IsNil() {