From 43237b10406cf1712a8699dedfcc5d4c4d7c0b79 Mon Sep 17 00:00:00 2001 From: "James W. Brinkerhoff" Date: Mon, 28 Oct 2024 14:55:25 -0400 Subject: [PATCH 1/3] providers/supermicro/supermicro.go: Initialize sum 'client' during serviceclient init --- providers/supermicro/supermicro.go | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/providers/supermicro/supermicro.go b/providers/supermicro/supermicro.go index e7d224f0..7d6b44df 100644 --- a/providers/supermicro/supermicro.go +++ b/providers/supermicro/supermicro.go @@ -136,7 +136,7 @@ func NewClient(host, user, pass string, log logr.Logger, opts ...Option) *Client opt(defaultConfig) } - serviceClient := newBmcServiceClient( + serviceClient, err := newBmcServiceClient( host, defaultConfig.Port, user, @@ -144,6 +144,10 @@ func NewClient(host, user, pass string, log logr.Logger, opts ...Option) *Client httpclient.Build(defaultConfig.httpClientSetupFuncs...), ) + if err != nil { + return nil + } + return &Client{ serviceClient: serviceClient, log: log, @@ -443,12 +447,20 @@ type serviceClient struct { sum *sum.Sum } -func newBmcServiceClient(host, port, user, pass string, client *http.Client) *serviceClient { +func newBmcServiceClient(host, port, user, pass string, client *http.Client) (*serviceClient, error) { + sc := &serviceClient{host: host, port: port, user: user, pass: pass, client: client} + if !strings.HasPrefix(host, "https://") && !strings.HasPrefix(host, "http://") { - host = "https://" + host + sc.host = "https://" + host + } + + s, err := sum.New(host, user, pass) + if err != nil { + return nil, err } + sc.sum = s - return &serviceClient{host: host, port: port, user: user, pass: pass, client: client} + return sc, nil } func (c *serviceClient) setCsrfToken(t string) { From 9705c0421cb74b4ae8a492ffa39b69bb2908073d Mon Sep 17 00:00:00 2001 From: "James W. Brinkerhoff" Date: Mon, 28 Oct 2024 15:02:00 -0400 Subject: [PATCH 2/3] Update tests to validate new error return is nil --- providers/supermicro/firmware_bios_test.go | 8 ++++++-- providers/supermicro/x11_firmware_bmc_test.go | 17 ++++++++++++----- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/providers/supermicro/firmware_bios_test.go b/providers/supermicro/firmware_bios_test.go index f4bdfb53..dfe6d531 100644 --- a/providers/supermicro/firmware_bios_test.go +++ b/providers/supermicro/firmware_bios_test.go @@ -80,7 +80,9 @@ func Test_setComponentUpdateMisc(t *testing.T) { t.Fatal(err) } - serviceClient := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build()) + serviceClient, err := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build()) + assert.Nil(t, err) + serviceClient.csrfToken = "foobar" client := &x11{serviceClient: serviceClient, log: logr.Discard()} @@ -169,7 +171,9 @@ func Test_setBIOSFirmwareInstallMode(t *testing.T) { t.Fatal(err) } - serviceClient := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build()) + serviceClient, err := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build()) + assert.Nil(t, err) + serviceClient.csrfToken = "foobar" client := &x11{serviceClient: serviceClient, log: logr.Discard()} diff --git a/providers/supermicro/x11_firmware_bmc_test.go b/providers/supermicro/x11_firmware_bmc_test.go index 9c7024f5..b20e0c81 100644 --- a/providers/supermicro/x11_firmware_bmc_test.go +++ b/providers/supermicro/x11_firmware_bmc_test.go @@ -91,7 +91,9 @@ func TestX11SetBMCFirmwareInstallMode(t *testing.T) { t.Fatal(err) } - serviceClient := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build()) + serviceClient, err := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build()) + assert.Nil(t, err) + client := &x11{serviceClient: serviceClient, log: logr.Discard()} if err := client.setBMCFirmwareInstallMode(context.Background()); err != nil { @@ -184,7 +186,8 @@ func TestX11UploadBMCFirmware(t *testing.T) { defer os.Remove(binPath) } - serviceClient := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build()) + serviceClient, err := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build()) + assert.Nil(t, err) serviceClient.csrfToken = "foobar" client := &x11{serviceClient: serviceClient, log: logr.Discard()} @@ -265,7 +268,8 @@ func TestX11VerifyBMCFirmwareVersion(t *testing.T) { t.Fatal(err) } - serviceClient := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build()) + serviceClient, err := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build()) + assert.Nil(t, err) serviceClient.csrfToken = "foobar" client := &x11{serviceClient: serviceClient, log: logr.Discard()} @@ -346,7 +350,8 @@ func TestX11InitiateBMCFirmwareInstall(t *testing.T) { t.Fatal(err) } - serviceClient := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build()) + serviceClient, err := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build()) + assert.Nil(t, err) serviceClient.csrfToken = "foobar" client := &x11{serviceClient: serviceClient, log: logr.Discard()} @@ -509,7 +514,9 @@ func TestX11StatusBMCFirmwareInstall(t *testing.T) { t.Fatal(err) } - serviceClient := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build()) + serviceClient, err := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build()) + assert.Nil(t, err) + serviceClient.csrfToken = "foobar" client := &x11{serviceClient: serviceClient, log: logr.Discard()} From 0a6db1fa55b8e8303e0f7a93588e0bb398685b2b Mon Sep 17 00:00:00 2001 From: "James W. Brinkerhoff" Date: Tue, 29 Oct 2024 10:06:38 -0400 Subject: [PATCH 3/3] providers/supermicro/supermicro.go: Explain why we return nil in NewClient() --- providers/supermicro/supermicro.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/providers/supermicro/supermicro.go b/providers/supermicro/supermicro.go index 7d6b44df..6fc9b50a 100644 --- a/providers/supermicro/supermicro.go +++ b/providers/supermicro/supermicro.go @@ -144,6 +144,9 @@ func NewClient(host, user, pass string, log logr.Logger, opts ...Option) *Client httpclient.Build(defaultConfig.httpClientSetupFuncs...), ) + // We probably want to treat this as a fatal error and/or pass the error back to the caller + // I did not want to chase that thread atm, so we intentionally return nil here if + // newBmcServiceClient returns an error. if err != nil { return nil }