fix: use client.Client for deleting objects in cleaner (#1373)

Author: pmalek

pkg/clusters/cleanup.go

@@ -3,20 +3,15 @@ package clusters
 import (
 	"context"
 	"fmt"
-	"strings"
 	"sync"
 
 	"golang.org/x/sync/errgroup"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/watch"
-	"k8s.io/client-go/dynamic"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	gatewayv1 "sigs.k8s.io/gateway-api/apis/v1"
-	gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
-	gatewayv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"
 )
 
 // -----------------------------------------------------------------------------
@@ -27,15 +22,19 @@ import (
 // used during integration tests to clean up test resources.
 type Cleaner struct {
 	cluster    Cluster
+	scheme     *runtime.Scheme
 	objects    []client.Object
 	manifests  []string
 	namespaces []*corev1.Namespace
 	lock       sync.RWMutex
 }
 
 // NewCleaner provides a new initialized *Cleaner object.
-func NewCleaner(cluster Cluster) *Cleaner {
-	return &Cleaner{cluster: cluster}
+func NewCleaner(cluster Cluster, scheme *runtime.Scheme) *Cleaner {
+	return &Cleaner{
+		cluster: cluster,
+		scheme:  scheme,
+	}
 }
 
 // -----------------------------------------------------------------------------
@@ -63,15 +62,16 @@ func (c *Cleaner) AddNamespace(namespace *corev1.Namespace) {
 func (c *Cleaner) Cleanup(ctx context.Context) error {
 	c.lock.RLock()
 	defer c.lock.RUnlock()
-	dyn, err := dynamic.NewForConfig(c.cluster.Config())
+
+	cl, err := client.New(c.cluster.Config(), client.Options{
+		Scheme: c.scheme,
+	})
 	if err != nil {
 		return err
 	}
 
 	for _, obj := range c.objects {
-		resource := resourceDeleterForObj(dyn, obj)
-
-		if err := resource.Delete(ctx, obj.GetName(), metav1.DeleteOptions{}); err != nil {
+		if err := cl.Delete(ctx, obj); err != nil {
 			if !errors.IsNotFound(err) {
 				return err
 			}
@@ -128,100 +128,6 @@ func (c *Cleaner) Cleanup(ctx context.Context) error {
 	return g.Wait()
 }
 
-// fixupObjKinds takes a client.Object and checks if it's of one of the gateway
-// API types and if so then it adjusts that object's Kind and APIVersion.
-// This possibly might also need other types to be included but those are enough
-// for our needs for now especially since that will help cleaning up non-namespaced
-// GatewayClasses which are not cleaned up on namespace removal also done in
-// Cleanup().
-//
-// The reason we need this is that when decoding to go structs APIVersion and Kind
-// are dropper because the type info is inherent in the object.
-// Decoding to unstructured objects (like the dynamic client does) preserves that
-// information.
-// There should be a better way of doing this.
-//
-// Possibly related:
-// - https://github.com/kubernetes/kubernetes/issues/3030
-// - https://github.com/kubernetes/kubernetes/issues/80609
-func fixupObjKinds(obj client.Object) client.Object {
-	// If Kind and APIVersion are set then we're good.
-	if obj.GetObjectKind().GroupVersionKind().Kind != "" && obj.GetResourceVersion() != "" {
-		return obj
-	}
-
-	// Otherwise try to fix that up by performing type assertions and filling
-	// those 2 fields accordingly.
-	switch o := obj.(type) {
-	case *gatewayv1.GatewayClass:
-		o.Kind = "GatewayClass"
-		o.APIVersion = gatewayv1.GroupVersion.String()
-		return o
-	case *gatewayv1.Gateway:
-		o.Kind = "Gateway"
-		o.APIVersion = gatewayv1.GroupVersion.String()
-		return o
-	case *gatewayv1.HTTPRoute:
-		o.Kind = "HTTPRoute"
-		o.APIVersion = gatewayv1.GroupVersion.String()
-		return o
-
-	case *gatewayv1alpha2.TCPRoute:
-		o.Kind = "TCPRoute"
-		o.APIVersion = gatewayv1alpha2.GroupVersion.String()
-		return o
-	case *gatewayv1alpha2.UDPRoute:
-		o.Kind = "UDPRoute"
-		o.APIVersion = gatewayv1alpha2.GroupVersion.String()
-		return o
-	case *gatewayv1alpha2.TLSRoute:
-		o.Kind = "TLSRoute"
-		o.APIVersion = gatewayv1alpha2.GroupVersion.String()
-		return o
-	case *gatewayv1beta1.ReferenceGrant:
-		o.Kind = "ReferenceGrant"
-		o.APIVersion = gatewayv1beta1.GroupVersion.String()
-		return o
-
-	default:
-		return obj
-	}
-}
-
-type deleter interface {
-	Delete(ctx context.Context, name string, options metav1.DeleteOptions, subresources ...string) error
-}
-
-func resourceDeleterForObj(dyn *dynamic.DynamicClient, obj client.Object) deleter {
-	obj = fixupObjKinds(obj)
-
-	var (
-		namespace = obj.GetNamespace()
-		kind      = obj.GetObjectKind()
-		gvk       = kind.GroupVersionKind()
-	)
-
-	var gvr schema.GroupVersionResource
-	switch gvk.Kind {
-	// GatewayClass is a special case because gatewayclass + "s" is not a plural
-	// of gatewayclass.
-	case "GatewayClass":
-		gvr = schema.GroupVersionResource{
-			Group:    gvk.Group,
-			Version:  gvk.Version,
-			Resource: "gatewayclasses",
-		}
-	default:
-		res := strings.ToLower(gvk.Kind) + "s"
-		gvr = gvk.GroupVersion().WithResource(res)
-	}
-
-	if namespace == "" {
-		return dyn.Resource(gvr)
-	}
-	return dyn.Resource(gvr).Namespace(namespace)
-}
-
 // DumpDiagnostics dumps diagnostics from the underlying cluster.
 //
 // Deprecated: Users should use Cluster.DumpDiagnostics().

pkg/clusters/cleanup_test.go

@@ -4,128 +4,14 @@ import (
 	"fmt"
 	"testing"
 
-	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime/schema"
-	"sigs.k8s.io/controller-runtime/pkg/client"
-	gatewayv1 "sigs.k8s.io/gateway-api/apis/v1"
-	gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
-	gatewayv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"
+	"k8s.io/apimachinery/pkg/runtime"
 )
 
-func TestFixupObjKinds(t *testing.T) {
-	testcases := []struct {
-		name     string
-		obj      client.Object
-		expected schema.GroupVersionKind
-	}{
-		{
-			name: "gatewayclass",
-			obj: &gatewayv1.GatewayClass{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "my-gatewayclass",
-				},
-			},
-			expected: schema.GroupVersionKind{
-				Group:   "gateway.networking.k8s.io",
-				Version: "v1",
-				Kind:    "GatewayClass",
-			},
-		},
-		{
-			name: "gateway",
-			obj: &gatewayv1.Gateway{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "my-gateway",
-				},
-			},
-			expected: schema.GroupVersionKind{
-				Group:   "gateway.networking.k8s.io",
-				Version: "v1",
-				Kind:    "Gateway",
-			},
-		},
-		{
-			name: "httproute",
-			obj: &gatewayv1.HTTPRoute{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "my-httproute",
-				},
-			},
-			expected: schema.GroupVersionKind{
-				Group:   "gateway.networking.k8s.io",
-				Version: "v1",
-				Kind:    "HTTPRoute",
-			},
-		},
-		{
-			name: "tcproute",
-			obj: &gatewayv1alpha2.TCPRoute{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "my-tcproute",
-				},
-			},
-			expected: schema.GroupVersionKind{
-				Group:   "gateway.networking.k8s.io",
-				Version: "v1alpha2",
-				Kind:    "TCPRoute",
-			},
-		},
-		{
-			name: "udproute",
-			obj: &gatewayv1alpha2.UDPRoute{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "my-udproute",
-				},
-			},
-			expected: schema.GroupVersionKind{
-				Group:   "gateway.networking.k8s.io",
-				Version: "v1alpha2",
-				Kind:    "UDPRoute",
-			},
-		},
-		{
-			name: "tlsroute",
-			obj: &gatewayv1alpha2.TLSRoute{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "my-tlsroute",
-				},
-			},
-			expected: schema.GroupVersionKind{
-				Group:   "gateway.networking.k8s.io",
-				Version: "v1alpha2",
-				Kind:    "TLSRoute",
-			},
-		},
-		{
-			name: "referencegrant",
-			obj: &gatewayv1beta1.ReferenceGrant{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "my-referencegrant",
-				},
-			},
-			expected: schema.GroupVersionKind{
-				Group:   "gateway.networking.k8s.io",
-				Version: "v1beta1",
-				Kind:    "ReferenceGrant",
-			},
-		},
-	}
-
-	for _, tc := range testcases {
-		t.Run(tc.name, func(t *testing.T) {
-			obj := fixupObjKinds(tc.obj)
-			assert.Equal(t, tc.expected.Group, obj.GetObjectKind().GroupVersionKind().Group)
-			assert.Equal(t, tc.expected.Kind, obj.GetObjectKind().GroupVersionKind().Kind)
-			assert.Equal(t, tc.expected.Version, obj.GetObjectKind().GroupVersionKind().Version)
-		})
-	}
-}
-
 func TestCleanerCanBeUsedConcurrently(*testing.T) {
-	cleaner := NewCleaner(nil)
-	for i := 0; i < 100; i++ {
+	cleaner := NewCleaner(nil, runtime.NewScheme())
+	for i := range 100 {
 		go func() {
 			cleaner.Add(&corev1.Pod{})
 		}()

test/integration/cleaner_test.go

@@ -11,6 +11,8 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
 	gatewayv1 "sigs.k8s.io/gateway-api/apis/v1"
 	gatewayclient "sigs.k8s.io/gateway-api/pkg/client/clientset/versioned"
 
@@ -32,7 +34,10 @@ func TestCleaner(t *testing.T) {
 	require.NoError(t, err)
 
 	cluster := env.Cluster()
-	cleaner := clusters.NewCleaner(cluster)
+	scheme := runtime.NewScheme()
+	require.NoError(t, clientgoscheme.AddToScheme(scheme))
+	require.NoError(t, gatewayv1.Install(scheme))
+	cleaner := clusters.NewCleaner(cluster, scheme)
 	t.Cleanup(func() { cleaner.Cleanup(context.Background()) })
 
 	t.Log("waiting for the test environment to be ready")