diff --git a/charts/solar/files/role.yaml b/charts/solar/files/role.yaml index 4306a181..ebe2b469 100644 --- a/charts/solar/files/role.yaml +++ b/charts/solar/files/role.yaml @@ -39,11 +39,8 @@ rules: - solar.opendefense.cloud resources: - components - - componentversions - - profiles - - registries - - registrybindings verbs: + - delete - get - list - patch @@ -64,6 +61,19 @@ rules: - targets/finalizers verbs: - update +- apiGroups: + - solar.opendefense.cloud + resources: + - componentversions + - profiles + - registries + - registrybindings + verbs: + - get + - list + - patch + - update + - watch - apiGroups: - solar.opendefense.cloud resources: diff --git a/cmd/solar-controller-manager/main.go b/cmd/solar-controller-manager/main.go index d954a8ab..fb3c7c34 100644 --- a/cmd/solar-controller-manager/main.go +++ b/cmd/solar-controller-manager/main.go @@ -277,6 +277,15 @@ func main() { os.Exit(1) } + if err := (&controller.ComponentReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + APIReader: mgr.GetAPIReader(), + }).SetupWithManager(mgr); err != nil { + setupLog.Error(err, "unable to create controller", "controller", "component") + os.Exit(1) + } + if err := (&controller.ReleaseBindingReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), diff --git a/pkg/controller/component_controller.go b/pkg/controller/component_controller.go new file mode 100644 index 00000000..a6cb338e --- /dev/null +++ b/pkg/controller/component_controller.go @@ -0,0 +1,255 @@ +// Copyright 2026 BWI GmbH and Solution Arsenal contributors +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "context" + "slices" + "time" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + + solarv1alpha1 "go.opendefense.cloud/solar/api/solar/v1alpha1" +) + +// ComponentReconciler owns the Component lifecycle: it keeps the +// componentRefFinalizer in sync with live ComponentVersions (deletion +// protection) and garbage-collects a Component once its last live +// ComponentVersion is gone. Reconciles are serialized per Component, so the +// count-then-act decision cannot interleave with itself for the same object. +type ComponentReconciler struct { + client.Client + Scheme *runtime.Scheme + // APIReader reads directly from the API server, bypassing the informer + // cache. Used as the authoritative zero-live-CV check before deleting, + // because the cached index can lag behind a just-created CV. + APIReader client.Reader + // WatchNamespace restricts reconciliation to this namespace. + // Should be empty in production (watches all namespaces). + // Intended for use in integration tests only. + WatchNamespace string +} + +//+kubebuilder:rbac:groups=solar.opendefense.cloud,resources=components,verbs=get;list;watch;update;patch;delete +//+kubebuilder:rbac:groups=solar.opendefense.cloud,resources=components/finalizers,verbs=update +//+kubebuilder:rbac:groups=solar.opendefense.cloud,resources=componentversions,verbs=get;list;watch + +func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + log := ctrl.LoggerFrom(ctx) + + log.V(1).Info("Component is being reconciled", "req", req) + + if r.WatchNamespace != "" && req.Namespace != r.WatchNamespace { + return ctrl.Result{}, nil + } + + comp := &solarv1alpha1.Component{} + if err := r.Get(ctx, req.NamespacedName, comp); err != nil { + if apierrors.IsNotFound(err) { + return ctrl.Result{}, nil + } + + return ctrl.Result{}, errLogAndWrap(log, err, "failed to get Component") + } + + live, err := r.countLiveCVsCached(ctx, comp) + if err != nil { + return ctrl.Result{}, err + } + + if live > 0 { + return ctrl.Result{}, r.ensureProtectionFinalizer(ctx, comp) + } + + // live == 0: only GC Components that carry the protection finalizer. The + // finalizer means a live CV was observed at some point; without it the + // Component was either just created by discovery (its first CV not yet + // visible) or created manually, and neither must be deleted here. + if !slices.Contains(comp.Finalizers, componentRefFinalizer) { + return ctrl.Result{}, nil + } + + // Re-read the Component straight from the API server. This serves two + // purposes: it is the authoritative check for componentRefFinalizer + // having been removed out-of-band (e.g. a manual kubectl edit) since the + // cached Get above, so we don't attempt to delete a Component that no + // longer carries our finalizer; and it gives the fresh UID that feeds + // the delete precondition below, so the delete targets the exact object + // version this reconcile evaluated. + fresh := &solarv1alpha1.Component{} + if err := r.APIReader.Get(ctx, req.NamespacedName, fresh); err != nil { + if apierrors.IsNotFound(err) { + return ctrl.Result{}, nil + } + + return ctrl.Result{}, errLogAndWrap(log, err, "failed to get Component from API server") + } + if !slices.Contains(fresh.Finalizers, componentRefFinalizer) { + return ctrl.Result{}, nil + } + comp = fresh + + // Authoritative re-check straight from the API server; custom field + // indexes are cache-only, so filter in code. + liveDirect, err := r.countLiveCVsDirect(ctx, comp) + if err != nil { + return ctrl.Result{}, err + } + if liveDirect > 0 { + // Cache lagged behind a just-created CV; its watch event re-enqueues us. + return ctrl.Result{}, nil + } + + // Delete first (the finalizer holds the object in Terminating), then strip + // the finalizer. If we crash in between, a later reconcile finds + // live == 0 with the finalizer present and finishes the removal. + if comp.DeletionTimestamp.IsZero() { + if err := client.IgnoreNotFound(r.Delete(ctx, comp, client.Preconditions{UID: &comp.UID})); err != nil { + return ctrl.Result{}, errLogAndWrap(log, err, "failed to delete unreferenced Component") + } + log.V(1).Info("Deleted unreferenced Component", "component", comp.Name) + } + + // Re-check once more before stripping the finalizer: a CV created in the + // window between the delete call above and this point is caught here and + // leaves the Component Terminating-and-protected instead of removed. A CV + // created after this check but before the patch below still loses its + // parent (a brief hard delete under a live reference): that residual is + // inherent to poll-then-act without cross-resource transactions, lasts + // milliseconds, and self-heals because the next discovery event re-creates + // the Component via the apiwriter's ensureComponent. + liveAfterDelete, err := r.countLiveCVsDirect(ctx, comp) + if err != nil { + return ctrl.Result{}, err + } + if liveAfterDelete > 0 { + return ctrl.Result{}, nil + } + + latest := &solarv1alpha1.Component{} + if err := r.Get(ctx, req.NamespacedName, latest); err != nil { + if apierrors.IsNotFound(err) { + return ctrl.Result{}, nil + } + + return ctrl.Result{}, errLogAndWrap(log, err, "failed to get latest Component for finalizer removal") + } + original := latest.DeepCopy() + latest.Finalizers = slices.DeleteFunc(latest.Finalizers, func(s string) bool { return s == componentRefFinalizer }) + if err := r.Patch(ctx, latest, client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})); err != nil { + if apierrors.IsNotFound(err) { + return ctrl.Result{}, nil + } + if apierrors.IsConflict(err) { + // Something changed concurrently; re-evaluate from scratch. + return ctrl.Result{RequeueAfter: time.Second}, nil + } + + return ctrl.Result{}, errLogAndWrap(log, err, "failed to remove protection finalizer from Component") + } + + return ctrl.Result{}, nil +} + +// ensureProtectionFinalizer adds componentRefFinalizer to comp unless it is +// already present or the Component is terminating (the API server rejects +// adding finalizers to terminating objects). +func (r *ComponentReconciler) ensureProtectionFinalizer(ctx context.Context, comp *solarv1alpha1.Component) error { + if !comp.DeletionTimestamp.IsZero() || slices.Contains(comp.Finalizers, componentRefFinalizer) { + return nil + } + + original := comp.DeepCopy() + comp.Finalizers = append(comp.Finalizers, componentRefFinalizer) + if err := r.Patch(ctx, comp, client.MergeFrom(original)); err != nil { + return errLogAndWrap(ctrl.LoggerFrom(ctx), err, "failed to add protection finalizer to Component") + } + + return nil +} + +// countLiveCVsCached counts non-terminating ComponentVersions referencing comp +// using the indexed informer cache. +func (r *ComponentReconciler) countLiveCVsCached(ctx context.Context, comp *solarv1alpha1.Component) (int, error) { + cvList := &solarv1alpha1.ComponentVersionList{} + if err := r.List(ctx, cvList, + client.InNamespace(comp.Namespace), + client.MatchingFields{indexCVByComponentName: comp.Name}, + ); err != nil { + return 0, errLogAndWrap(ctrl.LoggerFrom(ctx), err, "failed to list ComponentVersions for Component") + } + + live := 0 + for _, cv := range cvList.Items { + if cv.DeletionTimestamp.IsZero() { + live++ + } + } + + return live, nil +} + +// directListPageSize bounds each page of the uncached ComponentVersion list on +// the GC path; custom field indexes are cache-only, so the filter runs in code. +const directListPageSize = 500 + +// countLiveCVsDirect counts non-terminating ComponentVersions referencing comp +// by listing straight from the API server, page by page. +func (r *ComponentReconciler) countLiveCVsDirect(ctx context.Context, comp *solarv1alpha1.Component) (int, error) { + live := 0 + continueToken := "" + for { + cvList := &solarv1alpha1.ComponentVersionList{} + if err := r.APIReader.List(ctx, cvList, + client.InNamespace(comp.Namespace), + client.Limit(directListPageSize), + client.Continue(continueToken), + ); err != nil { + return 0, errLogAndWrap(ctrl.LoggerFrom(ctx), err, "failed to list ComponentVersions from API server") + } + + for _, cv := range cvList.Items { + if cv.Spec.ComponentRef.Name == comp.Name && cv.DeletionTimestamp.IsZero() { + live++ + } + } + + continueToken = cvList.Continue + if continueToken == "" { + return live, nil + } + } +} + +// mapCVToComponent maps ComponentVersion events to a reconcile request for the +// parent Component (same namespace, spec.componentRef.name). +func mapCVToComponent(_ context.Context, obj client.Object) []reconcile.Request { + cv, ok := obj.(*solarv1alpha1.ComponentVersion) + if !ok || cv.Spec.ComponentRef.Name == "" { + return nil + } + + return []reconcile.Request{ + { + NamespacedName: types.NamespacedName{ + Name: cv.Spec.ComponentRef.Name, + Namespace: cv.Namespace, + }, + }, + } +} + +// SetupWithManager sets up the controller with the Manager. +func (r *ComponentReconciler) SetupWithManager(mgr ctrl.Manager) error { + return ctrl.NewControllerManagedBy(mgr). + For(&solarv1alpha1.Component{}). + Watches(&solarv1alpha1.ComponentVersion{}, handler.EnqueueRequestsFromMapFunc(mapCVToComponent)). + Complete(r) +} diff --git a/pkg/controller/component_controller_test.go b/pkg/controller/component_controller_test.go new file mode 100644 index 00000000..354ebc14 --- /dev/null +++ b/pkg/controller/component_controller_test.go @@ -0,0 +1,187 @@ +// Copyright 2026 BWI GmbH and Solution Arsenal contributors +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + + solarv1alpha1 "go.opendefense.cloud/solar/api/solar/v1alpha1" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("ComponentReconciler", Ordered, func() { + var ( + newComponent = func(name string) *solarv1alpha1.Component { + return &solarv1alpha1.Component{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns.Name, + }, + Spec: solarv1alpha1.ComponentSpec{ + Scheme: "oci", + Registry: "registry.example.com", + Repository: "example/component", + }, + } + } + + newCV = func(name string, componentName string) *solarv1alpha1.ComponentVersion { + return &solarv1alpha1.ComponentVersion{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns.Name, + }, + Spec: solarv1alpha1.ComponentVersionSpec{ + ComponentRef: corev1.LocalObjectReference{Name: componentName}, + Tag: "v1.0.0", + }, + } + } + + // cleanup force-removes finalizers so namespace teardown never hangs. + cleanup = func(obj client.Object) { + patch := client.RawPatch(types.JSONPatchType, []byte(`[{"op":"replace","path":"/metadata/finalizers","value":[]}]`)) + _ = client.IgnoreNotFound(k8sClient.Patch(ctx, obj, patch)) + _ = client.IgnoreNotFound(k8sClient.Delete(ctx, obj)) + } + ) + + It("adds componentRefFinalizer to the Component while a live ComponentVersion references it", func() { + comp := newComponent("cr-comp-protect") + Expect(k8sClient.Create(ctx, comp)).To(Succeed()) + DeferCleanup(cleanup, comp) + + cv := newCV("cr-cv-protect", comp.Name) + Expect(k8sClient.Create(ctx, cv)).To(Succeed()) + DeferCleanup(cleanup, cv) + + Eventually(func(g Gomega) { + updated := &solarv1alpha1.Component{} + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), updated)).To(Succeed()) + g.Expect(updated.Finalizers).To(ContainElement(componentRefFinalizer)) + }, eventuallyTimeout).Should(Succeed()) + }) + + It("blocks Component deletion while a live ComponentVersion exists, completes it once the last CV is gone", func() { + comp := newComponent("cr-comp-blocked") + Expect(k8sClient.Create(ctx, comp)).To(Succeed()) + + cv := newCV("cr-cv-blocked", comp.Name) + Expect(k8sClient.Create(ctx, cv)).To(Succeed()) + + Eventually(func(g Gomega) { + updated := &solarv1alpha1.Component{} + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), updated)).To(Succeed()) + g.Expect(updated.Finalizers).To(ContainElement(componentRefFinalizer)) + }, eventuallyTimeout).Should(Succeed()) + + Expect(k8sClient.Delete(ctx, comp)).To(Succeed()) + + Consistently(func(g Gomega) { + updated := &solarv1alpha1.Component{} + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), updated)).To(Succeed()) + g.Expect(updated.DeletionTimestamp).NotTo(BeNil()) + }, consistentlyDuration).Should(Succeed()) + + Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, cv))).To(Succeed()) + + Eventually(func() bool { + return apierrors.IsNotFound(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), &solarv1alpha1.Component{})) + }, eventuallyTimeout).Should(BeTrue()) + }) + + It("garbage collects the Component when its last ComponentVersion is deleted", func() { + comp := newComponent("cr-comp-gc") + Expect(k8sClient.Create(ctx, comp)).To(Succeed()) + DeferCleanup(cleanup, comp) + + cv := newCV("cr-cv-gc", comp.Name) + Expect(k8sClient.Create(ctx, cv)).To(Succeed()) + + Eventually(func(g Gomega) { + updated := &solarv1alpha1.Component{} + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), updated)).To(Succeed()) + g.Expect(updated.Finalizers).To(ContainElement(componentRefFinalizer)) + }, eventuallyTimeout).Should(Succeed()) + + Expect(k8sClient.Delete(ctx, cv)).To(Succeed()) + + Eventually(func() bool { + return apierrors.IsNotFound(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), &solarv1alpha1.Component{})) + }, eventuallyTimeout).Should(BeTrue()) + }) + + It("does not GC the Component when a CV is deleted while another is created concurrently", func() { + comp := newComponent("cr-comp-recreate") + Expect(k8sClient.Create(ctx, comp)).To(Succeed()) + DeferCleanup(cleanup, comp) + + cvOld := newCV("cr-cv-old", comp.Name) + Expect(k8sClient.Create(ctx, cvOld)).To(Succeed()) + + Eventually(func(g Gomega) { + updated := &solarv1alpha1.Component{} + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), updated)).To(Succeed()) + g.Expect(updated.Finalizers).To(ContainElement(componentRefFinalizer)) + }, eventuallyTimeout).Should(Succeed()) + + // Delete the old CV and immediately create a replacement, simulating + // discovery re-creating a version during cleanup. + cvNew := newCV("cr-cv-new", comp.Name) + Expect(k8sClient.Delete(ctx, cvOld)).To(Succeed()) + Expect(k8sClient.Create(ctx, cvNew)).To(Succeed()) + DeferCleanup(cleanup, cvNew) + + // The Component must survive the churn and keep (or regain) protection. + Consistently(func(g Gomega) { + updated := &solarv1alpha1.Component{} + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), updated)).To(Succeed()) + g.Expect(updated.DeletionTimestamp).To(BeNil()) + }, consistentlyDuration).Should(Succeed()) + + Eventually(func(g Gomega) { + updated := &solarv1alpha1.Component{} + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), updated)).To(Succeed()) + g.Expect(updated.Finalizers).To(ContainElement(componentRefFinalizer)) + }, eventuallyTimeout).Should(Succeed()) + }) + + It("sweeps a Component that already carries componentRefFinalizer but has no live ComponentVersions", func() { + // Seeds the state left behind by a crash between the delete call and + // the finalizer strip (or an upgrade-time orphan predating GC): the + // finalizer is present, no ComponentVersion ever references it. The + // finalizer-adding patch itself is the update event that gets this + // Component onto the reconciler's queue. + comp := newComponent("cr-comp-orphan") + Expect(k8sClient.Create(ctx, comp)).To(Succeed()) + DeferCleanup(cleanup, comp) + + original := comp.DeepCopy() + comp.Finalizers = append(comp.Finalizers, componentRefFinalizer) + Expect(k8sClient.Patch(ctx, comp, client.MergeFrom(original))).To(Succeed()) + + Eventually(func() bool { + return apierrors.IsNotFound(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), &solarv1alpha1.Component{})) + }, eventuallyTimeout).Should(BeTrue()) + }) + + It("leaves a Component without any ComponentVersions alone", func() { + comp := newComponent("cr-comp-manual") + Expect(k8sClient.Create(ctx, comp)).To(Succeed()) + DeferCleanup(cleanup, comp) + + // No CV ever referenced it: no finalizer, no GC. + Consistently(func(g Gomega) { + updated := &solarv1alpha1.Component{} + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), updated)).To(Succeed()) + g.Expect(updated.Finalizers).NotTo(ContainElement(componentRefFinalizer)) + }, consistentlyDuration).Should(Succeed()) + }) +}) diff --git a/pkg/controller/componentversion_controller.go b/pkg/controller/componentversion_controller.go index 0a6cae3e..4d4f9237 100644 --- a/pkg/controller/componentversion_controller.go +++ b/pkg/controller/componentversion_controller.go @@ -9,15 +9,15 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" solarv1alpha1 "go.opendefense.cloud/solar/api/solar/v1alpha1" ) -// ComponentVersionReconciler manages the deletion-protection finalizer on the Component -// referenced by each ComponentVersion, preventing Component deletion while ComponentVersions exist. +// ComponentVersionReconciler manages the componentVersionFinalizer on each +// ComponentVersion so deletion is observable by other controllers. The +// componentRefFinalizer on the parent Component is owned by ComponentReconciler. type ComponentVersionReconciler struct { client.Client Scheme *runtime.Scheme @@ -29,8 +29,6 @@ type ComponentVersionReconciler struct { //+kubebuilder:rbac:groups=solar.opendefense.cloud,resources=componentversions,verbs=get;list;watch;update;patch //+kubebuilder:rbac:groups=solar.opendefense.cloud,resources=componentversions/finalizers,verbs=update -//+kubebuilder:rbac:groups=solar.opendefense.cloud,resources=components,verbs=get;list;watch;update;patch -//+kubebuilder:rbac:groups=solar.opendefense.cloud,resources=components/finalizers,verbs=update func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { log := ctrl.LoggerFrom(ctx) @@ -50,19 +48,9 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req return ctrl.Result{}, errLogAndWrap(log, err, "failed to get ComponentVersion") } - // Handle deletion: remove componentRefFinalizer from Component if no other CV references it. + // Handle deletion: remove the self-finalizer; the parent Component's + // protection finalizer and GC are handled by ComponentReconciler. if !cv.DeletionTimestamp.IsZero() { - if cv.Spec.ComponentRef.Name != "" { - comp := &solarv1alpha1.Component{} - if err := r.Get(ctx, types.NamespacedName{Name: cv.Spec.ComponentRef.Name, Namespace: cv.Namespace}, comp); err != nil { - if !apierrors.IsNotFound(err) { - return ctrl.Result{}, errLogAndWrap(log, err, "failed to get Component for finalizer cleanup") - } - } else if err := r.removeComponentRefFinalizer(ctx, cv, comp); err != nil { - return ctrl.Result{}, err - } - } - if slices.Contains(cv.Finalizers, componentVersionFinalizer) { latest := &solarv1alpha1.ComponentVersion{} if err := r.Get(ctx, req.NamespacedName, latest); err != nil { @@ -93,70 +81,9 @@ func (r *ComponentVersionReconciler) Reconcile(ctx context.Context, req ctrl.Req } } - // Protect the referenced Component from deletion. - if cv.Spec.ComponentRef.Name != "" { - comp := &solarv1alpha1.Component{} - if err := r.Get(ctx, types.NamespacedName{Name: cv.Spec.ComponentRef.Name, Namespace: cv.Namespace}, comp); err != nil { - if !apierrors.IsNotFound(err) { - return ctrl.Result{}, errLogAndWrap(log, err, "failed to get Component for protection finalizer") - } - } else if !slices.Contains(comp.Finalizers, componentRefFinalizer) { - latest := comp.DeepCopy() - latest.Finalizers = append(latest.Finalizers, componentRefFinalizer) - if err := r.Patch(ctx, latest, client.MergeFrom(comp)); err != nil { - return ctrl.Result{}, errLogAndWrap(log, err, "failed to add protection finalizer to Component") - } - } - } - return ctrl.Result{}, nil } -// removeComponentRefFinalizer removes componentRefFinalizer from comp when no other active -// ComponentVersion still references it (excluding the CV currently being deleted). -func (r *ComponentVersionReconciler) removeComponentRefFinalizer(ctx context.Context, deletingCV *solarv1alpha1.ComponentVersion, comp *solarv1alpha1.Component) error { - if !slices.Contains(comp.Finalizers, componentRefFinalizer) { - return nil - } - - cvList := &solarv1alpha1.ComponentVersionList{} - if err := r.List(ctx, cvList, - client.InNamespace(comp.Namespace), - client.MatchingFields{indexCVByComponentName: comp.Name}, - ); err != nil { - return errLogAndWrap(ctrl.LoggerFrom(ctx), err, "failed to list ComponentVersions for Component finalizer check") - } - - for _, cv := range cvList.Items { - if cv.Name == deletingCV.Name { - continue - } - if !cv.DeletionTimestamp.IsZero() { - continue - } - - return nil // another active ComponentVersion still references this Component - } - - freshComp := &solarv1alpha1.Component{} - if err := r.Get(ctx, client.ObjectKeyFromObject(comp), freshComp); err != nil { - if apierrors.IsNotFound(err) { - return nil - } - - return errLogAndWrap(ctrl.LoggerFrom(ctx), err, "failed to get latest Component for finalizer removal") - } - original := freshComp.DeepCopy() - freshComp.Finalizers = slices.DeleteFunc(freshComp.Finalizers, func(s string) bool { return s == componentRefFinalizer }) - if err := r.Patch(ctx, freshComp, client.MergeFrom(original)); err != nil { - return errLogAndWrap(ctrl.LoggerFrom(ctx), err, "failed to remove protection finalizer from Component") - } - - ctrl.LoggerFrom(ctx).V(1).Info("Removed protection finalizer from Component", "component", comp.Name) - - return nil -} - // SetupWithManager sets up the controller with the Manager. func (r *ComponentVersionReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). diff --git a/pkg/controller/componentversion_controller_test.go b/pkg/controller/componentversion_controller_test.go index 0905b46a..9534f2d5 100644 --- a/pkg/controller/componentversion_controller_test.go +++ b/pkg/controller/componentversion_controller_test.go @@ -46,9 +46,9 @@ var _ = Describe("ComponentVersionReconciler", Ordered, func() { } ) - Describe("protection finalizer on Component", func() { - It("adds componentVersionFinalizer to ComponentVersion and componentRefFinalizer to Component", func() { - comp := validComponent("dp-comp-a") + Describe("self-finalizer", func() { + It("adds componentVersionFinalizer to a live ComponentVersion", func() { + comp := validComponent("cvf-comp-add") Expect(k8sClient.Create(ctx, comp)).To(Succeed()) DeferCleanup(func() { patch := client.RawPatch(types.JSONPatchType, []byte(`[{"op":"replace","path":"/metadata/finalizers","value":[]}]`)) @@ -56,7 +56,7 @@ var _ = Describe("ComponentVersionReconciler", Ordered, func() { _ = client.IgnoreNotFound(k8sClient.Delete(ctx, comp)) }) - cv := validCV("dp-cv-a", comp.Name) + cv := validCV("cvf-cv-add", comp.Name) Expect(k8sClient.Create(ctx, cv)).To(Succeed()) DeferCleanup(func() { patch := client.RawPatch(types.JSONPatchType, []byte(`[{"op":"replace","path":"/metadata/finalizers","value":[]}]`)) @@ -65,50 +65,14 @@ var _ = Describe("ComponentVersionReconciler", Ordered, func() { }) Eventually(func(g Gomega) { - updatedCV := &solarv1alpha1.ComponentVersion{} - g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cv), updatedCV)).To(Succeed()) - g.Expect(updatedCV.Finalizers).To(ContainElement(componentVersionFinalizer)) - - updatedComp := &solarv1alpha1.Component{} - g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), updatedComp)).To(Succeed()) - g.Expect(updatedComp.Finalizers).To(ContainElement(componentRefFinalizer)) - }, eventuallyTimeout).Should(Succeed()) - }) - - It("blocks Component deletion while ComponentVersion references it", func() { - comp := validComponent("dp-comp-blocked") - Expect(k8sClient.Create(ctx, comp)).To(Succeed()) - - cv := validCV("dp-cv-blocked", comp.Name) - Expect(k8sClient.Create(ctx, cv)).To(Succeed()) - - // Wait for the protection finalizer to be added. - Eventually(func(g Gomega) { - updated := &solarv1alpha1.Component{} - g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), updated)).To(Succeed()) - g.Expect(updated.Finalizers).To(ContainElement(componentRefFinalizer)) + updated := &solarv1alpha1.ComponentVersion{} + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cv), updated)).To(Succeed()) + g.Expect(updated.Finalizers).To(ContainElement(componentVersionFinalizer)) }, eventuallyTimeout).Should(Succeed()) - - // Delete Component — it should be blocked (DeletionTimestamp set, not gone). - Expect(k8sClient.Delete(ctx, comp)).To(Succeed()) - - Consistently(func(g Gomega) { - updated := &solarv1alpha1.Component{} - g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), updated)).To(Succeed()) - g.Expect(updated.DeletionTimestamp).NotTo(BeNil()) - }, consistentlyDuration).Should(Succeed()) - - // Delete the ComponentVersion — controller removes componentRefFinalizer from Component, - // then removes componentVersionFinalizer, unblocking Component deletion. - Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, cv))).To(Succeed()) - - Eventually(func() bool { - return apierrors.IsNotFound(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), &solarv1alpha1.Component{})) - }, eventuallyTimeout).Should(BeTrue()) }) - It("removes componentRefFinalizer from Component when last ComponentVersion is deleted", func() { - comp := validComponent("dp-comp-last") + It("removes componentVersionFinalizer when the ComponentVersion is deleted", func() { + comp := validComponent("cvf-comp-del") Expect(k8sClient.Create(ctx, comp)).To(Succeed()) DeferCleanup(func() { patch := client.RawPatch(types.JSONPatchType, []byte(`[{"op":"replace","path":"/metadata/finalizers","value":[]}]`)) @@ -116,74 +80,20 @@ var _ = Describe("ComponentVersionReconciler", Ordered, func() { _ = client.IgnoreNotFound(k8sClient.Delete(ctx, comp)) }) - cv := validCV("dp-cv-last", comp.Name) + cv := validCV("cvf-cv-del", comp.Name) Expect(k8sClient.Create(ctx, cv)).To(Succeed()) - // Wait for protection finalizer. Eventually(func(g Gomega) { - updated := &solarv1alpha1.Component{} - g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), updated)).To(Succeed()) - g.Expect(updated.Finalizers).To(ContainElement(componentRefFinalizer)) + updated := &solarv1alpha1.ComponentVersion{} + g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(cv), updated)).To(Succeed()) + g.Expect(updated.Finalizers).To(ContainElement(componentVersionFinalizer)) }, eventuallyTimeout).Should(Succeed()) - // Delete the ComponentVersion — the controller's deletion handler removes componentRefFinalizer - // from Component (no other references), then removes componentVersionFinalizer. - Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, cv))).To(Succeed()) + Expect(k8sClient.Delete(ctx, cv)).To(Succeed()) - // The componentRefFinalizer should eventually be removed from Component. - Eventually(func(g Gomega) { - updated := &solarv1alpha1.Component{} - g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), updated)).To(Succeed()) - g.Expect(updated.Finalizers).NotTo(ContainElement(componentRefFinalizer)) - }, eventuallyTimeout).Should(Succeed()) - }) - - It("retains componentRefFinalizer when a second ComponentVersion still references the Component", func() { - comp := validComponent("dp-comp-multi") - Expect(k8sClient.Create(ctx, comp)).To(Succeed()) - DeferCleanup(func() { - patch := client.RawPatch(types.JSONPatchType, []byte(`[{"op":"replace","path":"/metadata/finalizers","value":[]}]`)) - _ = client.IgnoreNotFound(k8sClient.Patch(ctx, comp, patch)) - _ = client.IgnoreNotFound(k8sClient.Delete(ctx, comp)) - }) - - cv1 := validCV("dp-cv-multi-1", comp.Name) - Expect(k8sClient.Create(ctx, cv1)).To(Succeed()) - DeferCleanup(func() { - patch := client.RawPatch(types.JSONPatchType, []byte(`[{"op":"replace","path":"/metadata/finalizers","value":[]}]`)) - _ = client.IgnoreNotFound(k8sClient.Patch(ctx, cv1, patch)) - _ = client.IgnoreNotFound(k8sClient.Delete(ctx, cv1)) - }) - - cv2 := validCV("dp-cv-multi-2", comp.Name) - Expect(k8sClient.Create(ctx, cv2)).To(Succeed()) - DeferCleanup(func() { - patch := client.RawPatch(types.JSONPatchType, []byte(`[{"op":"replace","path":"/metadata/finalizers","value":[]}]`)) - _ = client.IgnoreNotFound(k8sClient.Patch(ctx, cv2, patch)) - _ = client.IgnoreNotFound(k8sClient.Delete(ctx, cv2)) - }) - - // Wait for the protection finalizer to be established. - Eventually(func(g Gomega) { - updated := &solarv1alpha1.Component{} - g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), updated)).To(Succeed()) - g.Expect(updated.Finalizers).To(ContainElement(componentRefFinalizer)) - }, eventuallyTimeout).Should(Succeed()) - - // Delete cv1 — cv2 still holds the reference, so component-ref must stay. - Expect(k8sClient.Delete(ctx, cv1)).To(Succeed()) - - // Wait for cv1 to be fully gone from the API. Eventually(func() bool { - return apierrors.IsNotFound(k8sClient.Get(ctx, client.ObjectKeyFromObject(cv1), &solarv1alpha1.ComponentVersion{})) + return apierrors.IsNotFound(k8sClient.Get(ctx, client.ObjectKeyFromObject(cv), &solarv1alpha1.ComponentVersion{})) }, eventuallyTimeout).Should(BeTrue()) - - // component-ref must remain because cv2 still references the Component. - Consistently(func(g Gomega) { - updated := &solarv1alpha1.Component{} - g.Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(comp), updated)).To(Succeed()) - g.Expect(updated.Finalizers).To(ContainElement(componentRefFinalizer)) - }, consistentlyDuration).Should(Succeed()) }) }) }) diff --git a/pkg/controller/suite_test.go b/pkg/controller/suite_test.go index fc6c2fee..de8086e2 100644 --- a/pkg/controller/suite_test.go +++ b/pkg/controller/suite_test.go @@ -50,6 +50,7 @@ var ( profileReconciler *ProfileReconciler renderArtifactReconciler *RenderArtifactReconciler componentVersionReconciler *ComponentVersionReconciler + componentReconciler *ComponentReconciler releaseBindingReconciler *ReleaseBindingReconciler registryBindingReconciler *RegistryBindingReconciler @@ -171,6 +172,13 @@ var _ = BeforeSuite(func() { } Expect(componentVersionReconciler.SetupWithManager(mgr)).To(Succeed()) + componentReconciler = &ComponentReconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + APIReader: mgr.GetAPIReader(), + } + Expect(componentReconciler.SetupWithManager(mgr)).To(Succeed()) + releaseBindingReconciler = &ReleaseBindingReconciler{ Client: mgr.GetClient(), Scheme: mgr.GetScheme(), @@ -214,6 +222,7 @@ var _ = BeforeEach(func() { profileReconciler.WatchNamespace = nsName renderArtifactReconciler.WatchNamespace = nsName componentVersionReconciler.WatchNamespace = nsName + componentReconciler.WatchNamespace = nsName releaseBindingReconciler.WatchNamespace = nsName registryBindingReconciler.WatchNamespace = nsName // Reset the fake deleter state for each test @@ -228,6 +237,7 @@ var _ = AfterEach(func() { profileReconciler.WatchNamespace = "cleanup-disabled" renderArtifactReconciler.WatchNamespace = "cleanup-disabled" componentVersionReconciler.WatchNamespace = "cleanup-disabled" + componentReconciler.WatchNamespace = "cleanup-disabled" releaseBindingReconciler.WatchNamespace = "cleanup-disabled" registryBindingReconciler.WatchNamespace = "cleanup-disabled" @@ -390,6 +400,7 @@ var _ = AfterEach(func() { profileReconciler.WatchNamespace = "" renderArtifactReconciler.WatchNamespace = "" componentVersionReconciler.WatchNamespace = "" + componentReconciler.WatchNamespace = "" releaseBindingReconciler.WatchNamespace = "" registryBindingReconciler.WatchNamespace = "" }) diff --git a/pkg/controller/target_controller_test.go b/pkg/controller/target_controller_test.go index 4faa365a..dfecdda2 100644 --- a/pkg/controller/target_controller_test.go +++ b/pkg/controller/target_controller_test.go @@ -2074,9 +2074,13 @@ var _ = Describe("resolveComponentSource", func() { createComponent("opendefense.cloud/demo") createRegistry("Registry.Example.COM", &corev1.LocalObjectReference{Name: "source-creds"}) - _, secretRef, err := targetReconciler.resolveComponentSource(ctx, cv, sourceNs.Name) - Expect(err).NotTo(HaveOccurred()) - Expect(secretRef).NotTo(BeNil()) + // resolveComponentSource reads via the informer cache, so the objects + // created above may not be visible synchronously. + Eventually(func(g Gomega) { + _, secretRef, err := targetReconciler.resolveComponentSource(ctx, cv, sourceNs.Name) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(secretRef).NotTo(BeNil()) + }, eventuallyTimeout).Should(Succeed()) }) It("returns the ref with no secret when no Registry matches", func() { @@ -2118,13 +2122,17 @@ var _ = Describe("resolveComponentSource", func() { createRegistry("registry.example.com", &corev1.LocalObjectReference{Name: "catalog-only-creds"}) createRegistryIn(renderNs.Name, "registry.example.com", &corev1.LocalObjectReference{Name: "render-ns-creds"}) - ref, secretRef, err := targetReconciler.resolveComponentSource(ctx, cv, renderNs.Name) - Expect(err).NotTo(HaveOccurred()) - Expect(ref).To(Equal("https://registry.example.com/components//opendefense.cloud/demo:v1.0.0")) - Expect(secretRef).NotTo(BeNil()) - // The RenderTask, and so the render Job, lives in renderNs — a Pod - // cannot mount catalog-only-creds from sourceNs. - Expect(secretRef.Name).To(Equal("render-ns-creds")) + // resolveComponentSource reads via the informer cache, so the objects + // created above may not be visible synchronously. + Eventually(func(g Gomega) { + ref, secretRef, err := targetReconciler.resolveComponentSource(ctx, cv, renderNs.Name) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(ref).To(Equal("https://registry.example.com/components//opendefense.cloud/demo:v1.0.0")) + g.Expect(secretRef).NotTo(BeNil()) + // The RenderTask, and so the render Job, lives in renderNs — a Pod + // cannot mount catalog-only-creds from sourceNs. + g.Expect(secretRef.Name).To(Equal("render-ns-creds")) + }, eventuallyTimeout).Should(Succeed()) }) It("reads anonymously when only the component's namespace has a Registry", func() { diff --git a/pkg/discovery/apiwriter/apiwriter.go b/pkg/discovery/apiwriter/apiwriter.go index 9870287f..78eeb2f3 100644 --- a/pkg/discovery/apiwriter/apiwriter.go +++ b/pkg/discovery/apiwriter/apiwriter.go @@ -225,37 +225,6 @@ func (rs *APIWriter) deleteComponentVersion(ctx context.Context, ev discovery.Wr return fmt.Errorf("failed to delete component version %s: %w", cv.Name, err) } rs.Logger().Info("deleted component version", "name", cv.Name, "digest", digest) - - // Clean up parent component if no other versions reference it. - parent := cv.Labels[componentLabel] - if parent != "" { - parentMatch := map[string]string{ - componentLabel: parent, - } - remaining, err := rs.client.ComponentVersions(rs.namespace).List(ctx, metav1.ListOptions{ - LabelSelector: labels.Set(parentMatch).String(), - }) - if err != nil { - return err - } - // The CV we just deleted still appears here in Terminating state (it carries - // a finalizer), so exclude it and any other terminating CVs; otherwise the - // parent Component delete is skipped and the Component is orphaned forever. - // FIXME: replace this inferred cleanup with a Component reconciler that - // owns deletion serialized per-Component (re-creation-safe follow-up). - active := 0 - for _, r := range remaining.Items { - if r.Name == cv.Name || !r.DeletionTimestamp.IsZero() { - continue - } - active++ - } - if active == 0 { - if err := client.IgnoreNotFound(rs.client.Components(rs.namespace).Delete(ctx, parent, metav1.DeleteOptions{})); err != nil { - return err - } - } - } } return nil diff --git a/pkg/discovery/apiwriter/apiwriter_test.go b/pkg/discovery/apiwriter/apiwriter_test.go index 368b19fc..2d37cae5 100644 --- a/pkg/discovery/apiwriter/apiwriter_test.go +++ b/pkg/discovery/apiwriter/apiwriter_test.go @@ -365,7 +365,7 @@ var _ = Describe("APIWriter", Ordered, func() { }) Describe("Deletion", func() { - It("should delete ComponentVersion and Component when a delete event is received", func() { + It("should delete only the ComponentVersion when a delete event is received", func() { Expect(writer.Start(ctx)).To(Succeed()) inputChan <- createEvent(discovery.EventCreated) Eventually(func() error { @@ -397,17 +397,11 @@ var _ = Describe("APIWriter", Ordered, func() { }).Should(HaveOccurred()) Expect(err.Error()).To(ContainSubstring("not found")) - Eventually(func() error { - select { - case errEvent := <-errChan: - Expect(errEvent.Error).NotTo(HaveOccurred()) - default: - } - _, err = solarClient.Components("default").Get(ctx, "opendefense-cloud-ocm-demo", metav1.GetOptions{}) - - return err - }).Should(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("not found")) + // The apiwriter no longer infers Component cleanup; that is the + // ComponentReconciler's job in a running cluster. The fake clientset + // used here has no reconciler, so the Component must remain. + _, err = solarClient.Components("default").Get(ctx, "opendefense-cloud-ocm-demo", metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) }) It("should delete ComponentVersion but keep Component when a delete event is received", func() { @@ -457,54 +451,5 @@ var _ = Describe("APIWriter", Ordered, func() { _, err := solarClient.Components("default").Get(ctx, "opendefense-cloud-ocm-demo", metav1.GetOptions{}) Expect(err).NotTo(HaveOccurred()) }) - - It("should delete the parent Component when the only sibling ComponentVersion is already terminating", func() { - Expect(writer.Start(ctx)).To(Succeed()) - - // Create the CV + Component via a normal create event. Wait for both so - // the later NotFound assertion can't pass merely because the Component - // was never created. - inputChan <- createEvent(discovery.EventCreated) - Eventually(func() error { - if _, err := solarClient.ComponentVersions("default").Get(ctx, "opendefense-cloud-ocm-demo-v26-4-2", metav1.GetOptions{}); err != nil { - return err - } - _, err := solarClient.Components("default").Get(ctx, "opendefense-cloud-ocm-demo", metav1.GetOptions{}) - - return err - }).ShouldNot(HaveOccurred()) - - // Seed a sibling CV for the same Component that is already terminating - // (deletionTimestamp + finalizer set). The real apiserver leaves such a - // CV listable until its finalizer clears, so it must NOT count as an - // active reference keeping the parent Component alive — otherwise the - // Component is orphaned forever (the e2e webhook GC failure). - now := metav1.Now() - terminating := &solarv1alpha1.ComponentVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "opendefense-cloud-ocm-demo-v26-5-0", - Namespace: "default", - DeletionTimestamp: &now, - Finalizers: []string{"solar.opendefense.cloud/componentversion-finalizer"}, - Labels: map[string]string{componentLabel: "opendefense-cloud-ocm-demo"}, - }, - } - _, err := solarClient.ComponentVersions("default").Create(ctx, terminating, metav1.CreateOptions{}) - Expect(err).NotTo(HaveOccurred()) - - // Delete the active CV; its only sibling is terminating, so the parent - // Component must be garbage-collected. - inputChan <- createEvent(discovery.EventDeleted) - Eventually(func() bool { - select { - case errEvent := <-errChan: - Expect(errEvent.Error).NotTo(HaveOccurred()) - default: - } - _, err := solarClient.Components("default").Get(ctx, "opendefense-cloud-ocm-demo", metav1.GetOptions{}) - - return apierrors.IsNotFound(err) - }).Should(BeTrue(), "parent Component should be deleted when its only sibling CV is terminating") - }) }) })