From 005dc5c38ec2f009ffa154b125f93f0feba21da4 Mon Sep 17 00:00:00 2001 From: Jacob Weinstock Date: Fri, 13 Dec 2024 17:27:03 -0700 Subject: [PATCH 1/2] 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 --- providers/supermicro/firmware_bios_test.go | 4 ++-- providers/supermicro/supermicro.go | 24 +++++++++---------- providers/supermicro/x11_firmware_bmc_test.go | 10 ++++---- 3 files changed, 19 insertions(+), 19 deletions(-) diff --git a/providers/supermicro/firmware_bios_test.go b/providers/supermicro/firmware_bios_test.go index dfe6d531..3ab4a128 100644 --- a/providers/supermicro/firmware_bios_test.go +++ b/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" diff --git a/providers/supermicro/supermicro.go b/providers/supermicro/supermicro.go index 1259642d..a197b9bf 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, err := newBmcServiceClient( + serviceClient := newBmcServiceClient( host, defaultConfig.Port, user, @@ -144,13 +144,6 @@ 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 - } - 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) { diff --git a/providers/supermicro/x11_firmware_bmc_test.go b/providers/supermicro/x11_firmware_bmc_test.go index b20e0c81..81cd8f08 100644 --- a/providers/supermicro/x11_firmware_bmc_test.go +++ b/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" From 988dbab4a830ff5d8ffb8dd0e54d2c5b4bd7256a Mon Sep 17 00:00:00 2001 From: Jacob Weinstock Date: Fri, 13 Dec 2024 17:36:31 -0700 Subject: [PATCH 2/2] Handle implementatins that might be nil: A recent bug showed that a nil is still a type so type asserting a nil will still work. We add a nil check so that methods on interfaces don't get called when the interface/implementation is nil. Signed-off-by: Jacob Weinstock --- bmc/bios.go | 12 ++++++++++++ bmc/boot_device.go | 6 ++++++ bmc/connection.go | 6 ++++++ bmc/firmware.go | 21 +++++++++++++++++++++ bmc/floppy.go | 6 ++++++ bmc/inventory.go | 3 +++ bmc/postcode.go | 3 +++ bmc/power.go | 6 ++++++ bmc/provider.go | 3 +++ bmc/reset.go | 3 +++ bmc/screenshot.go | 3 +++ bmc/sel.go | 9 +++++++++ bmc/sol.go | 3 +++ bmc/user.go | 12 ++++++++++++ bmc/virtual_media.go | 3 +++ 15 files changed, 99 insertions(+) diff --git a/bmc/bios.go b/bmc/bios.go index d578abe2..2acba9e3 100644 --- a/bmc/bios.go +++ b/bmc/bios.go @@ -152,6 +152,9 @@ Loop: func GetBiosConfigurationInterfaces(ctx context.Context, generic []interface{}) (biosConfig map[string]string, metadata Metadata, err error) { implementations := make([]biosConfigurationGetterProvider, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := biosConfigurationGetterProvider{name: getProviderName(elem)} switch p := elem.(type) { case BiosConfigurationGetter: @@ -178,6 +181,9 @@ func GetBiosConfigurationInterfaces(ctx context.Context, generic []interface{}) func SetBiosConfigurationInterfaces(ctx context.Context, generic []interface{}, biosConfig map[string]string) (metadata Metadata, err error) { implementations := make([]biosConfigurationSetterProvider, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := biosConfigurationSetterProvider{name: getProviderName(elem)} switch p := elem.(type) { case BiosConfigurationSetter: @@ -204,6 +210,9 @@ func SetBiosConfigurationInterfaces(ctx context.Context, generic []interface{}, func SetBiosConfigurationFromFileInterfaces(ctx context.Context, generic []interface{}, cfg string) (metadata Metadata, err error) { implementations := make([]biosConfigurationSetterProvider, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := biosConfigurationSetterProvider{name: getProviderName(elem)} switch p := elem.(type) { case BiosConfigurationSetter: @@ -230,6 +239,9 @@ func SetBiosConfigurationFromFileInterfaces(ctx context.Context, generic []inter func ResetBiosConfigurationInterfaces(ctx context.Context, generic []interface{}) (metadata Metadata, err error) { implementations := make([]biosConfigurationResetterProvider, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := biosConfigurationResetterProvider{name: getProviderName(elem)} switch p := elem.(type) { case BiosConfigurationResetter: diff --git a/bmc/boot_device.go b/bmc/boot_device.go index 27c236cf..a5d7c6d2 100644 --- a/bmc/boot_device.go +++ b/bmc/boot_device.go @@ -94,6 +94,9 @@ func setBootDevice(ctx context.Context, timeout time.Duration, bootDevice string func SetBootDeviceFromInterfaces(ctx context.Context, timeout time.Duration, bootDevice string, setPersistent, efiBoot bool, generic []interface{}) (ok bool, metadata Metadata, err error) { bdSetters := make([]bootDeviceProviders, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := bootDeviceProviders{name: getProviderName(elem)} switch p := elem.(type) { case BootDeviceSetter: @@ -148,6 +151,9 @@ func GetBootDeviceOverrideFromInterface( metadata = newMetadata() for _, elem := range providers { + if elem == nil { + continue + } switch p := elem.(type) { case BootDeviceOverrideGetter: provider := &bootOverrideProvider{name: getProviderName(elem), bootOverrideGetter: p} diff --git a/bmc/connection.go b/bmc/connection.go index 947fc1c1..8529af4c 100644 --- a/bmc/connection.go +++ b/bmc/connection.go @@ -62,6 +62,9 @@ func OpenConnectionFromInterfaces(ctx context.Context, timeout time.Duration, pr // For every provider, launch a goroutine that attempts to open a connection and report // back via the results channel what happened. for _, elem := range providers { + if elem == nil { + continue + } switch p := elem.(type) { case Opener: providerName := getProviderName(elem) @@ -138,6 +141,9 @@ func CloseConnectionFromInterfaces(ctx context.Context, generic []interface{}) ( closers := make([]connectionProviders, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := connectionProviders{name: getProviderName(elem)} switch p := elem.(type) { case Closer: diff --git a/bmc/firmware.go b/bmc/firmware.go index 523ade73..f6235c5a 100644 --- a/bmc/firmware.go +++ b/bmc/firmware.go @@ -71,6 +71,9 @@ func FirmwareInstallFromInterfaces(ctx context.Context, component, operationAppl implementations := make([]firmwareInstallerProvider, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := firmwareInstallerProvider{name: getProviderName(elem)} switch p := elem.(type) { case FirmwareInstaller: @@ -152,6 +155,9 @@ func FirmwareInstallStatusFromInterfaces(ctx context.Context, installVersion, co implementations := make([]firmwareInstallVerifierProvider, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := firmwareInstallVerifierProvider{name: getProviderName(elem)} switch p := elem.(type) { case FirmwareInstallVerifier: @@ -227,6 +233,9 @@ func FirmwareInstallUploadAndInitiateFromInterfaces(ctx context.Context, compone implementations := make([]firmwareInstallProvider, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := firmwareInstallProvider{name: getProviderName(elem)} switch p := elem.(type) { case FirmwareInstallProvider: @@ -306,6 +315,9 @@ func FirmwareInstallerUploadedFromInterfaces(ctx context.Context, component, upl implementations := make([]firmwareInstallerWithOptionsProvider, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := firmwareInstallerWithOptionsProvider{name: getProviderName(elem)} switch p := elem.(type) { case FirmwareInstallerUploaded: @@ -345,6 +357,9 @@ func FirmwareInstallStepsFromInterfaces(ctx context.Context, component string, g implementations := make([]firmwareInstallStepsGetterProvider, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := firmwareInstallStepsGetterProvider{name: getProviderName(elem)} switch p := elem.(type) { case FirmwareInstallStepsGetter: @@ -413,6 +428,9 @@ func FirmwareUploadFromInterfaces(ctx context.Context, component string, file *o implementations := make([]firmwareUploaderProvider, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := firmwareUploaderProvider{name: getProviderName(elem)} switch p := elem.(type) { case FirmwareUploader: @@ -527,6 +545,9 @@ func FirmwareTaskStatusFromInterfaces(ctx context.Context, kind bconsts.Firmware implementations := make([]firmwareTaskVerifierProvider, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := firmwareTaskVerifierProvider{name: getProviderName(elem)} switch p := elem.(type) { case FirmwareTaskVerifier: diff --git a/bmc/floppy.go b/bmc/floppy.go index 891a1131..41331409 100644 --- a/bmc/floppy.go +++ b/bmc/floppy.go @@ -55,6 +55,9 @@ func mountFloppyImage(ctx context.Context, image io.Reader, p []floppyImageUploa func MountFloppyImageFromInterfaces(ctx context.Context, image io.Reader, p []interface{}) (metadata Metadata, err error) { providers := make([]floppyImageUploaderProvider, 0) for _, elem := range p { + if elem == nil { + continue + } temp := floppyImageUploaderProvider{name: getProviderName(elem)} switch p := elem.(type) { case FloppyImageMounter: @@ -125,6 +128,9 @@ func unmountFloppyImage(ctx context.Context, p []floppyImageUnmounterProvider) ( func UnmountFloppyImageFromInterfaces(ctx context.Context, p []interface{}) (metadata Metadata, err error) { providers := make([]floppyImageUnmounterProvider, 0) for _, elem := range p { + if elem == nil { + continue + } temp := floppyImageUnmounterProvider{name: getProviderName(elem)} switch p := elem.(type) { case FloppyImageUnmounter: diff --git a/bmc/inventory.go b/bmc/inventory.go index 7ce067ea..7a9dd07b 100644 --- a/bmc/inventory.go +++ b/bmc/inventory.go @@ -57,6 +57,9 @@ func GetInventoryFromInterfaces(ctx context.Context, generic []interface{}) (dev implementations := make([]inventoryGetterProvider, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := inventoryGetterProvider{name: getProviderName(elem)} switch p := elem.(type) { case InventoryGetter: diff --git a/bmc/postcode.go b/bmc/postcode.go index a76c76ce..015e8cb4 100644 --- a/bmc/postcode.go +++ b/bmc/postcode.go @@ -57,6 +57,9 @@ func postCode(ctx context.Context, generic []postCodeGetterProvider) (status str func GetPostCodeInterfaces(ctx context.Context, generic []interface{}) (status string, code int, metadata Metadata, err error) { implementations := make([]postCodeGetterProvider, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := postCodeGetterProvider{name: getProviderName(elem)} switch p := elem.(type) { case PostCodeGetter: diff --git a/bmc/power.go b/bmc/power.go index 1b24edfa..e1641122 100644 --- a/bmc/power.go +++ b/bmc/power.go @@ -79,6 +79,9 @@ func SetPowerStateFromInterfaces(ctx context.Context, timeout time.Duration, sta powerSetter := make([]powerProviders, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := powerProviders{name: getProviderName(elem)} switch p := elem.(type) { case PowerSetter: @@ -132,6 +135,9 @@ func GetPowerStateFromInterfaces(ctx context.Context, timeout time.Duration, gen powerStateGetter := make([]powerProviders, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := powerProviders{name: getProviderName(elem)} switch p := elem.(type) { case PowerStateGetter: diff --git a/bmc/provider.go b/bmc/provider.go index b7b1c904..efed37c3 100644 --- a/bmc/provider.go +++ b/bmc/provider.go @@ -11,6 +11,9 @@ type Provider interface { // getProviderName returns the name a provider supplies if they implement the Provider interface // if not implemented then the concrete type is returned func getProviderName(provider interface{}) string { + if provider == nil { + return "" + } switch p := provider.(type) { case Provider: return p.Name() diff --git a/bmc/reset.go b/bmc/reset.go index b7c2d706..64b65ab8 100644 --- a/bmc/reset.go +++ b/bmc/reset.go @@ -61,6 +61,9 @@ func ResetBMCFromInterfaces(ctx context.Context, timeout time.Duration, resetTyp bmcSetters := make([]bmcProviders, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := bmcProviders{name: getProviderName(elem)} switch p := elem.(type) { case BMCResetter: diff --git a/bmc/screenshot.go b/bmc/screenshot.go index 9ab98af6..62812ccf 100644 --- a/bmc/screenshot.go +++ b/bmc/screenshot.go @@ -52,6 +52,9 @@ func screenshot(ctx context.Context, generic []screenshotGetterProvider) (image func ScreenshotFromInterfaces(ctx context.Context, generic []interface{}) (image []byte, fileType string, metadata Metadata, err error) { implementations := make([]screenshotGetterProvider, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := screenshotGetterProvider{name: getProviderName(elem)} switch p := elem.(type) { case ScreenshotGetter: diff --git a/bmc/sel.go b/bmc/sel.go index 002a25d1..de9c78ff 100644 --- a/bmc/sel.go +++ b/bmc/sel.go @@ -56,6 +56,9 @@ func clearSystemEventLog(ctx context.Context, timeout time.Duration, s []systemE func ClearSystemEventLogFromInterfaces(ctx context.Context, timeout time.Duration, generic []interface{}) (metadata Metadata, err error) { selServices := make([]systemEventLogProviders, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := systemEventLogProviders{name: getProviderName(elem)} switch p := elem.(type) { case SystemEventLog: @@ -107,6 +110,9 @@ func getSystemEventLog(ctx context.Context, timeout time.Duration, s []systemEve func GetSystemEventLogFromInterfaces(ctx context.Context, timeout time.Duration, generic []interface{}) (sel SystemEventLogEntries, metadata Metadata, err error) { selServices := make([]systemEventLogProviders, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := systemEventLogProviders{name: getProviderName(elem)} switch p := elem.(type) { case SystemEventLog: @@ -158,6 +164,9 @@ func getSystemEventLogRaw(ctx context.Context, timeout time.Duration, s []system func GetSystemEventLogRawFromInterfaces(ctx context.Context, timeout time.Duration, generic []interface{}) (eventlog string, metadata Metadata, err error) { selServices := make([]systemEventLogProviders, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := systemEventLogProviders{name: getProviderName(elem)} switch p := elem.(type) { case SystemEventLog: diff --git a/bmc/sol.go b/bmc/sol.go index 89c172b0..0caeffaa 100644 --- a/bmc/sol.go +++ b/bmc/sol.go @@ -53,6 +53,9 @@ func deactivateSOL(ctx context.Context, timeout time.Duration, b []deactivatorPr func DeactivateSOLFromInterfaces(ctx context.Context, timeout time.Duration, generic []interface{}) (metadata Metadata, err error) { deactivators := make([]deactivatorProvider, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := deactivatorProvider{name: getProviderName(elem)} switch p := elem.(type) { case SOLDeactivator: diff --git a/bmc/user.go b/bmc/user.go index 8dedab91..c4845d58 100644 --- a/bmc/user.go +++ b/bmc/user.go @@ -75,6 +75,9 @@ func createUser(ctx context.Context, timeout time.Duration, user, pass, role str func CreateUserFromInterfaces(ctx context.Context, timeout time.Duration, user, pass, role string, generic []interface{}) (ok bool, metadata Metadata, err error) { userCreators := make([]userProviders, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := userProviders{name: getProviderName(elem)} switch u := elem.(type) { case UserCreator: @@ -128,6 +131,9 @@ func updateUser(ctx context.Context, timeout time.Duration, user, pass, role str func UpdateUserFromInterfaces(ctx context.Context, timeout time.Duration, user, pass, role string, generic []interface{}) (ok bool, metadata Metadata, err error) { userUpdaters := make([]userProviders, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := userProviders{name: getProviderName(elem)} switch u := elem.(type) { case UserUpdater: @@ -181,6 +187,9 @@ func deleteUser(ctx context.Context, timeout time.Duration, user string, u []use func DeleteUserFromInterfaces(ctx context.Context, timeout time.Duration, user string, generic []interface{}) (ok bool, metadata Metadata, err error) { userDeleters := make([]userProviders, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := userProviders{name: getProviderName(elem)} switch u := elem.(type) { case UserDeleter: @@ -230,6 +239,9 @@ func readUsers(ctx context.Context, timeout time.Duration, u []userProviders) (u func ReadUsersFromInterfaces(ctx context.Context, timeout time.Duration, generic []interface{}) (users []map[string]string, metadata Metadata, err error) { userReaders := make([]userProviders, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := userProviders{name: getProviderName(elem)} switch u := elem.(type) { case UserReader: diff --git a/bmc/virtual_media.go b/bmc/virtual_media.go index 10ea5e56..6641a0ed 100644 --- a/bmc/virtual_media.go +++ b/bmc/virtual_media.go @@ -54,6 +54,9 @@ func setVirtualMedia(ctx context.Context, kind string, mediaURL string, b []virt func SetVirtualMediaFromInterfaces(ctx context.Context, kind string, mediaURL string, generic []interface{}) (ok bool, metadata Metadata, err error) { bdSetters := make([]virtualMediaProviders, 0) for _, elem := range generic { + if elem == nil { + continue + } temp := virtualMediaProviders{name: getProviderName(elem)} switch p := elem.(type) { case VirtualMediaSetter: