Merge pull request #1864 from dgageot/fix-1851

Verify patches and fail with a proper error message

pkg/skaffold/schema/profiles.go

@@ -156,12 +156,18 @@ func applyProfile(config *latest.SkaffoldPipeline, profile latest.Profile) error
 			op = "replace"
 		}
 
-		patches = append(patches, yamlpatch.Operation{
+		patch := yamlpatch.Operation{
 			Op:    yamlpatch.Op(op),
 			Path:  yamlpatch.OpPath(patch.Path),
 			From:  yamlpatch.OpPath(patch.From),
 			Value: patch.Value,
-		})
+		}
+
+		if !tryPatch(patch, buf) {
+			return fmt.Errorf("invalid path: %s", patch.Path)
+		}
+
+		patches = append(patches, patch)
 	}
 
 	buf, err = yamlpatch.Patch(patches).Apply(buf)
@@ -172,6 +178,20 @@ func applyProfile(config *latest.SkaffoldPipeline, profile latest.Profile) error
 	return yaml.Unmarshal(buf, config)
 }
 
+// tryPatch is here to verify patches one by one before we
+// apply them because yamlpatch.Patch is known to panic when a path
+// is not valid.
+func tryPatch(patch yamlpatch.Operation, buf []byte) (valid bool) {
+	defer func() {
+		if errPanic := recover(); errPanic != nil {
+			valid = false
+		}
+	}()
+
+	_, err := yamlpatch.Patch([]yamlpatch.Operation{patch}).Apply(buf)
+	return err == nil
+}
+
 func profilesByName(profiles []latest.Profile) map[string]latest.Profile {
 	byName := make(map[string]latest.Profile)
 	for _, profile := range profiles {

pkg/skaffold/schema/profiles_test.go

@@ -70,6 +70,34 @@ profiles:
 	testutil.CheckDeepEqual(t, "Dockerfile.second", pipeline.Build.Artifacts[1].DockerArtifact.DockerfilePath)
 }
 
+func TestApplyInvalidPatch(t *testing.T) {
+	config := `build:
+  artifacts:
+  - image: example
+profiles:
+- name: patches
+  patches:
+  - path: /build/artifacts/0/image/
+    value: replacement
+`
+
+	tmp, cleanup := testutil.NewTempDir(t)
+	defer cleanup()
+
+	yaml := fmt.Sprintf("apiVersion: %s\nkind: Config\n%s", latest.Version, config)
+	tmp.Write("skaffold.yaml", yaml)
+
+	parsed, err := ParseConfig(tmp.Path("skaffold.yaml"), false)
+	testutil.CheckError(t, false, err)
+
+	pipeline := parsed.(*latest.SkaffoldPipeline)
+	err = ApplyProfiles(pipeline, &cfg.SkaffoldOptions{
+		Profiles: []string{"patches"},
+	})
+
+	testutil.CheckErrorAndDeepEqual(t, true, err, "appying profile patches: invalid path: /build/artifacts/0/image/", err.Error())
+}
+
 func TestApplyProfiles(t *testing.T) {
 	tests := []struct {
 		description string