Merge pull request #112 from erikgb/ssa-migration

chore: migrate controllers to SSA

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
 }
 

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)))
+		})
+	})
 })

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

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)
+}

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
-}

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