Merge pull request #3141 from lavalamp/optimize

Fix e2e; add template function to make this possible

hack/e2e-suite/update.sh

@@ -51,21 +51,16 @@ function validate() {
     for id in "${pod_id_list[@]+${pod_id_list[@]}}"; do
       local template_string current_status current_image host_ip
 
-      # NB: This template string is a little subtle.
+      # NB: kubectl & kubecfg add the "exists" function to the standard template functions.
-      #
+      # This lets us check to see if the "running" entry exists for each of the containers
-      # Notes:
+      # we care about. Exists will never return an error and it's safe to check a chain of
-      #
+      # things, any one of which may not exist. In the below template, all of info, 
-      # The 'and' operator will return blank if any of the inputs are non-
+      # containername, and running might be nil, so the normal index function isn't very
-      # nil/false.  If they are all set, then it'll return the last one.
+      # helpful.
-      #
+      # This template is unit-tested in kubec{tl|fg}, so if you change it, update the unit test.
-      # The container is name has a dash in it and so we can't use the simple
-      # syntax.  Instead we need to quote that and use the 'index' operator.
-      #
-      # The value here is a structure with just a Time member.  This is
-      # currently always set to a zero time.
       #
       # You can read about the syntax here: http://golang.org/pkg/text/template/
-      template_string="{{and ((index .currentState.info \"${CONTROLLER_NAME}\").state.running.startedAt) .currentState.info.net.state.running.startedAt}}"
+      template_string="{{and (exists . \"currentState\" \"info\" \"${CONTROLLER_NAME}\" \"state\" \"running\") (exists . \"currentState\" \"info\" \"net\" \"state\" \"running\")}}"
       current_status=$($KUBECFG -template="${template_string}" get "pods/$id") || {
         if [[ $current_status =~ "pod \"${id}\" not found" ]]; then
           echo "  $id no longer exists"
@@ -76,11 +71,13 @@ function validate() {
           exit -1
         fi
       }
-      if [[ "$current_status" == "<no value>" ]]; then
+      if [[ "$current_status" == "false" ]]; then
-        echo "  $id is created but not running ${current_status}"
+        echo "  $id is created but not running."
         continue
       fi
 
+      echo "  $id is created and both net and update-demo containers are running: $current_status"
+
       template_string="{{(index .currentState.info \"${CONTROLLER_NAME}\").image}}"
       current_image=$($KUBECFG -template="${template_string}" get "pods/$id") || true
       if [[ "$current_image" != "${DOCKER_HUB_USER}/update-demo:${container_image_version}" ]]; then

pkg/kubecfg/resource_printer.go

@@ -327,25 +327,38 @@ func (h *HumanReadablePrinter) PrintObj(obj runtime.Object, output io.Writer) er
 
 // TemplatePrinter is an implementation of ResourcePrinter which formats data with a Go Template.
 type TemplatePrinter struct {
-	template *template.Template
+	rawTemplate string
+	template    *template.Template
 }
 
 func NewTemplatePrinter(tmpl []byte) (*TemplatePrinter, error) {
-	t, err := template.New("output").Parse(string(tmpl))
+	t, err := template.New("output").
+		Funcs(template.FuncMap{"exists": exists}).
+		Parse(string(tmpl))
 	if err != nil {
 		return nil, err
 	}
-	return &TemplatePrinter{t}, nil
+	return &TemplatePrinter{string(tmpl), t}, nil
 }
 
 // Print parses the data as JSON, and re-formats it with the Go Template.
 func (t *TemplatePrinter) Print(data []byte, w io.Writer) error {
-	obj := map[string]interface{}{}
+	out := map[string]interface{}{}
-	err := json.Unmarshal(data, &obj)
+	err := json.Unmarshal(data, &out)
 	if err != nil {
 		return err
 	}
-	return t.template.Execute(w, obj)
+	if err := t.safeExecute(w, out); err != nil {
+		// It is way easier to debug this stuff when it shows up in
+		// stdout instead of just stdin. So in addition to returning
+		// a nice error, also print useful stuff with the writer.
+		fmt.Fprintf(w, "Error executing template: %v\n", err)
+		fmt.Fprintf(w, "template was:\n%v\n", t.rawTemplate)
+		fmt.Fprintf(w, "raw data was:\n%v\n", string(data))
+		fmt.Fprintf(w, "object given to template engine was:\n%+v\n", out)
+		return fmt.Errorf("error executing template '%v': '%v'\n----data----\n%#v\n", t.rawTemplate, err, out)
+	}
+	return nil
 }
 
 // PrintObj formats the obj with the Go Template.
@@ -356,3 +369,93 @@ func (t *TemplatePrinter) PrintObj(obj runtime.Object, w io.Writer) error {
 	}
 	return t.Print(data, w)
 }
+
+// safeExecute tries to execute the template, but catches panics and returns an error
+// should the template engine panic.
+func (p *TemplatePrinter) safeExecute(w io.Writer, obj interface{}) error {
+	var panicErr error
+	// Sorry for the double anonymous function. There's probably a clever way
+	// to do this that has the defer'd func setting the value to be returned, but
+	// that would be even less obvious.
+	retErr := func() error {
+		defer func() {
+			if x := recover(); x != nil {
+				panicErr = fmt.Errorf("caught panic: %+v", x)
+			}
+		}()
+		return p.template.Execute(w, obj)
+	}()
+	if panicErr != nil {
+		return panicErr
+	}
+	return retErr
+}
+
+// exists returns true if it would be possible to call the index function
+// with these arguments.
+//
+// TODO: how to document this for users?
+//
+// index returns the result of indexing its first argument by the following
+// arguments.  Thus "index x 1 2 3" is, in Go syntax, x[1][2][3]. Each
+// indexed item must be a map, slice, or array.
+func exists(item interface{}, indices ...interface{}) bool {
+	v := reflect.ValueOf(item)
+	for _, i := range indices {
+		index := reflect.ValueOf(i)
+		var isNil bool
+		if v, isNil = indirect(v); isNil {
+			return false
+		}
+		switch v.Kind() {
+		case reflect.Array, reflect.Slice, reflect.String:
+			var x int64
+			switch index.Kind() {
+			case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
+				x = index.Int()
+			case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
+				x = int64(index.Uint())
+			default:
+				return false
+			}
+			if x < 0 || x >= int64(v.Len()) {
+				return false
+			}
+			v = v.Index(int(x))
+		case reflect.Map:
+			if !index.IsValid() {
+				index = reflect.Zero(v.Type().Key())
+			}
+			if !index.Type().AssignableTo(v.Type().Key()) {
+				return false
+			}
+			if x := v.MapIndex(index); x.IsValid() {
+				v = x
+			} else {
+				v = reflect.Zero(v.Type().Elem())
+			}
+		default:
+			return false
+		}
+	}
+	if _, isNil := indirect(v); isNil {
+		return false
+	}
+	return true
+}
+
+// stolen from text/template
+// indirect returns the item at the end of indirection, and a bool to indicate if it's nil.
+// We indirect through pointers and empty interfaces (only) because
+// non-empty interfaces have methods we might need.
+func indirect(v reflect.Value) (rv reflect.Value, isNil bool) {
+	for ; v.Kind() == reflect.Ptr || v.Kind() == reflect.Interface; v = v.Elem() {
+		if v.IsNil() {
+			return v, true
+		}
+		if v.Kind() == reflect.Interface && v.NumMethod() > 0 {
+			break
+		}
+	}
+	return v, false
+}

pkg/kubecfg/resource_printer_test.go

@@ -26,6 +26,8 @@ import (
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
+
 	"github.com/ghodss/yaml"
 )
 
@@ -177,3 +179,127 @@ func TestTemplateEmitsVersionedObjects(t *testing.T) {
 		t.Errorf("Expected %v, got %v", e, a)
 	}
 }
+
+func TestTemplatePanic(t *testing.T) {
+	tmpl := `{{and ((index .currentState.info "update-demo").state.running.startedAt) .currentState.info.net.state.running.startedAt}}`
+	printer, err := NewTemplatePrinter([]byte(tmpl))
+	if err != nil {
+		t.Fatalf("tmpl fail: %v", err)
+	}
+	buffer := &bytes.Buffer{}
+	err = printer.PrintObj(&api.Pod{}, buffer)
+	if err == nil {
+		t.Fatalf("expected that template to crash")
+	}
+	if buffer.String() == "" {
+		t.Errorf("no debugging info was printed")
+	}
+}
+
+func TestTemplateStrings(t *testing.T) {
+	// This unit tests the "exists" function as well as the template from update.sh
+	table := map[string]struct {
+		pod    api.Pod
+		expect string
+	}{
+		"nilInfo":   {api.Pod{}, "false"},
+		"emptyInfo": {api.Pod{Status: api.PodStatus{Info: api.PodInfo{}}}, "false"},
+		"containerExists": {
+			api.Pod{
+				Status: api.PodStatus{
+					Info: api.PodInfo{"update-demo": api.ContainerStatus{}},
+				},
+			},
+			"false",
+		},
+		"netExists": {
+			api.Pod{
+				Status: api.PodStatus{
+					Info: api.PodInfo{"net": api.ContainerStatus{}},
+				},
+			},
+			"false",
+		},
+		"bothExist": {
+			api.Pod{
+				Status: api.PodStatus{
+					Info: api.PodInfo{
+						"update-demo": api.ContainerStatus{},
+						"net":         api.ContainerStatus{},
+					},
+				},
+			},
+			"false",
+		},
+		"oneValid": {
+			api.Pod{
+				Status: api.PodStatus{
+					Info: api.PodInfo{
+						"update-demo": api.ContainerStatus{},
+						"net": api.ContainerStatus{
+							State: api.ContainerState{
+								Running: &api.ContainerStateRunning{
+									StartedAt: util.Time{},
+								},
+							},
+						},
+					},
+				},
+			},
+			"false",
+		},
+		"bothValid": {
+			api.Pod{
+				Status: api.PodStatus{
+					Info: api.PodInfo{
+						"update-demo": api.ContainerStatus{
+							State: api.ContainerState{
+								Running: &api.ContainerStateRunning{
+									StartedAt: util.Time{},
+								},
+							},
+						},
+						"net": api.ContainerStatus{
+							State: api.ContainerState{
+								Running: &api.ContainerStateRunning{
+									StartedAt: util.Time{},
+								},
+							},
+						},
+					},
+				},
+			},
+			"true",
+		},
+	}
+
+	// The point of this test is to verify that the below template works. If you change this
+	// template, you need to update hack/e2e-suite/update.sh.
+	tmpl :=
+		`{{and (exists . "currentState" "info" "update-demo" "state" "running") (exists . "currentState" "info" "net" "state" "running")}}`
+	useThisToDebug := `
+a: {{exists . "currentState"}}
+b: {{exists . "currentState" "info"}}
+c: {{exists . "currentState" "info" "update-demo"}}
+d: {{exists . "currentState" "info" "update-demo" "state"}}
+e: {{exists . "currentState" "info" "update-demo" "state" "running"}}
+f: {{exists . "currentState" "info" "update-demo" "state" "running" "startedAt"}}`
+	_ = useThisToDebug // don't complain about unused var
+
+	printer, err := NewTemplatePrinter([]byte(tmpl))
+	if err != nil {
+		t.Fatalf("tmpl fail: %v", err)
+	}
+
+	for name, item := range table {
+		buffer := &bytes.Buffer{}
+		err = printer.PrintObj(&item.pod, buffer)
+		if err != nil {
+			t.Errorf("%v: unexpected err: %v", name, err)
+			continue
+		}
+		if e, a := item.expect, buffer.String(); e != a {
+			t.Errorf("%v: expected %v, got %v", name, e, a)
+		}
+	}
+}

pkg/kubectl/resource_printer.go

@@ -390,7 +390,9 @@ type TemplatePrinter struct {
 }
 
 func NewTemplatePrinter(tmpl []byte, asVersion string, convertor runtime.ObjectConvertor) (*TemplatePrinter, error) {
-	t, err := template.New("output").Parse(string(tmpl))
+	t, err := template.New("output").
+		Funcs(template.FuncMap{"exists": exists}).
+		Parse(string(tmpl))
 	if err != nil {
 		return nil, err
 	}
@@ -416,12 +418,40 @@ func (p *TemplatePrinter) PrintObj(obj runtime.Object, w io.Writer) error {
 	if err := json.Unmarshal(data, &out); err != nil {
 		return err
 	}
-	if err = p.template.Execute(w, out); err != nil {
+	if err = p.safeExecute(w, out); err != nil {
-		return fmt.Errorf("error executing template '%v': '%v'\n----data----\n%#v\n", p.rawTemplate, err, out)
+		// It is way easier to debug this stuff when it shows up in
+		// stdout instead of just stdin. So in addition to returning
+		// a nice error, also print useful stuff with the writer.
+		fmt.Fprintf(w, "Error executing template: %v\n", err)
+		fmt.Fprintf(w, "template was:\n\t%v\n", p.rawTemplate)
+		fmt.Fprintf(w, "raw data was:\n\t%v\n", string(data))
+		fmt.Fprintf(w, "object given to template engine was:\n\t%+v\n", out)
+		return fmt.Errorf("error executing template '%v': '%v'\n----data----\n%+v\n", p.rawTemplate, err, out)
 	}
 	return nil
 }
 
+// safeExecute tries to execute the template, but catches panics and returns an error
+// should the template engine panic.
+func (p *TemplatePrinter) safeExecute(w io.Writer, obj interface{}) error {
+	var panicErr error
+	// Sorry for the double anonymous function. There's probably a clever way
+	// to do this that has the defer'd func setting the value to be returned, but
+	// that would be even less obvious.
+	retErr := func() error {
+		defer func() {
+			if x := recover(); x != nil {
+				panicErr = fmt.Errorf("caught panic: %+v", x)
+			}
+		}()
+		return p.template.Execute(w, obj)
+	}()
+	if panicErr != nil {
+		return panicErr
+	}
+	return retErr
+}
+
 func tabbedString(f func(io.Writer) error) (string, error) {
 	out := new(tabwriter.Writer)
 	buf := &bytes.Buffer{}
@@ -436,3 +466,72 @@ func tabbedString(f func(io.Writer) error) (string, error) {
 	str := string(buf.String())
 	return str, nil
 }
+
+// exists returns true if it would be possible to call the index function
+// with these arguments.
+//
+// TODO: how to document this for users?
+//
+// index returns the result of indexing its first argument by the following
+// arguments.  Thus "index x 1 2 3" is, in Go syntax, x[1][2][3]. Each
+// indexed item must be a map, slice, or array.
+func exists(item interface{}, indices ...interface{}) bool {
+	v := reflect.ValueOf(item)
+	for _, i := range indices {
+		index := reflect.ValueOf(i)
+		var isNil bool
+		if v, isNil = indirect(v); isNil {
+			return false
+		}
+		switch v.Kind() {
+		case reflect.Array, reflect.Slice, reflect.String:
+			var x int64
+			switch index.Kind() {
+			case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
+				x = index.Int()
+			case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
+				x = int64(index.Uint())
+			default:
+				return false
+			}
+			if x < 0 || x >= int64(v.Len()) {
+				return false
+			}
+			v = v.Index(int(x))
+		case reflect.Map:
+			if !index.IsValid() {
+				index = reflect.Zero(v.Type().Key())
+			}
+			if !index.Type().AssignableTo(v.Type().Key()) {
+				return false
+			}
+			if x := v.MapIndex(index); x.IsValid() {
+				v = x
+			} else {
+				v = reflect.Zero(v.Type().Elem())
+			}
+		default:
+			return false
+		}
+	}
+	if _, isNil := indirect(v); isNil {
+		return false
+	}
+	return true
+}
+
+// stolen from text/template
+// indirect returns the item at the end of indirection, and a bool to indicate if it's nil.
+// We indirect through pointers and empty interfaces (only) because
+// non-empty interfaces have methods we might need.
+func indirect(v reflect.Value) (rv reflect.Value, isNil bool) {
+	for ; v.Kind() == reflect.Ptr || v.Kind() == reflect.Interface; v = v.Elem() {
+		if v.IsNil() {
+			return v, true
+		}
+		if v.Kind() == reflect.Interface && v.NumMethod() > 0 {
+			break
+		}
+	}
+	return v, false
+}

pkg/kubectl/resource_printer_test.go

@@ -290,6 +290,130 @@ func TestTemplateEmitsVersionedObjects(t *testing.T) {
 	}
 }
 
+func TestTemplatePanic(t *testing.T) {
+	tmpl := `{{and ((index .currentState.info "update-demo").state.running.startedAt) .currentState.info.net.state.running.startedAt}}`
+	printer, err := NewTemplatePrinter([]byte(tmpl), testapi.Version(), api.Scheme)
+	if err != nil {
+		t.Fatalf("tmpl fail: %v", err)
+	}
+	buffer := &bytes.Buffer{}
+	err = printer.PrintObj(&api.Pod{}, buffer)
+	if err == nil {
+		t.Fatalf("expected that template to crash")
+	}
+	if buffer.String() == "" {
+		t.Errorf("no debugging info was printed")
+	}
+}
+
+func TestTemplateStrings(t *testing.T) {
+	// This unit tests the "exists" function as well as the template from update.sh
+	table := map[string]struct {
+		pod    api.Pod
+		expect string
+	}{
+		"nilInfo":   {api.Pod{}, "false"},
+		"emptyInfo": {api.Pod{Status: api.PodStatus{Info: api.PodInfo{}}}, "false"},
+		"containerExists": {
+			api.Pod{
+				Status: api.PodStatus{
+					Info: api.PodInfo{"update-demo": api.ContainerStatus{}},
+				},
+			},
+			"false",
+		},
+		"netExists": {
+			api.Pod{
+				Status: api.PodStatus{
+					Info: api.PodInfo{"net": api.ContainerStatus{}},
+				},
+			},
+			"false",
+		},
+		"bothExist": {
+			api.Pod{
+				Status: api.PodStatus{
+					Info: api.PodInfo{
+						"update-demo": api.ContainerStatus{},
+						"net":         api.ContainerStatus{},
+					},
+				},
+			},
+			"false",
+		},
+		"oneValid": {
+			api.Pod{
+				Status: api.PodStatus{
+					Info: api.PodInfo{
+						"update-demo": api.ContainerStatus{},
+						"net": api.ContainerStatus{
+							State: api.ContainerState{
+								Running: &api.ContainerStateRunning{
+									StartedAt: util.Time{},
+								},
+							},
+						},
+					},
+				},
+			},
+			"false",
+		},
+		"bothValid": {
+			api.Pod{
+				Status: api.PodStatus{
+					Info: api.PodInfo{
+						"update-demo": api.ContainerStatus{
+							State: api.ContainerState{
+								Running: &api.ContainerStateRunning{
+									StartedAt: util.Time{},
+								},
+							},
+						},
+						"net": api.ContainerStatus{
+							State: api.ContainerState{
+								Running: &api.ContainerStateRunning{
+									StartedAt: util.Time{},
+								},
+							},
+						},
+					},
+				},
+			},
+			"true",
+		},
+	}
+
+	// The point of this test is to verify that the below template works. If you change this
+	// template, you need to update hack/e2e-suite/update.sh.
+	tmpl :=
+		`{{and (exists . "currentState" "info" "update-demo" "state" "running") (exists . "currentState" "info" "net" "state" "running")}}`
+	useThisToDebug := `
+a: {{exists . "currentState"}}
+b: {{exists . "currentState" "info"}}
+c: {{exists . "currentState" "info" "update-demo"}}
+d: {{exists . "currentState" "info" "update-demo" "state"}}
+e: {{exists . "currentState" "info" "update-demo" "state" "running"}}
+f: {{exists . "currentState" "info" "update-demo" "state" "running" "startedAt"}}`
+	_ = useThisToDebug // don't complain about unused var
+
+	printer, err := NewTemplatePrinter([]byte(tmpl), "v1beta1", api.Scheme)
+	if err != nil {
+		t.Fatalf("tmpl fail: %v", err)
+	}
+
+	for name, item := range table {
+		buffer := &bytes.Buffer{}
+		err = printer.PrintObj(&item.pod, buffer)
+		if err != nil {
+			t.Errorf("%v: unexpected err: %v", name, err)
+			continue
+		}
+		if e, a := item.expect, buffer.String(); e != a {
+			t.Errorf("%v: expected %v, got %v", name, e, a)
+		}
+	}
+}
+
 func TestPrinters(t *testing.T) {
 	om := func(name string) api.ObjectMeta { return api.ObjectMeta{Name: name} }
 	templatePrinter, err := NewTemplatePrinter([]byte("{{.name}}"), testapi.Version(), api.Scheme)