Repository: metal3-io/baremetal-operator
Pull request #633 (2020-09-15 12:10:22)

Add idrac-redfish url type (continue)
This is a rebased and gofmted #419 

Original PR description below

-------------------------------------------

The idrac BMCs have partiular handling of boot mode as configuration
change requests which need to be honored only at power state changes
as opposed to immediately, because this can conflict and over-write
existing configuration jobs.

While we shouldn't be in this state without more advanced bmc
features being leveraged, it is best to use the vendor supported
hardware type and interfaces for using redfish with their hardware.

This boot interface defaults to using iPXE, as opposed to virtual
media. A separate idrac specific redfish virtual media interface
already existed in the baremetal-operator.

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

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/633#" title="Author self-approved">honza</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **russellb**
You can assign the PR to them by writing `/assign @russellb` 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":["russellb"]} -->
metal3-io-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/633#pullrequestreview-488647782" title="Approved">dtantsur</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/633#" title="Author self-approved">honza</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **russellb**
You can assign the PR to them by writing `/assign @russellb` 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":["russellb"]} -->
dhellmann: /test-integration

dhellmann: Does this imply that the "redfish" BMC type isn't actually useful? Should we remove it?
dtantsur: The redfish one can be used for iLO and potentially other compliant vendor (supermicro seems to have at least some degree of compliance).
dhellmann: /approve
metal3-io-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/633#issuecomment-693630384" title="Approved">dhellmann</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/633#pullrequestreview-488647782" title="Approved">dtantsur</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/633#" title="Author self-approved">honza</a>*

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

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

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

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

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":[]} -->
dhellmann: /cc @bfournie 
dhellmann: I would like to freeze go code changes for a few days to try to land https://github.com/metal3-io/baremetal-operator/pull/650 without having to rebase it, because rebasing will mean redoing the work from scratch.

/hold

metal3-io-bot: @honza: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
generate | 51977772cfe7daa496bb98d91bd103ec731c89fe | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/633/generate/1309754772093407234) | `/test generate`



<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 -->
dhellmann: #655 has merged

/hold cancel

metal3-io-bot: @honza: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
manifestlint | e031f83ebf80a09b091d25c28b176bc665c4bd0d | [link](https://prow.apps.ci.metal3.io/view/gcs/metal3-prow-artifacts/pr-logs/pull/metal3-io_baremetal-operator/633/manifestlint/1338412447517642754) | `/test manifestlint`



<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: Issues go stale after 90d of inactivity.
Mark the issue as fresh with `/remove-lifecycle stale`.
Stale issues will close after an additional 30d of inactivity.

If this issue is safe to close now please do so with `/close`.

/lifecycle stale
dtantsur: /remove-lifecycle stale

Looks mostly good, although needs updating because of the secure boot changes. Could you rebase and fix the failing job?

Patch:
diff --git a/docs/api.md b/docs/api.md
index 1a677a3b..9be413c1 100644
--- a/docs/api.md
+++ b/docs/api.md
@@ -36,6 +36,10 @@ communicate with them.
   * `idrac://` (or `idrac+http://` to disable TLS).
   * `idrac-virtualmedia://` to use virtual media instead of PXE
     for attaching the provisioning image to the host.
+  * `idrac-redfish://` may be used to manage iDRAC controller with the
+    Redfish protocol over HTTPS. The URL must also contain a path to
+    the Redfish API system endpoint.
+    `idrac-redfish://myhost.example/redfish/v1/Systems/System.Embedded.1`
 * Fujitsu iRMC
   * `irmc://<host>:<port>`, where `<port>` is optional if using the default.
 * HUAWEI ibmc
diff --git a/pkg/bmc/redfish.go b/pkg/bmc/redfish.go
index ef3479d1..11ac9df5 100644
--- a/pkg/bmc/redfish.go
+++ b/pkg/bmc/redfish.go
@@ -9,6 +9,7 @@ func init() {
 	schemes := []string{"http", "https"}
 	RegisterFactory("redfish", newRedfishAccessDetails, schemes)
 	RegisterFactory("ilo5-redfish", newRedfishAccessDetails, schemes)
+	RegisterFactory("idrac-redfish", newRedfishiDracAccessDetails, schemes)
 }
 
 func redfishDetails(parsedURL *url.URL, disableCertificateVerification bool) *redfishAccessDetails {
@@ -24,6 +25,12 @@ func newRedfishAccessDetails(parsedURL *url.URL, disableCertificateVerification
 	return redfishDetails(parsedURL, disableCertificateVerification), nil
 }
 
+func newRedfishiDracAccessDetails(parsedURL *url.URL, disableCertificateVerification bool) (AccessDetails, error) {
+	return &redfishiDracAccessDetails{
+		*redfishDetails(parsedURL, disableCertificateVerification),
+	}, nil
+}
+
 type redfishAccessDetails struct {
 	bmcType                        string
 	host                           string
@@ -31,6 +38,10 @@ type redfishAccessDetails struct {
 	disableCertificateVerification bool
 }
 
+type redfishiDracAccessDetails struct {
+	redfishAccessDetails
+}
+
 const redfishDefaultScheme = "https"
 
 func (a *redfishAccessDetails) Type() string {
@@ -110,3 +121,29 @@ func (a *redfishAccessDetails) VendorInterface() string {
 func (a *redfishAccessDetails) SupportsSecureBoot() bool {
 	return true
 }
+
+// iDrac Redfish Overrides
+
+func (a *redfishiDracAccessDetails) Driver() string {
+	return "idrac"
+}
+
+func (a *redfishiDracAccessDetails) BootInterface() string {
+	return "ipxe"
+}
+
+func (a *redfishiDracAccessDetails) ManagementInterface() string {
+	return "idrac-redfish"
+}
+
+func (a *redfishiDracAccessDetails) PowerInterface() string {
+	return "idrac-redfish"
+}
+
+func (a *redfishiDracAccessDetails) RAIDInterface() string {
+	return "no-raid"
+}
+
+func (a *redfishiDracAccessDetails) VendorInterface() string {
+	return "no-vendor"
+}


Test patch:
diff --git a/pkg/bmc/access_test.go b/pkg/bmc/access_test.go
index 50075d16..d93f3078 100644
--- a/pkg/bmc/access_test.go
+++ b/pkg/bmc/access_test.go
@@ -538,6 +538,18 @@ func TestStaticDriverInfo(t *testing.T) {
 			vendor:     "",
 		},
 
+		{
+			Scenario:   "idrac redfish",
+			input:      "idrac-redfish://192.168.122.1",
+			needsMac:   true,
+			driver:     "idrac",
+			boot:       "ipxe",
+			management: "idrac-redfish",
+			power:      "idrac-redfish",
+			raid:       "no-raid",
+			vendor:     "no-vendor",
+		},
+
 		{
 			Scenario: "ilo5 virtual media",
 			input:    "ilo5-virtualmedia://192.168.122.1",
@@ -886,6 +898,18 @@ func TestDriverInfo(t *testing.T) {
 			},
 		},
 
+		{
+			Scenario: "idrac redfish",
+			input:    "idrac-redfish://192.168.122.1/foo/bar",
+			expects: map[string]interface{}{
+				"redfish_address":   "https://192.168.122.1",
+				"redfish_system_id": "/foo/bar",
+				"redfish_password":  "",
+				"redfish_username":  "",
+				"redfish_verify_ca": false,
+			},
+		},
+
 		{
 			Scenario: "idrac virtual media",
 			input:    "idrac-virtualmedia://192.168.122.1/foo/bar",