HIVE-2391: VSphere zonal part deux

Followon addressing review from #2731.

MachinePool:
- Removed `Topology` override
- Restored ResourcePool and TagIDs overrides
- Removed `osImage` detected from arbitrary master; using whatever's
  passed through from FD Topology (which defaults sanely if unset).

Deprovision:
- Changed `--vsphere-vcenter` StringVar to `--vsphere-vcenters`
  StringSliceVar

Platform Creds:
- Redesigned to take `vcenters`, a list of vcenter
  server/username/password, matching (and unmarshaling into) the
  corresponding chunk of metadata.json.

Docs:
- Updated install-config sample to zonal shape.
- Documented new creds shape.

Author: 2uasimojo

apis/hive/v1/clusterdeprovision_types.go

@@ -133,9 +133,11 @@ type VSphereClusterDeprovision struct {
 	// necessary for communicating with the VCenter.
 	CertificatesSecretRef corev1.LocalObjectReference `json:"certificatesSecretRef"`
 	// DeprecatedVCenter is the vSphere vCenter hostname.
-	// Deprecated: use VCenters instead
+	// Deprecated: use VCenters instead.
+	// +optional
 	DeprecatedVCenter string `json:"vCenter"`
 	// VCenters are potentially multiple vCenter hostnames. Prefer this field over VCenter.
+	// +optional
 	VCenters []string `json:"vCenters"`
 }
 

apis/hive/v1/vsphere/machinepools.go

@@ -9,17 +9,10 @@ type MachinePool struct {
 
 	// ResourcePool is the name of the resource pool that will be used for virtual machines.
 	// If it is not present, a default value will be used.
-	// Deprecated: use Topology instead
 	// +optional
-	DeprecatedResourcePool string `json:"resourcePool,omitempty"`
+	ResourcePool string `json:"resourcePool,omitempty"`
 
 	// TagIDs is a list of up to 10 tags to add to the VMs that this machine set provisions in vSphere.
-	// Deprecated: use Topology instead
 	// +kubebuilder:validation:MaxItems:=10
-	DeprecatedTagIDs []string `json:"tagIDs,omitempty"`
-
-	// Topology is the vSphere topology that will be used for virtual machines.
-	// If it is not present, a default value will be used.
-	// +optional
-	Topology *vsphere.Topology `json:"topology,omitempty"`
+	TagIDs []string `json:"tagIDs,omitempty"`
 }

apis/hive/v1/vsphere/zz_generated.deepcopy.go

@@ -355,7 +355,7 @@ spec:
                         vCenter:
                           description: |-
                             DeprecatedVCenter is the vSphere vCenter hostname.
-                            Deprecated: use VCenters instead
+                            Deprecated: use VCenters instead.
                           type: string
                         vCenters:
                           description: VCenters are potentially multiple vCenter hostnames. Prefer this field over VCenter.
@@ -365,8 +365,6 @@ spec:
                       required:
                         - certificatesSecretRef
                         - credentialsSecretRef
-                        - vCenter
-                        - vCenters
                       type: object
                   type: object
               required:

config/crds/hive.openshift.io_clusterdeprovisions.yaml

@@ -844,94 +844,13 @@ spec:
                           description: |-
                             ResourcePool is the name of the resource pool that will be used for virtual machines.
                             If it is not present, a default value will be used.
-                            Deprecated: use Topology instead
                           type: string
                         tagIDs:
-                          description: |-
-                            TagIDs is a list of up to 10 tags to add to the VMs that this machine set provisions in vSphere.
-                            Deprecated: use Topology instead
+                          description: TagIDs is a list of up to 10 tags to add to the VMs that this machine set provisions in vSphere.
                           items:
                             type: string
                           maxItems: 10
                           type: array
-                        topology:
-                          description: |-
-                            Topology is the vSphere topology that will be used for virtual machines.
-                            If it is not present, a default value will be used.
-                          properties:
-                            computeCluster:
-                              description: |-
-                                computeCluster as the failure domain
-                                This is required to be a path
-                              maxLength: 2048
-                              minLength: 1
-                              type: string
-                            datacenter:
-                              description: |-
-                                datacenter is the vCenter datacenter in which virtual machines will be located
-                                and defined as the failure domain.
-                              maxLength: 80
-                              minLength: 1
-                              type: string
-                            datastore:
-                              description: |-
-                                datastore is the name or inventory path of the datastore in which the
-                                virtual machine is created/located.
-                              maxLength: 2048
-                              minLength: 1
-                              type: string
-                            folder:
-                              description: |-
-                                folder is the inventory path of the folder in which the
-                                virtual machine is created/located.
-                              maxLength: 2048
-                              minLength: 1
-                              pattern: ^/.*?/vm/.*?
-                              type: string
-                            hostGroup:
-                              description: |-
-                                hostGroup is the name of the vm-host group of type host within vCenter for this failure domain.
-                                hostGroup is limited to 80 characters.
-                                This field is required when the ZoneType is HostGroup
-                              maxLength: 80
-                              type: string
-                            networks:
-                              description: networks is the list of networks within this failure domain
-                              items:
-                                type: string
-                              maxItems: 10
-                              minItems: 1
-                              type: array
-                            resourcePool:
-                              description: |-
-                                resourcePool is the absolute path of the resource pool where virtual machines will be
-                                created. The absolute path is of the form /<datacenter>/host/<cluster>/Resources/<resourcepool>.
-                              maxLength: 2048
-                              minLength: 1
-                              pattern: ^/.*?/host/.*?/Resources.*
-                              type: string
-                            tagIDs:
-                              description: |-
-                                tagIDs is an optional set of tags to add to an instance. Specified tagIDs
-                                must use URN-notation instead of display names. A maximum of 10 tag IDs may be specified.
-                              example: urn:vmomi:InventoryServiceTag:5736bf56-49f5-4667-b38c-b97e09dc9578:GLOBAL
-                              items:
-                                type: string
-                              type: array
-                            template:
-                              description: |-
-                                template is the inventory path of the virtual machine or template
-                                that will be used for cloning.
-                              maxLength: 2048
-                              minLength: 1
-                              pattern: ^/.*?/vm/.*?
-                              type: string
-                          required:
-                            - computeCluster
-                            - datacenter
-                            - datastore
-                            - networks
-                          type: object
                         zones:
                           description: |-
                             Zones defines available zones

config/crds/hive.openshift.io_machinepools.yaml

@@ -31,8 +31,10 @@ import (
 	openstackcreds "github.com/openshift/hive/pkg/creds/openstack"
 	"github.com/openshift/hive/pkg/gcpclient"
 	"github.com/openshift/hive/pkg/util/scheme"
+
 	installertypes "github.com/openshift/installer/pkg/types"
 	installervsphere "github.com/openshift/installer/pkg/types/vsphere"
+	"github.com/openshift/installer/pkg/types/vsphere/conversion"
 	"github.com/openshift/installer/pkg/validate"
 )
 
@@ -346,6 +348,7 @@ OpenShift Installer publishes all the services of the cluster like API server an
 	flags.StringVar(&opt.OpenStackIngressFloatingIP, "openstack-ingress-floating-ip", "", "Floating IP address to use for cluster's Ingress service")
 
 	// vSphere flags
+	// TODO: This needs a full rewrite for zonal.
 	flags.StringVar(&opt.VSphereVCenter, "vsphere-vcenter", "", "Domain name or IP address of the vCenter")
 	flags.StringVar(&opt.VSphereDatacenter, "vsphere-datacenter", "", "Datacenter to use in the vCenter")
 	flags.StringVar(&opt.VSphereDefaultDataStore, "vsphere-default-datastore", "", "Default datastore to use for provisioning volumes")
@@ -731,15 +734,23 @@ func (o *Options) GenerateObjects() ([]runtime.Object, error) {
 		}
 		builder.CloudBuilder = openStackProvider
 	case constants.PlatformVSphere:
-		vsphereUsername := os.Getenv(constants.VSphereUsernameEnvVar)
-		if vsphereUsername == "" {
-			return nil, fmt.Errorf("no %s env var set, cannot proceed", constants.VSphereUsernameEnvVar)
+		// Little DRYer to set a username or password string (which one should be indicated via `label`)
+		// to `value` iff not already set in `receiver`.
+		// If the `receiver` and `value` are both unset, generate an error.
+		// This is for convenience as various paths need to do this conditionally, and the values could
+		// come from multiple places (env vars, json blob).
+		setCredValue := func(label string, receiver *string, value string) error {
+			if *receiver != "" {
+				return nil
+			}
+			if value == "" {
+				return fmt.Errorf("Missing VSphere %s: must be provided either via env var or platform spec.", label)
+			}
+			*receiver = value
+			return nil
 		}
-
+		vsphereUsername := os.Getenv(constants.VSphereUsernameEnvVar)
 		vspherePassword := os.Getenv(constants.VSpherePasswordEnvVar)
-		if vspherePassword == "" {
-			return nil, fmt.Errorf("no %s env var set, cannot proceed", constants.VSpherePasswordEnvVar)
-		}
 
 		vsphereCACerts := os.Getenv(constants.VSphereTLSCACertsEnvVar)
 		if o.VSphereCACerts != "" {
@@ -767,16 +778,7 @@ func (o *Options) GenerateObjects() ([]runtime.Object, error) {
 			if err != nil {
 				return nil, fmt.Errorf("error decoding platform %s: %w", o.VSpherePlatformSpecJSON, err)
 			}
-
-			// Set credentials on VCenters if using new structure
-			for i := range platform.VCenters {
-				if platform.VCenters[i].Username == "" {
-					platform.VCenters[i].Username = vsphereUsername
-				}
-				if platform.VCenters[i].Password == "" {
-					platform.VCenters[i].Password = vspherePassword
-				}
-			}
+			// We'll inject credentials from env vars later, if unset in the json blob
 		} else {
 			o.log.Info("Platform spec not provided, trying legacy flags")
 			// Try legacy flags
@@ -810,39 +812,74 @@ func (o *Options) GenerateObjects() ([]runtime.Object, error) {
 				return nil, fmt.Errorf("must provide --vsphere-vcenter or set %s env var", constants.VSphereVCenterEnvVar)
 			}
 
-			platform.DeprecatedUsername = vsphereUsername
-			platform.DeprecatedPassword = vspherePassword
+			if err := setCredValue("username", &platform.DeprecatedUsername, vsphereUsername); err != nil {
+				return nil, err
+			}
+			if err := setCredValue("password", &platform.DeprecatedPassword, vspherePassword); err != nil {
+				return nil, err
+			}
 			platform.DeprecatedNetwork = vSphereNetwork
 			platform.DeprecatedDatacenter = vSphereDatacenter
 			platform.DeprecatedDefaultDatastore = vSphereDatastore
 			platform.DeprecatedVCenter = vSphereVCenter
 			platform.DeprecatedFolder = o.VSphereFolder
 			platform.DeprecatedCluster = o.VSphereCluster
+			dummyInstallConfig := &installertypes.InstallConfig{
+				Platform: installertypes.Platform{
+					VSphere: &platform,
+				},
+			}
+			if err := conversion.ConvertInstallConfig(dummyInstallConfig); err != nil {
+				return nil, fmt.Errorf("failed to update deprecated vSphere fields")
+			}
 		}
 
+		// Build a new-style creds object
+		vcenterCreds := []installervsphere.VCenters{}
 		for i := range platform.VCenters {
-			if platform.VCenters[i].Username == "" {
-				platform.VCenters[i].Username = vsphereUsername
-			}
-			if platform.VCenters[i].Password == "" {
-				platform.VCenters[i].Password = vspherePassword
+			// Allow a hybrid input style: creds can be included in the json blob OR individually
+			// via env vars. The former takes precedence.
+			for i := range platform.VCenters {
+				if err := setCredValue("username", &platform.VCenters[i].Username, vsphereUsername); err != nil {
+					return nil, err
+				}
+				if err := setCredValue("password", &platform.VCenters[i].Password, vspherePassword); err != nil {
+					return nil, err
+				}
 			}
+			vcenterCreds = append(vcenterCreds, installervsphere.VCenters{
+				VCenter:  platform.VCenters[i].Server,
+				Username: platform.VCenters[i].Username,
+				Password: platform.VCenters[i].Password,
+			})
+		}
+		vcenterCredsb, err := json.Marshal(vcenterCreds)
+		if err != nil {
+			return nil, errors.Wrap(err, "failed to marhsal vcenter creds list")
 		}
 
 		if len(platform.APIVIPs) == 0 {
+			if o.VSphereAPIVIP == "" {
+				return nil, fmt.Errorf("Must supply an API VIP")
+			}
 			platform.APIVIPs = []string{o.VSphereAPIVIP}
 		}
 		if len(platform.IngressVIPs) == 0 {
+			if o.VSphereIngressVIP == "" {
+				return nil, fmt.Errorf("Must supply an Ingress VIP")
+			}
 			platform.IngressVIPs = []string{o.VSphereIngressVIP}
 		}
 
-		vsphereProvider := &clusterresource.VSphereCloudBuilder{
-			Username:       vsphereUsername,
-			Password:       vspherePassword,
-			CACert:         bytes.Join(caCerts, []byte("\n")),
-			Infrastructure: &platform,
-		}
-		builder.CloudBuilder = vsphereProvider
+		builder.CloudBuilder = clusterresource.NewVSphereCloudBuilder(
+			map[string][]byte{
+				constants.UsernameSecretKey: []byte(vsphereUsername),
+				constants.PasswordSecretKey: []byte(vspherePassword),
+				"vCenters":                  vcenterCredsb,
+			},
+			bytes.Join(caCerts, []byte("\n")),
+			&platform,
+		)
 	case constants.PlatformIBMCloud:
 		ibmCloudAPIKey := os.Getenv(constants.IBMCloudAPIKeyEnvVar)
 		if ibmCloudAPIKey == "" {