Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

baremetal: platform host validations #3232

Merged
merged 2 commits into from
Mar 20, 2020
Merged

baremetal: platform host validations #3232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
49e6c38
vendor: Added go-playground/validator module
andfasano Mar 4, 2020
508e7e4
validation: add host validations
andfasano Mar 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ require (
github.com/frankban/quicktest v1.7.2 // indirect
github.com/ghodss/yaml v1.0.1-0.20190212211648-25d852aebe32
github.com/go-logr/zapr v0.1.1 // indirect
github.com/go-playground/validator/v10 v10.2.0
github.com/golang/mock v1.3.1
github.com/google/go-cmp v0.3.2-0.20191028172631-481baca67f93 // indirect
github.com/google/martian v2.1.1-0.20190517191504-25dcb96d9e51+incompatible // indirect
Expand Down
10 changes: 10 additions & 0 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -614,6 +614,14 @@ github.com/go-openapi/validate v0.18.0/go.mod h1:Uh4HdOzKt19xGIGm1qHf/ofbX1YQ4Y+
github.com/go-openapi/validate v0.19.2/go.mod h1:1tRCw7m3jtI8eNWEEliiAqUIcBztB2KDnRCRMUi7GTA=
github.com/go-openapi/validate v0.19.5/go.mod h1:8DJv2CVJQ6kGNpFW6eV9N3JviE1C85nY1c2z52x1Gk4=
github.com/go-ozzo/ozzo-validation v3.5.0+incompatible/go.mod h1:gsEKFIVnabGBt6mXmxK0MoFy+cZoTJY6mu5Ll3LVLBU=
github.com/go-playground/assert/v2 v2.0.1 h1:MsBgLAaY856+nPRTKrp3/OZK38U/wa0CcBYNjji3q3A=
github.com/go-playground/assert/v2 v2.0.1/go.mod h1:VDjEfimB/XKnb+ZQfWdccd7VUvScMdVu0Titje2rxJ4=
github.com/go-playground/locales v0.13.0 h1:HyWk6mgj5qFqCT5fjGBuRArbVDfE4hi8+e8ceBS/t7Q=
github.com/go-playground/locales v0.13.0/go.mod h1:taPMhCMXrRLJO55olJkUXHZBHCxTMfnGwq/HNwmWNS8=
github.com/go-playground/universal-translator v0.17.0 h1:icxd5fm+REJzpZx7ZfpaD876Lmtgy7VtROAbHHXk8no=
github.com/go-playground/universal-translator v0.17.0/go.mod h1:UkSxE5sNxxRwHyU+Scu5vgOQjsIJAF8j9muTVoKLVtA=
github.com/go-playground/validator/v10 v10.2.0 h1:KgJ0snyC2R9VXYN2rneOtQcw5aHQB1Vv0sFl1UcHBOY=
github.com/go-playground/validator/v10 v10.2.0/go.mod h1:uOYAAleCW8F/7oMFd6aG0GOhaH6EGOAJShg8Id5JGkI=
github.com/go-sql-driver/mysql v1.4.0/go.mod h1:zAC/RDZ24gD3HViQzih4MyKcchzm+sOG5ZlKdlhCg5w=
github.com/go-sql-driver/mysql v1.4.1/go.mod h1:zAC/RDZ24gD3HViQzih4MyKcchzm+sOG5ZlKdlhCg5w=
github.com/go-stack/stack v1.8.0/go.mod h1:v0f6uXyyMGvRgIKkXu+yp6POWl0qKG85gN/melR3HDY=
Expand Down Expand Up @@ -1504,6 +1512,8 @@ github.com/kubernetes-sigs/kube-storage-version-migrator v0.0.0-20191127225502-5
github.com/kylelemons/godebug v0.0.0-20170820004349-d65d576e9348/go.mod h1:B69LEHPfb2qLo0BaaOLcbitczOKLWTsrBG9LczfCD4k=
github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0SNc=
github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw=
github.com/leodido/go-urn v1.2.0 h1:hpXL4XnriNwQ/ABnpepYM/1vCLWNDfUNts8dX3xTG6Y=
github.com/leodido/go-urn v1.2.0/go.mod h1:+8+nEpDfqqsY+g338gtMEUOtuK+4dEMhiQEgxpxOKII=
github.com/lib/pq v1.0.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo=
github.com/lib/pq v1.1.1/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo=
github.com/lib/pq v1.2.0 h1:LXpIM/LZ5xGFhOpXAQUIMM1HdyqzVYM13zNdjCEEcA0=
Expand Down
10 changes: 5 additions & 5 deletions pkg/types/baremetal/platform.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,18 @@ import (

// BMC stores the information about a baremetal host's management controller.
type BMC struct {
Username string `json:"username"`
Password string `json:"password"`
Address string `json:"address"`
Username string `json:"username" validate:"required"`
Password string `json:"password" validate:"required"`
Address string `json:"address" validate:"required,uniqueField"`
DisableCertificateVerification bool `json:"disableCertificateVerification"`
}

// Host stores all the configuration data for a baremetal host.
type Host struct {
Name string `json:"name,omitempty"`
Name string `json:"name,omitempty" validate:"required,uniqueField"`
BMC BMC `json:"bmc"`
Role string `json:"role"`
BootMACAddress string `json:"bootMACAddress"`
BootMACAddress string `json:"bootMACAddress" validate:"required,uniqueField"`
HardwareProfile string `json:"hardwareProfile"`
}

Expand Down
58 changes: 57 additions & 1 deletion pkg/types/baremetal/validation/platform.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,15 @@ import (
"fmt"
"net"
"net/url"
"reflect"
"strings"

"github.com/go-playground/validator/v10"

"github.com/openshift/installer/pkg/types"
"github.com/openshift/installer/pkg/types/baremetal"
"github.com/openshift/installer/pkg/validate"
"k8s.io/apimachinery/pkg/util/validation/field"
"strings"
)

// dynamicValidator is a function that validates certain fields in the platform.
Expand Down Expand Up @@ -61,6 +64,57 @@ func validateOSImageURI(uri string) error {
return nil
}

// validateHosts checks that hosts have all required fields set with appropriate values
func validateHosts(hosts []*baremetal.Host, fldPath *field.Path) 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 {
andfasano marked this conversation as resolved.
Show resolved Hide resolved
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()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

panics are less desirable in the code for installer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't sure about the standard adopted, I noticed that it was used somewhere (https://github.com/openshift/installer/search?q=panic+-path%3A%2Fvendor&unscoped_q=panic+-path%3A%2Fvendor) and thought it could have been useful in case of configuration mistake. But if it's not I'll remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A panic seems fair here, this would only happen if a dev adds the annotation that a field is uniqueValue and it's not Comparable.

}

return !valueFound
})

//Apply validations and translate errors
fldPath = fldPath.Child("hosts")

for idx, host := range hosts {
err := validate.Struct(host)
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
}

// ValidatePlatform checks that the specified platform is valid.
func ValidatePlatform(p *baremetal.Platform, n *types.Networking, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
Expand Down Expand Up @@ -156,6 +210,8 @@ func ValidatePlatform(p *baremetal.Platform, n *types.Networking, fldPath *field
}
}

allErrs = append(allErrs, validateHosts(p.Hosts, fldPath)...)

for _, validator := range dynamicValidators {
allErrs = append(allErrs, validator(p, fldPath)...)
}
Expand Down
248 changes: 247 additions & 1 deletion pkg/types/baremetal/validation/platform_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,11 +481,257 @@ func TestValidatePlatform(t *testing.T) {
network: network,
expected: "Invalid value: \"192.168.128.1\": \"192.168.128.1\" is not in the provisioning network",
},
{
name: "duplicate_bmc_address",
platform: &baremetal.Platform{
APIVIP: "192.168.111.2",
DNSVIP: "192.168.111.3",
IngressVIP: "192.168.111.4",
Hosts: []*baremetal.Host{
{
Name: "host1",
BootMACAddress: "CA:FE:CA:FE:00:00",
BMC: baremetal.BMC{
Username: "root",
Password: "password",
Address: "ipmi://192.168.111.1",
},
},
{
Name: "host2",
BootMACAddress: "CA:FE:CA:FE:00:01",
BMC: baremetal.BMC{
Username: "root",
Password: "password",
Address: "ipmi://192.168.111.1",
},
},
},
LibvirtURI: "qemu://system",
ProvisioningNetworkCIDR: ipnet.MustParseCIDR("172.22.0.0/24"),
ClusterProvisioningIP: "172.22.0.3",
BootstrapProvisioningIP: "172.22.0.2",
ExternalBridge: "br0",
ProvisioningBridge: "br1",
ProvisioningNetworkInterface: "ens3",
},
network: network,
expected: "baremetal.hosts\\[1\\].BMC.Address: Duplicate value: \"ipmi://192.168.111.1\"",
},
{
name: "bmc_address_required",
platform: &baremetal.Platform{
APIVIP: "192.168.111.2",
DNSVIP: "192.168.111.3",
IngressVIP: "192.168.111.4",
Hosts: []*baremetal.Host{
{
Name: "host1",
BootMACAddress: "CA:FE:CA:FE:00:00",
BMC: baremetal.BMC{
Username: "root",
Password: "password",
},
},
},
LibvirtURI: "qemu://system",
ProvisioningNetworkCIDR: ipnet.MustParseCIDR("172.22.0.0/24"),
ClusterProvisioningIP: "172.22.0.3",
BootstrapProvisioningIP: "172.22.0.2",
ExternalBridge: "br0",
ProvisioningBridge: "br1",
ProvisioningNetworkInterface: "ens3",
},
network: network,
expected: "baremetal.hosts\\[0\\].BMC.Address: Required value: missing Address",
},
{
name: "bmc_username_required",
platform: &baremetal.Platform{
APIVIP: "192.168.111.2",
DNSVIP: "192.168.111.3",
IngressVIP: "192.168.111.4",
Hosts: []*baremetal.Host{
{
Name: "host1",
BootMACAddress: "CA:FE:CA:FE:00:00",
BMC: baremetal.BMC{
Password: "password",
Address: "ipmi://192.168.111.1",
},
},
},
LibvirtURI: "qemu://system",
ProvisioningNetworkCIDR: ipnet.MustParseCIDR("172.22.0.0/24"),
ClusterProvisioningIP: "172.22.0.3",
BootstrapProvisioningIP: "172.22.0.2",
ExternalBridge: "br0",
ProvisioningBridge: "br1",
ProvisioningNetworkInterface: "ens3",
},
network: network,
expected: "baremetal.hosts\\[0\\].BMC.Username: Required value: missing Username",
},
{
name: "bmc_password_required",
platform: &baremetal.Platform{
APIVIP: "192.168.111.2",
DNSVIP: "192.168.111.3",
IngressVIP: "192.168.111.4",
Hosts: []*baremetal.Host{
{
Name: "host1",
BootMACAddress: "CA:FE:CA:FE:00:00",
BMC: baremetal.BMC{
Username: "root",
Address: "ipmi://192.168.111.1",
},
},
},
LibvirtURI: "qemu://system",
ProvisioningNetworkCIDR: ipnet.MustParseCIDR("172.22.0.0/24"),
ClusterProvisioningIP: "172.22.0.3",
BootstrapProvisioningIP: "172.22.0.2",
ExternalBridge: "br0",
ProvisioningBridge: "br1",
ProvisioningNetworkInterface: "ens3",
},
network: network,
expected: "baremetal.hosts\\[0\\].BMC.Password: Required value: missing Password",
},
{
name: "duplicate_host_name",
platform: &baremetal.Platform{
APIVIP: "192.168.111.2",
DNSVIP: "192.168.111.3",
IngressVIP: "192.168.111.4",
Hosts: []*baremetal.Host{
{
Name: "host1",
BootMACAddress: "CA:FE:CA:FE:00:00",
BMC: baremetal.BMC{
Username: "root",
Password: "password",
Address: "ipmi://192.168.111.1",
},
},
{
Name: "host1",
BootMACAddress: "CA:FE:CA:FE:00:01",
BMC: baremetal.BMC{
Username: "root",
Password: "password",
Address: "ipmi://192.168.111.2",
},
},
},
LibvirtURI: "qemu://system",
ProvisioningNetworkCIDR: ipnet.MustParseCIDR("172.22.0.0/24"),
ClusterProvisioningIP: "172.22.0.3",
BootstrapProvisioningIP: "172.22.0.2",
ExternalBridge: "br0",
ProvisioningBridge: "br1",
ProvisioningNetworkInterface: "ens3",
},
network: network,
expected: "baremetal.hosts\\[1\\].Name: Duplicate value: \"host1\"",
},
{
name: "duplicate_host_mac",
platform: &baremetal.Platform{
APIVIP: "192.168.111.2",
DNSVIP: "192.168.111.3",
IngressVIP: "192.168.111.4",
Hosts: []*baremetal.Host{
{
Name: "host1",
BootMACAddress: "CA:FE:CA:FE:CA:FE",
BMC: baremetal.BMC{
Username: "root",
Password: "password",
Address: "ipmi://192.168.111.1",
},
},
{
Name: "host2",
BootMACAddress: "CA:FE:CA:FE:CA:FE",
BMC: baremetal.BMC{
Username: "root",
Password: "password",
Address: "ipmi://192.168.111.2",
},
},
},
LibvirtURI: "qemu://system",
ProvisioningNetworkCIDR: ipnet.MustParseCIDR("172.22.0.0/24"),
ClusterProvisioningIP: "172.22.0.3",
BootstrapProvisioningIP: "172.22.0.2",
ExternalBridge: "br0",
ProvisioningBridge: "br1",
ProvisioningNetworkInterface: "ens3",
},
network: network,
expected: "baremetal.hosts\\[1\\].BootMACAddress: Duplicate value: \"CA:FE:CA:FE:CA:FE\"",
},
{
name: "missing_name",
platform: &baremetal.Platform{
APIVIP: "192.168.111.2",
DNSVIP: "192.168.111.3",
IngressVIP: "192.168.111.4",
Hosts: []*baremetal.Host{
{
BootMACAddress: "CA:FE:CA:FE:CA:FE",
BMC: baremetal.BMC{
Username: "root",
Password: "password",
Address: "ipmi://192.168.111.1",
},
},
},
LibvirtURI: "qemu://system",
ProvisioningNetworkCIDR: ipnet.MustParseCIDR("172.22.0.0/24"),
ClusterProvisioningIP: "172.22.0.3",
BootstrapProvisioningIP: "172.22.0.2",
ExternalBridge: "br0",
ProvisioningBridge: "br1",
ProvisioningNetworkInterface: "ens3",
},
network: network,
expected: "baremetal.hosts\\[0\\].Name: Required value: missing Name",
},
{
name: "missing_mac",
platform: &baremetal.Platform{
APIVIP: "192.168.111.2",
DNSVIP: "192.168.111.3",
IngressVIP: "192.168.111.4",
Hosts: []*baremetal.Host{
{
Name: "host1",
BMC: baremetal.BMC{
Username: "root",
Password: "password",
Address: "ipmi://192.168.111.1",
},
},
},
LibvirtURI: "qemu://system",
ProvisioningNetworkCIDR: ipnet.MustParseCIDR("172.22.0.0/24"),
ClusterProvisioningIP: "172.22.0.3",
BootstrapProvisioningIP: "172.22.0.2",
ExternalBridge: "br0",
ProvisioningBridge: "br1",
ProvisioningNetworkInterface: "ens3",
},
network: network,
expected: "baremetal.hosts\\[0\\].BootMACAddress: Required value: missing BootMACAddress",
},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
err := ValidatePlatform(tc.platform, tc.network, field.NewPath("test-path")).ToAggregate()
err := ValidatePlatform(tc.platform, tc.network, field.NewPath("baremetal")).ToAggregate()
if tc.expected == "" {
assert.NoError(t, err)
} else {
Expand Down
Loading