Redfish stream multipart uploads

examples/install-firmware/main.go

@@ -114,7 +114,7 @@ func main() {
 		}
 
 		switch state {
-		case constants.FirmwareInstallRunning:
+		case constants.FirmwareInstallRunning, constants.FirmwareInstallInitializing:
 			l.WithFields(logrus.Fields{"state": state, "component": *component}).Info("firmware install running")
 
 		case constants.FirmwareInstallFailed:

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 {
@@ -93,17 +100,17 @@ func (c *Conn) FirmwareInstall(ctx context.Context, component, applyAt string, f
 	// override the gofish HTTP client timeout,
 	// since the context timeout is set at Open() and is at a lower value than required for this operation.
 	//
-	// record the http client time to be restored
+	// record the http client timeout to be restored
 	httpClientTimeout := c.redfishwrapper.HttpClientTimeout()
 	defer func() {
 		c.redfishwrapper.SetHttpClientTimeout(httpClientTimeout)
 	}()
 
 	c.redfishwrapper.SetHttpClientTimeout(time.Until(ctxDeadline))
 
-	payload := map[string]io.Reader{
-		"UpdateParameters": bytes.NewReader(updateParameters),
-		"UpdateFile":       reader,
+	payload := &multipartPayload{
+		updateParameters: 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 []byte
+	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)
@@ -197,6 +209,59 @@ func (c *Conn) firmwareUpdateCompatible(ctx context.Context) (err error) {
 	return nil
 }
 
+// pipeReaderFakeSeeker wraps the io.PipeReader and implements the io.Seeker interface
+// to meet the API requirements for the Gofish client https://github.com/stmcginnis/gofish/blob/46b1b33645ed1802727dc4df28f5d3c3da722b15/client.go#L434
+//
+// The Gofish method linked does not currently perform seeks and so a PR will be suggested
+// to change the method signature to accept an io.Reader instead.
+type pipeReaderFakeSeeker struct {
+	*io.PipeReader
+}
+
+// Seek impelements the io.Seeker interface only to panic if called
+func (p pipeReaderFakeSeeker) Seek(offset int64, whence int) (int64, error) {
+	return 0, errors.New("Seek() not implemented for fake pipe reader seeker.")
+}
+
+// multipartPayloadSize prepares a temporary multipart form to determine the form size
+//
+// It creates a temporary form without reading in the update file payload and returns
+// sizeOf(form) + sizeOf(update file)
+func multipartPayloadSize(payload *multipartPayload) (int64, *bytes.Buffer, error) {
+	body := &bytes.Buffer{}
+	form := multipart.NewWriter(body)
+
+	// Add UpdateParameters field part
+	part, err := updateParametersFormField("UpdateParameters", form)
+	if err != nil {
+		return 0, body, err
+	}
+
+	if _, err = io.Copy(part, bytes.NewReader(payload.updateParameters)); err != nil {
+		return 0, body, err
+	}
+
+	// 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()) + finfo.Size(), body, nil
+}
+
 // runRequestWithMultipartPayload is a copy of https://github.com/stmcginnis/gofish/blob/main/client.go#L349
 // with a change to add the UpdateParameters multipart form field with a json content type header
 // the resulting form ends up in this format
@@ -218,34 +283,81 @@ func (c *Conn) firmwareUpdateCompatible(ctx context.Context) (err error) {
 
 // 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")
 	}
 
-	var payloadBuffer bytes.Buffer
-	var err error
-	payloadWriter := multipart.NewWriter(&payloadBuffer)
-	for key, reader := range payload {
-		var partWriter io.Writer
-		if file, ok := reader.(*os.File); ok {
-			// Add a file stream
-			if partWriter, err = payloadWriter.CreateFormFile(key, filepath.Base(file.Name())); err != nil {
-				return nil, err
-			}
-		} else {
-			// Add other fields
-			if partWriter, err = updateParametersFormField(key, payloadWriter); err != nil {
-				return nil, err
+	// A content-length header is passed in to indicate the payload size
+	//
+	// The Content-length is set explicitly since the payload is an io.Reader,
+	// https://github.com/golang/go/blob/ddad9b618cce0ed91d66f0470ddb3e12cfd7eeac/src/net/http/request.go#L861
+	//
+	// Without the content-length header the http client will set the Transfer-Encoding to 'chunked'
+	// and that does not work for some BMCs (iDracs).
+	contentLength, _, err := multipartPayloadSize(payload)
+	if err != nil {
+		return nil, errors.Wrap(err, "error determining multipart payload size")
+	}
+
+	headers := map[string]string{
+		"Content-Length": strconv.FormatInt(contentLength, 10),
+	}
+
+	// setup pipe
+	pipeReader, pipeWriter := io.Pipe()
+	defer pipeReader.Close()
+
+	// 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() {
+			if err != nil {
+				c.Log.Error(err, "multipart upload error occurred")
 			}
+		}()
+
+		defer pipeWriter.Close()
+
+		// 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(partWriter, reader); err != nil {
-			return nil, err
+
+		if _, err = io.Copy(parametersPart, bytes.NewReader(payload.updateParameters)); err != nil {
+			c.Log.Error(errMultiPartPayload, err.Error()+": UpdateParameters part copy error")
+
+			return
 		}
-	}
-	payloadWriter.Close()
 
-	return c.redfishwrapper.RunRawRequestWithHeaders(method, url, bytes.NewReader(payloadBuffer.Bytes()), payloadWriter.FormDataContentType(), nil)
+		// 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
+		form.Close()
+	}()
+
+	// pipeReader wrapped as a io.ReadSeeker to satisfy the gofish method signature
+	reader := pipeReaderFakeSeeker{pipeReader}
+
+	return c.redfishwrapper.RunRawRequestWithHeaders(method, url, reader, form.FormDataContentType(), headers)
 }
 
 // sets up the UpdateParameters MIMEHeader for the multipart form

providers/redfish/firmware_test.go

@@ -2,6 +2,7 @@ package redfish
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"io"
 	"log"
@@ -30,6 +31,9 @@ func multipartUpload(w http.ResponseWriter, r *http.Request) {
 		log.Fatal(err)
 	}
 
+	// payload size
+	expectedContentLength := "476"
+
 	expected := []string{
 		`Content-Disposition: form-data; name="UpdateParameters"`,
 		`Content-Type: application/json`,
@@ -46,11 +50,15 @@ func multipartUpload(w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
+	if r.Header.Get("Content-Length") != expectedContentLength {
+		log.Fatal("Header Content-Length does not match expected")
+	}
+
 	w.Header().Add("Location", "/redfish/v1/TaskService/Tasks/JID_467696020275")
 	w.WriteHeader(http.StatusAccepted)
 }
 
-func Test_FirmwareInstall(t *testing.T) {
+func TestFirmwareInstall(t *testing.T) {
 	// curl -Lv -s -k -u root:calvin \
 	// -F 'UpdateParameters={"Targets": [], "@Redfish.OperationApplyTime": "OnReset", "Oem": {}};type=application/json' \
 	// -F'foo.bin=@/tmp/dummyfile;application/octet-stream'
@@ -88,6 +96,17 @@ func Test_FirmwareInstall(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",
@@ -97,7 +116,7 @@ func Test_FirmwareInstall(t *testing.T) {
 			"invalidApplyAt",
 			false,
 			true,
-			nil,
+			&os.File{},
 			"",
 			bmclibErrs.ErrFirmwareInstall,
 			"invalid applyAt parameter",
@@ -151,7 +170,64 @@ func Test_FirmwareInstall(t *testing.T) {
 
 }
 
-func Test_firmwareUpdateCompatible(t *testing.T) {
+func TestMultipartPayloadSize(t *testing.T) {
+	updateParameters, err := json.Marshal(struct {
+		Targets            []string `json:"Targets"`
+		RedfishOpApplyTime string   `json:"@Redfish.OperationApplyTime"`
+		Oem                struct{} `json:"Oem"`
+	}{
+		[]string{},
+		"foobar",
+		struct{}{},
+	})
+
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	tmpdir := t.TempDir()
+	binPath := filepath.Join(tmpdir, "test.bin")
+	err = os.WriteFile(binPath, []byte(`HELLOWORLD`), 0600)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	testfileFH, err := os.Open(binPath)
+	if err != nil {
+		t.Fatalf("%s -> %s", err.Error(), binPath)
+	}
+
+	testCases := []struct {
+		testName     string
+		payload      *multipartPayload
+		expectedSize int64
+		errorMsg     string
+	}{
+		{
+			"content length as expected",
+			&multipartPayload{
+				updateParameters: updateParameters,
+				updateFile:       testfileFH,
+			},
+			475,
+			"",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.testName, func(t *testing.T) {
+			gotSize, _, err := multipartPayloadSize(tc.payload)
+			if tc.errorMsg != "" {
+				assert.Contains(t, err.Error(), tc.errorMsg)
+			}
+
+			assert.Nil(t, err)
+			assert.Equal(t, tc.expectedSize, gotSize)
+		})
+	}
+}
+
+func TestFirmwareUpdateCompatible(t *testing.T) {
 	err := mockClient.firmwareUpdateCompatible(context.TODO())
 	if err != nil {
 		t.Fatal(err)