Feat: Make RayCluster head node ingress optional #6852
Conversation
Signed-off-by: Fabio Grätz <fabio@cusp.ai>
fg91
added
security
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6852 +/- ##
==========================================
- Coverage 56.93% 56.21% -0.72%
==========================================
Files 929 772 -157
Lines 58144 47369 -10775
==========================================
- Hits 33102 26627 -6475
+ Misses 22000 18320 -3680
+ Partials 3042 2422 -620
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| core.K8sPod k8s_pod = 2; | ||
|
|
||
| // Optional. Whether to enable an ingress on the ray cluster head node | ||
| bool enable_ingress = 3; |
There was a problem hiding this comment.
Why don't we add it to the plugin config?
There was a problem hiding this comment.
I added it to the head node config because that's where this is configured in the RayCluster CRD and some of the other fields of the head node config can be configured from python too.
But I don't have a strong opinion, could change to the plugin config instead of the task_config in the task decorator if you prefer.
My main concern is that it should be disabled by default and opt-in to not accidentally expose things :)
There was a problem hiding this comment.
I think this config is more related to the cluster settings, so putting it in the plugin config map might be better.
There was a problem hiding this comment.
We could add it to the Ray proto in the future if anyone wants to turn it off for certain tasks only, although i think it's not the common use case.
There was a problem hiding this comment.
Agree. Can you please take another look?
1f5adfd
…buf message Signed-off-by: Fabio Grätz <fabio@cusp.ai>
| enableIngress := true | ||
| cfg := GetConfig() | ||
|
|
||
| enableIngress := cfg.EnableIngress |
There was a problem hiding this comment.
nit: I think we can put EnableIngress: & cfg.EnableIngress directly in HeadGroupSpec, as it's the only place that use this
There was a problem hiding this comment.
We could but the ServiceType - which is also part of the RayCluster's HeadGroupSpec - is also in our top level plugin config. My preference would be to try to keep it consistent here at least?
This is not a strong opinion though.
Why are the changes needed?
Currently, the flytepropeller ray plugin hardcodes the creation of an ingress for the ray cluster's head node, see here.
This is a potential security problem as the ray head node might be exposed to the public internet in clusters with a default ingress class without the user knowing.
What changes were proposed in this pull request?
EnableIngressto the ray plugin configHow was this patch tested?
Check all the applicable boxes
Related PRs
flyteorg/flytekit#3365