Handle errors so that we don't get nil pointer panics:

Ignoring an error and returning nil objects causes
`panic: runtime error: invalid memory address or nil pointer
dereference` issues.

Signed-off-by: Jacob Weinstock <[email protected]>

providers/supermicro/firmware_bios_test.go

@@ -80,7 +80,7 @@ func Test_setComponentUpdateMisc(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			serviceClient, err := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build())
+			serviceClient := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build())
 			assert.Nil(t, err)
 
 			serviceClient.csrfToken = "foobar"
@@ -171,7 +171,7 @@ func Test_setBIOSFirmwareInstallMode(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			serviceClient, err := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build())
+			serviceClient := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build())
 			assert.Nil(t, err)
 
 			serviceClient.csrfToken = "foobar"

providers/supermicro/supermicro.go

@@ -136,21 +136,14 @@ func NewClient(host, user, pass string, log logr.Logger, opts ...Option) *Client
 		opt(defaultConfig)
 	}
 
-	serviceClient, err := newBmcServiceClient(
+	serviceClient := newBmcServiceClient(
 		host,
 		defaultConfig.Port,
 		user,
 		pass,
 		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
-	}
-
 	return &Client{
 		serviceClient: serviceClient,
 		log:           log,
@@ -450,20 +443,27 @@ type serviceClient struct {
 	sum       *sum.Sum
 }
 
-func newBmcServiceClient(host, port, user, pass string, client *http.Client) (*serviceClient, error) {
+func newBmcServiceClient(host, port, user, pass string, client *http.Client) *serviceClient {
 	sc := &serviceClient{host: host, port: port, user: user, pass: pass, client: client}
 
 	if !strings.HasPrefix(host, "https://") && !strings.HasPrefix(host, "http://") {
 		sc.host = "https://" + host
 	}
 
+	// sum is only for firmware related operations. Failing the client entirely because of a sum error
+	// means all the other functionality is not available. I don't think that is what we want. So, instead
+	// of failing the function will just set sc.sum to nil. There are checks in place in this package to
+	// handle sc.sum being nil. The tradeoff here is that the reason that sum failed, which from the current
+	// code is only if the `sum` binary is not found, is not returned to the caller. So if firmware operations
+	// are not working, binary not found is the reason.
 	s, err := sum.New(host, user, pass)
 	if err != nil {
-		return nil, err
+		sc.sum = nil
+	} else {
+		sc.sum = s
 	}
-	sc.sum = s
 
-	return sc, nil
+	return sc
 }
 
 func (c *serviceClient) setCsrfToken(t string) {

providers/supermicro/x11_firmware_bmc_test.go

@@ -91,7 +91,7 @@ func TestX11SetBMCFirmwareInstallMode(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			serviceClient, err := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build())
+			serviceClient := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build())
 			assert.Nil(t, err)
 
 			client := &x11{serviceClient: serviceClient, log: logr.Discard()}
@@ -186,7 +186,7 @@ func TestX11UploadBMCFirmware(t *testing.T) {
 				defer os.Remove(binPath)
 			}
 
-			serviceClient, err := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build())
+			serviceClient := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build())
 			assert.Nil(t, err)
 			serviceClient.csrfToken = "foobar"
 			client := &x11{serviceClient: serviceClient, log: logr.Discard()}
@@ -268,7 +268,7 @@ func TestX11VerifyBMCFirmwareVersion(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			serviceClient, err := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build())
+			serviceClient := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build())
 			assert.Nil(t, err)
 			serviceClient.csrfToken = "foobar"
 			client := &x11{serviceClient: serviceClient, log: logr.Discard()}
@@ -350,7 +350,7 @@ func TestX11InitiateBMCFirmwareInstall(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			serviceClient, err := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build())
+			serviceClient := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build())
 			assert.Nil(t, err)
 			serviceClient.csrfToken = "foobar"
 			client := &x11{serviceClient: serviceClient, log: logr.Discard()}
@@ -514,7 +514,7 @@ func TestX11StatusBMCFirmwareInstall(t *testing.T) {
 				t.Fatal(err)
 			}
 
-			serviceClient, err := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build())
+			serviceClient := newBmcServiceClient(parsedURL.Hostname(), parsedURL.Port(), "foo", "bar", httpclient.Build())
 			assert.Nil(t, err)
 
 			serviceClient.csrfToken = "foobar"