Conversation
…e#6575) Signed-off-by: Yash Raj <kyashraj991@gmail.com> A
Summary of ChangesHello @ljluestc, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the authorization model by introducing single-cluster authorization, allowing users to be granted specific roles on individual member clusters without needing platform-wide administrative privileges. This change refines the 'checkClusterPermission' logic to be more flexible, enabling scenarios where users require limited access to particular clusters. Alongside this core authorization update, new API endpoints have been added to facilitate the management of cluster members, providing a comprehensive solution for granular access control. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces new API endpoints and backend logic for managing cluster members, enabling the listing, creation, removal, and updating of user roles within a specified cluster by manipulating ClusterRoleBinding resources. It also includes a minor fix for a JSON tag typo. Review comments highlighted that the RemoveClusterMember function has potential issues with loop variable addressing and error handling, as it currently returns on the first error, which could lead to an inconsistent state. Furthermore, the UpdateClusterMember implementation, which relies on a non-atomic remove-then-create operation, could result in a temporary loss of user access if the creation step fails.
| for _, crb := range crbList.Items { | ||
| if err := rtClient.Delete(context.Background(), &crb); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
This loop for deleting ClusterRoleBindings has a couple of issues:
- Loop variable address: You are taking the address of the loop variable
crb(&crb). In Go, this variable is reused in each iteration. WhileDeleteis synchronous and this might work, it's a common pitfall that can lead to bugs and is best avoided. Iterating by index is safer. - Error handling: If deleting one of the bindings fails, the function returns immediately. This can leave the user's permissions in an inconsistent state. It's more robust to attempt to delete all bindings and then report any accumulated errors.
Here is a suggestion to address both points.
| for _, crb := range crbList.Items { | |
| if err := rtClient.Delete(context.Background(), &crb); err != nil { | |
| return err | |
| } | |
| } | |
| return nil | |
| var allErrors []string | |
| for i := range crbList.Items { | |
| if err := rtClient.Delete(context.Background(), &crbList.Items[i]); err != nil { | |
| allErrors = append(allErrors, err.Error()) | |
| } | |
| } | |
| if len(allErrors) > 0 { | |
| return fmt.Errorf("failed to remove all cluster member roles: %s", strings.Join(allErrors, "; ")) | |
| } | |
| return nil |
| // Simple implementation: remove all existing bindings for user and add new one | ||
| if err := t.RemoveClusterMember(cluster, username); err != nil { | ||
| return err | ||
| } | ||
| return t.CreateClusterMember(cluster, username, role) |
There was a problem hiding this comment.
The comment acknowledges this is a "Simple implementation", but it's worth noting that this approach is not atomic. If RemoveClusterMember succeeds but CreateClusterMember fails, the user will be left without any roles in the cluster. This could lead to a temporary or permanent loss of access until manually fixed.
A more robust approach would be to ensure the operation is as atomic as possible. For example, you could list the existing bindings, create the new binding, and only then delete the old ones. If creating the new binding fails, the user still has their old role.
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR implements single-cluster authorization, allowing granular access control for users on specific member clusters.
Previously, the
checkClusterPermissionlogic contained a hardcoded check that effectively restricted cluster access to users with thecluster-adminrole (or platform admins). This prevented scenarios where a user (e.g., a VM owner) needed read-only or limited management access (cluster-viewer) to a specific connected cluster without having elevated platform privileges.Changes:
pkg/models/tenant/tenant.go: ThecheckClusterPermissionfunction now grants access if anyClusterRoleBindingexists for the user in the target cluster.Which issue(s) this PR fixes:
Fixes #6271
Special notes for reviewers:
The logic change is in
pkg/models/tenant/tenant.go. I replaced the temporary FIXME/TODO block that checked specifically foriamv1beta1.ClusterAdminwith a check that validates if the list ofClusterRoleBindingsfor the user is non-empty.Does this PR introduced a user-facing change?
Additional documentation, usage docs, etc.: