Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions docs/ga-test-report.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ All four phases executed. **5 real bugs found, all fixed with E2E + unit coverag
| B | service | PASS | — | — | create (file+flags), get, list, update, export. Map-form upstream nodes correctly rejected (400). |
| B | route | PASS | — | — | create (file+flags), get, list, update, export, delete. `get`/`delete` take id only; `update` uses `--uri`. |
| B | consumer | PASS | — | — | CRUD + export. MINOR: file `description:` key silently dropped (accepted key is `desc`). |
| B | credential | PASS | — | — | CRUD via server-returned id. MINOR: `create [id]` positional is treated as `name` (id is server-generated). |
| B | credential | PASS | — | — | CRUD via server-returned id. `create [id]` positional is now forwarded as the credential id via `PUT` (issue #36); a separate `--name` flag sets the display name. |
| B | ssl | **FIXED** | **BUG-1** | `TestSSL_UpdateFlagsRequireCertAndKey` | flag-based `ssl update` silently lost partial updates. |
| B | secret | PASS | — | — | CRUD (file+flags); id format is `provider/id`. |
| B | global-rule | PASS | — | — | CRUD (file+flags), export; id must equal the plugin name. MINOR: flag `--id` is required but its value is ignored by EE. |
Expand Down Expand Up @@ -67,7 +67,7 @@ A declarative file with a top-level `service_templates:` section validated as "C
## Minor observations (not fixed — low severity / by-design)

- **consumer**: `-f` file with a `description:` key is silently ignored; the accepted key is `desc`.
- **credential**: `create [id]` help text is misleading — the positional arg becomes the `name`; the `id` is server-generated. This is codified by `TestCredential_CreateWithPositionalID`, so it is current intended behavior.
- **credential**: `create [id]` now forwards the positional as the credential id via `PUT /apisix/admin/consumers/<u>/credentials/<id>` (issue #36). The display name has moved to a dedicated `--name` flag.
- **global-rule**: flag-based `create` requires `--id`, but API7 EE forces the id to equal the single plugin key, so the `--id` value is effectively ignored. File-based create errors clearly on a mismatch.
- **proto**: `--desc` and a `desc:` file field are silently dropped.
- **stream-route / EE behavior**: API7 EE rejects stream routes bound to a service it has classified as HTTP. Binding to a `type: stream` service works reliably. `a7` surfaces the EE error cleanly; this is EE-side behavior, not an `a7` bug.
Expand Down Expand Up @@ -124,7 +124,7 @@ Re-run after the Run 1 fixes (PR #31) and the `plugin-config` removal (PR #34) m
|---|---|---|---|---|
| R2-1 | 🟡 Bug | route | README documents `a7 route update <id> --desc "..."` but neither `route create` nor `route update` exposes a `--desc` flag. Description is only settable through `-f`. | ✅ Fixed in PR #39 / closes #35 |
| R2-2 | 🟡 UX | route | `a7 route list -g default` errors with `--service-id is required by API7 EE`. The e2e helper iterates services to aggregate routes; the CLI doesn't. | #42 (post-GA candidate) |
| R2-3 | 🟡 Bug | credential | `a7 credential create smoke-cred-X --consumer Y ...` returned a server-assigned UUID, ignoring the positional id. (Run 1 noted this as "intended" via `TestCredential_CreateWithPositionalID` — needs re-confirmation; if intended, drop the misleading `[id]` from the help.) | #36 |
| R2-3 | 🟡 Bug | credential | `a7 credential create smoke-cred-X --consumer Y ...` returned a server-assigned UUID, ignoring the positional id. | ✅ Fixed: positional now forwarded as id via `PUT`, `--name` added for display name (closes #36) |
| R2-4 | 🟡 Bug | global-rule | `a7 global-rule create --id X --plugins-json '{"cors":...}'` ignores `--id`; resource is created at `id=cors`. Run 1 noted "value is ignored by EE" as a minor; treat as a real bug: the CLI shouldn't accept a flag it will silently override. | #37 |
| R2-5 | 🟢 Cosmetic | stream-route | `-o yaml` renders `created_at: 1.779521636e+09` in scientific notation. Should be plain integer. | Note only, untracked |

Expand Down
28 changes: 24 additions & 4 deletions docs/user-guide/credential.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,22 +56,42 @@ a7 credential get key-1 --consumer jack -g default

### `a7 credential create`

Creates a new credential for a consumer from a JSON or YAML file.
Creates a new credential for a consumer.

**Usage:** `a7 credential create [id] --consumer <user> -g <gateway-group>`

The positional `[id]` is optional. When supplied (either as the positional or as `id:` inside a `--file` payload), the CLI sends `PUT /apisix/admin/consumers/<user>/credentials/<id>` and the credential is created with that exact id. When omitted, the CLI sends `POST` and the server assigns a UUID. If both the positional and the file specify an id, the positional wins.

| Flag | Short | Default | Description |
|------|-------|---------|-------------|
| `--consumer` | | | Consumer username (required) |
| `--gateway-group` | `-g` | | Target gateway group (required) |
| `--file` | `-f` | | Path to credential config file (required) |
| `--output` | `-o` | `yaml` | Output format (`json`, `yaml`) |
| `--file` | `-f` | | Path to credential config file (JSON or YAML) |
| `--name` | | | Credential display name (separate from the id) |
| `--desc` | | | Credential description |
| `--plugins-json` | | | Plugins as a JSON string |
| `--labels` | | | Labels in `key=value` format (repeatable) |
| `--output` | `-o` | `json` | Output format (`json`, `yaml`) |

**Examples:**

Create a credential from YAML:
Create a credential from YAML (id taken from the file's `id:` field, or auto-assigned if absent):
```bash
a7 credential create --consumer jack -g default -f key-auth.yaml
```

Create a credential with a client-chosen id and a display name:
```bash
a7 credential create my-key --consumer jack -g default --name "Jack's API key" \
--plugins-json '{"key-auth":{"key":"secret-token-val"}}'
```

Let the server assign the id:
```bash
a7 credential create --consumer jack -g default --name "Jack's API key" \
--plugins-json '{"key-auth":{"key":"secret-token-val"}}'
```

### `a7 credential update`

Updates an existing credential by ID using a JSON or YAML file. API7 EE uses JSON Patch (RFC 6902) for partial updates.
Expand Down
105 changes: 50 additions & 55 deletions pkg/cmd/credential/create/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ type Options struct {
File string
ID string

Name string
Desc string
PluginsJSON string
Labels []string
Expand All @@ -34,7 +35,7 @@ func NewCmd(f *cmd.Factory) *cobra.Command {
opts := &Options{IO: f.IOStreams, Client: f.HttpClient, Config: f.Config}
c := &cobra.Command{
Use: "create [id]",
Short: "Create a credential",
Short: "Create a credential (id is server-generated if omitted)",
Args: cobra.MaximumNArgs(1),
RunE: func(c *cobra.Command, args []string) error {
opts.Output, _ = c.Flags().GetString("output")
Expand All @@ -49,6 +50,7 @@ func NewCmd(f *cmd.Factory) *cobra.Command {

c.Flags().StringVar(&opts.Consumer, "consumer", "", "Consumer username")
c.Flags().StringVarP(&opts.File, "file", "f", "", "Path to JSON/YAML file with resource definition")
c.Flags().StringVar(&opts.Name, "name", "", "Credential display name")
c.Flags().StringVar(&opts.Desc, "desc", "", "Credential description")
c.Flags().StringVar(&opts.PluginsJSON, "plugins-json", "", "Plugins JSON string")
c.Flags().StringSliceVar(&opts.Labels, "labels", nil, "Labels in key=value format")
Expand All @@ -73,54 +75,43 @@ func actionRun(opts *Options) error {
if ggID == "" {
return fmt.Errorf("gateway group is required; use --gateway-group flag or set a default in context config")
}

if opts.File != "" {
payload, err := cmdutil.ReadResourceFile(opts.File, opts.IO.In)
if err != nil {
return err
}

if opts.ID != "" {
payload["name"] = opts.ID
delete(payload, "id")
} else if _, ok := payload["name"]; ok {
delete(payload, "id")
} else if id, ok := payload["id"]; ok {
payload["name"] = id
delete(payload, "id")
// Positional [id] overrides any id in the file payload.
id := opts.ID
if id == "" {
if raw, ok := payload["id"]; ok {
idStr, ok := raw.(string)
if !ok || strings.TrimSpace(idStr) == "" {
return fmt.Errorf("credential id must be a non-empty string")
}
id = idStr
}
}
// The id, if any, is carried in the URL path on PUT. Drop it from the body
// so the request body stays clean.
delete(payload, "id")

name, hasName, err := credentialNameFromPayload(payload)
if err != nil {
return err
if opts.Name != "" {
payload["name"] = opts.Name
}
// API7 EE requires a non-empty `name` on credentials. When the caller
// gave us an id but no explicit name, mirror the id into the name.
if _, hasName := payload["name"]; !hasName && id != "" {
payload["name"] = id
}

httpClient, err := opts.Client()
if err != nil {
return err
}

path := "/apisix/admin/consumers/" + opts.Consumer + "/credentials?gateway_group_id=" + ggID
client := api.NewClient(httpClient, cfg.BaseURL())
var body []byte
if hasName {
body, err = client.Put(fmt.Sprintf("/apisix/admin/consumers/%s/credentials/%s?gateway_group_id=%s", opts.Consumer, name, ggID), payload)
} else {
body, err = client.Post(path, payload)
}
if err != nil {
return fmt.Errorf("%s", cmdutil.FormatAPIError(err))
}

format := opts.Output
if format == "" {
format = "json"
}
return cmdutil.NewExporter(format, opts.IO.Out).WriteAPIResponse(body)
}

httpClient, err := opts.Client()
if err != nil {
return err
return submit(client, opts, ggID, id, payload)
}

pl := make(map[string]interface{})
Expand All @@ -139,41 +130,45 @@ func actionRun(opts *Options) error {
labels[parts[0]] = parts[1]
}

bodyReq := api.Credential{Name: opts.ID, Desc: opts.Desc}
name := opts.Name
if name == "" && opts.ID != "" {
// API7 EE requires a non-empty `name`. Mirror the id when the caller
// did not pass --name explicitly.
name = opts.ID
}
bodyReq := api.Credential{Name: name, Desc: opts.Desc}
Comment on lines +139 to +146
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Reject whitespace-only --name in non-file flow.

Line 140 trims --name, but if a user passes whitespace-only input and no positional id, the request proceeds with an empty name instead of failing fast (file flow rejects this case).

Suggested patch
 id := strings.TrimSpace(opts.ID)
 name := strings.TrimSpace(opts.Name)
+if opts.Name != "" && name == "" {
+	return fmt.Errorf("credential name must be a non-empty string")
+}
 if name == "" && id != "" {
 	// API7 EE requires a non-empty `name`. Mirror the id when the caller
 	// did not pass --name explicitly.
 	name = id
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
id := strings.TrimSpace(opts.ID)
name := strings.TrimSpace(opts.Name)
if name == "" && id != "" {
// API7 EE requires a non-empty `name`. Mirror the id when the caller
// did not pass --name explicitly.
name = id
}
bodyReq := api.Credential{Name: name, Desc: opts.Desc}
id := strings.TrimSpace(opts.ID)
name := strings.TrimSpace(opts.Name)
if opts.Name != "" && name == "" {
return fmt.Errorf("credential name must be a non-empty string")
}
if name == "" && id != "" {
// API7 EE requires a non-empty `name`. Mirror the id when the caller
// did not pass --name explicitly.
name = id
}
bodyReq := api.Credential{Name: name, Desc: opts.Desc}
🤖 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 `@pkg/cmd/credential/create/create.go` around lines 139 - 146, The current
logic trims opts.Name into name and silently allows a whitespace-only --name to
become empty when no id is supplied; change the validation so that if opts.Name
is non-empty (the user passed --name) but strings.TrimSpace(opts.Name) yields an
empty string and we're not in the file flow (i.e., id == ""), the command
returns an error instead of proceeding. Locate the block that sets id :=
strings.TrimSpace(opts.ID) and name := strings.TrimSpace(opts.Name) (and
constructs api.Credential{Name: name, Desc: opts.Desc}) and add a guard that
checks opts.Name != "" && name == "" && id == "" and returns a user-facing error
asking for a non-empty name.

if len(pl) > 0 {
bodyReq.Plugins = pl
}
if len(labels) > 0 {
bodyReq.Labels = labels
}

path := "/apisix/admin/consumers/" + opts.Consumer + "/credentials?gateway_group_id=" + ggID
client := api.NewClient(httpClient, cfg.BaseURL())
body, err := client.Post(path, bodyReq)
httpClient, err := opts.Client()
if err != nil {
return fmt.Errorf("%s", cmdutil.FormatAPIError(err))
return err
}
client := api.NewClient(httpClient, cfg.BaseURL())
return submit(client, opts, ggID, opts.ID, bodyReq)
}

var created api.Credential
if err := json.Unmarshal(body, &created); err != nil {
return fmt.Errorf("failed to decode response: %w", err)
func submit(client *api.Client, opts *Options, ggID, id string, body interface{}) error {
var (
raw []byte
err error
)
if id != "" {
raw, err = client.Put(fmt.Sprintf("/apisix/admin/consumers/%s/credentials/%s?gateway_group_id=%s", opts.Consumer, id, ggID), body)
} else {
raw, err = client.Post(fmt.Sprintf("/apisix/admin/consumers/%s/credentials?gateway_group_id=%s", opts.Consumer, ggID), body)
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if err != nil {
return fmt.Errorf("%s", cmdutil.FormatAPIError(err))
}

format := opts.Output
if format == "" {
format = "json"
}
return cmdutil.NewExporter(format, opts.IO.Out).Write(created)
}

func credentialNameFromPayload(payload map[string]interface{}) (string, bool, error) {
rawName, ok := payload["name"]
if !ok {
return "", false, nil
}
name, ok := rawName.(string)
if !ok || strings.TrimSpace(name) == "" {
return "", false, fmt.Errorf("credential name must be a non-empty string")
}
return name, true, nil
return cmdutil.NewExporter(format, opts.IO.Out).WriteAPIResponse(raw)
}
Loading
Loading