From 0ebf2ce7b21e1f8e0d6f46494061d34bc5c5c3ba Mon Sep 17 00:00:00 2001 From: Renzo Rojas Silva Date: Tue, 10 Sep 2024 08:19:14 -0400 Subject: [PATCH] feat: tests for new native client and changes in other places to make it work with new implementation --- integration/remote_config_dependency_test.go | 102 ++++++ pkg/skaffold/deploy/kubectl/kubectl_test.go | 12 +- pkg/skaffold/gcs/client/native.go | 250 ++++++++----- pkg/skaffold/gcs/client/native_test.go | 365 +++++++++++++++++++ pkg/skaffold/gcs/gsutil.go | 14 +- pkg/skaffold/gcs/gsutil_test.go | 20 +- pkg/skaffold/kubernetes/manifest/gcs.go | 11 +- 7 files changed, 663 insertions(+), 111 deletions(-) create mode 100644 pkg/skaffold/gcs/client/native_test.go diff --git a/integration/remote_config_dependency_test.go b/integration/remote_config_dependency_test.go index 706c3b81929..412ff643be4 100644 --- a/integration/remote_config_dependency_test.go +++ b/integration/remote_config_dependency_test.go @@ -120,3 +120,105 @@ requires: }) } } + +func TestRenderWithRemoteGCS(t *testing.T) { + tests := []struct { + description string + configFile string + args []string + shouldErr bool + expectedOutput string + expectedErrMsg string + }{ + { + description: "download all repo with same folders from subfolder", + configFile: `apiVersion: skaffold/v4beta11 +kind: Config +requires: + - googleCloudStorage: + source: gs://skaffold-remote-dependency-e2e-tests/test1/* + path: ./skaffold.yaml +`, + args: []string{"--tag", "fixed", "--default-repo=", "--digest-source", "tag"}, + expectedOutput: `apiVersion: v1 +kind: Pod +metadata: + name: getting-started +spec: + containers: + - image: skaffold-example:fixed + name: getting-started`, + }, + { + description: "download full repo with top sub folder", + configFile: `apiVersion: skaffold/v4beta11 +kind: Config +requires: + - googleCloudStorage: + source: gs://skaffold-remote-dependency-e2e-tests/test1 + path: ./test1/skaffold.yaml +`, + args: []string{"--tag", "fixed", "--default-repo=", "--digest-source", "tag"}, + expectedOutput: `apiVersion: v1 +kind: Pod +metadata: + name: getting-started +spec: + containers: + - image: skaffold-example:fixed + name: getting-started`, + }, + { + description: "download full repo with bucket name as top folder", + configFile: `apiVersion: skaffold/v4beta11 +kind: Config +requires: + - googleCloudStorage: + source: gs://skaffold-remote-dependency-e2e-tests + path: ./skaffold-remote-dependency-e2e-tests/test1/skaffold.yaml +`, + args: []string{"--tag", "fixed", "--default-repo=", "--digest-source", "tag"}, + expectedOutput: `apiVersion: v1 +kind: Pod +metadata: + name: getting-started +spec: + containers: + - image: skaffold-example:fixed + name: getting-started`, + }, + { + description: "download only all yaml files across bucket", + configFile: `apiVersion: skaffold/v4beta11 +kind: Config +requires: + - googleCloudStorage: + source: gs://skaffold-remote-dependency-e2e-tests/test1/**.yaml + path: ./skaffold.yaml +`, + args: []string{"--tag", "fixed", "--default-repo=", "--digest-source", "tag", "-p", "flat-structure"}, + expectedOutput: `apiVersion: v1 +kind: Pod +metadata: + name: getting-started +spec: + containers: + - image: skaffold-example:fixed + name: getting-started`, + }, + } + + for _, test := range tests { + testutil.Run(t, test.description, func(t *testutil.T) { + MarkIntegrationTest(t.T, NeedsGcp) + tmpDirRemoteRepo := t.NewTempDir() + tmpDirTest := t.NewTempDir() + + tmpDirTest.Write("skaffold.yaml", test.configFile) + args := append(test.args, "--remote-cache-dir", tmpDirRemoteRepo.Root()) + output, err := skaffold.Render(args...).InDir(tmpDirTest.Root()).RunWithCombinedOutput(t.T) + t.CheckNoError(err) + t.CheckDeepEqual(test.expectedOutput, string(output), testutil.YamlObj(t.T)) + }) + } +} diff --git a/pkg/skaffold/deploy/kubectl/kubectl_test.go b/pkg/skaffold/deploy/kubectl/kubectl_test.go index f638bb91bb2..5863ad340a0 100644 --- a/pkg/skaffold/deploy/kubectl/kubectl_test.go +++ b/pkg/skaffold/deploy/kubectl/kubectl_test.go @@ -23,7 +23,6 @@ import ( "fmt" "io" "os" - "path/filepath" "testing" "time" @@ -40,6 +39,11 @@ import ( "github.com/GoogleContainerTools/skaffold/v2/testutil" ) +type gcsClientMock struct{} + +func (g gcsClientMock) DownloadRecursive(ctx context.Context, src, dst string) error { + return nil +} func TestKubectlV1RenderDeploy(t *testing.T) { tests := []struct { description string @@ -520,13 +524,15 @@ func TestGCSManifests(t *testing.T) { RawK8s: []string{"gs://dev/deployment.yaml"}, }, commands: testutil. - CmdRunOut(fmt.Sprintf("gsutil cp -r %s %s", "gs://dev/deployment.yaml", filepath.Join(manifest.ManifestTmpDir, manifest.ManifestsFromGCS)), "log"). - AndRun("kubectl --context kubecontext --namespace testNamespace apply -f -"), + CmdRun("kubectl --context kubecontext --namespace testNamespace apply -f -"), }} for _, test := range tests { testutil.Run(t, test.description, func(t *testutil.T) { t.Override(&client.Client, deployutil.MockK8sClient) t.Override(&util.DefaultExecCommand, test.commands) + t.Override(&manifest.GetGCSClient, func() manifest.GCSClient { + return gcsClientMock{} + }) if err := os.MkdirAll(manifest.ManifestTmpDir, os.ModePerm); err != nil { t.Fatal(err) } diff --git a/pkg/skaffold/gcs/client/native.go b/pkg/skaffold/gcs/client/native.go index d6771ed7b5e..f5f5418e6f2 100644 --- a/pkg/skaffold/gcs/client/native.go +++ b/pkg/skaffold/gcs/client/native.go @@ -31,7 +31,21 @@ import ( "google.golang.org/api/iterator" ) -// Obj contains information about the GCS object. +var GetBucketManager = getBucketManager + +// bucketHandler defines the available interactions with a GCS bucket. +type bucketHandler interface { + // ListObjects lists the objects that match the given query. + ListObjects(ctx context.Context, q *storage.Query) ([]string, error) + // DownloadObject downloads the object with the given uri in the localPath. + DownloadObject(ctx context.Context, localPath, uri string) error + // UploadObject creates a files with the given content with the objName. + UploadObject(ctx context.Context, objName string, content *os.File) error + // Close closes the bucket handler connection. + Close() +} + +// uriInfo contains information about the GCS object URI. type uriInfo struct { // Bucket is the name of the GCS bucket. Bucket string @@ -48,17 +62,16 @@ type Native struct{} // Downloads the content that match the given src uri and subfolders. func (n *Native) DownloadRecursive(ctx context.Context, src, dst string) error { - sc, err := storage.NewClient(ctx) + uriInfo, err := n.parseGCSURI(src) if err != nil { - return fmt.Errorf("error creating GCS Client: %w", err) + return err } - defer sc.Close() - uriInfo, err := n.parseGCSURI(src) + bucket, err := GetBucketManager(ctx, uriInfo.Bucket) if err != nil { return err } - bucket := sc.Bucket(uriInfo.Bucket) + defer bucket.Close() files, err := n.filesToDownload(ctx, bucket, uriInfo) if err != nil { @@ -67,7 +80,14 @@ func (n *Native) DownloadRecursive(ctx context.Context, src, dst string) error { for uri, localPath := range files { fullPath := filepath.Join(dst, localPath) - if err := n.downloadFile(ctx, bucket, fullPath, uri); err != nil { + dir := filepath.Dir(fullPath) + if _, err := os.Stat(dir); os.IsNotExist(err) { + if err := os.MkdirAll(dir, os.ModePerm); err != nil { + return fmt.Errorf("failed to create directory: %v", err) + } + } + + if err := bucket.DownloadObject(ctx, fullPath, uri); err != nil { return err } } @@ -77,30 +97,28 @@ func (n *Native) DownloadRecursive(ctx context.Context, src, dst string) error { // Uploads a single file to the given dst. func (n *Native) UploadFile(ctx context.Context, src, dst string) error { - sc, err := storage.NewClient(ctx) - if err != nil { - return fmt.Errorf("error creating GCS Client: %w", err) - } - defer sc.Close() - f, err := os.Open(src) if err != nil { return fmt.Errorf("error opening file: %w", err) } defer f.Close() - uinfo, err := n.parseGCSURI(dst) + urinfo, err := n.parseGCSURI(dst) + if err != nil { + return err + } + + bucket, err := GetBucketManager(ctx, urinfo.Bucket) if err != nil { return err } - bucket := sc.Bucket(uinfo.Bucket) - isDirectory, err := n.isGCSDirectory(ctx, bucket, uinfo) + isDirectory, err := n.isGCSDirectory(ctx, bucket, urinfo) if err != nil { return err } - dstObj := uinfo.ObjPath + dstObj := urinfo.ObjPath if isDirectory { dstObj, err = url.JoinPath(dstObj, filepath.Base(src)) if err != nil { @@ -108,14 +126,7 @@ func (n *Native) UploadFile(ctx context.Context, src, dst string) error { } } - wc := bucket.Object(dstObj).NewWriter(ctx) - if _, err = io.Copy(wc, f); err != nil { - return fmt.Errorf("error copying file to GCS: %w", err) - } - if err := wc.Close(); err != nil { - return fmt.Errorf("error closing GCS writer: %w", err) - } - return nil + return bucket.UploadObject(ctx, dstObj, f) } func (n *Native) parseGCSURI(uri string) (uriInfo, error) { @@ -131,15 +142,20 @@ func (n *Native) parseGCSURI(uri string) (uriInfo, error) { return uriInfo{}, errors.New("bucket name is empty") } gcsobj.Bucket = u.Host + // If we do this with the url package it will scape the `?` character, breaking the glob. gcsobj.ObjPath = strings.TrimLeft(strings.ReplaceAll(uri, "gs://"+u.Host, ""), "/") return gcsobj, nil } -func (n *Native) filesToDownload(ctx context.Context, bucket *storage.BucketHandle, uinfo uriInfo) (map[string]string, error) { +func (n *Native) filesToDownload(ctx context.Context, bucket bucketHandler, urinfo uriInfo) (map[string]string, error) { uriToLocalPath := map[string]string{} - exactMatches, err := n.listObjects(ctx, bucket, &storage.Query{MatchGlob: uinfo.ObjPath}) + // The exact match is with the original glob expression. This could be: + // 1. a/b/c -> It will return the file `c` under a/b/ if it exists + // 2. a/b/c* -> It will return any file under a/b/ that starts with c, e.g, c1, c-other, etc + // 3. a/b/c** -> It will return any file that starts with 'c', and files inside any folder starting with 'c'. It is doing the recursion already + exactMatches, err := bucket.ListObjects(ctx, &storage.Query{MatchGlob: urinfo.ObjPath}) if err != nil { return nil, err } @@ -148,58 +164,40 @@ func (n *Native) filesToDownload(ctx context.Context, bucket *storage.BucketHand uriToLocalPath[match] = filepath.Base(match) } - recursiveMatches, err := n.recursiveListing(ctx, bucket, uinfo) + // Then, to mimic gsutil behavior, we assume the last part of the glob is a folder, so we complete the + // URI with the necessary wildcard to list the folder recursively. + recursiveMatches, err := n.recursiveListing(ctx, bucket, urinfo) if err != nil { return nil, err } - for _, match := range recursiveMatches { - uriToLocalPath[match] = match + for uri, match := range recursiveMatches { + uriToLocalPath[uri] = match } return uriToLocalPath, nil } -func (n *Native) listObjects(ctx context.Context, bucket *storage.BucketHandle, q *storage.Query) ([]string, error) { - matches := []string{} - it := bucket.Objects(ctx, q) - - for { - attrs, err := it.Next() - if err == iterator.Done { - break - } - - if err != nil { - return nil, fmt.Errorf("failed to iterate objects: %v", err) - } - - if attrs.Name != "" { - matches = append(matches, attrs.Name) - } - } - return matches, nil -} - -func (n *Native) recursiveListing(ctx context.Context, bucket *storage.BucketHandle, uinfo uriInfo) (map[string]string, error) { +func (n *Native) recursiveListing(ctx context.Context, bucket bucketHandler, urinfo uriInfo) (map[string]string, error) { uriToLocalPath := map[string]string{} - recursiveURI := n.uriForRecursiveSearch(uinfo.ObjPath) - recursiveMatches, err := n.listObjects(ctx, bucket, &storage.Query{MatchGlob: recursiveURI}) + recursiveURI := n.uriForRecursiveSearch(urinfo.ObjPath) + recursiveMatches, err := bucket.ListObjects(ctx, &storage.Query{MatchGlob: recursiveURI}) if err != nil { return nil, err } - prefixRemovalURI := n.uriForPrefixRemoval(uinfo.Full()) + prefixRemovalURI := n.uriForPrefixRemoval(urinfo.Full()) prefixRemovalRegex, err := n.wildcardToRegex(prefixRemovalURI) if err != nil { return nil, err } - shouldRecreateFolders := !strings.Contains(uinfo.ObjPath, "**") + // For glob patterns that have `**` (anywhere), gsutil doesn't recreate the folder structure. + shouldRecreateFolders := !strings.Contains(urinfo.ObjPath, "**") for _, match := range recursiveMatches { destPath := filepath.Base(match) if shouldRecreateFolders { - matchWithBucket := uinfo.Bucket + "/" + match + matchWithBucket := urinfo.Bucket + "/" + match destPath = string(prefixRemovalRegex.ReplaceAll([]byte(matchWithBucket), []byte(""))) } uriToLocalPath[match] = destPath @@ -208,37 +206,43 @@ func (n *Native) recursiveListing(ctx context.Context, bucket *storage.BucketHan return uriToLocalPath, nil } -func (n *Native) uriForRecursiveSearch(src string) string { +// uriForRecursiveSearch returns a modified URI is to cover globs like a/*/d*, to remove its prefix: +// For the case where the bucket has the following files: +// - a/b/d/sub1/file1 +// - a/c/d/sub2/sub3/file2 +// - a/e/d2/file2 +// The resulting files + folders should be: +// - d/sub1/file1 +// - d/sub2/sub3/file2 +// - d2/file2 +func (n *Native) uriForRecursiveSearch(uri string) string { // when we want to list all the bucket - if src == "" { + if uri == "" { return "**" } - - // a/b** or a/b/** - if strings.HasSuffix(src, "**") { - return src + // uri is a/b** or a/b/** + if strings.HasSuffix(uri, "**") { + return uri } - // a/b* and a/b/* become a/b** and a/b/** - if strings.HasSuffix(src, "*") { - return src + "*" + if strings.HasSuffix(uri, "*") { + return uri + "*" } // a/b/ becomes a/b/** - if strings.HasSuffix(src, "/") { - return src + "**" + if strings.HasSuffix(uri, "/") { + return uri + "**" } - // a/b becomes a/b/** - return src + "/**" + return uri + "/**" } -func (n *Native) uriForPrefixRemoval(src string) string { - if strings.HasSuffix(src, "/*") { - return strings.TrimSuffix(src, "*") +func (n *Native) uriForPrefixRemoval(uri string) string { + if strings.HasSuffix(uri, "/*") { + return strings.TrimSuffix(uri, "*") } - src = strings.TrimSuffix(src, "/") - idx := strings.LastIndex(src, "/") - return src[:idx+1] + uri = strings.TrimSuffix(uri, "/") + idx := strings.LastIndex(uri, "/") + return uri[:idx+1] } func (n *Native) wildcardToRegex(wildcard string) (*regexp.Regexp, error) { @@ -254,15 +258,70 @@ func (n *Native) wildcardToRegex(wildcard string) (*regexp.Regexp, error) { return regexp.Compile(regexStr) } -func (n *Native) downloadFile(ctx context.Context, bucket *storage.BucketHandle, localPath, uri string) error { - dir := filepath.Dir(localPath) - if _, err := os.Stat(dir); os.IsNotExist(err) { - if err := os.MkdirAll(dir, os.ModePerm); err != nil { - return fmt.Errorf("failed to create directory: %v", err) +func (n *Native) isGCSDirectory(ctx context.Context, bucket bucketHandler, urinfo uriInfo) (bool, error) { + if urinfo.ObjPath == "" { + return true, nil + } + + if strings.HasSuffix(urinfo.ObjPath, "/") { + return true, nil + } + + q := &storage.Query{Prefix: urinfo.ObjPath + "/"} + // GCS doesn't support empty "folders". + matches, err := bucket.ListObjects(ctx, q) + if err != nil { + return false, err + } + + if len(matches) > 0 { + return true, nil + } + + return false, nil +} + +func getBucketManager(ctx context.Context, bucketName string) (bucketHandler, error) { + sc, err := storage.NewClient(ctx) + if err != nil { + return nil, fmt.Errorf("error creating GCS Client: %w", err) + } + + return nativeBucketHandler{ + storageClient: sc, + bucket: sc.Bucket(bucketName), + }, nil +} + +// nativeBucketHandler implements a handler using the Cloud client libraries. +type nativeBucketHandler struct { + storageClient *storage.Client + bucket *storage.BucketHandle +} + +func (nb nativeBucketHandler) ListObjects(ctx context.Context, q *storage.Query) ([]string, error) { + matches := []string{} + it := nb.bucket.Objects(ctx, q) + + for { + attrs, err := it.Next() + if err == iterator.Done { + break + } + + if err != nil { + return nil, fmt.Errorf("failed to iterate objects: %v", err) + } + + if attrs.Name != "" { + matches = append(matches, attrs.Name) } } + return matches, nil +} - reader, err := bucket.Object(uri).NewReader(ctx) +func (nb nativeBucketHandler) DownloadObject(ctx context.Context, localPath, uri string) error { + reader, err := nb.bucket.Object(uri).NewReader(ctx) if err != nil { return fmt.Errorf("failed to read object: %v", err) } @@ -281,24 +340,17 @@ func (n *Native) downloadFile(ctx context.Context, bucket *storage.BucketHandle, return nil } -func (n *Native) isGCSDirectory(ctx context.Context, bucket *storage.BucketHandle, uinfo uriInfo) (bool, error) { - if uinfo.ObjPath == "" { - return true, nil - } - - if strings.HasSuffix(uinfo.ObjPath, "/") { - return true, nil - } - - q := &storage.Query{Prefix: uinfo.ObjPath + "/"} - matches, err := n.listObjects(ctx, bucket, q) - if err != nil { - return false, err +func (nb nativeBucketHandler) UploadObject(ctx context.Context, objName string, content *os.File) error { + wc := nb.bucket.Object(objName).NewWriter(ctx) + if _, err := io.Copy(wc, content); err != nil { + return fmt.Errorf("error copying file to GCS: %w", err) } - - if len(matches) > 0 { - return true, nil + if err := wc.Close(); err != nil { + return fmt.Errorf("error closing GCS writer: %w", err) } + return nil +} - return false, nil +func (nb nativeBucketHandler) Close() { + nb.storageClient.Close() } diff --git a/pkg/skaffold/gcs/client/native_test.go b/pkg/skaffold/gcs/client/native_test.go new file mode 100644 index 00000000000..25a9a20f788 --- /dev/null +++ b/pkg/skaffold/gcs/client/native_test.go @@ -0,0 +1,365 @@ +/* +Copyright 2024 The Skaffold Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package client + +import ( + "context" + "os" + "path/filepath" + "regexp" + "strings" + "testing" + + "cloud.google.com/go/storage" + "github.com/bmatcuk/doublestar" + + "github.com/GoogleContainerTools/skaffold/v2/testutil" +) + +// Regex to transform a/b** to a/b*/** +var escapeDoubleStarWithoutSlashRegex = regexp.MustCompile(`([^/])\*\*`) + +type repoHandlerMock struct { + root string + downloadedFiles map[string]string + uploadedFile string +} + +func (r *repoHandlerMock) filterOnlyFiles(paths []string) ([]string, error) { + matches := []string{} + for _, m := range paths { + fileInfo, err := os.Stat(m) + if err != nil { + return nil, err + } + + if !fileInfo.IsDir() { + withoutPrefix := strings.TrimPrefix(m, r.root+string(filepath.Separator)) + matches = append(matches, filepath.ToSlash(withoutPrefix)) + } + } + return matches, nil +} + +func (r *repoHandlerMock) matchGlob(matchGlob string) ([]string, error) { + glob := escapeDoubleStarWithoutSlashRegex.ReplaceAllString(matchGlob, `${1}*/**`) + glob = filepath.Join(r.root, glob) + globMatches, err := doublestar.Glob(glob) + if err != nil { + return nil, err + } + + return r.filterOnlyFiles(globMatches) +} + +func (r *repoHandlerMock) withPrefix(prefix string) ([]string, error) { + path := filepath.Join(r.root, prefix+"**") + matches, err := doublestar.Glob(path) + if err != nil { + return nil, err + } + + return r.filterOnlyFiles(matches) +} + +func (r *repoHandlerMock) ListObjects(ctx context.Context, q *storage.Query) ([]string, error) { + if q.MatchGlob != "" { + return r.matchGlob(q.MatchGlob) + } + + if q.Prefix != "" { + return r.withPrefix(q.Prefix) + } + + return nil, nil +} + +func (r *repoHandlerMock) DownloadObject(ctx context.Context, localPath, uri string) error { + r.downloadedFiles[uri] = localPath + return nil +} + +func (r *repoHandlerMock) UploadObject(ctx context.Context, objName string, content *os.File) error { + r.uploadedFile = objName + return nil +} + +func (r *repoHandlerMock) Close() {} + +func TestDownloadRecursive(t *testing.T) { + tests := []struct { + name string + uri string + dst string + availableFiles []string + expectedDownloadedFiles map[string]string + }{ + { + name: "exact match with flat output", + uri: "gs://bucket/dir1/manifest1.yaml", + dst: "download", + availableFiles: []string{ + "dir1/manifest1.yaml", + "manifest2.yaml", + "main.go", + }, + expectedDownloadedFiles: map[string]string{ + "dir1/manifest1.yaml": "download/manifest1.yaml", + }, + }, + { + name: "exact match with wildcard, flat output", + uri: "gs://bucket/*/manifest[12].yaml", + dst: "download", + availableFiles: []string{ + "dir1/manifest1.yaml", + "dir1/manifest3.yaml", + "dir2/manifest2.yaml", + }, + expectedDownloadedFiles: map[string]string{ + "dir1/manifest1.yaml": "download/manifest1.yaml", + "dir2/manifest2.yaml": "download/manifest2.yaml", + }, + }, + { + name: "exact match with ? wildcard with flat output", + uri: "gs://bucket/*/manifest?.yaml", + dst: "download", + availableFiles: []string{ + "dir1/manifest1.yaml", + "dir2/manifest2.yaml", + }, + expectedDownloadedFiles: map[string]string{ + "dir1/manifest1.yaml": "download/manifest1.yaml", + "dir2/manifest2.yaml": "download/manifest2.yaml", + }, + }, + { + name: "recursive match with folders creation", + uri: "gs://bucket/dir*", + dst: "download", + availableFiles: []string{ + "dir1/manifest1.yaml", + "dir2/manifest2.yaml", + }, + expectedDownloadedFiles: map[string]string{ + "dir1/manifest1.yaml": "download/dir1/manifest1.yaml", + "dir2/manifest2.yaml": "download/dir2/manifest2.yaml", + }, + }, + { + name: "recursive match with flat output", + uri: "gs://bucket/dir**", + dst: "download", + availableFiles: []string{ + "dir1/manifest1.yaml", + "dir2/manifest2.yaml", + }, + expectedDownloadedFiles: map[string]string{ + "dir1/manifest1.yaml": "download/manifest1.yaml", + "dir2/manifest2.yaml": "download/manifest2.yaml", + }, + }, + { + name: "recursive match from bucket with folders creation", + uri: "gs://bucket", + dst: "download", + availableFiles: []string{ + "dir1/manifest1.yaml", + "dir2/manifest2.yaml", + }, + expectedDownloadedFiles: map[string]string{ + "dir1/manifest1.yaml": "download/bucket/dir1/manifest1.yaml", + "dir2/manifest2.yaml": "download/bucket/dir2/manifest2.yaml", + }, + }, + { + name: "recursive match all bucket content with folders creation", + uri: "gs://bucket/*", + dst: "download", + availableFiles: []string{ + "dir1/manifest1.yaml", + "dir1/sub1/main.go", + "dir2/manifest2.yaml", + }, + expectedDownloadedFiles: map[string]string{ + "dir1/manifest1.yaml": "download/dir1/manifest1.yaml", + "dir1/sub1/main.go": "download/dir1/sub1/main.go", + "dir2/manifest2.yaml": "download/dir2/manifest2.yaml", + }, + }, + { + name: "recursive match all bucket content with flat structure", + uri: "gs://bucket/**", + dst: "download", + availableFiles: []string{ + "dir1/manifest1.yaml", + "dir1/sub1/main.go", + "manifest2.yaml", + }, + expectedDownloadedFiles: map[string]string{ + "dir1/manifest1.yaml": "download/manifest1.yaml", + "dir1/sub1/main.go": "download/main.go", + "manifest2.yaml": "download/manifest2.yaml", + }, + }, + { + name: "recursive match with folder creating and prefix removal", + uri: "gs://bucket/submodule/*/content/*", + dst: "download", + availableFiles: []string{ + "submodule/a/content/dir1/manifest1.yaml", + "submodule/b/content/dir2/manifest2.yaml", + "submodule/c/content/dir3/Dockerfile", + "submodule/dir4/main.go", + }, + expectedDownloadedFiles: map[string]string{ + "submodule/a/content/dir1/manifest1.yaml": "download/dir1/manifest1.yaml", + "submodule/b/content/dir2/manifest2.yaml": "download/dir2/manifest2.yaml", + "submodule/c/content/dir3/Dockerfile": "download/dir3/Dockerfile", + }, + }, + { + name: "recursive match with matching folder creating and prefix removal", + uri: "gs://bucket/submodule/*/content*", + dst: "download", + availableFiles: []string{ + "submodule/a/content1/dir1/manifest1.yaml", + "submodule/b/content2/dir2/manifest2.yaml", + }, + expectedDownloadedFiles: map[string]string{ + "submodule/a/content1/dir1/manifest1.yaml": "download/content1/dir1/manifest1.yaml", + "submodule/b/content2/dir2/manifest2.yaml": "download/content2/dir2/manifest2.yaml", + }, + }, + { + name: "no match", + uri: "gs://bucket/**/*.go", + dst: "download", + availableFiles: []string{ + "submodule/a/content/dir1/manifest1.yaml", + "submodule/b/content/dir2/manifest2.yaml", + "submodule/c/content/dir3/Dockerfile", + }, + expectedDownloadedFiles: map[string]string{}, + }, + } + + for _, test := range tests { + testutil.Run(t, test.name, func(t *testutil.T) { + td := t.NewTempDir() + td.Touch(test.availableFiles...) + root := td.Root() + + rh := &repoHandlerMock{ + root: root, + downloadedFiles: make(map[string]string), + } + t.Override(&GetBucketManager, func(ctx context.Context, bucketName string) (bucketHandler, error) { + return rh, nil + }) + + n := Native{} + err := n.DownloadRecursive(context.TODO(), test.uri, test.dst) + t.CheckNoError(err) + + for uri, local := range test.expectedDownloadedFiles { + test.expectedDownloadedFiles[uri] = filepath.FromSlash(local) + } + + t.CheckMapsMatch(test.expectedDownloadedFiles, rh.downloadedFiles) + }) + } +} + +func TestUploadFile(t *testing.T) { + tests := []struct { + name string + uri string + localFile string + availableFiles []string + expectedCreatedFile string + shouldError bool + }{ + { + name: "upload file to existing folder using local name", + uri: "gs://bucket/folder", + localFile: "manifest.yaml", + availableFiles: []string{ + "folder/main.go", + }, + expectedCreatedFile: "folder/manifest.yaml", + }, + { + name: "upload file to existing folder using new name", + uri: "gs://bucket/folder/newmanifest.yaml", + localFile: "manifest.yaml", + availableFiles: []string{ + "folder/main.go", + }, + expectedCreatedFile: "folder/newmanifest.yaml", + }, + { + name: "upload file to not existing subfolder using local name", + uri: "gs://bucket/folder/newfolder/", + localFile: "manifest.yaml", + availableFiles: []string{ + "folder/main.go", + }, + expectedCreatedFile: "folder/newfolder/manifest.yaml", + }, + { + name: "upload file to not existing subfolder using new name", + uri: "gs://bucket/folder/newfolder/newmanifest.yaml", + localFile: "manifest.yaml", + availableFiles: []string{ + "folder/main.go", + }, + expectedCreatedFile: "folder/newfolder/newmanifest.yaml", + }, + { + name: "upload file to root of bucket", + uri: "gs://bucket", + localFile: "manifest.yaml", + expectedCreatedFile: "manifest.yaml", + }, + } + + for _, test := range tests { + testutil.Run(t, test.name, func(t *testutil.T) { + bucketTd := t.NewTempDir() + bucketTd.Touch(test.availableFiles...) + bucketRoot := bucketTd.Root() + rh := &repoHandlerMock{ + root: bucketRoot, + downloadedFiles: make(map[string]string), + } + t.Override(&GetBucketManager, func(ctx context.Context, bucketName string) (bucketHandler, error) { + return rh, nil + }) + + localFileTd := t.NewTempDir() + localFileTd.Touch(test.localFile) + locaFullPath := filepath.Join(localFileTd.Root(), test.localFile) + + n := Native{} + err := n.UploadFile(context.TODO(), locaFullPath, test.uri) + t.CheckNoError(err) + t.CheckDeepEqual(test.expectedCreatedFile, rh.uploadedFile) + }) + } +} diff --git a/pkg/skaffold/gcs/gsutil.go b/pkg/skaffold/gcs/gsutil.go index 88def26d5b0..ab1a2ac181c 100644 --- a/pkg/skaffold/gcs/gsutil.go +++ b/pkg/skaffold/gcs/gsutil.go @@ -55,6 +55,16 @@ func (g *Gsutil) Copy(ctx context.Context, src, dst string, recursive bool) erro return nil } +// GetGCSClient returns a GCS client that uses Client libraries. +var GetGCSClient = func() gscClient { + return &client.Native{} +} + +type gscClient interface { + // Downloads the content that match the given src uri and subfolders. + DownloadRecursive(ctx context.Context, src, dst string) error +} + // SyncObjects syncs the target Google Cloud Storage objects with skaffold's local cache and returns the local path to the objects. func SyncObjects(ctx context.Context, g latest.GoogleCloudStorageInfo, opts config.SkaffoldOptions) (string, error) { remoteCacheDir, err := config.GetRemoteCacheDir(opts) @@ -90,8 +100,8 @@ func SyncObjects(ctx context.Context, g latest.GoogleCloudStorageInfo, opts conf } } - native := client.Native{} - if err := native.DownloadRecursive(ctx, g.Source, cacheDir); err != nil { + gcs := GetGCSClient() + if err := gcs.DownloadRecursive(ctx, g.Source, cacheDir); err != nil { return "", fmt.Errorf("failed to cache Google Cloud Storage objects from %q: %w", g.Source, err) } return cacheDir, nil diff --git a/pkg/skaffold/gcs/gsutil_test.go b/pkg/skaffold/gcs/gsutil_test.go index 87651bf4b9b..6cd46d547fb 100644 --- a/pkg/skaffold/gcs/gsutil_test.go +++ b/pkg/skaffold/gcs/gsutil_test.go @@ -77,6 +77,14 @@ func TestCopy(t *testing.T) { } } +type gcsClientMock struct { + err error +} + +func (g gcsClientMock) DownloadRecursive(ctx context.Context, src, dst string) error { + return g.err +} + func TestSyncObject(t *testing.T) { source := "gs://my-bucket/dir1/*" path := "configs/skaffold.yaml" @@ -144,13 +152,13 @@ func TestSyncObject(t *testing.T) { _ = syncRemote.Set(test.syncFlag) opts := config.SkaffoldOptions{RemoteCacheDir: td.Root(), SyncRemoteCache: *syncRemote} - var cmd *testutil.FakeCmd - if test.gsutilErr == nil { - cmd = testutil.CmdRunOut(fmt.Sprintf("gsutil cp -r %s %s", source, td.Path(sourceHash)), "logs") - } else { - cmd = testutil.CmdRunOutErr(fmt.Sprintf("gsutil cp -r %s %s", source, td.Path(sourceHash)), "logs", test.gsutilErr) + gcsClient := gcsClientMock{} + if test.gsutilErr != nil { + gcsClient.err = test.gsutilErr } - t.Override(&util.DefaultExecCommand, cmd) + t.Override(&GetGCSClient, func() gscClient { + return gcsClient + }) path, err := SyncObjects(context.Background(), test.g, opts) var expected string diff --git a/pkg/skaffold/kubernetes/manifest/gcs.go b/pkg/skaffold/kubernetes/manifest/gcs.go index d72dba3242e..44faa319074 100644 --- a/pkg/skaffold/kubernetes/manifest/gcs.go +++ b/pkg/skaffold/kubernetes/manifest/gcs.go @@ -28,6 +28,15 @@ import ( var ManifestsFromGCS = "manifests_from_gcs" +type GCSClient interface { + // Downloads the content that match the given src uri and subfolders. + DownloadRecursive(ctx context.Context, src, dst string) error +} + +var GetGCSClient = func() GCSClient { + return &client.Native{} +} + // DownloadFromGCS downloads all provided manifests from a remote GCS bucket, // and returns a relative path pointing to the GCS temp dir. func DownloadFromGCS(manifests []string) (string, error) { @@ -39,7 +48,7 @@ func DownloadFromGCS(manifests []string) (string, error) { if manifest == "" || !strings.HasPrefix(manifest, gcsPrefix) { return "", fmt.Errorf("%v is not a valid GCS path", manifest) } - gcs := client.Native{} + gcs := GetGCSClient() if err := gcs.DownloadRecursive(context.Background(), manifest, dir); err != nil { return "", fmt.Errorf("failed to download manifests fom GCS: %w", err) }