Merge pull request #8256 from killianmuldoon/pr-test-ownerref

🐛 Ensure ownerReference apiVersions are always up to date

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

@@ -1089,15 +1089,15 @@ func (r *KubeadmConfigReconciler) ensureBootstrapSecretOwnersRef(ctx context.Con
 		return errors.Wrapf(err, "failed to add KubeadmConfig %s as ownerReference to bootstrap Secret %s", scope.ConfigOwner.GetName(), secret.GetName())
 	}
 	if c := metav1.GetControllerOf(secret); c != nil && c.Kind != "KubeadmConfig" {
-		secret.OwnerReferences = util.RemoveOwnerRef(secret.OwnerReferences, *c)
+		secret.SetOwnerReferences(util.RemoveOwnerRef(secret.GetOwnerReferences(), *c))
 	}
-	secret.OwnerReferences = util.EnsureOwnerRef(secret.OwnerReferences, metav1.OwnerReference{
+	secret.SetOwnerReferences(util.EnsureOwnerRef(secret.GetOwnerReferences(), metav1.OwnerReference{
 		APIVersion: bootstrapv1.GroupVersion.String(),
 		Kind:       "KubeadmConfig",
 		UID:        scope.Config.UID,
 		Name:       scope.Config.Name,
 		Controller: pointer.Bool(true),
-	})
+	}))
 	err = patchHelper.Patch(ctx, secret)
 	if err != nil {
 		return errors.Wrapf(err, "could not add KubeadmConfig %s as ownerReference to bootstrap Secret %s", scope.ConfigOwner.GetName(), secret.GetName())

cmd/clusterctl/client/cluster/ownergraph.go

@@ -17,9 +17,15 @@ limitations under the License.
 package cluster
 
 import (
+	"strings"
+
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3"
 )
 
 // OwnerGraph contains a graph with all the objects considered by clusterctl move as nodes and the OwnerReference relationship
@@ -32,22 +38,6 @@ type OwnerGraphNode struct {
 	Owners []metav1.OwnerReference
 }
 
-func nodeToOwnerRef(n *node, attributes ownerReferenceAttributes) metav1.OwnerReference {
-	ref := metav1.OwnerReference{
-		Name:       n.identity.Name,
-		APIVersion: n.identity.APIVersion,
-		Kind:       n.identity.Kind,
-		UID:        n.identity.UID,
-	}
-	if attributes.BlockOwnerDeletion != nil {
-		ref.BlockOwnerDeletion = attributes.BlockOwnerDeletion
-	}
-	if attributes.Controller != nil {
-		ref.Controller = attributes.Controller
-	}
-	return ref
-}
-
 // GetOwnerGraph returns a graph with all the objects considered by clusterctl move as nodes and the OwnerReference relationship between those objects as edges.
 // NOTE: this data structure is exposed to allow implementation of E2E tests verifying that CAPI can properly rebuild its
 // own owner references; there is no guarantee about the stability of this API. Using this test with providers may require
@@ -64,20 +54,79 @@ func GetOwnerGraph(namespace, kubeconfigPath string) (OwnerGraph, error) {
 		return OwnerGraph{}, errors.Wrap(err, "failed to retrieve discovery types")
 	}
 
-	// Discovery the object graph for the selected types:
-	// - Nodes are defined the Kubernetes objects (Clusters, Machines etc.) identified during the discovery process.
-	// - Edges are derived by the OwnerReferences between nodes.
-	if err := graph.Discovery(namespace); err != nil {
-		return OwnerGraph{}, errors.Wrap(err, "failed to discover the object graph")
+	// graph.Discovery can not be used here as it will use the latest APIVersion for ownerReferences - not those
+	// present in the object 'metadata.ownerReferences`.
+	owners, err := discoverOwnerGraph(namespace, graph)
+	if err != nil {
+		return OwnerGraph{}, errors.Wrap(err, "failed to discovery ownerGraph types")
 	}
-	owners := OwnerGraph{}
-	// Using getMoveNodes here ensures only objects that are part of the Cluster are added to the OwnerGraph.
-	for _, v := range graph.getMoveNodes() {
-		n := OwnerGraphNode{Object: v.identity, Owners: []metav1.OwnerReference{}}
-		for owner, attributes := range v.owners {
-			n.Owners = append(n.Owners, nodeToOwnerRef(owner, attributes))
+	return owners, nil
+}
+
+func discoverOwnerGraph(namespace string, o *objectGraph) (OwnerGraph, error) {
+	selectors := []client.ListOption{}
+	if namespace != "" {
+		selectors = append(selectors, client.InNamespace(namespace))
+	}
+	ownerGraph := OwnerGraph{}
+
+	discoveryBackoff := newReadBackoff()
+	for _, discoveryType := range o.types {
+		typeMeta := discoveryType.typeMeta
+		objList := new(unstructured.UnstructuredList)
+
+		if err := retryWithExponentialBackoff(discoveryBackoff, func() error {
+			return getObjList(o.proxy, typeMeta, selectors, objList)
+		}); err != nil {
+			return nil, err
+		}
+
+		// if we are discovering Secrets, also secrets from the providers namespace should be included.
+		if discoveryType.typeMeta.GetObjectKind().GroupVersionKind().GroupKind() == corev1.SchemeGroupVersion.WithKind("SecretList").GroupKind() {
+			providers, err := o.providerInventory.List()
+			if err != nil {
+				return nil, err
+			}
+			for _, p := range providers.Items {
+				if p.Type == string(clusterctlv1.InfrastructureProviderType) {
+					providerNamespaceSelector := []client.ListOption{client.InNamespace(p.Namespace)}
+					providerNamespaceSecretList := new(unstructured.UnstructuredList)
+					if err := retryWithExponentialBackoff(discoveryBackoff, func() error {
+						return getObjList(o.proxy, typeMeta, providerNamespaceSelector, providerNamespaceSecretList)
+					}); err != nil {
+						return nil, err
+					}
+					objList.Items = append(objList.Items, providerNamespaceSecretList.Items...)
+				}
+			}
+		}
+		for _, obj := range objList.Items {
+			// Exclude the kube-root-ca.crt ConfigMap from the owner graph.
+			if obj.GetKind() == "ConfigMap" && obj.GetName() == "kube-root-ca.crt" {
+				continue
+			}
+			// Exclude the default service account from the owner graph.
+			// This Secret is not longer generated by default in Kubernetes 1.24+.
+			// This is not a CAPI related Secret, so it can be ignored.
+			if obj.GetKind() == "Secret" && strings.Contains(obj.GetName(), "default-token") {
+				continue
+			}
+			ownerGraph = addNodeToOwnerGraph(ownerGraph, obj)
 		}
-		owners[string(v.identity.UID)] = n
 	}
-	return owners, nil
+	return ownerGraph, nil
+}
+
+func addNodeToOwnerGraph(graph OwnerGraph, obj unstructured.Unstructured) OwnerGraph {
+	// write code to add a node to the ownerGraph
+	graph[string(obj.GetUID())] = OwnerGraphNode{
+		Owners: obj.GetOwnerReferences(),
+		Object: corev1.ObjectReference{
+			APIVersion: obj.GetAPIVersion(),
+			Kind:       obj.GetKind(),
+			Name:       obj.GetName(),
+			Namespace:  obj.GetNamespace(),
+		},
+	}
+	return graph
 }

cmd/clusterctl/internal/test/fake_objects.go

@@ -39,6 +39,7 @@ import (
 	addonsv1 "sigs.k8s.io/cluster-api/exp/addons/api/v1beta1"
 	expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
 	"sigs.k8s.io/cluster-api/internal/test/builder"
+	"sigs.k8s.io/cluster-api/util"
 )
 
 type FakeCluster struct {
@@ -563,14 +564,14 @@ func (f *FakeMachineDeployment) Objs(cluster *clusterv1.Cluster) []client.Object
 		machineDeploymentInfrastructure = NewFakeInfrastructureTemplate(f.name)
 	}
 	machineDeploymentInfrastructure.Namespace = cluster.Namespace
-	machineDeploymentInfrastructure.OwnerReferences = append(machineDeploymentInfrastructure.OwnerReferences, // Added by the machine set controller -- RECONCILED
+	machineDeploymentInfrastructure.SetOwnerReferences(util.EnsureOwnerRef(machineDeploymentInfrastructure.GetOwnerReferences(), // Added by the machine set controller -- RECONCILED
 		metav1.OwnerReference{
 			APIVersion: clusterv1.GroupVersion.String(),
 			Kind:       "Cluster",
 			Name:       cluster.Name,
 			UID:        cluster.UID,
 		},
-	)
+	))
 	setUID(machineDeploymentInfrastructure)
 
 	machineDeploymentBootstrap := &fakebootstrap.GenericBootstrapConfigTemplate{

controlplane/kubeadm/internal/controllers/controller.go

@@ -57,7 +57,10 @@ import (
 	"sigs.k8s.io/cluster-api/util/version"
 )
 
-const kcpManagerName = "capi-kubeadmcontrolplane"
+const (
+	kcpManagerName          = "capi-kubeadmcontrolplane"
+	kubeadmControlPlaneKind = "KubeadmControlPlane"
+)
 
 // +kubebuilder:rbac:groups=core,resources=events,verbs=get;list;watch;create;patch
 // +kubebuilder:rbac:groups=core,resources=secrets,verbs=get;list;watch;create;update;patch
@@ -290,7 +293,7 @@ func (r *KubeadmControlPlaneReconciler) reconcile(ctx context.Context, cluster *
 		config.ClusterConfiguration = &bootstrapv1.ClusterConfiguration{}
 	}
 	certificates := secret.NewCertificatesForInitialControlPlane(config.ClusterConfiguration)
-	controllerRef := metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind("KubeadmControlPlane"))
+	controllerRef := metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind(kubeadmControlPlaneKind))
 	if err := certificates.LookupOrGenerate(ctx, r.Client, util.ObjectKey(cluster), *controllerRef); err != nil {
 		log.Error(err, "unable to lookup or create cluster certificates")
 		conditions.MarkFalse(kcp, controlplanev1.CertificatesAvailableCondition, controlplanev1.CertificatesGenerationFailedReason, clusterv1.ConditionSeverityWarning, err.Error())
@@ -528,7 +531,7 @@ func (r *KubeadmControlPlaneReconciler) ClusterToKubeadmControlPlane(o client.Ob
 	}
 
 	controlPlaneRef := c.Spec.ControlPlaneRef
-	if controlPlaneRef != nil && controlPlaneRef.Kind == "KubeadmControlPlane" {
+	if controlPlaneRef != nil && controlPlaneRef.Kind == kubeadmControlPlaneKind {
 		return []ctrl.Request{{NamespacedName: client.ObjectKey{Namespace: controlPlaneRef.Namespace, Name: controlPlaneRef.Name}}}
 	}
 
@@ -888,21 +891,29 @@ func ensureCertificatesOwnerRef(ctx context.Context, ctrlclient client.Client, c
 		if err := ctrlclient.Get(ctx, secretKey, s); err != nil {
 			return errors.Wrapf(err, "failed to get Secret %s", secretKey)
 		}
-		// If the Type doesn't match the type used for secrets created by core components, KCP included
-		if s.Type != clusterv1.ClusterSecretType {
-			continue
-		}
+
 		patchHelper, err := patch.NewHelper(s, ctrlclient)
 		if err != nil {
 			return errors.Wrapf(err, "failed to create patchHelper for Secret %s", secretKey)
 		}
 
-		// Remove the current controller if one exists.
-		if controller := metav1.GetControllerOf(s); controller != nil {
-			s.SetOwnerReferences(util.RemoveOwnerRef(s.OwnerReferences, *controller))
+		controller := metav1.GetControllerOf(s)
+		// If the current controller is KCP, ensure the owner reference is up to date.
+		// Note: This ensures secrets created prior to v1alpha4 are updated to have the correct owner reference apiVersion.
+		if controller != nil && controller.Kind == kubeadmControlPlaneKind {
+			s.SetOwnerReferences(util.EnsureOwnerRef(s.GetOwnerReferences(), owner))
 		}
 
-		s.OwnerReferences = util.EnsureOwnerRef(s.OwnerReferences, owner)
+		// If the Type doesn't match the type used for secrets created by core components continue without altering the owner reference further.
+		// Note: This ensures that control plane related secrets created by KubeadmConfig are eventually owned by KCP.
+		// TODO: Remove this logic once standalone control plane machines are no longer allowed.
+		if s.Type == clusterv1.ClusterSecretType {
+			// Remove the current controller if one exists.
+			if controller != nil {
+				s.SetOwnerReferences(util.RemoveOwnerRef(s.GetOwnerReferences(), *controller))
+			}
+			s.SetOwnerReferences(util.EnsureOwnerRef(s.GetOwnerReferences(), owner))
+		}
 		if err := patchHelper.Patch(ctx, s); err != nil {
 			return errors.Wrapf(err, "failed to patch Secret %s with ownerReference %s", secretKey, owner.String())
 		}

controlplane/kubeadm/internal/controllers/controller_test.go

@@ -878,8 +878,8 @@ func TestKubeadmControlPlaneReconciler_ensureOwnerReferences(t *testing.T) {
 			// Set the Secret Type to any type which signals this Secret was is user-provided.
 			s.Type = corev1.SecretTypeOpaque
 			// Set the a controller owner reference of an unknown type on the secret.
-			s.SetOwnerReferences([]metav1.OwnerReference{
-				{
+			s.SetOwnerReferences(util.EnsureOwnerRef(s.GetOwnerReferences(),
+				metav1.OwnerReference{
 					APIVersion: bootstrapv1.GroupVersion.String(),
 					// This owner reference to a different controller should be preserved.
 					Kind:               "OtherController",
@@ -888,7 +888,7 @@ func TestKubeadmControlPlaneReconciler_ensureOwnerReferences(t *testing.T) {
 					Controller:         pointer.Bool(true),
 					BlockOwnerDeletion: pointer.Bool(true),
 				},
-			})
+			))
 
 			objs = append(objs, s)
 		}

controlplane/kubeadm/internal/controllers/helpers.go

@@ -29,7 +29,6 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apiserver/pkg/storage/names"
-	"k8s.io/klog/v2"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -56,7 +55,7 @@ func (r *KubeadmControlPlaneReconciler) reconcileKubeconfig(ctx context.Context,
 		return ctrl.Result{}, nil
 	}
 
-	controllerOwnerRef := *metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind("KubeadmControlPlane"))
+	controllerOwnerRef := *metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind(kubeadmControlPlaneKind))
 	clusterName := util.ObjectKey(cluster)
 	configSecret, err := secret.GetFromNamespacedName(ctx, r.Client, clusterName, secret.Kubeconfig)
 	switch {
@@ -102,46 +101,43 @@ func (r *KubeadmControlPlaneReconciler) reconcileKubeconfig(ctx context.Context,
 }
 
 // Ensure the KubeadmConfigSecret has an owner reference to the control plane if it is not a user-provided secret.
-func (r *KubeadmControlPlaneReconciler) adoptKubeconfigSecret(ctx context.Context, cluster *clusterv1.Cluster, configSecret *corev1.Secret, kcp *controlplanev1.KubeadmControlPlane) error {
-	log := ctrl.LoggerFrom(ctx)
-	controller := metav1.GetControllerOf(configSecret)
-
-	// If the Type doesn't match the CAPI-created secret type this is a no-op.
-	if configSecret.Type != clusterv1.ClusterSecretType {
-		return nil
-	}
-	// If the secret is already controlled by KCP this is a no-op.
-	if controller != nil && controller.Kind == "KubeadmControlPlane" {
-		return nil
-	}
-	log.Info("Adopting KubeConfig secret", "Secret", klog.KObj(configSecret))
-	patch, err := patch.NewHelper(configSecret, r.Client)
+func (r *KubeadmControlPlaneReconciler) adoptKubeconfigSecret(ctx context.Context, cluster *clusterv1.Cluster, configSecret *corev1.Secret, kcp *controlplanev1.KubeadmControlPlane) (reterr error) {
+	patchHelper, err := patch.NewHelper(configSecret, r.Client)
 	if err != nil {
 		return errors.Wrap(err, "failed to create patch helper for the kubeconfig secret")
 	}
+	defer func() {
+		if err := patchHelper.Patch(ctx, configSecret); err != nil {
+			reterr = errors.Wrap(err, "failed to patch the kubeconfig secret")
+		}
+	}()
+	controller := metav1.GetControllerOf(configSecret)
 
-	// If the kubeconfig secret was created by v1alpha2 controllers, and thus it has the Cluster as the owner instead of KCP.
-	// In this case remove the ownerReference to the Cluster.
-	if util.IsOwnedByObject(configSecret, cluster) {
-		configSecret.SetOwnerReferences(util.RemoveOwnerRef(configSecret.OwnerReferences, metav1.OwnerReference{
-			APIVersion: clusterv1.GroupVersion.String(),
-			Kind:       "Cluster",
-			Name:       cluster.Name,
-			UID:        cluster.UID,
-		}))
-	}
-
-	// Remove the current controller if one exists.
-	if controller != nil {
-		configSecret.SetOwnerReferences(util.RemoveOwnerRef(configSecret.OwnerReferences, *controller))
+	// If the current controller is KCP, ensure the owner reference is up to date and return early.
+	// Note: This ensures secrets created prior to v1alpha4 are updated to have the correct owner reference apiVersion.
+	if controller != nil && controller.Kind == kubeadmControlPlaneKind {
+		configSecret.SetOwnerReferences(util.EnsureOwnerRef(configSecret.GetOwnerReferences(), *metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind(kubeadmControlPlaneKind))))
+		return nil
 	}
 
-	// Add the KubeadmControlPlane as the controller for this secret.
-	configSecret.OwnerReferences = util.EnsureOwnerRef(configSecret.OwnerReferences,
-		*metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind("KubeadmControlPlane")))
-
-	if err := patch.Patch(ctx, configSecret); err != nil {
-		return errors.Wrap(err, "failed to patch the kubeconfig secret")
+	// If secret type is a CAPI-created secret ensure the owner reference is to KCP.
+	if configSecret.Type == clusterv1.ClusterSecretType {
+		// Remove the current controller if one exists and ensure KCP is the controller of the secret.
+		if controller != nil {
+			configSecret.SetOwnerReferences(util.RemoveOwnerRef(configSecret.GetOwnerReferences(), *controller))
+		}
+		configSecret.SetOwnerReferences(util.EnsureOwnerRef(configSecret.GetOwnerReferences(), *metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind(kubeadmControlPlaneKind))))
+
+		// If the kubeconfig secret was created by v1alpha2 controllers, and thus it has the Cluster as the owner instead of KCP.
+		// In this case remove the ownerReference to the Cluster.
+		if util.IsOwnedByObject(configSecret, cluster) {
+			configSecret.SetOwnerReferences(util.RemoveOwnerRef(configSecret.GetOwnerReferences(), metav1.OwnerReference{
+				APIVersion: clusterv1.GroupVersion.String(),
+				Kind:       "Cluster",
+				Name:       cluster.Name,
+				UID:        cluster.UID,
+			}))
+		}
 	}
 	return nil
 }
@@ -184,7 +180,7 @@ func (r *KubeadmControlPlaneReconciler) cloneConfigsAndGenerateMachine(ctx conte
 	// OwnerReference here without the Controller field set
 	infraCloneOwner := &metav1.OwnerReference{
 		APIVersion: controlplanev1.GroupVersion.String(),
-		Kind:       "KubeadmControlPlane",
+		Kind:       kubeadmControlPlaneKind,
 		Name:       kcp.Name,
 		UID:        kcp.UID,
 	}
@@ -260,7 +256,7 @@ func (r *KubeadmControlPlaneReconciler) generateKubeadmConfig(ctx context.Contex
 	// Create an owner reference without a controller reference because the owning controller is the machine controller
 	owner := metav1.OwnerReference{
 		APIVersion: controlplanev1.GroupVersion.String(),
-		Kind:       "KubeadmControlPlane",
+		Kind:       kubeadmControlPlaneKind,
 		Name:       kcp.Name,
 		UID:        kcp.UID,
 	}
@@ -408,7 +404,7 @@ func (r *KubeadmControlPlaneReconciler) computeDesiredMachine(kcp *controlplanev
 			Namespace: kcp.Namespace,
 			// Note: by setting the ownerRef on creation we signal to the Machine controller that this is not a stand-alone Machine.
 			OwnerReferences: []metav1.OwnerReference{
-				*metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind("KubeadmControlPlane")),
+				*metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind(kubeadmControlPlaneKind)),
 			},
 			Labels:      map[string]string{},
 			Annotations: map[string]string{},

exp/addons/api/v1beta1/clusterresourcesetbinding_types.go

@@ -106,7 +106,7 @@ func (c *ClusterResourceSetBinding) DeleteBinding(clusterResourceSet *ClusterRes
 			break
 		}
 	}
-	c.OwnerReferences = util.RemoveOwnerRef(c.OwnerReferences, metav1.OwnerReference{
+	c.OwnerReferences = util.RemoveOwnerRef(c.GetOwnerReferences(), metav1.OwnerReference{
 		APIVersion: clusterResourceSet.APIVersion,
 		Kind:       clusterResourceSet.Kind,
 		Name:       clusterResourceSet.Name,