refactor(k8s): replace kubectl delete with delete api [BE-12560] (#1768)

api/exec/kubernetes_deploy.go

@@ -112,7 +112,7 @@ func (deployer *KubernetesDeployer) command(operation string, userID portainer.U
 
 	operations := map[string]func(context.Context, []string) (string, error){
 		"apply":  client.ApplyDynamic,
-		"delete": client.Delete,
+		"delete": client.DeleteDynamic,
 	}
 
 	operationFunc, ok := operations[operation]

pkg/libkubectl/apply_dynamic_test.go

@@ -21,9 +21,8 @@ import (
 //   - With cluster: go test -v ./pkg/libkubectl -run TestApplyDynamic
 //   - Without cluster: Tests will skip automatically
 //
-// Cleanup: Tests automatically clean up resources by extracting resource identifiers
-// from manifests and using client.Delete(). Resources are deleted after each test
-// completes, keeping the cluster clean.
+// Cleanup: Tests automatically clean up resources using client.DeleteDynamic().
+// Resources are deleted after each test completes, keeping the cluster clean.
 
 // skipIfNoKubeconfig skips the test if no kubeconfig is available
 func skipIfNoKubeconfig(tb testing.TB) string {
@@ -48,54 +47,6 @@ func skipIfNoKubeconfig(tb testing.TB) string {
 	return kubeconfig
 }
 
-// extractResourceIdentifiers extracts resource identifiers (kind/name) from YAML manifests
-// Returns a slice of resource identifiers in the format "kind/name" that can be used with Delete()
-func extractResourceIdentifiers(manifests []string) []string {
-	var identifiers []string
-
-	for _, manifest := range manifests {
-		manifest = strings.TrimSpace(manifest)
-		if manifest == "" {
-			continue
-		}
-
-		// Split by document separator if multiple resources in one manifest
-		resources := strings.SplitSeq(manifest, "\n---\n")
-
-		for resource := range resources {
-			resource = strings.TrimSpace(resource)
-			if resource == "" {
-				continue
-			}
-
-			// Extract kind and name using simple string parsing
-			var kind, name string
-			lines := strings.SplitSeq(resource, "\n")
-			for line := range lines {
-				line = strings.TrimSpace(line)
-				if after, ok := strings.CutPrefix(line, "kind:"); ok {
-					kind = strings.TrimSpace(after)
-				} else if strings.HasPrefix(line, "name:") && name == "" {
-					// Get the first name (resource name, not nested names)
-					name = strings.TrimSpace(strings.TrimPrefix(line, "name:"))
-				}
-				// Stop after metadata section
-				if kind != "" && name != "" {
-					break
-				}
-			}
-
-			if kind != "" && name != "" {
-				// Convert kind to lowercase for kubectl format
-				identifier := strings.ToLower(kind) + "/" + name
-				identifiers = append(identifiers, identifier)
-			}
-		}
-	}
-
-	return identifiers
-}
-
 func TestApplyDynamic(t *testing.T) {
 	tests := []struct {
 		name      string
@@ -595,14 +546,9 @@ data:
 			// This runs even if the test fails, ensuring cluster stays clean
 			if !tt.wantErr && len(tt.manifests) > 0 {
 				t.Cleanup(func() {
-					// Extract resource identifiers (kind/name) from manifests
-					resourceIDs := extractResourceIdentifiers(tt.manifests)
-					if len(resourceIDs) > 0 {
-						// Delete resources using client.Delete with resource identifiers
-						_, err := client.Delete(context.Background(), resourceIDs)
-						if err != nil {
-							t.Logf("Warning: failed to cleanup resources: %v", err)
-						}
+					_, err := client.DeleteDynamic(context.Background(), tt.manifests)
+					if err != nil {
+						t.Logf("Warning: failed to cleanup resources: %v", err)
 					}
 				})
 			}

pkg/libkubectl/delete_dynamic.go

@@ -0,0 +1,146 @@
+package libkubectl
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"strings"
+
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/util/yaml"
+	"k8s.io/client-go/discovery"
+	"k8s.io/client-go/dynamic"
+	"k8s.io/client-go/restmapper"
+)
+
+// DeleteDynamic deletes Kubernetes resources using the dynamic client.
+// This is the counterpart to ApplyDynamic and can delete resources from inline YAML manifests.
+func (c *Client) DeleteDynamic(ctx context.Context, manifests []string) (string, error) {
+	// Create REST config from the factory
+	restConfig, err := c.factory.ToRESTConfig()
+	if err != nil {
+		return "", fmt.Errorf("failed to create REST config: %w", err)
+	}
+
+	// Create dynamic client
+	dynamicClient, err := dynamic.NewForConfig(restConfig)
+	if err != nil {
+		return "", fmt.Errorf("failed to create dynamic client: %w", err)
+	}
+
+	// Create discovery client for resource mapping
+	discoveryClient, err := discovery.NewDiscoveryClientForConfig(restConfig)
+	if err != nil {
+		return "", fmt.Errorf("failed to create discovery client: %w", err)
+	}
+
+	// Create REST mapper
+	groupResources, err := restmapper.GetAPIGroupResources(discoveryClient)
+	if err != nil {
+		return "", fmt.Errorf("failed to get API group resources: %w", err)
+	}
+	mapper := restmapper.NewDiscoveryRESTMapper(groupResources)
+
+	var results []string
+	var errs error
+
+	// Process each manifest
+	for _, manifest := range manifests {
+		manifest = strings.TrimSpace(manifest)
+		if manifest == "" {
+			continue
+		}
+
+		// Split by document separator if multiple resources in one manifest
+		for resource := range strings.SplitSeq(manifest, "\n---\n") {
+			resource = strings.TrimSpace(resource)
+			if resource == "" {
+				continue
+			}
+
+			result, err := c.deleteResource(ctx, dynamicClient, mapper, []byte(resource))
+			if err != nil {
+				errs = errors.Join(errs, err)
+				continue
+			}
+			results = append(results, result)
+		}
+	}
+
+	// Build output message
+	output := strings.Join(results, "\n")
+
+	if errs != nil {
+		if len(results) == 0 {
+			return "", fmt.Errorf("failed to delete resources: %s", errs.Error())
+		}
+		return output, fmt.Errorf("partially deleted resources with errors: %s", errs.Error())
+	}
+
+	return output, nil
+}
+
+// deleteResource deletes a single resource
+func (c *Client) deleteResource(ctx context.Context, dynamicClient dynamic.Interface, mapper meta.RESTMapper, resourceYAML []byte) (string, error) {
+	// Decode YAML to unstructured object
+	obj := &unstructured.Unstructured{}
+	decoder := yaml.NewYAMLOrJSONDecoder(strings.NewReader(string(resourceYAML)), 4096)
+	if err := decoder.Decode(obj); err != nil {
+		// Ignore decode errors for empty documents
+		return "", nil
+	}
+
+	// Skip empty objects
+	if obj.Object == nil {
+		return "", nil
+	}
+
+	// Get GVK (GroupVersionKind) from the object
+	gvk := obj.GroupVersionKind()
+	if gvk.Empty() {
+		return "", nil
+	}
+
+	// Map GVK to GVR (GroupVersionResource)
+	mapping, err := mapper.RESTMapping(gvk.GroupKind(), gvk.Version)
+	if err != nil {
+		return "", fmt.Errorf("failed to map resource type %s: %w", gvk.String(), err)
+	}
+
+	// Get namespace (if applicable)
+	namespace := obj.GetNamespace()
+	name := obj.GetName()
+
+	// Get the dynamic resource client
+	var resourceClient dynamic.ResourceInterface
+	if mapping.Scope.Name() == meta.RESTScopeNameNamespace {
+		// Namespaced resource
+		if namespace == "" {
+			namespace = "default"
+		}
+		resourceClient = dynamicClient.Resource(mapping.Resource).Namespace(namespace)
+	} else {
+		// Cluster-scoped resource
+		resourceClient = dynamicClient.Resource(mapping.Resource)
+	}
+
+	// Delete the resource
+	// Use nil GracePeriodSeconds to respect the resource's default grace period (consistent with kubectl)
+	deleteOptions := metav1.DeleteOptions{}
+
+	err = resourceClient.Delete(ctx, name, deleteOptions)
+	if err != nil {
+		// Ignore not found errors (consistent with kubectl delete --ignore-not-found behavior)
+		if apierrors.IsNotFound(err) {
+			return fmt.Sprintf("%s/%s deleted (not found)", strings.ToLower(gvk.Kind), name), nil
+		}
+		return "", fmt.Errorf("failed to delete %s %s/%s: %w", gvk.Kind, namespace, name, err)
+	}
+
+	// Format output message (consistent with kubectl output format)
+	resourceType := strings.ToLower(gvk.Kind)
+	return fmt.Sprintf("%s/%s deleted", resourceType, name), nil
+}

pkg/libkubectl/delete_dynamic_test.go

@@ -0,0 +1,235 @@
+package libkubectl
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestDeleteDynamic tests require a Kubernetes cluster.
+//
+// Running the tests:
+//   - With cluster: go test -v ./pkg/libkubectl -run TestDeleteDynamic
+//   - Without cluster: Tests will skip automatically
+//
+// Test focus: Deletion logic and error handling, not resource type coverage.
+// Resource type coverage is handled by ApplyDynamic tests.
+
+func TestDeleteDynamic(t *testing.T) {
+	tests := []struct {
+		name      string
+		manifests []string
+		wantErr   bool
+		desc      string
+	}{
+		{
+			name: "delete simple resource",
+			desc: "Test basic resource deletion - happy path",
+			manifests: []string{
+				`apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-delete-config
+  namespace: default
+data:
+  key1: value1`,
+			},
+			wantErr: false,
+		},
+		{
+			name: "delete multiple resources in one manifest",
+			desc: "Test deletion with multiple resources separated by ---",
+			manifests: []string{
+				`apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-delete-config1
+  namespace: default
+data:
+  key: value1
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-delete-config2
+  namespace: default
+data:
+  key: value2`,
+			},
+			wantErr: false,
+		},
+		{
+			name: "missing metadata name",
+			desc: "Test manifest with missing name in metadata",
+			manifests: []string{
+				`apiVersion: v1
+kind: ConfigMap
+metadata:
+  namespace: default
+data:
+  key: value`,
+			},
+			wantErr: true,
+		},
+		{
+			name: "invalid resource type",
+			desc: "Test deletion with non-existent resource type",
+			manifests: []string{
+				`apiVersion: v1
+kind: NonExistentResourceType
+metadata:
+  name: test-invalid-type
+  namespace: default`,
+			},
+			wantErr: true,
+		},
+		{
+			name: "invalid apiVersion",
+			desc: "Test deletion with invalid apiVersion",
+			manifests: []string{
+				`apiVersion: invalid/v999
+kind: ConfigMap
+metadata:
+  name: test-invalid-apiversion
+  namespace: default`,
+			},
+			wantErr: true,
+		},
+		{
+			name: "partial failure with multiple resources",
+			desc: "Test partial deletion with some invalid resources",
+			manifests: []string{
+				`apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: valid-config-for-delete
+  namespace: default
+data:
+  key: value
+---
+apiVersion: invalid/v999
+kind: ConfigMap
+metadata:
+  name: invalid-apiversion
+  namespace: default
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: another-valid-config-for-delete
+  namespace: default
+data:
+  key: value2`,
+			},
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			// Skip if no Kubernetes cluster is available
+			kubeconfig := skipIfNoKubeconfig(t)
+
+			// Create client with empty namespace to let manifest namespaces be used
+			client, err := NewClient(&ClientAccess{}, "", kubeconfig, false)
+			require.NoError(t, err, "Failed to create client")
+
+			// For tests that expect to delete existing resources, create them first
+			// Only create resources for happy path tests
+			shouldCreate := !tt.wantErr
+
+			if shouldCreate {
+				// Create the resources first
+				_, err := client.ApplyDynamic(context.Background(), tt.manifests)
+				require.NoError(t, err, "Failed to create resources for deletion test")
+			}
+
+			// Test DeleteDynamic
+			output, err := client.DeleteDynamic(context.Background(), tt.manifests)
+
+			if tt.wantErr {
+				require.Error(t, err, "DeleteDynamic() expected error but got none")
+			} else {
+				require.NoError(t, err)
+				require.NotEmpty(t, output, "DeleteDynamic() expected output but got empty string")
+				require.Contains(t, output, "deleted", "DeleteDynamic() output should contain 'deleted'")
+			}
+		})
+	}
+}
+
+// TestDeleteDynamicAlreadyDeleted tests the behavior when deleting resources that were already deleted
+func TestDeleteDynamicAlreadyDeleted(t *testing.T) {
+	kubeconfig := skipIfNoKubeconfig(t)
+
+	manifest := []string{
+		`apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-double-delete
+  namespace: default
+data:
+  key: value`,
+	}
+
+	client, err := NewClient(&ClientAccess{}, "", kubeconfig, false)
+	require.NoError(t, err, "Failed to create client")
+
+	// Create the resource
+	_, err = client.ApplyDynamic(context.Background(), manifest)
+	require.NoError(t, err, "Failed to create resource")
+
+	// Delete it once
+	_, err = client.DeleteDynamic(context.Background(), manifest)
+	require.NoError(t, err, "First DeleteDynamic() failed")
+
+	// Delete it again (should succeed since DeleteDynamic ignores not found errors)
+	_, err = client.DeleteDynamic(context.Background(), manifest)
+	require.NoError(t, err, "Second DeleteDynamic() should not return error for already deleted resource")
+}
+
+// TestDeleteDynamicPartialFailure tests deletion when some resources fail
+func TestDeleteDynamicPartialFailure(t *testing.T) {
+	kubeconfig := skipIfNoKubeconfig(t)
+
+	client, err := NewClient(&ClientAccess{}, "", kubeconfig, false)
+	require.NoError(t, err, "Failed to create client")
+
+	// Create one valid resource
+	validManifest := []string{
+		`apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-partial-delete-valid
+  namespace: default
+data:
+  key: value`,
+	}
+
+	_, err = client.ApplyDynamic(context.Background(), validManifest)
+	require.NoError(t, err, "Failed to create resource")
+
+	t.Cleanup(func() {
+		_, err = client.DeleteDynamic(context.Background(), validManifest)
+		require.NoError(t, err, "Cleanup DeleteDynamic() failed")
+	})
+
+	// Try to delete valid resource + non-existent resource
+	mixedManifests := []string{
+		`apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: test-partial-delete-valid
+  namespace: default
+---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: non-existent-resource-xyz
+  namespace: default`,
+	}
+
+	_, err = client.DeleteDynamic(context.Background(), mixedManifests)
+	require.NoError(t, err, "DeleteDynamic() should handle non-existent resources gracefully")
+}

pkg/libkubectl/delete_test.go

@@ -0,0 +1,141 @@
+package libkubectl
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"runtime"
+	"testing"
+)
+
+// BenchmarkDeleteDynamic tests require a Kubernetes cluster.
+// BenchmarkDeleteDynamic measures the performance and memory allocation of the DeleteDynamic function.
+// The resource is recreated before each delete (using StopTimer/StartTimer to exclude setup time),
+// so all iterations measure actual delete performance, not "not found" handling.
+func BenchmarkDeleteDynamic(b *testing.B) {
+	kubeconfig := skipIfNoKubeconfig(b)
+	client, err := NewClient(&ClientAccess{}, "default", kubeconfig, false)
+	if err != nil {
+		b.Fatalf("Failed to create client: %v", err)
+	}
+
+	manifests := []string{
+		`apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: benchmark-delete-test-config
+  namespace: default
+data:
+  key: value`,
+	}
+
+	ctx := context.Background()
+
+	// Apply the manifest once BEFORE the benchmark loop
+	_, err = client.ApplyDynamic(ctx, manifests)
+	if err != nil {
+		b.Fatalf("Failed to apply initial manifest: %v", err)
+	}
+
+	// Force GC and measure memory before benchmark
+	runtime.GC()
+	var memBefore runtime.MemStats
+	runtime.ReadMemStats(&memBefore)
+
+	for b.Loop() {
+		// Recreate the resource before each delete to ensure it always exists
+		b.StopTimer()
+		_, err = client.ApplyDynamic(ctx, manifests)
+		if err != nil {
+			b.Fatalf("Failed to apply manifest: %v", err)
+		}
+		b.StartTimer()
+
+		// Delete the resource (always exists)
+		_, err = client.DeleteDynamic(ctx, manifests)
+		if err != nil {
+			b.Errorf("Failed to delete dynamic manifests: %v", err)
+		}
+	}
+
+	// Force GC and measure memory after benchmark
+	runtime.GC()
+	var memAfter runtime.MemStats
+	runtime.ReadMemStats(&memAfter)
+
+	// Report retained memory (key metric for leak detection)
+	retained := int64(memAfter.Alloc) - int64(memBefore.Alloc)
+	retainedMB := float64(retained) / 1024 / 1024
+	b.ReportMetric(float64(retained), "B-retained")
+	b.ReportMetric(retainedMB, "MB-retained")
+}
+
+// BenchmarkDelete tests require a Kubernetes cluster.
+// BenchmarkDelete measures the performance and memory allocation of the Delete function.
+// The resource is recreated before each delete (using StopTimer/StartTimer to exclude setup time),
+// so all iterations measure actual delete performance, not "not found" handling.
+func BenchmarkDelete(b *testing.B) {
+	kubeconfig := skipIfNoKubeconfig(b)
+
+	client, err := NewClient(&ClientAccess{}, "default", kubeconfig, false)
+	if err != nil {
+		b.Fatalf("Failed to create client: %v", err)
+	}
+
+	// Delete expects file paths, so create a temp file
+	tmpDir := b.TempDir()
+	manifestFile := filepath.Join(tmpDir, "manifest.yaml")
+	manifestContent := `apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: benchmark-delete-file-test-config
+  namespace: default
+data:
+  key: value`
+
+	if err := os.WriteFile(manifestFile, []byte(manifestContent), 0644); err != nil {
+		b.Fatalf("Failed to create manifest file: %v", err)
+	}
+
+	manifests := []string{manifestFile}
+	manifestsInline := []string{manifestContent}
+	ctx := context.Background()
+
+	// Apply the manifest once BEFORE the benchmark loop using ApplyDynamic
+	_, err = client.ApplyDynamic(ctx, manifestsInline)
+	if err != nil {
+		b.Fatalf("Failed to apply initial manifest: %v", err)
+	}
+
+	// Force GC and measure memory before benchmark
+	runtime.GC()
+	var memBefore runtime.MemStats
+	runtime.ReadMemStats(&memBefore)
+
+	for b.Loop() {
+		// Recreate the resource before each delete to ensure it always exists
+		b.StopTimer()
+		_, err = client.ApplyDynamic(ctx, manifestsInline)
+		if err != nil {
+			b.Fatalf("Failed to apply manifest: %v", err)
+		}
+		b.StartTimer()
+
+		// Delete the resource using Delete (file-based, always exists)
+		_, err = client.Delete(ctx, manifests)
+		if err != nil {
+			b.Errorf("Failed to delete manifests: %v", err)
+		}
+	}
+
+	// Force GC and measure memory after benchmark
+	runtime.GC()
+	var memAfter runtime.MemStats
+	runtime.ReadMemStats(&memAfter)
+
+	// Report retained memory (key metric for leak detection)
+	retained := int64(memAfter.Alloc) - int64(memBefore.Alloc)
+	retainedMB := float64(retained) / 1024 / 1024
+	b.ReportMetric(float64(retained), "B-retained")
+	b.ReportMetric(retainedMB, "MB-retained")
+}