Repository: metal3-io/baremetal-operator
Pull request #1070 (2022-01-14 14:05:21)

Add physicalDisk and controller WebHook validation for HardwareRAID
This patch adds WebHook validation for Hardware RAID to validate the following:

1. ``physicalDisks`` specified without ``controller`` not allowed.
2. ``numberOfPhysicalDisks`` and length of ``physicalDisks`` must be consistent. 

This is in accordance with the WebHook validation outline provided in #855 . The functionality that enables these parameters in ``HardwareRAID`` is available in #1062 

The patch also provides unit test coverage for the validations added into the WebHook.


Patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation.go b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
index 6ee01754..dbaf2f52 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
@@ -27,8 +27,8 @@ func (host *BareMetalHost) validateHost() []error {
 		}
 	}
 
-	if err := validateRAID(host.Spec.RAID); err != nil {
-		errs = append(errs, err)
+	if raid_errors := validateRAID(host.Spec.RAID); raid_errors != nil {
+		errs = append(errs, raid_errors...)
 	}
 
 	errs = append(errs, validateBMCAccess(host.Spec, bmcAccess)...)
@@ -91,16 +91,34 @@ func validateBMCAccess(s BareMetalHostSpec, bmcAccess bmc.AccessDetails) []error
 	return errs
 }
 
-func validateRAID(r *RAIDConfig) error {
+func validateRAID(r *RAIDConfig) []error {
+	var errors []error
+
 	if r == nil {
 		return nil
 	}
 
+	// check if both hardware and software RAID are specified
 	if len(r.HardwareRAIDVolumes) > 0 && len(r.SoftwareRAIDVolumes) > 0 {
-		return fmt.Errorf("hardwareRAIDVolumes and softwareRAIDVolumes can not be set at the same time")
+		errors = append(errors, fmt.Errorf("hardwareRAIDVolumes and softwareRAIDVolumes can not be set at the same time"))
 	}
 
-	return nil
+	for index, volume := range r.HardwareRAIDVolumes {
+		// check if physicalDisks are specified without a controller
+		if len(volume.PhysicalDisks) != 0 {
+			if volume.Controller == "" {
+				errors = append(errors, fmt.Errorf("'physicalDisks' specified without 'controller' in hardware RAID volume %d", index))
+			}
+		}
+		// check if numberOfPhysicalDisks is not same as len(physicalDisks)
+		if volume.NumberOfPhysicalDisks != nil && len(volume.PhysicalDisks) != 0 {
+			if *volume.NumberOfPhysicalDisks != len(volume.PhysicalDisks) {
+				errors = append(errors, fmt.Errorf("the 'numberOfPhysicalDisks'[%d] and number of 'physicalDisks'[%d] is not same for volume %d", *volume.NumberOfPhysicalDisks, len(volume.PhysicalDisks), index))
+			}
+		}
+	}
+
+	return errors
 }
 
 func validateBMHName(bmhname string) error {


Test patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
index 66dd34c0..26e4beff 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
@@ -1,6 +1,7 @@
 package v1alpha1
 
 import (
+	"fmt"
 	"testing"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -43,6 +44,9 @@ func TestValidateCreate(t *testing.T) {
 	}
 	enable := true
 
+	// for RAID validation test cases
+	numberOfPhysicalDisks := 3
+
 	tests := []struct {
 		name      string
 		newBMH    *BareMetalHost
@@ -275,6 +279,60 @@ func TestValidateCreate(t *testing.T) {
 			oldBMH:    nil,
 			wantedErr: "",
 		},
+		{
+			name: "'physicalDisks' in HardwareRAID without 'controller'.",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					BMC: BMCDetails{
+						Address:         "idrac://127.0.0.1",
+						CredentialsName: "test1",
+					},
+					RAID: &RAIDConfig{
+						HardwareRAIDVolumes: []HardwareRAIDVolume{
+							{
+								SizeGibibytes: nil,
+								Level:         "",
+								Name:          "",
+								Rotational:    nil,
+								PhysicalDisks: []string{"Disk-1", "Disk-2"},
+							}, // end of RAID volume
+						}, // end of RAID volume slice
+					}, // end of RAID config
+				}, // end of BMH spec
+			},
+			oldBMH:    nil,
+			wantedErr: "'physicalDisks' specified without 'controller' in hardware RAID volume 0",
+		},
+		{
+			name: "'numberOfPhysicalDisks' not same as length of 'physicalDisks'",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					BMC: BMCDetails{
+						Address:         "idrac://127.0.0.1",
+						CredentialsName: "test1",
+					},
+					RAID: &RAIDConfig{
+						HardwareRAIDVolumes: []HardwareRAIDVolume{
+							{
+								SizeGibibytes:         nil,
+								Level:                 "",
+								Name:                  "",
+								Rotational:            nil,
+								Controller:            "Controller-1",
+								PhysicalDisks:         []string{"Disk-1", "Disk-2"},
+								NumberOfPhysicalDisks: &numberOfPhysicalDisks, // defined as 3 above
+							}, // end of RAID volume
+						}, // end of RAID volume slice
+					}, // end of RAID config
+				}, // end of BMH spec
+			},
+			oldBMH:    nil,
+			wantedErr: fmt.Sprintf("the 'numberOfPhysicalDisks'[%d] and number of 'physicalDisks'[2] is not same for volume 0", numberOfPhysicalDisks),
+		},
 	}
 
 	for _, tt := range tests {