Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

frontend: KubeObject: Refactor getAuthorization logic #2654

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

frontend: KubeObject: Refactor getAuthorization logic #2654

wants to merge 2 commits into from

Conversation

skoeva
Copy link
Contributor

@skoeva skoeva commented Dec 10, 2024 •
edited
Loading

This change updates the logic of the getAuthorization function in KubeObject, which previously intended to test auth by separating the group from its respective version when one of these was missing. Now, these two fields are linked and grabbed together from apiInfo when one is missing. A change for assigning the apiName (rather than the less descriptive pluralName) when the resource name is missing was also included.

Fixes: #2633

Testing

image

@skoeva skoeva added the frontend Issues related to the frontend label Dec 10, 2024
@skoeva skoeva self-assigned this Dec 10, 2024
@skoeva skoeva linked an issue Dec 10, 2024 that may be closed by this pull request
Headlamp does not show restart/edit/scale buttons on resources with sufficient permissions #2633
Open
@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Dec 10, 2024
@skoeva skoeva requested a review from a team December 10, 2024 16:45
@skoeva skoeva changed the title frontend: Fix table row action bugs frontend: KubeObject: Set apiVersion in constructor Dec 10, 2024
@skoeva skoeva force-pushed the row-action-bugs branch 3 times, most recently from 2150ef0 to f86d8b4 Compare December 11, 2024 15:15
@skoeva skoeva changed the title frontend: KubeObject: Set apiVersion in constructor frontend: KubeObject: Set missing attributes in attrs Dec 11, 2024
joaquimrocha
joaquimrocha reviewed Dec 11, 2024
View reviewed changes
Copy link
Collaborator

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

@skoeva Do we always need to set the group + version + resource when checking for permissions? I think the original logic was "check if we are checking permissions with e.g. the resource, if so, use this API version's one instead", in these changes it does: "even if we were not testing permissions using the resource, will just set it".

Are we sure this is the right approach? We ought to test variants of these calls.

@skoeva
Copy link
Contributor Author

skoeva commented Dec 11, 2024

@skoeva Do we always need to set the group + version + resource when checking for permissions? I think the original logic was "check if we are checking permissions with e.g. the resource, if so, use this API version's one instead", in these changes it does: "even if we were not testing permissions using the resource, will just set it".

Are we sure this is the right approach? We ought to test variants of these calls.

@joaquimrocha I think it would be fitting to have a complete resource to evaluate permissions for, no? The issue before was that the object was incomplete and therefore we were unable to check for its permissions. In what case would we prefer an incomplete object?

@joaquimrocha
Copy link
Collaborator

@joaquimrocha I think it would be fitting to have a complete resource to evaluate permissions for, no? The issue before was that the object was incomplete and therefore we were unable to check for its permissions. In what case would we prefer an incomplete object?

I just saying the nature of how we were checking things was more of if we use a resource name -> now try the resource name from this other version, and right now we're saying if a resource name is not set -> try the resource name from this version. So I believe this is incorrect to what is the point of that logic. It may be that the logic is wrong in some cases, but if we are missing fields, we should verify why that is instead of adding them from a new version, or by not replacing them with the current testing version, which is what these changes also do.

@skoeva
Copy link
Contributor Author

skoeva commented Dec 12, 2024

I just saying the nature of how we were checking things was more of if we use a resource name -> now try the resource name from this other version, and right now we're saying if a resource name is not set -> try the resource name from this version. So I believe this is incorrect to what is the point of that logic. It may be that the logic is wrong in some cases, but if we are missing fields, we should verify why that is instead of adding them from a new version, or by not replacing them with the current testing version, which is what these changes also do.

@joaquimrocha The code comments suggest that the changes made in this PR are more in line with the intended logic here:

image

Not sure if the original logic makes much sense in this case

joaquimrocha
joaquimrocha reviewed Dec 13, 2024
View reviewed changes
Copy link
Collaborator

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

I think you may have a point, now that look at this with fresher eyes. I think if that's the case, then the previous logic is indeed wrong, but seems like we're possibly mixing things (mixing the group from an API version with version from another), so maybe what we need to do is to use the full apiName + apiVersion here instead.
In either case, I'd kindly ask you to test this function and understand what happens when e.g. you add newer+fictitious apiVersion to a resource, and test this function (and the instances' getAuthorization), to simulate falling back to a working version.

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Dec 16, 2024
@skoeva skoeva changed the title frontend: KubeObject: Set missing attributes in attrs frontend: KubeObject: Refactor getAuthorization logic Dec 16, 2024
joaquimrocha
joaquimrocha requested changes Dec 20, 2024
View reviewed changes
Copy link
Collaborator

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

LGTM. Only a comment that should be deleted as it no longer makes sense.

frontend/src/lib/k8s/KubeObject.ts Show resolved Hide resolved
@skoeva skoeva force-pushed the row-action-bugs branch 3 times, most recently from f785705 to 8a31aeb Compare December 20, 2024 19:38
@skoeva
frontend: KubeObject: Use apiName for resource name
25834ad 
This change assigns the apiName (rather than the less descriptive
pluralName) when the resource name is missing.

Signed-off-by: Evangelos Skopelitis <[email protected]>
@skoeva
frontend: KubeObject: Refactor getAuthorization logic
cf5b206 
This change updates the logic of the getAuthorization function in
KubeObject, which previously intended to test auth by separating the
group from its respective version when one of these was missing. Now,
these two fields are linked and grabbed together from apiInfo when one
is missing.

Fixes: #2633

Signed-off-by: Evangelos Skopelitis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@farodin91 farodin91 farodin91 left review comments

@joaquimrocha joaquimrocha joaquimrocha requested changes

@sniok sniok sniok approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

@skoeva skoeva

Labels
frontend Issues related to the frontend size:S This PR changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Headlamp does not show restart/edit/scale buttons on resources with sufficient permissions
4 participants
@skoeva @joaquimrocha @farodin91 @sniok