Don't delete subns if anchor has conflict

See issue #1149. This affects the ability to turn a subnamespace into a
full namespace. If you remove the subnamespaceOf annotation, move the
namespace to a new parent, and *then* clean up the anchor, everything's
ok. But if you delete the anchor _before_ moving the namespace, the
namespace gets deleted, which is clearly wrong.

This change is the minimal safe change needed to fix this in v0.5. In
v0.6, we should restructure these functions to be more heavily based on
the explicit state of the anchor, not the implied state.

Tested: new e2e test fails (hangs, actually) without this change and
passes with it. All e2e tests pass on GKE 1.17.

Author: adrianludwin

incubator/hnc/internal/reconcilers/anchor.go

@@ -17,10 +17,11 @@ package reconcilers
 
 import (
 	"context"
+	"errors"
 
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -65,7 +66,7 @@ func (r *AnchorReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 	// purged.
 	inst, err := r.getInstance(ctx, pnm, nm)
 	if err != nil {
-		if errors.IsNotFound(err) {
+		if apierrors.IsNotFound(err) {
 			return ctrl.Result{}, nil
 		}
 		return ctrl.Result{}, err
@@ -118,13 +119,19 @@ func (r *AnchorReconciler) onDeleting(ctx context.Context, log logr.Logger, inst
 		return false, err
 	}
 
+	// Update the state of the anchor
+	//
+	// TODO: call this in the parent function after v0.5 so there's no special code path for the
+	// onDeleting case.
+	r.updateState(log, inst, snsInst)
+
 	log.Info("The anchor is being deleted", "deletingCRD", deletingCRD)
 	switch {
 	case len(inst.ObjectMeta.Finalizers) == 0:
 		// We've finished processing this, nothing to do.
 		log.Info("Do nothing since the finalizers are already gone.")
 		return true, nil
-	case r.shouldDeleteSubns(inst, snsInst, deletingCRD):
+	case r.shouldDeleteSubns(log, inst, snsInst, deletingCRD):
 		// The subnamespace is not already being deleted but it allows cascadingDelete or it's a leaf.
 		// Delete the subnamespace, unless the CRD is being deleted, in which case, we want to leave the
 		// namespaces alone.
@@ -143,7 +150,9 @@ func (r *AnchorReconciler) onDeleting(ctx context.Context, log logr.Logger, inst
 
 // shouldDeleteSubns returns true if the namespace still exists and it is a leaf
 // subnamespace or it allows cascading delete unless the CRD is being deleted.
-func (r *AnchorReconciler) shouldDeleteSubns(inst *api.SubnamespaceAnchor, nsInst *corev1.Namespace, deletingCRD bool) bool {
+//
+// TODO: fix comment post-v0.5
+func (r *AnchorReconciler) shouldDeleteSubns(log logr.Logger, inst *api.SubnamespaceAnchor, nsInst *corev1.Namespace, deletingCRD bool) bool {
 	r.forest.Lock()
 	defer r.forest.Unlock()
 
@@ -152,6 +161,27 @@ func (r *AnchorReconciler) shouldDeleteSubns(inst *api.SubnamespaceAnchor, nsIns
 		return false
 	}
 
+	// If the anchor isn't in the "ok" state, don't delete the namespace. If it's "conflict," we
+	// *definitely* don't want to delete it; if it's missing (and possibly being created), then there
+	// isn't really a good option but we'll eventually put a condition on the namespace so it's not a
+	// big deal.
+	//
+	// TODO: much of the remaining portion of this function can probably be replaced by this switch
+	// statement. In v0.5, we're playing it safe so I won't modify the rest of the function, but in
+	// v0.6 we should restructure.
+	switch inst.Status.State {
+	case api.Missing:
+		return false
+	case api.Conflict:
+		return false
+	case api.Ok:
+		// keep going...
+	default:
+		log.Error(errors.New("Illegal state"), "Unknown state", "state", inst.Status.State)
+		// Stay on the safe side - don't delete
+		return false
+	}
+
 	cnm := inst.Name
 	pnm := inst.Namespace
 	cns := r.forest.Get(cnm)
@@ -259,7 +289,7 @@ func (r *AnchorReconciler) getNamespace(ctx context.Context, nm string) (*corev1
 	ns := &corev1.Namespace{}
 	nnm := types.NamespacedName{Name: nm}
 	if err := r.Get(ctx, nnm, ns); err != nil {
-		if !errors.IsNotFound(err) {
+		if !apierrors.IsNotFound(err) {
 			return nil, err
 		}
 		return &corev1.Namespace{}, nil

incubator/hnc/test/e2e/issues_test.go

@@ -31,6 +31,26 @@ var _ = Describe("Issues", func() {
 			nsSubSub2, nsSubChild, nsSubSubChild)
 	}) 
 
+	It("Should not delete full namespace when a faulty anchor is deleted - issue #1149", func() {
+		// Setup
+		MustRun("kubectl create ns", nsParent)
+		MustRun("kubectl hns create", nsChild, "-n", nsParent)
+
+		// Wait for subns
+		MustRun("kubectl describe ns", nsChild)
+
+		// Remove annotation
+		MustRun("kubectl annotate ns", nsChild, "hnc.x-k8s.io/subnamespaceOf-")
+		RunShouldNotContain("subnamespaceOf", 1, "kubectl get -oyaml ns", nsChild)
+
+		// Delete anchor
+		MustRun("kubectl delete subns", nsChild, "-n", nsParent)
+		MustNotRun("kubectl get subns", nsChild, "-n", nsParent)
+
+		// Verify that namespace still exists
+		MustRun("kubectl describe ns", nsChild)
+	})
+
 	// Note that this was never actually a problem (only subnamespaces were affected) but it seems
 	// like a good thing to test anyway.
 	It("Should delete full namespaces with propagated objects - issue #1130", func() {