Skip to content

Commit

Permalink
pkg/tfvars: Pull from []Machine instead of InstallConfig
Browse files Browse the repository at this point in the history 
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, openshift#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]: openshift#792 (comment)
  • Loading branch information
@wking
wking committed Feb 7, 2019
1 parent 1ea94b1 commit 3326b6b
Show file tree
Hide file tree
Showing 12 changed files with 324 additions and 277 deletions.
12 changes: 8 additions & 4 deletions pkg/asset/cluster/cluster.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package cluster

import (
"fmt"
"io/ioutil"
"os"
"path/filepath"
Expand Down Expand Up @@ -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{
Expand All @@ -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")
}
Expand Down
123 changes: 106 additions & 17 deletions pkg/asset/cluster/tfvars.go
View file
Original file line number Diff line number Diff line change
@@ -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)
Expand All @@ -39,39 +59,102 @@ 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)
masterCount := len(mastersAsset.Machines)
if mastersAsset.Machines == nil {
masterCount = len(mastersAsset.MachinesDeprecated)
}
data, err := tfvars.TFVars(
clusterID.ClusterID,
installConfig.Config.ObjectMeta.Name,
installConfig.Config.BaseDomain,
&installConfig.Config.Networking.MachineCIDR.IPNet,
bootstrapIgn,
masterIgn,
masterCount,
)
if err != nil {
return errors.Wrap(err, "failed to get Tfvars")
return errors.Wrap(err, "failed to get Terraform variables")
}
t.FileList = []*asset.File{
{
Filename: TfVarsFileName,
Data: data,
},
}
t.File = &asset.File{
Filename: TfVarsFileName,
Data: data,

if masterCount == 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.MachinesDeprecated[0].Spec.ProviderSpec.Value.Object.(*libvirtprovider.LibvirtMachineProviderConfig),
string(*rhcosImage),
&installConfig.Config.Networking.MachineCIDR.IPNet,
installConfig.Config.Platform.Libvirt.Network.IfName,
masterCount,
)
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.MachinesDeprecated[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.
Expand All @@ -83,7 +166,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
}
69 changes: 10 additions & 59 deletions pkg/asset/machines/master.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,8 @@ package machines
import (
"fmt"

"github.com/ghodss/yaml"
machineapi "github.com/openshift/cluster-api/pkg/apis/machine/v1beta1"
"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"
Expand All @@ -26,8 +23,9 @@ import (

// Master generates the machines for the `master` machine pool.
type Master struct {
MachinesRaw []byte
UserDataSecretRaw []byte
Machines []machineapi.Machine
MachinesDeprecated []clusterapi.Machine
UserDataSecretRaw []byte
}

var _ asset.Asset = (*Master)(nil)
Expand Down Expand Up @@ -83,57 +81,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.MachinesDeprecated, 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 := listFromMachinesDeprecated(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 = []machineapi.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.MachinesDeprecated, 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 := listFromMachinesDeprecated(machines)
m.MachinesRaw, err = yaml.Marshal(list)
if err != nil {
return errors.Wrap(err, "failed to marshal")
}

openstack.ConfigMasters(m.MachinesDeprecated, ic.ObjectMeta.Name)
default:
return fmt.Errorf("invalid Platform")
}
Expand All @@ -148,29 +125,3 @@ func masterPool(pools []types.MachinePool) types.MachinePool {
}
return types.MachinePool{}
}

func listFromMachines(objs []machineapi.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
}

func listFromMachinesDeprecated(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
}
Loading

0 comments on commit 3326b6b

Please sign in to comment.