From 0b607fe5185dd30640e3a8dfb91427f33426b8cc Mon Sep 17 00:00:00 2001 From: Stefan Majewsky Date: Wed, 20 Mar 2024 15:29:52 +0100 Subject: [PATCH] fix new gocritic lints --- cmd/api/gui_redirect.go | 6 +++--- cmd/healthmonitor/main.go | 7 ++++--- internal/api/auth/api_test.go | 2 +- internal/api/registry/blobs.go | 7 ++++--- internal/api/registry/catalog_test.go | 2 +- internal/api/registry/shared_test.go | 4 ++-- internal/api/registry/tags_test.go | 2 +- internal/auth/filter.go | 14 ++++++++------ internal/auth/request.go | 7 ++++--- internal/keppel/errors.go | 6 ++---- internal/models/imageref.go | 7 ++++--- 11 files changed, 34 insertions(+), 30 deletions(-) diff --git a/cmd/api/gui_redirect.go b/cmd/api/gui_redirect.go index d23c6216..a1a5658a 100644 --- a/cmd/api/gui_redirect.go +++ b/cmd/api/gui_redirect.go @@ -82,9 +82,9 @@ func (g *guiRedirecter) tryRedirectToGUI(w http.ResponseWriter, r *http.Request) if policy.Matches(ip, repo.Name, auth.AnonymousUserIdentity.UserName()) { // do the redirect s := g.urlStr - s = strings.Replace(s, "%AUTH_TENANT_ID%", account.AuthTenantID, -1) - s = strings.Replace(s, "%ACCOUNT_NAME%", account.Name, -1) - s = strings.Replace(s, "%REPO_NAME%", repo.Name, -1) + s = strings.ReplaceAll(s, "%AUTH_TENANT_ID%", account.AuthTenantID) + s = strings.ReplaceAll(s, "%ACCOUNT_NAME%", account.Name) + s = strings.ReplaceAll(s, "%REPO_NAME%", repo.Name) w.Header().Set("Location", s) w.WriteHeader(http.StatusFound) return diff --git a/cmd/healthmonitor/main.go b/cmd/healthmonitor/main.go index 111effe3..b95d6b5e 100644 --- a/cmd/healthmonitor/main.go +++ b/cmd/healthmonitor/main.go @@ -211,11 +211,12 @@ func (j *healthMonitorJob) ReportHealthcheckResult(w http.ResponseWriter, r *htt lastResult := j.LastResult j.LastResultLock.RUnlock() - if lastResult == nil { + switch { + case lastResult == nil: http.Error(w, "still starting up", http.StatusServiceUnavailable) - } else if *lastResult { + case *lastResult: w.WriteHeader(http.StatusNoContent) - } else { + default: http.Error(w, "healthcheck failed", http.StatusInternalServerError) } } diff --git a/internal/api/auth/api_test.go b/internal/api/auth/api_test.go index 94e98d86..6a3590bc 100644 --- a/internal/api/auth/api_test.go +++ b/internal/api/auth/api_test.go @@ -790,7 +790,7 @@ func TestAnycastAndDomainRemappedTokens(t *testing.T) { } req.ExpectBody = expectedContents } else { - msg := strings.Replace(c.ErrorMessage, "%SERVICE%", domainPrefix+c.Service, -1) + msg := strings.ReplaceAll(c.ErrorMessage, "%SERVICE%", domainPrefix+c.Service) req.ExpectStatus = http.StatusBadRequest req.ExpectBody = assert.JSONObject{"details": msg} } diff --git a/internal/api/registry/blobs.go b/internal/api/registry/blobs.go index 8c352ec7..fde24ab9 100644 --- a/internal/api/registry/blobs.go +++ b/internal/api/registry/blobs.go @@ -91,18 +91,19 @@ func (a *API) handleGetOrHeadBlob(w http.ResponseWriter, r *http.Request) { responseWasWritten, err := a.processor().ReplicateBlob(r.Context(), *blob, *account, *repo, w) if err != nil { - if responseWasWritten { + switch { + case responseWasWritten: // we cannot write to `w` if br.Execute() wrote a response there already logg.Error("while trying to replicate blob %s in %s/%s: %s", blob.Digest, account.Name, repo.Name, err.Error()) - } else if errors.Is(err, processor.ErrConcurrentReplication) { + case errors.Is(err, processor.ErrConcurrentReplication): // special handling for GET during ongoing replication (429 Too Many // Requests is not a perfect match, but it's my best guess for getting // clients to automatically retry the request after a few seconds) w.Header().Set("Retry-After", "10") msg := "currently replicating on a different worker, please retry in a few seconds" keppel.ErrTooManyRequests.With(msg).WriteAsRegistryV2ResponseTo(w, r) - } else { + default: respondWithError(w, r, err) } } else if !responseWasWritten { diff --git a/internal/api/registry/catalog_test.go b/internal/api/registry/catalog_test.go index 0d5b000b..2986ed7d 100644 --- a/internal/api/registry/catalog_test.go +++ b/internal/api/registry/catalog_test.go @@ -133,7 +133,7 @@ func testNonEmptyCatalog(t *testing.T, s test.Setup) { expectedPage = expectedPage[:length] lastRepoName := expectedPage[len(expectedPage)-1] expectedHeaders["Link"] = fmt.Sprintf(`; rel="next"`, - strings.Replace(lastRepoName, "/", "%2F", -1), length, + strings.ReplaceAll(lastRepoName, "/", "%2F"), length, ) } diff --git a/internal/api/registry/shared_test.go b/internal/api/registry/shared_test.go index 800260ae..2053cf70 100644 --- a/internal/api/registry/shared_test.go +++ b/internal/api/registry/shared_test.go @@ -45,12 +45,12 @@ const authTenantID = "test1authtenant" func testWithPrimary(t *testing.T, setupOptions []test.SetupOption, action func(test.Setup)) { test.WithRoundTripper(func(tt *test.RoundTripper) { for _, withAnycast := range []bool{false, true} { - options := append(setupOptions, + setupOptions = append(setupOptions, test.WithAnycast(withAnycast), test.WithAccount(keppel.Account{Name: "test1", AuthTenantID: authTenantID}), test.WithQuotas, ) - s := test.NewSetup(t, options...) + s := test.NewSetup(t, setupOptions...) currentlyWithAnycast = withAnycast // run the tests for this scenario diff --git a/internal/api/registry/tags_test.go b/internal/api/registry/tags_test.go index dcd94ccc..95ffb8d7 100644 --- a/internal/api/registry/tags_test.go +++ b/internal/api/registry/tags_test.go @@ -108,7 +108,7 @@ func TestListTags(t *testing.T) { expectedPage = expectedPage[:length] lastRepoName := expectedPage[len(expectedPage)-1] expectedHeaders["Link"] = fmt.Sprintf(`; rel="next"`, - strings.Replace(lastRepoName, "/", "%2F", -1), length, + strings.ReplaceAll(lastRepoName, "/", "%2F"), length, ) } diff --git a/internal/auth/filter.go b/internal/auth/filter.go index 8f7531cd..f215a552 100644 --- a/internal/auth/filter.go +++ b/internal/auth/filter.go @@ -55,11 +55,12 @@ func filterAuthorized(ir IncomingRequest, uid keppel.UserIdentity, audience Audi } case "keppel_api": - if scope.Contains(PeerAPIScope) && uid.UserType() == keppel.PeerUser { + switch { + case scope.Contains(PeerAPIScope) && uid.UserType() == keppel.PeerUser: filtered.Actions = PeerAPIScope.Actions - } else if scope.Contains(InfoAPIScope) && uid.UserType() != keppel.AnonymousUser { + case scope.Contains(InfoAPIScope) && uid.UserType() != keppel.AnonymousUser: filtered.Actions = InfoAPIScope.Actions - } else { + default: filtered.Actions = nil } @@ -70,14 +71,15 @@ func filterAuthorized(ir IncomingRequest, uid keppel.UserIdentity, audience Audi } case "keppel_auth_tenant": - if audience.AccountName != "" { + switch { + case audience.AccountName != "": // this type of scope is only used by the Keppel API, which does not // allow domain-remapping anyway filtered.Actions = nil - } else if audience.IsAnycast { + case audience.IsAnycast: // defense in depth: any APIs requiring auth-tenant-level permission are not anycastable anyway filtered.Actions = nil - } else { + default: filtered.Actions = filterAuthTenantActions(scope.ResourceName, scope.Actions, uid) } diff --git a/internal/auth/request.go b/internal/auth/request.go index 686436db..08eb69f6 100644 --- a/internal/auth/request.go +++ b/internal/auth/request.go @@ -141,12 +141,13 @@ func (ir IncomingRequest) Authorize(ctx context.Context, cfg keppel.Configuratio return nil, rerr } if uid == nil { - if authHeader == "keppel" { + switch { + case authHeader == "keppel": // do not fallback if we were explicitly instructed to only use driver auth return nil, keppel.ErrUnauthorized.With("no credentials found in request") - } else if ir.NoImplicitAnonymous { + case ir.NoImplicitAnonymous: return nil, keppel.ErrUnauthorized.With("no bearer token found in request headers").WithHeader("Www-Authenticate", ir.buildAuthChallenge(cfg, audience, "")) - } else { + default: uid = AnonymousUserIdentity allowChallenge = true } diff --git a/internal/keppel/errors.go b/internal/keppel/errors.go index 86183fda..f2d47af4 100644 --- a/internal/keppel/errors.go +++ b/internal/keppel/errors.go @@ -60,10 +60,8 @@ const ( func (c RegistryV2ErrorCode) With(msg string, args ...any) *RegistryV2Error { if msg == "" { msg = apiErrorMessages[c] - } else { - if len(args) > 0 { - msg = fmt.Sprintf(msg, args...) - } + } else if len(args) > 0 { + msg = fmt.Sprintf(msg, args...) } return &RegistryV2Error{ Code: c, diff --git a/internal/models/imageref.go b/internal/models/imageref.go index b4f202fd..d6f8739e 100644 --- a/internal/models/imageref.go +++ b/internal/models/imageref.go @@ -83,7 +83,8 @@ func ParseImageReference(input string) (ImageReference, string, error) { } var ref ImageReference - if strings.Contains(imageURL.Path, "@") { + switch { + case strings.Contains(imageURL.Path, "@"): // input references a digest pathParts := ImageReferenceRx.FindStringSubmatch(imageURL.Path) parsedDigest, err := digest.Parse(pathParts[len(pathParts)-1]) @@ -95,7 +96,7 @@ func ParseImageReference(input string) (ImageReference, string, error) { RepoName: strings.TrimPrefix(pathParts[1], "/"), Reference: ManifestReference{Digest: parsedDigest}, } - } else if strings.Contains(imageURL.Path, ":") { + case strings.Contains(imageURL.Path, ":"): // input references a tag name pathParts := strings.SplitN(imageURL.Path, ":", 2) ref = ImageReference{ @@ -103,7 +104,7 @@ func ParseImageReference(input string) (ImageReference, string, error) { RepoName: strings.TrimPrefix(pathParts[0], "/"), Reference: ManifestReference{Tag: pathParts[1]}, } - } else { + default: // input references no tag or digest - use default tag ref = ImageReference{ Host: imageURL.Host,