Merge branch 'mk/golang-only-image-scaler' into 'master'

Image scaler binary with pure golang lib

See merge request gitlab-org/gitlab-workhorse!603

.gitignore

@@ -2,6 +2,7 @@ testdata/data
 testdata/scratch
 testdata/public
 /gitlab-workhorse
+/gitlab-resize-image
 /gitlab-zip-cat
 /gitlab-zip-metadata
 /_build

.gitlab-ci.yml

@@ -38,7 +38,7 @@ changelog:
     GITALY_ADDRESS: "tcp://gitaly:8075"
   script:
   - go version
-  - apt-get update && apt-get -y install libimage-exiftool-perl graphicsmagick
+  - apt-get update && apt-get -y install libimage-exiftool-perl
   - make test
 
 test using go 1.13:

Makefile

@@ -11,7 +11,7 @@ VERSION_STRING := v$(shell cat VERSION)
 endif
 BUILD_TIME := $(shell date -u +%Y%m%d.%H%M%S)
 GOBUILD := go build -ldflags "-X main.Version=$(VERSION_STRING) -X main.BuildTime=$(BUILD_TIME)"
-EXE_ALL := gitlab-zip-cat gitlab-zip-metadata gitlab-workhorse
+EXE_ALL := gitlab-resize-image gitlab-zip-cat gitlab-zip-metadata gitlab-workhorse
 INSTALL := install
 BUILD_TAGS := tracer_static tracer_static_jaeger continuous_profiler_stackdriver
 
@@ -40,6 +40,10 @@ $(TARGET_SETUP):
 	mkdir -p "$(TARGET_DIR)"
 	touch "$(TARGET_SETUP)"
 
+gitlab-resize-image: $(TARGET_SETUP) $(shell find cmd/gitlab-resize-image/ -name '*.go')
+	$(call message,Building $@)
+	$(GOBUILD) -tags "$(BUILD_TAGS)" -o $(BUILD_DIR)/$@ $(PKG)/cmd/$@
+
 gitlab-zip-cat:	$(TARGET_SETUP) $(shell find cmd/gitlab-zip-cat/ -name '*.go')
 	$(call message,Building $@)
 	$(GOBUILD) -tags "$(BUILD_TAGS)" -o $(BUILD_DIR)/$@ $(PKG)/cmd/$@
@@ -53,7 +57,7 @@ gitlab-workhorse:	$(TARGET_SETUP) $(shell find . -name '*.go' | grep -v '^\./_')
 	$(GOBUILD) -tags "$(BUILD_TAGS)" -o $(BUILD_DIR)/$@ $(PKG)
 
 .PHONY:	install
-install:	gitlab-workhorse gitlab-zip-cat gitlab-zip-metadata
+install:	gitlab-workhorse gitlab-resize-image gitlab-zip-cat gitlab-zip-metadata
 	$(call message,$@)
 	mkdir -p $(DESTDIR)$(PREFIX)/bin/
 	cd $(BUILD_DIR) && $(INSTALL) gitlab-workhorse gitlab-zip-cat gitlab-zip-metadata $(DESTDIR)$(PREFIX)/bin/

README.md

@@ -212,28 +212,6 @@ images. If you installed GitLab:
     sudo yum install perl-Image-ExifTool
     ```
 
-### GraphicsMagick (**experimental**)
-
-Workhorse has an experimental feature that allows us to rescale images on-the-fly.
-If you do not run Workhorse in a container where the `gm` tool is already installed,
-you will have to install it on your host machine instead:
-
-#### macOS
-
-```sh
-brew install graphicsmagick
-```
-
-#### Debian/Ubuntu
-
-```sh
-sudo apt-get install graphicsmagick
-```
-
-For installation on other platforms, please consult http://www.graphicsmagick.org/README.html.
-
-Note that Omnibus containers already come with `gm` installed.
-
 ## Error tracking
 
 GitLab-Workhorse supports remote error tracking with

changelogs/unreleased/mk-golang-only-image-scaler.yml

+---
+title: Switch image scaler to a Go-only solution
+merge_request: 603
+author:
+type: changed

cmd/gitlab-resize-image/main.go

+package main
+
+import (
+	"fmt"
+	"image"
+	"mime"
+	"os"
+	"strconv"
+
+	"github.com/disintegration/imaging"
+)
+
+func main() {
+	if err := _main(); err != nil {
+		fmt.Fprintf(os.Stderr, "%s: fatal: %v\n", os.Args[0], err)
+		os.Exit(1)
+	}
+}
+
+func _main() error {
+	widthParam := os.Getenv("GL_RESIZE_IMAGE_WIDTH")
+	requestedWidth, err := strconv.Atoi(widthParam)
+	if err != nil {
+		return fmt.Errorf("GL_RESIZE_IMAGE_WIDTH: %w", err)
+	}
+	contentType := os.Getenv("GL_RESIZE_IMAGE_CONTENT_TYPE")
+	if contentType == "" {
+		return fmt.Errorf("GL_RESIZE_IMAGE_CONTENT_TYPE is empty")
+	}
+
+	src, extension, err := image.Decode(os.Stdin)
+	if err != nil {
+		return fmt.Errorf("decode: %w", err)
+	}
+	if detectedType := mime.TypeByExtension("." + extension); detectedType != contentType {
+		return fmt.Errorf("MIME types do not match; requested: %s; actual: %s", contentType, detectedType)
+	}
+	format, err := imaging.FormatFromExtension(extension)
+	if err != nil {
+		return fmt.Errorf("find imaging format: %w", err)
+	}
+
+	image := imaging.Resize(src, requestedWidth, 0, imaging.Lanczos)
+	return imaging.Encode(os.Stdout, image, format)
+}

go.mod

@@ -9,6 +9,7 @@ require (
 	github.com/alecthomas/chroma v0.7.3
 	github.com/aws/aws-sdk-go v1.31.13
 	github.com/dgrijalva/jwt-go v3.2.0+incompatible
+	github.com/disintegration/imaging v1.6.2
 	github.com/getsentry/raven-go v0.1.2
 	github.com/golang/gddo v0.0.0-20190419222130-af0f2af80721
 	github.com/golang/protobuf v1.4.2

go.sum

@@ -155,6 +155,8 @@ github.com/dgrijalva/jwt-go v3.2.0+incompatible/go.mod h1:E3ru+11k8xSBh+hMPgOLZm
 github.com/dgryski/go-farm v0.0.0-20190423205320-6a90982ecee2/go.mod h1:SqUrOPUnsFjfmXRMNPybcSiG0BgUW2AuFH8PAnS2iTw=
 github.com/dimchansky/utfbom v1.1.0 h1:FcM3g+nofKgUteL8dm/UpdRXNC9KmADgTpLKsu0TRo4=
 github.com/dimchansky/utfbom v1.1.0/go.mod h1:rO41eb7gLfo8SF1jd9F8HplJm1Fewwi4mQvIirEdv+8=
+github.com/disintegration/imaging v1.6.2 h1:w1LecBlG2Lnp8B3jk5zSuNqd7b4DXhcjwek1ei82L+c=
+github.com/disintegration/imaging v1.6.2/go.mod h1:44/5580QXChDfwIclfc/PCwrr44amcmDAg8hxG0Ewe4=
 github.com/dlclark/regexp2 v1.2.0 h1:8sAhBGEM0dRWogWqWyQeIJnxjWO6oIjl8FKqREDsGfk=
 github.com/dlclark/regexp2 v1.2.0/go.mod h1:2pZnwuY/m+8K6iRw6wQdMtk+rH5tNGR1i55kozfMjCc=
 github.com/dustin/go-humanize v1.0.0/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk=
@@ -536,6 +538,8 @@ golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EH
 golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU=
 golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js=
 golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0=
+golang.org/x/image v0.0.0-20191009234506-e7c1f5e7dbb8 h1:hVwzHzIUGRjiF7EcUjqNxk3NCfkPxbDKRdnNE1Rpg0U=
+golang.org/x/image v0.0.0-20191009234506-e7c1f5e7dbb8/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0=
 golang.org/x/lint v0.0.0-20180702182130-06c8688daad7 h1:00BeQWmeaGazuOrq8Q5K5d3/cHaGuFrZzpaHBXfrsUA=
 golang.org/x/lint v0.0.0-20180702182130-06c8688daad7/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE=
 golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE=

internal/imageresizer/image_resizer.go

@@ -8,6 +8,7 @@ import (
 	"net/http"
 	"os"
 	"os/exec"
+	"strconv"
 	"strings"
 	"sync/atomic"
 	"syscall"
@@ -40,7 +41,10 @@ type processCounter struct {
 
 var numScalerProcs processCounter
 
-const maxImageScalerProcs = 100
+const (
+	maxImageScalerProcs     = 100
+	maxAllowedFileSizeBytes = 250 * 1000 // 250kB
+)
 
 // Images might be located remotely in object storage, in which case we need to stream
 // it via http(s)
@@ -88,7 +92,7 @@ func init() {
 	prometheus.MustRegister(imageResizeCompleted)
 }
 
-// This Injecter forks into graphicsmagick to resize an image identified by path or URL
+// This Injecter forks into a dedicated scaler process to resize an image identified by path or URL
 // and streams the resized image back to the client
 func (r *resizer) Inject(w http.ResponseWriter, req *http.Request, paramsData string) {
 	start := time.Now()
@@ -101,7 +105,7 @@ func (r *resizer) Inject(w http.ResponseWriter, req *http.Request, paramsData st
 		return
 	}
 
-	sourceImageReader, filesize, err := openSourceImage(params.Location)
+	sourceImageReader, fileSize, err := openSourceImage(params.Location)
 	if err != nil {
 		// This means we cannot even read the input image; fail fast.
 		helper.Fail500(w, req, fmt.Errorf("ImageResizer: Failed opening image data stream: %v", err))
@@ -115,13 +119,13 @@ func (r *resizer) Inject(w http.ResponseWriter, req *http.Request, paramsData st
 			"duration_s":        time.Since(start).Seconds(),
 			"target_width":      params.Width,
 			"content_type":      params.ContentType,
-			"original_filesize": filesize,
+			"original_filesize": fileSize,
 		}
 	}
 
-	// We first attempt to rescale the image; if this should fail for any reason, we
-	// simply fail over to rendering out the original image unchanged.
-	imageReader, resizeCmd, err := tryResizeImage(req, sourceImageReader, logger.Writer(), params)
+	// We first attempt to rescale the image; if this should fail for any reason, imageReader
+	// will point to the original image, i.e. we render it unchanged.
+	imageReader, resizeCmd, err := tryResizeImage(req, sourceImageReader, logger.Writer(), params, fileSize)
 	if err != nil {
 		// something failed, but we can still write out the original image, do don't return early
 		helper.LogErrorWithFields(req, err, *logFields(0))
@@ -130,24 +134,39 @@ func (r *resizer) Inject(w http.ResponseWriter, req *http.Request, paramsData st
 	imageResizeCompleted.Inc()
 
 	w.Header().Del("Content-Length")
-	bytesWritten, err := io.Copy(w, imageReader)
+	bytesWritten, err := serveImage(imageReader, w, resizeCmd)
 	if err != nil {
 		handleFailedCommand(w, req, bytesWritten, err, logFields(bytesWritten))
-	} else if err = resizeCmd.Wait(); err != nil {
-		handleFailedCommand(w, req, bytesWritten, err, logFields(bytesWritten))
-	} else {
-		logger.WithFields(*logFields(bytesWritten)).Printf("ImageResizer: Success")
+		return
 	}
+
+	logger.WithFields(*logFields(bytesWritten)).Printf("ImageResizer: Success")
 }
 
-func handleFailedCommand(w http.ResponseWriter, req *http.Request, bytesWritten int64, err error, logFields *log.Fields) {
+// Streams image data from the given reader to the given writer and returns the number of bytes written.
+// Errors are either served to the caller or merely logged, depending on whether any image data had
+// already been transmitted or not.
+func serveImage(r io.Reader, w io.Writer, resizeCmd *exec.Cmd) (int64, error) {
+	bytesWritten, err := io.Copy(w, r)
 	if err != nil {
-		if bytesWritten <= 0 {
-			helper.Fail500(w, req, err)
-		} else {
-			helper.LogErrorWithFields(req, err, *logFields)
+		return bytesWritten, err
+	}
+
+	if resizeCmd != nil {
+		if err = resizeCmd.Wait(); err != nil {
+			return bytesWritten, err
 		}
 	}
+
+	return bytesWritten, nil
+}
+
+func handleFailedCommand(w http.ResponseWriter, req *http.Request, bytesWritten int64, err error, logFields *log.Fields) {
+	if bytesWritten <= 0 {
+		helper.Fail500(w, req, err)
+	} else {
+		helper.LogErrorWithFields(req, err, *logFields)
+	}
 }
 
 func (r *resizer) unpackParameters(paramsData string) (*resizeParams, error) {
@@ -161,14 +180,18 @@ func (r *resizer) unpackParameters(paramsData string) (*resizeParams, error) {
 	}
 
 	if params.ContentType == "" {
-		return nil, fmt.Errorf("ImageResizer: Image MIME type must be set")
+		return nil, fmt.Errorf("ImageResizer: ContentType must be set")
 	}
 
 	return &params, nil
 }
 
 // Attempts to rescale the given image data, or in case of errors, falls back to the original image.
-func tryResizeImage(req *http.Request, r io.Reader, errorWriter io.Writer, params *resizeParams) (io.Reader, *exec.Cmd, error) {
+func tryResizeImage(req *http.Request, r io.Reader, errorWriter io.Writer, params *resizeParams, fileSize int64) (io.Reader, *exec.Cmd, error) {
+	if fileSize > maxAllowedFileSizeBytes {
+		return r, nil, fmt.Errorf("ImageResizer: %db exceeds maximum file size of %db", fileSize, maxAllowedFileSizeBytes)
+	}
+
 	if !numScalerProcs.tryIncrement() {
 		return r, nil, fmt.Errorf("ImageResizer: too many running scaler processes")
 	}
@@ -179,35 +202,22 @@ func tryResizeImage(req *http.Request, r io.Reader, errorWriter io.Writer, param
 		numScalerProcs.decrement()
 	}()
 
-	width := params.Width
-	gmFileSpec := determineFilePrefix(params.ContentType)
-	if gmFileSpec == "" {
-		return r, nil, fmt.Errorf("ImageResizer: unexpected MIME type: %s", params.ContentType)
-	}
-
-	resizeCmd, resizedImageReader, err := startResizeImageCommand(ctx, r, errorWriter, width, gmFileSpec)
+	resizeCmd, resizedImageReader, err := startResizeImageCommand(ctx, r, errorWriter, params)
 	if err != nil {
-		return r, nil, fmt.Errorf("ImageResizer: failed forking into graphicsmagick")
+		return r, nil, fmt.Errorf("ImageResizer: failed forking into scaler process: %w", err)
 	}
 	return resizedImageReader, resizeCmd, nil
 }
 
-func determineFilePrefix(contentType string) string {
-	switch contentType {
-	case "image/png":
-		return "png:"
-	case "image/jpeg":
-		return "jpg:"
-	default:
-		return ""
-	}
-}
-
-func startResizeImageCommand(ctx context.Context, imageReader io.Reader, errorWriter io.Writer, width uint, gmFileSpec string) (*exec.Cmd, io.ReadCloser, error) {
-	cmd := exec.CommandContext(ctx, "gm", "convert", "-resize", fmt.Sprintf("%dx", width), gmFileSpec+"-", "-")
+func startResizeImageCommand(ctx context.Context, imageReader io.Reader, errorWriter io.Writer, params *resizeParams) (*exec.Cmd, io.ReadCloser, error) {
+	cmd := exec.CommandContext(ctx, "gitlab-resize-image")
 	cmd.Stdin = imageReader
 	cmd.Stderr = errorWriter
 	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
+	cmd.Env = []string{
+		"GL_RESIZE_IMAGE_WIDTH=" + strconv.Itoa(int(params.Width)),
+		"GL_RESIZE_IMAGE_CONTENT_TYPE=" + params.ContentType,
+	}
 
 	stdout, err := cmd.StdoutPipe()
 	if err != nil {
@@ -227,13 +237,13 @@ func isURL(location string) bool {
 
 func openSourceImage(location string) (io.ReadCloser, int64, error) {
 	if isURL(location) {
-		return openFromUrl(location)
+		return openFromURL(location)
 	}
 
 	return openFromFile(location)
 }
 
-func openFromUrl(location string) (io.ReadCloser, int64, error) {
+func openFromURL(location string) (io.ReadCloser, int64, error) {
 	res, err := httpClient.Get(location)
 	if err != nil {
 		return nil, 0, err

internal/imageresizer/image_resizer_test.go

 package imageresizer
 
 import (
+	"bytes"
 	"encoding/base64"
 	"encoding/json"
 	"net/http"
 	"os"
 	"testing"
 
+	"gitlab.com/gitlab-org/labkit/log"
+
 	"github.com/stretchr/testify/require"
+
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper"
 )
 
 var r = resizer{}
 
+func TestMain(m *testing.M) {
+	if err := testhelper.BuildExecutables(); err != nil {
+		log.WithError(err).Fatal()
+	}
+
+	os.Exit(m.Run())
+}
+
 func TestUnpackParametersReturnsParamsInstanceForValidInput(t *testing.T) {
 	inParams := resizeParams{Location: "/path/to/img", Width: 64, ContentType: "image/png"}
 
@@ -37,19 +50,13 @@ func TestUnpackParametersReturnsErrorWhenContentTypeBlank(t *testing.T) {
 	require.Error(t, err, "expected error when ContentType is blank")
 }
 
-func TestDetermineFilePrefixFromMimeType(t *testing.T) {
-	require.Equal(t, "png:", determineFilePrefix("image/png"))
-	require.Equal(t, "jpg:", determineFilePrefix("image/jpeg"))
-	require.Equal(t, "", determineFilePrefix("unsupported"))
-}
-
 func TestTryResizeImageSuccess(t *testing.T) {
 	inParams := resizeParams{Location: "/path/to/img", Width: 64, ContentType: "image/png"}
 	inFile := testImage(t)
 	req, err := http.NewRequest("GET", "/foo", nil)
 	require.NoError(t, err)
 
-	reader, cmd, err := tryResizeImage(req, inFile, os.Stderr, &inParams)
+	reader, cmd, err := tryResizeImage(req, inFile, os.Stderr, &inParams, maxAllowedFileSizeBytes)
 
 	require.NoError(t, err)
 	require.NotNil(t, cmd)
@@ -57,29 +64,40 @@ func TestTryResizeImageSuccess(t *testing.T) {
 	require.NotEqual(t, inFile, reader)
 }
 
-func TestTryResizeImageFailsOverToOriginalImageWhenContentTypeNotSupported(t *testing.T) {
-	inParams := resizeParams{Location: "/path/to/img", Width: 64, ContentType: "not supported"}
+func TestTryResizeImageSkipsResizeWhenSourceImageTooLarge(t *testing.T) {
+	inParams := resizeParams{Location: "/path/to/img", Width: 64, ContentType: "image/png"}
 	inFile := testImage(t)
 	req, err := http.NewRequest("GET", "/foo", nil)
 	require.NoError(t, err)
 
-	reader, cmd, err := tryResizeImage(req, inFile, os.Stderr, &inParams)
+	reader, cmd, err := tryResizeImage(req, inFile, os.Stderr, &inParams, maxAllowedFileSizeBytes+1)
 
 	require.Error(t, err)
 	require.Nil(t, cmd)
-	require.Equal(t, inFile, reader)
+	require.Equal(t, inFile, reader, "Expected output streams to match")
 }
 
-func TestGraphicsMagickFailsWhenContentTypeNotMatchingFileContents(t *testing.T) {
+func TestTryResizeImageFailsWhenContentTypeNotMatchingFileContents(t *testing.T) {
 	inParams := resizeParams{Location: "/path/to/img", Width: 64, ContentType: "image/jpeg"}
-	inFile := testImage(t) // this is PNG file; gm should fail fast in this case
+	inFile := testImage(t) // this is a PNG file; the image scaler should fail fast in this case
 	req, err := http.NewRequest("GET", "/foo", nil)
 	require.NoError(t, err)
 
-	_, cmd, err := tryResizeImage(req, inFile, os.Stderr, &inParams)
+	_, cmd, err := tryResizeImage(req, inFile, os.Stderr, &inParams, maxAllowedFileSizeBytes)
+
+	require.NoError(t, err)
+	require.Error(t, cmd.Wait(), "Expected to fail due to content-type mismatch")
+}
+
+func TestServeImage(t *testing.T) {
+	inFile := testImage(t)
+	var writer bytes.Buffer
+
+	bytesWritten, err := serveImage(inFile, &writer, nil)
 
 	require.NoError(t, err)
-	require.Error(t, cmd.Wait())
+	require.Greater(t, bytesWritten, int64(0))
+	require.Equal(t, int64(len(writer.Bytes())), bytesWritten)
 }
 
 // The Rails applications sends a Base64 encoded JSON string carrying

internal/testhelper/testhelper.go

@@ -74,7 +74,7 @@ func TestServerWithHandler(url *regexp.Regexp, handler http.HandlerFunc) *httpte
 	}))
 }
 
-var workhorseExecutables = []string{"gitlab-workhorse", "gitlab-zip-cat", "gitlab-zip-metadata"}
+var workhorseExecutables = []string{"gitlab-workhorse", "gitlab-zip-cat", "gitlab-zip-metadata", "gitlab-resize-image"}
 
 func BuildExecutables() error {
 	rootDir := RootDir()

main_test.go

@@ -377,7 +377,7 @@ func TestImageResizing(t *testing.T) {
 
 	resp, body, err := doSendDataRequest(resourcePath, "send-scaled-img", jsonParams)
 	require.NoError(t, err, "send resize request")
-	require.Equal(t, 200, resp.StatusCode, "GET %q: status code", resourcePath)
+	require.Equal(t, 200, resp.StatusCode, "GET %q: body: %s", resourcePath, body)
 
 	img, err := png.Decode(bytes.NewReader(body))
 	require.NoError(t, err, "decode resized image")