Repository: grafana/tanka
Pull request #713 (2022-06-02 13:47:26)

Charts: Tighten validations
Closes https://github.com/grafana/tanka/issues/691 which is mostly a failure of error messages imo

Instead of getting a `no such file`, the error is now: 
```console
$ tk tool charts add https://helm.releases.hashicorp.com/vault@0.19.0
Adding 1 Charts ...
 Skipping https://helm.releases.hashicorp.com/vault@0.19.0: not of form 'repo/chart@version(:path)' where repo contains no special characters.
Error: 1 Chart(s) were skipped. Please check above logs for details
```

Also, adding an invalid repo now prints out a relevant error message instead of failing in helm commands
```console
$ tk tool charts add-repo https://helm.releases.hashicorp.com  https://helm.releases.hashicorp.com
 Skipping https://helm.releases.hashicorp.com: invalid name. cannot contain any special characters.
Error: 1 Repo(s) were skipped. Please check above logs for details
```



Patch:
diff --git a/docs/docs/exporting.md b/docs/docs/exporting.md
index 4c9dd8e6..ad9671d4 100644
--- a/docs/docs/exporting.md
+++ b/docs/docs/exporting.md
@@ -98,3 +98,17 @@ $ tk export exportDir environments/ --recursive
 # Recursive export with labelSelector
 $ tk export exportDir environments/ -r -l team=infra
 ```
+
+## Caching
+
+Tanka can also cache the results of the export. This is useful if you often export the same files and want to avoid recomputing them. The cache key is calculated from the main file and all of its transitive imports, so any change to any file possibly used in an environment will invalidate the cache.
+
+This is configured by two flags:
+
+- `--cache-path`: The local filesystem path where the cache will be stored. The cache is a flat directory of json files (one per environment).
+- `--cache-envs`: If exporting multiple environments, this flag can be used to specify, with regexes, which environments to cache. If not specified, all environments are cached.
+
+Notes:
+
+- Using the cache might be slower than evaluating jsonnet directy. It is only recommended for environments that are very CPU intensive to evaluate.
+- To use object storage, you can point the `--cache-path` to a FUSE mount, such as [`s3fs`](https://github.com/s3fs-fuse/s3fs-fuse)
diff --git a/pkg/helm/charts.go b/pkg/helm/charts.go
index 12ba0724..b41ec9e3 100644
--- a/pkg/helm/charts.go
+++ b/pkg/helm/charts.go
@@ -13,6 +13,12 @@ import (
 	"sigs.k8s.io/yaml"
 )
 
+var (
+	// https://regex101.com/r/7xFFtU/3
+	chartExp = regexp.MustCompile(`^(?P<chart>\w+\/.+)@(?P<version>[^:\n\s]+)(?:\:(?P<path>[\w-. ]+))?$`)
+	repoExp  = regexp.MustCompile(`^\w+$`)
+)
+
 // LoadChartfile opens a Chartfile tree
 func LoadChartfile(projectRoot string) (*Charts, error) {
 	// make sure project root is valid
@@ -99,7 +105,7 @@ func (c Charts) Vendor(prune bool) error {
 	expectedDirs := make(map[string]bool)
 
 	repositoriesUpdated := false
-	log.Println("Pulling Charts ...")
+	log.Println("Vendoring...")
 	for _, r := range c.Manifest.Requires {
 		chartName := parseReqName(r.Chart)
 		chartPath := filepath.Join(dir, chartName)
@@ -141,6 +147,10 @@ func (c Charts) Vendor(prune bool) error {
 			}
 			repositoriesUpdated = true
 		}
+		log.Println("Pulling Charts ...")
+		if repoName := parseReqRepo(r.Chart); !c.Manifest.Repositories.HasName(repoName) {
+			return fmt.Errorf("repository %q not found for chart %q", repoName, r.Chart)
+		}
 		err = c.Helm.Pull(r.Chart, r.Version.String(), PullOpts{
 			Destination:      dir,
 			ExtractDirectory: r.Directory,
@@ -227,6 +237,11 @@ func (c *Charts) AddRepos(repos ...Repo) error {
 			continue
 		}
 
+		if !repoExp.MatchString(r.Name) {
+			skip(r.Name, fmt.Errorf("invalid name. cannot contain any special characters"))
+			continue
+		}
+
 		c.Manifest.Repositories = append(c.Manifest.Repositories, r)
 		added++
 		log.Println(" OK:", r.Name)
@@ -271,29 +286,25 @@ func write(c Chartfile, dest string) error {
 	return os.WriteFile(dest, data, 0644)
 }
 
-// https://regex101.com/r/VAklNg/1
-var chartExp = regexp.MustCompile(`\w+\/.+@.+(\:.+)?`)
-
 // parseReq parses a requirement from a string of the format `repo/name@version`
 func parseReq(s string) (*Requirement, error) {
-	if !chartExp.MatchString(s) {
-		return nil, fmt.Errorf("not of form 'repo/chart@version(:path)'")
+	matches := chartExp.FindStringSubmatch(s)
+	if matches == nil {
+		return nil, fmt.Errorf("not of form 'repo/chart@version(:path)' where repo contains no special characters")
 	}
 
-	elems := strings.Split(s, ":")
-	directory := ""
-	if len(elems) > 1 {
-		s = elems[0]
-		directory = elems[1]
-	}
+	chart := matches[1]
 
-	elems = strings.Split(s, "@")
-	chart := elems[0]
-	ver, err := semver.NewVersion(elems[1])
+	ver, err := semver.NewVersion(matches[2])
 	if errors.Is(err, semver.ErrInvalidSemVer) {
-		return nil, fmt.Errorf("version is invalid")
+		return nil, fmt.Errorf("version is invalid: %s", matches[2])
 	} else if err != nil {
-		return nil, fmt.Errorf("version is invalid: %s", err)
+		return nil, fmt.Errorf("error parsing semver: %s", err)
+	}
+
+	directory := ""
+	if len(matches) == 4 {
+		directory = matches[3]
 	}
 
 	return &Requirement{
@@ -303,9 +314,16 @@ func parseReq(s string) (*Requirement, error) {
 	}, nil
 }
 
+// parseReqRepo parses a repo from a string of the format `repo/name`
+func parseReqRepo(s string) string {
+	elems := strings.SplitN(s, "/", 2)
+	repo := elems[0]
+	return repo
+}
+
 // parseReqName parses a name from a string of the format `repo/name`
 func parseReqName(s string) string {
-	elems := strings.Split(s, "/")
+	elems := strings.SplitN(s, "/", 2)
 	name := elems[1]
 	return name
 }
diff --git a/pkg/helm/spec.go b/pkg/helm/spec.go
index 563b9680..23db2b63 100644
--- a/pkg/helm/spec.go
+++ b/pkg/helm/spec.go
@@ -59,6 +59,17 @@ func (r Repos) Has(repo Repo) bool {
 	return false
 }
 
+// Has reports whether one of the repos has the given name
+func (r Repos) HasName(repoName string) bool {
+	for _, x := range r {
+		if x.Name == repoName {
+			return true
+		}
+	}
+
+	return false
+}
+
 // Requirement describes a single required Helm Chart.
 // Both, Chart and Version are required
 type Requirement struct {
diff --git a/pkg/jsonnet/imports.go b/pkg/jsonnet/imports.go
index ecc975b8..c91dc228 100644
--- a/pkg/jsonnet/imports.go
+++ b/pkg/jsonnet/imports.go
@@ -57,7 +57,7 @@ func TransitiveImports(dir string) ([]string, error) {
 	}
 
 	imports := make(map[string]bool)
-	if err = importRecursive(imports, vm, node, filepath.Base(entrypoint)); err != nil {
+	if err = importRecursiveStrict(imports, vm, node, filepath.Base(entrypoint)); err != nil {
 		return nil, err
 	}
 
@@ -85,10 +85,16 @@ func TransitiveImports(dir string) ([]string, error) {
 	return paths, nil
 }
 
+// importRecursiveStrict does the same as importRecursive, but returns an error
+// if a file is not found during when importing
+func importRecursiveStrict(list map[string]bool, vm *jsonnet.VM, node ast.Node, currentPath string) error {
+	return importRecursive(list, vm, node, currentPath, false)
+}
+
 // importRecursive takes a Jsonnet VM and recursively imports the AST. Every
 // found import is added to the `list` string slice, which will ultimately
 // contain all recursive imports
-func importRecursive(list map[string]bool, vm *jsonnet.VM, node ast.Node, currentPath string) error {
+func importRecursive(list map[string]bool, vm *jsonnet.VM, node ast.Node, currentPath string, ignoreMissing bool) error {
 	switch node := node.(type) {
 	// we have an `import`
 	case *ast.Import:
@@ -96,6 +102,9 @@ func importRecursive(list map[string]bool, vm *jsonnet.VM, node ast.Node, curren
 
 		contents, foundAt, err := vm.ImportAST(currentPath, p)
 		if err != nil {
+			if ignoreMissing {
+				return nil
+			}
 			return fmt.Errorf("importing '%s' from '%s': %w", p, currentPath, err)
 		}
 
@@ -106,7 +115,7 @@ func importRecursive(list map[string]bool, vm *jsonnet.VM, node ast.Node, curren
 
 		list[abs] = true
 
-		if err := importRecursive(list, vm, contents, foundAt); err != nil {
+		if err := importRecursive(list, vm, contents, foundAt, ignoreMissing); err != nil {
 			return err
 		}
 
@@ -116,6 +125,9 @@ func importRecursive(list map[string]bool, vm *jsonnet.VM, node ast.Node, curren
 
 		foundAt, err := vm.ResolveImport(currentPath, p)
 		if err != nil {
+			if ignoreMissing {
+				return nil
+			}
 			return errors.Wrap(err, "importing string")
 		}
 
@@ -129,7 +141,7 @@ func importRecursive(list map[string]bool, vm *jsonnet.VM, node ast.Node, curren
 	// neither `import` nor `importstr`, probably object or similar: try children
 	default:
 		for _, child := range toolutils.Children(node) {
-			if err := importRecursive(list, vm, child, currentPath); err != nil {
+			if err := importRecursive(list, vm, child, currentPath, ignoreMissing); err != nil {
 				return err
 			}
 		}
@@ -145,7 +157,7 @@ var fileHashes sync.Map
 func getSnippetHash(vm *jsonnet.VM, path, data string) (string, error) {
 	node, _ := jsonnet.SnippetToAST(path, data)
 	result := map[string]bool{}
-	if err := importRecursive(result, vm, node, path); err != nil {
+	if err := importRecursive(result, vm, node, path, true); err != nil {
 		return "", err
 	}
 	fileNames := []string{}


Test patch:
diff --git a/pkg/helm/charts_test.go b/pkg/helm/charts_test.go
index c8b1d655..33d651af 100644
--- a/pkg/helm/charts_test.go
+++ b/pkg/helm/charts_test.go
@@ -1,15 +1,72 @@
 package helm
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
 	"testing"
 
+	"github.com/Masterminds/semver"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+func TestParseReq(t *testing.T) {
+	testCases := []struct {
+		name     string
+		input    string
+		expected *Requirement
+		err      error
+	}{
+		{
+			name:  "valid",
+			input: "stable/package@1.0.0",
+			expected: &Requirement{
+				Chart:     "stable/package",
+				Version:   *semver.MustParse("1.0.0"),
+				Directory: "",
+			},
+		},
+		{
+			name:  "invalid-semver",
+			input: "stable/package@not-semver",
+			err:   fmt.Errorf("version is invalid: not-semver"),
+		},
+		{
+			name:  "with-path",
+			input: "stable/package-name@1.0.0:my-path",
+			expected: &Requirement{
+				Chart:     "stable/package-name",
+				Version:   *semver.MustParse("1.0.0"),
+				Directory: "my-path",
+			},
+		},
+		{
+			name:  "with-path-with-special-chars",
+			input: "stable/package@v1.24.0:my weird-path_test",
+			expected: &Requirement{
+				Chart:     "stable/package",
+				Version:   *semver.MustParse("v1.24.0"),
+				Directory: "my weird-path_test",
+			},
+		},
+		{
+			name:  "url-instead-of-repo",
+			input: "https://helm.releases.hashicorp.com/vault@0.19.0",
+			err:   errors.New("not of form 'repo/chart@version(:path)' where repo contains no special characters"),
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			req, err := parseReq(tc.input)
+			assert.Equal(t, tc.err, err)
+			assert.Equal(t, tc.expected, req)
+		})
+	}
+}
+
 func TestAddRepos(t *testing.T) {
 	c, err := InitChartfile(filepath.Join(t.TempDir(), Filename))
 	require.NoError(t, err)
@@ -20,6 +77,12 @@ func TestAddRepos(t *testing.T) {
 	)
 	assert.NoError(t, err)
 
+	// Only \w characters are allowed in repo names
+	err = c.AddRepos(
+		Repo{Name: "with-dashes", URL: "https://foo.com"},
+	)
+	assert.EqualError(t, err, "1 Repo(s) were skipped. Please check above logs for details")
+
 	err = c.AddRepos(
 		Repo{Name: "foo", URL: "https://foo.com"},
 	)