chore: migrate controllers to SSA

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
 }
 

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())

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)

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

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