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

[Bug]: RoleBinding deletion API call fails when a user with Edit permission deletes the model server #1320

Closed
1 task done
lucferbux opened this issue May 31, 2023 · 10 comments · Fixed by #1752
Closed
1 task done
Assignees
Labels
feature/model-serving Model Serving Feature field-priority Flag to track improvements that are for stability -- effort to put in front of new functionality kind/bug Something isn't working priority/normal An issue with the product; fix when possible rhods-1.33

Comments

@lucferbux
Copy link
Contributor

lucferbux commented May 31, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

In the scope of Project Sharing feature, a user B who got "Edit" permissions to project A owned by user A can delete the model server but it leaves a leftover in the cluster that is the rolebinding -view.
This happens because user B does not have enough permissions to delete the rolebinding. The screenshot reports the set of API calls triggered by the deletion of the model server by user B

[UPDATE]

As we comment in the discussion bellow, we are going to check permission with useAccessReview and disable the deletion if the user doesn't have enough permissions.

Expected Behavior

Either one of those:

  • rolebinding gets deleted
  • user B does not have the power of deleting a model server

Steps To Reproduce

  1. login RHODS Dashboard with basic user A
  2. create a DS Project project A
  3. share access to the project with basic user B, granting Edit permissions
  4. create a model server in project A with user A
  5. (optional) deploy a model (which user does not matter)
  6. delete the model server with user B

Workaround (if any)

No response

What browsers are you seeing the problem on?

No response

Open Data Hub Version

No response

Anything else

No response

@lucferbux lucferbux added kind/bug Something isn't working untriaged Indicates the newly create issue has not been triaged yet labels May 31, 2023
@lucferbux
Copy link
Contributor Author

@vconzola We have another issue with this, users with edit cannot delete model servings either, since they cannot delete the rolebinding resource, what should we do UX wise in this scenario?

@shalberd
Copy link
Contributor

question is: should user B even have the power to delete a model server in a shared project / namespace? In terms of clusterrole edit at namespace-level:

The edit role permits modifying and creating common application resources but gives no access to permissions, limit ranges, or quotas that also correspond to the typical developer user.

i.e. permissions = roles, rolebindings, all that stuff

You can think of creating custom clusterroles maybe, e.g. edit-odh or something like that :-)

https://www.redhat.com/sysadmin/rbac-openshift-role

@vconzola
Copy link

@lucferbux I think maybe we should disable the model server Edit and Delete buttons (and possibly Create too) for users with only Edit permissions. That might solve all of the issues we're seeing in this area. So Edit users would be able to deploy models using model servers in namespaces for which they have Edit permissions, but only Admin users would be able to Create, Edit, Delete. Thoughts?

@andrewballantyne
Copy link
Member

@lucferbux I think maybe we should disable the model server Edit and Delete buttons (and possibly Create too) for users with only Edit permissions. That might solve all of the issues we're seeing in this area. So Edit users would be able to deploy models using model servers in namespaces for which they have Edit permissions, but only Admin users would be able to Create, Edit, Delete. Thoughts?

There may also just been a holistic look at what "Edit" means for us -- If I invite you to my project, should you really be able to run around deleting my stuff? I dunno... As Sven alluded to -- maybe we make our own Edit rule variants...

I don't know if permissions were really chewed to the bone on the use-cases, it was just a means to grant Dashboard the ability to share projects -- and admin everything seemed kinda goofy. But there is a lot of small nuance here that we are missing.

cc @jedemo @kywalker-rh @vconzola might want to spin that off for another conversation / issue.

For this issue, my opinion is to not allow create or delete.. but leave edit the way we are going now. This way they can edit the replica count, in case they want to "spin down" the server. My two cents...

@jedemo
Copy link

jedemo commented Jun 2, 2023

We discussed changing the terminology for 'edit' permissions because we already have some cases where editing is not allowed. I think maybe we change to 'Contributor' and clearly document what that enables. I'm ok restricting 'delete' to admins.
@kywalker-rh

@lucferbux
Copy link
Contributor Author

Thanks @jedemo that should cover this issue and future functionality that relies on roles or rolebindings. I'm gonna open another task to cover that refactor.

@lucferbux
Copy link
Contributor Author

I've created #1335 just to track those changes.

@andrewballantyne
Copy link
Member

Let us be clear here -- this is not a UI change... this is a permission rework. Edit is not Edit anymore. We need to build a role and maintain it. The Dashboard shouldn't look at K8s as a suggestion -- we don't want our UI being awkward to what you can do in OpenShift Console or in the CLI... let us not just reimplement the Jupyter workaround nonsense that got us into a confusion about what we can and cannot do.

If we want to divorce Edit -- let us do it fully. Start with View and Add additional items (to view, edit, etc etc) as we see fit until it reaches what we want. And this is not a light processes.

Locking down the delete button because they cannot delete the full set of resources behind the scenes is programatically possible AND it follows K8s. Permission changes need to be a bigger discussion then a single one off on threads of issues.

I'm adding this to the UX Meetings on Thursdays to make sure we talk about this in the right context.

@kywalker-rh
Copy link

Commenting here to say I am on board with changing the "Edit" type in the permissions section to "Contributor". For the rest it sounds like there will be more conversation.

@Gkrumbach07 Gkrumbach07 added needs-info Further information is requested from the reporter or from another source priority/normal An issue with the product; fix when possible feature/model-serving Model Serving Feature and removed untriaged Indicates the newly create issue has not been triaged yet labels Jun 7, 2023
@andrewballantyne
Copy link
Member

Got to discuss this at the UX meeting last week. We are not going to do anything special here -- we are going to lean into access checks and disable deletion when it cannot complete.

@lucferbux lucferbux added field-priority Flag to track improvements that are for stability -- effort to put in front of new functionality and removed needs-info Further information is requested from the reporter or from another source labels Jul 26, 2023
@DaoDaoNoCode DaoDaoNoCode self-assigned this Sep 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/model-serving Model Serving Feature field-priority Flag to track improvements that are for stability -- effort to put in front of new functionality kind/bug Something isn't working priority/normal An issue with the product; fix when possible rhods-1.33
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

8 participants