Skip to content

Commit

Permalink
NETOBSERV-1314: cleanup old objects after upgrade (#427)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
jotak authored Sep 21, 2023
1 parent 4989474 commit 63798f7
Show file tree
Hide file tree
Showing 7 changed files with 236 additions and 18 deletions.
3 changes: 1 addition & 2 deletions controllers/ebpf/internal/permissions/permissions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Expand Down
4 changes: 4 additions & 0 deletions controllers/flowcollector_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
68 changes: 68 additions & 0 deletions pkg/cleanup/cleanup.go
Original file line number Diff line number Diff line change
@@ -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
}
90 changes: 90 additions & 0 deletions pkg/cleanup/cleanup_test.go
Original file line number Diff line number Diff line change
@@ -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())
}
8 changes: 8 additions & 0 deletions pkg/helper/flowcollector.go
Original file line number Diff line number Diff line change
@@ -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 {
Expand Down Expand Up @@ -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)
}
50 changes: 50 additions & 0 deletions pkg/test/kube_mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package test
import (
"context"
"errors"
"testing"

"github.com/stretchr/testify/mock"
v1 "k8s.io/api/core/v1"
Expand All @@ -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 {
Expand All @@ -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 {
Expand All @@ -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{}
Expand All @@ -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) {
Expand All @@ -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) {
Expand Down
31 changes: 15 additions & 16 deletions pkg/watchers/watcher_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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) {
Expand All @@ -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)
}

0 comments on commit 63798f7

Please sign in to comment.