From d7afdf214bd3cf73e75b6be2d64de372b885f29b Mon Sep 17 00:00:00 2001 From: Oscar Zhou <100548325+oscarzhou-portainer@users.noreply.github.com> Date: Tue, 3 Feb 2026 08:36:08 +1300 Subject: [PATCH] refactor(k8s): replace kubectl delete with delete api [BE-12560] (#1768) --- api/exec/kubernetes_deploy.go | 2 +- pkg/libkubectl/apply_dynamic_test.go | 64 +------ pkg/libkubectl/delete_dynamic.go | 146 ++++++++++++++++ pkg/libkubectl/delete_dynamic_test.go | 235 ++++++++++++++++++++++++++ pkg/libkubectl/delete_test.go | 141 ++++++++++++++++ 5 files changed, 528 insertions(+), 60 deletions(-) create mode 100644 pkg/libkubectl/delete_dynamic.go create mode 100644 pkg/libkubectl/delete_dynamic_test.go create mode 100644 pkg/libkubectl/delete_test.go diff --git a/api/exec/kubernetes_deploy.go b/api/exec/kubernetes_deploy.go index 5d0cf5ad9..b89c15e3c 100644 --- a/api/exec/kubernetes_deploy.go +++ b/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] diff --git a/pkg/libkubectl/apply_dynamic_test.go b/pkg/libkubectl/apply_dynamic_test.go index 6a39ca184..d2b2e1baf 100644 --- a/pkg/libkubectl/apply_dynamic_test.go +++ b/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) } }) } diff --git a/pkg/libkubectl/delete_dynamic.go b/pkg/libkubectl/delete_dynamic.go new file mode 100644 index 000000000..c1f0b2900 --- /dev/null +++ b/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 +} diff --git a/pkg/libkubectl/delete_dynamic_test.go b/pkg/libkubectl/delete_dynamic_test.go new file mode 100644 index 000000000..47c8819a6 --- /dev/null +++ b/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") +} diff --git a/pkg/libkubectl/delete_test.go b/pkg/libkubectl/delete_test.go new file mode 100644 index 000000000..f90413431 --- /dev/null +++ b/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") +}