Repository: metal3-io/baremetal-operator
Pull request #853 (2021-04-13 13:27:14)

baremetal: Add privLevel to bmc data
Add a new parameter to optionaly set the ipmi privilage
level used when connected to BMC's over ipmi, the default
"ADMINISTRATOR" isn't always available (e.g. in ibmcloud).

Currently works on bootstrap node but not in the metal3 pod, so I'm missing something
tested with
openshift/installer#4845

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/853#" title="Author self-approved">derekhiggins</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/853#pullrequestreview-639917933" title="Approved">dtantsur</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)~~ [dtantsur]

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":[]} -->
dtantsur: /test-integration
zaneb: > In ironic it is set on a per-host basis, we could have BMO set it per host based on a global setting but some day somebody would need it to be per-host.

TBH I am personally still unconvinced by this, for a couple of reasons:
1. There exist environments where *every* host will need some custom value set, so it makes sense to allow setting a default value globally.
2. YAGNI

I think what would convince me would be if, in an environment where this is needed on every host, there is some reason that it is easier to set this at the time of creating each BareMetalHost than at the time of deploying the bmo.
dtantsur: > There exist environments where every host will need some custom value set

There exist environments where every host have the same driver (likely most of them) and even the same credentials (root/calvin anyone?).

IPMI parameters may be different in multi-vendor environments, I can easily image that one hardware model *requires* OPERATOR, while the other *requires* ADMINISTRATOR.
stbenjam: I really dislike the idea of configurable global defaults for something that's BMC-specific. We are forever going to be encountering vendor-specific configuration that we need to customize for difference BMC types.  Do we add global flags for all of those kinds of things?

I really prefer the concept of using URL parameters to do these kinds of things in BMO (something like `ipmi://192.168.1.1/?privilegeLevel=operator`)...


dtantsur: @derekhiggins would it be possible to unit test the new addition?
metal3-io-bot: @derekhiggins: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
gofmt | 69f704b69d503a20db2bbd729803c03a3807d89b | [link](https://prow.apps.test.metal3.io/view/gs/metal3-prow/pr-logs/pull/metal3-io_baremetal-operator/853/gofmt/1389603869180301312) | `/test gofmt`

<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 -->
derekhiggins: > @derekhiggins would it be possible to unit test the new addition?

no prob, done
metal3-io-bot: @derekhiggins: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
gofmt | 69f704b69d503a20db2bbd729803c03a3807d89b | [link](https://prow.apps.test.metal3.io/view/gs/metal3-prow/pr-logs/pull/metal3-io_baremetal-operator/853/gofmt/1389603869180301312) | `/test gofmt`
golint | 69f704b69d503a20db2bbd729803c03a3807d89b | [link](https://prow.apps.test.metal3.io/view/gs/metal3-prow/pr-logs/pull/metal3-io_baremetal-operator/853/golint/1389603869478096896) | `/test golint`

<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: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/metal3-io/baremetal-operator/pull/853#pullrequestreview-639917933" title="Approved">dtantsur</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)~~ [dtantsur]

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":[]} -->
hardys: This lgtm - @zaneb you had concerns in a previous comment, are you OK to go ahead with this optional-query-string version, and if it proves necessary we could add some kind of configurable default in future?
zaneb: Haven't reviewed the patch, but after reading the arguments I am cool with this approach.
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/853#pullrequestreview-639917933" title="Approved">dtantsur</a>*, *<a href="https://github.com/metal3-io/baremetal-operator/pull/853#pullrequestreview-691532183" title="Approved">elfosardo</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)~~ [dtantsur]

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":[]} -->

Patch:
diff --git a/docs/api.md b/docs/api.md
index a5c8bb4e..d18e2c4f 100644
--- a/docs/api.md
+++ b/docs/api.md
@@ -32,6 +32,9 @@ communicate with them.
 * IPMI
   * `ipmi://<host>:<port>`, an unadorned `<host>:<port>` is also accepted
     and the port is optional, if using the default one (623).
+  * The ipmi privilege level can be set from the default(`ADMINISTRATOR`)
+    to `OPERATOR` with an option URL parameter `privilegelevel`.
+    `ipmi://<host>:<port>?privilegelevel=OPERATOR`
 * Dell iDRAC
   * `idrac://` (or `idrac+http://` to disable TLS).
   * `idrac-virtualmedia://` to use virtual media instead of PXE
diff --git a/pkg/bmc/ipmi.go b/pkg/bmc/ipmi.go
index ad8ce1e7..98c283a8 100644
--- a/pkg/bmc/ipmi.go
+++ b/pkg/bmc/ipmi.go
@@ -12,11 +12,24 @@ func init() {
 	RegisterFactory("libvirt", newIPMIAccessDetails, []string{})
 }
 
+func getPrivilegeLevel(rawquery string) string {
+	privilegelevel := "ADMINISTRATOR"
+	q, err := url.ParseQuery(rawquery)
+	if err != nil {
+		return privilegelevel
+	}
+	if val, ok := q["privilegelevel"]; ok {
+		return val[0]
+	}
+	return privilegelevel
+}
+
 func newIPMIAccessDetails(parsedURL *url.URL, disableCertificateVerification bool) (AccessDetails, error) {
 	return &ipmiAccessDetails{
 		bmcType:                        parsedURL.Scheme,
 		portNum:                        parsedURL.Port(),
 		hostname:                       parsedURL.Hostname(),
+		privilegelevel:                 getPrivilegeLevel(parsedURL.RawQuery),
 		disableCertificateVerification: disableCertificateVerification,
 	}, nil
 }
@@ -25,6 +38,7 @@ type ipmiAccessDetails struct {
 	bmcType                        string
 	portNum                        string
 	hostname                       string
+	privilegelevel                 string
 	disableCertificateVerification bool
 }
 
@@ -61,10 +75,11 @@ func (a *ipmiAccessDetails) DisableCertificateVerification() bool {
 // the kernel and ramdisk locations).
 func (a *ipmiAccessDetails) DriverInfo(bmcCreds Credentials) map[string]interface{} {
 	result := map[string]interface{}{
-		"ipmi_port":     a.portNum,
-		"ipmi_username": bmcCreds.Username,
-		"ipmi_password": bmcCreds.Password,
-		"ipmi_address":  a.hostname,
+		"ipmi_port":       a.portNum,
+		"ipmi_username":   bmcCreds.Username,
+		"ipmi_password":   bmcCreds.Password,
+		"ipmi_address":    a.hostname,
+		"ipmi_priv_level": a.privilegelevel,
 	}
 
 	if a.disableCertificateVerification {


Test patch:
diff --git a/pkg/bmc/access_test.go b/pkg/bmc/access_test.go
index 2cc06102..517b2cca 100644
--- a/pkg/bmc/access_test.go
+++ b/pkg/bmc/access_test.go
@@ -678,11 +678,25 @@ func TestDriverInfo(t *testing.T) {
 			Scenario: "ipmi default port",
 			input:    "ipmi://192.168.122.1",
 			expects: map[string]interface{}{
-				"ipmi_port":      ipmiDefaultPort,
-				"ipmi_password":  "",
-				"ipmi_username":  "",
-				"ipmi_address":   "192.168.122.1",
-				"ipmi_verify_ca": false,
+				"ipmi_port":       ipmiDefaultPort,
+				"ipmi_password":   "",
+				"ipmi_username":   "",
+				"ipmi_address":    "192.168.122.1",
+				"ipmi_verify_ca":  false,
+				"ipmi_priv_level": "ADMINISTRATOR",
+			},
+		},
+
+		{
+			Scenario: "ipmi setting privilege level",
+			input:    "ipmi://192.168.122.1?privilegelevel=OPERATOR",
+			expects: map[string]interface{}{
+				"ipmi_port":       ipmiDefaultPort,
+				"ipmi_password":   "",
+				"ipmi_username":   "",
+				"ipmi_address":    "192.168.122.1",
+				"ipmi_verify_ca":  false,
+				"ipmi_priv_level": "OPERATOR",
 			},
 		},