feat: add service-account api-keys commands#938
Conversation
Add a `service-account` (alias `sa`) command group with an `api-keys` (alias `ak`) subgroup to manage Service Account API keys via the CLI: - `create` — create an API key (requires --service-account and --description; optional --expires-at) - `revoke` — revoke an API key by KEY-ID, with a confirmation prompt (--yes to skip) - `rotate` — rotate an API key by KEY-ID, with a configurable grace period - `list` — list a service account's API keys Closes #937
|
Claude encountered an error after 4m 18s —— View job PR Review: feat: add service-account api-keys commands
Static AnalysisAll checks pass:
SummaryThe code is well-structured and the implementation is solid. Tests are thorough, covering happy paths, error cases, partial failures, dry-run behavior, and alias resolution. A few things to resolve: BlockingCommand hierarchy breaks the established CLI pattern. Every other command follows Non-blocking observations
Good work
|
…coded grace period default
|
First immediate thought - should colours be in a separate change? |
Well, using it here so hard to separate, but I can cut it out if you feel strongly about it 😄 |
tooky
left a comment
There was a problem hiding this comment.
How confident are we that the tests using fakes are going to standup to the API changing?
| } | ||
| // expires_at and last_used_at of 0 must render as N/A, not epoch zero (1970) | ||
| require.Contains(t, out, "N/A") | ||
| require.NotContains(t, out, "1970") |
There was a problem hiding this comment.
I can guess why this is here - but should maybe be explicit.
Is there a way 1970 could end up in the output. Number of milliseconds randomly or something.
There was a problem hiding this comment.
Can it end up there? I'm not sure expiry can be in the past (there is a check for that) and we're past creating then 😄
|
All other Kosli commands follows a |
Lipgloss is also nice for formatting text. A bit easier to read IMO, but maybe too much for this PR. |
I was intentionally avoiding introducing another dependency and Lipgloss pull in lot of things. Could be a separate thing if we starting using it more, but this prompt felt like an overkill |
@dangrondahl how do you see that here? Like this:
I can flip this around but it will create new verbs that are probably specific to keys, e.g. |
Could you please expand on that @tooky? |
19b4e79 to
b47a9d1
Compare
| "github.com/spf13/cobra" | ||
| ) | ||
|
|
||
| const serviceAccountDesc = `All Kosli service account operations.` |
There was a problem hiding this comment.
Architecture concern: kosli service-account api-keys <verb> breaks the established command pattern.
Every other command in this CLI follows kosli <verb> [resource] — e.g. kosli create flow, kosli list environments, kosli get trail. This PR introduces kosli <resource> <resource> <verb> (kosli service-account api-keys create).
As @dangrondahl noted in the PR comments, this is a significant departure. It introduces a new top-level noun group when the rest of the CLI is verb-first. Even if specific verbs like rotate don't have an existing verb group, they could be added (e.g. kosli rotate api-key or grouped under an existing verb with a new resource noun).
This is worth resolving before merging — it's a public-facing API design choice that's hard to change later.
| // standard foreground colors | ||
| ansiBlack = "\033[30m" | ||
| ansiRed = "\033[31m" | ||
| ansiGreen = "\033[32m" | ||
| ansiYellow = "\033[33m" | ||
| ansiBlue = "\033[34m" | ||
| ansiMagenta = "\033[35m" | ||
| ansiCyan = "\033[36m" | ||
| ansiWhite = "\033[37m" | ||
|
|
||
| // bright foreground colors | ||
| ansiBrightBlack = "\033[90m" | ||
| ansiBrightRed = "\033[91m" | ||
| ansiBrightGreen = "\033[92m" | ||
| ansiBrightYellow = "\033[93m" | ||
| ansiBrightBlue = "\033[94m" | ||
| ansiBrightMagenta = "\033[95m" | ||
| ansiBrightCyan = "\033[96m" | ||
| ansiBrightWhite = "\033[97m" |
There was a problem hiding this comment.
Nit: Most color constants are unused. Only ansiBold, ansiMagenta, and ansiGreen are referenced in production code. The rest are dead code — consider trimming to what's actually needed and adding others when a use arises (YAGNI). Not blocking.
b47a9d1 to
19b4e79
Compare
| func parseExpiresAt(value string) (int64, error) { | ||
| if value == "" { | ||
| return 0, nil | ||
| } | ||
|
|
||
| if epoch, err := strconv.ParseInt(value, 10, 64); err == nil { | ||
| return epoch, nil | ||
| } | ||
|
|
||
| formats := []string{ | ||
| "2006-1-2", | ||
| "2006-1-2 15:04:05", | ||
| time.RFC3339, | ||
| } | ||
| for _, format := range formats { | ||
| if t, err := time.Parse(format, value); err == nil { | ||
| return t.UTC().Unix(), nil | ||
| } | ||
| } | ||
|
|
||
| return 0, fmt.Errorf("invalid --expires-at value %q: expected an epoch timestamp or a date like '2006-01-02', '2006-01-02 15:04:05', or an RFC3339 timestamp", value) | ||
| } |
There was a problem hiding this comment.
Good implementation. parseExpiresAt is well-structured — accepts multiple intuitive date formats and falls through to a clear error. The decision to leave past-date validation to the server (per the author's reply to the earlier review) is reasonable and avoids client/server drift.
One minor note: the function silently returns 0 for "", which callers interpret as "no expiry" via *int64 + omitempty. This implicit sentinel works but is a subtle contract — worth being aware of if the function is reused elsewhere.
| // Rotated key values are only returned once, so collect each successful | ||
| // response and print what we have even if a later key fails (rather than | ||
| // losing the new keys that were already rotated). | ||
| keys := make([]json.RawMessage, 0, len(args)) | ||
| var runErr error | ||
| for _, keyID := range args { | ||
| url, err := url.JoinPath(global.Host, "api/v2/service-accounts", global.Org, o.serviceAccount, "api-keys", keyID, "rotate") | ||
| if err != nil { | ||
| runErr = err | ||
| break | ||
| } | ||
|
|
||
| reqParams := &requests.RequestParams{ | ||
| Method: http.MethodPost, | ||
| URL: url, | ||
| Payload: o.payload, | ||
| DryRun: global.DryRun, | ||
| Token: global.ApiToken, | ||
| } | ||
| response, err := kosliClient.Do(reqParams) | ||
| if err != nil { | ||
| runErr = err | ||
| break | ||
| } | ||
| if !global.DryRun { | ||
| keys = append(keys, json.RawMessage(response.Body)) | ||
| } | ||
| } | ||
|
|
||
| if !global.DryRun && len(keys) > 0 { | ||
| raw, err := json.Marshal(keys) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := output.FormattedPrint(string(raw), o.output, out, 0, | ||
| map[string]output.FormatOutputFunc{ | ||
| "table": printApiKeysAsTable, | ||
| "json": output.PrintJson, | ||
| }); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return runErr | ||
| } |
There was a problem hiding this comment.
Well designed partial-failure handling. Collecting json.RawMessage results and printing already-rotated keys before surfacing the error is the right approach — rotated key values are one-time-only, so losing them would be a real operational problem. The break on url.JoinPath error (line 115) correctly falls through to the printing path too. Good work.
| for i, keyID := range args { | ||
| url, err := url.JoinPath(global.Host, "api/v2/service-accounts", global.Org, o.serviceAccount, "api-keys", keyID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| reqParams := &requests.RequestParams{ | ||
| Method: http.MethodDelete, | ||
| URL: url, | ||
| DryRun: global.DryRun, | ||
| Token: global.ApiToken, | ||
| } | ||
| if _, err := kosliClient.Do(reqParams); err != nil { | ||
| // revocation is destructive and one-way: make clear which keys were | ||
| // already revoked before this one failed (user-facing, styled green), | ||
| // while keeping the returned error plain (no ANSI). | ||
| if i > 0 { | ||
| revoked := make([]string, i) | ||
| for j, k := range args[:i] { | ||
| revoked[j] = style(out, k, ansiBold, ansiGreen) | ||
| } | ||
| logger.Info("keys already revoked before this failure: %s", strings.Join(revoked, ", ")) | ||
| } | ||
| return fmt.Errorf("failed to revoke API key %s: %w", keyID, err) | ||
| } | ||
| if !global.DryRun { | ||
| logger.Info("API key %s for service account %s was revoked", style(out, keyID, ansiBold, ansiGreen), o.serviceAccount) | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistency: url.JoinPath error uses return err (line 101), which skips the partial-success reporting at lines 113-118.
If url.JoinPath fails after key k1 has already been revoked, the user won't see that k1 was successfully revoked. This is unlikely in practice (the URL components are known strings), but it's inconsistent with the intent of the partial-failure handling — and inconsistent with rotateApiKey.go which correctly uses break for the same scenario.
| for i, keyID := range args { | |
| url, err := url.JoinPath(global.Host, "api/v2/service-accounts", global.Org, o.serviceAccount, "api-keys", keyID) | |
| if err != nil { | |
| return err | |
| } | |
| reqParams := &requests.RequestParams{ | |
| Method: http.MethodDelete, | |
| URL: url, | |
| DryRun: global.DryRun, | |
| Token: global.ApiToken, | |
| } | |
| if _, err := kosliClient.Do(reqParams); err != nil { | |
| // revocation is destructive and one-way: make clear which keys were | |
| // already revoked before this one failed (user-facing, styled green), | |
| // while keeping the returned error plain (no ANSI). | |
| if i > 0 { | |
| revoked := make([]string, i) | |
| for j, k := range args[:i] { | |
| revoked[j] = style(out, k, ansiBold, ansiGreen) | |
| } | |
| logger.Info("keys already revoked before this failure: %s", strings.Join(revoked, ", ")) | |
| } | |
| return fmt.Errorf("failed to revoke API key %s: %w", keyID, err) | |
| } | |
| if !global.DryRun { | |
| logger.Info("API key %s for service account %s was revoked", style(out, keyID, ansiBold, ansiGreen), o.serviceAccount) | |
| } | |
| } | |
| url, err := url.JoinPath(global.Host, "api/v2/service-accounts", global.Org, o.serviceAccount, "api-keys", keyID) | |
| if err != nil { | |
| if i > 0 { | |
| revoked := make([]string, i) | |
| for j, k := range args[:i] { | |
| revoked[j] = style(out, k, ansiBold, ansiGreen) | |
| } | |
| logger.Info("keys already revoked before this failure: %s", strings.Join(revoked, ", ")) | |
| } | |
| return fmt.Errorf("failed to revoke API key %s: %w", keyID, err) | |
| } |
Alternatively, refactor to use the same break + post-loop pattern as rotateApiKey.go.
| func (suite *ServiceAccountApiKeysCommandTestSuite) TestApiKeysSuccessOutput() { | ||
| fake := httpfake.New() | ||
| defer fake.Close() | ||
| fake.NewHandler(). | ||
| Post("/api/v2/service-accounts/docs-cmd-test-user/test-sa/api-keys"). | ||
| Reply(201). | ||
| BodyString(`{"id":"id-1","key":"sk_created","description":"ci","created_at":1780584129.5,"expires_at":0}`) | ||
| fake.NewHandler(). | ||
| Get("/api/v2/service-accounts/docs-cmd-test-user/test-sa/api-keys"). | ||
| Reply(200). | ||
| BodyString(`[{"id":"id-1","description":"ci","created_at":1780584129.5,"expires_at":0,"last_used_at":0}]`) | ||
| fake.NewHandler(). | ||
| Post("/api/v2/service-accounts/docs-cmd-test-user/test-sa/api-keys/k1/rotate"). | ||
| Reply(201). | ||
| BodyString(`{"id":"k1","key":"sk_one","description":"one","created_at":1,"expires_at":0}`) | ||
| fake.NewHandler(). | ||
| Post("/api/v2/service-accounts/docs-cmd-test-user/test-sa/api-keys/k2/rotate"). | ||
| Reply(201). | ||
| BodyString(`{"id":"k2","key":"sk_two","description":"two","created_at":1,"expires_at":0}`) | ||
|
|
||
| args := fmt.Sprintf(" --host %s --org %s --api-token %s", fake.Server.URL, global.Org, global.ApiToken) | ||
| tests := []cmdTestCase{ | ||
| { | ||
| wantError: false, | ||
| name: "create prints the new key value", | ||
| cmd: "service-account api-keys create -s test-sa -d ci --output json" + args, | ||
| goldenRegex: `sk_created`, | ||
| }, | ||
| { | ||
| wantError: false, | ||
| name: "list prints the returned keys", | ||
| cmd: "service-account api-keys list -s test-sa --output json" + args, | ||
| goldenRegex: `id-1`, | ||
| }, | ||
| { | ||
| wantError: false, | ||
| name: "rotate of multiple keys prints all new key values", | ||
| cmd: "service-account api-keys rotate k1 k2 -s test-sa --output json" + args, | ||
| goldenRegex: `(?s)sk_one.*sk_two`, | ||
| }, | ||
| } | ||
|
|
||
| runTestCmd(suite.T(), tests) | ||
| } |
There was a problem hiding this comment.
Testing observation: tests use httpfake stubs, which won't catch API contract drift.
This was raised by @tooky — the fakes define the response shapes inline, so if the real API changes field names, types, or structure, these tests will still pass. This is fine for testing CLI logic (flag parsing, output formatting, error handling), but it means the response contract isn't verified.
Consider either:
- Adding a comment noting these are unit tests for CLI behavior, not API contract tests
- Extracting the fake response bodies into shared fixtures that can be compared against API docs or schema in a separate step
Not blocking — the tests are well-structured for what they test.
| const serviceAccountDesc = `All Kosli service account operations.` | ||
|
|
||
| func newServiceAccountCmd(out io.Writer) *cobra.Command { | ||
| cmd := &cobra.Command{ |
There was a problem hiding this comment.
Architecture: kosli service-account api-keys <verb> breaks the established CLI pattern.
Every other command in this CLI follows kosli <verb> [resource] — e.g. kosli create flow, kosli list environments, kosli get trail. This PR introduces kosli <resource> <resource> <verb> which is a significant departure.
As @dangrondahl raised in the PR comments, this needs resolving before merge — it's a public-facing API design choice that's hard to change later. Some options:
- Verb-first (matching existing pattern):
kosli create api-key,kosli list api-keys,kosli revoke api-key,kosli rotate api-key— new verbsrevoke/rotateare fine as top-level groups. - Current noun-first:
kosli service-account api-keys <verb>— more discoverable as a group, but inconsistent with 100% of existing commands.
This is a blocking design decision for the team — the code itself is well-structured either way, but consistency with the existing CLI surface matters.
| for i, keyID := range args { | ||
| url, err := url.JoinPath(global.Host, "api/v2/service-accounts", global.Org, o.serviceAccount, "api-keys", keyID) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Inconsistency with rotate: url.JoinPath error uses return err, skipping partial-success reporting.
In rotateApiKey.go, the same url.JoinPath error path uses break to fall through to the "print what we have" logic (line 115). Here, a return err exits immediately — if this fires after key k1 has already been revoked, the user won't see that k1 was successfully revoked.
url.JoinPath won't realistically fail here (the components are known strings), but the inconsistency with rotate's partial-failure design is worth noting. Consider using the same break-and-report pattern for symmetry.
| for i, keyID := range args { | |
| url, err := url.JoinPath(global.Host, "api/v2/service-accounts", global.Org, o.serviceAccount, "api-keys", keyID) | |
| if err != nil { | |
| return err | |
| } | |
| url, err := url.JoinPath(global.Host, "api/v2/service-accounts", global.Org, o.serviceAccount, "api-keys", keyID) | |
| if err != nil { | |
| if i > 0 { | |
| revoked := make([]string, i) | |
| for j, k := range args[:i] { | |
| revoked[j] = style(out, k, ansiBold, ansiGreen) | |
| } | |
| logger.Info("keys already revoked before this failure: %s", strings.Join(revoked, ", ")) | |
| } | |
| return fmt.Errorf("failed to revoke API key %s: %w", keyID, err) | |
| } |
| // standard foreground colors | ||
| ansiBlack = "\033[30m" | ||
| ansiRed = "\033[31m" | ||
| ansiGreen = "\033[32m" | ||
| ansiYellow = "\033[33m" | ||
| ansiBlue = "\033[34m" | ||
| ansiMagenta = "\033[35m" | ||
| ansiCyan = "\033[36m" | ||
| ansiWhite = "\033[37m" | ||
|
|
||
| // bright foreground colors | ||
| ansiBrightBlack = "\033[90m" | ||
| ansiBrightRed = "\033[91m" | ||
| ansiBrightGreen = "\033[92m" | ||
| ansiBrightYellow = "\033[93m" | ||
| ansiBrightBlue = "\033[94m" | ||
| ansiBrightMagenta = "\033[95m" | ||
| ansiBrightCyan = "\033[96m" | ||
| ansiBrightWhite = "\033[97m" | ||
| ) |
There was a problem hiding this comment.
Nit: Most color constants are unused. Only ansiBold, ansiMagenta, and ansiGreen are referenced in production code. The remaining 15 are dead code exercised only by the test for style() itself. Consider trimming to what's actually needed (YAGNI) — easy to add more later when a command needs them.
|
|
||
| const serviceAccountDesc = `All Kosli service account operations.` | ||
|
|
||
| func newServiceAccountCmd(out io.Writer) *cobra.Command { |
There was a problem hiding this comment.
Architecture: kosli service-account api-keys <verb> breaks the established CLI pattern.
Every other command in this CLI follows kosli <verb> [resource] — e.g. kosli create flow, kosli list environments, kosli get trail. This PR introduces kosli <resource> <resource> <verb> which is a significant departure from the existing convention.
As @dangrondahl raised in the PR comments, this is a public-facing UX decision worth resolving before merge. Some options:
- Verb-first (consistent with existing CLI):
kosli create api-key,kosli list api-keys,kosli revoke api-key,kosli rotate api-key—revoke/rotateare reasonable new top-level verb groups. - Keep noun-first:
kosli service-account api-keys <verb>— more discoverable as a group but inconsistent with every other command.
This is a team design decision rather than a code quality issue — but it will be hard to change once shipped.
| for i, keyID := range args { | ||
| url, err := url.JoinPath(global.Host, "api/v2/service-accounts", global.Org, o.serviceAccount, "api-keys", keyID) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Inconsistency with rotate: url.JoinPath error uses return err, skipping partial-success reporting.
In rotateApiKey.go:114-116, the same url.JoinPath error path uses break to fall through to the "print what we have" logic. Here, return err exits immediately — if this fires after args[0] has already been revoked, the user won't see that it was successfully revoked.
url.JoinPath won't realistically fail here, but for consistency with rotate's partial-failure handling pattern, consider:
| return err | |
| url, err := url.JoinPath(global.Host, "api/v2/service-accounts", global.Org, o.serviceAccount, "api-keys", keyID) | |
| if err != nil { | |
| if i > 0 { | |
| revoked := make([]string, i) | |
| for j, k := range args[:i] { | |
| revoked[j] = style(out, k, ansiBold, ansiGreen) | |
| } | |
| logger.Info("keys already revoked before this failure: %s", strings.Join(revoked, ", ")) | |
| } | |
| return fmt.Errorf("failed to revoke API key %s: %w", keyID, err) | |
| } |
Minor — not blocking.
| // standard foreground colors | ||
| ansiBlack = "\033[30m" | ||
| ansiRed = "\033[31m" | ||
| ansiGreen = "\033[32m" | ||
| ansiYellow = "\033[33m" | ||
| ansiBlue = "\033[34m" | ||
| ansiMagenta = "\033[35m" | ||
| ansiCyan = "\033[36m" | ||
| ansiWhite = "\033[37m" | ||
|
|
||
| // bright foreground colors | ||
| ansiBrightBlack = "\033[90m" | ||
| ansiBrightRed = "\033[91m" | ||
| ansiBrightGreen = "\033[92m" | ||
| ansiBrightYellow = "\033[93m" | ||
| ansiBrightBlue = "\033[94m" | ||
| ansiBrightMagenta = "\033[95m" | ||
| ansiBrightCyan = "\033[96m" | ||
| ansiBrightWhite = "\033[97m" |
There was a problem hiding this comment.
Nit: 15 of 18 color constants are unused in production code. Only ansiBold, ansiMagenta, and ansiGreen are referenced outside the test. Consider trimming to what's currently needed — easy to add more later when a command actually uses them. The test for style() validates the mechanism, not that these constants have consumers.
| func (suite *ServiceAccountApiKeysCommandTestSuite) TestApiKeysSuccessOutput() { | ||
| fake := httpfake.New() | ||
| defer fake.Close() | ||
| fake.NewHandler(). | ||
| Post("/api/v2/service-accounts/docs-cmd-test-user/test-sa/api-keys"). | ||
| Reply(201). | ||
| BodyString(`{"id":"id-1","key":"sk_created","description":"ci","created_at":1780584129.5,"expires_at":0}`) | ||
| fake.NewHandler(). | ||
| Get("/api/v2/service-accounts/docs-cmd-test-user/test-sa/api-keys"). | ||
| Reply(200). | ||
| BodyString(`[{"id":"id-1","description":"ci","created_at":1780584129.5,"expires_at":0,"last_used_at":0}]`) | ||
| fake.NewHandler(). | ||
| Post("/api/v2/service-accounts/docs-cmd-test-user/test-sa/api-keys/k1/rotate"). | ||
| Reply(201). | ||
| BodyString(`{"id":"k1","key":"sk_one","description":"one","created_at":1,"expires_at":0}`) | ||
| fake.NewHandler(). | ||
| Post("/api/v2/service-accounts/docs-cmd-test-user/test-sa/api-keys/k2/rotate"). | ||
| Reply(201). | ||
| BodyString(`{"id":"k2","key":"sk_two","description":"two","created_at":1,"expires_at":0}`) | ||
|
|
||
| args := fmt.Sprintf(" --host %s --org %s --api-token %s", fake.Server.URL, global.Org, global.ApiToken) | ||
| tests := []cmdTestCase{ | ||
| { | ||
| wantError: false, | ||
| name: "create prints the new key value", | ||
| cmd: "service-account api-keys create -s test-sa -d ci --output json" + args, | ||
| goldenRegex: `sk_created`, | ||
| }, | ||
| { | ||
| wantError: false, | ||
| name: "list prints the returned keys", | ||
| cmd: "service-account api-keys list -s test-sa --output json" + args, | ||
| goldenRegex: `id-1`, | ||
| }, | ||
| { | ||
| wantError: false, | ||
| name: "rotate of multiple keys prints all new key values", | ||
| cmd: "service-account api-keys rotate k1 k2 -s test-sa --output json" + args, | ||
| goldenRegex: `(?s)sk_one.*sk_two`, | ||
| }, | ||
| } | ||
|
|
||
| runTestCmd(suite.T(), tests) | ||
| } |
There was a problem hiding this comment.
Test contract risk (echoing @tooky's concern). These httpfake stubs define response shapes inline. If the real API changes field names, types, or structure, these tests will still pass — they validate CLI logic (flag parsing, output formatting, error handling) but not the API contract.
This is fine for what the tests cover, but worth noting explicitly — perhaps a comment at the top of this test noting it's a CLI-behavior test, not an API contract test. If the team has (or builds) an OpenAPI spec or schema, that would be the right place to validate the contract separately.
| }) | ||
| } | ||
|
|
||
| func printApiKeysListAsTable(raw string, out io.Writer, page int) error { |
There was a problem hiding this comment.
Minor: printApiKeysListAsTable uses map[string]interface{} while create/rotate use a typed struct.
The list endpoint returns metadata only (no secret key value), so using a different type is understandable. But the inconsistency means there are two separate code paths for rendering timestamps — apiKeyTableRows (typed, uses float64 directly) and this function (untyped, goes through interface{}). Both work correctly, but consider whether a shared apiKeyMetadata struct for list would reduce surface area for future drift.
Not blocking — the current implementation is correct.
Add a
service-account(aliassa) command group with anapi-keys(aliasak) subgroup to manage Service Account API keys via the CLI:create— create an API key (requires --service-account and --description; optional --expires-at)revoke— revoke an API key by KEY-ID, with a confirmation prompt (--assume-yesto skip)rotate— rotate an API key by KEY-ID, with a configurable grace periodlist— list a service account's API keysAlso adds a shared util for using terminal colours.
Closes #937