fix: log max config size exceeded (#663)

Fixes #652

Makefile

@@ -3,7 +3,7 @@ GIT_COMMIT = $(shell git rev-parse HEAD)
 RANDOM_SUFFIX := $(shell cat /dev/urandom | tr -dc 'a-z0-9' | head -c5)
 IMAGE_NAME = leg100/otfd
 IMAGE_TAG ?= $(VERSION)-$(RANDOM_SUFFIX)
-DBSTRING=postgres://postgres:password@localhost/otf
+DBSTRING=postgres:///otf
 LD_FLAGS = " \
     -s -w \
 	-X 'github.com/leg100/otf/internal.Version=$(VERSION)' \

internal/configversion/service.go

@@ -48,6 +48,7 @@ func NewService(opts Options) *Service {
 	svc.db = &pgdb{opts.DB}
 	svc.cache = opts.Cache
 	svc.tfeapi = &tfe{
+		Logger:        opts.Logger,
 		tfeClient:     &svc,
 		Signer:        opts.Signer,
 		Responder:     opts.Responder,

internal/configversion/tfe.go

@@ -10,6 +10,7 @@ import (
 	"reflect"
 	"time"
 
+	"github.com/go-logr/logr"
 	"github.com/gorilla/mux"
 	"github.com/leg100/otf/internal"
 	otfhttp "github.com/leg100/otf/internal/http"
@@ -23,6 +24,7 @@ import (
 type tfe struct {
 	tfeClient
 
+	logr.Logger
 	*surl.Signer
 	*tfeapi.Responder
 
@@ -155,6 +157,11 @@ func (a *tfe) uploadConfigurationVersion() http.HandlerFunc {
 					Code:    422,
 					Message: fmt.Sprintf("config exceeds maximum size (%d bytes)", a.maxConfigSize),
 				})
+				// Terraform CLI only informs the user that an HTTP 422 response
+				// was received, and they aren't informed that their config
+				// exceeds the max size. To help them diagnose the cause, the
+				// error is logged on the server side too.
+				a.Error(err, "uploaded config exceeds max size", "bytes", a.maxConfigSize)
 			} else {
 				tfeapi.Error(w, err)
 			}

internal/configversion/tfe_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	otfhttp "github.com/leg100/otf/internal/http"
+	"github.com/leg100/otf/internal/logr"
 
 	"github.com/gorilla/mux"
 	"github.com/stretchr/testify/assert"
@@ -16,6 +17,7 @@ import (
 
 func TestUploadConfigurationVersion(t *testing.T) {
 	api := &tfe{
+		Logger: logr.Discard(),
 		// only permit upto 100 byte uploads
 		maxConfigSize: 100,
 		tfeClient:     &fakeConfigService{},

internal/errors.go

@@ -31,10 +31,6 @@ var (
 	// ErrEmptyValue is returned when a value is set to an empty string
 	ErrEmptyValue = errors.New("value cannot be empty")
 
-	// ErrUploadTooLarge is returned when a user attempts to upload data that
-	// is too large.
-	ErrUploadTooLarge = errors.New("upload is too large")
-
 	// ErrTimeout is returned when a request exceeds a timeout.
 	ErrTimeout = errors.New("request timed out")
 
@@ -58,26 +54,6 @@ var (
 	ErrInvalidRepo = errors.New("repository path is invalid")
 )
 
-// Workspace errors
-var (
-	ErrWorkspaceAlreadyLocked         = errors.New("workspace already locked")
-	ErrWorkspaceLockedByDifferentUser = errors.New("workspace locked by different user")
-	ErrWorkspaceLockedByRun           = errors.New("workspace is locked by Run")
-	ErrWorkspaceAlreadyUnlocked       = errors.New("workspace already unlocked")
-	ErrWorkspaceUnlockDenied          = errors.New("unauthorized to unlock workspace")
-	ErrWorkspaceInvalidLock           = errors.New("invalid workspace lock")
-	ErrUnsupportedTerraformVersion    = errors.New("unsupported terraform version")
-)
-
-// Run errors
-var (
-	ErrRunDiscardNotAllowed     = errors.New("run was not paused for confirmation or priority; discard not allowed")
-	ErrRunCancelNotAllowed      = errors.New("run was not planning or applying; cancel not allowed")
-	ErrRunForceCancelNotAllowed = errors.New("run was not planning or applying, has not been canceled non-forcefully, or the cool-off period has not yet passed")
-	//
-	ErrPhaseAlreadyStarted = errors.New("phase already started")
-)
-
 type (
 	HTTPError struct {
 		Code    int

internal/run/errors.go

@@ -0,0 +1,11 @@
+package run
+
+import "errors"
+
+var (
+	ErrRunDiscardNotAllowed     = errors.New("run was not paused for confirmation or priority; discard not allowed")
+	ErrRunCancelNotAllowed      = errors.New("run was not planning or applying; cancel not allowed")
+	ErrRunForceCancelNotAllowed = errors.New("run was not planning or applying, has not been canceled non-forcefully, or the cool-off period has not yet passed")
+	//
+	ErrPhaseAlreadyStarted = errors.New("phase already started")
+)

internal/run/run.go

@@ -284,7 +284,7 @@ func (r *Run) Phase() internal.PhaseType {
 // Discard updates the state of a run to reflect it having been discarded.
 func (r *Run) Discard() error {
 	if !r.Discardable() {
-		return internal.ErrRunDiscardNotAllowed
+		return ErrRunDiscardNotAllowed
 	}
 	r.updateStatus(RunDiscarded, nil)
 
@@ -321,11 +321,11 @@ func (r *Run) Cancel(isUser, force bool) error {
 	if force {
 		if isUser {
 			if !r.ForceCancelable() {
-				return internal.ErrRunForceCancelNotAllowed
+				return ErrRunForceCancelNotAllowed
 			}
 		} else {
 			// only a user can forceably cancel a run.
-			return internal.ErrRunForceCancelNotAllowed
+			return ErrRunForceCancelNotAllowed
 		}
 	}
 	var signal bool
@@ -357,7 +357,7 @@ func (r *Run) Cancel(isUser, force bool) error {
 	if signal {
 		if r.CancelSignaledAt != nil {
 			// cannot send cancel signal more than once.
-			return internal.ErrRunCancelNotAllowed
+			return ErrRunCancelNotAllowed
 		}
 		// set timestamp to indicate signal is to be sent, but do not set
 		// status to RunCanceled yet.
@@ -492,7 +492,7 @@ func (r *Run) Start() error {
 		r.updateStatus(RunApplying, nil)
 		r.Apply.UpdateStatus(PhaseRunning)
 	case RunPlanning, RunApplying:
-		return internal.ErrPhaseAlreadyStarted
+		return ErrPhaseAlreadyStarted
 	default:
 		return ErrInvalidRunStateTransition
 	}

internal/run/run_test.go

@@ -215,14 +215,14 @@ func TestRun_States(t *testing.T) {
 		err := run.Cancel(true, false)
 		require.NoError(t, err)
 		err = run.Cancel(true, false)
-		assert.Equal(t, internal.ErrRunCancelNotAllowed, err)
+		assert.Equal(t, ErrRunCancelNotAllowed, err)
 	})
 
 	t.Run("cannot force cancel a run when no previous attempt has been made to cancel run gracefully", func(t *testing.T) {
 		run := newTestRun(ctx, CreateOptions{})
 		run.Status = RunPlanning
 		err := run.Cancel(true, true)
-		assert.Equal(t, internal.ErrRunForceCancelNotAllowed, err)
+		assert.Equal(t, ErrRunForceCancelNotAllowed, err)
 	})
 
 	t.Run("force cancel run when graceful cancel has already been attempted and cool off period has elapsed", func(t *testing.T) {
@@ -240,7 +240,7 @@ func TestRun_States(t *testing.T) {
 		run := newTestRun(ctx, CreateOptions{})
 		run.Status = RunPlanning
 		err := run.Cancel(false, true)
-		assert.Equal(t, internal.ErrRunForceCancelNotAllowed, err)
+		assert.Equal(t, ErrRunForceCancelNotAllowed, err)
 	})
 }
 

internal/run/service.go

@@ -291,7 +291,7 @@ func (s *Service) StartPhase(ctx context.Context, runID string, phase internal.P
 		// whereas the other agents receive this error which is a legitimate
 		// error condition and not something that should be reported to the
 		// user.
-		if !errors.Is(err, internal.ErrPhaseAlreadyStarted) {
+		if !errors.Is(err, ErrPhaseAlreadyStarted) {
 			s.Error(err, "starting "+string(phase), "id", runID)
 		}
 		return nil, err

internal/scheduler/queue.go

@@ -6,7 +6,6 @@ import (
 	"fmt"
 
 	"github.com/go-logr/logr"
-	"github.com/leg100/otf/internal"
 	otfrun "github.com/leg100/otf/internal/run"
 	"github.com/leg100/otf/internal/workspace"
 )
@@ -144,7 +143,7 @@ func (q *queue) scheduleRun(ctx context.Context, run *otfrun.Run) error {
 
 	ws, err := q.Lock(ctx, q.ws.ID, &run.ID)
 	if err != nil {
-		if errors.Is(err, internal.ErrWorkspaceAlreadyLocked) {
+		if errors.Is(err, workspace.ErrWorkspaceAlreadyLocked) {
 			// User has locked workspace in the small window of time between
 			// getting the lock above and attempting to enqueue plan.
 			q.V(0).Info("workspace locked by user; cannot schedule run", "run", run.ID)

internal/tfeapi/errors.go

@@ -9,18 +9,11 @@ import (
 )
 
 var codes = map[error]int{
-	internal.ErrResourceNotFound:         http.StatusNotFound,
-	internal.ErrAccessNotPermitted:       http.StatusForbidden,
-	internal.ErrUploadTooLarge:           http.StatusUnprocessableEntity,
-	internal.ErrInvalidTerraformVersion:  http.StatusUnprocessableEntity,
-	internal.ErrResourceAlreadyExists:    http.StatusConflict,
-	internal.ErrWorkspaceAlreadyLocked:   http.StatusConflict,
-	internal.ErrWorkspaceAlreadyUnlocked: http.StatusConflict,
-	internal.ErrWorkspaceLockedByRun:     http.StatusConflict,
-	internal.ErrRunDiscardNotAllowed:     http.StatusConflict,
-	internal.ErrRunCancelNotAllowed:      http.StatusConflict,
-	internal.ErrRunForceCancelNotAllowed: http.StatusConflict,
-	internal.ErrConflict:                 http.StatusConflict,
+	internal.ErrResourceNotFound:        http.StatusNotFound,
+	internal.ErrAccessNotPermitted:      http.StatusForbidden,
+	internal.ErrInvalidTerraformVersion: http.StatusUnprocessableEntity,
+	internal.ErrResourceAlreadyExists:   http.StatusConflict,
+	internal.ErrConflict:                http.StatusConflict,
 }
 
 func lookupHTTPCode(err error) int {

internal/workspace/errors.go

@@ -0,0 +1,21 @@
+package workspace
+
+import "errors"
+
+var (
+	ErrWorkspaceAlreadyLocked         = errors.New("workspace already locked")
+	ErrWorkspaceLockedByDifferentUser = errors.New("workspace locked by different user")
+	ErrWorkspaceLockedByRun           = errors.New("workspace is locked by Run")
+	ErrWorkspaceAlreadyUnlocked       = errors.New("workspace already unlocked")
+	ErrWorkspaceUnlockDenied          = errors.New("unauthorized to unlock workspace")
+	ErrWorkspaceInvalidLock           = errors.New("invalid workspace lock")
+	ErrUnsupportedTerraformVersion    = errors.New("unsupported terraform version")
+
+	ErrTagsRegexAndTriggerPatterns     = errors.New("cannot specify both tags-regex and trigger-patterns")
+	ErrTagsRegexAndAlwaysTrigger       = errors.New("cannot specify both tags-regex and always-trigger")
+	ErrTriggerPatternsAndAlwaysTrigger = errors.New("cannot specify both trigger-patterns and always-trigger")
+	ErrInvalidTriggerPattern           = errors.New("invalid trigger glob pattern")
+	ErrInvalidTagsRegex                = errors.New("invalid vcs tags regular expression")
+	ErrAgentExecutionModeWithoutPool   = errors.New("agent execution mode requires agent pool ID")
+	ErrNonAgentExecutionModeWithPool   = errors.New("agent pool ID can only be specified with agent execution mode")
+)

internal/workspace/lock.go

@@ -54,13 +54,13 @@ func (ws *Workspace) Enlock(id string, kind LockKind) error {
 		ws.Lock.id = id
 		return nil
 	}
-	return internal.ErrWorkspaceAlreadyLocked
+	return ErrWorkspaceAlreadyLocked
 }
 
 // Unlock the workspace.
 func (ws *Workspace) Unlock(id string, kind LockKind, force bool) error {
 	if ws.Lock == nil {
-		return internal.ErrWorkspaceAlreadyUnlocked
+		return ErrWorkspaceAlreadyUnlocked
 	}
 	if force {
 		ws.Lock = nil
@@ -79,9 +79,9 @@ func (ws *Workspace) Unlock(id string, kind LockKind, force bool) error {
 
 	// determine error message to return
 	if ws.Lock.LockKind == RunLock {
-		return internal.ErrWorkspaceLockedByRun
+		return ErrWorkspaceLockedByRun
 	}
-	return internal.ErrWorkspaceLockedByDifferentUser
+	return ErrWorkspaceLockedByDifferentUser
 }
 
 // lockButtonHelper helps the UI determine the button to display for

internal/workspace/lock_db.go

@@ -4,7 +4,6 @@ import (
 	"context"
 
 	"github.com/jackc/pgtype"
-	"github.com/leg100/otf/internal"
 	"github.com/leg100/otf/internal/sql"
 	"github.com/leg100/otf/internal/sql/pggen"
 )
@@ -39,7 +38,7 @@ func (db *pgdb) toggleLock(ctx context.Context, workspaceID string, togglefn fun
 			params.Username = pgtype.Text{String: ws.Lock.id, Status: pgtype.Present}
 			params.RunID = pgtype.Text{Status: pgtype.Null}
 		} else {
-			return internal.ErrWorkspaceInvalidLock
+			return ErrWorkspaceInvalidLock
 		}
 		_, err = q.UpdateWorkspaceLockByID(ctx, params)
 		if err != nil {

internal/workspace/lock_test.go

@@ -25,14 +25,14 @@ func TestWorkspace_Lock(t *testing.T) {
 	t.Run("user cannot lock a locked workspace", func(t *testing.T) {
 		ws := &Workspace{Lock: &Lock{id: "run-123", LockKind: RunLock}}
 		err := ws.Enlock("janitor", UserLock)
-		require.Equal(t, internal.ErrWorkspaceAlreadyLocked, err)
+		require.Equal(t, ErrWorkspaceAlreadyLocked, err)
 	})
 }
 
 func TestWorkspace_Unlock(t *testing.T) {
 	t.Run("cannot unlock workspace already unlocked", func(t *testing.T) {
 		err := (&Workspace{}).Unlock("janitor", UserLock, false)
-		require.Equal(t, internal.ErrWorkspaceAlreadyUnlocked, err)
+		require.Equal(t, ErrWorkspaceAlreadyUnlocked, err)
 	})
 	t.Run("user can unlock their own lock", func(t *testing.T) {
 		ws := &Workspace{Lock: &Lock{id: "janitor", LockKind: UserLock}}
@@ -43,7 +43,7 @@ func TestWorkspace_Unlock(t *testing.T) {
 	t.Run("user cannot unlock another user's lock", func(t *testing.T) {
 		ws := &Workspace{Lock: &Lock{id: "janitor", LockKind: UserLock}}
 		err := ws.Unlock("burglar", UserLock, false)
-		require.Equal(t, internal.ErrWorkspaceLockedByDifferentUser, err)
+		require.Equal(t, ErrWorkspaceLockedByDifferentUser, err)
 	})
 	t.Run("user can unlock a lock by force", func(t *testing.T) {
 		ws := &Workspace{Lock: &Lock{id: "janitor", LockKind: UserLock}}

internal/workspace/tfe.go

@@ -248,7 +248,11 @@ func (a *tfe) lockWorkspace(w http.ResponseWriter, r *http.Request) {
 
 	ws, err := a.Lock(r.Context(), id, nil)
 	if err != nil {
-		tfeapi.Error(w, err)
+		if errors.Is(err, ErrWorkspaceAlreadyLocked) {
+			http.Error(w, "", http.StatusConflict)
+		} else {
+			tfeapi.Error(w, err)
+		}
 		return
 	}
 
@@ -269,6 +273,35 @@ func (a *tfe) forceUnlockWorkspace(w http.ResponseWriter, r *http.Request) {
 	a.unlock(w, r, true)
 }
 
+func (a *tfe) unlock(w http.ResponseWriter, r *http.Request, force bool) {
+	id, err := decode.Param("workspace_id", r)
+	if err != nil {
+		tfeapi.Error(w, err)
+		return
+	}
+
+	ws, err := a.Unlock(r.Context(), id, nil, force)
+	if err != nil {
+		if errors.Is(err, ErrWorkspaceAlreadyUnlocked) || errors.Is(err, ErrWorkspaceLockedByRun) {
+			tfeapi.Error(w, &internal.HTTPError{
+				Code:    http.StatusConflict,
+				Message: err.Error(),
+			})
+		} else {
+			tfeapi.Error(w, err)
+		}
+		return
+	}
+
+	converted, err := a.convert(ws, r)
+	if err != nil {
+		tfeapi.Error(w, err)
+		return
+	}
+
+	a.Respond(w, r, converted, http.StatusOK)
+}
+
 func (a *tfe) deleteWorkspace(w http.ResponseWriter, r *http.Request) {
 	workspaceID, err := decode.Param("workspace_id", r)
 	if err != nil {
@@ -376,28 +409,6 @@ func (a *tfe) updateWorkspace(w http.ResponseWriter, r *http.Request, workspaceI
 	a.Respond(w, r, converted, http.StatusOK)
 }
 
-func (a *tfe) unlock(w http.ResponseWriter, r *http.Request, force bool) {
-	id, err := decode.Param("workspace_id", r)
-	if err != nil {
-		tfeapi.Error(w, err)
-		return
-	}
-
-	ws, err := a.Unlock(r.Context(), id, nil, force)
-	if err != nil {
-		tfeapi.Error(w, err)
-		return
-	}
-
-	converted, err := a.convert(ws, r)
-	if err != nil {
-		tfeapi.Error(w, err)
-		return
-	}
-
-	a.Respond(w, r, converted, http.StatusOK)
-}
-
 func (a *tfe) convert(from *Workspace, r *http.Request) (*types.Workspace, error) {
 	subject, err := internal.SubjectFromContext(r.Context())
 	if err != nil {