diff --git a/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go b/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go index 367da93d0..d513487be 100644 --- a/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go +++ b/controllers/controller/devworkspacerouting/devworkspacerouting_controller.go @@ -93,9 +93,10 @@ 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) return reconcile.Result{}, nil } return reconcile.Result{}, r.markRoutingFailed(instance, fmt.Sprintf("Invalid routingClass for DevWorkspace: %s", err)) @@ -125,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] @@ -149,6 +151,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", "endpointName", conflict.EndpointName, "workspaceName", conflict.WorkspaceName) + 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 } diff --git a/controllers/controller/devworkspacerouting/solvers/basic_solver.go b/controllers/controller/devworkspacerouting/solvers/basic_solver.go index 50df27fa1..45b880c0c 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 { @@ -47,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 } @@ -70,7 +83,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, s.client, s.logger) + 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..24e77ae29 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 ( @@ -31,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 } @@ -47,6 +60,11 @@ func (s *ClusterSolver) Finalize(*controllerv1alpha1.DevWorkspaceRouting) 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, s.client, s.logger) + 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..7fb0be42c 100644 --- a/controllers/controller/devworkspacerouting/solvers/common.go +++ b/controllers/controller/devworkspacerouting/solvers/common.go @@ -16,9 +16,14 @@ package solvers import ( + "context" + 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" @@ -29,14 +34,15 @@ 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 // 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,21 +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) + 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 + } + } else { + if existingService.Labels[constants.DevWorkspaceIDLabel] != meta.DevWorkspaceId { + return nil, &ServiceConflictError{ + EndpointName: endpoint.Name, + WorkspaceName: existingService.Labels[constants.DevWorkspaceNameLabel], + } + } + } + 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, + constants.DevWorkspaceIDLabel: meta.DevWorkspaceId, + constants.DevWorkspaceNameLabel: meta.DevWorkspaceName, }, Annotations: map[string]string{ constants.DevWorkspaceDiscoverableServiceAnnotation: "true", @@ -74,7 +95,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..61842c78e --- /dev/null +++ b/controllers/controller/devworkspacerouting/solvers/common_test.go @@ -0,0 +1,222 @@ +// 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 ( + "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-id", + DevWorkspaceName: "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-id", + constants.DevWorkspaceNameLabel: "other-workspace", + }, + }, + }, + }, + expectErr: true, + expectErrType: &ServiceConflictError{}, + expectMsg: "discoverable endpoint 'test-endpoint' is already in use by workspace 'other-workspace'", + }, + { + 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-id", + constants.DevWorkspaceNameLabel: "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-id", + constants.DevWorkspaceNameLabel: "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-id", + DevWorkspaceName: "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-id", + constants.DevWorkspaceNameLabel: "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..bb2d2561f 100644 --- a/controllers/controller/devworkspacerouting/solvers/errors.go +++ b/controllers/controller/devworkspacerouting/solvers/errors.go @@ -17,11 +17,27 @@ package solvers import ( "errors" + "fmt" "time" ) var _ error = (*RoutingNotReady)(nil) 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 { + EndpointName string + WorkspaceName string +} + +func (e *ServiceConflictError) Error() string { + 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) +} // 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..413b3723d 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" @@ -77,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{} @@ -100,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 } 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..ff250ffae 100644 --- a/webhook/workspace/handler/validate.go +++ b/webhook/workspace/handler/validate.go @@ -23,6 +23,9 @@ 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" ) @@ -75,9 +78,56 @@ 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 + } + for _, component := range otherWorkspace.Spec.Template.Components { + if component.Container != nil { + for _, endpoint := range component.Container.Endpoints { + if discoverableEndpoints[endpoint.Name] { + return &solvers.ServiceConflictError{ + EndpointName: endpoint.Name, + WorkspaceName: 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..6dc31e5a0 --- /dev/null +++ b/webhook/workspace/handler/validate_test.go @@ -0,0 +1,193 @@ +// 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 ( + "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("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") + 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.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) { + 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") + }) +}