Update virtualmachines service to armcompute/v5 SDK

This replaces use of the legacy /services SDK for compute only in the
virtualmachines service, with a minimal set of supporting changes. This
allows using Azure APIs beyond those available via the legacy SDK.

(cherry picked from commit f76ad07)

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.

(cherry picked from commit afd0f3f)

Author: mdbooth

go.mod

@@ -4,8 +4,10 @@ go 1.23.0
 
 require (
 	github.com/Azure/azure-sdk-for-go v68.0.0+incompatible
+	github.com/Azure/azure-sdk-for-go-extensions v0.1.8
 	github.com/Azure/azure-sdk-for-go/sdk/azcore v1.12.0
 	github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.6.0
+	github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.7.0
 	github.com/Azure/go-autorest/autorest v0.11.29
 	github.com/Azure/go-autorest/autorest/to v0.4.0
 	github.com/ghodss/yaml v1.0.0

go.sum

@@ -1,11 +1,19 @@
 github.com/Azure/azure-sdk-for-go v68.0.0+incompatible h1:fcYLmCpyNYRnvJbPerq7U0hS+6+I79yEDJBqVNcqUzU=
 github.com/Azure/azure-sdk-for-go v68.0.0+incompatible/go.mod h1:9XXNKU+eRnpl9moKnB4QOLf1HestfXbmab5FXxiDBjc=
+github.com/Azure/azure-sdk-for-go-extensions v0.1.8 h1:x8Vu78C4r8mh6V2yQKQRSWLU+EYBVwFsf3XECddyb6s=
+github.com/Azure/azure-sdk-for-go-extensions v0.1.8/go.mod h1:4su5NjJwhqFH2B/5zJSKOz7hazfr2y38Iu6W4ZK0HYA=
 github.com/Azure/azure-sdk-for-go/sdk/azcore v1.12.0 h1:1nGuui+4POelzDwI7RG56yfQJHCnKvwfMoU7VsEp+Zg=
 github.com/Azure/azure-sdk-for-go/sdk/azcore v1.12.0/go.mod h1:99EvauvlcJ1U06amZiksfYz/3aFGyIhWGHVyiZXtBAI=
 github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.6.0 h1:U2rTu3Ef+7w9FHKIAXM6ZyqF3UOWJZ12zIm8zECAFfg=
 github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.6.0/go.mod h1:9kIvujWAA58nmPmWB1m23fyWic1kYZMxD9CxaWn4Qpg=
 github.com/Azure/azure-sdk-for-go/sdk/internal v1.9.0 h1:H+U3Gk9zY56G3u872L82bk4thcsy2Gghb9ExT4Zvm1o=
 github.com/Azure/azure-sdk-for-go/sdk/internal v1.9.0/go.mod h1:mgrmMSgaLp9hmax62XQTd0N4aAqSE5E0DulSpVYK7vc=
+github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.7.0 h1:LkHbJbgF3YyvC53aqYGR+wWQDn2Rdp9AQdGndf9QvY4=
+github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5 v5.7.0/go.mod h1:QyiQdW4f4/BIfB8ZutZ2s+28RAgfa/pT+zS++ZHyM1I=
+github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v2 v2.0.0 h1:PTFGRSlMKCQelWwxUyYVEUqseBJVemLyqWJjvMyt0do=
+github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/internal/v2 v2.0.0/go.mod h1:LRr2FzBTQlONPPa5HREE5+RjSCTXl7BwOvYOaWTqCaI=
+github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.1.1 h1:7CBQ+Ei8SP2c6ydQTGCCrS35bDxgTMfoP2miAwK++OU=
+github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/resources/armresources v1.1.1/go.mod h1:c/wcGeGx5FUPbM/JltUYHZcKmigwyVLJlDq+4HdtXaw=
 github.com/Azure/go-ansiterm v0.0.0-20230124172434-306776ec8161 h1:L/gRVlceqvL25UVaW/CKtUDjefjrs0SPonmDGUVOYP0=
 github.com/Azure/go-ansiterm v0.0.0-20230124172434-306776ec8161/go.mod h1:xomTg63KZ2rFqZQzSB4Vz2SUXa1BpHTVz9L5PTmPC4E=
 github.com/Azure/go-autorest v14.2.0+incompatible h1:V5VMDjClD3GiElqLWO7mz2MxNAK/vTfRHdAubSIPRgs=

pkg/cloud/azure/actuators/clients.go

@@ -17,12 +17,47 @@ limitations under the License.
 package actuators
 
 import (
+	"net/http"
+
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore/cloud"
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
 	"github.com/Azure/go-autorest/autorest"
+
+	"github.com/openshift/machine-api-provider-azure/pkg/cloud/azure"
 )
 
 // AzureClients contains all the Azure clients used by the scopes.
 type AzureClients struct {
 	SubscriptionID          string
 	Authorizer              autorest.Authorizer
 	ResourceManagerEndpoint string
+
+	// For SDK v2
+	Token azcore.TokenCredential
+	Cloud cloud.Configuration
+}
+
+// ARMClientOptions returns default ARM client options for CAPZ SDK v2 requests.
+func (c *AzureClients) ARMClientOptions() *arm.ClientOptions {
+	opts := &arm.ClientOptions{}
+	opts.Cloud = c.Cloud
+	opts.PerCallPolicies = []policy.Policy{
+		userAgentPolicy{},
+	}
+	opts.Retry.MaxRetries = -1 // Less than zero means one try and no retries.
+
+	return opts
+}
+
+// userAgentPolicy extends the "User-Agent" header on requests.
+// It implements the policy.Policy interface.
+type userAgentPolicy struct{}
+
+// Do extends the "User-Agent" header of a request by appending CAPZ's user agent.
+func (p userAgentPolicy) Do(req *policy.Request) (*http.Response, error) {
+	// FIXME: Ought to include a version after our UA string if we have one
+	req.Raw().Header.Set("User-Agent", req.Raw().UserAgent()+" "+azure.UserAgent)
+	return req.Next()
 }

pkg/cloud/azure/actuators/machine/actuator.go

@@ -22,7 +22,7 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/Azure/go-autorest/autorest"
+	azureerrors "github.com/Azure/azure-sdk-for-go-extensions/pkg/errors"
 	machinev1 "github.com/openshift/api/machine/v1beta1"
 	machineapierrors "github.com/openshift/machine-api-operator/pkg/controller/machine"
 	"github.com/openshift/machine-api-provider-azure/pkg/cloud/azure"
@@ -106,15 +106,13 @@ func (a *Actuator) Create(ctx context.Context, machine *machinev1.Machine) error
 			klog.Errorf("Error storing machine info: %v", err)
 		}
 
-		var detailedError autorest.DetailedError
-		if errors.As(err, &detailedError) {
-			statusCode, ok := detailedError.StatusCode.(int)
+		if azErr := azureerrors.IsResponseError(err); azErr != nil {
 			// Any 4xx error that isn't invalid credentials should be a terminal failure.
 			// Invalid Credentials implies that the credentials expired between the scope creation and API calls,
 			// this may happen when CCO is refreshing credentials simultaneously.
 			// In this case we should retry as the credentials should be updated in the secret.
-			if ok && statusCode >= 400 && statusCode < 500 && !azure.InvalidCredentials(err) {
-				return a.handleMachineError(machine, machineapierrors.InvalidMachineConfiguration("failed to reconcile machine %q: %v", machine.Name, detailedError), createEventAction)
+			if azErr.StatusCode >= 400 && azErr.StatusCode < 500 && !azure.InvalidCredentials(err) {
+				return a.handleMachineError(machine, machineapierrors.InvalidMachineConfiguration("failed to reconcile machine %q: %v", machine.Name, err), createEventAction)
 			}
 		}
 

pkg/cloud/azure/actuators/machine/actuator_test.go

@@ -25,9 +25,9 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2021-11-01/compute"
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
+	"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"
 	"github.com/Azure/go-autorest/autorest/to"
 	"github.com/ghodss/yaml"
 	"github.com/golang/mock/gomock"
@@ -181,21 +181,22 @@ type FakeVMService struct {
 // Get returns fake success.
 func (s *FakeVMService) Get(ctx context.Context, spec azure.Spec) (interface{}, error) {
 	s.GetCallCount++
-	return compute.VirtualMachine{
+	vm := armcompute.VirtualMachine{
 		ID:   to.StringPtr(s.ID),
 		Name: to.StringPtr(s.Name),
-		VirtualMachineProperties: &compute.VirtualMachineProperties{
+		Properties: &armcompute.VirtualMachineProperties{
 			ProvisioningState: to.StringPtr(s.ProvisioningState),
-			NetworkProfile: &compute.NetworkProfile{
-				NetworkInterfaces: &[]compute.NetworkInterfaceReference{
+			NetworkProfile: &armcompute.NetworkProfile{
+				NetworkInterfaces: []*armcompute.NetworkInterfaceReference{
 					{
-						ID:                                  to.StringPtr("machine-test-nic"),
-						NetworkInterfaceReferenceProperties: &compute.NetworkInterfaceReferenceProperties{},
+						ID:         to.StringPtr("machine-test-nic"),
+						Properties: &armcompute.NetworkInterfaceReferenceProperties{},
 					},
 				},
 			},
 		},
-	}, nil
+	}
+	return armcompute.VirtualMachinesClientGetResponse{VirtualMachine: vm}, nil
 }
 
 // CreateOrUpdate returns fake success.
@@ -393,9 +394,10 @@ func (s *FakeVMCheckZonesService) Get(ctx context.Context, spec azure.Spec) (int
 		return nil, errors.New("vm not found")
 	}
 
-	return &compute.VirtualMachine{
-		VirtualMachineProperties: &compute.VirtualMachineProperties{},
-	}, nil
+	vm := armcompute.VirtualMachine{
+		Properties: &armcompute.VirtualMachineProperties{},
+	}
+	return armcompute.VirtualMachinesClientGetResponse{VirtualMachine: vm}, nil
 }
 
 // CreateOrUpdate returns fake success.
@@ -731,19 +733,20 @@ func TestMachineEvents(t *testing.T) {
 			})
 
 			networkSvc.EXPECT().CreateOrUpdate(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
-			vmSvc.EXPECT().Get(gomock.Any(), gomock.Any()).Return(compute.VirtualMachine{
+			vm := armcompute.VirtualMachine{
 				ID:   ptr.To[string]("vm-id"),
 				Name: ptr.To[string]("vm-name"),
-				VirtualMachineProperties: &compute.VirtualMachineProperties{
+				Properties: &armcompute.VirtualMachineProperties{
 					ProvisioningState: ptr.To[string]("Succeeded"),
-					HardwareProfile: &compute.HardwareProfile{
-						VMSize: compute.VirtualMachineSizeTypesStandardB2ms,
+					HardwareProfile: &armcompute.HardwareProfile{
+						VMSize: ptr.To(armcompute.VirtualMachineSizeTypesStandardB2Ms),
 					},
-					OsProfile: &compute.OSProfile{
+					OSProfile: &armcompute.OSProfile{
 						ComputerName: ptr.To[string]("vm-name"),
 					},
 				},
-			}, nil).AnyTimes()
+			}
+			vmSvc.EXPECT().Get(gomock.Any(), gomock.Any()).Return(armcompute.VirtualMachinesClientGetResponse{VirtualMachine: vm}, nil).AnyTimes()
 			vmSvc.EXPECT().Delete(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
 			disksSvc.EXPECT().Delete(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
 			networkSvc.EXPECT().Delete(gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
@@ -788,25 +791,21 @@ func TestStatusCodeBasedCreationErrors(t *testing.T) {
 	cases := []struct {
 		name       string
 		operation  func(actuator *Actuator, machine *machinev1.Machine)
-		event      string
-		statusCode interface{}
+		statusCode int
 		requeable  bool
 	}{
 		{
 			name:       "InvalidConfig",
-			event:      "Warning FailedCreate InvalidConfiguration: failed to reconcile machine \"azure-actuator-testing-machine\": compute.VirtualMachinesClient#CreateOrUpdate: MOCK: StatusCode=400",
 			statusCode: 400,
 			requeable:  false,
 		},
 		{
 			name:       "CreateMachine",
-			event:      "Warning FailedCreate CreateError: failed to reconcile machine \"azure-actuator-testing-machine\"s: failed to create vm azure-actuator-testing-machine: failed to create VM: failed to create or get machine: compute.VirtualMachinesClient#CreateOrUpdate: MOCK: StatusCode=300",
 			statusCode: 300,
 			requeable:  true,
 		},
 		{
 			name:       "CreateMachine",
-			event:      "Warning FailedCreate CreateError: failed to reconcile machine \"azure-actuator-testing-machine\"s: failed to create vm azure-actuator-testing-machine: failed to create VM: failed to create or get machine: compute.VirtualMachinesClient#CreateOrUpdate: MOCK: StatusCode=401",
 			statusCode: 401,
 			requeable:  true,
 		},
@@ -850,11 +849,13 @@ func TestStatusCodeBasedCreationErrors(t *testing.T) {
 			})
 
 			networkSvc.EXPECT().CreateOrUpdate(gomock.Any(), gomock.Any()).Return(nil).Times(1)
-			azureErr := autorest.NewError("compute.VirtualMachinesClient", "CreateOrUpdate", "MOCK")
-			azureErr.StatusCode = tc.statusCode
+			var azureErr error = &azcore.ResponseError{
+				ErrorCode:  fmt.Sprintf("StatusCode=%d", tc.statusCode),
+				StatusCode: tc.statusCode,
+			}
 			wrapErr := fmt.Errorf("failed to create or get machine: %w", azureErr)
 			vmSvc.EXPECT().CreateOrUpdate(gomock.Any(), gomock.Any()).Return(wrapErr).Times(1)
-			vmSvc.EXPECT().Get(gomock.Any(), gomock.Any()).Return(nil, autorest.NewError("compute.VirtualMachinesClient", "Get", "MOCK")).Times(1)
+			vmSvc.EXPECT().Get(gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("mock compute.VirtualMachinesClient Get error")).Times(1)
 			availabilityZonesSvc.EXPECT().Get(gomock.Any(), gomock.Any()).Return([]string{"testzone"}, nil).Times(1)
 
 			_, ok := machineActuator.Create(context.TODO(), machine).(*machineapierrors.RequeueAfterError)
@@ -867,11 +868,11 @@ func TestStatusCodeBasedCreationErrors(t *testing.T) {
 
 			select {
 			case event := <-eventsChannel:
-				if event != tc.event {
-					t.Errorf("Expected %q event, got %q", tc.event, event)
+				if !strings.Contains(event, azureErr.Error()) {
+					t.Errorf("Expected event to contain error %q, got %q", azureErr.Error(), event)
 				}
 			default:
-				t.Errorf("Expected %q event, got none", tc.event)
+				t.Errorf("Expected an event containing error %q, got none", azureErr.Error())
 			}
 		})
 	}