diff --git a/e2e/nomostest/gitproviders/bitbucket.go b/e2e/nomostest/gitproviders/bitbucket.go index 207594d497..8f88bdf217 100644 --- a/e2e/nomostest/gitproviders/bitbucket.go +++ b/e2e/nomostest/gitproviders/bitbucket.go @@ -15,20 +15,20 @@ package gitproviders import ( + "bytes" "encoding/json" "errors" "fmt" + "io" + "net/http" "net/url" "os/exec" - "strconv" "strings" "time" "github.com/GoogleContainerTools/config-sync/e2e" - "github.com/GoogleContainerTools/config-sync/e2e/nomostest/retry" + "github.com/GoogleContainerTools/config-sync/e2e/nomostest/gitproviders/util" "github.com/GoogleContainerTools/config-sync/e2e/nomostest/testlogger" - "github.com/google/uuid" - "go.uber.org/multierr" ) const ( @@ -36,9 +36,6 @@ const ( // PrivateSSHKey is secret name of the private SSH key stored in the Cloud Secret Manager. PrivateSSHKey = "config-sync-ci-ssh-private-key" - - repoNameMaxLength = 62 - localNameLength = 30 ) // BitbucketClient is the client that calls the Bitbucket REST APIs. @@ -48,16 +45,23 @@ type BitbucketClient struct { refreshToken string logger *testlogger.TestLogger workspace string + // repoSuffix is used to avoid overlap + repoSuffix string + httpClient *http.Client } // newBitbucketClient instantiates a new Bitbucket client. -func newBitbucketClient(logger *testlogger.TestLogger) (*BitbucketClient, error) { +func newBitbucketClient(repoSuffix string, logger *testlogger.TestLogger) (*BitbucketClient, error) { if *e2e.BitbucketWorkspace == "" { return nil, errors.New("bitbucket workspace cannot be empty; set with -bitbucket-workspace flag or E2E_BITBUCKET_WORKSPACE env var") } client := &BitbucketClient{ - logger: logger, - workspace: *e2e.BitbucketWorkspace, + logger: logger, + workspace: *e2e.BitbucketWorkspace, + repoSuffix: repoSuffix, + httpClient: &http.Client{ + Timeout: 10 * time.Second, + }, } var err error @@ -73,6 +77,10 @@ func newBitbucketClient(logger *testlogger.TestLogger) (*BitbucketClient, error) return client, nil } +func (b *BitbucketClient) fullName(name string) string { + return util.SanitizeBitbucketRepoName(b.repoSuffix, name) +} + // Type returns the provider type. func (b *BitbucketClient) Type() string { return e2e.Bitbucket @@ -93,25 +101,9 @@ func (b *BitbucketClient) SyncURL(name string) string { // CreateRepository calls the POST API to create a remote repository on Bitbucket. // The remote repo name is unique with a prefix of the local name. -func (b *BitbucketClient) CreateRepository(localName string) (string, error) { - u, err := uuid.NewRandom() - if err != nil { - return "", fmt.Errorf("failed to generate a new UUID: %w", err) - } - - // strip all hyphens from localName and uuid before appending - localName = strings.ReplaceAll(localName, "/", "") - localName = strings.ReplaceAll(localName, "-", "") - if len(localName) > localNameLength { - localName = localName[:localNameLength] - } - - uuid := strings.ReplaceAll(u.String(), "-", "") - - repoName := localName + uuid - if len(repoName) > repoNameMaxLength { - repoName = repoName[:repoNameMaxLength] - } +func (b *BitbucketClient) CreateRepository(name string) (string, error) { + fullName := b.fullName(name) + repoURL := fmt.Sprintf("https://api.bitbucket.org/2.0/repositories/%s/%s", b.workspace, fullName) // Create a remote repository on demand with a random localName. accessToken, err := b.refreshAccessToken() @@ -119,128 +111,134 @@ func (b *BitbucketClient) CreateRepository(localName string) (string, error) { return "", err } - out, err := exec.Command("curl", "-sX", "POST", - "-H", "Content-Type: application/json", - "-H", fmt.Sprintf("Authorization:Bearer %s", accessToken), - "-d", fmt.Sprintf("{\"scm\": \"git\",\"project\": {\"key\": \"%s\"},\"is_private\": \"true\"}", bitbucketProject), - fmt.Sprintf("https://api.bitbucket.org/2.0/repositories/%s/%s", b.workspace, repoName)).CombinedOutput() - + // Check if repository already exists + resp, err := b.sendRequest(http.MethodGet, repoURL, accessToken, nil) if err != nil { - return "", fmt.Errorf("%s: %w", string(out), err) + return "", err } - if strings.Contains(string(out), "\"type\": \"error\"") { - return "", errors.New(string(out)) + if resp.StatusCode == http.StatusOK { + return fullName, nil } - return repoName, nil -} -// DeleteRepositories calls the DELETE API to delete all remote repositories on Bitbucket. -// It deletes multiple repos in a single function in order to reuse the access_token. -func (b *BitbucketClient) DeleteRepositories(names ...string) error { - accessToken, err := b.refreshAccessToken() + payload := map[string]any{ + "scm": "git", + "project": map[string]string{ + "key": bitbucketProject, + }, + "is_private": true, + } + + // Marshal the data into JSON format + jsonPayload, err := json.Marshal(payload) if err != nil { - return err + return "", fmt.Errorf("error marshaling JSON: %w", err) } - return b.deleteRepos(accessToken, names...) -} + // Create new Bitbucket repository + resp, err = b.sendRequest(http.MethodPost, repoURL, accessToken, bytes.NewReader(jsonPayload)) -// TODO: This is a temporary workaround for the known issue -// go/acm-e2e-testing#bitbucket-repository-deletion-failure -// Until this is fixed, we are skipping removal of repos stuck in bad state -func (b *BitbucketClient) deleteRepos(accessToken string, names ...string) error { - var errs error - for _, name := range names { - _, err := retry.Retry(30*time.Second, func() error { - return b.deleteSingleRepo(accessToken, name) - }) - if err != nil { - // Skip repositories that are currently unavailable - if strings.Contains(err.Error(), "Repository currently not available") { - fmt.Printf("[BITBUCKET] Skipping repository %s: %v\n", name, err) - continue - } - errs = multierr.Append(errs, err) + defer func() { + if closeErr := resp.Body.Close(); closeErr != nil { + // Log the error as just printing it might be missed. + b.logger.Infof("failed to close response body: %v\n", closeErr) } - } - return errs -} - -// deleteSingleRepo deletes a single repository -func (b *BitbucketClient) deleteSingleRepo(accessToken, name string) error { - out, err := exec.Command("curl", "-sX", "DELETE", - "-H", fmt.Sprintf("Authorization:Bearer %s", accessToken), - fmt.Sprintf("https://api.bitbucket.org/2.0/repositories/%s/%s", - b.workspace, name)).CombinedOutput() + }() if err != nil { - return fmt.Errorf("%s: %w", string(out), err) + return "", err } - if len(out) != 0 { - return errors.New(string(out)) + body, err := io.ReadAll(resp.Body) + if err != nil { + return "", fmt.Errorf("error reading response body: %w", err) } - return nil + if resp.StatusCode != http.StatusOK { + return "", fmt.Errorf("failed to create repository: status %d: %s", resp.StatusCode, string(body)) + } + + return fullName, nil } -// DeleteObsoleteRepos deletes all repos that were created more than 24 hours ago. -func (b *BitbucketClient) DeleteObsoleteRepos() error { - accessToken, err := b.refreshAccessToken() +// sendRequest sends an HTTP request to the Bitbucket API. +func (b *BitbucketClient) sendRequest(method, url, accessToken string, body io.Reader) (*http.Response, error) { + req, err := http.NewRequest(method, url, body) if err != nil { - return err + return nil, fmt.Errorf("error creating request: %w", err) } - // Count total obsolete repos first - totalObsoleteRepos, err := b.countObsoleteRepos(accessToken) + req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", accessToken)) + req.Header.Set("Content-Type", "application/json") + + resp, err := b.httpClient.Do(req) if err != nil { - return err + return nil, fmt.Errorf("error sending request: %w", err) } - // TODO: This is a temporary workaround for the known issue - // go/acm-e2e-testing#bitbucket-repository-deletion-failure - // Until this is fixed, we are skipping removal of repos stuck in bad state. - // - // The 1000 limit is a guestimation that does not exceed the 1GB limit of - // workspace for a free account. If that number is exceeded, engineer - // will need to replace the workspace until the root cause of unresponsive - // repo is known. - if totalObsoleteRepos > 1000 { - return fmt.Errorf("total number of obsolete repositories (%d) exceeds 1000, manual cleanup required", totalObsoleteRepos) - } + return resp, nil +} - // Now delete the obsolete repos - page := 1 - for page != -1 { - page, err = b.deleteObsoleteReposByPage(accessToken, page) - if err != nil { - return err - } - } +// DeleteRepositories is a no-op because Bitbucket repo names are determined by the +// test cluster name and RSync namespace and name, so they can be reset and reused +// across test runs +func (b *BitbucketClient) DeleteRepositories(_ ...string) error { + b.logger.Info("[BITBUCKET] Skip deletion of repos") + return nil +} + +// DeleteObsoleteRepos is a no-op because Bitbucket repo names are determined by the +// test cluster name and RSync namespace and name, so it can be reused if it +// failed to be deleted after the test. +func (b *BitbucketClient) DeleteObsoleteRepos() error { return nil } func (b *BitbucketClient) refreshAccessToken() (string, error) { - out, err := exec.Command("curl", "-sX", "POST", "-u", - fmt.Sprintf("%s:%s", b.oauthKey, b.oauthSecret), - "https://bitbucket.org/site/oauth2/access_token", - "-d", "grant_type=refresh_token", - "-d", "refresh_token="+b.refreshToken).CombinedOutput() + tokenURL := "https://bitbucket.org/site/oauth2/access_token" + data := url.Values{} + data.Set("grant_type", "refresh_token") + data.Set("refresh_token", b.refreshToken) + req, err := http.NewRequest(http.MethodPost, tokenURL, strings.NewReader(data.Encode())) if err != nil { - return "", fmt.Errorf("%s: %w", string(out), err) + return "", fmt.Errorf("creating token refresh request: %w", err) } - var output map[string]interface{} - err = json.Unmarshal(out, &output) + req.SetBasicAuth(b.oauthKey, b.oauthSecret) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + resp, err := b.httpClient.Do(req) if err != nil { + return "", fmt.Errorf("sending token refresh request: %w", err) + } + defer func() { + if closeErr := resp.Body.Close(); closeErr != nil { + b.logger.Infof("failed to close response body: %v\n", closeErr) + } + }() + + body, err := io.ReadAll(resp.Body) + if err != nil { + return "", fmt.Errorf("reading token refresh response body: %w", err) + } + if resp.StatusCode != http.StatusOK { + return "", fmt.Errorf("refresh token failed with status %d", resp.StatusCode) + } + + var output map[string]interface{} + if err := json.Unmarshal(body, &output); err != nil { return "", fmt.Errorf("unmarshalling refresh token response: %w", err) } accessToken, ok := output["access_token"] if !ok { - return "", fmt.Errorf("no access_token: %s", string(out)) + return "", fmt.Errorf("no access_token in response") + } + + accessTokenStr, ok := accessToken.(string) + if !ok { + return "", fmt.Errorf("access_token is not a string: %T", accessToken) } - return accessToken.(string), nil + return accessTokenStr, nil } // FetchCloudSecret fetches secret from Google Cloud Secret Manager. @@ -255,94 +253,3 @@ func FetchCloudSecret(name string) (string, error) { } return string(out), nil } - -func (b *BitbucketClient) deleteObsoleteReposByPage(accessToken string, page int) (int, error) { - repos, nextPage, err := b.getObsoleteReposByPage(accessToken, page) - if err != nil { - return -1, err - } - - b.logger.Infof("Deleting the following repos: %s", strings.Join(repos, ", ")) - err = b.deleteRepos(accessToken, repos...) - return nextPage, err -} - -// countObsoleteRepos counts all obsolete repositories across all pages -func (b *BitbucketClient) countObsoleteRepos(accessToken string) (int, error) { - totalObsoleteRepos := 0 - page := 1 - for page != -1 { - repos, nextPage, err := b.getObsoleteReposByPage(accessToken, page) - if err != nil { - return 0, err - } - totalObsoleteRepos += len(repos) - page = nextPage - } - return totalObsoleteRepos, nil -} - -// getObsoleteReposByPage fetches obsolete repos for a given page -func (b *BitbucketClient) getObsoleteReposByPage(accessToken string, page int) ([]string, int, error) { - out, err := exec.Command("curl", "-sX", "GET", - "-H", fmt.Sprintf("Authorization:Bearer %s", accessToken), - fmt.Sprintf(`https://api.bitbucket.org/2.0/repositories/%s?q=project.key="%s"&page=%d`, - b.workspace, bitbucketProject, page)).CombinedOutput() - if err != nil { - return nil, -1, fmt.Errorf("%s: %w", string(out), err) - } - return b.filterObsoleteRepos(out) -} - -// filterObsoleteRepos extracts the names of the repos that were created more than 24 hours ago. -func (b *BitbucketClient) filterObsoleteRepos(bytes []byte) ([]string, int, error) { - nextPage := -1 - var response interface{} - if err := json.Unmarshal(bytes, &response); err != nil { - b.logger.Infof("error unmarshalling json in filterObsoleteRepos:\n%s\n", string(bytes)) - return nil, nextPage, err - } - - m := response.(map[string]interface{}) - next, found := m["next"] - if found { - u, err := url.Parse(next.(string)) - if err != nil { - return nil, nextPage, err - } - query, err := url.ParseQuery(u.RawQuery) - if err != nil { - return nil, nextPage, err - } - nextPage, err = strconv.Atoi(query.Get("page")) - if err != nil { - return nil, nextPage, err - } - } - - values, found := m["values"] - if !found { - return nil, nextPage, errors.New("no repos returned") - } - var result []string - for _, r := range values.([]interface{}) { - repo := r.(map[string]interface{}) - name, found := repo["name"] - if !found { - return nil, nextPage, errors.New("no name returned") - } - updatedOn, found := repo["updated_on"] - if !found { - return nil, nextPage, errors.New("no updated_on returned") - } - updatedTime, err := time.Parse(time.RFC3339, updatedOn.(string)) - if err != nil { - return nil, nextPage, err - } - // Only list those repos that were created more than 24 hours ago - if time.Now().After(updatedTime.Add(24 * time.Hour)) { - result = append(result, name.(string)) - } - } - return result, nextPage, nil -} diff --git a/e2e/nomostest/gitproviders/cloud_source_repository.go b/e2e/nomostest/gitproviders/cloud_source_repository.go index 6a1eba1992..b328aadbdf 100644 --- a/e2e/nomostest/gitproviders/cloud_source_repository.go +++ b/e2e/nomostest/gitproviders/cloud_source_repository.go @@ -50,7 +50,7 @@ func newCSRClient(repoPrefix string, shell *testshell.TestShell) *CSRClient { } func (c *CSRClient) fullName(name string) string { - return util.SanitizeRepoName("cs-e2e-"+c.repoPrefix, name) + return util.SanitizeGCPRepoName(c.repoPrefix, name) } // Type returns the provider type. diff --git a/e2e/nomostest/gitproviders/git-provider.go b/e2e/nomostest/gitproviders/git-provider.go index f7d35de818..a5665de105 100644 --- a/e2e/nomostest/gitproviders/git-provider.go +++ b/e2e/nomostest/gitproviders/git-provider.go @@ -51,7 +51,8 @@ type GitProvider interface { func NewGitProvider(t testing.NTB, provider, clusterName string, logger *testlogger.TestLogger, shell *testshell.TestShell) GitProvider { switch provider { case e2e.Bitbucket: - client, err := newBitbucketClient(logger) + repoSuffix := *e2e.GCPProject + "/" + clusterName + client, err := newBitbucketClient(repoSuffix, logger) if err != nil { t.Fatal(err) } diff --git a/e2e/nomostest/gitproviders/secure_source_manager.go b/e2e/nomostest/gitproviders/secure_source_manager.go index bff8e1ffe5..87f3087224 100644 --- a/e2e/nomostest/gitproviders/secure_source_manager.go +++ b/e2e/nomostest/gitproviders/secure_source_manager.go @@ -61,7 +61,7 @@ func newSSMClient(repoPrefix string, shell *testshell.TestShell, projectNumber s } func (c *SSMClient) fullName(name string) string { - return util.SanitizeRepoName(c.repoPrefix, name) + return util.SanitizeGCPRepoName(c.repoPrefix, name) } // Type returns the provider type. diff --git a/e2e/nomostest/gitproviders/util/reponame.go b/e2e/nomostest/gitproviders/util/reponame.go index 7701afe14b..fc94bc5939 100644 --- a/e2e/nomostest/gitproviders/util/reponame.go +++ b/e2e/nomostest/gitproviders/util/reponame.go @@ -22,19 +22,58 @@ import ( ) const ( - repoNameMaxLen = 63 - repoNameHashLen = 8 + defaultRepoNameMaxLen = 63 + bitbucketRepoNameMaxLen = 62 + repoNameHashLen = 8 ) -// SanitizeRepoName replaces all slashes with hyphens, and truncate the name. +// SanitizeGCPRepoName replaces all slashes with hyphens, and truncates the name. // repo name may contain between 3 and 63 lowercase letters, digits and hyphens. -func SanitizeRepoName(repoPrefix, name string) string { - fullName := "cs-e2e-" + repoPrefix + "-" + name +// This is used by CSR and SSM which are per-project resources +// The repo name will be of the form cs-e2e--- +func SanitizeGCPRepoName(repoPrefix, name string) string { + if name == "" { + return name // Requires at least the base repo name + } + fullName := "cs-e2e" + + if repoPrefix != "" { + fullName += "-" + repoPrefix + } + fullName += "-" + name + hashStr := hashName(fullName) + + return sanitize(fullName, hashStr, defaultRepoNameMaxLen) +} + +// SanitizeBitbucketRepoName replaces all slashes with hyphens, and truncates the name for Bitbucket. +// repo name may contain between 3 and 62 lowercase letters, digits and hyphens. +// The repo name will be of the form -- +func SanitizeBitbucketRepoName(repoSuffix, name string) string { + if name == "" { + return name // Requires at least the base repo name + } + + fullName := name + if repoSuffix != "" { + fullName += "-" + repoSuffix + } + hashStr := hashName(fullName) + + return sanitize(fullName, hashStr, bitbucketRepoNameMaxLen) +} + +func hashName(fullName string) string { hashBytes := sha1.Sum([]byte(fullName)) - hashStr := hex.EncodeToString(hashBytes[:])[:repoNameHashLen] + return hex.EncodeToString(hashBytes[:])[:repoNameHashLen] +} - if len(fullName) > repoNameMaxLen-1-repoNameHashLen { - fullName = fullName[:repoNameMaxLen-1-repoNameHashLen] +func sanitize(fullName, hashStr string, maxLen int) string { + if len(fullName) > maxLen-1-repoNameHashLen { + fullName = fullName[:maxLen-1-repoNameHashLen] } - return fmt.Sprintf("%s-%s", strings.ReplaceAll(fullName, "/", "-"), hashStr) + sanitizedName := strings.ReplaceAll(fullName, "/", "-") + sanitizedName = strings.TrimSuffix(sanitizedName, "-") // Avoids double dash before the hash. + + return fmt.Sprintf("%s-%s", sanitizedName, hashStr) } diff --git a/e2e/nomostest/gitproviders/util/reponame_test.go b/e2e/nomostest/gitproviders/util/reponame_test.go index 83bdf24858..daa3d66d3b 100644 --- a/e2e/nomostest/gitproviders/util/reponame_test.go +++ b/e2e/nomostest/gitproviders/util/reponame_test.go @@ -20,7 +20,7 @@ import ( "github.com/stretchr/testify/assert" ) -func TestSanitizeRepoName(t *testing.T) { +func TestSanitizeGCPRepoName(t *testing.T) { testCases := []struct { testName string repoPrefix string @@ -33,6 +33,12 @@ func TestSanitizeRepoName(t *testing.T) { repoName: "test-ns/repo-sync", expectedName: "cs-e2e-test-test-ns-repo-sync-19dcbc51", }, + { + testName: "The expected name shouldn't have a double dash in it", + repoPrefix: "my-test-cluster1", + repoName: "config-management-system/root-sync", + expectedName: "cs-e2e-my-test-cluster1-config-management-system-root-a5af55f0", + }, { testName: "RepoSync test/ns-repo-sync should not collide with RepoSync test-ns/repo-sync", repoPrefix: "test", @@ -49,14 +55,85 @@ func TestSanitizeRepoName(t *testing.T) { testName: "A very long repoName should be truncated", repoPrefix: "test", repoName: "config-management-system/root-sync-with-a-very-long-name", - expectedName: "cs-e2e-test-config-management-system-root-sync-with-a--0d0af6c0", + expectedName: "cs-e2e-test-config-management-system-root-sync-with-a-0d0af6c0", + }, + { + testName: "Empty repoPrefix", + repoPrefix: "", + repoName: "test-ns/repo-sync", + expectedName: "cs-e2e-test-ns-repo-sync-cf15cd55", + }, { + testName: "Empty repoName", + repoPrefix: "test", + repoName: "", + expectedName: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.testName, func(t *testing.T) { + gotName := SanitizeGCPRepoName(tc.repoPrefix, tc.repoName) + assert.Equal(t, tc.expectedName, gotName) + assert.LessOrEqual(t, len(gotName), defaultRepoNameMaxLen) + }) + } +} + +func TestSanitizeBitbucketRepoName(t *testing.T) { + testCases := []struct { + testName string + repoSuffix string + repoName string + expectedName string + }{ + { + testName: "RepoSync test-ns/repo-sync", + repoSuffix: "test", + repoName: "test-ns/repo-sync", + expectedName: "test-ns-repo-sync-test-3b350268", + }, + { + testName: "RepoSync test/ns-repo-sync should not collide with RepoSync test-ns/repo-sync", + repoSuffix: "test", + repoName: "test/ns-repo-sync", + expectedName: "test-ns-repo-sync-test-6831d08b", + }, + { + testName: "The expected name shouldn't have a double dash in it", + repoSuffix: "test", + repoName: "config-management-system/a-new-root-sync-with-ab-123", + expectedName: "config-management-system-a-new-root-sync-with-ab-123-da77fbd1", + }, + { + testName: "A very long repoSuffix should be truncated", + repoSuffix: "kpt-config-sync-ci-main/autopilot-rapid-latest-10", + repoName: "config-management-system/root-sync", + expectedName: "config-management-system-root-sync-kpt-config-sync-ci-6485bfa0", + }, + { + testName: "A very long repoName should be truncated", + repoSuffix: "test", + repoName: "config-management-system/root-sync-with-a-very-long-name", + expectedName: "config-management-system-root-sync-with-a-very-long-n-3b0dae1c", + }, + { + testName: "Empty repoSuffix", + repoSuffix: "", + repoName: "test-ns/repo-sync", + expectedName: "test-ns-repo-sync-08675d6b", + }, { + testName: "Empty repoName", + repoSuffix: "test", + repoName: "", + expectedName: "", }, } for _, tc := range testCases { t.Run(tc.testName, func(t *testing.T) { - gotName := SanitizeRepoName(tc.repoPrefix, tc.repoName) + gotName := SanitizeBitbucketRepoName(tc.repoSuffix, tc.repoName) assert.Equal(t, tc.expectedName, gotName) + assert.LessOrEqual(t, len(gotName), bitbucketRepoNameMaxLen) }) } }