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.

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

frontend: KubeObject: Set missing attributes in attrs #2654

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

skoeva
Copy link
Contributor

@skoeva skoeva commented Dec 10, 2024

Previously, the apiVersion was not being provided in the jsonData object in the resource table, and apiVersion is necessary for authenticating and displaying valid row actions. This change ensures that resource, group, and version are always provided to ensure that valid row actions for the given KubeObject are visible.

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
@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
Previously, the apiVersion was not being provided in the jsonData object
in the resource table, and apiVersion is necessary for authenticating
and displaying valid row actions. This change ensures that resource,
group, and version are always provided to ensure that valid row actions
for the given KubeObject are visible.

Fixes: #2633

Signed-off-by: Evangelos Skopelitis <[email protected]>
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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Issues related to the frontend size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
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