From d6d4a2de01a0fc333fc67b688af1db54804a47a3 Mon Sep 17 00:00:00 2001 From: Adrian Ludwin Date: Fri, 25 Sep 2020 11:26:24 -0400 Subject: [PATCH] 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. --- incubator/hnc/internal/reconcilers/anchor.go | 40 +++++++++++++++++--- incubator/hnc/test/e2e/issues_test.go | 20 ++++++++++ 2 files changed, 55 insertions(+), 5 deletions(-) diff --git a/incubator/hnc/internal/reconcilers/anchor.go b/incubator/hnc/internal/reconcilers/anchor.go index 5897b7ecf..20a4eee10 100644 --- a/incubator/hnc/internal/reconcilers/anchor.go +++ b/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 diff --git a/incubator/hnc/test/e2e/issues_test.go b/incubator/hnc/test/e2e/issues_test.go index 77e79813a..61bdc78ad 100644 --- a/incubator/hnc/test/e2e/issues_test.go +++ b/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() {