From 04d3dc75886a53fc5921473a9ca6bdb71fe03894 Mon Sep 17 00:00:00 2001 From: Erik Godding Boye Date: Fri, 2 Feb 2024 20:19:24 +0100 Subject: [PATCH] chore: migrate controllers to SSA --- controllers/namespace_controller.go | 40 ++++++------ controllers/namespace_controller_test.go | 82 ++++++++++++++++++++++-- controllers/propagate.go | 15 ++--- controllers/ssa_client.go | 44 +++++++++++++ controllers/subnamespace_controller.go | 44 ++----------- docs/subnamespaces.md | 2 +- 6 files changed, 154 insertions(+), 73 deletions(-) create mode 100644 controllers/ssa_client.go diff --git a/controllers/namespace_controller.go b/controllers/namespace_controller.go index 457af8c..f5f06e5 100644 --- a/controllers/namespace_controller.go +++ b/controllers/namespace_controller.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "path" - "reflect" accuratev2alpha1 "github.com/cybozu-go/accurate/api/accurate/v2alpha1" "github.com/cybozu-go/accurate/pkg/constants" @@ -13,6 +12,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" + corev1ac "k8s.io/client-go/applyconfigurations/core/v1" "k8s.io/client-go/util/workqueue" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -85,18 +85,17 @@ func (r *NamespaceReconciler) reconcile(ctx context.Context, ns *corev1.Namespac } func (r *NamespaceReconciler) propagateMeta(ctx context.Context, ns, parent *corev1.Namespace) error { - orig := ns.DeepCopy() + labels := make(map[string]string) + annotations := make(map[string]string) + for k, v := range parent.Labels { if ok := r.matchLabelKey(k); ok { - ns.Labels[k] = v + labels[k] = v } } for k, v := range parent.Annotations { if ok := r.matchAnnotationKey(k); ok { - if ns.Annotations == nil { - ns.Annotations = make(map[string]string) - } - ns.Annotations[k] = v + annotations[k] = v } } @@ -110,24 +109,26 @@ func (r *NamespaceReconciler) propagateMeta(ctx context.Context, ns, parent *cor } else { for k, v := range subNS.Spec.Labels { if ok := r.matchSubNamespaceLabelKey(k); ok { - ns.Labels[k] = v + labels[k] = v } } for k, v := range subNS.Spec.Annotations { if ok := r.matchSubNamespaceAnnotationKey(k); ok { - if ns.Annotations == nil { - ns.Annotations = make(map[string]string) - } - ns.Annotations[k] = v + annotations[k] = v } } } } - if !reflect.DeepEqual(ns.ObjectMeta, orig.ObjectMeta) { - if err := r.Update(ctx, ns); err != nil { - return fmt.Errorf("failed to propagate labels/annotations for namespace %s: %w", ns.Name, err) - } + ac := corev1ac.Namespace(ns.Name). + WithLabels(labels). + WithAnnotations(annotations) + ns, p, err := newNamespacePatch(ac) + if err != nil { + return err + } + if err := r.Patch(ctx, ns, p, fieldOwner, client.ForceOwnership); err != nil { + return fmt.Errorf("failed to propagate labels/annotations for namespace %s: %w", ns.Name, err) } return nil } @@ -250,12 +251,11 @@ func (r *NamespaceReconciler) propagateUpdate(ctx context.Context, res *unstruct return nil } - c2.SetResourceVersion(c.GetResourceVersion()) - if err := r.Update(ctx, c2); err != nil { - return err + if err := r.Patch(ctx, c2, applyPatch{c2}, fieldOwner, client.ForceOwnership); err != nil { + return fmt.Errorf("failed to apply %s/%s: %w", c2.GetNamespace(), c2.GetName(), err) } - logger.Info("updated a resource", "namespace", ns, "name", res.GetName(), "gvk", gvk.String()) + logger.Info("applied a resource", "namespace", ns, "name", res.GetName(), "gvk", gvk.String()) return nil } diff --git a/controllers/namespace_controller_test.go b/controllers/namespace_controller_test.go index 22d9f40..fa7dc2f 100644 --- a/controllers/namespace_controller_test.go +++ b/controllers/namespace_controller_test.go @@ -201,6 +201,12 @@ var _ = Describe("Namespace controller", func() { Eventually(komega.Object(ns1)).Should(HaveField("Labels", HaveKeyWithValue("foo.bar/baz", "123"))) + By("removing a label of template namespace") + Expect(komega.Update(tmpl, func() { + delete(tmpl.Labels, "foo.bar/baz") + })()).To(Succeed()) + Eventually(komega.Object(ns1)).Should(HaveField("Labels", Not(HaveKey("foo.bar/baz")))) + tmpl2 := &corev1.Namespace{} tmpl2.Name = "tmpl2" tmpl2.Labels = map[string]string{ @@ -229,12 +235,17 @@ var _ = Describe("Namespace controller", func() { Eventually(komega.Get(pSec2)).Should(Succeed()) Eventually(komega.Object(ns1)).Should(HaveField("Labels", HaveKeyWithValue("team", "maneki"))) + // assert that annotations/labels from previous template are removed + Expect(ns1.Annotations).To(Not(HaveKey("foo.bar/zot"))) + Expect(ns1.Annotations).To(Not(HaveKey("memo"))) + Expect(ns1.Annotations).To(Not(HaveKey("team"))) + Expect(ns1.Labels).To(Not(HaveKey("foo.bar/baz"))) + Expect(ns1.Labels).To(Not(HaveKey("memo"))) By("unsetting the template") Expect(komega.Update(ns1, func() { delete(ns1.Labels, constants.LabelTemplate) })()).To(Succeed()) - Eventually(komega.Get(pSec2)).Should(WithTransform(apierrors.IsNotFound, BeTrue())) }) @@ -278,6 +289,12 @@ var _ = Describe("Namespace controller", func() { Expect(instance.Annotations["memo"]).Should(Equal("test")) Expect(komega.Object(tmpl2)()).To(HaveField("Labels", HaveKeyWithValue("team", "neco"))) + + By("removing a label from root template namespace") + Expect(komega.Update(tmpl1, func() { + delete(tmpl1.Labels, "team") + })()).To(Succeed()) + Eventually(komega.Object(instance)).Should(HaveField("Labels", Not(HaveKey("team")))) }) It("should not delete resources in an independent namespace", func() { @@ -390,12 +407,9 @@ var _ = Describe("Namespace controller", func() { By("deleting an annotation in root namespace") Expect(komega.Update(root, func() { - delete(root.Labels, "baz.glob/c") + delete(root.Annotations, "baz.glob/c") })()).To(Succeed()) - // Cleaning up obsolete labels/annotations from sub-namespaces is currently unsupported - // See https://github.com/cybozu-go/accurate/issues/98 - Consistently(komega.Object(sub1)).Should(HaveField("Annotations", HaveKey("baz.glob/c"))) - //Eventually(komega.Object(sub1)).Should(HaveField("Annotations", Not(HaveKey("baz.glob/c")))) + Eventually(komega.Object(sub1)).Should(HaveField("Annotations", Not(HaveKey("baz.glob/c")))) By("changing the parent of sub2") root2 := &corev1.Namespace{} @@ -462,4 +476,60 @@ var _ = Describe("Namespace controller", func() { HaveField("Annotations", HaveKeyWithValue("memo", "tama")), )) }) + + Context("templated namespace", func() { + var ns1 *corev1.Namespace + + BeforeEach(func() { + tmpl := &corev1.Namespace{} + tmpl.GenerateName = "tmpl" + tmpl.Labels = map[string]string{ + constants.LabelType: constants.NSTypeTemplate, + "team": "label", + } + tmpl.Annotations = map[string]string{"memo": "annot"} + Expect(k8sClient.Create(ctx, tmpl)).To(Succeed()) + + ns1 = &corev1.Namespace{} + ns1.GenerateName = "ns1" + ns1.Labels = map[string]string{constants.LabelTemplate: tmpl.Name} + Expect(k8sClient.Create(ctx, ns1)).To(Succeed()) + + Eventually(komega.Object(ns1)).Should(HaveField("Labels", HaveKeyWithValue("team", "label"))) + Expect(ns1.Annotations).To(HaveKeyWithValue("memo", "annot")) + }) + + It("should have stable resourceVersion", func() { + resourceVersion := ns1.ResourceVersion + Consistently(komega.Object(ns1)).Should(HaveField("ResourceVersion", Equal(resourceVersion))) + }) + }) + + Context("sub namespace", func() { + var sub1 *corev1.Namespace + + BeforeEach(func() { + root := &corev1.Namespace{} + root.GenerateName = "root" + root.Labels = map[string]string{ + constants.LabelType: constants.NSTypeRoot, + "team": "label", + } + root.Annotations = map[string]string{"memo": "annot"} + Expect(k8sClient.Create(ctx, root)).To(Succeed()) + + sub1 = &corev1.Namespace{} + sub1.GenerateName = "sub1" + sub1.Labels = map[string]string{constants.LabelParent: root.Name} + Expect(k8sClient.Create(ctx, sub1)).To(Succeed()) + + Eventually(komega.Object(sub1)).Should(HaveField("Labels", HaveKeyWithValue("team", "label"))) + Expect(sub1.Annotations).To(HaveKeyWithValue("memo", "annot")) + }) + + It("should have stable resourceVersion", func() { + resourceVersion := sub1.ResourceVersion + Consistently(komega.Object(sub1)).Should(HaveField("ResourceVersion", Equal(resourceVersion))) + }) + }) }) diff --git a/controllers/propagate.go b/controllers/propagate.go index 6b06e6e..14bed6c 100644 --- a/controllers/propagate.go +++ b/controllers/propagate.go @@ -270,11 +270,10 @@ func (r *PropagateController) propagateUpdate(ctx context.Context, obj, parent * if parent != nil { clone := cloneResource(parent, obj.GetNamespace()) if !equality.Semantic.DeepDerivative(clone, obj) { - clone.SetResourceVersion(obj.GetResourceVersion()) - if err := r.Update(ctx, clone); err != nil { - return fmt.Errorf("failed to update: %w", err) + if err := r.Patch(ctx, clone, applyPatch{clone}, fieldOwner, client.ForceOwnership); err != nil { + return fmt.Errorf("failed to apply %s/%s: %w", clone.GetNamespace(), clone.GetName(), err) } - logger.Info("updated", "from", parent.GetNamespace()) + logger.Info("applied", "from", parent.GetNamespace()) return nil } } @@ -307,12 +306,10 @@ func (r *PropagateController) propagateUpdate(ctx context.Context, obj, parent * continue } - clone.SetResourceVersion(cres.GetResourceVersion()) - if err := r.Update(ctx, clone); err != nil { - return fmt.Errorf("failed to update %s/%s: %w", child.Name, name, err) + if err := r.Patch(ctx, clone, applyPatch{clone}, fieldOwner, client.ForceOwnership); err != nil { + return fmt.Errorf("failed to apply %s/%s: %w", clone.GetNamespace(), clone.GetName(), err) } - - logger.Info("updated a child resource", "subnamespace", child.Name) + logger.Info("applied a child resource", "subnamespace", child.Name) } return nil diff --git a/controllers/ssa_client.go b/controllers/ssa_client.go new file mode 100644 index 0000000..75058bd --- /dev/null +++ b/controllers/ssa_client.go @@ -0,0 +1,44 @@ +package controllers + +import ( + "encoding/json" + + accuratev2alpha1 "github.com/cybozu-go/accurate/api/accurate/v2alpha1" + accuratev2alpha1ac "github.com/cybozu-go/accurate/internal/applyconfigurations/accurate/v2alpha1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + corev1ac "k8s.io/client-go/applyconfigurations/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + fieldOwner client.FieldOwner = "accurate-controller" +) + +func newSubNamespacePatch(ac *accuratev2alpha1ac.SubNamespaceApplyConfiguration) (*accuratev2alpha1.SubNamespace, client.Patch, error) { + sn := &accuratev2alpha1.SubNamespace{} + sn.Name = *ac.Name + sn.Namespace = *ac.Namespace + + return sn, applyPatch{ac}, nil +} + +func newNamespacePatch(ac *corev1ac.NamespaceApplyConfiguration) (*corev1.Namespace, client.Patch, error) { + ns := &corev1.Namespace{} + ns.Name = *ac.Name + + return ns, applyPatch{ac}, nil +} + +type applyPatch struct { + // must use any type until apply configurations implements a common interface + patch any +} + +func (p applyPatch) Type() types.PatchType { + return types.ApplyPatchType +} + +func (p applyPatch) Data(_ client.Object) ([]byte, error) { + return json.Marshal(p.patch) +} diff --git a/controllers/subnamespace_controller.go b/controllers/subnamespace_controller.go index 0e4d6c5..0da4d84 100644 --- a/controllers/subnamespace_controller.go +++ b/controllers/subnamespace_controller.go @@ -2,7 +2,6 @@ package controllers import ( "context" - "encoding/json" "fmt" "time" @@ -25,10 +24,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -const ( - fieldOwner client.FieldOwner = "accurate-controller" -) - // SubNamespaceReconciler reconciles a SubNamespace object type SubNamespaceReconciler struct { client.Client @@ -95,8 +90,9 @@ func (r *SubNamespaceReconciler) finalize(ctx context.Context, sn *accuratev2alp logger.Info("deleted namespace", "name", sn.Name) DELETE: + orig := sn.DeepCopy() controllerutil.RemoveFinalizer(sn, constants.Finalizer) - return r.Update(ctx, sn) + return r.Patch(ctx, sn, client.MergeFrom(orig)) } func (r *SubNamespaceReconciler) reconcileNS(ctx context.Context, sn *accuratev2alpha1.SubNamespace) error { @@ -140,7 +136,11 @@ func (r *SubNamespaceReconciler) reconcileNS(ctx context.Context, sn *accuratev2 ) } - return r.patchSubNamespaceStatus(ctx, ac) + sn, p, err := newSubNamespacePatch(ac) + if err != nil { + return err + } + return r.Status().Patch(ctx, sn, p, fieldOwner, client.ForceOwnership) } // SetupWithManager sets up the controller with the Manager. @@ -184,33 +184,3 @@ func newStatusCondition(existingConditions []metav1.Condition, newCondition meta return newCondition } - -func (r *SubNamespaceReconciler) patchSubNamespaceStatus(ctx context.Context, ac *accuratev2alpha1ac.SubNamespaceApplyConfiguration) error { - sn := &accuratev2alpha1.SubNamespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: *ac.Name, - Namespace: *ac.Namespace, - }, - } - - encodedPatch, err := json.Marshal(ac) - if err != nil { - return err - } - - return r.Status().Patch(ctx, sn, applyPatch{encodedPatch}, fieldOwner, client.ForceOwnership) -} - -type applyPatch struct { - patch []byte -} - -var _ client.Patch = applyPatch{} - -func (p applyPatch) Type() types.PatchType { - return types.ApplyPatchType -} - -func (p applyPatch) Data(_ client.Object) ([]byte, error) { - return p.patch, nil -} diff --git a/docs/subnamespaces.md b/docs/subnamespaces.md index 949e34f..6fa0a66 100644 --- a/docs/subnamespaces.md +++ b/docs/subnamespaces.md @@ -30,7 +30,7 @@ metadata: team: foo ``` -Accurate only propagates labels/annotations that have been configured in that respect via the `labelKeys` and `annotationKeys` parameters in `config.yaml`. This prevents the propagation of labels/annotations that were not meant to do so. Accurate currently does not delete previously propagated labels when deleted from the parent namespace to prevent unintended deletions. Users are expected to manually delete labels/annotations that are no longer needed. +Accurate only propagates labels/annotations that have been configured in that respect via the `labelKeys` and `annotationKeys` parameters in `config.yaml`. This prevents the propagation of labels/annotations that were not meant to do so. ### Preparing resources for tenant users