diff --git a/.gitignore b/.gitignore index b3738ac9f0e6bc45910561ff4c9b5964c0f97728..d190115b867ededfd5f55c024e5e4677fa23e594 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,7 @@ testdata/data testdata/scratch testdata/public /gitlab-workhorse +/gitlab-resize-image /gitlab-zip-cat /gitlab-zip-metadata /_build diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 6a4379dbb552f06684069f72fedebb26bdc17021..69ab266bad263b006b9be946bf71fef4d60acde5 100644 --- a/.gitlab-ci.yml +++ b/.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: diff --git a/Makefile b/Makefile index 5945a4e67a7610c521579d22cb316bdb103a0e5b..140edfd35fd7328717a711e743cb23422387c68f 100644 --- a/Makefile +++ b/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/ diff --git a/README.md b/README.md index 7a20a390e3f9a71b403f487b0e7d3995af5dd2e2..aa2a1a2bc78d3bf7058f17dc4cdec8dbe31951fe 100644 --- a/README.md +++ b/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 diff --git a/changelogs/unreleased/mk-golang-only-image-scaler.yml b/changelogs/unreleased/mk-golang-only-image-scaler.yml new file mode 100644 index 0000000000000000000000000000000000000000..3433e28cf3d2469e6e8e9d61e3ec0bfb9bd61520 --- /dev/null +++ b/changelogs/unreleased/mk-golang-only-image-scaler.yml @@ -0,0 +1,5 @@ +--- +title: Switch image scaler to a Go-only solution +merge_request: 603 +author: +type: changed diff --git a/cmd/gitlab-resize-image/main.go b/cmd/gitlab-resize-image/main.go new file mode 100644 index 0000000000000000000000000000000000000000..43471c9ff432789270a244cfe69649e8ec85a87b --- /dev/null +++ b/cmd/gitlab-resize-image/main.go @@ -0,0 +1,45 @@ +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) +} diff --git a/go.mod b/go.mod index f49168e6ca56d40918852da0980218adaa27ac14..38b2e43336ab5a3d92b9b88eb5da3a4c27e830cc 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index 28707bcc4553e9709cca278c71d19fd7aad1cdbe..f4acf5e04f793e7d027876deea250e92fd17a4fe 100644 --- a/go.sum +++ b/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= diff --git a/internal/imageresizer/image_resizer.go b/internal/imageresizer/image_resizer.go index 9041496e7fa998cf0318e9c6484fdd562dec56a9..545fc8c8c8c3b7dd101cdd8ae4fed3e4ca699739 100644 --- a/internal/imageresizer/image_resizer.go +++ b/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 ¶ms, 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 diff --git a/internal/imageresizer/image_resizer_test.go b/internal/imageresizer/image_resizer_test.go index 9c15566146dc0efe10dc2aa7e1365ab824bafa29..12792d654a3974cb861abdc8f14aeac648168f51 100644 --- a/internal/imageresizer/image_resizer_test.go +++ b/internal/imageresizer/image_resizer_test.go @@ -1,17 +1,30 @@ 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 diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index 32f4fcfaebbb36f758a137dfc6f53562d904ac1b..40097bd453a94eb5336c2b340a492512033cb17a 100644 --- a/internal/testhelper/testhelper.go +++ b/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() diff --git a/main_test.go b/main_test.go index bc4b8e2408272500a6a8b7e8a4ab847fa4b74a2b..bd00b1bd56541cf30e5066e823ef1469c6215f62 100644 --- a/main_test.go +++ b/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")