fix: identical endpoint name conflicts#1521
Open
akurinnoy wants to merge 14 commits into
Open
Conversation
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
akurinnoy
requested review from
dkwon17,
ibuziuk and
rohanKanojia
as code owners
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
tolusha
reviewed
Oct 8, 2025
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
tolusha
reviewed
Oct 9, 2025
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
tolusha
approved these changes
Oct 9, 2025
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: akurinnoy, tolusha The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
|
New changes are detected. LGTM label has been removed. |
Member
|
I tested the PR with abovementioned steps and it seems to work as expected ✔️ |
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
Collaborator
Author
|
/retest |
dkwon17
reviewed
Nov 3, 2025
| // GetDiscoverableServicesForEndpoints converts the endpoint list into a set of services, each corresponding to a single discoverable | ||
| // endpoint from the list. Endpoints with the NoneEndpointExposure are ignored. | ||
| func GetDiscoverableServicesForEndpoints(endpoints map[string]controllerv1alpha1.EndpointList, meta DevWorkspaceMetadata) []corev1.Service { | ||
| func GetDiscoverableServicesForEndpoints(endpoints map[string]controllerv1alpha1.EndpointList, meta DevWorkspaceMetadata, cl client.Client, log logr.Logger) ([]corev1.Service, error) { |
Collaborator
There was a problem hiding this comment.
@akurinnoy is there a reason for implementing duplicate endpiont detection in both the webhook and the GetDiscoverableServicesForEndpoints function?
Collaborator
Author
There was a problem hiding this comment.
While the webhook prevents the DevWorkspaces (which will cause endpoint conflicts) from being created, the controller checks other cases:
- two workspaces created simultaneously - both of them may pass the webhook validation
- services can be created from other sources
- old workspaces, created before this validation
Is this the answer to your question?
Member
|
@akurinnoy : I ran your claude skill The webhook validation has a logic error that causes it to reject valid workspace creations. Currently, if the current workspace has a discoverable endpoint named X, and another workspace has any endpoint (discoverable OR NOT) named X, we reject it. Steps to reproduce
Do you think Workspace B should be allowed because Workspace A's endpoint is NOT discoverable and doesn't create a service named |
Contributor
|
Hi! I'm che-ai-assistant — I help with your pull requests. I check for new commands every 5m0s (if I am not busy :) ). Available commands:
|
Check discoverability of other workspace's endpoint before reporting a conflict. Previously, a non-discoverable endpoint with the same name would incorrectly trigger a rejection. Assisted-by: Claude Code (claude-opus-4-6) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
📝 WalkthroughWalkthroughThis PR implements conflict detection for DevWorkspace discoverable endpoints to prevent multiple workspaces from claiming the same service name. It refactors solvers to perform in-cluster ownership lookups, adds a new ChangesDiscoverable Endpoint Conflict Detection
Sequence Diagram(s)sequenceDiagram
participant Webhook as Admission Webhook
participant Controller as Routing Controller
participant Solver as DevWorkspace Solver
participant Client as Kubernetes API
Webhook->>Webhook: validateEndpoints: collect discoverable endpoint names
Webhook->>Client: list DevWorkspaces in namespace
Client-->>Webhook: return workspace list
Webhook->>Webhook: compare endpoint names across workspaces
alt Conflict detected
Webhook-->>Webhook: return ServiceConflictError
else No conflict
Webhook-->>Webhook: allow admission
end
Controller->>Solver: GetSolver with logger
Solver->>Solver: NewBasicSolver/NewClusterSolver with client+logger
Controller->>Solver: GetSpecObjects
Solver->>Solver: GetDiscoverableServicesForEndpoints
Solver->>Client: Get existing Service by name
Client-->>Solver: Service or not-found
alt Service exists and belongs to different workspace
Solver-->>Solver: return ServiceConflictError
else Service ok or not found
Solver-->>Solver: create/update Service with ownership labels
end
alt Error returned
Controller->>Controller: markRoutingFailed with conflict message
else Success
Controller->>Controller: continue reconciliation
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The PR spans multiple layers (solvers, controller, webhook) with moderate logic density. The core conflict detection logic is straightforward (metadata lookup and label comparison), but the changes are heterogeneous across different subsystems. Tests are comprehensive and provide good coverage of edge cases. Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@controllers/controller/devworkspacerouting/devworkspacerouting_controller.go`:
- Around line 128-133: The code sets workspaceMeta.DevWorkspaceName =
instance.Name but must use the parent DevWorkspace's name from
instance.OwnerReferences; update the DevWorkspaceMetadata population so
DevWorkspaceName is assigned from the owner reference whose Kind (or
APIVersion+Kind) indicates the parent DevWorkspace (e.g., iterate
instance.OwnerReferences, find ownerRef.Kind == "DevWorkspace" and use
ownerRef.Name), and fall back to instance.Name only if no such ownerRef exists;
change the assignment near workspaceMeta (DevWorkspaceMetadata,
workspaceMeta.DevWorkspaceName) to use that ownerRef-derived name.
In `@controllers/controller/devworkspacerouting/solvers/common_test.go`:
- Around line 16-27: Reorder the import block in common_test.go into three
groups with blank lines between them: (1) standard library (e.g., "testing"),
(2) third-party and Kubernetes packages (e.g.,
"github.com/stretchr/testify/assert", "k8s.io/api/core/v1" as corev1,
"k8s.io/apimachinery/pkg/apis/meta/v1" as metav1,
"k8s.io/apimachinery/pkg/runtime",
"sigs.k8s.io/controller-runtime/pkg/client/fake",
"sigs.k8s.io/controller-runtime/pkg/log/zap"), and (3) project-local imports
(e.g., "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" as
controllerv1alpha1 and
"github.com/devfile/devworkspace-operator/pkg/constants"); insert single blank
lines between these groups and then run 'make fmt' to apply formatting.
In `@webhook/workspace/handler/validate_test.go`:
- Around line 105-124: The test currently asserts a ServiceConflictError when
validateEndpoints (on WebhookHandler) is called with a workspace and a
deletingWorkspace (created via setupWorkspace) that has DeletionTimestamp set;
update the test so it reflects webhook behavior that ignores
deletion-timestamped workspaces: call handler.validateEndpoints(context.TODO(),
workspace) and assert no error (i.e., expect nil) instead of asserting
Error/ErrorAs for *solvers.ServiceConflictError, and remove or replace the
subsequent checks for conflictErr.EndpointName/WorkspaceName accordingly so the
test verifies that deletion-timestamped workspaces do not cause conflicts.
- Around line 16-31: Reorder the import block in the validate_test.go file into
three blank-line-separated groups: (1) standard library imports (context, os,
path/filepath, testing), (2) third-party and Kubernetes imports
(github.com/stretchr/testify/assert, sigs.k8s.io/... , k8s.io/... etc.), and (3)
project-local imports (github.com/devfile/... and your controller packages),
making sure each package stays in its respective group; after rearranging, run
make fmt to apply formatting.
In `@webhook/workspace/handler/validate.go`:
- Around line 81-84: validateEndpoints currently returns raw errors from
h.Client.List which are being appended to devfileErrors and treated as user
validation failures in ValidateDevfile; change validateEndpoints signature to
return (*solvers.ServiceConflictError, error) so real validation conflicts come
back as the typed value and infrastructure/API errors flow via the error return,
update validateEndpoints to return (nil, err) for h.Client.List failures and
(conflictErr, nil) for validation failures, then in ValidateDevfile (and the
other call site around lines 109-112) check the error return first and convert
infrastructure errors into admission.Errored(http.StatusBadRequest, err) (or
propagate the error) instead of appending to devfileErrors, only append
conflictErr.Error() to devfileErrors when the typed conflict value is returned.
- Around line 114-130: The validation loop currently only skips the same
workspace by UID but still inspects workspaces that are being deleted; modify
the loop that iterates workspaceList.Items (and the check around otherWorkspace)
to also skip any otherWorkspace with a non-nil DeletionTimestamp (i.e., treat
terminating workspaces as excluded from discoverable endpoint conflict checks),
so that discoverableEndpoints lookup and potential return of
solvers.ServiceConflictError only considers non-terminating workspaces.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 981c002a-3f92-488e-8077-9057055e9194
📒 Files selected for processing (12)
controllers/controller/devworkspacerouting/devworkspacerouting_controller.gocontrollers/controller/devworkspacerouting/solvers/basic_solver.gocontrollers/controller/devworkspacerouting/solvers/cluster_solver.gocontrollers/controller/devworkspacerouting/solvers/common.gocontrollers/controller/devworkspacerouting/solvers/common_test.gocontrollers/controller/devworkspacerouting/solvers/errors.gocontrollers/controller/devworkspacerouting/solvers/solver.gocontrollers/workspace/suite_test.gopkg/webhook/cluster_roles.gowebhook/workspace/handler/testdata/test-devworkspace.yamlwebhook/workspace/handler/validate.gowebhook/workspace/handler/validate_test.go
Comment on lines
128
to
133
| workspaceMeta := solvers.DevWorkspaceMetadata{ | ||
| DevWorkspaceId: instance.Spec.DevWorkspaceId, | ||
| Namespace: instance.Namespace, | ||
| PodSelector: instance.Spec.PodSelector, | ||
| DevWorkspaceId: instance.Spec.DevWorkspaceId, | ||
| DevWorkspaceName: instance.Name, | ||
| Namespace: instance.Namespace, | ||
| PodSelector: instance.Spec.PodSelector, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm how DevWorkspaceRouting objects are named relative to the parent DevWorkspace.
# Look for routing creation sites in the workspace controller / provision packages.
rg -nP --type=go -C5 '\bDevWorkspaceRouting\s*\{' -g '!**/vendor/**' -g '!**/zz_generated*'
rg -nP --type=go -C3 '\bObjectMeta:\s*metav1\.ObjectMeta\s*\{' -g '*routing*'Repository: devfile/devworkspace-operator
Length of output: 26087
🏁 Script executed:
#!/bin/bash
# Find the common.DevWorkspaceRoutingName function definition
rg -nP --type=go "func.*DevWorkspaceRoutingName" -g '!**/vendor/**'
# Show the context around where workspaceMeta is created in the controller
sed -n '120,140p' controllers/controller/devworkspacerouting/devworkspacerouting_controller.go
# Check if there's a reference to parent DevWorkspace in the routing controller
rg -nP --type=go "DevWorkspace\s*\{" controllers/controller/devworkspacerouting/devworkspacerouting_controller.go -C 3Repository: devfile/devworkspace-operator
Length of output: 879
🏁 Script executed:
#!/bin/bash
# Get the full implementation of DevWorkspaceRoutingName
cat -n pkg/common/naming.go | head -40
# Check if DevWorkspaceRouting object has parent reference or ownerRef pointing to DevWorkspace
rg -nP --type=go "OwnerReferences|ownerRef" controllers/controller/devworkspacerouting/devworkspacerouting_controller.go
# Search for webhook code that validates workspace names
fd webhook -t f --type go | head -5
rg -nP --type=go "otherWorkspace|DevWorkspaceName" --type=go -A 3 -B 3 | head -50Repository: devfile/devworkspace-operator
Length of output: 6551
🏁 Script executed:
#!/bin/bash
# Find webhook validation code that uses DevWorkspaceName
rg -nP --type=go "DevWorkspaceName|validateEndpoints" webhook/ --type go -A 5 -B 2 | head -100
# Check how DevWorkspaceRouting relates to parent DevWorkspace (ownerReferences)
rg -nP --type=go "OwnerReferences" controllers/workspace/ -A 3 -B 1
# Look at the full reconcile method context around line 128-133
sed -n '100,150p' controllers/controller/devworkspacerouting/devworkspacerouting_controller.goRepository: devfile/devworkspace-operator
Length of output: 13643
🏁 Script executed:
#!/bin/bash
# Find where ServiceConflictError is created and how it uses DevWorkspaceName
rg -nP --type=go "ServiceConflictError" pkg/ --type go -B 5 -A 5
# Check how solvers use DevWorkspaceMetadata.DevWorkspaceName
rg -nP --type=go "DevWorkspaceName" pkg/ --type go -C 3
# Find what information is available in the routing controller about the parent workspace
rg -nP --type=go "getDevWorkspace\|FindOwner" controllers/controller/devworkspacerouting/ --type go -C 3Repository: devfile/devworkspace-operator
Length of output: 6411
🏁 Script executed:
#!/bin/bash
# Find ServiceConflictError in solvers package
rg -nP --type=go "type ServiceConflictError\|ServiceConflictError\s*struct" pkg/solvers/ -A 10
# Check solvers package for DevWorkspaceMetadata usage
rg -nP --type=go "DevWorkspaceMetadata" pkg/solvers/ -B 2 -A 8 | head -100
# Look for conflict reporting with workspace names
rg -nP --type=go "OtherWorkspaceName\|ConflictingWorkspaceName\|conflict.*workspace" pkg/ --type go -C 3Repository: devfile/devworkspace-operator
Length of output: 179
🏁 Script executed:
#!/bin/bash
# Find where solvers package is located
find . -name "*.go" -type f | xargs grep -l "type ServiceConflictError" | head -5
# Find solvers imports
rg -nP --type=go "solvers\." controllers/controller/devworkspacerouting/ --type go | head -10
# Search for ServiceConflictError definition
rg -nP --type=go "ServiceConflictError" --type go -B 3 -A 10 | head -100Repository: devfile/devworkspace-operator
Length of output: 11248
🏁 Script executed:
#!/bin/bash
# Check where DevWorkspaceName is used in solvers
rg -nP --type=go "DevWorkspaceName\|OtherWorkspaceName" controllers/controller/devworkspacerouting/solvers/ --type go -B 3 -A 3
# Check if solvers checks for conflicts by comparing workspace metadata names
rg -nP --type=go "metadata\.DevWorkspaceName\|workspaceMeta\.DevWorkspaceName" controllers/controller/devworkspacerouting/solvers/ --type go -B 5 -A 5
# Look at the full errors.go to understand ServiceConflictError
cat -n controllers/controller/devworkspacerouting/solvers/errors.goRepository: devfile/devworkspace-operator
Length of output: 2871
🏁 Script executed:
#!/bin/bash
# Find where ServiceConflictError is instantiated in solvers
rg -nP --type=go "ServiceConflictError\s*\{" controllers/controller/devworkspacerouting/solvers/ --type go -B 5 -A 3
# Check the common_test.go file more fully to understand the concern
sed -n '70,100p' controllers/controller/devworkspacerouting/solvers/common_test.goRepository: devfile/devworkspace-operator
Length of output: 4589
🏁 Script executed:
#!/bin/bash
# Find all usages of DevWorkspaceName in the solvers code
rg -nP --type=go "DevWorkspaceName\|meta\.DevWorkspaceName" controllers/controller/devworkspacerouting/solvers/ --type go -C 3
# Check if DevWorkspaceName is compared with any service labels or workspace names
rg -nP --type=go "\.DevWorkspaceName\s*==" controllers/controller/devworkspacerouting/ --type go -C 5Repository: devfile/devworkspace-operator
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Let's trace how DevWorkspaceName is actually used by checking all references to it
rg -nP --type=go "DevWorkspaceName" controllers/controller/devworkspacerouting/solvers/ --type go
# Check what parameters GetSpecObjects and GetDiscoverableServicesForEndpoints use
rg -nP --type=go "GetSpecObjects\|GetDiscoverableServicesForEndpoints" controllers/controller/devworkspacerouting/solvers/ --type go -B 2 -A 8 | head -60
# Check the full common.go file around the conflict check
sed -n '50,75p' controllers/controller/devworkspacerouting/solvers/common.goRepository: devfile/devworkspace-operator
Length of output: 2144
🏁 Script executed:
#!/bin/bash
# Check if DevWorkspaceRouting has ownerReferences pointing to parent DevWorkspace
sed -n '75,200p' controllers/controller/devworkspacerouting/devworkspacerouting_controller.go | head -80
# Check if we can get parent workspace name from ownerReferences
rg -nP --type=go "GetOwnerReferences\|OwnerReferences" controllers/controller/devworkspacerouting/devworkspacerouting_controller.go --type go -B 2 -A 5
# Check how workspace controller creates the routing with ownerReferences
sed -n '100,120p' pkg/provision/workspace/routing.goRepository: devfile/devworkspace-operator
Length of output: 4073
🏁 Script executed:
#!/bin/bash
# Verify: Check if DevWorkspaceRouting object has ownerReferences that we can use to look up parent workspace
sed -n '105,130p' pkg/provision/workspace/routing.go
# Check if there's any code that looks up the parent DevWorkspace from the routing object
rg -nP --type=go "ownerRef.*DevWorkspace\|GetOwner" controllers/controller/devworkspacerouting/ --type go -C 3
# Verify test data - what are the expected workspace names in tests
sed -n '40,60p' controllers/controller/devworkspacerouting/solvers/common_test.goRepository: devfile/devworkspace-operator
Length of output: 1446
🏁 Script executed:
#!/bin/bash
# Check if there's a way to get parent workspace name from ownerReferences in the routing controller
rg -nP --type=go "for.*ownerRef\|OwnerReferences\|GetOwnerReferences" controllers/controller/devworkspacerouting/devworkspacerouting_controller.go --type go -B 2 -A 8
# Verify: Can we access parent workspace from routing object?
sed -n '1,80p' controllers/controller/devworkspacerouting/devworkspacerouting_controller.go | tail -30Repository: devfile/devworkspace-operator
Length of output: 1583
🏁 Script executed:
#!/bin/bash
# Final verification: check if the parent workspace name is accessible via ownerReferences
# and confirm the exact issue with how names diverge
# Show where SetControllerReference is used
rg -nP --type=go "SetControllerReference" pkg/provision/workspace/routing.go --type go -B 3 -A 3
# Check what happens after ownerReferences are set
sed -n '120,135p' pkg/provision/workspace/routing.go
# Verify the actual behavior: Get parent workspace name from ownerReference
rg -nP --type=go "ownerReferences\|OwnerReferences" apis/controller/v1alpha1/devworkspacerouting_types.go --type go -C 5Repository: devfile/devworkspace-operator
Length of output: 604
🏁 Script executed:
#!/bin/bash
# Final confirmation: Check if we can get parent workspace name from ownerReferences in the routing reconciler
# and verify the actual issue scenario
# Get the full reconcile method to see all available information
sed -n '72,180p' controllers/controller/devworkspacerouting/devworkspacerouting_controller.go | grep -A 5 "ownerRef\|OwnerReferences" || echo "No ownerReferences used in reconciler"
# Check test to confirm expected behavior
rg -nP --type=go "DevWorkspaceName.*routing-\|routing-.*DevWorkspaceName" controllers/controller/devworkspacerouting/ --type go
# Verify the actual naming mismatch by checking what parent workspace name should be
sed -n '40,70p' controllers/workspace/devworkspace_controller_test.goRepository: devfile/devworkspace-operator
Length of output: 1038
🏁 Script executed:
#!/bin/bash
# Let me verify the actual problem: check if parent workspace name is accessible via ownerReferences
# and whether this causes a real issue
# Get the type definition for DevWorkspaceRouting to see ownerReferences field
sed -n '1,100p' apis/controller/v1alpha1/devworkspacerouting_types.go
# Check if there's documentation on how DevWorkspaceName should be set
rg -nP --type=go "DevWorkspaceMetadata" apis/controller/v1alpha1/ --type go -B 5 -A 10Repository: devfile/devworkspace-operator
Length of output: 3967
Use the parent DevWorkspace.Name instead of instance.Name for DevWorkspaceName.
DevWorkspaceRouting objects are created with names like "routing-{workspaceId}" (based on the workspace ID), but the parent DevWorkspace can have any arbitrary name. The code currently sets workspaceMeta.DevWorkspaceName = instance.Name, which passes the routing object's name to solvers for service labeling. This causes a mismatch: services get labeled with the routing name (e.g., "routing-abc123"), but the webhook's conflict reporting uses the actual DevWorkspace.Name (e.g., "my-workspace"). When users see conflict errors, the workspace name attributed to the service owner will not match the actual workspace name, making error messages confusing.
Since DevWorkspaceRouting has an ownerReferences to its parent DevWorkspace, the parent workspace name should be retrieved from there and used for DevWorkspaceName instead of instance.Name.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controllers/controller/devworkspacerouting/devworkspacerouting_controller.go`
around lines 128 - 133, The code sets workspaceMeta.DevWorkspaceName =
instance.Name but must use the parent DevWorkspace's name from
instance.OwnerReferences; update the DevWorkspaceMetadata population so
DevWorkspaceName is assigned from the owner reference whose Kind (or
APIVersion+Kind) indicates the parent DevWorkspace (e.g., iterate
instance.OwnerReferences, find ownerRef.Kind == "DevWorkspace" and use
ownerRef.Name), and fall back to instance.Name only if no such ownerRef exists;
change the assignment near workspaceMeta (DevWorkspaceMetadata,
workspaceMeta.DevWorkspaceName) to use that ownerRef-derived name.
Comment on lines
+16
to
+27
| import ( | ||
| "testing" | ||
|
|
||
| controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" | ||
| "github.com/devfile/devworkspace-operator/pkg/constants" | ||
| "github.com/stretchr/testify/assert" | ||
| corev1 "k8s.io/api/core/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "sigs.k8s.io/controller-runtime/pkg/client/fake" | ||
| "sigs.k8s.io/controller-runtime/pkg/log/zap" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Organize imports into three groups per coding guidelines.
The imports should be organized into three groups separated by blank lines: (1) standard library, (2) third-party + Kubernetes, (3) project-local. As per coding guidelines, run 'make fmt' to enforce automatically.
♻️ Proposed fix
import (
"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"
+ "sigs.k8s.io/controller-runtime/pkg/client/fake"
+ "sigs.k8s.io/controller-runtime/pkg/log/zap"
+
controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1"
"github.com/devfile/devworkspace-operator/pkg/constants"
- "github.com/stretchr/testify/assert"
- corev1 "k8s.io/api/core/v1"
- metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
- "k8s.io/apimachinery/pkg/runtime"
- "sigs.k8s.io/controller-runtime/pkg/client/fake"
- "sigs.k8s.io/controller-runtime/pkg/log/zap"
)📝 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
| import ( | |
| "testing" | |
| controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" | |
| "github.com/devfile/devworkspace-operator/pkg/constants" | |
| "github.com/stretchr/testify/assert" | |
| corev1 "k8s.io/api/core/v1" | |
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |
| "k8s.io/apimachinery/pkg/runtime" | |
| "sigs.k8s.io/controller-runtime/pkg/client/fake" | |
| "sigs.k8s.io/controller-runtime/pkg/log/zap" | |
| ) | |
| import ( | |
| "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" | |
| "sigs.k8s.io/controller-runtime/pkg/client/fake" | |
| "sigs.k8s.io/controller-runtime/pkg/log/zap" | |
| controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" | |
| "github.com/devfile/devworkspace-operator/pkg/constants" | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controllers/controller/devworkspacerouting/solvers/common_test.go` around
lines 16 - 27, Reorder the import block in common_test.go into three groups with
blank lines between them: (1) standard library (e.g., "testing"), (2)
third-party and Kubernetes packages (e.g., "github.com/stretchr/testify/assert",
"k8s.io/api/core/v1" as corev1, "k8s.io/apimachinery/pkg/apis/meta/v1" as
metav1, "k8s.io/apimachinery/pkg/runtime",
"sigs.k8s.io/controller-runtime/pkg/client/fake",
"sigs.k8s.io/controller-runtime/pkg/log/zap"), and (3) project-local imports
(e.g., "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" as
controllerv1alpha1 and
"github.com/devfile/devworkspace-operator/pkg/constants"); insert single blank
lines between these groups and then run 'make fmt' to apply formatting.
Comment on lines
+16
to
+31
| import ( | ||
| "context" | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| dwv2 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" | ||
| "github.com/devfile/devworkspace-operator/controllers/controller/devworkspacerouting/solvers" | ||
| "github.com/stretchr/testify/assert" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| "sigs.k8s.io/controller-runtime/pkg/client/fake" | ||
| "sigs.k8s.io/yaml" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Organize imports into three groups per coding guidelines.
The imports should be organized into three groups separated by blank lines: (1) standard library, (2) third-party + Kubernetes, (3) project-local. As per coding guidelines, run 'make fmt' to enforce automatically.
♻️ Proposed fix
import (
"context"
"os"
"path/filepath"
"testing"
- dwv2 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
- "github.com/devfile/devworkspace-operator/controllers/controller/devworkspacerouting/solvers"
"github.com/stretchr/testify/assert"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/yaml"
+
+ dwv2 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
+ "github.com/devfile/devworkspace-operator/controllers/controller/devworkspacerouting/solvers"
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webhook/workspace/handler/validate_test.go` around lines 16 - 31, Reorder the
import block in the validate_test.go file into three blank-line-separated
groups: (1) standard library imports (context, os, path/filepath, testing), (2)
third-party and Kubernetes imports (github.com/stretchr/testify/assert,
sigs.k8s.io/... , k8s.io/... etc.), and (3) project-local imports
(github.com/devfile/... and your controller packages), making sure each package
stays in its respective group; after rearranging, run make fmt to apply
formatting.
Comment on lines
+105
to
+124
| t.Run("Conflict detected even when workspace is being deleted", func(t *testing.T) { | ||
| workspace := setupWorkspace(t, "workspace-1", "uid-1", "test-namespace") | ||
|
|
||
| // Workspace being deleted with same endpoint name | ||
| deletingWorkspace := setupWorkspace(t, "workspace-deleting", "uid-deleting", "test-namespace") | ||
| now := metav1.Now() | ||
| deletingWorkspace.DeletionTimestamp = &now | ||
| // Add finalizer - required by fake client when setting deletionTimestamp | ||
| deletingWorkspace.Finalizers = []string{"test-finalizer"} | ||
|
|
||
| fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(deletingWorkspace).Build() | ||
| handler := &WebhookHandler{Client: fakeClient} | ||
| err := handler.validateEndpoints(context.TODO(), workspace) | ||
| assert.Error(t, err, "Should detect conflict even with workspace being deleted") | ||
|
|
||
| var conflictErr *solvers.ServiceConflictError | ||
| assert.ErrorAs(t, err, &conflictErr, "Error should be a ServiceConflictError") | ||
| assert.Equal(t, "test-endpoint", conflictErr.EndpointName, "Conflict should be on 'test-endpoint'") | ||
| assert.Equal(t, "workspace-deleting", conflictErr.WorkspaceName, "Conflict should reference 'workspace-deleting'") | ||
| }) |
There was a problem hiding this comment.
Test expectation contradicts the expected webhook behavior.
According to the review stack context, the webhook validation should exclude deletion-timestamped workspaces from conflict detection (layer 5: "excluding same UID and deletion-timestamped workspaces"). However, this test expects a ServiceConflictError when validating against a workspace with a DeletionTimestamp.
The test should expect no error because workspaces being deleted should not block new workspace creation.
🔧 Proposed fix
- t.Run("Conflict detected even when workspace is being deleted", func(t *testing.T) {
+ t.Run("No conflict when other workspace is being deleted", func(t *testing.T) {
workspace := setupWorkspace(t, "workspace-1", "uid-1", "test-namespace")
// Workspace being deleted with same endpoint name
deletingWorkspace := setupWorkspace(t, "workspace-deleting", "uid-deleting", "test-namespace")
now := metav1.Now()
deletingWorkspace.DeletionTimestamp = &now
// Add finalizer - required by fake client when setting deletionTimestamp
deletingWorkspace.Finalizers = []string{"test-finalizer"}
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(deletingWorkspace).Build()
handler := &WebhookHandler{Client: fakeClient}
err := handler.validateEndpoints(context.TODO(), workspace)
- assert.Error(t, err, "Should detect conflict even with workspace being deleted")
-
- var conflictErr *solvers.ServiceConflictError
- assert.ErrorAs(t, err, &conflictErr, "Error should be a ServiceConflictError")
- assert.Equal(t, "test-endpoint", conflictErr.EndpointName, "Conflict should be on 'test-endpoint'")
- assert.Equal(t, "workspace-deleting", conflictErr.WorkspaceName, "Conflict should reference 'workspace-deleting'")
+ assert.NoError(t, err, "Should not conflict with workspace being deleted")
})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webhook/workspace/handler/validate_test.go` around lines 105 - 124, The test
currently asserts a ServiceConflictError when validateEndpoints (on
WebhookHandler) is called with a workspace and a deletingWorkspace (created via
setupWorkspace) that has DeletionTimestamp set; update the test so it reflects
webhook behavior that ignores deletion-timestamped workspaces: call
handler.validateEndpoints(context.TODO(), workspace) and assert no error (i.e.,
expect nil) instead of asserting Error/ErrorAs for
*solvers.ServiceConflictError, and remove or replace the subsequent checks for
conflictErr.EndpointName/WorkspaceName accordingly so the test verifies that
deletion-timestamped workspaces do not cause conflicts.
Comment on lines
+81
to
+84
| endpointErrors := h.validateEndpoints(ctx, wksp) | ||
| if endpointErrors != nil { | ||
| devfileErrors = append(devfileErrors, endpointErrors.Error()) | ||
| } |
There was a problem hiding this comment.
Don't report API/list failures as devfile validation errors.
When h.Client.List fails (API server hiccup, RBAC misconfiguration, context cancellation, etc.), validateEndpoints returns that raw error. In ValidateDevfile it is then appended to devfileErrors and surfaced to the user via admission.Denied(...) as if the user's devfile were invalid. Per the webhook handling guidelines, infrastructure errors should be returned via admission.Errored(...) so the API server can retry/surface them appropriately, and the user is not shown a misleading validation message.
As per coding guidelines: "For validating webhooks, return 'admission.Denied()' when rejecting requests, 'admission.Allowed()' when accepting, and 'admission.Errored(http.StatusBadRequest, err)' for decode errors"
🛠️ Suggested approach
One option is to distinguish list errors from validation errors at the call site:
- endpointErrors := h.validateEndpoints(ctx, wksp)
- if endpointErrors != nil {
- devfileErrors = append(devfileErrors, endpointErrors.Error())
- }
+ conflict, err := h.validateEndpoints(ctx, wksp)
+ if err != nil {
+ return admission.Errored(http.StatusInternalServerError, err)
+ }
+ if conflict != nil {
+ devfileErrors = append(devfileErrors, conflict.Error())
+ }And have validateEndpoints return (*solvers.ServiceConflictError, error) so a List failure flows through err while a real conflict flows through the typed value.
Also applies to: 109-112
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webhook/workspace/handler/validate.go` around lines 81 - 84,
validateEndpoints currently returns raw errors from h.Client.List which are
being appended to devfileErrors and treated as user validation failures in
ValidateDevfile; change validateEndpoints signature to return
(*solvers.ServiceConflictError, error) so real validation conflicts come back as
the typed value and infrastructure/API errors flow via the error return, update
validateEndpoints to return (nil, err) for h.Client.List failures and
(conflictErr, nil) for validation failures, then in ValidateDevfile (and the
other call site around lines 109-112) check the error return first and convert
infrastructure errors into admission.Errored(http.StatusBadRequest, err) (or
propagate the error) instead of appending to devfileErrors, only append
conflictErr.Error() to devfileErrors when the typed conflict value is returned.
Comment on lines
+114
to
+130
| for _, otherWorkspace := range workspaceList.Items { | ||
| if otherWorkspace.UID == workspace.UID { | ||
| continue | ||
| } | ||
| for _, component := range otherWorkspace.Spec.Template.Components { | ||
| if component.Container != nil { | ||
| for _, endpoint := range component.Container.Endpoints { | ||
| if discoverableEndpoints[endpoint.Name] && endpoint.Attributes.GetBoolean(string(v1alpha1.DiscoverableAttribute), nil) { | ||
| return &solvers.ServiceConflictError{ | ||
| EndpointName: endpoint.Name, | ||
| WorkspaceName: otherWorkspace.Name, | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Skip workspaces that are being deleted.
Only UID equality is checked when filtering out the requesting workspace. A DevWorkspace that is mid-deletion (i.e. DeletionTimestamp != nil) will still be in the list returned by h.Client.List and will cause this webhook to reject a fresh workspace that reuses the same discoverable endpoint name — even though the old service is on its way out. This both contradicts the AI summary ("excluding same UID and deletion-timestamped workspaces") and the PR's stated intent in the commit history ("adds a check for terminating workspaces"), and it produces a bad UX (users must wait for full deletion before they can re-create).
🐛 Proposed fix
for _, otherWorkspace := range workspaceList.Items {
- if otherWorkspace.UID == workspace.UID {
+ if otherWorkspace.UID == workspace.UID {
+ continue
+ }
+ if otherWorkspace.GetDeletionTimestamp() != nil {
continue
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webhook/workspace/handler/validate.go` around lines 114 - 130, The validation
loop currently only skips the same workspace by UID but still inspects
workspaces that are being deleted; modify the loop that iterates
workspaceList.Items (and the check around otherWorkspace) to also skip any
otherWorkspace with a non-nil DeletionTimestamp (i.e., treat terminating
workspaces as excluded from discoverable endpoint conflict checks), so that
discoverableEndpoints lookup and potential return of
solvers.ServiceConflictError only considers non-terminating workspaces.
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
4 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.
What does this PR do?
This PR implements conflict detection and prevention at both the controller and webhook levels.
What issues does this PR fix or reference?
fixes eclipse-che/che#23231
Is it tested? How?
Same-namespace conflict detection
oc wait --for=condition=Ready dw/endpoint-test-1 -n test-namespace --timeout=300sExpected result:
Cross-name isolation
PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with CheSummary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests