diff --git a/internal/services/runservice/scheduler/api/api.go b/internal/services/runservice/scheduler/api/api.go index 994cca7..11c8b17 100644 --- a/internal/services/runservice/scheduler/api/api.go +++ b/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 { - http.Error(w, err.Error(), http.StatusInternalServerError) - return + if err := httpResponse(w, http.StatusOK, cgts); err != nil { + h.log.Errorf("err: %+v", err) } } @@ -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 { - http.Error(w, err.Error(), http.StatusInternalServerError) - return + if err := httpResponse(w, http.StatusOK, res); err != nil { + h.log.Errorf("err: %+v", err) } } @@ -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 { - http.Error(w, err.Error(), http.StatusInternalServerError) - return + if err := httpResponse(w, http.StatusOK, res); err != nil { + h.log.Errorf("err: %+v", err) } } @@ -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 { - http.Error(w, err.Error(), http.StatusInternalServerError) - return + if err := httpResponse(w, http.StatusCreated, res); err != nil { + h.log.Errorf("err: %+v", err) } } @@ -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: diff --git a/internal/services/runservice/scheduler/command/command.go b/internal/services/runservice/scheduler/command/command.go index aaea957..fd0f596 100644 --- a/internal/services/runservice/scheduler/command/command.go +++ b/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