Repository: metal3-io/baremetal-operator
Pull request #1000 (2021-10-18 07:05:03)

Add BareMetalHost validation
This PR adds some BareMetalHost validations as issued in #855.

* Invalid BMC Config
* RAID requested when not supported by driver
* Missing Boot MAC when required by driver
* BIOS requested when not supported by driver
* Add validations for BootMode

metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **fmuyassarov** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @fmuyassarov` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["fmuyassarov"]} -->
metal3-io-bot: Hi @zhouhao3. Thanks for your PR.

I'm waiting for a [metal3-io](https://github.com/orgs/metal3-io/people) member to verify that this patch is reasonable to test. If it is, they should reply with `/ok-to-test` on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should [join the org](https://github.com/orgs/metal3-io/people) to skip this step.

Once the patch is verified, the new status will be reflected by the `ok-to-test` label.

I understand the commands that are listed [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

zhouhao3: Because this PR needs to be called [NewAccessDetails](https://github.com/metal3-io/baremetal-operator/blob/master/pkg/ironic/bmc/access.go#L126)  after the change, it is necessary to wait for #1007 to merge before making changes.

Hellcatlk: /test-v1b1-integration
metal3-io-bot: @zhouhao3: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
zhouhao3: Hi, Since 1017 has merged, I updated this PR, PTAL.
zhouhao3: @ardaguclu Thanks, all done, PTAL.
zhouhao3: /test-v1b1-centos-integration
/test-v1b1-integration
metal3-io-bot: @zhouhao3: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
unit | 34a4cb8db54d3e8099191ed5cba3517693c88f18 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/1000/unit/1468040561775611904) | true | `/test unit`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=1000). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:zhouhao3).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
zhouhao3: /retest unit
metal3-io-bot: @zhouhao3: The `/retest` command does not accept any targets.
The following commands are available to trigger required jobs:
* `/test generate`
* `/test gofmt`
* `/test golint`
* `/test gomod`
* `/test gosec`
* `/test manifestlint`
* `/test markdownlint`
* `/test shellcheck`
* `/test unit`

Use `/test all` to run the following jobs that were automatically triggered:
* `generate`
* `gofmt`
* `golint`
* `gomod`
* `gosec`
* `manifestlint`
* `unit`



<details>

In response to [this](https://github.com/metal3-io/baremetal-operator/pull/1000#issuecomment-987507250):

>/retest unit


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
zhouhao3: /test unit
zhouhao3: /test-v1b1-centos-integration
/test-v1b1-integration
zhouhao3: /test-v1b1-integration
/test-v1b1-centos-integration
ardaguclu: Thanks @zhouhao3. Code looks good to me, but validation rules need to be reviewed.
/ok-to-test
/test-v1b1-integration
/test-v1b1-centos-integration

cc: @dtantsur 
ardaguclu: Validations look sane too.
/lgtm
ardaguclu: /test-v1b1-integration
zhouhao3: /test-v1b1-integration
/test-v1b1-centos-integration
zhouhao3: @dtantsur PTAL
metal3-io-bot: New changes are detected. LGTM label has been removed.
metal3-io-bot: @zhouhao3: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
unit | 78a145ce592a6152fc9f3f72c0972c82c54a00e7 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/1000/unit/1470235700245827584) | true | `/test unit`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=1000). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:zhouhao3).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
zhouhao3: /test-v1b1-integration
/test-v1b1-centos-integration
metal3-io-bot: @zhouhao3: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
metal3-io-bot: @zhouhao3: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
unit | 9cde6114dc1a053f54884d840b146ec4252dfdba | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/1000/unit/1470712278021050368) | true | `/test unit`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=1000). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:zhouhao3).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
zhouhao3: /test-v1b1-integration
/test-v1b1-centos-integration
ardaguclu: /lgtm
zhouhao3: @andfasano PTAL
metal3-io-bot: New changes are detected. LGTM label has been removed.
zhouhao3: /test-v1b1-integration
/test-v1b1-centos-integration
kashifest: /hold
Holding for a while. Changing the default branch reference to `main` from `master`. Once the transition is done I will unhold it. 
kashifest: /hold cancel
The switch is done. BMO's default branch is now `main`
ardaguclu: /lgtm
zhouhao3: @andfasano PTAL
metal3-io-bot: New changes are detected. LGTM label has been removed.
metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **fmuyassarov** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @fmuyassarov` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=metal3-io%2Fbaremetal-operator).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/metal3-io/baremetal-operator/blob/main/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["fmuyassarov"]} -->
metal3-io-bot: @zhouhao3: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
unit | b9977979ec9c9035741766621f0c7b4bf9a93f8a | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/1000/unit/1478900677185900544) | true | `/test unit`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=1000). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:zhouhao3).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
metal3-io-bot: @zhouhao3: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
unit | d18f0dae44b48d10c72e45b88d23a59289816832 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/1000/unit/1478901362635837440) | true | `/test unit`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=1000). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:zhouhao3).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
metal3-io-bot: @zhouhao3: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
unit | e5b91302618aa39d0cca4e209bb8800595fdad09 | [link](https://prow.apps.test.metal3.io/view/s3/prow-logs/pr-logs/pull/metal3-io_baremetal-operator/1000/unit/1478903306578300928) | true | `/test unit`

[Full PR test history](https://prow.apps.test.metal3.io/pr-history?org=metal3-io&repo=baremetal-operator&pr=1000). [Your PR dashboard](https://prow.apps.test.metal3.io/pr?query=is:pr+state:open+author:zhouhao3).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
zhouhao3: /test-v1b1-integration
/test-v1b1-centos-integration
andfasano: /lgtm

Patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation.go b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
index f9843e39..6ee01754 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_validation.go
@@ -7,7 +7,7 @@ import (
 	"github.com/google/uuid"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 
-	_ "github.com/metal3-io/baremetal-operator/pkg/hardwareutils/bmc"
+	"github.com/metal3-io/baremetal-operator/pkg/hardwareutils/bmc"
 )
 
 // log is for logging in this package.
@@ -17,11 +17,22 @@ var log = logf.Log.WithName("baremetalhost-validation")
 func (host *BareMetalHost) validateHost() []error {
 	log.Info("validate create", "name", host.Name)
 	var errs []error
+	var bmcAccess bmc.AccessDetails
+
+	if host.Spec.BMC.Address != "" {
+		var err error
+		bmcAccess, err = bmc.NewAccessDetails(host.Spec.BMC.Address, host.Spec.BMC.DisableCertificateVerification)
+		if err != nil {
+			errs = append(errs, err)
+		}
+	}
 
 	if err := validateRAID(host.Spec.RAID); err != nil {
 		errs = append(errs, err)
 	}
 
+	errs = append(errs, validateBMCAccess(host.Spec, bmcAccess)...)
+
 	if err := validateBMHName(host.Name); err != nil {
 		errs = append(errs, err)
 	}
@@ -50,6 +61,36 @@ func (host *BareMetalHost) validateChanges(old *BareMetalHost) []error {
 	return errs
 }
 
+func validateBMCAccess(s BareMetalHostSpec, bmcAccess bmc.AccessDetails) []error {
+	var errs []error
+
+	if bmcAccess == nil {
+		return errs
+	}
+
+	if s.RAID != nil && len(s.RAID.HardwareRAIDVolumes) > 0 {
+		if bmcAccess.RAIDInterface() == "no-raid" {
+			errs = append(errs, fmt.Errorf("BMC driver %s does not support configuring RAID", bmcAccess.Type()))
+		}
+	}
+
+	if s.Firmware != nil {
+		if _, err := bmcAccess.BuildBIOSSettings((*bmc.FirmwareConfig)(s.Firmware)); err != nil {
+			errs = append(errs, err)
+		}
+	}
+
+	if bmcAccess.NeedsMAC() && s.BootMACAddress == "" {
+		errs = append(errs, fmt.Errorf("BMC driver %s requires a BootMACAddress value", bmcAccess.Type()))
+	}
+
+	if s.BootMode == UEFISecureBoot && !bmcAccess.SupportsSecureBoot() {
+		errs = append(errs, fmt.Errorf("BMC driver %s does not support secure boot", bmcAccess.Type()))
+	}
+
+	return errs
+}
+
 func validateRAID(r *RAIDConfig) error {
 	if r == nil {
 		return nil


Test patch:
diff --git a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
index e2d2aef5..66dd34c0 100644
--- a/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
+++ b/apis/metal3.io/v1alpha1/baremetalhost_validation_test.go
@@ -41,6 +41,7 @@ func TestValidateCreate(t *testing.T) {
 		Name:      "07564256-96ae-4315-ab03-8d34ece60fbb",
 		Namespace: "test-namespace",
 	}
+	enable := true
 
 	tests := []struct {
 		name      string
@@ -72,6 +73,11 @@ func TestValidateCreate(t *testing.T) {
 				TypeMeta:   tm,
 				ObjectMeta: om,
 				Spec: BareMetalHostSpec{
+					BootMACAddress: "01:02:03:04:05:06",
+					BMC: BMCDetails{
+						Address:         "irmc:127.0.1.1",
+						CredentialsName: "test1",
+					},
 					RAID: &RAIDConfig{
 						HardwareRAIDVolumes: []HardwareRAIDVolume{
 							{
@@ -93,6 +99,182 @@ func TestValidateCreate(t *testing.T) {
 			oldBMH:    nil,
 			wantedErr: "hardwareRAIDVolumes and softwareRAIDVolumes can not be set at the same time",
 		},
+		{
+			name: "supportBMCType",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					BootMACAddress: "01:02:03:04:05:06",
+					BMC: BMCDetails{
+						Address:         "irmc:127.0.1.1",
+						CredentialsName: "test1",
+					},
+				}},
+			oldBMH:    nil,
+			wantedErr: "",
+		},
+		{
+			name: "unsupportBMCType",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					BMC: BMCDetails{
+						Address:         "test:127.0.1.1",
+						CredentialsName: "test1",
+					},
+				}},
+			oldBMH:    nil,
+			wantedErr: "Unknown BMC type 'test' for address test:127.0.1.1",
+		},
+		{
+			name: "RAIDWithSupportBMC",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					RAID: &RAIDConfig{
+						HardwareRAIDVolumes: []HardwareRAIDVolume{
+							{
+								SizeGibibytes:         nil,
+								Level:                 "",
+								Name:                  "",
+								Rotational:            nil,
+								NumberOfPhysicalDisks: nil,
+							},
+						},
+					},
+					BootMACAddress: "01:02:03:04:05:06",
+					BMC: BMCDetails{
+						Address:         "irmc://127.0.1.1",
+						CredentialsName: "test1",
+					},
+				}},
+			oldBMH:    nil,
+			wantedErr: "",
+		},
+		{
+			name: "RAIDWithUnsupportBMC",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					RAID: &RAIDConfig{
+						HardwareRAIDVolumes: []HardwareRAIDVolume{
+							{
+								SizeGibibytes:         nil,
+								Level:                 "",
+								Name:                  "",
+								Rotational:            nil,
+								NumberOfPhysicalDisks: nil,
+							},
+						},
+					},
+					BMC: BMCDetails{
+						Address:         "ipmi://127.0.1.1",
+						CredentialsName: "test1",
+					},
+				}},
+			oldBMH:    nil,
+			wantedErr: "BMC driver ipmi does not support configuring RAID",
+		},
+		{
+			name: "FirmwareWithSupportBMC",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					Firmware: &FirmwareConfig{
+						VirtualizationEnabled: &enable,
+					},
+					BootMACAddress: "01:02:03:04:05:06",
+					BMC: BMCDetails{
+						Address:         "irmc://127.0.1.1",
+						CredentialsName: "test1",
+					},
+				}},
+			oldBMH:    nil,
+			wantedErr: "",
+		},
+		{
+			name: "FirmwareWithUnsupportBMC",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					Firmware: &FirmwareConfig{
+						VirtualizationEnabled: &enable,
+					},
+					BMC: BMCDetails{
+						Address:         "ipmi://127.0.1.1",
+						CredentialsName: "test1",
+					},
+				}},
+			oldBMH:    nil,
+			wantedErr: "firmware settings for ipmi are not supported",
+		},
+		{
+			name: "BootMACAddressRequiredWithoutBootMACAddress",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					BMC: BMCDetails{
+						Address:         "libvirt://127.0.1.1",
+						CredentialsName: "test1",
+					},
+				}},
+			oldBMH:    nil,
+			wantedErr: "BMC driver libvirt requires a BootMACAddress value",
+		},
+		{
+			name: "BootMACAddressRequiredWithBootMACAddress",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					BMC: BMCDetails{
+						Address:         "libvirt://127.0.1.1",
+						CredentialsName: "test1",
+					},
+					BootMACAddress: "00:00:00:00:00:00",
+				}},
+			oldBMH:    nil,
+			wantedErr: "",
+		},
+		{
+			name: "BootMACAddressRequired",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					BMC: BMCDetails{
+						Address:         "libvirt://127.0.1.1",
+						CredentialsName: "test1",
+					},
+					BootMACAddress: "00:00:00:00:00:00",
+					BootMode:       UEFISecureBoot,
+				}},
+			oldBMH:    nil,
+			wantedErr: "BMC driver libvirt does not support secure boot",
+		},
+		{
+			name: "UEFISecureBootWithSupportBMC",
+			newBMH: &BareMetalHost{
+				TypeMeta:   tm,
+				ObjectMeta: om,
+				Spec: BareMetalHostSpec{
+					BMC: BMCDetails{
+						Address:         "irmc://127.0.1.1",
+						CredentialsName: "test1",
+					},
+					BootMACAddress: "00:00:00:00:00:00",
+					BootMode:       UEFISecureBoot,
+				}},
+			oldBMH:    nil,
+			wantedErr: "",
+		},
 	}
 
 	for _, tt := range tests {