fix: allow same ServicePort with different hosts in driverIngressOptions#2835
fix: allow same ServicePort with different hosts in driverIngressOptions#2835puwun wants to merge 1 commit intokubeflow:masterfrom
Conversation
|
[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 an issue where users could not expose the same Spark driver port (e.g., 4040 for Spark UI) through multiple ingresses with different hostnames. The validation logic has been updated to allow this configuration, and naming has been adjusted to ensure unique resource names.
Changes:
- Modified validation to check for duplicate
(ServicePort, IngressURLFormat)pairs instead of rejecting duplicateServicePortvalues - Updated service and ingress naming to include an index suffix when multiple entries share the same port
- Added validation tests for the new behavior
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/webhook/sparkapplication_validator.go | Removed duplicate ServicePort check, added check for duplicate IngressURLFormat, and added redundant validation for (port, url) pairs |
| internal/webhook/sparkapplication_validator_test.go | Renamed test to reflect new behavior allowing same port with different hosts, added test for duplicate host validation |
| internal/controller/sparkapplication/controller.go | Added index parameter to driver ingress creation calls in the reconciliation loop |
| internal/controller/sparkapplication/driveringress.go | Added index parameter to naming functions and updated service/ingress name generation to include index suffix for non-zero indices |
| internal/controller/sparkapplication/driveringress_test.go | Updated test calls to include index parameter (0) for existing tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e3d9edf to
afbc0b6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Allow users to configure multiple driverIngressOptions entries with the same ServicePort but different IngressURLFormat values. Fixes kubeflow#2662 Signed-off-by: Pavan More <pavansmore05@gmail.com>
afbc0b6 to
860c9c4
Compare
nabuskey
left a comment
There was a problem hiding this comment.
Why do you want to use index for ingress name uniqueness?
Fixes #2662
Purpose of this PR
Users want to expose the same Spark driver port (e.g 4040 for Spark UI) through multiple ingresses with different hostnames. The current validation incorrectly rejects this configuration as "duplicate ServicePort".
Proposed changes:
(ServicePort, IngressURLFormat)pairs instead of justServicePort, allowing same port with different hostsChange Category
Checklist
Additional Notes
With this fix, the following configuration is now valid: