frontend: useKubeObject: probe endpoints with GET-by-name instead of List#4696
frontend: useKubeObject: probe endpoints with GET-by-name instead of List#4696Kaniikura wants to merge 2 commits intokubernetes-sigs:mainfrom
Conversation
|
Welcome @Kaniikura! |
There was a problem hiding this comment.
Pull request overview
Updates the frontend Kubernetes API v2 endpoint resolution logic so useKubeObject can load single-resource detail views for users who have get RBAC permission but not list, by probing candidate endpoints with GET-by-name instead of collection LIST.
Changes:
- Switch
useKubeObjectto calluseEndpointswith{ mode: 'getProbe', name }(and namespace) to avoid RBAClistauthorization during endpoint discovery. - Add GET-by-name probing (
getFirstWorkingGetEndpoint) alongside the existing list-based probing (getFirstWorkingListEndpoint) and unify probe error selection viathrowMostRelevantError. - Add a new
hooks.test.tsxsuite covering both probing modes and error prioritization behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| frontend/src/lib/k8s/api/v2/hooks.ts | Implements get-by-name endpoint probing, updates useKubeObject to use it, and adjusts probing/error selection logic. |
| frontend/src/lib/k8s/api/v2/hooks.test.tsx | Adds unit tests validating getProbe/listProbe behavior, error precedence, and configuration edge cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sniok
left a comment
There was a problem hiding this comment.
hi, thanks for opening a PR, I've left a couple of comments
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
illume
left a comment
There was a problem hiding this comment.
Nice work, thanks.
Can you please update the commit messages after the first one to match the git commit guidelines?
Also, please remove the revert commit by rebasing. We aim to have atomic commits, and within a single PR not have fixes for previous commits in the same PR. Instead rebase the fixes into existing commits.
b916517 to
ebf9954
Compare
|
Thanks for the feedback. I've cleaned up the commit history and updated the messages as requested. I also added the test case for cluster-scoped resources. Force-pushed the updates. Let me know if anything else is needed! |
|
A little nit, we use Capitals at the start of the git subject after the context...
|
illume
left a comment
There was a problem hiding this comment.
Thanks! 🎉
(I'll leave it open a while longer to give someone else a chance to have a look. Please ask in the headlamp channel on the Kubernetes slack for a review?)
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: illume, Kaniikura The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ebf9954 to
4acec81
Compare
|
Updated commit subjects to follow the capitalization guideline after the prefix. No code changes. |
|
Thanks for the suggestion to ask in Slack! |
sniok
left a comment
There was a problem hiding this comment.
thanks for updating pr. I left just one final comment
…List Updates the frontend Kubernetes API v2 endpoint resolution logic so useKubeObject can load single-resource detail views for users who have get RBAC permission but not list, by probing candidate endpoints with GET-by-name instead of collection LIST.
The afterEach in multiplexer.test.ts called pendingUnsubscribes.clear() without cancelling the stored timeouts. This left orphaned setTimeout callbacks that could fire after vitest tore down the jsdom environment, causing a sporadic "ReferenceError: WebSocket is not defined". Call forEach(clearTimeout) before clear() so all pending debounce timers are cancelled before the test environment is destroyed.
4acec81 to
a469b7a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
frontend/src/lib/k8s/api/v2/hooks.ts:251
- The PR description mentions adding separate functions
getFirstWorkingGetEndpointand renaming the existing logic togetFirstWorkingListEndpoint, but the actual implementation uses a singlegetWorkingEndpointfunction with conditional logic based on whethernameis provided. While the implementation is cleaner and works correctly, the description should be updated to match the actual code changes.
const getWorkingEndpoint = async (
endpoints: KubeObjectEndpoint[],
cluster: string,
namespace?: string,
name?: string
) => {
const promises = endpoints.map(endpoint => {
const resourceUrl = KubeObjectEndpoint.toUrl(endpoint, namespace);
// If a name is provided, we probe for that specific resource.
// Otherwise we probe for the list of resources.
const url = name ? makeUrl([resourceUrl, name]) : resourceUrl;
return clusterFetch(url, {
method: 'GET',
cluster: cluster ?? getCluster() ?? '',
}).then(() => endpoint);
});
return Promise.any(promises).catch((aggregateError: AggregateError) => {
// when no endpoint is available, throw an error
throw aggregateError.errors[0];
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
This PR fixes
useKubeObjectfailing when a user hasgetpermission but lackslistpermission, by switching endpoint probing from collection-URL LIST to name-based GET for single-resource views.Related Issue
Fixes #4674
Changes
useKubeObjectto passnameandnamespacetouseEndpoints, so endpoint discovery uses a name-specific GET request (/deployments/demo) instead of a collection GET (/deployments) that Kubernetes authorizes as thelistverbgetFirstWorkingGetEndpoint— parallel GET-by-name probing strategy for single-resource viewsgetFirstWorkingListEndpointfor claritySteps to Test
get(notlist) permission on a resource (e.g., Deployments) and bind it to a user/c/my-cluster/apps/v1/Deployment/default/my-deployment)listpermissionScreenshots (if applicable)
N/A — no UI changes; the fix is in the endpoint probing logic.
Notes for the Reviewer
listProbemode (used byuseKubeObjectList) is unchanged in behavior — the newgetProbemode is only used byuseKubeObjectfor single-resource detail views