From 0e3211f4ad113975f50e1f6338c28622ba0c9a1b Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Mon, 8 Feb 2021 10:15:12 -0800 Subject: [PATCH] fix: server upgrades should have more descriptive error messages (#11476) during rolling upgrade, provide a more descriptive error message and discourage rolling upgrade in such situations, allowing users to take action. additionally also rename `slashpath -> pathutil` to avoid a slighly mis-pronounced usage of `path` package. --- cmd/auth-handler.go | 1 + cmd/erasure-server-pool.go | 2 -- cmd/handler-utils.go | 71 ++++++++++++++++++++++++++++++-------- cmd/xl-storage.go | 56 +++++++++++------------------- 4 files changed, 78 insertions(+), 52 deletions(-) diff --git a/cmd/auth-handler.go b/cmd/auth-handler.go index 54974ce0b..570b03a98 100644 --- a/cmd/auth-handler.go +++ b/cmd/auth-handler.go @@ -498,6 +498,7 @@ func setAuthHandler(h http.Handler) http.Handler { // Validate Authorization header if its valid for JWT request. if _, _, authErr := webRequestAuthenticate(r); authErr != nil { w.WriteHeader(http.StatusUnauthorized) + w.Write([]byte(authErr.Error())) return } h.ServeHTTP(w, r) diff --git a/cmd/erasure-server-pool.go b/cmd/erasure-server-pool.go index 659cd9e5c..3911944dc 100644 --- a/cmd/erasure-server-pool.go +++ b/cmd/erasure-server-pool.go @@ -33,7 +33,6 @@ import ( "github.com/minio/minio-go/v7/pkg/tags" "github.com/minio/minio/cmd/config/storageclass" "github.com/minio/minio/cmd/logger" - "github.com/minio/minio/pkg/color" "github.com/minio/minio/pkg/madmin" "github.com/minio/minio/pkg/sync/errgroup" ) @@ -368,7 +367,6 @@ func (z *erasureServerPools) CrawlAndGetDataUsage(ctx context.Context, bf *bloom } if len(allBuckets) == 0 { - logger.Info(color.Green("data-crawl:") + " No buckets found, skipping crawl") updates <- DataUsageInfo{} // no buckets found update data usage to reflect latest state return nil } diff --git a/cmd/handler-utils.go b/cmd/handler-utils.go index cbee6d919..8ed1fddc6 100644 --- a/cmd/handler-utils.go +++ b/cmd/handler-utils.go @@ -419,20 +419,65 @@ func getResource(path string, host string, domains []string) (string, error) { return path, nil } -var regexVersion = regexp.MustCompile(`(\w\d+)`) +var regexVersion = regexp.MustCompile(`^/minio.*/(v\d+)/.*`) func extractAPIVersion(r *http.Request) string { - return regexVersion.FindString(r.URL.Path) + if matches := regexVersion.FindStringSubmatch(r.URL.Path); len(matches) > 1 { + return matches[1] + } + return "unknown" } func methodNotAllowedHandler(api string) func(w http.ResponseWriter, r *http.Request) { return func(w http.ResponseWriter, r *http.Request) { - code := "XMinio" + api + "VersionMismatch" - writeErrorResponseString(r.Context(), w, APIError{ - Code: code, - Description: "Not allowed (" + r.Method + " " + r.URL.String() + " on " + api + " API)", - HTTPStatusCode: http.StatusMethodNotAllowed, - }, r.URL) + if r.Method == http.MethodOptions { + return + } + version := extractAPIVersion(r) + switch { + case strings.HasPrefix(r.URL.Path, peerRESTPrefix): + desc := fmt.Sprintf("Server expects 'peer' API version '%s', instead found '%s' - *rolling upgrade is not allowed* - please make sure all servers are running the same MinIO version (%s)", peerRESTVersion, version, ReleaseTag) + writeErrorResponseString(r.Context(), w, APIError{ + Code: "XMinioPeerVersionMismatch", + Description: desc, + HTTPStatusCode: http.StatusUpgradeRequired, + }, r.URL) + case strings.HasPrefix(r.URL.Path, storageRESTPrefix): + desc := fmt.Sprintf("Server expects 'storage' API version '%s', instead found '%s' - *rolling upgrade is not allowed* - please make sure all servers are running the same MinIO version (%s)", storageRESTVersion, version, ReleaseTag) + writeErrorResponseString(r.Context(), w, APIError{ + Code: "XMinioStorageVersionMismatch", + Description: desc, + HTTPStatusCode: http.StatusUpgradeRequired, + }, r.URL) + case strings.HasPrefix(r.URL.Path, lockRESTPrefix): + desc := fmt.Sprintf("Server expects 'lock' API version '%s', instead found '%s' - *rolling upgrade is not allowed* - please make sure all servers are running the same MinIO version (%s)", lockRESTVersion, version, ReleaseTag) + writeErrorResponseString(r.Context(), w, APIError{ + Code: "XMinioLockVersionMismatch", + Description: desc, + HTTPStatusCode: http.StatusUpgradeRequired, + }, r.URL) + case strings.HasPrefix(r.URL.Path, adminPathPrefix): + var desc string + if version == "v1" { + desc = fmt.Sprintf("Server expects client requests with 'admin' API version '%s', found '%s', please upgrade the client to latest releases", madmin.AdminAPIVersion, version) + } else if version == madmin.AdminAPIVersion { + desc = fmt.Sprintf("This 'admin' API is not supported by server in '%s'", getMinioMode()) + } else { + desc = fmt.Sprintf("Unexpected client 'admin' API version found '%s', expected '%s', please downgrade the client to older releases", version, madmin.AdminAPIVersion) + } + writeErrorResponseJSON(r.Context(), w, APIError{ + Code: "XMinioAdminVersionMismatch", + Description: desc, + HTTPStatusCode: http.StatusUpgradeRequired, + }, r.URL) + default: + desc := fmt.Sprintf("Unknown API request at %s", r.URL.Path) + writeErrorResponse(r.Context(), w, APIError{ + Code: "XMinioUnknownAPIRequest", + Description: desc, + HTTPStatusCode: http.StatusBadRequest, + }, r.URL, guessIsBrowserReq(r)) + } } } @@ -444,24 +489,21 @@ func errorResponseHandler(w http.ResponseWriter, r *http.Request) { version := extractAPIVersion(r) switch { case strings.HasPrefix(r.URL.Path, peerRESTPrefix): - desc := fmt.Sprintf("Expected 'peer' API version '%s', instead found '%s', please upgrade the servers", - peerRESTVersion, version) + desc := fmt.Sprintf("Server expects 'peer' API version '%s', instead found '%s' - *rolling upgrade is not allowed* - please make sure all servers are running the same MinIO version (%s)", peerRESTVersion, version, ReleaseTag) writeErrorResponseString(r.Context(), w, APIError{ Code: "XMinioPeerVersionMismatch", Description: desc, HTTPStatusCode: http.StatusUpgradeRequired, }, r.URL) case strings.HasPrefix(r.URL.Path, storageRESTPrefix): - desc := fmt.Sprintf("Expected 'storage' API version '%s', instead found '%s', please upgrade the servers", - storageRESTVersion, version) + desc := fmt.Sprintf("Server expects 'storage' API version '%s', instead found '%s' - *rolling upgrade is not allowed* - please make sure all servers are running the same MinIO version (%s)", storageRESTVersion, version, ReleaseTag) writeErrorResponseString(r.Context(), w, APIError{ Code: "XMinioStorageVersionMismatch", Description: desc, HTTPStatusCode: http.StatusUpgradeRequired, }, r.URL) case strings.HasPrefix(r.URL.Path, lockRESTPrefix): - desc := fmt.Sprintf("Expected 'lock' API version '%s', instead found '%s', please upgrade the servers", - lockRESTVersion, version) + desc := fmt.Sprintf("Server expects 'lock' API version '%s', instead found '%s' - *rolling upgrade is not allowed* - please make sure all servers are running the same MinIO version (%s)", lockRESTVersion, version, ReleaseTag) writeErrorResponseString(r.Context(), w, APIError{ Code: "XMinioLockVersionMismatch", Description: desc, @@ -489,6 +531,7 @@ func errorResponseHandler(w http.ResponseWriter, r *http.Request) { HTTPStatusCode: http.StatusBadRequest, }, r.URL, guessIsBrowserReq(r)) } + } // gets host name for current node diff --git a/cmd/xl-storage.go b/cmd/xl-storage.go index 753ccf7fe..5a535876d 100644 --- a/cmd/xl-storage.go +++ b/cmd/xl-storage.go @@ -28,8 +28,7 @@ import ( "io/ioutil" "net/url" "os" - "path" - slashpath "path" + pathutil "path" "path/filepath" "runtime" "strings" @@ -625,26 +624,12 @@ func listVols(dirPath string) ([]VolInfo, error) { } volsInfo := make([]VolInfo, 0, len(entries)) for _, entry := range entries { - if !HasSuffix(entry, SlashSeparator) || !isValidVolname(slashpath.Clean(entry)) { + if !HasSuffix(entry, SlashSeparator) || !isValidVolname(pathutil.Clean(entry)) { // Skip if entry is neither a directory not a valid volume name. continue } - var fi os.FileInfo - fi, err = os.Lstat(pathJoin(dirPath, entry)) - if err != nil { - // If the file does not exist, skip the entry. - if osIsNotExist(err) { - continue - } else if isSysErrIO(err) { - return nil, errFaultyDisk - } - return nil, err - } volsInfo = append(volsInfo, VolInfo{ - Name: fi.Name(), - // As os.Lstat() doesn't carry other than ModTime(), use - // ModTime() as CreatedTime. - Created: fi.ModTime(), + Name: pathutil.Clean(entry), }) } return volsInfo, nil @@ -1373,7 +1358,7 @@ func (s *xlStorage) openFile(volume, path string, mode int) (f *os.File, err err } else { // Create top level directories if they don't exist. // with mode 0777 mkdir honors system umask. - if err = mkdirAll(slashpath.Dir(filePath), 0777); err != nil { + if err = mkdirAll(pathutil.Dir(filePath), 0777); err != nil { return nil, err } } @@ -1599,7 +1584,7 @@ func (s *xlStorage) CreateFile(ctx context.Context, volume, path string, fileSiz // Create top level directories if they don't exist. // with mode 0777 mkdir honors system umask. - if err = mkdirAll(slashpath.Dir(filePath), 0777); err != nil { + if err = mkdirAll(pathutil.Dir(filePath), 0777); err != nil { switch { case osIsPermission(err): return errFileAccessDenied @@ -1827,7 +1812,7 @@ func (s *xlStorage) CheckFile(ctx context.Context, volume string, path string) e return nil } - return checkFile(slashpath.Dir(p)) + return checkFile(pathutil.Dir(p)) } return checkFile(path) @@ -1843,8 +1828,8 @@ func deleteFile(basePath, deletePath string, recursive bool) error { return nil } isObjectDir := HasSuffix(deletePath, SlashSeparator) - basePath = slashpath.Clean(basePath) - deletePath = slashpath.Clean(deletePath) + basePath = pathutil.Clean(basePath) + deletePath = pathutil.Clean(deletePath) if !strings.HasPrefix(deletePath, basePath) || deletePath == basePath { return nil } @@ -1878,7 +1863,7 @@ func deleteFile(basePath, deletePath string, recursive bool) error { } } - deletePath = slashpath.Dir(deletePath) + deletePath = pathutil.Dir(deletePath) // Delete parent directory obviously not recursively. Errors for // parent directories shouldn't trickle down. @@ -1990,8 +1975,7 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath, dataDir, return err } - _, err = os.Lstat(dstVolumeDir) - if err != nil { + if _, err = os.Lstat(dstVolumeDir); err != nil { if osIsNotExist(err) { return errVolumeNotFound } else if isSysErrIO(err) { @@ -2000,8 +1984,8 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath, dataDir, return err } - srcFilePath := slashpath.Join(srcVolumeDir, pathJoin(srcPath, xlStorageFormatFile)) - dstFilePath := slashpath.Join(dstVolumeDir, pathJoin(dstPath, xlStorageFormatFile)) + srcFilePath := pathutil.Join(srcVolumeDir, pathJoin(srcPath, xlStorageFormatFile)) + dstFilePath := pathutil.Join(dstVolumeDir, pathJoin(dstPath, xlStorageFormatFile)) var srcDataPath string var dstDataPath string @@ -2010,7 +1994,7 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath, dataDir, // make sure to always use path.Join here, do not use pathJoin as // it would additionally add `/` at the end and it comes in the // way of renameAll(), parentDir creation. - dstDataPath = slashpath.Join(dstVolumeDir, dstPath, dataDir) + dstDataPath = pathutil.Join(dstVolumeDir, dstPath, dataDir) } if err = checkPathLength(srcFilePath); err != nil { @@ -2064,12 +2048,12 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath, dataDir, // bucket1/name1/xl.meta --> this should never // be allowed. { - entries, err := readDirN(path.Dir(dstFilePath), 1) + entries, err := readDirN(pathutil.Dir(dstFilePath), 1) if err != nil && err != errFileNotFound { return err } if len(entries) > 0 { - entry := path.Clean(entries[0]) + entry := pathutil.Clean(entries[0]) if entry != legacyDataDir { _, uerr := uuid.Parse(entry) if uerr != nil { @@ -2204,12 +2188,12 @@ func (s *xlStorage) RenameData(ctx context.Context, srcVolume, srcPath, dataDir, } // Remove parent dir of the source file if empty - if parentDir := slashpath.Dir(srcFilePath); isDirEmpty(parentDir) { + if parentDir := pathutil.Dir(srcFilePath); isDirEmpty(parentDir) { deleteFile(srcVolumeDir, parentDir, false) } if srcDataPath != "" { - if parentDir := slashpath.Dir(srcDataPath); isDirEmpty(parentDir) { + if parentDir := pathutil.Dir(srcDataPath); isDirEmpty(parentDir) { deleteFile(srcVolumeDir, parentDir, false) } } @@ -2258,11 +2242,11 @@ func (s *xlStorage) RenameFile(ctx context.Context, srcVolume, srcPath, dstVolum if !(srcIsDir && dstIsDir || !srcIsDir && !dstIsDir) { return errFileAccessDenied } - srcFilePath := slashpath.Join(srcVolumeDir, srcPath) + srcFilePath := pathutil.Join(srcVolumeDir, srcPath) if err = checkPathLength(srcFilePath); err != nil { return err } - dstFilePath := slashpath.Join(dstVolumeDir, dstPath) + dstFilePath := pathutil.Join(dstVolumeDir, dstPath) if err = checkPathLength(dstFilePath); err != nil { return err } @@ -2296,7 +2280,7 @@ func (s *xlStorage) RenameFile(ctx context.Context, srcVolume, srcPath, dstVolum } // Remove parent dir of the source file if empty - if parentDir := slashpath.Dir(srcFilePath); isDirEmpty(parentDir) { + if parentDir := pathutil.Dir(srcFilePath); isDirEmpty(parentDir) { deleteFile(srcVolumeDir, parentDir, false) }