From a9377a8e9f84a7ad13fbdbc25705c235daf719a1 Mon Sep 17 00:00:00 2001 From: Erik Godding Boye Date: Tue, 22 Oct 2024 19:20:22 +0200 Subject: [PATCH] fix: should resolve SubNamespace conflict when sub-namespace deleted --- cmd/accurate-controller/sub/run.go | 3 ++ controllers/subnamespace_controller.go | 47 ++++++++++++-------- controllers/subnamespace_controller_test.go | 15 +++++-- e2e/e2e_test.go | 48 +++++++++++++++++++++ e2e/testdata/conflicting-subnamespace.yaml | 5 +++ pkg/constants/indexer.go | 1 + pkg/indexing/indexing.go | 10 ++++- 7 files changed, 108 insertions(+), 21 deletions(-) create mode 100644 e2e/testdata/conflicting-subnamespace.yaml diff --git a/cmd/accurate-controller/sub/run.go b/cmd/accurate-controller/sub/run.go index a8b58e4..08e6787 100644 --- a/cmd/accurate-controller/sub/run.go +++ b/cmd/accurate-controller/sub/run.go @@ -133,6 +133,9 @@ func subMain(ns, addr string, port int) error { hooks.SetupNamespaceWebhook(mgr, dec) // SubNamespace reconciler & webhook + if err := indexing.SetupIndexForSubNamespace(ctx, mgr); err != nil { + return fmt.Errorf("failed to setup indexer for subnamespaces: %w", err) + } if err = (&controllers.SubNamespaceReconciler{ Client: mgr.GetClient(), }).SetupWithManager(mgr); err != nil { diff --git a/controllers/subnamespace_controller.go b/controllers/subnamespace_controller.go index a468f55..05de946 100644 --- a/controllers/subnamespace_controller.go +++ b/controllers/subnamespace_controller.go @@ -16,14 +16,15 @@ import ( "k8s.io/apimachinery/pkg/types" metav1ac "k8s.io/client-go/applyconfigurations/meta/v1" "k8s.io/client-go/util/csaupgrade" - "k8s.io/client-go/util/workqueue" kstatus "sigs.k8s.io/cli-utils/pkg/kstatus/status" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -154,30 +155,42 @@ func (r *SubNamespaceReconciler) reconcileNS(ctx context.Context, sn *accuratev2 // SetupWithManager sets up the controller with the Manager. func (r *SubNamespaceReconciler) SetupWithManager(mgr ctrl.Manager) error { - nsHandler := func(o client.Object, q workqueue.RateLimitingInterface) { + nsHandler := func(ctx context.Context, o client.Object) (requests []reconcile.Request) { parent := o.GetLabels()[constants.LabelParent] - if parent == "" { + if parent != "" { + requests = append(requests, reconcile.Request{NamespacedName: types.NamespacedName{ + Namespace: parent, + Name: o.GetName(), + }}) return } - q.Add(reconcile.Request{NamespacedName: types.NamespacedName{ - Namespace: parent, - Name: o.GetName(), - }}) + if o.GetDeletionTimestamp() != nil { + // The namespace has no parent and is in terminating state. + // Let's find all (conflicting) subnamespaces that might want to recreate it. + snList := &accuratev2.SubNamespaceList{} + err := r.List(ctx, snList, client.MatchingFields{constants.SubNamespaceNameKey: o.GetName()}) + if err != nil { + logger := log.FromContext(ctx) + logger.Error(err, "failed to list subnamespaces") + return + } + for _, sn := range snList.Items { + requests = append(requests, reconcile.Request{NamespacedName: types.NamespacedName{ + Namespace: sn.Namespace, + Name: sn.Name, + }}) + } + } + return } return ctrl.NewControllerManagedBy(mgr). For(&accuratev2.SubNamespace{}). - Watches(&corev1.Namespace{}, handler.Funcs{ - UpdateFunc: func(ctx context.Context, ev event.UpdateEvent, q workqueue.RateLimitingInterface) { - if ev.ObjectNew.GetDeletionTimestamp() != nil { - return - } - nsHandler(ev.ObjectOld, q) - }, - DeleteFunc: func(ctx context.Context, ev event.DeleteEvent, q workqueue.RateLimitingInterface) { - nsHandler(ev.Object, q) + Watches(&corev1.Namespace{}, handler.EnqueueRequestsFromMapFunc(nsHandler), builder.WithPredicates(predicate.Funcs{ + CreateFunc: func(e event.TypedCreateEvent[client.Object]) bool { + return false }, - }). + })). Complete(r) } diff --git a/controllers/subnamespace_controller_test.go b/controllers/subnamespace_controller_test.go index 33cf0a6..2c5c5aa 100644 --- a/controllers/subnamespace_controller_test.go +++ b/controllers/subnamespace_controller_test.go @@ -6,6 +6,7 @@ import ( accuratev2 "github.com/cybozu-go/accurate/api/accurate/v2" "github.com/cybozu-go/accurate/pkg/constants" + "github.com/cybozu-go/accurate/pkg/indexing" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" @@ -34,6 +35,9 @@ var _ = Describe("SubNamespace controller", func() { err = snr.SetupWithManager(mgr) Expect(err).ToNot(HaveOccurred()) + err = indexing.SetupIndexForSubNamespace(ctx, mgr) + Expect(err).NotTo(HaveOccurred()) + ctx, cancel := context.WithCancel(ctx) stopFunc = cancel go func() { @@ -50,7 +54,7 @@ var _ = Describe("SubNamespace controller", func() { time.Sleep(100 * time.Millisecond) }) - It("should create and delete sub namespaces", func() { + It("should create and delete sub-namespaces", func() { ns := &corev1.Namespace{} ns.Name = "test1" Expect(k8sClient.Create(ctx, ns)).To(Succeed()) @@ -92,9 +96,14 @@ var _ = Describe("SubNamespace controller", func() { Eventually(komega.Object(sn)).Should(HaveField("Status.ObservedGeneration", BeNumerically(">", 0))) Expect(sn.Status.Conditions).To(HaveLen(1)) Expect(sn.Status.Conditions[0].Reason).To(Equal(accuratev2.SubNamespaceConflict)) + + // It's tempting to test if a conflict can be resolved by deleting the conflicting namespace, + // but this is currently not possible because EnvTest does not support namespace deletion. + // See https://github.com/kubernetes-sigs/controller-runtime/issues/880 for details. + // This feature should be tested in e2e-tests. }) - It("should not delete a conflicting sub namespace", func() { + It("should not delete a conflicting sub-namespace", func() { ns := &corev1.Namespace{} ns.Name = "test3" Expect(k8sClient.Create(ctx, ns)).To(Succeed()) @@ -121,7 +130,7 @@ var _ = Describe("SubNamespace controller", func() { Consistently(komega.Object(sub1)).Should(HaveField("DeletionTimestamp", BeNil())) }) - It("should re-create a subnamespace if it is deleted", func() { + It("should re-create a sub-namespace if it is deleted", func() { ns := &corev1.Namespace{} ns.Name = "test4" Expect(k8sClient.Create(ctx, ns)).To(Succeed()) diff --git a/e2e/e2e_test.go b/e2e/e2e_test.go index f444aef..bffed73 100644 --- a/e2e/e2e_test.go +++ b/e2e/e2e_test.go @@ -23,6 +23,9 @@ var resourceQuota []byte //go:embed testdata/serviceaccountWithDummySecrets.yaml var serviceAccountWithDummySecretsYAML []byte +//go:embed testdata/conflicting-subnamespace.yaml +var conflictingSubnamespaceYAML []byte + var ( sealedJSON []byte k8sMinorVersion int @@ -292,6 +295,51 @@ var _ = Describe("kubectl accurate", func() { Expect(err).To(HaveOccurred()) }) + It("should (re)create sub-namespace when conflicting namespace deleted", func() { + By("preparing namespaces") + kubectlSafe(nil, "create", "ns", "conflict-root1") + kubectlSafe(nil, "accurate", "ns", "set-type", "conflict-root1", "root") + kubectlSafe(nil, "create", "ns", "conflict-sub1") + + By("creating conflicting subnamespace") + // Cannot use "kubectl accurate" here, since conflict is validated client-side. + kubectlSafe(conflictingSubnamespaceYAML, "apply", "-f", "-") + var conditions []metav1.Condition + Eventually(func() ([]metav1.Condition, error) { + out, err := kubectl(nil, "get", "-n", "conflict-root1", "subnamespaces", "conflict-sub1", "-o", "json") + if err != nil { + return nil, err + } + sn := &accuratev2.SubNamespace{} + if err := json.Unmarshal(out, sn); err != nil { + return nil, err + } + conditions = sn.Status.Conditions + return conditions, nil + }).Should(HaveLen(1)) + Expect(conditions[0].Reason).To(Equal("Conflict")) + + By("deleting conflicting namespace") + kubectlSafe(nil, "delete", "ns", "conflict-sub1") + Eventually(func() ([]metav1.Condition, error) { + out, err := kubectl(nil, "get", "-n", "conflict-root1", "subnamespaces", "conflict-sub1", "-o", "json") + if err != nil { + return nil, err + } + sn := &accuratev2.SubNamespace{} + if err := json.Unmarshal(out, sn); err != nil { + return nil, err + } + return sn.Status.Conditions, nil + }).Should(BeEmpty()) + out, err := kubectl(nil, "get", "ns", "conflict-sub1", "-o", "json") + Expect(err).NotTo(HaveOccurred()) + sn := &corev1.Namespace{} + err = json.Unmarshal(out, sn) + Expect(err).NotTo(HaveOccurred()) + Expect(sn.Labels).To(HaveKeyWithValue(constants.LabelParent, "conflict-root1")) + }) + It("should propagate ServiceAccount w/o secrets field", func() { // From Kubernetes 1.24, the auto-generation of secret-based service account tokens has been disabled by default. // So the secrets field in the ServiceAccount is not updated. But when upgrading Kubernetes from 1.23 or lower, diff --git a/e2e/testdata/conflicting-subnamespace.yaml b/e2e/testdata/conflicting-subnamespace.yaml new file mode 100644 index 0000000..fe129df --- /dev/null +++ b/e2e/testdata/conflicting-subnamespace.yaml @@ -0,0 +1,5 @@ +apiVersion: accurate.cybozu.com/v2 +kind: SubNamespace +metadata: + name: conflict-sub1 + namespace: conflict-root1 diff --git a/pkg/constants/indexer.go b/pkg/constants/indexer.go index 12f15a7..13c4d65 100644 --- a/pkg/constants/indexer.go +++ b/pkg/constants/indexer.go @@ -5,4 +5,5 @@ const ( NamespaceParentKey = "namespace.parent" NamespaceTemplateKey = "namespace.template" PropagateKey = "resource.propagate" + SubNamespaceNameKey = "subnamespace.name" ) diff --git a/pkg/indexing/indexing.go b/pkg/indexing/indexing.go index 2d8a92a..18de9c5 100644 --- a/pkg/indexing/indexing.go +++ b/pkg/indexing/indexing.go @@ -3,6 +3,7 @@ package indexing import ( "context" + accuratev2 "github.com/cybozu-go/accurate/api/accurate/v2" "github.com/cybozu-go/accurate/pkg/constants" corev1 "k8s.io/api/core/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -23,7 +24,7 @@ func SetupIndexForResource(ctx context.Context, mgr manager.Manager, res client. // SetupIndexForNamespace sets up indexers for namespaces. func SetupIndexForNamespace(ctx context.Context, mgr manager.Manager) error { ns := &corev1.Namespace{} - err := mgr.GetFieldIndexer().IndexField(context.Background(), ns, constants.NamespaceParentKey, func(rawObj client.Object) []string { + err := mgr.GetFieldIndexer().IndexField(ctx, ns, constants.NamespaceParentKey, func(rawObj client.Object) []string { parent := rawObj.GetLabels()[constants.LabelParent] if parent == "" { return nil @@ -42,3 +43,10 @@ func SetupIndexForNamespace(ctx context.Context, mgr manager.Manager) error { return []string{tmpl} }) } + +// SetupIndexForSubNamespace sets up indexers for subnamespaces. +func SetupIndexForSubNamespace(ctx context.Context, mgr manager.Manager) error { + return mgr.GetFieldIndexer().IndexField(ctx, &accuratev2.SubNamespace{}, constants.SubNamespaceNameKey, func(rawObj client.Object) []string { + return []string{rawObj.GetName()} + }) +}