diff --git a/internal/k8s/controllers/config_controller.go b/internal/k8s/controllers/config_controller.go index b292ee5f818..8713cd3278f 100644 --- a/internal/k8s/controllers/config_controller.go +++ b/internal/k8s/controllers/config_controller.go @@ -18,6 +18,7 @@ package controllers import ( "context" + "reflect" "github.com/davecgh/go-spew/spew" "github.com/go-kit/log" @@ -45,9 +46,14 @@ type ConfigReconciler struct { ValidateConfig config.Validate ForceReload func() BGPType string + currentConfig *config.Config } func (r *ConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + return requestHandler(r, ctx, req) +} + +var requestHandler = func(r *ConfigReconciler, ctx context.Context, req ctrl.Request) (ctrl.Result, error) { level.Info(r.Logger).Log("controller", "ConfigReconciler", "start reconcile", req.NamespacedName.String()) defer level.Info(r.Logger).Log("controller", "ConfigReconciler", "end reconcile", req.NamespacedName.String()) updates.Inc() @@ -127,12 +133,22 @@ func (r *ConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr } level.Debug(r.Logger).Log("controller", "ConfigReconciler", "rendered config", spew.Sdump(cfg)) + if r.currentConfig != nil && reflect.DeepEqual(r.currentConfig, cfg) { + level.Debug(r.Logger).Log("controller", "ConfigReconciler", "event", "configuration did not change, ignoring") + return ctrl.Result{}, nil + } + + r.currentConfig = cfg res := r.Handler(r.Logger, cfg) switch res { case SyncStateError: configStale.Set(1) updateErrors.Inc() + // if the configuration load failed, we reset the current config because this is gonna lead to a retry + // of the reconciliaton loop. If we don't reset, the retry will find the config identical and will exit, + // which is not what we want here. + r.currentConfig = nil level.Error(r.Logger).Log("controller", "ConfigReconciler", "metallb CRs and Secrets", dumpClusterResources(&resources), "event", "reload failed, retry") return ctrl.Result{}, retryError case SyncStateReprocessAll: diff --git a/internal/k8s/controllers/config_controller_test.go b/internal/k8s/controllers/config_controller_test.go index c4cb54b48f0..46f39564eba 100644 --- a/internal/k8s/controllers/config_controller_test.go +++ b/internal/k8s/controllers/config_controller_test.go @@ -36,6 +36,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" k8sscheme "k8s.io/client-go/kubernetes/scheme" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/envtest" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -144,6 +145,80 @@ func TestConfigController(t *testing.T) { } } +func TestSecretShouldntTrigger(t *testing.T) { + initObjects := objectsFromResources(configControllerValidResources) + fakeClient, err := newFakeClient(initObjects) + if err != nil { + t.Fatalf("test failed to create fake client: %v", err) + } + + handlerCalled := false + mockHandler := func(l log.Logger, cfg *config.Config) SyncState { + handlerCalled = true + return SyncStateSuccess + } + + r := &ConfigReconciler{ + Client: fakeClient, + Logger: log.NewNopLogger(), + Scheme: scheme, + Namespace: testNamespace, + ValidateConfig: config.DontValidate, + Handler: mockHandler, + ForceReload: func() {}, + } + req := reconcile.Request{ + NamespacedName: types.NamespacedName{ + Namespace: testNamespace, + }, + } + + _, err = r.Reconcile(context.TODO(), req) + if err != nil { + t.Fatalf("reconcile failed: %v", err) + } + if !handlerCalled { + t.Fatalf("handler not called") + } + handlerCalled = false + err = fakeClient.Create(context.TODO(), &v1beta2.BGPPeer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "peer2", + Namespace: testNamespace, + }, + Spec: v1beta2.BGPPeerSpec{ + MyASN: 42, + ASN: 142, + Address: "1.2.3.4", + BFDProfile: "default", + }, + }) + if err != nil { + t.Fatalf("create failed on peer2: %v", err) + } + _, err = r.Reconcile(context.TODO(), req) + if err != nil { + t.Fatalf("reconcile failed: %v", err) + } + if !handlerCalled { + t.Fatalf("handler not called") + } + + handlerCalled = false + err = fakeClient.Create(context.TODO(), &corev1.Secret{Type: corev1.SecretTypeBasicAuth, ObjectMeta: metav1.ObjectMeta{Name: "foo", Namespace: testNamespace}, + Data: map[string][]byte{"password": []byte([]byte("nopass"))}}) + if err != nil { + t.Fatalf("create failed on secret foo: %v", err) + } + _, err = r.Reconcile(context.TODO(), req) + if err != nil { + t.Fatalf("reconcile failed: %v", err) + } + if handlerCalled { + t.Fatalf("handler called") + } +} + func TestNodeEvent(t *testing.T) { g := NewGomegaWithT(t) testEnv := &envtest.Environment{ @@ -161,31 +236,27 @@ func TestNodeEvent(t *testing.T) { g.Expect(err).To(BeNil()) err = v1beta2.AddToScheme(k8sscheme.Scheme) g.Expect(err).To(BeNil()) - m, err := manager.New(cfg, manager.Options{}) + m, err := manager.New(cfg, manager.Options{MetricsBindAddress: "0"}) g.Expect(err).To(BeNil()) var configUpdate int var mutex sync.Mutex - mockHandler := func(l log.Logger, cfg *config.Config) SyncState { + oldRequestHandler := requestHandler + defer func() { requestHandler = oldRequestHandler }() + + requestHandler = func(r *ConfigReconciler, ctx context.Context, req ctrl.Request) (ctrl.Result, error) { mutex.Lock() defer mutex.Unlock() configUpdate++ - return SyncStateSuccess - } - var forceReload int - mockForceReload := func() { - mutex.Lock() - defer mutex.Unlock() - forceReload++ + return ctrl.Result{}, nil } + r := &ConfigReconciler{ Client: m.GetClient(), Logger: log.NewNopLogger(), Scheme: scheme, Namespace: testNamespace, ValidateConfig: config.DontValidate, - Handler: mockHandler, - ForceReload: mockForceReload, } err = r.SetupWithManager(m) g.Expect(err).To(BeNil()) @@ -209,6 +280,8 @@ func TestNodeEvent(t *testing.T) { defer mutex.Unlock() return configUpdate }, 5*time.Second, 200*time.Millisecond).Should(Equal(1)) + var forceReload int + g.Eventually(func() int { mutex.Lock() defer mutex.Unlock() diff --git a/internal/k8s/controllers/node_controller.go b/internal/k8s/controllers/node_controller.go index a1d737b3217..3c0931d0230 100644 --- a/internal/k8s/controllers/node_controller.go +++ b/internal/k8s/controllers/node_controller.go @@ -22,10 +22,13 @@ import ( "github.com/go-kit/log" "github.com/go-kit/log/level" + corev1 "k8s.io/api/core/v1" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/predicate" ) @@ -65,18 +68,45 @@ func (r *NodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. } func (r *NodeReconciler) SetupWithManager(mgr ctrl.Manager) error { - p := predicate.NewPredicateFuncs( - func(obj client.Object) bool { - node, ok := obj.(*v1.Node) + p := predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { + return r.filterOtherNodes(e.Object) + }, + DeleteFunc: func(e event.DeleteEvent) bool { + return r.filterOtherNodes(e.Object) + }, + GenericFunc: func(e event.GenericEvent) bool { + return r.filterOtherNodes(e.Object) + }, + UpdateFunc: func(e event.UpdateEvent) bool { + newNodeObj, ok := e.ObjectNew.(*corev1.Node) if !ok { - level.Error(r.Logger).Log("controller", "NodeReconciler", "error", "object is not node", "name", obj.GetName()) + level.Error(r.Logger).Log("controller", "NodeReconciler", "error", "new object is not node", "name", newNodeObj.GetName()) + return true + } + oldNodeObj, ok := e.ObjectOld.(*corev1.Node) + if !ok { + level.Error(r.Logger).Log("controller", "NodeReconciler", "error", "old object is not node", "name", oldNodeObj.GetName()) + return true + } + // If there is no changes in node labels, ignore event. + if labels.Equals(labels.Set(oldNodeObj.Labels), labels.Set(newNodeObj.Labels)) { return false } - return node.Name == r.NodeName - }) - + return r.filterOtherNodes(newNodeObj) + }, + } return ctrl.NewControllerManagedBy(mgr). For(&v1.Node{}). WithEventFilter(p). Complete(r) } + +func (r *NodeReconciler) filterOtherNodes(obj client.Object) bool { + node, ok := obj.(*v1.Node) + if !ok { + level.Error(r.Logger).Log("controller", "NodeReconciler", "error", "object is not node", "name", obj.GetName()) + return false + } + return node.Name == r.NodeName +} diff --git a/internal/k8s/controllers/node_controller_test.go b/internal/k8s/controllers/node_controller_test.go index f676044868c..28d47a28374 100644 --- a/internal/k8s/controllers/node_controller_test.go +++ b/internal/k8s/controllers/node_controller_test.go @@ -18,15 +18,24 @@ package controllers import ( "context" + "path/filepath" "reflect" + "sync" "testing" + "time" "github.com/go-kit/log" "github.com/google/go-cmp/cmp" + . "github.com/onsi/gomega" + v1beta1 "go.universe.tf/metallb/api/v1beta1" + v1beta2 "go.universe.tf/metallb/api/v1beta2" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + k8sscheme "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -107,3 +116,105 @@ func TestNodeController(t *testing.T) { } } } + +func TestNodeReconciler_SetupWithManager(t *testing.T) { + g := NewGomegaWithT(t) + testEnv := &envtest.Environment{ + CRDDirectoryPaths: []string{filepath.Join("../../..", "config", "crd", "bases")}, + ErrorIfCRDPathMissing: true, + Scheme: scheme, + } + cfg, err := testEnv.Start() + g.Expect(err).To(BeNil()) + defer func() { + err = testEnv.Stop() + g.Expect(err).To(BeNil()) + }() + err = v1beta1.AddToScheme(k8sscheme.Scheme) + g.Expect(err).To(BeNil()) + err = v1beta2.AddToScheme(k8sscheme.Scheme) + g.Expect(err).To(BeNil()) + m, err := manager.New(cfg, manager.Options{MetricsBindAddress: "0"}) + g.Expect(err).To(BeNil()) + + var configUpdate int + var mutex sync.Mutex + mockHandler := func(l log.Logger, n *corev1.Node) SyncState { + mutex.Lock() + defer mutex.Unlock() + configUpdate++ + return SyncStateSuccess + } + r := &NodeReconciler{ + Client: m.GetClient(), + Logger: log.NewNopLogger(), + Scheme: scheme, + Namespace: testNamespace, + Handler: mockHandler, + NodeName: "test-node", + } + err = r.SetupWithManager(m) + g.Expect(err).To(BeNil()) + ctx := context.Background() + go func() { + err = m.Start(ctx) + g.Expect(err).To(BeNil()) + }() + + // test new node event. + node := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "test-node"}, + Spec: corev1.NodeSpec{}, + } + node.Labels = make(map[string]string) + node.Labels["test"] = "e2e" + err = m.GetClient().Create(ctx, node) + g.Expect(err).To(BeNil()) + g.Eventually(func() int { + mutex.Lock() + defer mutex.Unlock() + return configUpdate + }, 5*time.Second, 200*time.Millisecond).Should(Equal(1)) + + // test update node event with no changes into node label. + g.Eventually(func() error { + err = m.GetClient().Get(ctx, types.NamespacedName{Name: "test-node"}, node) + if err != nil { + return err + } + node.Labels = make(map[string]string) + node.Spec.PodCIDR = "192.168.10.0/24" + node.Labels["test"] = "e2e" + err = m.GetClient().Update(ctx, node) + if err != nil { + return err + } + return nil + }, 5*time.Second, 200*time.Millisecond).Should(BeNil()) + g.Eventually(func() int { + mutex.Lock() + defer mutex.Unlock() + return configUpdate + }, 5*time.Second, 200*time.Millisecond).Should(Equal(1)) + + // test update node event with changes into node label. + g.Eventually(func() error { + err = m.GetClient().Get(ctx, types.NamespacedName{Name: "test-node"}, node) + if err != nil { + return err + } + node.Labels = make(map[string]string) + node.Labels["test"] = "e2e" + node.Labels["test"] = "update" + err = m.GetClient().Update(ctx, node) + if err != nil { + return err + } + return nil + }, 5*time.Second, 200*time.Millisecond).Should(BeNil()) + g.Eventually(func() int { + mutex.Lock() + defer mutex.Unlock() + return configUpdate + }, 5*time.Second, 200*time.Millisecond).Should(Equal(2)) +}