diff --git a/chart/templates/clusterrole-operator-manager-role.yaml b/chart/templates/clusterrole-operator-manager-role.yaml index f0cd39e..e723350 100644 --- a/chart/templates/clusterrole-operator-manager-role.yaml +++ b/chart/templates/clusterrole-operator-manager-role.yaml @@ -121,6 +121,7 @@ rules: - apiGroups: - serving.knative.dev resources: + - revisions - services verbs: - get diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 84ecaaf..2fa3d19 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -122,6 +122,7 @@ rules: - apiGroups: - serving.knative.dev resources: + - revisions - services verbs: - get diff --git a/internal/controller/decofile_controller.go b/internal/controller/decofile_controller.go index 5a54d61..ca7162d 100644 --- a/internal/controller/decofile_controller.go +++ b/internal/controller/decofile_controller.go @@ -27,17 +27,27 @@ import ( "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" + servingv1 "knative.dev/serving/pkg/apis/serving/v1" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" decositesv1alpha1 "github.com/deco-sites/decofile-operator/api/v1alpha1" ) const condTypePodsNotified = "PodsNotified" +// deploymentIdLabel is declared in notifier.go (same package). + // DecofileReconciler reconciles a Decofile object type DecofileReconciler struct { client.Client @@ -53,6 +63,7 @@ type DecofileReconciler struct { // +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=pods,verbs=get;list;watch +// +kubebuilder:rbac:groups=serving.knative.dev,resources=revisions,verbs=get;list;watch // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. @@ -80,6 +91,13 @@ func (r *DecofileReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c log.V(1).Info("Fetched Decofile", "duration", time.Since(fetchStart)) + // Sync Revision ownerReferences so deletion cascades when the Knative + // Revision referencing this Decofile is garbage-collected. + // Non-fatal: failures are logged but don't block reconcile. + if err := r.syncRevisionOwnerRefs(ctx, decofile); err != nil { + log.Error(err, "Failed to sync Revision ownerReferences (non-fatal)") + } + // Define the ConfigMap name configMapName := decofile.ConfigMapName() @@ -384,11 +402,148 @@ func updateCondition(decofile *decositesv1alpha1.Decofile, newCondition metav1.C decofile.Status.Conditions = append(decofile.Status.Conditions, newCondition) } +// syncRevisionOwnerRefs ensures the Decofile carries an ownerReference for +// every Knative Revision in the same namespace that targets it (matched via +// the app.deco/deploymentId label). When the Revision is later deleted — +// either by Knative GC (maxNonActiveRevisions) or by the knative-clean-* +// CronJobs — Kubernetes garbage collection cascades through Revision -> +// Decofile -> ConfigMap, so we never accumulate orphaned configuration. +// +// Multiple matching Revisions (e.g. during a rollback that reuses a +// deploymentId) all become owners; the Decofile is only GC'd once every +// owner Revision is gone. Stale UIDs are cleaned by Kubernetes GC on its +// own — we never remove ownerReferences here. +func (r *DecofileReconciler) syncRevisionOwnerRefs(ctx context.Context, decofile *decositesv1alpha1.Decofile) error { + log := logf.FromContext(ctx) + + deploymentId := decofile.Spec.DeploymentId + if deploymentId == "" { + deploymentId = decofile.Name + } + + revs := &servingv1.RevisionList{} + if err := r.List(ctx, revs, + client.InNamespace(decofile.Namespace), + client.MatchingLabels{deploymentIdLabel: deploymentId}, + ); err != nil { + return fmt.Errorf("list revisions for deploymentId=%s: %w", deploymentId, err) + } + if len(revs.Items) == 0 { + return nil + } + + existingUIDs := make(map[types.UID]bool, len(decofile.OwnerReferences)) + for _, or := range decofile.OwnerReferences { + existingUIDs[or.UID] = true + } + + toAdd := make([]metav1.OwnerReference, 0, len(revs.Items)) + for i := range revs.Items { + rev := &revs.Items[i] + if rev.DeletionTimestamp != nil { + continue + } + if existingUIDs[rev.UID] { + continue + } + toAdd = append(toAdd, metav1.OwnerReference{ + APIVersion: servingv1.SchemeGroupVersion.String(), + Kind: "Revision", + Name: rev.Name, + UID: rev.UID, + Controller: ptr.To(false), + BlockOwnerDeletion: ptr.To(false), + }) + } + if len(toAdd) == 0 { + return nil + } + + // Refetch + re-diff against the latest version to avoid optimistic + // concurrency conflicts with status updates that happen later in + // the reconcile loop. + fresh := &decositesv1alpha1.Decofile{} + if err := r.Get(ctx, client.ObjectKey{Name: decofile.Name, Namespace: decofile.Namespace}, fresh); err != nil { + return fmt.Errorf("refetch decofile: %w", err) + } + freshUIDs := make(map[types.UID]bool, len(fresh.OwnerReferences)) + for _, or := range fresh.OwnerReferences { + freshUIDs[or.UID] = true + } + var actualAdd []metav1.OwnerReference + for _, or := range toAdd { + if !freshUIDs[or.UID] { + actualAdd = append(actualAdd, or) + } + } + if len(actualAdd) == 0 { + return nil + } + fresh.OwnerReferences = append(fresh.OwnerReferences, actualAdd...) + if err := r.Update(ctx, fresh); err != nil { + return fmt.Errorf("update decofile owner refs: %w", err) + } + + for _, or := range actualAdd { + log.Info("Added Revision as Decofile owner", + "decofile", decofile.Name, "revision", or.Name, "uid", or.UID) + } + return nil +} + +// mapRevisionToDecofile maps a Revision event to the Decofile that should be +// reconciled because of it. Used by the Revision watch so that new Revisions +// (created after their Decofile) still trigger ownerReference sync. +func (r *DecofileReconciler) mapRevisionToDecofile(ctx context.Context, obj client.Object) []reconcile.Request { + rev, ok := obj.(*servingv1.Revision) + if !ok { + return nil + } + deploymentId := rev.Labels[deploymentIdLabel] + if deploymentId == "" { + return nil + } + decofiles := &decositesv1alpha1.DecofileList{} + if err := r.List(ctx, decofiles, client.InNamespace(rev.Namespace)); err != nil { + return nil + } + var reqs []reconcile.Request + for i := range decofiles.Items { + df := &decofiles.Items[i] + depId := df.Spec.DeploymentId + if depId == "" { + depId = df.Name + } + if depId == deploymentId { + reqs = append(reqs, reconcile.Request{ + NamespacedName: client.ObjectKey{Namespace: df.Namespace, Name: df.Name}, + }) + } + } + return reqs +} + // SetupWithManager sets up the controller with the Manager. func (r *DecofileReconciler) SetupWithManager(mgr ctrl.Manager) error { + // Only react to Revision Create — Updates and Deletes don't add new + // ownerRef linkage information (Kubernetes GC already handles deletes + // via existing ownerReferences). Skipping them keeps the controller + // from spinning on Status churn of running Revisions. + revisionCreateOnly := predicate.Funcs{ + CreateFunc: func(_ event.CreateEvent) bool { return true }, + UpdateFunc: func(_ event.UpdateEvent) bool { return false }, + DeleteFunc: func(_ event.DeleteEvent) bool { return false }, + GenericFunc: func(_ event.GenericEvent) bool { return false }, + } + return ctrl.NewControllerManagedBy(mgr). For(&decositesv1alpha1.Decofile{}). Owns(&corev1.ConfigMap{}). + Watches( + &servingv1.Revision{}, + handler.EnqueueRequestsFromMapFunc(r.mapRevisionToDecofile), + builder.WithPredicates(revisionCreateOnly), + ). Named("decofile"). WithOptions(controller.Options{ MaxConcurrentReconciles: 8, // Allow 8 parallel reconciliations diff --git a/internal/controller/decofile_owner_test.go b/internal/controller/decofile_owner_test.go new file mode 100644 index 0000000..8e030bd --- /dev/null +++ b/internal/controller/decofile_owner_test.go @@ -0,0 +1,302 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + servingv1 "knative.dev/serving/pkg/apis/serving/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + decositesv1alpha1 "github.com/deco-sites/decofile-operator/api/v1alpha1" +) + +func newOwnerTestScheme(t *testing.T) *runtime.Scheme { + t.Helper() + s := runtime.NewScheme() + if err := decositesv1alpha1.AddToScheme(s); err != nil { + t.Fatalf("add decosites scheme: %v", err) + } + if err := servingv1.AddToScheme(s); err != nil { + t.Fatalf("add servingv1 scheme: %v", err) + } + return s +} + +const testNamespace = "sites-foo" + +func makeRevision(name, deploymentId string, uid types.UID) *servingv1.Revision { + return &servingv1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: testNamespace, + UID: uid, + Labels: map[string]string{deploymentIdLabel: deploymentId}, + }, + } +} + +func makeDecofile(name, deploymentId string) *decositesv1alpha1.Decofile { + df := &decositesv1alpha1.Decofile{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: testNamespace, + }, + } + if deploymentId != "" { + df.Spec.DeploymentId = deploymentId + } + return df +} + +func TestSyncRevisionOwnerRefs_AddsOwnerWhenRevisionExists(t *testing.T) { + ctx := context.Background() + scheme := newOwnerTestScheme(t) + + df := makeDecofile("mhsygflbgo", "") + rev := makeRevision("foo-site-mhsygflbgo", "mhsygflbgo", "rev-uid-1") + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() + r := &DecofileReconciler{Client: c, Scheme: scheme} + + if err := r.syncRevisionOwnerRefs(ctx, df); err != nil { + t.Fatalf("sync failed: %v", err) + } + + got := &decositesv1alpha1.Decofile{} + if err := c.Get(ctx, client.ObjectKey{Name: "mhsygflbgo", Namespace: "sites-foo"}, got); err != nil { + t.Fatalf("get decofile: %v", err) + } + if len(got.OwnerReferences) != 1 { + t.Fatalf("want 1 owner, got %d", len(got.OwnerReferences)) + } + or := got.OwnerReferences[0] + if or.Kind != "Revision" || or.Name != "foo-site-mhsygflbgo" || or.UID != "rev-uid-1" { + t.Errorf("unexpected ownerRef: %+v", or) + } + if or.Controller == nil || *or.Controller { + t.Errorf("controller should be false, got %v", or.Controller) + } + if or.BlockOwnerDeletion == nil || *or.BlockOwnerDeletion { + t.Errorf("blockOwnerDeletion should be false, got %v", or.BlockOwnerDeletion) + } +} + +func TestSyncRevisionOwnerRefs_NoMatchingRevisionDoesNothing(t *testing.T) { + ctx := context.Background() + scheme := newOwnerTestScheme(t) + + df := makeDecofile("orphan", "") + // Revision with DIFFERENT deploymentId — must not be picked up. + rev := makeRevision("foo-site-other", "other", "rev-uid-2") + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() + r := &DecofileReconciler{Client: c, Scheme: scheme} + + if err := r.syncRevisionOwnerRefs(ctx, df); err != nil { + t.Fatalf("sync failed: %v", err) + } + + got := &decositesv1alpha1.Decofile{} + if err := c.Get(ctx, client.ObjectKey{Name: "orphan", Namespace: "sites-foo"}, got); err != nil { + t.Fatalf("get decofile: %v", err) + } + if len(got.OwnerReferences) != 0 { + t.Fatalf("want 0 owners for orphan, got %d", len(got.OwnerReferences)) + } +} + +func TestSyncRevisionOwnerRefs_IsIdempotent(t *testing.T) { + ctx := context.Background() + scheme := newOwnerTestScheme(t) + + df := makeDecofile("mhsygflbgo", "") + rev := makeRevision("foo-site-mhsygflbgo", "mhsygflbgo", "rev-uid-3") + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() + r := &DecofileReconciler{Client: c, Scheme: scheme} + + for i := 0; i < 3; i++ { + if err := r.syncRevisionOwnerRefs(ctx, df); err != nil { + t.Fatalf("sync %d failed: %v", i, err) + } + // Refresh local df to mirror what a real reconcile loop would see. + if err := c.Get(ctx, client.ObjectKey{Name: df.Name, Namespace: df.Namespace}, df); err != nil { + t.Fatalf("refresh: %v", err) + } + } + + if len(df.OwnerReferences) != 1 { + t.Fatalf("want exactly 1 ownerRef after 3 syncs, got %d", len(df.OwnerReferences)) + } +} + +func TestSyncRevisionOwnerRefs_SkipsRevisionBeingDeleted(t *testing.T) { + ctx := context.Background() + scheme := newOwnerTestScheme(t) + + df := makeDecofile("mhsygflbgo", "") + rev := makeRevision("foo-site-mhsygflbgo", "mhsygflbgo", "rev-uid-4") + now := metav1.Now() + rev.DeletionTimestamp = &now + rev.Finalizers = []string{"keep-alive-for-test"} + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() + r := &DecofileReconciler{Client: c, Scheme: scheme} + + if err := r.syncRevisionOwnerRefs(ctx, df); err != nil { + t.Fatalf("sync failed: %v", err) + } + + got := &decositesv1alpha1.Decofile{} + if err := c.Get(ctx, client.ObjectKey{Name: df.Name, Namespace: df.Namespace}, got); err != nil { + t.Fatalf("get decofile: %v", err) + } + if len(got.OwnerReferences) != 0 { + t.Fatalf("want 0 owners (terminating Revision skipped), got %d", len(got.OwnerReferences)) + } +} + +func TestSyncRevisionOwnerRefs_MultipleRevisionsBecomeMultipleOwners(t *testing.T) { + ctx := context.Background() + scheme := newOwnerTestScheme(t) + + df := makeDecofile("mhsygflbgo", "") + // Two Revisions with the same deploymentId (rollback scenario). + rev1 := makeRevision("foo-site-mhsygflbgo-old", "mhsygflbgo", "uid-old") + rev2 := makeRevision("foo-site-mhsygflbgo-new", "mhsygflbgo", "uid-new") + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev1, rev2).Build() + r := &DecofileReconciler{Client: c, Scheme: scheme} + + if err := r.syncRevisionOwnerRefs(ctx, df); err != nil { + t.Fatalf("sync failed: %v", err) + } + + got := &decositesv1alpha1.Decofile{} + if err := c.Get(ctx, client.ObjectKey{Name: df.Name, Namespace: df.Namespace}, got); err != nil { + t.Fatalf("get decofile: %v", err) + } + if len(got.OwnerReferences) != 2 { + t.Fatalf("want 2 ownerRefs (both Revisions keep the Decofile alive), got %d", len(got.OwnerReferences)) + } + uids := map[types.UID]bool{} + for _, or := range got.OwnerReferences { + uids[or.UID] = true + } + if !uids["uid-old"] || !uids["uid-new"] { + t.Errorf("missing one of the expected UIDs: %v", uids) + } +} + +func TestSyncRevisionOwnerRefs_RespectsExplicitDeploymentId(t *testing.T) { + ctx := context.Background() + scheme := newOwnerTestScheme(t) + + // Decofile name and explicit deploymentId differ — the explicit one wins. + df := makeDecofile("any-name", "explicit-dep") + rev := makeRevision("rev-by-explicit", "explicit-dep", "uid-explicit") + // Decoy Revision with deploymentId matching the Decofile name — must NOT + // be picked up because spec.deploymentId is set. + decoy := makeRevision("rev-decoy", "any-name", "uid-decoy") + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev, decoy).Build() + r := &DecofileReconciler{Client: c, Scheme: scheme} + + if err := r.syncRevisionOwnerRefs(ctx, df); err != nil { + t.Fatalf("sync failed: %v", err) + } + + got := &decositesv1alpha1.Decofile{} + if err := c.Get(ctx, client.ObjectKey{Name: df.Name, Namespace: df.Namespace}, got); err != nil { + t.Fatalf("get decofile: %v", err) + } + if len(got.OwnerReferences) != 1 { + t.Fatalf("want 1 ownerRef, got %d", len(got.OwnerReferences)) + } + if got.OwnerReferences[0].UID != "uid-explicit" { + t.Errorf("want uid-explicit, got %s", got.OwnerReferences[0].UID) + } +} + +func TestMapRevisionToDecofile_FindsByDefaultedDeploymentId(t *testing.T) { + ctx := context.Background() + scheme := newOwnerTestScheme(t) + + // Decofile uses metadata.name as effective deploymentId (spec.deploymentId empty). + df := makeDecofile("mhsygflbgo", "") + rev := makeRevision("foo-site-mhsygflbgo", "mhsygflbgo", "uid") + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() + r := &DecofileReconciler{Client: c, Scheme: scheme} + + reqs := r.mapRevisionToDecofile(ctx, rev) + if len(reqs) != 1 { + t.Fatalf("want 1 request, got %d", len(reqs)) + } + want := reconcile.Request{NamespacedName: client.ObjectKey{Namespace: "sites-foo", Name: "mhsygflbgo"}} + if reqs[0] != want { + t.Errorf("want %v, got %v", want, reqs[0]) + } +} + +func TestMapRevisionToDecofile_IgnoresRevisionWithoutLabel(t *testing.T) { + ctx := context.Background() + scheme := newOwnerTestScheme(t) + + df := makeDecofile("mhsygflbgo", "") + rev := &servingv1.Revision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "no-label", + Namespace: "sites-foo", + UID: "uid", + // Note: no deploymentIdLabel. + }, + } + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() + r := &DecofileReconciler{Client: c, Scheme: scheme} + + reqs := r.mapRevisionToDecofile(ctx, rev) + if len(reqs) != 0 { + t.Fatalf("want 0 requests, got %d", len(reqs)) + } +} + +func TestMapRevisionToDecofile_FindsByExplicitDeploymentId(t *testing.T) { + ctx := context.Background() + scheme := newOwnerTestScheme(t) + + df := makeDecofile("any-name", "explicit-dep") + rev := makeRevision("rev", "explicit-dep", "uid") + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(df, rev).Build() + r := &DecofileReconciler{Client: c, Scheme: scheme} + + reqs := r.mapRevisionToDecofile(ctx, rev) + if len(reqs) != 1 { + t.Fatalf("want 1 request, got %d", len(reqs)) + } + if reqs[0].Name != "any-name" { + t.Errorf("want any-name, got %s", reqs[0].Name) + } +}