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

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{

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{

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 {

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
 }

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,

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,

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,
 	}

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
+}

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.

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

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"`
 }

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"`
 }

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
+}

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
+}