fix: do not reconcile gateways with unsupported gw class (#711)

* fix: do not reconcile gateways with unsupported gw class * Apply suggestions from code review Co-authored-by: Jakub Warczarek <[email protected]> --------- Co-authored-by: Jakub Warczarek <[email protected]>
Kong · Oct 8, 2024 · 8afc14a · 8afc14a
1 parent 292fc6a
commit 8afc14a
Show file tree

Hide file tree

Showing 13 changed files with 290 additions and 110 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -113,6 +113,8 @@
   [#612](https://github.com/Kong/gateway-operator/pull/612)
 - Guard object counters with checks whether CRDs for them exist
   [#710](https://github.com/Kong/gateway-operator/pull/710)
+- Do not reconcile Gateways nor assign any finalizers when the referred GatewayClass is not supported.
+  [#711](https://github.com/Kong/gateway-operator/pull/711)
 
 ### Changes
 

diff --git a/controller/gateway/controller.go b/controller/gateway/controller.go
@@ -32,6 +32,7 @@ import (
 	"github.com/kong/gateway-operator/controller/pkg/watch"
 	operatorerrors "github.com/kong/gateway-operator/internal/errors"
 	gwtypes "github.com/kong/gateway-operator/internal/types"
+	"github.com/kong/gateway-operator/internal/utils/gatewayclass"
 	"github.com/kong/gateway-operator/pkg/consts"
 	gatewayutils "github.com/kong/gateway-operator/pkg/utils/gateway"
 	k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes"
@@ -119,10 +120,24 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		return ctrl.Result{}, nil
 	}
 
+	log.Trace(logger, "checking gatewayclass", gateway)
+	gwc, err := gatewayclass.Get(ctx, r.Client, string(gateway.Spec.GatewayClassName))
+	if err != nil {
+		if errors.As(err, &operatorerrors.ErrUnsupportedGatewayClass{}) {
+			log.Debug(logger, "resource not supported, ignoring", gateway,
+				"expectedGatewayClass", vars.ControllerName(),
+				"gatewayClass", gateway.Spec.GatewayClassName,
+				"reason", err.Error(),
+			)
+			return ctrl.Result{}, nil
+		}
+		return ctrl.Result{}, err
+	}
+
 	log.Trace(logger, "managing the gateway resource finalizers", gateway)
 	cpFinalizerSet := controllerutil.AddFinalizer(&gateway, string(GatewayFinalizerCleanupControlPlanes))
 	dpFinalizerSet := controllerutil.AddFinalizer(&gateway, string(GatewayFinalizerCleanupDataPlanes))
-	npFinalizerSet := controllerutil.AddFinalizer(&gateway, string(GatewayFinalizerCleanupNetworkpolicies))
+	npFinalizerSet := controllerutil.AddFinalizer(&gateway, string(GatewayFinalizerCleanupNetworkPolicies))
 	if cpFinalizerSet || dpFinalizerSet || npFinalizerSet {
 		log.Trace(logger, "Setting finalizers", gateway)
 		if err := r.Client.Update(ctx, &gateway); err != nil {
@@ -137,16 +152,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		return ctrl.Result{}, nil
 	}
 
-	log.Trace(logger, "checking gatewayclass", gateway)
-	gwc, err := r.verifyGatewayClassSupport(ctx, &gateway)
-	if err != nil {
-		if errors.Is(err, operatorerrors.ErrUnsupportedGateway) {
-			log.Debug(logger, "resource not supported, ignoring", gateway, "ExpectedGatewayClass", vars.ControllerName())
-			return ctrl.Result{}, nil
-		}
-		return ctrl.Result{}, err
-	}
-
 	if !gwc.IsAccepted() {
 		log.Debug(logger, "gatewayclass not accepted , ignoring", gateway)
 		return ctrl.Result{}, nil

diff --git a/controller/gateway/controller_cleanup.go b/controller/gateway/controller_cleanup.go
@@ -136,7 +136,7 @@ func (r *Reconciler) cleanup(
 		}
 	} else {
 		oldGateway := gateway.DeepCopy()
-		if controllerutil.RemoveFinalizer(gateway, string(GatewayFinalizerCleanupNetworkpolicies)) {
+		if controllerutil.RemoveFinalizer(gateway, string(GatewayFinalizerCleanupNetworkPolicies)) {
 			if err := r.Client.Patch(ctx, gateway, client.MergeFrom(oldGateway)); err != nil {
 				res, err := handleGatewayFinalizerPatchOrUpdateError(err, gateway, logger)
 				return true, res, err

diff --git a/controller/gateway/controller_consts.go b/controller/gateway/controller_consts.go
@@ -12,6 +12,6 @@ const (
 	GatewayFinalizerCleanupDataPlanes GatewayFinalizer = "gateway-operator.konghq.com/cleanup-dataplanes"
 	// GatewayFinalizerCleanupControlPlanes is the finalizer to cleanup owned controlplane resources.
 	GatewayFinalizerCleanupControlPlanes GatewayFinalizer = "gateway-operator.konghq.com/cleanup-controlplanes"
-	// GatewayFinalizerCleanupNetworkpolicies is the finalizer to cleanup owned network policies.
-	GatewayFinalizerCleanupNetworkpolicies GatewayFinalizer = "gateway-operator.konghq.com/cleanup-network-policies"
+	// GatewayFinalizerCleanupNetworkPolicies is the finalizer to cleanup owned network policies.
+	GatewayFinalizerCleanupNetworkPolicies GatewayFinalizer = "gateway-operator.konghq.com/cleanup-network-policies"
 )
diff --git a/controller/gateway/controller_reconciler_utils.go b/controller/gateway/controller_reconciler_utils.go
@@ -27,13 +27,11 @@ import (
 	"github.com/kong/gateway-operator/controller/pkg/secrets/ref"
 	operatorerrors "github.com/kong/gateway-operator/internal/errors"
 	gwtypes "github.com/kong/gateway-operator/internal/types"
-	"github.com/kong/gateway-operator/internal/utils/gatewayclass"
 	"github.com/kong/gateway-operator/pkg/consts"
 	gatewayutils "github.com/kong/gateway-operator/pkg/utils/gateway"
 	k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes"
 	k8sreduce "github.com/kong/gateway-operator/pkg/utils/kubernetes/reduce"
 	k8sresources "github.com/kong/gateway-operator/pkg/utils/kubernetes/resources"
-	"github.com/kong/gateway-operator/pkg/vars"
 )
 
 // -----------------------------------------------------------------------------
@@ -193,23 +191,6 @@ func gatewayAddressesFromService(svc corev1.Service) ([]gwtypes.GatewayStatusAdd
 	return addresses, nil
 }
 
-func (r *Reconciler) verifyGatewayClassSupport(ctx context.Context, gateway *gwtypes.Gateway) (*gatewayclass.Decorator, error) {
-	if gateway.Spec.GatewayClassName == "" {
-		return nil, operatorerrors.ErrUnsupportedGateway
-	}
-
-	gwc := gatewayclass.NewDecorator()
-	if err := r.Client.Get(ctx, client.ObjectKey{Name: string(gateway.Spec.GatewayClassName)}, gwc.GatewayClass); err != nil {
-		return nil, err
-	}
-
-	if string(gwc.Spec.ControllerName) != vars.ControllerName() {
-		return nil, operatorerrors.ErrUnsupportedGateway
-	}
-
-	return gwc, nil
-}
-
 func (r *Reconciler) getOrCreateGatewayConfiguration(ctx context.Context, gatewayClass *gatewayv1.GatewayClass) (*operatorv1beta1.GatewayConfiguration, error) {
 	gatewayConfig, err := r.getGatewayConfigForGatewayClass(ctx, gatewayClass)
 	if err != nil {

diff --git a/controller/gateway/controller_test.go b/controller/gateway/controller_test.go
@@ -51,6 +51,88 @@ func TestGatewayReconciler_Reconcile(t *testing.T) {
 		controlplaneSubResources []controllerruntimeclient.Object
 		testBody                 func(t *testing.T, reconciler Reconciler, gatewayReq reconcile.Request)
 	}{
+		{
+			name: "gateway class not found - reconciliation should fail",
+			gatewayReq: reconcile.Request{
+				NamespacedName: types.NamespacedName{
+					Namespace: "test-namespace",
+					Name:      "test-gateway",
+				},
+			},
+			gateway: &gwtypes.Gateway{
+				TypeMeta: metav1.TypeMeta{
+					APIVersion: "gateway.networking.k8s.io/v1beta1",
+					Kind:       "Gateway",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-gateway",
+					Namespace: "test-namespace",
+					UID:       types.UID(uuid.NewString()),
+				},
+				Spec: gatewayv1.GatewaySpec{
+					GatewayClassName: "not-existing-gatewayclass",
+				},
+			},
+			testBody: func(t *testing.T, r Reconciler, gatewayReq reconcile.Request) {
+				ctx := context.Background()
+				_, err := r.Reconcile(ctx, gatewayReq)
+				require.Error(t, err)
+			},
+		},
+		{
+			name: "gateway class found, but controller name is not matching - gateway is ignored",
+			gatewayReq: reconcile.Request{
+				NamespacedName: types.NamespacedName{
+					Namespace: "test-namespace",
+					Name:      "test-gateway",
+				},
+			},
+			gateway: &gwtypes.Gateway{
+				TypeMeta: metav1.TypeMeta{
+					APIVersion: "gateway.networking.k8s.io/v1beta1",
+					Kind:       "Gateway",
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-gateway",
+					Namespace: "test-namespace",
+					UID:       types.UID(uuid.NewString()),
+				},
+				Spec: gatewayv1.GatewaySpec{
+					GatewayClassName: "test-gatewayclass",
+				},
+			},
+			gatewayClass: &gatewayv1.GatewayClass{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-gatewayclass",
+				},
+				Spec: gatewayv1.GatewayClassSpec{
+					ControllerName: gatewayv1.GatewayController("not-existing-controller"),
+				},
+				Status: gatewayv1.GatewayClassStatus{
+					Conditions: []metav1.Condition{
+						{
+							Type:               string(gatewayv1.GatewayClassConditionStatusAccepted),
+							Status:             metav1.ConditionTrue,
+							ObservedGeneration: 0,
+							LastTransitionTime: metav1.Now(),
+							Reason:             string(gatewayv1.GatewayClassReasonAccepted),
+							Message:            "the gatewayclass has been accepted by the controller",
+						},
+					},
+				},
+			},
+			testBody: func(t *testing.T, r Reconciler, gatewayReq reconcile.Request) {
+				ctx := context.Background()
+				res, err := r.Reconcile(ctx, gatewayReq)
+				require.NoError(t, err, "reconciliation should not return an error")
+				require.Equal(t, res, reconcile.Result{}, "reconciliation should not return a requeue")
+
+				var gw gwtypes.Gateway
+				require.NoError(t, r.Client.Get(ctx, gatewayReq.NamespacedName, &gw))
+
+				require.Empty(t, gw.GetFinalizers(), "gateway should not have any finalizers as it's ignored")
+			},
+		},
 		{
 			name: "service connectivity",
 			gatewayReq: reconcile.Request{
@@ -169,6 +251,16 @@ func TestGatewayReconciler_Reconcile(t *testing.T) {
 				// the dataplane service starts with no IP assigned, the gateway must be not ready
 				_, err := reconciler.Reconcile(ctx, gatewayReq)
 				require.NoError(t, err, "reconciliation returned an error")
+
+				t.Log("verifying the gateway gets finalizers assigned")
+				var gateway gwtypes.Gateway
+				require.NoError(t, reconciler.Client.Get(ctx, gatewayReq.NamespacedName, &gateway))
+				require.ElementsMatch(t, gateway.GetFinalizers(), []string{
+					string(GatewayFinalizerCleanupControlPlanes),
+					string(GatewayFinalizerCleanupDataPlanes),
+					string(GatewayFinalizerCleanupNetworkPolicies),
+				})
+
 				// need to trigger the Reconcile again because the first one only updated the finalizers
 				_, err = reconciler.Reconcile(ctx, gatewayReq)
 				require.NoError(t, err, "reconciliation returned an error")
@@ -301,17 +393,19 @@ func TestGatewayReconciler_Reconcile(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			ObjectsToAdd := []controllerruntimeclient.Object{
+			objectsToAdd := []controllerruntimeclient.Object{
 				tc.gateway,
-				tc.gatewayClass,
+			}
+			if tc.gatewayClass != nil {
+				objectsToAdd = append(objectsToAdd, tc.gatewayClass)
 			}
 			for _, gatewaySubResource := range tc.gatewaySubResources {
 				k8sutils.SetOwnerForObject(gatewaySubResource, tc.gateway)
 				gatewayutils.LabelObjectAsGatewayManaged(gatewaySubResource)
 				if gatewaySubResource.GetName() == "test-dataplane" {
 					for _, dataplaneSubresource := range tc.dataplaneSubResources {
 						k8sutils.SetOwnerForObject(dataplaneSubresource, gatewaySubResource)
-						ObjectsToAdd = append(ObjectsToAdd, dataplaneSubresource)
+						objectsToAdd = append(objectsToAdd, dataplaneSubresource)
 					}
 				}
 				if gatewaySubResource.GetName() == "test-controlplane" {
@@ -324,17 +418,17 @@ func TestGatewayReconciler_Reconcile(t *testing.T) {
 						})
 					for _, controlplaneSubResource := range tc.controlplaneSubResources {
 						k8sutils.SetOwnerForObject(controlplaneSubResource, gatewaySubResource)
-						ObjectsToAdd = append(ObjectsToAdd, controlplaneSubResource)
+						objectsToAdd = append(objectsToAdd, controlplaneSubResource)
 					}
 				}
-				ObjectsToAdd = append(ObjectsToAdd, gatewaySubResource)
+				objectsToAdd = append(objectsToAdd, gatewaySubResource)
 			}
 
 			fakeClient := fakectrlruntimeclient.
 				NewClientBuilder().
 				WithScheme(scheme.Scheme).
-				WithObjects(ObjectsToAdd...).
-				WithStatusSubresource(ObjectsToAdd...).
+				WithObjects(objectsToAdd...).
+				WithStatusSubresource(objectsToAdd...).
 				Build()
 
 			reconciler := Reconciler{

diff --git a/controller/gateway/controller_watch.go b/controller/gateway/controller_watch.go
@@ -17,9 +17,11 @@ import (
 
 	operatorv1beta1 "github.com/kong/gateway-operator/api/v1beta1"
 	"github.com/kong/gateway-operator/controller/pkg/controlplane"
+	"github.com/kong/gateway-operator/controller/pkg/log"
 	"github.com/kong/gateway-operator/controller/pkg/secrets/ref"
 	operatorerrors "github.com/kong/gateway-operator/internal/errors"
 	gwtypes "github.com/kong/gateway-operator/internal/types"
+	"github.com/kong/gateway-operator/internal/utils/gatewayclass"
 	"github.com/kong/gateway-operator/pkg/consts"
 	k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes"
 	"github.com/kong/gateway-operator/pkg/utils/kubernetes/resources"
@@ -41,13 +43,13 @@ func (r *Reconciler) gatewayHasMatchingGatewayClass(obj client.Object) bool {
 		return false
 	}
 
-	_, err := r.verifyGatewayClassSupport(context.Background(), gateway)
+	_, err := gatewayclass.Get(context.Background(), r.Client, string(gateway.Spec.GatewayClassName))
 	if err != nil {
 		// filtering here is just an optimization, the reconciler will check the
 		// class as well. If we fail here it's most likely because of some failure
 		// of the Kubernetes API and it's technically better to enqueue the object
 		// than to drop it for eventual consistency during cluster outages.
-		return !errors.Is(err, operatorerrors.ErrUnsupportedGateway)
+		return !errors.As(err, &operatorerrors.ErrUnsupportedGatewayClass{})
 	}
 
 	return true
@@ -226,23 +228,24 @@ func (r *Reconciler) listManagedGatewaysInNamespace(ctx context.Context, obj cli
 	for _, gateway := range gateways.Items {
 		objKey := client.ObjectKey{Name: string(gateway.Spec.GatewayClassName)}
 
-		var gatewaClass gatewayv1.GatewayClass
-		if err := r.Client.Get(ctx, objKey, &gatewaClass); err != nil {
-			logger.Error(
-				fmt.Errorf("failed to get GatewayClass"),
-				"failed to Get Gateway's GatewayClass",
-				"gatewayClass", objKey.Name, "gateway", gateway.Name, "namespace", gateway.Namespace,
-			)
+		if _, err := gatewayclass.Get(ctx, r.Client, string(gateway.Spec.GatewayClassName)); err != nil {
+			if errors.As(err, &operatorerrors.ErrUnsupportedGatewayClass{}) {
+				log.Debug(logger, "gateway class not supported, ignoring", gateway)
+			} else {
+				log.Error(logger, err, "failed to get Gateway's GatewayClass", gateway,
+					"gatewayClass", objKey.Name,
+					"gateway", gateway.Name,
+					"namespace", gateway.Namespace,
+				)
+			}
 			continue
 		}
-		if string(gatewaClass.Spec.ControllerName) == vars.ControllerName() {
-			recs = append(recs, reconcile.Request{
-				NamespacedName: types.NamespacedName{
-					Namespace: gateway.Namespace,
-					Name:      gateway.Name,
-				},
-			})
-		}
+		recs = append(recs, reconcile.Request{
+			NamespacedName: types.NamespacedName{
+				Namespace: gateway.Namespace,
+				Name:      gateway.Name,
+			},
+		})
 	}
 	return recs
 }

diff --git a/controller/specialized/aigateway_controller.go b/controller/specialized/aigateway_controller.go
@@ -17,6 +17,7 @@ import (
 	"github.com/kong/gateway-operator/controller/pkg/log"
 	"github.com/kong/gateway-operator/controller/pkg/watch"
 	operatorerrors "github.com/kong/gateway-operator/internal/errors"
+	"github.com/kong/gateway-operator/internal/utils/gatewayclass"
 	k8sutils "github.com/kong/gateway-operator/pkg/utils/kubernetes"
 	"github.com/kong/gateway-operator/pkg/vars"
 )
@@ -66,9 +67,9 @@ func (r *AIGatewayReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	// and this check must be done here to ensure consistency.
 	//
 	// See: https://github.com/kubernetes-sigs/controller-runtime/issues/1996
-	gwc, err := r.verifyGatewayClassSupport(ctx, &aigateway)
+	gwc, err := gatewayclass.Get(ctx, r.Client, aigateway.Spec.GatewayClassName)
 	if err != nil {
-		if errors.Is(err, operatorerrors.ErrUnsupportedGateway) {
+		if errors.As(err, &operatorerrors.ErrUnsupportedGatewayClass{}) {
 			log.Debug(logger, "resource not supported, ignoring", aigateway, "ExpectedGatewayClass", vars.ControllerName())
 			return ctrl.Result{}, nil
 		}

diff --git a/controller/specialized/aigateway_controller_reconciler_utils.go b/controller/specialized/aigateway_controller_reconciler_utils.go