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 ResourceBinding for resources with :: in the name #4681

Open
a7i opened this issue Mar 5, 2024 · 8 comments · May be fixed by #4682
Open

Failed to create ResourceBinding for resources with :: in the name #4681

a7i opened this issue Mar 5, 2024 · 8 comments · May be fixed by #4682
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@a7i
Copy link
Contributor

a7i commented Mar 5, 2024

What happened:

If a Role has two colons in the name ::, then a ResourceBinding nor Work is created. Karmada replaces colon with dot (not sure why), but this causes issues as the name ends up having two dots in it which is not RFC 1123 compliant

Apply cluster policy(duplicate) failed: ResourceBinding.work.karmada.io "system..leader-locking-kube-controller-manager-role" is invalid: metadata.name: Invalid value: "system..leader-locking-kube-controller-manager-role": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

What you expected to happen:
The name to be sanitized by karmada-controller-manager

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

  1. Create a role called test::double-colon
  2. Create a PropagationPolicy for the role
  3. Observe events indicating failure in creating ResourceBinding

Anything else we need to know?:

To be fair, we should have never propagated this role (since it's a system role), but regardless it should be sanitized.

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
@a7i a7i linked a pull request Mar 5, 2024 that will close this issue
@XiShanYongYe-Chang
Copy link
Member

@a7i Thanks a lot for your feedback!

To be fair, we should have never propagated this role (since it's a system role)

I agree with you that propagate system Role don't seem to have a reasonable usage scenario. We should consider banning propagation system Role (and other RBAC resources, including RoleBinding, ClusterRole, and ClusterRoleBinding).

By the way, how did you discover the problem? Are you trying to propagate a system Role?

@a7i
Copy link
Contributor Author

a7i commented Mar 6, 2024

@XiShanYongYe-Chang that is correct, I accidentally propagated a system role.

With that being said, :: is valid in names for clusterrole/clusterolebinding/roles/rolebindings. See example below:

$ kubectl create role "foo::bar" --verb=get --resource=pods

role.rbac.authorization.k8s.io/foo::bar created
$ kubectl get role "foo::bar" -oyaml

apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
  name: foo::bar
  namespace: default
rules:
- apiGroups:
  - ""
  resources:
  - pods
  verbs:
  - get

@XiShanYongYe-Chang
Copy link
Member

Ask @RainbowMango to help take a look.

@RainbowMango
Copy link
Member

Karmada replaces colon with dot (not sure why),

That is because the CRD resources can not accept names with :, the comments here explained the reason.

// 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.

This is indeed a tradeoff to do the replacement. :(

To be fair, we should have never propagated this role (since it's a system role), but regardless it should be sanitized.

The trouble is that we can not assume there is one or two colons in the name. There may be more.

Echo from https://kubernetes.io/docs/reference/access-authn-authz/rbac/#referring-to-subjects:

Caution: The prefix system: is reserved for Kubernetes system use, so you should ensure that you don't have users or groups with names that start with system: by accident. Other than this special prefix, the RBAC authorization system does not require any format for usernames.

Given the RBAC system doesn't require any format, that means the users might create resources with any number of colons, like:

# kubectl create role "foo:::bar" --verb=get --resource=pods  // 3 colons
role.rbac.authorization.k8s.io/foo:::bar created
# kubectl create role "foo::::bar" --verb=get --resource=pods // 4 colons
role.rbac.authorization.k8s.io/foo::::bar created
# # kubectl create role "foo:::::bar" --verb=get --resource=pods // 5 colons
role.rbac.authorization.k8s.io/foo:::::bar created

How do we handle these cases? Or shall we handle that?

@a7i
Copy link
Contributor Author

a7i commented Mar 14, 2024

Perhaps we could replace : with - instead of .

Given that fo----bar is still a valid domain

@a7i
Copy link
Contributor Author

a7i commented Mar 14, 2024

kubectl create role "foo:::bar" --verb=get --resource=pods

role.rbac.authorization.k8s.io/foo:::bar created
kubectl label role "foo:::bar" foo---bar=test
 
role.rbac.authorization.k8s.io/foo:::bar labeled
kubectl get role foo:::bar -L foo---bar
NAME        CREATED AT             FOO---BAR
foo:::bar   2024-03-14T03:05:57Z   test

@XiShanYongYe-Chang
Copy link
Member

This is a feasible method.

@RainbowMango
Copy link
Member

Perhaps we could replace : with - instead of .

Yeah, but it might be a breaking change incompatible with previous versions. That is concerning. right?

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.

3 participants