From fc52140e5b23c9a22cbb9ef5e3803c2fb9a14b83 Mon Sep 17 00:00:00 2001 From: gbenhaim Date: Thu, 4 Jan 2024 11:05:19 +0200 Subject: [PATCH] RHTAP-1134: Support custom Gitlab instances When creating a Gitlab client, the base url of the Gitlab instance should be provided (unless the default is used, which is https://gitlab.com). With this change, it would be possible to use Gitlab instance other than gitlab.com. In addition, fixed an issue in "getWebhookByTargetUrl" where it tried to access a nil pointer of a request. Signed-off-by: gbenhaim --- pkg/git/gitlab/gitlab_client.go | 6 +- pkg/git/gitlab/gitlab_helper.go | 46 +++++++ pkg/git/gitlab/gitlab_helper_debug_test.go | 35 +++++- pkg/git/gitlab/gitlab_helper_test.go | 115 ++++++++++++++++++ .../gitproviderfactory/gitproviderfactory.go | 6 +- .../gitproviderfactory_test.go | 23 +++- 6 files changed, 219 insertions(+), 12 deletions(-) diff --git a/pkg/git/gitlab/gitlab_client.go b/pkg/git/gitlab/gitlab_client.go index f31a5315..3c9a783d 100644 --- a/pkg/git/gitlab/gitlab_client.go +++ b/pkg/git/gitlab/gitlab_client.go @@ -27,7 +27,7 @@ import ( ) // Allow mocking for tests -var NewGitlabClient func(accessToken string) (*GitlabClient, error) = newGitlabClient +var NewGitlabClient func(accessToken, baseUrl string) (*GitlabClient, error) = newGitlabClient var _ gp.GitProviderClient = (*GitlabClient)(nil) @@ -330,9 +330,9 @@ func (g *GitlabClient) GetConfiguredGitAppName() (string, string, error) { return "", "", fmt.Errorf("GitLab does not support applications") } -func newGitlabClient(accessToken string) (*GitlabClient, error) { +func newGitlabClient(accessToken, baseUrl string) (*GitlabClient, error) { glc := &GitlabClient{} - c, err := gitlab.NewClient(accessToken) + c, err := gitlab.NewClient(accessToken, gitlab.WithBaseURL(baseUrl)) if err != nil { return nil, err } diff --git a/pkg/git/gitlab/gitlab_helper.go b/pkg/git/gitlab/gitlab_helper.go index 29568d71..79c14dc9 100644 --- a/pkg/git/gitlab/gitlab_helper.go +++ b/pkg/git/gitlab/gitlab_helper.go @@ -30,6 +30,31 @@ import ( "net/url" ) +type FailedToParseUrlError struct { + url string + err string +} + +func (e FailedToParseUrlError) Error() string { + return fmt.Sprintf("Failed to parse url: %s, error: %s", e.url, e.err) +} + +type MissingSchemaError struct { + url string +} + +func (e MissingSchemaError) Error() string { + return fmt.Sprintf("Failed to detect schema in url %s", e.url) +} + +type MissingHostError struct { + url string +} + +func (e MissingHostError) Error() string { + return fmt.Sprintf("Failed to detect host in url %s", e.url) +} + func getProjectPathFromRepoUrl(repoUrl string) (string, error) { url, err := url.Parse(repoUrl) if err != nil { @@ -42,6 +67,24 @@ func getProjectPathFromRepoUrl(repoUrl string) (string, error) { ), nil } +func GetBaseUrl(repoUrl string) (string, error) { + url, err := url.Parse(repoUrl) + if err != nil { + return "", FailedToParseUrlError{url: repoUrl, err: err.Error()} + } + + if url.Scheme == "" { + return "", MissingSchemaError{repoUrl} + } + + if url.Host == "" { + return "", MissingHostError{repoUrl} + } + + // The gitlab client library expects the base url to have a tailing slash + return fmt.Sprintf("%s://%s/", url.Scheme, url.Host), nil +} + // refineGitHostingServiceError generates expected permanent error from GitHub response. // If no one is detected, the original error will be returned. // refineGitHostingServiceError should be called just after every GitHub API call. @@ -280,6 +323,9 @@ func (g *GitlabClient) getWebhookByTargetUrl(projectPath, webhookTargetUrl strin opts := &gitlab.ListProjectHooksOptions{PerPage: 100} webhooks, resp, err := g.client.Projects.ListProjectHooks(projectPath, opts) if err != nil { + if resp == nil { + return nil, err + } return nil, refineGitHostingServiceError(resp.Response, err) } for _, webhook := range webhooks { diff --git a/pkg/git/gitlab/gitlab_helper_debug_test.go b/pkg/git/gitlab/gitlab_helper_debug_test.go index ec8286fb..d5b0de43 100644 --- a/pkg/git/gitlab/gitlab_helper_debug_test.go +++ b/pkg/git/gitlab/gitlab_helper_debug_test.go @@ -38,7 +38,12 @@ func TestEnsurePaCMergeRequest(t *testing.T) { return } - glclient, err := NewGitlabClient(accessToken) + baseUrl, err := GetBaseUrl(repoUrl) + if err != nil { + t.Fatal(err) + } + + glclient, err := NewGitlabClient(accessToken, baseUrl) if err != nil { t.Fatal(err) } @@ -75,7 +80,12 @@ func TestUndoPaCMergeRequest(t *testing.T) { return } - glclient, err := NewGitlabClient(accessToken) + baseUrl, err := GetBaseUrl(repoUrl) + if err != nil { + t.Fatal(err) + } + + glclient, err := NewGitlabClient(accessToken, baseUrl) if err != nil { t.Fatal(err) } @@ -112,7 +122,12 @@ func TestSetupPaCWebhook(t *testing.T) { targetWebhookUrl := "https://pac.route.my-cluster.net" webhookSecretString := "d01b38971dad59514298d763f288392c08221043" - glclient, err := NewGitlabClient(accessToken) + baseUrl, err := GetBaseUrl(repoUrl) + if err != nil { + t.Fatal(err) + } + + glclient, err := NewGitlabClient(accessToken, baseUrl) if err != nil { t.Fatal(err) } @@ -130,7 +145,12 @@ func TestDeletePaCWebhook(t *testing.T) { targetWebhookUrl := "https://pac.route.my-cluster.net" - glclient, err := NewGitlabClient(accessToken) + baseUrl, err := GetBaseUrl(repoUrl) + if err != nil { + t.Fatal(err) + } + + glclient, err := NewGitlabClient(accessToken, baseUrl) if err != nil { t.Fatal(err) } @@ -146,7 +166,12 @@ func TestIsRepositoryPublic(t *testing.T) { return } - glclient, err := NewGitlabClient(accessToken) + baseUrl, err := GetBaseUrl(repoUrl) + if err != nil { + t.Fatal(err) + } + + glclient, err := NewGitlabClient(accessToken, baseUrl) if err != nil { t.Fatal(err) } diff --git a/pkg/git/gitlab/gitlab_helper_test.go b/pkg/git/gitlab/gitlab_helper_test.go index 75ae37c4..396f92ba 100644 --- a/pkg/git/gitlab/gitlab_helper_test.go +++ b/pkg/git/gitlab/gitlab_helper_test.go @@ -56,3 +56,118 @@ func TestGetProjectPathFromRepoUrl(t *testing.T) { } } } + +func TestGetHostFromRepoUrl(t *testing.T) { + var params = []struct { + in string + out string + err error + }{ + { + "https://gitlab.host.com/rhtap/tenants-config.git", + "https://gitlab.host.com/", + nil, + }, + { + "https://gitlab.com/projectname", + "https://gitlab.com/", + nil, + }, + { + "http!!://abc", + "", + FailedToParseUrlError{}, + }, + } + + for _, tt := range params { + t.Run(tt.in, func(t *testing.T) { + actual, err := GetBaseUrl(tt.in) + if err != nil && tt.err == nil { + t.Fatalf("Expected call to succeed, found error %s", err.Error()) + } + if err == nil && tt.err != nil { + t.Fatalf("Expected call to end with an error, but it succeed") + } + if actual != tt.out { + t.Fatalf("Expected %s found %s", tt.out, actual) + } + }) + } +} + +func TestGetHostFromRepoUrlFailToParseUrl(t *testing.T) { + var params = []struct { + in string + out string + }{ + { + "http!!://abc", + "", + }, + } + + for _, tt := range params { + t.Run(tt.in, func(t *testing.T) { + actual, err := GetBaseUrl(tt.in) + if actual != "" { + t.Fatalf("Expected an empty string result") + } + if _, ok := err.(FailedToParseUrlError); !ok { + t.Fatalf("Expected to received error of type FailedToParseUrlError got %T", err) + } + }) + } +} + +func TestGetHostFromRepoUrlMissingSchema(t *testing.T) { + var params = []struct { + in string + out string + }{ + { + "gitlab.com", + "", + }, + { + "gitlab.cee.redhat.com", + "", + }, + } + + for _, tt := range params { + t.Run(tt.in, func(t *testing.T) { + actual, err := GetBaseUrl(tt.in) + if actual != "" { + t.Fatalf("Expected an empty string result") + } + if _, ok := err.(MissingSchemaError); !ok { + t.Fatalf("Expected to received error of type MissingSchemaError got %T", err) + } + }) + } +} + +func TestGetHostFromRepoUrlMissingHost(t *testing.T) { + var params = []struct { + in string + out string + }{ + { + "http:///abc", + "", + }, + } + + for _, tt := range params { + t.Run(tt.in, func(t *testing.T) { + actual, err := GetBaseUrl(tt.in) + if actual != "" { + t.Fatalf("Expected an empty string result") + } + if _, ok := err.(MissingHostError); !ok { + t.Fatalf("Expected to received error of type MissingHostError got %T", err) + } + }) + } +} diff --git a/pkg/git/gitproviderfactory/gitproviderfactory.go b/pkg/git/gitproviderfactory/gitproviderfactory.go index d90f981e..da1db0ee 100644 --- a/pkg/git/gitproviderfactory/gitproviderfactory.go +++ b/pkg/git/gitproviderfactory/gitproviderfactory.go @@ -105,7 +105,11 @@ func createGitClient(gitClientConfig GitClientConfig) (gitprovider.GitProviderCl if isAppUsed { return nil, fmt.Errorf("GitLab does not have applications") } - return gitlab.NewGitlabClient(accessToken) + baseUrl, err := gitlab.GetBaseUrl(gitClientConfig.RepoUrl) + if err != nil { + return nil, err + } + return gitlab.NewGitlabClient(accessToken, baseUrl) case "bitbucket": return nil, boerrors.NewBuildOpError(boerrors.EUnknownGitProvider, fmt.Errorf("git provider %s is not supported", gitProvider)) diff --git a/pkg/git/gitproviderfactory/gitproviderfactory_test.go b/pkg/git/gitproviderfactory/gitproviderfactory_test.go index 9e24d7a9..821c255c 100644 --- a/pkg/git/gitproviderfactory/gitproviderfactory_test.go +++ b/pkg/git/gitproviderfactory/gitproviderfactory_test.go @@ -40,7 +40,7 @@ func TestGetContainerImageRepository(t *testing.T) { t.Errorf("should not be invoked") return nil } - gitlab.NewGitlabClient = func(accessToken string) (*gitlab.GitlabClient, error) { + gitlab.NewGitlabClient = func(accessToken, baseUrl string) (*gitlab.GitlabClient, error) { t.Errorf("should not be invoked") return nil, nil } @@ -213,16 +213,33 @@ func TestGetContainerImageRepository(t *testing.T) { "gitlab_token": []byte("token"), }, GitProvider: "gitlab", - RepoUrl: repoUrl, + RepoUrl: "https://gitlab.com/my-org/my-repo", IsAppInstallationExpected: true, }, allowConstructors: func() { - gitlab.NewGitlabClient = func(accessToken string) (*gitlab.GitlabClient, error) { + gitlab.NewGitlabClient = func(accessToken, baseUrl string) (*gitlab.GitlabClient, error) { + expectedBaseUrl := "https://gitlab.com/" + if baseUrl != expectedBaseUrl { + return nil, fmt.Errorf("Expected to get baseUrl: %s, got %s", expectedBaseUrl, baseUrl) + } return &gitlab.GitlabClient{}, nil } }, expectError: false, }, + { + name: "should fail to create Gitlab client since the base url can't be detected", + gitClientConfig: GitClientConfig{ + PacSecretData: map[string][]byte{ + "gitlab_token": []byte("token"), + }, + GitProvider: "gitlab", + RepoUrl: "https://", + IsAppInstallationExpected: false, + }, + allowConstructors: func() {}, + expectError: true, + }, { name: "should not create BitBucket client", gitClientConfig: GitClientConfig{