Repository: kubernetes-sigs/cluster-api-provider-azure (go)

Issue:
Machine deployment scale fails after v1alpha3 -> v1alpha4,v1beta1 upgrade
/kind bug

**What steps did you take and what happened:**
1. create v1alpha3 cluster with v1alpha3 providers
2. upgrade providers to v1beta1
3. scale md to 2
4. capi reconciliation fails with validation error in `AzureMachineTemplate`

**What did you expect to happen:**
machine deployment should scale to 2

**Anything else you would like to add:**
This is because we added new default webhooks in azure machine template which were not there in v1alpha3. When capi tries to update owner references for infrastructure reference (azuremachinetemplate), it applies the default webhooks, specifically, it sets `osdisk.CachingType = None`, but the default webhooks are not applied for the old spec. So, `validateUpdate` fails because we check for immutability. 


**Environment:**

- cluster-api-provider-azure version: 
- Kubernetes version: (use `kubectl version`): 
- OS (e.g. from `/etc/os-release`): 


Pull request:
cluster upgrade default fix for v1alpha3 -> v1alpha4
 <!-- If this is your first PR, welcome! Please make sure you read the [contributing guidelines](../CONTRIBUTING.md). -->

 <!-- Please label this pull request according to what type of issue you are addressing (see ../CONTRIBUTING.md) -->
**What type of PR is this?**
/kind bug
<!--
Add one of the following kinds:
/kind feature
/kind bug
/kind api-change
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind flake
/kind other
-->

**What this PR does / why we need it**:
Backports #1811 #1771 

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #1770

**Special notes for your reviewer**:

_Please confirm that if this PR changes any image versions, then that's the sole change this PR makes._

**TODOs**:
<!-- Put an "X" character inside the brackets of each completed task. Some may be optional depending on the PR. -->

- [ ] squashed commits
- [ ] includes documentation
- [x] adds unit tests

**Release note**:
<!--  Write your release note:
1. Enter your extended release note in the below block. If the PR requires additional action from users switching to the new release, include the string "action required".
2. If no release note is required, just write "NONE".
-->
```release-note
Fix default diff issue when upgrading clusters from v1alpha3 to v1alpha4

```


Patch:
diff --git a/api/v1alpha4/azurecluster_webhook.go b/api/v1alpha4/azurecluster_webhook.go
index 6af90486c..125c3b60a 100644
--- a/api/v1alpha4/azurecluster_webhook.go
+++ b/api/v1alpha4/azurecluster_webhook.go
@@ -85,10 +85,20 @@ func (c *AzureCluster) ValidateUpdate(oldRaw runtime.Object) error {
 	}
 
 	if !reflect.DeepEqual(c.Spec.AzureEnvironment, old.Spec.AzureEnvironment) {
-		allErrs = append(allErrs,
-			field.Invalid(field.NewPath("spec", "AzureEnvironment"),
-				c.Spec.AzureEnvironment, "field is immutable"),
-		)
+		// The equality failure could be because of default mismatch between v1alpha3 and v1alpha4. This happens because
+		// the new object `r` will have run through the default webhooks but the old object `old` would not have so.
+		// This means if the old object was in v1alpha3, it would not get the new defaults set in v1alpha4 resulting
+		// in object inequality. To workaround this, we set the v1alpha4 defaults here so that the old object also gets
+		// the new defaults.
+		old.setAzureEnvironmentDefault()
+
+		// if it's still not equal, return error.
+		if !reflect.DeepEqual(c.Spec.AzureEnvironment, old.Spec.AzureEnvironment) {
+			allErrs = append(allErrs,
+				field.Invalid(field.NewPath("spec", "AzureEnvironment"),
+					c.Spec.AzureEnvironment, "field is immutable"),
+			)
+		}
 	}
 
 	if !reflect.DeepEqual(c.Spec.NetworkSpec.PrivateDNSZoneName, old.Spec.NetworkSpec.PrivateDNSZoneName) {
diff --git a/api/v1alpha4/azuremachinetemplate_webhook.go b/api/v1alpha4/azuremachinetemplate_webhook.go
index 429a14c01..9534e6160 100644
--- a/api/v1alpha4/azuremachinetemplate_webhook.go
+++ b/api/v1alpha4/azuremachinetemplate_webhook.go
@@ -64,9 +64,25 @@ func (r *AzureMachineTemplate) ValidateUpdate(oldRaw runtime.Object) error {
 	old := oldRaw.(*AzureMachineTemplate)
 
 	if !reflect.DeepEqual(r.Spec.Template.Spec, old.Spec.Template.Spec) {
-		allErrs = append(allErrs,
-			field.Invalid(field.NewPath("AzureMachineTemplate", "spec", "template", "spec"), r, AzureMachineTemplateImmutableMsg),
-		)
+		// The equality failure could be because of default mismatch between v1alpha3 and v1alpha4. This happens because
+		// the new object `r` will have run through the default webhooks but the old object `old` would not have so.
+		// This means if the old object was in v1alpha3, it would not get the new defaults set in v1alpha4 resulting
+		// in object inequality. To workaround this, we set the v1alpha4 defaults here so that the old object also gets
+		// the new defaults.
+
+		// We need to set ssh key explicitly, otherwise Default() will create a new one.
+		if old.Spec.Template.Spec.SSHPublicKey == "" {
+			old.Spec.Template.Spec.SSHPublicKey = r.Spec.Template.Spec.SSHPublicKey
+		}
+
+		old.Default()
+
+		// if it's still not equal, return error.
+		if !reflect.DeepEqual(r.Spec.Template.Spec, old.Spec.Template.Spec) {
+			allErrs = append(allErrs,
+				field.Invalid(field.NewPath("AzureMachineTemplate", "spec", "template", "spec"), r, AzureMachineTemplateImmutableMsg),
+			)
+		}
 	}
 
 	if len(allErrs) == 0 {


Test patch:
diff --git a/api/v1alpha4/azurecluster_webhook_test.go b/api/v1alpha4/azurecluster_webhook_test.go
index da7875212..bb2b0862c 100644
--- a/api/v1alpha4/azurecluster_webhook_test.go
+++ b/api/v1alpha4/azurecluster_webhook_test.go
@@ -107,6 +107,9 @@ func TestAzureCluster_ValidateUpdate(t *testing.T) {
 	}{
 		{
 			name: "azurecluster with pre-existing vnet - valid spec",
+			oldCluster: func() *AzureCluster {
+				return createValidCluster()
+			}(),
 			cluster: func() *AzureCluster {
 				return createValidCluster()
 			}(),
@@ -114,6 +117,11 @@ func TestAzureCluster_ValidateUpdate(t *testing.T) {
 		},
 		{
 			name: "azurecluster without pre-existing vnet - valid spec",
+			oldCluster: func() *AzureCluster {
+				cluster := createValidCluster()
+				cluster.Spec.NetworkSpec.Vnet.ResourceGroup = ""
+				return cluster
+			}(),
 			cluster: func() *AzureCluster {
 				cluster := createValidCluster()
 				cluster.Spec.NetworkSpec.Vnet.ResourceGroup = ""
@@ -123,6 +131,9 @@ func TestAzureCluster_ValidateUpdate(t *testing.T) {
 		},
 		{
 			name: "azurecluster with pre-existing vnet - lack control plane subnet",
+			oldCluster: func() *AzureCluster {
+				return createValidCluster()
+			}(),
 			cluster: func() *AzureCluster {
 				cluster := createValidCluster()
 				cluster.Spec.NetworkSpec.Subnets = cluster.Spec.NetworkSpec.Subnets[1:]
@@ -132,6 +143,9 @@ func TestAzureCluster_ValidateUpdate(t *testing.T) {
 		},
 		{
 			name: "azurecluster with pre-existing vnet - lack node subnet",
+			oldCluster: func() *AzureCluster {
+				return createValidCluster()
+			}(),
 			cluster: func() *AzureCluster {
 				cluster := createValidCluster()
 				cluster.Spec.NetworkSpec.Subnets = cluster.Spec.NetworkSpec.Subnets[:1]
@@ -141,6 +155,9 @@ func TestAzureCluster_ValidateUpdate(t *testing.T) {
 		},
 		{
 			name: "azurecluster with pre-existing vnet - invalid resourcegroup name",
+			oldCluster: func() *AzureCluster {
+				return createValidCluster()
+			}(),
 			cluster: func() *AzureCluster {
 				cluster := createValidCluster()
 				cluster.Spec.NetworkSpec.Vnet.ResourceGroup = "invalid-name###"
@@ -150,6 +167,9 @@ func TestAzureCluster_ValidateUpdate(t *testing.T) {
 		},
 		{
 			name: "azurecluster with pre-existing vnet - invalid subnet name",
+			oldCluster: func() *AzureCluster {
+				return createValidCluster()
+			}(),
 			cluster: func() *AzureCluster {
 				cluster := createValidCluster()
 				cluster.Spec.NetworkSpec.Subnets = append(cluster.Spec.NetworkSpec.Subnets,
@@ -215,18 +235,14 @@ func TestAzureCluster_ValidateUpdate(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "azurecluster azureEnvironment is immutable",
-			oldCluster: &AzureCluster{
-				Spec: AzureClusterSpec{
-					AzureEnvironment: "AzureGermanCloud",
-				},
-			},
-			cluster: &AzureCluster{
-				Spec: AzureClusterSpec{
-					AzureEnvironment: "AzureChinaCloud",
-				},
-			},
-			wantErr: true,
+			name:       "azurecluster azureEnvironment default mismatch",
+			oldCluster: createValidCluster(),
+			cluster: func() *AzureCluster {
+				cluster := createValidCluster()
+				cluster.Spec.AzureEnvironment = "AzurePublicCloud"
+				return cluster
+			}(),
+			wantErr: false,
 		},
 		{
 			name: "control plane outbound lb is immutable",
@@ -248,6 +264,7 @@ func TestAzureCluster_ValidateUpdate(t *testing.T) {
 		},
 	}
 	for _, tc := range tests {
+		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
 			err := tc.cluster.ValidateUpdate(tc.oldCluster)
diff --git a/api/v1alpha4/azuremachinetemplate_webhook_test.go b/api/v1alpha4/azuremachinetemplate_webhook_test.go
index 9219c6072..92b0ec8a1 100644
--- a/api/v1alpha4/azuremachinetemplate_webhook_test.go
+++ b/api/v1alpha4/azuremachinetemplate_webhook_test.go
@@ -177,7 +177,7 @@ func TestAzureMachineTemplate_ValidateUpdate(t *testing.T) {
 								DiskSizeGB: to.Int32Ptr(11),
 							},
 							DataDisks:    []DataDisk{},
-							SSHPublicKey: "",
+							SSHPublicKey: "fake ssh key",
 						},
 					},
 				},
@@ -197,7 +197,7 @@ func TestAzureMachineTemplate_ValidateUpdate(t *testing.T) {
 								DiskSizeGB: to.Int32Ptr(11),
 							},
 							DataDisks:    []DataDisk{},
-							SSHPublicKey: "",
+							SSHPublicKey: "fake ssh key",
 						},
 					},
 				},
@@ -216,16 +216,104 @@ func TestAzureMachineTemplate_ValidateUpdate(t *testing.T) {
 								DiskSizeGB: to.Int32Ptr(11),
 							},
 							DataDisks:    []DataDisk{},
+							SSHPublicKey: "fake ssh key",
+						},
+					},
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "NewTemplate",
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "AzureMachineTemplate with default mismatch",
+			oldTemplate: &AzureMachineTemplate{
+				Spec: AzureMachineTemplateSpec{
+					Template: AzureMachineTemplateResource{
+						Spec: AzureMachineSpec{
+							VMSize:        "size",
+							FailureDomain: &failureDomain,
+							OSDisk: OSDisk{
+								OSType:      "type",
+								DiskSizeGB:  to.Int32Ptr(11),
+								CachingType: "",
+							},
+							DataDisks:    []DataDisk{},
 							SSHPublicKey: "",
 						},
 					},
 				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "OldTemplate",
+				},
+			},
+			template: &AzureMachineTemplate{
+				Spec: AzureMachineTemplateSpec{
+					Template: AzureMachineTemplateResource{
+						Spec: AzureMachineSpec{
+							VMSize:        "size",
+							FailureDomain: &failureDomain,
+							OSDisk: OSDisk{
+								OSType:      "type",
+								DiskSizeGB:  to.Int32Ptr(11),
+								CachingType: "None",
+							},
+							DataDisks:    []DataDisk{},
+							SSHPublicKey: "fake ssh key",
+						},
+					},
+				},
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "NewTemplate",
 				},
 			},
 			wantErr: false,
 		},
+		{
+			name: "AzureMachineTemplate ssh key removed",
+			oldTemplate: &AzureMachineTemplate{
+				Spec: AzureMachineTemplateSpec{
+					Template: AzureMachineTemplateResource{
+						Spec: AzureMachineSpec{
+							VMSize:        "size",
+							FailureDomain: &failureDomain,
+							OSDisk: OSDisk{
+								OSType:      "type",
+								DiskSizeGB:  to.Int32Ptr(11),
+								CachingType: "None",
+							},
+							DataDisks:    []DataDisk{},
+							SSHPublicKey: "some key",
+						},
+					},
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "OldTemplate",
+				},
+			},
+			template: &AzureMachineTemplate{
+				Spec: AzureMachineTemplateSpec{
+					Template: AzureMachineTemplateResource{
+						Spec: AzureMachineSpec{
+							VMSize:        "size",
+							FailureDomain: &failureDomain,
+							OSDisk: OSDisk{
+								OSType:      "type",
+								DiskSizeGB:  to.Int32Ptr(11),
+								CachingType: "None",
+							},
+							DataDisks:    []DataDisk{},
+							SSHPublicKey: "",
+						},
+					},
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "NewTemplate",
+				},
+			},
+			wantErr: true,
+		},
 	}
 
 	for _, amt := range tests {