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")) +}