From 16639332cedbd3de7a15393037b32ec175f0a808 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Tue, 24 Jan 2023 14:19:45 +1300 Subject: [PATCH] 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 | 154 +++++++++--------- pkg/asset/agent/mirror/cabundle_test.go | 20 ++- pkg/asset/agent/mirror/registriesconf_test.go | 68 ++++---- pkg/asset/installconfig/installconfig.go | 76 ++------- pkg/asset/installconfig/installconfigbase.go | 84 ++++++++++ pkg/asset/logging/filelogging_test.go | 6 +- 6 files changed, 230 insertions(+), 178 deletions(-) create mode 100644 pkg/asset/installconfig/installconfigbase.go diff --git a/pkg/asset/agent/manifests/util_test.go b/pkg/asset/agent/manifests/util_test.go index bed24e708a..1ec1fac508 100644 --- a/pkg/asset/agent/manifests/util_test.go +++ b/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"}, + }, }, }, }, diff --git a/pkg/asset/agent/mirror/cabundle_test.go b/pkg/asset/agent/mirror/cabundle_test.go index 61b9d1ec0b..770ea2d0ba 100644 --- a/pkg/asset/agent/mirror/cabundle_test.go +++ b/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----- `, + }, }, }, }, diff --git a/pkg/asset/agent/mirror/registriesconf_test.go b/pkg/asset/agent/mirror/registriesconf_test.go index 92240662fa..fa844710ac 100644 --- a/pkg/asset/agent/mirror/registriesconf_test.go +++ b/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", + }, }, }, }, diff --git a/pkg/asset/installconfig/installconfig.go b/pkg/asset/installconfig/installconfig.go index c3b0ccbec4..56d905f4fd 100644 --- a/pkg/asset/installconfig/installconfig.go +++ b/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() diff --git a/pkg/asset/installconfig/installconfigbase.go b/pkg/asset/installconfig/installconfigbase.go new file mode 100644 index 0000000000..4e4285e28a --- /dev/null +++ b/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 +} diff --git a/pkg/asset/logging/filelogging_test.go b/pkg/asset/logging/filelogging_test.go index ad0e76905e..7436c31995 100644 --- a/pkg/asset/logging/filelogging_test.go +++ b/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", + }, }, }, },