Delete the package container image when CFPackage is deleted

Issue: #2228
Co-authored-by: Giuseppe Capizzi <gcapizzi@vmware.com>

INSTALL.EKS.md

@@ -97,7 +97,9 @@ cat >ecr-policy.json <<EOF
         "ecr:InitiateLayerUpload",
         "ecr:PutImage",
         "ecr:UploadLayerPart",
-        "ecr:CreateRepository"
+        "ecr:CreateRepository",
+        "ecr:ListImages",
+        "ecr:BatchDeleteImage"
       ],
       "Resource": "*"
     }

controllers/controllers/workloads/cfpackage_controller.go

@@ -31,47 +31,91 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
+const cfPackageFinalizer string = "korifi.cloudfoundry.org/cfPackageController"
+
+//counterfeiter:generate -o fake -fake-name ImageDeleter . ImageDeleter
+
+type ImageDeleter interface {
+	Delete(ctx context.Context, imageRef string) error
+}
+
 // CFPackageReconciler reconciles a CFPackage object
 type CFPackageReconciler struct {
-	k8sClient client.Client
-	scheme    *runtime.Scheme
-	log       logr.Logger
+	k8sClient    client.Client
+	imageDeleter ImageDeleter
+	scheme       *runtime.Scheme
+	log          logr.Logger
 }
 
-func NewCFPackageReconciler(client client.Client, scheme *runtime.Scheme, log logr.Logger) *k8s.PatchingReconciler[korifiv1alpha1.CFPackage, *korifiv1alpha1.CFPackage] {
-	pkgReconciler := CFPackageReconciler{k8sClient: client, scheme: scheme, log: log}
+func NewCFPackageReconciler(
+	client client.Client,
+	imageDeleter ImageDeleter,
+	scheme *runtime.Scheme,
+	log logr.Logger,
+) *k8s.PatchingReconciler[korifiv1alpha1.CFPackage, *korifiv1alpha1.CFPackage] {
+	pkgReconciler := CFPackageReconciler{
+		k8sClient:    client,
+		imageDeleter: imageDeleter,
+		scheme:       scheme,
+		log:          log,
+	}
+
 	return k8s.NewPatchingReconciler[korifiv1alpha1.CFPackage, *korifiv1alpha1.CFPackage](log, client, &pkgReconciler)
 }
 
 //+kubebuilder:rbac:groups=korifi.cloudfoundry.org,resources=cfpackages,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=korifi.cloudfoundry.org,resources=cfpackages/status,verbs=get;update;patch
+//+kubebuilder:rbac:groups=korifi.cloudfoundry.org,resources=cfpackages/finalizers,verbs=get;update;patch
 
-// Reconcile is part of the main kubernetes reconciliation loop which aims to
-// move the current state of the cluster closer to the desired state.
-// TODO(user): Modify the Reconcile function to compare the state specified by
-// the CFPackage object against the actual cluster state, and then
-// perform operations to make the cluster state reflect the state specified by
-// the user.
-//
-// For more details, check Reconcile and its Result here:
-// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.8.3/pkg/reconcile
 func (r *CFPackageReconciler) ReconcileResource(ctx context.Context, cfPackage *korifiv1alpha1.CFPackage) (ctrl.Result, error) {
+	log := r.log.WithValues("namespace", cfPackage.Namespace, "name", cfPackage.Name)
+
+	if !cfPackage.GetDeletionTimestamp().IsZero() {
+		return r.finalize(ctx, log, cfPackage)
+	}
+
+	err := k8s.AddFinalizer(ctx, log, r.k8sClient, cfPackage, cfPackageFinalizer)
+	if err != nil {
+		log.Error(err, "Error adding finalizer")
+		return ctrl.Result{}, err
+	}
+
 	var cfApp korifiv1alpha1.CFApp
-	err := r.k8sClient.Get(ctx, types.NamespacedName{Name: cfPackage.Spec.AppRef.Name, Namespace: cfPackage.Namespace}, &cfApp)
+	err = r.k8sClient.Get(ctx, types.NamespacedName{Name: cfPackage.Spec.AppRef.Name, Namespace: cfPackage.Namespace}, &cfApp)
 	if err != nil {
-		r.log.Info("error when fetching CFApp", "reason", err)
+		log.Info("error when fetching CFApp", "reason", err)
 		return ctrl.Result{}, err
 	}
 
 	err = controllerutil.SetControllerReference(&cfApp, cfPackage, r.scheme)
 	if err != nil {
-		r.log.Info("unable to set owner reference on CFPackage", "reason", err)
+		log.Info("unable to set owner reference on CFPackage", "reason", err)
 		return ctrl.Result{}, err
 	}
 
 	return ctrl.Result{}, nil
 }
 
+func (r *CFPackageReconciler) finalize(ctx context.Context, log logr.Logger, cfPackage *korifiv1alpha1.CFPackage) (ctrl.Result, error) {
+	log = log.WithName("finalize")
+
+	if !controllerutil.ContainsFinalizer(cfPackage, cfPackageFinalizer) {
+		return ctrl.Result{}, nil
+	}
+
+	if cfPackage.Spec.Source.Registry.Image != "" {
+		if err := r.imageDeleter.Delete(ctx, cfPackage.Spec.Source.Registry.Image); err != nil {
+			log.Info("failed to delete image", "reason", err)
+		}
+	}
+
+	if controllerutil.RemoveFinalizer(cfPackage, cfPackageFinalizer) {
+		log.Info("finalizer removed")
+	}
+
+	return ctrl.Result{}, nil
+}
+
 // SetupWithManager sets up the controller with the Manager.
 func (r *CFPackageReconciler) SetupWithManager(mgr ctrl.Manager) *builder.Builder {
 	return ctrl.NewControllerManagedBy(mgr).

controllers/controllers/workloads/cfpackage_controller_test.go

@@ -2,8 +2,10 @@ package workloads_test
 
 import (
 	"context"
+	"errors"
 
 	"code.cloudfoundry.org/korifi/tools"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -30,6 +32,7 @@ var _ = Describe("CFPackageReconciler Integration Tests", func() {
 		cfPackageGUID = GenerateGUID()
 
 		cfApp = BuildCFAppCRObject(cfAppGUID, cfSpace.Status.GUID)
+
 		Expect(k8sClient.Create(context.Background(), cfApp)).To(Succeed())
 	})
 
@@ -57,4 +60,59 @@ var _ = Describe("CFPackageReconciler Integration Tests", func() {
 			}))
 		})
 	})
+
+	When("a CFPackage is deleted", func() {
+		BeforeEach(func() {
+			cfPackage = BuildCFPackageCRObject(cfPackageGUID, cfSpace.Status.GUID, cfAppGUID)
+		})
+
+		JustBeforeEach(func() {
+			Expect(k8sClient.Create(context.Background(), cfPackage)).To(Succeed())
+
+			// wait for package to have reconciled at least once
+			Eventually(func(g Gomega) {
+				g.Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(cfPackage), cfPackage)).To(Succeed())
+				g.Expect(cfPackage.Finalizers).ToNot(BeEmpty())
+			}).Should(Succeed())
+
+			Expect(k8sClient.Delete(context.Background(), cfPackage)).To(Succeed())
+		})
+
+		It("deletes itself and the corresponding source image", func() {
+			Eventually(func(g Gomega) {
+				g.Expect(imageDeleter.DeleteCallCount()).To(Equal(1))
+			}).Should(Succeed())
+
+			_, ref := imageDeleter.DeleteArgsForCall(0)
+			Expect(ref).To(Equal("PACKAGE_IMAGE"))
+
+			Eventually(func(g Gomega) {
+				g.Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(cfPackage), cfPackage)).To(MatchError(ContainSubstring("not found")))
+			}).Should(Succeed())
+		})
+
+		When("the package doesn't have an image set", func() {
+			BeforeEach(func() {
+				cfPackage.Spec.Source.Registry.Image = ""
+			})
+
+			It("doesn't try to delete any image", func() {
+				Consistently(func(g Gomega) {
+					g.Expect(imageDeleter.DeleteCallCount()).To(Equal(0))
+				}).Should(Succeed())
+			})
+		})
+
+		When("deletion fails", func() {
+			BeforeEach(func() {
+				imageDeleter.DeleteReturns(errors.New("oops"))
+			})
+
+			It("ignores the errors and finishes finalization", func() {
+				Eventually(func(g Gomega) {
+					g.Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(cfPackage), cfPackage)).To(MatchError(ContainSubstring("not found")))
+				}).Should(Succeed())
+			})
+		})
+	})
 })

controllers/controllers/workloads/suite_test.go

@@ -11,6 +11,7 @@ import (
 	. "code.cloudfoundry.org/korifi/controllers/controllers/shared"
 	. "code.cloudfoundry.org/korifi/controllers/controllers/workloads"
 	"code.cloudfoundry.org/korifi/controllers/controllers/workloads/env"
+	"code.cloudfoundry.org/korifi/controllers/controllers/workloads/fake"
 	"code.cloudfoundry.org/korifi/controllers/controllers/workloads/labels"
 	"code.cloudfoundry.org/korifi/controllers/controllers/workloads/testutils"
 	"code.cloudfoundry.org/korifi/tools/k8s"
@@ -42,6 +43,7 @@ var (
 	cfRootNamespace     string
 	cfOrg               *korifiv1alpha1.CFOrg
 	imageRegistrySecret *corev1.Secret
+	imageDeleter        *fake.ImageDeleter
 )
 
 const (
@@ -78,10 +80,6 @@ var _ = BeforeSuite(func() {
 
 	//+kubebuilder:scaffold:scheme
 
-	k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme})
-	Expect(err).NotTo(HaveOccurred())
-	Expect(k8sClient).NotTo(BeNil())
-
 	webhookInstallOptions := &testEnv.WebhookInstallOptions
 	k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{
 		Scheme:             scheme.Scheme,
@@ -93,6 +91,8 @@ var _ = BeforeSuite(func() {
 	})
 	Expect(err).NotTo(HaveOccurred())
 
+	k8sClient = k8sManager.GetClient()
+
 	cfRootNamespace = testutils.PrefixedGUID("root-namespace")
 
 	controllerConfig := &config.ControllerConfig{
@@ -137,8 +137,10 @@ var _ = BeforeSuite(func() {
 	)).SetupWithManager(k8sManager)
 	Expect(err).NotTo(HaveOccurred())
 
+	imageDeleter = new(fake.ImageDeleter)
 	err = (NewCFPackageReconciler(
 		k8sManager.GetClient(),
+		imageDeleter,
 		k8sManager.GetScheme(),
 		ctrl.Log.WithName("controllers").WithName("CFPackage"),
 	)).SetupWithManager(k8sManager)

controllers/main.go

@@ -43,6 +43,7 @@ import (
 	statesetfulrunnerv1 "code.cloudfoundry.org/korifi/statefulset-runner/api/v1"
 	statefulsetcontrollers "code.cloudfoundry.org/korifi/statefulset-runner/controllers"
 	"code.cloudfoundry.org/korifi/tools"
+	"code.cloudfoundry.org/korifi/tools/image"
 	"code.cloudfoundry.org/korifi/tools/registry"
 
 	buildv1alpha2 "github.com/pivotal/kpack/pkg/apis/build/v1alpha2"
@@ -159,6 +160,7 @@ func main() {
 
 		if err = (workloadscontrollers.NewCFPackageReconciler(
 			mgr.GetClient(),
+			image.NewClient(k8sClient, controllerConfig.CFRootNamespace, controllerConfig.ContainerRegistrySecretName),
 			mgr.GetScheme(),
 			ctrl.Log.WithName("controllers").WithName("CFPackage"),
 		)).SetupWithManager(mgr); err != nil {