deepequal check in helper.CreateOrUpdateWork() always return false, because resource.MarshalJSON() will add \n at the end of JSON

[zach593]

What happened:

deepequal check in helper.CreateOrUpdateWork() always return false, because resource.MarshalJSON() will add \n at the end of JSON, but the workloads read from work.Spec.Workload.Manifests never have this \n. So everytime this function is called, an update operation will occur.

Fortunately, the same thing doesn't happen to work-status-controller, rb/crb-status-controller. Although they are both using DeepEqual() on runtime.RawExtension, but the byte slices are obtained from resource interpret webhook.

What you expected to happen:

remove unecessary update operations.

How to reproduce it (as minimally and precisely as possible):

func Test(t *testing.T) {
	mgr, err := ctrl.NewManager(restConf, ctrl.Options{
		Scheme: gclient.NewSchema(),
	})
	if err != nil {
		panic(err)
	}
	go mgr.GetCache().Start(context.TODO())
	time.Sleep(time.Second)

	work := &workv1alpha1.Work{}
	err = mgr.GetClient().Get(context.TODO(), client.ObjectKey{Namespace: "example-ns", Name: "example-name"}, work)
	if err != nil {
		panic(err)
	}
	fmt.Println(work.Spec.Workload.Manifests[0].Raw)

	workload := &unstructured.Unstructured{}
	_ = json.Unmarshal(work.Spec.Workload.Manifests[0].Raw, workload)

	j0, _ := json.Marshal(workload)
	fmt.Println(j0)

	j1, _ := workload.MarshalJSON()
	fmt.Println(j1)
}

Anything else we need to know?:

@snowplayfire found this, I'm just providing some possible fixes here. Many thanks to her, I sincerely hope she has a happy holiday.

This is the root cause why binding controller can’t use DeepEqual() to skip the update operation when calling the ensureWork(). But for deepequal to work, we need another PR to copy part of the karmada webhook logic to the update operation, which I will complete as soon as possible.

For fixing this issue, one way is just need one line: replace resource.MarshalJSON() with json.Marshal(resource), it could work. #5939

But deepequal checks should be used in semantic scenarios, not to judge byte streams. Therefore, if we want to use semantic deepequal checking to fix it, we need to directly judge the unmarshaled workload read from work. #5940

Environment:

• Karmada version: any
• kubectl-karmada or karmadactl version (the result of kubectl-karmada version or karmadactl version):
• Others:

[zhzhuang-zju]

thanks @zach593 @snowplayfire
Follow the test file you provided, I have also reproduced this issue. workload.MarshalJSON() compared to json.Marshal does add \n to the end. And the evidence of this can be found in the code:

https://github.com/golang/go/blob/80a2982a801eaedc416d59801ac8fefcf1e4acf5/src/encoding/json/stream.go#L214-L221

[XiShanYongYe-Chang]

Thank you very much, it was a great find.

I still have a doubt. Before the work is updated, the manifest file ends with 10. However, why does the manifest in the work geted by using the client not end with 10? Where is the 10 digested?

[XiShanYongYe-Chang]

/assign @zach593

[zach593]

I still have a doubt. Before the work is updated, the manifest file ends with 10. However, why does the manifest in the work geted by using the client not end with 10? Where is the 10 digested?

When the object is passed to the kube-apiserver, the entire object will be unmarshaled as unstructured.Unstructured, the runtime.RawExtension field will be parsed as map[string]any, and that \n will be omitted.

[XiShanYongYe-Chang]

@zach593 Thank you for your answer.

[XiShanYongYe-Chang]

Fortunately, the same thing doesn't happen to work-status-controller, rb/crb-status-controller. Although they are both using DeepEqual() on runtime.RawExtension, but the byte slices are obtained from resource interpret webhook.

Hi @zach593, can you help point out the location of the decision logic code for these controllers?

[XiShanYongYe-Chang]

But for deepequal to work, we need another PR to copy part of the karmada webhook logic to the update operation, which I will complete as soon as possible.

I don't understand this clearly. Can you explain it a little bit more, maybe we can track it with a new issue?

[zach593]

I don't understand this clearly. Can you explain it a little bit more, maybe we can track it with a new issue?

already got one: #6017

[RainbowMango]

@snowplayfire found this, I'm just providing some possible fixes here. Many thanks to her, I sincerely hope she has a happy holiday.

Thanks to both of you. @zach593 and @snowplayfire.

[zach593]

Hi @zach593, can you help point out the location of the decision logic code for these controllers?

@XiShanYongYe-Chang

In work status controller, the location is

karmada/pkg/controllers/status/work_status_controller.go

Line 360 in 8313813

statusRaw, err := c.ResourceInterpreter.ReflectStatus(clusterObj)

If the DefaultInterpreter is used, it will eventally call

karmada/pkg/util/helper/workstatus.go

Lines 300 to 310 in 8313813

	func BuildStatusRawExtension(status interface{}) (*runtime.RawExtension, error) {
	statusJSON, err := json.Marshal(status)
	if err != nil {
	klog.Errorf("Failed to marshal status. Error: %v.", statusJSON)
	return nil, err
	}
	return &runtime.RawExtension{
	Raw: statusJSON,
	}, nil
	}

If other interpreters are implemented and enabled, then obviously this will depend on how the interpreter is implemented.

---

As for rb/crb status controller,

karmada/pkg/controllers/status/rb_status_controller.go

Lines 114 to 128 in 8313813

	func (c *RBStatusController) syncBindingStatus(ctx context.Context, binding *workv1alpha2.ResourceBinding) error {
	err := helper.AggregateResourceBindingWorkStatus(ctx, c.Client, binding, c.EventRecorder)
	if err != nil {
	klog.Errorf("Failed to aggregate workStatus to resourceBinding(%s/%s), Error: %v",
	binding.Namespace, binding.Name, err)
	return err
	}
	err = updateResourceStatus(ctx, c.DynamicClient, c.RESTMapper, c.ResourceInterpreter, c.EventRecorder, binding.Spec.Resource, binding.Status)
	if err != nil {
	return err
	}
	return nil
	}

The helper.AggregateResourceBindingWorkStatus() just passes the runtime.RawExtension from work to binding and don't know what's inside.

And the updateResourceStatus() part does not marshal unstructured.Unstructured to JSON and is irrelevant.