From 63798f7c93434a1dfbbbb7da7d781ccc6e6c2f75 Mon Sep 17 00:00:00 2001
From: Joel Takvorian <jtakvori@redhat.com>
Date: Thu, 21 Sep 2023 13:27:50 +0200
Subject: [PATCH] NETOBSERV-1314: cleanup old objects after upgrade (#427)
* NETOBSERV-1314: cleanup old objects after upgrade
* Deleting old objects: make sure we only delete owned objects
Add more tests
Also fixes an issue with the kube client mock assertions: some
assertions on function calls didn't have the intended effect when used with the wrong number of arguments
---
.../ebpf/internal/permissions/permissions.go | 3 +-
controllers/flowcollector_controller.go | 4 +
pkg/cleanup/cleanup.go | 68 ++++++++++++++
pkg/cleanup/cleanup_test.go | 90 +++++++++++++++++++
pkg/helper/flowcollector.go | 8 ++
pkg/test/kube_mock.go | 50 +++++++++++
pkg/watchers/watcher_test.go | 31 ++++---
7 files changed, 236 insertions(+), 18 deletions(-)
create mode 100644 pkg/cleanup/cleanup.go
create mode 100644 pkg/cleanup/cleanup_test.go
diff --git a/controllers/ebpf/internal/permissions/permissions.go b/controllers/ebpf/internal/permissions/permissions.go
index 823344ba5..a7705f32f 100644
--- a/controllers/ebpf/internal/permissions/permissions.go
+++ b/controllers/ebpf/internal/permissions/permissions.go
@@ -3,7 +3,6 @@ package permissions
import (
"context"
"fmt"
- "strings"
flowslatest "github.com/netobserv/network-observability-operator/api/v1beta1"
"github.com/netobserv/network-observability-operator/controllers/constants"
@@ -210,7 +209,7 @@ func (c *Reconciler) cleanupPreviousNamespace(ctx context.Context) error {
return fmt.Errorf("can't retrieve previous namespace: %w", err)
}
// Make sure we own that namespace
- if len(previous.OwnerReferences) > 0 && strings.HasPrefix(previous.OwnerReferences[0].APIVersion, flowslatest.GroupVersion.Group) {
+ if helper.IsOwned(previous) {
rlog.Info("Owning previous privileged namespace: deleting it")
if err := c.Delete(ctx, previous); err != nil {
if errors.IsNotFound(err) {
diff --git a/controllers/flowcollector_controller.go b/controllers/flowcollector_controller.go
index 38c5f5264..64681ec75 100644
--- a/controllers/flowcollector_controller.go
+++ b/controllers/flowcollector_controller.go
@@ -31,6 +31,7 @@ import (
"github.com/netobserv/network-observability-operator/controllers/operator"
"github.com/netobserv/network-observability-operator/controllers/ovs"
"github.com/netobserv/network-observability-operator/controllers/reconcilers"
+ "github.com/netobserv/network-observability-operator/pkg/cleanup"
"github.com/netobserv/network-observability-operator/pkg/conditions"
"github.com/netobserv/network-observability-operator/pkg/discover"
"github.com/netobserv/network-observability-operator/pkg/helper"
@@ -98,6 +99,9 @@ func (r *FlowCollectorReconciler) Reconcile(ctx context.Context, _ ctrl.Request)
}
ns := getNamespaceName(desired)
+ if err := cleanup.CleanPastReferences(ctx, r.Client, ns); err != nil {
+ return ctrl.Result{}, err
+ }
r.watcher.Reset(ns)
var didChange, isInProgress bool
diff --git a/pkg/cleanup/cleanup.go b/pkg/cleanup/cleanup.go
new file mode 100644
index 000000000..3f535ded7
--- /dev/null
+++ b/pkg/cleanup/cleanup.go
@@ -0,0 +1,68 @@
+package cleanup
+
+import (
+ "context"
+
+ "github.com/netobserv/network-observability-operator/pkg/helper"
+ corev1 "k8s.io/api/core/v1"
+ "k8s.io/apimachinery/pkg/api/errors"
+ "sigs.k8s.io/controller-runtime/pkg/client"
+ "sigs.k8s.io/controller-runtime/pkg/log"
+)
+
+var (
+ // Add to this list any object that we used to generate in past versions, and stopped doing so.
+ // For instance, with any object that was renamed between two releases of the operator:
+ // The old version with a different name could therefor remain on the cluster after an upgrade.
+ cleanupList = []cleanupItem{
+ {
+ // Old name of NetObserv grafana dashboard / configmap (noo 1.3)
+ ref: client.ObjectKey{Name: "grafana-dashboard-netobserv", Namespace: "openshift-config-managed"},
+ placeholder: &corev1.ConfigMap{},
+ },
+ }
+ // Need to run only once, at operator startup, this is not part of the reconcile loop
+ didRun = false
+)
+
+type cleanupItem struct {
+ ref client.ObjectKey
+ placeholder client.Object
+}
+
+func CleanPastReferences(ctx context.Context, cl client.Client, defaultNamespace string) error {
+ if didRun {
+ return nil
+ }
+ log := log.FromContext(ctx)
+ log.Info("Check and clean old objects")
+ // Search for all past references to clean up. If one is found, delete it.
+ for _, item := range cleanupList {
+ if item.ref.Namespace == "" {
+ item.ref.Namespace = defaultNamespace
+ }
+ if err := cl.Get(ctx, item.ref, item.placeholder); err != nil {
+ if errors.IsNotFound(err) {
+ continue
+ }
+ return err
+ }
+ // Make sure we own that object
+ if helper.IsOwned(item.placeholder) {
+ log.
+ WithValues("name", item.ref.Name).
+ WithValues("namespace", item.ref.Namespace).
+ Info("Deleting old object")
+ if err := cl.Delete(ctx, item.placeholder); err != nil {
+ return err
+ }
+ } else {
+ log.
+ WithValues("name", item.ref.Name).
+ WithValues("namespace", item.ref.Namespace).
+ Info("An object was found, but we don't own it - skip deletion")
+ }
+ }
+ didRun = true
+ return nil
+}
diff --git a/pkg/cleanup/cleanup_test.go b/pkg/cleanup/cleanup_test.go
new file mode 100644
index 000000000..bcccdd71b
--- /dev/null
+++ b/pkg/cleanup/cleanup_test.go
@@ -0,0 +1,90 @@
+package cleanup
+
+import (
+ "context"
+ "testing"
+
+ "github.com/netobserv/network-observability-operator/pkg/test"
+ "github.com/stretchr/testify/assert"
+ corev1 "k8s.io/api/core/v1"
+ v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+ "k8s.io/apimachinery/pkg/types"
+ "k8s.io/utils/pointer"
+)
+
+var oldDashboard = corev1.ConfigMap{
+ ObjectMeta: v1.ObjectMeta{
+ Name: "grafana-dashboard-netobserv",
+ Namespace: "openshift-config-managed",
+ OwnerReferences: []v1.OwnerReference{{
+ APIVersion: "flows.netobserv.io/v1beta1",
+ Kind: "FlowCollector",
+ Name: "cluster",
+ Controller: pointer.Bool(true),
+ }},
+ },
+ Data: map[string]string{},
+}
+
+func TestCleanPastReferences(t *testing.T) {
+ assert := assert.New(t)
+ clientMock := test.ClientMock{}
+ clientMock.MockConfigMap(&oldDashboard)
+ assert.Equal(1, clientMock.Len())
+ didRun = false
+
+ err := CleanPastReferences(context.Background(), &clientMock, "netobserv")
+ assert.NoError(err)
+ clientMock.AssertGetCalledWith(t, types.NamespacedName{Name: "grafana-dashboard-netobserv", Namespace: "openshift-config-managed"})
+ clientMock.AssertDeleteCalled(t)
+ assert.Equal(0, clientMock.Len())
+}
+
+func TestCleanPastReferences_Empty(t *testing.T) {
+ assert := assert.New(t)
+ clientMock := test.ClientMock{}
+ clientMock.MockNonExisting(types.NamespacedName{Name: "grafana-dashboard-netobserv", Namespace: "openshift-config-managed"})
+ assert.Equal(0, clientMock.Len())
+ didRun = false
+
+ err := CleanPastReferences(context.Background(), &clientMock, "netobserv")
+ assert.NoError(err)
+ clientMock.AssertGetCalledWith(t, types.NamespacedName{Name: "grafana-dashboard-netobserv", Namespace: "openshift-config-managed"})
+ clientMock.AssertDeleteNotCalled(t)
+}
+
+func TestCleanPastReferences_NotManaged(t *testing.T) {
+ assert := assert.New(t)
+ clientMock := test.ClientMock{}
+ unmanaged := oldDashboard
+ unmanaged.OwnerReferences = nil
+ clientMock.MockConfigMap(&unmanaged)
+ assert.Equal(1, clientMock.Len())
+ didRun = false
+
+ err := CleanPastReferences(context.Background(), &clientMock, "netobserv")
+ assert.NoError(err)
+ clientMock.AssertGetCalledWith(t, types.NamespacedName{Name: "grafana-dashboard-netobserv", Namespace: "openshift-config-managed"})
+ clientMock.AssertDeleteNotCalled(t)
+ assert.Equal(1, clientMock.Len())
+}
+
+func TestCleanPastReferences_DifferentOwner(t *testing.T) {
+ assert := assert.New(t)
+ clientMock := test.ClientMock{}
+ unmanaged := oldDashboard
+ unmanaged.OwnerReferences = []v1.OwnerReference{{
+ APIVersion: "something/v1beta1",
+ Kind: "SomethingElse",
+ Name: "SomethingElse",
+ }}
+ clientMock.MockConfigMap(&unmanaged)
+ assert.Equal(1, clientMock.Len())
+ didRun = false
+
+ err := CleanPastReferences(context.Background(), &clientMock, "netobserv")
+ assert.NoError(err)
+ clientMock.AssertGetCalledWith(t, types.NamespacedName{Name: "grafana-dashboard-netobserv", Namespace: "openshift-config-managed"})
+ clientMock.AssertDeleteNotCalled(t)
+ assert.Equal(1, clientMock.Len())
+}
diff --git a/pkg/helper/flowcollector.go b/pkg/helper/flowcollector.go
index 7853de830..895b38559 100644
--- a/pkg/helper/flowcollector.go
+++ b/pkg/helper/flowcollector.go
@@ -1,8 +1,11 @@
package helper
import (
+ "strings"
+
flowslatest "github.com/netobserv/network-observability-operator/api/v1beta1"
"github.com/netobserv/network-observability-operator/controllers/constants"
+ "sigs.k8s.io/controller-runtime/pkg/client"
)
func GetSampling(spec *flowslatest.FlowCollectorSpec) int {
@@ -146,3 +149,8 @@ func PtrInt32(i *int32) int32 {
}
return *i
}
+
+func IsOwned(obj client.Object) bool {
+ refs := obj.GetOwnerReferences()
+ return len(refs) > 0 && strings.HasPrefix(refs[0].APIVersion, flowslatest.GroupVersion.Group)
+}
diff --git a/pkg/test/kube_mock.go b/pkg/test/kube_mock.go
index f81a68266..18c08a4f0 100644
--- a/pkg/test/kube_mock.go
+++ b/pkg/test/kube_mock.go
@@ -3,6 +3,7 @@ package test
import (
"context"
"errors"
+ "testing"
"github.com/stretchr/testify/mock"
v1 "k8s.io/api/core/v1"
@@ -22,11 +23,19 @@ func key(obj client.Object) string {
return obj.GetObjectKind().GroupVersionKind().Kind + "/" + obj.GetNamespace() + "/" + obj.GetName()
}
+func (o *ClientMock) Len() int {
+ return len(o.objs)
+}
+
func (o *ClientMock) Get(ctx context.Context, nsname types.NamespacedName, obj client.Object, opts ...client.GetOption) error {
args := o.Called(ctx, nsname, obj, opts)
return args.Error(0)
}
+func (o *ClientMock) AssertGetCalledWith(t *testing.T, nsname types.NamespacedName) {
+ o.AssertCalled(t, "Get", mock.Anything, nsname, mock.Anything, mock.Anything)
+}
+
func (o *ClientMock) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
k := key(obj)
if _, exists := o.objs[k]; exists {
@@ -37,6 +46,15 @@ func (o *ClientMock) Create(ctx context.Context, obj client.Object, opts ...clie
return args.Error(0)
}
+func (o *ClientMock) AssertCreateCalled(t *testing.T) {
+ o.AssertCalled(t, "Create", mock.Anything, mock.Anything, mock.Anything)
+}
+
+func (o *ClientMock) AssertCreateNotCalled(t *testing.T) {
+ o.AssertNotCalled(t, "Create", mock.Anything, mock.Anything)
+ o.AssertNotCalled(t, "Create", mock.Anything, mock.Anything, mock.Anything)
+}
+
func (o *ClientMock) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
k := key(obj)
if _, exists := o.objs[k]; !exists {
@@ -47,6 +65,34 @@ func (o *ClientMock) Update(ctx context.Context, obj client.Object, opts ...clie
return args.Error(0)
}
+func (o *ClientMock) AssertUpdateCalled(t *testing.T) {
+ o.AssertCalled(t, "Update", mock.Anything, mock.Anything, mock.Anything)
+}
+
+func (o *ClientMock) AssertUpdateNotCalled(t *testing.T) {
+ o.AssertNotCalled(t, "Update", mock.Anything, mock.Anything)
+ o.AssertNotCalled(t, "Update", mock.Anything, mock.Anything, mock.Anything)
+}
+
+func (o *ClientMock) Delete(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
+ k := key(obj)
+ if _, exists := o.objs[k]; !exists {
+ return errors.New("doesn't exist")
+ }
+ delete(o.objs, k)
+ args := o.Called(ctx, obj, opts)
+ return args.Error(0)
+}
+
+func (o *ClientMock) AssertDeleteCalled(t *testing.T) {
+ o.AssertCalled(t, "Delete", mock.Anything, mock.Anything, mock.Anything)
+}
+
+func (o *ClientMock) AssertDeleteNotCalled(t *testing.T) {
+ o.AssertNotCalled(t, "Delete", mock.Anything, mock.Anything)
+ o.AssertNotCalled(t, "Delete", mock.Anything, mock.Anything, mock.Anything)
+}
+
func (o *ClientMock) MockSecret(obj *v1.Secret) {
if o.objs == nil {
o.objs = map[string]client.Object{}
@@ -56,8 +102,10 @@ func (o *ClientMock) MockSecret(obj *v1.Secret) {
arg := args.Get(2).(*v1.Secret)
arg.SetName(obj.GetName())
arg.SetNamespace(obj.GetNamespace())
+ arg.SetOwnerReferences(obj.GetOwnerReferences())
arg.Data = obj.Data
}).Return(nil)
+ o.On("Delete", mock.Anything, mock.Anything, mock.Anything).Return(nil)
}
func (o *ClientMock) MockConfigMap(obj *v1.ConfigMap) {
@@ -69,8 +117,10 @@ func (o *ClientMock) MockConfigMap(obj *v1.ConfigMap) {
arg := args.Get(2).(*v1.ConfigMap)
arg.SetName(obj.GetName())
arg.SetNamespace(obj.GetNamespace())
+ arg.SetOwnerReferences(obj.GetOwnerReferences())
arg.Data = obj.Data
}).Return(nil)
+ o.On("Delete", mock.Anything, mock.Anything, mock.Anything).Return(nil)
}
func (o *ClientMock) UpdateObject(obj client.Object) {
diff --git a/pkg/watchers/watcher_test.go b/pkg/watchers/watcher_test.go
index 5c0df9a81..8b2474caa 100644
--- a/pkg/watchers/watcher_test.go
+++ b/pkg/watchers/watcher_test.go
@@ -8,7 +8,6 @@ import (
"github.com/netobserv/network-observability-operator/pkg/helper"
"github.com/netobserv/network-observability-operator/pkg/test"
"github.com/stretchr/testify/assert"
- "github.com/stretchr/testify/mock"
corev1 "k8s.io/api/core/v1"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
@@ -190,9 +189,9 @@ func TestNoCopy(t *testing.T) {
_, _, err := watcher.ProcessMTLSCerts(context.Background(), cl, &lokiTLS, baseNamespace)
assert.NoError(err)
- clientMock.AssertCalled(t, "Get", mock.Anything, types.NamespacedName{Name: lokiCA.Name, Namespace: lokiCA.Namespace}, mock.Anything, mock.Anything)
- clientMock.AssertNotCalled(t, "Create")
- clientMock.AssertNotCalled(t, "Update")
+ clientMock.AssertGetCalledWith(t, types.NamespacedName{Name: lokiCA.Name, Namespace: lokiCA.Namespace})
+ clientMock.AssertCreateNotCalled(t)
+ clientMock.AssertUpdateNotCalled(t)
}
func TestCopyCertificate(t *testing.T) {
@@ -209,10 +208,10 @@ func TestCopyCertificate(t *testing.T) {
_, _, err := watcher.ProcessMTLSCerts(context.Background(), cl, &otherLokiTLS, baseNamespace)
assert.NoError(err)
- clientMock.AssertCalled(t, "Get", mock.Anything, types.NamespacedName{Name: otherLokiCA.Name, Namespace: otherLokiCA.Namespace}, mock.Anything, mock.Anything)
- clientMock.AssertCalled(t, "Get", mock.Anything, types.NamespacedName{Name: otherLokiCA.Name, Namespace: baseNamespace}, mock.Anything, mock.Anything)
- clientMock.AssertCalled(t, "Create", mock.Anything, mock.Anything, mock.Anything)
- clientMock.AssertNotCalled(t, "Update")
+ clientMock.AssertGetCalledWith(t, types.NamespacedName{Name: otherLokiCA.Name, Namespace: otherLokiCA.Namespace})
+ clientMock.AssertGetCalledWith(t, types.NamespacedName{Name: otherLokiCA.Name, Namespace: baseNamespace})
+ clientMock.AssertCreateCalled(t)
+ clientMock.AssertUpdateNotCalled(t)
}
func TestUpdateCertificate(t *testing.T) {
@@ -236,10 +235,10 @@ func TestUpdateCertificate(t *testing.T) {
_, _, err := watcher.ProcessMTLSCerts(context.Background(), cl, &otherLokiTLS, baseNamespace)
assert.NoError(err)
- clientMock.AssertCalled(t, "Get", mock.Anything, types.NamespacedName{Name: otherLokiCA.Name, Namespace: otherLokiCA.Namespace}, mock.Anything, mock.Anything)
- clientMock.AssertCalled(t, "Get", mock.Anything, types.NamespacedName{Name: otherLokiCA.Name, Namespace: baseNamespace}, mock.Anything, mock.Anything)
- clientMock.AssertNotCalled(t, "Create")
- clientMock.AssertCalled(t, "Update", mock.Anything, mock.Anything, mock.Anything)
+ clientMock.AssertGetCalledWith(t, types.NamespacedName{Name: otherLokiCA.Name, Namespace: otherLokiCA.Namespace})
+ clientMock.AssertGetCalledWith(t, types.NamespacedName{Name: otherLokiCA.Name, Namespace: baseNamespace})
+ clientMock.AssertCreateNotCalled(t)
+ clientMock.AssertUpdateCalled(t)
}
func TestNoUpdateCertificate(t *testing.T) {
@@ -262,8 +261,8 @@ func TestNoUpdateCertificate(t *testing.T) {
_, _, err := watcher.ProcessMTLSCerts(context.Background(), cl, &otherLokiTLS, baseNamespace)
assert.NoError(err)
- clientMock.AssertCalled(t, "Get", mock.Anything, types.NamespacedName{Name: otherLokiCA.Name, Namespace: otherLokiCA.Namespace}, mock.Anything, mock.Anything)
- clientMock.AssertCalled(t, "Get", mock.Anything, types.NamespacedName{Name: otherLokiCA.Name, Namespace: baseNamespace}, mock.Anything, mock.Anything)
- clientMock.AssertNotCalled(t, "Create")
- clientMock.AssertNotCalled(t, "Update")
+ clientMock.AssertGetCalledWith(t, types.NamespacedName{Name: otherLokiCA.Name, Namespace: otherLokiCA.Namespace})
+ clientMock.AssertGetCalledWith(t, types.NamespacedName{Name: otherLokiCA.Name, Namespace: baseNamespace})
+ clientMock.AssertCreateNotCalled(t)
+ clientMock.AssertUpdateNotCalled(t)
}