OCPBUGS-55372: Fix regression on ASH with armcompute/v5

The change "Update virtualmachines service to armcompute/v5 SDK" did not
update the StackHub implementation of the virtualmachines service, which
caused a failure in the machine actuator due to the difference in
type returned by Get().

We fix this by moving to a common implementation for both StackHub and
public clouds.

Having a common implementation in virtualmachines also requires a change
to the networkinterfaces Service. The networkinterfaces service Get()
method returns different types for StackHub and public clouds, which
were previously cast to the anticipated types by different
implementations of the virtualmachines service. As we don't require the
full type, we add a new type safe method which returns only the ID,
meaning a common implementation is now safe.

Author: mdbooth

pkg/cloud/azure/services/networkinterfaces/networkinterfaces.go

@@ -60,13 +60,31 @@ func (s *Service) Get(ctx context.Context, spec azure.Spec) (interface{}, error)
 	if !ok {
 		return network.Interface{}, errors.New("invalid network interface specification")
 	}
-	nic, err := s.Client.Get(ctx, s.Scope.MachineConfig.ResourceGroup, nicSpec.Name, "")
+	return s.get(ctx, nicSpec.Name)
+}
+
+func (s *Service) get(ctx context.Context, name string) (*network.Interface, error) {
+	nic, err := s.Client.Get(ctx, s.Scope.MachineConfig.ResourceGroup, name, "")
 	if err != nil && azure.ResourceNotFound(err) {
-		return nil, fmt.Errorf("network interface %s not found: %w", nicSpec.Name, err)
+		return nil, fmt.Errorf("network interface %s not found: %w", name, err)
 	} else if err != nil {
-		return nic, err
+		return nil, err
 	}
-	return nic, nil
+	return &nic, nil
+}
+
+// GetID returns the ID of the network interface with the given name
+func (s *Service) GetID(ctx context.Context, name string) (string, error) {
+	nic, err := s.get(ctx, name)
+	if err != nil {
+		return "", err
+	}
+
+	if nic.ID == nil {
+		return "", fmt.Errorf("network interface %s does not have an ID", name)
+	}
+
+	return *nic.ID, nil
 }
 
 // CreateOrUpdate creates or updates a network interface.

pkg/cloud/azure/services/networkinterfaces/networkinterfaces_stack.go

@@ -40,13 +40,31 @@ func (s *StackHubService) Get(ctx context.Context, spec azure.Spec) (interface{}
 	if !ok {
 		return network.Interface{}, errors.New("invalid network interface specification")
 	}
-	nic, err := s.Client.Get(ctx, s.Scope.MachineConfig.ResourceGroup, nicSpec.Name, "")
+	return s.get(ctx, nicSpec.Name)
+}
+
+func (s *StackHubService) get(ctx context.Context, name string) (*network.Interface, error) {
+	nic, err := s.Client.Get(ctx, s.Scope.MachineConfig.ResourceGroup, name, "")
 	if err != nil && azure.ResourceNotFound(err) {
-		return nil, fmt.Errorf("network interface %s not found: %w", nicSpec.Name, err)
+		return nil, fmt.Errorf("network interface %s not found: %w", name, err)
 	} else if err != nil {
-		return nic, err
+		return nil, err
 	}
-	return nic, nil
+	return &nic, nil
+}
+
+// GetID returns the ID of the network interface with the given name
+func (s *StackHubService) GetID(ctx context.Context, name string) (string, error) {
+	nic, err := s.get(ctx, name)
+	if err != nil {
+		return "", err
+	}
+
+	if nic.ID == nil {
+		return "", fmt.Errorf("network interface %s does not have an ID", name)
+	}
+
+	return *nic.ID, nil
 }
 
 // CreateOrUpdate creates or updates a network interface.

pkg/cloud/azure/services/networkinterfaces/service.go

@@ -17,6 +17,8 @@ limitations under the License.
 package networkinterfaces
 
 import (
+	"context"
+
 	"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network"
 	"github.com/Azure/go-autorest/autorest"
 	"github.com/openshift/machine-api-provider-azure/pkg/cloud/azure"
@@ -29,6 +31,24 @@ type Service struct {
 	Scope  *actuators.MachineScope
 }
 
+// ServiceWithGetID implements the azure.Service interface, but additionally
+// provides a type safe method to return the ID of an object.
+//
+// The StackHub implementation of networkinterfaces uses a different API version
+// of 'network' to the non-StackHub version, which with the legacy SDK used here
+// means they return different types. To avoid potential errors from forgetting
+// this difference at the point of use, GetID returns a string and handles type
+// differences internally.
+//
+// This is useful in the virtualmachines service, which references a nic by ID
+// and does not require the full object.
+type ServiceWithGetID interface {
+	azure.Service
+
+	// GetID returns the ID of the object with the given name
+	GetID(ctx context.Context, name string) (string, error)
+}
+
 // getGroupsClient creates a new groups client from subscriptionid.
 func getNetworkInterfacesClient(resourceManagerEndpoint, subscriptionID string, authorizer autorest.Authorizer) network.InterfacesClient {
 	nicClient := network.NewInterfacesClientWithBaseURI(resourceManagerEndpoint, subscriptionID)
@@ -38,7 +58,7 @@ func getNetworkInterfacesClient(resourceManagerEndpoint, subscriptionID string,
 }
 
 // NewService creates a new groups service.
-func NewService(scope *actuators.MachineScope) azure.Service {
+func NewService(scope *actuators.MachineScope) ServiceWithGetID {
 	if scope.IsStackHub() {
 		return NewStackHubService(scope)
 	}

pkg/cloud/azure/services/networkinterfaces/service_stack.go

@@ -38,7 +38,7 @@ func getNetworkInterfacesClientStackHub(resourceManagerEndpoint, subscriptionID
 }
 
 // NewStackHubService creates a new groups service.
-func NewStackHubService(scope *actuators.MachineScope) azure.Service {
+func NewStackHubService(scope *actuators.MachineScope) ServiceWithGetID {
 	return &StackHubService{
 		Client: getNetworkInterfacesClientStackHub(scope.ResourceManagerEndpoint, scope.SubscriptionID, scope.Authorizer),
 		Scope:  scope,

pkg/cloud/azure/services/virtualmachines/service.go

@@ -23,6 +23,17 @@ import (
 	"github.com/openshift/machine-api-provider-azure/pkg/cloud/azure/actuators"
 )
 
+// stackHubAPIVersionVirtualMachines is the API version which will be used when
+// connecting to a StackHub cloud.
+//
+// Bumping this value changes the minimum version requirements of the target
+// StackHub deployment. Changes to this value should not be backported, and
+// require a release note.
+//
+// For public cloud deployments, the latest API version supported by the SDK
+// will be used.
+const stackHubAPIVersionVirtualMachines = "2019-03-01"
+
 // Service provides operations on resource groups
 type Service struct {
 	Client *armcompute.VirtualMachinesClient
@@ -31,11 +42,13 @@ type Service struct {
 
 // NewService creates a new groups service.
 func NewService(scope *actuators.MachineScope) azure.Service {
+	options := scope.ARMClientOptions()
+
 	if scope.IsStackHub() {
-		return NewServiceStackHub(scope)
+		options.APIVersion = stackHubAPIVersionVirtualMachines
 	}
 
-	vmClient, err := armcompute.NewVirtualMachinesClient(scope.SubscriptionID, scope.Token, scope.ARMClientOptions())
+	vmClient, err := armcompute.NewVirtualMachinesClient(scope.SubscriptionID, scope.Token, options)
 	if err != nil {
 		return azure.NewServiceClientError(err)
 	}

pkg/cloud/azure/services/virtualmachines/service_stack.go

@@ -27,7 +27,6 @@ import (
 	"strconv"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5"
-	"github.com/Azure/azure-sdk-for-go/services/network/mgmt/2021-02-01/network"
 	"github.com/Azure/go-autorest/autorest/to"
 	machinev1 "github.com/openshift/api/machine/v1beta1"
 	apierrors "github.com/openshift/machine-api-operator/pkg/controller/machine"
@@ -116,20 +115,15 @@ func (s *Service) CreateOrUpdate(ctx context.Context, spec azure.Spec) error {
 	}
 
 	klog.V(2).Infof("getting nic %s", vmSpec.NICName)
-	nicInterface, err := networkinterfaces.NewService(s.Scope).Get(ctx, &networkinterfaces.Spec{Name: vmSpec.NICName})
+	nicID, err := networkinterfaces.NewService(s.Scope).GetID(ctx, vmSpec.NICName)
 	if err != nil {
 		return err
 	}
-
-	nic, ok := nicInterface.(network.Interface)
-	if !ok {
-		return errors.New("error getting network security group")
-	}
 	klog.V(2).Infof("got nic %s", vmSpec.NICName)
 
 	klog.V(2).Infof("creating vm %s ", vmSpec.Name)
 
-	virtualMachine, err := s.deriveVirtualMachineParameters(vmSpec, nic)
+	virtualMachine, err := s.deriveVirtualMachineParameters(vmSpec, nicID)
 	if err != nil {
 		return err
 	}
@@ -164,7 +158,7 @@ func generateOSProfile(vmSpec *Spec) (*armcompute.OSProfile, error) {
 		sshKeyData = string(ssh.MarshalAuthorizedKey(publicRsaKey))
 	}
 
-	randomPassword, err := GenerateRandomString(32)
+	randomPassword, err := generateRandomString(32)
 	if err != nil {
 		return nil, fmt.Errorf("failed to generate random string: %w", err)
 	}
@@ -216,7 +210,11 @@ func generateOSProfile(vmSpec *Spec) (*armcompute.OSProfile, error) {
 // Derive virtual machine parameters for CreateOrUpdate API call based
 // on the provided virtual machine specification, resource location,
 // subscription ID, and the network interface.
-func (s *Service) deriveVirtualMachineParameters(vmSpec *Spec, nic network.Interface) (*armcompute.VirtualMachine, error) {
+func (s *Service) deriveVirtualMachineParameters(vmSpec *Spec, nicID string) (*armcompute.VirtualMachine, error) {
+	if s.Scope.IsStackHub() {
+		return s.deriveVirtualMachineParametersStackHub(vmSpec, nicID)
+	}
+
 	osProfile, err := generateOSProfile(vmSpec)
 	if err != nil {
 		return nil, err
@@ -269,7 +267,7 @@ func (s *Service) deriveVirtualMachineParameters(vmSpec *Spec, nic network.Inter
 			NetworkProfile: &armcompute.NetworkProfile{
 				NetworkInterfaces: []*armcompute.NetworkInterfaceReference{
 					{
-						ID: nic.ID,
+						ID: &nicID,
 						Properties: &armcompute.NetworkInterfaceReferenceProperties{
 							Primary: to.BoolPtr(true),
 						},
@@ -368,12 +366,12 @@ func (s *Service) Delete(ctx context.Context, spec azure.Spec) error {
 	return err
 }
 
-// GenerateRandomString returns a URL-safe, base64 encoded
+// generateRandomString returns a URL-safe, base64 encoded
 // securely generated random string.
 // It will return an error if the system's secure random
 // number generator fails to function correctly, in which
 // case the caller should not continue.
-func GenerateRandomString(n int) (string, error) {
+func generateRandomString(n int) (string, error) {
 	b := make([]byte, n)
 	_, err := rand.Read(b)
 	// Note that err == nil only if we read len(b) bytes.