From 288d2fbead9f8e4f4f378cdcece067215f8e2caf Mon Sep 17 00:00:00 2001 From: Michael Vogt Date: Mon, 26 Aug 2024 20:58:20 +0200 Subject: [PATCH] worker: remove `api.BasePath` and replace with MakeHTTPErrorHandler() The `api.BasePath` is package global but the way it is used is that multiple clients can (potentially) have different BasePath and the way it is written right now could lead to confusing bugs. This commit tweaks the `api` package so that `BasePath` does not need to be set. The only place that this is needed is for the `api.APIError()` which can just get the basePath as an argument. With that we also need a helper to construct the api error handler from the static HTTPErrorHandler to a dynamic MakeHTTPErrorHandler() that will return a handler for the given basePath. This should make https://github.com/osbuild/osbuild-composer/pull/4328 obsolete. --- internal/worker/api/api.go | 3 -- internal/worker/api/errors.go | 74 ++++++++++++++++++----------------- internal/worker/client.go | 8 +--- internal/worker/server.go | 26 ++++++------ 4 files changed, 52 insertions(+), 59 deletions(-) diff --git a/internal/worker/api/api.go b/internal/worker/api/api.go index 8ec115b999..4a7ca039c2 100644 --- a/internal/worker/api/api.go +++ b/internal/worker/api/api.go @@ -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" diff --git a/internal/worker/api/errors.go b/internal/worker/api/errors.go index 157dea08cd..d6f812bacf 100644 --- a/internal/worker/api/errors.go +++ b/internal/worker/api/errors.go @@ -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) @@ -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", }, @@ -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) } diff --git a/internal/worker/client.go b/internal/worker/client.go index 6da714d96e..c5d4869224 100644 --- a/internal/worker/client.go +++ b/internal/worker/client.go @@ -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) } @@ -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) } diff --git a/internal/worker/server.go b/internal/worker/server.go index 665e835d5e..d749a02b91 100644 --- a/internal/worker/server.go +++ b/internal/worker/server.go @@ -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 @@ -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{ @@ -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 } @@ -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", }, @@ -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) @@ -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", }) @@ -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, @@ -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", }, @@ -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", }, @@ -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", }) @@ -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", },