From 73a2890277136d3bbc6eb97edd7c2ff7d01c11e2 Mon Sep 17 00:00:00 2001 From: Matt Butcher Date: Wed, 6 Jul 2016 15:31:12 -0600 Subject: [PATCH] fix(engine): change template naming Template paths were relative to the chart that contained them, which meant that all templates were named 'template/SOMETHING'. This made it trivially easy to hit namespace collisions as in #933. Template path names are essentially opaque strings so this patch simply changes them to be qualified by parent chart. --- cmd/tiller/release_server_test.go | 12 ++++++------ pkg/engine/engine.go | 19 +++++++++++++++---- pkg/engine/engine_test.go | 29 ++++++++++++++++------------- 3 files changed, 37 insertions(+), 23 deletions(-) diff --git a/cmd/tiller/release_server_test.go b/cmd/tiller/release_server_test.go index 4b03caf6f..47981e640 100644 --- a/cmd/tiller/release_server_test.go +++ b/cmd/tiller/release_server_test.go @@ -134,7 +134,7 @@ func TestInstallRelease(t *testing.T) { t.Errorf("Expected manifest in %v", res) } - if !strings.Contains(rel.Manifest, "---\n# Source: hello\nhello: world") { + if !strings.Contains(rel.Manifest, "---\n# Source: hello/hello\nhello: world") { t.Errorf("unexpected output: %s", rel.Manifest) } } @@ -150,8 +150,8 @@ func TestInstallReleaseDryRun(t *testing.T) { {Name: "hello", Data: []byte("hello: world")}, {Name: "goodbye", Data: []byte("goodbye: world")}, {Name: "empty", Data: []byte("")}, - {Name: "with-partials", Data: []byte("hello: {{ template \"partials/_planet\" . }}")}, - {Name: "partials/_planet", Data: []byte("Earth")}, + {Name: "with-partials", Data: []byte(`hello: {{ template "_planet" . }}`)}, + {Name: "partials/_planet", Data: []byte(`{{define "_planet"}}Earth{{end}}`)}, {Name: "hooks", Data: []byte(manifestWithHook)}, }, }, @@ -165,11 +165,11 @@ func TestInstallReleaseDryRun(t *testing.T) { t.Errorf("Expected release name.") } - if !strings.Contains(res.Release.Manifest, "---\n# Source: hello\nhello: world") { + if !strings.Contains(res.Release.Manifest, "---\n# Source: hello/hello\nhello: world") { t.Errorf("unexpected output: %s", res.Release.Manifest) } - if !strings.Contains(res.Release.Manifest, "---\n# Source: goodbye\ngoodbye: world") { + if !strings.Contains(res.Release.Manifest, "---\n# Source: hello/goodbye\ngoodbye: world") { t.Errorf("unexpected output: %s", res.Release.Manifest) } @@ -177,7 +177,7 @@ func TestInstallReleaseDryRun(t *testing.T) { t.Errorf("Should contain partial content. %s", res.Release.Manifest) } - if strings.Contains(res.Release.Manifest, "hello: {{ template \"partials/_planet\" . }}") { + if strings.Contains(res.Release.Manifest, "hello: {{ template \"_planet\" . }}") { t.Errorf("Should not contain partial templates itself. %s", res.Release.Manifest) } diff --git a/pkg/engine/engine.go b/pkg/engine/engine.go index 737e92abd..934917701 100644 --- a/pkg/engine/engine.go +++ b/pkg/engine/engine.go @@ -19,6 +19,8 @@ package engine import ( "bytes" "fmt" + "log" + "path" "strings" "text/template" @@ -105,6 +107,7 @@ func (e *Engine) render(tpls map[string]renderable) (map[string]string, error) { } files := []string{} for fname, r := range tpls { + log.Printf("Preparing template %s", fname) t = t.New(fname).Funcs(e.FuncMap) if _, err := t.Parse(r.tpl); err != nil { return map[string]string{}, fmt.Errorf("parse error in %q: %s", fname, err) @@ -137,7 +140,7 @@ func (e *Engine) render(tpls map[string]renderable) (map[string]string, error) { // As it goes, it also prepares the values in a scope-sensitive manner. func allTemplates(c *chart.Chart, vals chartutil.Values) map[string]renderable { templates := map[string]renderable{} - recAllTpls(c, templates, vals, true) + recAllTpls(c, templates, vals, true, "") return templates } @@ -145,7 +148,7 @@ func allTemplates(c *chart.Chart, vals chartutil.Values) map[string]renderable { // // As it recurses, it also sets the values to be appropriate for the template // scope. -func recAllTpls(c *chart.Chart, templates map[string]renderable, parentVals chartutil.Values, top bool) { +func recAllTpls(c *chart.Chart, templates map[string]renderable, parentVals chartutil.Values, top bool, parentID string) { // This should never evaluate to a nil map. That will cause problems when // values are appended later. cvals := chartutil.Values{} @@ -170,11 +173,19 @@ func recAllTpls(c *chart.Chart, templates map[string]renderable, parentVals char } } + newParentID := c.Metadata.Name + if parentID != "" { + // We artificially reconstruct the chart path to child templates. This + // creates a namespaced filename that can be used to track down the source + // of a particular template declaration. + newParentID = path.Join(parentID, "charts", newParentID) + } + for _, child := range c.Dependencies { - recAllTpls(child, templates, cvals, false) + recAllTpls(child, templates, cvals, false, newParentID) } for _, t := range c.Templates { - templates[t.Name] = renderable{ + templates[path.Join(newParentID, t.Name)] = renderable{ tpl: string(t.Data), vals: cvals, } diff --git a/pkg/engine/engine_test.go b/pkg/engine/engine_test.go index b11603e4f..46ec43e54 100644 --- a/pkg/engine/engine_test.go +++ b/pkg/engine/engine_test.go @@ -75,16 +75,16 @@ func TestRender(t *testing.T) { } expect := "Spouter Inn" - if out["test1"] != expect { + if out["moby/test1"] != expect { t.Errorf("Expected %q, got %q", expect, out["test1"]) } expect = "ishmael" - if out["test2"] != expect { + if out["moby/test2"] != expect { t.Errorf("Expected %q, got %q", expect, out["test2"]) } expect = "" - if out["test3"] != expect { + if out["moby/test3"] != expect { t.Errorf("Expected %q, got %q", expect, out["test3"]) } @@ -188,11 +188,13 @@ func TestRenderDependency(t *testing.T) { deptpl := `{{define "myblock"}}World{{end}}` toptpl := `Hello {{template "myblock"}}` ch := &chart.Chart{ + Metadata: &chart.Metadata{Name: "outerchart"}, Templates: []*chart.Template{ {Name: "outer", Data: []byte(toptpl)}, }, Dependencies: []*chart.Chart{ { + Metadata: &chart.Metadata{Name: "innerchart"}, Templates: []*chart.Template{ {Name: "inner", Data: []byte(deptpl)}, }, @@ -211,7 +213,7 @@ func TestRenderDependency(t *testing.T) { } expect := "Hello World" - if out["outer"] != expect { + if out["outerchart/outer"] != expect { t.Errorf("Expected %q, got %q", expect, out["outer"]) } @@ -220,10 +222,11 @@ func TestRenderDependency(t *testing.T) { func TestRenderNestedValues(t *testing.T) { e := New() - innerpath := "charts/inner/templates/inner.tpl" + innerpath := "templates/inner.tpl" outerpath := "templates/outer.tpl" - deepestpath := "charts/inner/charts/deepest/templates/deepest.tpl" - checkrelease := "charts/inner/charts/deepest/templates/release.tpl" + // Ensure namespacing rules are working. + deepestpath := "templates/inner.tpl" + checkrelease := "templates/release.tpl" deepest := &chart.Chart{ Metadata: &chart.Metadata{Name: "deepest"}, @@ -288,19 +291,19 @@ global: t.Fatalf("failed to render templates: %s", err) } - if out[outerpath] != "Gather ye rosebuds while ye may" { + if out["top/"+outerpath] != "Gather ye rosebuds while ye may" { t.Errorf("Unexpected outer: %q", out[outerpath]) } - if out[innerpath] != "Old time is still a-flyin'" { + if out["top/charts/herrick/"+innerpath] != "Old time is still a-flyin'" { t.Errorf("Unexpected inner: %q", out[innerpath]) } - if out[deepestpath] != "And this same flower that smiles to-day" { + if out["top/charts/herrick/charts/deepest/"+deepestpath] != "And this same flower that smiles to-day" { t.Errorf("Unexpected deepest: %q", out[deepestpath]) } - if out[checkrelease] != "Tomorrow will be dyin" { + if out["top/charts/herrick/charts/deepest/"+checkrelease] != "Tomorrow will be dyin" { t.Errorf("Unexpected release: %q", out[checkrelease]) } } @@ -340,8 +343,8 @@ func TestRenderBuiltinValues(t *testing.T) { } expects := map[string]string{ - "Lavinia": "LaviniaLatiumAeneid", - "Aeneas": "AeneasTroyAeneid", + "Troy/charts/Latium/Lavinia": "Troy/charts/Latium/LaviniaLatiumAeneid", + "Troy/Aeneas": "Troy/AeneasTroyAeneid", } for file, expect := range expects { if out[file] != expect {