Multiple LFS improvements (#10667)

* Add more logging in the LFS server Adds more logging in the LFS server and stops sending internal server error information to the client * Add LFS Lock cursor implementation * Simplify Claims in LFS and remove the float64 casts Signed-off-by: Andrew Thornton <[email protected]> Co-authored-by: Lauris BH <[email protected]>
go-gitea · Mar 9, 2020 · 9269b7f · 9269b7f
1 parent 3fc4f36
commit 9269b7f
Show file tree

Hide file tree

Showing 7 changed files with 165 additions and 52 deletions.
diff --git a/cmd/serv.go b/cmd/serv.go
@@ -18,6 +18,7 @@ import (
  "time"
 
  "code.gitea.io/gitea/models"
+ "code.gitea.io/gitea/modules/lfs"
  "code.gitea.io/gitea/modules/log"
  "code.gitea.io/gitea/modules/pprof"
  "code.gitea.io/gitea/modules/private"
@@ -213,12 +214,14 @@ func runServ(c *cli.Context) error {
  url := fmt.Sprintf("%s%s/%s.git/info/lfs", setting.AppURL, url.PathEscape(results.OwnerName), url.PathEscape(results.RepoName))
 
  now := time.Now()
- claims := jwt.MapClaims{
- "repo": results.RepoID,
- "op": lfsVerb,
- "exp": now.Add(setting.LFS.HTTPAuthExpiry).Unix(),
- "nbf": now.Unix(),
- "user": results.UserID,
+ claims := lfs.Claims{
+ StandardClaims: jwt.StandardClaims{
+ ExpiresAt: now.Add(setting.LFS.HTTPAuthExpiry).Unix(),
+ NotBefore: now.Unix(),
+ },
+ RepoID: results.RepoID,
+ Op: lfsVerb,
+ UserID: results.UserID,
  }
  token := jwt.NewWithClaims(jwt.SigningMethodHS256, claims)
 

diff --git a/custom/conf/app.ini.sample b/custom/conf/app.ini.sample
@@ -313,6 +313,8 @@ LFS_JWT_SECRET =
 LFS_HTTP_AUTH_EXPIRY = 20m
 ; Maximum allowed LFS file size in bytes (Set to 0 for no limit).
 LFS_MAX_FILE_SIZE = 0
+; Maximum number of locks returned per page
+LFS_LOCKS_PAGING_NUM = 50
 ; Allow graceful restarts using SIGHUP to fork
 ALLOW_GRACEFUL_RESTARTS = true
 ; After a restart the parent will finish ongoing requests before

diff --git a/docs/content/doc/advanced/config-cheat-sheet.en-us.md b/docs/content/doc/advanced/config-cheat-sheet.en-us.md
@@ -193,6 +193,7 @@ Values containing `#` or `;` must be quoted using `` ` `` or `"""`.
 - `LFS_JWT_SECRET`: **\<empty\>**: LFS authentication secret, change this a unique string.
 - `LFS_HTTP_AUTH_EXPIRY`: **20m**: LFS authentication validity period in time.Duration, pushes taking longer than this may fail.
 - `LFS_MAX_FILE_SIZE`: **0**: Maximum allowed LFS file size in bytes (Set to 0 for no limit).
+- `LFS_LOCK_PAGING_NUM`: **50**: Maximum number of LFS Locks returned per page.
 - `REDIRECT_OTHER_PORT`: **false**: If true and `PROTOCOL` is https, allows redirecting http requests on `PORT_TO_REDIRECT` to the https port Gitea listens on.
 - `PORT_TO_REDIRECT`: **80**: Port for the http redirection service to listen on. Used when `REDIRECT_OTHER_PORT` is true.
 - `ENABLE_LETSENCRYPT`: **false**: If enabled you must set `DOMAIN` to valid internet facing domain (ensure DNS is set and port 80 is accessible by letsencrypt validation server).

diff --git a/modules/lfs/content_store.go b/modules/lfs/content_store.go
@@ -9,6 +9,7 @@ import (
  "path/filepath"
 
  "code.gitea.io/gitea/models"
+ "code.gitea.io/gitea/modules/log"
 )
 
 var (
@@ -28,10 +29,14 @@ func (s *ContentStore) Get(meta *models.LFSMetaObject, fromByte int64) (io.ReadC
 
  f, err := os.Open(path)
  if err != nil {
+ log.Error("Whilst trying to read LFS OID[%s]: Unable to open %s Error: %v", meta.Oid, path, err)
  return nil, err
  }
  if fromByte > 0 {
  _, err = f.Seek(fromByte, os.SEEK_CUR)
+ if err != nil {
+ log.Error("Whilst trying to read LFS OID[%s]: Unable to seek to %d Error: %v", meta.Oid, fromByte, err)
+ }
  }
  return f, err
 }
@@ -43,11 +48,13 @@ func (s *ContentStore) Put(meta *models.LFSMetaObject, r io.Reader) error {
 
  dir := filepath.Dir(path)
  if err := os.MkdirAll(dir, 0750); err != nil {
+ log.Error("Whilst putting LFS OID[%s]: Unable to create the LFS directory: %s Error: %v", meta.Oid, dir, err)
  return err
  }
 
  file, err := os.OpenFile(tmpPath, os.O_CREATE|os.O_WRONLY|os.O_EXCL, 0640)
  if err != nil {
+ log.Error("Whilst putting LFS OID[%s]: Unable to open temporary file for writing: %s Error: %v", tmpPath, err)
  return err
  }
  defer os.Remove(tmpPath)
@@ -57,6 +64,7 @@ func (s *ContentStore) Put(meta *models.LFSMetaObject, r io.Reader) error {
 
  written, err := io.Copy(hw, r)
  if err != nil {
+ log.Error("Whilst putting LFS OID[%s]: Failed to copy to tmpPath: %s Error: %v", meta.Oid, tmpPath, err)
  file.Close()
  return err
  }
@@ -71,7 +79,12 @@ func (s *ContentStore) Put(meta *models.LFSMetaObject, r io.Reader) error {
  return errHashMismatch
  }
 
- return os.Rename(tmpPath, path)
+ if err := os.Rename(tmpPath, path); err != nil {
+ log.Error("Whilst putting LFS OID[%s]: Unable to move tmp file to final destination: %s Error: %v", meta.Oid, path, err)
+ return err
+ }
+
+ return nil
 }
 
 // Exists returns true if the object exists in the content store.
@@ -91,6 +104,7 @@ func (s *ContentStore) Verify(meta *models.LFSMetaObject) (bool, error) {
  if os.IsNotExist(err) || err == nil && fi.Size() != meta.Size {
  return false, nil
  } else if err != nil {
+ log.Error("Unable stat file: %s for LFS OID[%s] Error: %v", path, meta.Oid, err)
  return false, err
  }
 

diff --git a/modules/lfs/locks.go b/modules/lfs/locks.go
@@ -19,10 +19,12 @@ import (
 //checkIsValidRequest check if it a valid request in case of bad request it write the response to ctx.
 func checkIsValidRequest(ctx *context.Context) bool {
  if !setting.LFS.StartServer {
+ log.Debug("Attempt to access LFS server but LFS server is disabled")
  writeStatus(ctx, 404)
  return false
  }
  if !MetaMatcher(ctx.Req) {
+ log.Info("Attempt access LOCKs without accepting the correct media type: %s", metaMediaType)
  writeStatus(ctx, 400)
  return false
  }
@@ -47,7 +49,7 @@ func handleLockListOut(ctx *context.Context, repo *models.Repository, lock *mode
  return
  }
  ctx.JSON(500, api.LFSLockError{
- Message: "unable to list locks : " + err.Error(),
+ Message: "unable to list locks : Internal Server Error",
  })
  return
  }
@@ -65,6 +67,7 @@ func handleLockListOut(ctx *context.Context, repo *models.Repository, lock *mode
 // GetListLockHandler list locks
 func GetListLockHandler(ctx *context.Context) {
  if !checkIsValidRequest(ctx) {
+ // Status is written in checkIsValidRequest
  return
  }
  ctx.Resp.Header().Set("Content-Type", metaMediaType)
@@ -87,7 +90,17 @@ func GetListLockHandler(ctx *context.Context) {
  })
  return
  }
- //TODO handle query cursor and limit
+
+ cursor := ctx.QueryInt("cursor")
+ if cursor < 0 {
+ cursor = 0
+ }
+ limit := ctx.QueryInt("limit")
+ if limit > setting.LFS.LocksPagingNum && setting.LFS.LocksPagingNum > 0 {
+ limit = setting.LFS.LocksPagingNum
+ } else if limit < 0 {
+ limit = 0
+ }
  id := ctx.Query("id")
  if id != "" { //Case where we request a specific id
  v, err := strconv.ParseInt(id, 10, 64)
@@ -98,37 +111,50 @@ func GetListLockHandler(ctx *context.Context) {
  return
  }
  lock, err := models.GetLFSLockByID(v)
+ if err != nil && !models.IsErrLFSLockNotExist(err) {
+ log.Error("Unable to get lock with ID[%s]: Error: %v", v, err)
+ }
  handleLockListOut(ctx, repository, lock, err)
  return
  }
 
  path := ctx.Query("path")
  if path != "" { //Case where we request a specific id
  lock, err := models.GetLFSLock(repository, path)
+ if err != nil && !models.IsErrLFSLockNotExist(err) {
+ log.Error("Unable to get lock for repository %-v with path %s: Error: %v", repository, path, err)
+ }
  handleLockListOut(ctx, repository, lock, err)
  return
  }
 
  //If no query params path or id
- lockList, err := models.GetLFSLockByRepoID(repository.ID, 0, 0)
+ lockList, err := models.GetLFSLockByRepoID(repository.ID, cursor, limit)
  if err != nil {
+ log.Error("Unable to list locks for repository ID[%d]: Error: %v", repository.ID, err)
  ctx.JSON(500, api.LFSLockError{
- Message: "unable to list locks : " + err.Error(),
+ Message: "unable to list locks : Internal Server Error",
  })
  return
  }
  lockListAPI := make([]*api.LFSLock, len(lockList))
+ next := ""
  for i, l := range lockList {
  lockListAPI[i] = l.APIFormat()
  }
+ if limit > 0 && len(lockList) == limit {
+ next = strconv.Itoa(cursor + 1)
+ }
  ctx.JSON(200, api.LFSLockList{
  Locks: lockListAPI,
+ Next: next,
  })
 }
 
 // PostLockHandler create lock
 func PostLockHandler(ctx *context.Context) {
  if !checkIsValidRequest(ctx) {
+ // Status is written in checkIsValidRequest
  return
  }
  ctx.Resp.Header().Set("Content-Type", metaMediaType)
@@ -139,7 +165,7 @@ func PostLockHandler(ctx *context.Context) {
 
  repository, err := models.GetRepositoryByOwnerAndName(userName, repoName)
  if err != nil {
- log.Debug("Could not find repository: %s/%s - %s", userName, repoName, err)
+ log.Error("Unable to get repository: %s/%s Error: %v", userName, repoName, err)
  writeStatus(ctx, 404)
  return
  }
@@ -159,6 +185,7 @@ func PostLockHandler(ctx *context.Context) {
  defer bodyReader.Close()
  dec := json.NewDecoder(bodyReader)
  if err := dec.Decode(&req); err != nil {
+ log.Warn("Failed to decode lock request as json. Error: %v", err)
  writeStatus(ctx, 400)
  return
  }
@@ -183,8 +210,9 @@ func PostLockHandler(ctx *context.Context) {
  })
  return
  }
+ log.Error("Unable to CreateLFSLock in repository %-v at %s for user %-v: Error: %v", repository, req.Path, ctx.User, err)
  ctx.JSON(500, api.LFSLockError{
- Message: "internal server error : " + err.Error(),
+ Message: "internal server error : Internal Server Error",
  })
  return
  }
@@ -194,6 +222,7 @@ func PostLockHandler(ctx *context.Context) {
 // VerifyLockHandler list locks for verification
 func VerifyLockHandler(ctx *context.Context) {
  if !checkIsValidRequest(ctx) {
+ // Status is written in checkIsValidRequest
  return
  }
  ctx.Resp.Header().Set("Content-Type", metaMediaType)
@@ -204,7 +233,7 @@ func VerifyLockHandler(ctx *context.Context) {
 
  repository, err := models.GetRepositoryByOwnerAndName(userName, repoName)
  if err != nil {
- log.Debug("Could not find repository: %s/%s - %s", userName, repoName, err)
+ log.Error("Unable to get repository: %s/%s Error: %v", userName, repoName, err)
  writeStatus(ctx, 404)
  return
  }
@@ -219,14 +248,28 @@ func VerifyLockHandler(ctx *context.Context) {
  return
  }
 
- //TODO handle body json cursor and limit
- lockList, err := models.GetLFSLockByRepoID(repository.ID, 0, 0)
+ cursor := ctx.QueryInt("cursor")
+ if cursor < 0 {
+ cursor = 0
+ }
+ limit := ctx.QueryInt("limit")
+ if limit > setting.LFS.LocksPagingNum && setting.LFS.LocksPagingNum > 0 {
+ limit = setting.LFS.LocksPagingNum
+ } else if limit < 0 {
+ limit = 0
+ }
+ lockList, err := models.GetLFSLockByRepoID(repository.ID, cursor, limit)
  if err != nil {
+ log.Error("Unable to list locks for repository ID[%d]: Error: %v", repository.ID, err)
  ctx.JSON(500, api.LFSLockError{
- Message: "unable to list locks : " + err.Error(),
+ Message: "unable to list locks : Internal Server Error",
  })
  return
  }
+ next := ""
+ if limit > 0 && len(lockList) == limit {
+ next = strconv.Itoa(cursor + 1)
+ }
  lockOursListAPI := make([]*api.LFSLock, 0, len(lockList))
  lockTheirsListAPI := make([]*api.LFSLock, 0, len(lockList))
  for _, l := range lockList {
@@ -239,12 +282,14 @@ func VerifyLockHandler(ctx *context.Context) {
  ctx.JSON(200, api.LFSLockListVerify{
  Ours: lockOursListAPI,
  Theirs: lockTheirsListAPI,
+ Next: next,
  })
 }
 
 // UnLockHandler delete locks
 func UnLockHandler(ctx *context.Context) {
  if !checkIsValidRequest(ctx) {
+ // Status is written in checkIsValidRequest
  return
  }
  ctx.Resp.Header().Set("Content-Type", metaMediaType)
@@ -255,7 +300,7 @@ func UnLockHandler(ctx *context.Context) {
 
  repository, err := models.GetRepositoryByOwnerAndName(userName, repoName)
  if err != nil {
- log.Debug("Could not find repository: %s/%s - %s", userName, repoName, err)
+ log.Error("Unable to get repository: %s/%s Error: %v", userName, repoName, err)
  writeStatus(ctx, 404)
  return
  }
@@ -275,6 +320,7 @@ func UnLockHandler(ctx *context.Context) {
  defer bodyReader.Close()
  dec := json.NewDecoder(bodyReader)
  if err := dec.Decode(&req); err != nil {
+ log.Warn("Failed to decode lock request as json. Error: %v", err)
  writeStatus(ctx, 400)
  return
  }
@@ -288,8 +334,9 @@ func UnLockHandler(ctx *context.Context) {
  })
  return
  }
+ log.Error("Unable to DeleteLFSLockByID[%d] by user %-v with force %t: Error: %v", ctx.ParamsInt64("lid"), ctx.User, req.Force, err)
  ctx.JSON(500, api.LFSLockError{
- Message: "unable to delete lock : " + err.Error(),
+ Message: "unable to delete lock : Internal Server Error",
  })
  return
  }