backend: Fix port-forward cache key collisions#5626
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: harrshita123 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 |
illume
left a comment
There was a problem hiding this comment.
Thanks for working on this.
would you mind rebasing this branch on top of the latest main rather than merging it in?
Why this matters
Merge commits from main make the PR history harder to review. Please rebase your branch on top of the latest main instead, then update the PR with the rebased commits.
There was a problem hiding this comment.
Pull request overview
This PR updates the backend port-forward cache key format and lookup behavior to prevent collisions between similarly named clusters (e.g., foo vs foobar) when listing, retrieving, stopping, or deleting port-forward entries.
Changes:
- Introduces a delimiter-based, escaped cache key format for port-forward entries and centralizes prefix generation.
- Makes port-forward listing filter by an escaped cluster-specific prefix instead of a raw
storeKeyPrefix+clusterprefix. - Adds a regression test for the
foovsfoobarcluster-name collision case.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
backend/pkg/portforward/store.go |
Adds escaped, delimiter-based key generation and updates list/lookup to use the shared key/prefix helpers. |
backend/pkg/portforward/internal_test.go |
Updates key-generator expectations and adds a regression test for prefix-collision behavior. |
backend/pkg/portforward/handler_test.go |
Updates integration test’s cache-key construction to match the new key format. |
illume
left a comment
There was a problem hiding this comment.
Thanks for this PR.
The open review comments from Copilot still need attention — can you have a look? Once addressed, please mark them as resolved.
@illume I resolved the commets . |
illume
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
it looks like there's a merge-main commit in this PR — could you rebase onto main instead?
Why this matters
Merge commits from main make the PR history harder to review. Please rebase your branch on top of the latest main instead, then update the PR with the rebased commits.
bec9774 to
7fa68a8
Compare
6d05f52 to
7fa68a8
Compare
@illume I rebased the commits . |
illume
left a comment
There was a problem hiding this comment.
Thanks for working on this.
would you mind rebasing this branch on top of the latest main rather than merging it in?
Why this matters
Merge commits from main make the PR history harder to review. Please rebase your branch on top of the latest main instead, then update the PR with the rebased commits.
|
@harrshita123 |
d73d0ad to
2927230
Compare
2927230 to
66f9edc
Compare
@illume |
ec8828b to
aae2f31
Compare
aae2f31 to
09f2e5a
Compare
illume
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
The backend test job in CI is failing. Run cd backend && go test ./... to reproduce the errors locally.
How to run the backend tests
Run cd backend && go test ./... to see all failures. Fix the failing tests and commit the result.
illume
left a comment
There was a problem hiding this comment.
Thanks for this PR.
The open review comments from Copilot still need attention — can you have a look? Once addressed, please mark them as resolved.
09f2e5a to
cdd5013
Compare
cdd5013 to
b2c6bd9
Compare
@illume |
Summary
This PR fixes a backend port-forward cache-key collision in Headlamp. Port-forward entries now use separator-safe cache keys, so clusters with similar
names no longer overlap when entries are listed, looked up, or deleted.
Related Issue
Fixes #5625
Changes
foovsfoobarcluster-name collision caseSteps to Test
fooandfoobar.Screenshots
Not applicable for this backend-only change.
Notes for the Reviewer
This is a focused backend fix. The change stays within the existing port-forward cache logic and adds a regression test for the collision scenario
described in the issue.