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 10 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 43.66% 43.63% -0.04%
+ Complexity 2218 2216 -2
============================================
Files 1049 1049
Lines 40580 40561 -19
Branches 4324 4325 +1
============================================
- Hits 17719 17698 -21
+ Misses 21766 21765 -1
- Partials 1095 1098 +3
*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)