fix: include user-defined ports in driver Service#2895
fix: include user-defined ports in driver Service#2895YassinNouh21 wants to merge 2 commits intokubeflow:masterfrom
Conversation
When ports are specified in spec.driver.ports, they are correctly added to the driver pod's container spec via the webhook, but the generated driver Service only includes the Spark UI port. This makes user-defined ports unreachable via the Service, breaking service mesh strict mode and custom metrics endpoints. Extract port list construction into buildDriverServicePorts() which appends spec.driver.ports entries alongside the primary Spark UI port. Fixes kubeflow#2891 Signed-off-by: yassinnouh21 <yassinnouh21@gmail.com>
|
🎉 Welcome to the Kubeflow Spark Operator! 🎉 Thanks for opening your first PR! We're happy to have you as part of our community 🚀 Here's what happens next:
Join the community:
Feel free to ask questions in the comments if you need any help or clarification! |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR fixes a gap in the SparkApplication controller where user-defined driver ports (spec.driver.ports) were added to the driver pod but were not exposed on the generated driver/UI Service, making those ports unreachable via the Service.
Changes:
- Extracted driver Service port construction into a helper (
buildDriverServicePorts()). - Updated driver Service creation to include both the primary Spark UI port and any
spec.driver.portsentries.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for _, p := range app.Spec.Driver.Ports { | ||
| ports = append(ports, corev1.ServicePort{ | ||
| Name: p.Name, | ||
| Port: p.ContainerPort, | ||
| TargetPort: intstr.FromInt32(p.ContainerPort), | ||
| Protocol: corev1.Protocol(p.Protocol), | ||
| }) |
There was a problem hiding this comment.
spec.driver.ports entries are appended verbatim, which can make the Service invalid if a user-defined port collides with the primary port (same port+protocol) or if two user-defined ports reuse the same .name. Kubernetes rejects Services with duplicate spec.ports[].name and (for the same protocol) duplicate spec.ports[].port, so reconciliation would fail for such apps. Consider de-duplicating/validating while building the list (e.g., skip or surface a clear error when a name or port+protocol is already used, including collisions with the primary UI port).
|
|
||
| for _, p := range app.Spec.Driver.Ports { | ||
| ports = append(ports, corev1.ServicePort{ | ||
| Name: p.Name, | ||
| Port: p.ContainerPort, | ||
| TargetPort: intstr.FromInt32(p.ContainerPort), | ||
| Protocol: corev1.Protocol(p.Protocol), | ||
| }) |
There was a problem hiding this comment.
This change introduces new behavior (propagating spec.driver.ports onto the generated driver/UI Service), but there are no unit tests asserting the additional ServicePorts are present. Adding a test case that sets app.Spec.Driver.Ports and verifies the created Service contains both the primary port and the custom ports (and ideally a collision case) would prevent regressions.
| for _, p := range app.Spec.Driver.Ports { | |
| ports = append(ports, corev1.ServicePort{ | |
| Name: p.Name, | |
| Port: p.ContainerPort, | |
| TargetPort: intstr.FromInt32(p.ContainerPort), | |
| Protocol: corev1.Protocol(p.Protocol), | |
| }) | |
| // Track used port names and numbers to avoid collisions between the primary | |
| // ServicePort and any user-defined ports from app.Spec.Driver.Ports. | |
| usedNames := map[string]struct{}{ | |
| portName: {}, | |
| } | |
| usedPorts := map[int32]struct{}{ | |
| port: {}, | |
| } | |
| for _, p := range app.Spec.Driver.Ports { | |
| // Skip ports that would collide with an existing ServicePort by name or port. | |
| if _, exists := usedNames[p.Name]; exists { | |
| continue | |
| } | |
| if _, exists := usedPorts[p.ContainerPort]; exists { | |
| continue | |
| } | |
| ports = append(ports, corev1.ServicePort{ | |
| Name: p.Name, | |
| Port: p.ContainerPort, | |
| TargetPort: intstr.FromInt32(p.ContainerPort), | |
| Protocol: corev1.Protocol(p.Protocol), | |
| }) | |
| usedNames[p.Name] = struct{}{} | |
| usedPorts[p.ContainerPort] = struct{}{} |
Address Copilot review feedback: - Skip user-defined ports that collide with the primary port by name or port number to prevent Kubernetes Service rejection - Add unit tests for buildDriverServicePorts covering: no user ports, appended user ports, duplicate port number, and duplicate port name Signed-off-by: yassinnouh21 <yassinnouh21@gmail.com>
Summary
spec.driver.portsare correctly added to driver pod containers via the webhook, but the generated driver Service only includes the Spark UI port (4040)buildDriverServicePorts()which appendsspec.driver.portsentries alongside the primary Spark UI portFixes #2891
Reproduction & Verification
Tested on a Kind cluster with spark-operator built from this branch:
Before (bug):
After (fix):
Test plan
go build ./...passes