Upgrade yaml.v2 to yaml.v3, improve testing and bugfixes #61

inteon · 2021-09-11T00:25:26Z

fixes: #58
fixes: #47
fixes: #45
fixes: #18

Thorough refactor of this repo, improves tests & tries to solve the most common problems (by upgrading to yaml.v3 and fixing some bugs).
JSONObjectToYAMLObject was removed, but this function does not seem to be used a lot (please correct me if I'm wrong).

This might have to be release as v2 of this go library.

k8s-ci-robot · 2021-09-11T00:25:29Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2021-09-11T00:25:33Z

Welcome @inteon!

It looks like this is your first PR to kubernetes-sigs/yaml 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/yaml has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-09-11T00:25:34Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: inteon
To complete the pull request process, please assign dims after the PR has been reviewed.
You can assign the PR to them by writing /assign @dims in a comment when ready.

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

lavalamp · 2021-09-15T21:00:55Z

I think @liggitt is a better person to ask.

luxas · 2021-09-16T08:48:23Z

Discussing with @inteon on the Kubernetes slack now (https://kubernetes.slack.com/archives/C0EG7JC6T/p1631778930193700?thread_ts=1631728530.180800&cid=C0EG7JC6T); there is a need to redesign this library more or less completely in a spec-driven way, i.e. write a KEP for what a v2 of this library should look like.

Furthermore, to be consistent with the k8s API machinery, we also need to depend on the same JSON logic from here https://github.com/kubernetes/apimachinery/blob/master/pkg/runtime/serializer/json/json.go#L113-L184, and hence I think creating a sigs.k8s.io/json encoding/json drop-in would be a prerequisite for a v2 of this library.

This again (sigs.k8s.io/json) would need its own KEP, due to a multitude of needs from kubernetes/enhancements#2082, the defaulting KEP, and kubernetes/enhancements#2970, the "strict" validation KEP (work in kubernetes/kubernetes#104433).

I have listed some issues needing resolution in https://github.com/luxas/deklarative/issues (a PoC/MVP repo for this kind of work), have some PoC work of what sigs.k8s.io/yaml v2 could look like in luxas/deklarative#8, and have identified some upstream issues that need fixing in https://github.com/json-iterator/go/issues. But there's more to this.

From the top of my head, these are the specifications:

sigs.k8s.io/json
- Round-trip all integers correctly, to not precision above 2^53 (we use a json-iterator extension for this in k8s.io/apimachinery)
- Provide a mode to error/warn for duplicate (Error for duplicate fields json-iterator/go#575) and unknown fields (k8s.io/apimachinery supports unknown fields, but not duplicate in json-only mode, which affects WIP: Strict schema validation for POST and PUT kubernetes/kubernetes#104433). Note that duplicate keys are forbidden in YAML, anyways, so duplicate keys should probably error by default, and not merge using this pretty non-user-friendly behavior (maps merge, scalars replace: Make json-iter decoder able to recognize duplicate fields luxas/deklarative#5, example: https://play.golang.org/p/CkCMdoGXc07)
- Be case sensitive (encoding/json is not, already enforced in k8s.io/apimachinery)
- Return struct errors (wrong line number in yaml syntax error message kubernetes/kubernetes#104607 (comment))
- Actually omit encoding empty structs marked omitempty (this doesn't work in any json library so far, but could probably be made an extension to json iterator)
- Encode json.RawMessage correctly with indentation
- Make sure the encoder/decoder supports "framing", like we depend on for e.g. Kube API server watches. This should more or less "just work", but needs testing.
sigs.k8s.io/yaml
- Use YAML 1.2 (e.g. no implicit typing)
- Most of the specifications of the above json package; hence use sigs.k8s.io/json for marshalling and unmarshalling Go types, to always be consistent with the JSON logic.
- Disallow all YAML features not allowed in JSON, or convert (?), e.g. non-string keys.
- Allow configurable sequence indentation level (problem in kyaml, ref yaml.v3 fork maintenance story kustomize#4033), and auto-detection.
- Return struct errors
- Make sure "framing" is supported, such that a stream of YAML documents can be split up individually

We need to make sure we agree on a specific subset of the YAML + JSON union that is considered "supported" for Kubernetes, and what is out of scope.

liggitt · 2021-09-16T11:32:39Z

kubernetes/kubernetes#105030 Is relevant to the json bits. It's more driven by an immediate desire to drop json-iterator because of correctness and compatibility reasons.

luxas · 2021-09-16T11:38:21Z

Oh, wow, I had no idea that was going on, thanks for the heads up @liggitt, will definitely take a look 👍!

liggitt · 2021-09-16T11:50:02Z

That PR is heavily oriented toward:

Replacing existing use of json-iterator (we only have two configurations we use, case-sensitive and strict)
Avoiding exposing additional API surface area
Using the standard library code as much as possible for compatibility and correctness reasons
Limiting additions to things we think we can reasonably upstream, and eventually collapse back onto the standard decoder

luxas · 2021-10-21T09:06:03Z

A heads-up that https://github.com/kubernetes-sigs/json is now available to use (thanks @liggitt!!)

It satisfies the above criteria as it:

Is case-sensitive
Preserves numbers
Collects/returns strict errors completely independently (more like "warnings") from the "normal" decode errors
Allows reporting duplicate field / unknown field policy invalidations
Supports returning the invalid byte offset for syntax errors; which allows a higher-level library (maybe like this one to report the surrounding context)

Meanwhile, k8s has moved on to using json.Marshal (kubernetes/kubernetes#105466), not json-iter, to fix the json.RawMessage issue described. That is just a note, no action item needed, as this library already uses json.Marshal, good that we now have consistency.

@inteon would you be up for rebasing this PR to use this new json library?

ncdc · 2021-10-21T11:51:56Z

/uncc

inteon · 2021-10-21T13:18:04Z

@luxas I updated the PR, so it now uses the new json library!
Furthermore, I changed the API to match the json library API, eg. added an option to disallow duplicate fields (both DisallowUnknownFields and DisallowDuplicateFields should be supported).
Please let me know what you think, and feel free to suggest changes or add some extra testcases.

liggitt

there's a lot of changes going on here that make this a bit hard to review as a piece... is it possible to do each of the following independently:

use sigs.k8s.io/json
switch to yaml.v3
make additions to the surface area exposed by this library to add new features

unmarshal.go

go.mod

marshal.go

inteon · 2021-10-23T10:17:59Z

@liggitt thanks for the first review, I made some additional changes based on your feedback. Next step will be to split this PR into multiple smaller PRs. Feel free to provide additional feedback. I'll let you know once I've split this PR into multiple smaller PRs, but this might take some time as I'm quite busy right now.

luxas

Thanks a lot @inteon for your great work 🥇 💯!

This is not a very straightforward change; it needs to be reasoned about quite deeply. I have added some comments on areas where we should still decide a way forward, but it probably requires some kind of broader SIG API Machinery and kyaml review and approval.

I don't really know if I've got the time to write a KEP around this right now, but it'd be great to formalize these things. Meanwhile, maybe we can discuss where we want to go in this PR?

marshal.go

unmarshal.go

.github/workflows/go.yml

marshal.go

luxas · 2021-10-25T13:41:40Z

cc @laverya @fejta @sttts @neolit123 @dims from #26.

As pointed out by @inteon in #26 (comment):

gnostic now also uses yaml.v3: google/gnostic#194

it hasn't been released yet, however, I guess it'll come in a 0.6.x

inteon · 2021-11-03T20:51:12Z

@luxas thanks for the review, I tried to fix all of your comments & rebased the PR on #65.
Please let me know if there is still something that should be changed.

luxas

Thanks for rebasing and upgrading! This starts looking really good 🤩

btw, was there a reason for dropping earlier versions of Go in the CI?

yaml.go

luxas · 2021-11-05T11:52:50Z

yaml.go

+	if jsonTarget.Kind() != reflect.Ptr || jsonTarget.IsNil() {
+		return nil, fmt.Errorf("provided object is not a valid pointer")
+	}


Was the remove / addition of this code snippet intended?

adding this check should be a solution for #30

yaml.go

luxas · 2021-11-05T11:57:10Z

yaml.go

@@ -336,7 +348,7 @@ func convertToJSONableObject(yamlObj interface{}, jsonTarget *reflect.Value) (in
 			case int64:
 				s = strconv.FormatInt(typedVal, 10)
 			case float64:
-				s = strconv.FormatFloat(typedVal, 'g', -1, 32)


does this need to merge to the current master branch too?

There was a reference to float32 vs float64 (go-yaml/yaml#83) in the original code.

yaml.go

luxas · 2021-11-05T12:16:30Z

yaml_test.go

 	"strconv"
 	"testing"
-
-	"github.com/davecgh/go-spew/spew"
-	yaml "gopkg.in/yaml.v2"
 )

 /* Test helper functions */


In the JSONToYAML and Marshal tests I would expect the sequence indentation style to have changed to wide instead of compact, due to an upstream change. Is that true? As discussed earlier, kustomize carries a fork of go-yaml for that reason, to provide customizability.

That upstream change blocks migration to yaml.v3. We don't want any externally visible change in serialization behavior.

Thanks for the information, I'll probably create a fork of go-yaml in this repo too & create an upstream PR (if no PR is present yet).

I don't think we're open to forking the yaml repo at this point. The kustomize fork is intended to be temporary and they are pursuing options to bring the V3 library back into a compatible state

the fixes kustomize needs to land in yaml.v3 to restore compatibility are either in go-yaml/yaml#756 or go-yaml/yaml#753

Ok, but can't we add a temporary fork here too? Seems like the kyaml team has been trying for a long time to get this option added. Would be great if we could not block this PR until upstream has fixed the issue.

no, it's not urgent enough to move off yaml.v2 to take on the added uncertainty / maintenance cost of a fork

we could use yaml.v3 for unmarshalling and yaml.v2 for marshalling; that would fix all problems, only downside is having 2 deps

… fields.go & update interface

inteon · 2021-11-05T17:36:09Z

Thanks for the review; looks like we're getting close to the "ideal" solution.
I think the biggest challenge that is still pending is the "sequence indentation style" and planning how to get this merged (maybe in different stages).

PS: the reason I removed the tests for go-versions below 1.17, is because sigs.k8s.io/json only supports that version

k8s-ci-robot · 2021-12-08T23:30:47Z

@inteon: PR needs rebase.

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/test-infra repository.

k8s-triage-robot · 2022-03-09T00:07:26Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

inteon · 2022-03-09T09:48:17Z

/remove-lifecycle stale

willfindlay · 2022-05-20T21:39:38Z

Any updates on this? My team needs a fix for #58 :(

dims · 2022-08-09T17:53:20Z

@inteon doesn't look like there is a possibility of this landing given the time and the situation has not changed ( #61 (comment) )

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 11, 2021

k8s-ci-robot requested review from lavalamp and ncdc September 11, 2021 00:25

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 11, 2021

luxas mentioned this pull request Sep 16, 2021

wrong line number in yaml syntax error message kubernetes/kubernetes#104607

Open

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2021

luxas mentioned this pull request Sep 23, 2021

add Encoder and Decoder #59

Closed

KnVerey mentioned this pull request Sep 29, 2021

yaml.v3 fork maintenance story kubernetes-sigs/kustomize#4033

Closed

k8s-ci-robot removed the request for review from ncdc October 21, 2021 11:51

inteon force-pushed the master branch from 6f514ac to 6ef1584 Compare October 21, 2021 13:11

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 21, 2021

liggitt reviewed Oct 21, 2021

View reviewed changes

unmarshal.go Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

marshal.go Outdated Show resolved Hide resolved

luxas reviewed Oct 25, 2021

View reviewed changes

This was referenced Oct 26, 2021

add support for strict errors go-yaml/yaml#796

Closed

Non functional improvements #65

Merged

inteon force-pushed the master branch from a4ea27a to 546762b Compare November 3, 2021 20:14

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 3, 2021

inteon force-pushed the master branch 2 times, most recently from 84a7f62 to 34069b9 Compare November 3, 2021 20:48

inteon force-pushed the master branch from 34069b9 to 6199427 Compare November 4, 2021 15:59

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 4, 2021

inteon requested a review from luxas November 4, 2021 16:03

luxas reviewed Nov 5, 2021

View reviewed changes

switch to 'sigs.k8s.io/json' and 'gopkg.in/yaml.v3' & cleanup/ update…

efe7d3f

… fields.go & update interface

inteon force-pushed the master branch from 6199427 to efe7d3f Compare November 5, 2021 17:31

tomasaschan mentioned this pull request Nov 15, 2021

Long strings are marshalled to invalid YAML; incorrect line breaks inserted #68

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2021

natasha41575 mentioned this pull request Dec 30, 2021

Preserve comments kubernetes-sigs/kustomize#4353

Closed

carlisia mentioned this pull request Feb 15, 2022

Upgrade to sigs.k8s.io/yaml v1.4 knative-extensions/net-contour#725

Closed

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 9, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 9, 2022

liggitt mentioned this pull request Jun 17, 2022

Update to gopkg.in/yaml.v3 #80

Closed

achrefbensaad mentioned this pull request Jul 18, 2022

add in cluster tests + fix synk vuln SNYK-CC-K8S-8 kubearmor/KubeArmor#762

Open

dims closed this Aug 9, 2022

scnewma mentioned this pull request Mar 15, 2023

YAML on/off as keys cannot be queried correctly open-policy-agent/opa#5754

Open

Upgrade yaml.v2 to yaml.v3, improve testing and bugfixes #61

Upgrade yaml.v2 to yaml.v3, improve testing and bugfixes #61

Conversation

inteon commented Sep 11, 2021 • edited Loading

k8s-ci-robot commented Sep 11, 2021

k8s-ci-robot commented Sep 11, 2021

k8s-ci-robot commented Sep 11, 2021

lavalamp commented Sep 15, 2021

luxas commented Sep 16, 2021 • edited Loading

liggitt commented Sep 16, 2021

luxas commented Sep 16, 2021

liggitt commented Sep 16, 2021

luxas commented Oct 21, 2021

ncdc commented Oct 21, 2021

inteon commented Oct 21, 2021

liggitt left a comment

Choose a reason for hiding this comment

inteon commented Oct 23, 2021 • edited Loading

luxas left a comment

Choose a reason for hiding this comment

luxas commented Oct 25, 2021

inteon commented Nov 3, 2021 • edited Loading

luxas left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inteon Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inteon Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

inteon commented Nov 5, 2021

k8s-ci-robot commented Dec 8, 2021

k8s-triage-robot commented Mar 9, 2022

inteon commented Mar 9, 2022

willfindlay commented May 20, 2022

dims commented Aug 9, 2022

inteon commented Sep 11, 2021 •

edited

Loading

luxas commented Sep 16, 2021 •

edited

Loading

inteon commented Oct 23, 2021 •

edited

Loading

inteon commented Nov 3, 2021 •

edited

Loading

luxas left a comment •

edited

Loading

inteon Nov 5, 2021 •

edited

Loading

inteon Nov 5, 2021 •

edited

Loading