mengw15 commented on code in PR #8073:
URL: https://github.com/apache/texera/pull/8073#discussion_r3931673108


##########
bin/k8s/values.yaml:
##########
@@ -195,6 +195,61 @@ webserver:
     type: ClusterIP
     port: 8080
 
+notebookMigrationService:
+  # Turns the whole notebook migration tool on or off: the service, its route, 
the per-user
+  # JupyterLab pool, and the button in the workspace.
+  enabled: false
+  name: notebook-migration-service
+  numOfPods: 1
+  serviceAccountName: notebook-migration-service-service-account
+  imageName: texera-notebook-migration-service
+  service:
+    type: ClusterIP
+    port: 9098
+  # Origin the browser reaches Texera on, used for the JupyterLab iframe URL 
and for the
+  # CSP that lets Texera embed it. Required wherever there is no DNS name, 
such as a
+  # port-forward or a NodePort. Falls back to the gateway hostname when left 
empty.
+  publicOrigin: ""
+  # HMAC key each user's JupyterLab token is derived from. Nothing is stored, 
so this must
+  # stay stable across restarts or previously issued tokens stop matching.
+  # Development-only default. Production environments MUST override this with 
a different,
+  # securely generated secret.
+  jupyterTokenSecret: 
"c4e1f7a9b2d5c8e0f3a6b9d2e5f8a1c4b7d0e3f6a9c2b5d8e1f4a7c0b3d6e9f2"

Review Comment:
   This ships a real HMAC key in the public repo as the default. #8032's 
startup guard refuses an empty secret precisely because a public key lets 
anyone derive any user's token — a published default is exactly as public, but 
sails past the guard, and the failure is silent. On any deployment that flips 
`enabled` without overriding this, anyone can compute `HMAC(default, uid)` for 
every uid, and `/jupyter/<uid>/` is deliberately unauthenticated at the 
gateway, so the token is the entire wall.
   
   `required` would move the failure to install time, matching the guard's 
philosophy; values-development.yaml (which doesn't set this today) can then 
carry an explicitly dev-scoped value so the Minikube flow keeps working.



##########
bin/k8s/templates/base/notebook-migration-service/notebook-migration-service-deployment.yaml:
##########
@@ -0,0 +1,99 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+{{- if .Values.notebookMigrationService.enabled }}
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: {{ .Release.Name }}-{{ .Values.notebookMigrationService.name }}
+  namespace: {{ .Release.Namespace }}
+  labels:
+    app: {{ .Release.Name }}-{{ .Values.notebookMigrationService.name }}
+spec:
+  replicas: {{ .Values.notebookMigrationService.numOfPods | default 1 }}
+  selector:
+    matchLabels:
+      app: {{ .Release.Name }}-{{ .Values.notebookMigrationService.name }}
+  template:
+    metadata:
+      labels:
+        app: {{ .Release.Name }}-{{ .Values.notebookMigrationService.name }}
+    spec:
+      # Needed to create and delete each user's JupyterLab pod in the pool 
namespace.
+      serviceAccountName: {{ 
.Values.notebookMigrationService.serviceAccountName }}
+      containers:
+        - name: {{ .Values.notebookMigrationService.name }}
+          image: {{ .Values.texera.imageRegistry }}/{{ 
.Values.notebookMigrationService.imageName }}:{{ .Values.texera.imageTag }}
+          imagePullPolicy: {{ .Values.texeraImages.pullPolicy }}
+          ports:
+            - containerPort: {{ .Values.notebookMigrationService.service.port 
}}
+          env:
+            - name: STORAGE_JDBC_URL
+              value: jdbc:postgresql://{{ .Release.Name 
}}-postgresql:5432/texera_db?currentSchema=texera_db,public
+            - name: STORAGE_JDBC_PASSWORD
+              valueFrom:
+                secretKeyRef:
+                  name: {{ .Release.Name }}-postgresql
+                  key: postgres-password
+            # Resolve each user's JupyterLab rather than one shared server.
+            - name: KUBERNETES_JUPYTER_ENABLED
+              value: "true"
+            - name: KUBERNETES_JUPYTER_NAMESPACE
+              value: {{ .Values.jupyterPool.namespace }}
+            - name: KUBERNETES_JUPYTER_SERVICE_NAME
+              value: {{ .Values.jupyterPool.name }}-svc
+            - name: KUBERNETES_JUPYTER_IMAGE_NAME
+              value: {{ .Values.texera.imageRegistry }}/{{ 
.Values.jupyterPool.imageName }}:{{ .Values.texera.imageTag }}
+            - name: KUBERNETES_JUPYTER_CPU_LIMIT
+              value: "{{ .Values.jupyterPool.resources.cpuLimit }}"
+            - name: KUBERNETES_JUPYTER_MEMORY_LIMIT
+              value: {{ .Values.jupyterPool.resources.memoryLimit }}
+            # The pod's own prefix and the browser-facing address are rendered 
from one
+            # basePath, so they cannot drift apart.
+            - name: KUBERNETES_JUPYTER_BASE_URL
+              value: {{ .Values.jupyterPool.basePath }}
+            {{- $origin := .Values.notebookMigrationService.publicOrigin }}
+            {{- if and (not $origin) .Values.gatewayConfig 
.Values.gatewayConfig.hostname }}
+            {{- $origin = printf "https://%s"; .Values.gatewayConfig.hostname }}

Review Comment:
   The scheme can't be inferred from the hostname alone: the chart declares 
both listeners unconditionally, so `hostname` without a cert issuer is an HTTP 
deployment, and this renders an https:// origin that nothing serves — CSP and 
allow_origin then reject the iframe with no error surfaced anywhere. With TLS 
terminated at an external LB the guess is right, so it's genuinely undecidable 
from here.
   
   Not blocking: gating the inference on TLS actually being configured 
(issuer/tlsSecretName), or requiring `publicOrigin` when it isn't, would make 
the broken case fail loud instead of silently eating the iframe.



##########
bin/k8s/templates/base/gateway/gateway-routes.yaml:
##########
@@ -135,6 +148,13 @@ spec:
         - path:
             type: PathPrefix
             value: /api/pve
+        {{- if .Values.notebookMigrationService.enabled }}
+        # Per-user JupyterLab. ExtAuthz reads the uid from the path and 
rewrites Host to
+        # that user's pod; the per-user Jupyter token is what authorizes the 
request.
+        - path:
+            type: PathPrefix
+            value: /jupyter

Review Comment:
   `jupyterPool.basePath` drives the pod's base_url, the recorded internal URL 
and the public template — but this match is a literal, and so is the uid regex 
in AccessControlResource, so any basePath other than /jupyter 404s at the 
gateway while the pods faithfully serve the new prefix. A knob half the system 
obeys is worse than no knob.
   
   Not blocking: either template this from the value (and derive the regex 
prefix from `kubernetes.jupyter-base-url`, which access-control-service already 
reads), or drop the knob and hardcode the path with a comment saying so.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to