Repository: grafana/tanka
Pull request #574 (2021-06-17 18:33:00)

Fix support for multiple --tla-code arguments
While running the `tk apply` command with multiple `--tla-code` arguments, each containing a `key='value'` parameter, I noticed that the JSONNET code would not receive them correctly. More specifically, instead of being passed as named parameters, they are passed as positional parameters and since they're stored in a [`jsonnet.InjectedCode`](https://github.com/grafana/tanka/blob/17da685bbff3baee75e529b13c19d8f047015395/pkg/jsonnet/eval.go#L16) map, they end up being serialised in an arbitrary order which differs each time Tanka is run. I added a test in this PR which highlights this issue (you might need to run it multiple times to get a failure).

Note that this issue only surfaces when using multiple `--tla-code` args when calling `tk apply`.

I am rather new to this tool, so I'm not fully aware of all the ways in which people use the `--tla-code` argument. Please let me know if I missed any other edge cases.


Patch:
diff --git a/.drone/drone.yml b/.drone/drone.yml
index 184c56f7..6e991bc3 100644
--- a/.drone/drone.yml
+++ b/.drone/drone.yml
@@ -291,4 +291,8 @@ get:
   path: infra/data/ci/docker_hub
   name: password
 
+---
+kind: signature
+hmac: 72469494de5902df2a0a375f1a38f1b064dcc2c3639d4978b86392f2b7c46f46
+
 ...
diff --git a/.vscode/settings.json b/.vscode/settings.json
new file mode 100644
index 00000000..008074e2
--- /dev/null
+++ b/.vscode/settings.json
@@ -0,0 +1,3 @@
+{
+    "go.formatTool": "gofmt"
+}
\ No newline at end of file
diff --git a/Makefile b/Makefile
index f1bfbe18..4a903d76 100644
--- a/Makefile
+++ b/Makefile
@@ -36,3 +36,5 @@ container: static
 # CI
 drone:
 	drone jsonnet --source .drone/drone.jsonnet --target .drone/drone.yml --stream --format
+	drone lint .drone/drone.yml
+	drone sign --save grafana/tanka .drone/drone.yml
diff --git a/pkg/tanka/evaluators.go b/pkg/tanka/evaluators.go
index 927419e3..9e9338c3 100644
--- a/pkg/tanka/evaluators.go
+++ b/pkg/tanka/evaluators.go
@@ -21,7 +21,7 @@ func evalJsonnet(path string, opts jsonnet.Opts) (raw string, err error) {
 	if opts.EvalScript != "" {
 		var tla []string
 		for k := range opts.TLACode {
-			tla = append(tla, k)
+			tla = append(tla, k+"="+k)
 		}
 		evalScript := fmt.Sprintf(`
   local main = (import '%s');
diff --git a/pkg/tanka/export.go b/pkg/tanka/export.go
index 8916bd0a..69109595 100644
--- a/pkg/tanka/export.go
+++ b/pkg/tanka/export.go
@@ -175,7 +175,7 @@ func writeExportFile(path string, data []byte) error {
 
 func createTemplate(format string, env manifest.Manifest) (*template.Template, error) {
 	// Replace all os.path separators in string with BelRune for creating subfolders
-	replaceFormat := strings.Replace(format, string(os.PathSeparator), BelRune, -1)
+	replaceFormat := replaceTmplText(format, string(os.PathSeparator), BelRune)
 
 	envMap := template.FuncMap{"env": func() manifest.Manifest { return env }}
 
@@ -189,6 +189,24 @@ func createTemplate(format string, env manifest.Manifest) (*template.Template, e
 	return template, nil
 }
 
+func replaceTmplText(s, old, new string) string {
+	parts := []string{}
+	l := strings.Index(s, "{{")
+	r := strings.Index(s, "}}") + 2
+
+	for l != -1 && l < r {
+		// replace only in text between template action blocks
+		text := strings.ReplaceAll(s[:l], old, new)
+		action := s[l:r]
+		parts = append(parts, text, action)
+		s = s[r:]
+		l = strings.Index(s, "{{")
+		r = strings.Index(s, "}}") + 2
+	}
+	parts = append(parts, strings.ReplaceAll(s, old, new))
+	return strings.Join(parts, "")
+}
+
 func applyTemplate(template *template.Template, m manifest.Manifest) (path string, err error) {
 	buf := bytes.Buffer{}
 	if err := template.Execute(&buf, m); err != nil {


Test patch:
diff --git a/pkg/tanka/evaluators_test.go b/pkg/tanka/evaluators_test.go
new file mode 100644
index 00000000..0d3013c9
--- /dev/null
+++ b/pkg/tanka/evaluators_test.go
@@ -0,0 +1,33 @@
+package tanka
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/grafana/tanka/pkg/jsonnet"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestEvalJsonnet(t *testing.T) {
+	var tlaCode jsonnet.InjectedCode
+	// Pass in the mandatory parameters as TLA codes, but note that only `foo`
+	// contains `data`, which is a valid key inside the `o` object defined in
+	// testdata/cases/withtlas/main.jsonnet. If they are passed as positional
+	// parameters, then their names are ignored, which will lead to arbitrary
+	// failures because the order in which they're passed is random.
+	// `evalJsonnet` has been updated to pass them as named parameters.
+	tlaCode.Set("foo", "'data'")
+	tlaCode.Set("bar", "'kaboom'")
+	tlaCode.Set("baz", "'kaboom'")
+
+	opts := jsonnet.Opts{
+		EvalScript: "main",
+		TLACode:    tlaCode,
+	}
+
+	// This will fail intermittently if TLAs are passed as positional
+	// parameters.
+	json, err := evalJsonnet("testdata/cases/withtlas", opts)
+	assert.NoError(t, err)
+	assert.Equal(t, `"foovalue"`, strings.TrimSpace(json))
+}
diff --git a/pkg/tanka/export_test.go b/pkg/tanka/export_test.go
new file mode 100644
index 00000000..9663c695
--- /dev/null
+++ b/pkg/tanka/export_test.go
@@ -0,0 +1,33 @@
+package tanka
+
+import "testing"
+
+func Test_replaceTmplText(t *testing.T) {
+	type args struct {
+		s   string
+		old string
+		new string
+	}
+	tests := []struct {
+		name string
+		args args
+		want string
+	}{
+		{"text only", args{"a", "a", "b"}, "b"},
+		{"action blocks", args{"{{a}}{{.}}", "a", "b"}, "{{a}}{{.}}"},
+		{"mixed", args{"a{{a}}a{{a}}a", "a", "b"}, "b{{a}}b{{a}}b"},
+		{"invalid template format handled as text", args{"a}}a{{a", "a", "b"}, "b}}b{{b"},
+		{
+			name: "keep path separator in action block",
+			args: args{`{{index .metadata.labels "app.kubernetes.io/name"}}/{{.metadata.name}}`, "/", BelRune},
+			want: "{{index .metadata.labels \"app.kubernetes.io/name\"}}\u0007{{.metadata.name}}",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := replaceTmplText(tt.args.s, tt.args.old, tt.args.new); got != tt.want {
+				t.Errorf("replaceInTmplText() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
diff --git a/pkg/tanka/testdata/cases/withtlas/main.jsonnet b/pkg/tanka/testdata/cases/withtlas/main.jsonnet
new file mode 100644
index 00000000..c6442130
--- /dev/null
+++ b/pkg/tanka/testdata/cases/withtlas/main.jsonnet
@@ -0,0 +1,5 @@
+local o = {
+  data: 'foovalue',
+};
+
+function(foo, bar, baz) o[foo]