From 7d2ad01e1aafe05c34fabdba1a59318193c4c878 Mon Sep 17 00:00:00 2001 From: Michael Shitrit Date: Tue, 4 Mar 2025 11:04:16 +0200 Subject: [PATCH 1/8] Extract a common validation Signed-off-by: Michael Shitrit --- pkg/types/baremetal/validation/platform.go | 49 +--------------- pkg/types/common/common.go | 66 ++++++++++++++++++++++ 2 files changed, 68 insertions(+), 47 deletions(-) create mode 100644 pkg/types/common/common.go diff --git a/pkg/types/baremetal/validation/platform.go b/pkg/types/baremetal/validation/platform.go index 0400608fc5..4444be5642 100644 --- a/pkg/types/baremetal/validation/platform.go +++ b/pkg/types/baremetal/validation/platform.go @@ -23,6 +23,7 @@ import ( "github.com/openshift/installer/pkg/ipnet" "github.com/openshift/installer/pkg/types" "github.com/openshift/installer/pkg/types/baremetal" + "github.com/openshift/installer/pkg/types/common" "github.com/openshift/installer/pkg/validate" ) @@ -180,53 +181,7 @@ func validateDHCPRange(p *baremetal.Platform, fldPath *field.Path) (allErrs fiel // validateHostsBase validates the hosts based on a filtering function func validateHostsBase(hosts []*baremetal.Host, fldPath *field.Path, filter validator.FilterFunc) field.ErrorList { - hostErrs := field.ErrorList{} - - values := make(map[string]map[interface{}]struct{}) - - //Initialize a new validator and register a custom validation rule for the tag `uniqueField` - validate := validator.New() - validate.RegisterValidation("uniqueField", func(fl validator.FieldLevel) bool { - valueFound := false - fieldName := fl.Parent().Type().Name() + "." + fl.FieldName() - fieldValue := fl.Field().Interface() - - if fl.Field().Type().Comparable() { - if _, present := values[fieldName]; !present { - values[fieldName] = make(map[interface{}]struct{}) - } - - fieldValues := values[fieldName] - if _, valueFound = fieldValues[fieldValue]; !valueFound { - fieldValues[fieldValue] = struct{}{} - } - } else { - panic(fmt.Sprintf("Cannot apply validation rule 'uniqueField' on field %s", fl.FieldName())) - } - - return !valueFound - }) - - //Apply validations and translate errors - fldPath = fldPath.Child("hosts") - - for idx, host := range hosts { - err := validate.StructFiltered(host, filter) - if err != nil { - hostType := reflect.TypeOf(hosts).Elem().Elem().Name() - for _, err := range err.(validator.ValidationErrors) { - childName := fldPath.Index(idx).Child(err.Namespace()[len(hostType)+1:]) - switch err.Tag() { - case "required": - hostErrs = append(hostErrs, field.Required(childName, "missing "+err.Field())) - case "uniqueField": - hostErrs = append(hostErrs, field.Duplicate(childName, err.Value())) - } - } - } - } - - return hostErrs + return common.ValidateUniqueAndRequiredFields(hosts, fldPath, filter, "hosts") } // filterHostsBMC is a function to control whether to filter BMC details of Hosts diff --git a/pkg/types/common/common.go b/pkg/types/common/common.go new file mode 100644 index 0000000000..0a489da968 --- /dev/null +++ b/pkg/types/common/common.go @@ -0,0 +1,66 @@ +package common + +import ( + "errors" + "fmt" + "reflect" + + "github.com/go-playground/validator/v10" + "k8s.io/apimachinery/pkg/util/validation/field" +) + +// ValidateUniqueAndRequiredFields validated unique fields are indeed unique and that required fields exist on a generic element. +func ValidateUniqueAndRequiredFields[T any](elements []T, fldPath *field.Path, filter validator.FilterFunc, fieldName string) field.ErrorList { + errs := field.ErrorList{} + + values := make(map[string]map[interface{}]struct{}) + + // Initialize a new validator and register a custom validation rule for the tag `uniqueField`. + validate := validator.New() + if err := validate.RegisterValidation("uniqueField", func(fl validator.FieldLevel) bool { + valueFound := false + fieldName := fl.Parent().Type().Name() + "." + fl.FieldName() + fieldValue := fl.Field().Interface() + + if fl.Field().Type().Comparable() { + if _, present := values[fieldName]; !present { + values[fieldName] = make(map[interface{}]struct{}) + } + + fieldValues := values[fieldName] + if _, valueFound = fieldValues[fieldValue]; !valueFound { + fieldValues[fieldValue] = struct{}{} + } + } else { + panic(fmt.Sprintf("Cannot apply validation rule 'uniqueField' on field %s", fl.FieldName())) + } + + return !valueFound + }); err != nil { + fmt.Printf("unexpected error registering validation: %v\n", err) + } + + // Apply validations and translate errors. + + fldPath = fldPath.Child(fieldName) + + for idx, element := range elements { + err := validate.StructFiltered(element, filter) + if err != nil { + elementType := reflect.TypeOf(elements).Elem().Elem().Name() + var validationErrs validator.ValidationErrors + if errors.As(err, &validationErrs) { + for _, fieldErr := range validationErrs { + childName := fldPath.Index(idx).Child(fieldErr.Namespace()[len(elementType)+1:]) + switch fieldErr.Tag() { + case "required": + errs = append(errs, field.Required(childName, "missing "+fieldErr.Field())) + case "uniqueField": + errs = append(errs, field.Duplicate(childName, fieldErr.Value())) + } + } + } + } + } + return errs +} From fb01fcc85a92c2aa475d683d4301d918f6668561 Mon Sep 17 00:00:00 2001 From: Michael Shitrit Date: Tue, 4 Mar 2025 11:10:46 +0200 Subject: [PATCH 2/8] Add fencing credentials to platform none Signed-off-by: Michael Shitrit --- pkg/types/common/common.go | 8 ++++++++ pkg/types/none/platform.go | 8 +++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/pkg/types/common/common.go b/pkg/types/common/common.go index 0a489da968..fb957adaff 100644 --- a/pkg/types/common/common.go +++ b/pkg/types/common/common.go @@ -9,6 +9,14 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" ) +// FencingCredential stores the information about a baremetal host's management controller. +type FencingCredential struct { + HostName string `json:"hostName,omitempty" validate:"required,uniqueField"` + Username string `json:"username" validate:"required"` + Password string `json:"password" validate:"required"` + Address string `json:"address" validate:"required,uniqueField"` +} + // ValidateUniqueAndRequiredFields validated unique fields are indeed unique and that required fields exist on a generic element. func ValidateUniqueAndRequiredFields[T any](elements []T, fldPath *field.Path, filter validator.FilterFunc, fieldName string) field.ErrorList { errs := field.ErrorList{} diff --git a/pkg/types/none/platform.go b/pkg/types/none/platform.go index 32ac0f07a5..92761186a4 100644 --- a/pkg/types/none/platform.go +++ b/pkg/types/none/platform.go @@ -1,5 +1,11 @@ package none +import "github.com/openshift/installer/pkg/types/common" + // Platform stores any global configuration used for generic // platforms. -type Platform struct{} +type Platform struct { + // FencingCredentials stores the information about a baremetal host's management controller. + // +optional + FencingCredentials []*common.FencingCredential `json:"fencingCredentials,omitempty"` +} From d722d7e2a63c2b325c35fde3e4cf1ea6a74f675f Mon Sep 17 00:00:00 2001 From: Michael Shitrit Date: Tue, 4 Mar 2025 12:00:25 +0200 Subject: [PATCH 3/8] add FencingCredentials validations to platform none Signed-off-by: Michael Shitrit --- pkg/asset/agent/installconfig.go | 14 ++++++++++++++ .../installconfig/platformprovisioncheck.go | 8 +++++++- pkg/types/common/common.go | 11 +++++++++++ pkg/types/none/validation/platform.go | 17 +++++++++++++++++ 4 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 pkg/types/none/validation/platform.go diff --git a/pkg/asset/agent/installconfig.go b/pkg/asset/agent/installconfig.go index 107e4cde3a..f32c904d76 100644 --- a/pkg/asset/agent/installconfig.go +++ b/pkg/asset/agent/installconfig.go @@ -20,6 +20,7 @@ import ( baremetalvalidation "github.com/openshift/installer/pkg/types/baremetal/validation" "github.com/openshift/installer/pkg/types/external" "github.com/openshift/installer/pkg/types/none" + nonevalidation "github.com/openshift/installer/pkg/types/none/validation" "github.com/openshift/installer/pkg/types/validation" "github.com/openshift/installer/pkg/types/vsphere" ) @@ -150,6 +151,10 @@ func (a *OptionalInstallConfig) validatePlatformsByName(installConfig *types.Ins allErrs = append(allErrs, a.validateBMCConfig(installConfig)...) } + if installConfig.Platform.Name() == none.Name { + allErrs = append(allErrs, a.validateFencingCredentials(installConfig)...) + } + return allErrs } @@ -361,6 +366,15 @@ func (a *OptionalInstallConfig) validateBMCConfig(installConfig *types.InstallCo return allErrs } +func (a *OptionalInstallConfig) validateFencingCredentials(installConfig *types.InstallConfig) field.ErrorList { + var allErrs field.ErrorList + + fieldPath := field.NewPath("Platform", "None") + allErrs = append(allErrs, nonevalidation.ValidateFencingCredentials(installConfig.Platform.None.FencingCredentials, fieldPath)...) + + return allErrs +} + func warnUnusedConfig(installConfig *types.InstallConfig) { for i, compute := range installConfig.Compute { if compute.Hyperthreading != "Enabled" { diff --git a/pkg/asset/installconfig/platformprovisioncheck.go b/pkg/asset/installconfig/platformprovisioncheck.go index ea240e3534..3240af7eef 100644 --- a/pkg/asset/installconfig/platformprovisioncheck.go +++ b/pkg/asset/installconfig/platformprovisioncheck.go @@ -27,6 +27,7 @@ import ( "github.com/openshift/installer/pkg/types/gcp" "github.com/openshift/installer/pkg/types/ibmcloud" "github.com/openshift/installer/pkg/types/none" + nonevalidation "github.com/openshift/installer/pkg/types/none/validation" "github.com/openshift/installer/pkg/types/nutanix" "github.com/openshift/installer/pkg/types/openstack" "github.com/openshift/installer/pkg/types/ovirt" @@ -173,7 +174,12 @@ func (a *PlatformProvisionCheck) Generate(ctx context.Context, dependencies asse if err != nil { return err } - case external.Name, none.Name: + case none.Name: + err := nonevalidation.ValidateFencingCredentials(ic.Config.None.FencingCredentials, field.NewPath("Platform", "None")).ToAggregate() + if err != nil { + return err + } + case external.Name: // no special provisioning requirements to check case nutanix.Name: err := nutanixconfig.ValidateForProvisioning(ic.Config) diff --git a/pkg/types/common/common.go b/pkg/types/common/common.go index fb957adaff..7ca9139298 100644 --- a/pkg/types/common/common.go +++ b/pkg/types/common/common.go @@ -72,3 +72,14 @@ func ValidateUniqueAndRequiredFields[T any](elements []T, fldPath *field.Path, f } return errs } + +// ValidateTwoFencingCredentials in case fencing credentials exists validates there are exactly 2. +func ValidateTwoFencingCredentials(fencingCredentials []*FencingCredential, fldPath *field.Path) field.ErrorList { + errs := field.ErrorList{} + fencingCredentialsLength := len(fencingCredentials) + if fencingCredentialsLength > 0 && fencingCredentialsLength != 2 { + errs = append(errs, field.Forbidden(fldPath, fmt.Sprintf("there should be exactly two fencingCredentials to support the two node cluster, instead %d fencingCredentials were found", fencingCredentialsLength))) + } + + return errs +} diff --git a/pkg/types/none/validation/platform.go b/pkg/types/none/validation/platform.go new file mode 100644 index 0000000000..039a47dd00 --- /dev/null +++ b/pkg/types/none/validation/platform.go @@ -0,0 +1,17 @@ +package validation + +import ( + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/openshift/installer/pkg/types/common" +) + +// ValidateFencingCredentials checks that the provided fencing credentials are valid. +func ValidateFencingCredentials(fencingCredentials []*common.FencingCredential, fldPath *field.Path) (errors field.ErrorList) { + allErrs := field.ErrorList{} + + allErrs = append(allErrs, common.ValidateUniqueAndRequiredFields(fencingCredentials, fldPath, func([]byte) bool { return false }, "fencingCredentials")...) + allErrs = append(allErrs, common.ValidateTwoFencingCredentials(fencingCredentials, fldPath.Child("FencingCredentials"))...) + + return allErrs +} From 8a2acaeb7f4d6068535c76ef52858f2e122613ab Mon Sep 17 00:00:00 2001 From: Michael Shitrit Date: Wed, 26 Feb 2025 15:59:05 +0200 Subject: [PATCH 4/8] add Unit tests for validation logic Signed-off-by: Michael Shitrit --- pkg/types/none/validation/platform_test.go | 188 +++++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 pkg/types/none/validation/platform_test.go diff --git a/pkg/types/none/validation/platform_test.go b/pkg/types/none/validation/platform_test.go new file mode 100644 index 0000000000..8c86ac8347 --- /dev/null +++ b/pkg/types/none/validation/platform_test.go @@ -0,0 +1,188 @@ +package validation + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/openshift/installer/pkg/types" + "github.com/openshift/installer/pkg/types/common" + "github.com/openshift/installer/pkg/types/none" +) + +func TestValidateProvisioning(t *testing.T) { + cases := []struct { + name string + config *types.InstallConfig + platform *none.Platform + expected string + }{ + { + name: "valid_empty_credentials", + platform: platform(). + FencingCredentials().build(), + expected: "", + }, + { + name: "valid_two_credentials", + platform: platform(). + FencingCredentials(fc1(), + fc2()).build(), + expected: "", + }, + { + name: "invalid_number_of_credentials", + platform: platform(). + FencingCredentials(fc1(), + fc2(), + fc3()).build(), + expected: "Forbidden: there should be exactly two fencingCredentials to support the two node cluster, instead 3 fencingCredentials were found", + }, + { + name: "duplicate_bmc_address", + platform: platform(). + FencingCredentials( + fc1().BMCAddress("ipmi://192.168.111.1"), + fc2().BMCAddress("ipmi://192.168.111.1")).build(), + expected: "none.fencingCredentials\\[1\\].Address: Duplicate value: \"ipmi://192.168.111.1\"", + }, + { + name: "bmc_address_required", + platform: platform(). + FencingCredentials(fc1().BMCAddress("")).build(), + expected: "none.fencingCredentials\\[0\\].Address: Required value: missing Address", + }, + { + name: "bmc_username_required", + platform: platform(). + FencingCredentials(fc1().BMCUsername("")).build(), + expected: "none.fencingCredentials\\[0\\].Username: Required value: missing Username", + }, + { + name: "bmc_password_required", + platform: platform(). + FencingCredentials(fc1().BMCPassword("")).build(), + expected: "none.fencingCredentials\\[0\\].Password: Required value: missing Password", + }, + { + name: "host_name_required", + platform: platform(). + FencingCredentials(fc1().HostName("")).build(), + expected: "none.fencingCredentials\\[0\\].HostName: Required value: missing HostName", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // Build default wrapping installConfig + if tc.config == nil { + tc.config = installConfig().build() + } + tc.config.None = tc.platform + + err := ValidateFencingCredentials(tc.platform.FencingCredentials, field.NewPath("none")).ToAggregate() + + if tc.expected == "" { + assert.NoError(t, err) + } else { + assert.Regexp(t, tc.expected, err) + } + }) + } +} + +type fcBuilder struct { + common.FencingCredential +} + +func fc1() *fcBuilder { + return &fcBuilder{ + common.FencingCredential{ + HostName: "host1", + Username: "root", + Password: "password", + Address: "ipmi://192.168.111.1", + }, + } +} + +func fc2() *fcBuilder { + return &fcBuilder{ + common.FencingCredential{ + HostName: "host2", + Username: "root", + Password: "password", + Address: "ipmi://192.168.111.2", + }, + } +} + +func fc3() *fcBuilder { + return &fcBuilder{ + common.FencingCredential{ + HostName: "host3", + Username: "root", + Password: "password", + Address: "ipmi://192.168.111.3", + }, + } +} + +func (hb *fcBuilder) build() *common.FencingCredential { + return &hb.FencingCredential +} + +func (hb *fcBuilder) HostName(value string) *fcBuilder { + hb.FencingCredential.HostName = value + return hb +} + +func (hb *fcBuilder) BMCAddress(value string) *fcBuilder { + hb.FencingCredential.Address = value + return hb +} + +func (hb *fcBuilder) BMCUsername(value string) *fcBuilder { + hb.FencingCredential.Username = value + return hb +} + +func (hb *fcBuilder) BMCPassword(value string) *fcBuilder { + hb.FencingCredential.Password = value + return hb +} + +type platformBuilder struct { + none.Platform +} + +func platform() *platformBuilder { + return &platformBuilder{ + none.Platform{}} +} + +func (pb *platformBuilder) build() *none.Platform { + return &pb.Platform +} + +func (pb *platformBuilder) FencingCredentials(builders ...*fcBuilder) *platformBuilder { + pb.Platform.FencingCredentials = nil + for _, builder := range builders { + pb.Platform.FencingCredentials = append(pb.Platform.FencingCredentials, builder.build()) + } + return pb +} + +type installConfigBuilder struct { + types.InstallConfig +} + +func installConfig() *installConfigBuilder { + return &installConfigBuilder{ + InstallConfig: types.InstallConfig{}, + } +} + +func (icb *installConfigBuilder) build() *types.InstallConfig { + return &icb.InstallConfig +} From 0f8a44292801b1bc8ccf6ef08861a6bb45670e32 Mon Sep 17 00:00:00 2001 From: Michael Shitrit Date: Tue, 4 Mar 2025 17:28:52 +0200 Subject: [PATCH 5/8] Add a feature gate Signed-off-by: Michael Shitrit --- pkg/types/none/validation/featuregates.go | 21 +++++++++++++++ pkg/types/validation/featuregate_test.go | 31 ++++++++++++++++++++++ pkg/types/validation/installconfig.go | 3 +++ pkg/types/validation/installconfig_test.go | 4 +++ 4 files changed, 59 insertions(+) create mode 100644 pkg/types/none/validation/featuregates.go diff --git a/pkg/types/none/validation/featuregates.go b/pkg/types/none/validation/featuregates.go new file mode 100644 index 0000000000..3f172b0ad9 --- /dev/null +++ b/pkg/types/none/validation/featuregates.go @@ -0,0 +1,21 @@ +package validation + +import ( + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/openshift/api/features" + "github.com/openshift/installer/pkg/types" + "github.com/openshift/installer/pkg/types/featuregates" +) + +// GatedFeatures determines all of the install config fields that should +// be validated to ensure that the proper featuregate is enabled when the field is used. +func GatedFeatures(c *types.InstallConfig) []featuregates.GatedInstallConfigFeature { + return []featuregates.GatedInstallConfigFeature{ + { + FeatureGateName: features.FeatureGateDualReplica, + Condition: c.None.FencingCredentials != nil, + Field: field.NewPath("platform", "none", "fencingCredentials"), + }, + } +} diff --git a/pkg/types/validation/featuregate_test.go b/pkg/types/validation/featuregate_test.go index 2de53a4326..1a74412b31 100644 --- a/pkg/types/validation/featuregate_test.go +++ b/pkg/types/validation/featuregate_test.go @@ -7,6 +7,7 @@ import ( v1 "github.com/openshift/api/config/v1" "github.com/openshift/installer/pkg/types" + "github.com/openshift/installer/pkg/types/common" "github.com/openshift/installer/pkg/types/dns" "github.com/openshift/installer/pkg/types/vsphere" ) @@ -125,6 +126,36 @@ func TestFeatureGates(t *testing.T) { return c }(), }, + { + name: "None fencingCredentials is not allowed with Feature Gates disabled", + installConfig: func() *types.InstallConfig { + c := validInstallConfig() + c.None = validNonePlatform() + c.None.FencingCredentials = append(c.None.FencingCredentials, []*common.FencingCredential{{HostName: "host1"}, {HostName: "host2"}}...) + return c + }(), + expected: `^platform.none.fencingCredentials: Forbidden: this field is protected by the DualReplica feature gate which must be enabled through either the TechPreviewNoUpgrade or CustomNoUpgrade feature set$`, + }, + { + name: "None fencingCredentials is allowed with TechPreviewNoUpgrade Feature Set", + installConfig: func() *types.InstallConfig { + c := validInstallConfig() + c.FeatureSet = v1.TechPreviewNoUpgrade + c.None = validNonePlatform() + c.None.FencingCredentials = append(c.None.FencingCredentials, []*common.FencingCredential{{HostName: "host1"}, {HostName: "host2"}}...) + return c + }(), + }, + { + name: "None fencingCredentials is allowed with DevPreviewNoUpgrade Feature Set", + installConfig: func() *types.InstallConfig { + c := validInstallConfig() + c.FeatureSet = v1.DevPreviewNoUpgrade + c.None = validNonePlatform() + c.None.FencingCredentials = append(c.None.FencingCredentials, []*common.FencingCredential{{HostName: "host1"}, {HostName: "host2"}}...) + return c + }(), + }, } for _, tc := range cases { diff --git a/pkg/types/validation/installconfig.go b/pkg/types/validation/installconfig.go index 94a4f4841c..8a3654fc4b 100644 --- a/pkg/types/validation/installconfig.go +++ b/pkg/types/validation/installconfig.go @@ -37,6 +37,7 @@ import ( gcpvalidation "github.com/openshift/installer/pkg/types/gcp/validation" "github.com/openshift/installer/pkg/types/ibmcloud" ibmcloudvalidation "github.com/openshift/installer/pkg/types/ibmcloud/validation" + nonevalidation "github.com/openshift/installer/pkg/types/none/validation" "github.com/openshift/installer/pkg/types/nutanix" nutanixvalidation "github.com/openshift/installer/pkg/types/nutanix/validation" "github.com/openshift/installer/pkg/types/openstack" @@ -1460,6 +1461,8 @@ func validateGatedFeatures(c *types.InstallConfig) field.ErrorList { gatedFeatures = append(gatedFeatures, gcpvalidation.GatedFeatures(c)...) case c.VSphere != nil: gatedFeatures = append(gatedFeatures, vspherevalidation.GatedFeatures(c)...) + case c.None != nil: + gatedFeatures = append(gatedFeatures, nonevalidation.GatedFeatures(c)...) case c.AWS != nil: gatedFeatures = append(gatedFeatures, awsvalidation.GatedFeatures(c)...) } diff --git a/pkg/types/validation/installconfig_test.go b/pkg/types/validation/installconfig_test.go index b0d5cdbc80..2dec77ab6e 100644 --- a/pkg/types/validation/installconfig_test.go +++ b/pkg/types/validation/installconfig_test.go @@ -205,6 +205,10 @@ func validNutanixPlatform() *nutanix.Platform { } } +func validNonePlatform() *none.Platform { + return &none.Platform{} +} + func validIPv4NetworkingConfig() *types.Networking { return &types.Networking{ NetworkType: "OVNKubernetes", From 1881d649e03fc4c50085a13e35f459a2c26116e6 Mon Sep 17 00:00:00 2001 From: Michael Shitrit Date: Thu, 13 Mar 2025 10:26:45 +0200 Subject: [PATCH 6/8] Apply review feedback: - Add validation for number of CP replicas Signed-off-by: Michael Shitrit --- pkg/asset/agent/installconfig.go | 5 +-- .../installconfig/platformprovisioncheck.go | 2 +- pkg/types/common/common.go | 15 ++++--- pkg/types/none/validation/platform.go | 8 ++-- pkg/types/none/validation/platform_test.go | 40 ++++++++++++++----- 5 files changed, 47 insertions(+), 23 deletions(-) diff --git a/pkg/asset/agent/installconfig.go b/pkg/asset/agent/installconfig.go index f32c904d76..8c1f146f5e 100644 --- a/pkg/asset/agent/installconfig.go +++ b/pkg/asset/agent/installconfig.go @@ -368,10 +368,7 @@ func (a *OptionalInstallConfig) validateBMCConfig(installConfig *types.InstallCo func (a *OptionalInstallConfig) validateFencingCredentials(installConfig *types.InstallConfig) field.ErrorList { var allErrs field.ErrorList - - fieldPath := field.NewPath("Platform", "None") - allErrs = append(allErrs, nonevalidation.ValidateFencingCredentials(installConfig.Platform.None.FencingCredentials, fieldPath)...) - + allErrs = append(allErrs, nonevalidation.ValidateFencingCredentials(installConfig)...) return allErrs } diff --git a/pkg/asset/installconfig/platformprovisioncheck.go b/pkg/asset/installconfig/platformprovisioncheck.go index 3240af7eef..7fda8fed5c 100644 --- a/pkg/asset/installconfig/platformprovisioncheck.go +++ b/pkg/asset/installconfig/platformprovisioncheck.go @@ -175,7 +175,7 @@ func (a *PlatformProvisionCheck) Generate(ctx context.Context, dependencies asse return err } case none.Name: - err := nonevalidation.ValidateFencingCredentials(ic.Config.None.FencingCredentials, field.NewPath("Platform", "None")).ToAggregate() + err := nonevalidation.ValidateFencingCredentials(ic.Config).ToAggregate() if err != nil { return err } diff --git a/pkg/types/common/common.go b/pkg/types/common/common.go index 7ca9139298..100d99d170 100644 --- a/pkg/types/common/common.go +++ b/pkg/types/common/common.go @@ -74,12 +74,17 @@ func ValidateUniqueAndRequiredFields[T any](elements []T, fldPath *field.Path, f } // ValidateTwoFencingCredentials in case fencing credentials exists validates there are exactly 2. -func ValidateTwoFencingCredentials(fencingCredentials []*FencingCredential, fldPath *field.Path) field.ErrorList { +func ValidateTwoFencingCredentials(numOfCpReplicas int64, fencingCredentials []*FencingCredential, fldPath *field.Path) field.ErrorList { errs := field.ErrorList{} - fencingCredentialsLength := len(fencingCredentials) - if fencingCredentialsLength > 0 && fencingCredentialsLength != 2 { - errs = append(errs, field.Forbidden(fldPath, fmt.Sprintf("there should be exactly two fencingCredentials to support the two node cluster, instead %d fencingCredentials were found", fencingCredentialsLength))) + numOfFencingCredentials := len(fencingCredentials) + if numOfCpReplicas == 2 { + if numOfFencingCredentials != 2 { + errs = append(errs, field.Forbidden(fldPath, fmt.Sprintf("there should be exactly two fencingCredentials to support the two node cluster, instead %d fencingCredentials were found", numOfFencingCredentials))) + } + } else { + if numOfFencingCredentials != 0 { + errs = append(errs, field.Forbidden(fldPath, fmt.Sprintf("there should not be any fencingCredentials configured for a non dual replica control plane (Two Nodes Fencing) cluster, instead %d fencingCredentials were found", numOfFencingCredentials))) + } } - return errs } diff --git a/pkg/types/none/validation/platform.go b/pkg/types/none/validation/platform.go index 039a47dd00..bd5be48033 100644 --- a/pkg/types/none/validation/platform.go +++ b/pkg/types/none/validation/platform.go @@ -3,15 +3,17 @@ package validation import ( "k8s.io/apimachinery/pkg/util/validation/field" + "github.com/openshift/installer/pkg/types" "github.com/openshift/installer/pkg/types/common" ) // ValidateFencingCredentials checks that the provided fencing credentials are valid. -func ValidateFencingCredentials(fencingCredentials []*common.FencingCredential, fldPath *field.Path) (errors field.ErrorList) { +func ValidateFencingCredentials(installConfig *types.InstallConfig) (errors field.ErrorList) { + fldPath := field.NewPath("platform", "none") + fencingCredentials := installConfig.Platform.None.FencingCredentials allErrs := field.ErrorList{} - allErrs = append(allErrs, common.ValidateUniqueAndRequiredFields(fencingCredentials, fldPath, func([]byte) bool { return false }, "fencingCredentials")...) - allErrs = append(allErrs, common.ValidateTwoFencingCredentials(fencingCredentials, fldPath.Child("FencingCredentials"))...) + allErrs = append(allErrs, common.ValidateTwoFencingCredentials(*installConfig.ControlPlane.Replicas, fencingCredentials, fldPath.Child("fencingCredentials"))...) return allErrs } diff --git a/pkg/types/none/validation/platform_test.go b/pkg/types/none/validation/platform_test.go index 8c86ac8347..5dcc11b520 100644 --- a/pkg/types/none/validation/platform_test.go +++ b/pkg/types/none/validation/platform_test.go @@ -4,7 +4,6 @@ import ( "testing" "github.com/stretchr/testify/assert" - "k8s.io/apimachinery/pkg/util/validation/field" "github.com/openshift/installer/pkg/types" "github.com/openshift/installer/pkg/types/common" @@ -19,20 +18,23 @@ func TestValidateProvisioning(t *testing.T) { expected string }{ { - name: "valid_empty_credentials", + config: installConfig().AddCpReplicas(3).build(), + name: "valid_empty_credentials", platform: platform(). FencingCredentials().build(), expected: "", }, { - name: "valid_two_credentials", + config: installConfig().AddCpReplicas(2).build(), + name: "valid_two_credentials", platform: platform(). FencingCredentials(fc1(), fc2()).build(), expected: "", }, { - name: "invalid_number_of_credentials", + config: installConfig().AddCpReplicas(2).build(), + name: "invalid_number_of_credentials_for_dual_replica", platform: platform(). FencingCredentials(fc1(), fc2(), @@ -40,7 +42,16 @@ func TestValidateProvisioning(t *testing.T) { expected: "Forbidden: there should be exactly two fencingCredentials to support the two node cluster, instead 3 fencingCredentials were found", }, { - name: "duplicate_bmc_address", + config: installConfig().AddCpReplicas(3).build(), + name: "invalid_number_of_credentials_for_non_dual_replica", + platform: platform(). + FencingCredentials(fc1(), + fc2()).build(), + expected: "platform.none.fencingCredentials: Forbidden: there should not be any fencingCredentials configured for a non dual replica control plane \\(Two Nodes Fencing\\) cluster, instead 2 fencingCredentials were found", + }, + { + config: installConfig().AddCpReplicas(2).build(), + name: "duplicate_bmc_address", platform: platform(). FencingCredentials( fc1().BMCAddress("ipmi://192.168.111.1"), @@ -48,25 +59,29 @@ func TestValidateProvisioning(t *testing.T) { expected: "none.fencingCredentials\\[1\\].Address: Duplicate value: \"ipmi://192.168.111.1\"", }, { - name: "bmc_address_required", + config: installConfig().AddCpReplicas(2).build(), + name: "bmc_address_required", platform: platform(). FencingCredentials(fc1().BMCAddress("")).build(), expected: "none.fencingCredentials\\[0\\].Address: Required value: missing Address", }, { - name: "bmc_username_required", + config: installConfig().AddCpReplicas(2).build(), + name: "bmc_username_required", platform: platform(). FencingCredentials(fc1().BMCUsername("")).build(), expected: "none.fencingCredentials\\[0\\].Username: Required value: missing Username", }, { - name: "bmc_password_required", + config: installConfig().AddCpReplicas(2).build(), + name: "bmc_password_required", platform: platform(). FencingCredentials(fc1().BMCPassword("")).build(), expected: "none.fencingCredentials\\[0\\].Password: Required value: missing Password", }, { - name: "host_name_required", + config: installConfig().AddCpReplicas(2).build(), + name: "host_name_required", platform: platform(). FencingCredentials(fc1().HostName("")).build(), expected: "none.fencingCredentials\\[0\\].HostName: Required value: missing HostName", @@ -80,7 +95,7 @@ func TestValidateProvisioning(t *testing.T) { } tc.config.None = tc.platform - err := ValidateFencingCredentials(tc.platform.FencingCredentials, field.NewPath("none")).ToAggregate() + err := ValidateFencingCredentials(tc.config).ToAggregate() if tc.expected == "" { assert.NoError(t, err) @@ -183,6 +198,11 @@ func installConfig() *installConfigBuilder { } } +func (icb *installConfigBuilder) AddCpReplicas(numOfCpReplicas int64) *installConfigBuilder { + icb.InstallConfig.ControlPlane = &types.MachinePool{Replicas: &numOfCpReplicas} + return icb +} + func (icb *installConfigBuilder) build() *types.InstallConfig { return &icb.InstallConfig } From b8edc890a68b81ece8da8390c76e28deb0468c77 Mon Sep 17 00:00:00 2001 From: Michael Shitrit Date: Tue, 18 Mar 2025 18:33:08 +0200 Subject: [PATCH 7/8] Refactor Structure - Fencing credentials refactored to a Fencing parent holding a list of credentials - Fencing credentials moved under ControlPlane - Validations and tests updated Signed-off-by: Michael Shitrit --- .../install.openshift.io_installconfigs.yaml | 78 +++++++ pkg/asset/agent/installconfig.go | 11 - .../installconfig/platformprovisioncheck.go | 8 +- pkg/types/common/common.go | 24 -- pkg/types/machinepools.go | 19 ++ pkg/types/none/platform.go | 8 +- pkg/types/none/validation/featuregates.go | 21 -- pkg/types/none/validation/platform.go | 19 -- pkg/types/none/validation/platform_test.go | 208 ----------------- pkg/types/validation/featuregate_test.go | 31 --- pkg/types/validation/installconfig.go | 42 +++- pkg/types/validation/installconfig_test.go | 211 +++++++++++++++++- 12 files changed, 345 insertions(+), 335 deletions(-) delete mode 100644 pkg/types/none/validation/featuregates.go delete mode 100644 pkg/types/none/validation/platform.go delete mode 100644 pkg/types/none/validation/platform_test.go diff --git a/data/data/install.openshift.io_installconfigs.yaml b/data/data/install.openshift.io_installconfigs.yaml index 827005223d..4cdfc90402 100644 --- a/data/data/install.openshift.io_installconfigs.yaml +++ b/data/data/install.openshift.io_installconfigs.yaml @@ -57,6 +57,32 @@ spec: - "" - amd64 type: string + fencing: + description: Fencing stores the information about a baremetal host's + management controller. + properties: + credentials: + description: Credentials stores the information about a baremetal + host's management controller. + items: + description: Credential stores the information about a baremetal + host's management controller. + properties: + address: + type: string + hostName: + type: string + password: + type: string + username: + type: string + required: + - address + - password + - username + type: object + type: array + type: object hyperthreading: default: Enabled description: |- @@ -1221,6 +1247,32 @@ spec: - "" - amd64 type: string + fencing: + description: Fencing stores the information about a baremetal host's + management controller. + properties: + credentials: + description: Credentials stores the information about a baremetal + host's management controller. + items: + description: Credential stores the information about a baremetal + host's management controller. + properties: + address: + type: string + hostName: + type: string + password: + type: string + username: + type: string + required: + - address + - password + - username + type: object + type: array + type: object hyperthreading: default: Enabled description: |- @@ -2324,6 +2376,32 @@ spec: - "" - amd64 type: string + fencing: + description: Fencing stores the information about a baremetal host's + management controller. + properties: + credentials: + description: Credentials stores the information about a baremetal + host's management controller. + items: + description: Credential stores the information about a baremetal + host's management controller. + properties: + address: + type: string + hostName: + type: string + password: + type: string + username: + type: string + required: + - address + - password + - username + type: object + type: array + type: object hyperthreading: default: Enabled description: |- diff --git a/pkg/asset/agent/installconfig.go b/pkg/asset/agent/installconfig.go index 8c1f146f5e..107e4cde3a 100644 --- a/pkg/asset/agent/installconfig.go +++ b/pkg/asset/agent/installconfig.go @@ -20,7 +20,6 @@ import ( baremetalvalidation "github.com/openshift/installer/pkg/types/baremetal/validation" "github.com/openshift/installer/pkg/types/external" "github.com/openshift/installer/pkg/types/none" - nonevalidation "github.com/openshift/installer/pkg/types/none/validation" "github.com/openshift/installer/pkg/types/validation" "github.com/openshift/installer/pkg/types/vsphere" ) @@ -151,10 +150,6 @@ func (a *OptionalInstallConfig) validatePlatformsByName(installConfig *types.Ins allErrs = append(allErrs, a.validateBMCConfig(installConfig)...) } - if installConfig.Platform.Name() == none.Name { - allErrs = append(allErrs, a.validateFencingCredentials(installConfig)...) - } - return allErrs } @@ -366,12 +361,6 @@ func (a *OptionalInstallConfig) validateBMCConfig(installConfig *types.InstallCo return allErrs } -func (a *OptionalInstallConfig) validateFencingCredentials(installConfig *types.InstallConfig) field.ErrorList { - var allErrs field.ErrorList - allErrs = append(allErrs, nonevalidation.ValidateFencingCredentials(installConfig)...) - return allErrs -} - func warnUnusedConfig(installConfig *types.InstallConfig) { for i, compute := range installConfig.Compute { if compute.Hyperthreading != "Enabled" { diff --git a/pkg/asset/installconfig/platformprovisioncheck.go b/pkg/asset/installconfig/platformprovisioncheck.go index 7fda8fed5c..ea240e3534 100644 --- a/pkg/asset/installconfig/platformprovisioncheck.go +++ b/pkg/asset/installconfig/platformprovisioncheck.go @@ -27,7 +27,6 @@ import ( "github.com/openshift/installer/pkg/types/gcp" "github.com/openshift/installer/pkg/types/ibmcloud" "github.com/openshift/installer/pkg/types/none" - nonevalidation "github.com/openshift/installer/pkg/types/none/validation" "github.com/openshift/installer/pkg/types/nutanix" "github.com/openshift/installer/pkg/types/openstack" "github.com/openshift/installer/pkg/types/ovirt" @@ -174,12 +173,7 @@ func (a *PlatformProvisionCheck) Generate(ctx context.Context, dependencies asse if err != nil { return err } - case none.Name: - err := nonevalidation.ValidateFencingCredentials(ic.Config).ToAggregate() - if err != nil { - return err - } - case external.Name: + case external.Name, none.Name: // no special provisioning requirements to check case nutanix.Name: err := nutanixconfig.ValidateForProvisioning(ic.Config) diff --git a/pkg/types/common/common.go b/pkg/types/common/common.go index 100d99d170..0a489da968 100644 --- a/pkg/types/common/common.go +++ b/pkg/types/common/common.go @@ -9,14 +9,6 @@ import ( "k8s.io/apimachinery/pkg/util/validation/field" ) -// FencingCredential stores the information about a baremetal host's management controller. -type FencingCredential struct { - HostName string `json:"hostName,omitempty" validate:"required,uniqueField"` - Username string `json:"username" validate:"required"` - Password string `json:"password" validate:"required"` - Address string `json:"address" validate:"required,uniqueField"` -} - // ValidateUniqueAndRequiredFields validated unique fields are indeed unique and that required fields exist on a generic element. func ValidateUniqueAndRequiredFields[T any](elements []T, fldPath *field.Path, filter validator.FilterFunc, fieldName string) field.ErrorList { errs := field.ErrorList{} @@ -72,19 +64,3 @@ func ValidateUniqueAndRequiredFields[T any](elements []T, fldPath *field.Path, f } return errs } - -// ValidateTwoFencingCredentials in case fencing credentials exists validates there are exactly 2. -func ValidateTwoFencingCredentials(numOfCpReplicas int64, fencingCredentials []*FencingCredential, fldPath *field.Path) field.ErrorList { - errs := field.ErrorList{} - numOfFencingCredentials := len(fencingCredentials) - if numOfCpReplicas == 2 { - if numOfFencingCredentials != 2 { - errs = append(errs, field.Forbidden(fldPath, fmt.Sprintf("there should be exactly two fencingCredentials to support the two node cluster, instead %d fencingCredentials were found", numOfFencingCredentials))) - } - } else { - if numOfFencingCredentials != 0 { - errs = append(errs, field.Forbidden(fldPath, fmt.Sprintf("there should not be any fencingCredentials configured for a non dual replica control plane (Two Nodes Fencing) cluster, instead %d fencingCredentials were found", numOfFencingCredentials))) - } - } - return errs -} diff --git a/pkg/types/machinepools.go b/pkg/types/machinepools.go index 2283ba0b1d..aabcbee2b1 100644 --- a/pkg/types/machinepools.go +++ b/pkg/types/machinepools.go @@ -78,6 +78,10 @@ type MachinePool struct { // +kubebuilder:default=amd64 // +optional Architecture Architecture `json:"architecture,omitempty"` + + // Fencing stores the information about a baremetal host's management controller. + // +optional + Fencing *Fencing `json:"fencing,omitempty"` } // MachinePoolPlatform is the platform-specific configuration for a machine @@ -145,3 +149,18 @@ func (p *MachinePoolPlatform) Name() string { return "" } } + +// Fencing stores the information about a baremetal host's management controller. +type Fencing struct { + // Credentials stores the information about a baremetal host's management controller. + // +optional + Credentials []*Credential `json:"credentials,omitempty"` +} + +// Credential stores the information about a baremetal host's management controller. +type Credential struct { + HostName string `json:"hostName,omitempty" validate:"required,uniqueField"` + Username string `json:"username" validate:"required"` + Password string `json:"password" validate:"required"` + Address string `json:"address" validate:"required,uniqueField"` +} diff --git a/pkg/types/none/platform.go b/pkg/types/none/platform.go index 92761186a4..32ac0f07a5 100644 --- a/pkg/types/none/platform.go +++ b/pkg/types/none/platform.go @@ -1,11 +1,5 @@ package none -import "github.com/openshift/installer/pkg/types/common" - // Platform stores any global configuration used for generic // platforms. -type Platform struct { - // FencingCredentials stores the information about a baremetal host's management controller. - // +optional - FencingCredentials []*common.FencingCredential `json:"fencingCredentials,omitempty"` -} +type Platform struct{} diff --git a/pkg/types/none/validation/featuregates.go b/pkg/types/none/validation/featuregates.go deleted file mode 100644 index 3f172b0ad9..0000000000 --- a/pkg/types/none/validation/featuregates.go +++ /dev/null @@ -1,21 +0,0 @@ -package validation - -import ( - "k8s.io/apimachinery/pkg/util/validation/field" - - "github.com/openshift/api/features" - "github.com/openshift/installer/pkg/types" - "github.com/openshift/installer/pkg/types/featuregates" -) - -// GatedFeatures determines all of the install config fields that should -// be validated to ensure that the proper featuregate is enabled when the field is used. -func GatedFeatures(c *types.InstallConfig) []featuregates.GatedInstallConfigFeature { - return []featuregates.GatedInstallConfigFeature{ - { - FeatureGateName: features.FeatureGateDualReplica, - Condition: c.None.FencingCredentials != nil, - Field: field.NewPath("platform", "none", "fencingCredentials"), - }, - } -} diff --git a/pkg/types/none/validation/platform.go b/pkg/types/none/validation/platform.go deleted file mode 100644 index bd5be48033..0000000000 --- a/pkg/types/none/validation/platform.go +++ /dev/null @@ -1,19 +0,0 @@ -package validation - -import ( - "k8s.io/apimachinery/pkg/util/validation/field" - - "github.com/openshift/installer/pkg/types" - "github.com/openshift/installer/pkg/types/common" -) - -// ValidateFencingCredentials checks that the provided fencing credentials are valid. -func ValidateFencingCredentials(installConfig *types.InstallConfig) (errors field.ErrorList) { - fldPath := field.NewPath("platform", "none") - fencingCredentials := installConfig.Platform.None.FencingCredentials - allErrs := field.ErrorList{} - allErrs = append(allErrs, common.ValidateUniqueAndRequiredFields(fencingCredentials, fldPath, func([]byte) bool { return false }, "fencingCredentials")...) - allErrs = append(allErrs, common.ValidateTwoFencingCredentials(*installConfig.ControlPlane.Replicas, fencingCredentials, fldPath.Child("fencingCredentials"))...) - - return allErrs -} diff --git a/pkg/types/none/validation/platform_test.go b/pkg/types/none/validation/platform_test.go deleted file mode 100644 index 5dcc11b520..0000000000 --- a/pkg/types/none/validation/platform_test.go +++ /dev/null @@ -1,208 +0,0 @@ -package validation - -import ( - "testing" - - "github.com/stretchr/testify/assert" - - "github.com/openshift/installer/pkg/types" - "github.com/openshift/installer/pkg/types/common" - "github.com/openshift/installer/pkg/types/none" -) - -func TestValidateProvisioning(t *testing.T) { - cases := []struct { - name string - config *types.InstallConfig - platform *none.Platform - expected string - }{ - { - config: installConfig().AddCpReplicas(3).build(), - name: "valid_empty_credentials", - platform: platform(). - FencingCredentials().build(), - expected: "", - }, - { - config: installConfig().AddCpReplicas(2).build(), - name: "valid_two_credentials", - platform: platform(). - FencingCredentials(fc1(), - fc2()).build(), - expected: "", - }, - { - config: installConfig().AddCpReplicas(2).build(), - name: "invalid_number_of_credentials_for_dual_replica", - platform: platform(). - FencingCredentials(fc1(), - fc2(), - fc3()).build(), - expected: "Forbidden: there should be exactly two fencingCredentials to support the two node cluster, instead 3 fencingCredentials were found", - }, - { - config: installConfig().AddCpReplicas(3).build(), - name: "invalid_number_of_credentials_for_non_dual_replica", - platform: platform(). - FencingCredentials(fc1(), - fc2()).build(), - expected: "platform.none.fencingCredentials: Forbidden: there should not be any fencingCredentials configured for a non dual replica control plane \\(Two Nodes Fencing\\) cluster, instead 2 fencingCredentials were found", - }, - { - config: installConfig().AddCpReplicas(2).build(), - name: "duplicate_bmc_address", - platform: platform(). - FencingCredentials( - fc1().BMCAddress("ipmi://192.168.111.1"), - fc2().BMCAddress("ipmi://192.168.111.1")).build(), - expected: "none.fencingCredentials\\[1\\].Address: Duplicate value: \"ipmi://192.168.111.1\"", - }, - { - config: installConfig().AddCpReplicas(2).build(), - name: "bmc_address_required", - platform: platform(). - FencingCredentials(fc1().BMCAddress("")).build(), - expected: "none.fencingCredentials\\[0\\].Address: Required value: missing Address", - }, - { - config: installConfig().AddCpReplicas(2).build(), - name: "bmc_username_required", - platform: platform(). - FencingCredentials(fc1().BMCUsername("")).build(), - expected: "none.fencingCredentials\\[0\\].Username: Required value: missing Username", - }, - { - config: installConfig().AddCpReplicas(2).build(), - name: "bmc_password_required", - platform: platform(). - FencingCredentials(fc1().BMCPassword("")).build(), - expected: "none.fencingCredentials\\[0\\].Password: Required value: missing Password", - }, - { - config: installConfig().AddCpReplicas(2).build(), - name: "host_name_required", - platform: platform(). - FencingCredentials(fc1().HostName("")).build(), - expected: "none.fencingCredentials\\[0\\].HostName: Required value: missing HostName", - }, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - // Build default wrapping installConfig - if tc.config == nil { - tc.config = installConfig().build() - } - tc.config.None = tc.platform - - err := ValidateFencingCredentials(tc.config).ToAggregate() - - if tc.expected == "" { - assert.NoError(t, err) - } else { - assert.Regexp(t, tc.expected, err) - } - }) - } -} - -type fcBuilder struct { - common.FencingCredential -} - -func fc1() *fcBuilder { - return &fcBuilder{ - common.FencingCredential{ - HostName: "host1", - Username: "root", - Password: "password", - Address: "ipmi://192.168.111.1", - }, - } -} - -func fc2() *fcBuilder { - return &fcBuilder{ - common.FencingCredential{ - HostName: "host2", - Username: "root", - Password: "password", - Address: "ipmi://192.168.111.2", - }, - } -} - -func fc3() *fcBuilder { - return &fcBuilder{ - common.FencingCredential{ - HostName: "host3", - Username: "root", - Password: "password", - Address: "ipmi://192.168.111.3", - }, - } -} - -func (hb *fcBuilder) build() *common.FencingCredential { - return &hb.FencingCredential -} - -func (hb *fcBuilder) HostName(value string) *fcBuilder { - hb.FencingCredential.HostName = value - return hb -} - -func (hb *fcBuilder) BMCAddress(value string) *fcBuilder { - hb.FencingCredential.Address = value - return hb -} - -func (hb *fcBuilder) BMCUsername(value string) *fcBuilder { - hb.FencingCredential.Username = value - return hb -} - -func (hb *fcBuilder) BMCPassword(value string) *fcBuilder { - hb.FencingCredential.Password = value - return hb -} - -type platformBuilder struct { - none.Platform -} - -func platform() *platformBuilder { - return &platformBuilder{ - none.Platform{}} -} - -func (pb *platformBuilder) build() *none.Platform { - return &pb.Platform -} - -func (pb *platformBuilder) FencingCredentials(builders ...*fcBuilder) *platformBuilder { - pb.Platform.FencingCredentials = nil - for _, builder := range builders { - pb.Platform.FencingCredentials = append(pb.Platform.FencingCredentials, builder.build()) - } - return pb -} - -type installConfigBuilder struct { - types.InstallConfig -} - -func installConfig() *installConfigBuilder { - return &installConfigBuilder{ - InstallConfig: types.InstallConfig{}, - } -} - -func (icb *installConfigBuilder) AddCpReplicas(numOfCpReplicas int64) *installConfigBuilder { - icb.InstallConfig.ControlPlane = &types.MachinePool{Replicas: &numOfCpReplicas} - return icb -} - -func (icb *installConfigBuilder) build() *types.InstallConfig { - return &icb.InstallConfig -} diff --git a/pkg/types/validation/featuregate_test.go b/pkg/types/validation/featuregate_test.go index 1a74412b31..2de53a4326 100644 --- a/pkg/types/validation/featuregate_test.go +++ b/pkg/types/validation/featuregate_test.go @@ -7,7 +7,6 @@ import ( v1 "github.com/openshift/api/config/v1" "github.com/openshift/installer/pkg/types" - "github.com/openshift/installer/pkg/types/common" "github.com/openshift/installer/pkg/types/dns" "github.com/openshift/installer/pkg/types/vsphere" ) @@ -126,36 +125,6 @@ func TestFeatureGates(t *testing.T) { return c }(), }, - { - name: "None fencingCredentials is not allowed with Feature Gates disabled", - installConfig: func() *types.InstallConfig { - c := validInstallConfig() - c.None = validNonePlatform() - c.None.FencingCredentials = append(c.None.FencingCredentials, []*common.FencingCredential{{HostName: "host1"}, {HostName: "host2"}}...) - return c - }(), - expected: `^platform.none.fencingCredentials: Forbidden: this field is protected by the DualReplica feature gate which must be enabled through either the TechPreviewNoUpgrade or CustomNoUpgrade feature set$`, - }, - { - name: "None fencingCredentials is allowed with TechPreviewNoUpgrade Feature Set", - installConfig: func() *types.InstallConfig { - c := validInstallConfig() - c.FeatureSet = v1.TechPreviewNoUpgrade - c.None = validNonePlatform() - c.None.FencingCredentials = append(c.None.FencingCredentials, []*common.FencingCredential{{HostName: "host1"}, {HostName: "host2"}}...) - return c - }(), - }, - { - name: "None fencingCredentials is allowed with DevPreviewNoUpgrade Feature Set", - installConfig: func() *types.InstallConfig { - c := validInstallConfig() - c.FeatureSet = v1.DevPreviewNoUpgrade - c.None = validNonePlatform() - c.None.FencingCredentials = append(c.None.FencingCredentials, []*common.FencingCredential{{HostName: "host1"}, {HostName: "host2"}}...) - return c - }(), - }, } for _, tc := range cases { diff --git a/pkg/types/validation/installconfig.go b/pkg/types/validation/installconfig.go index 8a3654fc4b..f1553ab1e2 100644 --- a/pkg/types/validation/installconfig.go +++ b/pkg/types/validation/installconfig.go @@ -31,13 +31,13 @@ import ( azurevalidation "github.com/openshift/installer/pkg/types/azure/validation" "github.com/openshift/installer/pkg/types/baremetal" baremetalvalidation "github.com/openshift/installer/pkg/types/baremetal/validation" + "github.com/openshift/installer/pkg/types/common" "github.com/openshift/installer/pkg/types/external" "github.com/openshift/installer/pkg/types/featuregates" "github.com/openshift/installer/pkg/types/gcp" gcpvalidation "github.com/openshift/installer/pkg/types/gcp/validation" "github.com/openshift/installer/pkg/types/ibmcloud" ibmcloudvalidation "github.com/openshift/installer/pkg/types/ibmcloud/validation" - nonevalidation "github.com/openshift/installer/pkg/types/none/validation" "github.com/openshift/installer/pkg/types/nutanix" nutanixvalidation "github.com/openshift/installer/pkg/types/nutanix/validation" "github.com/openshift/installer/pkg/types/openstack" @@ -123,6 +123,13 @@ func ValidateInstallConfig(c *types.InstallConfig, usingAgentMethod bool) field. 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"))...) + if len(c.ControlPlane.Fencing.Credentials) > 0 { + if c.EnabledFeatureGates().Enabled(features.FeatureGateDualReplica) { + allErrs = append(allErrs, validateFencingCredentials(c)...) + } else { + allErrs = append(allErrs, field.Forbidden(field.NewPath("controlPlane").Child("fencing").Child("credentials"), fmt.Sprintf("%s feature must be enabled in order to use Two Node Fencing (TNF) cluster deployment", features.FeatureGateDualReplica))) + } + } } else { allErrs = append(allErrs, field.Required(field.NewPath("controlPlane"), "controlPlane is required")) } @@ -1461,8 +1468,6 @@ func validateGatedFeatures(c *types.InstallConfig) field.ErrorList { gatedFeatures = append(gatedFeatures, gcpvalidation.GatedFeatures(c)...) case c.VSphere != nil: gatedFeatures = append(gatedFeatures, vspherevalidation.GatedFeatures(c)...) - case c.None != nil: - gatedFeatures = append(gatedFeatures, nonevalidation.GatedFeatures(c)...) case c.AWS != nil: gatedFeatures = append(gatedFeatures, awsvalidation.GatedFeatures(c)...) } @@ -1545,3 +1550,34 @@ func isV4NodeSubnetLargeEnough(cn []types.ClusterNetworkEntry, nodeSubnet *ipnet // reserve one IP for the gw, one IP for network and one for broadcasting return maxNodesNum < (1<<(addrLen-intSubnetMask) - 3), nil } + +// validateCredentialsNumber in case fencing credentials exists validates there are exactly 2. +func validateCredentialsNumber(numOfCpReplicas int64, fencing *types.Fencing, fldPath *field.Path) field.ErrorList { + errs := field.ErrorList{} + var numOfCredentials int + if fencing != nil { + numOfCredentials = len(fencing.Credentials) + } + if numOfCpReplicas == 2 { + if numOfCredentials != 2 { + errs = append(errs, field.Forbidden(fldPath, fmt.Sprintf("there should be exactly two fencing credentials to support the two node cluster, instead %d credentials were found", numOfCredentials))) + } + } else { + if numOfCredentials != 0 { + errs = append(errs, field.Forbidden(fldPath, fmt.Sprintf("there should not be any fencing credentials configured for a non dual replica control plane (Two Nodes Fencing) cluster, instead %d credentials were found", numOfCredentials))) + } + } + return errs +} + +func validateFencingCredentials(installConfig *types.InstallConfig) (errors field.ErrorList) { + fldPath := field.NewPath("controlPlane", "fencing") + fencingCredentials := installConfig.ControlPlane.Fencing + allErrs := field.ErrorList{} + if fencingCredentials != nil { + allErrs = append(allErrs, common.ValidateUniqueAndRequiredFields(fencingCredentials.Credentials, fldPath, func([]byte) bool { return false }, "credentials")...) + } + allErrs = append(allErrs, validateCredentialsNumber(*installConfig.ControlPlane.Replicas, fencingCredentials, fldPath.Child("credentials"))...) + + return allErrs +} diff --git a/pkg/types/validation/installconfig_test.go b/pkg/types/validation/installconfig_test.go index 2dec77ab6e..e33153780f 100644 --- a/pkg/types/validation/installconfig_test.go +++ b/pkg/types/validation/installconfig_test.go @@ -205,10 +205,6 @@ func validNutanixPlatform() *nutanix.Platform { } } -func validNonePlatform() *none.Platform { - return &none.Platform{} -} - func validIPv4NetworkingConfig() *types.Networking { return &types.Networking{ NetworkType: "OVNKubernetes", @@ -2801,3 +2797,210 @@ func TestValidateReleaseArchitecture(t *testing.T) { }) }) } + +func TestValidateTNF(t *testing.T) { + cases := []struct { + name string + config *types.InstallConfig + machinePool *types.MachinePool + expected string + }{ + { + config: installConfig().CpReplicas(3).build(), + name: "valid_empty_credentials", + expected: "", + }, + { + config: installConfig(). + MachinePool(machinePool(). + Credential(c1(), c2())). + CpReplicas(2). + build(), + name: "valid_two_credentials", + expected: "", + }, + { + config: installConfig(). + MachinePool(machinePool(). + Credential(c1(), c2(), c3())). + CpReplicas(2).build(), + name: "invalid_number_of_credentials_for_dual_replica", + expected: "controlPlane.fencing.credentials: Forbidden: there should be exactly two fencing credentials to support the two node cluster, instead 3 credentials were found", + }, + { + config: installConfig(). + MachinePool(machinePool(). + Credential(c1(), c2())). + CpReplicas(3).build(), + name: "invalid_number_of_credentials_for_non_dual_replica", + expected: "controlPlane.fencing.credentials: Forbidden: there should not be any fencing credentials configured for a non dual replica control plane \\(Two Nodes Fencing\\) cluster, instead 2 credentials were found", + }, + { + config: installConfig(). + MachinePool(machinePool(). + Credential( + c1().BMCAddress("ipmi://192.168.111.1"), + c2().BMCAddress("ipmi://192.168.111.1"))). + CpReplicas(2).build(), + name: "duplicate_bmc_address", + expected: "controlPlane.fencing.credentials\\[1\\].Address: Duplicate value: \"ipmi://192.168.111.1\"", + }, + { + config: installConfig(). + MachinePool(machinePool(). + Credential(c1().BMCAddress(""), c2())). + CpReplicas(2).build(), + name: "bmc_address_required", + expected: "controlPlane.fencing.credentials\\[0\\].Address: Required value: missing Address", + }, + { + config: installConfig(). + MachinePool(machinePool(). + Credential(c1(), c2().BMCUsername(""))). + CpReplicas(2).build(), + name: "bmc_username_required", + expected: "controlPlane.fencing.credentials\\[1\\].Username: Required value: missing Username", + }, + { + config: installConfig(). + MachinePool(machinePool(). + Credential(c1().BMCPassword(""), c2())). + CpReplicas(2).build(), + name: "bmc_password_required", + expected: "controlPlane.fencing.credentials\\[0\\].Password: Required value: missing Password", + }, + { + config: installConfig(). + MachinePool(machinePool(). + Credential(c1().HostName(""), c2())). + CpReplicas(2).build(), + name: "host_name_required", + expected: "controlPlane.fencing.credentials\\[0\\].HostName: Required value: missing HostName", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + // Build default wrapping installConfig + if tc.config == nil { + tc.config = installConfig().build() + } + + err := validateFencingCredentials(tc.config).ToAggregate() + + if tc.expected == "" { + assert.NoError(t, err) + } else { + assert.Regexp(t, tc.expected, err) + } + }) + } +} + +type credentialBuilder struct { + types.Credential +} + +func (hb *credentialBuilder) build() *types.Credential { + return &hb.Credential +} + +func c1() *credentialBuilder { + return &credentialBuilder{ + types.Credential{ + HostName: "host1", + Username: "root", + Password: "password", + Address: "ipmi://192.168.111.1", + }, + } +} + +func c2() *credentialBuilder { + return &credentialBuilder{ + types.Credential{ + HostName: "host2", + Username: "root", + Password: "password", + Address: "ipmi://192.168.111.2", + }, + } +} + +func c3() *credentialBuilder { + return &credentialBuilder{ + types.Credential{ + HostName: "host3", + Username: "root", + Password: "password", + Address: "ipmi://192.168.111.3", + }, + } +} + +func (hb *credentialBuilder) HostName(value string) *credentialBuilder { + hb.Credential.HostName = value + return hb +} + +func (hb *credentialBuilder) BMCAddress(value string) *credentialBuilder { + hb.Credential.Address = value + return hb +} + +func (hb *credentialBuilder) BMCUsername(value string) *credentialBuilder { + hb.Credential.Username = value + return hb +} + +func (hb *credentialBuilder) BMCPassword(value string) *credentialBuilder { + hb.Credential.Password = value + return hb +} + +type machinePoolBuilder struct { + types.MachinePool +} + +func (pb *machinePoolBuilder) build() *types.MachinePool { + return &pb.MachinePool +} + +func machinePool() *machinePoolBuilder { + return &machinePoolBuilder{ + types.MachinePool{}} +} + +func (pb *machinePoolBuilder) Credential(builders ...*credentialBuilder) *machinePoolBuilder { + pb.MachinePool.Fencing = &types.Fencing{} + for _, builder := range builders { + pb.MachinePool.Fencing.Credentials = append(pb.MachinePool.Fencing.Credentials, builder.build()) + } + return pb +} + +type installConfigBuilder struct { + types.InstallConfig +} + +func installConfig() *installConfigBuilder { + return &installConfigBuilder{ + InstallConfig: types.InstallConfig{}, + } +} + +func (icb *installConfigBuilder) CpReplicas(numOfCpReplicas int64) *installConfigBuilder { + if icb.InstallConfig.ControlPlane == nil { + icb.InstallConfig.ControlPlane = &types.MachinePool{} + } + icb.InstallConfig.ControlPlane.Replicas = &numOfCpReplicas + return icb +} + +func (icb *installConfigBuilder) MachinePool(cpBuilder *machinePoolBuilder) *installConfigBuilder { + icb.InstallConfig.ControlPlane = cpBuilder.build() + return icb +} + +func (icb *installConfigBuilder) build() *types.InstallConfig { + return &icb.InstallConfig +} From e2e3ccf8c274780d60c96dba648be99faa8a1158 Mon Sep 17 00:00:00 2001 From: Michael Shitrit Date: Thu, 20 Mar 2025 15:38:35 +0200 Subject: [PATCH 8/8] Apply review feedback - refactor some method location for clearer code - use feature gate in a better structured way to align with standards - Add compute validation - tests for added changes - use log in a old legacy method - better API description Signed-off-by: Michael Shitrit --- .../install.openshift.io_installconfigs.yaml | 15 ++-- go.mod | 2 +- pkg/asset/agent/installconfig_test.go | 2 +- .../configimage/installconfig_test.go | 2 +- pkg/types/common/common.go | 3 +- pkg/types/defaults/validation/featuregates.go | 21 +++++ pkg/types/machinepools.go | 1 + pkg/types/validation/featuregate_test.go | 27 ++++++ pkg/types/validation/installconfig.go | 32 +++++--- pkg/types/validation/installconfig_test.go | 82 +++++++++++++++---- 10 files changed, 148 insertions(+), 39 deletions(-) create mode 100644 pkg/types/defaults/validation/featuregates.go diff --git a/data/data/install.openshift.io_installconfigs.yaml b/data/data/install.openshift.io_installconfigs.yaml index 4cdfc90402..66ffa83f6e 100644 --- a/data/data/install.openshift.io_installconfigs.yaml +++ b/data/data/install.openshift.io_installconfigs.yaml @@ -58,8 +58,9 @@ spec: - amd64 type: string fencing: - description: Fencing stores the information about a baremetal host's - management controller. + description: |- + Fencing stores the information about a baremetal host's management controller. + Fencing may only be set for control plane nodes. properties: credentials: description: Credentials stores the information about a baremetal @@ -1248,8 +1249,9 @@ spec: - amd64 type: string fencing: - description: Fencing stores the information about a baremetal host's - management controller. + description: |- + Fencing stores the information about a baremetal host's management controller. + Fencing may only be set for control plane nodes. properties: credentials: description: Credentials stores the information about a baremetal @@ -2377,8 +2379,9 @@ spec: - amd64 type: string fencing: - description: Fencing stores the information about a baremetal host's - management controller. + description: |- + Fencing stores the information about a baremetal host's management controller. + Fencing may only be set for control plane nodes. properties: credentials: description: Credentials stores the information about a baremetal diff --git a/go.mod b/go.mod index 4c7c2dc17a..8f39c21d58 100644 --- a/go.mod +++ b/go.mod @@ -46,6 +46,7 @@ require ( github.com/aws/aws-sdk-go-v2/service/route53 v1.48.6 github.com/aws/aws-sdk-go-v2/service/s3 v1.53.1 github.com/aws/aws-sdk-go-v2/service/sts v1.28.6 + github.com/aws/smithy-go v1.22.3 github.com/cavaliercoder/go-cpio v0.0.0-20180626203310-925f9528c45e github.com/clarketm/json v1.14.1 github.com/containers/image/v5 v5.31.0 @@ -183,7 +184,6 @@ require ( github.com/aws/aws-sdk-go-v2/service/internal/s3shared v1.17.5 // indirect github.com/aws/aws-sdk-go-v2/service/sso v1.20.5 // indirect github.com/aws/aws-sdk-go-v2/service/ssooidc v1.23.4 // indirect - github.com/aws/smithy-go v1.22.3 // indirect github.com/beorn7/perks v1.0.1 // indirect github.com/blang/semver v3.5.1+incompatible // indirect github.com/blang/semver/v4 v4.0.0 // indirect diff --git a/pkg/asset/agent/installconfig_test.go b/pkg/asset/agent/installconfig_test.go index c9b65f832b..c1e4abe16e 100644 --- a/pkg/asset/agent/installconfig_test.go +++ b/pkg/asset/agent/installconfig_test.go @@ -474,7 +474,7 @@ platform: pullSecret: "{\"auths\":{\"example.com\":{\"auth\":\"authorization value\"}}}" `, expectedFound: false, - expectedError: "invalid install-config configuration: ControlPlane.Replicas: Invalid value: 2: ControlPlane.Replicas can only be set to 5, 4, 3, or 1. Found 2", + expectedError: "invalid install-config configuration: [controlPlane.fencing.credentials: Forbidden: there should be exactly two fencing credentials to support the two node cluster, instead 0 credentials were found, ControlPlane.Replicas: Invalid value: 2: ControlPlane.Replicas can only be set to 5, 4, 3, or 1. Found 2]", }, { name: "invalid platform for SNO cluster", diff --git a/pkg/asset/imagebased/configimage/installconfig_test.go b/pkg/asset/imagebased/configimage/installconfig_test.go index e7b983e8b7..ad82675e23 100644 --- a/pkg/asset/imagebased/configimage/installconfig_test.go +++ b/pkg/asset/imagebased/configimage/installconfig_test.go @@ -101,7 +101,7 @@ platform: pullSecret: "{\"auths\":{\"example.com\":{\"auth\":\"authorization value\"}}}" `, expectedFound: false, - expectedError: "invalid install-config configuration: ControlPlane.Replicas: Required value: Only Single Node OpenShift (SNO) is supported, total number of ControlPlane.Replicas must be 1. Found 2", + expectedError: "invalid install-config configuration: [controlPlane.fencing.credentials: Forbidden: there should be exactly two fencing credentials to support the two node cluster, instead 0 credentials were found, ControlPlane.Replicas: Required value: Only Single Node OpenShift (SNO) is supported, total number of ControlPlane.Replicas must be 1. Found 2]", }, { name: "invalid number of MachineNetworks", diff --git a/pkg/types/common/common.go b/pkg/types/common/common.go index 0a489da968..64886a020a 100644 --- a/pkg/types/common/common.go +++ b/pkg/types/common/common.go @@ -6,6 +6,7 @@ import ( "reflect" "github.com/go-playground/validator/v10" + "github.com/sirupsen/logrus" "k8s.io/apimachinery/pkg/util/validation/field" ) @@ -37,7 +38,7 @@ func ValidateUniqueAndRequiredFields[T any](elements []T, fldPath *field.Path, f return !valueFound }); err != nil { - fmt.Printf("unexpected error registering validation: %v\n", err) + logrus.Error("Unexpected error registering validation", err) } // Apply validations and translate errors. diff --git a/pkg/types/defaults/validation/featuregates.go b/pkg/types/defaults/validation/featuregates.go new file mode 100644 index 0000000000..8476854fcb --- /dev/null +++ b/pkg/types/defaults/validation/featuregates.go @@ -0,0 +1,21 @@ +package validation + +import ( + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/openshift/api/features" + "github.com/openshift/installer/pkg/types" + "github.com/openshift/installer/pkg/types/featuregates" +) + +// GatedFeatures determines all of the install config fields that should +// be validated to ensure that the proper featuregate is enabled when the field is used. +func GatedFeatures(c *types.InstallConfig) []featuregates.GatedInstallConfigFeature { + return []featuregates.GatedInstallConfigFeature{ + { + FeatureGateName: features.FeatureGateDualReplica, + Condition: c.ControlPlane != nil && c.ControlPlane.Fencing != nil, + Field: field.NewPath("platform", "none", "fencingCredentials"), + }, + } +} diff --git a/pkg/types/machinepools.go b/pkg/types/machinepools.go index aabcbee2b1..e818e2b3c1 100644 --- a/pkg/types/machinepools.go +++ b/pkg/types/machinepools.go @@ -80,6 +80,7 @@ type MachinePool struct { Architecture Architecture `json:"architecture,omitempty"` // Fencing stores the information about a baremetal host's management controller. + // Fencing may only be set for control plane nodes. // +optional Fencing *Fencing `json:"fencing,omitempty"` } diff --git a/pkg/types/validation/featuregate_test.go b/pkg/types/validation/featuregate_test.go index 2de53a4326..9bd1df97a2 100644 --- a/pkg/types/validation/featuregate_test.go +++ b/pkg/types/validation/featuregate_test.go @@ -125,6 +125,33 @@ func TestFeatureGates(t *testing.T) { return c }(), }, + { + name: "FencingCredentials is not allowed with Feature Gates disabled", + installConfig: func() *types.InstallConfig { + c := validInstallConfig() + c.ControlPlane.Fencing = &types.Fencing{Credentials: []*types.Credential{{HostName: "host1"}, {HostName: "host2"}}} + return c + }(), + expected: `^platform.none.fencingCredentials: Forbidden: this field is protected by the DualReplica feature gate which must be enabled through either the TechPreviewNoUpgrade or CustomNoUpgrade feature set$`, + }, + { + name: "FencingCredentials is allowed with TechPreviewNoUpgrade Feature Set", + installConfig: func() *types.InstallConfig { + c := validInstallConfig() + c.FeatureSet = v1.TechPreviewNoUpgrade + c.ControlPlane.Fencing = &types.Fencing{Credentials: []*types.Credential{{HostName: "host1"}, {HostName: "host2"}}} + return c + }(), + }, + { + name: "FencingCredentials is allowed with DevPreviewNoUpgrade Feature Set", + installConfig: func() *types.InstallConfig { + c := validInstallConfig() + c.FeatureSet = v1.DevPreviewNoUpgrade + c.ControlPlane.Fencing = &types.Fencing{Credentials: []*types.Credential{{HostName: "host1"}, {HostName: "host2"}}} + return c + }(), + }, } for _, tc := range cases { diff --git a/pkg/types/validation/installconfig.go b/pkg/types/validation/installconfig.go index f1553ab1e2..bd9d8e141d 100644 --- a/pkg/types/validation/installconfig.go +++ b/pkg/types/validation/installconfig.go @@ -32,6 +32,7 @@ import ( "github.com/openshift/installer/pkg/types/baremetal" baremetalvalidation "github.com/openshift/installer/pkg/types/baremetal/validation" "github.com/openshift/installer/pkg/types/common" + defaultsvalidation "github.com/openshift/installer/pkg/types/defaults/validation" "github.com/openshift/installer/pkg/types/external" "github.com/openshift/installer/pkg/types/featuregates" "github.com/openshift/installer/pkg/types/gcp" @@ -122,14 +123,7 @@ func ValidateInstallConfig(c *types.InstallConfig, usingAgentMethod bool) field. } 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"))...) - if len(c.ControlPlane.Fencing.Credentials) > 0 { - if c.EnabledFeatureGates().Enabled(features.FeatureGateDualReplica) { - allErrs = append(allErrs, validateFencingCredentials(c)...) - } else { - allErrs = append(allErrs, field.Forbidden(field.NewPath("controlPlane").Child("fencing").Child("credentials"), fmt.Sprintf("%s feature must be enabled in order to use Two Node Fencing (TNF) cluster deployment", features.FeatureGateDualReplica))) - } - } + allErrs = append(allErrs, validateControlPlane(c, field.NewPath("controlPlane"))...) } else { allErrs = append(allErrs, field.Required(field.NewPath("controlPlane"), "controlPlane is required")) } @@ -752,8 +746,10 @@ func validateOVNIPv4InternalJoinSubnet(n *types.Networking, fldPath *field.Path) return allErrs } -func validateControlPlane(platform *types.Platform, pool *types.MachinePool, fldPath *field.Path) field.ErrorList { +func validateControlPlane(installConfig *types.InstallConfig, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} + platform := &installConfig.Platform + pool := installConfig.ControlPlane if pool.Name != types.MachinePoolControlPlaneRoleName { allErrs = append(allErrs, field.NotSupported(fldPath.Child("name"), pool.Name, []string{types.MachinePoolControlPlaneRoleName})) } @@ -761,6 +757,7 @@ func validateControlPlane(platform *types.Platform, pool *types.MachinePool, fld allErrs = append(allErrs, field.Invalid(fldPath.Child("replicas"), pool.Replicas, "number of control plane replicas must be positive")) } allErrs = append(allErrs, ValidateMachinePool(platform, pool, fldPath)...) + allErrs = append(allErrs, validateFencingCredentials(installConfig)...) return allErrs } @@ -812,6 +809,10 @@ func validateCompute(platform *types.Platform, control *types.MachinePool, pools allErrs = append(allErrs, field.Invalid(poolFldPath.Child("architecture"), p.Architecture, "heteregeneous multi-arch is not supported; compute pool architecture must match control plane")) } allErrs = append(allErrs, ValidateMachinePool(platform, &p, poolFldPath)...) + + if p.Fencing != nil { + allErrs = append(allErrs, field.Invalid(poolFldPath.Child("fencing"), p.Fencing, "fencing is only valid for control plane")) + } } return allErrs } @@ -1472,6 +1473,10 @@ func validateGatedFeatures(c *types.InstallConfig) field.ErrorList { gatedFeatures = append(gatedFeatures, awsvalidation.GatedFeatures(c)...) } + if c.ControlPlane != nil { + gatedFeatures = append(gatedFeatures, defaultsvalidation.GatedFeatures(c)...) + } + fg := c.EnabledFeatureGates() errMsgTemplate := "this field is protected by the %s feature gate which must be enabled through either the TechPreviewNoUpgrade or CustomNoUpgrade feature set" @@ -1552,8 +1557,13 @@ func isV4NodeSubnetLargeEnough(cn []types.ClusterNetworkEntry, nodeSubnet *ipnet } // validateCredentialsNumber in case fencing credentials exists validates there are exactly 2. -func validateCredentialsNumber(numOfCpReplicas int64, fencing *types.Fencing, fldPath *field.Path) field.ErrorList { +func validateCredentialsNumber(controlPlane *types.MachinePool, fencing *types.Fencing, fldPath *field.Path) field.ErrorList { errs := field.ErrorList{} + if controlPlane == nil || controlPlane.Replicas == nil { + // invalid use case covered by a different validation. + return errs + } + numOfCpReplicas := *controlPlane.Replicas var numOfCredentials int if fencing != nil { numOfCredentials = len(fencing.Credentials) @@ -1577,7 +1587,7 @@ func validateFencingCredentials(installConfig *types.InstallConfig) (errors fiel if fencingCredentials != nil { allErrs = append(allErrs, common.ValidateUniqueAndRequiredFields(fencingCredentials.Credentials, fldPath, func([]byte) bool { return false }, "credentials")...) } - allErrs = append(allErrs, validateCredentialsNumber(*installConfig.ControlPlane.Replicas, fencingCredentials, fldPath.Child("credentials"))...) + allErrs = append(allErrs, validateCredentialsNumber(installConfig.ControlPlane, fencingCredentials, fldPath.Child("credentials"))...) return allErrs } diff --git a/pkg/types/validation/installconfig_test.go b/pkg/types/validation/installconfig_test.go index e33153780f..d91ce6e420 100644 --- a/pkg/types/validation/installconfig_test.go +++ b/pkg/types/validation/installconfig_test.go @@ -5,6 +5,7 @@ import ( "net" "testing" + "github.com/aws/smithy-go/ptr" "github.com/stretchr/testify/assert" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/validation/field" @@ -2800,10 +2801,11 @@ func TestValidateReleaseArchitecture(t *testing.T) { func TestValidateTNF(t *testing.T) { cases := []struct { - name string - config *types.InstallConfig - machinePool *types.MachinePool - expected string + name string + config *types.InstallConfig + machinePool *types.MachinePool + checkCompute bool + expected string }{ { config: installConfig().CpReplicas(3).build(), @@ -2812,7 +2814,7 @@ func TestValidateTNF(t *testing.T) { }, { config: installConfig(). - MachinePool(machinePool(). + MachinePoolCP(machinePool(). Credential(c1(), c2())). CpReplicas(2). build(), @@ -2821,7 +2823,7 @@ func TestValidateTNF(t *testing.T) { }, { config: installConfig(). - MachinePool(machinePool(). + MachinePoolCP(machinePool(). Credential(c1(), c2(), c3())). CpReplicas(2).build(), name: "invalid_number_of_credentials_for_dual_replica", @@ -2829,7 +2831,7 @@ func TestValidateTNF(t *testing.T) { }, { config: installConfig(). - MachinePool(machinePool(). + MachinePoolCP(machinePool(). Credential(c1(), c2())). CpReplicas(3).build(), name: "invalid_number_of_credentials_for_non_dual_replica", @@ -2837,7 +2839,7 @@ func TestValidateTNF(t *testing.T) { }, { config: installConfig(). - MachinePool(machinePool(). + MachinePoolCP(machinePool(). Credential( c1().BMCAddress("ipmi://192.168.111.1"), c2().BMCAddress("ipmi://192.168.111.1"))). @@ -2847,7 +2849,7 @@ func TestValidateTNF(t *testing.T) { }, { config: installConfig(). - MachinePool(machinePool(). + MachinePoolCP(machinePool(). Credential(c1().BMCAddress(""), c2())). CpReplicas(2).build(), name: "bmc_address_required", @@ -2855,7 +2857,7 @@ func TestValidateTNF(t *testing.T) { }, { config: installConfig(). - MachinePool(machinePool(). + MachinePoolCP(machinePool(). Credential(c1(), c2().BMCUsername(""))). CpReplicas(2).build(), name: "bmc_username_required", @@ -2863,7 +2865,7 @@ func TestValidateTNF(t *testing.T) { }, { config: installConfig(). - MachinePool(machinePool(). + MachinePoolCP(machinePool(). Credential(c1().BMCPassword(""), c2())). CpReplicas(2).build(), name: "bmc_password_required", @@ -2871,22 +2873,39 @@ func TestValidateTNF(t *testing.T) { }, { config: installConfig(). - MachinePool(machinePool(). + MachinePoolCP(machinePool(). Credential(c1().HostName(""), c2())). CpReplicas(2).build(), name: "host_name_required", expected: "controlPlane.fencing.credentials\\[0\\].HostName: Required value: missing HostName", }, + { + config: installConfig(). + MachinePoolCP(machinePool(). + Architecture(types.ArchitectureAMD64). + Credential(c1(), c2())). + MachinePoolCompute( + machinePool().Name("worker"). + Hyperthreading(types.HyperthreadingDisabled). + Architecture(types.ArchitectureAMD64). + Replicas(ptr.Int64(3)). + Credential(c1())). + CpReplicas(2).build(), + name: "host_name_required", + checkCompute: true, + expected: `compute\[\d+\]\.fencing: Invalid value: types\.Fencing\{Credentials:\[\]\*types\.Credential\{\(\*types\.Credential\)\(\S+\)\}\}: fencing is only valid for control plane`, + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { // Build default wrapping installConfig - if tc.config == nil { - tc.config = installConfig().build() + var err error + if tc.checkCompute { + err = validateCompute(&tc.config.Platform, tc.config.ControlPlane, tc.config.Compute, field.NewPath("compute"), false).ToAggregate() + } else { + err = validateFencingCredentials(tc.config).ToAggregate() } - err := validateFencingCredentials(tc.config).ToAggregate() - if tc.expected == "" { assert.NoError(t, err) } else { @@ -2970,6 +2989,26 @@ func machinePool() *machinePoolBuilder { types.MachinePool{}} } +func (pb *machinePoolBuilder) Name(name string) *machinePoolBuilder { + pb.MachinePool.Name = name + return pb +} + +func (pb *machinePoolBuilder) Replicas(replicas *int64) *machinePoolBuilder { + pb.MachinePool.Replicas = replicas + return pb +} + +func (pb *machinePoolBuilder) Architecture(architecture types.Architecture) *machinePoolBuilder { + pb.MachinePool.Architecture = architecture + return pb +} + +func (pb *machinePoolBuilder) Hyperthreading(hyperthreading types.HyperthreadingMode) *machinePoolBuilder { + pb.MachinePool.Hyperthreading = hyperthreading + return pb +} + func (pb *machinePoolBuilder) Credential(builders ...*credentialBuilder) *machinePoolBuilder { pb.MachinePool.Fencing = &types.Fencing{} for _, builder := range builders { @@ -2996,8 +3035,15 @@ func (icb *installConfigBuilder) CpReplicas(numOfCpReplicas int64) *installConfi return icb } -func (icb *installConfigBuilder) MachinePool(cpBuilder *machinePoolBuilder) *installConfigBuilder { - icb.InstallConfig.ControlPlane = cpBuilder.build() +func (icb *installConfigBuilder) MachinePoolCP(builder *machinePoolBuilder) *installConfigBuilder { + icb.InstallConfig.ControlPlane = builder.build() + return icb +} + +func (icb *installConfigBuilder) MachinePoolCompute(builders ...*machinePoolBuilder) *installConfigBuilder { + for _, builder := range builders { + icb.InstallConfig.Compute = append(icb.InstallConfig.Compute, *builder.build()) + } return icb }