From 1dc6da83e2e55983684d0bd1b3c34b27bb9a3f70 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Sat, 23 May 2026 16:50:38 +0800 Subject: [PATCH 1/3] fix(route): add --desc flag to create and update The README documented `a7 route update --desc "..."` but neither `route create` nor `route update` exposed a --desc flag; the field was only settable via -f. This made the documented invocation fail with 'unknown flag: --desc'. - Add --desc to flag-based create; wire to api.Route.Desc. - Add --desc to flag-based update with DescSet tracking so `--desc ""` explicitly clears the description (rather than being treated as 'unchanged'). - Unit tests cover the request-body wiring for both paths and the clear-via-empty-string case. - E2E TestRoute_DescFlagCRUD round-trips create / update / clear against a real instance. Closes #35. --- pkg/cmd/route/create/create.go | 3 ++ pkg/cmd/route/create/create_test.go | 39 +++++++++++++++ pkg/cmd/route/update/update.go | 8 ++++ pkg/cmd/route/update/update_test.go | 74 +++++++++++++++++++++++++++++ test/e2e/route_test.go | 43 +++++++++++++++++ 5 files changed, 167 insertions(+) diff --git a/pkg/cmd/route/create/create.go b/pkg/cmd/route/create/create.go index 3796431..6f7944a 100644 --- a/pkg/cmd/route/create/create.go +++ b/pkg/cmd/route/create/create.go @@ -24,6 +24,7 @@ type Options struct { File string Name string + Desc string URI string Paths []string Methods []string @@ -47,6 +48,7 @@ func NewCmd(f *cmd.Factory) *cobra.Command { } c.Flags().StringVar(&opts.Name, "name", "", "Route name") + c.Flags().StringVar(&opts.Desc, "desc", "", "Route description") c.Flags().StringVarP(&opts.File, "file", "f", "", "Path to JSON/YAML file with resource definition") c.Flags().StringVar(&opts.URI, "uri", "", "Route URI (single path, APISIX compat)") c.Flags().StringSliceVar(&opts.Paths, "path", nil, "Route path (repeatable, API7 EE format)") @@ -136,6 +138,7 @@ func actionRun(opts *Options) error { bodyReq := api.Route{ Name: opts.Name, + Desc: opts.Desc, Paths: paths, Methods: opts.Methods, Host: opts.Host, diff --git a/pkg/cmd/route/create/create_test.go b/pkg/cmd/route/create/create_test.go index 2105218..3f5423e 100644 --- a/pkg/cmd/route/create/create_test.go +++ b/pkg/cmd/route/create/create_test.go @@ -238,3 +238,42 @@ func TestCreateRoute_FromYAMLFile(t *testing.T) { } registry.Verify(t) } + +// TestCreateRoute_DescFlag guards the regression where flag-based `route create` +// had no `--desc` flag despite the README documenting one. The description must +// reach the API request body. +func TestCreateRoute_DescFlag(t *testing.T) { + ios, _, out, _ := iostreams.Test() + registry := &httpmock.Registry{} + registry.RegisterResponder(http.MethodPost, "/apisix/admin/routes", func(r *http.Request) (httpmock.Response, error) { + body, err := io.ReadAll(r.Body) + if err != nil { + return httpmock.Response{}, err + } + if !strings.Contains(string(body), `"desc":"my description"`) { + t.Fatalf("expected --desc to land in request body, got: %s", string(body)) + } + return httpmock.JSONResponse(`{"id":"r-desc","name":"demo","desc":"my description","service_id":"svc1"}`), nil + }) + + opts := &Options{ + IO: ios, + Client: func() (*http.Client, error) { return registry.GetClient(), nil }, + Config: func() (config.Config, error) { + return &mockConfig{baseURL: "http://api.local", gatewayGroup: "gg1"}, nil + }, + GatewayGroup: "gg1", + URI: "/demo", + Name: "demo", + Desc: "my description", + ServiceID: "svc1", + } + + if err := actionRun(opts); err != nil { + t.Fatalf("actionRun failed: %v", err) + } + if !strings.Contains(out.String(), "my description") { + t.Fatalf("expected desc in output, got: %s", out.String()) + } + registry.Verify(t) +} diff --git a/pkg/cmd/route/update/update.go b/pkg/cmd/route/update/update.go index 307b782..2079ffb 100644 --- a/pkg/cmd/route/update/update.go +++ b/pkg/cmd/route/update/update.go @@ -25,6 +25,8 @@ type Options struct { ID string Name string + Desc string + DescSet bool URI string Methods []string Host string @@ -48,11 +50,13 @@ func NewCmd(f *cmd.Factory) *cobra.Command { opts.GatewayGroup, _ = c.Flags().GetString("gateway-group") opts.StatusSet = c.Flags().Changed("status") opts.PrioritySet = c.Flags().Changed("priority") + opts.DescSet = c.Flags().Changed("desc") return actionRun(opts) }, } c.Flags().StringVar(&opts.Name, "name", "", "Route name") + c.Flags().StringVar(&opts.Desc, "desc", "", "Route description (pass empty string to clear)") c.Flags().StringVar(&opts.URI, "uri", "", "Route URI") c.Flags().StringSliceVar(&opts.Methods, "methods", nil, "Allowed HTTP methods") c.Flags().StringVar(&opts.Host, "host", "", "Route host") @@ -123,6 +127,10 @@ func actionRun(opts *Options) error { if opts.Name != "" { bodyReq.Name = opts.Name } + // DescSet lets the user explicitly clear the description with --desc "". + if opts.DescSet { + bodyReq.Desc = opts.Desc + } if opts.URI != "" { bodyReq.URI = "" bodyReq.URIs = nil diff --git a/pkg/cmd/route/update/update_test.go b/pkg/cmd/route/update/update_test.go index e0d0f49..65f570e 100644 --- a/pkg/cmd/route/update/update_test.go +++ b/pkg/cmd/route/update/update_test.go @@ -130,3 +130,77 @@ func TestUpdateRoute_FromFile(t *testing.T) { } registry.Verify(t) } + +// TestUpdateRoute_DescFlag guards the regression where `route update` had no +// `--desc` flag despite the README documenting one. The new description must +// reach the PUT body. +func TestUpdateRoute_DescFlag(t *testing.T) { + ios, _, out, _ := iostreams.Test() + registry := &httpmock.Registry{} + registry.Register(http.MethodGet, "/apisix/admin/routes/r1", httpmock.JSONResponse(`{"id":"r1","name":"old-name","desc":"old desc","paths":["/demo"],"service_id":"svc1","status":1}`)) + registry.RegisterResponder(http.MethodPut, "/apisix/admin/routes/r1", func(req *http.Request) (httpmock.Response, error) { + var payload map[string]interface{} + if err := json.NewDecoder(req.Body).Decode(&payload); err != nil { + return httpmock.Response{}, fmt.Errorf("decode request body: %w", err) + } + if payload["desc"] != "updated desc" { + return httpmock.Response{}, fmt.Errorf("expected updated desc in payload, got desc=%v", payload["desc"]) + } + return httpmock.JSONResponse(`{"id":"r1","name":"old-name","desc":"updated desc","paths":["/demo"],"service_id":"svc1","status":1}`), nil + }) + + opts := &Options{ + IO: ios, + Client: func() (*http.Client, error) { return registry.GetClient(), nil }, + Config: func() (config.Config, error) { + return &mockConfig{baseURL: "http://api.local", gatewayGroup: "gg1"}, nil + }, + ID: "r1", + GatewayGroup: "gg1", + Desc: "updated desc", + DescSet: true, + } + if err := actionRun(opts); err != nil { + t.Fatalf("actionRun failed: %v", err) + } + if !strings.Contains(out.String(), "updated desc") { + t.Fatalf("expected updated desc in output: %s", out.String()) + } + registry.Verify(t) +} + +// TestUpdateRoute_DescFlagCanClear verifies that passing --desc "" explicitly +// clears the existing description (rather than being treated as "unset"). +func TestUpdateRoute_DescFlagCanClear(t *testing.T) { + ios, _, _, _ := iostreams.Test() + registry := &httpmock.Registry{} + registry.Register(http.MethodGet, "/apisix/admin/routes/r1", httpmock.JSONResponse(`{"id":"r1","name":"old-name","desc":"old desc","paths":["/demo"],"service_id":"svc1","status":1}`)) + registry.RegisterResponder(http.MethodPut, "/apisix/admin/routes/r1", func(req *http.Request) (httpmock.Response, error) { + var payload map[string]interface{} + if err := json.NewDecoder(req.Body).Decode(&payload); err != nil { + return httpmock.Response{}, fmt.Errorf("decode request body: %w", err) + } + // With api.Route's `omitempty` desc tag, clearing should drop the field + // from the payload entirely; an empty string would also be acceptable. + if d, present := payload["desc"]; present && d != "" { + return httpmock.Response{}, fmt.Errorf("expected desc to be cleared, got desc=%v", d) + } + return httpmock.JSONResponse(`{"id":"r1","name":"old-name","paths":["/demo"],"service_id":"svc1","status":1}`), nil + }) + + opts := &Options{ + IO: ios, + Client: func() (*http.Client, error) { return registry.GetClient(), nil }, + Config: func() (config.Config, error) { + return &mockConfig{baseURL: "http://api.local", gatewayGroup: "gg1"}, nil + }, + ID: "r1", + GatewayGroup: "gg1", + Desc: "", + DescSet: true, + } + if err := actionRun(opts); err != nil { + t.Fatalf("actionRun failed: %v", err) + } + registry.Verify(t) +} diff --git a/test/e2e/route_test.go b/test/e2e/route_test.go index 851da54..83e4204 100644 --- a/test/e2e/route_test.go +++ b/test/e2e/route_test.go @@ -3,6 +3,7 @@ package e2e import ( + "encoding/json" "fmt" "io" "net/http" @@ -383,3 +384,45 @@ func TestRoute_TrafficForwarding(t *testing.T) { } assert.Equal(t, 200, status) } + +// TestRoute_DescFlagCRUD guards the regression where `route create` and +// `route update` exposed no `--desc` flag despite the README documenting one. +// Covers create-with-desc, update-with-new-desc, and update-with-empty-desc +// (which clears the field). +func TestRoute_DescFlagCRUD(t *testing.T) { + env := setupEnv(t) + svcID := "e2e-route-desc-svc" + routeID := "e2e-route-desc" + t.Cleanup(func() { + deleteRouteViaAdmin(t, routeID) + deleteServiceViaAdmin(t, svcID) + }) + createTestServiceViaCLI(t, env, svcID) + + stdout, stderr, err := runA7WithEnv(env, "route", "create", + "--name", routeID, "--path", "/"+routeID, "--service-id", svcID, + "--desc", "initial desc", "-g", gatewayGroup, "-o", "json") + require.NoError(t, err, "stdout=%s stderr=%s", stdout, stderr) + var created map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(stdout), &created)) + assert.Equal(t, "initial desc", created["desc"], "create --desc should land in response") + rtID, ok := created["id"].(string) + require.True(t, ok && rtID != "", "create response should contain an id: %v", created) + t.Cleanup(func() { deleteRouteViaAdmin(t, rtID) }) + + stdout, stderr, err = runA7WithEnv(env, "route", "update", rtID, + "--desc", "updated desc", "-g", gatewayGroup, "-o", "json") + require.NoError(t, err, "stdout=%s stderr=%s", stdout, stderr) + var updated map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(stdout), &updated)) + assert.Equal(t, "updated desc", updated["desc"], "update --desc should land in response") + + stdout, stderr, err = runA7WithEnv(env, "route", "update", rtID, + "--desc", "", "-g", gatewayGroup, "-o", "json") + require.NoError(t, err, "stdout=%s stderr=%s", stdout, stderr) + var cleared map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(stdout), &cleared)) + if d, present := cleared["desc"]; present { + assert.Equal(t, "", d, "passing --desc \"\" should clear the description") + } +} From 4080fcc24d1d0be2237a6cea010eb818ff58137d Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Sat, 23 May 2026 17:44:50 +0800 Subject: [PATCH 2/3] test(route): address PR #39 review on TestRoute_DescFlagCRUD - Drop the pre-registered name-based cleanup that referenced `routeID` (which is the route's name, not its id). The id-only cleanup registered after parsing `created["id"]` is the only correct one; the pre-reg was a no-op that masked leaks on early failures. - Treat absent, nil, and "" as a cleared desc via a small helper. API7 EE serializes a cleared field as `null`; the previous check produced a present-but-nil key and would have failed on a working CI environment. - Add a follow-up `route get` after the clear-update and assert the same on the persisted server state, not just the update response. Guards against the API echoing an empty desc without persisting it. --- test/e2e/route_test.go | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/test/e2e/route_test.go b/test/e2e/route_test.go index 83e4204..a87f771 100644 --- a/test/e2e/route_test.go +++ b/test/e2e/route_test.go @@ -393,10 +393,7 @@ func TestRoute_DescFlagCRUD(t *testing.T) { env := setupEnv(t) svcID := "e2e-route-desc-svc" routeID := "e2e-route-desc" - t.Cleanup(func() { - deleteRouteViaAdmin(t, routeID) - deleteServiceViaAdmin(t, svcID) - }) + t.Cleanup(func() { deleteServiceViaAdmin(t, svcID) }) createTestServiceViaCLI(t, env, svcID) stdout, stderr, err := runA7WithEnv(env, "route", "create", @@ -422,7 +419,22 @@ func TestRoute_DescFlagCRUD(t *testing.T) { require.NoError(t, err, "stdout=%s stderr=%s", stdout, stderr) var cleared map[string]interface{} require.NoError(t, json.Unmarshal([]byte(stdout), &cleared)) - if d, present := cleared["desc"]; present { - assert.Equal(t, "", d, "passing --desc \"\" should clear the description") + assertDescCleared(t, cleared, "update --desc \"\" should clear the description in response") + + // Verify the clear persisted server-side, not just in the update response. + var persisted map[string]interface{} + runA7JSON(t, env, &persisted, "route", "get", rtID, "-g", gatewayGroup, "-o", "json") + assertDescCleared(t, persisted, "cleared desc should not be persisted") +} + +// assertDescCleared treats absent, nil, and "" all as a successfully-cleared +// desc — the API may serialize a cleared field as `null`, omit it entirely +// (json `omitempty`), or echo an empty string. +func assertDescCleared(t testTB, obj map[string]interface{}, msg string) { + t.Helper() + d, present := obj["desc"] + if !present || d == nil { + return } + assert.Equal(t, "", d, msg) } From 7c7f3c5dd3d0bb7b85c7f9893d7123167554387a Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Sat, 23 May 2026 18:18:01 +0800 Subject: [PATCH 3/3] test(e2e): resolve gateway group by name, skip ingress in fallback The previous resolver picked GET /api/gateway_groups list[0] and ignored both the env var and the group type. That made CI silently dependent on remote-server ordering: when the dev environment's first-returned group became an api7_ingress_controller-typed one, every mutating test failed 13 minutes in with a cryptic 'permission denied' from the CP middleware (`gateway_group.go:24`: ingress groups reject writes for any auth mode other than admin_key). - Treat A7_GATEWAY_GROUP=default as a literal name lookup, not a sentinel; honor any explicit name and fail fast at startup with the list of available names if no match. - Fall back path (empty name) now decodes Type and skips api7_ingress_controller groups instead of taking list[0]. - Startup log now prints the resolved name -> id mapping so the next environment drift is diagnosable from the first log line. --- test/e2e/setup_test.go | 52 ++++++++++++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 12 deletions(-) diff --git a/test/e2e/setup_test.go b/test/e2e/setup_test.go index 35842e1..9f9c7ba 100644 --- a/test/e2e/setup_test.go +++ b/test/e2e/setup_test.go @@ -134,17 +134,20 @@ func TestMain(m *testing.M) { os.Exit(1) } - // Resolve the actual gateway group UUID. API7 EE uses UUID-style IDs, - // not human-readable names like "default". - if gatewayGroup == "default" || gatewayGroup == "" { - ggID, err := resolveFirstGatewayGroupID() - if err != nil { - fmt.Fprintf(os.Stderr, "failed to resolve gateway group ID: %v\n", err) - os.Exit(1) - } - gatewayGroup = ggID - fmt.Fprintf(os.Stderr, "Resolved gateway group ID: %s\n", gatewayGroup) + // API7 EE uses UUID-style ids for runtime API calls but env vars carry + // names; resolve name -> id. An explicit name is honored literally; only + // the empty/"default" case falls back to "first writable group". + wanted := gatewayGroup + if wanted == "default" || wanted == "" { + wanted = "default" + } + ggID, err := resolveGatewayGroupID(wanted) + if err != nil { + fmt.Fprintf(os.Stderr, "failed to resolve gateway group: %v\n", err) + os.Exit(1) } + gatewayGroup = ggID + fmt.Fprintf(os.Stderr, "Resolved gateway group %q -> %s\n", wanted, gatewayGroup) os.Exit(m.Run()) } @@ -403,7 +406,12 @@ func createTestRouteWithServiceViaCLI(t testTB, env []string, routeID, serviceID require.NoError(t, err, "stdout=%s stderr=%s", stdout, stderr) } -func resolveFirstGatewayGroupID() (string, error) { +// resolveGatewayGroupID looks up a gateway group by exact name; if wanted is +// empty it falls back to the first non-ingress group. Ingress-controller +// gateway groups (Type == "api7_ingress_controller") reject POST/PUT/PATCH/ +// DELETE for any auth mode other than admin_key, so picking one as the test +// target silently breaks every mutating test in CI. +func resolveGatewayGroupID(wanted string) (string, error) { req, err := http.NewRequest(http.MethodGet, adminURL+"/api/gateway_groups", nil) if err != nil { return "", err @@ -426,6 +434,7 @@ func resolveFirstGatewayGroupID() (string, error) { List []struct { ID string `json:"id"` Name string `json:"name"` + Type string `json:"type"` } `json:"list"` } if err := json.Unmarshal(body, &result); err != nil { @@ -434,5 +443,24 @@ func resolveFirstGatewayGroupID() (string, error) { if len(result.List) == 0 { return "", fmt.Errorf("no gateway groups found") } - return result.List[0].ID, nil + + if wanted != "" { + for _, g := range result.List { + if g.Name == wanted { + return g.ID, nil + } + } + names := make([]string, 0, len(result.List)) + for _, g := range result.List { + names = append(names, g.Name) + } + return "", fmt.Errorf("gateway group %q not found; available: %v", wanted, names) + } + + for _, g := range result.List { + if g.Type != "api7_ingress_controller" { + return g.ID, nil + } + } + return "", fmt.Errorf("no non-ingress gateway group found; ingress-controller groups reject writes without admin_key auth") }