karmada-io / karmada

Open, Multi-Cloud, Multi-Cluster Kubernetes Orchestration
https://karmada.io
Apache License 2.0
4.05k stars 790 forks source link

Is clusterresourcebinding.name suitable by kind and name #3627

Open weilaaa opened 10 months ago

weilaaa commented 10 months ago

I have two different resources, the Kind is the same, but Group and Version are not. Those as bellow:

clusters.cluster.example1.com/v1 named cluster.a
clusters.cluster.example2.com/v1beta1 named cluster.a

Now, I noticed clusterresourcebinding.name is generated by kind and name which means for clusters.cluster.example1.com/v1 and clusters.cluster.example2.com/v1beta1, the name of their clusterresourcebinding is the same, both are cluster.a-cluster. This seems to be problematic.

// GenerateBindingName will generate binding name by kind and name
func GenerateBindingName(kind, name string) string {
    // The name of resources, like 'Role'/'ClusterRole'/'RoleBinding'/'ClusterRoleBinding',
    // may contain symbols(like ':') that are not allowed by CRD resources which require the
    // name can be used as a DNS subdomain name. So, we need to replace it.
    // These resources may also allow for other characters(like '&','$') that are not allowed
    // by CRD resources, we only handle the most common ones now for performance concerns.
    // For more information about the DNS subdomain name, please refer to
    // https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names.
    if strings.Contains(name, ":") {
        name = strings.ReplaceAll(name, ":", ".")
    }

    return strings.ToLower(name + "-" + kind)
}
weilaaa commented 10 months ago

@RainbowMango cc

chaunceyjiang commented 10 months ago

Yes, This is a known issue. refer to https://github.com/karmada-io/karmada/pull/2969

RainbowMango commented 10 months ago

Hi @weilaaa glad to see you are back. Yes, it's a known issue and also a tricky one. We don't have a better solution for it yet. :(

weilaaa commented 10 months ago

I read the pr #2969 and all guys comments , IMHO, I don't like RB name with two format exist, the one is kind-name, the other is kind-name-rand.String(5), which makes user confused, even though RB is not user-face api. K8s used CollisionCount but it kept final name as name-hash.

weilaaa commented 10 months ago

I ask is there a way to migrate the existing RB data to the new RB named by name-hash without affecting the user's use.

For example, we clone the corresponding new RB based on the existing RB, and mark the existing RB as deprecated. At this time, the newly added RB will use the new name-hash name format. Just in case, we can also record CollisionCount in the annotation to prevent hash collisions. Besides that, GenerateBindingName does only one thing, simply generating the name is like K8s generating the bash suffix.

RainbowMango commented 9 months ago

I ask is there a way to migrate the existing RB data to the new RB named by name-hash without affecting the user's use.

Yes, this is also the thing that concerns me the most. I expect a solution that could transform the legacy RB without perception.

For example, we clone the corresponding new RB based on the existing RB, and mark the existing RB as deprecated. At this time, the newly added RB will use the new name-hash name format.

Sounds good to me. Speaking of the naming rule of RB, the naming of "Work" also has similar issues. I analyzed the issue and draft some ideas at this document, you can take a look.

This is a very challenging task, and now is also a very good time to solve this problem(Now is the beginning of the v1.7 version cycle.) I'll appreciate it if you want to lead the effort.

weilaaa commented 9 months ago

Thanks for the chance. I'll do some investigation first to check out the impact and possible solutions.

RainbowMango commented 7 months ago

I'm trying to figure out which PR/Issue should be included in the coming v1.7 release which is planned at the end of this month. I guess we don't have enough time for this feature, so I'm moving this to v1.8.

FYI, We are working on this, the relevant topic was talked at the community meeting, and see #4000 for the progress.