From 9d4cfa1eb43d4382bb4929d2a11177d0df962aa7 Mon Sep 17 00:00:00 2001 From: Karol Szwaj Date: Sun, 18 Jun 2023 21:25:20 +0200 Subject: [PATCH] Add rbac role and process finalizers Signed-off-by: Karol Szwaj --- .../templates/generated/rbac/roles.yaml | 1 + internal/provider/kubernetes/controller.go | 51 +++++++-------- .../provider/kubernetes/controller_test.go | 5 +- .../provider/kubernetes/kubernetes_test.go | 6 -- internal/provider/kubernetes/rbac.go | 2 +- internal/utils/slice/slice.go | 29 +++++++++ internal/utils/slice/slice_test.go | 63 +++++++++++++++++++ 7 files changed, 123 insertions(+), 34 deletions(-) create mode 100644 internal/utils/slice/slice.go create mode 100644 internal/utils/slice/slice_test.go diff --git a/charts/gateway-helm/templates/generated/rbac/roles.yaml b/charts/gateway-helm/templates/generated/rbac/roles.yaml index ba7e19113108..fef92b017103 100644 --- a/charts/gateway-helm/templates/generated/rbac/roles.yaml +++ b/charts/gateway-helm/templates/generated/rbac/roles.yaml @@ -31,6 +31,7 @@ rules: verbs: - get - list + - patch - update - watch - apiGroups: diff --git a/internal/provider/kubernetes/controller.go b/internal/provider/kubernetes/controller.go index 2866cdc04fb0..9f1be183b34c 100644 --- a/internal/provider/kubernetes/controller.go +++ b/internal/provider/kubernetes/controller.go @@ -19,7 +19,6 @@ import ( "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -36,6 +35,7 @@ import ( "github.com/envoyproxy/gateway/internal/message" "github.com/envoyproxy/gateway/internal/provider/utils" "github.com/envoyproxy/gateway/internal/status" + "github.com/envoyproxy/gateway/internal/utils/slice" ) const ( @@ -154,7 +154,7 @@ func (r *gatewayAPIReconciler) Reconcile(ctx context.Context, request reconcile. // The gatewayclass was marked for deletion and the finalizer removed, // so clean-up dependents. if !gwClass.DeletionTimestamp.IsZero() && - !controllerutil.ContainsFinalizer(&gwClass, gatewayClassFinalizer) { + !slice.ContainsString(gwClass.Finalizers, gatewayClassFinalizer) { r.log.Info("gatewayclass marked for deletion") cc.removeMatch(&gwClass) @@ -249,7 +249,7 @@ func (r *gatewayAPIReconciler) Reconcile(ctx context.Context, request reconcile. } // Process the parametersRef of the accepted GatewayClass. - if acceptedGC.Spec.ParametersRef != nil && acceptedGC.DeletionTimestamp == nil { + if acceptedGC.Spec.ParametersRef != 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 { @@ -268,13 +268,6 @@ func (r *gatewayAPIReconciler) Reconcile(ctx context.Context, request reconcile. // 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 refsEnvoyProxy(acceptedGC) { - if err := r.removeFinalizer(ctx, resourceTree.EnvoyProxy); err != nil { - r.log.Error(err, fmt.Sprintf("failed to remove finalizer from envoy proxy %s", - resourceTree.EnvoyProxy.Name)) - return reconcile.Result{}, err - } - } // 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", @@ -282,13 +275,6 @@ func (r *gatewayAPIReconciler) Reconcile(ctx context.Context, request reconcile. return reconcile.Result{}, err } } else { - if refsEnvoyProxy(acceptedGC) { - if err := r.addFinalizer(ctx, resourceTree.EnvoyProxy); err != nil { - r.log.Error(err, fmt.Sprintf("failed adding finalizer to envoy proxy %s", - resourceTree.EnvoyProxy.Name)) - return reconcile.Result{}, err - } - } // finalize the accepted GatewayClass. if err := r.addFinalizer(ctx, acceptedGC); err != nil { r.log.Error(err, fmt.Sprintf("failed adding finalizer to gatewayclass %s", @@ -884,18 +870,18 @@ func secretGatewayIndexFunc(rawObj client.Object) []string { func (r *gatewayAPIReconciler) addFinalizer(ctx context.Context, obj client.Object) error { switch objType := obj.(type) { case *gwapiv1b1.GatewayClass: - if !controllerutil.ContainsFinalizer(objType, gatewayClassFinalizer) { + if !slice.ContainsString(objType.Finalizers, gatewayClassFinalizer) { base := client.MergeFrom(objType.DeepCopy()) - controllerutil.AddFinalizer(objType, gatewayClassFinalizer) + 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 !controllerutil.ContainsFinalizer(objType, gatewayClassFinalizer) { + if !slice.ContainsString(objType.Finalizers, gatewayClassFinalizer) { base := client.MergeFrom(objType.DeepCopy()) - controllerutil.AddFinalizer(objType, gatewayClassFinalizer) + 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) } @@ -910,18 +896,18 @@ func (r *gatewayAPIReconciler) addFinalizer(ctx context.Context, obj client.Obje func (r *gatewayAPIReconciler) removeFinalizer(ctx context.Context, obj client.Object) error { switch objType := obj.(type) { case *gwapiv1b1.GatewayClass: - if controllerutil.ContainsFinalizer(objType, gatewayClassFinalizer) { + if slice.ContainsString(objType.Finalizers, gatewayClassFinalizer) { base := client.MergeFrom(objType.DeepCopy()) - controllerutil.RemoveFinalizer(objType, gatewayClassFinalizer) + 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 controllerutil.ContainsFinalizer(objType, gatewayClassFinalizer) { + if slice.ContainsString(objType.Finalizers, gatewayClassFinalizer) { base := client.MergeFrom(objType.DeepCopy()) - controllerutil.RemoveFinalizer(objType, gatewayClassFinalizer) + 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) } @@ -1331,12 +1317,27 @@ func (r *gatewayAPIReconciler) processParamsRef(ctx context.Context, gc *gwapiv1 for i := range epList.Items { ep := epList.Items[i] r.log.Info("processing envoyproxy", "namespace", ep.Namespace, "name", ep.Name) + + 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 + } + if classRefsEnvoyProxy(gc, &ep) { found = true if err := (&ep).Validate(); err != nil { validationErr = fmt.Errorf("invalid envoyproxy: %v", err) continue } + 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 + } valid = true resourceTree.EnvoyProxy = &ep break diff --git a/internal/provider/kubernetes/controller_test.go b/internal/provider/kubernetes/controller_test.go index 833101be4cca..fab2b5ad7bf1 100644 --- a/internal/provider/kubernetes/controller_test.go +++ b/internal/provider/kubernetes/controller_test.go @@ -481,8 +481,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 bb1e411602b2..03aad9906036 100644 --- a/internal/provider/kubernetes/kubernetes_test.go +++ b/internal/provider/kubernetes/kubernetes_test.go @@ -195,12 +195,6 @@ func testGatewayClassWithParamRef(ctx context.Context, t *testing.T, provider *P return false }, defaultWait, defaultTick) - // Ensure the envoyproxy has been finalized. - require.Eventually(t, func() bool { - err := cli.Get(ctx, types.NamespacedName{Name: ep.Name}, ep) - return err == nil && slices.Contains(ep.Finalizers, gatewayClassFinalizer) - }, defaultWait, defaultTick) - // Ensure the resource map contains the EnvoyProxy. res, ok := resources.GatewayAPIResources.Load(gc.Name) assert.Equal(t, ok, true) diff --git a/internal/provider/kubernetes/rbac.go b/internal/provider/kubernetes/rbac.go index 3f55aeb91ab9..14d55f32744c 100644 --- a/internal/provider/kubernetes/rbac.go +++ b/internal/provider/kubernetes/rbac.go @@ -10,7 +10,7 @@ package kubernetes // +kubebuilder:rbac:groups="gateway.networking.k8s.io",resources=gatewayclasses/status;gateways/status;httproutes/status;grpcroutes/status;tlsroutes/status;tcproutes/status;udproutes/status,verbs=update // RBAC for Envoy Gateway custom resources. -// +kubebuilder:rbac:groups="config.gateway.envoyproxy.io",resources=envoyproxies,verbs=get;list;watch;update +// +kubebuilder:rbac:groups="config.gateway.envoyproxy.io",resources=envoyproxies,verbs=get;list;watch;update;patch // +kubebuilder:rbac:groups="gateway.envoyproxy.io",resources=authenticationfilters;ratelimitfilters,verbs=get;list;watch;update // RBAC for watched resources of Gateway API controllers. diff --git a/internal/utils/slice/slice.go b/internal/utils/slice/slice.go new file mode 100644 index 000000000000..cc75f9f0e597 --- /dev/null +++ b/internal/utils/slice/slice.go @@ -0,0 +1,29 @@ +// Copyright Envoy Gateway Authors +// SPDX-License-Identifier: Apache-2.0 +// The full text of the Apache license is available in the LICENSE file at +// the root of the repo. + +package slice + +// ContainsString checks if a given slice of strings contains the provided string. +func ContainsString(slice []string, s string) bool { + for _, item := range slice { + if item == s { + return true + } + } + return false +} + +// RemoveString returns a newly created []string that contains all items from slice that +// are not equal to s. +func RemoveString(slice []string, s string) []string { + var newSlice []string + for _, item := range slice { + if item == s { + continue + } + newSlice = append(newSlice, item) + } + return newSlice +} diff --git a/internal/utils/slice/slice_test.go b/internal/utils/slice/slice_test.go new file mode 100644 index 000000000000..8b76632e9728 --- /dev/null +++ b/internal/utils/slice/slice_test.go @@ -0,0 +1,63 @@ +// Copyright Envoy Gateway Authors +// SPDX-License-Identifier: Apache-2.0 +// The full text of the Apache license is available in the LICENSE file at +// the root of the repo. + +package slice + +import ( + "reflect" + "testing" +) + +func TestContainsString(t *testing.T) { + src := []string{"aa", "bb", "cc"} + + if !ContainsString(src, "bb") { + t.Errorf("ContainsString didn't find the string as expected") + } + + src = make([]string, 0) + if ContainsString(src, "") { + t.Errorf("The result returned is not the expected result") + } +} + +func TestRemoveString(t *testing.T) { + tests := []struct { + testName string + input []string + remove string + want []string + }{ + { + testName: "Nil input slice", + input: nil, + remove: "", + want: nil, + }, + { + testName: "Remove a string from input slice", + input: []string{"a", "ab", "cdef"}, + remove: "ab", + want: []string{"a", "cdef"}, + }, + { + testName: "Slice doesn't contain the string", + input: []string{"a", "ab", "cdef"}, + remove: "NotPresentInSlice", + want: []string{"a", "ab", "cdef"}, + }, + { + testName: "All strings removed, result is nil", + input: []string{"a"}, + remove: "a", + want: nil, + }, + } + for _, tt := range tests { + if got := RemoveString(tt.input, tt.remove); !reflect.DeepEqual(got, tt.want) { + t.Errorf("%v: RemoveString(%v, %q) = %v WANT %v", tt.testName, tt.input, tt.remove, got, tt.want) + } + } +}