Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove use of unsafe json-iterator / reflect2 libraries #13434

Closed
liggitt opened this issue Oct 22, 2021 · 2 comments · Fixed by #13439
Closed

Remove use of unsafe json-iterator / reflect2 libraries #13434

liggitt opened this issue Oct 22, 2021 · 2 comments · Fixed by #13439

Comments

@liggitt
Copy link
Contributor

liggitt commented Oct 22, 2021

The etcd client currently depends on json-iterator via the client/v2 package:

etcd/client/v2/json.go

Lines 60 to 73 in ef1f71a

// caseSensitiveJsonIterator returns a jsoniterator API that's configured to be
// case-sensitive when unmarshalling, and otherwise compatible with
// the encoding/json standard library.
func caseSensitiveJsonIterator() jsoniter.API {
config := jsoniter.Config{
EscapeHTML: true,
SortMapKeys: true,
ValidateJsonRawMessage: true,
CaseSensitive: true,
}.Froze()
// Force jsoniter to decode number to interface{} via int64/float64, if possible.
config.RegisterExtension(&customNumberExtension{})
return config
}

This is still relied on by portions of the v3 libraries:

"go.etcd.io/etcd/client/v2"

The approach taken by json-iterator and its reflect library is not stable (it pins to internal details of the stdlib reflect package), and requires maintenance per go release, which does not appear to be sustainable (golang/go#48238 (comment))

In addition to the maintenance aspect, it is hard to reason about the safety of the implementation (golang/go#48238 (comment))

Kubernetes has transitioned away from json-iterator and is using sigs.k8s.io/json - a library based on the stdlib json decoder that added in case-sensitivity and int-preserving behavior.

@hexfusion
Copy link
Contributor

@liggitt thanks, I appreciate the detailed issue.

/assign

@hexfusion hexfusion assigned hexfusion and unassigned hexfusion Oct 22, 2021
@hexfusion
Copy link
Contributor

@chaochn47 PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants