kpt fn sink outputs invalid JSON to .json files

[yhrn]

Expected behavior

> mkdir config
> echo '{"a": "a long string that would absolutely certainly see a newline introduced by the YAML marshaller"}' >config/a.json
> kpt fn source config | kpt fn sink config
> cat config a.json
{
  "a": "a long string that would absolutely certainly see a newline introduced by the YAML marshaller"
}

Actual behavior

> mkdir config
> echo '{"a": "a long string that would absolutely certainly see a newline introduced by the YAML marshaller"}' >config/a.json
> kpt fn source config | kpt fn sink config
> cat config a.json
{"a": "a long string that would absolutely certainly see a newline introduced by the
    YAML marshaller"}

As you can see this is not valid JSON because of the newline in the string, which is treated as just a single whitespace in YAML.

Information

Kpt version: 0.39.2

This could be seen as a follow up to #647 and kubernetes-sigs/kustomize/pull/2546.

I have opened kubernetes-sigs/kustomize/pull/3934 to address the issue but I haven't seen much activity there. Even if it gets accepted I'm not sure how easy it will be to get it into kpt considering the kustomize deps here are lagging a bit and there have been breaking changes since (as described in the PR).

For more context on the relevance of this I was trying to run a function on a directory containing a Kustomization using the new custom OpenAPI feature, which requires you to have a JSON formatted OpenAPI schema in the same directory (YAML is not supported and it can't be placed outside of the kustomization directory). I wanted to run this imperatively, writing the output to another directory before doing kustomize build so changing the schema file extension to something other than .json wouldn't work either since it wouldn't be included in the output.

[mikebz]

@Shell32-Natsu and @natasha41575 is the PR something you can take a look at on the kustomize side?

[droot]

I have opened kubernetes-sigs/kustomize/pull/3934 to address the issue but I haven't seen much activity there. Even if it gets accepted I'm not sure how easy it will be to get it into kpt considering the kustomize deps here are lagging a bit and there have been breaking changes since (as described in the PR).

Thanks for the PR, it's nicely done. I just reviewed it and have a minor comment. I will check how easy it will be to get this in on master branch. On next branch, we have been proactive in using recent versions of kyaml.

For more context on the relevance of this I was trying to run a function on a directory containing a Kustomization using the new custom OpenAPI feature, which requires you to have a JSON formatted OpenAPI schema in the same directory (YAML is not supported and it can't be placed outside of the kustomization directory). I wanted to run this imperatively, writing the output to another directory before doing kustomize build so changing the schema file extension to something other than .json wouldn't work either since it wouldn't be included in the output.

I will discuss with @natasha41575 if there is a way to solve this issue and post an update.

[natasha41575]

YAML is not supported and it can't be placed outside of the kustomization directory

Would it help if kustomize supported specifying the OpenAPI schema in yaml? We might be able to do that. Please feel free to file an issue for that in kustomize if that's something you want to see and I can follow up.

[droot]

@yhrn Had a chat with @natasha41575 . Using krmignore file to ignore the openschema file is another option, she is going to try that out and confirm if that will solve the original issue you are running into.

[yhrn]

YAML is not supported and it can't be placed outside of the kustomization directory

Would it help if kustomize supported specifying the OpenAPI schema in yaml? We might be able to do that. Please feel free to file an issue for that in kustomize if that's something you want to see and I can follow up.

That would be another way of solving the exact use case I had here and it would also be nice to have since for consistency and because YAML is often a bit more ergonomic to deal with, but I also think the PR I opened makes sense regardless. Opened an issue: kubernetes-sigs/kustomize/issues/3961

@yhrn Had a chat with @natasha41575 . Using krmignore file to ignore the openschema file is another option, she is going to try that out and confirm if that will solve the original issue you are running into.

Can you provide some more info on krmignore? If it causes kpt fn source to completely ignore the file then I can achieve the same thing by changing the extension and it won't solve my use case since I'm reading from one directory and writing to another. But if the file is still read by kpt fn source but bypasses any functions before being written by kpt fn sink then it's a great option.

[droot]

but I also think the PR I opened makes sense regardless.

Totally agreed.

Can you provide some more info on krmignore? If it causes kpt fn source to completely ignore the file then I can achieve the same thing by changing the extension and it won't solve my use case since I'm reading from one directory and writing to another. But if the file is still read by kpt fn source but bypasses any functions before being written by kpt fn sink then it's a great option.

Aah, you are right, krmignore won't solve the problem and yes it is equivalent to changing the extension.

So if this is an imperative workflow, one (not-so-great) option is to copy the schema in the destination dir at the beginning of the imperative steps (also adding it to krmignore file so that kpt source ignores it). It's way of telling kpt ignore this file, I will handle it myself, but it will be available for kustomize build step in the destination directory after the kpt chain.

[yhrn]

Aah, you are right, krmignore won't solve the problem and yes it is equivalent to changing the extension.

So if this is an imperative workflow, one (not-so-great) option is to copy the schema in the destination dir at the beginning of the imperative steps (also adding it to krmignore file so that kpt source ignores it). It's way of telling kpt ignore this file, I will handle it myself, but it will be available for kustomize build step in the destination directory after the kpt chain.

Yes, but I really would like to avoid that because I'm trying to put a generic pattern in place with some tooling around it where I apply a set of functions, defined in one directory, to another source directory, then run kustomize build on the fn output and finally writing that to a separate destination. Given this I would need to introduce some mechanism/convention for copying some files separately between the source and destination, which I think would be very unfortunate.

[droot]

@yhrn completely agree. My suggestion was sort of a ugly hack to unblock you :). I see that the kustomize PR is merged (Huge thanks for that), we will have the kpt next branch (v1.0+) updated very soon as we are waiting for a few more fixes in the kyaml pkg to land.

@natasha41575 is investigating how easy it is to get kpt alpha updated with latest kyaml. Given that it hasn't been updated in a while, update might be bumpy.

[droot]

kyaml v0.10.21 is released that contains the @yhrn PR. https://github.com/GoogleContainerTools/kpt/pull/2181 upgrades kpt next branch to latest kyaml release.

So far the upgrade of kyaml on kpt alpha isn't looking bad. Hoping to get this merged today.

[natasha41575]

I spoke with @droot and @mortent and it looks like this change will be difficult to get into v0 without breaking something. It may make more sense to close this issue as the fix is in v1 and we are hopeful that customers will migrate to it soon. Have you had the chance to try out v1 yet?

[yhrn]

I haven't had time to try v1 a lot yet and I'm not sure how quickly it will be an option for us. Even if all the features are there I assume it will take some time for it to stabilize enough for production usage. I realize v0 is viewed as some kind of alpha but it's still been in use for quite a while.

[natasha41575]

@yhrn Understood. We are working to see what we can do about the breaking changes in go-yaml. After that we will revisit this and see if doing the upgrade in v0 is feasible.

[yhrn]

@natasha41575 I guess another option is to release a patched kyaml v0.10.17 version, e.g. v0.10.17-patch1 that only adds the relevant fixes to kyaml v0.10.17. I have already done this here and here (note that the replace directive is only necessary because I'm referencing my fork of Kustomize where the module paths don't match the repo)

[natasha41575]

I guess another option is to release a patched kyaml v0.10.17 version, e.g. v0.10.17-patch1

Will keep this in mind if the change in go-yaml isn't enough, but it looks like someone is working on fixing the indentation issue (https://github.com/go-yaml/yaml/issues/720) after which I am hoping we won't need a replace directive anymore.

I'm going to keep this in the backlog while we wait for an update on that.