Skip to content

Frontend: PodDetails: Fix wrong verbs#3506

Merged
illume merged 1 commit intokubernetes-sigs:mainfrom
cavus700:main
Jul 3, 2025
Merged

Frontend: PodDetails: Fix wrong verbs#3506
illume merged 1 commit intokubernetes-sigs:mainfrom
cavus700:main

Conversation

@cavus700
Copy link
Contributor

This PR fixes the verbs, used to determine if the "exec" and "attach" button is visible in the PodDetails.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 23, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: cavus700 / name: Cavus700 (acac409)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jun 23, 2025
@k8s-ci-robot
Copy link
Contributor

Welcome @cavus700!

It looks like this is your first PR to kubernetes-sigs/headlamp 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/headlamp has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 23, 2025
Copy link
Contributor

@skoeva skoeva left a comment

Choose a reason for hiding this comment

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

Thanks for looking into this issue!

To get any changes reviewed/merged, be sure to sign the CLA linked in the GitHub bot's message. Also, be sure to rebase your feature branch against main to keep things up to date

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 24, 2025
@cavus700
Copy link
Contributor Author

Hello @skoeva,
I have signed the CLA and rebased my branch :)

This comment was marked as outdated.

Copy link
Contributor

@skoeva skoeva left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for contributing!

@cavus700
Copy link
Contributor Author

I think the logic was correct before this PR and the "get" verb should remain in the check. Otherwise if the verb is not part of the role the terminal connection will fail.
The full explanation is in my other issue: #3516

@farodin91
Copy link
Contributor

Check both?

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 27, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the visibility logic for the “Exec” and “Attach” buttons in PodDetails by adding the correct “create” permission checks on their respective subresources.

  • Wrapped the Exec action button in an additional AuthVisible requiring create on exec
  • Wrapped the Attach action button in an additional AuthVisible requiring create on attach
  • Closed the newly added wrappers to ensure both checks apply
Comments suppressed due to low confidence (4)

frontend/src/components/pod/Details.tsx:523

  • [nitpick] Consider adding tests to verify that the exec button visibility toggles correctly based on the create permission for the exec subresource.
                <AuthVisible item={item} authVerb="create" subresource="exec">

frontend/src/components/pod/Details.tsx:547

  • [nitpick] Add tests to ensure the attach button is hidden or shown based on the create permission for the attach subresource.
                <AuthVisible item={item} authVerb="create" subresource="attach">

frontend/src/components/pod/Details.tsx:522

  • The outer AuthVisible is checking get on the exec subresource. It should check get on the pod resource itself (no subresource), and let the inner wrapper handle create on exec.
              <AuthVisible item={item} authVerb="get" subresource="exec">

frontend/src/components/pod/Details.tsx:546

  • Similar to exec, this get check is scoped to the attach subresource. Move the subresource attribute to the inner wrapper and let this outer check validate get on the pod.
              <AuthVisible item={item} authVerb="get" subresource="attach">

@illume
Copy link
Contributor

illume commented Jul 3, 2025

Thanks. Yeah, it does need get and create.

kubectl proxy &
[1] 544340
$ Starting to serve on 127.0.0.1:8001

curl http://localhost:8001/api/v1 | jq '.resources[] | select(.name=="pods/exec")'
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 10742    0 10742    0     0   673k      0 --:--:-- --:--:-- --:--:--  699k
{
  "name": "pods/exec",
  "singularName": "",
  "namespaced": true,
  "kind": "PodExecOptions",
  "verbs": [
    "create",
    "get"
  ]
}
$
curl http://localhost:8001/api/v1 | jq '.resources[] | select(.name=="pods/attach")'
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 10742    0 10742    0     0  1839k      0 --:--:-- --:--:-- --:--:-- 2098k
{
  "name": "pods/attach",
  "singularName": "",
  "namespaced": true,
  "kind": "PodAttachOptions",
  "verbs": [
    "create",
    "get"
  ]
}

Copy link
Contributor

@illume illume left a comment

Choose a reason for hiding this comment

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

🎉

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cavus700, illume, skoeva

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

The pull request process is described 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 3, 2025
@illume illume merged commit 66654dc into kubernetes-sigs:main Jul 3, 2025
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

Development

Successfully merging this pull request may close these issues.

5 participants