From d734c69750d414c1e20283a7bb207bf4f025dcf8 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Tue, 24 Jan 2023 16:13:16 +1300 Subject: [PATCH 1/5] Add installconfig.MakeAsset() utility function Refactor unit tests to use installconfig.MakeAsset() to generate an installconfig.InstallConfig asset from a types.InstallConfig. --- .../master_ignition_customizations_test.go | 7 +++--- pkg/asset/ignition/machine/master_test.go | 7 +++--- .../worker_ignition_customizations_test.go | 7 +++--- pkg/asset/ignition/machine/worker_test.go | 7 +++--- pkg/asset/installconfig/installconfig.go | 7 ++++++ pkg/asset/machines/master_test.go | 25 ++++++++----------- pkg/asset/machines/worker_test.go | 16 ++++++------ pkg/asset/manifests/infrastructure_test.go | 2 +- pkg/asset/manifests/ingress_test.go | 6 ++--- 9 files changed, 41 insertions(+), 43 deletions(-) diff --git a/pkg/asset/ignition/machine/master_ignition_customizations_test.go b/pkg/asset/ignition/machine/master_ignition_customizations_test.go index 69dd6983167..b8f8377a0fe 100644 --- a/pkg/asset/ignition/machine/master_ignition_customizations_test.go +++ b/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) diff --git a/pkg/asset/ignition/machine/master_test.go b/pkg/asset/ignition/machine/master_test.go index d547f42ec2d..ac6b84de624 100644 --- a/pkg/asset/ignition/machine/master_test.go +++ b/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) diff --git a/pkg/asset/ignition/machine/worker_ignition_customizations_test.go b/pkg/asset/ignition/machine/worker_ignition_customizations_test.go index 595faff8be6..599567b0682 100644 --- a/pkg/asset/ignition/machine/worker_ignition_customizations_test.go +++ b/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) diff --git a/pkg/asset/ignition/machine/worker_test.go b/pkg/asset/ignition/machine/worker_test.go index 91e0833511f..a623fb8b7b1 100644 --- a/pkg/asset/ignition/machine/worker_test.go +++ b/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) diff --git a/pkg/asset/installconfig/installconfig.go b/pkg/asset/installconfig/installconfig.go index b92b27e0f4e..c3b0ccbec4f 100644 --- a/pkg/asset/installconfig/installconfig.go +++ b/pkg/asset/installconfig/installconfig.go @@ -45,6 +45,13 @@ 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{ + Config: config, + } +} + // Dependencies returns all of the dependencies directly needed by an // InstallConfig asset. func (a *InstallConfig) Dependencies() []asset.Asset { diff --git a/pkg/asset/machines/master_test.go b/pkg/asset/machines/master_test.go index 85e147ca4ba..299b36f2a6a 100644 --- a/pkg/asset/machines/master_test.go +++ b/pkg/asset/machines/master_test.go @@ -134,8 +134,8 @@ spec: UUID: "test-uuid", InfraID: "test-infra-id", }, - &installconfig.InstallConfig{ - Config: &types.InstallConfig{ + installconfig.MakeAsset( + &types.InstallConfig{ ObjectMeta: metav1.ObjectMeta{ Name: "test-cluster", }, @@ -156,8 +156,7 @@ spec: }, }, }, - }, - }, + }), (*rhcos.Image)(pointer.StringPtr("test-image")), (*rhcos.Release)(pointer.StringPtr("412.86.202208101040-0")), &machine.Master{ @@ -185,8 +184,8 @@ spec: func TestControlPlaneIsNotModified(t *testing.T) { parents := asset.Parents{} - installConfig := installconfig.InstallConfig{ - Config: &types.InstallConfig{ + installConfig := installconfig.MakeAsset( + &types.InstallConfig{ ObjectMeta: metav1.ObjectMeta{ Name: "test-cluster", }, @@ -209,15 +208,14 @@ func TestControlPlaneIsNotModified(t *testing.T) { }, }, }, - }, - } + }) parents.Add( &installconfig.ClusterID{ UUID: "test-uuid", InfraID: "test-infra-id", }, - &installConfig, + installConfig, (*rhcos.Image)(pointer.StringPtr("test-image")), (*rhcos.Release)(pointer.StringPtr("412.86.202208101040-0")), &machine.Master{ @@ -239,8 +237,8 @@ func TestControlPlaneIsNotModified(t *testing.T) { func TestBaremetalGeneratedAssetFiles(t *testing.T) { parents := asset.Parents{} - installConfig := installconfig.InstallConfig{ - Config: &types.InstallConfig{ + installConfig := installconfig.MakeAsset( + &types.InstallConfig{ ObjectMeta: metav1.ObjectMeta{ Name: "test-cluster", }, @@ -279,15 +277,14 @@ func TestBaremetalGeneratedAssetFiles(t *testing.T) { Platform: types.MachinePoolPlatform{}, }, }, - }, - } + }) parents.Add( &installconfig.ClusterID{ UUID: "test-uuid", InfraID: "test-infra-id", }, - &installConfig, + installConfig, (*rhcos.Image)(pointer.StringPtr("test-image")), (*rhcos.Release)(pointer.StringPtr("412.86.202208101040-0")), &machine.Master{ diff --git a/pkg/asset/machines/worker_test.go b/pkg/asset/machines/worker_test.go index 35654bbdafb..4abb12eec7b 100644 --- a/pkg/asset/machines/worker_test.go +++ b/pkg/asset/machines/worker_test.go @@ -128,8 +128,8 @@ spec: UUID: "test-uuid", InfraID: "test-infra-id", }, - &installconfig.InstallConfig{ - Config: &types.InstallConfig{ + installconfig.MakeAsset( + &types.InstallConfig{ ObjectMeta: metav1.ObjectMeta{ Name: "test-cluster", }, @@ -152,8 +152,7 @@ spec: }, }, }, - }, - }, + }), (*rhcos.Image)(pointer.StringPtr("test-image")), (*rhcos.Release)(pointer.StringPtr("412.86.202208101040-0")), &machine.Worker{ @@ -181,8 +180,8 @@ spec: func TestComputeIsNotModified(t *testing.T) { parents := asset.Parents{} - installConfig := installconfig.InstallConfig{ - Config: &types.InstallConfig{ + installConfig := installconfig.MakeAsset( + &types.InstallConfig{ ObjectMeta: metav1.ObjectMeta{ Name: "test-cluster", }, @@ -208,15 +207,14 @@ func TestComputeIsNotModified(t *testing.T) { }, }, }, - }, - } + }) parents.Add( &installconfig.ClusterID{ UUID: "test-uuid", InfraID: "test-infra-id", }, - &installConfig, + installConfig, (*rhcos.Image)(pointer.StringPtr("test-image")), (*rhcos.Release)(pointer.StringPtr("412.86.202208101040-0")), &machine.Worker{ diff --git a/pkg/asset/manifests/infrastructure_test.go b/pkg/asset/manifests/infrastructure_test.go index 6d17f41c843..d0cd13c5d60 100644 --- a/pkg/asset/manifests/infrastructure_test.go +++ b/pkg/asset/manifests/infrastructure_test.go @@ -58,7 +58,7 @@ func TestGenerateInfrastructure(t *testing.T) { UUID: "test-uuid", InfraID: "test-infra-id", }, - &installconfig.InstallConfig{Config: tc.installConfig}, + installconfig.MakeAsset(tc.installConfig), &CloudProviderConfig{}, &AdditionalTrustBundleConfig{}, ) diff --git a/pkg/asset/manifests/ingress_test.go b/pkg/asset/manifests/ingress_test.go index fddbe5f8c78..b4f23196871 100644 --- a/pkg/asset/manifests/ingress_test.go +++ b/pkg/asset/manifests/ingress_test.go @@ -158,10 +158,10 @@ func TestGenerateIngerssDefaultPlacement(t *testing.T) { UUID: "test-uuid", InfraID: "test-infra-id", }, - &installconfig.InstallConfig{ - Config: installConfigFromTopologies(t, tc.installConfigBuildOptions, + installconfig.MakeAsset( + installConfigFromTopologies(t, tc.installConfigBuildOptions, tc.controlPlaneTopology, tc.infrastructureTopology), - }, + ), ) ingressAsset := &Ingress{} err := ingressAsset.Generate(parents) From 16639332cedbd3de7a15393037b32ec175f0a808 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Tue, 24 Jan 2023 14:19:45 +1300 Subject: [PATCH 2/5] 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 | 152 +++++++++--------- 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, 229 insertions(+), 177 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 bed24e708a4..1ec1fac5082 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), }, - }, - ClusterNetwork: []types.ClusterNetworkEntry{ { - CIDR: ipnet.IPNet{IPNet: *newCidr}, - HostPrefix: 23, + Name: "worker-machine-pool-2", + Replicas: pointer.Int64Ptr(3), }, }, - ServiceNetwork: []ipnet.IPNet{ - *ipnet.MustParseCIDR("172.30.0.0/16"), + 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", }, - 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 61b9d1ec0b7..770ea2d0ba2 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 92240662fad..fa844710ac3 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 c3b0ccbec4f..56d905f4fd2 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 - 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) } @@ -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 00000000000..4e4285e28a5 --- /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 ad0e76905ea..7436c31995e 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", + }, }, }, }, From 3b0279ff6cbbec0a72aaa879ec4718cee4377e97 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Tue, 24 Jan 2023 15:46:37 +1300 Subject: [PATCH 3/5] Embed installconfig.AssetBase in OptionalInstallConfig Instead of embedding the full InstallConfig struct, just embed the common base struct. --- pkg/asset/agent/installconfig.go | 20 ++- pkg/asset/agent/installconfig_test.go | 4 +- pkg/asset/agent/manifests/util_test.go | 158 +++++++++--------- pkg/asset/agent/mirror/cabundle_test.go | 24 ++- pkg/asset/agent/mirror/registriesconf_test.go | 74 ++++---- 5 files changed, 136 insertions(+), 144 deletions(-) diff --git a/pkg/asset/agent/installconfig.go b/pkg/asset/agent/installconfig.go index 3bccd4d3f3b..37036326963 100644 --- a/pkg/asset/agent/installconfig.go +++ b/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); err != nil { + allErrs = append(allErrs, err...) + } if err := a.validateSupportedPlatforms(installConfig); err != nil { allErrs = append(allErrs, err...) diff --git a/pkg/asset/agent/installconfig_test.go b/pkg/asset/agent/installconfig_test.go index 591b1071839..cf53ab35c7e 100644 --- a/pkg/asset/agent/installconfig_test.go +++ b/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 //host//...]`, + 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, 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 //host//...]`, }, { name: "invalid configuration for none platform for sno", diff --git a/pkg/asset/agent/manifests/util_test.go b/pkg/asset/agent/manifests/util_test.go index 1ec1fac5082..bd65cbfe9f9 100644 --- a/pkg/asset/agent/manifests/util_test.go +++ b/pkg/asset/agent/manifests/util_test.go @@ -72,53 +72,51 @@ 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", - Namespace: "cluster-0", + AssetBase: installconfig.AssetBase{ + 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), }, - BaseDomain: "testing.com", - PullSecret: testSecret, - SSHKey: testSSHKey, - ControlPlane: &types.MachinePool{ - Name: "master", + { + Name: "worker-machine-pool-2", Replicas: pointer.Int64Ptr(3), - Platform: types.MachinePoolPlatform{}, }, - Compute: []types.MachinePool{ + }, + Networking: &types.Networking{ + MachineNetwork: []types.MachineNetworkEntry{ { - Name: "worker-machine-pool-1", - Replicas: pointer.Int64Ptr(2), + CIDR: ipnet.IPNet{IPNet: *machineNetCidr}, }, + }, + ClusterNetwork: []types.ClusterNetworkEntry{ { - Name: "worker-machine-pool-2", - Replicas: pointer.Int64Ptr(3), + 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"), }, - Platform: types.Platform{ - BareMetal: &baremetal.Platform{ - APIVIPs: []string{"192.168.122.10"}, - IngressVIPs: []string{"192.168.122.11"}, - }, + NetworkType: "OVNKubernetes", + }, + Platform: types.Platform{ + BareMetal: &baremetal.Platform{ + APIVIPs: []string{"192.168.122.10"}, + IngressVIPs: []string{"192.168.122.11"}, }, }, }, @@ -135,59 +133,57 @@ 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", - Namespace: "cluster-0", + AssetBase: installconfig.AssetBase{ + 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), }, - BaseDomain: "testing.com", - PullSecret: testSecret, - SSHKey: testSSHKey, - ControlPlane: &types.MachinePool{ - Name: "master", + { + Name: "worker-machine-pool-2", Replicas: pointer.Int64Ptr(3), - Platform: types.MachinePoolPlatform{}, }, - Compute: []types.MachinePool{ + }, + Networking: &types.Networking{ + MachineNetwork: []types.MachineNetworkEntry{ { - Name: "worker-machine-pool-1", - Replicas: pointer.Int64Ptr(2), + CIDR: ipnet.IPNet{IPNet: *machineNetCidr}, }, { - Name: "worker-machine-pool-2", - Replicas: pointer.Int64Ptr(3), + CIDR: ipnet.IPNet{IPNet: *machineNetCidrIPv6}, }, }, - Networking: &types.Networking{ - MachineNetwork: []types.MachineNetworkEntry{ - { - CIDR: ipnet.IPNet{IPNet: *machineNetCidr}, - }, - { - CIDR: ipnet.IPNet{IPNet: *machineNetCidrIPv6}, - }, - }, - ClusterNetwork: []types.ClusterNetworkEntry{ - { - CIDR: ipnet.IPNet{IPNet: *newCidr}, - HostPrefix: 23, - }, - { - CIDR: ipnet.IPNet{IPNet: *newCidrIPv6}, - HostPrefix: 64, - }, + ClusterNetwork: []types.ClusterNetworkEntry{ + { + CIDR: ipnet.IPNet{IPNet: *newCidr}, + HostPrefix: 23, }, - ServiceNetwork: []ipnet.IPNet{ - *ipnet.MustParseCIDR("172.30.0.0/16"), *ipnet.MustParseCIDR("fd02::/112"), + { + CIDR: ipnet.IPNet{IPNet: *newCidrIPv6}, + HostPrefix: 64, }, }, - Platform: types.Platform{ - BareMetal: &baremetal.Platform{ - APIVIPs: []string{"192.168.122.10"}, - IngressVIPs: []string{"192.168.122.11"}, - }, + 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"}, }, }, }, @@ -206,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", diff --git a/pkg/asset/agent/mirror/cabundle_test.go b/pkg/asset/agent/mirror/cabundle_test.go index 770ea2d0ba2..8e0e8b02789 100644 --- a/pkg/asset/agent/mirror/cabundle_test.go +++ b/pkg/asset/agent/mirror/cabundle_test.go @@ -35,12 +35,10 @@ 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", - }, + AssetBase: installconfig.AssetBase{ + Config: &types.InstallConfig{ + ObjectMeta: v1.ObjectMeta{ + Namespace: "cluster-0", }, }, }, @@ -52,13 +50,12 @@ 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", - }, - AdditionalTrustBundle: ` + AssetBase: installconfig.AssetBase{ + Config: &types.InstallConfig{ + ObjectMeta: v1.ObjectMeta{ + Namespace: "cluster-0", + }, + AdditionalTrustBundle: ` -----BEGIN CERTIFICATE----- MIIDZTCCAk2gAwIBAgIURbA8lR+5xlJZUoOXK66AHFWd3uswDQYJKoZIhvcNAQEL BQAwQjELMAkGA1UEBhMCWFgxFTATBgNVBAcMDERlZmF1bHQgQ2l0eTEcMBoGA1UE @@ -81,7 +78,6 @@ 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 fa844710ac3..0c289d15018 100644 --- a/pkg/asset/agent/mirror/registriesconf_test.go +++ b/pkg/asset/agent/mirror/registriesconf_test.go @@ -37,12 +37,10 @@ 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", - }, + AssetBase: installconfig.AssetBase{ + Config: &types.InstallConfig{ + ObjectMeta: v1.ObjectMeta{ + Namespace: "cluster-0", }, }, }, @@ -58,24 +56,22 @@ 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", - }, - 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", + }, + 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", - }, + }, + { + Source: "quay.io/openshift-release-dev/ocp-v4.0-art-dev", + Mirrors: []string{ + "virthost.ostest.test.metalkube.org:5000/localimages/local-release-image", }, }, }, @@ -93,24 +89,22 @@ 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", - }, - 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", + }, + 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", - }, + }, + { + Source: "quay.io/openshift-release-dev/ocp-v4.0-art-dev", + Mirrors: []string{ + "virthost.ostest.test.metalkube.org:5000/localimages/local-release-image", }, }, }, From d5b6742b1888802582fd496061c7882dd9b34898 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Tue, 24 Jan 2023 18:04:11 +1300 Subject: [PATCH 4/5] Explicitly pass install method to install-config platform validations Instead of trying to infer the installation method from an unreliable parsing of the command line arguments, pass a flag to explicitly identify the agent-based install method. --- pkg/asset/agent/installconfig.go | 2 +- pkg/asset/agent/installconfig_test.go | 2 +- pkg/asset/installconfig/installconfig.go | 2 +- pkg/asset/installconfig/vsphere/validation.go | 2 +- pkg/types/baremetal/validation/platform.go | 4 +--- pkg/types/baremetal/validation/platform_test.go | 2 +- pkg/types/validation/installconfig.go | 10 +++++----- pkg/types/validation/installconfig_test.go | 2 +- pkg/types/vsphere/validation/platform.go | 4 ++-- pkg/types/vsphere/validation/platform_test.go | 2 +- pkg/validate/method.go | 16 ---------------- 11 files changed, 15 insertions(+), 33 deletions(-) delete mode 100644 pkg/validate/method.go diff --git a/pkg/asset/agent/installconfig.go b/pkg/asset/agent/installconfig.go index 37036326963..3b52267dbd6 100644 --- a/pkg/asset/agent/installconfig.go +++ b/pkg/asset/agent/installconfig.go @@ -63,7 +63,7 @@ func (a *OptionalInstallConfig) Load(f asset.FileFetcher) (bool, error) { func (a *OptionalInstallConfig) validateInstallConfig(installConfig *types.InstallConfig) field.ErrorList { var allErrs field.ErrorList - if err := validation.ValidateInstallConfig(a.Config); err != nil { + if err := validation.ValidateInstallConfig(a.Config, true); err != nil { allErrs = append(allErrs, err...) } diff --git a/pkg/asset/agent/installconfig_test.go b/pkg/asset/agent/installconfig_test.go index cf53ab35c7e..b65f2eae354 100644 --- a/pkg/asset/agent/installconfig_test.go +++ b/pkg/asset/agent/installconfig_test.go @@ -102,7 +102,7 @@ platform: pullSecret: "{\"auths\":{\"example.com\":{\"auth\":\"authorization value\"}}}" `, expectedFound: false, - 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, 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 //host//...]`, + 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", diff --git a/pkg/asset/installconfig/installconfig.go b/pkg/asset/installconfig/installconfig.go index 56d905f4fd2..9049b1cc3cf 100644 --- a/pkg/asset/installconfig/installconfig.go +++ b/pkg/asset/installconfig/installconfig.go @@ -141,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") } diff --git a/pkg/asset/installconfig/vsphere/validation.go b/pkg/asset/installconfig/vsphere/validation.go index 2292facd377..06c90574c07 100644 --- a/pkg/asset/installconfig/vsphere/validation.go +++ b/pkg/asset/installconfig/vsphere/validation.go @@ -59,7 +59,7 @@ func Validate(ic *types.InstallConfig) error { if ic.Platform.VSphere == nil { return errors.New(field.Required(field.NewPath("platform", "vsphere"), "vSphere validation requires a vSphere platform configuration").Error()) } - return validation.ValidatePlatform(ic.Platform.VSphere, field.NewPath("platform").Child("vsphere"), ic).ToAggregate() + return validation.ValidatePlatform(ic.Platform.VSphere, false, field.NewPath("platform").Child("vsphere"), ic).ToAggregate() } func getVCenterClient(failureDomain vsphere.FailureDomain, ic *types.InstallConfig) (*validationContext, ClientLogout, error) { diff --git a/pkg/types/baremetal/validation/platform.go b/pkg/types/baremetal/validation/platform.go index b6c2e66a33f..b3669ed0279 100644 --- a/pkg/types/baremetal/validation/platform.go +++ b/pkg/types/baremetal/validation/platform.go @@ -382,7 +382,7 @@ func validateProvisioningNetworkDisabledSupported(hosts []*baremetal.Host, fldPa } // ValidatePlatform checks that the specified platform is valid. -func ValidatePlatform(p *baremetal.Platform, n *types.Networking, fldPath *field.Path, c *types.InstallConfig) field.ErrorList { +func ValidatePlatform(p *baremetal.Platform, agentBasedInstallation bool, n *types.Networking, fldPath *field.Path, c *types.InstallConfig) field.ErrorList { allErrs := field.ErrorList{} provisioningNetwork := sets.NewString(string(baremetal.ManagedProvisioningNetwork), @@ -405,8 +405,6 @@ func ValidatePlatform(p *baremetal.Platform, n *types.Networking, fldPath *field } } - agentBasedInstallation := validate.IsAgentBasedInstallation() - if !agentBasedInstallation && p.Hosts == nil { allErrs = append(allErrs, field.Invalid(fldPath.Child("hosts"), p.Hosts, "bare metal hosts are missing")) } diff --git a/pkg/types/baremetal/validation/platform_test.go b/pkg/types/baremetal/validation/platform_test.go index bcf22d9069a..d7f31aed7ef 100644 --- a/pkg/types/baremetal/validation/platform_test.go +++ b/pkg/types/baremetal/validation/platform_test.go @@ -348,7 +348,7 @@ interfaces: tc.config.BareMetal = tc.platform } - err := ValidatePlatform(tc.config.BareMetal, network(), field.NewPath("baremetal"), tc.config).ToAggregate() + err := ValidatePlatform(tc.config.BareMetal, false, network(), field.NewPath("baremetal"), tc.config).ToAggregate() if tc.expected == "" { assert.NoError(t, err) diff --git a/pkg/types/validation/installconfig.go b/pkg/types/validation/installconfig.go index bb0e32c20fe..d88ad85496b 100644 --- a/pkg/types/validation/installconfig.go +++ b/pkg/types/validation/installconfig.go @@ -54,7 +54,7 @@ import ( var pluginsUsingHostPrefix = sets.NewString(string(operv1.NetworkTypeOpenShiftSDN), string(operv1.NetworkTypeOVNKubernetes)) // ValidateInstallConfig checks that the specified install config is valid. -func ValidateInstallConfig(c *types.InstallConfig) field.ErrorList { +func ValidateInstallConfig(c *types.InstallConfig, usingAgentMethod bool) field.ErrorList { allErrs := field.ErrorList{} if c.TypeMeta.APIVersion == "" { return field.ErrorList{field.Required(field.NewPath("apiVersion"), "install-config version required")} @@ -114,7 +114,7 @@ func ValidateInstallConfig(c *types.InstallConfig) field.ErrorList { } else { allErrs = append(allErrs, field.Required(field.NewPath("networking"), "networking is required")) } - allErrs = append(allErrs, validatePlatform(&c.Platform, field.NewPath("platform"), c.Networking, c)...) + allErrs = append(allErrs, validatePlatform(&c.Platform, usingAgentMethod, field.NewPath("platform"), c.Networking, c)...) if c.ControlPlane != nil { allErrs = append(allErrs, validateControlPlane(&c.Platform, c.ControlPlane, field.NewPath("controlPlane"))...) } else { @@ -673,7 +673,7 @@ func ValidateIPinMachineCIDR(vip string, n *types.Networking) error { return fmt.Errorf("IP expected to be in one of the machine networks: %s", strings.Join(networks, ",")) } -func validatePlatform(platform *types.Platform, fldPath *field.Path, network *types.Networking, c *types.InstallConfig) field.ErrorList { +func validatePlatform(platform *types.Platform, usingAgentMethod bool, fldPath *field.Path, network *types.Networking, c *types.InstallConfig) field.ErrorList { allErrs := field.ErrorList{} activePlatform := platform.Name() platforms := make([]string, len(types.PlatformNames)) @@ -722,12 +722,12 @@ func validatePlatform(platform *types.Platform, fldPath *field.Path, network *ty } if platform.VSphere != nil { validate(vsphere.Name, platform.VSphere, func(f *field.Path) field.ErrorList { - return vspherevalidation.ValidatePlatform(platform.VSphere, f, c) + return vspherevalidation.ValidatePlatform(platform.VSphere, usingAgentMethod, f, c) }) } if platform.BareMetal != nil { validate(baremetal.Name, platform.BareMetal, func(f *field.Path) field.ErrorList { - return baremetalvalidation.ValidatePlatform(platform.BareMetal, network, f, c) + return baremetalvalidation.ValidatePlatform(platform.BareMetal, usingAgentMethod, network, f, c) }) } if platform.Ovirt != nil { diff --git a/pkg/types/validation/installconfig_test.go b/pkg/types/validation/installconfig_test.go index 9aa962c0ab3..74511661b4b 100644 --- a/pkg/types/validation/installconfig_test.go +++ b/pkg/types/validation/installconfig_test.go @@ -2040,7 +2040,7 @@ func TestValidateInstallConfig(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - err := ValidateInstallConfig(tc.installConfig).ToAggregate() + err := ValidateInstallConfig(tc.installConfig, false).ToAggregate() if tc.expectedError == "" { assert.NoError(t, err) } else { diff --git a/pkg/types/vsphere/validation/platform.go b/pkg/types/vsphere/validation/platform.go index a2f452e23e2..54dbd115521 100644 --- a/pkg/types/vsphere/validation/platform.go +++ b/pkg/types/vsphere/validation/platform.go @@ -16,7 +16,7 @@ import ( ) // ValidatePlatform checks that the specified platform is valid. -func ValidatePlatform(p *vsphere.Platform, fldPath *field.Path, c *types.InstallConfig) field.ErrorList { +func ValidatePlatform(p *vsphere.Platform, agentBasedInstallation bool, fldPath *field.Path, c *types.InstallConfig) field.ErrorList { isLegacyUpi := false // This is to cover existing UPI non-zonal case // where neither network or cluster is required. @@ -33,7 +33,7 @@ func ValidatePlatform(p *vsphere.Platform, fldPath *field.Path, c *types.Install allErrs = append(allErrs, validateDiskType(p, fldPath)...) } - if !validate.IsAgentBasedInstallation() { + if !agentBasedInstallation { if len(p.VCenters) == 0 { return append(allErrs, field.Required(fldPath.Child("vcenters"), "must be defined")) } diff --git a/pkg/types/vsphere/validation/platform_test.go b/pkg/types/vsphere/validation/platform_test.go index 6994d323363..7b4aede648f 100644 --- a/pkg/types/vsphere/validation/platform_test.go +++ b/pkg/types/vsphere/validation/platform_test.go @@ -288,7 +288,7 @@ func TestValidatePlatform(t *testing.T) { tc.config = installConfig().build() tc.config.VSphere = tc.platform } - err := ValidatePlatform(tc.platform, field.NewPath("test-path"), tc.config).ToAggregate() + err := ValidatePlatform(tc.platform, false, field.NewPath("test-path"), tc.config).ToAggregate() if tc.expectedError == "" { assert.NoError(t, err) } else { diff --git a/pkg/validate/method.go b/pkg/validate/method.go deleted file mode 100644 index ba7cd9d32c7..00000000000 --- a/pkg/validate/method.go +++ /dev/null @@ -1,16 +0,0 @@ -package validate - -import "os" - -// IsAgentBasedInstallation determines whether we are using the 'agent' -// subcommand to install. -func IsAgentBasedInstallation() bool { - if len(os.Args) > 1 { - for _, arg := range os.Args { - if arg == "agent" { - return true - } - } - } - return false -} From ddd644e9195112697240b1628d183bb20cbbe6d9 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Wed, 25 Jan 2023 21:18:17 +1300 Subject: [PATCH 5/5] Fix spelling mistake --- pkg/asset/installconfig/installconfigbase.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/asset/installconfig/installconfigbase.go b/pkg/asset/installconfig/installconfigbase.go index 4e4285e28a5..1eeab893daf 100644 --- a/pkg/asset/installconfig/installconfigbase.go +++ b/pkg/asset/installconfig/installconfigbase.go @@ -50,7 +50,7 @@ func (a *AssetBase) LoadFromFile(f asset.FileFetcher) (found bool, err error) { 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") + err = errors.Wrapf(err, "failed to parse first occurrence 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 {