-
Notifications
You must be signed in to change notification settings - Fork 69
fix: identical endpoint name conflicts #1521
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
04aa51a
08a1a5e
88e0d3c
d9682c9
4879878
dc0a395
c7ddd74
91f1014
9de6b51
338237f
654ee01
44d04a8
1ea13cf
7a5ff10
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akurinnoy is there a reason for implementing duplicate endpiont detection in both the webhook and the
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While the webhook prevents the DevWorkspaces (which will cause endpoint conflicts) from being created, the controller checks other cases:
Is this the answer to your question? |
||
| 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. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: devfile/devworkspace-operator
Length of output: 26087
🏁 Script executed:
Repository: devfile/devworkspace-operator
Length of output: 879
🏁 Script executed:
Repository: devfile/devworkspace-operator
Length of output: 6551
🏁 Script executed:
Repository: devfile/devworkspace-operator
Length of output: 13643
🏁 Script executed:
Repository: devfile/devworkspace-operator
Length of output: 6411
🏁 Script executed:
Repository: devfile/devworkspace-operator
Length of output: 179
🏁 Script executed:
Repository: devfile/devworkspace-operator
Length of output: 11248
🏁 Script executed:
Repository: devfile/devworkspace-operator
Length of output: 2871
🏁 Script executed:
Repository: devfile/devworkspace-operator
Length of output: 4589
🏁 Script executed:
Repository: devfile/devworkspace-operator
Length of output: 55
🏁 Script executed:
Repository: devfile/devworkspace-operator
Length of output: 2144
🏁 Script executed:
Repository: devfile/devworkspace-operator
Length of output: 4073
🏁 Script executed:
Repository: devfile/devworkspace-operator
Length of output: 1446
🏁 Script executed:
Repository: devfile/devworkspace-operator
Length of output: 1583
🏁 Script executed:
Repository: devfile/devworkspace-operator
Length of output: 604
🏁 Script executed:
Repository: devfile/devworkspace-operator
Length of output: 1038
🏁 Script executed:
Repository: devfile/devworkspace-operator
Length of output: 3967
Use the parent
DevWorkspace.Nameinstead ofinstance.NameforDevWorkspaceName.DevWorkspaceRoutingobjects are created with names like"routing-{workspaceId}"(based on the workspace ID), but the parentDevWorkspacecan have any arbitrary name. The code currently setsworkspaceMeta.DevWorkspaceName = instance.Name, which passes the routing object's name to solvers for service labeling. This causes a mismatch: services get labeled with the routing name (e.g.,"routing-abc123"), but the webhook's conflict reporting uses the actualDevWorkspace.Name(e.g.,"my-workspace"). When users see conflict errors, the workspace name attributed to the service owner will not match the actual workspace name, making error messages confusing.Since
DevWorkspaceRoutinghas anownerReferencesto its parentDevWorkspace, the parent workspace name should be retrieved from there and used forDevWorkspaceNameinstead ofinstance.Name.🤖 Prompt for AI Agents