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

Reconcile loop for Aggregated ClusterRoles #6060

Open
a7i opened this issue Jan 16, 2025 · 8 comments
Open

Reconcile loop for Aggregated ClusterRoles #6060

a7i opened this issue Jan 16, 2025 · 8 comments
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@a7i
Copy link
Contributor

a7i commented Jan 16, 2025

What would you like to be added:

Ability to have ClusterRole with aggregated rules across member clusters.

If one of the member clusters has an Aggregated ClusterRole that the Karmada control plane lacks, it leads to a reconciliation conflict between the Karmada control plane and the member cluster controller-manager.

Why is this needed:

karmada control-plane installation comes with extra kinds, which aggregate to view [ref] -- these kinds do not exist in the member clusters. This issue also applies to any other components aggregating to view.

reproduce:

  1. Setup local karmada with ./hack/local-up-karmada.sh
  2. Create a ClusterPropagationPolicy to propagate a ClusterRole
cat <<EOF | kubectl --context karmada-apiserver --kubeconfig ~/.kube/karmada.config apply -f -
apiVersion: policy.karmada.io/v1alpha1
kind: ClusterPropagationPolicy
metadata:
  name: aggregated-example
spec:
  placement:
    clusterAffinity:
      clusterNames:
        - member1
        - member2
        - member3
  resourceSelectors:
  - apiVersion: rbac.authorization.k8s.io/v1
    kind: ClusterRole
    name: aggregated-example
EOF
  1. Create the ClusterRoles
cat <<EOF | kubectl --context karmada-apiserver --kubeconfig ~/.kube/karmada.config apply -f -
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  name: aggregated-example
aggregationRule:
  clusterRoleSelectors:
    - matchLabels:
        rbac.authorization.k8s.io/aggregate-to-view: "true"
rules: [] # aggregated roles cannot have any rules
EOF
  1. Observe update spam for member3 in a span of 1 second
time 1s kubectl --context member3 --kubeconfig ~/.kube/members.config get clusterroles aggregated-example -ocustom-columns='NAME:.metadata.name,RESOURCE-VERSION:.metadata.resourceVersion' -w
NAME                 RESOURCE-VERSION
aggregated-example   1459
aggregated-example   1460
aggregated-example   1461
aggregated-example   1462
aggregated-example   1463
aggregated-example   1464
aggregated-example   1465
aggregated-example   1466
aggregated-example   1467
aggregated-example   1468
aggregated-example   1469
aggregated-example   1470
aggregated-example   1471
aggregated-example   1472
aggregated-example   1473
aggregated-example   1474
aggregated-example   1475
aggregated-example   1476
aggregated-example   1477
aggregated-example   1478
aggregated-example   1479
aggregated-example   1480
aggregated-example   1481
aggregated-example   1482
aggregated-example   1483
aggregated-example   1484
aggregated-example   1485
aggregated-example   1486
aggregated-example   1487
aggregated-example   1488
aggregated-example   1489
aggregated-example   1490
aggregated-example   1491
aggregated-example   1492
aggregated-example   1493
aggregated-example   1494
aggregated-example   1495
@a7i a7i added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 16, 2025
@XiShanYongYe-Chang
Copy link
Member

Hi @a7i, thanks for your feedback. This is a special example of the propagation. What the user wants is to propagate the ClusterRole before aggregation to the member cluster, not the ClusterRole after aggregation. Do I understand correctly?

@a7i
Copy link
Contributor Author

a7i commented Jan 17, 2025

Hi @XiShanYongYe-Chang -- not necessarily, this seems to happen with any ClusterRole having rbac.authorization.k8s.io/aggregate-to-view: "true" -- this is because the karmada control-plane has additional kinds which get added to view permissions. I have updated the original issue to reflect this.

@zhzhuang-zju
Copy link
Contributor

Aggregated ClusterRoles allow for multiple ClusterRoles that match a label selector to be merged into a single ClusterRole. The rules of this aggregated ClusterRole are determined by the ClusterRoles that match the label selector. Consequently, the same Aggregated ClusterRole may have different rules across different clusters, which can ultimately lead to conflicts between the control plane and member clusters. @a7i Am I right? How do you plan to resolve it?

@zhzhuang-zju
Copy link
Contributor

Can this conflict between the control plane and member clusters be resolved by implementing a retention policy?

cat <<EOF | kubectl --context karmada-apiserver --kubeconfig ~/.kube/karmada.config apply -f -
apiVersion: config.karmada.io/v1alpha1
kind: ResourceInterpreterCustomization
metadata:
  name: clusterrole-rules-retention
spec:
  target:
    apiVersion: rbac.authorization.k8s.io/v1
    kind: ClusterRole
  customizations:
    retention:
      luaScript: >
        function Retain(desiredObj, observedObj)
          if desiredObj.aggregationRule == nil then
            return desiredObj
          end
          desiredObj.rules = observedObj.rules
          return desiredObj
        end
EOF

@XiShanYongYe-Chang
Copy link
Member

I prefer to solve this problem internally. If you leave it to the resource interpreter, the ric resources may be overwritten or deleted by the user.

@a7i
Copy link
Contributor Author

a7i commented Jan 21, 2025

Am I right? How do you plan to resolve it?

That is correct!

Can this conflict between the control plane and member clusters be resolved by implementing a retention policy?

Yes, that's how we're getting around it today. Similar to how you implemented it.

# ClusterRoles that use aggregationRule,
# get rules aggregated to them from member-cluster controller-manager.
# this means that karmada and member-cluster controller-manager will disagree on the definition.
# this is a stop-gap and is not meant for production.
apiVersion: config.karmada.io/v1alpha1
kind: ResourceInterpreterCustomization
metadata:
  name: cluster-role
  labels:
    role: interpreter
spec:
  target:
    apiVersion: rbac.authorization.k8s.io/v1
    kind: ClusterRole
  customizations:
    # @param desiredObj: the object in the source cluster to be propagated to the target cluster
    # @param observedObj: the object in the target cluster
    # @return: the object to be propagated to the target cluster
    retention:
      luaScript: |
        function hasAggregationRule(desiredObj)
          return desiredObj.aggregationRule ~= nil
        end
        
        function Retain(desiredObj, observedObj)
           if hasAggregationRule(observedObj) then
              return observedObj
          end
          return desiredObj
        end

I prefer to solve this problem internally.

I agree and happy to contribute, if you suggest

@XiShanYongYe-Chang
Copy link
Member

I agree and happy to contribute, if you suggest

Thank you. I'm not sure if it's the final solution, but maybe we can try it.

@zhzhuang-zju
Copy link
Contributor

Yes, that's how we're getting around it today. Similar to how you implemented it.

@a7i Just as a reminder, this retention policy will completely prevent the re-overwriting of modifications to Aggregated ClusterRoles in member clusters by the control plane, not sure if this is what you want. For example, when modifying the aggregationRule field of Aggregated ClusterRoles in member clusters, the control plane will also not overwrite and synchronize these changes.

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

No branches or pull requests

3 participants