From ee92dec5f7b8c6834baf63c4d51c77a7f63d6bed Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 22 Sep 2022 18:24:47 -0400 Subject: [PATCH] fix: reduce request transaction lifetime PostgreSQL row and table locks are held until the end of the transaction. Previously we were using a single transaction for the entire lifetime of the request. One of the first things we did with that transaction was update the access key and user record associated with the request. That update would block any other requests from that user until the first request finished. For short lived requests that was fine, but longer requests (anything making HTTP requests to an IDP, or blocking and waiting for updates) would prevent the user from making concurrent requests. This commit fixes the problem by committing the "middleware" transaction first, then starting a new transaction for the request handler. The middleware transaction should be short lived, because does only a few database operations and returns. It doesn't block or make external requests. --- internal/server/config.go | 6 +----- internal/server/middleware.go | 39 +++++++++++++++++++++++++++++++---- internal/server/routes.go | 31 ++++++++++++++++------------ 3 files changed, 54 insertions(+), 22 deletions(-) diff --git a/internal/server/config.go b/internal/server/config.go index 9b97ce2237..d8bf8f1f43 100644 --- a/internal/server/config.go +++ b/internal/server/config.go @@ -610,11 +610,7 @@ func (s Server) loadConfig(config Config) error { if err != nil { return err } - defer func() { - if err := tx.Rollback(); err != nil { - logging.L.Error().Err(err).Msg("failed to rollback database transaction") - } - }() + defer logError(tx.Rollback, "failed to rollback loadConfig transaction") tx = tx.WithOrgID(org.ID) if config.DefaultOrganizationDomain != org.Domain { diff --git a/internal/server/middleware.go b/internal/server/middleware.go index 5f9df30521..74985d2314 100644 --- a/internal/server/middleware.go +++ b/internal/server/middleware.go @@ -75,7 +75,13 @@ func handleInfraDestinationHeader(c *gin.Context) error { // gin.Context. // See validateRequestOrganization for a related function used for unauthenticated // routes. -func authenticateRequest(c *gin.Context, tx *data.Transaction, srv *Server) error { +func authenticateRequest(c *gin.Context, srv *Server) error { + tx, err := srv.db.Begin(c.Request.Context()) + if err != nil { + return err + } + defer logError(tx.Rollback, "failed to rollback middleware transaction") + authned, err := requireAccessKey(c, tx, srv) if err != nil { return err @@ -86,6 +92,7 @@ func authenticateRequest(c *gin.Context, tx *data.Transaction, srv *Server) erro return internal.ErrBadRequest } + // TODO: move to caller rCtx := access.RequestContext{ Request: c.Request, DBTxn: tx.WithOrgID(authned.Organization.ID), @@ -96,6 +103,10 @@ func authenticateRequest(c *gin.Context, tx *data.Transaction, srv *Server) erro if err := handleInfraDestinationHeader(c); err != nil { return err } + if err := tx.Commit(); err != nil { + return err + } + rCtx.DBTxn = nil return nil } @@ -131,7 +142,13 @@ func validateOrgMatchesRequest(req *http.Request, tx data.GormTxn, accessKeyOrg // // validateRequestOrganization is also responsible for adding RequestContext to the // gin.Context. -func validateRequestOrganization(c *gin.Context, tx *data.Transaction, srv *Server) error { +func validateRequestOrganization(c *gin.Context, srv *Server) error { + tx, err := srv.db.Begin(c.Request.Context()) + if err != nil { + return err + } + defer logError(tx.Rollback, "failed to rollback middleware transaction") + // ignore errors, access key is not required authned, _ := requireAccessKey(c, tx, srv) @@ -152,12 +169,15 @@ func validateRequestOrganization(c *gin.Context, tx *data.Transaction, srv *Serv } if org != nil { authned.Organization = org - tx = tx.WithOrgID(authned.Organization.ID) } + if err := tx.Commit(); err != nil { + return err + } + + // TODO: move to caller rCtx := access.RequestContext{ Request: c.Request, - DBTxn: tx, Authenticated: authned, } c.Set(access.RequestContextKey, rCtx) @@ -296,3 +316,14 @@ func reqBearerToken(c *gin.Context, opts Options) (string, error) { return bearer, nil } + +// logError calls fn and writes a log line at the warning level if the error is +// not nil. The log level is a warning because the error is not handled, which +// generally indicates the problem is not a critical error. +// logError accepts a function instead of an error so that it can be used with +// defer. +func logError(fn func() error, msg string) { + if err := fn(); err != nil { + logging.L.Warn().Err(err).Msg(msg) + } +} diff --git a/internal/server/routes.go b/internal/server/routes.go index 92886be96e..f656a7ccec 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -13,6 +13,7 @@ import ( "github.com/infrahq/infra/api" "github.com/infrahq/infra/internal" + "github.com/infrahq/infra/internal/access" "github.com/infrahq/infra/internal/logging" "github.com/infrahq/infra/internal/server/redis" "github.com/infrahq/infra/internal/validate" @@ -197,26 +198,18 @@ func wrapRoute[Req, Res any](a *API, routeID routeIdentifier, route route[Req, R } } - tx, err := a.server.db.Begin(c.Request.Context()) - if err != nil { - return err - } - defer func() { - if err := tx.Rollback(); err != nil { - logging.L.Error().Err(err).Msg("failed to rollback database transaction") - } - }() - + var err error if route.noAuthentication { - err = validateRequestOrganization(c, tx, a.server) + err = validateRequestOrganization(c, a.server) } else { - err = authenticateRequest(c, tx, a.server) + err = authenticateRequest(c, a.server) } if err != nil { return err } - org := getRequestContext(c).Authenticated.Organization + rCtx := getRequestContext(c) + org := rCtx.Authenticated.Organization if !route.noOrgRequired { if org == nil { return internal.ErrBadRequest @@ -235,6 +228,18 @@ func wrapRoute[Req, Res any](a *API, routeID routeIdentifier, route route[Req, R return err } + tx, err := a.server.db.Begin(c.Request.Context()) + if err != nil { + return err + } + defer logError(tx.Rollback, "failed to rollback request handler transaction") + + if org := rCtx.Authenticated.Organization; org != nil { + tx = tx.WithOrgID(org.ID) + } + rCtx.DBTxn = tx + c.Set(access.RequestContextKey, rCtx) + resp, err := route.handler(c, req) if err != nil { return err