Clean up organization memberships when users are deleted (#544)

## Summary

- Adds a finalizer to the UserController that deletes all
OrganizationMemberships referencing a user before allowing the User
object to be removed
- Extends the OrganizationMembership validation webhook to allow
last-owner membership deletion when the referenced User is being deleted
(verified via direct API server read, not cache)
- Fixes the Organization webhook and UserInvitation controller to set
correct User ownerReferences on OrganizationMemberships
- Adds a two-pass self-delete in the OrganizationMembership controller
for existing orphaned memberships where the User no longer exists

## Context

When a User was deleted, OrganizationMembership resources referencing
that user were not cleaned up because they had no User ownerReference
(webhook creation path) or had a malformed one (invitation path used
`.Group` instead of `.String()` for APIVersion). The orphaned
memberships left their owned PolicyBindings stuck in
`SubjectValidationFailed` state.

Closes #536

## Test plan

- [x] Webhook tests verify deletion is allowed when User has
DeletionTimestamp
- [x] Webhook tests verify deletion is allowed when User is already gone
- [x] Webhook tests verify last-owner guard still blocks when User is
active
- [x] All existing unit tests pass
- [x] Chainsaw e2e test validates full lifecycle: create user + org →
verify membership → delete user → verify cleanup
- [x] e2e test passes in CI

> [!NOTE]
> The CI workflow reports a failure due to pre-existing e2e test issues
on `main` (`note-multicluster-subject`,
`clusternote-multicluster-subject`, `crm-note-contact-lifecycle`). These
are unrelated to this PR and are being resolved in #549.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Author: scotwells

config/controller-manager/overlays/core-control-plane/rbac/role.yaml

@@ -94,6 +94,7 @@ rules:
   resources:
   - groups/finalizers
   - platforminvitations/status
+  - userinvitations/finalizers
   - userinvitations/status
   verbs:
   - update
@@ -149,6 +150,7 @@ rules:
   - delete
   - get
   - list
+  - patch
   - update
   - watch
 - apiGroups:
@@ -295,6 +297,7 @@ rules:
   - organizationmemberships
   verbs:
   - create
+  - delete
   - get
   - list
   - watch

internal/controllers/iam/user_controller.go

@@ -6,20 +6,24 @@ import (
 	"strings"
 
 	iamv1alpha1 "go.miloapis.com/milo/pkg/apis/iam/v1alpha1"
+	resourcemanagerv1alpha1 "go.miloapis.com/milo/pkg/apis/resourcemanager/v1alpha1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
 const (
-	userFinalizerKey                = "iam.miloapis.com/user"
+	// userMembershipCleanupFinalizer ensures OrganizationMembership resources are
+	// deleted before the User object is removed from the API server.
+	userMembershipCleanupFinalizer  = "iam.miloapis.com/user-membership-cleanup"
 	userReadyConditionType          = "Ready"
 	platformAccessApprovalIndexKey  = "iam.miloapis.com/platformaccessapprovalkey"
 	platformAccessRejectionIndexKey = "iam.miloapis.com/platformaccessrejectionkey"
@@ -44,6 +48,7 @@ type UserController struct {
 // +kubebuilder:rbac:groups=iam.miloapis.com,resources=userpreferences,verbs=get;list;watch;update;patch
 // +kubebuilder:rbac:groups=iam.miloapis.com,resources=platformaccessapprovals,verbs=get;list;watch
 // +kubebuilder:rbac:groups=iam.miloapis.com,resources=platformaccessrejections,verbs=get;list;watch
+// +kubebuilder:rbac:groups=resourcemanager.miloapis.com,resources=organizationmemberships,verbs=list;delete
 
 // Reconcile is the main reconciliation loop for the UserController.
 func (r *UserController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
@@ -59,12 +64,31 @@ func (r *UserController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 	}
 	log.Info("reconciling User", "user", user.Name)
 
-	// Stop reconciling if deletion in progress.
+	// When the user is being deleted, clean up OrganizationMembership resources
+	// before the object is removed.
 	if !user.DeletionTimestamp.IsZero() {
-		log.Info("User is being deleted, skipping reconciliation", "user", user.Name)
+		if controllerutil.ContainsFinalizer(user, userMembershipCleanupFinalizer) {
+			if err := r.cleanupOrganizationMemberships(ctx, user); err != nil {
+				log.Error(err, "failed to clean up OrganizationMemberships during user deletion")
+				return ctrl.Result{}, err
+			}
+			controllerutil.RemoveFinalizer(user, userMembershipCleanupFinalizer)
+			if err := r.Client.Update(ctx, user); err != nil {
+				return ctrl.Result{}, fmt.Errorf("failed to remove membership cleanup finalizer: %w", err)
+			}
+		}
 		return ctrl.Result{}, nil
 	}
 
+	// Ensure the membership-cleanup finalizer is present on every active User so
+	// that OrganizationMemberships are deleted before the User is removed.
+	if !controllerutil.ContainsFinalizer(user, userMembershipCleanupFinalizer) {
+		controllerutil.AddFinalizer(user, userMembershipCleanupFinalizer)
+		if err := r.Client.Update(ctx, user); err != nil {
+			return ctrl.Result{}, fmt.Errorf("failed to add membership cleanup finalizer: %w", err)
+		}
+	}
+
 	// Ensure owner references are set on PolicyBinding and UserPreference resources
 	if err := r.ensureOwnerReferences(ctx, user); err != nil {
 		log.Error(err, "Failed to ensure owner references")
@@ -237,6 +261,33 @@ func (r *UserController) ensureOwnerReferences(ctx context.Context, user *iamv1a
 	return nil
 }
 
+// cleanupOrganizationMemberships deletes all OrganizationMembership resources that
+// reference the given user. This is called when a user is being deleted so that
+// memberships (including last-owner memberships) are removed before the User
+// object is garbage-collected.
+//
+// Depends on the "spec.userRef.name" field index registered by
+// OrganizationMembershipController.SetupWithManager. Both controllers share the
+// same manager, so the index is available when this function runs.
+func (r *UserController) cleanupOrganizationMemberships(ctx context.Context, user *iamv1alpha1.User) error {
+	log := log.FromContext(ctx).WithName("cleanup-organization-memberships")
+
+	var membershipList resourcemanagerv1alpha1.OrganizationMembershipList
+	if err := r.Client.List(ctx, &membershipList, client.MatchingFields{"spec.userRef.name": user.Name}); err != nil {
+		return fmt.Errorf("failed to list OrganizationMemberships for user %s: %w", user.Name, err)
+	}
+
+	for i := range membershipList.Items {
+		membership := &membershipList.Items[i]
+		log.Info("deleting OrganizationMembership for deleted user", "membership", membership.Name, "namespace", membership.Namespace)
+		if err := r.Client.Delete(ctx, membership); err != nil && !apierrors.IsNotFound(err) {
+			return fmt.Errorf("failed to delete OrganizationMembership %s/%s: %w", membership.Namespace, membership.Name, err)
+		}
+	}
+
+	return nil
+}
+
 // hasOwnerReference checks if the owner reference already exists
 func hasOwnerReference(refs []metav1.OwnerReference, ref metav1.OwnerReference) bool {
 	for _, r := range refs {

internal/controllers/iam/userinvitation_controller.go

@@ -555,7 +555,7 @@ func (r *UserInvitationController) createOrganizationMembership(ctx context.Cont
 			Namespace: fmt.Sprintf("organization-%s", ui.Spec.OrganizationRef.Name),
 			OwnerReferences: []metav1.OwnerReference{
 				{
-					APIVersion: iamv1alpha1.SchemeGroupVersion.Group,
+					APIVersion: iamv1alpha1.SchemeGroupVersion.String(),
 					Kind:       "User",
 					Name:       user.GetName(),
 					UID:        user.GetUID(),

internal/controllers/resourcemanager/organization_membership_controller.go

@@ -131,6 +131,22 @@ func (r *OrganizationMembershipController) Reconcile(ctx context.Context, req ct
 	if err := r.Client.Get(ctx, userKey, &user); err != nil {
 		if apierrors.IsNotFound(err) {
 			logger.Info("referenced user not found", "user", organizationMembership.Spec.UserRef.Name)
+
+			// Two-pass self-delete: if we already set UserNotFound on a previous
+			// reconcile, delete the membership now so it does not linger after
+			// the owning User is gone. The second reconcile is triggered by the
+			// status condition update below, which re-enqueues the membership.
+			existingCondition := apimeta.FindStatusCondition(organizationMembership.Status.Conditions, OrganizationMembershipReady)
+			if existingCondition != nil && existingCondition.Reason == UserNotFoundReason {
+				logger.Info("deleting OrganizationMembership because referenced user no longer exists",
+					"membership", organizationMembership.Name,
+					"user", organizationMembership.Spec.UserRef.Name)
+				if err := r.Client.Delete(ctx, &organizationMembership); err != nil && !apierrors.IsNotFound(err) {
+					return ctrl.Result{}, fmt.Errorf("failed to self-delete organization membership: %w", err)
+				}
+				return ctrl.Result{}, nil
+			}
+
 			readyCondition.Status = metav1.ConditionFalse
 			readyCondition.Reason = UserNotFoundReason
 			readyCondition.Message = fmt.Sprintf("User '%s' does not exist. Please ensure the user name is correct and the user account has been created.", organizationMembership.Spec.UserRef.Name)

internal/webhooks/resourcemanager/v1alpha1/organization_webhook.go

@@ -181,6 +181,14 @@ func (v *OrganizationValidator) createOrganizationMembership(ctx context.Context
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      fmt.Sprintf("member-%s", user.Name),
 			Namespace: fmt.Sprintf("organization-%s", org.Name),
+			OwnerReferences: []metav1.OwnerReference{
+				{
+					APIVersion: iamv1alpha1.SchemeGroupVersion.String(),
+					Kind:       "User",
+					Name:       user.Name,
+					UID:        user.UID,
+				},
+			},
 		},
 		Spec: resourcemanagerv1alpha1.OrganizationMembershipSpec{
 			OrganizationRef: resourcemanagerv1alpha1.OrganizationReference{

internal/webhooks/resourcemanager/v1alpha1/organizationmembership_webhook.go

@@ -30,6 +30,7 @@ func SetupOrganizationMembershipWebhooksWithManager(mgr ctrl.Manager, organizati
 		For(&resourcemanagerv1alpha1.OrganizationMembership{}).
 		WithValidator(&OrganizationMembershipValidator{
 			client:             mgr.GetClient(),
+			apiReader:          mgr.GetAPIReader(),
 			ownerRoleName:      organizationOwnerRoleName,
 			ownerRoleNamespace: organizationOwnerRoleNamespace,
 		}).
@@ -39,6 +40,7 @@ func SetupOrganizationMembershipWebhooksWithManager(mgr ctrl.Manager, organizati
 // OrganizationMembershipValidator validates OrganizationMemberships
 type OrganizationMembershipValidator struct {
 	client             client.Client
+	apiReader          client.Reader
 	decoder            admission.Decoder
 	ownerRoleName      string
 	ownerRoleNamespace string
@@ -85,6 +87,13 @@ func (v *OrganizationMembershipValidator) ValidateDelete(ctx context.Context, ob
 		return nil, nil
 	}
 
+	// Allow deletion when the referenced user is itself being deleted — the
+	// UserController is responsible for cleaning up orphaned memberships and
+	// must not be blocked by the last-owner guard.
+	if v.allowDeletionBecauseUserIsBeingDeleted(ctx, membership) {
+		return nil, nil
+	}
+
 	if v.allowOwnerDeletionDuringTeardown(ctx, membership) {
 		return nil, nil
 	}
@@ -141,6 +150,33 @@ func (v *OrganizationMembershipValidator) allowOwnerDeletionDuringTeardown(ctx c
 	return organization.DeletionTimestamp != nil
 }
 
+// allowDeletionBecauseUserIsBeingDeleted returns true when the membership should be
+// allowed to be deleted because the referenced user has a non-zero DeletionTimestamp.
+// The UserController adds a finalizer and drives membership cleanup; we must not
+// block it with the last-owner guard.
+//
+// Uses the direct API reader (not the informer cache) to avoid stale reads that
+// could incorrectly bypass the last-owner business invariant.
+func (v *OrganizationMembershipValidator) allowDeletionBecauseUserIsBeingDeleted(ctx context.Context, membership *resourcemanagerv1alpha1.OrganizationMembership) bool {
+	userName := membership.Spec.UserRef.Name
+	if userName == "" {
+		return false
+	}
+
+	var user iamv1alpha1.User
+	if err := v.apiReader.Get(ctx, client.ObjectKey{Name: userName}, &user); err != nil {
+		if apierrors.IsNotFound(err) {
+			// User is already gone — allow the membership deletion.
+			return true
+		}
+		organizationmembershiplog.Error(err, "failed to fetch user while validating delete",
+			"user", userName)
+		return false
+	}
+
+	return user.DeletionTimestamp != nil
+}
+
 // isNamespaceTerminating returns true if the namespace is terminating or already deleted.
 func (v *OrganizationMembershipValidator) isNamespaceTerminating(ctx context.Context, namespace string) bool {
 	if namespace == "" {