From 00095d2865c8ff31740df94cb7439f19e51f1bd8 Mon Sep 17 00:00:00 2001 From: Andrew Liaw Date: Tue, 1 Sep 2026 11:16:54 +0800 Subject: [PATCH] scalesets: distinguish a GitHub 403 from a 401 ScaleSetClient.Do mapped both 401 and 403 onto the bare ErrUnauthorized sentinel. GitHub answers 403 for a secondary rate limit and for SSO enforcement on an organization, neither of which says anything about the credentials, so callers reading that error conclude the credentials are dead when they are usually fine and the refusal is temporary. GARM does this to itself in the scaleset worker: on a failed RemoveRunner it treats ErrUnauthorized as "our credentials may have expired or are incorrect" and parks the runner, with a TODO to deactivate the scale set and stop its listener. A rate limit that would have cleared on the next pass is enough to trigger that. Return a ForbiddenError for 403 instead. It reports as an UnauthorizedError, so every caller that only asks whether the request was refused keeps working, while callers that need to act on the difference can single it out with errors.Is(err, &ForbiddenError{}) checked before the broader case. This follows RunnerTransitionError, which reports as a BadRequestError the same way. handleError maps it to HTTP 403, so API clients can tell the two apart as well. Both branches also keep the response body and URL, as every other status branch already does. They previously returned a bare sentinel, so an operator reading the log had nothing to tell a revoked credential from a rate limit. Signed-off-by: Andrew Liaw --- apiserver/controllers/controllers.go | 7 ++ internal/errors/errors.go | 41 +++++++++ internal/errors/errors_test.go | 93 +++++++++++++++++++ util/github/scalesets/client.go | 16 +++- util/github/scalesets/client_test.go | 132 +++++++++++++++++++++++++++ 5 files changed, 287 insertions(+), 2 deletions(-) create mode 100644 internal/errors/errors_test.go create mode 100644 util/github/scalesets/client_test.go diff --git a/apiserver/controllers/controllers.go b/apiserver/controllers/controllers.go index a46a6c7e3..d4f3e46f7 100644 --- a/apiserver/controllers/controllers.go +++ b/apiserver/controllers/controllers.go @@ -33,6 +33,7 @@ import ( "github.com/cloudbase/garm/apiserver/params" "github.com/cloudbase/garm/auth" "github.com/cloudbase/garm/config" + internalErrors "github.com/cloudbase/garm/internal/errors" "github.com/cloudbase/garm/metrics" runnerParams "github.com/cloudbase/garm/params" "github.com/cloudbase/garm/runner" //nolint:typecheck @@ -109,6 +110,12 @@ func handleError(ctx context.Context, w http.ResponseWriter, err error) { case errors.Is(err, gErrors.ErrNotFound): w.WriteHeader(http.StatusNotFound) apiErr.Error = "Not Found" + // Checked before the unauthorized case below: a ForbiddenError reports as an + // UnauthorizedError too, so that callers asking only "was this refused?" keep + // working, and the broader case would otherwise swallow it. + case errors.Is(err, &internalErrors.ForbiddenError{}): + w.WriteHeader(http.StatusForbidden) + apiErr.Error = "Forbidden" case errors.Is(err, gErrors.ErrUnauthorized): w.WriteHeader(http.StatusUnauthorized) apiErr.Error = "Not Authorized" diff --git a/internal/errors/errors.go b/internal/errors/errors.go index b42e945c2..179c25498 100644 --- a/internal/errors/errors.go +++ b/internal/errors/errors.go @@ -102,3 +102,44 @@ func RunnerIsTerminal(s params.RunnerStatus) bool { return false } } + +// NewForbiddenError returns a ForbiddenError carrying the message the forge +// replied with. +func NewForbiddenError(msg string, a ...any) error { + return &ForbiddenError{ + msg: fmt.Sprintf(msg, a...), + } +} + +// ForbiddenError is returned when a forge answers a request with 403 rather +// than 401. Both mean the request was refused, but only 401 implies the +// credentials are wrong: GitHub also answers 403 for a secondary rate limit or +// for SSO enforcement on an organization, neither of which says anything about +// the credentials, and both of which clear on their own or with an operator +// action that is not a credential rotation. +// +// It reports as a runnerErrors.UnauthorizedError so that callers which only +// ask "was this refused?" keep working unchanged. Callers that need to act on +// the difference — retrying a transient refusal instead of declaring the +// credentials dead — can single it out with errors.Is(err, &ForbiddenError{}), +// which must be checked before the broader unauthorized case. +type ForbiddenError struct { + msg string +} + +func (e *ForbiddenError) Error() string { + return e.msg +} + +func (e *ForbiddenError) Is(target error) bool { + if target == nil { + return false + } + + switch target.(type) { + case *ForbiddenError, *runnerErrors.UnauthorizedError: + return true + default: + return false + } +} diff --git a/internal/errors/errors_test.go b/internal/errors/errors_test.go new file mode 100644 index 000000000..ac96e31a0 --- /dev/null +++ b/internal/errors/errors_test.go @@ -0,0 +1,93 @@ +// Copyright 2026 Cloudbase Solutions SRL +// +// Licensed under the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. You may obtain +// a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +// License for the specific language governing permissions and limitations +// under the License. +package errors + +import ( + "errors" + "fmt" + "testing" + + "github.com/stretchr/testify/require" + + runnerErrors "github.com/cloudbase/garm-provider-common/errors" +) + +func TestForbiddenErrorIdentity(t *testing.T) { + forbidden := NewForbiddenError("forbidden while calling %s", "https://example.com") + + tests := []struct { + name string + target error + want bool + }{ + { + name: "matches itself, so callers can single out a 403", + target: &ForbiddenError{}, + want: true, + }, + { + // The compatibility guarantee: callers that only ask "was this + // refused?" must keep working after 403 stopped returning + // ErrUnauthorized outright. + name: "still reports as unauthorized", + target: runnerErrors.ErrUnauthorized, + want: true, + }, + { + name: "is not a not-found", + target: runnerErrors.ErrNotFound, + want: false, + }, + { + name: "is not a bad request", + target: runnerErrors.ErrBadRequest, + want: false, + }, + { + name: "does not match a nil target", + target: nil, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.want, errors.Is(forbidden, tt.target)) + }) + } +} + +func TestForbiddenErrorKeepsItsMessage(t *testing.T) { + err := NewForbiddenError("forbidden while calling %s: %q", "https://example.com", "rate limited") + + require.EqualError(t, err, `forbidden while calling https://example.com: "rate limited"`) +} + +func TestForbiddenErrorSurvivesWrapping(t *testing.T) { + // Callers report failures with %w up the stack, so the distinction has to + // survive being wrapped or it is useless where it is actually read. + wrapped := fmt.Errorf("removing runner from github: %w", NewForbiddenError("forbidden")) + + require.True(t, errors.Is(wrapped, &ForbiddenError{})) + require.True(t, errors.Is(wrapped, runnerErrors.ErrUnauthorized)) +} + +func TestUnauthorizedErrorIsNotForbidden(t *testing.T) { + // The inverse of the compatibility guarantee: a plain 401 must not be + // mistaken for a 403, or the retry decision inverts. + err := runnerErrors.NewUnauthorizedError("unauthorized while calling https://example.com") + + require.True(t, errors.Is(err, runnerErrors.ErrUnauthorized)) + require.False(t, errors.Is(err, &ForbiddenError{})) +} diff --git a/util/github/scalesets/client.go b/util/github/scalesets/client.go index 9583d41a4..956cb2a2e 100644 --- a/util/github/scalesets/client.go +++ b/util/github/scalesets/client.go @@ -23,6 +23,7 @@ import ( "github.com/google/go-github/v84/github" runnerErrors "github.com/cloudbase/garm-provider-common/errors" + internalErrors "github.com/cloudbase/garm/internal/errors" "github.com/cloudbase/garm/metrics" "github.com/cloudbase/garm/params" "github.com/cloudbase/garm/runner/common" @@ -111,8 +112,19 @@ func (s *ScaleSetClient) Do(req *http.Request) (*http.Response, error) { return nil, runnerErrors.NewBadRequestError("bad request while calling %s: %q", req.URL.String(), string(body)) case 409: return nil, runnerErrors.NewConflictError("conflict while calling %s: %q", req.URL.String(), string(body)) - case 401, 403: - return nil, runnerErrors.ErrUnauthorized + case 401: + // The credentials were rejected outright. Keep the body: it is the only + // thing that says whether the token expired, was revoked, or was never + // valid for this resource. + return nil, runnerErrors.NewUnauthorizedError( + fmt.Sprintf("unauthorized while calling %s: %q", req.URL.String(), string(body))) + case 403: + // Not the same as 401. GitHub answers 403 for a secondary rate limit and + // for SSO enforcement as well as for a genuine permission problem, so + // collapsing it into ErrUnauthorized tells callers the credentials are + // dead when they are usually fine and the refusal is temporary. + return nil, internalErrors.NewForbiddenError( + "forbidden while calling %s: %q", req.URL.String(), string(body)) default: return nil, fmt.Errorf("request to %s failed with status code %d: %q", req.URL.String(), resp.StatusCode, string(body)) } diff --git a/util/github/scalesets/client_test.go b/util/github/scalesets/client_test.go new file mode 100644 index 000000000..da564b0ae --- /dev/null +++ b/util/github/scalesets/client_test.go @@ -0,0 +1,132 @@ +// Copyright 2026 Cloudbase Solutions SRL +// +// Licensed under the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. You may obtain +// a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +// License for the specific language governing permissions and limitations +// under the License. +package scalesets + +import ( + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" + + runnerErrors "github.com/cloudbase/garm-provider-common/errors" + internalErrors "github.com/cloudbase/garm/internal/errors" +) + +// doAgainstStatus dispatches one request through ScaleSetClient.Do against a +// server that answers with the given status and body. +func doAgainstStatus(t *testing.T, status int, body string) error { + t.Helper() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(status) + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(srv.Close) + + req, err := http.NewRequest(http.MethodDelete, srv.URL, nil) + require.NoError(t, err) + + cli := &ScaleSetClient{httpClient: srv.Client()} + resp, err := cli.Do(req) + if resp != nil { + _ = resp.Body.Close() + } + return err +} + +func TestDoMapsRefusalStatuses(t *testing.T) { + tests := []struct { + name string + // status GitHub answered with. + status int + // isForbidden is whether the caller can tell this apart as a 403. + isForbidden bool + }{ + { + // 401 is the only status that says the credentials themselves were + // rejected. + name: "unauthorized is not forbidden", + status: http.StatusUnauthorized, + isForbidden: false, + }, + { + // GitHub answers 403 for a secondary rate limit or SSO enforcement, + // which say nothing about the credentials. + name: "forbidden is distinguishable", + status: http.StatusForbidden, + isForbidden: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := doAgainstStatus(t, tt.status, "some detail") + + require.Error(t, err) + // Both remain unauthorized, so callers that only ask whether the + // request was refused are unaffected by the split. + require.True(t, errors.Is(err, runnerErrors.ErrUnauthorized)) + require.Equal(t, tt.isForbidden, errors.Is(err, &internalErrors.ForbiddenError{})) + }) + } +} + +func TestDoKeepsRefusalDetail(t *testing.T) { + // Both refusals used to return a bare sentinel, so an operator reading the + // log had no way to tell a dead credential from a rate limit. Every other + // status branch reports the URL and body; these now do too. + tests := []struct { + name string + status int + body string + }{ + {name: "unauthorized", status: http.StatusUnauthorized, body: "bad credentials"}, + {name: "forbidden", status: http.StatusForbidden, body: "secondary rate limit"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := doAgainstStatus(t, tt.status, tt.body) + + require.ErrorContains(t, err, tt.body) + }) + } +} + +func TestDoLeavesOtherStatusesAlone(t *testing.T) { + tests := []struct { + name string + status int + target error + }{ + {name: "not found", status: http.StatusNotFound, target: runnerErrors.ErrNotFound}, + {name: "bad request", status: http.StatusBadRequest, target: runnerErrors.ErrBadRequest}, + {name: "conflict", status: http.StatusConflict, target: &runnerErrors.ConflictError{}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := doAgainstStatus(t, tt.status, "detail") + + require.ErrorIs(t, err, tt.target) + require.False(t, errors.Is(err, runnerErrors.ErrUnauthorized)) + }) + } +} + +func TestDoPassesSuccessThrough(t *testing.T) { + require.NoError(t, doAgainstStatus(t, http.StatusOK, "ok")) +}