Add per provider config: #326

jacobweinstock · 2023-04-30T04:15:29Z

What does this PR implement/change/remove?

There exists some config that cannot be shared across all providers. For example, the port for ipmitool is generally 623 but is generally 16992 for intelAMT, cipher suites for ipmitool, or TLS certificates. Allowing per-provider config enables customizing this type of data.

This introduces a * breaking change * to bmclib.NewClient. It no longer takes in a port. The ports for each provider have a default defined and each one can be overridden with a functional option. WithIpmitoolPort, for example.

I think fundamentally the behavior from the consumer side doesn't change. Consumers will still use With functions to specify per provider config. This change just organizes it differently under the hood.

Checklist

Tests added
Similar commits squashed

The HW vendor this change applies to (if applicable)

The HW model number, product name this change applies to (if applicable)

The BMC firmware and/or BIOS versions that this change applies to (if applicable)

What version of tooling - vendor specific or opensource does this change depend on (if applicable)

Description for changelog/release notes

codecov · 2023-04-30T04:17:09Z

Codecov Report

Patch coverage: 44.82% and project coverage change: -0.41 ⚠️

Comparison is base (00e7ccb) 44.55% compared to head (46acd6e) 44.14%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #326      +/-   ##
==========================================
- Coverage   44.55%   44.14%   -0.41%     
==========================================
  Files          36       37       +1     
  Lines        2790     2904     +114     
==========================================
+ Hits         1243     1282      +39     
- Misses       1422     1494      +72     
- Partials      125      128       +3

Impacted Files	Coverage Δ
bmc/reset.go	`94.59% <ø> (ø)`
internal/redfishwrapper/client.go	`22.22% <0.00%> (ø)`
providers/ipmitool/ipmitool.go	`0.00% <0.00%> (ø)`
option.go	`23.91% <23.91%> (ø)`
providers/intelamt/intelamt.go	`84.05% <50.00%> (-15.95%)`	⬇️
providers/redfish/redfish.go	`25.55% <50.00%> (+8.36%)`	⬆️
providers/asrockrack/asrockrack.go	`44.89% <71.42%> (ø)`
client.go	`55.88% <90.19%> (+5.88%)`	⬆️
internal/httpclient/httpclient.go	`88.57% <100.00%> (+4.78%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

joelrebel

Thanks for the efforts here, left a question/comment inline..

joelrebel · 2023-05-01T09:06:43Z

client.go

-		redfishVersionsNotCompatible: []string{},
+		Logger:   logr.Discard(),
+		Registry: registrar.NewRegistry(),
+		providerConfig: providerConfig{


Is it possible to have the providerConfig (or have Options) type be exported as a struct/interface by each provider - so the provider specific configuration and http client checks can stay within the respective packages under providers/{asrockrack,redfish..}/ ?

This would keep the client package free from provider specifics.

Hey @joelrebel , thanks for looking at this. I've updated to have all providers define their own config struct and then client.go just collects them into a single struct. Is this more of what you had in mind? Also, this PR is still very much a WIP.

Will take a look thanks!, apologies I missed the WIP/draft label on the PR.

Hey @joelrebel, this PR should be ready for a proper review whenever you have cycles. Thanks!

There exists some config that cannot be shared across all providers. For example, the port for ipmitool is generally 623, but is generally 16992 for intelAMT. Allowing per provider config enables customizing this type of data. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

This allows the provider to specify all config it needs. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

This makes New functions cleaner. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Add functional arguments for optional Ipmi struct values. Add additional struct values. Make cipher suite 3 the first suite tried. 3 is more commonly used then 17 so most machines will not try with 17 first. Add debug logging of ipmitool command line options. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

The new function signature does not return an error. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

This uniforms providers to using func args. Updates the client to work with this. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Fix basic auth test and example. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

The registering of ipmitool provider doesnt ignore the error when its constructor is called. This means that systems without ipmitool will see that the ipmitool provider does not get registered. Previously it would always be registered. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Allow for a 10 millisecond error of margin. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Previously it was not. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

This makes it more flexible and follows the pattern the other providers are using. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

joelrebel

Thanks for all the fixes and changes here 👍 This does make passing parameters to individual providers easier and succinct.
The README requires some updating based on these changes, apart from that I've left a few comments, nothing major.

internal/ipmi/ipmi.go

option.go

client.go

gofish is an implementation detail that shouldnt be exposed like this. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Clean up. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Extra precaution for race conditions. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

jacobweinstock · 2023-05-10T15:55:51Z

Thanks for all the fixes and changes here 👍 This does make passing parameters to individual providers easier and succinct. The README requires some updating based on these changes, apart from that I've left a few comments, nothing major.

Thanks, @joelrebel. Could you point me to the needed changes in the README.md, please? I'm not finding anything specific.

joelrebel · 2023-05-10T16:49:53Z

Thanks, @joelrebel. Could you point me to the needed changes in the README.md, please? I'm not finding anything specific.

sure, in the README we have examples with bmclib.NewClient() that currently accept a port as a parameter, which is not required anymore right?

Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

jacobweinstock · 2023-05-10T17:08:45Z

Thanks, @joelrebel. Could you point me to the needed changes in the README.md, please? I'm not finding anything specific.

sure, in the README we have examples with bmclib.NewClient() that currently accept a port as a parameter, which is not required anymore right?

Ahh, yes. thank you. I have updated the readme.

`-race` flag was added to `make test` and requires CGO. go: -race requires cgo; enable cgo by setting CGO_ENABLED=1 Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

joelrebel

Thanks for the work here @jacobweinstock

joelrebel reviewed May 1, 2023

View reviewed changes

jacobweinstock mentioned this pull request May 2, 2023

Question: Is there any plan to support to set a specific cipher suite for ipmi? tinkerbell/rufio#89

Closed

jacobweinstock force-pushed the per-provider-config branch 2 times, most recently from 0fa5f77 to 60a606a Compare May 5, 2023 05:41

jacobweinstock requested a review from joelrebel May 5, 2023 05:42

jacobweinstock marked this pull request as ready for review May 5, 2023 06:37

jacobweinstock force-pushed the per-provider-config branch from 1143a51 to b8708bd Compare May 5, 2023 15:54

jacobweinstock added breaking-change bmclib v2 labels May 5, 2023

jacobweinstock force-pushed the per-provider-config branch from bb15d15 to 4a7719f Compare May 5, 2023 18:28

jacobweinstock added 16 commits May 8, 2023 09:49

Refactor to use provider specific config structs:

6bb0b1d

This allows the provider to specify all config it needs. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Updated go.mod

f9f861f

Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Move all option related code to option.go

a6d27a5

Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Remove error return from default http.Client constructor:

02f085e

This makes New functions cleaner. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Update default http.Client constructor:

fe4dfaa

The new function signature does not return an error. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Add functional arguments and options:

fbec04f

This uniforms providers to using func args. Updates the client to work with this. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Fix merge conflicts:

cfa88bf

Fix basic auth test and example. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Fix defaultTimeout test:

47f65c1

Allow for a 10 millisecond error of margin. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Make gofish port customizable:

f0e83f8

Previously it was not. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Use gofish opts for port instead of a func parameter:

b52ee97

This makes it more flexible and follows the pattern the other providers are using. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Make asrockrack port configurable.

a22508e

Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Add missing function back after main merge.

7dd578a

Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Main merge clean up

d91f2e7

Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

jacobweinstock force-pushed the per-provider-config branch from 98b0635 to d91f2e7 Compare May 8, 2023 15:51

This was referenced May 8, 2023

Rufio Machine port is ignored tinkerbell/rufio#103

Open

Tinkerbell provider Regression in older hardwares aws/eks-anywhere#5682

Closed

jacobweinstock mentioned this pull request May 9, 2023

Update bmclib to latest version tinkerbell/rufio#105

Closed

joelrebel reviewed May 10, 2023

View reviewed changes

internal/ipmi/ipmi.go Outdated Show resolved Hide resolved

internal/ipmi/ipmi.go Show resolved Hide resolved

option.go Outdated Show resolved Hide resolved

client.go Show resolved Hide resolved

jacobweinstock added 3 commits May 10, 2023 09:31

Rename gofish options to redfish:

054fde6

gofish is an implementation detail that shouldnt be exposed like this. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Remove debug print line, run go fmt:

d1528bf

Clean up. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Clone the http.Client transport for each provider:

532dade

Extra precaution for race conditions. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

Update README to reflect function signature change

9e2d7b3

Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

jacobweinstock force-pushed the per-provider-config branch from cfd8410 to 60a0cde Compare May 10, 2023 17:09

Enable CGO:

46acd6e

`-race` flag was added to `make test` and requires CGO. go: -race requires cgo; enable cgo by setting CGO_ENABLED=1 Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>

jacobweinstock force-pushed the per-provider-config branch from 60a0cde to 46acd6e Compare May 10, 2023 17:11

jacobweinstock requested a review from joelrebel May 10, 2023 17:27

joelrebel approved these changes May 11, 2023

View reviewed changes

jacobweinstock added the ready-to-merge label May 11, 2023

mergify bot merged commit 5f3cb35 into bmc-toolbox:main May 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add per provider config: #326

Add per provider config: #326

jacobweinstock commented Apr 30, 2023 •

edited

Loading

codecov bot commented Apr 30, 2023 •

edited

Loading

joelrebel left a comment

joelrebel May 1, 2023 •

edited

Loading

jacobweinstock May 4, 2023

joelrebel May 5, 2023

jacobweinstock May 8, 2023 •

edited

Loading

joelrebel left a comment •

edited

Loading

jacobweinstock commented May 10, 2023

joelrebel commented May 10, 2023

jacobweinstock commented May 10, 2023

joelrebel left a comment

Add per provider config: #326

Add per provider config: #326

Conversation

jacobweinstock commented Apr 30, 2023 • edited Loading

What does this PR implement/change/remove?

Checklist

The HW vendor this change applies to (if applicable)

The HW model number, product name this change applies to (if applicable)

The BMC firmware and/or BIOS versions that this change applies to (if applicable)

What version of tooling - vendor specific or opensource does this change depend on (if applicable)

Description for changelog/release notes

codecov bot commented Apr 30, 2023 • edited Loading

Codecov Report

joelrebel left a comment

Choose a reason for hiding this comment

joelrebel May 1, 2023 • edited Loading

Choose a reason for hiding this comment

jacobweinstock May 4, 2023

Choose a reason for hiding this comment

joelrebel May 5, 2023

Choose a reason for hiding this comment

jacobweinstock May 8, 2023 • edited Loading

Choose a reason for hiding this comment

joelrebel left a comment • edited Loading

Choose a reason for hiding this comment

jacobweinstock commented May 10, 2023

joelrebel commented May 10, 2023

jacobweinstock commented May 10, 2023

joelrebel left a comment

Choose a reason for hiding this comment

jacobweinstock commented Apr 30, 2023 •

edited

Loading

codecov bot commented Apr 30, 2023 •

edited

Loading

joelrebel May 1, 2023 •

edited

Loading

jacobweinstock May 8, 2023 •

edited

Loading

joelrebel left a comment •

edited

Loading