Refactor Boxcutter controller to rely on kubernetes for resource cleanup (#2451)

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>
Co-authored-by: Per Goncalves da Silva <pegoncal@redhat.com>

Author: perdasilva

internal/operator-controller/controllers/clusterextensionrevision_controller.go

@@ -141,7 +141,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 	}
 
 	if !rev.DeletionTimestamp.IsZero() || rev.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived {
-		return c.teardown(ctx, rev, revision)
+		return c.teardown(ctx, rev)
 	}
 
 	revVersion := rev.GetAnnotations()[labels.BundleVersionKey]
@@ -275,33 +275,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 	return ctrl.Result{}, nil
 }
 
-func (c *ClusterExtensionRevisionReconciler) teardown(ctx context.Context, rev *ocv1.ClusterExtensionRevision, revision *boxcutter.Revision) (ctrl.Result, error) {
-	l := log.FromContext(ctx)
-
-	revisionEngine, err := c.RevisionEngineFactory.CreateRevisionEngine(ctx, rev)
-	if err != nil {
-		markAsAvailableUnknown(rev, ocv1.ClusterExtensionRevisionReasonReconciling, err.Error())
-		return ctrl.Result{}, fmt.Errorf("failed to create revision engine for teardown: %v", err)
-	}
-
-	tres, err := revisionEngine.Teardown(ctx, *revision)
-	if err != nil {
-		if tres != nil {
-			l.V(1).Info("teardown failure report", "report", tres.String())
-		}
-		markAsAvailableUnknown(rev, ocv1.ClusterExtensionRevisionReasonReconciling, err.Error())
-		return ctrl.Result{}, fmt.Errorf("revision teardown: %v", err)
-	}
-
-	// Log detailed teardown reports only in debug mode (V(1)) to reduce verbosity.
-	l.V(1).Info("teardown report", "report", tres.String())
-	if !tres.IsComplete() {
-		// TODO: If it is not complete, it seems like it would be good to update
-		//  the status in some way to tell the user that the teardown is still
-		//  in progress.
-		return ctrl.Result{}, nil
-	}
-
+func (c *ClusterExtensionRevisionReconciler) teardown(ctx context.Context, rev *ocv1.ClusterExtensionRevision) (ctrl.Result, error) {
 	if err := c.TrackingCache.Free(ctx, rev); err != nil {
 		markAsAvailableUnknown(rev, ocv1.ClusterExtensionRevisionReasonReconciling, err.Error())
 		return ctrl.Result{}, fmt.Errorf("error stopping informers: %v", err)

internal/operator-controller/controllers/clusterextensionrevision_controller_test.go

@@ -697,40 +697,6 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) {
 				require.True(t, apierrors.IsNotFound(err))
 			},
 		},
-		{
-			name:           "set Available:Unknown:Reconciling and surface tear down errors when deleted",
-			revisionResult: mockRevisionResult{},
-			existingObjs: func() []client.Object {
-				ext := newTestClusterExtension()
-				rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName, ext, testScheme)
-				rev1.Finalizers = []string{
-					"olm.operatorframework.io/teardown",
-				}
-				rev1.DeletionTimestamp = &metav1.Time{Time: time.Now()}
-				return []client.Object{rev1, ext}
-			},
-			revisionEngineTeardownFn: func(t *testing.T) func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) {
-				return func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) {
-					return nil, fmt.Errorf("some teardown error")
-				}
-			},
-			expectedErr: "some teardown error",
-			validate: func(t *testing.T, c client.Client) {
-				t.Log("cluster revision is not deleted and still contains finalizer")
-				rev := &ocv1.ClusterExtensionRevision{}
-				err := c.Get(t.Context(), client.ObjectKey{
-					Name: clusterExtensionRevisionName,
-				}, rev)
-				require.NoError(t, err)
-				require.NotContains(t, "olm.operatorframework.io/teardown", rev.Finalizers)
-				cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.ClusterExtensionRevisionTypeAvailable)
-				require.NotNil(t, cond)
-				require.Equal(t, metav1.ConditionUnknown, cond.Status)
-				require.Equal(t, ocv1.ClusterExtensionRevisionReasonReconciling, cond.Reason)
-				require.Equal(t, "some teardown error", cond.Message)
-				require.Equal(t, int64(1), cond.ObservedGeneration)
-			},
-		},
 		{
 			name:           "set Available:Unknown:Reconciling and surface tracking cache cleanup errors when deleted",
 			revisionResult: mockRevisionResult{},
@@ -851,34 +817,6 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) {
 				require.NotContains(t, rev.Finalizers, "olm.operatorframework.io/teardown")
 			},
 		},
-		{
-			name:           "surfaces revision teardown error when in archived state",
-			revisionResult: mockRevisionResult{},
-			existingObjs: func() []client.Object {
-				ext := newTestClusterExtension()
-				rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName, ext, testScheme)
-				rev1.Finalizers = []string{
-					"olm.operatorframework.io/teardown",
-				}
-				rev1.Spec.LifecycleState = ocv1.ClusterExtensionRevisionLifecycleStateArchived
-				return []client.Object{rev1, ext}
-			},
-			revisionEngineTeardownFn: func(t *testing.T) func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) {
-				return func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionTeardownOption) (machinery.RevisionTeardownResult, error) {
-					return nil, fmt.Errorf("some teardown error")
-				}
-			},
-			expectedErr: "some teardown error",
-			validate: func(t *testing.T, c client.Client) {
-				t.Log("cluster revision is not deleted and still contains finalizer")
-				rev := &ocv1.ClusterExtensionRevision{}
-				err := c.Get(t.Context(), client.ObjectKey{
-					Name: clusterExtensionRevisionName,
-				}, rev)
-				require.NoError(t, err)
-				require.NotContains(t, "olm.operatorframework.io/teardown", rev.Finalizers)
-			},
-		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			// create extension and cluster extension
@@ -1439,34 +1377,4 @@ func Test_ClusterExtensionRevisionReconciler_getScopedClient_Errors(t *testing.T
 		require.Contains(t, err.Error(), "failed to create revision engine")
 		require.Contains(t, err.Error(), "token getter failed")
 	})
-
-	t.Run("factory fails during teardown", func(t *testing.T) {
-		ext := newTestClusterExtension()
-		rev := newTestClusterExtensionRevision(t, "test-rev", ext, testScheme)
-		rev.DeletionTimestamp = &metav1.Time{Time: time.Now()}
-		rev.Finalizers = []string{"olm.operatorframework.io/teardown"}
-
-		testClient := fake.NewClientBuilder().
-			WithScheme(testScheme).
-			WithObjects(ext, rev).
-			Build()
-
-		failingFactory := &mockRevisionEngineFactory{
-			createErr: errors.New("serviceaccount not found"),
-		}
-
-		reconciler := &controllers.ClusterExtensionRevisionReconciler{
-			Client:                testClient,
-			RevisionEngineFactory: failingFactory,
-			TrackingCache:         &mockTrackingCache{client: testClient},
-		}
-
-		_, err := reconciler.Reconcile(t.Context(), ctrl.Request{
-			NamespacedName: types.NamespacedName{Name: "test-rev"},
-		})
-
-		require.Error(t, err)
-		require.Contains(t, err.Error(), "failed to create revision engine for teardown")
-		require.Contains(t, err.Error(), "serviceaccount not found")
-	})
 }