From 059b754ae098c55eb3b2466e5aaf849e131ea974 Mon Sep 17 00:00:00 2001 From: Joel Rebello Date: Wed, 6 Sep 2023 09:53:02 +0200 Subject: [PATCH] providers/redfish: simplify the multipart payload build - use a struct instead This makes the code easier to read and maintain. As of now we limit the method to expect an os.File as an io.Reader, once its clear theres other implementations of io.Readers required we can accept those and figure how to determine the file size for the multipart payload --- providers/redfish/firmware.go | 147 +++++++++++++++-------------- providers/redfish/firmware_test.go | 21 ++++- 2 files changed, 91 insertions(+), 77 deletions(-) diff --git a/providers/redfish/firmware.go b/providers/redfish/firmware.go index d0293b73..a91bedca 100644 --- a/providers/redfish/firmware.go +++ b/providers/redfish/firmware.go @@ -25,6 +25,7 @@ import ( var ( errInsufficientCtxTimeout = errors.New("remaining context timeout insufficient to install firmware") + errMultiPartPayload = errors.New("error preparing multipart payload") ) // SupportedFirmwareApplyAtValues returns the supported redfish firmware applyAt values @@ -37,10 +38,16 @@ func SupportedFirmwareApplyAtValues() []string { // FirmwareInstall uploads and initiates the firmware install process func (c *Conn) FirmwareInstall(ctx context.Context, component, applyAt string, forceInstall bool, reader io.Reader) (taskID string, err error) { + // limit to *os.File until theres a need for other types of readers + updateFile, ok := reader.(*os.File) + if !ok { + return "", errors.Wrap(bmclibErrs.ErrFirmwareInstall, "method expects an *os.File object") + } + // validate firmware update mechanism is supported err = c.firmwareUpdateCompatible(ctx) if err != nil { - return "", err + return "", errors.Wrap(bmclibErrs.ErrFirmwareInstall, err.Error()) } // validate applyAt parameter @@ -59,7 +66,7 @@ func (c *Conn) FirmwareInstall(ctx context.Context, component, applyAt string, f // list redfish firmware install task if theres one present task, err := c.GetFirmwareInstallTaskQueued(ctx, component) if err != nil { - return "", err + return "", errors.Wrap(bmclibErrs.ErrFirmwareInstall, err.Error()) } if task != nil { @@ -101,9 +108,9 @@ func (c *Conn) FirmwareInstall(ctx context.Context, component, applyAt string, f c.redfishwrapper.SetHttpClientTimeout(time.Until(ctxDeadline)) - payload := []map[string]io.Reader{ - {"UpdateParameters": bytes.NewReader(updateParameters)}, - {"UpdateFile": reader}, + payload := &multipartPayload{ + updateParameters: bytes.NewReader(updateParameters), + updateFile: updateFile, } resp, err := c.runRequestWithMultipartPayload(http.MethodPost, "/redfish/v1/UpdateService/MultipartUpload", payload) @@ -127,6 +134,11 @@ func (c *Conn) FirmwareInstall(ctx context.Context, component, applyAt string, f return taskID, nil } +type multipartPayload struct { + updateParameters io.Reader + updateFile *os.File +} + // FirmwareInstallStatus returns the status of the firmware install task queued func (c *Conn) FirmwareInstallStatus(ctx context.Context, installVersion, component, taskID string) (state string, err error) { vendor, _, err := c.DeviceVendorModel(ctx) @@ -215,54 +227,46 @@ func (p pipeReaderFakeSeeker) Seek(offset int64, whence int) (int64, error) { // // It creates a temporary form without reading in the update file payload and returns // sizeOf(form) + sizeOf(update file) -func multipartPayloadSize(payload []map[string]io.Reader) (int64, *bytes.Buffer, error) { +func multipartPayloadSize(payload *multipartPayload) (int64, *bytes.Buffer, error) { body := &bytes.Buffer{} form := multipart.NewWriter(body) - var size int64 - var err error - for idx, elem := range payload { - for key, reader := range elem { - var part io.Writer - if file, ok := reader.(*os.File); ok { - // Add update file fields - if _, err = form.CreateFormFile(key, filepath.Base(file.Name())); err != nil { - return 0, body, err - } - - // determine file size - finfo, err := file.Stat() - if err != nil { - return 0, body, err - } - - size = finfo.Size() - - } else { - // Add other fields - if part, err = updateParametersFormField(key, form); err != nil { - return 0, body, err - } - - buf := bytes.Buffer{} - teeReader := io.TeeReader(reader, &buf) - - if _, err = io.Copy(part, teeReader); err != nil { - return 0, body, err - } - - // place it back so its available to be read again - payload[idx][key] = bytes.NewReader(buf.Bytes()) - } - } + // Add UpdateParameters field part + part, err := updateParametersFormField("UpdateParameters", form) + if err != nil { + return 0, body, err + } + + // a buffer to save the contents of the updateParameters reader + buf := bytes.Buffer{} + teeReader := io.TeeReader(payload.updateParameters, &buf) + + if _, err = io.Copy(part, teeReader); err != nil { + return 0, body, err + } + + // restore the reader + payload.updateParameters = bytes.NewReader(buf.Bytes()) + + // Add updateFile form + _, err = form.CreateFormFile("UpdateFile", filepath.Base(payload.updateFile.Name())) + if err != nil { + return 0, body, err } + // determine update file size + finfo, err := payload.updateFile.Stat() + if err != nil { + return 0, body, err + } + + // add terminating boundary to multipart form err = form.Close() if err != nil { return 0, body, err } - return int64(body.Len()) + size, body, nil + return int64(body.Len()) + finfo.Size(), body, nil } // runRequestWithMultipartPayload is a copy of https://github.com/stmcginnis/gofish/blob/main/client.go#L349 @@ -286,7 +290,7 @@ func multipartPayloadSize(payload []map[string]io.Reader) (int64, *bytes.Buffer, // hey. // --------------------------1771f60800cb2801-- -func (c *Conn) runRequestWithMultipartPayload(method, url string, payload []map[string]io.Reader) (*http.Response, error) { +func (c *Conn) runRequestWithMultipartPayload(method, url string, payload *multipartPayload) (*http.Response, error) { if url == "" { return nil, fmt.Errorf("unable to execute request, no target provided") } @@ -314,6 +318,7 @@ func (c *Conn) runRequestWithMultipartPayload(method, url string, payload []map[ // initiate a mulitpart writer form := multipart.NewWriter(pipeWriter) + // go routine blocks on the io.Copy until the http request is made go func() { var err error defer func() { @@ -324,34 +329,32 @@ func (c *Conn) runRequestWithMultipartPayload(method, url string, payload []map[ defer pipeWriter.Close() - for _, elem := range payload { - for key, reader := range elem { - var part io.Writer - // add update file multipart form header - // - // Content-Disposition: form-data; name="UpdateFile"; filename="dum - // myfile" - // Content-Type: application/octet-stream - if file, ok := reader.(*os.File); ok { - // Add a file stream - if part, err = form.CreateFormFile(key, filepath.Base(file.Name())); err != nil { - return - } - } else { - // add update parameters multipart form header - // - // Content-Disposition: form-data; name="UpdateParameters" - // Content-Type: application/json - if part, err = updateParametersFormField(key, form); err != nil { - return - } - } - - // copy from source into form part writer - if _, err = io.Copy(part, reader); err != nil { - return - } - } + // Add UpdateParameters part + parametersPart, err := updateParametersFormField("UpdateParameters", form) + if err != nil { + c.Log.Error(errMultiPartPayload, err.Error()+": UpdateParameters part copy error") + + return + } + + if _, err = io.Copy(parametersPart, payload.updateParameters); err != nil { + c.Log.Error(errMultiPartPayload, err.Error()+": UpdateParameters part copy error") + + return + } + + // Add UpdateFile part + updateFilePart, err := form.CreateFormFile("UpdateFile", filepath.Base(payload.updateFile.Name())) + if err != nil { + c.Log.Error(errMultiPartPayload, err.Error()+": UpdateFile part create error") + + return + } + + if _, err = io.Copy(updateFilePart, payload.updateFile); err != nil { + c.Log.Error(errMultiPartPayload, err.Error()+": UpdateFile part copy error") + + return } // add terminating boundary to multipart form diff --git a/providers/redfish/firmware_test.go b/providers/redfish/firmware_test.go index 26c9131e..62763ce5 100644 --- a/providers/redfish/firmware_test.go +++ b/providers/redfish/firmware_test.go @@ -97,6 +97,17 @@ func TestFirmwareInstall(t *testing.T) { false, nil, "", + bmclibErrs.ErrFirmwareInstall, + "method expects an *os.File object", + "expect *os.File object", + }, + { + common.SlugBIOS, + constants.FirmwareApplyOnReset, + false, + false, + &os.File{}, + "", errInsufficientCtxTimeout, "", "remaining context deadline", @@ -106,7 +117,7 @@ func TestFirmwareInstall(t *testing.T) { "invalidApplyAt", false, true, - nil, + &os.File{}, "", bmclibErrs.ErrFirmwareInstall, "invalid applyAt parameter", @@ -189,15 +200,15 @@ func TestMultipartPayloadSize(t *testing.T) { testCases := []struct { testName string - payload []map[string]io.Reader + payload *multipartPayload expectedSize int64 errorMsg string }{ { "content length as expected", - []map[string]io.Reader{ - {"UpdateParameters": bytes.NewReader(updateParameters)}, - {"UpdateFile": testfileFH}, + &multipartPayload{ + updateParameters: bytes.NewReader(updateParameters), + updateFile: testfileFH, }, 475, "",