Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

worker: remove api.BasePath and replace with MakeHTTPErrorHandler() #4330

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

worker: remove api.BasePath and replace with MakeHTTPErrorHandler() #4330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions internal/worker/api/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
//go:generate go run github.com/deepmap/oapi-codegen/cmd/oapi-codegen -package=api -generate types,server,spec -o api.gen.go openapi.yml

package api

// default basepath, can be overwritten
var BasePath = "/api/worker/v1"
74 changes: 38 additions & 36 deletions internal/worker/api/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ func HTTPErrorWithInternal(code ServiceErrorCode, internalErr error) error {

// Convert a ServiceErrorCode into an Error as defined in openapi.v2.yml
// serviceError is optional, prevents multiple find() calls
func APIError(code ServiceErrorCode, serviceError *serviceError, c echo.Context) *Error {
func APIError(basePath string, code ServiceErrorCode, serviceError *serviceError, c echo.Context) *Error {
se := serviceError
if se == nil {
se = find(code)
Expand All @@ -131,7 +131,7 @@ func APIError(code ServiceErrorCode, serviceError *serviceError, c echo.Context)

return &Error{
ObjectReference: ObjectReference{
Href: fmt.Sprintf("%s/errors/%d", BasePath, se.code),
Href: fmt.Sprintf("%s/errors/%d", basePath, se.code),
Id: fmt.Sprintf("%d", se.code),
Kind: "Error",
},
Expand All @@ -156,43 +156,45 @@ func apiErrorFromEchoError(echoError *echo.HTTPError) ServiceErrorCode {
}

// Convert an echo error into an AOC compliant one so we send a correct json error response
func HTTPErrorHandler(echoError error, c echo.Context) {
doResponse := func(code ServiceErrorCode, c echo.Context, internal error) {
if !c.Response().Committed {
var err error
sec := find(code)
apiErr := APIError(code, sec, c)

if sec.httpStatus == http.StatusInternalServerError {
c.Logger().Errorf("Internal server error. Internal: %v, Code: %s, OperationId: %s",
internal, apiErr.Code, apiErr.OperationId)
} else {
c.Logger().Infof("Code: %s, OperationId: %s, Internal: %v",
apiErr.Code, apiErr.OperationId, internal)
}

if c.Request().Method == http.MethodHead {
err = c.NoContent(sec.httpStatus)
} else {
err = c.JSON(sec.httpStatus, apiErr)
}
if err != nil {
c.Logger().Errorf("Failed to return error response: %v", err)
func MakeHTTPErrorHandler(basePath string) func(error, echo.Context) {
return func(echoError error, c echo.Context) {
doResponse := func(code ServiceErrorCode, c echo.Context, internal error) {
if !c.Response().Committed {
var err error
sec := find(code)
apiErr := APIError(basePath, code, sec, c)

if sec.httpStatus == http.StatusInternalServerError {
c.Logger().Errorf("Internal server error. Internal: %v, Code: %s, OperationId: %s",
internal, apiErr.Code, apiErr.OperationId)
} else {
c.Logger().Infof("Code: %s, OperationId: %s, Internal: %v",
apiErr.Code, apiErr.OperationId, internal)
}

if c.Request().Method == http.MethodHead {
err = c.NoContent(sec.httpStatus)
} else {
err = c.JSON(sec.httpStatus, apiErr)
}
if err != nil {
c.Logger().Errorf("Failed to return error response: %v", err)
}
}
}
}

he, ok := echoError.(*echo.HTTPError)
if !ok {
doResponse(ErrorNotHTTPError, c, echoError)
return
}
he, ok := echoError.(*echo.HTTPError)
if !ok {
doResponse(ErrorNotHTTPError, c, echoError)
return
}

sec, ok := he.Message.(ServiceErrorCode)
if !ok {
// No service code was set, so Echo threw this error
doResponse(apiErrorFromEchoError(he), c, he.Internal)
return
sec, ok := he.Message.(ServiceErrorCode)
if !ok {
// No service code was set, so Echo threw this error
doResponse(apiErrorFromEchoError(he), c, he.Internal)
return
}
doResponse(sec, c, he.Internal)
}
doResponse(sec, c, he.Internal)
}
8 changes: 2 additions & 6 deletions internal/worker/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,7 @@ func NewClient(conf ClientConfig) (*Client, error) {
return nil, err
}

api.BasePath = conf.BasePath

serverURL, err = serverURL.Parse(api.BasePath + "/")
serverURL, err = serverURL.Parse(conf.BasePath + "/")
if err != nil {
panic(err)
}
Expand Down Expand Up @@ -136,9 +134,7 @@ func NewClientUnix(conf ClientConfig) *Client {
panic(err)
}

api.BasePath = conf.BasePath

serverURL, err = serverURL.Parse(api.BasePath + "/")
serverURL, err = serverURL.Parse(conf.BasePath + "/")
if err != nil {
panic(err)
}
Expand Down
26 changes: 12 additions & 14 deletions internal/worker/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,6 @@ func NewServer(logger *log.Logger, jobs jobqueue.JobQueue, config Config) *Serve
s.config.WorkerWatchFreq = time.Second * 300
}

api.BasePath = config.BasePath

go s.WatchHeartbeats()
go s.WatchWorkers()
return s
Expand All @@ -103,7 +101,7 @@ func (s *Server) Handler() http.Handler {
e.Logger = common.Logger()

// log errors returned from handlers
e.HTTPErrorHandler = api.HTTPErrorHandler
e.HTTPErrorHandler = api.MakeHTTPErrorHandler(s.config.BasePath)
e.Use(middleware.Recover())
e.Pre(common.OperationIDMiddleware)
handler := apiHandlers{
Expand All @@ -117,7 +115,7 @@ func (s *Server) Handler() http.Handler {
mws = append(mws, auth.TenantChannelMiddleware(s.config.TenantProviderFields, api.HTTPError(api.ErrorTenantNotFound)))
}
mws = append(mws, prometheus.HTTPDurationMiddleware(prometheus.WorkerSubsystem))
api.RegisterHandlers(e.Group(api.BasePath, mws...), &handler)
api.RegisterHandlers(e.Group(s.config.BasePath, mws...), &handler)

return e
}
Expand Down Expand Up @@ -863,7 +861,7 @@ func (h *apiHandlers) GetOpenapi(ctx echo.Context) error {
func (h *apiHandlers) GetStatus(ctx echo.Context) error {
return ctx.JSON(http.StatusOK, &api.StatusResponse{
ObjectReference: api.ObjectReference{
Href: fmt.Sprintf("%s/status", api.BasePath),
Href: fmt.Sprintf("%s/status", h.server.config.BasePath),
Id: "status",
Kind: "Status",
},
Expand All @@ -877,7 +875,7 @@ func (h *apiHandlers) GetError(ctx echo.Context, id string) error {
return api.HTTPErrorWithInternal(api.ErrorInvalidErrorId, err)
}

apiError := api.APIError(api.ServiceErrorCode(errorId), nil, ctx)
apiError := api.APIError(h.server.config.BasePath, api.ServiceErrorCode(errorId), nil, ctx)
// If the service error wasn't found, it's a 404 in this instance
if apiError.Id == fmt.Sprintf("%d", api.ErrorServiceErrorNotFound) {
return api.HTTPError(api.ErrorErrorNotFound)
Expand Down Expand Up @@ -916,7 +914,7 @@ func (h *apiHandlers) RequestJob(ctx echo.Context) error {
if err != nil {
if err == jobqueue.ErrDequeueTimeout {
return ctx.JSON(http.StatusNoContent, api.ObjectReference{
Href: fmt.Sprintf("%s/jobs", api.BasePath),
Href: fmt.Sprintf("%s/jobs", h.server.config.BasePath),
Id: uuid.Nil.String(),
Kind: "RequestJob",
})
Expand All @@ -938,12 +936,12 @@ func (h *apiHandlers) RequestJob(ctx echo.Context) error {

response := api.RequestJobResponse{
ObjectReference: api.ObjectReference{
Href: fmt.Sprintf("%s/jobs", api.BasePath),
Href: fmt.Sprintf("%s/jobs", h.server.config.BasePath),
Id: jobId.String(),
Kind: "RequestJob",
},
Location: fmt.Sprintf("%s/jobs/%v", api.BasePath, jobToken),
ArtifactLocation: fmt.Sprintf("%s/jobs/%v/artifacts/", api.BasePath, jobToken),
Location: fmt.Sprintf("%s/jobs/%v", h.server.config.BasePath, jobToken),
ArtifactLocation: fmt.Sprintf("%s/jobs/%v/artifacts/", h.server.config.BasePath, jobToken),
Type: jobType,
Args: respArgs,
DynamicArgs: respDynArgs,
Expand All @@ -970,7 +968,7 @@ func (h *apiHandlers) GetJob(ctx echo.Context, tokenstr string) error {
if jobId == uuid.Nil {
return ctx.JSON(http.StatusOK, api.GetJobResponse{
ObjectReference: api.ObjectReference{
Href: fmt.Sprintf("%s/jobs/%v", api.BasePath, token),
Href: fmt.Sprintf("%s/jobs/%v", h.server.config.BasePath, token),
Id: token.String(),
Kind: "JobStatus",
},
Expand All @@ -987,7 +985,7 @@ func (h *apiHandlers) GetJob(ctx echo.Context, tokenstr string) error {

return ctx.JSON(http.StatusOK, api.GetJobResponse{
ObjectReference: api.ObjectReference{
Href: fmt.Sprintf("%s/jobs/%v", api.BasePath, token),
Href: fmt.Sprintf("%s/jobs/%v", h.server.config.BasePath, token),
Id: token.String(),
Kind: "JobStatus",
},
Expand Down Expand Up @@ -1020,7 +1018,7 @@ func (h *apiHandlers) UpdateJob(ctx echo.Context, idstr string) error {
}

return ctx.JSON(http.StatusOK, api.UpdateJobResponse{
Href: fmt.Sprintf("%s/jobs/%v", api.BasePath, token),
Href: fmt.Sprintf("%s/jobs/%v", h.server.config.BasePath, token),
Id: token.String(),
Kind: "UpdateJobResponse",
})
Expand Down Expand Up @@ -1077,7 +1075,7 @@ func (h *apiHandlers) PostWorkers(ctx echo.Context) error {

return ctx.JSON(http.StatusCreated, api.PostWorkersResponse{
ObjectReference: api.ObjectReference{
Href: fmt.Sprintf("%s/workers", api.BasePath),
Href: fmt.Sprintf("%s/workers", h.server.config.BasePath),
Id: workerID.String(),
Kind: "WorkerID",
},
Expand Down
Loading