feat: add createNamespace and deleteGeneratedNamespaces operations - #23
Merged
nader-ziada merged 1 commit intoFeb 25, 2026
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds synchronization support and Kubernetes namespace management to the Extension struct. New functions generate and track temporary namespaces, with handlers to create namespaces with random suffixes and delete all generated namespaces with thread-safe operations. Changes
Sequence DiagramsequenceDiagram
participant Handler as Extension Handler
participant Tracker as generatedNamespaces List
participant K8s as Kubernetes Client
participant API as Kubernetes API
rect rgba(100, 150, 255, 0.5)
Note over Handler: handleCreateNamespace Flow
Handler->>Handler: Validate client & input prefix
Handler->>Handler: Generate random suffix
Handler->>Handler: Construct Namespace object
Handler->>K8s: Create Namespace
K8s->>API: POST namespace
API-->>K8s: Namespace created
K8s-->>Handler: Success
Handler->>Tracker: Lock & append namespace name
Tracker-->>Handler: Namespace tracked
end
rect rgba(150, 200, 100, 0.5)
Note over Handler: handleDeleteGeneratedNamespaces Flow
Handler->>Tracker: Lock & snapshot namespace list
Tracker-->>Handler: Namespace list snapshot
Handler->>Tracker: Clear list
Tracker-->>Handler: List cleared
loop For each tracked namespace
Handler->>K8s: Delete Namespace
K8s->>API: DELETE namespace
API-->>K8s: Success or not-found
K8s-->>Handler: Result
Handler->>Handler: Aggregate errors
end
Handler-->>Handler: Return aggregated result
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/extension/namespace_test.go`:
- Around line 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.
In `@pkg/extension/namespace.go`:
- Around line 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.
- Around line 39-49: Validate the incoming args["prefix"] (the local variable
prefix) against Kubernetes/RFC1123 DNS label rules before building name :=
fmt.Sprintf("%s-%s", prefix, suffix): ensure prefix only contains lowercase
alphanumerics and hyphens, does not start or end with a hyphen (use regex like
^[a-z0-9]([a-z0-9-]*[a-z0-9])?$) and enforce a length limit so
len(prefix)+1+len(suffix) <= 63 (compute maxPrefixLen = 63 - 1 - len(suffix));
if validation fails return sdk.Failure(fmt.Errorf(...)), nil. Use the existing
generateSuffix(3) result to compute the allowed prefix length and reference
prefix, generateSuffix, and name when adding the checks and error returns.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/extension/extension.gopkg/extension/namespace.gopkg/extension/namespace_test.gopkg/extension/operations.go
Comment on lines
+162
to
+170
| 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, | ||
| }, |
There was a problem hiding this comment.
"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.
Comment on lines
+39
to
+49
| prefix, _ := args["prefix"].(string) | ||
| if prefix == "" { | ||
| return sdk.Failure(fmt.Errorf("prefix is required")), nil | ||
| } | ||
|
|
||
| suffix, err := generateSuffix(3) | ||
| if err != nil { | ||
| return sdk.Failure(err), nil | ||
| } | ||
|
|
||
| name := fmt.Sprintf("%s-%s", prefix, suffix) |
There was a problem hiding this comment.
Consider validating prefix against Kubernetes naming constraints.
The generated name prefix-suffix is passed directly to the K8s API with no upfront validation. A prefix containing uppercase letters, underscores, or a leading/trailing hyphen — or a prefix long enough to push the total beyond 63 characters (RFC 1123 DNS label limit) — will cause the API to reject the request with an error message that may be harder to act on than an upfront check.
🛡️ Proposed guard (adjust regex/limit to taste)
+import "regexp"
+
+var validPrefixRe = regexp.MustCompile(`^[a-z0-9][a-z0-9\-]*[a-z0-9]$|^[a-z0-9]$`)
+
prefix, _ := args["prefix"].(string)
if prefix == "" {
return sdk.Failure(fmt.Errorf("prefix is required")), nil
}
+// suffix is 6 chars + 1 hyphen separator = 7
+if len(prefix)+7 > 63 {
+ return sdk.Failure(fmt.Errorf("prefix %q too long: generated name would exceed 63 characters", prefix)), nil
+}
+if !validPrefixRe.MatchString(prefix) {
+ return sdk.Failure(fmt.Errorf("prefix %q is not a valid DNS label segment (lowercase alphanumeric and hyphens only)", prefix)), 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 39 - 49, Validate the incoming
args["prefix"] (the local variable prefix) against Kubernetes/RFC1123 DNS label
rules before building name := fmt.Sprintf("%s-%s", prefix, suffix): ensure
prefix only contains lowercase alphanumerics and hyphens, does not start or end
with a hyphen (use regex like ^[a-z0-9]([a-z0-9-]*[a-z0-9])?$) and enforce a
length limit so len(prefix)+1+len(suffix) <= 63 (compute maxPrefixLen = 63 - 1 -
len(suffix)); if validation fails return sdk.Failure(fmt.Errorf(...)), nil. Use
the existing generateSuffix(3) result to compute the allowed prefix length and
reference prefix, generateSuffix, and name when adding the checks and error
returns.
Comment on lines
+95
to
+99
| e.mu.Lock() | ||
| namespaces := make([]string, len(e.generatedNamespaces)) | ||
| copy(namespaces, e.generatedNamespaces) | ||
| e.generatedNamespaces = nil | ||
| e.mu.Unlock() |
There was a problem hiding this comment.
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.
Add namespace generation, tracking, and cleanup operations to support dynamic namespace names in eval tasks. createNamespace generates a namespace with a random hex suffix (e.g. vm-test-a1b2c3) and tracks it internally. deleteGeneratedNamespaces cleans up all tracked namespaces, silently ignoring already-deleted ones. Ref: mcpchecker/mcpchecker#213 Signed-off-by: Matthias Wessendorf <mwessend@redhat.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
matzew
force-pushed
the
namespace-generation-tracking
branch
from
February 24, 2026 13:49
f09ab4b to
c0fac97
Compare
|
LGTM |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add namespace generation, tracking, and cleanup operations to support dynamic namespace names in eval tasks. createNamespace generates a namespace with a random hex suffix (e.g. vm-test-a1b2c3) and tracks it internally. deleteGeneratedNamespaces cleans up all tracked namespaces, silently ignoring already-deleted ones.
Ref: mcpchecker/mcpchecker#213
Summary by CodeRabbit
Release Notes
New Features
Tests