diff --git a/pkg/asset/installconfig/vsphere/validation.go b/pkg/asset/installconfig/vsphere/validation.go index de74af6cfe4..392c61651ff 100644 --- a/pkg/asset/installconfig/vsphere/validation.go +++ b/pkg/asset/installconfig/vsphere/validation.go @@ -20,7 +20,7 @@ func Validate(ic *types.InstallConfig) error { return errors.New(field.Required(field.NewPath("platform", "vsphere"), "vSphere validation requires a vSphere platform configuration").Error()) } - allErrs = append(allErrs, validation.ValidatePlatform(ic.Platform.VSphere, ic.Networking, field.NewPath("platform").Child("vsphere"))...) + allErrs = append(allErrs, validation.ValidatePlatform(ic.Platform.VSphere, field.NewPath("platform").Child("vsphere"))...) return allErrs.ToAggregate() } diff --git a/pkg/types/validation/installconfig.go b/pkg/types/validation/installconfig.go index 1c63e7fd4ca..17feaa01b0b 100644 --- a/pkg/types/validation/installconfig.go +++ b/pkg/types/validation/installconfig.go @@ -436,9 +436,7 @@ 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, c.Networking, f) - }) + validate(vsphere.Name, platform.VSphere, func(f *field.Path) field.ErrorList { return vspherevalidation.ValidatePlatform(platform.VSphere, f) }) } if platform.BareMetal != nil { validate(baremetal.Name, platform.BareMetal, func(f *field.Path) field.ErrorList { diff --git a/pkg/types/vsphere/validation/platform.go b/pkg/types/vsphere/validation/platform.go index e472372ef7d..1d65a509f8c 100644 --- a/pkg/types/vsphere/validation/platform.go +++ b/pkg/types/vsphere/validation/platform.go @@ -2,18 +2,16 @@ package validation import ( "fmt" - "net" "strings" "k8s.io/apimachinery/pkg/util/validation/field" - "github.com/openshift/installer/pkg/types" "github.com/openshift/installer/pkg/types/vsphere" "github.com/openshift/installer/pkg/validate" ) // ValidatePlatform checks that the specified platform is valid. -func ValidatePlatform(p *vsphere.Platform, network *types.Networking, fldPath *field.Path) field.ErrorList { +func ValidatePlatform(p *vsphere.Platform, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} if len(p.VCenter) == 0 { allErrs = append(allErrs, field.Required(fldPath.Child("vCenter"), "must specify the name of the vCenter")) @@ -39,7 +37,7 @@ func ValidatePlatform(p *vsphere.Platform, network *types.Networking, fldPath *f // If all VIPs are empty, skip IP validation. All VIPs are required to be defined together. if strings.Join([]string{p.APIVIP, p.IngressVIP}, "") != "" { - allErrs = append(allErrs, validateVIPs(p, network, fldPath)...) + allErrs = append(allErrs, validateVIPs(p, fldPath)...) } // folder is optional, but if provided should pass validation @@ -62,63 +60,30 @@ func ValidateForProvisioning(p *vsphere.Platform, fldPath *field.Path) field.Err allErrs = append(allErrs, field.Required(fldPath.Child("network"), "must specify the network")) } - if len(p.APIVIP) == 0 { - allErrs = append(allErrs, field.Required(fldPath.Child("apiVIP"), "must specify a VIP for API")) - } - - if len(p.IngressVIP) == 0 { - allErrs = append(allErrs, field.Required(fldPath.Child("ingressVIP"), "must specify a VIP for Ingress")) - } + allErrs = append(allErrs, validateVIPs(p, fldPath)...) return allErrs } -// ipInNetwork return true if the given ip is within one of the machine networks. -func ipInNetwork(vipIP net.IP, machineNetwork []types.MachineNetworkEntry) bool { - for _, machine := range machineNetwork { - if machine.CIDR.Contains(vipIP) { - return true - } - } - return false -} - // validateVIPs checks that all required VIPs are provided and are valid IP addresses. -func validateVIPs(p *vsphere.Platform, network *types.Networking, fldPath *field.Path) field.ErrorList { +func validateVIPs(p *vsphere.Platform, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} - if network == nil { - return append(allErrs, field.Invalid(field.NewPath("networking"), network, "must specify the machine networks")) - } - - if len(p.APIVIP) != 0 { - ip := net.ParseIP(p.APIVIP) - if ip != nil { - if !ipInNetwork(ip, network.MachineNetwork) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("apiVIP"), p.APIVIP, "must be contained within one of the machine networks")) - } - } else { - allErrs = append(allErrs, field.Invalid(fldPath.Child("apiVIP"), p.APIVIP, "not a valid IP address")) - } - } else { - allErrs = append(allErrs, field.Required(fldPath.Child("apiVIP"), "must specify a VIP for both API and Ingress VIPs when specifying either")) + if len(p.APIVIP) == 0 { + allErrs = append(allErrs, field.Required(fldPath.Child("apiVIP"), "must specify a VIP for the API")) + } else if err := validate.IP(p.APIVIP); err != nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("apiVIP"), p.APIVIP, err.Error())) } - if len(p.IngressVIP) != 0 { - ip := net.ParseIP(p.IngressVIP) - if ip != nil { - if !ipInNetwork(ip, network.MachineNetwork) { - allErrs = append(allErrs, field.Invalid(fldPath.Child("ingressVIP"), p.IngressVIP, "must be contained within one of the machine networks")) - } - } else { - allErrs = append(allErrs, field.Invalid(fldPath.Child("ingressVIP"), p.IngressVIP, "not a valid IP address")) - } - } else { - allErrs = append(allErrs, field.Required(fldPath.Child("ingressVIP"), "must specify a VIP for both API and Ingress VIPs when specifying either")) + if len(p.IngressVIP) == 0 { + allErrs = append(allErrs, field.Required(fldPath.Child("ingressVIP"), "must specify a VIP for Ingress")) + } else if err := validate.IP(p.IngressVIP); err != nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("ingressVIP"), p.IngressVIP, err.Error())) } if len(p.APIVIP) != 0 && len(p.IngressVIP) != 0 && p.APIVIP == p.IngressVIP { allErrs = append(allErrs, field.Invalid(fldPath.Child("apiVIP"), p.APIVIP, "IPs for both API and Ingress should not be the same.")) } + return allErrs } diff --git a/pkg/types/vsphere/validation/platform_test.go b/pkg/types/vsphere/validation/platform_test.go index 61fdfe655a7..052c75f5b59 100644 --- a/pkg/types/vsphere/validation/platform_test.go +++ b/pkg/types/vsphere/validation/platform_test.go @@ -1,14 +1,11 @@ package validation import ( - "fmt" "testing" "github.com/stretchr/testify/assert" "k8s.io/apimachinery/pkg/util/validation/field" - "github.com/openshift/installer/pkg/ipnet" - "github.com/openshift/installer/pkg/types" "github.com/openshift/installer/pkg/types/vsphere" ) @@ -19,31 +16,18 @@ func validPlatform() *vsphere.Platform { Password: "test-password", Datacenter: "test-datacenter", DefaultDatastore: "test-datastore", - APIVIP: "192.168.111.2", - IngressVIP: "192.168.111.3", } } -func validNetwork() *types.Networking { - n := types.Networking{} - cidr := ipnet.MustParseCIDR("192.168.111.1/24") - n.MachineNetwork = append(n.MachineNetwork, types.MachineNetworkEntry{ - CIDR: *cidr, - }) - return &n -} - func TestValidatePlatform(t *testing.T) { cases := []struct { name string platform *vsphere.Platform - networking *types.Networking expectedError string }{ { - name: "minimal", - platform: validPlatform(), - networking: validNetwork(), + name: "minimal", + platform: validPlatform(), }, { name: "missing vCenter name", @@ -52,7 +36,6 @@ func TestValidatePlatform(t *testing.T) { p.VCenter = "" return p }(), - networking: validNetwork(), expectedError: `^test-path\.vCenter: Required value: must specify the name of the vCenter$`, }, { @@ -62,7 +45,6 @@ func TestValidatePlatform(t *testing.T) { p.Username = "" return p }(), - networking: validNetwork(), expectedError: `^test-path\.username: Required value: must specify the username$`, }, { @@ -72,7 +54,6 @@ func TestValidatePlatform(t *testing.T) { p.Password = "" return p }(), - networking: validNetwork(), expectedError: `^test-path\.password: Required value: must specify the password$`, }, { @@ -82,7 +63,6 @@ func TestValidatePlatform(t *testing.T) { p.Datacenter = "" return p }(), - networking: validNetwork(), expectedError: `^test-path\.datacenter: Required value: must specify the datacenter$`, }, { @@ -92,7 +72,6 @@ func TestValidatePlatform(t *testing.T) { p.DefaultDatastore = "" return p }(), - networking: validNetwork(), expectedError: `^test-path\.defaultDatastore: Required value: must specify the default datastore$`, }, { @@ -103,7 +82,6 @@ func TestValidatePlatform(t *testing.T) { p.IngressVIP = "192.168.111.3" return p }(), - networking: validNetwork(), // expectedError: `^test-path\.apiVIP: Invalid value: "": "" is not a valid IP`, }, { @@ -114,8 +92,7 @@ func TestValidatePlatform(t *testing.T) { p.IngressVIP = "192.168.111.3" return p }(), - networking: validNetwork(), - expectedError: `^test-path\.apiVIP: Required value: must specify a VIP for both API and Ingress VIPs when specifying either`, + expectedError: `^test-path\.apiVIP: Required value: must specify a VIP for the API`, }, { name: "missing Ingress VIP", @@ -125,8 +102,7 @@ func TestValidatePlatform(t *testing.T) { p.IngressVIP = "" return p }(), - networking: validNetwork(), - expectedError: `^test-path\.ingressVIP: Required value: must specify a VIP for both API and Ingress VIPs when specifying either`, + expectedError: `^test-path\.ingressVIP: Required value: must specify a VIP for Ingress`, }, { name: "Invalid API VIP", @@ -136,8 +112,7 @@ func TestValidatePlatform(t *testing.T) { p.IngressVIP = "192.168.111.2" return p }(), - networking: validNetwork(), - expectedError: `^test-path.apiVIP: Invalid value: "192.168.111": not a valid IP address$`, + expectedError: `^test-path.apiVIP: Invalid value: "192.168.111": "192.168.111" is not a valid IP`, }, { name: "Invalid Ingress VIP", @@ -147,8 +122,7 @@ func TestValidatePlatform(t *testing.T) { p.IngressVIP = "192.168.111" return p }(), - networking: validNetwork(), - expectedError: `^test-path.ingressVIP: Invalid value: "192.168.111": not a valid IP address$`, + expectedError: `^test-path.ingressVIP: Invalid value: "192.168.111": "192.168.111" is not a valid IP`, }, { name: "Same API and Ingress VIP", @@ -158,7 +132,6 @@ func TestValidatePlatform(t *testing.T) { p.IngressVIP = "192.168.111.1" return p }(), - networking: validNetwork(), expectedError: `^test-path.apiVIP: Invalid value: "192.168.111.1": IPs for both API and Ingress should not be the same`, }, { @@ -168,7 +141,6 @@ func TestValidatePlatform(t *testing.T) { p.VCenter = "tEsT-vCenter" return p }(), - networking: validNetwork(), expectedError: `^test-path\.vCenter: Invalid value: "tEsT-vCenter": must be the domain name or IP address of the vCenter`, }, { @@ -178,55 +150,12 @@ func TestValidatePlatform(t *testing.T) { p.VCenter = "https://test-center" return p }(), - networking: validNetwork(), expectedError: `^test-path\.vCenter: Invalid value: "https://test-center": must be the domain name or IP address of the vCenter$`, }, - { - name: "APIVIP not in machine CIDR", - platform: func() *vsphere.Platform { - p := validPlatform() - p.APIVIP = "192.168.1.1" - p.IngressVIP = "192.168.0.1" - return p - }(), - networking: func() *types.Networking { - p := types.Networking{} - value, err := ipnet.ParseCIDR("192.168.0.0/24") - if err != nil { - fmt.Println(err) - } - p.MachineNetwork = append(p.MachineNetwork, types.MachineNetworkEntry{ - CIDR: *value, - }) - return &p - }(), - expectedError: `^test-path.apiVIP: Invalid value: "192.168.1.1": must be contained within one of the machine networks$`, - }, - { - name: "IngressVIP not in machine CIDR", - platform: func() *vsphere.Platform { - p := validPlatform() - p.APIVIP = "192.168.0.1" - p.IngressVIP = "192.168.1.1" - return p - }(), - networking: func() *types.Networking { - p := types.Networking{} - value, err := ipnet.ParseCIDR("192.168.0.0/24") - if err != nil { - fmt.Println(err) - } - p.MachineNetwork = append(p.MachineNetwork, types.MachineNetworkEntry{ - CIDR: *value, - }) - return &p - }(), - expectedError: `^test-path.ingressVIP: Invalid value: "192.168.1.1": must be contained within one of the machine networks$`, - }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - err := ValidatePlatform(tc.platform, tc.networking, field.NewPath("test-path")).ToAggregate() + err := ValidatePlatform(tc.platform, field.NewPath("test-path")).ToAggregate() if tc.expectedError == "" { assert.NoError(t, err) } else {