Merge pull request #10076 from patrickdillon/az-mkt

CORS-3657: Default Azure Installs to Marketplace Images

Author: openshift-merge-bot[bot]

.golangci.yaml

@@ -33,7 +33,6 @@ linters:
   - revive
   - staticcheck
   - stylecheck
-  - tenv
   - thelper
   - typecheck
   - unconvert
@@ -148,6 +147,8 @@ linters-settings:
     - name: redefines-builtin-id
     - name: bool-literal-in-expr
     - name: constant-logical-expr
+  gocyclo:
+    min-complexity: 45
 issues:
   include:
   - EXC0012 # EXC0012 revive: issue about not having a comment on exported.

pkg/asset/cluster/tfvars/tfvars.go

@@ -373,14 +373,6 @@ func (t *TerraformVariables) Generate(ctx context.Context, parents asset.Parents
 		for i, w := range workers {
 			workerConfigs[i] = w.Spec.Template.Spec.ProviderSpec.Value.Object.(*machinev1beta1.AzureMachineProviderSpec) //nolint:errcheck // legacy, pre-linter
 		}
-		client, err := installConfig.Azure.Client()
-		if err != nil {
-			return err
-		}
-		hyperVGeneration, err := client.GetHyperVGenerationVersion(ctx, masterConfigs[0].VMSize, masterConfigs[0].Location, "")
-		if err != nil {
-			return err
-		}
 
 		preexistingnetwork := installConfig.Config.Azure.VirtualNetwork != ""
 
@@ -427,7 +419,6 @@ func (t *TerraformVariables) Generate(ctx context.Context, parents asset.Parents
 				OutboundType:                    installConfig.Config.Azure.OutboundType,
 				BootstrapIgnStub:                bootstrapIgnStub,
 				BootstrapIgnitionURLPlaceholder: bootstrapIgnURLPlaceholder,
-				HyperVGeneration:                hyperVGeneration,
 				VMArchitecture:                  installConfig.Config.ControlPlane.Architecture,
 				InfrastructureName:              clusterID.InfraID,
 				KeyVault:                        managedKeys.KeyVault,

pkg/asset/installconfig/azure/client.go

@@ -38,7 +38,6 @@ type API interface {
 	ListResourceIDsByGroup(ctx context.Context, groupName string) ([]string, error)
 	GetStorageEndpointSuffix(ctx context.Context) (string, error)
 	GetDiskEncryptionSet(ctx context.Context, subscriptionID, groupName string, diskEncryptionSetName string) (*azenc.DiskEncryptionSet, error)
-	GetHyperVGenerationVersion(ctx context.Context, instanceType string, region string, imageHyperVGen string) (string, error)
 	GetMarketplaceImage(ctx context.Context, region, publisher, offer, sku, version string) (azenc.VirtualMachineImage, error)
 	AreMarketplaceImageTermsAccepted(ctx context.Context, publisher, offer, sku string) (bool, error)
 	GetVMCapabilities(ctx context.Context, instanceType, region string) (map[string]string, error)
@@ -364,17 +363,6 @@ func (c *Client) GetVMCapabilities(ctx context.Context, instanceType, region str
 	return capabilities, nil
 }
 
-// GetHyperVGenerationVersion gets the HyperVGeneration version for the given instance type and marketplace image version, if specified. Defaults to V2 if either V1 or V2
-// available.
-func (c *Client) GetHyperVGenerationVersion(ctx context.Context, instanceType string, region string, imageHyperVGen string) (version string, err error) {
-	capabilities, err := c.GetVMCapabilities(ctx, instanceType, region)
-	if err != nil {
-		return "", err
-	}
-
-	return GetHyperVGenerationVersion(capabilities, imageHyperVGen)
-}
-
 // GetMarketplaceImage get the specified marketplace VM image.
 func (c *Client) GetMarketplaceImage(ctx context.Context, region, publisher, offer, sku, version string) (azenc.VirtualMachineImage, error) {
 	client := azenc.NewVirtualMachineImagesClientWithBaseURI(c.ssn.Environment.ResourceManagerEndpoint, c.ssn.Credentials.SubscriptionID)

pkg/asset/installconfig/azure/metadata.go

@@ -8,7 +8,9 @@ import (
 
 	"sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"
 
+	"github.com/openshift/installer/pkg/types"
 	typesazure "github.com/openshift/installer/pkg/types/azure"
+	azuredefaults "github.com/openshift/installer/pkg/types/azure/defaults"
 )
 
 // Metadata holds additional metadata for InstallConfig resources that
@@ -23,6 +25,13 @@ type Metadata struct {
 	region            string
 	ZonesSubnetMap    map[string][]string
 
+	controlPlane    *types.MachinePool
+	compute         *types.MachinePool
+	defaultPlatform *typesazure.MachinePool
+
+	controlPlaneCapabilities map[string]string
+	computeCapabilities      map[string]string
+
 	// CloudName indicates the Azure cloud environment (e.g. public, gov't).
 	CloudName typesazure.CloudEnvironment `json:"cloudName,omitempty"`
 
@@ -42,18 +51,21 @@ type Metadata struct {
 }
 
 // NewMetadata initializes a new Metadata object.
-func NewMetadata(cloudName typesazure.CloudEnvironment, armEndpoint string, region string) *Metadata {
-	return NewMetadataWithCredentials(cloudName, armEndpoint, nil, region)
+func NewMetadata(az *typesazure.Platform, controlPlane, compute *types.MachinePool) *Metadata {
+	return NewMetadataWithCredentials(az, controlPlane, compute, nil)
 }
 
 // NewMetadataWithCredentials initializes a new Metadata object
 // with prepopulated Azure credentials.
-func NewMetadataWithCredentials(cloudName typesazure.CloudEnvironment, armEndpoint string, credentials *Credentials, region string) *Metadata {
+func NewMetadataWithCredentials(az *typesazure.Platform, controlPlane, compute *types.MachinePool, credentials *Credentials) *Metadata {
 	return &Metadata{
-		CloudName:   cloudName,
-		ARMEndpoint: armEndpoint,
-		Credentials: credentials,
-		region:      region,
+		CloudName:       az.CloudName,
+		ARMEndpoint:     az.ARMEndpoint,
+		Credentials:     credentials,
+		controlPlane:    controlPlane,
+		compute:         compute,
+		region:          az.Region,
+		defaultPlatform: az.DefaultMachinePlatform,
 	}
 }
 
@@ -198,3 +210,73 @@ func (m *Metadata) GenerateZonesSubnetMap(subnetSpec []typesazure.SubnetSpec, de
 	}
 	return m.ZonesSubnetMap, nil
 }
+
+// ControlPlaneCapabilities returns the capabilities for the instance type of control-plane
+// nodes from the Azure API.
+func (m *Metadata) ControlPlaneCapabilities() (map[string]string, error) {
+	if m.controlPlaneCapabilities == nil {
+		caps, err := m.getCapabilities(m.controlPlane, azuredefaults.ControlPlaneInstanceType)
+		if err != nil {
+			return nil, fmt.Errorf("failed to get control plane capabilities: %w", err)
+		}
+		m.controlPlaneCapabilities = caps
+	}
+	return m.controlPlaneCapabilities, nil
+}
+
+// ComputeCapabilities returns the capabilities for the instance type of compute nodes
+// from the Azure API.
+func (m *Metadata) ComputeCapabilities() (map[string]string, error) {
+	if m.computeCapabilities == nil {
+		caps, err := m.getCapabilities(m.compute, azuredefaults.ComputeInstanceType)
+		if err != nil {
+			return nil, fmt.Errorf("failed to get compute capabilities: %w", err)
+		}
+		m.computeCapabilities = caps
+	}
+	return m.computeCapabilities, nil
+}
+
+// ControlPlaneHyperVGeneration returns the HyperVGeneration for the control-plane
+// instances. If the instance type supports both V1 & V2, V2 is returned.
+func (m *Metadata) ControlPlaneHyperVGeneration() (string, error) {
+	caps, err := m.ControlPlaneCapabilities()
+	if err != nil {
+		return "", fmt.Errorf("unable to get control plane capabilities: %w", err)
+	}
+	return GetHyperVGenerationVersion(caps, "")
+}
+
+// ComputeHyperVGeneration returns the HyperVGeneration for the compute instances.
+// If the instance type supports both V1 & V2, V2 is returned.
+func (m *Metadata) ComputeHyperVGeneration() (string, error) {
+	caps, err := m.ComputeCapabilities()
+	if err != nil {
+		return "", fmt.Errorf("unable to get compute capabilities: %w", err)
+	}
+	return GetHyperVGenerationVersion(caps, "")
+}
+
+type machinePoolInstanceTypeFunc func(typesazure.CloudEnvironment, string, types.Architecture) string
+
+func (m *Metadata) getCapabilities(mPool *types.MachinePool, defaultInstanceType machinePoolInstanceTypeFunc) (map[string]string, error) {
+	if mPool == nil {
+		return nil, fmt.Errorf("unable to get capabilities because machinepool is not populated in metadata")
+	}
+	instType := defaultInstanceType(m.CloudName, m.region, mPool.Architecture)
+	if dmp := m.defaultPlatform; dmp != nil && dmp.InstanceType != "" {
+		instType = dmp.InstanceType
+	}
+	if mPool.Platform.Azure != nil && mPool.Platform.Azure.InstanceType != "" {
+		instType = mPool.Platform.Azure.InstanceType
+	}
+	client, err := m.Client()
+	if err != nil {
+		return nil, fmt.Errorf("failed to get azure client %w", err)
+	}
+	caps, err := client.GetVMCapabilities(context.TODO(), instType, m.region)
+	if err != nil {
+		return nil, err
+	}
+	return caps, nil
+}

pkg/asset/installconfig/azure/mock/azureclient_generated.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"net"
 	"net/url"
-	"os"
 	"slices"
 	"sort"
 	"strconv"
@@ -53,23 +52,23 @@ var computeReq = resourceRequirements{
 }
 
 // Validate executes platform-specific validation.
-func Validate(client API, ic *types.InstallConfig) error {
+func Validate(client API, meta *Metadata, ic *types.InstallConfig) error {
 	allErrs := field.ErrorList{}
 
 	allErrs = append(allErrs, validateNetworks(client, ic.Azure, field.NewPath("platform").Child("azure"))...)
 	allErrs = append(allErrs, validateRegion(client, field.NewPath("platform").Child("azure").Child("region"), ic.Azure)...)
 	if ic.Azure.CloudName == aztypes.StackCloud {
 		allErrs = append(allErrs, validateAzureStackDiskType(client, ic)...)
 	}
-	allErrs = append(allErrs, validateInstanceTypes(client, ic)...)
+	allErrs = append(allErrs, validateInstanceTypes(client, meta, ic)...)
 	if ic.Azure.CloudName == aztypes.StackCloud && ic.Azure.ClusterOSImage != "" {
 		StorageEndpointSuffix, err := client.GetStorageEndpointSuffix(context.TODO())
 		if err != nil {
 			return err
 		}
 		allErrs = append(allErrs, validateAzureStackClusterOSImage(StorageEndpointSuffix, ic.Azure.ClusterOSImage, field.NewPath("platform").Child("azure"))...)
 	}
-	allErrs = append(allErrs, validateMarketplaceImages(client, ic)...)
+	allErrs = append(allErrs, validateMarketplaceImages(client, meta, ic)...)
 	allErrs = append(allErrs, validateBootDiagnostics(client, ic)...)
 	allErrs = append(allErrs, validateCustomSubnets(client, field.NewPath("platform").Child("azure").Child("subnetSpec"), ic)...)
 	return allErrs.ToAggregate()
@@ -353,12 +352,15 @@ func validateUltraSSD(client API, fieldPath *field.Path, icZones []string, regio
 }
 
 // ValidateInstanceType ensures the instance type has sufficient Vcpu, Memory, and a valid family type.
-func ValidateInstanceType(client API, fieldPath *field.Path, region, instanceType, diskType string, req resourceRequirements, ultraSSDEnabled bool, vmNetworkingType string, icZones []string, architecture types.Architecture, securityType aztypes.SecurityTypes) field.ErrorList {
+func ValidateInstanceType(client API, fieldPath *field.Path, region, instanceType, diskType string, req resourceRequirements, ultraSSDEnabled bool, vmNetworkingType string, icZones []string, architecture types.Architecture, securityType aztypes.SecurityTypes, capabilities map[string]string) field.ErrorList {
 	allErrs := field.ErrorList{}
 
-	capabilities, err := client.GetVMCapabilities(context.TODO(), instanceType, region)
-	if err != nil {
-		return append(allErrs, field.Invalid(fieldPath.Child("type"), instanceType, err.Error()))
+	var err error
+	if capabilities == nil {
+		capabilities, err = client.GetVMCapabilities(context.TODO(), instanceType, region)
+		if err != nil {
+			return append(allErrs, field.Invalid(fieldPath.Child("type"), instanceType, err.Error()))
+		}
 	}
 
 	allErrs = append(allErrs, validateMininumRequirements(fieldPath.Child("type"), req, instanceType, capabilities)...)
@@ -386,7 +388,7 @@ func ValidateInstanceType(client API, fieldPath *field.Path, region, instanceTyp
 }
 
 // validateInstanceTypes checks that the user-provided instance types are valid.
-func validateInstanceTypes(client API, ic *types.InstallConfig) field.ErrorList {
+func validateInstanceTypes(client API, meta *Metadata, ic *types.InstallConfig) field.ErrorList {
 	allErrs := field.ErrorList{}
 
 	var securityType aztypes.SecurityTypes
@@ -451,7 +453,7 @@ func validateInstanceTypes(client API, ic *types.InstallConfig) field.ErrorList
 			zones = defaultZones
 		}
 		ultraSSDEnabled := strings.EqualFold(ultraSSDCapability, "Enabled")
-		allErrs = append(allErrs, ValidateInstanceType(client, fieldPath, ic.Azure.Region, instanceType, diskType, controlPlaneReq, ultraSSDEnabled, vmNetworkingType, zones, architecture, securityType)...)
+		allErrs = append(allErrs, ValidateInstanceType(client, fieldPath, ic.Azure.Region, instanceType, diskType, controlPlaneReq, ultraSSDEnabled, vmNetworkingType, zones, architecture, securityType, meta.controlPlaneCapabilities)...)
 	}
 
 	for idx, compute := range ic.Compute {
@@ -488,7 +490,7 @@ func validateInstanceTypes(client API, ic *types.InstallConfig) field.ErrorList
 			}
 			ultraSSDEnabled := strings.EqualFold(ultraSSDCapability, "Enabled")
 			allErrs = append(allErrs, ValidateInstanceType(client, fieldPath.Child("platform", "azure"),
-				ic.Azure.Region, instanceType, diskType, computeReq, ultraSSDEnabled, vmNetworkingType, zones, architecture, securityType)...)
+				ic.Azure.Region, instanceType, diskType, computeReq, ultraSSDEnabled, vmNetworkingType, zones, architecture, securityType, meta.computeCapabilities)...)
 		}
 	}
 
@@ -507,7 +509,7 @@ func validateInstanceTypes(client API, ic *types.InstallConfig) field.ErrorList
 		fieldPath := field.NewPath("platform", "azure", "defaultMachinePlatform")
 		ultraSSDEnabled := strings.EqualFold(defaultUltraSSDCapability, "Enabled")
 		allErrs = append(allErrs, ValidateInstanceType(client, fieldPath,
-			ic.Azure.Region, defaultInstanceType, defaultDiskType, minReq, ultraSSDEnabled, defaultVMNetworkingType, defaultZones, architecture, securityType)...)
+			ic.Azure.Region, defaultInstanceType, defaultDiskType, minReq, ultraSSDEnabled, defaultVMNetworkingType, defaultZones, architecture, securityType, nil)...)
 	}
 	return allErrs
 }
@@ -662,46 +664,12 @@ func ValidateForProvisioning(client API, ic *types.InstallConfig) error {
 	allErrs = append(allErrs, validateResourceGroup(client, field.NewPath("platform").Child("azure"), ic.Azure)...)
 	allErrs = append(allErrs, ValidateDiskEncryptionSet(client, ic)...)
 	allErrs = append(allErrs, ValidateSecurityProfileDiskEncryptionSet(client, ic)...)
-	allErrs = append(allErrs, validateSkipImageUpload(field.NewPath("image"), ic)...)
 	if ic.Azure.CloudName == aztypes.StackCloud {
 		allErrs = append(allErrs, checkAzureStackClusterOSImageSet(ic.Azure.ClusterOSImage, field.NewPath("platform").Child("azure"))...)
 	}
 	return allErrs.ToAggregate()
 }
 
-func validateSkipImageUpload(fieldPath *field.Path, ic *types.InstallConfig) field.ErrorList {
-	allErrs := field.ErrorList{}
-	defaultOSImage := aztypes.OSImage{}
-	if ic.Azure.DefaultMachinePlatform != nil {
-		defaultOSImage = aztypes.OSImage{
-			Plan:      ic.Azure.DefaultMachinePlatform.OSImage.Plan,
-			Publisher: ic.Azure.DefaultMachinePlatform.OSImage.Publisher,
-			SKU:       ic.Azure.DefaultMachinePlatform.OSImage.SKU,
-			Version:   ic.Azure.DefaultMachinePlatform.OSImage.Version,
-			Offer:     ic.Azure.DefaultMachinePlatform.OSImage.Offer,
-		}
-	}
-	controlPlaneOSImage := defaultOSImage
-	if ic.ControlPlane.Platform.Azure != nil {
-		controlPlaneOSImage = ic.ControlPlane.Platform.Azure.OSImage
-	}
-	allErrs = append(allErrs, validateOSImage(fieldPath.Child("controlplane"), controlPlaneOSImage)...)
-	computeOSImage := defaultOSImage
-	if len(ic.Compute) > 0 && ic.Compute[0].Platform.Azure != nil {
-		computeOSImage = ic.Compute[0].Platform.Azure.OSImage
-	}
-	allErrs = append(allErrs, validateOSImage(fieldPath.Child("compute"), computeOSImage)...)
-	return allErrs
-}
-func validateOSImage(fieldPath *field.Path, osImage aztypes.OSImage) field.ErrorList {
-	if _, ok := os.LookupEnv("OPENSHIFT_INSTALL_SKIP_IMAGE_UPLOAD"); ok {
-		if len(osImage.SKU) > 0 {
-			return nil
-		}
-		return field.ErrorList{field.Invalid(fieldPath, "image", "cannot skip image upload without marketplace image specified")}
-	}
-	return nil
-}
 func validateResourceGroup(client API, fieldPath *field.Path, platform *aztypes.Platform) field.ErrorList {
 	allErrs := field.ErrorList{}
 	if len(platform.ResourceGroupName) == 0 {
@@ -766,7 +734,7 @@ func validateAzureStackClusterOSImage(StorageEndpointSuffix string, ClusterOSIma
 	return allErrs
 }
 
-func validateMarketplaceImages(client API, installConfig *types.InstallConfig) field.ErrorList {
+func validateMarketplaceImages(client API, meta *Metadata, installConfig *types.InstallConfig) field.ErrorList {
 	var allErrs field.ErrorList
 
 	region := installConfig.Azure.Region
@@ -796,7 +764,7 @@ func validateMarketplaceImages(client API, installConfig *types.InstallConfig) f
 			instanceType = defaults.ControlPlaneInstanceType(cloudName, region, installConfig.ControlPlane.Architecture)
 		}
 
-		capabilities, err := client.GetVMCapabilities(context.Background(), instanceType, region)
+		capabilities, err := meta.ControlPlaneCapabilities()
 		if err != nil {
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("platform", "azure", "type"), instanceType, err.Error()))
 		}
@@ -838,7 +806,7 @@ func validateMarketplaceImages(client API, installConfig *types.InstallConfig) f
 			instanceType = defaults.ComputeInstanceType(cloudName, region, compute.Architecture)
 		}
 
-		capabilities, err := client.GetVMCapabilities(context.Background(), instanceType, region)
+		capabilities, err := meta.ComputeCapabilities()
 		if err != nil {
 			allErrs = append(allErrs, field.Invalid(fldPath.Child("platform", "azure", "type"), instanceType, err.Error()))
 			continue