feat: Add Python Virtual Environment Support: Add k8s Gateway Configuration#5138
feat: Add Python Virtual Environment Support: Add k8s Gateway Configuration#5138SarahAsad23 wants to merge 11 commits into
Conversation
github-actions
Bot
added
engine
frontend
SarahAsad23
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5138 +/- ##
============================================
- Coverage 47.15% 47.13% -0.02%
+ Complexity 2348 2343 -5
============================================
Files 1042 1042
Lines 39989 39993 +4
Branches 4260 4261 +1
============================================
- Hits 18855 18850 -5
- Misses 20012 20016 +4
- Partials 1122 1127 +5
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
kunwp1
left a comment
kunwp1
left a comment
There was a problem hiding this comment.
Looks good in general. Left some comments.
| @Produces(Array(MediaType.APPLICATION_JSON)) | ||
| def getSystemPackages: util.Map[String, util.List[String]] = { | ||
| def getSystemPackages( | ||
| @QueryParam("isLocal") isLocal: Boolean |
There was a problem hiding this comment.
isLocal describes where the backend is running. I can see that there is a security issue where a malicious user can flip isLocal. I suggest to derive isLocal from KubernetesConfig (there's already a config flag for this) and drop the param.
| private val wsapiWorkflowWebsocket: Regex = """.*/wsapi/workflow-websocket.*""".r | ||
| private val apiExecutionsStats: Regex = """.*/api/executions/[0-9]+/stats/[0-9]+.*""".r | ||
| private val apiExecutionsResultExport: Regex = """.*/api/executions/result/export.*""".r | ||
| private val pveRoute: Regex = """.*/(?:api/|wsapi/)?pve(?:/.*)?""".r |
There was a problem hiding this comment.
Suggested by Claude:
.*/(?:api/|wsapi/)?pve(?:/.*)? is overly permissive — the leading .*/ will match any path ending in …/pve or …/pve/anything, not just the expected /api/pve / /wsapi/pve / /pve shapes. Consistent with how wsapiWorkflowWebsocket / apiExecutionsStats are written above, so not out of line for this file, but the PVE routes here are well-defined enough to anchor more tightly, e.g.:
private val pveRoute: Regex = """^/?(?:auth/)?(?:api/|wsapi/)?pve(?:/.*)?$""".rAlso applies to pvePvesCuidPath and pvePackagesCuidPath below. Worth double-checking whether uriInfo.getPath here includes the auth/ prefix from the enclosing @Path("/auth") resource — your manual test probably already covered this, but the regex shape depends on it.
| path match { | ||
| case wsapiWorkflowWebsocket() | apiExecutionsStats() | apiExecutionsResultExport() => | ||
| case wsapiWorkflowWebsocket() | apiExecutionsStats() | apiExecutionsResultExport() | | ||
| pveRoute() => |
There was a problem hiding this comment.
PR description says "Tested manually." Worth adding a small unit test on AccessControlResource.authorize that covers such as:
/pve/system?cuid=N→ 200 (query-string cuid)/pve/pves/N→ 200 (path-segment cuid via the DELETE route)/pve/N/myenv/packages/numpy→ 200 (path-segment cuid via the packages route)/pve/no-cuid-anywhere→ 403 (cuid extraction falls through to empty → NumberFormatException → FORBIDDEN)- a non-PVE garbage path → 403
3 participants
What changes were proposed in this PR?
This PR is an extension of PR #4484, #4902, #5035, and #5069. It adds Kubernetes gateway routing and access control configurations.
Any related issues, documentation, discussions?
This change is part of ongoing efforts to support environment isolation and reproducibility within Texera. Related issue includes #4296. This PR closes sub-issue #5137.
How was this PR tested?
Tested manually.
Was this PR authored or co-authored using generative AI tooling?
Co-authored using: Claude Code (claude-opus-4-7)