Use YAMLToJSONStrict instead of YAMLToJSON when loading patches

pkg/cluster/internal/create/actions/config/config.go

@@ -26,6 +26,7 @@ import (
 	"sigs.k8s.io/kind/pkg/cluster/constants"
 	"sigs.k8s.io/kind/pkg/cluster/nodes"
 	"sigs.k8s.io/kind/pkg/errors"
+	"sigs.k8s.io/kind/pkg/log"
 
 	"sigs.k8s.io/kind/pkg/cluster/internal/create/actions"
 	"sigs.k8s.io/kind/pkg/cluster/internal/kubeadm"
@@ -87,7 +88,7 @@ func (a *Action) Execute(ctx *actions.ActionContext) error {
 	kubeadmConfigPlusPatches := func(node nodes.Node, data kubeadm.ConfigData) func() error {
 		return func() error {
 			data.NodeName = node.String()
-			kubeadmConfig, err := getKubeadmConfig(ctx.Config, data, node, provider)
+			kubeadmConfig, err := getKubeadmConfig(ctx.Config, data, node, provider, ctx.Logger)
 			if err != nil {
 				// TODO(bentheelder): logging here
 				return errors.Wrap(err, "failed to generate kubeadm config content")
@@ -154,7 +155,7 @@ func (a *Action) Execute(ctx *actions.ActionContext) error {
 
 // getKubeadmConfig generates the kubeadm config contents for the cluster
 // by running data through the template and applying patches as needed.
-func getKubeadmConfig(cfg *config.Cluster, data kubeadm.ConfigData, node nodes.Node, provider string) (path string, err error) {
+func getKubeadmConfig(cfg *config.Cluster, data kubeadm.ConfigData, node nodes.Node, provider string, logger log.Logger) (path string, err error) {
 	kubeVersion, err := nodeutils.KubeVersion(node)
 	if err != nil {
 		// TODO(bentheelder): logging here
@@ -224,14 +225,14 @@ func getKubeadmConfig(cfg *config.Cluster, data kubeadm.ConfigData, node nodes.N
 
 	clusterPatches, clusterJSONPatches := allPatchesFromConfig(cfg)
 	// apply cluster-level patches first
-	patchedConfig, err := patch.KubeYAML(cf, clusterPatches, clusterJSONPatches)
+	patchedConfig, err := patch.KubeYAML(cf, clusterPatches, clusterJSONPatches, logger)
 	if err != nil {
 		return "", err
 	}
 
 	// if needed, apply current node's patches
 	if len(configNode.KubeadmConfigPatches) > 0 || len(configNode.KubeadmConfigPatchesJSON6902) > 0 {
-		patchedConfig, err = patch.KubeYAML(patchedConfig, configNode.KubeadmConfigPatches, configNode.KubeadmConfigPatchesJSON6902)
+		patchedConfig, err = patch.KubeYAML(patchedConfig, configNode.KubeadmConfigPatches, configNode.KubeadmConfigPatchesJSON6902, logger)
 		if err != nil {
 			return "", err
 		}

pkg/cluster/internal/create/actions/installcni/cni.go

@@ -110,7 +110,7 @@ func (a *action) Execute(ctx *actions.ActionContext) error {
 			Patch:   patchValue,
 		}
 
-		patchedConfig, err := patch.KubeYAML(manifest, nil, []config.PatchJSON6902{controlPlanePatch6902})
+		patchedConfig, err := patch.KubeYAML(manifest, nil, []config.PatchJSON6902{controlPlanePatch6902}, ctx.Logger)
 		if err != nil {
 			return err
 		}

pkg/internal/patch/kubeyaml.go

@@ -17,6 +17,7 @@ limitations under the License.
 package patch
 
 import (
+	"sigs.k8s.io/kind/pkg/log"
 	"strings"
 
 	"sigs.k8s.io/kind/pkg/errors"
@@ -34,13 +35,13 @@ import (
 //
 // Patches match if their kind and apiVersion match a document, with the exception
 // that if the patch does not set apiVersion it will be ignored.
-func KubeYAML(toPatch string, patches []string, patches6902 []config.PatchJSON6902) (string, error) {
+func KubeYAML(toPatch string, patches []string, patches6902 []config.PatchJSON6902, logger log.Logger) (string, error) {
 	// pre-process, including splitting up documents etc.
 	resources, err := parseResources(toPatch)
 	if err != nil {
 		return "", errors.Wrap(err, "failed to parse yaml to patch")
 	}
-	mergePatches, err := parseMergePatches(patches)
+	mergePatches, err := parseMergePatches(patches, logger)
 	if err != nil {
 		return "", errors.Wrap(err, "failed to parse patches")
 	}

pkg/internal/patch/kubeyaml_test.go

@@ -21,6 +21,7 @@ import (
 
 	"sigs.k8s.io/kind/pkg/internal/apis/config"
 	"sigs.k8s.io/kind/pkg/internal/assert"
+	"sigs.k8s.io/kind/pkg/log"
 )
 
 func TestKubeYAML(t *testing.T) {
@@ -61,12 +62,19 @@ func TestKubeYAML(t *testing.T) {
 			ExpectError:     false,
 			ExpectOutput:    normalKubeadmConfigTrivialPatchedAnd6902Patched,
 		},
+		{
+			Name:         "kubeadm config one merge-patch with malformed yaml (duplicate key)",
+			ToPatch:      normalKubeadmConfig,
+			Patches:      []string{malformedPatchWithDuplicateKey},
+			ExpectOutput: malformedPatchWithDuplicateKeyKustomized,
+			ExpectError:  false,
+		},
 	}
 	for _, tc := range cases {
 		tc := tc // capture test case
 		t.Run(tc.Name, func(t *testing.T) {
 			t.Parallel()
-			out, err := KubeYAML(tc.ToPatch, tc.Patches, tc.PatchesJSON6902)
+			out, err := KubeYAML(tc.ToPatch, tc.Patches, tc.PatchesJSON6902, log.NoopLogger{})
 			assert.ExpectError(t, tc.ExpectError, err)
 			if err == nil {
 				assert.StringEqual(t, tc.ExpectOutput, out)
@@ -402,3 +410,83 @@ kind: KubeProxyConfiguration
 metadata:
   name: config
 `
+const malformedPatchWithDuplicateKey = `
+kind: ClusterConfiguration
+apiVersion: kubeadm.k8s.io/v1beta2
+
+scheduler:
+  extraArgs:
+   some-duplicate-key: value1
+   some-duplicate-key: value2
+`
+
+const malformedPatchWithDuplicateKeyKustomized = `apiServer:
+  certSANs:
+  - localhost
+  - 127.0.0.1
+apiVersion: kubeadm.k8s.io/v1beta2
+clusterName: kind
+controlPlaneEndpoint: 192.168.9.3:6443
+controllerManager:
+  extraArgs:
+    enable-hostpath-provisioner: "true"
+kind: ClusterConfiguration
+kubernetesVersion: v1.15.3
+metadata:
+  name: config
+networking:
+  podSubnet: 10.244.0.0/16
+  serviceSubnet: 10.96.0.0/12
+scheduler:
+  extraArgs:
+    some-duplicate-key: value2
+---
+apiVersion: kubeadm.k8s.io/v1beta2
+bootstrapTokens:
+- token: abcdef.0123456789abcdef
+kind: InitConfiguration
+localAPIEndpoint:
+  advertiseAddress: 192.168.9.6
+  bindPort: 6443
+metadata:
+  name: config
+nodeRegistration:
+  criSocket: /run/containerd/containerd.sock
+  kubeletExtraArgs:
+    fail-swap-on: "false"
+    node-ip: 192.168.9.6
+---
+apiVersion: kubeadm.k8s.io/v1beta2
+controlPlane:
+  localAPIEndpoint:
+    advertiseAddress: 192.168.9.6
+    bindPort: 6443
+discovery:
+  bootstrapToken:
+    apiServerEndpoint: 192.168.9.3:6443
+    token: abcdef.0123456789abcdef
+    unsafeSkipCAVerification: true
+kind: JoinConfiguration
+metadata:
+  name: config
+nodeRegistration:
+  criSocket: /run/containerd/containerd.sock
+  kubeletExtraArgs:
+    fail-swap-on: "false"
+    node-ip: 192.168.9.6
+---
+apiVersion: kubelet.config.k8s.io/v1beta1
+evictionHard:
+  imagefs.available: 0%
+  nodefs.available: 0%
+  nodefs.inodesFree: 0%
+imageGCHighThresholdPercent: 100
+kind: KubeletConfiguration
+metadata:
+  name: config
+---
+apiVersion: kubeproxy.config.k8s.io/v1alpha1
+kind: KubeProxyConfiguration
+metadata:
+  name: config
+`

pkg/internal/patch/mergepatch.go

@@ -17,9 +17,9 @@ limitations under the License.
 package patch
 
 import (
-	"sigs.k8s.io/yaml"
-
 	"sigs.k8s.io/kind/pkg/errors"
+	"sigs.k8s.io/kind/pkg/log"
+	"sigs.k8s.io/yaml"
 )
 
 type mergePatch struct {
@@ -28,7 +28,7 @@ type mergePatch struct {
 	matchInfo matchInfo // for matching resources
 }
 
-func parseMergePatches(rawPatches []string) ([]mergePatch, error) {
+func parseMergePatches(rawPatches []string, logger log.Logger) ([]mergePatch, error) {
 	patches := []mergePatch{}
 	// split document streams before trying to parse them
 	splitRawPatches := make([]string, 0, len(rawPatches))
@@ -44,9 +44,17 @@ func parseMergePatches(rawPatches []string) ([]mergePatch, error) {
 		if err != nil {
 			return nil, errors.WithStack(err)
 		}
-		json, err := yaml.YAMLToJSON([]byte(raw))
+		json, err := yaml.YAMLToJSONStrict([]byte(raw))
 		if err != nil {
-			return nil, errors.WithStack(err)
+			logger.Warnf("Failed to strictly convert patch of kind %s to json: \"%v\". "+
+				"Trying more permissive conversion.",
+				matchInfo.Kind,
+				err,
+			)
+			json, err = yaml.YAMLToJSON([]byte(raw))
+			if err != nil {
+				return nil, errors.WithStack(err)
+			}
 		}
 		patches = append(patches, mergePatch{
 			raw:       raw,