Address comments

internal/controllers/machinedeployment/mdutil/util.go

@@ -39,7 +39,6 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
-	"sigs.k8s.io/cluster-api/internal/util/compare"
 	"sigs.k8s.io/cluster-api/util/conversion"
 )
 
@@ -374,38 +373,46 @@ func getMachineSetFraction(ms clusterv1.MachineSet, md clusterv1.MachineDeployme
 
 // MachineTemplateUpToDate returns true if the current MachineTemplateSpec is up-to-date with a corresponding desired MachineTemplateSpec.
 // Note: The comparison does not consider any in-place propagated fields, as well as the version from external references.
-func MachineTemplateUpToDate(current, desired *clusterv1.MachineTemplateSpec) (upToDate bool, diff string, conditionMessages []string, err error) {
+func MachineTemplateUpToDate(current, desired *clusterv1.MachineTemplateSpec) (upToDate bool, logMessages, conditionMessages []string) {
 	currentCopy := MachineTemplateDeepCopyRolloutFields(current)
 	desiredCopy := MachineTemplateDeepCopyRolloutFields(desired)
 
 	if !reflect.DeepEqual(currentCopy.Spec.Version, desiredCopy.Spec.Version) {
+		logMessages = append(logMessages, fmt.Sprintf("spec.version %s, %s required", ptr.Deref(currentCopy.Spec.Version, "nil"), ptr.Deref(desiredCopy.Spec.Version, "nil")))
 		conditionMessages = append(conditionMessages, fmt.Sprintf("Version %s, %s required", ptr.Deref(currentCopy.Spec.Version, "nil"), ptr.Deref(desiredCopy.Spec.Version, "nil")))
 	}
 
+	// Note: we return a message based on desired.bootstrap.ConfigRef != nil, but we always compare the entire bootstrap
+	// struct to catch cases when either configRef or dataSecretName is set in current vs desired (usually MachineTemplates
+	// have ConfigRef != nil, might be in some edge case dataSecret are used, but switching from one to another is not a
+	// common operation so it is acceptable to handle it in this way).
 	if current.Spec.Bootstrap.ConfigRef != nil {
-		if !reflect.DeepEqual(currentCopy.Spec.Bootstrap.ConfigRef, desiredCopy.Spec.Bootstrap.ConfigRef) {
+		if !reflect.DeepEqual(currentCopy.Spec.Bootstrap, desiredCopy.Spec.Bootstrap) {
+			logMessages = append(logMessages, fmt.Sprintf("spec.bootstrap.configRef %s/%s, %s/%s required", currentCopy.Spec.Bootstrap.ConfigRef.Kind, currentCopy.Spec.Bootstrap.ConfigRef.Name, ptr.Deref(desiredCopy.Spec.Bootstrap.ConfigRef, corev1.ObjectReference{}).Kind, ptr.Deref(desiredCopy.Spec.Bootstrap.ConfigRef, corev1.ObjectReference{}).Name))
 			conditionMessages = append(conditionMessages, fmt.Sprintf("%s is not up-to-date", current.Spec.Bootstrap.ConfigRef.Kind))
 		}
 	} else {
-		if !reflect.DeepEqual(currentCopy.Spec.Bootstrap.DataSecretName, desiredCopy.Spec.Bootstrap.DataSecretName) {
-			conditionMessages = append(conditionMessages, "spec.bootstrap.dataSecretName is not up to date")
+		if !reflect.DeepEqual(currentCopy.Spec.Bootstrap, desiredCopy.Spec.Bootstrap) {
+			logMessages = append(logMessages, fmt.Sprintf("spec.bootstrap.dataSecretName %s, %s required", ptr.Deref(currentCopy.Spec.Bootstrap.DataSecretName, "nil"), ptr.Deref(desiredCopy.Spec.Bootstrap.DataSecretName, "nil")))
+			conditionMessages = append(conditionMessages, fmt.Sprintf("spec.bootstrap.dataSecretName %s, %s required", ptr.Deref(currentCopy.Spec.Bootstrap.DataSecretName, "nil"), ptr.Deref(desiredCopy.Spec.Bootstrap.DataSecretName, "nil")))
 		}
 	}
 
 	if !reflect.DeepEqual(currentCopy.Spec.InfrastructureRef, desiredCopy.Spec.InfrastructureRef) {
+		logMessages = append(logMessages, fmt.Sprintf("spec.infrastructureRef %s/%s, %s/%s required", currentCopy.Spec.InfrastructureRef.Kind, currentCopy.Spec.InfrastructureRef.Name, desiredCopy.Spec.InfrastructureRef.Kind, desiredCopy.Spec.InfrastructureRef.Name))
 		conditionMessages = append(conditionMessages, fmt.Sprintf("%s is not up-to-date", currentCopy.Spec.InfrastructureRef.Kind))
 	}
 
 	if !reflect.DeepEqual(currentCopy.Spec.FailureDomain, desiredCopy.Spec.FailureDomain) {
+		logMessages = append(logMessages, fmt.Sprintf("spec.failureDomain %s, %s required", ptr.Deref(currentCopy.Spec.FailureDomain, "nil"), ptr.Deref(desiredCopy.Spec.FailureDomain, "nil")))
 		conditionMessages = append(conditionMessages, fmt.Sprintf("Failure domain %s, %s required", ptr.Deref(currentCopy.Spec.FailureDomain, "nil"), ptr.Deref(desiredCopy.Spec.FailureDomain, "nil")))
 	}
 
 	if len(conditionMessages) > 0 {
-		_, diff, err = compare.Diff(currentCopy, desiredCopy)
-		return false, diff, conditionMessages, err
+		return false, logMessages, conditionMessages
 	}
 
-	return true, "", nil, nil
+	return true, nil, nil
 }
 
 // MachineTemplateDeepCopyRolloutFields copies a MachineTemplateSpec
@@ -414,6 +421,9 @@ func MachineTemplateUpToDate(current, desired *clusterv1.MachineTemplateSpec) (u
 func MachineTemplateDeepCopyRolloutFields(template *clusterv1.MachineTemplateSpec) *clusterv1.MachineTemplateSpec {
 	templateCopy := template.DeepCopy()
 
+	// Moving MD from one cluster to another is not supported.
+	templateCopy.Spec.ClusterName = ""
+
 	// Drop labels and annotations
 	templateCopy.Labels = nil
 	templateCopy.Annotations = nil
@@ -460,19 +470,16 @@ func FindNewMachineSet(deployment *clusterv1.MachineDeployment, msList []*cluste
 	var matchingMachineSets []*clusterv1.MachineSet
 	var diffs []string
 	for _, ms := range msList {
-		upToDate, diff, _, err := MachineTemplateUpToDate(&ms.Spec.Template, &deployment.Spec.Template)
-		if err != nil {
-			return nil, "", errors.Wrapf(err, "failed to compare MachineDeployment spec template with MachineSet %s", ms.Name)
-		}
+		upToDate, logMessages, _ := MachineTemplateUpToDate(&ms.Spec.Template, &deployment.Spec.Template)
 		if upToDate {
 			matchingMachineSets = append(matchingMachineSets, ms)
 		} else {
-			diffs = append(diffs, fmt.Sprintf("MachineSet %s: diff: %s", ms.Name, diff))
+			diffs = append(diffs, fmt.Sprintf("MachineSet %s: diff: %s", ms.Name, strings.Join(logMessages, ", ")))
 		}
 	}
 
 	if len(matchingMachineSets) == 0 {
-		return nil, fmt.Sprintf("couldn't find MachineSet matching MachineDeployment spec template: %s", strings.Join(diffs, ",")), nil
+		return nil, fmt.Sprintf("couldn't find MachineSet matching MachineDeployment spec template: %s", strings.Join(diffs, "; ")), nil
 	}
 
 	// If RolloutAfter is not set, pick the first matching MachineSet.