-
Notifications
You must be signed in to change notification settings - Fork 709
[ENG-724] rpk: add support of roles for cloud clusters #29162
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for Redpanda Cloud clusters to the rpk security role commands by integrating the Cloud's public API. The changes enable role management operations (create, list, assign, unassign, describe, delete) to work with both self-hosted clusters (via the Admin API) and Cloud clusters (via the public API), with a check to prevent usage on serverless clusters.
Key changes:
- Adds Cloud API client initialization and separate code paths for Cloud vs. self-hosted clusters
- Implements conversion functions between Admin API and Cloud API data structures
- Updates all role commands to check cluster type and route to appropriate API
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/go/rpk/pkg/cli/security/role/BUILD |
Adds dependencies for Cloud API protobuf definitions and connect client |
src/go/rpk/pkg/cli/security/role/role.go |
Adds conversion functions between Admin API RoleMember and Cloud API RoleMembership types |
src/go/rpk/pkg/cli/security/role/create.go |
Adds Cloud API support for role creation with cluster type detection |
src/go/rpk/pkg/cli/security/role/list.go |
Adds Cloud API support for listing roles with filter parameters |
src/go/rpk/pkg/cli/security/role/assign.go |
Adds Cloud API support for assigning roles to principals |
src/go/rpk/pkg/cli/security/role/unassign.go |
Adds Cloud API support for unassigning roles from principals |
src/go/rpk/pkg/cli/security/role/describe.go |
Adds Cloud API support for describing roles including new describeAndPrintRoleCloud function |
src/go/rpk/pkg/cli/security/role/delete.go |
Adds Cloud API support for deleting roles with confirmation prompt |
| func roleMemberToMembership(members []rpadmin.RoleMember) []*dataplanev1.RoleMembership { | ||
| result := make([]*dataplanev1.RoleMembership, len(members)) | ||
| for i, m := range members { | ||
| result[i] = &dataplanev1.RoleMembership{Principal: m.PrincipalType + ":" + m.Name} |
Copilot
AI
Jan 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string concatenation to build the principal format should be consistent with how it's parsed. Consider extracting a constant or function to ensure the format (e.g., "User:username") matches the expected parsing logic throughout the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skipping this one on purpose.
c22a748 to
1a24b28
Compare
Retry command for Build#78613please wait until all jobs are finished before running the slash command |
1a24b28 to
2cc8a23
Compare
|
|
Cc: @micheleRP |
graham-rp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I'd really love to see this get a bit more testable, but I don't think that'd be hugely valuable for this specific set of commands
| RoleName: roleName, | ||
| })) | ||
| if err != nil { | ||
| return fmt.Errorf("unable to retrieve role %q: %v", roleName, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| return fmt.Errorf("unable to retrieve role %q: %v", roleName, err) | |
| return fmt.Errorf("unable to retrieve role %q: %w", roleName, err) |
nit: I don't think it particularly matters here, but %w would allow us to unwrap the error down the line if need be
| if memberships == nil { | ||
| return []rpadmin.RoleMember{} | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
| if memberships == nil { | |
| return []rpadmin.RoleMember{} | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I removed it from roleMemberToMembership but not from here 👍
This enables the Cloud's public API usage in rpk security role against cloud clusters. This feature is not available in serverless yet, hence the check at the beginning of every command.
2cc8a23 to
0b378d4
Compare
|
|
/backport v25.3.x |
This enables the Cloud's public API usage in
rpk security roleagainst cloud clusters.This feature is not available in serverless yet, hence the check at the beginning of every command.
Sample commands
These are the same outputs as the self-hosted command, and it still supports the
--formatflag where it exists.Create
List
Assign
Describe
Unassign
Delete
Backports Required
Release Notes
Features
rpk security rolecommand.