Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: migrate controllers to SSA #112

Merged
merged 1 commit into from
Feb 16, 2024
Merged

chore: migrate controllers to SSA #112

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 20 additions & 20 deletions controllers/namespace_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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
}
}

Expand All @@ -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
}
Expand Down Expand Up @@ -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
}

Expand Down
82 changes: 76 additions & 6 deletions controllers/namespace_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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()))
})

Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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{}
Expand Down Expand Up @@ -462,4 +476,60 @@ var _ = Describe("Namespace controller", func() {
HaveField("Annotations", HaveKeyWithValue("memo", "tama")),
))
})

Context("templated namespace", func() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you want this test to confirm?
I think confirming that ns is created from template and that labels and annotations are propagated has already been tested here.

accurate/controllers/namespace_controller_test.go

Line 163 in cda331b

Expect(ns1.Labels).To(HaveKeyWithValue("foo.bar/baz", "baz"))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is to ensure consistent reconciles. Reconcile loops is the one thing to fear when migrating to SSA.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to you, I understood.

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)))
})
})
})
15 changes: 6 additions & 9 deletions controllers/propagate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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
Expand Down
44 changes: 44 additions & 0 deletions controllers/ssa_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)
}
44 changes: 7 additions & 37 deletions controllers/subnamespace_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package controllers

import (
"context"
"encoding/json"
"fmt"
"time"

Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
}
2 changes: 1 addition & 1 deletion docs/subnamespaces.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR includes a change in specifications.
Could you please tell me why you are changing this behavior?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yamatcha
This is the very reason why @erikgb is making this PR.
Without SSA, it was difficult to remove labels and annotations safely.
Now with SSA, removing them is a safe operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly! @ymmt2005 is correct that this is the main motivation for this PR. There is yet no 1. class support for SSA in controller-runtime, but I have been involved in migrating several operators to SSA using the same approach as in this PR. So I am pretty sure it works, and it's also properly tested now IMO. The main concern with SSA migration is the risk of getting never-ending reconcile loops, but I have added new tests for this.

We would be extremely happy if this PR could eventually get merged and released. #98 is a semi-blocker for our further rollout of Accurate in our clusters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erikgb
Thank you for the detailed description!
I missed issue refered in PR description.


### Preparing resources for tenant users

Expand Down
Loading