From 334d2243eb38bfcf92dccafa56f307ad6d40bcd7 Mon Sep 17 00:00:00 2001 From: Erik Godding Boye Date: Fri, 8 Dec 2023 19:37:19 +0100 Subject: [PATCH] chore: migrate controllers to SSA --- controllers/namespace_controller.go | 61 ++++++++---------------- controllers/namespace_controller_test.go | 37 ++++++++++---- controllers/propagate.go | 47 +++++------------- controllers/ssa_client.go | 44 +++++++++++++++++ controllers/subnamespace_controller.go | 41 ++-------------- docs/subnamespaces.md | 2 +- 6 files changed, 111 insertions(+), 121 deletions(-) create mode 100644 controllers/ssa_client.go diff --git a/controllers/namespace_controller.go b/controllers/namespace_controller.go index 457af8c..d989517 100644 --- a/controllers/namespace_controller.go +++ b/controllers/namespace_controller.go @@ -4,15 +4,14 @@ import ( "context" "fmt" "path" - "reflect" accuratev2alpha1 "github.com/cybozu-go/accurate/api/accurate/v2alpha1" "github.com/cybozu-go/accurate/pkg/constants" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/equality" 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 +84,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 +108,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 } @@ -231,31 +231,12 @@ func (r *NamespaceReconciler) propagateUpdate(ctx context.Context, res *unstruct logger := log.FromContext(ctx) gvk := res.GroupVersionKind() - c := &unstructured.Unstructured{} - c.SetGroupVersionKind(gvk) - err := r.Get(ctx, client.ObjectKey{Namespace: ns, Name: res.GetName()}, c) - if err != nil { - if !apierrors.IsNotFound(err) { - return err - } - if err := r.Create(ctx, cloneResource(res, ns)); err != nil { - return err - } - logger.Info("created a resource", "namespace", ns, "name", res.GetName(), "gvk", gvk.String()) - return nil - } - - c2 := cloneResource(res, ns) - if equality.Semantic.Equalities.DeepDerivative(c2, c) { - return nil - } - - c2.SetResourceVersion(c.GetResourceVersion()) - if err := r.Update(ctx, c2); err != nil { - return err + clone := cloneResource(res, ns) + 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 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 3e8f849..1973935 100644 --- a/controllers/namespace_controller_test.go +++ b/controllers/namespace_controller_test.go @@ -222,6 +222,22 @@ var _ = Describe("Namespace controller", func() { return ns1.Labels["foo.bar/baz"] }).Should(Equal("123")) + By("removing a label of template namespace") + tmpl = &corev1.Namespace{} + err = k8sClient.Get(ctx, client.ObjectKey{Name: "tmpl"}, tmpl) + Expect(err).NotTo(HaveOccurred()) + delete(tmpl.Labels, "foo.bar/baz") + err = k8sClient.Update(ctx, tmpl) + Expect(err).NotTo(HaveOccurred()) + + Eventually(func() (map[string]string, error) { + ns1 = &corev1.Namespace{} + if err := k8sClient.Get(ctx, client.ObjectKey{Name: "ns1"}, ns1); err != nil { + return nil, err + } + return ns1.Labels, nil + }).Should(Not(HaveKey("foo.bar/baz"))) + tmpl2 := &corev1.Namespace{} tmpl2.Name = "tmpl2" tmpl2.Labels = map[string]string{ @@ -262,6 +278,12 @@ var _ = Describe("Namespace controller", func() { } return ns1.Labels["team"] }).Should(Equal("maneki")) + // assert that labels/annotations from previous template are removed + Expect(ns1.Labels).To(Not(HaveKey("foo.bar/baz"))) + Expect(ns1.Labels).To(Not(HaveKey("memo"))) + Expect(ns1.Annotations).To(Not(HaveKey("foo.bar/zot"))) + Expect(ns1.Annotations).To(Not(HaveKey("memo"))) + Expect(ns1.Annotations).To(Not(HaveKey("team"))) By("unsetting the template") ns1 = &corev1.Namespace{} @@ -484,17 +506,15 @@ var _ = Describe("Namespace controller", func() { }).Should(Equal("nuco")) By("deleting an annotation in root namespace") - delete(root.Labels, "baz.glob/c") - Eventually(func() error { + delete(root.Annotations, "baz.glob/c") + Expect(k8sClient.Update(ctx, root)).To(Succeed()) + Eventually(func() (map[string]string, error) { sub1 = &corev1.Namespace{} if err := k8sClient.Get(ctx, client.ObjectKey{Name: "sub1"}, sub1); err != nil { - return err + return nil, err } - if _, ok := sub1.Annotations["baz.glob/c"]; !ok { - return errors.New("annotation has been deleted") - } - return nil - }).Should(Succeed()) + return sub1.Annotations, nil + }).Should(Not(HaveKey("baz.glob/c"))) By("changing the parent of sub2") root2 := &corev1.Namespace{} @@ -506,6 +526,7 @@ var _ = Describe("Namespace controller", func() { err = k8sClient.Create(ctx, root2) Expect(err).NotTo(HaveOccurred()) + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(sub2), sub2)).To(Succeed()) sub2.Labels[constants.LabelParent] = "root2" err = k8sClient.Update(ctx, sub2) Expect(err).NotTo(HaveOccurred()) diff --git a/controllers/propagate.go b/controllers/propagate.go index 6b06e6e..04d6208 100644 --- a/controllers/propagate.go +++ b/controllers/propagate.go @@ -9,11 +9,9 @@ import ( "github.com/cybozu-go/accurate/pkg/constants" "github.com/cybozu-go/accurate/pkg/feature" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime/schema" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" @@ -265,18 +263,16 @@ func (r *PropagateController) propagateCreate(ctx context.Context, obj *unstruct func (r *PropagateController) propagateUpdate(ctx context.Context, obj, parent *unstructured.Unstructured) error { logger := log.FromContext(ctx) - name := obj.GetName() 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) - } - logger.Info("updated", "from", parent.GetNamespace()) - return nil + 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("applied", "from", parent.GetNamespace()) + + // update obj after apply from parent to prepare it for child propagation below + obj = clone } // propagate to child namespaces, if any. @@ -286,33 +282,11 @@ func (r *PropagateController) propagateUpdate(ctx context.Context, obj, parent * } for _, child := range children.Items { - cres := r.res.DeepCopy() - err := r.Get(ctx, client.ObjectKey{Namespace: child.Name, Name: name}, cres) - if err != nil { - if !apierrors.IsNotFound(err) { - return fmt.Errorf("failed to lookup %s/%s: %w", child.Name, name, err) - } - - clone := cloneResource(obj, child.Name) - if err := r.Create(ctx, clone); err != nil { - return fmt.Errorf("failed to create %s/%s: %w", child.Name, name, err) - } - - logger.Info("created a child resource", "subnamespace", child.Name) - continue - } - clone := cloneResource(obj, child.Name) - if equality.Semantic.DeepDerivative(clone, cres) { - 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 @@ -328,7 +302,8 @@ func (r *PropagateController) checkController(ctx context.Context, obj *unstruct logger := log.FromContext(ctx) owner := &unstructured.Unstructured{} - owner.SetGroupVersionKind(schema.FromAPIVersionAndKind(cref.APIVersion, cref.Kind)) + owner.SetAPIVersion(cref.APIVersion) + owner.SetKind(cref.Kind) if err := r.reader.Get(ctx, client.ObjectKey{Namespace: obj.GetNamespace(), Name: cref.Name}, owner); err != nil { if apierrors.IsNotFound(err) { logger.Info("the controller object is not found", "gvk", owner.GroupVersionKind().String(), "owner", cref.Name) 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..127fed4 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 @@ -140,7 +135,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 +183,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