Skip to content

Commit 82e568d

Browse files
committed
Address Copilot PR review feedback across 11 files
- Fix job status vocabulary: standardize on succeeded/failed/cancelled across backend SSE stream, frontend page, and API schemas - Fix jobs page using wrong field name (type → job_type) to match API - Add SSE auth support: get_current_user now accepts token via query param for EventSource which cannot set Authorization headers - Fix workspace status vocabulary: add creating to filter, remove unused starting/stopping, sync creating phase from CRD - Return HTTP 500 when workspace CRD creation fails instead of 201 - Fix NodePort default range (31299 → 31209) to match Kind config - Make workspace access_url configurable via WORKSPACE_ACCESS_BASE_URL - Add datashader/geopandas auto-detection to SDK visualization module - Pin workspace Dockerfile base image to python-3.11 (was :latest) - Add security comments to Dockerfile and RBAC noting production needs - Fix conftest.py default DB credentials (gacwr → openuba) - Log warnings on operator handler import failures instead of silently swallowing errors - Fix port-forward.sh comment claiming setsid when not used - Only emit SSE status events when status actually changes
1 parent b7c30a1 commit 82e568d

11 files changed

Lines changed: 77 additions & 35 deletions

File tree

core/api_routers/jobs.py

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,7 @@ async def stream_job_metrics(
198198

199199
async def event_generator():
200200
last_count = 0
201+
last_status = job.status
201202
while True:
202203
if await request.is_disconnected():
203204
break
@@ -220,20 +221,22 @@ async def event_generator():
220221
}
221222
last_count = current_count
222223

223-
# also send job status updates
224+
# send job status updates only when something changed
224225
job_now = repo.get_by_id(job_id)
225226
if job_now:
226-
yield {
227-
"event": "status",
228-
"data": json.dumps({
229-
"status": job_now.status,
230-
"progress": job_now.progress,
231-
"epoch_current": job_now.epoch_current,
232-
"epoch_total": job_now.epoch_total,
233-
"loss": job_now.loss,
234-
})
235-
}
236-
if job_now.status in ("completed", "failed", "cancelled"):
227+
if job_now.status != last_status or current_count > last_count:
228+
yield {
229+
"event": "status",
230+
"data": json.dumps({
231+
"status": job_now.status,
232+
"progress": job_now.progress,
233+
"epoch_current": job_now.epoch_current,
234+
"epoch_total": job_now.epoch_total,
235+
"loss": job_now.loss,
236+
})
237+
}
238+
last_status = job_now.status
239+
if job_now.status in ("succeeded", "failed", "cancelled"):
237240
yield {"event": "done", "data": json.dumps({"status": job_now.status})}
238241
break
239242

core/api_routers/workspaces.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,13 +102,21 @@ async def launch_workspace(
102102
repo = WorkspaceRepository(db)
103103
_create_workspace_crd(workspace, repo, workspace_token=workspace_token)
104104

105+
# re-read workspace to reflect any status changes from CRD creation
106+
workspace = repo.get_by_id(workspace.id)
107+
if workspace.status == "failed":
108+
raise HTTPException(
109+
status_code=500,
110+
detail=workspace.error_message or "failed to create workspace CRD"
111+
)
112+
105113
logger.info(f"workspace launched: {workspace.id}")
106114
return workspace
107115

108116

109117
@router.get("/workspaces", response_model=List[WorkspaceResponse])
110118
async def list_workspaces(
111-
status: Optional[str] = Query(None, pattern="^(pending|starting|running|stopping|stopped|failed)$"),
119+
status: Optional[str] = Query(None, pattern="^(pending|creating|running|stopped|failed)$"),
112120
limit: int = Query(100, ge=1, le=1000),
113121
offset: int = Query(0, ge=0),
114122
db: Session = Depends(get_db),
@@ -188,6 +196,8 @@ def _sync_workspace_crd_status(workspace, repo: WorkspaceRepository):
188196
access_url=cr_status.get("access_url", workspace.access_url),
189197
node_port=cr_status.get("node_port", workspace.node_port),
190198
)
199+
elif cr_phase == "creating" and workspace.status == "pending":
200+
workspace = repo.update(workspace.id, status="creating")
191201
elif cr_phase == "failed" and workspace.status != "failed":
192202
workspace = repo.update(
193203
workspace.id,

core/auth.py

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
import logging
88
from typing import Optional
99
from datetime import datetime, timedelta
10-
from fastapi import Depends, HTTPException, status
10+
from fastapi import Depends, HTTPException, Request, status
1111
from fastapi.security import HTTPBearer, HTTPAuthorizationCredentials
1212
from jose import JWTError, jwt
1313
from passlib.context import CryptContext
@@ -54,24 +54,33 @@ def create_access_token(data: dict, expires_delta: Optional[timedelta] = None) -
5454

5555

5656
async def get_current_user(
57-
credentials: Optional[HTTPAuthorizationCredentials] = Depends(security)
57+
request: Request,
58+
credentials: Optional[HTTPAuthorizationCredentials] = Depends(security),
5859
) -> dict:
5960
'''
6061
get current user from jwt token
6162
validates that the user still exists in the database (handles stale tokens
6263
after database resets)
64+
supports token via Authorization header or query param (for SSE/EventSource)
6365
'''
6466
credentials_exception = HTTPException(
6567
status_code=status.HTTP_401_UNAUTHORIZED,
6668
detail="could not validate credentials",
6769
headers={"WWW-Authenticate": "Bearer"},
6870
)
6971

70-
if credentials is None:
72+
# try bearer token from header first, fall back to query param
73+
# query param is needed for SSE (EventSource can't set headers)
74+
token = None
75+
if credentials:
76+
token = credentials.credentials
77+
else:
78+
token = request.query_params.get("token")
79+
80+
if not token:
7181
raise credentials_exception
7282

7383
try:
74-
token = credentials.credentials
7584
payload = jwt.decode(token, SECRET_KEY, algorithms=[ALGORITHM])
7685
username: str = payload.get("sub")
7786
if username is None:

core/operator/main.py

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,24 +3,25 @@
33
import yaml
44
import os
55
import json
6+
import logging
67

78
# import workspace handler to register its kopf handlers
89
try:
910
import workspace_handler # noqa: F401
1011
except ImportError:
1112
try:
1213
import core.operator.workspace_handler # noqa: F401
13-
except ImportError:
14-
pass
14+
except ImportError as e:
15+
logging.getLogger(__name__).warning(f"workspace handler not available: {e}")
1516

1617
# import pipeline handler to register its kopf handlers
1718
try:
1819
import pipeline_handler # noqa: F401
1920
except ImportError:
2021
try:
2122
import core.operator.pipeline_handler # noqa: F401
22-
except ImportError:
23-
pass
23+
except ImportError as e:
24+
logging.getLogger(__name__).warning(f"pipeline handler not available: {e}")
2425

2526
# Setup K8s client
2627
if os.getenv("KUBERNETES_SERVICE_HOST"):

core/services/workspace_service.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@
3737

3838
# nodeport range for workspaces
3939
NODE_PORT_START = int(os.getenv("WORKSPACE_NODE_PORT_START", "31200"))
40-
NODE_PORT_END = int(os.getenv("WORKSPACE_NODE_PORT_END", "31299"))
40+
NODE_PORT_END = int(os.getenv("WORKSPACE_NODE_PORT_END", "31209"))
4141

4242

4343
class WorkspaceService:
@@ -78,7 +78,8 @@ def launch_workspace(
7878

7979
# set allocated nodeport and access_url
8080
# access_url is set now so the frontend can start probing immediately
81-
access_url = f"http://localhost:{node_port}"
81+
base_url = os.getenv("WORKSPACE_ACCESS_BASE_URL", "http://localhost")
82+
access_url = f"{base_url}:{node_port}"
8283
workspace = self.repo.update(
8384
workspace.id,
8485
node_port=node_port,

core/tests/e2e/conftest.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
E2E_BACKEND_PORT = int(os.getenv("E2E_BACKEND_PORT", "8000"))
2525
E2E_DATABASE_URL = os.getenv(
2626
"E2E_DATABASE_URL",
27-
"postgresql://gacwr:gacwr@localhost:5432/openuba"
27+
"postgresql://openuba:openuba@localhost:5432/openuba"
2828
)
2929

3030

docker/workspace/Dockerfile

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
FROM jupyter/scipy-notebook:latest
1+
FROM jupyter/scipy-notebook:python-3.11
22

33
USER root
44

@@ -36,6 +36,9 @@ RUN pip install --no-cache-dir \
3636

3737
# configure JupyterLab for iframe embedding (token-less, XSRF-disabled, CSP-allowed)
3838
# this allows the OpenUBA frontend to embed JupyterLab seamlessly in an iframe
39+
# SECURITY: this is for local development only. For production, use per-workspace
40+
# tokens (via OPENUBA_TOKEN / K8s Secret), restrict allow_origin to the frontend
41+
# origin, and keep XSRF enabled.
3942
# NOTE: we append to the base image config AND write a user-level config to ensure
4043
# our settings take effect regardless of base image entrypoint behavior
4144
RUN cat >> /etc/jupyter/jupyter_server_config.py << 'JCONF'

interface/app/jobs/page.tsx

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,9 @@ const API_URL = process.env.NEXT_PUBLIC_API_URL || 'http://localhost:8000'
1010
const statusColors: Record<string, string> = {
1111
pending: 'bg-yellow-500/20 text-yellow-400',
1212
running: 'bg-blue-500/20 text-blue-400',
13-
completed: 'bg-green-500/20 text-green-400',
13+
succeeded: 'bg-green-500/20 text-green-400',
1414
failed: 'bg-red-500/20 text-red-400',
15-
stopped: 'bg-gray-500/20 text-gray-400',
15+
cancelled: 'bg-gray-500/20 text-gray-400',
1616
}
1717

1818
const typeColors: Record<string, string> = {
@@ -23,7 +23,7 @@ const typeColors: Record<string, string> = {
2323

2424
interface Job {
2525
id: string
26-
type: string
26+
job_type: string
2727
model?: string
2828
model_id?: string
2929
status: string
@@ -97,8 +97,8 @@ export default function JobsPage() {
9797
{jobs.map((job) => (
9898
<tr key={job.id} className="hover:bg-muted/20 transition-colors">
9999
<td className="px-4 py-3">
100-
<span className={`inline-flex items-center rounded-full px-2 py-0.5 text-xs font-medium ${typeColors[job.type] || 'bg-gray-500/20 text-gray-400'}`}>
101-
{job.type}
100+
<span className={`inline-flex items-center rounded-full px-2 py-0.5 text-xs font-medium ${typeColors[job.job_type] || 'bg-gray-500/20 text-gray-400'}`}>
101+
{job.job_type}
102102
</span>
103103
</td>
104104
<td className="px-4 py-3 font-mono text-xs">
@@ -114,15 +114,15 @@ export default function JobsPage() {
114114
<div className="h-1.5 w-20 rounded-full bg-muted/40 overflow-hidden">
115115
<div
116116
className={`h-full rounded-full transition-all ${
117-
job.status === 'completed' ? 'bg-green-500' :
117+
job.status === 'succeeded' ? 'bg-green-500' :
118118
job.status === 'failed' ? 'bg-red-500' :
119119
'bg-blue-500'
120120
}`}
121-
style={{ width: `${job.progress ?? (job.status === 'completed' ? 100 : 0)}%` }}
121+
style={{ width: `${job.progress ?? (job.status === 'succeeded' ? 100 : 0)}%` }}
122122
/>
123123
</div>
124124
<span className="text-xs text-muted-foreground tabular-nums">
125-
{job.progress ?? (job.status === 'completed' ? 100 : 0)}%
125+
{job.progress ?? (job.status === 'succeeded' ? 100 : 0)}%
126126
</span>
127127
</div>
128128
</td>

k8s/operator-rbac.yaml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,9 @@ apiVersion: rbac.authorization.k8s.io/v1
88
kind: ClusterRole
99
metadata:
1010
name: openuba-operator-role
11+
# NOTE: ClusterRole is required because CRDs (openuba.io/*) are cluster-scoped.
12+
# The operator only manages resources in the openuba namespace (enforced in code).
13+
# For multi-tenant production, consider namespace-scoped Roles for core resources.
1114
rules:
1215
# Framework: knowing which other operators are running (i.e. peering).
1316
- apiGroups: [kopf.dev]

scripts/port-forward.sh

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
#!/bin/bash
22
# scripts/port-forward.sh
3-
# Starts kubectl port-forwards as a persistent background process group.
4-
# Survives parent shell/make exit via setsid.
3+
# Starts kubectl port-forwards as background processes.
4+
# Lives as long as this script is running (killed when parent exits).
55

66
pkill -f "kubectl.*port-forward.*openuba" 2>/dev/null || true
77
sleep 1

0 commit comments

Comments
 (0)