fix: fixing bug with resources deletion (authPolicy and TRLP) after removing model ref (#611)

JIRA - https://redhat.atlassian.net/browse/RHOAIENG-55153

Both reconcilers only iterated over current spec.modelRefs. When a model
was removed from the spec, its generated AuthPolicy /
TokenRateLimitPolicy was never cleaned up - leaving stale authpolicy
and/or trlp on the cluster.

The fix adds a cleanup step after the main reconcile loop. It lists all
managed generated policies, checks their annotation to see if this CR
contributed, and deletes any whose model is no longer in spec.modelRefs.
The existing watch mechanism then triggers remaining CRs to rebuild the
aggregated policy. The same cleanup runs in the finalizer deletion path
as well.

Four unit tests cover single-policy removal and multi-policy aggregation
for both controllers. All existing tests pass. The fix was verified on a
live OC cluster with three scenarios: single AuthPolicy removal, single
TRLP removal, and two-policy aggregation rebuild. All passed.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
* Automatic cleanup of orphaned authentication policies and
token-rate-limit policies when model references are removed.

* **Changes**
* Authentication now uses API-key validation exclusively; Kubernetes
token path removed.
* Authorization rules converted to pattern-matching over API-key
validation.
* Response headers limited to API-key validation fields; subscription
header injection removed.

* **Tests**
* Added reconciliation tests covering removal and aggregation behavior
for auth and TRLP resources.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Signed-off-by: Dmytro Zaharnytskyi <zdmytro@redhat.com>

Author: dmytro-zaharnytskyi

maas-controller/pkg/controller/maas/maasauthpolicy_controller.go

@@ -21,6 +21,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"slices"
 	"sort"
 	"strings"
 
@@ -561,9 +562,56 @@ allow {
 			}
 		}
 	}
+	if err := r.cleanupStaleAuthPolicies(ctx, log, policy); err != nil {
+		return nil, err
+	}
+
 	return refs, nil
 }
 
+// cleanupStaleAuthPolicies deletes aggregated AuthPolicies for models that this
+// policy previously contributed to but no longer references in spec.modelRefs.
+// Generated AuthPolicies track contributing policies in the
+// "maas.opendatahub.io/auth-policies" annotation (namespace-qualified: "ns/name").
+func (r *MaaSAuthPolicyReconciler) cleanupStaleAuthPolicies(ctx context.Context, log logr.Logger, policy *maasv1alpha1.MaaSAuthPolicy) error {
+	currentModels := make(map[string]bool, len(policy.Spec.ModelRefs))
+	for _, ref := range policy.Spec.ModelRefs {
+		currentModels[ref.Namespace+"/"+ref.Name] = true
+	}
+
+	allManaged := &unstructured.UnstructuredList{}
+	allManaged.SetGroupVersionKind(schema.GroupVersionKind{Group: "kuadrant.io", Version: "v1", Kind: "AuthPolicyList"})
+	if err := r.List(ctx, allManaged, client.MatchingLabels{
+		"app.kubernetes.io/managed-by": "maas-controller",
+		"app.kubernetes.io/part-of":    "maas-auth-policy",
+	}); err != nil {
+		if apierrors.IsNotFound(err) || apimeta.IsNoMatchError(err) {
+			return nil
+		}
+		return fmt.Errorf("failed to list managed AuthPolicies for stale cleanup: %w", err)
+	}
+
+	for i := range allManaged.Items {
+		ap := &allManaged.Items[i]
+		modelName := ap.GetLabels()["maas.opendatahub.io/model"]
+		if modelName == "" {
+			continue
+		}
+		modelKey := ap.GetNamespace() + "/" + modelName
+		if currentModels[modelKey] {
+			continue
+		}
+		if !slices.Contains(strings.Split(ap.GetAnnotations()["maas.opendatahub.io/auth-policies"], ","), policy.Name) {
+			continue
+		}
+		log.Info("Cleaning up stale AuthPolicy for removed modelRef", "model", modelKey, "authPolicy", ap.GetName())
+		if err := r.deleteModelAuthPolicy(ctx, log, ap.GetNamespace(), modelName); err != nil {
+			return fmt.Errorf("failed to clean up stale AuthPolicy for removed model %s: %w", modelKey, err)
+		}
+	}
+	return nil
+}
+
 // deleteModelAuthPolicy deletes the aggregated AuthPolicy for a model in the given namespace.
 func (r *MaaSAuthPolicyReconciler) deleteModelAuthPolicy(ctx context.Context, log logr.Logger, modelNamespace, modelName string) error {
 	// Always delete the aggregated AuthPolicy so remaining MaaSAuthPolicies rebuild it
@@ -605,6 +653,11 @@ func (r *MaaSAuthPolicyReconciler) handleDeletion(ctx context.Context, log logr.
 				return ctrl.Result{}, err
 			}
 		}
+		// Also clean up stale AuthPolicies from modelRefs that were removed
+		// before the CR was deleted (edge case: edit + delete before reconcile).
+		if err := r.cleanupStaleAuthPolicies(ctx, log, policy); err != nil {
+			return ctrl.Result{}, err
+		}
 		controllerutil.RemoveFinalizer(policy, maasAuthPolicyFinalizer)
 		if err := r.Update(ctx, policy); err != nil {
 			return ctrl.Result{}, err

maas-controller/pkg/controller/maas/maasauthpolicy_controller_test.go

@@ -271,6 +271,179 @@ func TestMaaSAuthPolicyReconciler_DeleteAnnotation(t *testing.T) {
 		})
 	}
 }
+// TestMaaSAuthPolicyReconciler_RemoveModelRef verifies that removing a modelRef from
+// a MaaSAuthPolicy deletes the aggregated AuthPolicy for the removed model while
+// keeping the AuthPolicy for the remaining model intact.
+func TestMaaSAuthPolicyReconciler_RemoveModelRef(t *testing.T) {
+	const (
+		modelA         = "model-a"
+		modelB         = "model-b"
+		namespace      = "default"
+		httpRouteA     = "maas-model-" + modelA
+		httpRouteB     = "maas-model-" + modelB
+		authPolicyA    = "maas-auth-" + modelA
+		authPolicyB    = "maas-auth-" + modelB
+		maasPolicyName = "policy-1"
+	)
+
+	modelRefA := newMaaSModelRef(modelA, namespace, "ExternalModel", modelA)
+	modelRefB := newMaaSModelRef(modelB, namespace, "ExternalModel", modelB)
+	routeA := newHTTPRoute(httpRouteA, namespace)
+	routeB := newHTTPRoute(httpRouteB, namespace)
+
+	maasPolicy := newMaaSAuthPolicy(maasPolicyName, namespace, "team-a",
+		maasv1alpha1.ModelRef{Name: modelA, Namespace: namespace},
+		maasv1alpha1.ModelRef{Name: modelB, Namespace: namespace},
+	)
+
+	c := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithRESTMapper(testRESTMapper()).
+		WithObjects(modelRefA, modelRefB, routeA, routeB, maasPolicy).
+		WithStatusSubresource(&maasv1alpha1.MaaSAuthPolicy{}).
+		Build()
+
+	r := &MaaSAuthPolicyReconciler{Client: c, Scheme: scheme, MaaSAPINamespace: "maas-system"}
+	ctx := context.Background()
+	req := ctrl.Request{NamespacedName: types.NamespacedName{Name: maasPolicyName, Namespace: namespace}}
+
+	if _, err := r.Reconcile(ctx, req); err != nil {
+		t.Fatalf("Initial reconcile: %v", err)
+	}
+
+	// Both AuthPolicies should exist
+	for _, name := range []string{authPolicyA, authPolicyB} {
+		ap := &unstructured.Unstructured{}
+		ap.SetGroupVersionKind(schema.GroupVersionKind{Group: "kuadrant.io", Version: "v1", Kind: "AuthPolicy"})
+		if err := c.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, ap); err != nil {
+			t.Fatalf("AuthPolicy %q not found after initial reconcile: %v", name, err)
+		}
+	}
+
+	// Remove model B from the policy
+	freshPolicy := &maasv1alpha1.MaaSAuthPolicy{}
+	if err := c.Get(ctx, types.NamespacedName{Name: maasPolicyName, Namespace: namespace}, freshPolicy); err != nil {
+		t.Fatalf("Get MaaSAuthPolicy: %v", err)
+	}
+	freshPolicy.Spec.ModelRefs = []maasv1alpha1.ModelRef{{Name: modelA, Namespace: namespace}}
+	if err := c.Update(ctx, freshPolicy); err != nil {
+		t.Fatalf("Update MaaSAuthPolicy: %v", err)
+	}
+
+	if _, err := r.Reconcile(ctx, req); err != nil {
+		t.Fatalf("Reconcile after removing modelRef: %v", err)
+	}
+
+	// AuthPolicy for model A should still exist
+	apA := &unstructured.Unstructured{}
+	apA.SetGroupVersionKind(schema.GroupVersionKind{Group: "kuadrant.io", Version: "v1", Kind: "AuthPolicy"})
+	if err := c.Get(ctx, types.NamespacedName{Name: authPolicyA, Namespace: namespace}, apA); err != nil {
+		t.Errorf("AuthPolicy for model A should still exist: %v", err)
+	}
+
+	// AuthPolicy for model B should be DELETED
+	apB := &unstructured.Unstructured{}
+	apB.SetGroupVersionKind(schema.GroupVersionKind{Group: "kuadrant.io", Version: "v1", Kind: "AuthPolicy"})
+	err := c.Get(ctx, types.NamespacedName{Name: authPolicyB, Namespace: namespace}, apB)
+	if !apierrors.IsNotFound(err) {
+		t.Errorf("AuthPolicy for model B should be deleted after removing modelRef, but got: %v", err)
+	}
+}
+
+// TestMaaSAuthPolicyReconciler_RemoveModelRef_Aggregation verifies that when one policy
+// drops a model that another policy still references, the aggregated AuthPolicy is deleted
+// (forcing a rebuild by the remaining policy) without affecting unrelated models.
+func TestMaaSAuthPolicyReconciler_RemoveModelRef_Aggregation(t *testing.T) {
+	const (
+		modelA      = "model-a"
+		modelB      = "model-b"
+		namespace   = "default"
+		httpRouteA  = "maas-model-" + modelA
+		httpRouteB  = "maas-model-" + modelB
+		authPolicyB = "maas-auth-" + modelB
+	)
+
+	modelRefA := newMaaSModelRef(modelA, namespace, "ExternalModel", modelA)
+	modelRefB := newMaaSModelRef(modelB, namespace, "ExternalModel", modelB)
+	routeA := newHTTPRoute(httpRouteA, namespace)
+	routeB := newHTTPRoute(httpRouteB, namespace)
+
+	// ap1 references [A, B], ap2 references [B]
+	ap1 := newMaaSAuthPolicy("ap1", namespace, "team-1",
+		maasv1alpha1.ModelRef{Name: modelA, Namespace: namespace},
+		maasv1alpha1.ModelRef{Name: modelB, Namespace: namespace},
+	)
+	ap2 := newMaaSAuthPolicy("ap2", namespace, "team-2",
+		maasv1alpha1.ModelRef{Name: modelB, Namespace: namespace},
+	)
+
+	c := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithRESTMapper(testRESTMapper()).
+		WithObjects(modelRefA, modelRefB, routeA, routeB, ap1, ap2).
+		WithStatusSubresource(&maasv1alpha1.MaaSAuthPolicy{}).
+		Build()
+
+	r := &MaaSAuthPolicyReconciler{Client: c, Scheme: scheme, MaaSAPINamespace: "maas-system"}
+	ctx := context.Background()
+
+	// Reconcile both policies to create aggregated AuthPolicies
+	req1 := ctrl.Request{NamespacedName: types.NamespacedName{Name: "ap1", Namespace: namespace}}
+	req2 := ctrl.Request{NamespacedName: types.NamespacedName{Name: "ap2", Namespace: namespace}}
+	if _, err := r.Reconcile(ctx, req1); err != nil {
+		t.Fatalf("Reconcile ap1: %v", err)
+	}
+	if _, err := r.Reconcile(ctx, req2); err != nil {
+		t.Fatalf("Reconcile ap2: %v", err)
+	}
+
+	// AuthPolicy for model B should exist with both policies contributing
+	apB := &unstructured.Unstructured{}
+	apB.SetGroupVersionKind(schema.GroupVersionKind{Group: "kuadrant.io", Version: "v1", Kind: "AuthPolicy"})
+	if err := c.Get(ctx, types.NamespacedName{Name: authPolicyB, Namespace: namespace}, apB); err != nil {
+		t.Fatalf("AuthPolicy for model B not found: %v", err)
+	}
+
+	// Remove model B from ap1 (ap2 still references it)
+	freshAP1 := &maasv1alpha1.MaaSAuthPolicy{}
+	if err := c.Get(ctx, types.NamespacedName{Name: "ap1", Namespace: namespace}, freshAP1); err != nil {
+		t.Fatalf("Get ap1: %v", err)
+	}
+	freshAP1.Spec.ModelRefs = []maasv1alpha1.ModelRef{{Name: modelA, Namespace: namespace}}
+	if err := c.Update(ctx, freshAP1); err != nil {
+		t.Fatalf("Update ap1: %v", err)
+	}
+
+	// Reconcile ap1 → should delete stale AuthPolicy for model B
+	if _, err := r.Reconcile(ctx, req1); err != nil {
+		t.Fatalf("Reconcile ap1 after removing modelRef: %v", err)
+	}
+
+	// AuthPolicy for model B should be deleted (stale from ap1's perspective)
+	apB = &unstructured.Unstructured{}
+	apB.SetGroupVersionKind(schema.GroupVersionKind{Group: "kuadrant.io", Version: "v1", Kind: "AuthPolicy"})
+	err := c.Get(ctx, types.NamespacedName{Name: authPolicyB, Namespace: namespace}, apB)
+	if !apierrors.IsNotFound(err) {
+		t.Fatalf("AuthPolicy for model B should be deleted after ap1 drops it, but got: %v", err)
+	}
+
+	// Reconcile ap2 → should recreate AuthPolicy for model B with only ap2's subjects
+	if _, err := r.Reconcile(ctx, req2); err != nil {
+		t.Fatalf("Reconcile ap2 after ap1 drop: %v", err)
+	}
+
+	apB = &unstructured.Unstructured{}
+	apB.SetGroupVersionKind(schema.GroupVersionKind{Group: "kuadrant.io", Version: "v1", Kind: "AuthPolicy"})
+	if err := c.Get(ctx, types.NamespacedName{Name: authPolicyB, Namespace: namespace}, apB); err != nil {
+		t.Errorf("AuthPolicy for model B should be rebuilt by ap2: %v", err)
+	}
+
+	// Verify only ap2 is in the annotation (ap1 no longer contributes)
+	ann := apB.GetAnnotations()["maas.opendatahub.io/auth-policies"]
+	if ann != "ap2" {
+		t.Errorf("AuthPolicy annotation = %q, want %q (only ap2 should contribute)", ann, "ap2")
+	}
+}
 
 // TestMaaSAuthPolicyReconciler_MultiplePoliciesDeletion verifies that when multiple
 // MaaSAuthPolicies reference the same model, deleting one does not delete the aggregated

maas-controller/pkg/controller/maas/maassubscription_controller.go

@@ -21,6 +21,7 @@ import (
 	"errors"
 	"fmt"
 	"sort"
+	"slices"
 	"strings"
 
 	"github.com/go-logr/logr"
@@ -128,6 +129,9 @@ func (r *MaaSSubscriptionReconciler) reconcileTokenRateLimitPolicies(ctx context
 			return err
 		}
 	}
+	if err := r.cleanupStaleTRLPs(ctx, log, subscription); err != nil {
+		return err
+	}
 	return nil
 }
 
@@ -346,6 +350,49 @@ func (r *MaaSSubscriptionReconciler) reconcileTRLPForModel(ctx context.Context,
 	return nil
 }
 
+// cleanupStaleTRLPs deletes aggregated TokenRateLimitPolicies for models that this
+// subscription previously contributed to but no longer references in spec.modelRefs.
+// Generated TRLPs track contributing subscriptions in the
+// "maas.opendatahub.io/subscriptions" annotation.
+func (r *MaaSSubscriptionReconciler) cleanupStaleTRLPs(ctx context.Context, log logr.Logger, subscription *maasv1alpha1.MaaSSubscription) error {
+	currentModels := make(map[string]bool, len(subscription.Spec.ModelRefs))
+	for _, ref := range subscription.Spec.ModelRefs {
+		currentModels[ref.Namespace+"/"+ref.Name] = true
+	}
+
+	allManaged := &unstructured.UnstructuredList{}
+	allManaged.SetGroupVersionKind(schema.GroupVersionKind{Group: "kuadrant.io", Version: "v1alpha1", Kind: "TokenRateLimitPolicyList"})
+	if err := r.List(ctx, allManaged, client.MatchingLabels{
+		"app.kubernetes.io/managed-by": "maas-controller",
+		"app.kubernetes.io/part-of":    "maas-subscription",
+	}); err != nil {
+		if apierrors.IsNotFound(err) || apimeta.IsNoMatchError(err) {
+			return nil
+		}
+		return fmt.Errorf("failed to list managed TokenRateLimitPolicies for stale cleanup: %w", err)
+	}
+
+	for i := range allManaged.Items {
+		trlp := &allManaged.Items[i]
+		modelName := trlp.GetLabels()["maas.opendatahub.io/model"]
+		if modelName == "" {
+			continue
+		}
+		modelKey := trlp.GetNamespace() + "/" + modelName
+		if currentModels[modelKey] {
+			continue
+		}
+		if !slices.Contains(strings.Split(trlp.GetAnnotations()["maas.opendatahub.io/subscriptions"], ","), subscription.Name) {
+			continue
+		}
+		log.Info("Cleaning up stale TokenRateLimitPolicy for removed modelRef", "model", modelKey, "trlp", trlp.GetName())
+		if err := r.deleteModelTRLP(ctx, log, trlp.GetNamespace(), modelName); err != nil {
+			return fmt.Errorf("failed to clean up stale TokenRateLimitPolicy for removed model %s: %w", modelKey, err)
+		}
+	}
+	return nil
+}
+
 // deleteModelTRLP deletes the aggregated TokenRateLimitPolicy for a model in the given namespace.
 func (r *MaaSSubscriptionReconciler) deleteModelTRLP(ctx context.Context, log logr.Logger, modelNamespace, modelName string) error {
 	// Always delete the aggregated TokenRateLimitPolicy so remaining MaaSSubscriptions rebuild it
@@ -400,7 +447,11 @@ func (r *MaaSSubscriptionReconciler) handleDeletion(ctx context.Context, log log
 				return ctrl.Result{}, err
 			}
 		}
-
+		// Also clean up stale TRLPs from modelRefs that were removed
+		// before the CR was deleted (edge case: edit + delete before reconcile).
+		if err := r.cleanupStaleTRLPs(ctx, log, subscription); err != nil {
+			return ctrl.Result{}, err
+		}
 		controllerutil.RemoveFinalizer(subscription, maasSubscriptionFinalizer)
 		if err := r.Update(ctx, subscription); err != nil {
 			return ctrl.Result{}, err