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

[DNM 2.10.3] Add schema links and resource methods for resource verb patch #450

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

crobby
Copy link
Contributor

@crobby crobby commented Jan 14, 2025

NOTE: overriding the apiserver version until rancher/apiserver#128 is merged/tagged

Issue: rancher/rancher#40712

Previously, we have not factored-in nor included the patch resource permission in our object links or schema for resource methods.

With this change, if a user has patch permission on an object, it will be reflected in both the links for an object as well as the resource methods on the schema.


Testing:
Manual testing done
Unit tests added


Helpful testing information:

To see the schema for something like apps.deployments, you can go to: /v1/schemas/apps.deployments
In the resultant json, if the user has patch permissions, you should see something like...

"resourceMethods": [ 4 items
"GET",
"DELETE",
"PUT",
"PATCH"
],

included in the yaml blob.

Likewise, if you look at the api view for an object, you will see the patch link defined when a user has patch permissions on that object.
{ "patch": "https://localhost:9443/apis/apps/v1/namespaces/default/deployments/nginx-deployment", "self": "https://localhost:9443/v1/apps.deployments/default/nginx-deployment", "update": "https://localhost:9443/apis/apps/v1/namespaces/default/deployments/nginx-deployment", "view": "https://localhost:9443/apis/apps/v1/namespaces/default/deployments/nginx-deployment" }

@crobby crobby requested a review from a team as a code owner January 14, 2025 20:14
@crobby crobby marked this pull request as draft January 14, 2025 20:15
@crobby crobby marked this pull request as ready for review January 15, 2025 18:53
@crobby crobby changed the title Add schema links and resource methods for resource verb patch [2.10.3] Add schema links and resource methods for resource verb patch Jan 15, 2025
Copy link
Contributor

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

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

This needs to be brought into alignment with main after a recent fix to steve.

@crobby
Copy link
Contributor Author

crobby commented Jan 15, 2025

This needs to be brought into alignment with main after a recent fix to steve.

It looks like apiserver sets the links for self, update, and remove, but not for patch. I think it's probably something that I'll need to add there first, and then the patch link should be there so I can make this look like the other verbs here in Steve

@crobby crobby changed the title [2.10.3] Add schema links and resource methods for resource verb patch [DNM 2.10.3] Add schema links and resource methods for resource verb patch Jan 16, 2025
@crobby crobby requested a review from ericpromislow January 16, 2025 11:29
@crobby
Copy link
Contributor Author

crobby commented Jan 16, 2025

Ok, I've gone made a PR for apiserver to include patch links.
After gluing them all together with Rancher, I now see....
Screenshot from 2025-01-16 11-09-13
as expected.

@crobby crobby requested a review from a team January 16, 2025 19:48
@@ -8,6 +8,7 @@ replace (
github.com/crewjam/saml => github.com/rancher/saml v0.2.0
github.com/knative/pkg => github.com/rancher/pkg v0.0.0-20181214184433-b04c0947ad2f
github.com/matryer/moq => github.com/rancher/moq v0.0.0-20190404221404-ee5226d43009
github.com/rancher/apiserver => github.com/crobby/apiserver v0.5.2-0.20250116110826-e76f13e6c0bd
Copy link
Contributor

Choose a reason for hiding this comment

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

I typically leave PRs in draft mode when I'm referencing local repos until they get merged in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fair enough

Copy link
Contributor

@ericpromislow ericpromislow left a comment

Choose a reason for hiding this comment

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

Everything but the references to crobby/apiserver looks good.

@crobby crobby marked this pull request as draft January 16, 2025 23:21
@crobby crobby requested a review from a team January 17, 2025 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants