From 54ce6fb2db8648994e14d5c72e4444ccaaf0f077 Mon Sep 17 00:00:00 2001 From: Arko Dasgupta Date: Wed, 18 Oct 2023 16:55:55 -0700 Subject: [PATCH] Revert "provider: finalize EnvoyProxy referenced by a managed GatewayClass(#1534) (#2000) Revert "provider: finalize EnvoyProxy referenced by a managed GatewayClass (#1534)" This reverts commit e158ef139a7a038c97e95666d3a2d7bd08f3f998. Signed-off-by: Arko Dasgupta --- internal/provider/kubernetes/controller.go | 107 +++++------------ .../provider/kubernetes/controller_test.go | 113 +----------------- .../provider/kubernetes/kubernetes_test.go | 14 --- 3 files changed, 30 insertions(+), 204 deletions(-) diff --git a/internal/provider/kubernetes/controller.go b/internal/provider/kubernetes/controller.go index dc7f1b3e6d1..801d4d1d247 100644 --- a/internal/provider/kubernetes/controller.go +++ b/internal/provider/kubernetes/controller.go @@ -322,13 +322,15 @@ func (r *gatewayAPIReconciler) Reconcile(ctx context.Context, _ reconcile.Reques } // Process the parametersRef of the accepted GatewayClass. - 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") + 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 } - 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 { @@ -339,6 +341,7 @@ 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", @@ -1015,56 +1018,28 @@ func secretGatewayIndexFunc(rawObj client.Object) []string { return secretReferences } -// 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 *egv1a1.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) - } +// 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) } - return nil } - - return fmt.Errorf("%s is not supported for finalizer processing", obj.GetObjectKind().GroupVersionKind().Kind) + return nil } -// 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 *egv1a1.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) - } +// 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) } - return nil } - - return fmt.Errorf("%s is not supported for finalizer processing", obj.GetObjectKind().GroupVersionKind().Kind) + return nil } // subscribeAndUpdateStatus subscribes to gateway API object status updates and @@ -1629,12 +1604,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 gc.Spec.ParametersRef != nil { - if !refsEnvoyProxy(gc) { - return fmt.Errorf("unsupported parametersRef for gatewayclass %s", gc.Name) - } + if !refsEnvoyProxy(gc) { + return fmt.Errorf("unsupported parametersRef for gatewayclass %s", gc.Name) } epList := new(egv1a1.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 { return fmt.Errorf("failed to list envoyproxies in namespace %s: %v", r.namespace, err) @@ -1652,40 +1626,15 @@ 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 d6374c96645..c9fa986b159 100644 --- a/internal/provider/kubernetes/controller_test.go +++ b/internal/provider/kubernetes/controller_test.go @@ -87,114 +87,6 @@ func TestAddGatewayClassFinalizer(t *testing.T) { } } -func TestAddEnvoyProxyFinalizer(t *testing.T) { - testCases := []struct { - name string - ep *egv1a1.EnvoyProxy - expect []string - }{ - { - name: "envoyproxy with no finalizers", - ep: &egv1a1.EnvoyProxy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - }, - }, - expect: []string{gatewayClassFinalizer}, - }, - { - name: "envoyproxy with a different finalizer", - ep: &egv1a1.EnvoyProxy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-ep", - Finalizers: []string{"fooFinalizer"}, - }, - }, - expect: []string{"fooFinalizer", gatewayClassFinalizer}, - }, - { - name: "envoyproxy with existing gatewayclass finalizer", - ep: &egv1a1.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 *egv1a1.EnvoyProxy - expect []string - }{ - { - name: "envoyproxy with no finalizers", - ep: &egv1a1.EnvoyProxy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test", - }, - }, - expect: nil, - }, - { - name: "envoyproxy with a different finalizer", - ep: &egv1a1.EnvoyProxy{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-ep", - Finalizers: []string{"fooFinalizer"}, - }, - }, - expect: []string{"fooFinalizer"}, - }, - { - name: "envoyproxy with existing gatewayclass finalizer", - ep: &egv1a1.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 @@ -471,9 +363,8 @@ func TestProcessParamsRef(t *testing.T) { }, ep: &egv1a1.EnvoyProxy{ ObjectMeta: metav1.ObjectMeta{ - Namespace: config.DefaultNamespace, - Name: "test", - Finalizers: []string{gatewayClassFinalizer}, + Namespace: config.DefaultNamespace, + Name: "test", }, }, expected: true, diff --git a/internal/provider/kubernetes/kubernetes_test.go b/internal/provider/kubernetes/kubernetes_test.go index 7158fd477cf..dcd272c22d0 100644 --- a/internal/provider/kubernetes/kubernetes_test.go +++ b/internal/provider/kubernetes/kubernetes_test.go @@ -201,20 +201,6 @@ 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) {