Skip to content

Commit

Permalink
Feat/decode gitlab error (#246)
Browse files Browse the repository at this point in the history 
* feat: decode bad post Gitlab requests

This commit allows users to gain better insight on 4xx errors from Gitlab.

* chore: logs with context in target manager

* refactor: generic gitlab API error message
  • Loading branch information
@puffitos
puffitos authored Jan 20, 2025
1 parent 0038405 commit 86ed6cd
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 37 deletions.
44 changes: 22 additions & 22 deletions pkg/sparrow/targets/manager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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()

Expand All @@ -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:
}

Expand All @@ -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
}

Expand All @@ -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)

Expand All @@ -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
}

Expand All @@ -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
}

Expand All @@ -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)
}
Expand All @@ -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
}

Expand Down
44 changes: 29 additions & 15 deletions pkg/sparrow/targets/remote/gitlab/gitlab.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,17 @@ type Config struct {
Branch string `yaml:"branch" mapstructure:"branch"`
}

// apiError wraps non-expected API errors & status codes
// during the interaction with the gitlab API
type apiError struct {
message string
code int
}

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
func New(cfg Config) remote.Interactor {
c := &client{
Expand Down Expand Up @@ -130,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)
Expand Down Expand Up @@ -196,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
Expand Down Expand Up @@ -262,14 +273,9 @@ 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.
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
Expand Down Expand Up @@ -311,14 +317,9 @@ 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.
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
Expand Down Expand Up @@ -365,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
Expand Down Expand Up @@ -433,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,
}
}

0 comments on commit 86ed6cd

Please sign in to comment.