AGENT-526: Refactor Agent InstallConfig embedding

pkg/asset/agent/installconfig.go

@@ -14,6 +14,7 @@ import (
 	"github.com/openshift/installer/pkg/types/baremetal"
 	baremetaldefaults "github.com/openshift/installer/pkg/types/baremetal/defaults"
 	"github.com/openshift/installer/pkg/types/none"
+	"github.com/openshift/installer/pkg/types/validation"
 	"github.com/openshift/installer/pkg/types/vsphere"
 )
 
@@ -24,10 +25,12 @@ const (
 // OptionalInstallConfig is an InstallConfig where the default is empty, rather
 // than generated from running the survey.
 type OptionalInstallConfig struct {
-	installconfig.InstallConfig
+	installconfig.AssetBase
 	Supplied bool
 }
 
+var _ asset.WritableAsset = (*OptionalInstallConfig)(nil)
+
 // Dependencies returns all of the dependencies directly needed by an
 // InstallConfig asset.
 func (a *OptionalInstallConfig) Dependencies() []asset.Asset {
@@ -45,21 +48,24 @@ func (a *OptionalInstallConfig) Generate(parents asset.Parents) error {
 
 // Load returns the installconfig from disk.
 func (a *OptionalInstallConfig) Load(f asset.FileFetcher) (bool, error) {
-
-	var foundValidatedDefaultInstallConfig bool
-
-	foundValidatedDefaultInstallConfig, err := a.InstallConfig.Load(f)
-	if foundValidatedDefaultInstallConfig && err == nil {
+	found, err := a.LoadFromFile(f)
+	if found && err == nil {
 		a.Supplied = true
 		if err := a.validateInstallConfig(a.Config).ToAggregate(); err != nil {
 			return false, errors.Wrapf(err, "invalid install-config configuration")
 		}
+		if err := a.RecordFile(); err != nil {
+			return false, err
+		}
 	}
-	return foundValidatedDefaultInstallConfig, err
+	return found, err
 }
 
 func (a *OptionalInstallConfig) validateInstallConfig(installConfig *types.InstallConfig) field.ErrorList {
 	var allErrs field.ErrorList
+	if err := validation.ValidateInstallConfig(a.Config, true); err != nil {
+		allErrs = append(allErrs, err...)
+	}
 
 	if err := a.validateSupportedPlatforms(installConfig); err != nil {
 		allErrs = append(allErrs, err...)

pkg/asset/agent/installconfig_test.go

@@ -86,7 +86,7 @@ platform:
 pullSecret: "{\"auths\":{\"example.com\":{\"auth\":\"authorization value\"}}}"
 `,
 			expectedFound: false,
-			expectedError: "failed to create install config: invalid \"install-config.yaml\" file: [platform.baremetal.apiVIPs: Required value: must specify at least one VIP for the API, platform.baremetal.apiVIPs: Required value: must specify VIP for API, when VIP for ingress is set]",
+			expectedError: "invalid install-config configuration: [platform.baremetal.apiVIPs: Required value: must specify at least one VIP for the API, platform.baremetal.apiVIPs: Required value: must specify VIP for API, when VIP for ingress is set]",
 		},
 		{
 			name: "Required values not set for vsphere platform",
@@ -102,7 +102,7 @@ platform:
 pullSecret: "{\"auths\":{\"example.com\":{\"auth\":\"authorization value\"}}}"
 `,
 			expectedFound: false,
-			expectedError: `failed to create install config: invalid "install-config.yaml" file: [platform.vsphere.apiVIPs: Invalid value: "192.168.122.10": IP expected to be in one of the machine networks: 10.0.0.0/16, platform.vsphere.ingressVIPs: Required value: must specify VIP for ingress, when VIP for API is set, platform.vsphere.vcenters.server: Required value: must be the domain name or IP address of the vCenter, platform.vsphere.vcenters.username: Required value: must specify the username, platform.vsphere.vcenters.password: Required value: must specify the password, platform.vsphere.vcenters.datacenters: Required value: must specify at least one datacenter, platform.vsphere.failureDomains.server: Required value: must specify a vCenter server, platform.vsphere.failureDomains.topology.datacenter: Required value: must specify a datacenter, platform.vsphere.failureDomains.topology.datastore: Required value: must specify a datastore, platform.vsphere.failureDomains.topology.computeCluster: Required value: must specify a computeCluster, platform.vsphere.failureDomains.topology.resourcePool: Invalid value: "//Resources": full path of resource pool must be provided in format /<datacenter>/host/<cluster>/...]`,
+			expectedError: `invalid install-config configuration: [platform.vsphere.apiVIPs: Invalid value: "192.168.122.10": IP expected to be in one of the machine networks: 10.0.0.0/16, platform.vsphere.ingressVIPs: Required value: must specify VIP for ingress, when VIP for API is set]`,
 		},
 		{
 			name: "invalid configuration for none platform for sno",

pkg/asset/agent/manifests/util_test.go

@@ -72,7 +72,7 @@ func getValidOptionalInstallConfig() *agent.OptionalInstallConfig {
 	_, machineNetCidr, _ := net.ParseCIDR("10.10.11.0/24")
 
 	return &agent.OptionalInstallConfig{
-		InstallConfig: installconfig.InstallConfig{
+		AssetBase: installconfig.AssetBase{
 			Config: &types.InstallConfig{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "ocp-edge-cluster-0",
@@ -133,7 +133,7 @@ func getValidOptionalInstallConfigDualStack() *agent.OptionalInstallConfig {
 	_, machineNetCidrIPv6, _ := net.ParseCIDR("2001:db8:5dd8:c956::/64")
 
 	return &agent.OptionalInstallConfig{
-		InstallConfig: installconfig.InstallConfig{
+		AssetBase: installconfig.AssetBase{
 			Config: &types.InstallConfig{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      "ocp-edge-cluster-0",
@@ -202,7 +202,7 @@ func getValidOptionalInstallConfigDualStackDualVIPs() *agent.OptionalInstallConf
 // getProxyValidOptionalInstallConfig returns a valid optional install config for proxied installation
 func getProxyValidOptionalInstallConfig() *agent.OptionalInstallConfig {
 	validIC := getValidOptionalInstallConfig()
-	validIC.InstallConfig.Config.Proxy = &types.Proxy{
+	validIC.Config.Proxy = &types.Proxy{
 		HTTPProxy:  "http://10.10.10.11:80",
 		HTTPSProxy: "http://my-lab-proxy.org:443",
 		NoProxy:    "internal.com",

pkg/asset/agent/mirror/cabundle_test.go

@@ -35,7 +35,7 @@ func TestCaBundle_Generate(t *testing.T) {
 			dependencies: []asset.Asset{
 				&agent.OptionalInstallConfig{
 					Supplied: true,
-					InstallConfig: installconfig.InstallConfig{
+					AssetBase: installconfig.AssetBase{
 						Config: &types.InstallConfig{
 							ObjectMeta: v1.ObjectMeta{
 								Namespace: "cluster-0",
@@ -50,7 +50,7 @@ func TestCaBundle_Generate(t *testing.T) {
 			dependencies: []asset.Asset{
 				&agent.OptionalInstallConfig{
 					Supplied: true,
-					InstallConfig: installconfig.InstallConfig{
+					AssetBase: installconfig.AssetBase{
 						Config: &types.InstallConfig{
 							ObjectMeta: v1.ObjectMeta{
 								Namespace: "cluster-0",

pkg/asset/agent/mirror/registriesconf_test.go

@@ -37,7 +37,7 @@ func TestRegistriesConf_Generate(t *testing.T) {
 			dependencies: []asset.Asset{
 				&agent.OptionalInstallConfig{
 					Supplied: true,
-					InstallConfig: installconfig.InstallConfig{
+					AssetBase: installconfig.AssetBase{
 						Config: &types.InstallConfig{
 							ObjectMeta: v1.ObjectMeta{
 								Namespace: "cluster-0",
@@ -56,7 +56,7 @@ func TestRegistriesConf_Generate(t *testing.T) {
 			dependencies: []asset.Asset{
 				&agent.OptionalInstallConfig{
 					Supplied: true,
-					InstallConfig: installconfig.InstallConfig{
+					AssetBase: installconfig.AssetBase{
 						Config: &types.InstallConfig{
 							ObjectMeta: v1.ObjectMeta{
 								Namespace: "cluster-0",
@@ -89,7 +89,7 @@ func TestRegistriesConf_Generate(t *testing.T) {
 			dependencies: []asset.Asset{
 				&agent.OptionalInstallConfig{
 					Supplied: true,
-					InstallConfig: installconfig.InstallConfig{
+					AssetBase: installconfig.AssetBase{
 						Config: &types.InstallConfig{
 							ObjectMeta: v1.ObjectMeta{
 								Namespace: "cluster-0",

pkg/asset/ignition/machine/master_ignition_customizations_test.go

@@ -35,8 +35,8 @@ func TestMasterIgnitionCustomizationsGenerate(t *testing.T) {
 
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
-			installConfig := &installconfig.InstallConfig{
-				Config: &types.InstallConfig{
+			installConfig := installconfig.MakeAsset(
+				&types.InstallConfig{
 					Networking: &types.Networking{
 						ServiceNetwork: []ipnet.IPNet{*ipnet.MustParseCIDR("10.0.1.0/24")},
 					},
@@ -45,8 +45,7 @@ func TestMasterIgnitionCustomizationsGenerate(t *testing.T) {
 							Region: "us-east",
 						},
 					},
-				},
-			}
+				})
 
 			rootCA := &tls.RootCA{}
 			err := rootCA.Generate(nil)

pkg/asset/ignition/machine/master_test.go

@@ -17,8 +17,8 @@ import (
 
 // TestMasterGenerate tests generating the master asset.
 func TestMasterGenerate(t *testing.T) {
-	installConfig := &installconfig.InstallConfig{
-		Config: &types.InstallConfig{
+	installConfig := installconfig.MakeAsset(
+		&types.InstallConfig{
 			ObjectMeta: metav1.ObjectMeta{
 				Name: "test-cluster",
 			},
@@ -35,8 +35,7 @@ func TestMasterGenerate(t *testing.T) {
 				Name:     "master",
 				Replicas: pointer.Int64Ptr(3),
 			},
-		},
-	}
+		})
 
 	rootCA := &tls.RootCA{}
 	err := rootCA.Generate(nil)

pkg/asset/ignition/machine/worker_ignition_customizations_test.go

@@ -35,8 +35,8 @@ func TestWorkerIgnitionCustomizationsGenerate(t *testing.T) {
 
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
-			installConfig := &installconfig.InstallConfig{
-				Config: &types.InstallConfig{
+			installConfig := installconfig.MakeAsset(
+				&types.InstallConfig{
 					Networking: &types.Networking{
 						ServiceNetwork: []ipnet.IPNet{*ipnet.MustParseCIDR("10.0.1.0/24")},
 					},
@@ -45,8 +45,7 @@ func TestWorkerIgnitionCustomizationsGenerate(t *testing.T) {
 							Region: "us-east",
 						},
 					},
-				},
-			}
+				})
 
 			rootCA := &tls.RootCA{}
 			err := rootCA.Generate(nil)

pkg/asset/ignition/machine/worker_test.go

@@ -15,8 +15,8 @@ import (
 
 // TestWorkerGenerate tests generating the worker asset.
 func TestWorkerGenerate(t *testing.T) {
-	installConfig := &installconfig.InstallConfig{
-		Config: &types.InstallConfig{
+	installConfig := installconfig.MakeAsset(
+		&types.InstallConfig{
 			Networking: &types.Networking{
 				ServiceNetwork: []ipnet.IPNet{*ipnet.MustParseCIDR("10.0.1.0/24")},
 			},
@@ -25,8 +25,7 @@ func TestWorkerGenerate(t *testing.T) {
 					Region: "us-east",
 				},
 			},
-		},
-	}
+		})
 
 	rootCA := &tls.RootCA{}
 	err := rootCA.Generate(nil)

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"`
@@ -45,6 +39,15 @@ type InstallConfig struct {
 
 var _ asset.WritableAsset = (*InstallConfig)(nil)
 
+// MakeAsset returns an InstallConfig asset containing a given InstallConfig CR.
+func MakeAsset(config *types.InstallConfig) *InstallConfig {
+	return &InstallConfig{
+		AssetBase: AssetBase{
+			Config: config,
+		},
+	}
+}
+
 // Dependencies returns all of the dependencies directly needed by an
 // InstallConfig asset.
 func (a *InstallConfig) Dependencies() []asset.Asset {
@@ -104,63 +107,24 @@ func (a *InstallConfig) Generate(parents asset.Parents) error {
 	a.Config.PowerVS = platform.PowerVS
 	a.Config.Nutanix = platform.Nutanix
 
-	return a.finish("")
-}
-
-// Name returns the human-friendly name of the asset.
-func (a *InstallConfig) Name() string {
-	return "Install Config"
-}
+	defaults.SetInstallConfigDefaults(a.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{}
+	return a.finish("")
 }
 
 // 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)
 	}
@@ -177,7 +141,7 @@ func (a *InstallConfig) finish(filename string) error {
 		a.PowerVS = icpowervs.NewMetadata(a.Config.BaseDomain)
 	}
 
-	if err := validation.ValidateInstallConfig(a.Config).ToAggregate(); err != nil {
+	if err := validation.ValidateInstallConfig(a.Config, false).ToAggregate(); err != nil {
 		if filename == "" {
 			return errors.Wrap(err, "invalid install config")
 		}
@@ -188,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()