🌱 Set ClusterctlMoveHierarchyLabel on HelmChartProxy in CRD definition

[Jont828]

What this PR does / why we need it: Have the HelmChartProxy controller set the ClusterctlMoveHierarchyLabel on HelmChartProxies so that it will be discovered by clusterctl move.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #103

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jont828

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Jont828]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Jont828]

The alternative approach would be to have the user set the label in the YAML like here in CAPZ. That would allow users to opt-out, but the downside would be that users would have to remember to do that every time.

[CecileRobertMichon]

if we decide this approach is better we could also change the CAPZ AzureClusterIdentity approach

[Jont828]

@CecileRobertMichon @jackfrancis TL;DR HelmChartProxies need the clusterctl.ClusterctlMoveHierarchyLabel label attached in order for clusterctl move to work properly. Do you think it's appropriate to have the controller always add it, or instead leave it up to the user to opt-in?

[CecileRobertMichon]

@fabriziopandini do you have thoughts on this?

IMO if it can always be set so the user doesn't have to worry about it that's better, but what happens if the user is using the same HelmChartProxy across several clusters and moves 1 or some of the clusters?

note: if the goal is to set the label to every HelmChartProxy, the book indicates this can be done on the CRD itself so it applies to all HelmChartProxies instead of labelling each object.

Note. clusterctl.cluster.x-k8s.io/move and clusterctl.cluster.x-k8s.io/move-hierarchy labels could be applied to single objects or at the CRD level (the label applies to all the objects).

https://cluster-api.sigs.k8s.io/clusterctl/provider-contract.html?highlight=move-hier#move

[fabriziopandini]

@fabriziopandini do you have thoughts on this?

I will suggest adding the label on the CRD definition, so it is done once and for all (no need to add on single CRs, reconcile it).

[Jont828]

How would we set a label on the CRD level? Would it be a kubebuilder tag?

[Jont828]

@fabriziopandini I tried adding the kubebuilder label based on the docs, but it doesn't seem to be working. Are there any examples of this being used?

// +kubebuilder:metadata:labels=["clusterctl.cluster.x-k8s.io/move-hierarchy: true"]

[fabriziopandini]

How would we set a label on the CRD level? Would it be a kubebuilder tag?

You can add it by creating a kustomize patch similar to this one.
Make sure that [name]( name: helmchartproxies.addons.cluster.x-k8s.io) is the name of the CRD you want to patch, replace annotations with labels: and the label you want to add.

Make sure to add your next patch to the kustomization file

[Jont828]

@fabriziopandini I've followed your suggestion and added the label to the CRD definition such that I have the following output. However, the label doesn't appear on the resources when I run kubectl get and they do not appear in the output of clusterctl move. Am I doing something wrong?

$ kc describe crd helmchartproxies.addons.cluster.x-k8s.io
Name:         helmchartproxies.addons.cluster.x-k8s.io
Namespace:
Labels:       cluster.x-k8s.io/provider=helm
              clusterctl.cluster.x-k8s.io/move-hierarchy=true
Annotations:  cert-manager.io/inject-ca-from: caaph-system/caaph-serving-cert
              controller-gen.kubebuilder.io/version: v0.10.0
API Version:  apiextensions.k8s.io/v1
...

apiVersion: addons.cluster.x-k8s.io/v1alpha1
kind: HelmChartProxy
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"addons.cluster.x-k8s.io/v1alpha1","kind":"HelmChartProxy","metadata":{"annotations":{},"name":"calico-cni","namespace":"default"},"spec":{"chartName":"tigera-operator","clusterSelector":{"matchLabels":{}},"namespace":"kube-system","releaseName":"calico","repoURL":"https://docs.tigera.io/calico/charts","valuesTemplate":"installation:\n  cni:\n    type: Calico\n    ipam:\n      type: HostLocal\n  calicoNetwork:\n    bgp: Disabled\n    mtu: 1350\n    ipPools:{{range $i, $cidr := .Cluster.spec.clusterNetwork.pods.cidrBlocks }}\n    - cidr: {{ $cidr }}\n      encapsulation: None\n      natOutgoing: Enabled\n      nodeSelector: all(){{end}}\n"}}
  creationTimestamp: "2023-10-11T01:03:12Z"
  finalizers:
  - helmchartproxy.addons.cluster.x-k8s.io
  generation: 1
  name: calico-cni
  namespace: default
  resourceVersion: "1493"
  uid: 335bc313-c6df-4d04-b8f8-05cf70a04923

[Jont828]

@CecileRobertMichon @fabriziopandini I think I figured it out! The CRD label will only show up in the definition when it is installed through make release-manifests and not make generate-manifests, and it will only work with clusterctl move if the CRDs are installed through clusterctl init --addon helm.

Since that pulls from the GitHub releases, we will only know for sure that it works once this is merged and we cut a new CAAPH release. However, I tested that addon-components.yaml is produced to include the label, and that clusterctl move works once we manually edit the HelmChartProxy CRD definition to include the label, so I'm confident that this works and we can go ahead for review/merge!

[CecileRobertMichon]

comment needs to be updated

[CecileRobertMichon]

one nit, otherwise lgtm

[CecileRobertMichon]

/lgtm