Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/extension/extension.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"os"
"path/filepath"
"strings"
"sync"

"github.com/mcpchecker/mcpchecker/pkg/extension/sdk"
"k8s.io/client-go/dynamic"
Expand All @@ -17,6 +18,9 @@ import (
type Extension struct {
*sdk.Extension
client ResourceClient

mu sync.Mutex
generatedNamespaces []string
}

// New creates a new Kubernetes extension
Expand Down
142 changes: 142 additions & 0 deletions pkg/extension/namespace.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
package extension

import (
"context"
"crypto/rand"
"fmt"
"strings"

"github.com/mcpchecker/mcpchecker/pkg/extension/sdk"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
)

var namespaceGVR = schema.GroupVersionResource{Group: "", Version: "v1", Resource: "namespaces"}

const alphanumeric = "abcdefghijklmnopqrstuvwxyz0123456789"

// generateID returns a random lowercase alphanumeric string of the given length.
// This matches the {random.id} spec from mcpchecker/mcpchecker#102.
func generateID(length int) (string, error) {
b := make([]byte, length)
if _, err := rand.Read(b); err != nil {
return "", fmt.Errorf("failed to generate random id: %w", err)
}
for i := range b {
b[i] = alphanumeric[int(b[i])%len(alphanumeric)]
}
return string(b), nil
}

func (e *Extension) handleCreateNamespace(ctx context.Context, req *sdk.OperationRequest) (*sdk.OperationResult, error) {
if e.client == nil {
return sdk.Failure(fmt.Errorf("kubernetes client not initialized")), nil
}

args, ok := req.Args.(map[string]any)
if !ok {
return sdk.Failure(fmt.Errorf("args must be an object")), nil
}

prefix, _ := args["prefix"].(string)
if prefix == "" {
return sdk.Failure(fmt.Errorf("prefix is required")), nil
}

id, err := generateID(8)
if err != nil {
return sdk.Failure(err), nil
}

name := fmt.Sprintf("%s-%s", prefix, id)

obj := &unstructured.Unstructured{
Object: map[string]any{
"apiVersion": "v1",
"kind": "Namespace",
"metadata": map[string]any{
"name": name,
},
},
}

e.LogInfo(ctx, "Creating namespace", map[string]any{
"name": name,
})

result, err := e.client.Create(ctx, namespaceGVR, obj, "")
if err != nil {
e.LogError(ctx, "Failed to create namespace", map[string]any{
"name": name,
"error": err.Error(),
})
return sdk.Failure(fmt.Errorf("failed to create namespace: %w", err)), nil
}

e.mu.Lock()
e.generatedNamespaces = append(e.generatedNamespaces, result.GetName())
e.mu.Unlock()

e.LogInfo(ctx, "Namespace created successfully", map[string]any{
"name": result.GetName(),
})

return sdk.SuccessWithOutputs(
fmt.Sprintf("Created namespace %s", result.GetName()),
map[string]string{
"namespace": result.GetName(),
},
), nil
}

func (e *Extension) handleDeleteGeneratedNamespaces(ctx context.Context, req *sdk.OperationRequest) (*sdk.OperationResult, error) {
if e.client == nil {
return sdk.Failure(fmt.Errorf("kubernetes client not initialized")), nil
}

e.mu.Lock()
namespaces := make([]string, len(e.generatedNamespaces))
copy(namespaces, e.generatedNamespaces)
e.generatedNamespaces = nil
e.mu.Unlock()
Comment on lines +99 to +103

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Pre-emptive clear of generatedNamespaces causes permanent tracking loss on partial deletion failure.

e.generatedNamespaces is set to nil before any delete calls execute (Line 98). If any deletion fails with a non-NotFound error, those namespace names are already gone from tracking. A second call to deleteGeneratedNamespaces finds nothing to do — the leaked namespaces can only be recovered manually.

Consider collecting namespaces that fail to delete and restoring them to tracking before returning the error, so the operation is retryable:

🔁 Proposed fix
 e.mu.Lock()
 namespaces := make([]string, len(e.generatedNamespaces))
 copy(namespaces, e.generatedNamespaces)
 e.generatedNamespaces = nil
 e.mu.Unlock()

 // ... len check, log, deleteOpts setup ...

+var failedNamespaces []string
 var errs []string
 for _, ns := range namespaces {
     err := e.client.Delete(ctx, namespaceGVR, ns, "", deleteOpts)
     if err != nil {
         if apierrors.IsNotFound(err) {
             e.LogInfo(ctx, "Namespace already deleted (ignored)", map[string]any{
                 "name": ns,
             })
             continue
         }
         e.LogError(ctx, "Failed to delete namespace", map[string]any{
             "name":  ns,
             "error": err.Error(),
         })
+        failedNamespaces = append(failedNamespaces, ns)
         errs = append(errs, fmt.Sprintf("%s: %s", ns, err.Error()))
     }
 }

 if len(errs) > 0 {
+    // Restore failed namespaces so the caller can retry.
+    e.mu.Lock()
+    e.generatedNamespaces = append(failedNamespaces, e.generatedNamespaces...)
+    e.mu.Unlock()
     return sdk.Failure(fmt.Errorf("failed to delete namespaces: %s", strings.Join(errs, "; "))), nil
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/extension/namespace.go` around lines 95 - 99, The code copies
e.generatedNamespaces and then sets e.generatedNamespaces = nil before
performing deletions, which permanently loses names if some deletes fail; modify
the deleteGeneratedNamespaces logic so you copy the list under e.mu, but do NOT
clear e.generatedNamespaces until after successful deletions, or if you must
clear early keep a temp slice of names and on any non-NotFound delete error
re-acquire e.mu and append the failed namespace names back into
e.generatedNamespaces before returning the error; treat NotFound as success and
ensure all mutations to e.generatedNamespaces are done while holding e.mu to
avoid races.


if len(namespaces) == 0 {
return sdk.Success("No generated namespaces to delete"), nil
}

e.LogInfo(ctx, "Deleting generated namespaces", map[string]any{
"count": len(namespaces),
"namespaces": namespaces,
})

propagation := metav1.DeletePropagationForeground
deleteOpts := metav1.DeleteOptions{
PropagationPolicy: &propagation,
}

var errs []string
for _, ns := range namespaces {
err := e.client.Delete(ctx, namespaceGVR, ns, "", deleteOpts)
if err != nil {
if apierrors.IsNotFound(err) {
e.LogInfo(ctx, "Namespace already deleted (ignored)", map[string]any{
"name": ns,
})
continue
}
e.LogError(ctx, "Failed to delete namespace", map[string]any{
"name": ns,
"error": err.Error(),
})
errs = append(errs, fmt.Sprintf("%s: %s", ns, err.Error()))
}
}

if len(errs) > 0 {
return sdk.Failure(fmt.Errorf("failed to delete namespaces: %s", strings.Join(errs, "; "))), nil
}

return sdk.Success(fmt.Sprintf("Deleted %d generated namespace(s)", len(namespaces))), nil
}
219 changes: 219 additions & 0 deletions pkg/extension/namespace_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,219 @@
package extension

import (
"context"
"errors"
"strings"
"testing"

"github.com/mcpchecker/mcpchecker/pkg/extension/sdk"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
)

func TestHandleCreateNamespace(t *testing.T) {
tests := []struct {
name string
args any
client *mockClient
wantSuccess bool
checkOutputs func(t *testing.T, result *sdk.OperationResult, ext *Extension)
}{
{
name: "successful create",
args: map[string]any{
"prefix": "vm-test",
},
client: &mockClient{
createFn: func(ctx context.Context, gvr schema.GroupVersionResource, obj *unstructured.Unstructured, namespace string) (*unstructured.Unstructured, error) {
return obj.DeepCopy(), nil
},
},
wantSuccess: true,
checkOutputs: func(t *testing.T, result *sdk.OperationResult, ext *Extension) {
t.Helper()
ns, ok := result.Outputs["namespace"]
if !ok {
t.Fatal("expected namespace output key")
}
if !strings.HasPrefix(ns, "vm-test-") {
t.Errorf("namespace %q does not have prefix vm-test-", ns)
}
// ID should be 8 lowercase alphanumeric chars per mcpchecker#102
id := strings.TrimPrefix(ns, "vm-test-")
if len(id) != 8 {
t.Errorf("expected 8-char id, got %q (len=%d)", id, len(id))
}

ext.mu.Lock()
defer ext.mu.Unlock()
if len(ext.generatedNamespaces) != 1 {
t.Fatalf("expected 1 tracked namespace, got %d", len(ext.generatedNamespaces))
}
if ext.generatedNamespaces[0] != ns {
t.Errorf("tracked namespace %q != output namespace %q", ext.generatedNamespaces[0], ns)
}
},
},
{
name: "invalid args type",
args: "not a map",
client: &mockClient{},
wantSuccess: false,
},
{
name: "missing prefix",
args: map[string]any{
"other": "value",
},
client: &mockClient{},
wantSuccess: false,
},
{
name: "client error",
args: map[string]any{
"prefix": "vm-test",
},
client: &mockClient{
createFn: func(ctx context.Context, gvr schema.GroupVersionResource, obj *unstructured.Unstructured, namespace string) (*unstructured.Unstructured, error) {
return nil, errors.New("connection refused")
},
},
wantSuccess: false,
checkOutputs: func(t *testing.T, result *sdk.OperationResult, ext *Extension) {
t.Helper()
ext.mu.Lock()
defer ext.mu.Unlock()
if len(ext.generatedNamespaces) != 0 {
t.Errorf("expected no tracked namespaces on error, got %d", len(ext.generatedNamespaces))
}
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ext := &Extension{
Extension: sdk.NewExtension(sdk.ExtensionInfo{Name: "test"}),
client: tt.client,
}

req := &sdk.OperationRequest{Args: tt.args}
result, err := ext.handleCreateNamespace(context.Background(), req)

if err != nil {
t.Fatalf("handleCreateNamespace() returned error: %v", err)
}
if result.Success != tt.wantSuccess {
t.Errorf("handleCreateNamespace() success = %v, want %v (error: %s)", result.Success, tt.wantSuccess, result.Error)
}
if tt.checkOutputs != nil {
tt.checkOutputs(t, result, ext)
}
})
}
}

func TestHandleDeleteGeneratedNamespaces(t *testing.T) {
tests := []struct {
name string
tracked []string
client *mockClient
wantSuccess bool
checkTracked func(t *testing.T, ext *Extension)
}{
{
name: "no tracked namespaces",
tracked: nil,
client: &mockClient{},
wantSuccess: true,
},
{
name: "successful deletion of two namespaces",
tracked: []string{"vm-test-abc123", "vm-test-def456"},
client: &mockClient{
deleteFn: func(ctx context.Context, gvr schema.GroupVersionResource, name, namespace string, opts metav1.DeleteOptions) error {
return nil
},
},
wantSuccess: true,
checkTracked: func(t *testing.T, ext *Extension) {
t.Helper()
ext.mu.Lock()
defer ext.mu.Unlock()
if len(ext.generatedNamespaces) != 0 {
t.Errorf("expected tracking cleared, got %d namespaces", len(ext.generatedNamespaces))
}
},
},
{
name: "not-found error silently ignored",
tracked: []string{"vm-test-gone"},
client: &mockClient{
deleteFn: func(ctx context.Context, gvr schema.GroupVersionResource, name, namespace string, opts metav1.DeleteOptions) error {
return apierrors.NewNotFound(schema.GroupResource{Resource: "namespaces"}, name)
},
},
wantSuccess: true,
},
{
name: "non-not-found error causes failure",
tracked: []string{"vm-test-err"},
client: &mockClient{
deleteFn: func(ctx context.Context, gvr schema.GroupVersionResource, name, namespace string, opts metav1.DeleteOptions) error {
return errors.New("permission denied")
},
},
wantSuccess: false,
},
Comment on lines +162 to +170

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

"non-not-found error" case is missing a checkTracked assertion.

handleDeleteGeneratedNamespaces clears generatedNamespaces under the lock before attempting any deletes. When deletion fails with a non-NotFound error, tracking is already nil — but the test doesn't assert this, leaving the pre-emptive-clear behavior completely unverified. This is especially important to document here, because it means failed namespaces cannot be retried via a second call to deleteGeneratedNamespaces.

✅ Proposed addition
 		{
 			name:    "non-not-found error causes failure",
 			tracked: []string{"vm-test-err"},
 			client: &mockClient{
 				deleteFn: func(ctx context.Context, gvr schema.GroupVersionResource, name, namespace string, opts metav1.DeleteOptions) error {
 					return errors.New("permission denied")
 				},
 			},
 			wantSuccess: false,
+			checkTracked: func(t *testing.T, ext *Extension) {
+				t.Helper()
+				ext.mu.Lock()
+				defer ext.mu.Unlock()
+				// tracking is cleared pre-emptively even when deletion fails
+				if len(ext.generatedNamespaces) != 0 {
+					t.Errorf("expected tracking cleared on error, got %d namespaces", len(ext.generatedNamespaces))
+				}
+			},
 		},
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
name: "non-not-found error causes failure",
tracked: []string{"vm-test-err"},
client: &mockClient{
deleteFn: func(ctx context.Context, gvr schema.GroupVersionResource, name, namespace string, opts metav1.DeleteOptions) error {
return errors.New("permission denied")
},
},
wantSuccess: false,
},
name: "non-not-found error causes failure",
tracked: []string{"vm-test-err"},
client: &mockClient{
deleteFn: func(ctx context.Context, gvr schema.GroupVersionResource, name, namespace string, opts metav1.DeleteOptions) error {
return errors.New("permission denied")
},
},
wantSuccess: false,
checkTracked: func(t *testing.T, ext *Extension) {
t.Helper()
ext.mu.Lock()
defer ext.mu.Unlock()
// tracking is cleared pre-emptively even when deletion fails
if len(ext.generatedNamespaces) != 0 {
t.Errorf("expected tracking cleared on error, got %d namespaces", len(ext.generatedNamespaces))
}
},
},
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/extension/namespace_test.go` around lines 162 - 170, Add a checkTracked
assertion for the "non-not-found error causes failure" test case to verify that
generatedNamespaces is cleared even when deletion fails: in the test case where
mockClient.deleteFn returns errors.New("permission denied"), after calling
handleDeleteGeneratedNamespaces (or deleteGeneratedNamespaces if that helper is
used) assert via checkTracked that generatedNamespaces is nil/empty; reference
the test case name, the mockClient.deleteFn, the generatedNamespaces field and
the checkTracked helper so the assertion is added next to the existing
wantSuccess:false expectation to document pre-emptive clearing behavior.

}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
ext := &Extension{
Extension: sdk.NewExtension(sdk.ExtensionInfo{Name: "test"}),
client: tt.client,
generatedNamespaces: tt.tracked,
}

req := &sdk.OperationRequest{Args: map[string]any{}}
result, err := ext.handleDeleteGeneratedNamespaces(context.Background(), req)

if err != nil {
t.Fatalf("handleDeleteGeneratedNamespaces() returned error: %v", err)
}
if result.Success != tt.wantSuccess {
t.Errorf("handleDeleteGeneratedNamespaces() success = %v, want %v (error: %s)", result.Success, tt.wantSuccess, result.Error)
}
if tt.checkTracked != nil {
tt.checkTracked(t, ext)
}
})
}
}

func TestGenerateID(t *testing.T) {
id, err := generateID(8)
if err != nil {
t.Fatalf("generateID() error: %v", err)
}
if len(id) != 8 {
t.Errorf("expected 8-char id, got %q (len=%d)", id, len(id))
}
for _, c := range id {
if !strings.ContainsRune(alphanumeric, c) {
t.Errorf("id contains invalid character %q", string(c))
}
}

// Verify uniqueness (two calls should produce different results)
id2, err := generateID(8)
if err != nil {
t.Fatalf("generateID() error: %v", err)
}
if id == id2 {
t.Errorf("expected unique ids, got %q twice", id)
}
}
Loading