Skip to content

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

@zach593

Description

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)
}

image

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:

Metadata

Assignees

Labels

kind/bugCategorizes issue or PR as related to a bug.

Type

No type

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions