Use YAMLToJSONStrict instead of YAMLToJSON when loading patches

[tom1299]

Currently when yamls with duplicate key entries are used for config patches, neither a warning nor an error message is displayed. For example using this malformed patch (See duplicate key in the last line) :

kind: Cluster
apiVersion: kind.x-k8s.io/v1alpha4
nodes:
- role: control-plane
kubeadmConfigPatches:
- |
  kind: InitConfiguration
  nodeRegistration:
    kubeletExtraArgs:
      anonymous-auth: "true"
      anonymous-auth: "false"

with kind create cluster --config ./kind-config.yaml -v=9 will result in the following kubeadm.config (only relavant lines are shown):

  kubeletExtraArgs:
    anonymous-auth: "false"

This PR contains a change to use YAMLToJSONStrict instead of the more relaxed YAMLToJSON. I also added a test case for an example with a duplicate key.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tom1299
Once this PR has been reviewed and has the lgtm label, please assign bentheelder for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[k8s-ci-robot]

Hi @tom1299. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[stmcginnis]

Some may expect the more lenient handling at this point. Not sure how big of an impact this would be.

[tom1299]

@stmcginnis I was thinking that as well. kubeadm init displays a warning but continues to setup the node when the kubeadm.config contains duplicate keys. I could alter the logic to first try strict, and if there was an error, display that and do it the lenient way. That would reflect the behavior of kubeadm.

[tom1299]

@stmcginnis I changed the implementation to a more permissive approach which just writes a warning to stderr:

$ kind create cluster --config ./kind-config.yaml 
Creating cluster "kind" ...
 ✓ Ensuring node image (kindest/node:v1.34.0) 🖼
 ✓ Preparing nodes 📦  
⠈⠑ Writing configuration 📜 
WARN: Error converting patch of kind InitConfiguration to json: "yaml: unmarshal errors:
  line 5: key "anonymous-auth" already set in map". Trying more permissive conversion.
 ✓ Writing configuration 📜 
 ✓ Starting control-plane 🕹️ 
 ✓ Installing CNI 🔌 
 ✓ Installing StorageClass 💾 
Set kubectl context to "kind-kind"
You can now use your cluster with:

kubectl cluster-info --context kind-kind

Thanks for using kind! 😊

The warning messes up the output a little bit though. Maybe there is a better way to log this error (?) This change would not impact existing malformed patches and would reflect the current behaviour of kubeadm init as this command also logs a warning and continues.

[stmcginnis]

A couple small things that will cause CI failure.

Not sure if we have a better way to get the logger instance. I know some people using kind as a library, so I don't love changing the method signatures. But if that's what we have to do...

[tom1299]

I didn't like changing the signatures of the functions either. Especially for such a small change. But I couldn't really find a better way.

[stmcginnis]

/ok-to-test

@BenTheElder @aojea what do you think about the method signature change? I think it may be worth it to have this output. I actually have run into an instance where some templating errors were duplicating keys and it was not at all obvious why the resulting cluster wasn't configured right.

[aojea]

+1 to strict parsing, I got bitted several times because of that and it takes a lot of time to figure out the problem

@BenTheElder is the best person for the plumbing of the logger