Skip to content

Commit

Permalink
Merge pull request #3372 from patrickdillon/vsphere-ipi-validate
Browse files Browse the repository at this point in the history
vSphere: Add IPI-specific validation.
  • Loading branch information
openshift-merge-robot authored Apr 2, 2020
2 parents 62e4064 + c7540c0 commit b74e391
Show file tree
Hide file tree
Showing 9 changed files with 1,768 additions and 1,110 deletions.
2,576 changes: 1,482 additions & 1,094 deletions docs/design/resource_dep.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
7 changes: 4 additions & 3 deletions pkg/asset/cluster/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,13 @@ func (c *Cluster) Dependencies() []asset.Asset {
return []asset.Asset{
&installconfig.ClusterID{},
&installconfig.InstallConfig{},
// PlatformCredsCheck checks the creds (and asks, if needed).
// PlatformPermsCheck checks for required account permissions.
// PlatformCredsCheck, PlatformPermsCheck and PlatformProvisionCheck
// perform validations & check perms required to provision infrastructure.
// We do not actually use them in this asset directly, hence
// they are put in the dependencies but not fetched in Generate
// they are put in the dependencies but not fetched in Generate.
&installconfig.PlatformCredsCheck{},
&installconfig.PlatformPermsCheck{},
&installconfig.PlatformProvisionCheck{},
&TerraformVariables{},
&password.KubeadminPassword{},
}
Expand Down
4 changes: 4 additions & 0 deletions pkg/asset/installconfig/installconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
icazure "github.com/openshift/installer/pkg/asset/installconfig/azure"
icgcp "github.com/openshift/installer/pkg/asset/installconfig/gcp"
icopenstack "github.com/openshift/installer/pkg/asset/installconfig/openstack"
icvsphere "github.com/openshift/installer/pkg/asset/installconfig/vsphere"
"github.com/openshift/installer/pkg/types"
"github.com/openshift/installer/pkg/types/conversion"
"github.com/openshift/installer/pkg/types/defaults"
Expand Down Expand Up @@ -173,5 +174,8 @@ func (a *InstallConfig) platformValidation() error {
if a.Config.Platform.AWS != nil {
return aws.Validate(context.TODO(), a.AWS, a.Config)
}
if a.Config.Platform.VSphere != nil {
return icvsphere.Validate(a.Config)
}
return field.ErrorList{}.ToAggregate()
}
57 changes: 57 additions & 0 deletions pkg/asset/installconfig/platformprovisioncheck.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package installconfig

import (
"fmt"

"github.com/openshift/installer/pkg/asset"
vsconfig "github.com/openshift/installer/pkg/asset/installconfig/vsphere"
"github.com/openshift/installer/pkg/types/aws"
"github.com/openshift/installer/pkg/types/azure"
"github.com/openshift/installer/pkg/types/baremetal"
"github.com/openshift/installer/pkg/types/gcp"
"github.com/openshift/installer/pkg/types/libvirt"
"github.com/openshift/installer/pkg/types/none"
"github.com/openshift/installer/pkg/types/openstack"
"github.com/openshift/installer/pkg/types/ovirt"
"github.com/openshift/installer/pkg/types/vsphere"
)

// PlatformProvisionCheck is an asset that validates the install-config platform for
// any requirements specific for provisioning infrastructure.
type PlatformProvisionCheck struct {
}

var _ asset.Asset = (*PlatformProvisionCheck)(nil)

// Dependencies returns the dependencies for PlatformProvisionCheck
func (a *PlatformProvisionCheck) Dependencies() []asset.Asset {
return []asset.Asset{
&InstallConfig{},
}
}

// Generate queries for input from the user.
func (a *PlatformProvisionCheck) Generate(dependencies asset.Parents) error {
ic := &InstallConfig{}
dependencies.Get(ic)

var err error
platform := ic.Config.Platform.Name()
switch platform {
case vsphere.Name:
err = vsconfig.ValidateForProvisioning(ic.Config)
if err != nil {
return err
}
case azure.Name, aws.Name, baremetal.Name, gcp.Name, libvirt.Name, none.Name, openstack.Name, ovirt.Name:
// no special provisioning requirements to check
default:
err = fmt.Errorf("unknown platform type %q", platform)
}
return err
}

// Name returns the human-friendly name of the asset.
func (a *PlatformProvisionCheck) Name() string {
return "Platform Provisioning Check"
}
35 changes: 35 additions & 0 deletions pkg/asset/installconfig/vsphere/validation.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package vsphere

import (
"github.com/pkg/errors"
"k8s.io/apimachinery/pkg/util/validation/field"

"github.com/openshift/installer/pkg/types"
"github.com/openshift/installer/pkg/types/vsphere/validation"
)

// Validate executes platform-specific validation.
func Validate(ic *types.InstallConfig) error {
allErrs := field.ErrorList{}
if ic.Platform.VSphere == nil {
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, field.NewPath("platform").Child("vsphere"))...)

return allErrs.ToAggregate()
}

// ValidateForProvisioning performs platform validation specifically for installer-
// provisioned infrastructure. In this case, self-hosted networking is a requirement
// when the installer creates infrastructure for vSphere clusters.
func ValidateForProvisioning(ic *types.InstallConfig) error {
allErrs := field.ErrorList{}
if ic.Platform.VSphere == nil {
return errors.New(field.Required(field.NewPath("platform", "vsphere"), "vSphere validation requires a vSphere platform configuration").Error())
}

allErrs = append(allErrs, validation.ValidateForProvisioning(ic.Platform.VSphere, field.NewPath("platform").Child("vsphere"))...)

return allErrs.ToAggregate()
}
106 changes: 106 additions & 0 deletions pkg/asset/installconfig/vsphere/validation_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
package vsphere

import (
"testing"

"github.com/stretchr/testify/assert"

"github.com/openshift/installer/pkg/ipnet"
"github.com/openshift/installer/pkg/types"
"github.com/openshift/installer/pkg/types/vsphere"
)

var (
validCIDR = "10.0.0.0/16"
)

func validIPIInstallConfig() *types.InstallConfig {
return &types.InstallConfig{
Networking: &types.Networking{
MachineNetwork: []types.MachineNetworkEntry{
{CIDR: *ipnet.MustParseCIDR(validCIDR)},
},
},
Publish: types.ExternalPublishingStrategy,
Platform: types.Platform{
VSphere: &vsphere.Platform{
Cluster: "valid_cluster",
Datacenter: "valid_dc",
DefaultDatastore: "valid_ds",
Network: "valid_network",
Password: "valid_password",
Username: "valid_username",
VCenter: "valid_vcenter",
APIVIP: "192.168.111.0",
IngressVIP: "192.168.111.1",
DNSVIP: "192.168.111.2",
},
},
}
}

func validUPIInstallConfig() *types.InstallConfig {
return &types.InstallConfig{
Networking: &types.Networking{
MachineNetwork: []types.MachineNetworkEntry{
{CIDR: *ipnet.MustParseCIDR(validCIDR)},
},
},
Publish: types.ExternalPublishingStrategy,
Platform: types.Platform{
VSphere: &vsphere.Platform{
Datacenter: "valid_dc",
DefaultDatastore: "valid_ds",
Password: "valid_password",
Username: "valid_username",
VCenter: "valid_vcenter",
},
},
}
}

func TestValidate(t *testing.T) {
tests := []struct {
name string
installConfig *types.InstallConfig
validationMethod func(*types.InstallConfig) error
expectErr string
}{{
name: "valid UPI install config",
installConfig: validUPIInstallConfig(),
validationMethod: Validate,
}, {
name: "valid IPI install config",
installConfig: validIPIInstallConfig(),
validationMethod: ValidateForProvisioning,
}, {
name: "invalid IPI - no network",
installConfig: func() *types.InstallConfig {
c := validIPIInstallConfig()
c.Platform.VSphere.Network = ""
return c
}(),
validationMethod: ValidateForProvisioning,
expectErr: `^platform\.vsphere\.network: Required value: must specify the network$`,
}, {
name: "invalid IPI - no cluster",
installConfig: func() *types.InstallConfig {
c := validIPIInstallConfig()
c.Platform.VSphere.Cluster = ""
return c
}(),
validationMethod: ValidateForProvisioning,
expectErr: `^platform\.vsphere\.cluster: Required value: must specify the cluster$`,
}}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
err := test.validationMethod(test.installConfig)
if test.expectErr == "" {
assert.NoError(t, err)
} else {
assert.Regexp(t, test.expectErr, err.Error())
}
})
}
}
2 changes: 1 addition & 1 deletion pkg/types/vsphere/platform.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,5 +43,5 @@ type Platform struct {
DefaultMachinePlatform *MachinePool `json:"defaultMachinePlatform,omitempty"`

// Network specifies the name of the network to be used by the cluster.
Network string `json:"network,omitempty"` //TODO: determine if this should be omitempty or required
Network string `json:"network,omitempty"`
}
52 changes: 43 additions & 9 deletions pkg/types/vsphere/validation/platform.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,49 @@ func ValidatePlatform(p *vsphere.Platform, fldPath *field.Path) field.ErrorList

// If all VIPs are empty, skip IP validation. All VIPs are required to be defined together.
if strings.Join([]string{p.APIVIP, p.IngressVIP, p.DNSVIP}, "") != "" {
if err := validate.IP(p.APIVIP); err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("apiVIP"), p.APIVIP, err.Error()))
}
if err := validate.IP(p.IngressVIP); err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("ingressVIP"), p.IngressVIP, err.Error()))
}
if err := validate.IP(p.DNSVIP); err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("dnsVIP"), p.DNSVIP, err.Error()))
}
allErrs = append(allErrs, validateVIPs(p, fldPath)...)
}

return allErrs
}

// ValidateForProvisioning checks that the specified platform is valid.
func ValidateForProvisioning(p *vsphere.Platform, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}

if len(p.Cluster) == 0 {
allErrs = append(allErrs, field.Required(fldPath.Child("cluster"), "must specify the cluster"))
}

if len(p.Network) == 0 {
allErrs = append(allErrs, field.Required(fldPath.Child("network"), "must specify the network"))
}

allErrs = append(allErrs, validateVIPs(p, fldPath)...)

return allErrs
}

// ValidateVIPs checks that all required VIPs are provided and are valid IP addresses.
func validateVIPs(p *vsphere.Platform, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}

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 {
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.DNSVIP) == 0 {
allErrs = append(allErrs, field.Required(fldPath.Child("dnsVIP"), "must specify a VIP for DNS"))
} else if err := validate.IP(p.DNSVIP); err != nil {
allErrs = append(allErrs, field.Invalid(fldPath.Child("dnsVIP"), p.DNSVIP, err.Error()))
}

return allErrs
Expand Down
39 changes: 36 additions & 3 deletions pkg/types/vsphere/validation/platform_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ func TestValidatePlatform(t *testing.T) {
p.DNSVIP = "192.168.111.4"
return p
}(),
expectedError: `^test-path\.apiVIP: Invalid value: "": "" is not a valid IP`,
expectedError: `^test-path\.apiVIP: Required value: must specify a VIP for the API`,
},
{
name: "missing Ingress VIP",
Expand All @@ -105,7 +105,7 @@ func TestValidatePlatform(t *testing.T) {
p.DNSVIP = "192.168.111.4"
return p
}(),
expectedError: `^test-path\.ingressVIP: Invalid value: "": "" is not a valid IP`,
expectedError: `^test-path\.ingressVIP: Required value: must specify a VIP for Ingress`,
},
{
name: "missing DNS VIP",
Expand All @@ -116,7 +116,40 @@ func TestValidatePlatform(t *testing.T) {
p.DNSVIP = ""
return p
}(),
expectedError: `^test-path\.dnsVIP: Invalid value: "": "" is not a valid IP`,
expectedError: `^test-path\.dnsVIP: Required value: must specify a VIP for DNS`,
},
{
name: "Invalid API VIP",
platform: func() *vsphere.Platform {
p := validPlatform()
p.APIVIP = "192.168.111"
p.IngressVIP = "192.168.111.2"
p.DNSVIP = "192.168.111.3"
return p
}(),
expectedError: `^test-path.apiVIP: Invalid value: "192.168.111": "192.168.111" is not a valid IP`,
},
{
name: "Invalid Ingress VIP",
platform: func() *vsphere.Platform {
p := validPlatform()
p.APIVIP = "192.168.111.1"
p.IngressVIP = "192.168.111"
p.DNSVIP = "192.168.111.3"
return p
}(),
expectedError: `^test-path.ingressVIP: Invalid value: "192.168.111": "192.168.111" is not a valid IP`,
},
{
name: "Invalid DNS VIP",
platform: func() *vsphere.Platform {
p := validPlatform()
p.APIVIP = "192.168.111.2"
p.IngressVIP = "192.168.111.3"
p.DNSVIP = "192.168.111"
return p
}(),
expectedError: `^test-path.dnsVIP: Invalid value: "192.168.111": "192.168.111" is not a valid IP`,
},
}
for _, tc := range cases {
Expand Down

0 comments on commit b74e391

Please sign in to comment.