Merge pull request #150 from erikgb/allow-cascade-delete

feat: opt-in allowing cascade delete of namespaces

Author: zoetrope

.github/workflows/helm.yaml

@@ -43,12 +43,14 @@ jobs:
           kubectl -n cert-manager wait --for=condition=available --timeout=180s --all deployments
       - name: Prepare values.yaml
         run: |
-          LATEST=$(curl -s "https://api.github.com/repos/cybozu-go/accurate/releases/latest" | jq -r .tag_name)
-          APP_VERSION=${LATEST#v}
+          docker build -t accurate:dev .
+          kind load docker-image accurate:dev --name=chart-testing
           mkdir -p charts/accurate/ci/
           cat > charts/accurate/ci/ci-values.yaml <<EOF
           image:
-             tag: $APP_VERSION
+            repository: accurate
+            tag: dev
+            pullPolicy: Never
           EOF
       - name: Run chart-testing (install)
         run: ct install --config ct.yaml

Makefile

@@ -97,7 +97,7 @@ envtest: setup-envtest
 	source <($(SETUP_ENVTEST) use -p env); \
 		go test -v -count 1 -race ./controllers -ginkgo.progress -ginkgo.v -ginkgo.fail-fast
 	source <($(SETUP_ENVTEST) use -p env); \
-		go test -v -count 1 -race ./hooks -ginkgo.progress -ginkgo.v
+		go test -v -count 1 -race ./hooks/... -ginkgo.progress -ginkgo.v
 
 .PHONY: test
 test: test-tools

charts/accurate/README.md

@@ -74,6 +74,7 @@ $ helm install --create-namespace --namespace accurate accurate -f values.yaml a
 | controller.replicas                              | int    | `2`                                                                                                                                                                               | Specify the number of replicas of the controller Pod.                                                                                                                                                                         |
 | controller.resources                             | object | `{"requests":{"cpu":"100m","memory":"20Mi"}}`                                                                                                                                     | Specify resources.                                                                                                                                                                                                            |
 | controller.terminationGracePeriodSeconds         | int    | `10`                                                                                                                                                                              | Specify terminationGracePeriodSeconds.                                                                                                                                                                                        |
+| webhook.allowCascadingDeletion                   | bool   | `false`                                                                                                                                                                           | Enable to allow cascading deletion of namespaces. Accurate webhooks will only allow deletion of a namespace with children if this option is enabled.                                                                          |
 | image.pullPolicy                                 | string | `nil`                                                                                                                                                                             | Accurate image pullPolicy.                                                                                                                                                                                                    |
 | image.repository                                 | string | `"ghcr.io/cybozu-go/accurate"`                                                                                                                                                    | Accurate image repository to use.                                                                                                                                                                                             |
 | image.tag                                        | string | `{{ .Chart.AppVersion }}`                                                                                                                                                         | Accurate image tag to use.                                                                                                                                                                                                    |

charts/accurate/templates/deployment.yaml

@@ -26,8 +26,9 @@ spec:
           {{- with .Values.image.pullPolicy }}
           imagePullPolicy: {{ . }}
           {{- end }}
-          {{- with .Values.controller.extraArgs }}
           args:
+            - --webhook-allow-cascading-deletion={{ .Values.webhook.allowCascadingDeletion }}
+          {{- with .Values.controller.extraArgs }}
             {{- toYaml . | nindent 12 }}
           {{- end }}
           ports:

charts/accurate/values.yaml

@@ -133,3 +133,12 @@ controller:
     # common namespace-scoped resources.
     clusterRoles:
     - admin
+
+webhook:
+  # webhook.allowCascadingDeletion -- Enable to allow cascading deletion of namespaces.
+  # Accurate webhooks will only allow deletion of a namespace with children if this option is enabled.
+  # Deleting namespaces is very dangerous, and deleting sub-namespaces can result in entire subtrees
+  # of namespaces being deleted as well. So enable this option with care!
+  # That said, enabling this option can be very useful to allow modern GitOps controllers like FluxCD
+  # to operate without errors based on desired state specified in Git.
+  allowCascadingDeletion: false

cmd/accurate-controller/sub/root.go

@@ -29,6 +29,8 @@ var options struct {
 	certDir          string
 	qps              int
 	zapOpts          zap.Options
+
+	webhookAllowCascadingDeletion bool
 }
 
 var rootCmd = &cobra.Command{
@@ -74,6 +76,8 @@ func init() {
 	fs.StringVar(&options.certDir, "cert-dir", "", "webhook certificate directory")
 	fs.IntVar(&options.qps, "apiserver-qps-throttle", defaultQPS, "The maximum QPS to the API server.")
 
+	fs.BoolVar(&options.webhookAllowCascadingDeletion, "webhook-allow-cascading-deletion", false, "Set to true to allow cascading deletion of namespaces (namespaces with children)")
+
 	config.DefaultMutableFeatureGate.AddFlag(fs)
 
 	goflags := flag.NewFlagSet("klog", flag.ExitOnError)

cmd/accurate-controller/sub/run.go

@@ -130,7 +130,7 @@ func subMain(ns, addr string, port int) error {
 	}).SetupWithManager(mgr); err != nil {
 		return fmt.Errorf("unable to create Namespace controller: %w", err)
 	}
-	hooks.SetupNamespaceWebhook(mgr, dec)
+	hooks.SetupNamespaceWebhook(mgr, dec, options.webhookAllowCascadingDeletion)
 
 	// SubNamespace reconciler & webhook
 	if err := indexing.SetupIndexForSubNamespace(ctx, mgr); err != nil {
@@ -141,7 +141,7 @@ func subMain(ns, addr string, port int) error {
 	}).SetupWithManager(mgr); err != nil {
 		return fmt.Errorf("unable to create SubNamespace controller: %w", err)
 	}
-	if err = hooks.SetupSubNamespaceWebhook(mgr, dec, cfg.NamingPolicyRegexps); err != nil {
+	if err = hooks.SetupSubNamespaceWebhook(mgr, dec, cfg.NamingPolicyRegexps, options.webhookAllowCascadingDeletion); err != nil {
 		return fmt.Errorf("unable to create SubNamespace webhook: %w", err)
 	}
 

docs/design.md

@@ -46,7 +46,7 @@ Since these are fundamentally different requirements, we decided to develop our
 - Opt-in root namespaces
     - Only namespaces labeled with `accurate.cybozu.com/type: root` can be the root of a namespace tree.
 - Tenant users can create and delete sub-namespaces by creating and deleting a custom resource in a root or a sub-namespace.
-    - If a namespace has one or more sub-namespaces, Accurate prevents the deletion of the namespace.
+    - If a namespace has one or more sub-namespaces, Accurate prevents the deletion of the namespace - unless allow cascading deletion of namespaces is enabled.
 - Template namespace
     - Namespaces that are not a sub-namespace can specify a template from which labels, annotations, and resources can be propagated.
 - Admins can change the parent namespace of a sub-namespace.

hooks/allow-cascade-delete/cascade_delete_test.go

@@ -0,0 +1,91 @@
+package hooks_allow_cascade_delete
+
+import (
+	"context"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	accuratev2 "github.com/cybozu-go/accurate/api/accurate/v2"
+	"github.com/cybozu-go/accurate/pkg/constants"
+)
+
+var _ = Describe("Webhook allow cascade delete", func() {
+	var (
+		ctx  context.Context
+		root *corev1.Namespace
+	)
+
+	BeforeEach(func() {
+		ctx = context.Background()
+
+		root = &corev1.Namespace{}
+		root.GenerateName = "cascade-root-"
+		root.Labels = map[string]string{constants.LabelType: constants.NSTypeRoot}
+		Expect(k8sClient.Create(ctx, root)).To(Succeed())
+	})
+
+	Context("Namespace", func() {
+		It("should ALLOW deleting a root with children", func() {
+			sub := &corev1.Namespace{}
+			sub.GenerateName = "cascade-sub-"
+			sub.Labels = map[string]string{constants.LabelParent: root.Name}
+			Expect(k8sClient.Create(ctx, sub)).To(Succeed())
+
+			Expect(k8sClient.Delete(ctx, root)).To(Succeed())
+		})
+
+		It("should ALLOW deleting a sub-namespace with children", func() {
+			sub := &corev1.Namespace{}
+			sub.GenerateName = "cascade-sub-"
+			sub.Labels = map[string]string{constants.LabelParent: root.Name}
+			Expect(k8sClient.Create(ctx, sub)).To(Succeed())
+
+			subSub := &corev1.Namespace{}
+			subSub.GenerateName = "cascade-sub-sub-"
+			subSub.Labels = map[string]string{constants.LabelParent: sub.Name}
+			Expect(k8sClient.Create(ctx, subSub)).To(Succeed())
+
+			Expect(k8sClient.Delete(ctx, sub)).To(Succeed())
+		})
+
+		It("should DENY deleting a template with children", func() {
+			tmpl := &corev1.Namespace{}
+			tmpl.GenerateName = "cascade-tmpl-"
+			tmpl.Labels = map[string]string{constants.LabelType: constants.NSTypeTemplate}
+			Expect(k8sClient.Create(ctx, tmpl)).To(Succeed())
+
+			root.Labels[constants.LabelTemplate] = tmpl.Name
+			Expect(k8sClient.Update(ctx, root)).To(Succeed())
+
+			err := k8sClient.Delete(ctx, tmpl)
+			Expect(err).To(HaveOccurred())
+			Expect(errors.ReasonForError(err)).Should(Equal(metav1.StatusReasonForbidden))
+		})
+	})
+
+	Context("SubNamespace", func() {
+		It("should ALLOW deletion with child namespaces", func() {
+			sub := &accuratev2.SubNamespace{}
+			sub.Namespace = root.Name
+			sub.GenerateName = "cascade-sub-"
+			Expect(k8sClient.Create(ctx, sub)).To(Succeed())
+			// Create sub-namespace since no controllers present in this test setup
+			subNS := &corev1.Namespace{}
+			subNS.Name = sub.Name
+			subNS.Labels = map[string]string{constants.LabelParent: root.Name}
+			Expect(k8sClient.Create(ctx, subNS)).To(Succeed())
+
+			ns := &corev1.Namespace{}
+			ns.GenerateName = "cascade-sub-sub-"
+			ns.Labels = map[string]string{constants.LabelParent: subNS.Name}
+			Expect(k8sClient.Create(ctx, ns)).To(Succeed())
+
+			Expect(k8sClient.Delete(ctx, sub)).To(Succeed())
+		})
+	})
+})

hooks/allow-cascade-delete/suite_test.go

@@ -0,0 +1,160 @@
+package hooks_allow_cascade_delete
+
+import (
+	"context"
+	"crypto/tls"
+	"encoding/json"
+	"fmt"
+	"net"
+	"path/filepath"
+	"testing"
+	"time"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	admissionv1beta1 "k8s.io/api/admission/v1beta1"
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/envtest"
+	logf "sigs.k8s.io/controller-runtime/pkg/log"
+	"sigs.k8s.io/controller-runtime/pkg/log/zap"
+	"sigs.k8s.io/controller-runtime/pkg/metrics/server"
+	"sigs.k8s.io/controller-runtime/pkg/webhook"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+	"sigs.k8s.io/kustomize/api/krusty"
+	"sigs.k8s.io/kustomize/kyaml/filesys"
+
+	accuratev1 "github.com/cybozu-go/accurate/api/accurate/v1"
+	accuratev2 "github.com/cybozu-go/accurate/api/accurate/v2"
+	accuratev2alpha1 "github.com/cybozu-go/accurate/api/accurate/v2alpha1"
+	"github.com/cybozu-go/accurate/hooks"
+	"github.com/cybozu-go/accurate/pkg/indexing"
+)
+
+// These tests use Ginkgo (BDD-style Go testing framework). Refer to
+// http://onsi.github.io/ginkgo/ to learn more about Ginkgo.
+
+var k8sClient client.Client
+var testEnv *envtest.Environment
+var cancelMgr context.CancelFunc
+
+func TestAPIs(t *testing.T) {
+	RegisterFailHandler(Fail)
+
+	RunSpecs(t, "Webhook Suite")
+}
+
+var _ = BeforeSuite(func() {
+	logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)))
+
+	ctx, cancel := context.WithCancel(context.TODO())
+	cancelMgr = cancel
+
+	scheme := runtime.NewScheme()
+	err := accuratev1.AddToScheme(scheme)
+	Expect(err).NotTo(HaveOccurred())
+	err = accuratev2alpha1.AddToScheme(scheme)
+	Expect(err).NotTo(HaveOccurred())
+	err = accuratev2.AddToScheme(scheme)
+	Expect(err).NotTo(HaveOccurred())
+	err = clientgoscheme.AddToScheme(scheme)
+	Expect(err).NotTo(HaveOccurred())
+	err = admissionv1beta1.AddToScheme(scheme)
+	Expect(err).NotTo(HaveOccurred())
+
+	//+kubebuilder:scaffold:scheme
+
+	By("bootstrapping test environment")
+	testEnv = &envtest.Environment{
+		Scheme: scheme,
+		CRDs:   loadCRDs(),
+		WebhookInstallOptions: envtest.WebhookInstallOptions{
+			Paths: []string{filepath.Join("..", "..", "config", "webhook")},
+		},
+	}
+
+	cfg, err := testEnv.Start()
+	Expect(err).NotTo(HaveOccurred())
+	Expect(cfg).NotTo(BeNil())
+
+	k8sClient, err = client.New(cfg, client.Options{Scheme: scheme})
+	Expect(err).NotTo(HaveOccurred())
+	Expect(k8sClient).NotTo(BeNil())
+
+	webhookInstallOptions := &testEnv.WebhookInstallOptions
+	mgr, err := ctrl.NewManager(cfg, ctrl.Options{
+		Scheme: scheme,
+		WebhookServer: webhook.NewServer(webhook.Options{
+			Host:    webhookInstallOptions.LocalServingHost,
+			Port:    webhookInstallOptions.LocalServingPort,
+			CertDir: webhookInstallOptions.LocalServingCertDir,
+		}),
+		LeaderElection: false,
+		Metrics:        server.Options{BindAddress: "0"},
+	})
+	Expect(err).NotTo(HaveOccurred())
+
+	err = indexing.SetupIndexForNamespace(ctx, mgr)
+	Expect(err).NotTo(HaveOccurred())
+
+	dec := admission.NewDecoder(scheme)
+	hooks.SetupNamespaceWebhook(mgr, dec, true)
+
+	Expect(err).NotTo(HaveOccurred())
+	err = hooks.SetupSubNamespaceWebhook(mgr, dec, nil, true)
+	Expect(err).NotTo(HaveOccurred())
+
+	go func() {
+		err = mgr.Start(ctx)
+		if err != nil {
+			Expect(err).NotTo(HaveOccurred())
+		}
+	}()
+
+	// wait for the webhook server to get ready
+	dialer := &net.Dialer{Timeout: time.Second}
+	addrPort := fmt.Sprintf("%s:%d", webhookInstallOptions.LocalServingHost, webhookInstallOptions.LocalServingPort)
+	Eventually(func() error {
+		conn, err := tls.DialWithDialer(dialer, "tcp", addrPort, &tls.Config{InsecureSkipVerify: true})
+		if err != nil {
+			return err
+		}
+		conn.Close()
+		return nil
+	}).Should(Succeed())
+
+})
+
+var _ = AfterSuite(func() {
+	cancelMgr()
+	time.Sleep(50 * time.Millisecond)
+	By("tearing down the test environment")
+	err := testEnv.Stop()
+	Expect(err).NotTo(HaveOccurred())
+})
+
+func loadCRDs() []*apiextensionsv1.CustomResourceDefinition {
+	kOpts := krusty.MakeDefaultOptions()
+	k := krusty.MakeKustomizer(kOpts)
+	m, err := k.Run(filesys.FileSystemOrOnDisk{}, filepath.Join("..", "..", "config", "crd"))
+	Expect(err).To(Succeed())
+	resources := m.Resources()
+
+	crds := make([]*apiextensionsv1.CustomResourceDefinition, len(resources))
+	for i := range resources {
+		bytes, err := resources[i].MarshalJSON()
+		Expect(err).To(Succeed())
+
+		crd := &apiextensionsv1.CustomResourceDefinition{}
+		err = json.Unmarshal(bytes, crd)
+		Expect(err).To(Succeed())
+
+		crds[i] = crd
+	}
+
+	return crds
+}