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