Merge pull request #1115 from anirudhAgniRedhat/OCPBUGS-31550-deleting_SMCP_breaks_Gateway_API

OCPBUGS-31550: Gateway API - recreating SMCP which breaks Gateway API

Author: openshift-merge-bot[bot]

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 {