provider: finalize EnvoyProxy referenced by a managed GatewayClass (#…

…1534) * Add/remove finalizers for envoy proxies Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * Refactor finalizers funcs Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * add finalizer check to the tests Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * Add rbac role and process finalizers Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * improve logging Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * Update test case remove finalizer Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * Rework processing ParamsRef Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * Remove roles.yaml Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * Add patch to rbac.tpl Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * Add integration test for envoyproxy finalizers Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> * Restore envoyproxy status checks Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com> --------- Signed-off-by: Karol Szwaj <karol.szwaj@gmail.com>
envoyproxy · Sep 12, 2023 · e158ef1 · e158ef1
1 parent 4e5b1ce
commit e158ef1
Show file tree

Hide file tree

Showing 4 changed files with 205 additions and 30 deletions.
diff --git a/charts/gateway-helm/templates/_rbac.tpl b/charts/gateway-helm/templates/_rbac.tpl
@@ -70,6 +70,7 @@ verbs:
 - list
 - update
 - watch
+- patch
 {{- end }}
 
 {{- define "eg.rbac.namespaced.gateway.envoyproxy" -}}

diff --git a/internal/provider/kubernetes/controller.go b/internal/provider/kubernetes/controller.go
@@ -298,15 +298,13 @@ func (r *gatewayAPIReconciler) Reconcile(ctx context.Context, _ reconcile.Reques
 	}
 
 	// Process the parametersRef of the accepted GatewayClass.
-	if acceptedGC.Spec.ParametersRef != nil && acceptedGC.DeletionTimestamp == nil {
-		if err := r.processParamsRef(ctx, acceptedGC, resourceTree); err != nil {
-			msg := fmt.Sprintf("%s: %v", status.MsgGatewayClassInvalidParams, err)
-			if err := r.gatewayClassUpdater(ctx, acceptedGC, false, string(gwapiv1b1.GatewayClassReasonInvalidParameters), msg); err != nil {
-				r.log.Error(err, "unable to update GatewayClass status")
-			}
-			r.log.Error(err, "failed to process parametersRef for gatewayclass", "name", acceptedGC.Name)
-			return reconcile.Result{}, err
+	if err := r.processParamsRef(ctx, acceptedGC, resourceTree); err != nil {
+		msg := fmt.Sprintf("%s: %v", status.MsgGatewayClassInvalidParams, err)
+		if err := r.gatewayClassUpdater(ctx, acceptedGC, false, string(gwapiv1b1.GatewayClassReasonInvalidParameters), msg); err != nil {
+			r.log.Error(err, "unable to update GatewayClass status")
 		}
+		r.log.Error(err, "failed to process parametersRef for gatewayclass", "name", acceptedGC.Name)
+		return reconcile.Result{}, err
 	}
 
 	if err := r.gatewayClassUpdater(ctx, acceptedGC, true, string(gwapiv1b1.GatewayClassReasonAccepted), status.MsgValidGatewayClass); err != nil {
@@ -317,7 +315,6 @@ func (r *gatewayAPIReconciler) Reconcile(ctx context.Context, _ reconcile.Reques
 	// Update finalizer on the gateway class based on the resource tree.
 	if len(resourceTree.Gateways) == 0 {
 		r.log.Info("No gateways found for accepted gatewayclass")
-
 		// If needed, remove the finalizer from the accepted GatewayClass.
 		if err := r.removeFinalizer(ctx, acceptedGC); err != nil {
 			r.log.Error(err, fmt.Sprintf("failed to remove finalizer from gatewayclass %s",
@@ -957,28 +954,56 @@ func secretGatewayIndexFunc(rawObj client.Object) []string {
 	return secretReferences
 }
 
-// removeFinalizer removes the gatewayclass finalizer from the provided gc, if it exists.
-func (r *gatewayAPIReconciler) removeFinalizer(ctx context.Context, gc *gwapiv1b1.GatewayClass) error {
-	if slice.ContainsString(gc.Finalizers, gatewayClassFinalizer) {
-		base := client.MergeFrom(gc.DeepCopy())
-		gc.Finalizers = slice.RemoveString(gc.Finalizers, gatewayClassFinalizer)
-		if err := r.client.Patch(ctx, gc, base); err != nil {
-			return fmt.Errorf("failed to remove finalizer from gatewayclass %s: %w", gc.Name, err)
+// addGcFinalizer adds the gatewayclass or envoyproxy finalizer to the provided object, if it doesn't exist.
+func (r *gatewayAPIReconciler) addFinalizer(ctx context.Context, obj client.Object) error {
+	switch objType := obj.(type) {
+	case *gwapiv1b1.GatewayClass:
+		if !slice.ContainsString(objType.Finalizers, gatewayClassFinalizer) {
+			base := client.MergeFrom(objType.DeepCopy())
+			objType.Finalizers = append(objType.Finalizers, gatewayClassFinalizer)
+			if err := r.client.Patch(ctx, objType, base); err != nil {
+				return fmt.Errorf("failed to add finalizer to Gateway Class %s: %w", objType.Name, err)
+			}
+		}
+		return nil
+	case *egcfgv1a1.EnvoyProxy:
+		if !slice.ContainsString(objType.Finalizers, gatewayClassFinalizer) {
+			base := client.MergeFrom(objType.DeepCopy())
+			objType.Finalizers = append(objType.Finalizers, gatewayClassFinalizer)
+			if err := r.client.Patch(ctx, objType, base); err != nil {
+				return fmt.Errorf("failed to add finalizer to Envoy Proxy %s: %w", objType.Name, err)
+			}
 		}
+		return nil
 	}
-	return nil
+
+	return fmt.Errorf("%s is not supported for finalizer processing", obj.GetObjectKind().GroupVersionKind().Kind)
 }
 
-// addFinalizer adds the gatewayclass finalizer to the provided gc, if it doesn't exist.
-func (r *gatewayAPIReconciler) addFinalizer(ctx context.Context, gc *gwapiv1b1.GatewayClass) error {
-	if !slice.ContainsString(gc.Finalizers, gatewayClassFinalizer) {
-		base := client.MergeFrom(gc.DeepCopy())
-		gc.Finalizers = append(gc.Finalizers, gatewayClassFinalizer)
-		if err := r.client.Patch(ctx, gc, base); err != nil {
-			return fmt.Errorf("failed to add finalizer to gatewayclass %s: %w", gc.Name, err)
+// removeFinalizer removes the gatewayclass or envoyproxy finalizer from the provided object, if it exists.
+func (r *gatewayAPIReconciler) removeFinalizer(ctx context.Context, obj client.Object) error {
+	switch objType := obj.(type) {
+	case *gwapiv1b1.GatewayClass:
+		if slice.ContainsString(objType.Finalizers, gatewayClassFinalizer) {
+			base := client.MergeFrom(objType.DeepCopy())
+			objType.Finalizers = slice.RemoveString(objType.Finalizers, gatewayClassFinalizer)
+			if err := r.client.Patch(ctx, objType, base); err != nil {
+				return fmt.Errorf("failed to add finalizer to Gateway Class %s: %w", objType.Name, err)
+			}
+		}
+		return nil
+	case *egcfgv1a1.EnvoyProxy:
+		if slice.ContainsString(objType.Finalizers, gatewayClassFinalizer) {
+			base := client.MergeFrom(objType.DeepCopy())
+			objType.Finalizers = slice.RemoveString(objType.Finalizers, gatewayClassFinalizer)
+			if err := r.client.Patch(ctx, objType, base); err != nil {
+				return fmt.Errorf("failed to add finalizer to Envoy Proxy %s: %w", objType.Name, err)
+			}
 		}
+		return nil
 	}
-	return nil
+
+	return fmt.Errorf("%s is not supported for finalizer processing", obj.GetObjectKind().GroupVersionKind().Kind)
 }
 
 // subscribeAndUpdateStatus subscribes to gateway API object status updates and
@@ -1417,10 +1442,11 @@ func (r *gatewayAPIReconciler) hasManagedClass(obj client.Object) bool {
 
 // processParamsRef processes the parametersRef of the provided GatewayClass.
 func (r *gatewayAPIReconciler) processParamsRef(ctx context.Context, gc *gwapiv1b1.GatewayClass, resourceTree *gatewayapi.Resources) error {
-	if !refsEnvoyProxy(gc) {
-		return fmt.Errorf("unsupported parametersRef for gatewayclass %s", gc.Name)
+	if gc.Spec.ParametersRef != nil {
+		if !refsEnvoyProxy(gc) {
+			return fmt.Errorf("unsupported parametersRef for gatewayclass %s", gc.Name)
+		}
 	}
-
 	epList := new(egcfgv1a1.EnvoyProxyList)
 	// The EnvoyProxy must be in the same namespace as EG.
 	if err := r.client.List(ctx, epList, &client.ListOptions{Namespace: r.namespace}); err != nil {
@@ -1439,15 +1465,40 @@ func (r *gatewayAPIReconciler) processParamsRef(ctx context.Context, gc *gwapiv1
 		ep := epList.Items[i]
 		r.log.Info("processing envoyproxy", "namespace", ep.Namespace, "name", ep.Name)
 		if classRefsEnvoyProxy(gc, &ep) {
+			if !gc.DeletionTimestamp.IsZero() {
+				if err := r.removeFinalizer(ctx, &ep); err != nil {
+					r.log.Error(err, fmt.Sprintf("failed to remove finalizer from envoy proxy %s",
+						ep.Name))
+					return err
+				}
+				return nil
+			}
 			found = true
 			if err := validation.ValidateEnvoyProxy(&ep); err != nil {
 				validationErr = fmt.Errorf("invalid envoyproxy: %v", err)
 				continue
 			}
 			valid = true
+
+			if err := r.addFinalizer(ctx, &ep); err != nil {
+				r.log.Error(err, fmt.Sprintf("failed adding finalizer to envoy proxy %s",
+					ep.Name))
+				return err
+			}
+
 			resourceTree.EnvoyProxy = &ep
 			break
 		}
+
+		// Remove finalizer from EnvoyProxy when GatewayClass stops referencing it
+		if slice.ContainsString(ep.Finalizers, gatewayClassFinalizer) {
+			if err := r.removeFinalizer(ctx, &ep); err != nil {
+				r.log.Error(err, fmt.Sprintf("failed to remove finalizer from envoy proxy %s",
+					ep.Name))
+				return err
+			}
+			return nil
+		}
 	}
 
 	if !found {

diff --git a/internal/provider/kubernetes/controller_test.go b/internal/provider/kubernetes/controller_test.go
@@ -87,6 +87,114 @@ func TestAddGatewayClassFinalizer(t *testing.T) {
 	}
 }
 
+func TestAddEnvoyProxyFinalizer(t *testing.T) {
+	testCases := []struct {
+		name   string
+		ep     *egcfgv1a1.EnvoyProxy
+		expect []string
+	}{
+		{
+			name: "envoyproxy with no finalizers",
+			ep: &egcfgv1a1.EnvoyProxy{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+			},
+			expect: []string{gatewayClassFinalizer},
+		},
+		{
+			name: "envoyproxy with a different finalizer",
+			ep: &egcfgv1a1.EnvoyProxy{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "test-ep",
+					Finalizers: []string{"fooFinalizer"},
+				},
+			},
+			expect: []string{"fooFinalizer", gatewayClassFinalizer},
+		},
+		{
+			name: "envoyproxy with existing gatewayclass finalizer",
+			ep: &egcfgv1a1.EnvoyProxy{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "test-ep",
+					Finalizers: []string{gatewayClassFinalizer},
+				},
+			},
+			expect: []string{gatewayClassFinalizer},
+		},
+	}
+	// Create the reconciler.
+	r := new(gatewayAPIReconciler)
+	ctx := context.Background()
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			r.client = fakeclient.NewClientBuilder().WithScheme(envoygateway.GetScheme()).WithObjects(tc.ep).Build()
+			err := r.addFinalizer(ctx, tc.ep)
+			require.NoError(t, err)
+			key := types.NamespacedName{Name: tc.ep.Name}
+			err = r.client.Get(ctx, key, tc.ep)
+			require.NoError(t, err)
+			require.Equal(t, tc.expect, tc.ep.Finalizers)
+		})
+	}
+}
+
+func TestRemoveEnvoyProxyFinalizer(t *testing.T) {
+	testCases := []struct {
+		name   string
+		ep     *egcfgv1a1.EnvoyProxy
+		expect []string
+	}{
+		{
+			name: "envoyproxy with no finalizers",
+			ep: &egcfgv1a1.EnvoyProxy{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+			},
+			expect: nil,
+		},
+		{
+			name: "envoyproxy with a different finalizer",
+			ep: &egcfgv1a1.EnvoyProxy{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "test-ep",
+					Finalizers: []string{"fooFinalizer"},
+				},
+			},
+			expect: []string{"fooFinalizer"},
+		},
+		{
+			name: "envoyproxy with existing gatewayclass finalizer",
+			ep: &egcfgv1a1.EnvoyProxy{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "test-ep",
+					Finalizers: []string{gatewayClassFinalizer},
+				},
+			},
+			expect: nil,
+		},
+	}
+
+	// Create the reconciler.
+	r := new(gatewayAPIReconciler)
+	ctx := context.Background()
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			r.client = fakeclient.NewClientBuilder().WithScheme(envoygateway.GetScheme()).WithObjects(tc.ep).Build()
+			err := r.removeFinalizer(ctx, tc.ep)
+			require.NoError(t, err)
+			key := types.NamespacedName{Name: tc.ep.Name}
+			err = r.client.Get(ctx, key, tc.ep)
+			require.NoError(t, err)
+			require.Equal(t, tc.expect, tc.ep.Finalizers)
+		})
+	}
+}
 func TestRemoveGatewayClassFinalizer(t *testing.T) {
 	testCases := []struct {
 		name   string
@@ -363,8 +471,9 @@ func TestProcessParamsRef(t *testing.T) {
 			},
 			ep: &egcfgv1a1.EnvoyProxy{
 				ObjectMeta: metav1.ObjectMeta{
-					Namespace: config.DefaultNamespace,
-					Name:      "test",
+					Namespace:  config.DefaultNamespace,
+					Name:       "test",
+					Finalizers: []string{gatewayClassFinalizer},
 				},
 			},
 			expected: true,

diff --git a/internal/provider/kubernetes/kubernetes_test.go b/internal/provider/kubernetes/kubernetes_test.go
@@ -202,6 +202,20 @@ func testGatewayClassWithParamRef(ctx context.Context, t *testing.T, provider *P
 	res, ok := resources.GatewayAPIResources.Load(gc.Name)
 	assert.Equal(t, ok, true)
 	assert.Equal(t, res.EnvoyProxy.Spec, ep.Spec)
+
+	// Ensure the envoyproxy has been finalized.
+	require.Eventually(t, func() bool {
+		err := cli.Get(ctx, types.NamespacedName{Name: ep.Name, Namespace: testNs}, ep)
+		return err == nil && slices.Contains(ep.Finalizers, gatewayClassFinalizer)
+	}, defaultWait, defaultTick)
+
+	//Ensure that envoyproxy finalizer will be removed when GatewayClass stops referencing it
+	gc.Spec.ParametersRef = nil
+	require.NoError(t, cli.Update(ctx, gc))
+	require.Eventually(t, func() bool {
+		err := cli.Get(ctx, types.NamespacedName{Name: ep.Name, Namespace: testNs}, ep)
+		return err == nil && !slices.Contains(ep.Finalizers, gatewayClassFinalizer)
+	}, defaultWait, defaultTick)
 }
 
 func testGatewayScheduledStatus(ctx context.Context, t *testing.T, provider *Provider, resources *message.ProviderResources) {
-Original file line number
+Diff line change
@@ Expand Up / @@ -70,6 +70,7 @@ verbs: @@
     - list
     - update
     - watch
+    - patch
     {{- end }}
     {{- define "eg.rbac.namespaced.gateway.envoyproxy" -}}
@@ Expand Down @@