Typo in CAPV controller manager ClusterRole label selector

[srm09]

/kind bug

What steps did you take and what happened:
I was looking at the aggregated role that CAPV uses for the controller manager. It specifically uses that to allow provider service account controller to be able to create roles/role bindings with extra RBAC permissions that the ones granted to the manager for normal operation.
It uses a ClusterRole with label selector capv.infrastucture.cluster.x-k8s.io/aggregate-to-manager: "true". As you can see there is a typo in infrastructure.

What did you expect to happen:
The label selector should not have the typo. I'd propose for it to be replaced with a newer label selector capv.infrastructure.cluster.x-k8s.io/aggregate-to-manager: "true" (without the typo) and the label selector with the typo to be deprecated and eventually phased out in favor of the newer one.

Anything else you would like to add:
For a few releases, CAPV should support both and release note the new label selector as well as the deprecation of the existing one with the date probable schedule of the deletion.

Environment:

• Cluster-api-provider-vsphere version:
• Kubernetes version: (use kubectl version):
• OS (e.g. from /etc/os-release):

[sbueringer]

Overall sounds good to me. Can we have both labels at the same time?

I was looking at the CAPV repo and didn't fine a role with permissions with the "capv. infrastucture.cluster.x-k8s.io/aggregate-to-manager" label. Can you provide some context around how this is used?

[srm09]

The ProviderServiceAccount CR when reconciled, generates a service account in the workload cluster with permissions specified via a set of RBAC rules.
For the CAPV controller to be able to generate a Role/RoleBinding with those permissions, CAPV's controller manager role should have those RBAC permissions as well. The idea behind the aggregated ClusterRole is
An end user who needs a ProviderServiceAccount (with specific permissions) created, adds a ClusterRole with those permissions and the selector label set. CAPV's aggregated manager role picks up the permissions and is ultimately able to reconcile the ProviderServiceAccount CR.

[sbueringer]

Sounds good to me!

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[kumarankit999]

Hey @srm09 , Thanks for raising this Issue.
It would be great If you could refer to the link to that page where we have to do the Changes!

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle rotten
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[zhanggbj]

/remove-lifecycle rotten

[chrischdi]

There are two occurrences for the wrong label capv.infrastucture.cluster.x-k8s.io/aggregate-to-manager:

1. At the ClusterRole manager-role as label:

https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/blob/main/config/base/manager_role_aggregation_patch.yaml#L6

2. At the Clusterrole aggregated-manager-role at aggregationRule.clusterRoleSelectors[]matchLabels

https://github.com/kubernetes-sigs/cluster-api-provider-vsphere/blob/main/config/rbac/aggregate_role.yaml#L8

To fix the issue we should propably do a multi-step approach with multiple phases:

1. Migration phase: make both labels work, the old and the new

• Add a second label to the manager-role (here):

  capv.infrastructure.cluster.x-k8s.io/aggregate-to-manager: "true"

• Add a second entry at aggregationRule.clusterRoleSelectors at the aggregated-manager-role:

  - matchLabels:
      capv.infrastructure.cluster.x-k8s.io/aggregate-to-manager: "true"

2. Wait phase: wait for X minor releases having both labels in place

3. Cleanup phase: cleanup the old labels in both places.

Alternative approach

The alternative approach would be to just fix the string in both places.

This would add risk to existing users, if they created an additional ClusterRole which references the manager-role via the old label (by an aggregationRule).

[chrischdi]

Before proceeding here with an implementation, I think we need consensus on if the alternative approach is okay (because it would be easier) or if we have to go down the long road.

Note: the above needs a review if I missed something here.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[chrischdi]

/remove-lifecycle stale

@sbueringer / @fabriziopandini : I propose to take the safe path and:

1. for v1.11: add the additional label in both places (so v1.11 has both labels)
2. for v1.12: do nothing (so v1.12 has both labels)
3. for v1.13: remove the old label (TBD, if we want to remove it even later)

[sbueringer]

Sounds good to me

(cc @zhanggbj PTAL to figure out what our options are for where we use CAPV (not sure how the CAPV manifests are used there, maybe we can just directly use the new label there))

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[chrischdi]

/remove-lifecycle stale