Conversation
PostStartInjector.watchAndInject() was passing the user's OAuth token kubeconfig to k8s.Watch. If the user's RBAC role does not include the watch verb on devworkspaces, the watch fails silently and kubeconfig injection never happens, leaving oc to fall back to the pod's SA token. Use the dashboard SA kubeconfig for the Watch instead. The SA has unconditional watch permissions in user namespaces. The kubeConfigApi (carrying the user's token for exec and kubeconfig content) is unchanged. Fixes: CRW-11193 Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
If the Kubernetes Watch fails to start, fall back to polling the DevWorkspace via GET every 10 s for up to 300 s. Inject on Running; stop without injection on Failed, Failing, Stopped, Stopping, or Terminating. While polling is active the workspace key remains in activeWatches to prevent duplicate start attempts. Fixes: CRW-11193 Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
Replace raw phase strings with DevWorkspaceStatus enum values from a new devworkspaceClient/devWorkspaceStatus.ts module (mirrors the frontend enum; backend cannot import across package boundaries). Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
Replace the standalone k8s.Watch in PostStartInjector with devworkspaceApi.watchInNamespace() — the same code path used by the WebSocket SUBSCRIBE DEV_WORKSPACE channel. Benefits: - No separate watch infrastructure; reuses existing DevWorkspace service - User OAuth token for both watch and polling (passed via devworkspaceApi) - stopWatching() called after injection (clean unsubscribe) - Shared injectCredentials() helper removes duplication between watch-path and polling-fallback Fixes: CRW-11193 Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
Remove the backend-local copy (devWorkspaceStatus.ts) and the frontend inline definition. Both now import from @eclipse-che/common so the enum has a single canonical source. Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
- Remove numeric separators from timeout constants - Replace POLL_STOP_PHASES Set with isPollStopPhase() function Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: olexii4 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi! I'm che-ai-assistant — I help with your pull requests. Available commands:
|
- Move DevWorkspaceStatus import to top of types.ts (import/order) - Fix numeric separators in PostStartInjector.spec.ts - Add comment on devworkspaceApi instance isolation in devworkspaces.ts - Add watchInNamespace/stopWatching stubs to getDevWorkspaceClient mock Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
|
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1606 (linux/amd64, linux/arm64, linux/s390x) kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1606", name: che-dashboard}]}}]" |
|
/che-ai-assistant ok-pr-review Review is complete. Please check the review comments below. |
tolusha
left a comment
There was a problem hiding this comment.
Comprehensive Review Complete
This PR successfully fixes the regression where oc whoami returns the workspace pod service account identity instead of the logged-in user's identity. The architectural decision to reuse watchInNamespace() is sound and the token propagation chain is verified end-to-end.
Summary
- ✅ Standard Review: Ready to merge - no blocking issues
- ✅ Deep Review: Design is sound - good abstraction quality and test coverage
- ✅ Impact Review: No system-level concerns
Key Findings
Watch Scope Optimization (PostStartInjector.ts:273): The watch monitors all DevWorkspaces in the namespace rather than using a field selector for the specific workspace. The listener filters by name, so correctness is preserved, but this increases API server load in namespaces with many workspaces.
Test Coverage Gaps: Missing tests for watchInNamespace promise rejection path and the case where Running arrives without a devworkspaceId but the watch should stay active.
Naming Clarity: isPollStopPhase returns true for RUNNING (the success case), which creates a double-negative pattern at the call site. Consider renaming to isTerminalOrReadyPhase.
Positive Highlights
- Excellent PR description with clear root cause analysis
- Polling fallback adds meaningful resilience
- DevWorkspaceStatus enum consolidation improves code organization
- Instance isolation comment at devworkspaces.ts:71-74 is exemplary documentation
- Comprehensive test rewrite covers all critical paths
Verdict: ✅ Approve - This is a well-executed regression fix with thorough testing and sound architecture.
isPollStopPhase included RUNNING alongside terminal phases and required callers to exclude it explicitly with `&& phase !== RUNNING`, creating a double-negative. Rename to isTerminalPhase() (RUNNING removed from the set) so each call site reads directly: watch: if (phase && isTerminalPhase(phase)) → abort polling: if (!phase || (phase !== RUNNING && !isTerminalPhase(phase))) → keep polling Addresses review feedback on PR #1606. Assisted-by: Claude Sonnet 4.6 Signed-off-by: Oleksii Orel <oorel@redhat.com>
…unction coverage (CRW-11193)
The export { DevWorkspaceStatus } from './dto/api' in src/index.ts compiles
to an Object.defineProperty getter that Istanbul counts as a function. The
existing test never accessed DevWorkspaceStatus via the package index, leaving
that getter uncovered and dropping function coverage to 96% (24/25), which
failed the 100% threshold enforced by jest config.
Assisted-by: Claude Sonnet 4.6
Signed-off-by: Oleksii Orel <oorel@redhat.com>
|
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1606 (linux/amd64, linux/arm64, linux/s390x) kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1606", name: che-dashboard}]}}]" |
1 similar comment
|
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1606 (linux/amd64, linux/arm64, linux/s390x) kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1606", name: che-dashboard}]}}]" |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1606 +/- ##
==========================================
- Coverage 92.38% 92.36% -0.02%
==========================================
Files 587 587
Lines 60363 60463 +100
Branches 4670 4681 +11
==========================================
+ Hits 55767 55848 +81
- Misses 4537 4556 +19
Partials 59 59 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
What does this PR do?
This PR fixes
oc whoamireturning the workspace pod service account identity instead of the logged-inuser's identity in workspace terminals after Dev Spaces 3.28.1.
Root Cause
PR #1475 moved kubeconfig injection from
the frontend WebSocket handler to a backend
PostStartInjectorthat sets up a Kubernetes Watchwhen a workspace is started (PATCH or POST with
spec.started=true).Before #1475, the backend's own service account performed the watch (via the existing WebSocket
infrastructure), and the user's token was only needed at injection time — sourced from the
frontend's POST request to
/namespace/:ns/devworkspaceId/:id/kubeconfig. This difference inwatch identity is what introduced the regression.
Fix
Two changes in sequence:
1. Reuse
watchInNamespace()— same code path as WebSocketSUBSCRIBE DEV_WORKSPACE.The standalone
k8s.Watchis replaced withdevworkspaceApi.watchInNamespace(), the exact samemethod the WebSocket channel uses in
DevWorkspaceApiService. This eliminates separate watchinfrastructure and lets the listener call
stopWatching()(clean unsubscribe) after injection.devworkspaceApiis passed from the route handler and carries the user's OAuth token, so bothwatch and polling use the user's identity throughout.
A shared
injectCredentials()private helper removes duplication between the watch path and thepolling fallback.
2. Polling fallback when the watch receives an ERROR event.
If the watch reports an error (RBAC gap, network hiccup), a polling fallback kicks in: GET the
devworkspace via
devworkspaceApi.getByName()every 10 s for up to 300 s. Injection happens onRunning; polling stops without injection onFailed,Failing,Stopped,Stopping, orTerminating. The workspace key stays registered inactiveWatcheswhile polling is active soduplicate start calls are still blocked.
Screenshot/screencast of this PR
N/A — backend-only behavioral fix, no UI changes.
What issues does this PR fix or reference?
fixes https://redhat.atlassian.net/browse/CRW-11193
Is it tested? How?
oc whoami— must return the OpenShift user name, not a service account.oc get nodes— must succeed (or fail with a clear permission error, not a serviceaccount identity mismatch).
Release Notes
Fixed workspace terminal CLI (
oc/kubectl) using the pod service account identity instead ofthe logged-in user's credentials after a workspace starts.
Docs PR