[Feature] Final Built-In Ingress Support with Host, Path, PathType, and TLS Fields#4901
[Feature] Final Built-In Ingress Support with Host, Path, PathType, and TLS Fields#4901alimaazamat wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef04c2813f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR extends KubeRay’s built-in (Kubernetes) Ingress support by allowing users to configure the generated Ingress rule host/path/pathType and TLS entries via new HeadGroupSpec fields, and ensures existing Ingress resources get updated when the desired spec changes.
Changes:
- Add new API fields on
HeadGroupSpec:ingressHost,ingressPath,ingressPathType, andingressTLS(plus corresponding enum/type definitions). - Update Ingress construction to honor these fields (host, path, pathType, TLS).
- Reconcile existing Ingresses by updating labels/annotations/spec when they drift from the desired Ingress, with unit tests covering the update helper and the new builder behavior.
Reviewed changes
Copilot reviewed 14 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ray-operator/pkg/client/applyconfiguration/utils.go | Registers apply-configuration type lookup for IngressTLSConfig. |
| ray-operator/pkg/client/applyconfiguration/ray/v1/ingresstlsconfig.go | Adds generated apply-configuration for the new IngressTLSConfig type. |
| ray-operator/pkg/client/applyconfiguration/ray/v1/headgroupspec.go | Adds generated apply-configuration fields/builders for ingress host/path/pathType/TLS. |
| ray-operator/controllers/ray/raycluster_controller.go | Updates ingress reconciliation to update an existing Ingress when desired fields change; adds ingressNeedsUpdate. |
| ray-operator/controllers/ray/raycluster_controller_test.go | Adds unit tests for ingressNeedsUpdate. |
| ray-operator/controllers/ray/common/ingress.go | Updates ingress builder to consume new HeadGroupSpec ingress fields (host/path/pathType/TLS). |
| ray-operator/controllers/ray/common/ingress_test.go | Adds coverage for building Ingress with the new ingress fields set. |
| ray-operator/config/crd/bases/ray.io_rayservices.yaml | Exposes new ingress fields in RayService CRD schema. |
| ray-operator/config/crd/bases/ray.io_rayjobs.yaml | Exposes new ingress fields in RayJob CRD schema. |
| ray-operator/config/crd/bases/ray.io_raycronjobs.yaml | Exposes new ingress fields in RayCronJob CRD schema. |
| ray-operator/config/crd/bases/ray.io_rayclusters.yaml | Exposes new ingress fields in RayCluster CRD schema. |
| ray-operator/apis/ray/v1/zz_generated.deepcopy.go | Adds deepcopy support for new ingress-related API fields/types. |
| ray-operator/apis/ray/v1/raycluster_types.go | Introduces IngressPathType enum and IngressTLSConfig, and wires them into HeadGroupSpec. |
| helm-chart/kuberay-operator/crds/ray.io_rayservices.yaml | Mirrors CRD schema updates for Helm distribution. |
| helm-chart/kuberay-operator/crds/ray.io_rayjobs.yaml | Mirrors CRD schema updates for Helm distribution. |
| helm-chart/kuberay-operator/crds/ray.io_raycronjobs.yaml | Mirrors CRD schema updates for Helm distribution. |
| helm-chart/kuberay-operator/crds/ray.io_rayclusters.yaml | Mirrors CRD schema updates for Helm distribution. |
| docs/reference/api.md | Documents the new ingress fields and new IngressPathType/IngressTLSConfig API types. |
Files not reviewed (4)
- ray-operator/apis/ray/v1/zz_generated.deepcopy.go: Language not supported
- ray-operator/pkg/client/applyconfiguration/ray/v1/headgroupspec.go: Language not supported
- ray-operator/pkg/client/applyconfiguration/ray/v1/ingresstlsconfig.go: Language not supported
- ray-operator/pkg/client/applyconfiguration/utils.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
win5923
left a comment
There was a problem hiding this comment.
Thanks for the PR! Will review later, have you tested this with different ingress controllers?
|
cc @Tom-Newton, since you've raised on ingress-related PRs before, I'd be interested in your thoughts on whether this approach makes sense. |
Thanks for working on this. I think a lot of users will appreciate improved ingress support. I haven't looked particularly throroughly, but I think this PR does not make ingress annotations configurable, so it would not be sufficient for us. We have been using a patched version of kuberay that includes #3871. |
I think RayCluster's annotations are propagated to the generated Ingress. Would that address this use case, or is there a reason we need ingress-specific annotations instead? |
Good point. I think that should work for us. |
|
Actually there is another challenge we have. We would like to configure these settings on the helm chart level, not on the |
What stops you from doing that on the helm chart? |
Maybe I've misunderstood, but it looks like this PR adds the configs to |
|
@Tom-Newton
|
Fair enough. We'll probably just keep using our patched version. |
Signed-off-by: alimaazamat <alima.azamat2003@gmail.com>
Signed-off-by: alimaazamat <alima.azamat2003@gmail.com>
Signed-off-by: alimaazamat <alima.azamat2003@gmail.com>
… comparisons Signed-off-by: alimaazamat <alima.azamat2003@gmail.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit f2987da. Configure here.
| SecretName: tls.SecretName, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Empty TLS causes update loop
High Severity
When no TLS is configured, BuildIngressForHeadService sets spec.tls to a non-nil empty slice, while the API usually stores that field as nil. The new ingressNeedsUpdate path uses reflect.DeepEqual on the full spec, so those forms never match and the controller can issue an ingress update on every reconcile.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f2987da. Configure here.



Why are these changes needed?
Make the built in ingress support more configurable as the FINAL ingress support in KubeRay.
Support new fields when present, users can now override the host, path, pathtype, and TLS entries.
Since ingress is deprecated this is the final support we will provide. No more advancements will happen so we don't really need to think about future support for this.
Related issue number
Closes #4598
Checks