Skip to content
This repository was archived by the owner on Jun 26, 2023. It is now read-only.

Don't delete subns if anchor has conflict #1150

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 35 additions & 5 deletions incubator/hnc/internal/reconcilers/anchor.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand All @@ -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()

Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down
20 changes: 20 additions & 0 deletions incubator/hnc/test/e2e/issues_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down