Skip to content

Commit

Permalink
providers/redfish: simplify the multipart payload build - use a struc…
Browse files Browse the repository at this point in the history 
…t 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
  • Loading branch information
@joelrebel
joelrebel committed Sep 6, 2023
1 parent 6cf0168 commit 059b754
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 77 deletions.
147 changes: 75 additions & 72 deletions providers/redfish/firmware.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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")
}
Expand Down Expand Up @@ -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() {
Expand All @@ -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
Expand Down
21 changes: 16 additions & 5 deletions providers/redfish/firmware_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -106,7 +117,7 @@ func TestFirmwareInstall(t *testing.T) {
"invalidApplyAt",
false,
true,
nil,
&os.File{},
"",
bmclibErrs.ErrFirmwareInstall,
"invalid applyAt parameter",
Expand Down Expand Up @@ -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,
"",
Expand Down

0 comments on commit 059b754

Please sign in to comment.