fix: fix GenerateBindingReferenceKey bug

[JarHMJ]

Signed-off-by: huangminjie minjie.huang@daocloud.io

What type of PR is this?
/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

karmada-controller-manager: fixed the issue of generating binding reference key.

[RainbowMango]

cc @XiShanYongYe-Chang
/priority important-soon

We should check if this bugfix would introduce in-compatible effects. That is more important than the bug.

[XiShanYongYe-Chang]

Ok, I will check it.

[XiShanYongYe-Chang]

I'm sorry, It's my fault.

To prevent version incompatibility when users upgrade Karmada from v0.10.0 to a later version (for example, v.0.11.0), the current fix maybe not be enough. We need to introduce a new group of labels:

ResourceBindingReferenceHashKey = "resourcebinding.karmada.io/hash-key"
ClusterResourceBindingReferenceHashKey = "clusterresourcebinding.karmada.io/hash-key"

to replace the current group of labels:

ResourceBindingReferenceKey = "resourcebinding.karmada.io/key"
ClusterResourceBindingReferenceKey = "clusterresourcebinding.karmada.io/key"

The current group of labels can be removed in the next version.

The two groups of labels both need to be processed, this may be complex. For example, a user creates the nginx deployment in the test namespace with v0.10.0 version. In v0.11.0 version, user wants to delete it, so we need to process both ResourceBindingReferenceKey and ResourceBindingReferenceHashKey labels.

[XiShanYongYe-Chang]

@JarHMJ , Would you like to fix it in this way?

[JarHMJ]

@XiShanYongYe-Chang Ok，I see.

You means that the label ResourceBindingReferenceKey is not precise，so you want use ResourceBindingReferenceHashKey to replace the old label. Right?

I would like to do this feature.

I think this pr should be merged as hotfix, because this leads to some error when deployment has same name in diffrenet namespce.

[XiShanYongYe-Chang]

You means that the label ResourceBindingReferenceKey is not precise

I don't mean that the label isn't precise enough. I mean we need another group of labels to indicate correct key.

I think this pr should be merged as hotfix, because this leads to some error when deployment has same name in diffrenet namespce.

It's really a bug. I want the old one to stay the same, use a new method to generate the correct key, and then use the new label to replace the old one. For example:

diff --git a/pkg/controllers/binding/common.go b/pkg/controllers/binding/common.go
index a7131bcd..52251ae5 100644
--- a/pkg/controllers/binding/common.go
+++ b/pkg/controllers/binding/common.go
@@ -193,8 +193,8 @@ func mergeLabel(workload *unstructured.Unstructured, workNamespace string, bindi
        util.MergeLabel(workload, workv1alpha2.WorkNamespaceLabel, workNamespace)
        util.MergeLabel(workload, workv1alpha2.WorkNameLabel, names.GenerateWorkName(workload.GetKind(), workload.GetName(), workload.GetNamespace()))
        if scope == apiextensionsv1.NamespaceScoped {
-               util.MergeLabel(workload, workv1alpha2.ResourceBindingReferenceKey, names.GenerateBindingReferenceKey(binding.GetNamespace(), binding.GetName()))
-               workLabel[workv1alpha2.ResourceBindingReferenceKey] = names.GenerateBindingReferenceKey(binding.GetNamespace(), binding.GetName())
+               util.MergeLabel(workload, workv1alpha2.ResourceBindingReferenceHashKey, names.GenerateBindingReferenceHashKey(binding.GetNamespace(), binding.GetName()))
+               workLabel[workv1alpha2.ResourceBindingReferenceHashKey] = names.GenerateBindingReferenceHashKey(binding.GetNamespace(), binding.GetName())
        } else {
                util.MergeLabel(workload, workv1alpha2.ClusterResourceBindingReferenceKey, names.GenerateBindingReferenceKey("", binding.GetName()))
                workLabel[workv1alpha2.ClusterResourceBindingReferenceKey] = names.GenerateBindingReferenceKey("", binding.GetName())
diff --git a/pkg/util/names/names.go b/pkg/util/names/names.go
index 01d5fe13..2511e69b 100644
--- a/pkg/util/names/names.go
+++ b/pkg/util/names/names.go
@@ -71,6 +71,19 @@ func GenerateBindingReferenceKey(namespace, name string) string {
        return rand.SafeEncodeString(fmt.Sprint(hash.Sum32()))
 }

+// GenerateBindingReferenceHashKey will generate the hash key of binding object with the hash of its namespace and name.
+func GenerateBindingReferenceHashKey(namespace, name string) string {
+       var bindingName string
+       if len(namespace) != 0 {
+               bindingName = namespace + "-" + name
+       } else {
+               bindingName = name
+       }
+       hash := fnv.New32a()
+       hashutil.DeepHashObject(hash, bindingName)
+       return rand.SafeEncodeString(fmt.Sprint(hash.Sum32()))
+}

In the next version, we can delete the old logic, so the upgrade can be not affected.

[JarHMJ]

@XiShanYongYe-Chang 👌I will try to fix in your way

[XiShanYongYe-Chang]

@JarHMJ Thanks very much.

[JarHMJ]

@XiShanYongYe-Chang done.

[JarHMJ]

@XiShanYongYe-Chang Should I clean up the old code?

	// TODO: Delete this logic in the next release to prevent incompatibility when upgrading the current release (v0.10.0).
	workList, err := GetWorksByLabelSelector(c, labels.SelectorFromSet(
		labels.Set{
			workv1alpha2.ClusterResourceBindingLabel: binding.Name,
		},
	))
	if err != nil {
		klog.Errorf("Failed to get works by ClusterResourceBinding(%s): %v", binding.Name, err)
		return err
	}
	targetWorks = append(targetWorks, workList.Items...)

But #1006 has done it。

[XiShanYongYe-Chang]

@XiShanYongYe-Chang done.

Thanks, I will review it ASAP.

Should I clean up the old code?

This pr can don't care about it.

[RainbowMango]

I'm thinking about cutting a new bugfix release(v0.10.1) to solve this issue.
Let's hold on for a while.

[pigletfly]

I think we could provide a way for users to fix current rb ReferenceKey, so it can avoid deal the backward compatibility in the controller. steps as follow:

1. stop karmada-controller-manager
2. run the fix script to update all rb ReferenceKey into correct value
3. update karmada-controller-manager to the correct version, which has fixed GenerateBindingReferenceKey func

[RainbowMango]

Hi @pigletfly
I suggest fixing this issue first and then cutting a new release(v0.10.1).
People who are using v0.9.0 can upgrade to v1.10.1 (skip v0.10.0).

How do you think?

[RainbowMango]

I'd like to take the patch from this PR and take the unit test from #1013.

[RainbowMango]

@JarHMJ Can you help revert the patch to the first version? something like:

diff --git a/pkg/util/names/names.go b/pkg/util/names/names.go
index 01d5fe13..98a0fabc 100644
--- a/pkg/util/names/names.go
+++ b/pkg/util/names/names.go
@@ -61,7 +61,7 @@ func GenerateBindingName(kind, name string) string {
 // GenerateBindingReferenceKey will generate the key of binding object with the hash of its namespace and name.
 func GenerateBindingReferenceKey(namespace, name string) string {
        var bindingName string
-       if len(namespace) == 0 {
+       if len(namespace) > 0 {
                bindingName = namespace + "-" + name
        } else {
                bindingName = name

[JarHMJ]

@RainbowMango done

[RainbowMango]

Thanks for your quick response.
Let's see if @pigletfly has other comments.

[pigletfly]

Hi @pigletfly I suggest fixing this issue first and then cutting a new release(v0.10.1). People who are using v0.9.0 can upgrade to v1.10.1 (skip v0.10.0).

How do you think?

agreed

[RainbowMango]

OK. Let's do it.
/lgtm
/approve

[karmada-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [RainbowMango]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment