[SRVKE-1193] Remove sugar resources (openshift-knative#1669)

* [SRVKE-1193] Remove sugar resources

- Remove deployment
- Remove service for monitoring
- Remove service monitor

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

* Add e2e test

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

* Convert dashboard unit test to E2E test

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

* Lint

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

* Fix linter error

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

knative-operator/pkg/controller/knativeeventing/knativeeventing_controller.go

@@ -5,10 +5,8 @@ import (
 	"fmt"
 	"os"
 
-	"github.com/openshift-knative/serverless-operator/knative-operator/pkg/common"
-	"github.com/openshift-knative/serverless-operator/knative-operator/pkg/monitoring"
-	"github.com/openshift-knative/serverless-operator/knative-operator/pkg/monitoring/dashboards"
-	socommon "github.com/openshift-knative/serverless-operator/pkg/common"
+	monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
+	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -24,6 +22,11 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/controller-runtime/pkg/source"
+
+	"github.com/openshift-knative/serverless-operator/knative-operator/pkg/common"
+	"github.com/openshift-knative/serverless-operator/knative-operator/pkg/monitoring"
+	"github.com/openshift-knative/serverless-operator/knative-operator/pkg/monitoring/dashboards"
+	socommon "github.com/openshift-knative/serverless-operator/pkg/common"
 )
 
 const (
@@ -110,7 +113,7 @@ func (r *ReconcileKnativeEventing) Reconcile(ctx context.Context, request reconc
 	}
 
 	instance := original.DeepCopy()
-	reconcileErr := r.reconcileKnativeEventing(instance)
+	reconcileErr := r.reconcileKnativeEventing(ctx, instance)
 
 	if !equality.Semantic.DeepEqual(original.Status, instance.Status) {
 		if err := r.client.Status().Update(context.TODO(), instance); err != nil {
@@ -126,9 +129,10 @@ func (r *ReconcileKnativeEventing) Reconcile(ctx context.Context, request reconc
 	return reconcile.Result{}, reconcileErr
 }
 
-func (r *ReconcileKnativeEventing) reconcileKnativeEventing(instance *operatorv1alpha1.KnativeEventing) error {
+func (r *ReconcileKnativeEventing) reconcileKnativeEventing(ctx context.Context, instance *operatorv1alpha1.KnativeEventing) error {
 	stages := []func(*operatorv1alpha1.KnativeEventing) error{
 		r.ensureFinalizers,
+		r.deleteSugar(ctx),
 		r.installDashboards,
 	}
 	for _, stage := range stages {
@@ -187,3 +191,42 @@ func (r *ReconcileKnativeEventing) delete(instance *operatorv1alpha1.KnativeEven
 	}
 	return nil
 }
+
+func (r *ReconcileKnativeEventing) deleteSugar(ctx context.Context) func(eventing *operatorv1alpha1.KnativeEventing) error {
+	return func(eventing *operatorv1alpha1.KnativeEventing) error {
+		ns := "knative-eventing"
+		nsClient := client.NewNamespacedClient(r.client, ns)
+
+		sugarControllerDeployment := &appsv1.Deployment{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: ns,
+				Name:      "sugar-controller",
+			},
+		}
+		if err := nsClient.Delete(ctx, sugarControllerDeployment); err != nil && !errors.IsNotFound(err) {
+			return fmt.Errorf("failed to delete deployment %s/%s: %w", sugarControllerDeployment.Namespace, sugarControllerDeployment.Name, err)
+		}
+
+		sugarControllerServiceForMonitoring := &corev1.Service{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: ns,
+				Name:      "sugar-controller-sm-service",
+			},
+		}
+		if err := nsClient.Delete(ctx, sugarControllerServiceForMonitoring); err != nil && !errors.IsNotFound(err) {
+			return fmt.Errorf("failed to delete service %s/%s: %w", sugarControllerServiceForMonitoring.Namespace, sugarControllerServiceForMonitoring.Name, err)
+		}
+
+		sugarControllerServiceMonitor := &monitoringv1.ServiceMonitor{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: ns,
+				Name:      "sugar-controller-sm",
+			},
+		}
+		if err := nsClient.Delete(ctx, sugarControllerServiceForMonitoring); err != nil && !errors.IsNotFound(err) {
+			return fmt.Errorf("failed to delete service monitor %s/%s: %w", sugarControllerServiceMonitor.Namespace, sugarControllerServiceMonitor.Name, err)
+		}
+
+		return nil
+	}
+}

knative-operator/pkg/controller/knativeeventing/knativeeventing_controller_test.go

@@ -1,42 +1,12 @@
 package knativeeventing
 
 import (
-	"context"
 	"os"
-	"testing"
 
-	"github.com/openshift-knative/serverless-operator/knative-operator/pkg/apis"
-	"github.com/openshift-knative/serverless-operator/knative-operator/pkg/monitoring/dashboards"
-	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes/scheme"
-	operatorv1alpha1 "knative.dev/operator/pkg/apis/operator/v1alpha1"
-	"sigs.k8s.io/controller-runtime/pkg/client/fake"
-	"sigs.k8s.io/controller-runtime/pkg/reconcile"
-)
 
-var (
-	ke = &operatorv1alpha1.KnativeEventing{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "knative-eventing",
-			Namespace: "knative-eventing",
-		},
-	}
-	req = reconcile.Request{
-		NamespacedName: types.NamespacedName{Namespace: ke.Namespace, Name: ke.Name},
-	}
-	dashboardNamespace = &corev1.Namespace{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: dashboards.ConfigManagedNamespace,
-		},
-	}
-	eventingNamespace = &corev1.Namespace{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: "knative-eventing",
-		},
-	}
+	"github.com/openshift-knative/serverless-operator/knative-operator/pkg/apis"
+	"github.com/openshift-knative/serverless-operator/knative-operator/pkg/monitoring/dashboards"
 )
 
 func init() {
@@ -45,105 +15,3 @@ func init() {
 
 	apis.AddToScheme(scheme.Scheme)
 }
-
-// TestEventingReconcile runs Reconcile to verify if eventing resources are created/deleted.
-func TestEventingReconcile(t *testing.T) {
-	tests := []struct {
-		name           string
-		ownerName      string
-		ownerNamespace string
-		deleted        bool
-	}{{
-		name:           "reconcile request with same KnativeEventing owner",
-		ownerName:      "knative-eventing",
-		ownerNamespace: "knative-eventing",
-		deleted:        true,
-	}, {
-		name:           "reconcile request with different KnativeEventing owner name",
-		ownerName:      "FOO",
-		ownerNamespace: "knative-eventing",
-		deleted:        false,
-	}, {
-		name:           "reconcile request with different KnativeEventing owner namespace",
-		ownerName:      "knative-eventing",
-		ownerNamespace: "FOO",
-		deleted:        false,
-	}}
-
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			cl := fake.NewClientBuilder().WithObjects(ke, dashboardNamespace, eventingNamespace).Build()
-			r := &ReconcileKnativeEventing{client: cl, scheme: scheme.Scheme}
-			// Reconcile to initialize
-			if _, err := r.Reconcile(context.Background(), req); err != nil {
-				t.Fatalf("reconcile: (%v)", err)
-			}
-			// Check if Eventing dashboard configmaps are available
-			resourcesCM := &corev1.ConfigMap{}
-			err := cl.Get(context.TODO(), types.NamespacedName{Name: "grafana-dashboard-definition-knative-eventing-resources", Namespace: dashboardNamespace.Name}, resourcesCM)
-			if err != nil {
-				t.Fatalf("get: (%v)", err)
-			}
-			brokerCM := &corev1.ConfigMap{}
-			err = cl.Get(context.TODO(), types.NamespacedName{Name: "grafana-dashboard-definition-knative-eventing-broker", Namespace: dashboardNamespace.Name}, brokerCM)
-			if err != nil {
-				t.Fatalf("get: (%v)", err)
-			}
-			sourceCM := &corev1.ConfigMap{}
-			err = cl.Get(context.TODO(), types.NamespacedName{Name: "grafana-dashboard-definition-knative-eventing-source", Namespace: dashboardNamespace.Name}, sourceCM)
-			if err != nil {
-				t.Fatalf("get: (%v)", err)
-			}
-			channelCM := &corev1.ConfigMap{}
-			err = cl.Get(context.TODO(), types.NamespacedName{Name: "grafana-dashboard-definition-knative-eventing-channel", Namespace: dashboardNamespace.Name}, channelCM)
-			if err != nil {
-				t.Fatalf("get: (%v)", err)
-			}
-			// Delete Dashboard configmaps.
-			err = cl.Delete(context.TODO(), resourcesCM)
-			if err != nil {
-				t.Fatalf("delete: (%v)", err)
-			}
-			err = cl.Delete(context.TODO(), brokerCM)
-			if err != nil {
-				t.Fatalf("delete: (%v)", err)
-			}
-			err = cl.Delete(context.TODO(), sourceCM)
-			if err != nil {
-				t.Fatalf("delete: (%v)", err)
-			}
-			err = cl.Delete(context.TODO(), channelCM)
-			if err != nil {
-				t.Fatalf("delete: (%v)", err)
-			}
-			// Reconcile again with test requests.
-			req := reconcile.Request{
-				NamespacedName: types.NamespacedName{Namespace: test.ownerNamespace, Name: test.ownerName},
-			}
-			if _, err := r.Reconcile(context.Background(), req); err != nil {
-				t.Fatalf("reconcile: (%v)", err)
-			}
-			var checkError = func(t *testing.T, err error) {
-				if test.deleted {
-					if err != nil {
-						t.Fatalf("get: (%v)", err)
-					}
-				}
-				if !test.deleted {
-					if !errors.IsNotFound(err) {
-						t.Fatalf("get: (%v)", err)
-					}
-				}
-			}
-			// Check again if Eventing dashboard configmaps are available
-			err = cl.Get(context.TODO(), types.NamespacedName{Name: "grafana-dashboard-definition-knative-eventing-resources", Namespace: dashboardNamespace.Name}, resourcesCM)
-			checkError(t, err)
-			err = cl.Get(context.TODO(), types.NamespacedName{Name: "grafana-dashboard-definition-knative-eventing-broker", Namespace: dashboardNamespace.Name}, brokerCM)
-			checkError(t, err)
-			err = cl.Get(context.TODO(), types.NamespacedName{Name: "grafana-dashboard-definition-knative-eventing-source", Namespace: dashboardNamespace.Name}, sourceCM)
-			checkError(t, err)
-			err = cl.Get(context.TODO(), types.NamespacedName{Name: "grafana-dashboard-definition-knative-eventing-channel", Namespace: dashboardNamespace.Name}, sourceCM)
-			checkError(t, err)
-		})
-	}
-}

openshift-knative-operator/pkg/monitoring/monitoring_eventing.go

@@ -10,7 +10,7 @@ import (
 )
 
 var (
-	eventingDeployments = sets.NewString("eventing-controller", "eventing-webhook", "imc-controller", "imc-dispatcher", "mt-broker-controller", "mt-broker-filter", "mt-broker-ingress", "sugar-controller")
+	eventingDeployments = sets.NewString("eventing-controller", "eventing-webhook", "imc-controller", "imc-dispatcher", "mt-broker-controller", "mt-broker-filter", "mt-broker-ingress")
 )
 
 func ReconcileMonitoringForEventing(ctx context.Context, api kubernetes.Interface, ke *operatorv1alpha1.KnativeEventing) error {

test/e2e/dashboards.go

@@ -0,0 +1,51 @@
+package e2e
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/wait"
+
+	"github.com/openshift-knative/serverless-operator/test"
+)
+
+var (
+	EventingDashboards = []string{
+		"grafana-dashboard-definition-knative-eventing-resources",
+		"grafana-dashboard-definition-knative-eventing-broker",
+		"grafana-dashboard-definition-knative-eventing-kafka-broker",
+		"grafana-dashboard-definition-knative-eventing-source",
+		"grafana-dashboard-definition-knative-eventing-channel",
+		"grafana-dashboard-definition-knative-eventing-kafka-sink",
+	}
+)
+
+func VerifyDashboards(t *testing.T, caCtx *test.Context, dashboards []string) {
+	t.Run("Verify dashboards", func(t *testing.T) {
+		t.Parallel()
+
+		ns := "openshift-config-managed"
+		ctx := context.Background()
+
+		for _, d := range dashboards {
+			t.Run(d, func(t *testing.T) {
+				err := wait.PollImmediate(time.Second, time.Minute, func() (bool, error) {
+					_, err := caCtx.Clients.Kube.CoreV1().ConfigMaps(ns).Get(ctx, d, metav1.GetOptions{})
+					if err != nil && !apierrors.IsNotFound(err) {
+						return false, err
+					}
+					if apierrors.IsNotFound(err) {
+						return false, nil
+					}
+					return true, nil
+				})
+				if err != nil {
+					t.Error(err)
+				}
+			})
+		}
+	})
+}

test/e2e/knative_eventing_test.go

@@ -53,10 +53,18 @@ func TestKnativeEventing(t *testing.T) {
 		VerifyNoDisallowedImageReference(t, caCtx, eventingNamespace)
 	})
 
+	t.Run("Verify sugar controller deletion", func(t *testing.T) {
+		if err := test.CheckNoDeployment(caCtx.Clients.Kube, eventingNamespace, "sugar-controller"); err != nil {
+			t.Errorf("sugar-controller is still present: %v", err)
+		}
+	})
+
 	t.Run("Verify job succeeded", func(t *testing.T) {
 		upgrade.VerifyPostInstallJobs(caCtx, upgrade.VerifyPostJobsConfig{
 			Namespace:    eventingNamespace,
 			FailOnNoJobs: true,
 		})
 	})
+
+	VerifyDashboards(t, caCtx, EventingDashboards)
 }

test/service.go

@@ -4,13 +4,16 @@ import (
 	"context"
 	"fmt"
 	"strings"
+	"time"
 
 	routev1 "github.com/openshift/api/route/v1"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/wait"
+	"k8s.io/client-go/kubernetes"
 	servingv1 "knative.dev/serving/pkg/apis/serving/v1"
 	servingv1alpha1 "knative.dev/serving/pkg/apis/serving/v1alpha1"
 )
@@ -101,6 +104,20 @@ func CheckDeploymentScale(ctx *Context, ns, name string, scale int) error {
 	return nil
 }
 
+func CheckNoDeployment(kube *kubernetes.Clientset, ns, name string) error {
+	return wait.PollImmediate(time.Second, time.Minute, func() (bool, error) {
+		_, err := kube.AppsV1().Deployments(ns).Get(context.Background(), name, metav1.GetOptions{})
+		if err != nil && apierrors.IsNotFound(err) {
+			return true, nil
+		}
+		if err != nil {
+			return false, err
+		}
+
+		return false, nil
+	})
+}
+
 func WaitForServiceState(ctx *Context, name, namespace string, inState func(s *servingv1.Service, err error) (bool, error)) (*servingv1.Service, error) {
 	var (
 		lastState *servingv1.Service

test/upgrade/upgrade_test.go

@@ -144,6 +144,7 @@ func postUpgradeTests(ctx *test.Context) []pkgupgrade.Operation {
 		kafkaupgrade.SourcePostUpgradeTest(),
 		kafkabrokerupgrade.BrokerPostUpgradeTest(),
 		kafkabrokerupgrade.SinkPostUpgradeTest(),
+		upgrade.VerifySugarControllerDeletion(ctx),
 	)
 	tests = append(tests, servingupgrade.ServingPostUpgradeTests()...)
 	return tests

test/upgrade/verify_upgrade.go

@@ -0,0 +1,15 @@
+package upgrade
+
+import (
+	"knative.dev/pkg/test/upgrade"
+
+	"github.com/openshift-knative/serverless-operator/test"
+)
+
+func VerifySugarControllerDeletion(ctx *test.Context) upgrade.Operation {
+	return upgrade.NewOperation("Verify sugar-controller deletion", func(c upgrade.Context) {
+		if err := test.CheckNoDeployment(ctx.Clients.Kube, "knative-eventing", "sugar-controller"); err != nil {
+			c.T.Error(err)
+		}
+	})
+}