Repository: Azure/ARO-RP
Pull request #2167 (2022-06-09 18:41:14)

Fix azureproviderspec regression
### Which issue this PR addresses:

Fixes https://msazure.visualstudio.com/AzureRedHatOpenShift/_workitems/edit/14682300

### What this PR does / why we need it:

Fix on a regression for azureproviderspec not utilizing the new way to decode/marshal the contents.  

### Test plan for issue:

Ran PUCM on 4.9 cluster with older version.  

### Is there any documentation that needs to be updated for this PR?

No


bennerv: /azp run e2e
azure-pipelines[bot]: <samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>
bennerv: /azp run e2e
azure-pipelines[bot]: <samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>

Patch:
diff --git a/hack/validate-imports/allowed-import-names.yaml b/hack/validate-imports/allowed-import-names.yaml
index 12e6b51b0..a637e9f6e 100644
--- a/hack/validate-imports/allowed-import-names.yaml
+++ b/hack/validate-imports/allowed-import-names.yaml
@@ -22,6 +22,9 @@ allowedImportNames:
     - utildiscovery
   github.com/Azure/ARO-RP/pkg/util/embed:
     - utilembed
+  github.com/Azure/ARO-RP/pkg/util/machine:
+    - utilmachine
+    - ""
   github.com/Azure/ARO-RP/pkg/util/namespace:
     - utilnamespace
     - ""
diff --git a/pkg/cluster/fixmcsuserdata.go b/pkg/cluster/fixmcsuserdata.go
index fccce111d..80e7641d7 100644
--- a/pkg/cluster/fixmcsuserdata.go
+++ b/pkg/cluster/fixmcsuserdata.go
@@ -5,7 +5,6 @@ package cluster
 
 import (
 	"context"
-	"fmt"
 	"net"
 	"net/url"
 	"strings"
@@ -14,10 +13,10 @@ import (
 	"github.com/ugorji/go/codec"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/util/retry"
 
 	"github.com/Azure/ARO-RP/pkg/api"
+	utilmachine "github.com/Azure/ARO-RP/pkg/util/machine"
 	_ "github.com/Azure/ARO-RP/pkg/util/scheme"
 )
 
@@ -82,16 +81,11 @@ func getUserDataSecretReference(objMeta *metav1.ObjectMeta, spec *machinev1beta1
 		return nil, nil
 	}
 
-	o, _, err := scheme.Codecs.UniversalDeserializer().Decode(spec.ProviderSpec.Value.Raw, nil, nil)
+	machineProviderSpec, err := utilmachine.UnmarshalAzureProviderSpec(spec.Name, utilmachine.Machine, spec.ProviderSpec.Value.Raw)
 	if err != nil {
 		return nil, err
 	}
 
-	machineProviderSpec, ok := o.(*machinev1beta1.AzureMachineProviderSpec)
-	if !ok {
-		return nil, fmt.Errorf("failed to read provider spec: %T", o)
-	}
-
 	if machineProviderSpec.UserDataSecret == nil {
 		return nil, nil
 	}
diff --git a/pkg/operator/controllers/machine/machine.go b/pkg/operator/controllers/machine/machine.go
index 5e26890e3..901c03b01 100644
--- a/pkg/operator/controllers/machine/machine.go
+++ b/pkg/operator/controllers/machine/machine.go
@@ -5,16 +5,14 @@ package machine
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
-	"strings"
 
 	machinev1beta1 "github.com/openshift/api/machine/v1beta1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/kubernetes/scheme"
 
 	"github.com/Azure/ARO-RP/pkg/api"
 	"github.com/Azure/ARO-RP/pkg/api/validate"
+	utilmachine "github.com/Azure/ARO-RP/pkg/util/machine"
 	_ "github.com/Azure/ARO-RP/pkg/util/scheme"
 )
 
@@ -39,27 +37,9 @@ func (r *Reconciler) machineValid(ctx context.Context, machine *machinev1beta1.M
 		return []error{fmt.Errorf("machine %s: provider spec missing", machine.Name)}
 	}
 
-	var machineProviderSpec *machinev1beta1.AzureMachineProviderSpec
-
-	if strings.Contains(string(machine.Spec.ProviderSpec.Value.Raw), "azureproviderconfig.openshift.io") {
-		machineProviderSpec = &machinev1beta1.AzureMachineProviderSpec{}
-		err := json.Unmarshal(machine.Spec.ProviderSpec.Value.Raw, machineProviderSpec)
-		if err != nil {
-			return []error{fmt.Errorf("machine %s: failed to unmarshal the 'azureproviderconfig.openshift.io' provider spec: %q", machine.Name, err.Error())}
-		}
-	} else {
-		o, _, err := scheme.Codecs.UniversalDeserializer().Decode(machine.Spec.ProviderSpec.Value.Raw, nil, nil)
-		if err != nil {
-			return []error{err}
-		}
-
-		var ok bool
-		machineProviderSpec, ok = o.(*machinev1beta1.AzureMachineProviderSpec)
-		if !ok {
-			// This should never happen: codecs uses scheme that has only one registered type
-			// and if something is wrong with the provider spec - decoding should fail
-			return []error{fmt.Errorf("machine %s: failed to read provider spec: %T", machine.Name, o)}
-		}
+	machineProviderSpec, err := utilmachine.UnmarshalAzureProviderSpec(machine.Name, utilmachine.Machine, machine.Spec.ProviderSpec.Value.Raw)
+	if err != nil {
+		errs = append(errs, err)
 	}
 
 	// Validate VM size in machine provider spec
diff --git a/pkg/util/clusterdata/worker_profiles_task.go b/pkg/util/clusterdata/worker_profiles_task.go
index 1dc15bd6f..549c7d353 100644
--- a/pkg/util/clusterdata/worker_profiles_task.go
+++ b/pkg/util/clusterdata/worker_profiles_task.go
@@ -9,14 +9,13 @@ import (
 	"sort"
 
 	"github.com/Azure/go-autorest/autorest/azure"
-	machinev1beta1 "github.com/openshift/api/machine/v1beta1"
 	machineclient "github.com/openshift/client-go/machine/clientset/versioned"
 	"github.com/sirupsen/logrus"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
 
 	"github.com/Azure/ARO-RP/pkg/api"
+	utilmachine "github.com/Azure/ARO-RP/pkg/util/machine"
 	_ "github.com/Azure/ARO-RP/pkg/util/scheme"
 )
 
@@ -75,17 +74,12 @@ func (ef *workerProfilesEnricherTask) FetchData(ctx context.Context, callbacks c
 			continue
 		}
 
-		o, _, err := scheme.Codecs.UniversalDeserializer().Decode(machineset.Spec.Template.Spec.ProviderSpec.Value.Raw, nil, nil)
-		if err != nil {
-			ef.log.Info(err)
-			continue
-		}
+		machineProviderSpec, err := utilmachine.UnmarshalAzureProviderSpec(machineset.Name, utilmachine.MachineSet, machineset.Spec.Template.Spec.ProviderSpec.Value.Raw)
 
-		machineProviderSpec, ok := o.(*machinev1beta1.AzureMachineProviderSpec)
-		if !ok {
-			// This should never happen: codecs uses scheme that has only one registered type
-			// and if something is wrong with the provider spec - decoding should fail
-			ef.log.Infof("failed to read provider spec from the machine set %q: %T", machineset.Name, o)
+		if err != nil {
+			// If this happens, the azure machine provider spec type/apiversion may have been updated and
+			// we need to handle it appropriately
+			ef.log.Info(err.Error())
 			continue
 		}
 
diff --git a/pkg/util/machine/azureproviderspec.go b/pkg/util/machine/azureproviderspec.go
new file mode 100644
index 000000000..50064aac8
--- /dev/null
+++ b/pkg/util/machine/azureproviderspec.go
@@ -0,0 +1,51 @@
+package machine
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"encoding/json"
+	"fmt"
+	"strings"
+
+	machinev1beta1 "github.com/openshift/api/machine/v1beta1"
+	"k8s.io/client-go/kubernetes/scheme"
+
+	_ "github.com/Azure/ARO-RP/pkg/util/scheme"
+)
+
+type MachineType string
+
+const (
+	MachineSet        MachineType = "machineset"
+	Machine           MachineType = "machine"
+	azureProviderSpec             = "azureproviderconfig.openshift.io"
+)
+
+// UnmarshalAzureProviderSpec unmarshals an Azure Provider Spec set on machines or machinesets
+// it contains backward compatibility for the older azureproviderconfig which is no longer in use
+// in OCP 4.10+
+func UnmarshalAzureProviderSpec(name string, mType MachineType, rawProviderSpec []byte) (*machinev1beta1.AzureMachineProviderSpec, error) {
+	if strings.Contains(string(rawProviderSpec), azureProviderSpec) {
+		machineProviderSpec := &machinev1beta1.AzureMachineProviderSpec{}
+		err := json.Unmarshal(rawProviderSpec, machineProviderSpec)
+		if err != nil {
+			return nil, fmt.Errorf("%s %s: failed to unmarshal the %q provider spec: %q", mType, name, azureProviderSpec, err.Error())
+		}
+		return machineProviderSpec, nil
+	}
+
+	o, _, err := scheme.Codecs.UniversalDeserializer().Decode(rawProviderSpec, nil, nil)
+	if err != nil {
+		return nil, err
+	}
+
+	machineProviderSpec, ok := o.(*machinev1beta1.AzureMachineProviderSpec)
+	if !ok {
+		// If this happens, the azure machine provider spec type/apiversion may have been updated and
+		// we need to handle it appropriately
+		return nil, fmt.Errorf("%s %s: failed to read provider spec: %T", mType, name, o)
+	}
+
+	return machineProviderSpec, nil
+}


Test patch:
diff --git a/pkg/util/machine/azureproviderspec_test.go b/pkg/util/machine/azureproviderspec_test.go
new file mode 100644
index 000000000..3a6bbeddd
--- /dev/null
+++ b/pkg/util/machine/azureproviderspec_test.go
@@ -0,0 +1,154 @@
+package machine
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"bytes"
+	"fmt"
+	"reflect"
+	"strings"
+	"testing"
+
+	"github.com/Azure/go-autorest/autorest/to"
+	"github.com/davecgh/go-spew/spew"
+	machinev1beta1 "github.com/openshift/api/machine/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	kruntime "k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	kjson "k8s.io/apimachinery/pkg/runtime/serializer/json"
+	"k8s.io/client-go/kubernetes/scheme"
+	azureproviderv1beta1 "sigs.k8s.io/cluster-api-provider-azure/pkg/apis/azureprovider/v1beta1"
+)
+
+type testData struct {
+	name         string
+	machineType  MachineType
+	providerSpec []byte
+	wantErr      string
+}
+
+var machineAzureProviderSpec = &machinev1beta1.AzureMachineProviderSpec{
+	Image: machinev1beta1.Image{
+		Publisher: "azureopenshift",
+		Offer:     "aro4",
+		SKU:       "aro_499",
+		Version:   "499.99.30000610",
+	},
+	Location:             "neversayneverland",
+	NetworkResourceGroup: "networkRG",
+	OSDisk: machinev1beta1.OSDisk{
+		DiskSizeGB: int32(9001),
+		OSType:     "Linux",
+		ManagedDisk: machinev1beta1.ManagedDiskParameters{
+			StorageAccountType: "Premium_LRS",
+		},
+	},
+	PublicIP:           false,
+	PublicLoadBalancer: "",
+	ResourceGroup:      "myRG",
+	VMSize:             "Standard_D8s_v3",
+	Vnet:               "myVnet",
+	Zone:               to.StringPtr("2"),
+}
+
+var azAzureProviderSpec = &azureproviderv1beta1.AzureMachineProviderSpec{
+	Image: azureproviderv1beta1.Image{
+		Publisher: "azureopenshift",
+		Offer:     "aro4",
+		SKU:       "aro_499",
+		Version:   "499.99.30000610",
+	},
+	Location:             "neversayneverland",
+	NetworkResourceGroup: "networkRG",
+	OSDisk: azureproviderv1beta1.OSDisk{
+		DiskSizeGB: int32(9001),
+		OSType:     "Linux",
+		ManagedDisk: azureproviderv1beta1.ManagedDiskParameters{
+			StorageAccountType: "Premium_LRS",
+		},
+	},
+	PublicIP:           false,
+	PublicLoadBalancer: "",
+	ResourceGroup:      "myRG",
+	VMSize:             "Standard_D8s_v3",
+	Vnet:               "myVnet",
+	Zone:               to.StringPtr("2"),
+}
+
+func marshalAzureMachineProviderSpec(t *testing.T, spec kruntime.Object) []byte {
+	serializer := kjson.NewSerializerWithOptions(
+		kjson.DefaultMetaFactory, scheme.Scheme, scheme.Scheme,
+		kjson.SerializerOptions{Yaml: false},
+	)
+
+	json := scheme.Codecs.CodecForVersions(serializer, nil, schema.GroupVersions(scheme.Scheme.PrioritizedVersionsAllGroups()), nil)
+
+	buf := &bytes.Buffer{}
+	err := json.Encode(spec, buf)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	return buf.Bytes()
+}
+
+func TestUnmarshalAzureProviderSpec(t *testing.T) {
+	machineName := "myMachine"
+
+	// Register old type so we can encode the CR to bytes for use only in test
+	scheme.Scheme.AddKnownTypes(azureproviderv1beta1.SchemeGroupVersion, &azureproviderv1beta1.AzureMachineProviderSpec{})
+
+	machineProviderSpecBytes := marshalAzureMachineProviderSpec(t, machineAzureProviderSpec)
+	azProviderSpecBytes := marshalAzureMachineProviderSpec(t, azAzureProviderSpec)
+
+	for _, tt := range []testData{
+		{
+			name:         "valid - machine.openshift.io provider spec",
+			providerSpec: machineProviderSpecBytes,
+		},
+		{
+			name:         "valid - azureproviderconfig.openshift.io provider spec",
+			providerSpec: azProviderSpecBytes,
+		},
+		{
+			name:         "fail - azureproviderconfig.openshift.io invalid json",
+			providerSpec: []byte("azureproviderconfig.openshift.io"),
+			machineType:  MachineSet,
+			wantErr:      fmt.Sprintf("%s %s: failed to unmarshal the \"azureproviderconfig.openshift.io\" provider spec: %q", MachineSet, machineName, "invalid character 'a' looking for beginning of value"),
+		},
+		{
+			name:         "fail - unable to decode type",
+			providerSpec: []byte(`{"apiVersion": "invalid/v1beta1","kind": "AzureMachineProviderSpec"}`),
+			machineType:  MachineSet,
+			wantErr:      `no kind "AzureMachineProviderSpec" is registered for version "invalid/v1beta1" in scheme`,
+		},
+		{
+			name:         "fail - invalid type",
+			providerSpec: []byte(`{"apiVersion": "v1","kind": "Node"}`),
+			machineType:  MachineSet,
+			wantErr:      `failed to read provider spec:`,
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			actualProviderSpec, err := UnmarshalAzureProviderSpec(machineName, tt.machineType, tt.providerSpec)
+			if err == nil {
+				if tt.wantErr != "" {
+					t.Error(err)
+				}
+
+				// Ignoring check to support both azureproviderconfig and machine providerSpec APIVersions
+				actualProviderSpec.TypeMeta = metav1.TypeMeta{}
+
+				if !reflect.DeepEqual(actualProviderSpec, machineAzureProviderSpec) {
+					t.Errorf("got '%v' wanted '%v'", spew.Sdump(actualProviderSpec), spew.Sdump(machineAzureProviderSpec))
+				}
+			} else {
+				// Use contains as one test case contains line numbers which may change
+				if !strings.Contains(err.Error(), tt.wantErr) {
+					t.Error(err)
+				}
+			}
+		})
+	}
+}