pkg/tfvars: Pull from []Machine instead of InstallConfig

The previous implementation pulled from the install-config, but that
missed downstream changes like AWS instance type defaults being
applied in pkg/asset/machines.  With this commit, we pull that
information from the cluster-API types, since they're the last touch
point for that data.  Some remaining information still comes from the
InstallConfig, but I've split it into explicit arguments to avoid
confusion about where data is coming from when InstallConfig's machine
pools, etc. overlap with clusterapi.Machine fields.

Unfortunately, Master.Machines is not loadable.  This commit fixes the
"Go defaults applied to Master.Machines do not affect Terraform" issue
(which is good), but it will not fix the "I ran create manifests and
edited openshift/99_openshift-cluster-api_master-machines.yaml"
because that is loaded back in downstream of the Master asset.  We'll
address that in follow-up work.

This commit also splits the platform-specific Terraform variable
generation into separate functions generating separate files.  I've
used *.auto.tfvars filenames because Terraform loads those
automatically from the current directory [1].  But that only helps
folks who are trying to run our generated Terraform by hand; as
described in d19cad5 (destroy/bootstrap: explicit pass
`disable-bootstrap.tfvars` for libvirt, 2018-12-13, #900), the
installer runs outside the Terraform directory and needs to pass this
through to Terraform explicitly.

The code copying the platform-specific Terraform variables file down
into the temporary directory for bootstrap deletion has an IsNotExist
check.  At the moment, all of our platforms except for 'none' generate
a platform-specific Terraform variables file.  But we're starting to
move towards "treat platforms you don't recognize as no-ops" in most
locations (e.g. [2]), so we have the check to make "I guess there
wasn't a platform-specific Terraform variables file for this platform"
non-fatal.

I'd prefer if FileList could be an internal property (.files?), but we
currently require public fields for reloading from disk during
multiple-invocation creation [3].

[1]: https://www.terraform.io/docs/configuration/variables.html#variable-files
[2]: https://github.com/openshift/installer/pull/1112/files#diff-6532666297c6115f7774f93ebab396c4R156
[3]: #792 (comment)

pkg/asset/cluster/cluster.go

@@ -1,6 +1,7 @@
 package cluster
 
 import (
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -66,9 +67,12 @@ func (c *Cluster) Generate(parents asset.Parents) (err error) {
 	}
 	defer os.RemoveAll(tmpDir)
 
-	terraformVariablesFile := terraformVariables.Files()[0]
-	if err := ioutil.WriteFile(filepath.Join(tmpDir, terraformVariablesFile.Filename), terraformVariablesFile.Data, 0600); err != nil {
-		return errors.Wrap(err, "failed to write terraform.tfvars file")
+	extraArgs := []string{}
+	for _, file := range terraformVariables.Files() {
+		if err := ioutil.WriteFile(filepath.Join(tmpDir, file.Filename), file.Data, 0600); err != nil {
+			return err
+		}
+		extraArgs = append(extraArgs, fmt.Sprintf("-var-file=%s", filepath.Join(tmpDir, file.Filename)))
 	}
 
 	c.FileList = []*asset.File{
@@ -79,7 +83,7 @@ func (c *Cluster) Generate(parents asset.Parents) (err error) {
 	}
 
 	logrus.Infof("Creating cluster...")
-	stateFile, err := terraform.Apply(tmpDir, installConfig.Config.Platform.Name())
+	stateFile, err := terraform.Apply(tmpDir, installConfig.Config.Platform.Name(), extraArgs...)
 	if err != nil {
 		err = errors.Wrap(err, "failed to create cluster")
 	}

pkg/asset/cluster/tfvars.go

@@ -1,27 +1,47 @@
 package cluster
 
 import (
+	"fmt"
 	"os"
 
+	libvirtprovider "github.com/openshift/cluster-api-provider-libvirt/pkg/apis/libvirtproviderconfig/v1alpha1"
 	"github.com/openshift/installer/pkg/asset"
 	"github.com/openshift/installer/pkg/asset/ignition/bootstrap"
 	"github.com/openshift/installer/pkg/asset/ignition/machine"
 	"github.com/openshift/installer/pkg/asset/installconfig"
+	"github.com/openshift/installer/pkg/asset/machines"
 	"github.com/openshift/installer/pkg/asset/rhcos"
 	"github.com/openshift/installer/pkg/tfvars"
+	awstfvars "github.com/openshift/installer/pkg/tfvars/aws"
+	libvirttfvars "github.com/openshift/installer/pkg/tfvars/libvirt"
+	openstacktfvars "github.com/openshift/installer/pkg/tfvars/openstack"
+	"github.com/openshift/installer/pkg/types/aws"
+	"github.com/openshift/installer/pkg/types/libvirt"
+	"github.com/openshift/installer/pkg/types/none"
+	"github.com/openshift/installer/pkg/types/openstack"
 	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
+	awsprovider "sigs.k8s.io/cluster-api-provider-aws/pkg/apis/awsproviderconfig/v1alpha1"
+	openstackprovider "sigs.k8s.io/cluster-api-provider-openstack/pkg/apis/openstackproviderconfig/v1alpha1"
 )
 
 const (
 	// TfVarsFileName is the filename for Terraform variables.
-	TfVarsFileName  = "terraform.tfvars"
+	TfVarsFileName = "terraform.tfvars"
+
+	// TfPlatformVarsFileName is a template for platform-specific
+	// Terraform variable files.
+	//
+	// https://www.terraform.io/docs/configuration/variables.html#variable-files
+	TfPlatformVarsFileName = "terraform.%s.auto.tfvars"
+
 	tfvarsAssetName = "Terraform Variables"
 )
 
 // TerraformVariables depends on InstallConfig and
 // Ignition to generate the terrafor.tfvars.
 type TerraformVariables struct {
-	File *asset.File
+	FileList []*asset.File
 }
 
 var _ asset.WritableAsset = (*TerraformVariables)(nil)
@@ -39,39 +59,98 @@ func (t *TerraformVariables) Dependencies() []asset.Asset {
 		new(rhcos.Image),
 		&bootstrap.Bootstrap{},
 		&machine.Master{},
+		&machines.Master{},
 	}
 }
 
 // Generate generates the terraform.tfvars file.
 func (t *TerraformVariables) Generate(parents asset.Parents) error {
 	clusterID := &installconfig.ClusterID{}
 	installConfig := &installconfig.InstallConfig{}
-	bootstrap := &bootstrap.Bootstrap{}
-	master := &machine.Master{}
+	bootstrapIgnAsset := &bootstrap.Bootstrap{}
+	masterIgnAsset := &machine.Master{}
+	mastersAsset := &machines.Master{}
 	rhcosImage := new(rhcos.Image)
-	parents.Get(clusterID, installConfig, bootstrap, master, rhcosImage)
+	parents.Get(clusterID, installConfig, bootstrapIgnAsset, masterIgnAsset, mastersAsset, rhcosImage)
 
-	bootstrapIgn := string(bootstrap.Files()[0].Data)
-	masterIgn := string(master.Files()[0].Data)
+	bootstrapIgn := string(bootstrapIgnAsset.Files()[0].Data)
+	masterIgn := string(masterIgnAsset.Files()[0].Data)
 
-	data, err := tfvars.TFVars(clusterID.ClusterID, installConfig.Config, string(*rhcosImage), bootstrapIgn, masterIgn)
+	data, err := tfvars.TFVars(
+		clusterID.ClusterID,
+		installConfig.Config.ObjectMeta.Name,
+		installConfig.Config.BaseDomain,
+		&installConfig.Config.Networking.MachineCIDR.IPNet,
+		bootstrapIgn,
+		masterIgn,
+		len(mastersAsset.Machines),
+	)
 	if err != nil {
-		return errors.Wrap(err, "failed to get Tfvars")
+		return errors.Wrap(err, "failed to get Terraform variables")
 	}
-	t.File = &asset.File{
-		Filename: TfVarsFileName,
-		Data:     data,
+	t.FileList = []*asset.File{
+		{
+			Filename: TfVarsFileName,
+			Data:     data,
+		},
+	}
+
+	if len(mastersAsset.Machines) == 0 {
+		return errors.Errorf("master slice cannot be empty")
+	}
+
+	switch platform := installConfig.Config.Platform.Name(); platform {
+	case aws.Name:
+		data, err = awstfvars.TFVars(
+			mastersAsset.Machines[0].Spec.ProviderSpec.Value.Object.(*awsprovider.AWSMachineProviderConfig),
+		)
+		if err != nil {
+			return errors.Wrapf(err, "failed to get %s Terraform variables", platform)
+		}
+		t.FileList = append(t.FileList, &asset.File{
+			Filename: fmt.Sprintf(TfPlatformVarsFileName, platform),
+			Data:     data,
+		})
+	case libvirt.Name:
+		data, err = libvirttfvars.TFVars(
+			mastersAsset.Machines[0].Spec.ProviderSpec.Value.Object.(*libvirtprovider.LibvirtMachineProviderConfig),
+			string(*rhcosImage),
+			&installConfig.Config.Networking.MachineCIDR.IPNet,
+			installConfig.Config.Platform.Libvirt.Network.IfName,
+			len(mastersAsset.Machines),
+		)
+		if err != nil {
+			return errors.Wrapf(err, "failed to get %s Terraform variables", platform)
+		}
+		t.FileList = append(t.FileList, &asset.File{
+			Filename: fmt.Sprintf(TfPlatformVarsFileName, platform),
+			Data:     data,
+		})
+	case none.Name:
+	case openstack.Name:
+		data, err = openstacktfvars.TFVars(
+			mastersAsset.Machines[0].Spec.ProviderSpec.Value.Object.(*openstackprovider.OpenstackProviderSpec),
+			installConfig.Config.Platform.OpenStack.Region,
+			installConfig.Config.Platform.OpenStack.ExternalNetwork,
+			installConfig.Config.Platform.OpenStack.TrunkSupport,
+		)
+		if err != nil {
+			return errors.Wrapf(err, "failed to get %s Terraform variables", platform)
+		}
+		t.FileList = append(t.FileList, &asset.File{
+			Filename: fmt.Sprintf(TfPlatformVarsFileName, platform),
+			Data:     data,
+		})
+	default:
+		logrus.Warnf("unrecognized platform %s", platform)
 	}
 
 	return nil
 }
 
 // Files returns the files generated by the asset.
 func (t *TerraformVariables) Files() []*asset.File {
-	if t.File != nil {
-		return []*asset.File{t.File}
-	}
-	return []*asset.File{}
+	return t.FileList
 }
 
 // Load reads the terraform.tfvars from disk.
@@ -83,7 +162,13 @@ func (t *TerraformVariables) Load(f asset.FileFetcher) (found bool, err error) {
 		}
 		return false, err
 	}
+	t.FileList = []*asset.File{file}
+
+	fileList, err := f.FetchByPattern(fmt.Sprintf(TfPlatformVarsFileName, "*"))
+	if err != nil {
+		return false, err
+	}
+	t.FileList = append(t.FileList, fileList...)
 
-	t.File = file
 	return true, nil
 }

pkg/asset/machines/master.go

@@ -3,10 +3,7 @@ package machines
 import (
 	"fmt"
 
-	"github.com/ghodss/yaml"
 	"github.com/pkg/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
 	clusterapi "sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1"
 
 	"github.com/openshift/installer/pkg/asset"
@@ -25,7 +22,7 @@ import (
 
 // Master generates the machines for the `master` machine pool.
 type Master struct {
-	MachinesRaw       []byte
+	Machines          []clusterapi.Machine
 	UserDataSecretRaw []byte
 }
 
@@ -82,57 +79,36 @@ func (m *Master) Generate(dependencies asset.Parents) error {
 			mpool.Zones = azs
 		}
 		pool.Platform.AWS = &mpool
-		machines, err := aws.Machines(clusterID.ClusterID, ic, &pool, string(*rhcosImage), "master", "master-user-data")
+		m.Machines, err = aws.Machines(clusterID.ClusterID, ic, &pool, string(*rhcosImage), "master", "master-user-data")
 		if err != nil {
 			return errors.Wrap(err, "failed to create master machine objects")
 		}
-		aws.ConfigMasters(machines, ic.ObjectMeta.Name)
-
-		list := listFromMachines(machines)
-		raw, err := yaml.Marshal(list)
-		if err != nil {
-			return errors.Wrap(err, "failed to marshal")
-		}
-		m.MachinesRaw = raw
+		aws.ConfigMasters(m.Machines, ic.ObjectMeta.Name)
 	case libvirttypes.Name:
 		mpool := defaultLibvirtMachinePoolPlatform()
 		mpool.Set(ic.Platform.Libvirt.DefaultMachinePlatform)
 		mpool.Set(pool.Platform.Libvirt)
 		pool.Platform.Libvirt = &mpool
-		machines, err := libvirt.Machines(clusterID.ClusterID, ic, &pool, "master", "master-user-data")
+		m.Machines, err = libvirt.Machines(clusterID.ClusterID, ic, &pool, "master", "master-user-data")
 		if err != nil {
 			return errors.Wrap(err, "failed to create master machine objects")
 		}
-
-		list := listFromMachines(machines)
-		raw, err := yaml.Marshal(list)
-		if err != nil {
-			return errors.Wrap(err, "failed to marshal")
-		}
-		m.MachinesRaw = raw
 	case nonetypes.Name:
 		// This is needed to ensure that roundtrip generate-load tests pass when
 		// comparing this value. Otherwise, generate will use a nil value while
-		// load will use an empty byte slice.
-		m.MachinesRaw = []byte{}
+		// load will use an empty slice.
+		m.Machines = []clusterapi.Machine{}
 	case openstacktypes.Name:
 		mpool := defaultOpenStackMachinePoolPlatform(ic.Platform.OpenStack.FlavorName)
 		mpool.Set(ic.Platform.OpenStack.DefaultMachinePlatform)
 		mpool.Set(pool.Platform.OpenStack)
 		pool.Platform.OpenStack = &mpool
 
-		machines, err := openstack.Machines(clusterID.ClusterID, ic, &pool, string(*rhcosImage), "master", "master-user-data")
+		m.Machines, err = openstack.Machines(clusterID.ClusterID, ic, &pool, string(*rhcosImage), "master", "master-user-data")
 		if err != nil {
 			return errors.Wrap(err, "failed to create master machine objects")
 		}
-		openstack.ConfigMasters(machines, ic.ObjectMeta.Name)
-
-		list := listFromMachines(machines)
-		m.MachinesRaw, err = yaml.Marshal(list)
-		if err != nil {
-			return errors.Wrap(err, "failed to marshal")
-		}
-
+		openstack.ConfigMasters(m.Machines, ic.ObjectMeta.Name)
 	default:
 		return fmt.Errorf("invalid Platform")
 	}
@@ -147,16 +123,3 @@ func masterPool(pools []types.MachinePool) types.MachinePool {
 	}
 	return types.MachinePool{}
 }
-
-func listFromMachines(objs []clusterapi.Machine) *metav1.List {
-	list := &metav1.List{
-		TypeMeta: metav1.TypeMeta{
-			APIVersion: "v1",
-			Kind:       "List",
-		},
-	}
-	for idx := range objs {
-		list.Items = append(list.Items, runtime.RawExtension{Object: &objs[idx]})
-	}
-	return list
-}

pkg/asset/manifests/openshift.go

@@ -12,15 +12,19 @@ import (
 	// clientconfig out of go-yaml. We'll use it here
 	// until that happens.
 	// https://github.com/openshift/installer/pull/854
-	"gopkg.in/yaml.v2"
+	goyaml "gopkg.in/yaml.v2"
 
+	"github.com/ghodss/yaml"
 	"github.com/gophercloud/utils/openstack/clientconfig"
 	"github.com/openshift/installer/pkg/asset"
 	"github.com/openshift/installer/pkg/asset/installconfig"
 	"github.com/openshift/installer/pkg/asset/machines"
 	osmachine "github.com/openshift/installer/pkg/asset/machines/openstack"
 	"github.com/openshift/installer/pkg/asset/password"
 	"github.com/openshift/installer/pkg/asset/templates/content/openshift"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	clusterapi "sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1"
 )
 
 const (
@@ -95,7 +99,7 @@ func (o *Openshift) Generate(dependencies asset.Parents) error {
 			osmachine.CloudName: cloud,
 		}
 
-		marshalled, err := yaml.Marshal(clouds)
+		marshalled, err := goyaml.Marshal(clouds)
 		if err != nil {
 			return err
 		}
@@ -122,11 +126,17 @@ func (o *Openshift) Generate(dependencies asset.Parents) error {
 		cloudCredsSecret,
 		kubeadminPasswordSecret,
 		roleCloudCredsSecretReader)
+
+	masterMachines, err := listFromMachines(master.Machines)
+	if err != nil {
+		return err
+	}
+
 	assetData := map[string][]byte{
 		"99_binding-discovery.yaml":                             []byte(bindingDiscovery.Files()[0].Data),
 		"99_kubeadmin-password-secret.yaml":                     applyTemplateData(kubeadminPasswordSecret.Files()[0].Data, templateData),
 		"99_openshift-cluster-api_cluster.yaml":                 clusterk8sio.Raw,
-		"99_openshift-cluster-api_master-machines.yaml":         master.MachinesRaw,
+		"99_openshift-cluster-api_master-machines.yaml":         masterMachines,
 		"99_openshift-cluster-api_master-user-data-secret.yaml": master.UserDataSecretRaw,
 		"99_openshift-cluster-api_worker-machineset.yaml":       worker.MachineSetRaw,
 		"99_openshift-cluster-api_worker-user-data-secret.yaml": worker.UserDataSecretRaw,
@@ -166,3 +176,17 @@ func (o *Openshift) Load(f asset.FileFetcher) (bool, error) {
 	asset.SortFiles(o.FileList)
 	return len(fileList) > 0, nil
 }
+
+func listFromMachines(objs []clusterapi.Machine) ([]byte, error) {
+	list := &metav1.List{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: "v1",
+			Kind:       "List",
+		},
+	}
+	for idx := range objs {
+		list.Items = append(list.Items, runtime.RawExtension{Object: &objs[idx]})
+	}
+
+	return yaml.Marshal(list)
+}