Merge pull request #23 from mittwald/fix/delete-not-ready

delete a resource, even if it is not yet ready

pkg/controller/instance/instance_controller.go

@@ -5,6 +5,8 @@ import (
 	"errors"
 	"reflect"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
 	"github.com/mittwald/harbor-operator/pkg/config"
 	"github.com/mittwald/harbor-operator/pkg/controller/internal"
 
@@ -13,7 +15,6 @@ import (
 	registriesv1alpha1 "github.com/mittwald/harbor-operator/pkg/apis/registries/v1alpha1"
 	"github.com/mittwald/harbor-operator/pkg/internal/helper"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
@@ -114,18 +115,13 @@ func (r *ReconcileInstance) Reconcile(request reconcile.Request) (reconcile.Resu
 
 	originalInstance := harbor.DeepCopy()
 
-	// Add finalizers to the CR object
-	if harbor.DeletionTimestamp == nil {
-		var hasFinalizer bool
-		for i := range harbor.Finalizers {
-			if harbor.Finalizers[i] == FinalizerName {
-				hasFinalizer = true
-			}
-		}
-		if !hasFinalizer {
-			helper.PushFinalizer(harbor, FinalizerName)
-			return r.patchInstance(ctx, originalInstance, harbor)
-		}
+	if harbor.DeletionTimestamp != nil {
+		now := metav1.Now()
+		harbor.Status.Phase = registriesv1alpha1.InstanceStatusPhase{
+			Name:           registriesv1alpha1.InstanceStatusPhaseTerminating,
+			Message:        "Deleted",
+			LastTransition: &now}
+		return r.patchInstance(ctx, originalInstance, harbor)
 	}
 
 	switch harbor.Status.Phase.Name {
@@ -148,6 +144,8 @@ func (r *ReconcileInstance) Reconcile(request reconcile.Request) (reconcile.Resu
 			return reconcile.Result{}, err
 		}
 
+		helper.PushFinalizer(harbor, FinalizerName)
+
 		err = r.installOrUpgradeHelmChart(chartSpec)
 		if err != nil {
 			return reconcile.Result{}, err
@@ -169,15 +167,6 @@ func (r *ReconcileInstance) Reconcile(request reconcile.Request) (reconcile.Resu
 		}
 
 	case registriesv1alpha1.InstanceStatusPhaseReady:
-		if harbor.DeletionTimestamp != nil {
-			now := metav1.Now()
-			harbor.Status.Phase = registriesv1alpha1.InstanceStatusPhase{
-				Name:           registriesv1alpha1.InstanceStatusPhaseTerminating,
-				Message:        "Deleted",
-				LastTransition: &now}
-			return r.patchInstance(ctx, originalInstance, harbor)
-		}
-
 		if harbor.Spec.GarbageCollection != nil {
 			if err := r.reconcileGarbageCollection(ctx, harbor); err != nil {
 				return reconcile.Result{}, err
@@ -193,6 +182,7 @@ func (r *ReconcileInstance) Reconcile(request reconcile.Request) (reconcile.Resu
 		if err != nil {
 			return reconcile.Result{}, err
 		}
+
 		if harbor.Status.SpecHash != specHash {
 			harbor.Status.Phase.Name = registriesv1alpha1.InstanceStatusPhaseInstalling
 			harbor.Status.SpecHash = specHash

pkg/controller/instance/instance_controller_test.go

@@ -2,10 +2,11 @@ package instance
 
 import (
 	"context"
-	"github.com/mittwald/harbor-operator/pkg/internal/helper"
 	"testing"
 	"time"
 
+	"github.com/mittwald/harbor-operator/pkg/internal/helper"
+
 	"github.com/golang/mock/gomock"
 	helmclient "github.com/mittwald/go-helm-client"
 	helmclientmock "github.com/mittwald/go-helm-client/mock"
@@ -156,22 +157,6 @@ func TestInstanceController_Transition_Installing(t *testing.T) {
 		t.Error("reconciliation was not requeued")
 	}
 
-	res, err = r.Reconcile(req)
-	if err != nil {
-		t.Fatalf("reconcile returned error: (%v)", err)
-	}
-
-	instance = &registriesv1alpha1.Instance{}
-
-	err = r.client.Get(context.TODO(), req.NamespacedName, instance)
-	if err != nil {
-		t.Errorf("could not get instance: %v", err)
-	}
-
-	if !res.Requeue {
-		t.Error("reconciliation was not requeued")
-	}
-
 	if instance.Status.Phase.Name != registriesv1alpha1.InstanceStatusPhaseInstalling {
 		t.Errorf("instance status unexpected, status: %s, expected: %s", instance.Status.Phase.Name, registriesv1alpha1.InstanceStatusPhaseInstalling)
 	}
@@ -182,7 +167,6 @@ func TestInstanceController_Transition_Installing(t *testing.T) {
 func TestInstanceController_Instance_Installation(t *testing.T) {
 	i := testingregistriesv1alpha1.CreateInstance("harbor", "foobar")
 	i.Status.Phase.Name = registriesv1alpha1.InstanceStatusPhaseInstalling
-	i.DeletionTimestamp = &metav1.Time{Time: time.Now()}
 
 	chartSecret := testingregistriesv1alpha1.CreateSecret(i.Name+"-harbor-core", "foobar")
 	r := buildReconcileWithFakeClientWithMocks([]runtime.Object{&i, &chartSecret})
@@ -242,7 +226,6 @@ func TestInstanceController_Instance_Deletion(t *testing.T) {
 	i := testingregistriesv1alpha1.CreateInstance("harbor", "foobar")
 	i.Status.Phase.Name = registriesv1alpha1.InstanceStatusPhaseTerminating
 	i.SetFinalizers([]string{"harbor-operator.registries.mittwald.de"})
-	i.DeletionTimestamp = &metav1.Time{Time: time.Now()}
 
 	r := buildReconcileWithFakeClientWithMocks([]runtime.Object{&i})
 
@@ -380,12 +363,30 @@ func TestInstanceController_Add_Finalizer(t *testing.T) {
 
 	// Create mock instance + secret
 	i := testingregistriesv1alpha1.CreateInstance("test-instance", ns)
-	i.Status.Phase.Name = registriesv1alpha1.InstanceStatusPhaseReady
+	i.Status.Phase.Name = registriesv1alpha1.InstanceStatusPhaseInstalling
+	i.Spec.GarbageCollection = nil
 
 	instanceSecret := testingregistriesv1alpha1.CreateSecret(i.Name+"-harbor-core", ns)
 
 	r := buildReconcileWithFakeClientWithMocks([]runtime.Object{&i, &instanceSecret})
 
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+	mockClient := helmclientmock.NewMockClient(ctrl)
+	gomock.InOrder(
+		mockClient.EXPECT().UpdateChartRepos(),
+		mockClient.EXPECT().InstallOrUpgradeChart(&helmclient.ChartSpec{
+			ReleaseName: i.Spec.HelmChart.ReleaseName,
+			ChartName:   i.Spec.HelmChart.ChartName,
+			Namespace:   i.Spec.HelmChart.Namespace,
+			ValuesYaml:  i.Spec.HelmChart.ValuesYaml,
+			Version:     i.Spec.HelmChart.Version,
+		}),
+	)
+	r.helmClientReceiver = func(repoCache, repoConfig, namespace string) (helmclient.Client, error) {
+		return helmclient.Client(mockClient), nil
+	}
+
 	req := reconcile.Request{
 		NamespacedName: types.NamespacedName{
 			Name:      i.Name,
@@ -403,11 +404,9 @@ func TestInstanceController_Add_Finalizer(t *testing.T) {
 
 	instance := &registriesv1alpha1.Instance{}
 	err = r.client.Get(context.TODO(), req.NamespacedName, instance)
-
 	if err != nil {
 		t.Error("could not get instance")
 	}
-
 	if instance.Finalizers == nil || len(instance.Finalizers) == 0 {
 		t.Error("finalizer has not been added")
 	}
@@ -424,12 +423,30 @@ func TestInstanceController_Existing_Finalizer(t *testing.T) {
 
 	// Create mock instance + secret
 	i := testingregistriesv1alpha1.CreateInstance("test-instance", ns)
-	i.Status.Phase.Name = registriesv1alpha1.InstanceStatusPhaseReady
+	i.Status.Phase.Name = registriesv1alpha1.InstanceStatusPhaseInstalling
+	i.Spec.GarbageCollection = nil
 
 	instanceSecret := testingregistriesv1alpha1.CreateSecret(i.Name+"-harbor-core", ns)
 
 	r := buildReconcileWithFakeClientWithMocks([]runtime.Object{&i, &instanceSecret})
 
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+	mockClient := helmclientmock.NewMockClient(ctrl)
+	gomock.InOrder(
+		mockClient.EXPECT().UpdateChartRepos(),
+		mockClient.EXPECT().InstallOrUpgradeChart(&helmclient.ChartSpec{
+			ReleaseName: i.Spec.HelmChart.ReleaseName,
+			ChartName:   i.Spec.HelmChart.ChartName,
+			Namespace:   i.Spec.HelmChart.Namespace,
+			ValuesYaml:  i.Spec.HelmChart.ValuesYaml,
+			Version:     i.Spec.HelmChart.Version,
+		}),
+	)
+	r.helmClientReceiver = func(repoCache, repoConfig, namespace string) (helmclient.Client, error) {
+		return helmclient.Client(mockClient), nil
+	}
+
 	req := reconcile.Request{
 		NamespacedName: types.NamespacedName{
 			Name:      i.Name,

pkg/controller/registry/registry_controller.go

@@ -95,6 +95,11 @@ func (r *ReconcileRegistry) Reconcile(request reconcile.Request) (reconcile.Resu
 
 	originalRegistry := registry.DeepCopy()
 
+	if registry.ObjectMeta.DeletionTimestamp != nil {
+		registry.Status = registriesv1alpha1.RegistryStatus{Phase: registriesv1alpha1.RegistryStatusPhaseTerminating}
+		return r.patchRegistry(ctx, originalRegistry, registry)
+	}
+
 	// Fetch the Instance
 	harbor, err := internal.FetchReadyHarborInstance(ctx, registry.Namespace, registry.Spec.ParentInstance.Name, r.client)
 	if err != nil {
@@ -118,27 +123,15 @@ func (r *ReconcileRegistry) Reconcile(request reconcile.Request) (reconcile.Resu
 		return reconcile.Result{Requeue: true}, err
 	}
 
-	// Add finalizers to the CR object
-	if registry.DeletionTimestamp == nil {
-		var hasFinalizer bool
-		for i := range registry.Finalizers {
-			if registry.Finalizers[i] == FinalizerName {
-				hasFinalizer = true
-			}
-		}
-		if !hasFinalizer {
-			helper.PushFinalizer(registry, FinalizerName)
-			return r.patchRegistry(ctx, originalRegistry, registry)
-		}
-	}
-
 	switch registry.Status.Phase {
 	default:
 		return reconcile.Result{}, nil
 	case registriesv1alpha1.RegistryStatusPhaseUnknown:
 		registry.Status = registriesv1alpha1.RegistryStatus{Phase: registriesv1alpha1.RegistryStatusPhaseCreating}
 
 	case registriesv1alpha1.RegistryStatusPhaseCreating:
+		helper.PushFinalizer(registry, FinalizerName)
+
 		// Install the registry
 		err = r.assertExistingRegistry(harborClient, registry)
 		if err != nil {
@@ -147,13 +140,6 @@ func (r *ReconcileRegistry) Reconcile(request reconcile.Request) (reconcile.Resu
 
 		registry.Status = registriesv1alpha1.RegistryStatus{Phase: registriesv1alpha1.RegistryStatusPhaseReady}
 	case registriesv1alpha1.RegistryStatusPhaseReady:
-		// Compare the state of spec to the state of what the API returns
-		// If the Registry object is deleted, assume that the repository needs deletion, too
-		if registry.ObjectMeta.DeletionTimestamp != nil {
-			registry.Status = registriesv1alpha1.RegistryStatus{Phase: registriesv1alpha1.RegistryStatusPhaseTerminating}
-			return r.patchRegistry(ctx, originalRegistry, registry)
-		}
-
 		err := r.assertExistingRegistry(harborClient, registry)
 		if err != nil {
 			return reconcile.Result{}, err