runservice api: improve response handling

* Command: use ErrBadRequest
* Always return a json message also on error. For internal errors return a
generic "internal server error" message to not leak the real internal error to
clients
* Return 201 Created on resource creation
* Return 204 No Content on resource deletion and other action with no json
output

internal/services/runservice/scheduler/api/api.go

@@ -31,6 +31,7 @@ import (
 	"github.com/sorintlab/agola/internal/services/runservice/scheduler/readdb"
 	"github.com/sorintlab/agola/internal/services/runservice/scheduler/store"
 	"github.com/sorintlab/agola/internal/services/runservice/types"
+	"github.com/sorintlab/agola/internal/util"
 	"github.com/sorintlab/agola/internal/wal"
 
 	"github.com/gorilla/mux"
@@ -38,6 +39,58 @@ import (
 	"go.uber.org/zap"
 )
 
+type ErrorResponse struct {
+	Message string `json:"message"`
+}
+
+func ErrorResponseFromError(err error) *ErrorResponse {
+	if util.IsErrBadRequest(err) {
+		return &ErrorResponse{Message: err.Error()}
+	}
+
+	// on generic error return an generic message to not leak the real error
+	return &ErrorResponse{Message: "internal server error"}
+}
+
+func httpError(w http.ResponseWriter, err error) bool {
+	if err != nil {
+		response := ErrorResponseFromError(err)
+		resj, merr := json.Marshal(response)
+		if merr != nil {
+			w.WriteHeader(http.StatusInternalServerError)
+			return true
+		}
+		if util.IsErrBadRequest(err) {
+			w.WriteHeader(http.StatusBadRequest)
+			w.Write(resj)
+		} else {
+			w.WriteHeader(http.StatusInternalServerError)
+			w.Write(resj)
+		}
+		return true
+	}
+
+	return false
+}
+
+func httpResponse(w http.ResponseWriter, code int, res interface{}) error {
+	w.Header().Set("Content-Type", "application/json")
+
+	if res != nil {
+		resj, err := json.Marshal(res)
+		if err != nil {
+			httpError(w, err)
+			return err
+		}
+		w.WriteHeader(code)
+		_, err = w.Write(resj)
+		return err
+	}
+
+	w.WriteHeader(code)
+	return nil
+}
+
 type LogsHandler struct {
 	log *zap.SugaredLogger
 	e   *etcd.Store
@@ -267,9 +320,8 @@ func (h *ChangeGroupsUpdateTokensHandler) ServeHTTP(w http.ResponseWriter, r *ht
 		return
 	}
 
-	if err := json.NewEncoder(w).Encode(cgts); err != nil {
+	if err := httpResponse(w, http.StatusOK, cgts); err != nil {
-		http.Error(w, err.Error(), http.StatusInternalServerError)
+		h.log.Errorf("err: %+v", err)
-		return
 	}
 }
 
@@ -327,9 +379,8 @@ func (h *RunHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		RunConfig: rc,
 	}
 
-	if err := json.NewEncoder(w).Encode(res); err != nil {
+	if err := httpResponse(w, http.StatusOK, res); err != nil {
-		http.Error(w, err.Error(), http.StatusInternalServerError)
+		h.log.Errorf("err: %+v", err)
-		return
 	}
 }
 
@@ -412,13 +463,12 @@ func (h *RunsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	resp := &GetRunsResponse{
+	res := &GetRunsResponse{
 		Runs:                    runs,
 		ChangeGroupsUpdateToken: cgts,
 	}
-	if err := json.NewEncoder(w).Encode(resp); err != nil {
+	if err := httpResponse(w, http.StatusOK, res); err != nil {
-		http.Error(w, err.Error(), http.StatusInternalServerError)
+		h.log.Errorf("err: %+v", err)
-		return
 	}
 }
 
@@ -474,7 +524,7 @@ func (h *RunCreateHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	rb, err := h.ch.CreateRun(ctx, creq)
 	if err != nil {
 		h.log.Errorf("err: %+v", err)
-		http.Error(w, err.Error(), http.StatusBadRequest)
+		httpError(w, err)
 		return
 	}
 
@@ -483,9 +533,8 @@ func (h *RunCreateHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		RunConfig: rb.Rc,
 	}
 
-	if err := json.NewEncoder(w).Encode(res); err != nil {
+	if err := httpResponse(w, http.StatusCreated, res); err != nil {
-		http.Error(w, err.Error(), http.StatusInternalServerError)
+		h.log.Errorf("err: %+v", err)
-		return
 	}
 }
 
@@ -536,7 +585,8 @@ func (h *RunActionsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 			ChangeGroupsUpdateToken: req.ChangeGroupsUpdateToken,
 		}
 		if err := h.ch.ChangeRunPhase(ctx, creq); err != nil {
-			http.Error(w, err.Error(), http.StatusBadRequest)
+			h.log.Errorf("err: %+v", err)
+			httpError(w, err)
 			return
 		}
 	case RunActionTypeStop:
@@ -545,7 +595,8 @@ func (h *RunActionsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 			ChangeGroupsUpdateToken: req.ChangeGroupsUpdateToken,
 		}
 		if err := h.ch.StopRun(ctx, creq); err != nil {
-			http.Error(w, err.Error(), http.StatusBadRequest)
+			h.log.Errorf("err: %+v", err)
+			httpError(w, err)
 			return
 		}
 	default:
@@ -600,7 +651,8 @@ func (h *RunTaskActionsHandler) ServeHTTP(w http.ResponseWriter, r *http.Request
 			ChangeGroupsUpdateToken: req.ChangeGroupsUpdateToken,
 		}
 		if err := h.ch.ApproveRunTask(ctx, creq); err != nil {
-			http.Error(w, err.Error(), http.StatusBadRequest)
+			h.log.Errorf("err: %+v", err)
+			httpError(w, err)
 			return
 		}
 	default:

internal/services/runservice/scheduler/command/command.go

@@ -152,10 +152,10 @@ func (s *CommandHandler) newRun(ctx context.Context, req *RunCreateRequest) (*ty
 	var run *types.Run
 
 	if req.Group == "" {
-		return nil, errors.Errorf("run group is empty")
+		return nil, util.NewErrBadRequest(errors.Errorf("run group is empty"))
 	}
 	if !path.IsAbs(req.Group) {
-		return nil, errors.Errorf("run group %q must be an absolute path", req.Group)
+		return nil, util.NewErrBadRequest(errors.Errorf("run group %q must be an absolute path", req.Group))
 	}
 
 	// generate a new run sequence that will be the same for the run, runconfig and rundata
@@ -166,14 +166,14 @@ func (s *CommandHandler) newRun(ctx context.Context, req *RunCreateRequest) (*ty
 	id := seq.String()
 
 	if err := runconfig.CheckRunConfig(rc); err != nil {
-		return nil, err
+		return nil, util.NewErrBadRequest(err)
 	}
 	// set the run config ID
 	rc.ID = id
 
 	// generate tasks levels
 	if err := runconfig.GenTasksLevels(rc); err != nil {
-		return nil, err
+		return nil, util.NewErrBadRequest(err)
 	}
 
 	rd := &types.RunData{
@@ -185,7 +185,7 @@ func (s *CommandHandler) newRun(ctx context.Context, req *RunCreateRequest) (*ty
 
 	run, err = s.genRun(ctx, rc, rd)
 	if err != nil {
-		return nil, err
+		return nil, util.NewErrBadRequest(err)
 	}
 	s.log.Debugf("created run: %s", util.Dump(run))
 
@@ -208,14 +208,14 @@ func (s *CommandHandler) recreateRun(ctx context.Context, req *RunCreateRequest)
 	s.log.Infof("creating run from existing run")
 	rc, err := store.LTSGetRunConfig(s.wal, req.RunID)
 	if err != nil {
-		return nil, errors.Wrapf(err, "runconfig %q doens't exist", req.RunID)
+		return nil, util.NewErrBadRequest(errors.Wrapf(err, "runconfig %q doens't exist", req.RunID))
 	}
 	// update the run config ID
 	rc.ID = id
 
 	rd, err := store.LTSGetRunData(s.wal, req.RunID)
 	if err != nil {
-		return nil, errors.Wrapf(err, "rundata %q doens't exist", req.RunID)
+		return nil, util.NewErrBadRequest(errors.Wrapf(err, "rundata %q doens't exist", req.RunID))
 	}
 	// update the run data ID
 	rd.ID = id
@@ -225,16 +225,16 @@ func (s *CommandHandler) recreateRun(ctx context.Context, req *RunCreateRequest)
 		return nil, err
 	}
 	if run == nil {
-		return nil, errors.Wrapf(err, "run %q doens't exist", req.RunID)
+		return nil, util.NewErrBadRequest(errors.Wrapf(err, "run %q doens't exist", req.RunID))
 	}
 
 	if req.FromStart {
 		if canRestart, reason := run.CanRestartFromScratch(); !canRestart {
-			return nil, errors.Errorf("run cannot be restarted: %s", reason)
+			return nil, util.NewErrBadRequest(errors.Errorf("run cannot be restarted: %s", reason))
 		}
 	} else {
 		if canRestart, reason := run.CanRestartFromFailedTasks(); !canRestart {
-			return nil, errors.Errorf("run cannot be restarted: %s", reason)
+			return nil, util.NewErrBadRequest(errors.Errorf("run cannot be restarted: %s", reason))
 		}
 	}
 
@@ -414,15 +414,15 @@ func (s *CommandHandler) ApproveRunTask(ctx context.Context, req *RunTaskApprove
 
 	task, ok := r.RunTasks[req.TaskID]
 	if !ok {
-		return errors.Errorf("run %q doesn't have task %q", r.ID, req.TaskID)
+		return util.NewErrBadRequest(errors.Errorf("run %q doesn't have task %q", r.ID, req.TaskID))
 	}
 
 	if !task.WaitingApproval {
-		return errors.Errorf("run %q, task %q is not in waiting approval state", r.ID, req.TaskID)
+		return util.NewErrBadRequest(errors.Errorf("run %q, task %q is not in waiting approval state", r.ID, req.TaskID))
 	}
 
 	if task.Approved {
-		return errors.Errorf("run %q, task %q is already approved", r.ID, req.TaskID)
+		return util.NewErrBadRequest(errors.Errorf("run %q, task %q is already approved", r.ID, req.TaskID))
 	}
 
 	task.WaitingApproval = false