Merge branch '298-set-img-resizing-const-via-env-vars' into 'master'

Allow configure Img Resizing params

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

changelogs/unreleased/298-set-img-resizing-const-via-env-vars.yml

+---
+title: Allow configure image resizing params
+merge_request: 615
+author:
+type: changed

config.toml.example

@@ -11,3 +11,7 @@ URL = "unix:/home/git/gitlab/redis/redis.socket"
 [object_store.azurerm]
   azure_storage_account_name = "YOUR ACCOUNT NAME"
   azure_storage_access_key = "YOUR ACCOUNT KEY"
+
+[image_resizer]
+  max_scaler_procs = 100
+  max_filesize = 250000

internal/config/config.go

@@ -80,6 +80,11 @@ type RedisConfig struct {
 	MaxActive       *int
 }
 
+type ImageResizerConfig struct {
+	MaxScalerProcs uint32 `toml:"max_scaler_procs"`
+	MaxFilesize    uint64 `toml:"max_filesize"`
+}
+
 type Config struct {
 	Redis                    *RedisConfig              `toml:"redis"`
 	Backend                  *url.URL                  `toml:"-"`
@@ -97,11 +102,23 @@ type Config struct {
 	ObjectStorageConfig      ObjectStorageConfig       `toml:"-"`
 	ObjectStorageCredentials *ObjectStorageCredentials `toml:"object_storage"`
 	PropagateCorrelationID   bool                      `toml:"-"`
+	ImageResizerConfig       *ImageResizerConfig       `toml:"image_resizer"`
+}
+
+const (
+	DefaultImageResizerMaxScalerProcs = 100
+	DefaultImageResizerMaxFilesize    = 250 * 1000 // 250kB
+)
+
+var DefaultImageResizerConfig = &ImageResizerConfig{
+	MaxScalerProcs: DefaultImageResizerMaxScalerProcs,
+	MaxFilesize:    DefaultImageResizerMaxFilesize,
 }
 
 // LoadConfig from a file
 func LoadConfig(filename string) (*Config, error) {
-	cfg := &Config{}
+	cfg := &Config{ImageResizerConfig: DefaultImageResizerConfig}
+
 	if _, err := toml.DecodeFile(filename, cfg); err != nil {
 		return nil, err
 	}

internal/config/config_test.go

@@ -80,6 +80,42 @@ func TestRegisterGoCloudURLOpeners(t *testing.T) {
 	require.Equal(t, []string{"azblob"}, cfg.ObjectStorageConfig.URLMux.BucketSchemes())
 }
 
+func TestLoadDefaultConfig(t *testing.T) {
+	config := ``
+
+	tmpFile, cfg := loadTempConfig(t, config)
+	defer os.Remove(tmpFile.Name())
+
+	expected := Config{
+		ImageResizerConfig: &ImageResizerConfig{
+			MaxScalerProcs: DefaultImageResizerMaxScalerProcs,
+			MaxFilesize:    DefaultImageResizerMaxFilesize,
+		},
+	}
+
+	require.Equal(t, expected, *cfg)
+}
+
+func TestLoadImageResizerConfig(t *testing.T) {
+	config := `
+[image_resizer]
+max_scaler_procs = 200
+max_filesize = 350000
+`
+
+	tmpFile, cfg := loadTempConfig(t, config)
+	defer os.Remove(tmpFile.Name())
+
+	require.NotNil(t, cfg.ImageResizerConfig, "Expected image resizer config")
+
+	expected := ImageResizerConfig{
+		MaxScalerProcs: 200,
+		MaxFilesize:    350000,
+	}
+
+	require.Equal(t, expected, *cfg.ImageResizerConfig)
+}
+
 func loadTempConfig(t *testing.T, config string) (f *os.File, cfg *Config) {
 	tmpFile, err := ioutil.TempFile(os.TempDir(), "test-")
 	require.NoError(t, err)

internal/imageresizer/image_resizer.go

@@ -14,6 +14,8 @@ import (
 	"syscall"
 	"time"
 
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/config"
+
 	"github.com/prometheus/client_golang/prometheus"
 
 	"gitlab.com/gitlab-org/labkit/correlation"
@@ -25,9 +27,10 @@ import (
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/senddata"
 )
 
-type resizer struct{ senddata.Prefix }
-
-var SendScaledImage = &resizer{"send-scaled-img:"}
+type Resizer struct {
+	config.Config
+	senddata.Prefix
+}
 
 type resizeParams struct {
 	Location    string
@@ -41,11 +44,6 @@ type processCounter struct {
 
 var numScalerProcs processCounter
 
-const (
-	maxImageScalerProcs     = 100
-	maxAllowedFileSizeBytes = 250 * 1000 // 250kB
-)
-
 var envInjector = tracing.NewEnvInjector()
 
 // Images might be located remotely in object storage, in which case we need to stream
@@ -123,9 +121,13 @@ func init() {
 	prometheus.MustRegister(imageResizeDurations)
 }
 
+func NewResizer(cfg config.Config) *Resizer {
+	return &Resizer{cfg, "send-scaled-img:"}
+}
+
 // 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) {
+func (r *Resizer) Inject(w http.ResponseWriter, req *http.Request, paramsData string) {
 	start := time.Now()
 	logger := log.ContextLogger(req.Context())
 	params, err := r.unpackParameters(paramsData)
@@ -156,7 +158,7 @@ func (r *resizer) Inject(w http.ResponseWriter, req *http.Request, paramsData st
 
 	// 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)
+	imageReader, resizeCmd, err := tryResizeImage(req, sourceImageReader, logger.Writer(), params, fileSize, r.Config.ImageResizerConfig)
 	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))
@@ -204,7 +206,7 @@ func handleFailedCommand(w http.ResponseWriter, req *http.Request, bytesWritten
 	}
 }
 
-func (r *resizer) unpackParameters(paramsData string) (*resizeParams, error) {
+func (r *Resizer) unpackParameters(paramsData string) (*resizeParams, error) {
 	var params resizeParams
 	if err := r.Unpack(&params, paramsData); err != nil {
 		return nil, err
@@ -222,12 +224,12 @@ func (r *resizer) unpackParameters(paramsData string) (*resizeParams, error) {
 }
 
 // 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, 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)
+func tryResizeImage(req *http.Request, r io.Reader, errorWriter io.Writer, params *resizeParams, fileSize int64, cfg *config.ImageResizerConfig) (io.Reader, *exec.Cmd, error) {
+	if fileSize > int64(cfg.MaxFilesize) {
+		return r, nil, fmt.Errorf("ImageResizer: %db exceeds maximum file size of %db", fileSize, cfg.MaxFilesize)
 	}
 
-	if !numScalerProcs.tryIncrement() {
+	if !numScalerProcs.tryIncrement(int32(cfg.MaxScalerProcs)) {
 		return r, nil, fmt.Errorf("ImageResizer: too many running scaler processes")
 	}
 
@@ -312,8 +314,8 @@ func openFromFile(location string) (io.ReadCloser, int64, error) {
 
 // Only allow more scaling requests if we haven't yet reached the maximum
 // allowed number of concurrent scaler processes
-func (c *processCounter) tryIncrement() bool {
-	if p := atomic.AddInt32(&c.n, 1); p > maxImageScalerProcs {
+func (c *processCounter) tryIncrement(maxScalerProcs int32) bool {
+	if p := atomic.AddInt32(&c.n, 1); p > maxScalerProcs {
 		c.decrement()
 		imageResizeConcurrencyLimitExceeds.Inc()
 

internal/imageresizer/image_resizer_test.go

@@ -8,6 +8,8 @@ import (
 	"os"
 	"testing"
 
+	"gitlab.com/gitlab-org/gitlab-workhorse/internal/config"
+
 	"gitlab.com/gitlab-org/labkit/log"
 
 	"github.com/stretchr/testify/require"
@@ -15,7 +17,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/testhelper"
 )
 
-var r = resizer{}
+var r = Resizer{}
 
 func TestMain(m *testing.M) {
 	if err := testhelper.BuildExecutables(); err != nil {
@@ -56,7 +58,14 @@ func TestTryResizeImageSuccess(t *testing.T) {
 	req, err := http.NewRequest("GET", "/foo", nil)
 	require.NoError(t, err)
 
-	reader, cmd, err := tryResizeImage(req, inFile, os.Stderr, &inParams, maxAllowedFileSizeBytes)
+	reader, cmd, err := tryResizeImage(
+		req,
+		inFile,
+		os.Stderr,
+		&inParams,
+		int64(config.DefaultImageResizerConfig.MaxFilesize),
+		config.DefaultImageResizerConfig,
+	)
 
 	require.NoError(t, err)
 	require.NotNil(t, cmd)
@@ -70,7 +79,14 @@ func TestTryResizeImageSkipsResizeWhenSourceImageTooLarge(t *testing.T) {
 	req, err := http.NewRequest("GET", "/foo", nil)
 	require.NoError(t, err)
 
-	reader, cmd, err := tryResizeImage(req, inFile, os.Stderr, &inParams, maxAllowedFileSizeBytes+1)
+	reader, cmd, err := tryResizeImage(
+		req,
+		inFile,
+		os.Stderr,
+		&inParams,
+		int64(config.DefaultImageResizerConfig.MaxFilesize)+1,
+		config.DefaultImageResizerConfig,
+	)
 
 	require.Error(t, err)
 	require.Nil(t, cmd)
@@ -83,7 +99,14 @@ func TestTryResizeImageFailsWhenContentTypeNotMatchingFileContents(t *testing.T)
 	req, err := http.NewRequest("GET", "/foo", nil)
 	require.NoError(t, err)
 
-	_, cmd, err := tryResizeImage(req, inFile, os.Stderr, &inParams, maxAllowedFileSizeBytes)
+	_, cmd, err := tryResizeImage(
+		req,
+		inFile,
+		os.Stderr,
+		&inParams,
+		int64(config.DefaultImageResizerConfig.MaxFilesize),
+		config.DefaultImageResizerConfig,
+	)
 
 	require.NoError(t, err)
 	require.Error(t, cmd.Wait(), "Expected to fail due to content-type mismatch")

internal/upstream/routes.go

@@ -142,7 +142,7 @@ func (ro *routeEntry) isMatch(cleanedPath string, req *http.Request) bool {
 	return ok
 }
 
-func buildProxy(backend *url.URL, version string, rt http.RoundTripper) http.Handler {
+func buildProxy(backend *url.URL, version string, rt http.RoundTripper, cfg config.Config) http.Handler {
 	proxier := proxypkg.NewProxy(backend, version, rt)
 
 	return senddata.SendData(
@@ -154,7 +154,7 @@ func buildProxy(backend *url.URL, version string, rt http.RoundTripper) http.Han
 		git.SendSnapshot,
 		artifacts.SendEntry,
 		sendurl.SendURL,
-		imageresizer.SendScaledImage,
+		imageresizer.NewResizer(cfg),
 	)
 }
 
@@ -170,11 +170,11 @@ func (u *upstream) configureRoutes() {
 	)
 
 	static := &staticpages.Static{DocumentRoot: u.DocumentRoot}
-	proxy := buildProxy(u.Backend, u.Version, u.RoundTripper)
+	proxy := buildProxy(u.Backend, u.Version, u.RoundTripper, u.Config)
 	cableProxy := proxypkg.NewProxy(u.CableBackend, u.Version, u.CableRoundTripper)
 
 	signingTripper := secret.NewRoundTripper(u.RoundTripper, u.Version)
-	signingProxy := buildProxy(u.Backend, u.Version, signingTripper)
+	signingProxy := buildProxy(u.Backend, u.Version, signingTripper, u.Config)
 
 	preparers := createUploadPreparers(u.Config)
 	uploadPath := path.Join(u.DocumentRoot, "uploads/tmp")

main.go

@@ -157,6 +157,7 @@ func main() {
 		APIQueueTimeout:          *apiQueueTimeout,
 		APICILongPollingDuration: *apiCiLongPollingDuration,
 		PropagateCorrelationID:   *propagateCorrelationID,
+		ImageResizerConfig:       config.DefaultImageResizerConfig,
 	}
 
 	if *configFile != "" {
@@ -167,6 +168,7 @@ func main() {
 
 		cfg.Redis = cfgFromFile.Redis
 		cfg.ObjectStorageCredentials = cfgFromFile.ObjectStorageCredentials
+		cfg.ImageResizerConfig = cfgFromFile.ImageResizerConfig
 
 		if cfg.Redis != nil {
 			redis.Configure(cfg.Redis, redis.DefaultDialFunc)

main_test.go

@@ -645,9 +645,10 @@ func testAuthServer(t *testing.T, url *regexp.Regexp, params url.Values, code in
 
 func newUpstreamConfig(authBackend string) *config.Config {
 	return &config.Config{
-		Version:      "123",
-		DocumentRoot: testDocumentRoot,
-		Backend:      helper.URLMustParse(authBackend),
+		Version:            "123",
+		DocumentRoot:       testDocumentRoot,
+		Backend:            helper.URLMustParse(authBackend),
+		ImageResizerConfig: config.DefaultImageResizerConfig,
 	}
 }