Skip to content

Commit 04aa51a

Browse files
committed
fix: identical endpoint name conflicts
Signed-off-by: Oleksii Kurinnyi <[email protected]>
1 parent 4699f67 commit 04aa51a

File tree

14 files changed

+615
-12
lines changed

14 files changed

+615
-12
lines changed

controllers/controller/devworkspacerouting/devworkspacerouting_controller.go

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,7 @@ func (r *DevWorkspaceRoutingReconciler) Reconcile(ctx context.Context, req ctrl.
9696
solver, err := r.SolverGetter.GetSolver(r.Client, instance.Spec.RoutingClass)
9797
if err != nil {
9898
if errors.Is(err, solvers.RoutingNotSupported) {
99+
reqLogger.Info("Routing class not supported by this controller, skipping reconciliation", "routingClass", instance.Spec.RoutingClass)
99100
return reconcile.Result{}, nil
100101
}
101102
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.
131132
}
132133

133134
restrictedAccess, setRestrictedAccess := instance.Annotations[constants.DevWorkspaceRestrictedAccessAnnotation]
134-
routingObjects, err := solver.GetSpecObjects(instance, workspaceMeta)
135+
routingObjects, err := solver.GetSpecObjects(instance, workspaceMeta, r.Client, reqLogger)
135136
if err != nil {
136137
var notReady *solvers.RoutingNotReady
137138
if errors.As(err, &notReady) {
@@ -149,6 +150,12 @@ func (r *DevWorkspaceRoutingReconciler) Reconcile(ctx context.Context, req ctrl.
149150
return reconcile.Result{}, r.markRoutingFailed(instance, fmt.Sprintf("Unable to provision networking for DevWorkspace: %s", invalid))
150151
}
151152

153+
var conflict *solvers.ServiceConflictError
154+
if errors.As(err, &conflict) {
155+
reqLogger.Error(conflict, "Routing controller detected a service conflict", "serviceName", conflict.Reason)
156+
return reconcile.Result{}, r.markRoutingFailed(instance, fmt.Sprintf("Unable to provision networking for DevWorkspace: %s", conflict))
157+
}
158+
152159
// generic error, just fail the reconciliation
153160
return reconcile.Result{}, err
154161
}
@@ -208,6 +215,10 @@ func (r *DevWorkspaceRoutingReconciler) Reconcile(ctx context.Context, req ctrl.
208215
if errors.As(err, &failError) {
209216
return reconcile.Result{}, r.markRoutingFailed(instance, err.Error())
210217
}
218+
conflictErr := &solvers.HostnameConflictError{}
219+
if errors.As(err, &conflictErr) {
220+
return reconcile.Result{}, r.markRoutingFailed(instance, conflictErr.Error())
221+
}
211222
reqLogger.Error(err, "Error syncing routes")
212223
return reconcile.Result{Requeue: true}, r.reconcileStatus(instance, nil, nil, false, "Preparing routes")
213224
} else if !routesInSync {
@@ -222,6 +233,10 @@ func (r *DevWorkspaceRoutingReconciler) Reconcile(ctx context.Context, req ctrl.
222233
if errors.As(err, &failError) {
223234
return reconcile.Result{}, r.markRoutingFailed(instance, err.Error())
224235
}
236+
conflictErr := &solvers.HostnameConflictError{}
237+
if errors.As(err, &conflictErr) {
238+
return reconcile.Result{}, r.markRoutingFailed(instance, conflictErr.Error())
239+
}
225240
reqLogger.Error(err, "Error syncing ingresses")
226241
return reconcile.Result{Requeue: true}, r.reconcileStatus(instance, nil, nil, false, "Preparing ingresses")
227242
} else if !ingressesInSync {

controllers/controller/devworkspacerouting/solvers/basic_solver.go

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@ import (
2020
"github.com/devfile/devworkspace-operator/pkg/config"
2121
"github.com/devfile/devworkspace-operator/pkg/constants"
2222
"github.com/devfile/devworkspace-operator/pkg/infrastructure"
23+
"github.com/go-logr/logr"
24+
"sigs.k8s.io/controller-runtime/pkg/client"
2325
)
2426

2527
var routeAnnotations = func(endpointName string, endpointAnnotations map[string]string) map[string]string {
@@ -59,7 +61,7 @@ func (s *BasicSolver) Finalize(*controllerv1alpha1.DevWorkspaceRouting) error {
5961
return nil
6062
}
6163

62-
func (s *BasicSolver) GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRouting, workspaceMeta DevWorkspaceMetadata) (RoutingObjects, error) {
64+
func (s *BasicSolver) GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRouting, workspaceMeta DevWorkspaceMetadata, cl client.Client, log logr.Logger) (RoutingObjects, error) {
6365
routingObjects := RoutingObjects{}
6466

6567
// TODO: Use workspace-scoped ClusterHostSuffix to allow overriding
@@ -70,7 +72,11 @@ func (s *BasicSolver) GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRou
7072

7173
spec := routing.Spec
7274
services := getServicesForEndpoints(spec.Endpoints, workspaceMeta)
73-
services = append(services, GetDiscoverableServicesForEndpoints(spec.Endpoints, workspaceMeta)...)
75+
discoverableServices, err := GetDiscoverableServicesForEndpoints(spec.Endpoints, workspaceMeta, cl, log)
76+
if err != nil {
77+
return RoutingObjects{}, err
78+
}
79+
services = append(services, discoverableServices...)
7480
routingObjects.Services = services
7581
if infrastructure.IsOpenShift() {
7682
routingObjects.Routes = getRoutesForSpec(routingSuffix, spec.Endpoints, workspaceMeta)

controllers/controller/devworkspacerouting/solvers/cluster_solver.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ import (
2424
corev1 "k8s.io/api/core/v1"
2525

2626
controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1"
27+
"github.com/go-logr/logr"
28+
"sigs.k8s.io/controller-runtime/pkg/client"
2729
)
2830

2931
const (
@@ -44,9 +46,14 @@ func (s *ClusterSolver) Finalize(*controllerv1alpha1.DevWorkspaceRouting) error
4446
return nil
4547
}
4648

47-
func (s *ClusterSolver) GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRouting, workspaceMeta DevWorkspaceMetadata) (RoutingObjects, error) {
49+
func (s *ClusterSolver) GetSpecObjects(routing *controllerv1alpha1.DevWorkspaceRouting, workspaceMeta DevWorkspaceMetadata, cl client.Client, log logr.Logger) (RoutingObjects, error) {
4850
spec := routing.Spec
4951
services := getServicesForEndpoints(spec.Endpoints, workspaceMeta)
52+
discoverableServices, err := GetDiscoverableServicesForEndpoints(spec.Endpoints, workspaceMeta, cl, log)
53+
if err != nil {
54+
return RoutingObjects{}, err
55+
}
56+
services = append(services, discoverableServices...)
5057
podAdditions := &controllerv1alpha1.PodAdditions{}
5158
if s.TLS {
5259
readOnlyMode := int32(420)

controllers/controller/devworkspacerouting/solvers/common.go

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,15 @@
1616
package solvers
1717

1818
import (
19+
"context"
20+
"fmt"
21+
1922
controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1"
2023
"github.com/devfile/devworkspace-operator/pkg/common"
2124
"github.com/devfile/devworkspace-operator/pkg/constants"
25+
"github.com/go-logr/logr"
26+
"k8s.io/apimachinery/pkg/api/errors"
27+
"sigs.k8s.io/controller-runtime/pkg/client"
2228

2329
routeV1 "github.com/openshift/api/route/v1"
2430
corev1 "k8s.io/api/core/v1"
@@ -36,7 +42,7 @@ type DevWorkspaceMetadata struct {
3642

3743
// GetDiscoverableServicesForEndpoints converts the endpoint list into a set of services, each corresponding to a single discoverable
3844
// endpoint from the list. Endpoints with the NoneEndpointExposure are ignored.
39-
func GetDiscoverableServicesForEndpoints(endpoints map[string]controllerv1alpha1.EndpointList, meta DevWorkspaceMetadata) []corev1.Service {
45+
func GetDiscoverableServicesForEndpoints(endpoints map[string]controllerv1alpha1.EndpointList, meta DevWorkspaceMetadata, cl client.Client, log logr.Logger) ([]corev1.Service, error) {
4046
var services []corev1.Service
4147
for _, machineEndpoints := range endpoints {
4248
for _, endpoint := range machineEndpoints {
@@ -45,18 +51,36 @@ func GetDiscoverableServicesForEndpoints(endpoints map[string]controllerv1alpha1
4551
}
4652

4753
if endpoint.Attributes.GetBoolean(string(controllerv1alpha1.DiscoverableAttribute), nil) {
48-
// Create service with name matching endpoint
49-
// TODO: This could cause a reconcile conflict if multiple workspaces define the same discoverable endpoint
50-
// Also endpoint names may not be valid as service names
54+
serviceName := common.EndpointName(endpoint.Name)
55+
log.Info("Checking for existing service for discoverable endpoint", "serviceName", serviceName)
56+
existingService := &corev1.Service{}
57+
err := cl.Get(context.TODO(), client.ObjectKey{Name: serviceName, Namespace: meta.Namespace}, existingService)
58+
if err != nil {
59+
if !errors.IsNotFound(err) {
60+
log.Error(err, "Failed to get service from cluster", "serviceName", serviceName)
61+
return nil, err
62+
}
63+
log.Info("No existing service found", "serviceName", serviceName)
64+
} else {
65+
log.Info("Found existing service", "serviceName", serviceName)
66+
if existingService.Labels[constants.DevWorkspaceIDLabel] != meta.DevWorkspaceId {
67+
log.Info("Service conflict detected", "serviceName", serviceName, "existingWorkspaceId", existingService.Labels[constants.DevWorkspaceIDLabel], "currentWorkspaceId", meta.DevWorkspaceId)
68+
return nil, &ServiceConflictError{
69+
Reason: fmt.Sprintf("discoverable endpoint %s conflicts with existing service", endpoint.Name),
70+
}
71+
}
72+
log.Info("Existing service is owned by the same workspace", "serviceName", serviceName)
73+
}
74+
5175
servicePort := corev1.ServicePort{
52-
Name: common.EndpointName(endpoint.Name),
76+
Name: serviceName,
5377
Protocol: corev1.ProtocolTCP,
5478
Port: int32(endpoint.TargetPort),
5579
TargetPort: intstr.FromInt(endpoint.TargetPort),
5680
}
5781
services = append(services, corev1.Service{
5882
ObjectMeta: metav1.ObjectMeta{
59-
Name: common.EndpointName(endpoint.Name),
83+
Name: serviceName,
6084
Namespace: meta.Namespace,
6185
Labels: map[string]string{
6286
constants.DevWorkspaceIDLabel: meta.DevWorkspaceId,
@@ -74,7 +98,7 @@ func GetDiscoverableServicesForEndpoints(endpoints map[string]controllerv1alpha1
7498
}
7599
}
76100
}
77-
return services
101+
return services, nil
78102
}
79103

80104
// GetServiceForEndpoints returns a single service that exposes all endpoints of given exposure types, possibly also including the discoverable types.
Lines changed: 203 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,203 @@
1+
package solvers
2+
3+
import (
4+
"testing"
5+
6+
controllerv1alpha1 "github.com/devfile/devworkspace-operator/apis/controller/v1alpha1"
7+
"github.com/devfile/devworkspace-operator/pkg/constants"
8+
"github.com/stretchr/testify/assert"
9+
corev1 "k8s.io/api/core/v1"
10+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
11+
"k8s.io/apimachinery/pkg/runtime"
12+
"sigs.k8s.io/controller-runtime/pkg/client/fake"
13+
"sigs.k8s.io/controller-runtime/pkg/log/zap"
14+
)
15+
16+
func TestGetDiscoverableServicesForEndpoints(t *testing.T) {
17+
testLog := zap.New(zap.UseDevMode(true))
18+
scheme := runtime.NewScheme()
19+
_ = corev1.AddToScheme(scheme)
20+
_ = controllerv1alpha1.AddToScheme(scheme)
21+
22+
discoverableEndpoint := controllerv1alpha1.Endpoint{
23+
Name: "test-endpoint",
24+
TargetPort: 8080,
25+
Exposure: controllerv1alpha1.PublicEndpointExposure,
26+
Attributes: controllerv1alpha1.Attributes{}.
27+
PutBoolean(string(controllerv1alpha1.DiscoverableAttribute), true),
28+
}
29+
endpoints := map[string]controllerv1alpha1.EndpointList{
30+
"machine1": {discoverableEndpoint},
31+
}
32+
33+
meta := DevWorkspaceMetadata{
34+
DevWorkspaceId: "current-workspace",
35+
Namespace: "test-namespace",
36+
}
37+
38+
tests := []struct {
39+
name string
40+
existing []runtime.Object
41+
expectErr bool
42+
expectErrType error
43+
expectMsg string
44+
}{
45+
{
46+
name: "No existing service",
47+
existing: []runtime.Object{},
48+
expectErr: false,
49+
},
50+
{
51+
name: "Existing service with different owner in same namespace",
52+
existing: []runtime.Object{
53+
&corev1.Service{
54+
ObjectMeta: metav1.ObjectMeta{
55+
Name: "test-endpoint",
56+
Namespace: "test-namespace",
57+
Labels: map[string]string{
58+
constants.DevWorkspaceIDLabel: "other-workspace",
59+
},
60+
},
61+
},
62+
},
63+
expectErr: true,
64+
expectErrType: &ServiceConflictError{},
65+
expectMsg: "conflicts with existing service",
66+
},
67+
{
68+
name: "Existing service with same owner (reconciliation)",
69+
existing: []runtime.Object{
70+
&corev1.Service{
71+
ObjectMeta: metav1.ObjectMeta{
72+
Name: "test-endpoint",
73+
Namespace: "test-namespace",
74+
Labels: map[string]string{
75+
constants.DevWorkspaceIDLabel: "current-workspace",
76+
},
77+
},
78+
},
79+
},
80+
expectErr: false,
81+
},
82+
{
83+
name: "Service with same name in different namespace (should not conflict)",
84+
existing: []runtime.Object{
85+
&corev1.Service{
86+
ObjectMeta: metav1.ObjectMeta{
87+
Name: "test-endpoint",
88+
Namespace: "other-namespace",
89+
Labels: map[string]string{
90+
constants.DevWorkspaceIDLabel: "other-workspace",
91+
},
92+
},
93+
},
94+
},
95+
expectErr: false,
96+
},
97+
{
98+
name: "Service without workspace ID label",
99+
existing: []runtime.Object{
100+
&corev1.Service{
101+
ObjectMeta: metav1.ObjectMeta{
102+
Name: "test-endpoint",
103+
Namespace: "test-namespace",
104+
Labels: map[string]string{},
105+
},
106+
},
107+
},
108+
expectErr: true,
109+
expectErrType: &ServiceConflictError{},
110+
},
111+
}
112+
113+
for _, tt := range tests {
114+
t.Run(tt.name, func(t *testing.T) {
115+
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(tt.existing...).Build()
116+
_, err := GetDiscoverableServicesForEndpoints(endpoints, meta, fakeClient, testLog)
117+
118+
if tt.expectErr {
119+
assert.Error(t, err, "Expected an error but got none")
120+
if tt.expectErrType != nil {
121+
assert.IsType(t, tt.expectErrType, err, "Error is of unexpected type")
122+
}
123+
if tt.expectMsg != "" {
124+
assert.Contains(t, err.Error(), tt.expectMsg)
125+
}
126+
} else {
127+
assert.NoError(t, err, "Got unexpected error")
128+
}
129+
})
130+
}
131+
}
132+
133+
func TestGetDiscoverableServicesForEndpoints_MultipleEndpoints(t *testing.T) {
134+
testLog := zap.New(zap.UseDevMode(true))
135+
scheme := runtime.NewScheme()
136+
_ = corev1.AddToScheme(scheme)
137+
_ = controllerv1alpha1.AddToScheme(scheme)
138+
139+
endpoints := map[string]controllerv1alpha1.EndpointList{
140+
"machine1": {
141+
{
142+
Name: "postgresql",
143+
TargetPort: 5432,
144+
Exposure: controllerv1alpha1.InternalEndpointExposure,
145+
Attributes: controllerv1alpha1.Attributes{}.
146+
PutBoolean(string(controllerv1alpha1.DiscoverableAttribute), true),
147+
},
148+
{
149+
Name: "redis",
150+
TargetPort: 6379,
151+
Exposure: controllerv1alpha1.InternalEndpointExposure,
152+
Attributes: controllerv1alpha1.Attributes{}.
153+
PutBoolean(string(controllerv1alpha1.DiscoverableAttribute), true),
154+
},
155+
{
156+
Name: "http",
157+
TargetPort: 8080,
158+
Exposure: controllerv1alpha1.PublicEndpointExposure,
159+
// Not discoverable
160+
},
161+
},
162+
}
163+
164+
meta := DevWorkspaceMetadata{
165+
DevWorkspaceId: "current-workspace",
166+
Namespace: "test-namespace",
167+
PodSelector: map[string]string{
168+
constants.DevWorkspaceIDLabel: "current-workspace",
169+
},
170+
}
171+
172+
t.Run("Multiple discoverable endpoints without conflicts", func(t *testing.T) {
173+
fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build()
174+
services, err := GetDiscoverableServicesForEndpoints(endpoints, meta, fakeClient, testLog)
175+
assert.NoError(t, err)
176+
assert.Len(t, services, 2, "Should create 2 discoverable services (postgresql and redis, not http)")
177+
178+
serviceNames := make(map[string]bool)
179+
for _, svc := range services {
180+
serviceNames[svc.Name] = true
181+
}
182+
assert.True(t, serviceNames["postgresql"], "Should have postgresql service")
183+
assert.True(t, serviceNames["redis"], "Should have redis service")
184+
assert.False(t, serviceNames["http"], "Should not have http service (not discoverable)")
185+
})
186+
187+
t.Run("Conflict on one of multiple endpoints", func(t *testing.T) {
188+
existingService := &corev1.Service{
189+
ObjectMeta: metav1.ObjectMeta{
190+
Name: "postgresql",
191+
Namespace: "test-namespace",
192+
Labels: map[string]string{
193+
constants.DevWorkspaceIDLabel: "other-workspace",
194+
},
195+
},
196+
}
197+
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(existingService).Build()
198+
_, err := GetDiscoverableServicesForEndpoints(endpoints, meta, fakeClient, testLog)
199+
assert.Error(t, err, "Should error when one endpoint conflicts")
200+
assert.IsType(t, &ServiceConflictError{}, err)
201+
assert.Contains(t, err.Error(), "postgresql")
202+
})
203+
}

0 commit comments

Comments
 (0)