OCPBUGS-31550: Ensure SMCP and Gateway API subscription are reconciled upon deletion

This change ensures that the SMCP and Gateway API subscription get recreated if deleted.

The OSSM operator installs the maistra.io CRDs. Need to create the SMCP watch after the OSSM operator is installed.

Acknowledging @rfredette's input in #1115 (comment).

Co-authored-by: Ryan Fredette <[email protected]>

Author: anirudhAgniRedhat

pkg/operator/controller/gatewayclass/controller.go

@@ -2,13 +2,20 @@ package gatewayclass
 
 import (
 	"context"
+	"fmt"
+	"sync"
 
 	logf "github.com/openshift/cluster-ingress-operator/pkg/log"
+	operatorcontroller "github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
+
+	maistrav2 "github.com/maistra/istio-operator/pkg/apis/maistra/v2"
+	operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
 
 	"k8s.io/client-go/tools/record"
 
 	gatewayapiv1 "sigs.k8s.io/gateway-api/apis/v1"
 
+	"k8s.io/apimachinery/pkg/types"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 
 	"sigs.k8s.io/controller-runtime/pkg/cache"
@@ -35,6 +42,7 @@ const (
 )
 
 var log = logf.Logger.WithName(controllerName)
+var gatewayClassController controller.Controller
 
 // NewUnmanaged creates and returns a controller that watches gatewayclasses and
 // installs and configures Istio.  This is an unmanaged controller, which means
@@ -61,6 +69,15 @@ func NewUnmanaged(mgr manager.Manager, config Config) (controller.Controller, er
 	if err := c.Watch(source.Kind[client.Object](operatorCache, &gatewayapiv1.GatewayClass{}, &handler.EnqueueRequestForObject{}, isOurGatewayClass, notIstioGatewayClass)); err != nil {
 		return nil, err
 	}
+
+	isServiceMeshSubscription := predicate.NewPredicateFuncs(func(o client.Object) bool {
+		return o.GetName() == operatorcontroller.ServiceMeshSubscriptionName().Name
+	})
+	if err = c.Watch(source.Kind[client.Object](operatorCache, &operatorsv1alpha1.Subscription{},
+		enqueueRequestForDefaultGatewayClassController(config.OperandNamespace), isServiceMeshSubscription)); err != nil {
+		return nil, err
+	}
+	gatewayClassController = c
 	return c, nil
 }
 
@@ -81,6 +98,23 @@ type reconciler struct {
 	client   client.Client
 	cache    cache.Cache
 	recorder record.EventRecorder
+
+	startSMCPWatch sync.Once
+}
+
+func enqueueRequestForDefaultGatewayClassController(namespace string) handler.EventHandler {
+	return handler.EnqueueRequestsFromMapFunc(
+		func(ctx context.Context, a client.Object) []reconcile.Request {
+			return []reconcile.Request{
+				{
+					NamespacedName: types.NamespacedName{
+						Namespace: namespace,
+						Name:      OpenShiftDefaultGatewayClassName,
+					},
+				},
+			}
+		},
+	)
 }
 
 // Reconcile expects request to refer to a GatewayClass and creates or
@@ -95,10 +129,23 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 
 	var errs []error
 	if _, _, err := r.ensureServiceMeshOperatorSubscription(ctx); err != nil {
-		errs = append(errs, err)
+		errs = append(errs, fmt.Errorf("failed to ensure ServiceMeshOperatorSubscription: %w", err))
 	}
 	if _, _, err := r.ensureServiceMeshControlPlane(ctx, &gatewayclass); err != nil {
 		errs = append(errs, err)
+	} else {
+		// The OSSM operator installs the maistra.io CRDs, Need to create the SMCP watch after the OSSM operator is installed.
+		// Using sync.Once here, to start the watch for SMCP, which should run only once.
+		r.startSMCPWatch.Do(func() {
+			isOurSMCP := predicate.NewPredicateFuncs(func(o client.Object) bool {
+				return o.GetName() == operatorcontroller.ServiceMeshControlPlaneName(r.config.OperandNamespace).Name
+			})
+			if err = gatewayClassController.Watch(source.Kind[client.Object](r.cache, &maistrav2.ServiceMeshControlPlane{}, enqueueRequestForDefaultGatewayClassController(r.config.OperandNamespace), isOurSMCP)); err != nil {
+				log.Error(err, "failed to watch ServiceMeshControlPlane", "request", request)
+				errs = append(errs, err)
+			}
+		})
 	}
+
 	return reconcile.Result{}, utilerrors.NewAggregate(errs)
 }

pkg/operator/controller/names.go

@@ -47,6 +47,10 @@ const (
 	// Remote worker label, used for node affinity of router deployment.
 	// Router should not run on remote worker nodes
 	RemoteWorkerLabel = "node.openshift.io/remote-worker"
+
+	// OpenshiftOperatorNamespace is the default namespace for
+	// the openshift operator resources.
+	OpenshiftOperatorNamespace = "openshift-operators"
 )
 
 // IngressClusterOperatorName returns the namespaced name of the ClusterOperator

pkg/operator/operator.go

@@ -147,6 +147,7 @@ func New(config operatorconfig.Config, kubeConfig *rest.Config) (*Operator, erro
 				operatorcontroller.DefaultOperandNamespace:               {},
 				operatorcontroller.DefaultCanaryNamespace:                {},
 				operatorcontroller.GlobalMachineSpecifiedConfigNamespace: {},
+				operatorcontroller.OpenshiftOperatorNamespace:            {},
 			},
 		},
 		// Use a non-caching client everywhere. The default split client does not

test/e2e/gateway_api_test.go

@@ -99,6 +99,7 @@ func testGatewayAPIResources(t *testing.T) {
 // - the OSSM Istio operator is installed successfully and has status Running and Ready. e.g. istio-operator-9f5c88857-2xfrr  -n openshift-operators
 // - Istiod is installed successfully and has status Running and Ready.  e.g istiod-openshift-gateway-867bb8d5c7-4z6mp -n openshift-ingress
 // - the SMCP is created successfully (OSSM 2.x).
+// - deletes SMCP and subscription and tests if it gets recreated
 func testGatewayAPIIstioInstallation(t *testing.T) {
 	t.Helper()
 
@@ -118,6 +119,22 @@ func testGatewayAPIIstioInstallation(t *testing.T) {
 	if err := assertSMCP(t); err != nil {
 		t.Fatalf("failed to find expected SMCP: %v", err)
 	}
+	// delete existing SMCP to test it gets recreated
+	if err := deleteExistingSMCP(t); err != nil {
+		t.Fatalf("failed to delete existing SMCP: %v", err)
+	}
+	// check if SMCP gets recreated
+	if err := assertSMCP(t); err != nil {
+		t.Fatalf("failed to find expected SMCP: %v", err)
+	}
+	// delete existing Subscription to test it gets recreated
+	if err := deleteExistingSubscription(t, openshiftOperatorsNamespace, expectedSubscriptionName); err != nil {
+		t.Fatalf("failed to delete existing Subscription %s: %v", expectedSubscriptionName, err)
+	}
+	// checks if subscription gets recreated.
+	if err := assertSubscription(t, openshiftOperatorsNamespace, expectedSubscriptionName); err != nil {
+		t.Fatalf("failed to find expected Subscription %s: %v", expectedSubscriptionName, err)
+	}
 }
 
 // testGatewayAPIObjects tests that Gateway API objects can be created successfully.

test/e2e/util_gatewayapi_test.go

@@ -289,6 +289,51 @@ func assertSubscription(t *testing.T, namespace, subName string) error {
 	return err
 }
 
+// deleteExistingSubscription deletes if the subscription of the given name exists and returns an error if not.
+func deleteExistingSubscription(t *testing.T, namespace, subName string) error {
+	t.Helper()
+	existingSubscription := &operatorsv1alpha1.Subscription{}
+	newSubscription := &operatorsv1alpha1.Subscription{}
+	nsName := types.NamespacedName{Namespace: namespace, Name: subName}
+
+	err := wait.PollUntilContextTimeout(context.Background(), 1*time.Second, 30*time.Second, false, func(context context.Context) (bool, error) {
+		if err := kclient.Get(context, nsName, existingSubscription); err != nil {
+			t.Logf("failed to get Subscription %s: %v", nsName.Name, err)
+			return false, nil
+		}
+		return true, nil
+	})
+	if err != nil {
+		return fmt.Errorf("timed out getting subscription %s: %w", nsName.Name, err)
+	}
+	// deleting Subscription.
+	err = kclient.Delete(context.Background(), existingSubscription)
+	if err != nil {
+		return err
+	}
+	err = wait.PollUntilContextTimeout(context.Background(), 1*time.Second, 1*time.Minute, false, func(ctx context.Context) (bool, error) {
+		if err := kclient.Get(ctx, nsName, newSubscription); err != nil {
+			if kerrors.IsNotFound(err) {
+				return true, nil
+			}
+			t.Logf("failed to get deleted subscription %s: %v", nsName.Name, err)
+			return false, nil
+		}
+		// if new Subscription got recreated while the poll ensures the Subscription is deleted.
+		if newSubscription != nil && newSubscription.UID != existingSubscription.UID {
+			return true, nil
+		}
+		t.Logf("Subscription %s still exists", nsName.Name)
+		return false, nil
+	})
+	if err != nil {
+		return fmt.Errorf("timed out waiting for Subscription %s to be deleted: %v", nsName.Name, err)
+	}
+	t.Logf("deleted Subscription %s", nsName.Name)
+	return nil
+
+}
+
 // assertOSSMOperator checks if the OSSM Istio operator gets successfully installed
 // and returns an error if not.
 func assertOSSMOperator(t *testing.T) error {
@@ -625,6 +670,51 @@ func assertSMCP(t *testing.T) error {
 	return err
 }
 
+// deleteExistingSMCP deletes if the SMCP exists and returns an error if not.
+func deleteExistingSMCP(t *testing.T) error {
+	t.Helper()
+	existingSMCP := &maistrav2.ServiceMeshControlPlane{}
+	newSMCP := &maistrav2.ServiceMeshControlPlane{}
+	nsName := types.NamespacedName{Namespace: operatorcontroller.DefaultOperandNamespace, Name: openshiftSMCPName}
+
+	err := wait.PollUntilContextTimeout(context.Background(), 1*time.Second, 30*time.Second, false, func(context context.Context) (bool, error) {
+		if err := kclient.Get(context, nsName, existingSMCP); err != nil {
+			t.Logf("failed to get smcp %s: %v", nsName.Name, err)
+			return false, nil
+		}
+		return true, nil
+	})
+	if err != nil {
+		return fmt.Errorf("timed out getting smcp %s: %w", nsName.Name, err)
+	}
+	// deleting SMCP.
+	err = kclient.Delete(context.Background(), existingSMCP)
+	if err != nil {
+		t.Errorf("failed to delete smcp %s: %v", nsName.Name, err)
+		return err
+	}
+	err = wait.PollUntilContextTimeout(context.Background(), 1*time.Second, 1*time.Minute, false, func(ctx context.Context) (bool, error) {
+		if err := kclient.Get(ctx, nsName, newSMCP); err != nil {
+			if kerrors.IsNotFound(err) {
+				return true, nil
+			}
+			t.Logf("failed to get deleted SMCP %s: %v", nsName.Name, err)
+			return false, nil
+		}
+		// if new SMCP got recreated while the poll ensures the SMCP is deleted.
+		if newSMCP != nil && newSMCP.UID != existingSMCP.UID {
+			return true, nil
+		}
+		t.Logf("smcp %s still exists", nsName.Name)
+		return false, nil
+	})
+	if err != nil {
+		return fmt.Errorf("timed out waiting for SMCP %s to be deleted: %v", nsName.Name, err)
+	}
+	t.Logf("deleted smcp %s", nsName.Name)
+	return nil
+}
+
 // assertDNSRecord checks to make sure a DNSRecord exists in a ready state,
 // and returns an error if not.
 func assertDNSRecord(t *testing.T, recordName types.NamespacedName) error {