From 296f1600f1c6f2fc79a9b8fd85b450b0d0a956be Mon Sep 17 00:00:00 2001 From: lvlcn-t <75443136+lvlcn-t@users.noreply.github.com> Date: Thu, 5 Sep 2024 12:27:15 +0000 Subject: [PATCH 1/3] feat: add configurable branches for gitlab target manager --- README.md | 10 +- pkg/sparrow/targets/manager.go | 3 - pkg/sparrow/targets/remote/gitlab/gitlab.go | 270 ++++++++++++------ .../targets/remote/gitlab/gitlab_test.go | 163 +++++++++-- pkg/sparrow/targets/remote/remote.go | 9 +- 5 files changed, 321 insertions(+), 134 deletions(-) diff --git a/README.md b/README.md index 9ce45f4a..c7aab7dd 100644 --- a/README.md +++ b/README.md @@ -272,6 +272,9 @@ targetManager: # The ID of your GitLab project. This is where Sparrow will register itself # and grab the list of other Sparrows from projectId: 18923 + # The branch to use for the state file + # If not set, it tries to resolve the default branch otherwise it uses the 'main' branch + branch: main # Configures the telemetry exporter. # Omitting this section will disable telemetry. @@ -345,6 +348,7 @@ in the startup YAML configuration file as shown in the [example configuration](# | ------------------------------------ | ----------------------------------------------------------------------------------------------------------------------------------------------- | | `targetManager.enabled` | Whether to enable the target manager. Defaults to false | | `targetManager.type` | Type of the target manager. Options: `gitlab` | +| `targetManager.scheme` | Should the target register itself as http or https. Can be `http` or `https`. This needs to be set to `https`, when `api.tls.enabled` == `true` | | `targetManager.checkInterval` | Interval for checking new targets. | | `targetManager.unhealthyThreshold` | Threshold for marking a target as unhealthy. 0 means no cleanup. | | `targetManager.registrationInterval` | Interval for registering the current sparrow at the target backend. 0 means no registration. | @@ -352,7 +356,7 @@ in the startup YAML configuration file as shown in the [example configuration](# | `targetManager.gitlab.baseUrl` | Base URL of the GitLab instance. | | `targetManager.gitlab.token` | Token for authenticating with the GitLab instance. | | `targetManager.gitlab.projectId` | Project ID for the GitLab project used as a remote state backend. | -| `targetManager.scheme` | Should the target register itself as http or https. Can be `http` or `https`. This needs to be set to `https`, when `api.tls.enabled` == `true` | +| `targetManager.gitlab.branch` | Branch to use for the state file. If not set, it tries to resolve the default branch otherwise it uses the `main` branch. | Currently, only one target manager exists: the Gitlab target manager. It uses a gitlab project as the remote state backend. The various `sparrow` instances can register themselves as targets in the project. @@ -506,8 +510,8 @@ dns: | ---------------- | ----------------- | ---------------------------------------------------------------------------- | | `interval` | `duration` | Interval to perform the Traceroute check. | | `timeout` | `duration` | Timeout for every hop. | -| `retry.count` | `integer` | Number of retries for the latency check. | -| `retry.delay` | `duration` | Initial delay between retries for the latency check. | +| `retry.count` | `integer` | Number of retries for the latency check. | +| `retry.delay` | `duration` | Initial delay between retries for the latency check. | | `maxHops` | `integer` | Maximum number of hops to try before giving up. | | `targets` | `list of objects` | List of targets to traceroute to. | | `targets[].addr` | `string` | The address of the target to traceroute to. Can be an IP address or DNS name | diff --git a/pkg/sparrow/targets/manager.go b/pkg/sparrow/targets/manager.go index 1907db1d..6da5144d 100644 --- a/pkg/sparrow/targets/manager.go +++ b/pkg/sparrow/targets/manager.go @@ -130,7 +130,6 @@ func (t *manager) Shutdown(ctx context.Context) error { if t.registered { f := remote.File{ - Branch: "main", AuthorEmail: fmt.Sprintf("%s@sparrow", t.name), AuthorName: t.name, CommitMessage: "Unregistering global target", @@ -166,7 +165,6 @@ func (t *manager) register(ctx context.Context) error { } f := remote.File{ - Branch: "main", AuthorEmail: fmt.Sprintf("%s@sparrow", t.name), AuthorName: t.name, CommitMessage: "Initial registration", @@ -198,7 +196,6 @@ func (t *manager) update(ctx context.Context) error { } f := remote.File{ - Branch: "main", AuthorEmail: fmt.Sprintf("%s@sparrow", t.name), AuthorName: t.name, CommitMessage: "Updated registration", diff --git a/pkg/sparrow/targets/remote/gitlab/gitlab.go b/pkg/sparrow/targets/remote/gitlab/gitlab.go index 55a39f04..022a7705 100644 --- a/pkg/sparrow/targets/remote/gitlab/gitlab.go +++ b/pkg/sparrow/targets/remote/gitlab/gitlab.go @@ -22,11 +22,12 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" - "io" "net/http" "net/url" "strings" + "time" "github.com/caas-team/sparrow/pkg/checks" "github.com/caas-team/sparrow/pkg/sparrow/targets/remote" @@ -38,12 +39,8 @@ var _ remote.Interactor = (*client)(nil) // client is the implementation of the remote.Interactor for gitlab type client struct { - // baseUrl is the URL of the gitlab instance - baseUrl string - // projectID is the ID of the project in the gitlab instance that contains the global targets - projectID int - // token is the personal access token used to authenticate with the gitlab instance - token string + // Config contains the configuration for the gitlab client + config Config // client is the http client used to interact with the gitlab instance client *http.Client } @@ -56,15 +53,21 @@ type Config struct { Token string `yaml:"token" mapstructure:"token"` // ProjectID is the ID of the project in the gitlab instance that contains the global targets ProjectID int `yaml:"projectId" mapstructure:"projectId"` + // Branch is the branch to use for the gitlab repository + Branch string `yaml:"branch" mapstructure:"branch"` } func New(cfg Config) remote.Interactor { - return &client{ - baseUrl: cfg.BaseURL, - token: cfg.Token, - projectID: cfg.ProjectID, - client: &http.Client{}, + c := &client{ + config: cfg, + client: &http.Client{ + Timeout: 30 * time.Second, + }, } + if c.config.Branch == "" { + c.config.Branch = c.fetchDefaultBranch() + } + return c } // FetchFiles fetches the files from the global targets repository from the configured gitlab repository @@ -72,7 +75,7 @@ func (c *client) FetchFiles(ctx context.Context) ([]checks.GlobalTarget, error) log := logger.FromContext(ctx) fl, err := c.fetchFileList(ctx) if err != nil { - log.Error("Failed to fetch files", "error", err) + log.ErrorContext(ctx, "Failed to fetch files", "error", err) return nil, err } @@ -80,160 +83,172 @@ func (c *client) FetchFiles(ctx context.Context) ([]checks.GlobalTarget, error) for _, f := range fl { gl, err := c.fetchFile(ctx, f) if err != nil { - log.Error("Failed fetching files", "error", err) + log.ErrorContext(ctx, "Failed fetching files", "error", err) return nil, err } result = append(result, gl) } - log.Info("Successfully fetched all target files", "files", len(result)) + log.InfoContext(ctx, "Successfully fetched all target files", "files", len(result)) return result, nil } // fetchFile fetches the file from the global targets repository from the configured gitlab repository -func (c *client) fetchFile(ctx context.Context, f string) (checks.GlobalTarget, error) { +func (c *client) fetchFile(ctx context.Context, f string) (res checks.GlobalTarget, err error) { log := logger.FromContext(ctx).With("file", f) - var res checks.GlobalTarget // URL encode the name n := url.PathEscape(f) req, err := http.NewRequestWithContext(ctx, http.MethodGet, - fmt.Sprintf("%s/api/v4/projects/%d/repository/files/%s/raw?ref=main", c.baseUrl, c.projectID, n), + fmt.Sprintf("%s/api/v4/projects/%d/repository/files/%s/raw", c.config.BaseURL, c.config.ProjectID, n), http.NoBody, ) if err != nil { - log.Error("Failed to create request", "error", err) + log.ErrorContext(ctx, "Failed to create request", "error", err) return res, err } - req.Header.Add("PRIVATE-TOKEN", c.token) + req.Header.Add("PRIVATE-TOKEN", c.config.Token) req.Header.Add("Content-Type", "application/json") + query := req.URL.Query() + query.Add("ref", c.config.Branch) + req.URL.RawQuery = query.Encode() - resp, err := c.client.Do(req) //nolint:bodyclose // closed in defer + resp, err := c.client.Do(req) if err != nil { - log.Error("Failed to fetch file", "error", err) + log.ErrorContext(ctx, "Failed to fetch file", "error", err) return res, err } - defer func(Body io.ReadCloser) { - err = Body.Close() - if err != nil { - log.Error("Failed to close response body", "error", err) + defer func() { + cErr := resp.Body.Close() + if cErr != nil { + log.ErrorContext(ctx, "Failed to close response body", "error", cErr) + err = errors.Join(err, cErr) } - }(resp.Body) + }() if resp.StatusCode != http.StatusOK { - log.Error("Failed to fetch file", "status", resp.Status) + log.ErrorContext(ctx, "Failed to fetch file", "status", resp.Status) return res, fmt.Errorf("request failed, status is %s", resp.Status) } err = json.NewDecoder(resp.Body).Decode(&res) if err != nil { - log.Error("Failed to decode file after fetching", "error", err) + log.ErrorContext(ctx, "Failed to decode file after fetching", "error", err) return res, err } - log.Debug("Successfully fetched file") + log.DebugContext(ctx, "Successfully fetched file") return res, nil } // fetchFileList fetches the filenames from the global targets repository from the configured gitlab repository, // so they may be fetched individually -func (c *client) fetchFileList(ctx context.Context) ([]string, error) { +func (c *client) fetchFileList(ctx context.Context) (files []string, err error) { log := logger.FromContext(ctx) - log.Debug("Fetching file list from gitlab") + log.DebugContext(ctx, "Fetching file list from gitlab") type file struct { Name string `json:"name"` } req, err := http.NewRequestWithContext(ctx, http.MethodGet, - fmt.Sprintf("%s/api/v4/projects/%d/repository/tree?ref=main", c.baseUrl, c.projectID), + fmt.Sprintf("%s/api/v4/projects/%d/repository/tree", c.config.BaseURL, c.config.ProjectID), http.NoBody, ) if err != nil { - log.Error("Failed to create request", "error", err) + log.ErrorContext(ctx, "Failed to create request", "error", err) return nil, err } - req.Header.Add("PRIVATE-TOKEN", c.token) + req.Header.Add("PRIVATE-TOKEN", c.config.Token) req.Header.Add("Content-Type", "application/json") + query := req.URL.Query() + query.Add("ref", c.config.Branch) + req.URL.RawQuery = query.Encode() - resp, err := c.client.Do(req) //nolint:bodyclose // closed in defer + resp, err := c.client.Do(req) if err != nil { - log.Error("Failed to fetch file list", "error", err) + log.ErrorContext(ctx, "Failed to fetch file list", "error", err) return nil, err } - defer func(Body io.ReadCloser) { - err = Body.Close() - if err != nil { - log.Error("Failed to close response body", "error", err) + defer func() { + cErr := resp.Body.Close() + if cErr != nil { + log.ErrorContext(ctx, "Failed to close response body", "error", cErr) + err = errors.Join(err, cErr) } - }(resp.Body) + }() if resp.StatusCode != http.StatusOK { - log.Error("Failed to fetch file list", "status", resp.Status) + log.ErrorContext(ctx, "Failed to fetch file list", "status", resp.Status) return nil, fmt.Errorf("request failed, status is %s", resp.Status) } var fl []file err = json.NewDecoder(resp.Body).Decode(&fl) if err != nil { - log.Error("Failed to decode file list", "error", err) + log.ErrorContext(ctx, "Failed to decode file list", "error", err) return nil, err } - var result []string for _, f := range fl { if strings.HasSuffix(f.Name, ".json") { - result = append(result, f.Name) + files = append(files, f.Name) } } - log.Debug("Successfully fetched file list", "files", len(result)) - return result, nil + log.DebugContext(ctx, "Successfully fetched file list", "files", len(files)) + return files, nil } // PutFile commits the current instance to the configured gitlab repository // as a global target for other sparrow instances to discover -func (c *client) PutFile(ctx context.Context, body remote.File) error { //nolint: dupl,gocritic // no need to refactor yet +func (c *client) PutFile(ctx context.Context, file remote.File) (err error) { //nolint: dupl,gocritic // no need to refactor yet log := logger.FromContext(ctx) - log.Debug("Registering sparrow instance to gitlab") + log.DebugContext(ctx, "Registering sparrow instance to gitlab") // chose method based on whether the registration has already happened - n := url.PathEscape(body.Name) - b, err := body.Bytes() + n := url.PathEscape(file.Name) + b, err := file.Serialize(c.config.Branch) if err != nil { - log.Error("Failed to create request", "error", err) + log.ErrorContext(ctx, "Failed to create request", "error", err) return err } req, err := http.NewRequestWithContext(ctx, http.MethodPut, - fmt.Sprintf("%s/api/v4/projects/%d/repository/files/%s", c.baseUrl, c.projectID, n), + fmt.Sprintf("%s/api/v4/projects/%d/repository/files/%s", c.config.BaseURL, c.config.ProjectID, n), bytes.NewBuffer(b), ) if err != nil { - log.Error("Failed to create request", "error", err) + log.ErrorContext(ctx, "Failed to create request", "error", err) return err } - req.Header.Add("PRIVATE-TOKEN", c.token) + req.Header.Add("PRIVATE-TOKEN", c.config.Token) req.Header.Add("Content-Type", "application/json") - resp, err := c.client.Do(req) //nolint:bodyclose // closed in defer + resp, err := c.client.Do(req) if err != nil { - log.Error("Failed to push registration file", "error", err) + log.ErrorContext(ctx, "Failed to push registration file", "error", err) return err } - defer func(Body io.ReadCloser) { - err = Body.Close() - if err != nil { - log.Error("Failed to close response body", "error", err) + defer func() { + cErr := resp.Body.Close() + if cErr != nil { + log.ErrorContext(ctx, "Failed to close response body", "error", cErr) + err = errors.Join(err, cErr) } - }(resp.Body) + }() + + if resp.StatusCode == http.StatusBadRequest { + log.ErrorContext(ctx, "Failed to push registration file, this is probably because the branch is protected", "status", resp.Status) + return fmt.Errorf("request failed, status is %s", resp.Status) + } if resp.StatusCode != http.StatusOK { - log.Error("Failed to push registration file", "status", resp.Status) + log.ErrorContext(ctx, "Failed to push registration file", "status", resp.Status) return fmt.Errorf("request failed, status is %s", resp.Status) } @@ -244,51 +259,56 @@ func (c *client) PutFile(ctx context.Context, body remote.File) error { //nolint // as a global target for other sparrow instances to discover func (c *client) PostFile(ctx context.Context, body remote.File) error { //nolint:dupl,gocritic // no need to refactor yet log := logger.FromContext(ctx) - log.Debug("Posting registration file to gitlab") + log.DebugContext(ctx, "Posting registration file to gitlab") // chose method based on whether the registration has already happened n := url.PathEscape(body.Name) - b, err := body.Bytes() + b, err := body.Serialize(c.config.Branch) if err != nil { - log.Error("Failed to create request", "error", err) + log.ErrorContext(ctx, "Failed to create request", "error", err) return err } req, err := http.NewRequestWithContext(ctx, http.MethodPost, - fmt.Sprintf("%s/api/v4/projects/%d/repository/files/%s", c.baseUrl, c.projectID, n), + fmt.Sprintf("%s/api/v4/projects/%d/repository/files/%s", c.config.BaseURL, c.config.ProjectID, n), bytes.NewBuffer(b), ) if err != nil { - log.Error("Failed to create request", "error", err) + log.ErrorContext(ctx, "Failed to create request", "error", err) return err } - req.Header.Add("PRIVATE-TOKEN", c.token) + req.Header.Add("PRIVATE-TOKEN", c.config.Token) req.Header.Add("Content-Type", "application/json") - resp, err := c.client.Do(req) //nolint:bodyclose // closed in defer + resp, err := c.client.Do(req) if err != nil { - log.Error("Failed to post file", "error", err) + log.ErrorContext(ctx, "Failed to post file", "error", err) return err } - defer func(Body io.ReadCloser) { - err = Body.Close() - if err != nil { - log.Error("Failed to close response body", "error", err) + defer func() { + cErr := resp.Body.Close() + if cErr != nil { + log.ErrorContext(ctx, "Failed to close response body", "error", cErr) + err = errors.Join(err, cErr) } - }(resp.Body) + }() + + if resp.StatusCode == http.StatusBadRequest { + log.ErrorContext(ctx, "Failed to post file, this is probably because the branch is protected", "status", resp.Status) + return fmt.Errorf("request failed, status is %s", resp.Status) + } if resp.StatusCode != http.StatusCreated { - log.Error("Failed to post file", "status", resp.Status) + log.ErrorContext(ctx, "Failed to post file", "status", resp.Status) return fmt.Errorf("request failed, status is %s", resp.Status) } return nil } -// DeleteFile deletes the file matching the filename from the configured -// gitlab repository +// DeleteFile deletes the file matching the filename from the configured gitlab repository func (c *client) DeleteFile(ctx context.Context, file remote.File) error { //nolint:gocritic // no performance concerns yet log := logger.FromContext(ctx).With("file", file) @@ -296,44 +316,106 @@ func (c *client) DeleteFile(ctx context.Context, file remote.File) error { //nol return fmt.Errorf("filename is empty") } - log.Debug("Deleting file from gitlab") + log.DebugContext(ctx, "Deleting file from gitlab") n := url.PathEscape(file.Name) - b, err := file.Bytes() + b, err := file.Serialize(c.config.Branch) if err != nil { - log.Error("Failed to create request", "error", err) + log.ErrorContext(ctx, "Failed to create request", "error", err) return err } req, err := http.NewRequestWithContext(ctx, http.MethodDelete, - fmt.Sprintf("%s/api/v4/projects/%d/repository/files/%s", c.baseUrl, c.projectID, n), + fmt.Sprintf("%s/api/v4/projects/%d/repository/files/%s", c.config.BaseURL, c.config.ProjectID, n), bytes.NewBuffer(b), ) if err != nil { - log.Error("Failed to create request", "error", err) + log.ErrorContext(ctx, "Failed to create request", "error", err) return err } - req.Header.Add("PRIVATE-TOKEN", c.token) + req.Header.Add("PRIVATE-TOKEN", c.config.Token) req.Header.Add("Content-Type", "application/json") - resp, err := c.client.Do(req) //nolint:bodyclose // closed in defer + resp, err := c.client.Do(req) if err != nil { - log.Error("Failed to delete file", "error", err) + log.ErrorContext(ctx, "Failed to delete file", "error", err) return err } - defer func(Body io.ReadCloser) { - err = Body.Close() - if err != nil { - log.Error("Failed to close response body", "error", err) + defer func() { + cErr := resp.Body.Close() + if cErr != nil { + log.ErrorContext(ctx, "Failed to close response body", "error", cErr) + err = errors.Join(err, cErr) } - }(resp.Body) + }() if resp.StatusCode != http.StatusNoContent { - log.Error("Failed to delete file", "status", resp.Status) + log.ErrorContext(ctx, "Failed to delete file", "status", resp.Status) return fmt.Errorf("request failed, status is %s", resp.Status) } return nil } + +const fallbackBranch = "main" + +type branch struct { + Name string `json:"name"` + Default bool `json:"default"` +} + +func (c *client) fetchDefaultBranch() string { + ctx := context.Background() + log := logger.FromContext(ctx).With("gitlab", c.config.BaseURL) + + log.DebugContext(ctx, "No branch configured, fetching default branch") + req, err := http.NewRequestWithContext(ctx, + http.MethodGet, + fmt.Sprintf("%s/api/v4/projects/%d/repository/branches", c.config.BaseURL, c.config.ProjectID), + http.NoBody, + ) + if err != nil { + log.ErrorContext(ctx, "Failed to create request", "error", err) + return fallbackBranch + } + + req.Header.Add("PRIVATE-TOKEN", c.config.Token) + req.Header.Add("Content-Type", "application/json") + + resp, err := c.client.Do(req) + if err != nil { + log.ErrorContext(ctx, "Failed to fetch branches", "error", err) + return fallbackBranch + } + + defer func() { + err = resp.Body.Close() + if err != nil { + log.ErrorContext(ctx, "Failed to close response body", "error", err) + } + }() + + if resp.StatusCode != http.StatusOK { + log.ErrorContext(ctx, "Failed to fetch branches", "status", resp.Status) + return fallbackBranch + } + + var branches []branch + err = json.NewDecoder(resp.Body).Decode(&branches) + if err != nil { + log.ErrorContext(ctx, "Failed to decode branches", "error", err) + return fallbackBranch + } + + for _, b := range branches { + if b.Default { + log.DebugContext(ctx, "Successfully fetched default branch", "branch", b.Name) + return b.Name + } + } + + log.WarnContext(ctx, "No default branch found, using fallback", "fallback", fallbackBranch) + return fallbackBranch +} diff --git a/pkg/sparrow/targets/remote/gitlab/gitlab_test.go b/pkg/sparrow/targets/remote/gitlab/gitlab_test.go index 0ee6d522..6b60aa50 100644 --- a/pkg/sparrow/targets/remote/gitlab/gitlab_test.go +++ b/pkg/sparrow/targets/remote/gitlab/gitlab_test.go @@ -111,10 +111,13 @@ func Test_gitlab_fetchFileList(t *testing.T) { httpmock.RegisterResponder("GET", "http://test/api/v4/projects/1/repository/tree?ref=main", resp) g := &client{ - baseUrl: "http://test", - projectID: 1, - token: "test", - client: http.DefaultClient, + config: Config{ + BaseURL: "http://test", + ProjectID: 1, + Token: "test", + Branch: fallbackBranch, + }, + client: http.DefaultClient, } got, err := g.fetchFileList(context.Background()) if (err != nil) != tt.wantErr { @@ -191,10 +194,13 @@ func Test_gitlab_FetchFiles(t *testing.T) { httpmock.Activate() defer httpmock.DeactivateAndReset() g := &client{ - baseUrl: "http://test", - projectID: 1, - token: "test", - client: http.DefaultClient, + config: Config{ + BaseURL: "http://test", + ProjectID: 1, + Token: "test", + Branch: fallbackBranch, + }, + client: http.DefaultClient, } for _, tt := range tests { @@ -277,10 +283,13 @@ func Test_gitlab_fetchFiles_error_cases(t *testing.T) { httpmock.Activate() defer httpmock.DeactivateAndReset() g := &client{ - baseUrl: "http://test", - projectID: 1, - token: "test", - client: http.DefaultClient, + config: Config{ + BaseURL: "http://test", + ProjectID: 1, + Token: "test", + Branch: fallbackBranch, + }, + client: http.DefaultClient, } for _, tt := range tests { @@ -317,7 +326,6 @@ func TestClient_PutFile(t *testing.T) { //nolint:dupl // no need to refactor yet { name: "success", file: remote.File{ - Branch: "main", AuthorEmail: "test@sparrow", AuthorName: "sparrpw", Content: checks.GlobalTarget{ @@ -332,7 +340,6 @@ func TestClient_PutFile(t *testing.T) { //nolint:dupl // no need to refactor yet { name: "failure - API error", file: remote.File{ - Branch: "main", AuthorEmail: "test@sparrow", AuthorName: "sparrpw", Content: checks.GlobalTarget{ @@ -354,10 +361,13 @@ func TestClient_PutFile(t *testing.T) { //nolint:dupl // no need to refactor yet httpmock.Activate() defer httpmock.DeactivateAndReset() g := &client{ - baseUrl: "http://test", - projectID: 1, - token: "test", - client: http.DefaultClient, + config: Config{ + BaseURL: "http://test", + ProjectID: 1, + Token: "test", + Branch: fallbackBranch, + }, + client: http.DefaultClient, } for _, tt := range tests { @@ -391,7 +401,6 @@ func TestClient_PostFile(t *testing.T) { //nolint:dupl // no need to refactor ye { name: "success", file: remote.File{ - Branch: "main", AuthorEmail: "test@sparrow", AuthorName: "sparrpw", Content: checks.GlobalTarget{ @@ -406,7 +415,6 @@ func TestClient_PostFile(t *testing.T) { //nolint:dupl // no need to refactor ye { name: "failure - API error", file: remote.File{ - Branch: "main", AuthorEmail: "test@sparrow", AuthorName: "sparrpw", Content: checks.GlobalTarget{ @@ -428,10 +436,13 @@ func TestClient_PostFile(t *testing.T) { //nolint:dupl // no need to refactor ye httpmock.Activate() defer httpmock.DeactivateAndReset() g := &client{ - baseUrl: "http://test", - projectID: 1, - token: "test", - client: http.DefaultClient, + config: Config{ + BaseURL: "http://test", + ProjectID: 1, + Token: "test", + Branch: fallbackBranch, + }, + client: http.DefaultClient, } for _, tt := range tests { @@ -483,10 +494,13 @@ func TestClient_DeleteFile(t *testing.T) { defer httpmock.DeactivateAndReset() projID := 1 g := &client{ - baseUrl: "http://test", - projectID: projID, - token: "test", - client: http.DefaultClient, + config: Config{ + BaseURL: "http://test", + ProjectID: 1, + Token: "test", + Branch: fallbackBranch, + }, + client: http.DefaultClient, } for _, tt := range tests { @@ -499,7 +513,6 @@ func TestClient_DeleteFile(t *testing.T) { CommitMessage: "Deleted registration file", AuthorName: "sparrow-test", AuthorEmail: "sparrow-test@sparrow", - Branch: "main", } if err := g.DeleteFile(context.Background(), f); (err != nil) != tt.wantErr { t.Fatalf("DeleteFile() error = %v, wantErr %v", err, tt.wantErr) @@ -507,3 +520,95 @@ func TestClient_DeleteFile(t *testing.T) { }) } } + +func TestClient_fetchDefaultBranch(t *testing.T) { + tests := []struct { + name string + code int + want string + response any + }{ + { + name: "success", + code: http.StatusOK, + want: "master", + response: []branch{ + { + Name: "master", + Default: true, + }, + }, + }, + { + name: "success - multiple branches", + code: http.StatusOK, + want: "release", + response: []branch{ + { + Name: "master", + Default: false, + }, + { + Name: "release", + Default: true, + }, + }, + }, + { + name: "success - multiple branches without default", + code: http.StatusOK, + want: fallbackBranch, + response: []branch{ + { + Name: "master", + Default: false, + }, + { + Name: "release", + Default: false, + }, + }, + }, + { + name: "failure - API error", + code: http.StatusInternalServerError, + want: fallbackBranch, + }, + { + name: "failure - invalid response", + code: http.StatusOK, + want: fallbackBranch, + response: struct { + Invalid bool `json:"invalid"` + }{ + Invalid: true, + }, + }, + } + + httpmock.Activate() + defer httpmock.DeactivateAndReset() + g := &client{ + config: Config{ + BaseURL: "http://test", + ProjectID: 1, + Token: "test", + }, + client: http.DefaultClient, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resp, err := httpmock.NewJsonResponder(tt.code, tt.response) + if err != nil { + t.Fatalf("error creating mock response: %v", err) + } + httpmock.RegisterResponder(http.MethodGet, "http://test/api/v4/projects/1/repository/branches", resp) + + got := g.fetchDefaultBranch() + if got != tt.want { + t.Errorf("(*client).fetchDefaultBranch() got = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/sparrow/targets/remote/remote.go b/pkg/sparrow/targets/remote/remote.go index 866ffe4e..8147d255 100644 --- a/pkg/sparrow/targets/remote/remote.go +++ b/pkg/sparrow/targets/remote/remote.go @@ -43,7 +43,6 @@ type Interactor interface { // File represents a file in the global targets repository type File struct { - Branch string AuthorEmail string AuthorName string CommitMessage string @@ -51,9 +50,9 @@ type File struct { Name string } -// Bytes returns the File as a byte array. The Content -// is base64 encoded for Gitlab API compatibility. -func (f *File) Bytes() (b []byte, err error) { +// Serialize serializes the file to a byte slice. The branch is used to determine the branch to commit to +// The serialized file is base64 encoded. +func (f *File) Serialize(branch string) (b []byte, err error) { content, err := json.Marshal(f.Content) if err != nil { return nil, err @@ -70,7 +69,7 @@ func (f *File) Bytes() (b []byte, err error) { return nil, err } return json.Marshal(map[string]string{ - "branch": f.Branch, + "branch": branch, "author_email": f.AuthorEmail, "author_name": f.AuthorName, "content": string(content), From f221146955b96ff3ba1f817eae26f4a2cc41a487 Mon Sep 17 00:00:00 2001 From: lvlcn-t <75443136+lvlcn-t@users.noreply.github.com> Date: Thu, 5 Sep 2024 22:32:19 +0200 Subject: [PATCH 2/3] refactor: simplify error handling * refactor: simplify error handling * refactor: use unnamed return values * chore: rm special error handling case in favor of comment on special case --- pkg/sparrow/targets/remote/gitlab/gitlab.go | 70 +++++++++------------ 1 file changed, 31 insertions(+), 39 deletions(-) diff --git a/pkg/sparrow/targets/remote/gitlab/gitlab.go b/pkg/sparrow/targets/remote/gitlab/gitlab.go index 022a7705..ce91ef11 100644 --- a/pkg/sparrow/targets/remote/gitlab/gitlab.go +++ b/pkg/sparrow/targets/remote/gitlab/gitlab.go @@ -39,7 +39,7 @@ var _ remote.Interactor = (*client)(nil) // client is the implementation of the remote.Interactor for gitlab type client struct { - // Config contains the configuration for the gitlab client + // config contains the configuration for the gitlab client config Config // client is the http client used to interact with the gitlab instance client *http.Client @@ -57,6 +57,7 @@ type Config struct { Branch string `yaml:"branch" mapstructure:"branch"` } +// New creates a new gitlab client func New(cfg Config) remote.Interactor { c := &client{ config: cfg, @@ -93,8 +94,9 @@ func (c *client) FetchFiles(ctx context.Context) ([]checks.GlobalTarget, error) } // fetchFile fetches the file from the global targets repository from the configured gitlab repository -func (c *client) fetchFile(ctx context.Context, f string) (res checks.GlobalTarget, err error) { +func (c *client) fetchFile(ctx context.Context, f string) (checks.GlobalTarget, error) { log := logger.FromContext(ctx).With("file", f) + var res checks.GlobalTarget // URL encode the name n := url.PathEscape(f) req, err := http.NewRequestWithContext(ctx, @@ -119,11 +121,7 @@ func (c *client) fetchFile(ctx context.Context, f string) (res checks.GlobalTarg } defer func() { - cErr := resp.Body.Close() - if cErr != nil { - log.ErrorContext(ctx, "Failed to close response body", "error", cErr) - err = errors.Join(err, cErr) - } + err = errors.Join(err, resp.Body.Close()) }() if resp.StatusCode != http.StatusOK { @@ -143,7 +141,7 @@ func (c *client) fetchFile(ctx context.Context, f string) (res checks.GlobalTarg // fetchFileList fetches the filenames from the global targets repository from the configured gitlab repository, // so they may be fetched individually -func (c *client) fetchFileList(ctx context.Context) (files []string, err error) { +func (c *client) fetchFileList(ctx context.Context) ([]string, error) { log := logger.FromContext(ctx) log.DebugContext(ctx, "Fetching file list from gitlab") type file struct { @@ -173,11 +171,7 @@ func (c *client) fetchFileList(ctx context.Context) (files []string, err error) } defer func() { - cErr := resp.Body.Close() - if cErr != nil { - log.ErrorContext(ctx, "Failed to close response body", "error", cErr) - err = errors.Join(err, cErr) - } + err = errors.Join(err, resp.Body.Close()) }() if resp.StatusCode != http.StatusOK { @@ -192,6 +186,7 @@ func (c *client) fetchFileList(ctx context.Context) (files []string, err error) return nil, err } + var files []string for _, f := range fl { if strings.HasSuffix(f.Name, ".json") { files = append(files, f.Name) @@ -204,7 +199,7 @@ func (c *client) fetchFileList(ctx context.Context) (files []string, err error) // PutFile commits the current instance to the configured gitlab repository // as a global target for other sparrow instances to discover -func (c *client) PutFile(ctx context.Context, file remote.File) (err error) { //nolint: dupl,gocritic // no need to refactor yet +func (c *client) PutFile(ctx context.Context, file remote.File) error { //nolint: dupl,gocritic // no need to refactor yet log := logger.FromContext(ctx) log.DebugContext(ctx, "Registering sparrow instance to gitlab") @@ -235,18 +230,14 @@ func (c *client) PutFile(ctx context.Context, file remote.File) (err error) { // } defer func() { - cErr := resp.Body.Close() - if cErr != nil { - log.ErrorContext(ctx, "Failed to close response body", "error", cErr) - err = errors.Join(err, cErr) - } + err = errors.Join(err, resp.Body.Close()) }() - if resp.StatusCode == http.StatusBadRequest { - log.ErrorContext(ctx, "Failed to push registration file, this is probably because the branch is protected", "status", resp.Status) - return fmt.Errorf("request failed, status is %s", resp.Status) - } - + // 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) @@ -257,13 +248,13 @@ func (c *client) PutFile(ctx context.Context, file remote.File) (err error) { // // PostFile commits the current instance to the configured gitlab repository // as a global target for other sparrow instances to discover -func (c *client) PostFile(ctx context.Context, body remote.File) error { //nolint:dupl,gocritic // no need to refactor yet +func (c *client) PostFile(ctx context.Context, file remote.File) error { //nolint:dupl,gocritic // no need to refactor yet log := logger.FromContext(ctx) log.DebugContext(ctx, "Posting registration file to gitlab") // chose method based on whether the registration has already happened - n := url.PathEscape(body.Name) - b, err := body.Serialize(c.config.Branch) + n := url.PathEscape(file.Name) + b, err := file.Serialize(c.config.Branch) if err != nil { log.ErrorContext(ctx, "Failed to create request", "error", err) return err @@ -288,18 +279,14 @@ func (c *client) PostFile(ctx context.Context, body remote.File) error { //nolin } defer func() { - cErr := resp.Body.Close() - if cErr != nil { - log.ErrorContext(ctx, "Failed to close response body", "error", cErr) - err = errors.Join(err, cErr) - } + err = errors.Join(err, resp.Body.Close()) }() - if resp.StatusCode == http.StatusBadRequest { - log.ErrorContext(ctx, "Failed to post file, this is probably because the branch is protected", "status", resp.Status) - return fmt.Errorf("request failed, status is %s", resp.Status) - } - + // 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) @@ -359,13 +346,18 @@ func (c *client) DeleteFile(ctx context.Context, file remote.File) error { //nol return nil } +// fallbackBranch is the branch to use if no default branch is found const fallbackBranch = "main" +// branch is a struct to decode the branches from the gitlab API type branch struct { - Name string `json:"name"` - Default bool `json:"default"` + // Name is the name of the branch + Name string `json:"name"` + // Default is true if the branch is the default branch + Default bool `json:"default"` } +// fetchDefaultBranch fetches the default branch from the gitlab API func (c *client) fetchDefaultBranch() string { ctx := context.Background() log := logger.FromContext(ctx).With("gitlab", c.config.BaseURL) From 23efc4ea7adfe9a956198dc39f4ddc83b967ac29 Mon Sep 17 00:00:00 2001 From: lvlcn-t <75443136+lvlcn-t@users.noreply.github.com> Date: Tue, 10 Sep 2024 18:36:56 +0200 Subject: [PATCH 3/3] chore: simplify response body error handling --- pkg/sparrow/targets/remote/gitlab/gitlab.go | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/pkg/sparrow/targets/remote/gitlab/gitlab.go b/pkg/sparrow/targets/remote/gitlab/gitlab.go index ce91ef11..510adf39 100644 --- a/pkg/sparrow/targets/remote/gitlab/gitlab.go +++ b/pkg/sparrow/targets/remote/gitlab/gitlab.go @@ -331,11 +331,7 @@ func (c *client) DeleteFile(ctx context.Context, file remote.File) error { //nol } defer func() { - cErr := resp.Body.Close() - if cErr != nil { - log.ErrorContext(ctx, "Failed to close response body", "error", cErr) - err = errors.Join(err, cErr) - } + err = errors.Join(err, resp.Body.Close()) }() if resp.StatusCode != http.StatusNoContent { @@ -383,10 +379,7 @@ func (c *client) fetchDefaultBranch() string { } defer func() { - err = resp.Body.Close() - if err != nil { - log.ErrorContext(ctx, "Failed to close response body", "error", err) - } + err = errors.Join(err, resp.Body.Close()) }() if resp.StatusCode != http.StatusOK {