fix: fix ControlPlane managed resource labels during migration from o…

…lder operator versions

CHANGELOG.md

@@ -30,6 +30,12 @@
 - Proper `User-Agent` header is now set on outgoing HTTP requests.
   [#387](https://github.com/Kong/gateway-operator/pull/387)
 
+### Fixed
+
+- Fixed `ControlPlane` cluster wide resources not migrating to new ownership labels
+  (introduced in 1.3.0) when upgrading the operator form 1.2 (or older) to 1.3.0.
+  [#369](https://github.com/Kong/gateway-operator/pull/369)
+
 ## [v1.3.0]
 
 > Release date: 2024-06-24

controller/controlplane/controller.go

@@ -63,9 +63,6 @@ func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
 	validatinWebhookConfigurationOwnerPredicate.UpdateFunc = func(e event.UpdateEvent) bool {
 		return r.validatingWebhookConfigurationHasControlPlaneOwner(e.ObjectOld)
 	}
-	validatinWebhookConfigurationOwnerPredicate.DeleteFunc = func(e event.DeleteEvent) bool {
-		return r.validatingWebhookConfigurationHasControlPlaneOwner(e.Object)
-	}
 
 	return ctrl.NewControllerManagedBy(mgr).
 		// watch ControlPlane objects

controller/controlplane/controller_reconciler_utils.go

@@ -293,16 +293,41 @@ func (r *Reconciler) ensureClusterRole(
 	ctx context.Context,
 	cp *operatorv1beta1.ControlPlane,
 ) (createdOrUpdated bool, cr *rbacv1.ClusterRole, err error) {
-	managedByLabelSet := k8sutils.GetManagedByLabelSet(cp)
+	// TODO: This performs a migration from the old managedBy label to the new one.
+	// After several versions of soak time we can remove handling the legeacy label set.
+	// PR that introduced the new set of labels: https://github.com/Kong/gateway-operator/pull/259
+	// PR: that introduced the migration: https://github.com/Kong/gateway-operator/pull/369
+	clusterRolesLegacy, err := k8sutils.ListClusterRoles(
+		ctx,
+		r.Client,
+		client.MatchingLabels(k8sutils.GetLegacyManagedByLabelSet(cp)),
+	)
+	if err != nil {
+		return false, nil, err
+	}
+
 	clusterRoles, err := k8sutils.ListClusterRoles(
 		ctx,
 		r.Client,
-		client.MatchingLabels(managedByLabelSet),
+		client.MatchingLabels(k8sutils.GetManagedByLabelSet(cp)),
 	)
 	if err != nil {
 		return false, nil, err
 	}
 
+	// NOTE: This is a temporary workaround to handle the migration from the old managedBy label to the new one.
+	// The reason for this is because those label sets overlap.
+	for i, cr := range clusterRolesLegacy {
+		nn := client.ObjectKeyFromObject(&cr)
+		if lo.ContainsBy(clusterRoles, func(iterator rbacv1.ClusterRole) bool {
+			return client.ObjectKeyFromObject(&iterator) == nn
+		}) {
+			continue
+		}
+
+		clusterRoles = append(clusterRoles, clusterRolesLegacy[i])
+	}
+
 	count := len(clusterRoles)
 	if count > 1 {
 		if err := k8sreduce.ReduceClusterRoles(ctx, r.Client, clusterRoles); err != nil {
@@ -350,17 +375,41 @@ func (r *Reconciler) ensureClusterRoleBinding(
 ) (createdOrUpdate bool, crb *rbacv1.ClusterRoleBinding, err error) {
 	logger := log.GetLogger(ctx, "controlplane.ensureClusterRoleBinding", r.DevelopmentMode)
 
-	managedByLabelSet := k8sutils.GetManagedByLabelSet(cp)
+	// TODO: This performs a migration from the old managedBy label to the new one.
+	// After several versions of soak time we can remove handling the legeacy label set.
+	// PR that introduced the new set of labels: https://github.com/Kong/gateway-operator/pull/259
+	// PR: that introduced the migration: https://github.com/Kong/gateway-operator/pull/369
+	clusterRoleBindingsLegacy, err := k8sutils.ListClusterRoleBindings(
+		ctx,
+		r.Client,
+		client.MatchingLabels(k8sutils.GetLegacyManagedByLabelSet(cp)),
+	)
+	if err != nil {
+		return false, nil, err
+	}
 
 	clusterRoleBindings, err := k8sutils.ListClusterRoleBindings(
 		ctx,
 		r.Client,
-		client.MatchingLabels(managedByLabelSet),
+		client.MatchingLabels(k8sutils.GetManagedByLabelSet(cp)),
 	)
 	if err != nil {
 		return false, nil, err
 	}
 
+	// NOTE: This is a temporary workaround to handle the migration from the old managedBy label to the new one.
+	// The reason for this is because those label sets overlap.
+	for i, crb := range clusterRoleBindingsLegacy {
+		nn := client.ObjectKeyFromObject(&crb)
+		if lo.ContainsBy(clusterRoleBindings, func(iterator rbacv1.ClusterRoleBinding) bool {
+			return client.ObjectKeyFromObject(&iterator) == nn
+		}) {
+			continue
+		}
+
+		clusterRoleBindings = append(clusterRoleBindings, clusterRoleBindingsLegacy[i])
+	}
+
 	count := len(clusterRoleBindings)
 	if count > 1 {
 		if err := k8sreduce.ReduceClusterRoleBindings(ctx, r.Client, clusterRoleBindings); err != nil {
@@ -503,24 +552,38 @@ func (r *Reconciler) ensureOwnedClusterRolesDeleted(
 	ctx context.Context,
 	cp *operatorv1beta1.ControlPlane,
 ) (deletions bool, err error) {
-	managedByLabelSet := k8sutils.GetManagedByLabelSet(cp)
+	clusterRolesLegacy, err := k8sutils.ListClusterRoles(
+		ctx,
+		r.Client,
+		client.MatchingLabels(k8sutils.GetLegacyManagedByLabelSet(cp)),
+	)
+	if err != nil {
+		return false, err
+	}
 
 	clusterRoles, err := k8sutils.ListClusterRoles(
-		ctx, r.Client,
-		client.MatchingLabels(managedByLabelSet),
+		ctx,
+		r.Client,
+		client.MatchingLabels(k8sutils.GetManagedByLabelSet(cp)),
 	)
 	if err != nil {
 		return false, err
 	}
 
+	clusterRoles = append(clusterRoles, clusterRolesLegacy...)
+
 	var (
 		deleted bool
 		errs    []error
 	)
 	for i := range clusterRoles {
 		err = r.Client.Delete(ctx, &clusterRoles[i])
-		if err != nil && !k8serrors.IsNotFound(err) {
+		if err != nil {
+			if k8serrors.IsNotFound(err) {
+				continue
+			}
 			errs = append(errs, err)
+			continue
 		}
 		deleted = true
 	}
@@ -535,24 +598,38 @@ func (r *Reconciler) ensureOwnedClusterRoleBindingsDeleted(
 	ctx context.Context,
 	cp *operatorv1beta1.ControlPlane,
 ) (deletions bool, err error) {
-	managedByLabelSet := k8sutils.GetManagedByLabelSet(cp)
+	clusterRoleBindingsLegacy, err := k8sutils.ListClusterRoleBindings(
+		ctx,
+		r.Client,
+		client.MatchingLabels(k8sutils.GetLegacyManagedByLabelSet(cp)),
+	)
+	if err != nil {
+		return false, err
+	}
 
 	clusterRoleBindings, err := k8sutils.ListClusterRoleBindings(
-		ctx, r.Client,
-		client.MatchingLabels(managedByLabelSet),
+		ctx,
+		r.Client,
+		client.MatchingLabels(k8sutils.GetManagedByLabelSet(cp)),
 	)
 	if err != nil {
 		return false, err
 	}
 
+	clusterRoleBindings = append(clusterRoleBindings, clusterRoleBindingsLegacy...)
+
 	var (
 		deleted bool
 		errs    []error
 	)
 	for i := range clusterRoleBindings {
 		err = r.Client.Delete(ctx, &clusterRoleBindings[i])
-		if err != nil && !k8serrors.IsNotFound(err) {
+		if err != nil {
+			if k8serrors.IsNotFound(err) {
+				continue
+			}
 			errs = append(errs, err)
+			continue
 		}
 		deleted = true
 	}
@@ -561,26 +638,44 @@ func (r *Reconciler) ensureOwnedClusterRoleBindingsDeleted(
 }
 
 func (r *Reconciler) ensureOwnedValidatingWebhookConfigurationDeleted(ctx context.Context,
-	cp *operatorv1beta1.ControlPlane) (deletions bool, err error) {
-	managedByLabelSet := k8sutils.GetManagedByLabelSet(cp)
+	cp *operatorv1beta1.ControlPlane,
+) (deletions bool, err error) {
+	// TODO: This performs a migration from the old managedBy label to the new one.
+	// After several versions of soak time we can remove handling the legeacy label set.
+	// PR that introduced the new set of labels: https://github.com/Kong/gateway-operator/pull/259
+	// PR: that introduced the migration: https://github.com/Kong/gateway-operator/pull/369
+	validatingWebhookConfigurationsLegacy, err := k8sutils.ListValidatingWebhookConfigurations(
+		ctx,
+		r.Client,
+		client.MatchingLabels(k8sutils.GetLegacyManagedByLabelSet(cp)),
+	)
+	if err != nil {
+		return false, fmt.Errorf("failed listing webhook configurations for owner: %w", err)
+	}
 
 	validatingWebhookConfigurations, err := k8sutils.ListValidatingWebhookConfigurations(
 		ctx,
 		r.Client,
-		client.MatchingLabels(managedByLabelSet),
+		client.MatchingLabels(k8sutils.GetManagedByLabelSet(cp)),
 	)
 	if err != nil {
 		return false, fmt.Errorf("failed listing webhook configurations for owner: %w", err)
 	}
 
+	validatingWebhookConfigurations = append(validatingWebhookConfigurations, validatingWebhookConfigurationsLegacy...)
+
 	var (
 		deleted bool
 		errs    []error
 	)
 	for i := range validatingWebhookConfigurations {
 		err = r.Client.Delete(ctx, &validatingWebhookConfigurations[i])
-		if err != nil && !k8serrors.IsNotFound(err) {
+		if err != nil {
+			if k8serrors.IsNotFound(err) {
+				continue
+			}
 			errs = append(errs, err)
+			continue
 		}
 		deleted = true
 	}
@@ -671,15 +766,42 @@ func (r *Reconciler) ensureValidatingWebhookConfiguration(
 ) (op.Result, error) {
 	logger := log.GetLogger(ctx, "controlplane.ensureValidatingWebhookConfiguration", r.DevelopmentMode)
 
-	managedByLabelSet := k8sutils.GetManagedByLabelSet(cp)
+	// TODO: This performs a migration from the old managedBy label to the new one.
+	// After several versions of soak time we can remove handling the legeacy label set.
+	// PR that introduced the new set of labels: https://github.com/Kong/gateway-operator/pull/259
+	// PR: that introduced the migration: https://github.com/Kong/gateway-operator/pull/369
+	validatingWebhookConfigurationsLegacy, err := k8sutils.ListValidatingWebhookConfigurationsForOwner(
+		ctx,
+		r.Client,
+		cp.GetUID(),
+		// NOTE: this uses only the 1 label to find the legacy webhook configurations not the label set
+		// because app:<name> is not set on ValidatingWebhookConfiguration.
+		client.MatchingLabels(k8sutils.GetLegacyManagedByLabel(cp)),
+	)
+	if err != nil {
+		return op.Noop, fmt.Errorf("failed listing webhook configurations for owner: %w", err)
+	}
 
 	validatingWebhookConfigurations, err := k8sutils.ListValidatingWebhookConfigurations(
 		ctx,
 		r.Client,
-		client.MatchingLabels(managedByLabelSet),
+		client.MatchingLabels(k8sutils.GetManagedByLabelSet(cp)),
 	)
 	if err != nil {
-		return op.Noop, err
+		return op.Noop, fmt.Errorf("failed listing webhook configurations for owner: %w", err)
+	}
+
+	// NOTE: This is a temporary workaround to handle the migration from the old managedBy label to the new one.
+	// The reason for this is because those label sets overlap.
+	for i, vwc := range validatingWebhookConfigurationsLegacy {
+		nn := client.ObjectKeyFromObject(&vwc)
+		if lo.ContainsBy(validatingWebhookConfigurations, func(iterator admregv1.ValidatingWebhookConfiguration) bool {
+			return client.ObjectKeyFromObject(&iterator) == nn
+		}) {
+			continue
+		}
+
+		validatingWebhookConfigurations = append(validatingWebhookConfigurations, validatingWebhookConfigurationsLegacy[i])
 	}
 
 	count := len(validatingWebhookConfigurations)
@@ -736,7 +858,8 @@ func (r *Reconciler) ensureValidatingWebhookConfiguration(
 
 		updated, webhookConfiguration.ObjectMeta = k8sutils.EnsureObjectMetaIsUpdated(webhookConfiguration.ObjectMeta, generatedWebhookConfiguration.ObjectMeta)
 
-		if !cmp.Equal(webhookConfiguration.Webhooks, generatedWebhookConfiguration.Webhooks) {
+		if !cmp.Equal(webhookConfiguration.Webhooks, generatedWebhookConfiguration.Webhooks) ||
+			!cmp.Equal(webhookConfiguration.Labels, generatedWebhookConfiguration.Labels) {
 			webhookConfiguration.Webhooks = generatedWebhookConfiguration.Webhooks
 			updated = true
 		}

controller/controlplane/controller_watch.go

@@ -17,6 +17,7 @@ import (
 	operatorerrors "github.com/kong/gateway-operator/internal/errors"
 	"github.com/kong/gateway-operator/internal/utils/index"
 	"github.com/kong/gateway-operator/pkg/consts"
+	k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes"
 )
 
 // -----------------------------------------------------------------------------
@@ -74,18 +75,17 @@ func (r *Reconciler) validatingWebhookConfigurationHasControlPlaneOwner(obj clie
 // ClusterScopedObjHasControlPlaneOwner checks if the cluster-scoped object has a control plane owner.
 // The check is performed through the managed-by-name label.
 func (r *Reconciler) ClusterScopedObjHasControlPlaneOwner(ctx context.Context, obj client.Object) bool {
-	controlplaneList := &operatorv1beta1.ControlPlaneList{}
-	if err := r.Client.List(ctx, controlplaneList); err != nil {
+	var controlplaneList operatorv1beta1.ControlPlaneList
+	if err := r.Client.List(ctx, &controlplaneList); err != nil {
 		// filtering here is just an optimization. If we fail here it's most likely because of some failure
 		// of the Kubernetes API and it's technically better to enqueue the object
 		// than to drop it for eventual consistency during cluster outages.
 		log.FromContext(ctx).Error(err, "could not list controlplanes in map func")
 		return true
 	}
 
-	for _, controlplane := range controlplaneList.Items {
-		// When resources are cluster-scoped, the owner is set through a label.
-		if obj.GetLabels()[consts.GatewayOperatorManagedByNameLabel] == controlplane.Name {
+	for i := range controlplaneList.Items {
+		if objectIsOwnedByControlPlane(obj, &controlplaneList.Items[i]) {
 			return true
 		}
 	}
@@ -146,23 +146,48 @@ func (r *Reconciler) getControlPlaneRequestFromManagedByNameLabel(ctx context.Co
 		return
 	}
 
-	for _, controlplane := range controlplanes.Items {
-		// For cluster-scoped resources, the owner is set through a label.
-		if obj.GetLabels()[consts.GatewayOperatorManagedByNameLabel] == controlplane.Name {
-			return []reconcile.Request{
-				{
-					NamespacedName: types.NamespacedName{
-						Namespace: controlplane.Namespace,
-						Name:      controlplane.Name,
-					},
+	for i := range controlplanes.Items {
+		if !objectIsOwnedByControlPlane(obj, &controlplanes.Items[i]) {
+			continue
+		}
+
+		return []reconcile.Request{
+			{
+				NamespacedName: types.NamespacedName{
+					Namespace: controlplanes.Items[i].Namespace,
+					Name:      controlplanes.Items[i].Name,
 				},
-			}
+			},
 		}
 	}
 
 	return
 }
 
+// objectIsOwnedByControlPlane checks if the object is owned by the control plane.
+//
+// NOTE: We are using the managed-by-name label to identify the owner of the resource
+// To keep backward compatibility, we also check the owner reference which
+// are not used anymore for cluster-scoped resources since that's considered
+// an error.
+func objectIsOwnedByControlPlane(obj client.Object, cp *operatorv1beta1.ControlPlane) bool {
+	if k8sutils.IsOwnedByRefUID(obj, cp.GetUID()) {
+		return true
+	}
+
+	if labels := obj.GetLabels(); len(labels) > 0 {
+		if labels[consts.GatewayOperatorManagedByNameLabel] == cp.Name {
+			if obj.GetNamespace() != "" {
+				return cp.GetNamespace() == obj.GetNamespace()
+			} else {
+				return true
+			}
+		}
+	}
+
+	return false
+}
+
 func (r *Reconciler) getControlPlanesFromDataPlaneDeployment(ctx context.Context, obj client.Object) (recs []reconcile.Request) {
 	deployment, ok := obj.(*appsv1.Deployment)
 	if !ok {