Skip to content

fix(backup): read registry auth secret from workspace namespace#1597

Merged
dkwon17 merged 6 commits into
mainfrom
fix/CRW-11079
Jun 10, 2026
Merged

fix(backup): read registry auth secret from workspace namespace#1597
dkwon17 merged 6 commits into
mainfrom
fix/CRW-11079

Conversation

@akurinnoy

@akurinnoy akurinnoy commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes the Backups tab showing no entries for deleted workspaces even when backup images still exist in the registry.

getRegistryCredentials() in registryApi.ts was reading the auth secret from the operator namespace using the SA kubeconfig. The Dashboard SA has no RBAC to read secrets there — every call returned 403 Forbidden, causing the registry listing to fall back to unauthenticated access and return [] for private registries.

DWO always copies the configured auth secret into the workspace namespace as devworkspace-backup-registry-auth. This PR switches getRegistryCredentials() to read it from there using the user kubeconfig.

Note: this is one of two fixes needed to fully resolve CRW-11079. The other is in devworkspace-operator#1631 — without it, DWO sets an ownerRef on the workspace-namespace secret causing it to be GC'd on workspace deletion.

Screenshot/screencast of this PR

What issues does this PR fix or reference?

Fixes https://issues.redhat.com/browse/CRW-11079

Related: devfile/devworkspace-operator#1631

Is it tested? How?

  1. Deploy Eclipse Che with the dashboard image from this PR and DWO that doesn't set ownerRef on the workspace namespace auth secret (e.g. DWO quay.io/devfile/devworkspace-controller:sha-ec6de18).
  2. Configure DevWorkspaceOperatorConfig with backup enabled and a private registry (e.g. quay.io).
  3. Create a workspace, stop it, wait for the backup job to complete, then delete the workspace.
  4. Open the Dashboard and navigate to the Backups tab, see: the deleted workspace entry is visible with a (Deleted) label.
  5. Without this fix: repeat with the stock dashboard image, see: the Backups tab is empty.

Release Notes

Fixed: backup entries for deleted workspaces are now visible in the Backups tab when using a private OCI registry.

Docs PR

N/A

The Dashboard backend's getRegistryCredentials() was reading the auth
secret from the operator namespace using the SA kubeconfig, which results
in HTTP 403 Forbidden since the Dashboard SA has no RBAC to read secrets
in that namespace.

DWO always copies the configured auth secret into every workspace namespace
as 'devworkspace-backup-registry-auth'. Switch to reading that secret from
the workspace namespace using the user kubeconfig, which the Dashboard SA
is permitted to access.

This fixes the case where deleted workspaces do not appear in the Backups
tab: with the old code the registry listing was always unauthenticated,
causing it to silently return an empty result for private registries.

Resolves: CRW-11079

Assisted-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
@akurinnoy akurinnoy requested review from ibuziuk and olexii4 as code owners June 4, 2026 17:00
@che-bot

che-bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Click here to review and test in web IDE: Contribute

@tolusha

tolusha commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Hi! I'm che-ai-assistant — I help with your pull requests.

Available commands:

  • /che-ai-assistant generate-che-doc — Generate a documentation PR based on this PR's changes
  • /che-ai-assistant ok-pr-review — Run a comprehensive PR review (summary, code review, deep review, impact analysis)
  • /che-ai-assistant help — Show this help message

@akurinnoy

akurinnoy commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

/che-ai-assistant ok-pr-review

Review is complete. Please check the review comments below.

… lookup

Remove the SA kubeconfig mock for getRegistryCredentials — the method
now reads the fixed-name secret from the workspace namespace via the
user kubeconfig instead of reading the operator-configured secret name
from the operator namespace via the SA kubeconfig.

Assisted-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>

@tolusha tolusha left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Comprehensive review completed across multiple dimensions:

  • Standard Review: ✅ Approve
  • Deep Review: ✅ Design is Sound
  • Impact Review: ✅ No system-level concerns

✅ Excellent work overall

This is a well-executed fix with several strong points:

  • Clean scope - Only touches the two files that need to change, no unnecessary refactoring
  • Strong documentation - PR description clearly explains root cause, fix approach, DWO dependency, and manual test steps
  • Good comments - Inline comments explain the "why" (e.g., why reading from workspace namespace instead of operator namespace)
  • Security improvement - Shift from SA to user kubeconfig follows least-privilege principles
  • Test quality - All 8 test cases properly updated with improved assertion rigor (now verifies both secret name and namespace)

Suggestions for improvement

I've posted a few inline suggestions below - all are optional improvements, not blocking issues:

  1. Add debug logging when secret read fails for better operational troubleshooting
  2. Document the DWO contract with a source reference
  3. Consider removing unused authSecret from getBackupRegistryPath() return type
  4. Add a regression test to verify SA API is never used

Noted for coordination

The PR correctly documents the dependency on DWO PR #1631 (removing ownerReferences so the secret survives workspace deletion). Ensure both changes ship together for the full fix.


Review generated by Claude Code with ok-pr-review

@tolusha

tolusha commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Suggestion: Add regression test for SA API usage

Consider adding a test in registryApi.spec.ts that verifies prepareCoreV1API is called exactly once (with user kubeconfig only), ensuring no SA CoreV1 API is accidentally reintroduced. This would guard against future regressions.

@github-actions

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1597 (linux/amd64, linux/arm64)

kubectl patch command
kubectl 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-1597", name: che-dashboard}]}}]"

akurinnoy added 2 commits June 5, 2026 17:22
- Add warn log when auth secret read fails (was silently returning empty)
- Add DWO source reference to BACKUP_REGISTRY_AUTH_SECRET_NAME constant
- Remove unused authSecret from getBackupRegistryPath() return type

Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
Verify that prepareCoreV1API is called exactly once with the user
kubeconfig. If someone re-adds saCoreV1Api, this test fails.

Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
@codecov

codecov Bot commented Jun 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.45%. Comparing base (65f699e) to head (66b7e6e).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1597      +/-   ##
==========================================
- Coverage   92.45%   92.45%   -0.01%     
==========================================
  Files         587      585       -2     
  Lines       60049    59926     -123     
  Branches     4627     4619       -8     
==========================================
- Hits        55518    55404     -114     
+ Misses       4472     4463       -9     
  Partials       59       59              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@openshift-ci openshift-ci Bot added lgtm and removed lgtm labels Jun 9, 2026
@akurinnoy

akurinnoy commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Suggestion: Add regression test for SA API usage

Consider adding a test in registryApi.spec.ts that verifies prepareCoreV1API is called exactly once (with user kubeconfig only), ensuring no SA CoreV1 API is accidentally reintroduced. This would guard against future regressions.

Done — added in 3721e89.

akurinnoy added 2 commits June 9, 2026 16:58
Fix import sort order and prettier line-wrapping for logger.warn call.

Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
… fix

Fix import sort order: move logger import after prepareCoreV1API.

Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1597 (linux/amd64, linux/arm64)

kubectl patch command
kubectl 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-1597", name: che-dashboard}]}}]"

@dkwon17

dkwon17 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

I was able to successfully test this PR:
image

image

@openshift-ci

openshift-ci Bot commented Jun 9, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: akurinnoy, dkwon17, svor

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rohanKanojia

Copy link
Copy Markdown

I tested the PR following the steps mentioned in descrition and can confirm it works as expected. I was able to see deleted backups in backup tab
Screenshot_20260610_154354

@dkwon17 dkwon17 merged commit 52b10f6 into main Jun 10, 2026
20 checks passed
@dkwon17 dkwon17 deleted the fix/CRW-11079 branch June 10, 2026 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants