Refactor installconfig.AssetBase out of InstallConfig

Split the parts of the InstallConfig asset consumed by the agent
installer out into a separate AssetBase struct, so that the agent
installer need not embed the whole of InstallConfig. This will allow us
to do different validations where necessary in the agent installer.

pkg/asset/agent/manifests/util_test.go

@@ -73,50 +73,52 @@ func getValidOptionalInstallConfig() *agent.OptionalInstallConfig {
 
 	return &agent.OptionalInstallConfig{
 		InstallConfig: installconfig.InstallConfig{
-			Config: &types.InstallConfig{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "ocp-edge-cluster-0",
-					Namespace: "cluster-0",
-				},
-				BaseDomain: "testing.com",
-				PullSecret: testSecret,
-				SSHKey:     testSSHKey,
-				ControlPlane: &types.MachinePool{
-					Name:     "master",
-					Replicas: pointer.Int64Ptr(3),
-					Platform: types.MachinePoolPlatform{},
-				},
-				Compute: []types.MachinePool{
-					{
-						Name:     "worker-machine-pool-1",
-						Replicas: pointer.Int64Ptr(2),
+			AssetBase: installconfig.AssetBase{
+				Config: &types.InstallConfig{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "ocp-edge-cluster-0",
+						Namespace: "cluster-0",
 					},
-					{
-						Name:     "worker-machine-pool-2",
+					BaseDomain: "testing.com",
+					PullSecret: testSecret,
+					SSHKey:     testSSHKey,
+					ControlPlane: &types.MachinePool{
+						Name:     "master",
 						Replicas: pointer.Int64Ptr(3),
+						Platform: types.MachinePoolPlatform{},
 					},
-				},
-				Networking: &types.Networking{
-					MachineNetwork: []types.MachineNetworkEntry{
+					Compute: []types.MachinePool{
 						{
-							CIDR: ipnet.IPNet{IPNet: *machineNetCidr},
+							Name:     "worker-machine-pool-1",
+							Replicas: pointer.Int64Ptr(2),
+						},
+						{
+							Name:     "worker-machine-pool-2",
+							Replicas: pointer.Int64Ptr(3),
 						},
 					},
-					ClusterNetwork: []types.ClusterNetworkEntry{
-						{
-							CIDR:       ipnet.IPNet{IPNet: *newCidr},
-							HostPrefix: 23,
+					Networking: &types.Networking{
+						MachineNetwork: []types.MachineNetworkEntry{
+							{
+								CIDR: ipnet.IPNet{IPNet: *machineNetCidr},
+							},
 						},
+						ClusterNetwork: []types.ClusterNetworkEntry{
+							{
+								CIDR:       ipnet.IPNet{IPNet: *newCidr},
+								HostPrefix: 23,
+							},
+						},
+						ServiceNetwork: []ipnet.IPNet{
+							*ipnet.MustParseCIDR("172.30.0.0/16"),
+						},
+						NetworkType: "OVNKubernetes",
 					},
-					ServiceNetwork: []ipnet.IPNet{
-						*ipnet.MustParseCIDR("172.30.0.0/16"),
-					},
-					NetworkType: "OVNKubernetes",
-				},
-				Platform: types.Platform{
-					BareMetal: &baremetal.Platform{
-						APIVIPs:     []string{"192.168.122.10"},
-						IngressVIPs: []string{"192.168.122.11"},
+					Platform: types.Platform{
+						BareMetal: &baremetal.Platform{
+							APIVIPs:     []string{"192.168.122.10"},
+							IngressVIPs: []string{"192.168.122.11"},
+						},
 					},
 				},
 			},
@@ -134,56 +136,58 @@ func getValidOptionalInstallConfigDualStack() *agent.OptionalInstallConfig {
 
 	return &agent.OptionalInstallConfig{
 		InstallConfig: installconfig.InstallConfig{
-			Config: &types.InstallConfig{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "ocp-edge-cluster-0",
-					Namespace: "cluster-0",
-				},
-				BaseDomain: "testing.com",
-				PullSecret: testSecret,
-				SSHKey:     testSSHKey,
-				ControlPlane: &types.MachinePool{
-					Name:     "master",
-					Replicas: pointer.Int64Ptr(3),
-					Platform: types.MachinePoolPlatform{},
-				},
-				Compute: []types.MachinePool{
-					{
-						Name:     "worker-machine-pool-1",
-						Replicas: pointer.Int64Ptr(2),
+			AssetBase: installconfig.AssetBase{
+				Config: &types.InstallConfig{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "ocp-edge-cluster-0",
+						Namespace: "cluster-0",
 					},
-					{
-						Name:     "worker-machine-pool-2",
+					BaseDomain: "testing.com",
+					PullSecret: testSecret,
+					SSHKey:     testSSHKey,
+					ControlPlane: &types.MachinePool{
+						Name:     "master",
 						Replicas: pointer.Int64Ptr(3),
+						Platform: types.MachinePoolPlatform{},
 					},
-				},
-				Networking: &types.Networking{
-					MachineNetwork: []types.MachineNetworkEntry{
+					Compute: []types.MachinePool{
 						{
-							CIDR: ipnet.IPNet{IPNet: *machineNetCidr},
+							Name:     "worker-machine-pool-1",
+							Replicas: pointer.Int64Ptr(2),
 						},
 						{
-							CIDR: ipnet.IPNet{IPNet: *machineNetCidrIPv6},
+							Name:     "worker-machine-pool-2",
+							Replicas: pointer.Int64Ptr(3),
 						},
 					},
-					ClusterNetwork: []types.ClusterNetworkEntry{
-						{
-							CIDR:       ipnet.IPNet{IPNet: *newCidr},
-							HostPrefix: 23,
+					Networking: &types.Networking{
+						MachineNetwork: []types.MachineNetworkEntry{
+							{
+								CIDR: ipnet.IPNet{IPNet: *machineNetCidr},
+							},
+							{
+								CIDR: ipnet.IPNet{IPNet: *machineNetCidrIPv6},
+							},
 						},
-						{
-							CIDR:       ipnet.IPNet{IPNet: *newCidrIPv6},
-							HostPrefix: 64,
+						ClusterNetwork: []types.ClusterNetworkEntry{
+							{
+								CIDR:       ipnet.IPNet{IPNet: *newCidr},
+								HostPrefix: 23,
+							},
+							{
+								CIDR:       ipnet.IPNet{IPNet: *newCidrIPv6},
+								HostPrefix: 64,
+							},
+						},
+						ServiceNetwork: []ipnet.IPNet{
+							*ipnet.MustParseCIDR("172.30.0.0/16"), *ipnet.MustParseCIDR("fd02::/112"),
 						},
 					},
-					ServiceNetwork: []ipnet.IPNet{
-						*ipnet.MustParseCIDR("172.30.0.0/16"), *ipnet.MustParseCIDR("fd02::/112"),
-					},
-				},
-				Platform: types.Platform{
-					BareMetal: &baremetal.Platform{
-						APIVIPs:     []string{"192.168.122.10"},
-						IngressVIPs: []string{"192.168.122.11"},
+					Platform: types.Platform{
+						BareMetal: &baremetal.Platform{
+							APIVIPs:     []string{"192.168.122.10"},
+							IngressVIPs: []string{"192.168.122.11"},
+						},
 					},
 				},
 			},

pkg/asset/agent/mirror/cabundle_test.go

@@ -36,9 +36,11 @@ func TestCaBundle_Generate(t *testing.T) {
 				&agent.OptionalInstallConfig{
 					Supplied: true,
 					InstallConfig: installconfig.InstallConfig{
-						Config: &types.InstallConfig{
-							ObjectMeta: v1.ObjectMeta{
-								Namespace: "cluster-0",
+						AssetBase: installconfig.AssetBase{
+							Config: &types.InstallConfig{
+								ObjectMeta: v1.ObjectMeta{
+									Namespace: "cluster-0",
+								},
 							},
 						},
 					},
@@ -51,11 +53,12 @@ func TestCaBundle_Generate(t *testing.T) {
 				&agent.OptionalInstallConfig{
 					Supplied: true,
 					InstallConfig: installconfig.InstallConfig{
-						Config: &types.InstallConfig{
-							ObjectMeta: v1.ObjectMeta{
-								Namespace: "cluster-0",
-							},
-							AdditionalTrustBundle: `
+						AssetBase: installconfig.AssetBase{
+							Config: &types.InstallConfig{
+								ObjectMeta: v1.ObjectMeta{
+									Namespace: "cluster-0",
+								},
+								AdditionalTrustBundle: `
 -----BEGIN CERTIFICATE-----
 MIIDZTCCAk2gAwIBAgIURbA8lR+5xlJZUoOXK66AHFWd3uswDQYJKoZIhvcNAQEL
 BQAwQjELMAkGA1UEBhMCWFgxFTATBgNVBAcMDERlZmF1bHQgQ2l0eTEcMBoGA1UE
@@ -78,6 +81,7 @@ S655uiFW5AX2wDVUcQEDCOiEn6SI9DTt5oQjWPMxPf+rEyfQ2f1QwVez7cyr6Qc5
 OIUk31HnM/Fj
 -----END CERTIFICATE-----
 `,
+							},
 						},
 					},
 				},

pkg/asset/agent/mirror/registriesconf_test.go

@@ -38,9 +38,11 @@ func TestRegistriesConf_Generate(t *testing.T) {
 				&agent.OptionalInstallConfig{
 					Supplied: true,
 					InstallConfig: installconfig.InstallConfig{
-						Config: &types.InstallConfig{
-							ObjectMeta: v1.ObjectMeta{
-								Namespace: "cluster-0",
+						AssetBase: installconfig.AssetBase{
+							Config: &types.InstallConfig{
+								ObjectMeta: v1.ObjectMeta{
+									Namespace: "cluster-0",
+								},
 							},
 						},
 					},
@@ -57,21 +59,23 @@ func TestRegistriesConf_Generate(t *testing.T) {
 				&agent.OptionalInstallConfig{
 					Supplied: true,
 					InstallConfig: installconfig.InstallConfig{
-						Config: &types.InstallConfig{
-							ObjectMeta: v1.ObjectMeta{
-								Namespace: "cluster-0",
-							},
-							ImageContentSources: []types.ImageContentSource{
-								{
-									Source: "registry.ci.openshift.org/origin/release",
-									Mirrors: []string{
-										"virthost.ostest.test.metalkube.org:5000/localimages/local-release-image",
-									},
+						AssetBase: installconfig.AssetBase{
+							Config: &types.InstallConfig{
+								ObjectMeta: v1.ObjectMeta{
+									Namespace: "cluster-0",
 								},
-								{
-									Source: "quay.io/openshift-release-dev/ocp-v4.0-art-dev",
-									Mirrors: []string{
-										"virthost.ostest.test.metalkube.org:5000/localimages/local-release-image",
+								ImageContentSources: []types.ImageContentSource{
+									{
+										Source: "registry.ci.openshift.org/origin/release",
+										Mirrors: []string{
+											"virthost.ostest.test.metalkube.org:5000/localimages/local-release-image",
+										},
+									},
+									{
+										Source: "quay.io/openshift-release-dev/ocp-v4.0-art-dev",
+										Mirrors: []string{
+											"virthost.ostest.test.metalkube.org:5000/localimages/local-release-image",
+										},
 									},
 								},
 							},
@@ -90,21 +94,23 @@ func TestRegistriesConf_Generate(t *testing.T) {
 				&agent.OptionalInstallConfig{
 					Supplied: true,
 					InstallConfig: installconfig.InstallConfig{
-						Config: &types.InstallConfig{
-							ObjectMeta: v1.ObjectMeta{
-								Namespace: "cluster-0",
-							},
-							ImageContentSources: []types.ImageContentSource{
-								{
-									Source: "registry.ci.openshift.org/ocp/release",
-									Mirrors: []string{
-										"virthost.ostest.test.metalkube.org:5000/localimages/local-release-image",
-									},
+						AssetBase: installconfig.AssetBase{
+							Config: &types.InstallConfig{
+								ObjectMeta: v1.ObjectMeta{
+									Namespace: "cluster-0",
 								},
-								{
-									Source: "quay.io/openshift-release-dev/ocp-v4.0-art-dev",
-									Mirrors: []string{
-										"virthost.ostest.test.metalkube.org:5000/localimages/local-release-image",
+								ImageContentSources: []types.ImageContentSource{
+									{
+										Source: "registry.ci.openshift.org/ocp/release",
+										Mirrors: []string{
+											"virthost.ostest.test.metalkube.org:5000/localimages/local-release-image",
+										},
+									},
+									{
+										Source: "quay.io/openshift-release-dev/ocp-v4.0-art-dev",
+										Mirrors: []string{
+											"virthost.ostest.test.metalkube.org:5000/localimages/local-release-image",
+										},
 									},
 								},
 							},

pkg/asset/installconfig/installconfig.go

@@ -2,12 +2,8 @@ package installconfig
 
 import (
 	"context"
-	"os"
-	"strings"
 
-	"github.com/ghodss/yaml"
 	"github.com/pkg/errors"
-	"github.com/sirupsen/logrus"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 
@@ -23,7 +19,6 @@ import (
 	icpowervs "github.com/openshift/installer/pkg/asset/installconfig/powervs"
 	icvsphere "github.com/openshift/installer/pkg/asset/installconfig/vsphere"
 	"github.com/openshift/installer/pkg/types"
-	"github.com/openshift/installer/pkg/types/conversion"
 	"github.com/openshift/installer/pkg/types/defaults"
 	"github.com/openshift/installer/pkg/types/validation"
 )
@@ -34,8 +29,7 @@ const (
 
 // InstallConfig generates the install-config.yaml file.
 type InstallConfig struct {
-	Config       *types.InstallConfig   `json:"config"`
-	File         *asset.File            `json:"file"`
+	AssetBase
 	AWS          *aws.Metadata          `json:"aws,omitempty"`
 	Azure        *icazure.Metadata      `json:"azure,omitempty"`
 	IBMCloud     *icibmcloud.Metadata   `json:"ibmcloud,omitempty"`
@@ -48,7 +42,9 @@ var _ asset.WritableAsset = (*InstallConfig)(nil)
 // MakeAsset returns an InstallConfig asset containing a given InstallConfig CR.
 func MakeAsset(config *types.InstallConfig) *InstallConfig {
 	return &InstallConfig{
-		Config: config,
+		AssetBase: AssetBase{
+			Config: config,
+		},
 	}
 }
 
@@ -111,63 +107,24 @@ func (a *InstallConfig) Generate(parents asset.Parents) error {
 	a.Config.PowerVS = platform.PowerVS
 	a.Config.Nutanix = platform.Nutanix
 
+	defaults.SetInstallConfigDefaults(a.Config)
+
 	return a.finish("")
 }
 
-// Name returns the human-friendly name of the asset.
-func (a *InstallConfig) Name() string {
-	return "Install Config"
-}
-
-// Files returns the files generated by the asset.
-func (a *InstallConfig) Files() []*asset.File {
-	if a.File != nil {
-		return []*asset.File{a.File}
-	}
-	return []*asset.File{}
-}
-
 // Load returns the installconfig from disk.
 func (a *InstallConfig) Load(f asset.FileFetcher) (found bool, err error) {
-	file, err := f.FetchByName(installConfigFilename)
-	if err != nil {
-		if os.IsNotExist(err) {
-			return false, nil
-		}
-		return false, errors.Wrap(err, asset.InstallConfigError)
-	}
-
-	config := &types.InstallConfig{}
-	if err := yaml.UnmarshalStrict(file.Data, config, yaml.DisallowUnknownFields); err != nil {
-		err = errors.Wrapf(err, "failed to unmarshal %s", installConfigFilename)
-		if !strings.Contains(err.Error(), "unknown field") {
-			return false, errors.Wrap(err, asset.InstallConfigError)
-		}
-		err = errors.Wrapf(err, "failed to parse first occurence of unknown field")
-		logrus.Warnf(err.Error())
-		logrus.Info("Attempting to unmarshal while ignoring unknown keys because strict unmarshaling failed")
-		if err = yaml.UnmarshalStrict(file.Data, config); err != nil {
-			err = errors.Wrapf(err, "failed to unmarshal %s", installConfigFilename)
+	found, err = a.LoadFromFile(f)
+	if found && err == nil {
+		if err := a.finish(installConfigFilename); err != nil {
 			return false, errors.Wrap(err, asset.InstallConfigError)
 		}
 	}
-	a.Config = config
 
-	// Upconvert any deprecated fields
-	if err := conversion.ConvertInstallConfig(a.Config); err != nil {
-		return false, errors.Wrap(errors.Wrap(err, "failed to upconvert install config"), asset.InstallConfigError)
-	}
-
-	err = a.finish(installConfigFilename)
-	if err != nil {
-		return false, errors.Wrap(err, asset.InstallConfigError)
-	}
-	return true, nil
+	return found, err
 }
 
 func (a *InstallConfig) finish(filename string) error {
-	defaults.SetInstallConfigDefaults(a.Config)
-
 	if a.Config.AWS != nil {
 		a.AWS = aws.NewMetadata(a.Config.Platform.AWS.Region, a.Config.Platform.AWS.Subnets, a.Config.AWS.ServiceEndpoints)
 	}
@@ -195,17 +152,12 @@ func (a *InstallConfig) finish(filename string) error {
 		return err
 	}
 
-	data, err := yaml.Marshal(a.Config)
-	if err != nil {
-		return errors.Wrap(err, "failed to Marshal InstallConfig")
-	}
-	a.File = &asset.File{
-		Filename: installConfigFilename,
-		Data:     data,
-	}
-	return nil
+	return a.RecordFile()
 }
 
+// platformValidation runs validations that require connecting to the
+// underlying platform. In some cases, platforms also duplicate validations
+// that have already been checked by validation.ValidateInstallConfig().
 func (a *InstallConfig) platformValidation() error {
 	if a.Config.Platform.AlibabaCloud != nil {
 		client, err := a.AlibabaCloud.Client()

pkg/asset/installconfig/installconfigbase.go

@@ -0,0 +1,84 @@
+package installconfig
+
+import (
+	"os"
+	"strings"
+
+	"github.com/ghodss/yaml"
+	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
+
+	"github.com/openshift/installer/pkg/asset"
+	"github.com/openshift/installer/pkg/types"
+	"github.com/openshift/installer/pkg/types/conversion"
+	"github.com/openshift/installer/pkg/types/defaults"
+)
+
+// AssetBase is the base structure for the separate InstallConfig assets used
+// in the agent-based and IPI/UPI installation methods.
+type AssetBase struct {
+	Config *types.InstallConfig `json:"config"`
+	File   *asset.File          `json:"file"`
+}
+
+// Files returns the files generated by the asset.
+func (a *AssetBase) Files() []*asset.File {
+	if a.File != nil {
+		return []*asset.File{a.File}
+	}
+	return []*asset.File{}
+}
+
+// Name returns the human-friendly name of the asset.
+func (a *AssetBase) Name() string {
+	return "Install Config"
+}
+
+// LoadFromFile returns the installconfig from disk.
+func (a *AssetBase) LoadFromFile(f asset.FileFetcher) (found bool, err error) {
+	file, err := f.FetchByName(installConfigFilename)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return false, nil
+		}
+		return false, errors.Wrap(err, asset.InstallConfigError)
+	}
+
+	config := &types.InstallConfig{}
+	if err := yaml.UnmarshalStrict(file.Data, config, yaml.DisallowUnknownFields); err != nil {
+		err = errors.Wrapf(err, "failed to unmarshal %s", installConfigFilename)
+		if !strings.Contains(err.Error(), "unknown field") {
+			return false, errors.Wrap(err, asset.InstallConfigError)
+		}
+		err = errors.Wrapf(err, "failed to parse first occurence of unknown field")
+		logrus.Warnf(err.Error())
+		logrus.Info("Attempting to unmarshal while ignoring unknown keys because strict unmarshaling failed")
+		if err = yaml.UnmarshalStrict(file.Data, config); err != nil {
+			err = errors.Wrapf(err, "failed to unmarshal %s", installConfigFilename)
+			return false, errors.Wrap(err, asset.InstallConfigError)
+		}
+	}
+	a.Config = config
+
+	// Upconvert any deprecated fields
+	if err := conversion.ConvertInstallConfig(a.Config); err != nil {
+		return false, errors.Wrap(errors.Wrap(err, "failed to upconvert install config"), asset.InstallConfigError)
+	}
+
+	defaults.SetInstallConfigDefaults(a.Config)
+
+	return true, nil
+}
+
+// RecordFile generates the asset manifest file from the config CR.
+func (a *AssetBase) RecordFile() error {
+	data, err := yaml.Marshal(a.Config)
+	if err != nil {
+		return errors.Wrap(err, "failed to Marshal InstallConfig")
+	}
+	a.File = &asset.File{
+		Filename: installConfigFilename,
+		Data:     data,
+	}
+	return nil
+}

pkg/asset/logging/filelogging_test.go

@@ -35,8 +35,10 @@ func TestLogFilesChanged(t *testing.T) {
 			name: "test asset with one file",
 			assets: []asset.WritableAsset{
 				&installconfig.InstallConfig{
-					File: &asset.File{
-						Filename: "a.yaml",
+					AssetBase: installconfig.AssetBase{
+						File: &asset.File{
+							Filename: "a.yaml",
+						},
 					},
 				},
 			},