diff --git a/Makefile b/Makefile index 7e970dc..d9725d5 100644 --- a/Makefile +++ b/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 diff --git a/charts/accurate/README.md b/charts/accurate/README.md index f4cd652..2f8b08b 100644 --- a/charts/accurate/README.md +++ b/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. | diff --git a/charts/accurate/templates/deployment.yaml b/charts/accurate/templates/deployment.yaml index 8db33ed..b4e3cc9 100644 --- a/charts/accurate/templates/deployment.yaml +++ b/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: diff --git a/charts/accurate/values.yaml b/charts/accurate/values.yaml index ce4b715..a7c538c 100644 --- a/charts/accurate/values.yaml +++ b/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 diff --git a/cmd/accurate-controller/sub/root.go b/cmd/accurate-controller/sub/root.go index 3d4bae3..34ceb00 100644 --- a/cmd/accurate-controller/sub/root.go +++ b/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) diff --git a/cmd/accurate-controller/sub/run.go b/cmd/accurate-controller/sub/run.go index 08e6787..22b58a6 100644 --- a/cmd/accurate-controller/sub/run.go +++ b/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) } diff --git a/docs/design.md b/docs/design.md index 0e69c85..8a18238 100644 --- a/docs/design.md +++ b/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. diff --git a/hooks/allow-cascade-delete/cascade_delete_test.go b/hooks/allow-cascade-delete/cascade_delete_test.go new file mode 100644 index 0000000..def210d --- /dev/null +++ b/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()) + }) + }) +}) diff --git a/hooks/allow-cascade-delete/suite_test.go b/hooks/allow-cascade-delete/suite_test.go new file mode 100644 index 0000000..f25d33a --- /dev/null +++ b/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 +} diff --git a/hooks/namespace.go b/hooks/namespace.go index 98c061d..0bf018d 100644 --- a/hooks/namespace.go +++ b/hooks/namespace.go @@ -19,7 +19,8 @@ import ( type namespaceValidator struct { client.Client - dec admission.Decoder + dec admission.Decoder + allowCascadingDeletion bool } var _ admission.Handler = &namespaceValidator{} @@ -178,10 +179,10 @@ func (v *namespaceValidator) handleUpdate(ctx context.Context, nsNew, nsOld *cor func (v *namespaceValidator) handleDelete(ctx context.Context, ns *corev1.Namespace) admission.Response { key := constants.NamespaceParentKey switch { - case ns.Labels[constants.LabelType] == constants.NSTypeRoot: + case ns.Labels[constants.LabelType] == constants.NSTypeRoot && !v.allowCascadingDeletion: case ns.Labels[constants.LabelType] == constants.NSTypeTemplate: key = constants.NamespaceTemplateKey - case ns.Labels[constants.LabelParent] != "": + case ns.Labels[constants.LabelParent] != "" && !v.allowCascadingDeletion: default: return admission.Allowed("") } @@ -198,10 +199,11 @@ func (v *namespaceValidator) handleDelete(ctx context.Context, ns *corev1.Namesp } // SetupNamespaceWebhook registers the webhook for Namespace -func SetupNamespaceWebhook(mgr manager.Manager, dec admission.Decoder) { +func SetupNamespaceWebhook(mgr manager.Manager, dec admission.Decoder, allowCascadingDeletion bool) { v := &namespaceValidator{ - Client: mgr.GetClient(), - dec: dec, + Client: mgr.GetClient(), + dec: dec, + allowCascadingDeletion: allowCascadingDeletion, } serv := mgr.GetWebhookServer() serv.Register("/validate-v1-namespace", &webhook.Admission{Handler: v}) diff --git a/hooks/subnamespace.go b/hooks/subnamespace.go index 3193074..096b6e6 100644 --- a/hooks/subnamespace.go +++ b/hooks/subnamespace.go @@ -57,8 +57,9 @@ func (m *subNamespaceMutator) Handle(ctx context.Context, req admission.Request) type subNamespaceValidator struct { client.Client - dec admission.Decoder - namingPolicies []config.NamingPolicyRegexp + dec admission.Decoder + namingPolicies []config.NamingPolicyRegexp + allowCascadingDeletion bool } var _ admission.Handler = &subNamespaceValidator{} @@ -114,6 +115,10 @@ func (v *subNamespaceValidator) handleCreate(ctx context.Context, sn *accuratev2 } func (v *subNamespaceValidator) handleDelete(ctx context.Context, sn *accuratev2.SubNamespace) admission.Response { + if v.allowCascadingDeletion { + return admission.Allowed("") + } + ns := &corev1.Namespace{} if err := v.Get(ctx, client.ObjectKey{Name: sn.Name}, ns); err != nil { if apierrors.IsNotFound(err) { @@ -174,7 +179,7 @@ func (v *subNamespaceValidator) notMatchingNamingPolicy(ctx context.Context, ns, } // SetupSubNamespaceWebhook registers the webhooks for SubNamespace -func SetupSubNamespaceWebhook(mgr manager.Manager, dec admission.Decoder, namingPolicyRegexps []config.NamingPolicyRegexp) error { +func SetupSubNamespaceWebhook(mgr manager.Manager, dec admission.Decoder, namingPolicyRegexps []config.NamingPolicyRegexp, allowCascadingDeletion bool) error { for _, s := range []runtime.Object{&accuratev1.SubNamespace{}, &accuratev2alpha1.SubNamespace{}, &accuratev2.SubNamespace{}} { err := ctrl.NewWebhookManagedBy(mgr). For(s). @@ -192,9 +197,10 @@ func SetupSubNamespaceWebhook(mgr manager.Manager, dec admission.Decoder, naming serv.Register("/mutate-accurate-cybozu-com-v2-subnamespace", &webhook.Admission{Handler: m}) v := &subNamespaceValidator{ - Client: mgr.GetClient(), - dec: dec, - namingPolicies: namingPolicyRegexps, + Client: mgr.GetClient(), + dec: dec, + namingPolicies: namingPolicyRegexps, + allowCascadingDeletion: allowCascadingDeletion, } serv.Register("/validate-accurate-cybozu-com-v2-subnamespace", &webhook.Admission{Handler: v}) return nil diff --git a/hooks/suite_test.go b/hooks/suite_test.go index 69af450..8f5a6fd 100644 --- a/hooks/suite_test.go +++ b/hooks/suite_test.go @@ -103,7 +103,7 @@ var _ = BeforeSuite(func() { Expect(err).NotTo(HaveOccurred()) dec := admission.NewDecoder(scheme) - SetupNamespaceWebhook(mgr, dec) + SetupNamespaceWebhook(mgr, dec, false) conf := config.Config{ NamingPolicies: []config.NamingPolicy{ @@ -139,7 +139,7 @@ var _ = BeforeSuite(func() { } err = conf.Validate(mgr.GetRESTMapper()) Expect(err).NotTo(HaveOccurred()) - err = SetupSubNamespaceWebhook(mgr, dec, conf.NamingPolicyRegexps) + err = SetupSubNamespaceWebhook(mgr, dec, conf.NamingPolicyRegexps, false) Expect(err).NotTo(HaveOccurred()) go func() {