From b2d6fa405b3ed9c669f6d0a3c61d2d600cb50cf0 Mon Sep 17 00:00:00 2001 From: staebler Date: Tue, 27 Nov 2018 17:42:44 -0500 Subject: [PATCH] validate: simplify CIDR validation Much of the code that made of the CIDR validation was there to give a custom description of what made the CIDR provided invalid. This has been removed in favor of using the error returned by the golang library's ParseCIDR function. Additionally, the code to determine whether two CIDRs overlap was unnecessary complex because it did not take advantage of the fact that CIDRs only overlap if one is a subset of the other. https://jira.coreos.com/browse/CORS-850 --- Gopkg.lock | 1 + pkg/asset/ignition/machine/master_test.go | 8 +- pkg/asset/ignition/machine/worker_test.go | 8 +- pkg/asset/installconfig/aws/aws.go | 38 ++---- pkg/asset/installconfig/installconfig.go | 5 + pkg/asset/installconfig/libvirt/libvirt.go | 10 +- .../installconfig/openstack/openstack.go | 7 +- pkg/asset/machines/libvirt/machines.go | 2 +- pkg/ipnet/ipnet.go | 19 +++ pkg/tfvars/libvirt/libvirt.go | 4 +- pkg/tfvars/tfvars.go | 15 ++- pkg/types/aws/machinepool.go | 6 +- pkg/types/aws/platform.go | 33 ++++- pkg/types/aws/validation/machinepool.go | 19 +++ pkg/types/aws/validation/platform.go | 37 +++++ pkg/types/libvirt/network.go | 6 +- pkg/types/libvirt/platform.go | 2 +- pkg/types/libvirt/validation/machinepool.go | 19 +++ pkg/types/libvirt/validation/platform.go | 26 ++++ pkg/types/openstack/platform.go | 7 +- pkg/types/openstack/validation/machinepool.go | 19 +++ pkg/types/openstack/validation/platform.go | 23 ++++ pkg/types/validation/installconfig.go | 126 ++++++++++++++++++ pkg/types/validation/machinepools.go | 69 ++++++++++ pkg/validate/validate.go | 108 ++++++--------- pkg/validate/validate_test.go | 114 ++++++++-------- 26 files changed, 538 insertions(+), 193 deletions(-) create mode 100644 pkg/types/aws/validation/machinepool.go create mode 100644 pkg/types/aws/validation/platform.go create mode 100644 pkg/types/libvirt/validation/machinepool.go create mode 100644 pkg/types/libvirt/validation/platform.go create mode 100644 pkg/types/openstack/validation/machinepool.go create mode 100644 pkg/types/openstack/validation/platform.go create mode 100644 pkg/types/validation/installconfig.go create mode 100644 pkg/types/validation/machinepools.go diff --git a/Gopkg.lock b/Gopkg.lock index c681dd90f4a..e6e5b471a1b 100644 --- a/Gopkg.lock +++ b/Gopkg.lock @@ -878,6 +878,7 @@ "k8s.io/apimachinery/pkg/util/errors", "k8s.io/apimachinery/pkg/util/sets", "k8s.io/apimachinery/pkg/util/validation", + "k8s.io/apimachinery/pkg/util/validation/field", "k8s.io/apimachinery/pkg/util/wait", "k8s.io/apimachinery/pkg/watch", "k8s.io/client-go/kubernetes", diff --git a/pkg/asset/ignition/machine/master_test.go b/pkg/asset/ignition/machine/master_test.go index f0ce4eed3e9..ae4da49d3b9 100644 --- a/pkg/asset/ignition/machine/master_test.go +++ b/pkg/asset/ignition/machine/master_test.go @@ -1,7 +1,6 @@ package machine import ( - "net" "testing" "github.com/stretchr/testify/assert" @@ -24,12 +23,7 @@ func TestMasterGenerate(t *testing.T) { }, BaseDomain: "test-domain", Networking: types.Networking{ - ServiceCIDR: ipnet.IPNet{ - IPNet: func(s string) net.IPNet { - _, cidr, _ := net.ParseCIDR(s) - return *cidr - }("10.0.1.0/24"), - }, + ServiceCIDR: *ipnet.MustParseCIDR("10.0.1.0/24"), }, Platform: types.Platform{ AWS: &aws.Platform{ diff --git a/pkg/asset/ignition/machine/worker_test.go b/pkg/asset/ignition/machine/worker_test.go index 3858c035819..ba79aaa2813 100644 --- a/pkg/asset/ignition/machine/worker_test.go +++ b/pkg/asset/ignition/machine/worker_test.go @@ -1,7 +1,6 @@ package machine import ( - "net" "testing" "github.com/stretchr/testify/assert" @@ -19,12 +18,7 @@ func TestWorkerGenerate(t *testing.T) { installConfig := &installconfig.InstallConfig{ Config: &types.InstallConfig{ Networking: types.Networking{ - ServiceCIDR: ipnet.IPNet{ - IPNet: func(s string) net.IPNet { - _, cidr, _ := net.ParseCIDR(s) - return *cidr - }("10.0.1.0/24"), - }, + ServiceCIDR: *ipnet.MustParseCIDR("10.0.1.0/24"), }, Platform: types.Platform{ AWS: &aws.Platform{ diff --git a/pkg/asset/installconfig/aws/aws.go b/pkg/asset/installconfig/aws/aws.go index e357381ea8a..54a47266673 100644 --- a/pkg/asset/installconfig/aws/aws.go +++ b/pkg/asset/installconfig/aws/aws.go @@ -16,41 +16,19 @@ import ( "github.com/sirupsen/logrus" survey "gopkg.in/AlecAivazis/survey.v1" + "github.com/openshift/installer/pkg/ipnet" "github.com/openshift/installer/pkg/types/aws" ) -const ( - defaultVPCCIDR = "10.0.0.0/16" -) - var ( - validAWSRegions = map[string]string{ - "ap-northeast-1": "Tokyo", - "ap-northeast-2": "Seoul", - "ap-northeast-3": "Osaka-Local", - "ap-south-1": "Mumbai", - "ap-southeast-1": "Singapore", - "ap-southeast-2": "Sydney", - "ca-central-1": "Central", - "cn-north-1": "Beijing", - "cn-northwest-1": "Ningxia", - "eu-central-1": "Frankfurt", - "eu-west-1": "Ireland", - "eu-west-2": "London", - "eu-west-3": "Paris", - "sa-east-1": "São Paulo", - "us-east-1": "N. Virginia", - "us-east-2": "Ohio", - "us-west-1": "N. California", - "us-west-2": "Oregon", - } + defaultVPCCIDR = ipnet.MustParseCIDR("10.0.0.0/16") ) // Platform collects AWS-specific configuration. func Platform() (*aws.Platform, error) { - longRegions := make([]string, 0, len(validAWSRegions)) - shortRegions := make([]string, 0, len(validAWSRegions)) - for id, location := range validAWSRegions { + longRegions := make([]string, 0, len(aws.ValidRegions)) + shortRegions := make([]string, 0, len(aws.ValidRegions)) + for id, location := range aws.ValidRegions { longRegions = append(longRegions, fmt.Sprintf("%s (%s)", id, location)) shortRegions = append(shortRegions, id) } @@ -59,7 +37,7 @@ func Platform() (*aws.Platform, error) { }) defaultRegion := "us-east-1" - _, ok := validAWSRegions[defaultRegion] + _, ok := aws.ValidRegions[defaultRegion] if !ok { panic(fmt.Sprintf("installer bug: invalid default AWS region %q", defaultRegion)) } @@ -81,7 +59,7 @@ func Platform() (*aws.Platform, error) { defaultRegionPointer := ssn.Config.Region if defaultRegionPointer != nil && *defaultRegionPointer != "" { - _, ok := validAWSRegions[*defaultRegionPointer] + _, ok := aws.ValidRegions[*defaultRegionPointer] if ok { defaultRegion = *defaultRegionPointer } else { @@ -98,7 +76,7 @@ func Platform() (*aws.Platform, error) { Prompt: &survey.Select{ Message: "Region", Help: "The AWS region to be used for installation.", - Default: fmt.Sprintf("%s (%s)", defaultRegion, validAWSRegions[defaultRegion]), + Default: fmt.Sprintf("%s (%s)", defaultRegion, aws.ValidRegions[defaultRegion]), Options: longRegions, }, Validate: survey.ComposeValidators(survey.Required, func(ans interface{}) error { diff --git a/pkg/asset/installconfig/installconfig.go b/pkg/asset/installconfig/installconfig.go index 359056f7b4a..533154bb8e8 100644 --- a/pkg/asset/installconfig/installconfig.go +++ b/pkg/asset/installconfig/installconfig.go @@ -12,6 +12,7 @@ import ( "github.com/openshift/installer/pkg/asset" "github.com/openshift/installer/pkg/ipnet" "github.com/openshift/installer/pkg/types" + "github.com/openshift/installer/pkg/types/validation" ) const ( @@ -156,6 +157,10 @@ func (a *InstallConfig) Load(f asset.FileFetcher) (found bool, err error) { return false, errors.Wrapf(err, "failed to unmarshal") } + if err := validation.ValidateInstallConfig(config).ToAggregate(); err != nil { + return false, errors.Wrapf(err, "invalid %q file", installConfigFilename) + } + a.File, a.Config = file, config return true, nil } diff --git a/pkg/asset/installconfig/libvirt/libvirt.go b/pkg/asset/installconfig/libvirt/libvirt.go index f12d8da9758..a9e86acace3 100644 --- a/pkg/asset/installconfig/libvirt/libvirt.go +++ b/pkg/asset/installconfig/libvirt/libvirt.go @@ -7,14 +7,18 @@ import ( "github.com/pkg/errors" survey "gopkg.in/AlecAivazis/survey.v1" + "github.com/openshift/installer/pkg/ipnet" "github.com/openshift/installer/pkg/rhcos" "github.com/openshift/installer/pkg/types/libvirt" "github.com/openshift/installer/pkg/validate" ) const ( - defaultNetworkIfName = "tt0" - defaultNetworkIPRange = "192.168.126.0/24" + defaultNetworkIfName = "tt0" +) + +var ( + defaultNetworkIPRange = ipnet.MustParseCIDR("192.168.126.0/24") ) // Platform collects libvirt-specific configuration. @@ -42,7 +46,7 @@ func Platform() (*libvirt.Platform, error) { return &libvirt.Platform{ Network: libvirt.Network{ IfName: defaultNetworkIfName, - IPRange: defaultNetworkIPRange, + IPRange: *defaultNetworkIPRange, }, DefaultMachinePlatform: &libvirt.MachinePool{ Image: qcowImage, diff --git a/pkg/asset/installconfig/openstack/openstack.go b/pkg/asset/installconfig/openstack/openstack.go index 2af3f81df1e..f1de960a53e 100644 --- a/pkg/asset/installconfig/openstack/openstack.go +++ b/pkg/asset/installconfig/openstack/openstack.go @@ -12,11 +12,12 @@ import ( "github.com/pkg/errors" survey "gopkg.in/AlecAivazis/survey.v1" + "github.com/openshift/installer/pkg/ipnet" "github.com/openshift/installer/pkg/types/openstack" ) -const ( - defaultVPCCIDR = "10.0.0.0/16" +var ( + defaultNetworkCIDR = ipnet.MustParseCIDR("10.0.0.0/16") ) // Read the valid cloud names from the clouds.yaml @@ -235,7 +236,7 @@ func Platform() (*openstack.Platform, error) { } return &openstack.Platform{ - NetworkCIDRBlock: defaultVPCCIDR, + NetworkCIDRBlock: *defaultNetworkCIDR, Region: region, BaseImage: image, Cloud: cloud, diff --git a/pkg/asset/machines/libvirt/machines.go b/pkg/asset/machines/libvirt/machines.go index 718f5bdc741..bd18c139d20 100644 --- a/pkg/asset/machines/libvirt/machines.go +++ b/pkg/asset/machines/libvirt/machines.go @@ -77,7 +77,7 @@ func provider(clusterName string, platform *libvirt.Platform, userDataSecret str BaseVolumeID: fmt.Sprintf("/var/lib/libvirt/images/%s-base", clusterName), }, NetworkInterfaceName: clusterName, - NetworkInterfaceAddress: platform.Network.IPRange, + NetworkInterfaceAddress: platform.Network.IPRange.String(), Autostart: false, URI: platform.URI, } diff --git a/pkg/ipnet/ipnet.go b/pkg/ipnet/ipnet.go index eff16ee0c8f..354fbfa67a3 100644 --- a/pkg/ipnet/ipnet.go +++ b/pkg/ipnet/ipnet.go @@ -70,3 +70,22 @@ func (ipnet *IPNet) UnmarshalJSON(b []byte) (err error) { return nil } + +// ParseCIDR parses a CIDR from its string representation. +func ParseCIDR(s string) (*IPNet, error) { + _, cidr, err := net.ParseCIDR(s) + if err != nil { + return nil, err + } + return &IPNet{IPNet: *cidr}, nil +} + +// MustParseCIDR parses a CIDR from its string representation. If the parse fails, +// the function will panic. +func MustParseCIDR(s string) *IPNet { + cidr, err := ParseCIDR(s) + if err != nil { + panic(err) + } + return cidr +} diff --git a/pkg/tfvars/libvirt/libvirt.go b/pkg/tfvars/libvirt/libvirt.go index 12f4442c19a..8643daf077d 100644 --- a/pkg/tfvars/libvirt/libvirt.go +++ b/pkg/tfvars/libvirt/libvirt.go @@ -18,8 +18,8 @@ type Libvirt struct { // Network describes a libvirt network configuration. type Network struct { - IfName string `json:"libvirt_network_if,omitempty"` - IPRange string `json:"libvirt_ip_range,omitempty"` + IfName string `json:"libvirt_network_if"` + IPRange string `json:"libvirt_ip_range"` } // TFVars fills in computed Terraform variables. diff --git a/pkg/tfvars/tfvars.go b/pkg/tfvars/tfvars.go index 8825ec267cf..4a0122f1cd7 100644 --- a/pkg/tfvars/tfvars.go +++ b/pkg/tfvars/tfvars.go @@ -82,9 +82,14 @@ func TFVars(cfg *types.InstallConfig, bootstrapIgn, masterIgn string) ([]byte, e } config.AWS = aws.AWS{ - Region: cfg.Platform.AWS.Region, - ExtraTags: cfg.Platform.AWS.UserTags, - VPCCIDRBlock: cfg.Platform.AWS.VPCCIDRBlock, + Region: cfg.Platform.AWS.Region, + ExtraTags: cfg.Platform.AWS.UserTags, + VPCCIDRBlock: func() string { + if cfg.Platform.AWS.VPCCIDRBlock == nil { + return "" + } + return cfg.Platform.AWS.VPCCIDRBlock.String() + }(), EC2AMIOverride: ami, } } else if cfg.Platform.Libvirt != nil { @@ -96,7 +101,7 @@ func TFVars(cfg *types.InstallConfig, bootstrapIgn, masterIgn string) ([]byte, e URI: cfg.Platform.Libvirt.URI, Network: libvirt.Network{ IfName: cfg.Platform.Libvirt.Network.IfName, - IPRange: cfg.Platform.Libvirt.Network.IPRange, + IPRange: cfg.Platform.Libvirt.Network.IPRange.String(), }, Image: cfg.Platform.Libvirt.DefaultMachinePlatform.Image, MasterIPs: masterIPs, @@ -110,7 +115,7 @@ func TFVars(cfg *types.InstallConfig, bootstrapIgn, masterIgn string) ([]byte, e } else if cfg.Platform.OpenStack != nil { config.OpenStack = openstack.OpenStack{ Region: cfg.Platform.OpenStack.Region, - NetworkCIDRBlock: cfg.Platform.OpenStack.NetworkCIDRBlock, + NetworkCIDRBlock: cfg.Platform.OpenStack.NetworkCIDRBlock.String(), BaseImage: cfg.Platform.OpenStack.BaseImage, } config.OpenStack.Credentials.Cloud = cfg.Platform.OpenStack.Cloud diff --git a/pkg/types/aws/machinepool.go b/pkg/types/aws/machinepool.go index 1e5331b512c..b19dcea1ad2 100644 --- a/pkg/types/aws/machinepool.go +++ b/pkg/types/aws/machinepool.go @@ -53,10 +53,10 @@ func (a *MachinePool) Set(required *MachinePool) { // EC2RootVolume defines the storage for an ec2 instance. type EC2RootVolume struct { - // IOPS defines the iops for the instance. + // IOPS defines the iops for the storage. IOPS int `json:"iops"` - // Size defines the size of the instance. + // Size defines the size of the storage. Size int `json:"size"` - // Type defines the type of the instance. + // Type defines the type of the storage. Type string `json:"type"` } diff --git a/pkg/types/aws/platform.go b/pkg/types/aws/platform.go index 407834f46c7..d99ca07ce08 100644 --- a/pkg/types/aws/platform.go +++ b/pkg/types/aws/platform.go @@ -1,5 +1,35 @@ package aws +import ( + "github.com/openshift/installer/pkg/ipnet" +) + +var ( + // ValidRegions is a map of the known AWS regions. The key of the map is + // the short name of the region. The value of the map is the long name of + // the region. + ValidRegions = map[string]string{ + "ap-northeast-1": "Tokyo", + "ap-northeast-2": "Seoul", + "ap-northeast-3": "Osaka-Local", + "ap-south-1": "Mumbai", + "ap-southeast-1": "Singapore", + "ap-southeast-2": "Sydney", + "ca-central-1": "Central", + "cn-north-1": "Beijing", + "cn-northwest-1": "Ningxia", + "eu-central-1": "Frankfurt", + "eu-west-1": "Ireland", + "eu-west-2": "London", + "eu-west-3": "Paris", + "sa-east-1": "São Paulo", + "us-east-1": "N. Virginia", + "us-east-2": "Ohio", + "us-west-1": "N. California", + "us-west-2": "Oregon", + } +) + // Platform stores all the global configuration that all machinesets // use. type Platform struct { @@ -15,5 +45,6 @@ type Platform struct { DefaultMachinePlatform *MachinePool `json:"defaultMachinePlatform,omitempty"` // VPCCIDRBlock - VPCCIDRBlock string `json:"vpcCIDRBlock"` + // +optional + VPCCIDRBlock *ipnet.IPNet `json:"vpcCIDRBlock,omitempty"` } diff --git a/pkg/types/aws/validation/machinepool.go b/pkg/types/aws/validation/machinepool.go new file mode 100644 index 00000000000..813659dc1e6 --- /dev/null +++ b/pkg/types/aws/validation/machinepool.go @@ -0,0 +1,19 @@ +package validation + +import ( + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/openshift/installer/pkg/types/aws" +) + +// ValidateMachinePool checks that the specified machine pool is valid. +func ValidateMachinePool(p *aws.MachinePool, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if p.IOPS < 0 { + allErrs = append(allErrs, field.Invalid(fldPath.Child("iops"), p.IOPS, "Storage IOPS must be positive")) + } + if p.Size < 0 { + allErrs = append(allErrs, field.Invalid(fldPath.Child("size"), p.IOPS, "Storage size must be positive")) + } + return allErrs +} diff --git a/pkg/types/aws/validation/platform.go b/pkg/types/aws/validation/platform.go new file mode 100644 index 00000000000..e662acfd0e1 --- /dev/null +++ b/pkg/types/aws/validation/platform.go @@ -0,0 +1,37 @@ +package validation + +import ( + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/openshift/installer/pkg/types/aws" + "github.com/openshift/installer/pkg/validate" +) + +var ( + validRegionValues = func() []string { + validValues := make([]string, len(aws.ValidRegions)) + i := 0 + for r := range aws.ValidRegions { + validValues[i] = r + i++ + } + return validValues + }() +) + +// ValidatePlatform checks that the specified platform is valid. +func ValidatePlatform(p *aws.Platform, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if _, ok := aws.ValidRegions[p.Region]; !ok { + allErrs = append(allErrs, field.NotSupported(fldPath.Child("region"), p.Region, validRegionValues)) + } + if p.DefaultMachinePlatform != nil { + allErrs = append(allErrs, ValidateMachinePool(p.DefaultMachinePlatform, fldPath.Child("defaultMachinePlatform"))...) + } + if p.VPCCIDRBlock != nil { + if err := validate.SubnetCIDR(&p.VPCCIDRBlock.IPNet); err != nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("vpcCIDRBlock"), p.VPCCIDRBlock, err.Error())) + } + } + return allErrs +} diff --git a/pkg/types/libvirt/network.go b/pkg/types/libvirt/network.go index 98198f7ff20..e584ee25227 100644 --- a/pkg/types/libvirt/network.go +++ b/pkg/types/libvirt/network.go @@ -1,9 +1,13 @@ package libvirt +import ( + "github.com/openshift/installer/pkg/ipnet" +) + // Network is the configuration of the libvirt network. type Network struct { // IfName is the name of the network interface. IfName string `json:"if"` // IPRange is the range of IPs to use. - IPRange string `json:"ipRange"` + IPRange ipnet.IPNet `json:"ipRange"` } diff --git a/pkg/types/libvirt/platform.go b/pkg/types/libvirt/platform.go index 0a41a14d378..e9dbe6fbac9 100644 --- a/pkg/types/libvirt/platform.go +++ b/pkg/types/libvirt/platform.go @@ -21,5 +21,5 @@ type Platform struct { Network Network `json:"network"` // MasterIPs - MasterIPs []net.IP `json:"masterIPs"` + MasterIPs []net.IP `json:"masterIPs,omitempty"` } diff --git a/pkg/types/libvirt/validation/machinepool.go b/pkg/types/libvirt/validation/machinepool.go new file mode 100644 index 00000000000..42c330a7668 --- /dev/null +++ b/pkg/types/libvirt/validation/machinepool.go @@ -0,0 +1,19 @@ +package validation + +import ( + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/openshift/installer/pkg/types/libvirt" + "github.com/openshift/installer/pkg/validate" +) + +// ValidateMachinePool checks that the specified machine pool is valid. +func ValidateMachinePool(p *libvirt.MachinePool, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if p.Image != "" { + if err := validate.URI(p.Image); err != nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("image"), p.Image, err.Error())) + } + } + return allErrs +} diff --git a/pkg/types/libvirt/validation/platform.go b/pkg/types/libvirt/validation/platform.go new file mode 100644 index 00000000000..626efe0068e --- /dev/null +++ b/pkg/types/libvirt/validation/platform.go @@ -0,0 +1,26 @@ +package validation + +import ( + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/openshift/installer/pkg/types/libvirt" + "github.com/openshift/installer/pkg/validate" +) + +// ValidatePlatform checks that the specified platform is valid. +func ValidatePlatform(p *libvirt.Platform, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if err := validate.URI(p.URI); err != nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("uri"), p.URI, err.Error())) + } + if p.DefaultMachinePlatform != nil { + allErrs = append(allErrs, ValidateMachinePool(p.DefaultMachinePlatform, fldPath.Child("defaultMachinePlatform"))...) + } + if p.Network.IfName == "" { + allErrs = append(allErrs, field.Required(fldPath.Child("network").Child("if"), p.Network.IfName)) + } + if err := validate.SubnetCIDR(&p.Network.IPRange.IPNet); err != nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("network").Child("ipRange"), p.Network.IPRange, err.Error())) + } + return allErrs +} diff --git a/pkg/types/openstack/platform.go b/pkg/types/openstack/platform.go index 8a65d3e19f4..f8dcb670c55 100644 --- a/pkg/types/openstack/platform.go +++ b/pkg/types/openstack/platform.go @@ -1,5 +1,9 @@ package openstack +import ( + "github.com/openshift/installer/pkg/ipnet" +) + // Platform stores all the global configuration that all // machinesets use. type Platform struct { @@ -12,8 +16,7 @@ type Platform struct { DefaultMachinePlatform *MachinePool `json:"defaultMachinePlatform,omitempty"` // NetworkCIDRBlock - // +optional - NetworkCIDRBlock string `json:"NetworkCIDRBlock"` + NetworkCIDRBlock ipnet.IPNet `json:"NetworkCIDRBlock"` // BaseImage // Name of image to use from OpenStack cloud diff --git a/pkg/types/openstack/validation/machinepool.go b/pkg/types/openstack/validation/machinepool.go new file mode 100644 index 00000000000..9e099dcecb3 --- /dev/null +++ b/pkg/types/openstack/validation/machinepool.go @@ -0,0 +1,19 @@ +package validation + +import ( + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/openshift/installer/pkg/types/openstack" +) + +// ValidateMachinePool checks that the specified machine pool is valid. +func ValidateMachinePool(p *openstack.MachinePool, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if p.RootVolume.IOPS < 0 { + allErrs = append(allErrs, field.Invalid(fldPath.Child("rootVolume").Child("iops"), p.RootVolume.IOPS, "Root volume IOPS must be positive")) + } + if p.RootVolume.Size < 0 { + allErrs = append(allErrs, field.Invalid(fldPath.Child("rootVolume").Child("size"), p.RootVolume.IOPS, "Root volume size must be positive")) + } + return allErrs +} diff --git a/pkg/types/openstack/validation/platform.go b/pkg/types/openstack/validation/platform.go new file mode 100644 index 00000000000..fc31868da92 --- /dev/null +++ b/pkg/types/openstack/validation/platform.go @@ -0,0 +1,23 @@ +package validation + +import ( + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/openshift/installer/pkg/types/openstack" + "github.com/openshift/installer/pkg/validate" +) + +// ValidatePlatform checks that the specified platform is valid. +func ValidatePlatform(p *openstack.Platform, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if p.DefaultMachinePlatform != nil { + allErrs = append(allErrs, ValidateMachinePool(p.DefaultMachinePlatform, fldPath.Child("defaultMachinePlatform"))...) + } + if err := validate.SubnetCIDR(&p.NetworkCIDRBlock.IPNet); err != nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("NetworkCIDRBlock"), p.NetworkCIDRBlock, err.Error())) + } + if err := validate.URI(p.BaseImage); err != nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("baseImage"), p.BaseImage, err.Error())) + } + return allErrs +} diff --git a/pkg/types/validation/installconfig.go b/pkg/types/validation/installconfig.go new file mode 100644 index 00000000000..7c12f4705ab --- /dev/null +++ b/pkg/types/validation/installconfig.go @@ -0,0 +1,126 @@ +package validation + +import ( + "fmt" + "net" + + netopv1 "github.com/openshift/cluster-network-operator/pkg/apis/networkoperator/v1" + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/openshift/installer/pkg/types" + "github.com/openshift/installer/pkg/types/aws" + awsvalidation "github.com/openshift/installer/pkg/types/aws/validation" + "github.com/openshift/installer/pkg/types/libvirt" + libvirtvalidation "github.com/openshift/installer/pkg/types/libvirt/validation" + "github.com/openshift/installer/pkg/types/openstack" + openstackvalidation "github.com/openshift/installer/pkg/types/openstack/validation" + "github.com/openshift/installer/pkg/validate" +) + +// ValidateInstallConfig checks that the specified install config is valid. +func ValidateInstallConfig(c *types.InstallConfig) field.ErrorList { + allErrs := field.ErrorList{} + if c.ObjectMeta.Name == "" { + allErrs = append(allErrs, field.Required(field.NewPath("metadata", "name"), "cluster name required")) + } + if c.ClusterID == "" { + allErrs = append(allErrs, field.Required(field.NewPath("clusterID"), "cluster ID required")) + } + if c.SSHKey != "" { + if err := validate.SSHPublicKey(c.SSHKey); err != nil { + allErrs = append(allErrs, field.Invalid(field.NewPath("sshKey"), c.SSHKey, err.Error())) + } + } + if err := validate.DomainName(c.BaseDomain); err != nil { + allErrs = append(allErrs, field.Invalid(field.NewPath("baseDomain"), c.BaseDomain, err.Error())) + } + allErrs = append(allErrs, validateNetworking(&c.Networking, field.NewPath("networking"))...) + allErrs = append(allErrs, validateMachinePools(c.Machines, field.NewPath("machines"), c.Platform.Name())...) + allErrs = append(allErrs, validatePlatform(&c.Platform, field.NewPath("platform"))...) + if err := validate.ImagePullSecret(c.PullSecret); err != nil { + allErrs = append(allErrs, field.Invalid(field.NewPath("pullSecret"), c.PullSecret, err.Error())) + } + return allErrs +} + +func validateNetworking(n *types.Networking, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + if !validate.ValidNetworkTypes[n.Type] { + allErrs = append(allErrs, field.NotSupported(fldPath.Child("type"), n.Type, validate.ValidNetworkTypeValues)) + } + if err := validate.SubnetCIDR(&n.ServiceCIDR.IPNet); err != nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("serviceCIDR"), n.ServiceCIDR, err.Error())) + } + for i, cn := range n.ClusterNetworks { + allErrs = append(allErrs, validateClusterNetwork(&cn, fldPath.Child("clusterNetworks").Index(i), &n.ServiceCIDR.IPNet)...) + } + if !n.PodCIDR.IPNet.IP.IsUnspecified() { + if err := validate.SubnetCIDR(&n.PodCIDR.IPNet); err != nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("podCIDR"), n.PodCIDR, err.Error())) + } + if validate.DoCIDRsOverlap(&n.ServiceCIDR.IPNet, &n.PodCIDR.IPNet) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("podCIDR"), n.PodCIDR, "podCIDR must not overlap with serviceCIDR")) + } + } + if len(n.ClusterNetworks) == 0 && n.PodCIDR.IPNet.IP.IsUnspecified() { + allErrs = append(allErrs, field.Invalid(fldPath, n, "either clusterNetworks or podCIDR is required")) + } + if len(n.ClusterNetworks) != 0 && !n.PodCIDR.IPNet.IP.IsUnspecified() { + allErrs = append(allErrs, field.Invalid(fldPath.Child("podCIDR"), n.PodCIDR, "cannot use podCIDR when clusterNetworks is used")) + } + return allErrs +} + +func validateClusterNetwork(cn *netopv1.ClusterNetwork, fldPath *field.Path, serviceCIDR *net.IPNet) field.ErrorList { + allErrs := field.ErrorList{} + _, cidr, err := net.ParseCIDR(cn.CIDR) + if err != nil { + allErrs = append(allErrs, field.Invalid(fldPath.Child("cidr"), cn.CIDR, err.Error())) + } + if validate.DoCIDRsOverlap(cidr, serviceCIDR) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("cidr"), cn.CIDR, "cluster network CIDR must not overlap with serviceCIDR")) + } + if cn.HostSubnetLength <= 0 { + allErrs = append(allErrs, field.Invalid(fldPath.Child("hostSubnetLength"), cn.HostSubnetLength, "cluster network host subnet length must be positive")) + } + if _, bits := cidr.Mask.Size(); cn.HostSubnetLength > uint32(bits) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("hostSubnetLength"), cn.HostSubnetLength, "cluster network host subnet lenght must not be greater than CIDR length")) + } + return allErrs +} + +func validateMachinePools(pools []types.MachinePool, fldPath *field.Path, platform string) field.ErrorList { + allErrs := field.ErrorList{} + if len(pools) != 2 { + allErrs = append(allErrs, field.Required(fldPath, "must have a single 'master' and a single 'worker' machine pool")) + } + for i, p := range pools { + allErrs = append(allErrs, ValidateMachinePool(&p, fldPath.Index(i), platform)...) + } + return allErrs +} + +func validatePlatform(platform *types.Platform, fldPath *field.Path) field.ErrorList { + allErrs := field.ErrorList{} + activePlatform := "" + validate := func(n string, value interface{}, validation func(*field.Path) field.ErrorList) { + if activePlatform != "" { + allErrs = append(allErrs, field.Invalid(fldPath, platform, fmt.Sprintf("must only specify a single type of platform; cannot use both %q and %q", activePlatform, n))) + } else { + activePlatform = n + } + validation(fldPath.Child(n)) + } + if platform.AWS != nil { + validate(aws.Name, platform.AWS, func(f *field.Path) field.ErrorList { return awsvalidation.ValidatePlatform(platform.AWS, f) }) + } + if platform.Libvirt != nil { + validate(libvirt.Name, platform.Libvirt, func(f *field.Path) field.ErrorList { return libvirtvalidation.ValidatePlatform(platform.Libvirt, f) }) + } + if platform.OpenStack != nil { + validate(openstack.Name, platform.OpenStack, func(f *field.Path) field.ErrorList { + return openstackvalidation.ValidatePlatform(platform.OpenStack, f) + }) + } + return allErrs +} diff --git a/pkg/types/validation/machinepools.go b/pkg/types/validation/machinepools.go new file mode 100644 index 00000000000..58306285f5e --- /dev/null +++ b/pkg/types/validation/machinepools.go @@ -0,0 +1,69 @@ +package validation + +import ( + "fmt" + + "k8s.io/apimachinery/pkg/util/validation/field" + + "github.com/openshift/installer/pkg/types" + "github.com/openshift/installer/pkg/types/aws" + awsvalidation "github.com/openshift/installer/pkg/types/aws/validation" + "github.com/openshift/installer/pkg/types/libvirt" + libvirtvalidation "github.com/openshift/installer/pkg/types/libvirt/validation" + "github.com/openshift/installer/pkg/types/openstack" + openstackvalidation "github.com/openshift/installer/pkg/types/openstack/validation" +) + +var ( + validMachinePoolNames = map[string]bool{ + "master": true, + "worker": true, + } + + validMachinePoolNameValues = func() []string { + validValues := make([]string, len(validMachinePoolNames)) + i := 0 + for n := range validMachinePoolNames { + validValues[i] = n + i++ + } + return validValues + }() +) + +// ValidateMachinePool checks that the specified machine pool is valid. +func ValidateMachinePool(p *types.MachinePool, fldPath *field.Path, platform string) field.ErrorList { + allErrs := field.ErrorList{} + if !validMachinePoolNames[p.Name] { + allErrs = append(allErrs, field.NotSupported(fldPath.Child("name"), p.Name, validMachinePoolNameValues)) + } + if p.Replicas != nil { + if *p.Replicas <= 0 { + allErrs = append(allErrs, field.Invalid(fldPath.Child("replicas"), p.Replicas, "number of replicas must be positive")) + } + } + allErrs = append(allErrs, validateMachinePoolPlatform(&p.Platform, fldPath.Child("platform"), platform)...) + return allErrs +} + +func validateMachinePoolPlatform(p *types.MachinePoolPlatform, fldPath *field.Path, platform string) field.ErrorList { + allErrs := field.ErrorList{} + validate := func(n string, value interface{}, validation func(*field.Path) field.ErrorList) { + f := fldPath.Child(n) + if platform == n { + allErrs = append(allErrs, validation(f)...) + } else { + allErrs = append(allErrs, field.Invalid(f, value, fmt.Sprintf("cannot specify %q for machine pool when cluster is using %q", n, platform))) + } + } + if p.AWS != nil { + validate(aws.Name, p.AWS, func(f *field.Path) field.ErrorList { return awsvalidation.ValidateMachinePool(p.AWS, f) }) + } + if p.Libvirt != nil { + validate(libvirt.Name, p.Libvirt, func(f *field.Path) field.ErrorList { return libvirtvalidation.ValidateMachinePool(p.Libvirt, f) }) + } + if p.OpenStack != nil { + validate(openstack.Name, p.OpenStack, func(f *field.Path) field.ErrorList { return openstackvalidation.ValidateMachinePool(p.OpenStack, f) }) + } + return allErrs +} diff --git a/pkg/validate/validate.go b/pkg/validate/validate.go index dc89541f916..bc744b64c99 100644 --- a/pkg/validate/validate.go +++ b/pkg/validate/validate.go @@ -8,15 +8,42 @@ import ( "net" "net/url" "regexp" - "strconv" "strings" "unicode/utf8" + netopv1 "github.com/openshift/cluster-network-operator/pkg/apis/networkoperator/v1" "golang.org/x/crypto/ssh" k8serrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/validation" ) +var ( + dockerBridgeCIDR = func() *net.IPNet { + _, cidr, _ := net.ParseCIDR("172.17.0.0/16") + return cidr + }() + + // ValidNetworkTypes is a collection of the valid network types. + ValidNetworkTypes = map[netopv1.NetworkType]bool{ + netopv1.NetworkTypeOpenshiftSDN: true, + netopv1.NetworkTypeOVNKubernetes: true, + netopv1.NetworkTypeCalico: true, + netopv1.NetworkTypeKuryr: true, + } + + // ValidNetworkTypeValues is a slice filled with the valid network types as + // strings. + ValidNetworkTypeValues = func() []string { + validValues := make([]string, len(ValidNetworkTypes)) + i := 0 + for t := range ValidNetworkTypes { + validValues[i] = string(t) + i++ + } + return validValues + }() +) + func validateSubdomain(v string) error { validationMessages := validation.IsDNS1123Subdomain(v) if len(validationMessages) == 0 { @@ -105,80 +132,23 @@ func nonEmpty(v string) error { return nil } -// SubnetCIDR checks if the given string is a valid CIDR for a master nodes or worker nodes subnet and returns an error if not. -func SubnetCIDR(v string) error { - if err := nonEmpty(v); err != nil { - return err - } - - split := strings.Split(v, "/") - - if len(split) == 1 { - return errors.New("must provide a CIDR netmask (eg, /24)") +// SubnetCIDR checks if the given IP net is a valid CIDR for a master nodes or worker nodes subnet and returns an error if not. +func SubnetCIDR(cidr *net.IPNet) error { + if cidr.IP.To4() == nil { + return errors.New("must use IPv4") } - - if len(split) != 2 { - return errors.New("invalid IPv4 address") - } - - ip := split[0] - - if err := IPv4(ip); err != nil { - return errors.New("invalid IPv4 address") - } - - if mask, err := strconv.Atoi(split[1]); err != nil || mask < 0 || mask > 32 { - return errors.New("invalid netmask size (must be between 0 and 32)") - } - - // Catch any invalid CIDRs not caught by the checks above - if _, _, err := net.ParseCIDR(v); err != nil { - return errors.New("invalid CIDR") + if cidr.IP.IsUnspecified() { + return errors.New("address must be specified") } - - if strings.HasPrefix(ip, "172.17.") { - return errors.New("overlaps with default Docker Bridge subnet (172.17.0.0/16)") + if DoCIDRsOverlap(cidr, dockerBridgeCIDR) { + return fmt.Errorf("overlaps with default Docker Bridge subnet (%v)", cidr.String()) } - return nil } -// CIDRsDontOverlap ensures two given CIDRs don't overlap -// with one another. CIDR starting IPs are canonicalized -// before being compared. -func CIDRsDontOverlap(acidr, bcidr string) error { - _, a, err := net.ParseCIDR(acidr) - if err != nil { - return fmt.Errorf("invalid CIDR %q: %v", acidr, err) - } - if err := canonicalizeIP(&a.IP); err != nil { - return fmt.Errorf("invalid CIDR %q: %v", acidr, err) - } - _, b, err := net.ParseCIDR(bcidr) - if err != nil { - return fmt.Errorf("invalid CIDR %q: %v", bcidr, err) - } - if err := canonicalizeIP(&b.IP); err != nil { - return fmt.Errorf("invalid CIDR %q: %v", bcidr, err) - } - err = fmt.Errorf("%q and %q overlap", acidr, bcidr) - // IPs are of different families. - if len(a.IP) != len(b.IP) { - return nil - } - if a.Contains(b.IP) { - return err - } - if a.Contains(lastIP(b)) { - return err - } - if b.Contains(a.IP) { - return err - } - if b.Contains(lastIP(a)) { - return err - } - return nil +// DoCIDRsOverlap returns true if one of the CIDRs is a subset of the other. +func DoCIDRsOverlap(acidr, bcidr *net.IPNet) bool { + return acidr.Contains(bcidr.IP) || bcidr.Contains(acidr.IP) } // IPv4 checks if the given string is a valid IP v4 address and returns an error if not. diff --git a/pkg/validate/validate_test.go b/pkg/validate/validate_test.go index b2d3ff2a1c4..363b56bc1ef 100644 --- a/pkg/validate/validate_test.go +++ b/pkg/validate/validate_test.go @@ -3,7 +3,6 @@ package validate import ( "fmt" "net" - "regexp" "strings" "testing" @@ -144,34 +143,36 @@ func TestIPv4(t *testing.T) { } func TestSubnetCIDR(t *testing.T) { - const netmaskSizeMsg = "invalid netmask size (must be between 0 and 32)" - - tests := []test{ - {"", emptyMsg}, - {" ", emptyMsg}, - {"/16", invalidIPMsg}, - {"0.0.0.0/0", ""}, - {"0.0.0.0/32", ""}, - {"1.2.3.4", noCIDRNetmaskMsg}, - {"1.2.3.", noCIDRNetmaskMsg}, - {"1.2.3.4.", noCIDRNetmaskMsg}, - {"1.2.3.4/0", ""}, - {"1.2.3.4/1", ""}, - {"1.2.3.4/31", ""}, - {"1.2.3.4/32", ""}, - {"1.2.3./16", invalidIPMsg}, - {"1.2.3.4./16", invalidIPMsg}, - {"1.2.3.4/33", netmaskSizeMsg}, - {"1.2.3.4/-1", netmaskSizeMsg}, - {"1.2.3.4/abc", netmaskSizeMsg}, - {"172.17.1.2", noCIDRNetmaskMsg}, - {"172.17.1.2/", netmaskSizeMsg}, - {"172.17.1.2/33", netmaskSizeMsg}, - {"172.17.1.2/20", "overlaps with default Docker Bridge subnet (172.17.0.0/16)"}, - {"255.255.255.255/1", ""}, - {"255.255.255.255/32", ""}, + cases := []struct { + cidr string + valid bool + }{ + {"0.0.0.0/32", false}, + {"1.2.3.4/0", false}, + {"1.2.3.4/1", false}, + {"1.2.3.4/31", true}, + {"1.2.3.4/32", true}, + {"0:0:0:0:0:1:102:304/116", false}, + {"0:0:0:0:0:ffff:102:304/116", true}, + {"172.17.1.2/20", false}, + {"172.17.1.2/8", false}, + {"255.255.255.255/1", false}, + {"255.255.255.255/32", true}, + } + for _, tc := range cases { + t.Run(tc.cidr, func(t *testing.T) { + _, cidr, err := net.ParseCIDR(tc.cidr) + if err != nil { + t.Fatalf("could not parse cidr: %v", err) + } + err = SubnetCIDR(cidr) + if tc.valid { + assert.NoError(t, err) + } else { + assert.Error(t, err) + } + }) } - runTests(t, "SubnetCIDR", SubnetCIDR, tests) } func TestDomainName(t *testing.T) { @@ -236,48 +237,45 @@ func TestEmail(t *testing.T) { runTests(t, "Email", Email, tests) } -func TestCIDRsDontOverlap(t *testing.T) { +func TestDoCIDRsOverlap(t *testing.T) { cases := []struct { - a string - b string - err *regexp.Regexp + a string + b string + overlap bool }{ { - a: "192.168.0.0/24", - b: "192.168.0.0/24", - err: regexp.MustCompile("^\"192.168.0.0/24\" and \"192.168.0.0/24\" overlap$"), + a: "192.168.0.0/30", + b: "192.168.0.3/30", + overlap: true, }, { - a: "192.168.0.0/24", - b: "192.168.0.3/24", - err: regexp.MustCompile("^\"192.168.0.0/24\" and \"192.168.0.3/24\" overlap$"), + a: "192.168.0.0/30", + b: "192.168.0.4/30", + overlap: false, }, { - a: "192.168.0.0/30", - b: "192.168.0.3/30", - err: regexp.MustCompile("^\"192.168.0.0/30\" and \"192.168.0.3/30\" overlap$"), + a: "192.168.0.0/29", + b: "192.168.0.4/30", + overlap: true, }, { - a: "192.168.0.0/30", - b: "192.168.0.4/30", - }, - { - a: "0.0.0.0/0", - b: "192.168.0.0/24", - err: regexp.MustCompile("^\"0.0.0.0/0\" and \"192.168.0.0/24\" overlap$"), + a: "0.0.0.0/0", + b: "192.168.0.0/24", + overlap: true, }, } - - for _, testCase := range cases { - t.Run(fmt.Sprintf("%s %s", testCase.a, testCase.b), func(t *testing.T) { - err := CIDRsDontOverlap(testCase.a, testCase.b) - if testCase.err == nil { - if err != nil { - t.Fatal(err) - } - } else { - assert.Regexp(t, testCase.err, err) + for _, tc := range cases { + t.Run(fmt.Sprintf("%s %s", tc.a, tc.b), func(t *testing.T) { + _, a, err := net.ParseCIDR(tc.a) + if err != nil { + t.Fatalf("could not parse cidr %q: %v", tc.a, err) + } + _, b, err := net.ParseCIDR(tc.b) + if err != nil { + t.Fatalf("could not parse cidr %q: %v", tc.b, err) } + actual := DoCIDRsOverlap(a, b) + assert.Equal(t, tc.overlap, actual) }) } }