From 04aa51a7c09847946319a74c7bbc8b1e0669f50f Mon Sep 17 00:00:00 2001 From: Oleksii Kurinnyi Date: Thu, 2 Oct 2025 13:36:48 +0300 Subject: [PATCH 01/11] fix: identical endpoint name conflicts Signed-off-by: Oleksii Kurinnyi --- .../devworkspacerouting_controller.go | 17 +- .../solvers/basic_solver.go | 10 +- .../solvers/cluster_solver.go | 9 +- .../devworkspacerouting/solvers/common.go | 38 +++- .../solvers/common_test.go | 203 ++++++++++++++++++ .../devworkspacerouting/solvers/errors.go | 27 +++ .../devworkspacerouting/solvers/solver.go | 3 +- .../devworkspacerouting/sync_ingresses.go | 31 +++ .../devworkspacerouting/sync_routes.go | 27 +++ controllers/workspace/suite_test.go | 4 + pkg/webhook/cluster_roles.go | 13 ++ .../handler/testdata/test-devworkspace.yaml | 16 ++ webhook/workspace/handler/validate.go | 50 +++++ webhook/workspace/handler/validate_test.go | 179 +++++++++++++++ 14 files changed, 615 insertions(+), 12 deletions(-) create mode 100644 controllers/controller/devworkspacerouting/solvers/common_test.go create mode 100644 webhook/workspace/handler/testdata/test-devworkspace.yaml create mode 100644 webhook/workspace/handler/validate_test.go diff --git a/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go b/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go index 367da93d0..deaed827f 100644 --- a/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go +++ b/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go @@ -96,6 +96,7 @@ func (r *DevWorkspaceRoutingReconciler) Reconcile(ctx context.Context, req ctrl. solver, err := r.SolverGetter.GetSolver(r.Client, instance.Spec.RoutingClass) if err != nil { if errors.Is(err, solvers.RoutingNotSupported) { + reqLogger.Info("Routing class not supported by this controller, skipping reconciliation", "routingClass", instance.Spec.RoutingClass) return reconcile.Result{}, nil } return reconcile.Result{}, r.markRoutingFailed(instance, fmt.Sprintf("Invalid routingClass for DevWorkspace: %s", err)) @@ -131,7 +132,7 @@ func (r *DevWorkspaceRoutingReconciler) Reconcile(ctx context.Context, req ctrl. } restrictedAccess, setRestrictedAccess := instance.Annotations[constants.DevWorkspaceRestrictedAccessAnnotation] - routingObjects, err := solver.GetSpecObjects(instance, workspaceMeta) + routingObjects, err := solver.GetSpecObjects(instance, workspaceMeta, r.Client, reqLogger) if err != nil { var notReady *solvers.RoutingNotReady if errors.As(err, ¬Ready) { @@ -149,6 +150,12 @@ func (r *DevWorkspaceRoutingReconciler) Reconcile(ctx context.Context, req ctrl. return reconcile.Result{}, r.markRoutingFailed(instance, fmt.Sprintf("Unable to provision networking for DevWorkspace: %s", invalid)) } + var conflict *solvers.ServiceConflictError + if errors.As(err, &conflict) { + reqLogger.Error(conflict, "Routing controller detected a service conflict", "serviceName", conflict.Reason) + return reconcile.Result{}, r.markRoutingFailed(instance, fmt.Sprintf("Unable to provision networking for DevWorkspace: %s", conflict)) + } + // generic error, just fail the reconciliation return reconcile.Result{}, err } @@ -208,6 +215,10 @@ func (r *DevWorkspaceRoutingReconciler) Reconcile(ctx context.Context, req ctrl. if errors.As(err, &failError) { return reconcile.Result{}, r.markRoutingFailed(instance, err.Error()) } + conflictErr := &solvers.HostnameConflictError{} + if errors.As(err, &conflictErr) { + return reconcile.Result{}, r.markRoutingFailed(instance, conflictErr.Error()) + } reqLogger.Error(err, "Error syncing routes") return reconcile.Result{Requeue: true}, r.reconcileStatus(instance, nil, nil, false, "Preparing routes") } else if !routesInSync { @@ -222,6 +233,10 @@ func (r *DevWorkspaceRoutingReconciler) Reconcile(ctx context.Context, req ctrl. if errors.As(err, &failError) { return reconcile.Result{}, r.markRoutingFailed(instance, err.Error()) } + conflictErr := &solvers.HostnameConflictError{} + if errors.As(err, &conflictErr) { + return reconcile.Result{}, r.markRoutingFailed(instance, conflictErr.Error()) + } reqLogger.Error(err, "Error syncing ingresses") return reconcile.Result{Requeue: true}, r.reconcileStatus(instance, nil, nil, false, "Preparing ingresses") } else if !ingressesInSync { diff --git a/controllers/controller/devworkspacerouting/solvers/basic_solver.go b/controllers/controller/devworkspacerouting/solvers/basic_solver.go index 50df27fa1..8bc418ccc 100644 --- a/controllers/controller/devworkspacerouting/solvers/basic_solver.go +++ b/controllers/controller/devworkspacerouting/solvers/basic_solver.go @@ -20,6 +20,8 @@ import ( "github.com/devfile/devworkspace-operator/pkg/config" "github.com/devfile/devworkspace-operator/pkg/constants" "github.com/devfile/devworkspace-operator/pkg/infrastructure" + "github.com/go-logr/logr" + "sigs.k8s.io/controller-runtime/pkg/client" ) var routeAnnotations = func(endpointName string, endpointAnnotations map[string]string) map[string]string { @@ -59,7 +61,7 @@ func (s *BasicSolver) Finalize(*controllerv1alpha1.DevWorkspaceRouting) error { return nil } -func (s *BasicSolver) GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRouting, workspaceMeta DevWorkspaceMetadata) (RoutingObjects, error) { +func (s *BasicSolver) GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRouting, workspaceMeta DevWorkspaceMetadata, cl client.Client, log logr.Logger) (RoutingObjects, error) { routingObjects := RoutingObjects{} // TODO: Use workspace-scoped ClusterHostSuffix to allow overriding @@ -70,7 +72,11 @@ func (s *BasicSolver) GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRou spec := routing.Spec services := getServicesForEndpoints(spec.Endpoints, workspaceMeta) - services = append(services, GetDiscoverableServicesForEndpoints(spec.Endpoints, workspaceMeta)...) + discoverableServices, err := GetDiscoverableServicesForEndpoints(spec.Endpoints, workspaceMeta, cl, log) + if err != nil { + return RoutingObjects{}, err + } + services = append(services, discoverableServices...) routingObjects.Services = services if infrastructure.IsOpenShift() { routingObjects.Routes = getRoutesForSpec(routingSuffix, spec.Endpoints, workspaceMeta) diff --git a/controllers/controller/devworkspacerouting/solvers/cluster_solver.go b/controllers/controller/devworkspacerouting/solvers/cluster_solver.go index 7c5462252..e6d0bc71b 100644 --- a/controllers/controller/devworkspacerouting/solvers/cluster_solver.go +++ b/controllers/controller/devworkspacerouting/solvers/cluster_solver.go @@ -24,6 +24,8 @@ import ( corev1 "k8s.io/api/core/v1" controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" + "github.com/go-logr/logr" + "sigs.k8s.io/controller-runtime/pkg/client" ) const ( @@ -44,9 +46,14 @@ func (s *ClusterSolver) Finalize(*controllerv1alpha1.DevWorkspaceRouting) error return nil } -func (s *ClusterSolver) GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRouting, workspaceMeta DevWorkspaceMetadata) (RoutingObjects, error) { +func (s *ClusterSolver) GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRouting, workspaceMeta DevWorkspaceMetadata, cl client.Client, log logr.Logger) (RoutingObjects, error) { spec := routing.Spec services := getServicesForEndpoints(spec.Endpoints, workspaceMeta) + discoverableServices, err := GetDiscoverableServicesForEndpoints(spec.Endpoints, workspaceMeta, cl, log) + if err != nil { + return RoutingObjects{}, err + } + services = append(services, discoverableServices...) podAdditions := &controllerv1alpha1.PodAdditions{} if s.TLS { readOnlyMode := int32(420) diff --git a/controllers/controller/devworkspacerouting/solvers/common.go b/controllers/controller/devworkspacerouting/solvers/common.go index b55367ad8..fcc642429 100644 --- a/controllers/controller/devworkspacerouting/solvers/common.go +++ b/controllers/controller/devworkspacerouting/solvers/common.go @@ -16,9 +16,15 @@ package solvers import ( + "context" + "fmt" + controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" "github.com/devfile/devworkspace-operator/pkg/common" "github.com/devfile/devworkspace-operator/pkg/constants" + "github.com/go-logr/logr" + "k8s.io/apimachinery/pkg/api/errors" + "sigs.k8s.io/controller-runtime/pkg/client" routeV1 "github.com/openshift/api/route/v1" corev1 "k8s.io/api/core/v1" @@ -36,7 +42,7 @@ type DevWorkspaceMetadata struct { // 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) { var services []corev1.Service for _, machineEndpoints := range endpoints { for _, endpoint := range machineEndpoints { @@ -45,18 +51,36 @@ func GetDiscoverableServicesForEndpoints(endpoints map[string]controllerv1alpha1 } if endpoint.Attributes.GetBoolean(string(controllerv1alpha1.DiscoverableAttribute), nil) { - // Create service with name matching endpoint - // TODO: This could cause a reconcile conflict if multiple workspaces define the same discoverable endpoint - // Also endpoint names may not be valid as service names + serviceName := common.EndpointName(endpoint.Name) + log.Info("Checking for existing service for discoverable endpoint", "serviceName", serviceName) + existingService := &corev1.Service{} + err := cl.Get(context.TODO(), client.ObjectKey{Name: serviceName, Namespace: meta.Namespace}, existingService) + if err != nil { + if !errors.IsNotFound(err) { + log.Error(err, "Failed to get service from cluster", "serviceName", serviceName) + return nil, err + } + log.Info("No existing service found", "serviceName", serviceName) + } else { + log.Info("Found existing service", "serviceName", serviceName) + if existingService.Labels[constants.DevWorkspaceIDLabel] != meta.DevWorkspaceId { + log.Info("Service conflict detected", "serviceName", serviceName, "existingWorkspaceId", existingService.Labels[constants.DevWorkspaceIDLabel], "currentWorkspaceId", meta.DevWorkspaceId) + return nil, &ServiceConflictError{ + Reason: fmt.Sprintf("discoverable endpoint %s conflicts with existing service", endpoint.Name), + } + } + log.Info("Existing service is owned by the same workspace", "serviceName", serviceName) + } + servicePort := corev1.ServicePort{ - Name: common.EndpointName(endpoint.Name), + Name: serviceName, Protocol: corev1.ProtocolTCP, Port: int32(endpoint.TargetPort), TargetPort: intstr.FromInt(endpoint.TargetPort), } services = append(services, corev1.Service{ ObjectMeta: metav1.ObjectMeta{ - Name: common.EndpointName(endpoint.Name), + Name: serviceName, Namespace: meta.Namespace, Labels: map[string]string{ constants.DevWorkspaceIDLabel: meta.DevWorkspaceId, @@ -74,7 +98,7 @@ func GetDiscoverableServicesForEndpoints(endpoints map[string]controllerv1alpha1 } } } - return services + return services, nil } // GetServiceForEndpoints returns a single service that exposes all endpoints of given exposure types, possibly also including the discoverable types. diff --git a/controllers/controller/devworkspacerouting/solvers/common_test.go b/controllers/controller/devworkspacerouting/solvers/common_test.go new file mode 100644 index 000000000..65882cb5d --- /dev/null +++ b/controllers/controller/devworkspacerouting/solvers/common_test.go @@ -0,0 +1,203 @@ +package solvers + +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" +) + +func TestGetDiscoverableServicesForEndpoints(t *testing.T) { + testLog := zap.New(zap.UseDevMode(true)) + scheme := runtime.NewScheme() + _ = corev1.AddToScheme(scheme) + _ = controllerv1alpha1.AddToScheme(scheme) + + discoverableEndpoint := controllerv1alpha1.Endpoint{ + Name: "test-endpoint", + TargetPort: 8080, + Exposure: controllerv1alpha1.PublicEndpointExposure, + Attributes: controllerv1alpha1.Attributes{}. + PutBoolean(string(controllerv1alpha1.DiscoverableAttribute), true), + } + endpoints := map[string]controllerv1alpha1.EndpointList{ + "machine1": {discoverableEndpoint}, + } + + meta := DevWorkspaceMetadata{ + DevWorkspaceId: "current-workspace", + Namespace: "test-namespace", + } + + tests := []struct { + name string + existing []runtime.Object + expectErr bool + expectErrType error + expectMsg string + }{ + { + name: "No existing service", + existing: []runtime.Object{}, + expectErr: false, + }, + { + name: "Existing service with different owner in same namespace", + existing: []runtime.Object{ + &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-endpoint", + Namespace: "test-namespace", + Labels: map[string]string{ + constants.DevWorkspaceIDLabel: "other-workspace", + }, + }, + }, + }, + expectErr: true, + expectErrType: &ServiceConflictError{}, + expectMsg: "conflicts with existing service", + }, + { + name: "Existing service with same owner (reconciliation)", + existing: []runtime.Object{ + &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-endpoint", + Namespace: "test-namespace", + Labels: map[string]string{ + constants.DevWorkspaceIDLabel: "current-workspace", + }, + }, + }, + }, + expectErr: false, + }, + { + name: "Service with same name in different namespace (should not conflict)", + existing: []runtime.Object{ + &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-endpoint", + Namespace: "other-namespace", + Labels: map[string]string{ + constants.DevWorkspaceIDLabel: "other-workspace", + }, + }, + }, + }, + expectErr: false, + }, + { + name: "Service without workspace ID label", + existing: []runtime.Object{ + &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-endpoint", + Namespace: "test-namespace", + Labels: map[string]string{}, + }, + }, + }, + expectErr: true, + expectErrType: &ServiceConflictError{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(tt.existing...).Build() + _, err := GetDiscoverableServicesForEndpoints(endpoints, meta, fakeClient, testLog) + + if tt.expectErr { + assert.Error(t, err, "Expected an error but got none") + if tt.expectErrType != nil { + assert.IsType(t, tt.expectErrType, err, "Error is of unexpected type") + } + if tt.expectMsg != "" { + assert.Contains(t, err.Error(), tt.expectMsg) + } + } else { + assert.NoError(t, err, "Got unexpected error") + } + }) + } +} + +func TestGetDiscoverableServicesForEndpoints_MultipleEndpoints(t *testing.T) { + testLog := zap.New(zap.UseDevMode(true)) + scheme := runtime.NewScheme() + _ = corev1.AddToScheme(scheme) + _ = controllerv1alpha1.AddToScheme(scheme) + + endpoints := map[string]controllerv1alpha1.EndpointList{ + "machine1": { + { + Name: "postgresql", + TargetPort: 5432, + Exposure: controllerv1alpha1.InternalEndpointExposure, + Attributes: controllerv1alpha1.Attributes{}. + PutBoolean(string(controllerv1alpha1.DiscoverableAttribute), true), + }, + { + Name: "redis", + TargetPort: 6379, + Exposure: controllerv1alpha1.InternalEndpointExposure, + Attributes: controllerv1alpha1.Attributes{}. + PutBoolean(string(controllerv1alpha1.DiscoverableAttribute), true), + }, + { + Name: "http", + TargetPort: 8080, + Exposure: controllerv1alpha1.PublicEndpointExposure, + // Not discoverable + }, + }, + } + + meta := DevWorkspaceMetadata{ + DevWorkspaceId: "current-workspace", + Namespace: "test-namespace", + PodSelector: map[string]string{ + constants.DevWorkspaceIDLabel: "current-workspace", + }, + } + + t.Run("Multiple discoverable endpoints without conflicts", func(t *testing.T) { + fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build() + services, err := GetDiscoverableServicesForEndpoints(endpoints, meta, fakeClient, testLog) + assert.NoError(t, err) + assert.Len(t, services, 2, "Should create 2 discoverable services (postgresql and redis, not http)") + + serviceNames := make(map[string]bool) + for _, svc := range services { + serviceNames[svc.Name] = true + } + assert.True(t, serviceNames["postgresql"], "Should have postgresql service") + assert.True(t, serviceNames["redis"], "Should have redis service") + assert.False(t, serviceNames["http"], "Should not have http service (not discoverable)") + }) + + t.Run("Conflict on one of multiple endpoints", func(t *testing.T) { + existingService := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "postgresql", + Namespace: "test-namespace", + Labels: map[string]string{ + constants.DevWorkspaceIDLabel: "other-workspace", + }, + }, + } + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(existingService).Build() + _, err := GetDiscoverableServicesForEndpoints(endpoints, meta, fakeClient, testLog) + assert.Error(t, err, "Should error when one endpoint conflicts") + assert.IsType(t, &ServiceConflictError{}, err) + assert.Contains(t, err.Error(), "postgresql") + }) +} diff --git a/controllers/controller/devworkspacerouting/solvers/errors.go b/controllers/controller/devworkspacerouting/solvers/errors.go index ffcdd7b7b..2d0c07348 100644 --- a/controllers/controller/devworkspacerouting/solvers/errors.go +++ b/controllers/controller/devworkspacerouting/solvers/errors.go @@ -21,7 +21,34 @@ import ( ) var _ error = (*RoutingNotReady)(nil) + +// HostnameConflictError is an error returned when a hostname required by a DevWorkspace is already in use +// by another resource on the cluster. +type HostnameConflictError struct { + // Reason provides a textual summary of the conflict + Reason string +} + +func (e *HostnameConflictError) Error() string { + return e.Reason +} + var _ error = (*RoutingInvalid)(nil) +var _ error = (*ServiceConflictError)(nil) + +// ServiceConflictError is returned when a discoverable endpoint has a name that is already in use by +// another DevWorkspace's service. +type ServiceConflictError struct { + Reason string +} + +func (e *ServiceConflictError) Error() string { + reason := "" + if len(e.Reason) > 0 { + reason = e.Reason + } + return "workspace routing has a service conflict: " + reason +} // RoutingNotSupported is used by the solvers when they supported the routingclass of the workspace they've been asked to route var RoutingNotSupported = errors.New("routingclass not supported by this controller") diff --git a/controllers/controller/devworkspacerouting/solvers/solver.go b/controllers/controller/devworkspacerouting/solvers/solver.go index c5c5a5c87..2d278f579 100644 --- a/controllers/controller/devworkspacerouting/solvers/solver.go +++ b/controllers/controller/devworkspacerouting/solvers/solver.go @@ -18,6 +18,7 @@ package solvers import ( "fmt" + "github.com/go-logr/logr" routeV1 "github.com/openshift/api/route/v1" corev1 "k8s.io/api/core/v1" networkingv1 "k8s.io/api/networking/v1" @@ -52,7 +53,7 @@ type RoutingSolver interface { // The implementors can also create any additional objects not captured by the RoutingObjects struct. If that's // the case they are required to set the restricted access annotation on any objects created according to the // restricted access specified by the routing. - GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRouting, workspaceMeta DevWorkspaceMetadata) (RoutingObjects, error) + GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRouting, workspaceMeta DevWorkspaceMetadata, client client.Client, log logr.Logger) (RoutingObjects, error) // GetExposedEndpoints retreives the URL for each endpoint in a devfile spec from a set of RoutingObjects. // Returns is a map from component ids (as defined in the devfile) to the list of endpoints for that component diff --git a/controllers/controller/devworkspacerouting/sync_ingresses.go b/controllers/controller/devworkspacerouting/sync_ingresses.go index 6d21fae5a..ba1ce5b4c 100644 --- a/controllers/controller/devworkspacerouting/sync_ingresses.go +++ b/controllers/controller/devworkspacerouting/sync_ingresses.go @@ -19,6 +19,7 @@ import ( "context" "fmt" + "github.com/devfile/devworkspace-operator/controllers/controller/devworkspacerouting/solvers" "github.com/devfile/devworkspace-operator/pkg/constants" "github.com/devfile/devworkspace-operator/pkg/provision/sync" @@ -32,6 +33,14 @@ import ( func (r *DevWorkspaceRoutingReconciler) syncIngresses(routing *controllerv1alpha1.DevWorkspaceRouting, specIngresses []networkingv1.Ingress) (ok bool, clusterIngresses []networkingv1.Ingress, err error) { ingressesInSync := true + for _, ingress := range specIngresses { + for _, rule := range ingress.Spec.Rules { + if err := r.checkForIngressConflict(rule.Host, routing); err != nil { + return false, nil, err + } + } + } + clusterIngresses, err = r.getClusterIngresses(routing) if err != nil { return false, nil, err @@ -90,6 +99,28 @@ func (r *DevWorkspaceRoutingReconciler) getClusterIngresses(routing *controllerv return found.Items, nil } +func (r *DevWorkspaceRoutingReconciler) checkForIngressConflict(hostname string, owner *controllerv1alpha1.DevWorkspaceRouting) error { + ingresses := &networkingv1.IngressList{} + if err := r.List(context.TODO(), ingresses, client.InNamespace(owner.Namespace)); err != nil { + return err + } + for _, ingress := range ingresses.Items { + for _, rule := range ingress.Spec.Rules { + if rule.Host == hostname { + for _, ownerRef := range ingress.GetOwnerReferences() { + if ownerRef.UID == owner.UID { + return nil + } + } + return &solvers.HostnameConflictError{ + Reason: fmt.Sprintf("hostname %s is already in use by ingress %s", hostname, ingress.Name), + } + } + } + } + return nil +} + func getIngressesToDelete(clusterIngresses, specIngresses []networkingv1.Ingress) []networkingv1.Ingress { var toDelete []networkingv1.Ingress for _, clusterIngress := range clusterIngresses { diff --git a/controllers/controller/devworkspacerouting/sync_routes.go b/controllers/controller/devworkspacerouting/sync_routes.go index 4a542a140..ed2f82a27 100644 --- a/controllers/controller/devworkspacerouting/sync_routes.go +++ b/controllers/controller/devworkspacerouting/sync_routes.go @@ -19,6 +19,7 @@ import ( "context" "fmt" + "github.com/devfile/devworkspace-operator/controllers/controller/devworkspacerouting/solvers" "github.com/devfile/devworkspace-operator/pkg/constants" "github.com/devfile/devworkspace-operator/pkg/provision/sync" routeV1 "github.com/openshift/api/route/v1" @@ -31,6 +32,12 @@ import ( func (r *DevWorkspaceRoutingReconciler) syncRoutes(routing *controllerv1alpha1.DevWorkspaceRouting, specRoutes []routeV1.Route) (ok bool, clusterRoutes []routeV1.Route, err error) { routesInSync := true + for _, route := range specRoutes { + if err := r.checkForRouteConflict(route.Spec.Host, routing); err != nil { + return false, nil, err + } + } + clusterRoutes, err = r.getClusterRoutes(routing) if err != nil { return false, nil, err @@ -100,6 +107,26 @@ func (r *DevWorkspaceRoutingReconciler) getClusterRoutes(routing *controllerv1al return routes, nil } +func (r *DevWorkspaceRoutingReconciler) checkForRouteConflict(hostname string, owner *controllerv1alpha1.DevWorkspaceRouting) error { + routes := &routeV1.RouteList{} + if err := r.List(context.TODO(), routes, client.InNamespace(owner.Namespace)); err != nil { + return err + } + for _, route := range routes.Items { + if route.Spec.Host == hostname { + for _, ownerRef := range route.GetOwnerReferences() { + if ownerRef.UID == owner.UID { + return nil + } + } + return &solvers.HostnameConflictError{ + Reason: fmt.Sprintf("hostname %s is already in use by route %s", hostname, route.Name), + } + } + } + return nil +} + func getRoutesToDelete(clusterRoutes, specRoutes []routeV1.Route) []routeV1.Route { var toDelete []routeV1.Route for _, clusterRoute := range clusterRoutes { diff --git a/controllers/workspace/suite_test.go b/controllers/workspace/suite_test.go index 92a1612a5..e44402027 100644 --- a/controllers/workspace/suite_test.go +++ b/controllers/workspace/suite_test.go @@ -22,6 +22,7 @@ import ( "runtime" "testing" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" "sigs.k8s.io/controller-runtime/pkg/webhook" dwv1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha1" @@ -121,6 +122,9 @@ var _ = BeforeSuite(func() { mgr, err := ctrl.NewManager(cfg, ctrl.Options{ Scheme: scheme.Scheme, NewCache: cacheFunc, + Metrics: metricsserver.Options{ + BindAddress: "0", // Disable metrics server to avoid port conflicts + }, WebhookServer: webhook.NewServer(webhook.Options{ Port: 9443, }), diff --git a/pkg/webhook/cluster_roles.go b/pkg/webhook/cluster_roles.go index cce26efae..db1b83c7c 100755 --- a/pkg/webhook/cluster_roles.go +++ b/pkg/webhook/cluster_roles.go @@ -103,6 +103,19 @@ func getSpecClusterRole() (*v1.ClusterRole, error) { "watch", }, }, + { + APIGroups: []string{ + "workspace.devfile.io", + }, + Resources: []string{ + "devworkspaces", + }, + Verbs: []string{ + "get", + "list", + "watch", + }, + }, { APIGroups: []string{ "authentication.k8s.io", diff --git a/webhook/workspace/handler/testdata/test-devworkspace.yaml b/webhook/workspace/handler/testdata/test-devworkspace.yaml new file mode 100644 index 000000000..82fc943e2 --- /dev/null +++ b/webhook/workspace/handler/testdata/test-devworkspace.yaml @@ -0,0 +1,16 @@ +apiVersion: workspace.devfile.io/v1alpha2 +kind: DevWorkspace +metadata: + name: test-devworkspace +spec: + started: true + template: + components: + - name: test-component + container: + image: test-image + endpoints: + - name: test-endpoint + targetPort: 8080 + attributes: + discoverable: "true" \ No newline at end of file diff --git a/webhook/workspace/handler/validate.go b/webhook/workspace/handler/validate.go index 2faa33c88..307ed01bf 100644 --- a/webhook/workspace/handler/validate.go +++ b/webhook/workspace/handler/validate.go @@ -23,6 +23,8 @@ import ( dwv2 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" devfilevalidation "github.com/devfile/api/v2/pkg/validation" + "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) @@ -75,9 +77,57 @@ func (h *WebhookHandler) ValidateDevfile(ctx context.Context, req admission.Requ } } + endpointErrors := h.validateEndpoints(ctx, wksp) + if endpointErrors != nil { + devfileErrors = append(devfileErrors, endpointErrors.Error()) + } + if len(devfileErrors) > 0 { return admission.Denied(fmt.Sprintf("\n%s\n", strings.Join(devfileErrors, "\n"))) } return admission.Allowed("No Devfile errors were found") } + +func (h *WebhookHandler) validateEndpoints(ctx context.Context, workspace *dwv2.DevWorkspace) error { + discoverableEndpoints := map[string]bool{} + for _, component := range workspace.Spec.Template.Components { + if component.Container != nil { + for _, endpoint := range component.Container.Endpoints { + if endpoint.Attributes.GetBoolean(string(v1alpha1.DiscoverableAttribute), nil) { + discoverableEndpoints[endpoint.Name] = true + } + } + } + } + + if len(discoverableEndpoints) == 0 { + return nil + } + + workspaceList := &dwv2.DevWorkspaceList{} + if err := h.Client.List(ctx, workspaceList, client.InNamespace(workspace.Namespace)); err != nil { + return err + } + + for _, otherWorkspace := range workspaceList.Items { + if otherWorkspace.UID == workspace.UID { + continue + } + // Skip workspaces that are being deleted + if otherWorkspace.DeletionTimestamp != nil { + continue + } + for _, component := range otherWorkspace.Spec.Template.Components { + if component.Container != nil { + for _, endpoint := range component.Container.Endpoints { + if discoverableEndpoints[endpoint.Name] { + return fmt.Errorf("discoverable endpoint '%s' is already in use by workspace '%s'", endpoint.Name, otherWorkspace.Name) + } + } + } + } + } + + return nil +} diff --git a/webhook/workspace/handler/validate_test.go b/webhook/workspace/handler/validate_test.go new file mode 100644 index 000000000..5a15b576a --- /dev/null +++ b/webhook/workspace/handler/validate_test.go @@ -0,0 +1,179 @@ +package handler + +import ( + "context" + "os" + "path/filepath" + "testing" + + dwv2 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + "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" +) + +func loadObjectFromFile(objName string, obj client.Object, filename string) error { + path := filepath.Join("testdata", filename) + bytes, err := os.ReadFile(path) + if err != nil { + return err + } + err = yaml.Unmarshal(bytes, obj) + if err != nil { + return err + } + obj.SetName(objName) + return nil +} + +func TestValidateEndpoints(t *testing.T) { + scheme := runtime.NewScheme() + _ = dwv2.AddToScheme(scheme) + + t.Run("Conflict in same namespace", func(t *testing.T) { + // Workspace with a discoverable endpoint in namespace "test-namespace" + workspace := &dwv2.DevWorkspace{} + err := loadObjectFromFile("workspace-1", workspace, "test-devworkspace.yaml") + assert.NoError(t, err, "Failed to load test workspace") + workspace.SetUID(types.UID("uid-1")) + workspace.SetNamespace("test-namespace") + + // Another workspace with a conflicting discoverable endpoint in the SAME namespace + otherWorkspaceSameNS := &dwv2.DevWorkspace{} + err = loadObjectFromFile("workspace-2", otherWorkspaceSameNS, "test-devworkspace.yaml") + assert.NoError(t, err, "Failed to load other test workspace") + otherWorkspaceSameNS.SetUID(types.UID("uid-2")) + otherWorkspaceSameNS.SetNamespace("test-namespace") + + // Test for conflict in same namespace + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(otherWorkspaceSameNS).Build() + handler := &WebhookHandler{Client: fakeClient} + err = handler.validateEndpoints(context.TODO(), workspace) + assert.Error(t, err, "Expected a conflict error for workspaces in the same namespace") + assert.Contains(t, err.Error(), "already in use by workspace") + assert.Contains(t, err.Error(), "test-endpoint") + }) + + t.Run("No conflict in different namespace", func(t *testing.T) { + // Workspace in "test-namespace" + workspace := &dwv2.DevWorkspace{} + err := loadObjectFromFile("workspace-1", workspace, "test-devworkspace.yaml") + assert.NoError(t, err, "Failed to load test workspace") + workspace.SetUID(types.UID("uid-1")) + workspace.SetNamespace("test-namespace") + + // Another workspace with the same endpoint name but in a DIFFERENT namespace + otherWorkspaceDiffNS := &dwv2.DevWorkspace{} + err = loadObjectFromFile("workspace-3", otherWorkspaceDiffNS, "test-devworkspace.yaml") + assert.NoError(t, err, "Failed to load third test workspace") + otherWorkspaceDiffNS.SetUID(types.UID("uid-3")) + otherWorkspaceDiffNS.SetNamespace("other-namespace") + + // Test no conflict in different namespace (workspace only queries its own namespace) + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(otherWorkspaceDiffNS).Build() + handler := &WebhookHandler{Client: fakeClient} + err = handler.validateEndpoints(context.TODO(), workspace) + assert.NoError(t, err, "Did not expect an error for workspaces in different namespaces") + }) + + t.Run("No conflict when endpoint name is different", func(t *testing.T) { + workspace := &dwv2.DevWorkspace{} + err := loadObjectFromFile("workspace-1", workspace, "test-devworkspace.yaml") + assert.NoError(t, err, "Failed to load test workspace") + workspace.SetUID(types.UID("uid-1")) + workspace.SetNamespace("test-namespace") + workspace.Spec.Template.Components[0].Container.Endpoints[0].Name = "new-endpoint" + + otherWorkspace := &dwv2.DevWorkspace{} + err = loadObjectFromFile("workspace-2", otherWorkspace, "test-devworkspace.yaml") + assert.NoError(t, err, "Failed to load other test workspace") + otherWorkspace.SetUID(types.UID("uid-2")) + otherWorkspace.SetNamespace("test-namespace") + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(otherWorkspace).Build() + handler := &WebhookHandler{Client: fakeClient} + err = handler.validateEndpoints(context.TODO(), workspace) + assert.NoError(t, err, "Did not expect an error for different endpoint names") + }) + + t.Run("No conflict when workspace is being deleted", func(t *testing.T) { + workspace := &dwv2.DevWorkspace{} + err := loadObjectFromFile("workspace-1", workspace, "test-devworkspace.yaml") + assert.NoError(t, err, "Failed to load test workspace") + workspace.SetUID(types.UID("uid-1")) + workspace.SetNamespace("test-namespace") + + // Workspace being deleted with same endpoint name + deletingWorkspace := &dwv2.DevWorkspace{} + err = loadObjectFromFile("workspace-deleting", deletingWorkspace, "test-devworkspace.yaml") + assert.NoError(t, err, "Failed to load deleting workspace") + deletingWorkspace.SetUID(types.UID("uid-deleting")) + deletingWorkspace.SetNamespace("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.NoError(t, err, "Did not expect an error for workspace being deleted") + }) + + t.Run("No conflict when workspace has no discoverable endpoints", func(t *testing.T) { + workspace := &dwv2.DevWorkspace{} + err := loadObjectFromFile("workspace-1", workspace, "test-devworkspace.yaml") + assert.NoError(t, err, "Failed to load test workspace") + workspace.SetUID(types.UID("uid-1")) + workspace.SetNamespace("test-namespace") + // Remove discoverable attribute + workspace.Spec.Template.Components[0].Container.Endpoints[0].Attributes = nil + + otherWorkspace := &dwv2.DevWorkspace{} + err = loadObjectFromFile("workspace-2", otherWorkspace, "test-devworkspace.yaml") + assert.NoError(t, err, "Failed to load other test workspace") + otherWorkspace.SetUID(types.UID("uid-2")) + otherWorkspace.SetNamespace("test-namespace") + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(otherWorkspace).Build() + handler := &WebhookHandler{Client: fakeClient} + err = handler.validateEndpoints(context.TODO(), workspace) + assert.NoError(t, err, "Did not expect an error when workspace has no discoverable endpoints") + }) + + t.Run("Multiple workspaces in different namespaces can have same endpoint", func(t *testing.T) { + // Workspace 1 in namespace-a + workspace1 := &dwv2.DevWorkspace{} + err := loadObjectFromFile("workspace-ns-a", workspace1, "test-devworkspace.yaml") + assert.NoError(t, err, "Failed to load workspace 1") + workspace1.SetUID(types.UID("uid-ns-a")) + workspace1.SetNamespace("namespace-a") + + // Workspace 2 in namespace-b (will be in the fake client as existing) + workspace2 := &dwv2.DevWorkspace{} + err = loadObjectFromFile("workspace-ns-b", workspace2, "test-devworkspace.yaml") + assert.NoError(t, err, "Failed to load workspace 2") + workspace2.SetUID(types.UID("uid-ns-b")) + workspace2.SetNamespace("namespace-b") + + // Workspace 3 in namespace-c (will be in the fake client as existing) + workspace3 := &dwv2.DevWorkspace{} + err = loadObjectFromFile("workspace-ns-c", workspace3, "test-devworkspace.yaml") + assert.NoError(t, err, "Failed to load workspace 3") + workspace3.SetUID(types.UID("uid-ns-c")) + workspace3.SetNamespace("namespace-c") + + // All three workspaces exist, but in different namespaces + fakeClient := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(workspace2, workspace3).Build() + handler := &WebhookHandler{Client: fakeClient} + + // Validating workspace1 should succeed (different namespaces) + err = handler.validateEndpoints(context.TODO(), workspace1) + assert.NoError(t, err, "Should allow same endpoint name in different namespaces") + }) +} From 08a1a5e4c16d74588fa90f04ddff47a4f128f680 Mon Sep 17 00:00:00 2001 From: Oleksii Kurinnyi Date: Wed, 8 Oct 2025 15:38:11 +0300 Subject: [PATCH 02/11] fixup! fix: identical endpoint name conflicts Signed-off-by: Oleksii Kurinnyi --- .../devworkspacerouting/solvers/common_test.go | 13 +++++++++++++ webhook/workspace/handler/validate_test.go | 13 +++++++++++++ 2 files changed, 26 insertions(+) diff --git a/controllers/controller/devworkspacerouting/solvers/common_test.go b/controllers/controller/devworkspacerouting/solvers/common_test.go index 65882cb5d..b6b58a533 100644 --- a/controllers/controller/devworkspacerouting/solvers/common_test.go +++ b/controllers/controller/devworkspacerouting/solvers/common_test.go @@ -1,3 +1,16 @@ +// Copyright (c) 2019-2025 Red Hat, Inc. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package solvers import ( diff --git a/webhook/workspace/handler/validate_test.go b/webhook/workspace/handler/validate_test.go index 5a15b576a..4dd17c08a 100644 --- a/webhook/workspace/handler/validate_test.go +++ b/webhook/workspace/handler/validate_test.go @@ -1,3 +1,16 @@ +// Copyright (c) 2019-2025 Red Hat, Inc. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + package handler import ( From 88e0d3ccbf1923295a6e08d0d1f78e31203ac1bc Mon Sep 17 00:00:00 2001 From: Oleksii Kurinnyi Date: Wed, 8 Oct 2025 17:57:07 +0300 Subject: [PATCH 03/11] fixup! fixup! fix: identical endpoint name conflicts Signed-off-by: Oleksii Kurinnyi --- .../devworkspacerouting_controller.go | 4 ++-- .../devworkspacerouting/solvers/basic_solver.go | 17 ++++++++++++++--- .../solvers/cluster_solver.go | 17 ++++++++++++++--- .../devworkspacerouting/solvers/solver.go | 13 +++++++------ 4 files changed, 37 insertions(+), 14 deletions(-) diff --git a/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go b/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go index deaed827f..cc1315148 100644 --- a/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go +++ b/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go @@ -93,7 +93,7 @@ func (r *DevWorkspaceRoutingReconciler) Reconcile(ctx context.Context, req ctrl. return reconcile.Result{}, r.markRoutingFailed(instance, "DevWorkspaceRouting requires field routingClass to be set") } - solver, err := r.SolverGetter.GetSolver(r.Client, instance.Spec.RoutingClass) + solver, err := r.SolverGetter.GetSolver(r.Client, reqLogger, instance.Spec.RoutingClass) if err != nil { if errors.Is(err, solvers.RoutingNotSupported) { reqLogger.Info("Routing class not supported by this controller, skipping reconciliation", "routingClass", instance.Spec.RoutingClass) @@ -132,7 +132,7 @@ func (r *DevWorkspaceRoutingReconciler) Reconcile(ctx context.Context, req ctrl. } restrictedAccess, setRestrictedAccess := instance.Annotations[constants.DevWorkspaceRestrictedAccessAnnotation] - routingObjects, err := solver.GetSpecObjects(instance, workspaceMeta, r.Client, reqLogger) + routingObjects, err := solver.GetSpecObjects(instance, workspaceMeta) if err != nil { var notReady *solvers.RoutingNotReady if errors.As(err, ¬Ready) { diff --git a/controllers/controller/devworkspacerouting/solvers/basic_solver.go b/controllers/controller/devworkspacerouting/solvers/basic_solver.go index 8bc418ccc..45b880c0c 100644 --- a/controllers/controller/devworkspacerouting/solvers/basic_solver.go +++ b/controllers/controller/devworkspacerouting/solvers/basic_solver.go @@ -49,10 +49,21 @@ var nginxIngressAnnotations = func(endpointName string, endpointAnnotations map[ // According to the current cluster there is different behavior: // Kubernetes: use Ingresses without TLS // OpenShift: use Routes with TLS enabled -type BasicSolver struct{} +type BasicSolver struct { + client client.Client + logger logr.Logger +} var _ RoutingSolver = (*BasicSolver)(nil) +// NewBasicSolver creates a new BasicSolver with the provided dependencies +func NewBasicSolver(client client.Client, logger logr.Logger) *BasicSolver { + return &BasicSolver{ + client: client, + logger: logger, + } +} + func (s *BasicSolver) FinalizerRequired(*controllerv1alpha1.DevWorkspaceRouting) bool { return false } @@ -61,7 +72,7 @@ func (s *BasicSolver) Finalize(*controllerv1alpha1.DevWorkspaceRouting) error { return nil } -func (s *BasicSolver) GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRouting, workspaceMeta DevWorkspaceMetadata, cl client.Client, log logr.Logger) (RoutingObjects, error) { +func (s *BasicSolver) GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRouting, workspaceMeta DevWorkspaceMetadata) (RoutingObjects, error) { routingObjects := RoutingObjects{} // TODO: Use workspace-scoped ClusterHostSuffix to allow overriding @@ -72,7 +83,7 @@ func (s *BasicSolver) GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRou spec := routing.Spec services := getServicesForEndpoints(spec.Endpoints, workspaceMeta) - discoverableServices, err := GetDiscoverableServicesForEndpoints(spec.Endpoints, workspaceMeta, cl, log) + discoverableServices, err := GetDiscoverableServicesForEndpoints(spec.Endpoints, workspaceMeta, s.client, s.logger) if err != nil { return RoutingObjects{}, err } diff --git a/controllers/controller/devworkspacerouting/solvers/cluster_solver.go b/controllers/controller/devworkspacerouting/solvers/cluster_solver.go index e6d0bc71b..24e77ae29 100644 --- a/controllers/controller/devworkspacerouting/solvers/cluster_solver.go +++ b/controllers/controller/devworkspacerouting/solvers/cluster_solver.go @@ -33,11 +33,22 @@ const ( ) type ClusterSolver struct { - TLS bool + TLS bool + client client.Client + logger logr.Logger } var _ RoutingSolver = (*ClusterSolver)(nil) +// NewClusterSolver creates a new ClusterSolver with the provided dependencies +func NewClusterSolver(client client.Client, logger logr.Logger, tls bool) *ClusterSolver { + return &ClusterSolver{ + TLS: tls, + client: client, + logger: logger, + } +} + func (s *ClusterSolver) FinalizerRequired(*controllerv1alpha1.DevWorkspaceRouting) bool { return false } @@ -46,10 +57,10 @@ func (s *ClusterSolver) Finalize(*controllerv1alpha1.DevWorkspaceRouting) error return nil } -func (s *ClusterSolver) GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRouting, workspaceMeta DevWorkspaceMetadata, cl client.Client, log logr.Logger) (RoutingObjects, error) { +func (s *ClusterSolver) GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRouting, workspaceMeta DevWorkspaceMetadata) (RoutingObjects, error) { spec := routing.Spec services := getServicesForEndpoints(spec.Endpoints, workspaceMeta) - discoverableServices, err := GetDiscoverableServicesForEndpoints(spec.Endpoints, workspaceMeta, cl, log) + discoverableServices, err := GetDiscoverableServicesForEndpoints(spec.Endpoints, workspaceMeta, s.client, s.logger) if err != nil { return RoutingObjects{}, err } diff --git a/controllers/controller/devworkspacerouting/solvers/solver.go b/controllers/controller/devworkspacerouting/solvers/solver.go index 2d278f579..70ed7b39a 100644 --- a/controllers/controller/devworkspacerouting/solvers/solver.go +++ b/controllers/controller/devworkspacerouting/solvers/solver.go @@ -53,7 +53,7 @@ type RoutingSolver interface { // The implementors can also create any additional objects not captured by the RoutingObjects struct. If that's // the case they are required to set the restricted access annotation on any objects created according to the // restricted access specified by the routing. - GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRouting, workspaceMeta DevWorkspaceMetadata, client client.Client, log logr.Logger) (RoutingObjects, error) + GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRouting, workspaceMeta DevWorkspaceMetadata) (RoutingObjects, error) // GetExposedEndpoints retreives the URL for each endpoint in a devfile spec from a set of RoutingObjects. // Returns is a map from component ids (as defined in the devfile) to the list of endpoints for that component @@ -78,7 +78,7 @@ type RoutingSolverGetter interface { // the routingClass is not recognized, and any other error if the routingClass is invalid (e.g. an OpenShift-only // routingClass on a vanilla Kubernetes platform). Note that an empty routingClass is handled by the DevWorkspace controller itself, // and should not be handled by external controllers. - GetSolver(client client.Client, routingClass controllerv1alpha1.DevWorkspaceRoutingClass) (solver RoutingSolver, err error) + GetSolver(client client.Client, logger logr.Logger, routingClass controllerv1alpha1.DevWorkspaceRoutingClass) (solver RoutingSolver, err error) } type SolverGetter struct{} @@ -101,18 +101,19 @@ func (_ *SolverGetter) HasSolver(routingClass controllerv1alpha1.DevWorkspaceRou } } -func (_ *SolverGetter) GetSolver(_ client.Client, routingClass controllerv1alpha1.DevWorkspaceRoutingClass) (RoutingSolver, error) { +func (_ *SolverGetter) GetSolver(client client.Client, logger logr.Logger, routingClass controllerv1alpha1.DevWorkspaceRoutingClass) (RoutingSolver, error) { isOpenShift := infrastructure.IsOpenShift() + switch routingClass { case controllerv1alpha1.DevWorkspaceRoutingBasic: - return &BasicSolver{}, nil + return NewBasicSolver(client, logger), nil case controllerv1alpha1.DevWorkspaceRoutingCluster: - return &ClusterSolver{}, nil + return NewClusterSolver(client, logger, false), nil case controllerv1alpha1.DevWorkspaceRoutingClusterTLS, controllerv1alpha1.DevWorkspaceRoutingWebTerminal: if !isOpenShift { return nil, fmt.Errorf("routing class %s only supported on OpenShift", routingClass) } - return &ClusterSolver{TLS: true}, nil + return NewClusterSolver(client, logger, true), nil default: return nil, RoutingNotSupported } From d9682c9c286eafee8c2245a37a028867173b1667 Mon Sep 17 00:00:00 2001 From: Oleksii Kurinnyi Date: Wed, 8 Oct 2025 18:14:58 +0300 Subject: [PATCH 04/11] chore: remove excecive defensive checks Signed-off-by: Oleksii Kurinnyi --- .../devworkspacerouting_controller.go | 8 ----- .../devworkspacerouting/solvers/errors.go | 12 ------- .../devworkspacerouting/solvers/solver.go | 2 +- .../devworkspacerouting/sync_ingresses.go | 31 ------------------- .../devworkspacerouting/sync_routes.go | 27 ---------------- 5 files changed, 1 insertion(+), 79 deletions(-) diff --git a/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go b/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go index cc1315148..02c3181d3 100644 --- a/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go +++ b/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go @@ -215,10 +215,6 @@ func (r *DevWorkspaceRoutingReconciler) Reconcile(ctx context.Context, req ctrl. if errors.As(err, &failError) { return reconcile.Result{}, r.markRoutingFailed(instance, err.Error()) } - conflictErr := &solvers.HostnameConflictError{} - if errors.As(err, &conflictErr) { - return reconcile.Result{}, r.markRoutingFailed(instance, conflictErr.Error()) - } reqLogger.Error(err, "Error syncing routes") return reconcile.Result{Requeue: true}, r.reconcileStatus(instance, nil, nil, false, "Preparing routes") } else if !routesInSync { @@ -233,10 +229,6 @@ func (r *DevWorkspaceRoutingReconciler) Reconcile(ctx context.Context, req ctrl. if errors.As(err, &failError) { return reconcile.Result{}, r.markRoutingFailed(instance, err.Error()) } - conflictErr := &solvers.HostnameConflictError{} - if errors.As(err, &conflictErr) { - return reconcile.Result{}, r.markRoutingFailed(instance, conflictErr.Error()) - } reqLogger.Error(err, "Error syncing ingresses") return reconcile.Result{Requeue: true}, r.reconcileStatus(instance, nil, nil, false, "Preparing ingresses") } else if !ingressesInSync { diff --git a/controllers/controller/devworkspacerouting/solvers/errors.go b/controllers/controller/devworkspacerouting/solvers/errors.go index 2d0c07348..6ee70a504 100644 --- a/controllers/controller/devworkspacerouting/solvers/errors.go +++ b/controllers/controller/devworkspacerouting/solvers/errors.go @@ -21,18 +21,6 @@ import ( ) var _ error = (*RoutingNotReady)(nil) - -// HostnameConflictError is an error returned when a hostname required by a DevWorkspace is already in use -// by another resource on the cluster. -type HostnameConflictError struct { - // Reason provides a textual summary of the conflict - Reason string -} - -func (e *HostnameConflictError) Error() string { - return e.Reason -} - var _ error = (*RoutingInvalid)(nil) var _ error = (*ServiceConflictError)(nil) diff --git a/controllers/controller/devworkspacerouting/solvers/solver.go b/controllers/controller/devworkspacerouting/solvers/solver.go index 70ed7b39a..413b3723d 100644 --- a/controllers/controller/devworkspacerouting/solvers/solver.go +++ b/controllers/controller/devworkspacerouting/solvers/solver.go @@ -103,7 +103,7 @@ func (_ *SolverGetter) HasSolver(routingClass controllerv1alpha1.DevWorkspaceRou func (_ *SolverGetter) GetSolver(client client.Client, logger logr.Logger, routingClass controllerv1alpha1.DevWorkspaceRoutingClass) (RoutingSolver, error) { isOpenShift := infrastructure.IsOpenShift() - + switch routingClass { case controllerv1alpha1.DevWorkspaceRoutingBasic: return NewBasicSolver(client, logger), nil diff --git a/controllers/controller/devworkspacerouting/sync_ingresses.go b/controllers/controller/devworkspacerouting/sync_ingresses.go index ba1ce5b4c..6d21fae5a 100644 --- a/controllers/controller/devworkspacerouting/sync_ingresses.go +++ b/controllers/controller/devworkspacerouting/sync_ingresses.go @@ -19,7 +19,6 @@ import ( "context" "fmt" - "github.com/devfile/devworkspace-operator/controllers/controller/devworkspacerouting/solvers" "github.com/devfile/devworkspace-operator/pkg/constants" "github.com/devfile/devworkspace-operator/pkg/provision/sync" @@ -33,14 +32,6 @@ import ( func (r *DevWorkspaceRoutingReconciler) syncIngresses(routing *controllerv1alpha1.DevWorkspaceRouting, specIngresses []networkingv1.Ingress) (ok bool, clusterIngresses []networkingv1.Ingress, err error) { ingressesInSync := true - for _, ingress := range specIngresses { - for _, rule := range ingress.Spec.Rules { - if err := r.checkForIngressConflict(rule.Host, routing); err != nil { - return false, nil, err - } - } - } - clusterIngresses, err = r.getClusterIngresses(routing) if err != nil { return false, nil, err @@ -99,28 +90,6 @@ func (r *DevWorkspaceRoutingReconciler) getClusterIngresses(routing *controllerv return found.Items, nil } -func (r *DevWorkspaceRoutingReconciler) checkForIngressConflict(hostname string, owner *controllerv1alpha1.DevWorkspaceRouting) error { - ingresses := &networkingv1.IngressList{} - if err := r.List(context.TODO(), ingresses, client.InNamespace(owner.Namespace)); err != nil { - return err - } - for _, ingress := range ingresses.Items { - for _, rule := range ingress.Spec.Rules { - if rule.Host == hostname { - for _, ownerRef := range ingress.GetOwnerReferences() { - if ownerRef.UID == owner.UID { - return nil - } - } - return &solvers.HostnameConflictError{ - Reason: fmt.Sprintf("hostname %s is already in use by ingress %s", hostname, ingress.Name), - } - } - } - } - return nil -} - func getIngressesToDelete(clusterIngresses, specIngresses []networkingv1.Ingress) []networkingv1.Ingress { var toDelete []networkingv1.Ingress for _, clusterIngress := range clusterIngresses { diff --git a/controllers/controller/devworkspacerouting/sync_routes.go b/controllers/controller/devworkspacerouting/sync_routes.go index ed2f82a27..4a542a140 100644 --- a/controllers/controller/devworkspacerouting/sync_routes.go +++ b/controllers/controller/devworkspacerouting/sync_routes.go @@ -19,7 +19,6 @@ import ( "context" "fmt" - "github.com/devfile/devworkspace-operator/controllers/controller/devworkspacerouting/solvers" "github.com/devfile/devworkspace-operator/pkg/constants" "github.com/devfile/devworkspace-operator/pkg/provision/sync" routeV1 "github.com/openshift/api/route/v1" @@ -32,12 +31,6 @@ import ( func (r *DevWorkspaceRoutingReconciler) syncRoutes(routing *controllerv1alpha1.DevWorkspaceRouting, specRoutes []routeV1.Route) (ok bool, clusterRoutes []routeV1.Route, err error) { routesInSync := true - for _, route := range specRoutes { - if err := r.checkForRouteConflict(route.Spec.Host, routing); err != nil { - return false, nil, err - } - } - clusterRoutes, err = r.getClusterRoutes(routing) if err != nil { return false, nil, err @@ -107,26 +100,6 @@ func (r *DevWorkspaceRoutingReconciler) getClusterRoutes(routing *controllerv1al return routes, nil } -func (r *DevWorkspaceRoutingReconciler) checkForRouteConflict(hostname string, owner *controllerv1alpha1.DevWorkspaceRouting) error { - routes := &routeV1.RouteList{} - if err := r.List(context.TODO(), routes, client.InNamespace(owner.Namespace)); err != nil { - return err - } - for _, route := range routes.Items { - if route.Spec.Host == hostname { - for _, ownerRef := range route.GetOwnerReferences() { - if ownerRef.UID == owner.UID { - return nil - } - } - return &solvers.HostnameConflictError{ - Reason: fmt.Sprintf("hostname %s is already in use by route %s", hostname, route.Name), - } - } - } - return nil -} - func getRoutesToDelete(clusterRoutes, specRoutes []routeV1.Route) []routeV1.Route { var toDelete []routeV1.Route for _, clusterRoute := range clusterRoutes { From 4879878cd6f1a22ff070e5031cd617dfe066532e Mon Sep 17 00:00:00 2001 From: Oleksii Kurinnyi Date: Thu, 9 Oct 2025 11:59:35 +0300 Subject: [PATCH 05/11] chore: remove excessive logs. Signed-off-by: Oleksii Kurinnyi --- controllers/controller/devworkspacerouting/solvers/common.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/controllers/controller/devworkspacerouting/solvers/common.go b/controllers/controller/devworkspacerouting/solvers/common.go index fcc642429..0387b97fc 100644 --- a/controllers/controller/devworkspacerouting/solvers/common.go +++ b/controllers/controller/devworkspacerouting/solvers/common.go @@ -52,7 +52,6 @@ func GetDiscoverableServicesForEndpoints(endpoints map[string]controllerv1alpha1 if endpoint.Attributes.GetBoolean(string(controllerv1alpha1.DiscoverableAttribute), nil) { serviceName := common.EndpointName(endpoint.Name) - log.Info("Checking for existing service for discoverable endpoint", "serviceName", serviceName) existingService := &corev1.Service{} err := cl.Get(context.TODO(), client.ObjectKey{Name: serviceName, Namespace: meta.Namespace}, existingService) if err != nil { @@ -60,16 +59,12 @@ func GetDiscoverableServicesForEndpoints(endpoints map[string]controllerv1alpha1 log.Error(err, "Failed to get service from cluster", "serviceName", serviceName) return nil, err } - log.Info("No existing service found", "serviceName", serviceName) } else { - log.Info("Found existing service", "serviceName", serviceName) if existingService.Labels[constants.DevWorkspaceIDLabel] != meta.DevWorkspaceId { - log.Info("Service conflict detected", "serviceName", serviceName, "existingWorkspaceId", existingService.Labels[constants.DevWorkspaceIDLabel], "currentWorkspaceId", meta.DevWorkspaceId) return nil, &ServiceConflictError{ Reason: fmt.Sprintf("discoverable endpoint %s conflicts with existing service", endpoint.Name), } } - log.Info("Existing service is owned by the same workspace", "serviceName", serviceName) } servicePort := corev1.ServicePort{ From dc0a395848eec834c0183cc99d34148903400fa4 Mon Sep 17 00:00:00 2001 From: Oleksii Kurinnyi Date: Thu, 9 Oct 2025 12:03:34 +0300 Subject: [PATCH 06/11] chore: consistent error Signed-off-by: Oleksii Kurinnyi --- webhook/workspace/handler/validate.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/webhook/workspace/handler/validate.go b/webhook/workspace/handler/validate.go index 307ed01bf..5919983c9 100644 --- a/webhook/workspace/handler/validate.go +++ b/webhook/workspace/handler/validate.go @@ -24,6 +24,7 @@ import ( dwv2 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" devfilevalidation "github.com/devfile/api/v2/pkg/validation" "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" + "github.com/devfile/devworkspace-operator/controllers/controller/devworkspacerouting/solvers" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) @@ -122,7 +123,9 @@ func (h *WebhookHandler) validateEndpoints(ctx context.Context, workspace *dwv2. if component.Container != nil { for _, endpoint := range component.Container.Endpoints { if discoverableEndpoints[endpoint.Name] { - return fmt.Errorf("discoverable endpoint '%s' is already in use by workspace '%s'", endpoint.Name, otherWorkspace.Name) + return &solvers.ServiceConflictError{ + Reason: fmt.Sprintf("discoverable endpoint '%s' is already in use by workspace '%s'", endpoint.Name, otherWorkspace.Name), + } } } } From c7ddd74ce60d2c2441362c072341eaa4b88e050f Mon Sep 17 00:00:00 2001 From: Oleksii Kurinnyi Date: Thu, 9 Oct 2025 12:15:20 +0300 Subject: [PATCH 07/11] fixup! chore: consistent error Signed-off-by: Oleksii Kurinnyi --- .../controller/devworkspacerouting/solvers/common.go | 3 +-- .../controller/devworkspacerouting/solvers/errors.go | 11 ++++++----- webhook/workspace/handler/validate.go | 3 ++- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/controllers/controller/devworkspacerouting/solvers/common.go b/controllers/controller/devworkspacerouting/solvers/common.go index 0387b97fc..dc37707b2 100644 --- a/controllers/controller/devworkspacerouting/solvers/common.go +++ b/controllers/controller/devworkspacerouting/solvers/common.go @@ -17,7 +17,6 @@ package solvers import ( "context" - "fmt" controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1" "github.com/devfile/devworkspace-operator/pkg/common" @@ -62,7 +61,7 @@ func GetDiscoverableServicesForEndpoints(endpoints map[string]controllerv1alpha1 } else { if existingService.Labels[constants.DevWorkspaceIDLabel] != meta.DevWorkspaceId { return nil, &ServiceConflictError{ - Reason: fmt.Sprintf("discoverable endpoint %s conflicts with existing service", endpoint.Name), + EndpointName: endpoint.Name, } } } diff --git a/controllers/controller/devworkspacerouting/solvers/errors.go b/controllers/controller/devworkspacerouting/solvers/errors.go index 6ee70a504..bcafae869 100644 --- a/controllers/controller/devworkspacerouting/solvers/errors.go +++ b/controllers/controller/devworkspacerouting/solvers/errors.go @@ -17,6 +17,7 @@ package solvers import ( "errors" + "fmt" "time" ) @@ -27,15 +28,15 @@ var _ error = (*ServiceConflictError)(nil) // ServiceConflictError is returned when a discoverable endpoint has a name that is already in use by // another DevWorkspace's service. type ServiceConflictError struct { - Reason string + EndpointName string + WorkspaceName string } func (e *ServiceConflictError) Error() string { - reason := "" - if len(e.Reason) > 0 { - reason = e.Reason + if (e.WorkspaceName == "") { + return fmt.Sprintf("discoverable endpoint '%s' is already in use by another workspace", e.EndpointName) } - return "workspace routing has a service conflict: " + reason + return fmt.Sprintf("discoverable endpoint '%s' is already in use by workspace '%s'", e.EndpointName, e.WorkspaceName) } // RoutingNotSupported is used by the solvers when they supported the routingclass of the workspace they've been asked to route diff --git a/webhook/workspace/handler/validate.go b/webhook/workspace/handler/validate.go index 5919983c9..7e25ba9c0 100644 --- a/webhook/workspace/handler/validate.go +++ b/webhook/workspace/handler/validate.go @@ -124,7 +124,8 @@ func (h *WebhookHandler) validateEndpoints(ctx context.Context, workspace *dwv2. for _, endpoint := range component.Container.Endpoints { if discoverableEndpoints[endpoint.Name] { return &solvers.ServiceConflictError{ - Reason: fmt.Sprintf("discoverable endpoint '%s' is already in use by workspace '%s'", endpoint.Name, otherWorkspace.Name), + EndpointName: endpoint.Name, + WorkspaceName: otherWorkspace.Name, } } } From 91f10145c7e912072080c296ef17671a81cc7db4 Mon Sep 17 00:00:00 2001 From: Oleksii Kurinnyi Date: Thu, 9 Oct 2025 12:18:01 +0300 Subject: [PATCH 08/11] fixup! fixup! chore: consistent error Signed-off-by: Oleksii Kurinnyi --- controllers/controller/devworkspacerouting/solvers/errors.go | 4 ++-- webhook/workspace/handler/validate.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/controllers/controller/devworkspacerouting/solvers/errors.go b/controllers/controller/devworkspacerouting/solvers/errors.go index bcafae869..bb2d2561f 100644 --- a/controllers/controller/devworkspacerouting/solvers/errors.go +++ b/controllers/controller/devworkspacerouting/solvers/errors.go @@ -28,12 +28,12 @@ var _ error = (*ServiceConflictError)(nil) // ServiceConflictError is returned when a discoverable endpoint has a name that is already in use by // another DevWorkspace's service. type ServiceConflictError struct { - EndpointName string + EndpointName string WorkspaceName string } func (e *ServiceConflictError) Error() string { - if (e.WorkspaceName == "") { + if e.WorkspaceName == "" { return fmt.Sprintf("discoverable endpoint '%s' is already in use by another workspace", e.EndpointName) } return fmt.Sprintf("discoverable endpoint '%s' is already in use by workspace '%s'", e.EndpointName, e.WorkspaceName) diff --git a/webhook/workspace/handler/validate.go b/webhook/workspace/handler/validate.go index 7e25ba9c0..f4a2b54a6 100644 --- a/webhook/workspace/handler/validate.go +++ b/webhook/workspace/handler/validate.go @@ -124,7 +124,7 @@ func (h *WebhookHandler) validateEndpoints(ctx context.Context, workspace *dwv2. for _, endpoint := range component.Container.Endpoints { if discoverableEndpoints[endpoint.Name] { return &solvers.ServiceConflictError{ - EndpointName: endpoint.Name, + EndpointName: endpoint.Name, WorkspaceName: otherWorkspace.Name, } } From 9de6b51dc9e13dc2e2b8b38d220139bb5097ccb0 Mon Sep 17 00:00:00 2001 From: Oleksii Kurinnyi Date: Thu, 9 Oct 2025 12:57:03 +0300 Subject: [PATCH 09/11] fixup! fixup! fixup! chore: consistent error Signed-off-by: Oleksii Kurinnyi --- .../devworkspacerouting_controller.go | 9 +++---- .../devworkspacerouting/solvers/common.go | 13 ++++++---- .../solvers/common_test.go | 24 ++++++++++++------- 3 files changed, 28 insertions(+), 18 deletions(-) diff --git a/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go b/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go index 02c3181d3..d513487be 100644 --- a/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go +++ b/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go @@ -126,9 +126,10 @@ func (r *DevWorkspaceRoutingReconciler) Reconcile(ctx context.Context, req ctrl. } 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, } restrictedAccess, setRestrictedAccess := instance.Annotations[constants.DevWorkspaceRestrictedAccessAnnotation] @@ -152,7 +153,7 @@ func (r *DevWorkspaceRoutingReconciler) Reconcile(ctx context.Context, req ctrl. var conflict *solvers.ServiceConflictError if errors.As(err, &conflict) { - reqLogger.Error(conflict, "Routing controller detected a service conflict", "serviceName", conflict.Reason) + reqLogger.Error(conflict, "Routing controller detected a service conflict", "endpointName", conflict.EndpointName, "workspaceName", conflict.WorkspaceName) return reconcile.Result{}, r.markRoutingFailed(instance, fmt.Sprintf("Unable to provision networking for DevWorkspace: %s", conflict)) } diff --git a/controllers/controller/devworkspacerouting/solvers/common.go b/controllers/controller/devworkspacerouting/solvers/common.go index dc37707b2..7fb0be42c 100644 --- a/controllers/controller/devworkspacerouting/solvers/common.go +++ b/controllers/controller/devworkspacerouting/solvers/common.go @@ -34,9 +34,10 @@ import ( ) type DevWorkspaceMetadata struct { - DevWorkspaceId string - Namespace string - PodSelector map[string]string + DevWorkspaceId string + DevWorkspaceName string + Namespace string + PodSelector map[string]string } // GetDiscoverableServicesForEndpoints converts the endpoint list into a set of services, each corresponding to a single discoverable @@ -61,7 +62,8 @@ func GetDiscoverableServicesForEndpoints(endpoints map[string]controllerv1alpha1 } else { if existingService.Labels[constants.DevWorkspaceIDLabel] != meta.DevWorkspaceId { return nil, &ServiceConflictError{ - EndpointName: endpoint.Name, + EndpointName: endpoint.Name, + WorkspaceName: existingService.Labels[constants.DevWorkspaceNameLabel], } } } @@ -77,7 +79,8 @@ func GetDiscoverableServicesForEndpoints(endpoints map[string]controllerv1alpha1 Name: serviceName, Namespace: meta.Namespace, Labels: map[string]string{ - constants.DevWorkspaceIDLabel: meta.DevWorkspaceId, + constants.DevWorkspaceIDLabel: meta.DevWorkspaceId, + constants.DevWorkspaceNameLabel: meta.DevWorkspaceName, }, Annotations: map[string]string{ constants.DevWorkspaceDiscoverableServiceAnnotation: "true", diff --git a/controllers/controller/devworkspacerouting/solvers/common_test.go b/controllers/controller/devworkspacerouting/solvers/common_test.go index b6b58a533..61842c78e 100644 --- a/controllers/controller/devworkspacerouting/solvers/common_test.go +++ b/controllers/controller/devworkspacerouting/solvers/common_test.go @@ -44,8 +44,9 @@ func TestGetDiscoverableServicesForEndpoints(t *testing.T) { } meta := DevWorkspaceMetadata{ - DevWorkspaceId: "current-workspace", - Namespace: "test-namespace", + DevWorkspaceId: "current-workspace-id", + DevWorkspaceName: "current-workspace", + Namespace: "test-namespace", } tests := []struct { @@ -68,14 +69,15 @@ func TestGetDiscoverableServicesForEndpoints(t *testing.T) { Name: "test-endpoint", Namespace: "test-namespace", Labels: map[string]string{ - constants.DevWorkspaceIDLabel: "other-workspace", + constants.DevWorkspaceIDLabel: "other-workspace-id", + constants.DevWorkspaceNameLabel: "other-workspace", }, }, }, }, expectErr: true, expectErrType: &ServiceConflictError{}, - expectMsg: "conflicts with existing service", + expectMsg: "discoverable endpoint 'test-endpoint' is already in use by workspace 'other-workspace'", }, { name: "Existing service with same owner (reconciliation)", @@ -85,7 +87,8 @@ func TestGetDiscoverableServicesForEndpoints(t *testing.T) { Name: "test-endpoint", Namespace: "test-namespace", Labels: map[string]string{ - constants.DevWorkspaceIDLabel: "current-workspace", + constants.DevWorkspaceIDLabel: "current-workspace-id", + constants.DevWorkspaceNameLabel: "current-workspace", }, }, }, @@ -100,7 +103,8 @@ func TestGetDiscoverableServicesForEndpoints(t *testing.T) { Name: "test-endpoint", Namespace: "other-namespace", Labels: map[string]string{ - constants.DevWorkspaceIDLabel: "other-workspace", + constants.DevWorkspaceIDLabel: "other-workspace-id", + constants.DevWorkspaceNameLabel: "other-workspace", }, }, }, @@ -175,8 +179,9 @@ func TestGetDiscoverableServicesForEndpoints_MultipleEndpoints(t *testing.T) { } meta := DevWorkspaceMetadata{ - DevWorkspaceId: "current-workspace", - Namespace: "test-namespace", + DevWorkspaceId: "current-workspace-id", + DevWorkspaceName: "current-workspace", + Namespace: "test-namespace", PodSelector: map[string]string{ constants.DevWorkspaceIDLabel: "current-workspace", }, @@ -203,7 +208,8 @@ func TestGetDiscoverableServicesForEndpoints_MultipleEndpoints(t *testing.T) { Name: "postgresql", Namespace: "test-namespace", Labels: map[string]string{ - constants.DevWorkspaceIDLabel: "other-workspace", + constants.DevWorkspaceIDLabel: "other-workspace-id", + constants.DevWorkspaceNameLabel: "other-workspace", }, }, } From 338237fe1c7b594af3c70f230a486952c2a37301 Mon Sep 17 00:00:00 2001 From: Oleksii Kurinnyi Date: Thu, 9 Oct 2025 13:06:01 +0300 Subject: [PATCH 10/11] chore: check terminating workspaces Signed-off-by: Oleksii Kurinnyi --- webhook/workspace/handler/validate.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/webhook/workspace/handler/validate.go b/webhook/workspace/handler/validate.go index f4a2b54a6..ff250ffae 100644 --- a/webhook/workspace/handler/validate.go +++ b/webhook/workspace/handler/validate.go @@ -115,10 +115,6 @@ func (h *WebhookHandler) validateEndpoints(ctx context.Context, workspace *dwv2. if otherWorkspace.UID == workspace.UID { continue } - // Skip workspaces that are being deleted - if otherWorkspace.DeletionTimestamp != nil { - continue - } for _, component := range otherWorkspace.Spec.Template.Components { if component.Container != nil { for _, endpoint := range component.Container.Endpoints { From 654ee01ccfe1c92e0576a5043682fa9d12e1903c Mon Sep 17 00:00:00 2001 From: Oleksii Kurinnyi Date: Thu, 9 Oct 2025 14:53:58 +0300 Subject: [PATCH 11/11] fixup! chore: check terminating workspaces Signed-off-by: Oleksii Kurinnyi --- webhook/workspace/handler/validate_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/webhook/workspace/handler/validate_test.go b/webhook/workspace/handler/validate_test.go index 4dd17c08a..6dc31e5a0 100644 --- a/webhook/workspace/handler/validate_test.go +++ b/webhook/workspace/handler/validate_test.go @@ -113,7 +113,7 @@ func TestValidateEndpoints(t *testing.T) { assert.NoError(t, err, "Did not expect an error for different endpoint names") }) - t.Run("No conflict when workspace is being deleted", func(t *testing.T) { + t.Run("Conflict detected even when workspace is being deleted", func(t *testing.T) { workspace := &dwv2.DevWorkspace{} err := loadObjectFromFile("workspace-1", workspace, "test-devworkspace.yaml") assert.NoError(t, err, "Failed to load test workspace") @@ -134,7 +134,8 @@ func TestValidateEndpoints(t *testing.T) { fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(deletingWorkspace).Build() handler := &WebhookHandler{Client: fakeClient} err = handler.validateEndpoints(context.TODO(), workspace) - assert.NoError(t, err, "Did not expect an error for workspace being deleted") + assert.Error(t, err, "Should detect conflict even with workspace being deleted") + assert.Contains(t, err.Error(), "workspace-deleting") }) t.Run("No conflict when workspace has no discoverable endpoints", func(t *testing.T) {