From 06da3b96a278838f1eeb1fd099a80636801482f1 Mon Sep 17 00:00:00 2001 From: Anna Song Date: Thu, 5 Jan 2023 11:47:59 -0500 Subject: [PATCH] Localize resources (#4912) * Localize resources * Improve readability * Add integration tests * Group test helper functions --- api/internal/localizer/errors.go | 13 +- api/internal/localizer/localizer.go | 51 ++- api/internal/localizer/localizer_test.go | 50 +++ api/krusty/localizer/runner_test.go | 377 ++++++++++++++++++++++- 4 files changed, 466 insertions(+), 25 deletions(-) diff --git a/api/internal/localizer/errors.go b/api/internal/localizer/errors.go index 35e69a39b..90437b420 100644 --- a/api/internal/localizer/errors.go +++ b/api/internal/localizer/errors.go @@ -10,9 +10,18 @@ type ResourceLoadError struct { FileError error } -var _ error = ResourceLoadError{} - func (rle ResourceLoadError) Error() string { return fmt.Sprintf(`when parsing as inline received error: %s when parsing as filepath received error: %s`, rle.InlineError, rle.FileError) } + +type PathLocalizeError struct { + Path string + FileError error + RootError error +} + +func (ple PathLocalizeError) Error() string { + return fmt.Sprintf(`could not localize path %q as file: %s; could not localize path %q as directory: %s`, + ple.Path, ple.FileError, ple.Path, ple.RootError) +} diff --git a/api/internal/localizer/localizer.go b/api/internal/localizer/localizer.go index d117396fd..f53b8ae56 100644 --- a/api/internal/localizer/localizer.go +++ b/api/internal/localizer/localizer.go @@ -148,11 +148,15 @@ func (lc *localizer) localizeNativeFields(kust *types.Kustomization) error { kust.Crds, lc.localizeFile, }, + "resources": { + kust.Resources, + lc.localizeResource, + }, } { for i, path := range field.paths { locPath, err := field.locFn(path) if err != nil { - return errors.WrapPrefixf(err, "unable to localize %s path", fieldName) + return errors.WrapPrefixf(err, "unable to localize %s entry", fieldName) } field.paths[i] = locPath } @@ -198,8 +202,6 @@ func (lc *localizer) localizeNativeFields(kust *types.Kustomization) error { kust.Replacements[i].Path = newPath } } - - // TODO(annasong): localize all other kustomization fields: resources return nil } @@ -254,6 +256,39 @@ func (lc *localizer) localizePatches(patches []types.Patch) error { return nil } +// localizeResource localizes resource path, a file or root, and returns the +// localized path +func (lc *localizer) localizeResource(path string) (string, error) { + var locPath string + + content, fileErr := lc.ldr.Load(path) + // The following check catches the case where path is a repo root. + // Load on a repo will successfully return its README in HTML. + // Because HTML does not follow resource formatting, we then correctly try + // to localize path as a root. + if fileErr == nil { + _, resErr := lc.rFactory.NewResMapFromBytes(content) + if resErr != nil { + fileErr = errors.WrapPrefixf(resErr, "invalid resource at file %q", path) + } else { + locPath, fileErr = lc.localizeFileWithContent(path, content) + } + } + if fileErr != nil { + var rootErr error + locPath, rootErr = lc.localizeDir(path) + if rootErr != nil { + err := PathLocalizeError{ + Path: path, + FileError: fileErr, + RootError: rootErr, + } + return "", err + } + } + return locPath, nil +} + // localizeFile localizes file path and returns the localized path func (lc *localizer) localizeFile(path string) (string, error) { content, err := lc.ldr.Load(path) @@ -267,8 +302,9 @@ func (lc *localizer) localizeFile(path string) (string, error) { func (lc *localizer) localizeFileWithContent(path string, content []byte) (string, error) { var locPath string if loader.IsRemoteFile(path) { - // TODO(annasong): You need to check if you can add a localize directory here to store - // the remote file content. There may be a directory that shares the localize directory name. + if lc.fSys.Exists(lc.root.Join(LocalizeDir)) { + return "", errors.Errorf("%s already contains %s needed to store file %q", lc.root, LocalizeDir, path) + } locPath = locFilePath(path) } else { // ldr has checked that path must be relative; this is subject to change in beta. @@ -306,8 +342,9 @@ func (lc *localizer) localizeDir(path string) (string, error) { } var locPath string if repo := ldr.Repo(); repo != "" { - // TODO(annasong): You need to check if you can add a localize directory here to store - // the remote file content. There may be a directory that shares the localize directory name. + if lc.fSys.Exists(lc.root.Join(LocalizeDir)) { + return "", errors.Errorf("%s already contains %s needed to store root %q", lc.root, LocalizeDir, path) + } locPath, err = locRootPath(path, repo, root, lc.fSys) if err != nil { return "", err diff --git a/api/internal/localizer/localizer_test.go b/api/internal/localizer/localizer_test.go index bbfc5b08c..83d5584de 100644 --- a/api/internal/localizer/localizer_test.go +++ b/api/internal/localizer/localizer_test.go @@ -1013,3 +1013,53 @@ nameSuffix: -test } checkLocalizeInTargetSuccess(t, kustAndComponents) } + +func TestLocalizeResources(t *testing.T) { + kustAndResources := map[string]string{ + "kustomization.yaml": `apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +resources: +- pod.yaml +- ../../alpha +`, + "pod.yaml": podConfiguration, + "../../alpha/kustomization.yaml": `apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +namePrefix: my- +`, + } + expected, actual := makeFileSystems(t, "/a/b", kustAndResources) + + err := Run("/a/b", "/", "", actual) + require.NoError(t, err) + + addFiles(t, expected, "/localized-b/a/b", kustAndResources) + checkFSys(t, expected, actual) +} + +func TestLocalizePathError(t *testing.T) { + kustAndResources := map[string]string{ + "kustomization.yaml": `apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +resources: +- b +`, + } + expected, actual := makeFileSystems(t, "/a", kustAndResources) + + err := Run("/a", "/", "", actual) + + const expectedFileErr = `invalid file reference: '/a/b' must resolve to a file` + const expectedRootErr = `unable to localize root "b": unable to find one of 'kustomization.yaml', 'kustomization.yml' or 'Kustomization' in directory '/a/b'` + var actualErr PathLocalizeError + require.ErrorAs(t, err, &actualErr) + require.Equal(t, "b", actualErr.Path) + require.EqualError(t, actualErr.FileError, expectedFileErr) + require.EqualError(t, actualErr.RootError, expectedRootErr) + + const expectedErrPrefix = `unable to localize target "/a": unable to localize resources entry` + require.EqualError(t, err, fmt.Sprintf(`%s: could not localize path "b" as file: %s; could not localize path "b" as directory: %s`, + expectedErrPrefix, expectedFileErr, expectedRootErr)) + + checkFSys(t, expected, actual) +} diff --git a/api/krusty/localizer/runner_test.go b/api/krusty/localizer/runner_test.go index bcd3759d5..a16419ef1 100644 --- a/api/krusty/localizer/runner_test.go +++ b/api/krusty/localizer/runner_test.go @@ -141,9 +141,68 @@ const ( } } ` + + simpleURL = "https://github.com/kubernetes-sigs/kustomize//api/krusty/testdata/localize/simple" + + simpleKustomization = `apiVersion: kustomize.config.k8s.io/v1beta1 +kind: Kustomization +namePrefix: localize- +resources: +- deployment.yaml +- service.yaml +` + + simpleDeployment = `apiVersion: apps/v1 +kind: Deployment +metadata: + name: test-deployment-simple + labels: + app: deployment-simple +spec: + selector: + matchLabels: + app: simple + template: + metadata: + labels: + app: simple + spec: + containers: + - name: nginx + image: nginx:1.16 + ports: + - containerPort: 8080 +` + simpleService = `apiVersion: v1 +kind: Service +metadata: + name: test-service-simple +spec: + selector: + app: deployment-simple + ports: + - protocol: TCP + port: 80 + targetPort: 8080 +` + + remoteHPA = `apiVersion: autoscaling/v2 +kind: HorizontalPodAutoscaler +metadata: + name: hpa-deployment +spec: + scaleTargetRef: + apiVersion: apps/v1 + kind: Deployment + name: localize-test-deployment-simple + minReplicas: 1 + maxReplicas: 10` + + urlQuery = "?submodules=0&ref=kustomize/v4.5.7&timeout=300" ) -func prepareFs(t *testing.T, files map[string]string) (memoryFs filesys.FileSystem, actualFs filesys.FileSystem, testDir filesys.ConfirmedDir) { +func prepareFs(t *testing.T, dirs []string, files map[string]string) ( + memoryFs filesys.FileSystem, actualFs filesys.FileSystem, testDir filesys.ConfirmedDir) { t.Helper() memoryFs = filesys.MakeFsInMemory() @@ -153,6 +212,9 @@ func prepareFs(t *testing.T, files map[string]string) (memoryFs filesys.FileSyst require.NoError(t, err) setupDir(t, memoryFs, testDir.String(), files) + for _, dirPath := range dirs { + require.NoError(t, actualFs.MkdirAll(testDir.Join(dirPath))) + } setupDir(t, actualFs, testDir.String(), files) t.Cleanup(func() { @@ -170,20 +232,80 @@ func setupDir(t *testing.T, targetFs filesys.FileSystem, parentDir string, files } } -func getLocFilePath(t *testing.T, pathFromTestdata []string) string { +func setWorkingDir(t *testing.T, workingDir string) { t.Helper() - localizedPathDirs := []string{LocalizeDir, "raw.githubusercontent.com", "kubernetes-sigs", "kustomize", - "kustomize", "v4.5.7", "api", "krusty", "testdata"} - return filepath.Join(append(localizedPathDirs, pathFromTestdata...)...) + wd, err := os.Getwd() + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, os.Chdir(wd)) + }) + + err = os.Chdir(workingDir) + require.NoError(t, err) +} + +func link(t *testing.T, testDir filesys.ConfirmedDir, links map[string]string) { + t.Helper() + + for newLink, file := range links { + require.NoError(t, os.Symlink(testDir.Join(file), testDir.Join(newLink))) + } +} + +func simplePathAndFiles(t *testing.T) (locPath string, files map[string]string) { + t.Helper() + + locPath = filepath.Join(LocalizeDir, "github.com", + "kubernetes-sigs", "kustomize", "kustomize", "v4.5.7", + "api", "krusty", "testdata", "localize", "simple") + files = map[string]string{ + "kustomization.yaml": simpleKustomization, + "deployment.yaml": simpleDeployment, + "service.yaml": simpleService, + } + return +} + +func remotePathAndFiles(t *testing.T) (locPath string, files map[string]string) { + t.Helper() + + locPath = filepath.Join(LocalizeDir, "github.com", + "kubernetes-sigs", "kustomize", "master", + "api", "krusty", "testdata", "localize", "remote") + simplePath, simpleFiles := simplePathAndFiles(t) + files = map[string]string{ + "kustomization.yaml": fmt.Sprintf(`apiVersion: kustomize.config.k8s.io/v1beta1 +commonLabels: + purpose: remoteReference +kind: Kustomization +resources: +- %s +- hpa.yaml +`, simplePath), + "hpa.yaml": remoteHPA, + } + for path, content := range simpleFiles { + files[filepath.Join(simplePath, path)] = content + } + return } // checkFs checks fsActual, the real file system, against fsExpected, a file system in memory, for contents -// in directory walkDir. +// in directory walkDir. checkFs does not allow symlinks. func checkFs(t *testing.T, walkDir string, fsExpected filesys.FileSystem, fsActual filesys.FileSystem) { t.Helper() - err := fsExpected.Walk(walkDir, func(path string, info fs.FileInfo, err error) error { + err := fsActual.Walk(walkDir, func(path string, info fs.FileInfo, err error) error { + require.NoError(t, err) + + require.NotEqual(t, os.ModeSymlink, info.Mode()&os.ModeSymlink) + require.True(t, fsExpected.Exists(path), "unexpected file %q", path) + return nil + }) + require.NoError(t, err) + + err = fsExpected.Walk(walkDir, func(path string, info fs.FileInfo, err error) error { require.NoError(t, err) if info.IsDir() { @@ -200,16 +322,129 @@ func checkFs(t *testing.T, walkDir string, fsExpected filesys.FileSystem, fsActu return nil }) require.NoError(t, err) +} - err = fsActual.Walk(walkDir, func(path string, info fs.FileInfo, err error) error { - require.NoError(t, err) +func TestWorkingDir(t *testing.T) { + files := map[string]string{ + filepath.Join("target", "kustomization.yaml"): fmt.Sprintf(`resources: +- %s +`, filepath.Join("..", "base")), + filepath.Join("base", "kustomization.yaml"): `resources: +- deployment.yaml +`, + filepath.Join("base", "deployment.yaml"): simpleDeployment, + } + fsExpected, fsActual, wd := prepareFs(t, []string{"target", "base"}, files) + setWorkingDir(t, wd.String()) - // no symlinks yet - require.NotEqual(t, os.ModeSymlink, info.Mode()&os.ModeSymlink) - require.True(t, fsExpected.Exists(path)) - return nil - }) + err := localizer.Run(fsActual, "target", ".", "") require.NoError(t, err) + + dst := wd.Join("localized-target") + setupDir(t, fsExpected, dst, files) + checkFs(t, dst, fsExpected, fsActual) +} + +func TestSymlinks(t *testing.T) { + // test directory + // - link to target + // - link to base + // - link to file + // - target (and scope) + // - link to kustomization + // - base + // - nested root + // - file + // - kustomization + fsExpected, fsActual, testDir := prepareFs(t, []string{"target", + filepath.Join("target", "base"), + filepath.Join("target", "nested")}, map[string]string{ + filepath.Join("target", "base", "kustomization.yaml"): `namePrefix: test- +`, + filepath.Join("target", "nested", "kustomization"): fmt.Sprintf(`resources: +- %s +- %s +`, filepath.Join("..", "file-link"), filepath.Join("..", "base-link")), + filepath.Join("target", "nested", "file"): simpleDeployment, + }) + link(t, testDir, map[string]string{ + "target-link": "target", + "base-link": filepath.Join("target", "base"), + "file-link": filepath.Join("target", "nested", "file"), + filepath.Join("target", "kustomization.yaml"): filepath.Join("target", "nested", "kustomization"), + }) + setWorkingDir(t, testDir.String()) + + err := localizer.Run(fsActual, "target-link", "target", "") + require.NoError(t, err) + + dst := testDir.Join("localized-target") + setupDir(t, fsExpected, dst, map[string]string{ + "kustomization.yaml": fmt.Sprintf(`resources: +- %s +- base +`, filepath.Join("nested", "file")), + filepath.Join("base", "kustomization.yaml"): `namePrefix: test- +`, + filepath.Join("nested", "file"): simpleDeployment, + }) + checkFs(t, dst, fsExpected, fsActual) +} + +func TestRemoteTargetDefaultDst(t *testing.T) { + fsExpected, fsActual, testDir := prepareFs(t, nil, nil) + setWorkingDir(t, testDir.String()) + + const target = simpleURL + urlQuery + err := localizer.Run(fsActual, target, "", "") + require.NoError(t, err) + + dst := testDir.Join("localized-simple-kustomize-v4.5.7") + _, files := simplePathAndFiles(t) + setupDir(t, fsExpected, + filepath.Join(dst, "api", "krusty", "testdata", "localize", "simple"), + files) + checkFs(t, testDir.String(), fsExpected, fsActual) +} + +func TestBadArgs(t *testing.T) { + badDst := filepath.Join("non-existing", "dst") + + for name, test := range map[string]struct { + target string + scope string + dst string + err string + }{ + "target_no_ref": { + target: simpleURL, + err: `localize remote root "https://github.com/kubernetes-sigs/kustomize//api/krusty/testdata/localize/simple" missing ref query string parameter`, + }, + "non-empty_scope": { + target: simpleURL + urlQuery, + scope: ".", + err: fmt.Sprintf(`invalid localize scope ".": scope "." specified for remote localize target "%s"`, simpleURL+urlQuery), + }, + "dst_in_non-existing_dir": { + target: ".", + dst: badDst, + err: fmt.Sprintf(`invalid localize destination "%s": unable to create localize destination directory: mkdir %s: no such file or directory`, badDst, badDst), + }, + } { + t.Run(name, func(t *testing.T) { + kust := map[string]string{ + "kustomization.yaml": "namePrefix: test-", + } + fsExpected, fsActual, testDir := prepareFs(t, nil, kust) + setWorkingDir(t, testDir.String()) + + err := localizer.Run(fsActual, test.target, test.scope, test.dst) + require.EqualError(t, err, test.err) + + setupDir(t, fsExpected, testDir.String(), kust) + checkFs(t, testDir.String(), fsExpected, fsActual) + }) + } } func TestRemoteFile(t *testing.T) { @@ -218,7 +453,7 @@ kind: Kustomization openapi: path: %s ` - fsExpected, fsActual, testDir := prepareFs(t, map[string]string{ + fsExpected, fsActual, testDir := prepareFs(t, nil, map[string]string{ "kustomization.yaml": fmt.Sprintf(kustf, `https://raw.githubusercontent.com/kubernetes-sigs/kustomize/kustomize/v4.5.7/api/krusty/testdata/customschema.json`), }) @@ -226,10 +461,120 @@ openapi: err := localizer.Run(fsActual, testDir.String(), "", dst) require.NoError(t, err) - localizedPath := getLocFilePath(t, []string{"customschema.json"}) + localizedPath := filepath.Join(LocalizeDir, "raw.githubusercontent.com", + "kubernetes-sigs", "kustomize", "kustomize", "v4.5.7", "api", "krusty", + "testdata", "customschema.json") setupDir(t, fsExpected, dst, map[string]string{ "kustomization.yaml": fmt.Sprintf(kustf, localizedPath), localizedPath: customSchema, }) checkFs(t, testDir.String(), fsExpected, fsActual) } + +func TestRemoteRoot(t *testing.T) { + fsExpected, fsActual, testDir := prepareFs(t, nil, map[string]string{ + "kustomization.yaml": fmt.Sprintf(`resources: +- %s +`, simpleURL+urlQuery), + }) + + dst := testDir.Join("dst") + err := localizer.Run(fsActual, testDir.String(), "", dst) + require.NoError(t, err) + + localizedPath, files := simplePathAndFiles(t) + setupDir(t, fsExpected, dst, map[string]string{ + "kustomization.yaml": fmt.Sprintf(`resources: +- %s +`, localizedPath), + }) + setupDir(t, fsExpected, filepath.Join(dst, localizedPath), files) + checkFs(t, dst, fsExpected, fsActual) +} + +func TestNestedRemoteRoots(t *testing.T) { + fsExpected, fsActual, testDir := prepareFs(t, nil, map[string]string{ + // TODO(annasong): Change the ref to the release after kustomize/v4.5.7. + // We need changes to remote post-kustomize/v4.5.7. + "kustomization.yaml": `resources: +- https://github.com/kubernetes-sigs/kustomize//api/krusty/testdata/localize/remote?submodules=0&ref=master&timeout=300 +`, + }) + + dst := testDir.Join("dst") + err := localizer.Run(fsActual, testDir.String(), "", dst) + require.NoError(t, err) + + localizedPath, files := remotePathAndFiles(t) + setupDir(t, fsExpected, dst, map[string]string{ + "kustomization.yaml": fmt.Sprintf(`resources: +- %s +`, localizedPath), + }) + setupDir(t, fsExpected, filepath.Join(dst, localizedPath), files) + checkFs(t, dst, fsExpected, fsActual) +} + +func TestResourcesRepoNotFile(t *testing.T) { + const repo = "https://github.com/kubernetes-sigs/kustomize" + urlQuery + kustomization := map[string]string{ + "kustomization.yaml": fmt.Sprintf(`resources: +- %s +`, repo), + } + fsExpected, fsActual, testDir := prepareFs(t, nil, kustomization) + + err := localizer.Run(fsActual, testDir.String(), "", testDir.Join("dst")) + + const readmeErr = `yaml: line 28: mapping values are not allowed in this context` + fileErr := fmt.Sprintf(`invalid resource at file "%s": MalformedYAMLError: %s`, repo, readmeErr) + rootErr := fmt.Sprintf(`unable to localize root "%s": unable to find one of 'kustomization.yaml', 'kustomization.yml' or 'Kustomization'`, repo) + var actualErr PathLocalizeError + require.ErrorAs(t, err, &actualErr) + require.Equal(t, repo, actualErr.Path) + require.EqualError(t, actualErr.FileError, fileErr) + require.ErrorContains(t, actualErr.RootError, rootErr) + + setupDir(t, fsExpected, testDir.String(), kustomization) + checkFs(t, testDir.String(), fsExpected, fsActual) +} + +func TestRemoteRootNoRef(t *testing.T) { + const root = simpleURL + "?submodules=0&timeout=300" + kustomization := map[string]string{ + "kustomization.yaml": fmt.Sprintf(`resources: +- %s +`, root), + } + fsExpected, fsActual, testDir := prepareFs(t, nil, kustomization) + + err := localizer.Run(fsActual, testDir.String(), "", testDir.Join("dst")) + + const fileErr = "invalid file reference: URL is a git repository" + rootErr := fmt.Sprintf(`localize remote root "%s" missing ref query string parameter`, root) + var actualErr PathLocalizeError + require.ErrorAs(t, err, &actualErr) + require.Equal(t, root, actualErr.Path) + require.EqualError(t, actualErr.FileError, fileErr) + require.EqualError(t, actualErr.RootError, rootErr) + + setupDir(t, fsExpected, testDir.String(), kustomization) + checkFs(t, testDir.String(), fsExpected, fsActual) +} + +func TestExistingCacheDir(t *testing.T) { + const remoteFile = `https://raw.githubusercontent.com/kubernetes-sigs/kustomize/master/api/krusty/testdata/localize/simple/deployment.yaml` + file := map[string]string{ + "kustomization.yaml": fmt.Sprintf(`resources: +- %s +`, remoteFile), + filepath.Join(LocalizeDir, "file"): "existing", + } + fsExpected, fsActual, testDir := prepareFs(t, []string{LocalizeDir}, file) + + err := localizer.Run(fsActual, testDir.String(), "", testDir.Join("dst")) + require.ErrorContains(t, err, fmt.Sprintf(`already contains localized-files needed to store file "%s"`, remoteFile)) + + setupDir(t, fsExpected, testDir.String(), file) + checkFs(t, testDir.String(), fsExpected, fsActual) +}