From 066efdc927e894868029a0677abe6b9e353d2fd2 Mon Sep 17 00:00:00 2001 From: Bruno Bressi Date: Wed, 25 Dec 2024 18:49:31 +0100 Subject: [PATCH 1/3] feat: decode bad post Gitlab requests This commit allows users to gain better insight on 4xx errors from Gitlab. --- pkg/sparrow/targets/remote/gitlab/gitlab.go | 41 ++++++++++++++++----- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/pkg/sparrow/targets/remote/gitlab/gitlab.go b/pkg/sparrow/targets/remote/gitlab/gitlab.go index 73963ee3..ef172b7e 100644 --- a/pkg/sparrow/targets/remote/gitlab/gitlab.go +++ b/pkg/sparrow/targets/remote/gitlab/gitlab.go @@ -61,6 +61,15 @@ type Config struct { Branch string `yaml:"branch" mapstructure:"branch"` } +// badRequestError is an error type for bad requests to the gitlab API +type badRequestError struct { + Message string `json:"message"` +} + +func (e badRequestError) Error() string { + return e.Message +} + // New creates a new gitlab client func New(cfg Config) remote.Interactor { c := &client{ @@ -262,11 +271,17 @@ func (c *client) PutFile(ctx context.Context, file remote.File) error { //nolint err = errors.Join(err, resp.Body.Close()) }() - // Unfortunately the Gitlab API does not give any information about what went wrong and just returns a 400 Bad Request in case of - // an error while committing the file. A probable cause for this is that the branch does not exist or is protected. - // The documentation documents some more possible errors, but not all of them: https://docs.gitlab.com/ee/api/repository_files.html#update-existing-file-in-repository - // Therefore we just check for the status code and return an error if it is not 200 OK. - // This is not ideal, but the best we can do with the current API without implementing a full blown error handling mechanism. + // In case a of bad request we return the error message + // for more clarity. See https://docs.gitlab.com/ee/api/rest/troubleshooting.html#status-code-400 + // for more information. + if resp.StatusCode == http.StatusBadRequest { + var bErr badRequestError + if err = json.NewDecoder(resp.Body).Decode(&bErr); err == nil { + return bErr + } + log.ErrorContext(ctx, "Failed to unmarshall bad request error", "error", err) + } + if resp.StatusCode != http.StatusOK { log.ErrorContext(ctx, "Failed to push registration file", "status", resp.Status) return fmt.Errorf("request failed, status is %s", resp.Status) @@ -311,11 +326,17 @@ func (c *client) PostFile(ctx context.Context, file remote.File) error { //nolin err = errors.Join(err, resp.Body.Close()) }() - // Unfortunately the Gitlab API does not give any information about what went wrong and just returns a 400 Bad Request in case of - // an error while committing the file. A probable cause for this is that the branch does not exist or is protected. - // The documentation documents some more possible errors, but not all of them: https://docs.gitlab.com/ee/api/repository_files.html#update-existing-file-in-repository - // Therefore we just check for the status code and return an error if it is not 200 OK. - // This is not ideal, but the best we can do with the current API without implementing a full blown error handling mechanism. + // In case a of bad request we return the error message + // for more clarity. See https://docs.gitlab.com/ee/api/rest/troubleshooting.html#status-code-400 + // for more information. + if resp.StatusCode == http.StatusBadRequest { + var bErr badRequestError + if err = json.NewDecoder(resp.Body).Decode(&bErr); err == nil { + return bErr + } + log.ErrorContext(ctx, "Failed to unmarshall bad request error", "error", err) + } + if resp.StatusCode != http.StatusCreated { log.ErrorContext(ctx, "Failed to post file", "status", resp.Status) return fmt.Errorf("request failed, status is %s", resp.Status) From 451928acbf5e9c98be0ee281d0e05a470d6f64bb Mon Sep 17 00:00:00 2001 From: Bruno Bressi Date: Wed, 25 Dec 2024 18:50:47 +0100 Subject: [PATCH 2/3] chore: logs with context in target manager --- pkg/sparrow/targets/manager.go | 44 +++++++++++++++++----------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/pkg/sparrow/targets/manager.go b/pkg/sparrow/targets/manager.go index 67e6c01f..c5c49501 100644 --- a/pkg/sparrow/targets/manager.go +++ b/pkg/sparrow/targets/manager.go @@ -106,31 +106,31 @@ func (t *manager) Reconcile(ctx context.Context) error { defer registrationTimer.Stop() defer updateTimer.Stop() - log.Info("Starting target manager reconciliation") + log.InfoContext(ctx, "Starting target manager reconciliation") for { select { case <-ctx.Done(): - log.Error("Error while reconciling targets", "err", ctx.Err()) + log.ErrorContext(ctx, "Error while reconciling targets", "error", ctx.Err()) return ctx.Err() case <-t.done: - log.Info("Target manager reconciliation stopped") + log.InfoContext(ctx, "Target manager reconciliation stopped") return nil case <-checkTimer.C: err := t.refreshTargets(ctx) if err != nil { - log.Warn("Failed to get global targets", "error", err) + log.WarnContext(ctx, "Failed to get global targets", "error", err) } checkTimer.Reset(t.cfg.CheckInterval) case <-registrationTimer.C: err := t.register(ctx) if err != nil { - log.Warn("Failed to register self as global target", "error", err) + log.WarnContext(ctx, "Failed to register self as global target", "error", err) } registrationTimer.Reset(t.cfg.RegistrationInterval) case <-updateTimer.C: err := t.update(ctx) if err != nil { - log.Warn("Failed to update registration", "error", err) + log.WarnContext(ctx, "Failed to update registration", "error", err) } updateTimer.Reset(t.cfg.UpdateInterval) } @@ -151,7 +151,7 @@ func (t *manager) Shutdown(ctx context.Context) error { errC := ctx.Err() log := logger.FromContext(ctx) - log.Debug("Shut down signal received") + log.DebugContext(ctx, "Shut down signal received") ctxS, cancel := context.WithTimeout(context.Background(), shutdownTimeout) defer cancel() @@ -164,17 +164,17 @@ func (t *manager) Shutdown(ctx context.Context) error { f.SetFileName(fmt.Sprintf("%s.json", t.name)) err := t.interactor.DeleteFile(ctxS, f) if err != nil { - log.Error("Failed to shutdown gracefully", "error", err) + log.ErrorContext(ctx, "Failed to shutdown gracefully", "error", err) return fmt.Errorf("failed to shutdown gracefully: %w", errors.Join(errC, err)) } t.registered = false t.metrics.registered.Set(0) - log.Info("Successfully unregistered as global target") + log.InfoContext(ctx, "Successfully unregistered as global target") } select { case t.done <- struct{}{}: - log.Debug("Stopping gitlab reconciliation routine") + log.DebugContext(ctx, "Stopping gitlab reconciliation routine") default: } @@ -189,7 +189,7 @@ func (t *manager) register(ctx context.Context) error { defer t.mu.Unlock() if t.registered { - log.Debug("Already registered as global target") + log.DebugContext(ctx, "Already registered as global target") return nil } @@ -201,13 +201,13 @@ func (t *manager) register(ctx context.Context) error { } f.SetFileName(fmt.Sprintf("%s.json", t.name)) - log.Debug("Registering as global target") + log.DebugContext(ctx, "Registering as global target") err := t.interactor.PostFile(ctx, f) if err != nil { - log.Error("Failed to register global gitlabTargetManager", "error", err) + log.ErrorContext(ctx, "Failed to register global gitlabTargetManager", "error", err) return err } - log.Info("Successfully registered") + log.InfoContext(ctx, "Successfully registered") t.registered = true t.metrics.registered.Set(1) @@ -221,7 +221,7 @@ func (t *manager) update(ctx context.Context) error { t.mu.Lock() defer t.mu.Unlock() if !t.registered { - log.Debug("Not registered as global target, no update done.") + log.DebugContext(ctx, "Not registered as global target, no update done.") return nil } @@ -233,13 +233,13 @@ func (t *manager) update(ctx context.Context) error { } f.SetFileName(fmt.Sprintf("%s.json", t.name)) - log.Debug("Updating instance registration") + log.DebugContext(ctx, "Updating instance registration") err := t.interactor.PutFile(ctx, f) if err != nil { - log.Error("Failed to update registration", "error", err) + log.ErrorContext(ctx, "Failed to update registration", "error", err) return err } - log.Debug("Successfully updated registration") + log.DebugContext(ctx, "Successfully updated registration") return nil } @@ -251,14 +251,14 @@ func (t *manager) refreshTargets(ctx context.Context) error { var healthyTargets []checks.GlobalTarget targets, err := t.interactor.FetchFiles(ctx) if err != nil { - log.Error("Failed to update global targets", "error", err) + log.ErrorContext(ctx, "Failed to update global targets", "error", err) return err } // filter unhealthy targets - this may be removed in the future for _, target := range targets { if !t.registered && target.Url == fmt.Sprintf("%s://%s", t.cfg.Scheme, t.name) { - log.Debug("Found self as global target", "lastSeenMin", time.Since(target.LastSeen).Minutes()) + log.DebugContext(ctx, "Found self as global target", "lastSeenMinsAgo", time.Since(target.LastSeen).Minutes()) t.registered = true t.metrics.registered.Set(1) } @@ -269,14 +269,14 @@ func (t *manager) refreshTargets(ctx context.Context) error { } if time.Now().Add(-t.cfg.UnhealthyThreshold).After(target.LastSeen) { - log.Debug("Skipping unhealthy target", "target", target) + log.DebugContext(ctx, "Skipping unhealthy target", "target", target) continue } healthyTargets = append(healthyTargets, target) } t.targets = healthyTargets - log.Debug("Updated global targets", "targets", len(t.targets)) + log.DebugContext(ctx, "Updated global targets", "targets", len(t.targets)) return nil } From 88466dc674e28a8c3b360ff48e9672bd66e2e069 Mon Sep 17 00:00:00 2001 From: Bruno Bressi Date: Wed, 25 Dec 2024 19:19:20 +0100 Subject: [PATCH 3/3] refactor: generic gitlab API error message --- pkg/sparrow/targets/remote/gitlab/gitlab.go | 57 +++++++++------------ 1 file changed, 25 insertions(+), 32 deletions(-) diff --git a/pkg/sparrow/targets/remote/gitlab/gitlab.go b/pkg/sparrow/targets/remote/gitlab/gitlab.go index ef172b7e..b67d20ad 100644 --- a/pkg/sparrow/targets/remote/gitlab/gitlab.go +++ b/pkg/sparrow/targets/remote/gitlab/gitlab.go @@ -61,13 +61,15 @@ type Config struct { Branch string `yaml:"branch" mapstructure:"branch"` } -// badRequestError is an error type for bad requests to the gitlab API -type badRequestError struct { - Message string `json:"message"` +// apiError wraps non-expected API errors & status codes +// during the interaction with the gitlab API +type apiError struct { + message string + code int } -func (e badRequestError) Error() string { - return e.Message +func (e apiError) Error() string { + return fmt.Sprintf("gitlab API sent an unexpected status code (%d) with the following error message: %s", e.code, e.message) } // New creates a new gitlab client @@ -139,7 +141,7 @@ func (c *client) fetchFile(ctx context.Context, f string) (checks.GlobalTarget, if resp.StatusCode != http.StatusOK { log.ErrorContext(ctx, "Failed to fetch file", "status", resp.Status) - return res, fmt.Errorf("request failed, status is %s", resp.Status) + return res, toError(resp) } err = json.NewDecoder(resp.Body).Decode(&res) @@ -205,7 +207,7 @@ func (c *client) fetchNextFileList(ctx context.Context, reqUrl string) ([]string if resp.StatusCode != http.StatusOK { log.ErrorContext(ctx, "Failed to fetch file list", "status", resp.Status) - return nil, fmt.Errorf("request failed, status is %s", resp.Status) + return nil, toError(resp) } var fl []file @@ -271,20 +273,9 @@ func (c *client) PutFile(ctx context.Context, file remote.File) error { //nolint err = errors.Join(err, resp.Body.Close()) }() - // In case a of bad request we return the error message - // for more clarity. See https://docs.gitlab.com/ee/api/rest/troubleshooting.html#status-code-400 - // for more information. - if resp.StatusCode == http.StatusBadRequest { - var bErr badRequestError - if err = json.NewDecoder(resp.Body).Decode(&bErr); err == nil { - return bErr - } - log.ErrorContext(ctx, "Failed to unmarshall bad request error", "error", err) - } - if resp.StatusCode != http.StatusOK { log.ErrorContext(ctx, "Failed to push registration file", "status", resp.Status) - return fmt.Errorf("request failed, status is %s", resp.Status) + return toError(resp) } return nil @@ -326,20 +317,9 @@ func (c *client) PostFile(ctx context.Context, file remote.File) error { //nolin err = errors.Join(err, resp.Body.Close()) }() - // In case a of bad request we return the error message - // for more clarity. See https://docs.gitlab.com/ee/api/rest/troubleshooting.html#status-code-400 - // for more information. - if resp.StatusCode == http.StatusBadRequest { - var bErr badRequestError - if err = json.NewDecoder(resp.Body).Decode(&bErr); err == nil { - return bErr - } - log.ErrorContext(ctx, "Failed to unmarshall bad request error", "error", err) - } - if resp.StatusCode != http.StatusCreated { log.ErrorContext(ctx, "Failed to post file", "status", resp.Status) - return fmt.Errorf("request failed, status is %s", resp.Status) + return toError(resp) } return nil @@ -386,7 +366,7 @@ func (c *client) DeleteFile(ctx context.Context, file remote.File) error { //nol if resp.StatusCode != http.StatusNoContent { log.ErrorContext(ctx, "Failed to delete file", "status", resp.Status) - return fmt.Errorf("request failed, status is %s", resp.Status) + return toError(resp) } return nil @@ -454,3 +434,16 @@ func (c *client) fetchDefaultBranch() string { log.WarnContext(ctx, "No default branch found, using fallback", "fallback", fallbackBranch) return fallbackBranch } + +// toError reads the error response from the gitlab API +func toError(resp *http.Response) error { + buf := new(bytes.Buffer) + _, err := buf.ReadFrom(resp.Body) + if err != nil { + return fmt.Errorf("failed to read response body from API: %w", err) + } + return apiError{ + message: buf.String(), + code: resp.StatusCode, + } +}