Improve eventing istio resources lifecycle (#2365)

Improve handling of the transition enabled <-> disable for the istio feature flag. Signed-off-by: Pierangelo Di Pilato <[email protected]> Co-authored-by: Pierangelo Di Pilato <[email protected]>
openshift-knative · Nov 2, 2023 · 09eb0bd · 09eb0bd
1 parent f46356d
commit 09eb0bd
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 3 deletions.
diff --git a/openshift-knative-operator/pkg/eventing/extension.go b/openshift-knative-operator/pkg/eventing/extension.go
@@ -6,13 +6,20 @@ import (
 	"os"
 
 	mf "github.com/manifestival/manifestival"
+	"go.uber.org/zap"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/api/resource"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/kubernetes"
 	"knative.dev/operator/pkg/apis/operator/base"
 	operatorv1beta1 "knative.dev/operator/pkg/apis/operator/v1beta1"
 	operator "knative.dev/operator/pkg/reconciler/common"
 	kubeclient "knative.dev/pkg/client/injection/kube/client"
 	"knative.dev/pkg/controller"
+	"knative.dev/pkg/injection/clients/dynamicclient"
+	"knative.dev/pkg/logging"
 	"knative.dev/pkg/ptr"
 
 	"github.com/openshift-knative/serverless-operator/openshift-knative-operator/pkg/common"
@@ -25,12 +32,16 @@ const requiredNsEnvName = "REQUIRED_EVENTING_NAMESPACE"
 // NewExtension creates a new extension for a Knative Eventing controller.
 func NewExtension(ctx context.Context, _ *controller.Impl) operator.Extension {
 	return &extension{
-		kubeclient: kubeclient.Get(ctx),
+		kubeclient:    kubeclient.Get(ctx),
+		dynamicclient: dynamicclient.Get(ctx),
+		logger:        logging.FromContext(ctx),
 	}
 }
 
 type extension struct {
-	kubeclient kubernetes.Interface
+	kubeclient    kubernetes.Interface
+	dynamicclient dynamic.Interface
+	logger        *zap.SugaredLogger
 }
 
 func (e *extension) Manifests(ke base.KComponent) ([]mf.Manifest, error) {
@@ -44,10 +55,29 @@ func (e *extension) Manifests(ke base.KComponent) ([]mf.Manifest, error) {
 	}
 	if enabled := eventingistio.IsEnabled(ke.GetSpec().GetConfig()); enabled {
 		m = append(m, p)
+	} else {
+		// This handles the case when it transitions from "enabled" to "disabled".
+		e.deleteResourcesSilently(p)
 	}
 	return m, nil
 }
 
+func (e *extension) deleteResourcesSilently(m mf.Manifest) {
+	for _, np := range m.Resources() {
+		r /* plural */, _ /* singular  */ := meta.UnsafeGuessKindToResource(np.GroupVersionKind())
+		err := e.dynamicclient.Resource(r).
+			Namespace(np.GetNamespace()).
+			Delete(context.Background(), np.GetName(), metav1.DeleteOptions{})
+		if err != nil && !apierrors.IsNotFound(err) {
+			// Do not fail completely, just log the error
+			e.logger.Warnw("Failed to delete resource",
+				zap.Any("resource", r),
+				zap.String("namespace", np.GetNamespace()),
+				zap.String("name", np.GetName()))
+		}
+	}
+}
+
 func (e *extension) Transformers(ke base.KComponent) []mf.Transformer {
 	tf := []mf.Transformer{
 		common.InjectCommonLabelIntoNamespace(),

diff --git a/openshift-knative-operator/pkg/eventing/extension_test.go b/openshift-knative-operator/pkg/eventing/extension_test.go
@@ -11,11 +11,13 @@ import (
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/utils/pointer"
 	"knative.dev/operator/pkg/apis/operator/base"
 	operatorv1beta1 "knative.dev/operator/pkg/apis/operator/v1beta1"
 	"knative.dev/pkg/apis"
 	kubefake "knative.dev/pkg/client/injection/kube/client/fake"
+	dynamicfake "knative.dev/pkg/injection/clients/dynamicclient/fake"
 
 	"github.com/openshift-knative/serverless-operator/openshift-knative-operator/pkg/common"
 	"github.com/openshift-knative/serverless-operator/openshift-knative-operator/pkg/monitoring"
@@ -138,6 +140,7 @@ func TestReconcile(t *testing.T) {
 
 			ke := c.in.DeepCopy()
 			ctx, _ := kubefake.With(context.Background(), &eventingNamespace)
+			ctx, _ = dynamicfake.With(ctx, scheme.Scheme)
 			ext := NewExtension(ctx, nil)
 			ext.Reconcile(context.Background(), ke)
 
@@ -271,6 +274,7 @@ func TestMonitoring(t *testing.T) {
 			c.expected.Namespace = ke.Namespace
 			ctx, _ := ocpfake.With(context.Background(), objs...)
 			ctx, kube := kubefake.With(ctx, &eventingNamespace)
+			ctx, _ = dynamicfake.With(ctx, scheme.Scheme)
 			ext := NewExtension(ctx, nil)
 			shouldEnableMonitoring, err := c.setupMonitoringToggle()
 

diff --git a/test/monitoringe2e/monitoring_test.go b/test/monitoringe2e/monitoring_test.go
@@ -35,7 +35,7 @@ func TestKnativeMetrics(t *testing.T) {
 	t.Run("verify Eventing metrics work correctly", func(t *testing.T) {
 		// Eventing control plane metrics should work
 		if err := VerifyMetrics(ctx, eventingMetricQueries); err != nil {
-			t.Fatal("Failed to verify that Eventing control plane metrics work correctly", err)
+			t.Fatal("Failed to verify that Eventing metrics work correctly", err)
 		}
 	})