MergeFrom can return invalid patch data when using int64 properties

[jfremy]

I don’t believe this is a known issue (at least, did not find mention of it) so I wanted to raise it.
This bug impacts uses of client.MergeFrom but also helper function like CreateOrPatch (which uses MergeFrom).

Summary: If you have an object (K8s or CRD) that has int64 properties, the MergeFrom call will very likely generate invalid patch data if the values considered cannot be exactly encoded in a float64.

The reason is that the jsonpatch library used to generate the patch is unaware of the underlying type and therefore uses “standard” json marshaling/unmarshaling. So when unmarshaling numbers (int/float), they are stored as float64.
If the number cannot be exactly encoded into a float64, we start seeing incorrect behaviors

The current flow to generate a patch is

• [controller-runtime] generate JSON []byte representation of both objects - marshaling is correctly based on type
• [jsonpatch] unmarshal each JSON []byte to map[string]interface{} - unmarshaling can yield loss in precision on numbers / truncated int
• [jsonpatch] process the values and evaluate whether there is a change - decision is based on invalid values
• [jsonpatch] marshal patch JSON - marshaling will store value with precision loss

There are two kinds of impact:

• store the wrong value in an object (you end up storing the value with loss of precision)
• do no detect a change between the two objects and omit the change in the patch.

I don’t consider it a bug in jsonpatch. The library is following the JSON standard

Looking at the issue, I think that fixing it would require to switch to another json patch generation implementation that is aware of the underlying data types but that’s probably a big change.

I’m including a simple test program to illustrate the issue - MergeFrom generates an empty patch despite the change (I’ve used a Job because it was an existing object with an int64 property).

package main

import (
	v1 "k8s.io/api/batch/v1"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
	"sigs.k8s.io/controller-runtime/pkg/client"
	"slices"
	"testing"
)

func TestPatch(t *testing.T) {
	var largeInt64 int64 = 9223372036854775807
	var similarLargeInt64 int64 = 9223372036854775800
	var differentEnoughInt64 int64 = 8223372036854775807
	j := v1.Job{
		ObjectMeta: metav1.ObjectMeta{
			Namespace: "test",
			Name:      "test",
		},
		Spec: v1.JobSpec{
			ActiveDeadlineSeconds: &largeInt64,
		},
	}
	patch := client.MergeFrom(j.DeepCopy())

	j.Spec.ActiveDeadlineSeconds = &differentEnoughInt64
	data, err := patch.Data(&j)
	if err != nil {
		t.Fatalf("patch returned an error: %s", err)
	}
	if slices.Equal(data, []byte("{}")) {
		t.Fatalf("patch returned an empty patch: %s", string(data))
	}

	j.Spec.ActiveDeadlineSeconds = &similarLargeInt64

	data, err = patch.Data(&j)
	if err != nil {
		t.Fatalf("patch returned an error: %s", err)
	}
	if slices.Equal(data, []byte("{}")) {
                // It will fail here because the two int64 values are too close and get truncated to the same value
		t.Fatalf("patch returned an empty patch: %s", string(data))
	}
}

[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

[alvaroaleman]

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