MergeFrom can return invalid patch data when using int64 properties about controller-runtime

[alvaroaleman]

Created evanphx/json-patch#189 to ask about the possibilities and stance there

[alvaroaleman]

/kind bug

That's not good. The actual patch generation is here:

controller-runtime/pkg/client/patch.go

Line 136 in 2154ffb

return jsonpatch.CreateMergePatch(originalJSON, modifiedJSON)

which calls into this lib: https://github.com/evanphx/json-patch

I would assume the root of the problem is that the int64 ends up getting converted to a float64 and the float64 can not represent the difference. We can try to open an issue in the json-patch repo to see if this can be fixed.

[jfremy]

/kind bug

That's not good. The actual patch generation is here:

controller-runtime/pkg/client/patch.go

Line 136 in 2154ffb

return jsonpatch.CreateMergePatch(originalJSON, modifiedJSON)

which calls into this lib: https://github.com/evanphx/json-patch
I would assume the root of the problem is that the int64 ends up getting converted to a float64 and the float64 can not represent the difference. We can try to open an issue in the json-patch repo to see if this can be fixed.

I filled the bug here because I’m not sure this can be really considered a bug in json-patch.
There is no integer type in the JSON format, just numbers. RFC-8259 says that implementations can set limits on precision and cite float64 as a good compromise.
Since json-patch does not have any type info, it unmarshals to map[string]interface{} which means that the golang json unmarshaler uses the “default” behavior (number -> float64). Which is how we end up with this.

[alvaroaleman]

@jfremy actually on second thought I am unsure if this is solveable at all. Because at the end of the day, patch or not, this will get serialized to json and sent to the apiserver. At that point, the golang json (de-)serializer will end up truncating portions of large int64s. So the only ways on how this could be fixed that I can think of are:

1. Use a different marshaller in the k8s apiserver and here that doesn't always use float64s to represent numbers
2. Change the stdlib marshallers behavior to not always use float64 for numbers
3. Change the job api to represent this as a string

All of these do not seem very practical unfortunately. Do you have other ideas? And out of curiosity, in what context did you run into this?

It might be worth reporting this in kubernetes/kubernetes btw, as this issue doesn't seem specific to controller-runtime.

[jfremy]

I have a CRD where I store a "seed" in an int64 (so I can end up with values in the whole int64 range).
I had an API server update call that was working fine and when I switched to patch (using MergeFrom), it stopped working.
That's how I ended up discovering the issue.

Regarding the scope of the issue, I think it is limited to MergeFrom / controller-runtime and does not impact the API server:

• golang json will do the "right" thing when marshaling/unmarshaling to/from typed objects
• Kubernetes seems to have an overloaded json decoder that when decoding json numbers tries to decode as int64 first and float64 if it did not succeed (https://github.com/kubernetes/apimachinery/blob/master/pkg/util/json/json.go).

I've seen the API server handle updates with large int64 correctly so that part seems to work.
I'll look into the support for json patch and see if it works too to confirm - that's easy to test

Using the apimachinery json decoder in the jsonpatch library would likely fix the issue - but I do not believe json-patch supports setting a custom json decoder

[alvaroaleman]

Yeah you are right, it actually works correctly in the golang json marshaller, seems I didn't look correctly yesterday.

But that just brings us back to square zero, we need a fix in json-patch or a different patch lib.

[jfremy]

Unfortunately, I agree. I don't see a way to fix it that does not imply modifying json-patch or switching to a different library that does what we want