fix: use Patch API for infra-client (envoyproxy#3034)

* fix(infrastructure): use Patch API instead Signed-off-by: Ardika Bagus <me@ardikabs.com> * chore: add interceptor for ApplyPatch on fake client Signed-off-by: Ardika Bagus <me@ardikabs.com> * chore: trigger make generate Signed-off-by: Ardika Bagus <me@ardikabs.com> * chore: remove update verb Signed-off-by: Ardika Bagus <me@ardikabs.com> * chore: SetUID no longer needed Signed-off-by: Ardika Bagus <me@ardikabs.com> --------- Signed-off-by: Ardika Bagus <me@ardikabs.com> (cherry picked from commit cc01bf5) Signed-off-by: Arko Dasgupta <arko@tetrate.io>
arkodg · Jun 12, 2024 · db87d44 · db87d44
1 parent a7a1370
commit db87d44
Show file tree

Hide file tree

Showing 8 changed files with 88 additions and 16 deletions.
diff --git a/charts/gateway-helm/templates/infra-manager-rbac.yaml b/charts/gateway-helm/templates/infra-manager-rbac.yaml
@@ -14,26 +14,26 @@ rules:
   verbs:
   - create
   - get
-  - update
   - delete
+  - patch
 - apiGroups:
   - apps
   resources:
   - deployments
   verbs:
   - create
   - get
-  - update
   - delete
+  - patch
 - apiGroups:
   - autoscaling
   resources:
   - horizontalpodautoscalers
   verbs:
   - create
   - get
-  - update
   - delete
+  - patch
 ---
 apiVersion: rbac.authorization.k8s.io/v1
 kind: RoleBinding

diff --git a/internal/infrastructure/kubernetes/infra_client.go b/internal/infrastructure/kubernetes/infra_client.go
@@ -38,8 +38,8 @@ func (cli *InfraClient) CreateOrUpdate(ctx context.Context, key client.ObjectKey
 			// Since the client.Object does not have a specific Spec field to compare
 			// just perform an update for now.
 			if updateChecker() {
-				specific.SetUID(current.GetUID())
-				if err := cli.Client.Update(ctx, specific); err != nil {
+				opts := []client.PatchOption{client.ForceOwnership, client.FieldOwner("envoy-gateway")}
+				if err := cli.Client.Patch(ctx, specific, client.Apply, opts...); err != nil {
 					return fmt.Errorf("for Update: %w", err)
 				}
 			}

diff --git a/internal/infrastructure/kubernetes/proxy_configmap_test.go b/internal/infrastructure/kubernetes/proxy_configmap_test.go
@@ -99,9 +99,16 @@ func TestCreateOrUpdateProxyConfigMap(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			var cli client.Client
 			if tc.current != nil {
-				cli = fakeclient.NewClientBuilder().WithScheme(envoygateway.GetScheme()).WithObjects(tc.current).Build()
+				cli = fakeclient.NewClientBuilder().
+					WithScheme(envoygateway.GetScheme()).
+					WithObjects(tc.current).
+					WithInterceptorFuncs(interceptorFunc).
+					Build()
 			} else {
-				cli = fakeclient.NewClientBuilder().WithScheme(envoygateway.GetScheme()).Build()
+				cli = fakeclient.NewClientBuilder().
+					WithScheme(envoygateway.GetScheme()).
+					WithInterceptorFuncs(interceptorFunc).
+					Build()
 			}
 			kube := NewInfra(cli, cfg)
 			r := proxy.NewResourceRender(kube.Namespace, infra.GetProxyInfra())

diff --git a/internal/infrastructure/kubernetes/proxy_deployment_test.go b/internal/infrastructure/kubernetes/proxy_deployment_test.go
@@ -106,9 +106,16 @@ func TestCreateOrUpdateProxyDeployment(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			var cli client.Client
 			if tc.current != nil {
-				cli = fakeclient.NewClientBuilder().WithScheme(envoygateway.GetScheme()).WithObjects(tc.current).Build()
+				cli = fakeclient.NewClientBuilder().
+					WithScheme(envoygateway.GetScheme()).
+					WithObjects(tc.current).
+					WithInterceptorFuncs(interceptorFunc).
+					Build()
 			} else {
-				cli = fakeclient.NewClientBuilder().WithScheme(envoygateway.GetScheme()).Build()
+				cli = fakeclient.NewClientBuilder().
+					WithScheme(envoygateway.GetScheme()).
+					WithInterceptorFuncs(interceptorFunc).
+					Build()
 			}
 
 			kube := NewInfra(cli, cfg)

diff --git a/internal/infrastructure/kubernetes/proxy_infra_test.go b/internal/infrastructure/kubernetes/proxy_infra_test.go
@@ -7,16 +7,21 @@ package kubernetes
 
 import (
 	"context"
+	"errors"
+	"fmt"
 	"reflect"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
+	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
 	gwapiv1 "sigs.k8s.io/gateway-api/apis/v1"
 
 	egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
@@ -28,10 +33,42 @@ import (
 )
 
 func newTestInfra(t *testing.T) *Infra {
-	cli := fakeclient.NewClientBuilder().WithScheme(envoygateway.GetScheme()).Build()
+	cli := fakeclient.NewClientBuilder().
+		WithScheme(envoygateway.GetScheme()).
+		WithInterceptorFuncs(interceptorFunc).
+		Build()
 	return newTestInfraWithClient(t, cli)
 }
 
+// Borrowing the interceptor from https://github.com/istio/istio/blob/2f54c6a52a5c6661d5eb9bd2277aab77304fee45/operator/pkg/helmreconciler/apply_test.go#L40
+// Interceptor is used for ApplyPatch as of this patch is not yet supported by the fake client, see https://github.com/kubernetes/kubernetes/issues/99953
+var interceptorFunc = interceptor.Funcs{Patch: func(
+	ctx context.Context,
+	clnt client.WithWatch,
+	obj client.Object,
+	patch client.Patch,
+	opts ...client.PatchOption,
+) error {
+	// Apply patches are supposed to upsert, but fake client fails if the object doesn't exist,
+	// if an apply patch occurs for an object that doesn't yet exist, create it.
+	if patch.Type() != types.ApplyPatchType {
+		return clnt.Patch(ctx, obj, patch, opts...)
+	}
+	check, ok := obj.DeepCopyObject().(client.Object)
+	if !ok {
+		return errors.New("could not check for object in fake client")
+	}
+	if err := clnt.Get(ctx, client.ObjectKeyFromObject(obj), check); kerrors.IsNotFound(err) {
+		if err := clnt.Create(ctx, check); err != nil {
+			return fmt.Errorf("could not inject object creation for fake: %w", err)
+		}
+	} else if err != nil {
+		return err
+	}
+	obj.SetResourceVersion(check.GetResourceVersion())
+	return clnt.Update(ctx, obj)
+}}
+
 func TestCmpBytes(t *testing.T) {
 	m1 := map[string][]byte{}
 	m1["a"] = []byte("aaa")

diff --git a/internal/infrastructure/kubernetes/proxy_serviceaccount_test.go b/internal/infrastructure/kubernetes/proxy_serviceaccount_test.go
@@ -174,9 +174,16 @@ func TestCreateOrUpdateProxyServiceAccount(t *testing.T) {
 
 			var cli client.Client
 			if tc.current != nil {
-				cli = fakeclient.NewClientBuilder().WithScheme(envoygateway.GetScheme()).WithObjects(tc.current).Build()
+				cli = fakeclient.NewClientBuilder().
+					WithScheme(envoygateway.GetScheme()).
+					WithObjects(tc.current).
+					WithInterceptorFuncs(interceptorFunc).
+					Build()
 			} else {
-				cli = fakeclient.NewClientBuilder().WithScheme(envoygateway.GetScheme()).Build()
+				cli = fakeclient.NewClientBuilder().
+					WithScheme(envoygateway.GetScheme()).
+					WithInterceptorFuncs(interceptorFunc).
+					Build()
 			}
 
 			kube := NewInfra(cli, cfg)

diff --git a/internal/infrastructure/kubernetes/ratelimit_deployment_test.go b/internal/infrastructure/kubernetes/ratelimit_deployment_test.go
@@ -68,9 +68,16 @@ func TestCreateOrUpdateRateLimitDeployment(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			var cli client.Client
 			if tc.current != nil {
-				cli = fakeclient.NewClientBuilder().WithScheme(envoygateway.GetScheme()).WithObjects(tc.current).Build()
+				cli = fakeclient.NewClientBuilder().
+					WithScheme(envoygateway.GetScheme()).
+					WithObjects(tc.current).
+					WithInterceptorFuncs(interceptorFunc).
+					Build()
 			} else {
-				cli = fakeclient.NewClientBuilder().WithScheme(envoygateway.GetScheme()).Build()
+				cli = fakeclient.NewClientBuilder().
+					WithScheme(envoygateway.GetScheme()).
+					WithInterceptorFuncs(interceptorFunc).
+					Build()
 			}
 
 			kube := NewInfra(cli, cfg)

diff --git a/internal/infrastructure/kubernetes/ratelimit_serviceaccount_test.go b/internal/infrastructure/kubernetes/ratelimit_serviceaccount_test.go
@@ -92,9 +92,16 @@ func TestCreateOrUpdateRateLimitServiceAccount(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			var cli client.Client
 			if tc.current != nil {
-				cli = fakeclient.NewClientBuilder().WithScheme(envoygateway.GetScheme()).WithObjects(tc.current).Build()
+				cli = fakeclient.NewClientBuilder().
+					WithScheme(envoygateway.GetScheme()).
+					WithObjects(tc.current).
+					WithInterceptorFuncs(interceptorFunc).
+					Build()
 			} else {
-				cli = fakeclient.NewClientBuilder().WithScheme(envoygateway.GetScheme()).Build()
+				cli = fakeclient.NewClientBuilder().
+					WithScheme(envoygateway.GetScheme()).
+					WithInterceptorFuncs(interceptorFunc).
+					Build()
 			}
 
 			cfg, err := config.New()