From e158ef139a7a038c97e95666d3a2d7bd08f3f998 Mon Sep 17 00:00:00 2001 From: Karol Szwaj Date: Wed, 13 Sep 2023 00:12:55 +0200 Subject: [PATCH] provider: finalize EnvoyProxy referenced by a managed GatewayClass (#1534) * Add/remove finalizers for envoy proxies Signed-off-by: Karol Szwaj * Refactor finalizers funcs Signed-off-by: Karol Szwaj * add finalizer check to the tests Signed-off-by: Karol Szwaj * Add rbac role and process finalizers Signed-off-by: Karol Szwaj * improve logging Signed-off-by: Karol Szwaj * Update test case remove finalizer Signed-off-by: Karol Szwaj * Rework processing ParamsRef Signed-off-by: Karol Szwaj * Remove roles.yaml Signed-off-by: Karol Szwaj * Add patch to rbac.tpl Signed-off-by: Karol Szwaj * Add integration test for envoyproxy finalizers Signed-off-by: Karol Szwaj * Restore envoyproxy status checks Signed-off-by: Karol Szwaj --------- Signed-off-by: Karol Szwaj --- charts/gateway-helm/templates/_rbac.tpl | 1 + internal/provider/kubernetes/controller.go | 107 ++++++++++++----- .../provider/kubernetes/controller_test.go | 113 +++++++++++++++++- .../provider/kubernetes/kubernetes_test.go | 14 +++ 4 files changed, 205 insertions(+), 30 deletions(-) diff --git a/charts/gateway-helm/templates/_rbac.tpl b/charts/gateway-helm/templates/_rbac.tpl index a9384d50ba8..d81033486ad 100644 --- 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 index 5623255420e..8b35930fe2a 100644 --- 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 index 03957a26998..1269a5b84a3 100644 --- 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 index a13038b0e4e..d05e823339e 100644 --- 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) {