Merge pull request #2457 from michelleN/hooks-bug

fix(tiller): track hooks in multi-def manifests

pkg/tiller/hooks.go

@@ -51,96 +51,149 @@ type manifest struct {
 	head    *util.SimpleHead
 }
 
-// sortManifests takes a map of filename/YAML contents and sorts them into hook types.
+type result struct {
+	hooks   []*release.Hook
+	generic []manifest
+}
+
+type manifestFile struct {
+	entries map[string]string
+	path    string
+	apis    chartutil.VersionSet
+}
+
+// sortManifests takes a map of filename/YAML contents, splits the file
+// by manifest entries, and sorts the entries into hook types.
 //
 // The resulting hooks struct will be populated with all of the generated hooks.
 // Any file that does not declare one of the hook types will be placed in the
 // 'generic' bucket.
 //
-// To determine hook type, this looks for a YAML structure like this:
-//
-//  kind: SomeKind
-//  apiVersion: v1
-// 	metadata:
-//		annotations:
-//			helm.sh/hook: pre-install
-//
-// Where HOOK_NAME is one of the known hooks.
-//
-// If a file declares more than one hook, it will be copied into all of the applicable
-// hook buckets. (Note: label keys are not unique within the labels section).
-//
 // Files that do not parse into the expected format are simply placed into a map and
 // returned.
 func sortManifests(files map[string]string, apis chartutil.VersionSet, sort SortOrder) ([]*release.Hook, []manifest, error) {
-	hs := []*release.Hook{}
-	generic := []manifest{}
+	result := &result{}
+
+	for filePath, c := range files {
 
-	for n, c := range files {
 		// Skip partials. We could return these as a separate map, but there doesn't
 		// seem to be any need for that at this time.
-		if strings.HasPrefix(path.Base(n), "_") {
+		if strings.HasPrefix(path.Base(filePath), "_") {
 			continue
 		}
-		// Skip empty files, and log this.
+		// Skip empty files and log this.
 		if len(strings.TrimSpace(c)) == 0 {
-			log.Printf("info: manifest %q is empty. Skipping.", n)
+			log.Printf("info: manifest %q is empty. Skipping.", filePath)
 			continue
 		}
 
-		var sh util.SimpleHead
-		err := yaml.Unmarshal([]byte(c), &sh)
+		manifestFile := &manifestFile{
+			entries: util.SplitManifests(c),
+			path:    filePath,
+			apis:    apis,
+		}
+
+		if err := manifestFile.sort(result); err != nil {
+			return result.hooks, result.generic, err
+		}
+	}
+
+	return result.hooks, sortByKind(result.generic, sort), nil
+}
+
+// sort takes a manifestFile object which may contain multiple resource definition
+// entries and sorts each entry by hook types, and saves the resulting hooks and
+// generic manifests (or non-hooks) to the result struct.
+//
+// To determine hook type, it looks for a YAML structure like this:
+//
+//  kind: SomeKind
+//  apiVersion: v1
+// 	metadata:
+//		annotations:
+//			helm.sh/hook: pre-install
+//
+func (file *manifestFile) sort(result *result) error {
+	for _, m := range file.entries {
+		var entry util.SimpleHead
+		err := yaml.Unmarshal([]byte(m), &entry)
 
 		if err != nil {
-			e := fmt.Errorf("YAML parse error on %s: %s", n, err)
-			return hs, generic, e
+			e := fmt.Errorf("YAML parse error on %s: %s", file.path, err)
+			return e
 		}
 
-		if sh.Version != "" && !apis.Has(sh.Version) {
-			return hs, generic, fmt.Errorf("apiVersion %q in %s is not available", sh.Version, n)
+		if entry.Version != "" && !file.apis.Has(entry.Version) {
+			return fmt.Errorf("apiVersion %q in %s is not available", entry.Version, file.path)
 		}
 
-		if sh.Metadata == nil || sh.Metadata.Annotations == nil || len(sh.Metadata.Annotations) == 0 {
-			generic = append(generic, manifest{name: n, content: c, head: &sh})
+		if !hasAnyAnnotation(entry) {
+			result.generic = append(result.generic, manifest{
+				name:    file.path,
+				content: m,
+				head:    &entry,
+			})
 			continue
 		}
 
-		hookTypes, ok := sh.Metadata.Annotations[hooks.HookAnno]
+		hookTypes, ok := entry.Metadata.Annotations[hooks.HookAnno]
 		if !ok {
-			generic = append(generic, manifest{name: n, content: c, head: &sh})
+			result.generic = append(result.generic, manifest{
+				name:    file.path,
+				content: m,
+				head:    &entry,
+			})
 			continue
 		}
 
-		hws, _ := sh.Metadata.Annotations[hooks.HookWeightAnno]
-		hw, err := strconv.Atoi(hws)
-		if err != nil {
-			hw = 0
-		}
+		hw := calculateHookWeight(entry)
 
 		h := &release.Hook{
-			Name:     sh.Metadata.Name,
-			Kind:     sh.Kind,
-			Path:     n,
-			Manifest: c,
+			Name:     entry.Metadata.Name,
+			Kind:     entry.Kind,
+			Path:     file.path,
+			Manifest: m,
 			Events:   []release.Hook_Event{},
-			Weight:   int32(hw),
+			Weight:   hw,
 		}
 
-		isHook := false
+		isKnownHook := false
 		for _, hookType := range strings.Split(hookTypes, ",") {
 			hookType = strings.ToLower(strings.TrimSpace(hookType))
 			e, ok := events[hookType]
 			if ok {
-				isHook = true
+				isKnownHook = true
 				h.Events = append(h.Events, e)
 			}
 		}
 
-		if !isHook {
+		if !isKnownHook {
 			log.Printf("info: skipping unknown hook: %q", hookTypes)
 			continue
 		}
-		hs = append(hs, h)
+
+		result.hooks = append(result.hooks, h)
+	}
+
+	return nil
+}
+
+func hasAnyAnnotation(entry util.SimpleHead) bool {
+	if entry.Metadata == nil ||
+		entry.Metadata.Annotations == nil ||
+		len(entry.Metadata.Annotations) == 0 {
+		return false
 	}
-	return hs, sortByKind(generic, sort), nil
+
+	return true
+}
+
+func calculateHookWeight(entry util.SimpleHead) int32 {
+	hws, _ := entry.Metadata.Annotations[hooks.HookWeightAnno]
+	hw, err := strconv.Atoi(hws)
+	if err != nil {
+		hw = 0
+	}
+
+	return int32(hw)
 }

pkg/tiller/hooks_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package tiller
 
 import (
+	"reflect"
 	"testing"
 
 	"github.com/ghodss/yaml"
@@ -29,17 +30,17 @@ import (
 func TestSortManifests(t *testing.T) {
 
 	data := []struct {
-		name     string
+		name     []string
 		path     string
-		kind     string
-		hooks    []release.Hook_Event
+		kind     []string
+		hooks    map[string][]release.Hook_Event
 		manifest string
 	}{
 		{
-			name:  "first",
+			name:  []string{"first"},
 			path:  "one",
-			kind:  "Job",
-			hooks: []release.Hook_Event{release.Hook_PRE_INSTALL},
+			kind:  []string{"Job"},
+			hooks: map[string][]release.Hook_Event{"first": {release.Hook_PRE_INSTALL}},
 			manifest: `apiVersion: v1
 kind: Job
 metadata:
@@ -51,10 +52,10 @@ metadata:
 `,
 		},
 		{
-			name:  "second",
+			name:  []string{"second"},
 			path:  "two",
-			kind:  "ReplicaSet",
-			hooks: []release.Hook_Event{release.Hook_POST_INSTALL},
+			kind:  []string{"ReplicaSet"},
+			hooks: map[string][]release.Hook_Event{"second": {release.Hook_POST_INSTALL}},
 			manifest: `kind: ReplicaSet
 apiVersion: v1beta1
 metadata:
@@ -63,10 +64,10 @@ metadata:
     "helm.sh/hook": post-install
 `,
 		}, {
-			name:  "third",
+			name:  []string{"third"},
 			path:  "three",
-			kind:  "ReplicaSet",
-			hooks: []release.Hook_Event{},
+			kind:  []string{"ReplicaSet"},
+			hooks: map[string][]release.Hook_Event{"third": nil},
 			manifest: `kind: ReplicaSet
 apiVersion: v1beta1
 metadata:
@@ -75,22 +76,21 @@ metadata:
     "helm.sh/hook": no-such-hook
 `,
 		}, {
-			name:  "fourth",
+			name:  []string{"fourth"},
 			path:  "four",
-			kind:  "Pod",
-			hooks: []release.Hook_Event{},
+			kind:  []string{"Pod"},
+			hooks: map[string][]release.Hook_Event{"fourth": nil},
 			manifest: `kind: Pod
 apiVersion: v1
 metadata:
   name: fourth
   annotations:
-    nothing: here
-`,
+    nothing: here`,
 		}, {
-			name:  "fifth",
+			name:  []string{"fifth"},
 			path:  "five",
-			kind:  "ReplicaSet",
-			hooks: []release.Hook_Event{release.Hook_POST_DELETE, release.Hook_POST_INSTALL},
+			kind:  []string{"ReplicaSet"},
+			hooks: map[string][]release.Hook_Event{"fifth": {release.Hook_POST_DELETE, release.Hook_POST_INSTALL}},
 			manifest: `kind: ReplicaSet
 apiVersion: v1beta1
 metadata:
@@ -100,19 +100,39 @@ metadata:
 `,
 		}, {
 			// Regression test: files with an underscore in the base name should be skipped.
-			name:     "sixth",
+			name:     []string{"sixth"},
 			path:     "six/_six",
-			kind:     "ReplicaSet",
-			hooks:    []release.Hook_Event{},
+			kind:     []string{"ReplicaSet"},
+			hooks:    map[string][]release.Hook_Event{"sixth": nil},
 			manifest: `invalid manifest`, // This will fail if partial is not skipped.
 		}, {
 			// Regression test: files with no content should be skipped.
-			name:     "seventh",
+			name:     []string{"seventh"},
 			path:     "seven",
-			kind:     "ReplicaSet",
-			hooks:    []release.Hook_Event{},
+			kind:     []string{"ReplicaSet"},
+			hooks:    map[string][]release.Hook_Event{"seventh": nil},
 			manifest: "",
 		},
+		{
+			name:  []string{"eighth", "example-test"},
+			path:  "eight",
+			kind:  []string{"ConfigMap", "Pod"},
+			hooks: map[string][]release.Hook_Event{"eighth": nil, "example-test": {release.Hook_RELEASE_TEST_SUCCESS}},
+			manifest: `kind: ConfigMap
+apiVersion: v1
+metadata:
+  name: eighth
+data:
+  name: value
+---
+apiVersion: v1
+kind: Pod
+metadata:
+  name: example-test
+  annotations:
+    "helm.sh/hook": test-success
+`,
+		},
 	}
 
 	manifests := make(map[string]string, len(data))
@@ -126,12 +146,12 @@ metadata:
 	}
 
 	// This test will fail if 'six' or 'seven' was added.
-	if len(generic) != 1 {
-		t.Errorf("Expected 1 generic manifest, got %d", len(generic))
+	if len(generic) != 2 {
+		t.Errorf("Expected 2 generic manifests, got %d", len(generic))
 	}
 
-	if len(hs) != 3 {
-		t.Errorf("Expected 3 hooks, got %d", len(hs))
+	if len(hs) != 4 {
+		t.Errorf("Expected 4 hooks, got %d", len(hs))
 	}
 
 	for _, out := range hs {
@@ -142,17 +162,30 @@ metadata:
 				if out.Path != expect.path {
 					t.Errorf("Expected path %s, got %s", expect.path, out.Path)
 				}
-				if out.Name != expect.name {
-					t.Errorf("Expected name %s, got %s", expect.name, out.Name)
+				nameFound := false
+				for _, expectedName := range expect.name {
+					if out.Name == expectedName {
+						nameFound = true
+					}
 				}
-				if out.Kind != expect.kind {
-					t.Errorf("Expected kind %s, got %s", expect.kind, out.Kind)
+				if !nameFound {
+					t.Errorf("Got unexpected name %s", out.Name)
 				}
-				for i := 0; i < len(out.Events); i++ {
-					if out.Events[i] != expect.hooks[i] {
-						t.Errorf("Expected event %d, got %d", expect.hooks[i], out.Events[i])
+				kindFound := false
+				for _, expectedKind := range expect.kind {
+					if out.Kind == expectedKind {
+						kindFound = true
 					}
 				}
+				if !kindFound {
+					t.Errorf("Got unexpected kind %s", out.Kind)
+				}
+
+				expectedHooks := expect.hooks[out.Name]
+				if !reflect.DeepEqual(expectedHooks, out.Events) {
+					t.Errorf("expected events: %v but got: %v", expectedHooks, out.Events)
+				}
+
 			}
 		}
 		if !found {
@@ -161,27 +194,40 @@ metadata:
 	}
 
 	// Verify the sort order
-	sorted := make([]manifest, len(data))
-	for i, s := range data {
-		var sh util.SimpleHead
-		err := yaml.Unmarshal([]byte(s.manifest), &sh)
-		if err != nil {
-			// This is expected for manifests that are corrupt or empty.
-			t.Log(err)
-		}
-		sorted[i] = manifest{
-			content: s.manifest,
-			name:    s.name,
-			head:    &sh,
+	sorted := []manifest{}
+	for _, s := range data {
+		manifests := util.SplitManifests(s.manifest)
+		mCount := 0
+		for _, m := range manifests {
+			name := s.name[mCount]
+
+			var sh util.SimpleHead
+			err := yaml.Unmarshal([]byte(m), &sh)
+			if err != nil {
+				// This is expected for manifests that are corrupt or empty.
+				t.Log(err)
+			}
+
+			//only keep track of non-hook manifests
+			if err == nil && s.hooks[name] == nil {
+				another := manifest{
+					content: m,
+					name:    name,
+					head:    &sh,
+				}
+				sorted = append(sorted, another)
+			}
+
+			mCount++
 		}
 	}
+
 	sorted = sortByKind(sorted, InstallOrder)
 	for i, m := range generic {
 		if m.content != sorted[i].content {
 			t.Errorf("Expected %q, got %q", m.content, sorted[i].content)
 		}
 	}
-
 }
 
 func TestVersionSet(t *testing.T) {

pkg/tiller/release_server_test.go

@@ -47,8 +47,7 @@ metadata:
   annotations:
     "helm.sh/hook": post-install,pre-delete
 data:
-  name: value
-`
+  name: value`
 
 var manifestWithTestHook = `
 apiVersion: v1
@@ -81,8 +80,7 @@ metadata:
   annotations:
     "helm.sh/hook": post-upgrade,pre-upgrade
 data:
-  name: value
-`
+  name: value`
 
 var manifestWithRollbackHooks = `apiVersion: v1
 kind: ConfigMap