[UX-33] rpk: migrate login + profile creation flow to the Public API. #24877
Conversation
This is done in the oauth.loadFlow of the normal login steps. Also renamed the publicapi.ControlPlaneClientSet to CloudClientSet since the clients can come from different services and not only the "controlplane" service.
r-vasquez
requested review from
malinskibeniamin
and removed request for
a team
r-vasquez
changed the title
|
checking PR |
| // which have pagination enabled. | ||
| func Paginate[T any]( | ||
| ctx context.Context, | ||
| maxPages int, |
There was a problem hiding this comment.
Is max pages necessary? why is nextTokenPage == "" not enough?
There was a problem hiding this comment.
Just being paranoid to avoid an infinite loop.
Basically to avoid the case of a rogue server that never stops sending the nextTokenPage with the last saved value.
| return Paginate(ctx, maxPages, fetchPage) | ||
| } | ||
|
|
||
| func (cpCl *CloudClientSet) ServerlessClusterForID(ctx context.Context, ID string) (*controlplanev1beta2.ServerlessCluster, error) { |
There was a problem hiding this comment.
[nit]: I don't know if we have a standard in rpk, but, what do you think call it GetServerlessClusterByID?
There was a problem hiding this comment.
Not an rpk standard but instead of Go, we try to be as close as possible to general naming conventions.
For this case in particular: Getters.
rpk instances where we follow this (some examples):
- https://github.com/redpanda-data/redpanda/blob/dev/src/go/rpk/pkg/cloudapi/api_installpack.go#L54
- https://github.com/redpanda-data/redpanda/blob/dev/src/go/rpk/pkg/cloudapi/api_organization.go#L26
rpk instances where we don't follow this:
to fulfill interfaces:
- https://github.com/redpanda-data/redpanda/blob/dev/src/go/rpk/pkg/publicapi/transform.go#L62
to add extra logic/defaults : - https://github.com/redpanda-data/redpanda/blob/dev/src/go/rpk/pkg/config/rpk_yaml.go (However, one could argue this could be named differently)
However, there are some other places where we have failed to follow this too in the past.
|
|
||
| // Clusters returns all the Clusters using the pagination feature to traverse | ||
| // all pages of the list. | ||
| func (cpCl *CloudClientSet) Clusters(ctx context.Context) ([]*controlplanev1beta2.Cluster, error) { |
There was a problem hiding this comment.
[nit]: same here "ListClusters" or "GetClusters"
| wg.Wait() | ||
|
|
||
| err := errors.Join(orgErr, rgsErr, vcErr, cErr) | ||
| return org, rgs, vcs, cs, err |
There was a problem hiding this comment.
is it safe to use var out of the go routine?
There was a problem hiding this comment.
In this case, it is. We are not modifying any variable concurrently, every pair of variables is modified separately in different goroutines, and we don't need any synchronization (mutex for example).
For context, this is the equivalent function of our current Cloud API helper function in: https://github.com/redpanda-data/redpanda/blob/dev/src/go/rpk/pkg/cloudapi/cloudapi.go#L66 but using the Public API
| Cluster: controlplanev1beta2connect.NewClusterServiceClient(httpCl, host, opts...), | ||
| }, nil | ||
| Organization: iamv1beta2connect.NewOrganizationServiceClient(httpCl, host, opts...), |
There was a problem hiding this comment.
How about passing those clients as parameters to NewCloudClientSet? This approach allows us to use mockgen to generate mocks for these clients, making tests easier to write and clearer to understand. Additionally, we can leverage protobuf to auto-generate the necessary mocks.
There was a problem hiding this comment.
Yes, having a way to inject the internal clients can be beneficial as we expand the PublicAPI usage, especially when we introduce more convoluted usage such as in creating a Cluster.
What we can do is to refactor the client creation to accept optional client configuration options, and default to our current implementation, this is what I'm thinking:
type CloudClientSet struct {
Cluster controlplanev1beta2connect.ClusterServiceClient
Organization iamv1beta2connect.OrganizationServiceClient
ResourceGroup controlplanev1beta2connect.ResourceGroupServiceClient
Serverless controlplanev1beta2connect.ServerlessClusterServiceClient
}
type CloudClientSetOptions struct {
connectOptions []connect.ClientOption
httpClient *http.Client
Cluster controlplanev1beta2connect.ClusterServiceClient
Organization iamv1beta2connect.OrganizationServiceClient
ResourceGroup controlplanev1beta2connect.ResourceGroupServiceClient
Serverless controlplanev1beta2connect.ServerlessClusterServiceClient
}
// Opt configures the CloudClientSet options.
type Opt func(*CloudClientSetOptions)
func NewCloudClientSet(host, authToken string, opts ...Opt) *CloudClientSet {
if host == "" {
host = ControlPlaneProdURL
}
// Default values
clOpts := &CloudClientSetOptions{
connectOptions: []connect.ClientOption{
connect.WithInterceptors(
newAuthInterceptor(authToken), // Add the Bearer token.
newLoggerInterceptor(), // Add logs to every request.
),
},
httpClient: &http.Client{Timeout: 30 * time.Second},
}
// Apply custom options
for _, opt := range opts {
opt(clOpts)
}
// Initialize the CloudClientSet with default clients
cl := &CloudClientSet{
Cluster: controlplanev1beta2connect.NewClusterServiceClient(clOpts.httpClient, host, clOpts.connectOptions...),
Organization: iamv1beta2connect.NewOrganizationServiceClient(clOpts.httpClient, host, clOpts.connectOptions...),
ResourceGroup: controlplanev1beta2connect.NewResourceGroupServiceClient(clOpts.httpClient, host, clOpts.connectOptions...),
Serverless: controlplanev1beta2connect.NewServerlessClusterServiceClient(clOpts.httpClient, host, clOpts.connectOptions...),
}
// Override with custom clients from options if provided
if clOpts.Cluster != nil {
cl.Cluster = clOpts.Cluster
}
if clOpts.Organization != nil {
cl.Organization = clOpts.Organization
}
if clOpts.ResourceGroup != nil {
cl.ResourceGroup = clOpts.ResourceGroup
}
if clOpts.Serverless != nil {
cl.Serverless = clOpts.Serverless
}
return cl
}
// Options setters:
func WithClusterServiceClient(cluster controlplanev1beta2connect.ClusterServiceClient) Opt {
return func(cl *CloudClientSetOptions) { cl.Cluster = cluster }
}
func WithOrganizationClient(organization iamv1beta2connect.OrganizationServiceClient) Opt {
return func(cl *CloudClientSetOptions) { cl.Organization = organization }
}
func WithResourceGroupClient(resourceGroup controlplanev1beta2connect.ResourceGroupServiceClient) Opt {
return func(cl *CloudClientSetOptions) { cl.ResourceGroup = resourceGroup }
}
func WithServerlessClient(serverless controlplanev1beta2connect.ServerlessClusterServiceClient) Opt {
return func(cl *CloudClientSetOptions) { cl.Serverless = serverless }
}
func WithConnectClientOptions(opts ...connect.ClientOption) Opt {
return func(cl *CloudClientSetOptions) { cl.connectOptions = append(cl.connectOptions, opts...) }
}
func WithHTTPClient(client *http.Client) Opt {
return func(cl *CloudClientSetOptions) { cl.httpClient = client }
}
However, such a refactor does not currently benefit us, as we don't currently need to mock any of our tests. So, I'll leave this as a reference for any future refactor for this client.
I've added a fake response to one of our tests as you suggested earlier: b142aa2
This is an ongoing migration to the new Cloud API. There is no difference for the end user.
Instead of setting the inTest boolean on the test and having testing code in the implementation.
r-vasquez
force-pushed
the
migrate-login-flow
branch
from
d6f54f6 to
b142aa2
Compare
CI test resultstest results on build#61325
|
| @@ -93,6 +94,25 @@ func TestLoadFlow(t *testing.T) { | |||
| "cloud.client_secret=" + tt.clientSecret, | |||
| }, | |||
| } | |||
| handler := func() http.HandlerFunc { | |||
| return func(w http.ResponseWriter, r *http.Request) { | |||
| if strings.Contains(r.URL.Path, "GetCurrentOrganization") { | |||
|
/backport v24.3.x |
This PR migrates the login + profile creation flow to the Public API client instead of the Cloud API. The end goal is to migrate away from the CloudAPI client.
There is no change for the end user and the commands remain the same. These are just internal changes.
Backports Required
Release Notes