binding reference key hash collision

[likakuli]

What happened:

work in different namespace with same binding reference key will affect each other which will result to the related work to be deleted and created continuously.

What you expected to happen:

work with same binding reference key NOT affect each other.

How to reproduce it (as minimally and precisely as possible): create two rb: rb1: resourcebinding.karmada.io/name: test16-4-deployment resourcebinding.karmada.io/namespace: ecp-stress-test-15870

rb2: esourcebinding.karmada.io/name: test23-3-configmap resourcebinding.karmada.io/namespace: ecp-stress-test-22982

check whether the works created by the two rb run as expected.

Anything else we need to know?: The root cause is that rb use ResourceBindingReferenceKey label which is generated by calculate a hash value using rb's namespace and name to find its related work. But the hash may collision, so if there are two rb and two work in two namespaces and the hash is same, then the binding controller will treat works created not by itself as orphan works and delete them.

https://github.com/karmada-io/karmada/blob/ee72948ead6929c18fa90e40d2031ca745a789d9/pkg/controllers/binding/binding_controller.go#L97-L114

https://github.com/karmada-io/karmada/blob/ee72948ead6929c18fa90e40d2031ca745a789d9/pkg/util/helper/binding.go#L120-L122

Environment:

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

[RainbowMango]

create two rb: rb1: resourcebinding.karmada.io/name: test16-4-deployment resourcebinding.karmada.io/namespace: ecp-stress-test-15870

rb2: esourcebinding.karmada.io/name: test23-3-configmap resourcebinding.karmada.io/namespace: ecp-stress-test-22982

Do you mean rb1(ecp-stress-test-15870/test16-4-deployment) and rb2(ecp-stress-test-22982/test23-3-configmap) will share same hash value? By GenerateBindingReferenceKey.

[RainbowMango]

Answer by myself: I added two tests as follows and reproduced it:

diff --git a/pkg/util/names/names_test.go b/pkg/util/names/names_test.go
index 5ccab0d5..a8fb243a 100644                                          
--- a/pkg/util/names/names_test.go                                       
+++ b/pkg/util/names/names_test.go                                       
@@ -102,6 +102,14 @@ func TestGenerateBindingReferenceKey(t *testing.T) {
                        name:      "mytest-deployment",                  
                        namespace: "",                                   
                },                                                       
+               {                                                        
+                       name:      "test16-4-deployment",
+                       namespace: "ecp-stress-test-15870",
+               },
+               {
+                       name:      "test23-3-configmap",
+                       namespace: "ecp-stress-test-22982",
+               },
        }
        result := map[string]struct{}{}
        for _, tt := range tests {

go test .\pkg\util\names\ -run=GenerateBindingReferenceKey
--- FAIL: TestGenerateBindingReferenceKey (0.00s)
    --- FAIL: TestGenerateBindingReferenceKey/test23-3-configmap (0.00s)
        names_test.go:119: duplicate key found 665ddd4546
FAIL
FAIL    github.com/karmada-io/karmada/pkg/util/names    0.333s

[likakuli]

We resolve it temporarily by adding rb's namespace and name info to work's label and update all places that use hash to get work.

[RainbowMango]

:) Actually, in the previous version we also use RB's namespace and name to label Work, but the length of label is restricted to <=64 that the binding's name might exceed. That's exactly the reason why we tend to hash.

[RainbowMango]

I've no idea how to solve it yet. cc guys who might be interested in it. @Garrybest @halfrost

[likakuli]

can use RB's UID as unique key add to Work's label?

[RainbowMango]

can use RB's UID as unique key add to Work's label?

That's an option I guess. Before making the decision, I wonder to know :

• What's the probability of conflict in the current situation?
• Does hash.Hash64 will be a relief than hash.Hash32?

[likakuli]

Or we can get work by hash label first and then filter it by its binding info in annotation

[Garrybest]

Emmm, I didn't expect we have met a coincidence here. Kubernetes also uses hash32 to generate podTemplate Hash or pod name Hash.

[Garrybest]

The origin namespace and name could be added into annotations, this field does not have a length restriction.

[RainbowMango]

The origin namespace and name could be added into annotations, this field does not have a length restriction.

The namespace and name are already in Work annotations, but we didn't use them. https://github.com/karmada-io/karmada/blob/4a243f447aa253ab7ec9e27d8830469c3ae0bdb1/pkg/controllers/binding/common.go#L203-L204

[RainbowMango]

Or we can get work by hash label first and then filter it by its binding info in annotation

I agree with it if you mean the resourcebinding.karmada.io/name and resourcebinding.karmada.io/namespace in Work object, like

spec:
  workload:
    manifests:
    - apiVersion: apps/v1
      kind: Deployment
      metadata:
        annotations:
          ...
          resourcebinding.karmada.io/name: nginx-deployment
          resourcebinding.karmada.io/namespace: default
          ...

[chaunceyjiang]

If use annotation, is it easy for existing users to upgrade?

[RainbowMango]

If use annotation, is it easy for existing users to upgrade?

Good thinking! No compatibility issue here, as we won't change the hash value and just adds the filter to avoid the collision.

[Garrybest]

Or we can get work by hash label first and then filter it by its binding info in annotation

I agree with it if you mean the resourcebinding.karmada.io/name and resourcebinding.karmada.io/namespace in Work object, like

spec:
  workload:
    manifests:
    - apiVersion: apps/v1
      kind: Deployment
      metadata:
        annotations:
          ...
          resourcebinding.karmada.io/name: nginx-deployment
          resourcebinding.karmada.io/namespace: default
          ...

+1, double check could fix this issue.

[chaunceyjiang]

If use annotation, is it easy for existing users to upgrade?

Good thinking! No compatibility issue here, as we won't change the hash value and just adds the filter to avoid the collision.

Got!

Can I try to fix this issue?

[chaunceyjiang]

apiVersion: work.karmada.io/v1alpha1
kind: Work
metadata:
  annotations:
    resourcebinding.karmada.io/name: local-hostpath-pvc-persistentvolumeclaim
    resourcebinding.karmada.io/namespace: default
  creationTimestamp: "2022-06-28T02:25:18Z"
  finalizers:
  - karmada.io/execution-controller
  generation: 1
  labels:
    resourcebinding.karmada.io/key: 6997fb46d8
  name: local-hostpath-pvc-6997fb46d8
  namespace: karmada-es-k8s-node3
  resourceVersion: "802232"
  uid: 52acc10f-6e45-410d-8936-debcebcc011e
spec:
  workload:
    manifests:
    - apiVersion: v1
      kind: PersistentVolumeClaim
      metadata:
        annotations:
          kubectl.kubernetes.io/last-applied-configuration: |
            {"apiVersion":"v1","kind":"PersistentVolumeClaim","metadata":{"annotations":{},"name":"local-hostpath-pvc","namespace":"default"},"spec":{"accessModes":["ReadWriteOnce"],"resources":{"requests":{"storage":"5G"}},"storageClassName":"openebs-hostpath"}}
          resourcebinding.karmada.io/name: local-hostpath-pvc-persistentvolumeclaim
          resourcebinding.karmada.io/namespace: default
          resourcetemplate.karmada.io/uid: b80314a1-b109-4e58-a77f-ae2e7a267b6a
        labels:
          propagationpolicy.karmada.io/name: nginx-propagation
          propagationpolicy.karmada.io/namespace: default
          resourcebinding.karmada.io/key: 6997fb46d8
          work.karmada.io/name: local-hostpath-pvc-6997fb46d8
          work.karmada.io/namespace: karmada-es-k8s-node3
        name: local-hostpath-pvc
        namespace: default
      spec:
        accessModes:
        - ReadWriteOnce
        resources:
          requests:
            storage: 5G
        storageClassName: openebs-hostpath

@RainbowMango Wait a minute. I think you mean is to use .spec.workload.manifests[*].metadata.annotations why not use .metadata.annotations？

[RainbowMango]

@chaunceyjiang Sure you can take it. /assign @chaunceyjiang

And you are right, we should use the annotations from .metadata.annotations.

kind: Work
metadata:
  annotations:
    resourcebinding.karmada.io/name: nginx-deployment
    resourcebinding.karmada.io/namespace: default

[likakuli]

Name collision happens both for rb and work.

For rb, its name is generated by original resource object name and kind, so if there are two resources with same name and kind but different group version info in same namespace, the rb's name will be same too.

For work, its name is generated by original resource object namespace, name and rb's hash. Namespace, Name and hash can be same at the same time for different original resource object, so the work's name can be same too.

All the places to create or update rb or work is called by CreateOrUpdate function which will result to override when name collision happens. It's dangerous.

[chaunceyjiang]

For rb, its name is generated by original resource object name and kind, so if there are two resources with same name and kind but different group version info in same namespace, the rb's name will be same too.

https://github.com/karmada-io/karmada/blob/41ab9a58e786a72e3165ad52d43983287d8ef6fe/pkg/detector/detector.go#L154-L164

https://github.com/karmada-io/karmada/blob/41ab9a58e786a72e3165ad52d43983287d8ef6fe/pkg/util/lifted/discovery.go#L38-L39

Seems to get the preferred veriosn, maybe the resource has only one version

[RainbowMango]

yeah, exactly like what @chaunceyjiang said. We only take the preferred version. That means even user created a v1beta1 we take it as the v1(like Deoployment).

[likakuli]

kind can also be same in different group. e.g. openkruise advanced statefulset's kind is statefulset and k8s also have a statefulset kind.

https://github.com/openkruise/kruise-api/blob/master/apps/v1beta1/statefulset_types.go

[XiShanYongYe-Chang]

kind can also be same in different group. e.g. openkruise advanced statefulset's kind is statefulset and k8s also have a statefulset kind.

If so, there may indeed be a problem.

[XiShanYongYe-Chang]

@dddddai also mentioned in the comment https://github.com/karmada-io/karmada/issues/2090#issuecomment-1170660576

This is a new issue for the current issue. Maybe we can create a new issue to track it.

[likakuli]

This is a new issue for the current issue. Maybe we can create a new issue to track it.

agree, i will open a new issue

[likakuli]

https://github.com/karmada-io/karmada/issues/2099