Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failed to create or update resource longer than 63 chars #4683

Closed
a7i opened this issue Mar 5, 2024 · 10 comments · Fixed by #4765
Closed

Failed to create or update resource longer than 63 chars #4683

a7i opened this issue Mar 5, 2024 · 10 comments · Fixed by #4765
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@a7i
Copy link
Contributor

a7i commented Mar 5, 2024

What happened:

we have a role called kube-system/compute-controller-manager-leader-election-rolebinding, given that karmada adds it in the label, it cannot be longer than 63 characters

4m55s       Warning   SyncFailed               rolebinding/compute-controller-manager-leader-election-rolebinding   Failed to create or update resource(kube-system/compute-controller-manager-leader-election-rolebinding) in member cluster(eks-sandbox): RoleBinding.rbac.authorization.k8s.io "compute-controller-manager-leader-election-rolebinding" is invalid: metadata.labels: Invalid value: "compute-controller-manager-leader-election-rolebinding-9d54c6969": must be no more than 63 characters

What you expected to happen:
I would have expected to only use annotations, but not sure what should happent to the label

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

  1. Create a role or rolebinding called compute-controller-manager-leader-election-rolebinding
  2. Create a PropagationPolicy for it
  3. Observe event failures / no resourcebinding getting created

Anything else we need to know?:

Environment:

  • Karmada version: v1.9.0
  • kubectl-karmada or karmadactl version (the result of kubectl-karmada version or karmadactl version):
  • Others:
@a7i a7i added the kind/bug Categorizes issue or PR as related to a bug. label Mar 5, 2024
@XiShanYongYe-Chang
Copy link
Member

Hi @a7i, the issue you mentioned is being resolved in #4000. Can you help confirm it?

@a7i
Copy link
Contributor Author

a7i commented Mar 6, 2024

Hi @XiShanYongYe-Chang that's exactly it. Anything we can help with? Happy to contribute

@XiShanYongYe-Chang
Copy link
Member

Hi @XiShanYongYe-Chang that's exactly it. Anything we can help with? Happy to contribute

I'll sort out how far this task has gone recently, and I'll give you a reply after I've sorted it out.

@grosser
Copy link
Contributor

grosser commented Mar 13, 2024

@XiShanYongYe-Chang any progress ... anything that we can help with ?

@XiShanYongYe-Chang
Copy link
Member

@XiShanYongYe-Chang any progress ... anything that we can help with ?

Hi @grosser, being analyzed 🔥 (Code walk-through shows that many changes are involved.) Later, I will list the current progress and to-do items and provide the analysis result as soon as possible.

@XiShanYongYe-Chang
Copy link
Member

XiShanYongYe-Chang commented Mar 14, 2024

In previous versions, permanent-id labels have been added to PropagationPolicy/ClusterPropagationPolicy, ResourceBinding/ClusterResourceBinding, and Work resources, including:

  • propagationpolicy.karmada.io/permanent-id
  • clusterpropagationpolicy.karmada.io/permanent-id
  • resourcebinding.karmada.io/permanent-id
  • clusterresourcebinding.karmada.io/permanent-id
  • work.karmada.io/permanent-id

This provides the basis for our subsequent use of the permanent-id labels instead of the namespace/name labels.

In addition, we added labels describing the uid in previous iterations:

  • propagationpolicy.karmada.io/uid
  • clusterpropagationpolicy.karmada.io/uid
  • resourcebinding.karmada.io/uid
  • clusterresourcebinding.karmada.io/uid
  • work.karmada.io/uid

The above labels need to be cleaned up.

Let’s summarize the tasks we still need to deal with. After completing the following tasks, the problem described in the current issue can be solved.

1, Unify and add permanent-id labels to PropagationPolicy/ClusterPropagationPolicy, ResourceBinding/ClusterResourceBinding, and Work resources in karmada-webhook. This task has submitted a PR and can continue to advance based on this PR: #4474
2, Replace the use of namespace/name labels in the code with permanent-id labels, including:

  • propagationpolicy.karmada.io/namespace, propagationpolicy.karmada.io/name -> propagationpolicy.karmada.io/permanent-id
  • clusterpropagationpolicy.karmada.io/name -> clusterpropagationpolicy.karmada.io/permanent-id
  • resourcebinding.karmada.io/key -> resourcebinding.karmada.io/permanent-id
  • clusterresourcebinding.karmada.io/key -> clusterresourcebinding.karmada.io/permanent-id
  • work.karmada.io/namespace, work.karmada.io/name -> work.karmada.io/permanent-id

3, Clean up the uid labels on the resource:

  • propagationpolicy.karmada.io/uid
  • clusterpropagationpolicy.karmada.io/uid
  • resourcebinding.karmada.io/uid
  • clusterresourcebinding.karmada.io/uid
  • work.karmada.io/uid

4, Clean up namespace/name labels on resources:

  • propagationpolicy.karmada.io/namespace
  • propagationpolicy.karmada.io/name
  • clusterpropagationpolicy.karmada.io/name
  • propagationpolicy.karmada.io/uid
  • resourcebinding.karmada.io/key
  • clusterresourcebinding.karmada.io/key
  • work.karmada.io/namespace
  • work.karmada.io/name

5, The names of PP and CPP are not allowed to exceed 63 characters. This is the verification logic of karmada-webhook:

if len(policy.Name) > validation.LabelValueMaxLength {
return admission.Errored(http.StatusBadRequest, fmt.Errorf("PropagationPolicy's name should be no more than %d characters", validation.LabelValueMaxLength))
}

The reason for adding this verification is that the names of PP and CPP will appear in In the resource template and the label value of RB, this check can be removed after we solve the issue.

@XiShanYongYe-Chang
Copy link
Member

Hi @a7i @grosser, I've got a list of things to do in #4711, would you like to get involved?

@grosser
Copy link
Contributor

grosser commented Mar 14, 2024

awesome list, thx!
I added helping out to our backlog (ticket 888 hehe) so we'll hopefully get this moving next week+

@RainbowMango
Copy link
Member

I guess we can have a quick fix for this.

The root cause is the length of the resource template name (compute-controller-manager-leader-election-rolebinding-9d54c6969) exceeds 63 chars, and the Work name is built with this name(resourcename-<hash value>), and the Work name eventually added to the workload at

util.MergeLabel(workload, workv1alpha1.WorkNamespaceLabel, workNamespace)
util.MergeLabel(workload, workv1alpha1.WorkNameLabel, names.GenerateWorkName(workload.GetKind(), workload.GetName(), workload.GetNamespace()))
.

The labels added to the workload are used for observability, we already added similar value to the annotation, so we can remove the redundant labels now.

@RainbowMango
Copy link
Member

/assign @XiShanYongYe-Chang
In favor of #4765

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

4 participants