refactor: centralise Platform API HTTP client#93
Conversation
Introduce internal/client.Client as a single shared implementation of SigV4-signed HTTP requests to the Platform API. Previously, signing logic was duplicated across three locations: - internal/commands/cluster/api.go (GET only, 15s timeout) - internal/services/cluster/service.go (POST only, inline, fresh http.Client per call) Each had inconsistencies: different timeouts, different return signatures (some omitted the status code), and service.go created a new http.Client on every call rather than reusing one. client.New(ctx) resolves the Platform API URL and AWS credentials once. The Client exposes Get, Post and Delete, each returning ([]byte, int, error) so callers can inspect the status code without the client second-guessing what counts as an error. All callers in commands/cluster and services/cluster are updated to use the new client. The SubmitClusterRequest.PlatformAPIURL and AWSConfig fields are replaced with a single Client field.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rrp-bot The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @rrp-bot. Thanks for your PR. I'm waiting for a openshift-online member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci
Bot
added
the
needs-ok-to-test
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift-online/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a reusable SigV4 HTTP client and migrates cluster listing, lookup, kubeconfig, and submission flows to use it. Existing response parsing, status handling, and output formatting remain in place. ChangesCluster API client migration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Command
participant Client
participant ClusterService
participant PlatformAPI
Command->>Client: New(ctx)
Client-->>Command: configured SigV4 client
Command->>ClusterService: submit payload with Client
ClusterService->>Client: Post("/api/v0/clusters", body)
Client->>PlatformAPI: signed HTTP request
PlatformAPI-->>Client: response body and status
Client-->>ClusterService: body, status, error
ClusterService-->>Command: parsed response
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/services/cluster/service.go (1)
41-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
AWSConfigfield appears dead after this migration.
SubmitClusterno longer readsreq.AWSConfig, and neither caller increate.gosets it (both set onlyPayload/Client/PlacementOverride). The PR objective statesAWSConfigwas replaced byClient, but the field is still declared here. Consider removing it (and the now-unnecessaryawsimport if it becomes unused) to match the stated contract.♻️ Proposed change
type SubmitClusterRequest struct { Payload map[string]interface{} Client *client.Client PlacementOverride string // Optional - overrides placement in payload if set - AWSConfig aws.Config }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/services/cluster/service.go` around lines 41 - 44, Remove the unused AWSConfig field from the relevant request struct and delete the aws import if no other symbols in the file require it. Verify SubmitCluster and all callers continue using Client, Payload, and PlacementOverride only.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/client/client.go`:
- Around line 51-68: The Client stores a one-time credential snapshot, causing
reused clients to use expired credentials. Update Client and New to retain
cfg.Credentials as the provider, then update do to call Retrieve(ctx) and use
the returned credentials for each request; preserve retrieval error handling and
remove the eager retrieval from New.
---
Nitpick comments:
In `@internal/services/cluster/service.go`:
- Around line 41-44: Remove the unused AWSConfig field from the relevant request
struct and delete the aws import if no other symbols in the file require it.
Verify SubmitCluster and all callers continue using Client, Payload, and
PlacementOverride only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift-online/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: dadecb87-7d0f-44d8-bcd3-f578cda192d7
📒 Files selected for processing (6)
internal/client/client.gointernal/commands/cluster/api.gointernal/commands/cluster/create.gointernal/commands/cluster/kubeconfig.gointernal/commands/cluster/list.gointernal/services/cluster/service.go
Storing the resolved awssdk.Credentials value meant a reused Client would sign with an expired credential snapshot after the STS session expires. Store the CredentialsProvider from the AWS config instead and call Retrieve(ctx) on each request — the SDK's CredentialsCache handles thread-safe caching and refresh automatically.
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
2 participants
Summary
internal/clientpackage with a singleClienttype that owns SigV4 signing, credential resolution, and HTTP transport for all Platform API callscommands/cluster/api.go,services/cluster/service.goSubmitClusterRequest.PlatformAPIURL+AWSConfigfields with a singleClientfieldProblem
SigV4 signing logic was duplicated across three places with meaningful inconsistencies:
commands/cluster/api.goservices/cluster/service.goAdding nodepool support in PR #92 would have added a fourth copy. This PR consolidates before that lands.
What changed
internal/client/client.New(ctx)resolves the Platform API URL and AWS credentials once and returns a*Client. All commands call this at the start of theirRunEhandler. The client exposesGet,Post, andDelete, each returning([]byte, int, error)— status code is always returned so callers decide what counts as an error rather than the client hard-coding 200.Note: Go is not available in this environment so the build could not be verified locally — CI will confirm.
Summary by CodeRabbit
New Features
Improvements