From 8075e5ee7430fa5f2881c844fa05ff9c94da1203 Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Tue, 16 Apr 2024 12:57:44 +0200 Subject: [PATCH] Refactor actions client error to include request id (#3430) Co-authored-by: Francesco Renzi --- .../ephemeralrunner_controller.go | 25 +- .../ephemeralrunner_controller_test.go | 6 +- .../ephemeralrunnerset_controller.go | 13 +- github/actions/client.go | 233 ++++++++++++++---- .../client_runner_scale_set_message_test.go | 2 +- .../client_runner_scale_set_session_test.go | 12 +- .../actions/client_runner_scale_set_test.go | 9 +- github/actions/errors.go | 91 +++++-- github/actions/errors_test.go | 206 ++++++++++++++++ github/actions/github_api_request_test.go | 8 +- 10 files changed, 509 insertions(+), 96 deletions(-) create mode 100644 github/actions/errors_test.go diff --git a/controllers/actions.github.com/ephemeralrunner_controller.go b/controllers/actions.github.com/ephemeralrunner_controller.go index d90f6459..8765c1a0 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller.go +++ b/controllers/actions.github.com/ephemeralrunner_controller.go @@ -21,7 +21,6 @@ import ( "errors" "fmt" "net/http" - "strings" "time" "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" @@ -295,14 +294,17 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ } func (r *EphemeralRunnerReconciler) cleanupRunnerFromService(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, log logr.Logger) (ctrl.Result, error) { - actionsError := &actions.ActionsError{} - err := r.deleteRunnerFromService(ctx, ephemeralRunner, log) - if err != nil { - if errors.As(err, &actionsError) && - actionsError.StatusCode == http.StatusBadRequest && - strings.Contains(actionsError.ExceptionName, "JobStillRunningException") { + if err := r.deleteRunnerFromService(ctx, ephemeralRunner, log); err != nil { + actionsError := &actions.ActionsError{} + if !errors.As(err, &actionsError) { + log.Error(err, "Failed to clean up runner from the service (not an ActionsError)") + return ctrl.Result{}, err + } + + if actionsError.StatusCode == http.StatusBadRequest && actionsError.IsException("JobStillRunningException") { log.Info("Runner is still running the job. Re-queue in 30 seconds") return ctrl.Result{RequeueAfter: 30 * time.Second}, nil + } log.Error(err, "Failed clean up runner from the service") @@ -310,10 +312,9 @@ func (r *EphemeralRunnerReconciler) cleanupRunnerFromService(ctx context.Context } log.Info("Successfully removed runner registration from service") - err = patch(ctx, r.Client, ephemeralRunner, func(obj *v1alpha1.EphemeralRunner) { + if err := patch(ctx, r.Client, ephemeralRunner, func(obj *v1alpha1.EphemeralRunner) { controllerutil.RemoveFinalizer(obj, ephemeralRunnerActionsFinalizerName) - }) - if err != nil { + }); err != nil { return ctrl.Result{}, err } @@ -528,7 +529,7 @@ func (r *EphemeralRunnerReconciler) updateStatusWithRunnerConfig(ctx context.Con } if actionsError.StatusCode != http.StatusConflict || - !strings.Contains(actionsError.ExceptionName, "AgentExistsException") { + !actionsError.IsException("AgentExistsException") { return ctrl.Result{}, fmt.Errorf("failed to generate JIT config with Actions service error: %v", err) } @@ -784,7 +785,7 @@ func (r EphemeralRunnerReconciler) runnerRegisteredWithService(ctx context.Conte } if actionsError.StatusCode != http.StatusNotFound || - !strings.Contains(actionsError.ExceptionName, "AgentNotFoundException") { + !actionsError.IsException("AgentNotFoundException") { return false, fmt.Errorf("failed to check if runner exists in GitHub service: %v", err) } diff --git a/controllers/actions.github.com/ephemeralrunner_controller_test.go b/controllers/actions.github.com/ephemeralrunner_controller_test.go index 4cc6ffde..2d45d87a 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller_test.go +++ b/controllers/actions.github.com/ephemeralrunner_controller_test.go @@ -671,8 +671,10 @@ var _ = Describe("EphemeralRunner", func() { fake.WithGetRunner( nil, &actions.ActionsError{ - StatusCode: http.StatusNotFound, - ExceptionName: "AgentNotFoundException", + StatusCode: http.StatusNotFound, + Err: &actions.ActionsExceptionError{ + ExceptionName: "AgentNotFoundException", + }, }, ), ), diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index 91319de8..b462e177 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -23,7 +23,6 @@ import ( "net/http" "sort" "strconv" - "strings" "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" "github.com/actions/actions-runner-controller/controllers/actions.github.com/metrics" @@ -483,10 +482,14 @@ func (r *EphemeralRunnerSetReconciler) deleteIdleEphemeralRunners(ctx context.Co func (r *EphemeralRunnerSetReconciler) deleteEphemeralRunnerWithActionsClient(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, actionsClient actions.ActionsService, log logr.Logger) (bool, error) { if err := actionsClient.RemoveRunner(ctx, int64(ephemeralRunner.Status.RunnerId)); err != nil { actionsError := &actions.ActionsError{} - if errors.As(err, &actionsError) && - actionsError.StatusCode == http.StatusBadRequest && - strings.Contains(actionsError.ExceptionName, "JobStillRunningException") { - // Runner is still running a job, proceed with the next one + if !errors.As(err, &actionsError) { + log.Error(err, "failed to remove runner from the service", "name", ephemeralRunner.Name, "runnerId", ephemeralRunner.Status.RunnerId) + return false, err + } + + if actionsError.StatusCode == http.StatusBadRequest && + actionsError.IsException("JobStillRunningException") { + log.Info("Runner is still running a job, skipping deletion", "name", ephemeralRunner.Name, "runnerId", ephemeralRunner.Status.RunnerId) return false, nil } diff --git a/github/actions/client.go b/github/actions/client.go index 1afbab9c..3470384f 100644 --- a/github/actions/client.go +++ b/github/actions/client.go @@ -355,15 +355,22 @@ func (c *Client) GetRunnerScaleSet(ctx context.Context, runnerGroupId int, runne } var runnerScaleSetList *runnerScaleSetsResponse - err = json.NewDecoder(resp.Body).Decode(&runnerScaleSetList) - if err != nil { - return nil, err + if err := json.NewDecoder(resp.Body).Decode(&runnerScaleSetList); err != nil { + return nil, &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: err, + } } if runnerScaleSetList.Count == 0 { return nil, nil } if runnerScaleSetList.Count > 1 { - return nil, fmt.Errorf("multiple runner scale sets found with name %s", runnerScaleSetName) + return nil, &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: fmt.Errorf("multiple runner scale sets found with name %q", runnerScaleSetName), + } } return &runnerScaleSetList.RunnerScaleSets[0], nil @@ -386,9 +393,12 @@ func (c *Client) GetRunnerScaleSetById(ctx context.Context, runnerScaleSetId int } var runnerScaleSet *RunnerScaleSet - err = json.NewDecoder(resp.Body).Decode(&runnerScaleSet) - if err != nil { - return nil, err + if err := json.NewDecoder(resp.Body).Decode(&runnerScaleSet); err != nil { + return nil, &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: err, + } } return runnerScaleSet, nil } @@ -408,23 +418,43 @@ func (c *Client) GetRunnerGroupByName(ctx context.Context, runnerGroup string) ( if resp.StatusCode != http.StatusOK { body, err := io.ReadAll(resp.Body) if err != nil { - return nil, err + return nil, &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: err, + } } - return nil, fmt.Errorf("unexpected status code: %d - body: %s", resp.StatusCode, string(body)) + return nil, fmt.Errorf("unexpected status code: %w", &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: errors.New(string(body)), + }) } var runnerGroupList *RunnerGroupList err = json.NewDecoder(resp.Body).Decode(&runnerGroupList) if err != nil { - return nil, err + return nil, &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: err, + } } if runnerGroupList.Count == 0 { - return nil, fmt.Errorf("no runner group found with name '%s'", runnerGroup) + return nil, &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: fmt.Errorf("no runner group found with name %q", runnerGroup), + } } if runnerGroupList.Count > 1 { - return nil, fmt.Errorf("multiple runner group found with name %s", runnerGroup) + return nil, &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: fmt.Errorf("multiple runner group found with name %q", runnerGroup), + } } return &runnerGroupList.RunnerGroups[0], nil @@ -450,9 +480,12 @@ func (c *Client) CreateRunnerScaleSet(ctx context.Context, runnerScaleSet *Runne return nil, ParseActionsErrorFromResponse(resp) } var createdRunnerScaleSet *RunnerScaleSet - err = json.NewDecoder(resp.Body).Decode(&createdRunnerScaleSet) - if err != nil { - return nil, err + if err := json.NewDecoder(resp.Body).Decode(&createdRunnerScaleSet); err != nil { + return nil, &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: err, + } } return createdRunnerScaleSet, nil } @@ -480,9 +513,12 @@ func (c *Client) UpdateRunnerScaleSet(ctx context.Context, runnerScaleSetId int, } var updatedRunnerScaleSet *RunnerScaleSet - err = json.NewDecoder(resp.Body).Decode(&updatedRunnerScaleSet) - if err != nil { - return nil, err + if err := json.NewDecoder(resp.Body).Decode(&updatedRunnerScaleSet); err != nil { + return nil, &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: err, + } } return updatedRunnerScaleSet, nil } @@ -547,15 +583,26 @@ func (c *Client) GetMessage(ctx context.Context, messageQueueUrl, messageQueueAc body, err := io.ReadAll(resp.Body) body = trimByteOrderMark(body) if err != nil { - return nil, err + return nil, &ActionsError{ + ActivityID: resp.Header.Get(HeaderActionsActivityID), + StatusCode: resp.StatusCode, + Err: err, + } + } + return nil, &MessageQueueTokenExpiredError{ + activityID: resp.Header.Get(HeaderActionsActivityID), + statusCode: resp.StatusCode, + msg: string(body), } - return nil, &MessageQueueTokenExpiredError{msg: string(body)} } var message *RunnerScaleSetMessage - err = json.NewDecoder(resp.Body).Decode(&message) - if err != nil { - return nil, err + if err := json.NewDecoder(resp.Body).Decode(&message); err != nil { + return nil, &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: err, + } } return message, nil } @@ -591,9 +638,17 @@ func (c *Client) DeleteMessage(ctx context.Context, messageQueueUrl, messageQueu body, err := io.ReadAll(resp.Body) body = trimByteOrderMark(body) if err != nil { - return err + return &ActionsError{ + ActivityID: resp.Header.Get(HeaderActionsActivityID), + StatusCode: resp.StatusCode, + Err: err, + } + } + return &MessageQueueTokenExpiredError{ + activityID: resp.Header.Get(HeaderActionsActivityID), + statusCode: resp.StatusCode, + msg: string(body), } - return &MessageQueueTokenExpiredError{msg: string(body)} } return nil } @@ -641,9 +696,18 @@ func (c *Client) doSessionRequest(ctx context.Context, method, path string, requ } if resp.StatusCode == expectedResponseStatusCode { - if responseUnmarshalTarget != nil { - return json.NewDecoder(resp.Body).Decode(responseUnmarshalTarget) + if responseUnmarshalTarget == nil { + return nil } + + if err := json.NewDecoder(resp.Body).Decode(responseUnmarshalTarget); err != nil { + return &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: err, + } + } + return nil } @@ -655,10 +719,18 @@ func (c *Client) doSessionRequest(ctx context.Context, method, path string, requ body, err := io.ReadAll(resp.Body) body = trimByteOrderMark(body) if err != nil { - return err + return &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: err, + } } - return fmt.Errorf("unexpected status code: %d - body: %s", resp.StatusCode, string(body)) + return fmt.Errorf("unexpected status code: %w", &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: errors.New(string(body)), + }) } func (c *Client) AcquireJobs(ctx context.Context, runnerScaleSetId int, messageQueueAccessToken string, requestIds []int64) ([]int64, error) { @@ -692,16 +764,28 @@ func (c *Client) AcquireJobs(ctx context.Context, runnerScaleSetId int, messageQ body, err := io.ReadAll(resp.Body) body = trimByteOrderMark(body) if err != nil { - return nil, err + return nil, &ActionsError{ + ActivityID: resp.Header.Get(HeaderActionsActivityID), + StatusCode: resp.StatusCode, + Err: err, + } } - return nil, &MessageQueueTokenExpiredError{msg: string(body)} + return nil, &MessageQueueTokenExpiredError{ + activityID: resp.Header.Get(HeaderActionsActivityID), + statusCode: resp.StatusCode, + msg: string(body), + } } var acquiredJobs *Int64List err = json.NewDecoder(resp.Body).Decode(&acquiredJobs) if err != nil { - return nil, err + return nil, &ActionsError{ + ActivityID: resp.Header.Get(HeaderActionsActivityID), + StatusCode: resp.StatusCode, + Err: err, + } } return acquiredJobs.Value, nil @@ -732,7 +816,11 @@ func (c *Client) GetAcquirableJobs(ctx context.Context, runnerScaleSetId int) (* var acquirableJobList *AcquirableJobList err = json.NewDecoder(resp.Body).Decode(&acquirableJobList) if err != nil { - return nil, err + return nil, &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: err, + } } return acquirableJobList, nil @@ -761,9 +849,12 @@ func (c *Client) GenerateJitRunnerConfig(ctx context.Context, jitRunnerSetting * } var runnerJitConfig *RunnerScaleSetJitRunnerConfig - err = json.NewDecoder(resp.Body).Decode(&runnerJitConfig) - if err != nil { - return nil, err + if err := json.NewDecoder(resp.Body).Decode(&runnerJitConfig); err != nil { + return nil, &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: err, + } } return runnerJitConfig, nil } @@ -786,9 +877,12 @@ func (c *Client) GetRunner(ctx context.Context, runnerId int64) (*RunnerReferenc } var runnerReference *RunnerReference - err = json.NewDecoder(resp.Body).Decode(&runnerReference) - if err != nil { - return nil, err + if err := json.NewDecoder(resp.Body).Decode(&runnerReference); err != nil { + return nil, &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: err, + } } return runnerReference, nil @@ -812,9 +906,12 @@ func (c *Client) GetRunnerByName(ctx context.Context, runnerName string) (*Runne } var runnerList *RunnerReferenceList - err = json.NewDecoder(resp.Body).Decode(&runnerList) - if err != nil { - return nil, err + if err := json.NewDecoder(resp.Body).Decode(&runnerList); err != nil { + return nil, &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: err, + } } if runnerList.Count == 0 { @@ -822,7 +919,11 @@ func (c *Client) GetRunnerByName(ctx context.Context, runnerName string) (*Runne } if runnerList.Count > 1 { - return nil, fmt.Errorf("multiple runner found with name %s", runnerName) + return nil, &ActionsError{ + StatusCode: resp.StatusCode, + ActivityID: resp.Header.Get(HeaderActionsActivityID), + Err: fmt.Errorf("multiple runner found with name %s", runnerName), + } } return &runnerList.RunnerReferences[0], nil @@ -895,12 +996,20 @@ func (c *Client) getRunnerRegistrationToken(ctx context.Context) (*registrationT if err != nil { return nil, err } - return nil, fmt.Errorf("unexpected response from Actions service during registration token call: %v - %v", resp.StatusCode, string(body)) + return nil, &GitHubAPIError{ + StatusCode: resp.StatusCode, + RequestID: resp.Header.Get(HeaderGitHubRequestID), + Err: errors.New(string(body)), + } } var registrationToken *registrationToken if err := json.NewDecoder(resp.Body).Decode(®istrationToken); err != nil { - return nil, err + return nil, &GitHubAPIError{ + StatusCode: resp.StatusCode, + RequestID: resp.Header.Get(HeaderGitHubRequestID), + Err: err, + } } return registrationToken, nil @@ -937,8 +1046,14 @@ func (c *Client) fetchAccessToken(ctx context.Context, gitHubConfigURL string, c // Format: https://docs.github.com/en/rest/apps/apps#create-an-installation-access-token-for-an-app var accessToken *accessToken - err = json.NewDecoder(resp.Body).Decode(&accessToken) - return accessToken, err + if err = json.NewDecoder(resp.Body).Decode(&accessToken); err != nil { + return nil, &GitHubAPIError{ + StatusCode: resp.StatusCode, + RequestID: resp.Header.Get(HeaderGitHubRequestID), + Err: err, + } + } + return accessToken, nil } type ActionsServiceAdminConnection struct { @@ -989,21 +1104,29 @@ func (c *Client) getActionsServiceAdminConnection(ctx context.Context, rt *regis break } - errStr := fmt.Sprintf("unexpected response from Actions service during registration call: %v", resp.StatusCode) + var innerErr error body, err := io.ReadAll(resp.Body) if err != nil { - err = fmt.Errorf("%s - %w", errStr, err) + innerErr = err } else { - err = fmt.Errorf("%s - %v", errStr, string(body)) + innerErr = errors.New(string(body)) } if resp.StatusCode != http.StatusUnauthorized && resp.StatusCode != http.StatusForbidden { - return nil, err + return nil, &GitHubAPIError{ + StatusCode: resp.StatusCode, + RequestID: resp.Header.Get(HeaderGitHubRequestID), + Err: innerErr, + } } retry++ if retry > 3 { - return nil, fmt.Errorf("unable to register runner after 3 retries: %v", err) + return nil, fmt.Errorf("unable to register runner after 3 retries: %w", &GitHubAPIError{ + StatusCode: resp.StatusCode, + RequestID: resp.Header.Get(HeaderGitHubRequestID), + Err: innerErr, + }) } time.Sleep(time.Duration(500 * int(time.Millisecond) * (retry + 1))) @@ -1011,7 +1134,11 @@ func (c *Client) getActionsServiceAdminConnection(ctx context.Context, rt *regis var actionsServiceAdminConnection *ActionsServiceAdminConnection if err := json.NewDecoder(resp.Body).Decode(&actionsServiceAdminConnection); err != nil { - return nil, err + return nil, &GitHubAPIError{ + StatusCode: resp.StatusCode, + RequestID: resp.Header.Get(HeaderGitHubRequestID), + Err: err, + } } return actionsServiceAdminConnection, nil diff --git a/github/actions/client_runner_scale_set_message_test.go b/github/actions/client_runner_scale_set_message_test.go index 0de77094..cb67c310 100644 --- a/github/actions/client_runner_scale_set_message_test.go +++ b/github/actions/client_runner_scale_set_message_test.go @@ -98,7 +98,7 @@ func TestGetMessage(t *testing.T) { t.Run("Status code not found", func(t *testing.T) { want := actions.ActionsError{ - Message: "Request returned status: 404 Not Found", + Err: errors.New("unknown exception"), StatusCode: 404, } server := newActionsServer(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { diff --git a/github/actions/client_runner_scale_set_session_test.go b/github/actions/client_runner_scale_set_session_test.go index 7b2ab69d..fff1b9f0 100644 --- a/github/actions/client_runner_scale_set_session_test.go +++ b/github/actions/client_runner_scale_set_session_test.go @@ -13,6 +13,8 @@ import ( "github.com/stretchr/testify/require" ) +const exampleRequestID = "5ddf2050-dae0-013c-9159-04421ad31b68" + func TestCreateMessageSession(t *testing.T) { ctx := context.Background() auth := &actions.ActionsAuth{ @@ -69,13 +71,17 @@ func TestCreateMessageSession(t *testing.T) { } want := &actions.ActionsError{ - ExceptionName: "CSharpExceptionNameHere", - Message: "could not do something", - StatusCode: http.StatusBadRequest, + ActivityID: exampleRequestID, + StatusCode: http.StatusBadRequest, + Err: &actions.ActionsExceptionError{ + ExceptionName: "CSharpExceptionNameHere", + Message: "could not do something", + }, } server := newActionsServer(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") + w.Header().Set(actions.HeaderActionsActivityID, exampleRequestID) w.WriteHeader(http.StatusBadRequest) resp := []byte(`{"typeName": "CSharpExceptionNameHere","message": "could not do something"}`) w.Write(resp) diff --git a/github/actions/client_runner_scale_set_test.go b/github/actions/client_runner_scale_set_test.go index 7f74190e..bb48ae5f 100644 --- a/github/actions/client_runner_scale_set_test.go +++ b/github/actions/client_runner_scale_set_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/actions/actions-runner-controller/github/actions" + "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -124,9 +125,15 @@ func TestGetRunnerScaleSet(t *testing.T) { }) t.Run("Multiple runner scale sets found", func(t *testing.T) { - wantErr := fmt.Errorf("multiple runner scale sets found with name %s", scaleSetName) + reqID := uuid.NewString() + wantErr := &actions.ActionsError{ + StatusCode: http.StatusOK, + ActivityID: reqID, + Err: fmt.Errorf("multiple runner scale sets found with name %q", scaleSetName), + } runnerScaleSetsResp := []byte(`{"count":2,"value":[{"id":1,"name":"ScaleSet"}]}`) server := newActionsServer(t, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set(actions.HeaderActionsActivityID, reqID) w.Write(runnerScaleSetsResp) })) diff --git a/github/actions/errors.go b/github/actions/errors.go index 7f483a0a..736520bd 100644 --- a/github/actions/errors.go +++ b/github/actions/errors.go @@ -2,63 +2,118 @@ package actions import ( "encoding/json" + "errors" "fmt" "io" "net/http" "strings" ) +// Header names for request IDs +const ( + HeaderActionsActivityID = "ActivityId" + HeaderGitHubRequestID = "X-GitHub-Request-Id" +) + +type GitHubAPIError struct { + StatusCode int + RequestID string + Err error +} + +func (e *GitHubAPIError) Error() string { + return fmt.Sprintf("github api error: StatusCode %d, RequestID %q: %v", e.StatusCode, e.RequestID, e.Err) +} + +func (e *GitHubAPIError) Unwrap() error { + return e.Err +} + type ActionsError struct { - ExceptionName string `json:"typeName,omitempty"` - Message string `json:"message,omitempty"` - StatusCode int + ActivityID string + StatusCode int + Err error } func (e *ActionsError) Error() string { - return fmt.Sprintf("%v - had issue communicating with Actions backend: %v", e.StatusCode, e.Message) + return fmt.Sprintf("actions error: StatusCode %d, AcivityId %q: %v", e.StatusCode, e.ActivityID, e.Err) +} + +func (e *ActionsError) Unwrap() error { + return e.Err +} + +func (e *ActionsError) IsException(target string) bool { + if ex, ok := e.Err.(*ActionsExceptionError); ok { + return strings.Contains(ex.ExceptionName, target) + } + + return false +} + +type ActionsExceptionError struct { + ExceptionName string `json:"typeName,omitempty"` + Message string `json:"message,omitempty"` +} + +func (e *ActionsExceptionError) Error() string { + return fmt.Sprintf("%s: %s", e.ExceptionName, e.Message) } func ParseActionsErrorFromResponse(response *http.Response) error { if response.ContentLength == 0 { - message := "Request returned status: " + response.Status return &ActionsError{ - ExceptionName: "unknown", - Message: message, - StatusCode: response.StatusCode, + ActivityID: response.Header.Get(HeaderActionsActivityID), + StatusCode: response.StatusCode, + Err: errors.New("unknown exception"), } } defer response.Body.Close() body, err := io.ReadAll(response.Body) if err != nil { - return err + return &ActionsError{ + ActivityID: response.Header.Get(HeaderActionsActivityID), + StatusCode: response.StatusCode, + Err: err, + } } body = trimByteOrderMark(body) contentType, ok := response.Header["Content-Type"] if ok && len(contentType) > 0 && strings.Contains(contentType[0], "text/plain") { message := string(body) - statusCode := response.StatusCode return &ActionsError{ - Message: message, - StatusCode: statusCode, + ActivityID: response.Header.Get(HeaderActionsActivityID), + StatusCode: response.StatusCode, + Err: errors.New(message), } } - actionsError := &ActionsError{StatusCode: response.StatusCode} - if err := json.Unmarshal(body, &actionsError); err != nil { - return err + var exception ActionsExceptionError + if err := json.Unmarshal(body, &exception); err != nil { + return &ActionsError{ + ActivityID: response.Header.Get(HeaderActionsActivityID), + StatusCode: response.StatusCode, + Err: err, + } } - return actionsError + return &ActionsError{ + ActivityID: response.Header.Get(HeaderActionsActivityID), + StatusCode: response.StatusCode, + Err: &exception, + } } type MessageQueueTokenExpiredError struct { - msg string + activityID string + statusCode int + msg string } func (e *MessageQueueTokenExpiredError) Error() string { - return e.msg + return fmt.Sprintf("MessageQueueTokenExpiredError: AcivityId %q, StatusCode %d: %s", e.activityID, e.statusCode, e.msg) } type HttpClientSideError struct { diff --git a/github/actions/errors_test.go b/github/actions/errors_test.go new file mode 100644 index 00000000..46310ba1 --- /dev/null +++ b/github/actions/errors_test.go @@ -0,0 +1,206 @@ +package actions_test + +import ( + "errors" + "io" + "net/http" + "strings" + "testing" + + "github.com/actions/actions-runner-controller/github/actions" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestActionsError(t *testing.T) { + t.Run("contains the status code, activity ID, and error", func(t *testing.T) { + err := &actions.ActionsError{ + ActivityID: "activity-id", + StatusCode: 404, + Err: errors.New("example error description"), + } + + s := err.Error() + assert.Contains(t, s, "StatusCode 404") + assert.Contains(t, s, "AcivityId \"activity-id\"") + assert.Contains(t, s, "example error description") + }) + + t.Run("unwraps the error", func(t *testing.T) { + err := &actions.ActionsError{ + ActivityID: "activity-id", + StatusCode: 404, + Err: &actions.ActionsExceptionError{ + ExceptionName: "exception-name", + Message: "example error message", + }, + } + + assert.Equal(t, err.Unwrap(), err.Err) + }) + + t.Run("is exception is ok", func(t *testing.T) { + err := &actions.ActionsError{ + ActivityID: "activity-id", + StatusCode: 404, + Err: &actions.ActionsExceptionError{ + ExceptionName: "exception-name", + Message: "example error message", + }, + } + + var exception *actions.ActionsExceptionError + assert.True(t, errors.As(err, &exception)) + + assert.True(t, err.IsException("exception-name")) + }) + + t.Run("is exception is not ok", func(t *testing.T) { + tt := map[string]*actions.ActionsError{ + "not an exception": { + ActivityID: "activity-id", + StatusCode: 404, + Err: errors.New("example error description"), + }, + "not target exception": { + ActivityID: "activity-id", + StatusCode: 404, + Err: &actions.ActionsExceptionError{ + ExceptionName: "exception-name", + Message: "example error message", + }, + }, + } + + targetException := "target-exception" + for name, err := range tt { + t.Run(name, func(t *testing.T) { + assert.False(t, err.IsException(targetException)) + }) + } + }) +} + +func TestActionsExceptionError(t *testing.T) { + t.Run("contains the exception name and message", func(t *testing.T) { + err := &actions.ActionsExceptionError{ + ExceptionName: "exception-name", + Message: "example error message", + } + + s := err.Error() + assert.Contains(t, s, "exception-name") + assert.Contains(t, s, "example error message") + }) +} + +func TestGitHubAPIError(t *testing.T) { + t.Run("contains the status code, request ID, and error", func(t *testing.T) { + err := &actions.GitHubAPIError{ + StatusCode: 404, + RequestID: "request-id", + Err: errors.New("example error description"), + } + + s := err.Error() + assert.Contains(t, s, "StatusCode 404") + assert.Contains(t, s, "RequestID \"request-id\"") + assert.Contains(t, s, "example error description") + }) + + t.Run("unwraps the error", func(t *testing.T) { + err := &actions.GitHubAPIError{ + StatusCode: 404, + RequestID: "request-id", + Err: errors.New("example error description"), + } + + assert.Equal(t, err.Unwrap(), err.Err) + }) +} + +func ParseActionsErrorFromResponse(t *testing.T) { + t.Run("empty content length", func(t *testing.T) { + response := &http.Response{ + ContentLength: 0, + Header: http.Header{ + actions.HeaderActionsActivityID: []string{"activity-id"}, + }, + StatusCode: 404, + } + + err := actions.ParseActionsErrorFromResponse(response) + require.Error(t, err) + assert.Equal(t, err.(*actions.ActionsError).ActivityID, "activity-id") + assert.Equal(t, err.(*actions.ActionsError).StatusCode, 404) + assert.Equal(t, err.(*actions.ActionsError).Err.Error(), "unknown exception") + }) + + t.Run("contains text plain error", func(t *testing.T) { + errorMessage := "example error message" + response := &http.Response{ + ContentLength: int64(len(errorMessage)), + Header: http.Header{ + actions.HeaderActionsActivityID: []string{"activity-id"}, + "Content-Type": []string{"text/plain"}, + }, + StatusCode: 404, + Body: io.NopCloser(strings.NewReader(errorMessage)), + } + + err := actions.ParseActionsErrorFromResponse(response) + require.Error(t, err) + var actionsError *actions.ActionsError + assert.ErrorAs(t, err, &actionsError) + assert.Equal(t, actionsError.ActivityID, "activity-id") + assert.Equal(t, actionsError.StatusCode, 404) + assert.Equal(t, actionsError.Err.Error(), errorMessage) + }) + + t.Run("contains json error", func(t *testing.T) { + errorMessage := `{"typeName":"exception-name","message":"example error message"}` + response := &http.Response{ + ContentLength: int64(len(errorMessage)), + Header: http.Header{ + actions.HeaderActionsActivityID: []string{"activity-id"}, + "Content-Type": []string{"application/json"}, + }, + StatusCode: 404, + Body: io.NopCloser(strings.NewReader(errorMessage)), + } + + err := actions.ParseActionsErrorFromResponse(response) + require.Error(t, err) + var actionsError *actions.ActionsError + assert.ErrorAs(t, err, &actionsError) + assert.Equal(t, actionsError.ActivityID, "activity-id") + assert.Equal(t, actionsError.StatusCode, 404) + + inner, ok := actionsError.Err.(*actions.ActionsExceptionError) + require.True(t, ok) + assert.Equal(t, inner.ExceptionName, "exception-name") + assert.Equal(t, inner.Message, "example error message") + }) + + t.Run("wrapped exception error", func(t *testing.T) { + errorMessage := `{"typeName":"exception-name","message":"example error message"}` + response := &http.Response{ + ContentLength: int64(len(errorMessage)), + Header: http.Header{ + actions.HeaderActionsActivityID: []string{"activity-id"}, + "Content-Type": []string{"application/json"}, + }, + StatusCode: 404, + Body: io.NopCloser(strings.NewReader(errorMessage)), + } + + err := actions.ParseActionsErrorFromResponse(response) + require.Error(t, err) + + var actionsExceptionError *actions.ActionsExceptionError + assert.ErrorAs(t, err, &actionsExceptionError) + + assert.Equal(t, actionsExceptionError.ExceptionName, "exception-name") + assert.Equal(t, actionsExceptionError.Message, "example error message") + }) +} diff --git a/github/actions/github_api_request_test.go b/github/actions/github_api_request_test.go index 78f740ec..18998cdd 100644 --- a/github/actions/github_api_request_test.go +++ b/github/actions/github_api_request_test.go @@ -139,7 +139,13 @@ func TestNewActionsServiceRequest(t *testing.T) { w.WriteHeader(http.StatusUnauthorized) w.Write([]byte(errMessage)) } - server := testserver.New(t, nil, testserver.WithActionsToken("random-token"), testserver.WithActionsToken(newToken), testserver.WithActionsRegistrationTokenHandler(unauthorizedHandler)) + server := testserver.New( + t, + nil, + testserver.WithActionsToken("random-token"), + testserver.WithActionsToken(newToken), + testserver.WithActionsRegistrationTokenHandler(unauthorizedHandler), + ) client, err := actions.NewClient(server.ConfigURLForOrg("my-org"), defaultCreds) require.NoError(t, err) expiringToken := "expiring-token"