Fix panic when upgrading configurations with patches

pkg/skaffold/schema/latest/config.go

@@ -18,7 +18,6 @@ package latest
 
 import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/util"
-	yamlpatch "github.com/krishicks/yaml-patch"
 )
 
 const Version string = "skaffold/v1beta9"
@@ -539,7 +538,7 @@ type JSONPatch struct {
 	From string `yaml:"from,omitempty"`
 
 	// Value is the value to apply. Can be any portion of yaml.
-	Value *yamlpatch.Node `yaml:"value,omitempty"`
+	Value *util.YamlpatchNode `yaml:"value,omitempty"`
 }
 
 // Activation criteria by which a profile is auto-activated.

pkg/skaffold/schema/profiles.go

@@ -159,11 +159,16 @@ func applyProfile(config *latest.SkaffoldConfig, profile latest.Profile) error {
 			op = "replace"
 		}
 
+		var value *yamlpatch.Node
+		if v := patch.Value; v != nil {
+			value = &v.Node
+		}
+
 		patch := yamlpatch.Operation{
 			Op:    yamlpatch.Op(op),
 			Path:  yamlpatch.OpPath(patch.Path),
 			From:  yamlpatch.OpPath(patch.From),
-			Value: patch.Value,
+			Value: value,
 		}
 
 		if !tryPatch(patch, buf) {

pkg/skaffold/schema/profiles_test.go

@@ -24,6 +24,7 @@ import (
 	cfg "github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/latest"
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/util"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 	yamlpatch "github.com/krishicks/yaml-patch"
 	"k8s.io/client-go/tools/clientcmd/api"
@@ -248,7 +249,7 @@ func TestApplyProfiles(t *testing.T) {
 					Name: "profile",
 					Patches: []latest.JSONPatch{{
 						Path:  "/build/artifacts/0/docker/dockerfile",
-						Value: yamlpatch.NewNode(str("Dockerfile.DEV")),
+						Value: &util.YamlpatchNode{Node: *yamlpatch.NewNode(str("Dockerfile.DEV"))},
 					}},
 				}),
 			),

pkg/skaffold/schema/util/util.go

@@ -21,6 +21,7 @@ import (
 	"reflect"
 	"strings"
 
+	yamlpatch "github.com/krishicks/yaml-patch"
 	"gopkg.in/yaml.v2"
 )
 
@@ -34,24 +35,64 @@ type HelmOverrides struct {
 	Values map[string]interface{} `yaml:",inline"`
 }
 
-type inlineYaml struct {
-	Yaml string
+// MarshalJSON implements JSON marshalling by including the value as an inline yaml fragment.
+func (h *HelmOverrides) MarshalJSON() ([]byte, error) {
+	return marshalInlineYaml(h)
 }
 
-func (h *HelmOverrides) MarshalJSON() ([]byte, error) {
-	yaml, err := yaml.Marshal(h)
+// UnmarshalYAML implements JSON unmarshalling by reading an inline yaml fragment.
+func (h *HelmOverrides) UnmarshalJSON(text []byte) error {
+	yml, err := unmarshalInlineYaml(text)
 	if err != nil {
-		return nil, err
+		return err
 	}
-	return json.Marshal(inlineYaml{string(yaml)})
+	return yaml.Unmarshal([]byte(yml), h)
 }
 
-func (h *HelmOverrides) UnmarshalJSON(text []byte) error {
-	in := inlineYaml{}
-	if err := json.Unmarshal(text, &in); err != nil {
+// YamlpatchNode wraps a `yamlpatch.Node` and makes it serializable to JSON.
+// The yaml serialization needs to be implemented manually, because the node may be
+// an arbitrary yaml fragment so that a field tag `yaml:",inline"` does not work here.
+type YamlpatchNode struct {
+	// node is an arbitrary yaml fragment
+	Node yamlpatch.Node
+}
+
+// MarshalJSON implements JSON marshalling by including the value as an inline yaml fragment.
+func (n *YamlpatchNode) MarshalJSON() ([]byte, error) {
+	return marshalInlineYaml(n)
+}
+
+// UnmarshalYAML implements JSON unmarshalling by reading an inline yaml fragment.
+func (n *YamlpatchNode) UnmarshalJSON(text []byte) error {
+	yml, err := unmarshalInlineYaml(text)
+	if err != nil {
 		return err
 	}
-	return yaml.Unmarshal([]byte(in.Yaml), h)
+	return yaml.Unmarshal([]byte(yml), n)
+}
+
+// MarshalYAML implements yaml.Marshaler.
+func (n *YamlpatchNode) MarshalYAML() (interface{}, error) {
+	return n.Node.MarshalYAML()
+}
+
+// UnmarshalYAML implements yaml.Unmarshaler
+func (n *YamlpatchNode) UnmarshalYAML(unmarshal func(interface{}) error) error {
+	return n.Node.UnmarshalYAML(unmarshal)
+}
+
+func marshalInlineYaml(in interface{}) ([]byte, error) {
+	yaml, err := yaml.Marshal(in)
+	if err != nil {
+		return nil, err
+	}
+	return json.Marshal(string(yaml))
+}
+
+func unmarshalInlineYaml(text []byte) (string, error) {
+	var in string
+	err := json.Unmarshal(text, &in)
+	return in, err
 }
 
 // IsOneOfField checks if a field is tagged with oneOf

pkg/skaffold/schema/util/util_test.go

@@ -24,14 +24,14 @@ import (
 	yaml "gopkg.in/yaml.v2"
 )
 
-const overridesYaml string = `global:
+const yamlFragment string = `global:
   enabled: true
   localstack: {}
 `
 
 func TestHelmOverridesMarshalling(t *testing.T) {
 	h := &HelmOverrides{}
-	err := yaml.Unmarshal([]byte(overridesYaml), h)
+	err := yaml.Unmarshal([]byte(yamlFragment), h)
 	testutil.CheckError(t, false, err)
 
 	asJSON, err := json.Marshal(h)
@@ -41,12 +41,12 @@ func TestHelmOverridesMarshalling(t *testing.T) {
 	testutil.CheckError(t, false, err)
 
 	actual, err := yaml.Marshal(h)
-	testutil.CheckErrorAndDeepEqual(t, false, err, overridesYaml, string(actual))
+	testutil.CheckErrorAndDeepEqual(t, false, err, yamlFragment, string(actual))
 }
 
 func TestHelmOverridesWhenEmbedded(t *testing.T) {
 	h := HelmOverrides{}
-	err := yaml.Unmarshal([]byte(overridesYaml), &h)
+	err := yaml.Unmarshal([]byte(yamlFragment), &h)
 	testutil.CheckError(t, false, err)
 
 	out, err := yaml.Marshal(struct {
@@ -59,3 +59,34 @@ func TestHelmOverridesWhenEmbedded(t *testing.T) {
     localstack: {}
 `, string(out))
 }
+
+func TestYamlpatchNodeMarshalling(t *testing.T) {
+	n := &YamlpatchNode{}
+	err := yaml.Unmarshal([]byte(yamlFragment), n)
+	testutil.CheckError(t, false, err)
+
+	asJSON, err := json.Marshal(n)
+	testutil.CheckError(t, false, err)
+
+	err = json.Unmarshal(asJSON, n)
+	testutil.CheckError(t, false, err)
+
+	actual, err := yaml.Marshal(n)
+	testutil.CheckErrorAndDeepEqual(t, false, err, yamlFragment, string(actual))
+}
+
+func TestYamlpatchNodeWhenEmbedded(t *testing.T) {
+	n := &YamlpatchNode{}
+	err := yaml.Unmarshal([]byte(yamlFragment), &n)
+	testutil.CheckError(t, false, err)
+
+	out, err := yaml.Marshal(struct {
+		Node *YamlpatchNode `yaml:"value,omitempty"`
+	}{n})
+
+	testutil.CheckErrorAndDeepEqual(t, false, err, `value:
+  global:
+    enabled: true
+    localstack: {}
+`, string(out))
+}

pkg/skaffold/schema/v1beta5/config.go

@@ -18,7 +18,6 @@ package v1beta5
 
 import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/util"
-	yamlpatch "github.com/krishicks/yaml-patch"
 )
 
 const Version string = "skaffold/v1beta5"
@@ -499,7 +498,7 @@ type JSONPatch struct {
 	From string `yaml:"from,omitempty"`
 
 	// Value is the value to apply. Can be any portion of yaml.
-	Value *yamlpatch.Node `yaml:"value,omitempty"`
+	Value *util.YamlpatchNode `yaml:"value,omitempty"`
 }
 
 // Activation criteria by which a profile is auto-activated.

pkg/skaffold/schema/v1beta6/config.go

@@ -18,7 +18,6 @@ package v1beta6
 
 import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/util"
-	yamlpatch "github.com/krishicks/yaml-patch"
 )
 
 const Version string = "skaffold/v1beta6"
@@ -523,7 +522,7 @@ type JSONPatch struct {
 	From string `yaml:"from,omitempty"`
 
 	// Value is the value to apply. Can be any portion of yaml.
-	Value *yamlpatch.Node `yaml:"value,omitempty"`
+	Value *util.YamlpatchNode `yaml:"value,omitempty"`
 }
 
 // Activation criteria by which a profile is auto-activated.

pkg/skaffold/schema/v1beta7/config.go

@@ -18,7 +18,6 @@ package v1beta7
 
 import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/util"
-	yamlpatch "github.com/krishicks/yaml-patch"
 )
 
 const Version string = "skaffold/v1beta7"
@@ -512,7 +511,7 @@ type JSONPatch struct {
 	From string `yaml:"from,omitempty"`
 
 	// Value is the value to apply. Can be any portion of yaml.
-	Value *yamlpatch.Node `yaml:"value,omitempty"`
+	Value *util.YamlpatchNode `yaml:"value,omitempty"`
 }
 
 // Activation criteria by which a profile is auto-activated.

pkg/skaffold/schema/v1beta8/config.go

@@ -18,7 +18,6 @@ package v1beta8
 
 import (
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/util"
-	yamlpatch "github.com/krishicks/yaml-patch"
 )
 
 const Version string = "skaffold/v1beta8"
@@ -539,7 +538,7 @@ type JSONPatch struct {
 	From string `yaml:"from,omitempty"`
 
 	// Value is the value to apply. Can be any portion of yaml.
-	Value *yamlpatch.Node `yaml:"value,omitempty"`
+	Value *util.YamlpatchNode `yaml:"value,omitempty"`
 }
 
 // Activation criteria by which a profile is auto-activated.

pkg/skaffold/schema/validation/validation.go

@@ -54,6 +54,9 @@ func visitStructs(s interface{}, visitor func(interface{}) error) []error {
 
 		// also check all fields of the current struct
 		for i := 0; i < t.NumField(); i++ {
+			if !v.Field(i).CanInterface() {
+				continue
+			}
 			if fieldErrs := visitStructs(v.Field(i).Interface(), visitor); fieldErrs != nil {
 				errs = append(errs, fieldErrs...)
 			}

pkg/skaffold/schema/validation/validation_test.go

@@ -192,6 +192,55 @@ func TestVisitStructs(t *testing.T) {
 			},
 			expectedErrs: 2,
 		},
+		{
+			name: "unexported fields",
+			input: struct {
+				a emptyStruct
+			}{
+				a: emptyStruct{},
+			},
+			expectedErrs: 1,
+		},
+		{
+			name: "exported and unexported fields",
+			input: struct {
+				a, A, b emptyStruct
+			}{
+				a: emptyStruct{},
+				A: emptyStruct{},
+				b: emptyStruct{},
+			},
+			expectedErrs: 2,
+		},
+		{
+			name: "unexported nil ptr fields",
+			input: struct {
+				a *emptyStruct
+			}{
+				a: nil,
+			},
+			expectedErrs: 1,
+		},
+		{
+			name: "unexported ptr fields",
+			input: struct {
+				a *emptyStruct
+			}{
+				a: &emptyStruct{},
+			},
+			expectedErrs: 1,
+		},
+		{
+			name: "unexported and exported ptr fields",
+			input: struct {
+				a, A, b *emptyStruct
+			}{
+				a: &emptyStruct{},
+				A: &emptyStruct{},
+				b: &emptyStruct{},
+			},
+			expectedErrs: 2,
+		},
 	}
 
 	for _, test := range tests {