From ce033b6e161c5a2da50f26bd888171d79226ddc3 Mon Sep 17 00:00:00 2001 From: ProbstenHias Date: Sat, 12 Sep 2026 13:36:04 +0200 Subject: [PATCH 1/5] feat(network): add address create, update, delete and reserve verbs Drive the legacy ipam/address client the same way network prefix does: flags as payload, identifier validation before any request, confirmation on delete and the same error shape as the registry-driven nouns. update reads the address first and carries the current rDNS name into the PUT, because go-anxcloud sends rdns_name without omitempty and an omitted flag would otherwise clear it. reserve is the leaf action verb docs/cli-design.md already allows; it takes --reservation-period as a duration and renders the reserved addresses as a list. --- README.md | 14 +- docs/cli-design.md | 7 +- internal/cli/conformance_test.go | 7 + internal/cli/network_address.go | 345 ++++++++++++++++++++++++++++++- internal/cli/network_test.go | 195 ++++++++++++++++- 5 files changed, 545 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index 795b91a..345eb72 100644 --- a/README.md +++ b/README.md @@ -79,6 +79,10 @@ anexia network vlan list --location --status Active anexia network vlan create --location --description "lab" --vm-provisioning anexia network vlan update --description "lab (retired)" --vm-provisioning=false anexia network address list --prefix --version 4 +anexia network address create --prefix --address 192.0.2.10 --description "web" --role Default --rdns web.example.com +anexia network address update --description "web (retired)" --role Default --rdns old-web.example.com +anexia network address delete --yes +anexia network address reserve --location --vlan --count 2 --prefix --reservation-period 30m ``` Boolean payload flags such as `--vm-provisioning` are switched off on `update` with an explicit @@ -209,8 +213,8 @@ implemented one. The distinction matters to whoever picks the work up, so the ta when the library says which, but a `-` is never evidence about the Engine on its own. The `core`, `network`, `dns` and `kubernetes` groups below are implemented. Within `network`, `vlan` has every -verb because go-anxcloud models it generically; `prefix` has every verb hand-written against the -older client, and `address` is read only for now because its write verbs are still to be declared. +verb because go-anxcloud models it generically; `prefix` and `address` have every verb hand-written against the +older client. The remaining groups, starting with `vsphere`, are roadmap items read off go-anxcloud v0.14.5 and not verified against the Engine. @@ -229,7 +233,7 @@ not verified against the Engine. | --- | :-: | :-: | :-: | :-: | :-: | --- | | `network vlan` | [x] | [x] | [x] | [x] | [x] | `--status` and `--location` filters [x]; a VLAN's location is fixed at creation, so `update` has no `--location` | | `network prefix` | [x] | [x] | [x] | [x] | [x] | `--search` [x]; a prefix's name is its Engine-assigned CIDR, so `update` offers `--description` only | -| `network address` | [x] | [x] | [ ] | [ ] | [ ] | `--search` [x]; field filters [x]; `reserve` [ ] | +| `network address` | [x] | [x] | [x] | [x] | [x] | `--search` [x]; field filters [x]; `reserve` [x] | ### vsphere @@ -326,8 +330,8 @@ handle updates differently from the other four, so their write verbs may not all The write verbs landed with the `dns` group, the first resources the Engine lets the CLI write through the registry, and `network vlan` followed the same way. Most other resources reachable today -are read-only in the Engine anyway. `core tag` and `network prefix` drive the legacy client directly; -addresses are writable in the library and their write verbs are still to be declared. +are read-only in the Engine anyway. `core tag`, `network prefix` and `network address` drive the legacy +client directly. ## Development diff --git a/docs/cli-design.md b/docs/cli-design.md index 32d44ab..aa034ca 100644 --- a/docs/cli-design.md +++ b/docs/cli-design.md @@ -64,8 +64,8 @@ A resource that is only reachable inside another, such as a DNS record inside it `Scope` hook. Scope flags are required, and unlike list filters they apply to every verb, because without one there is no collection to address at all. -Some resources the CLI already reaches are writable in the Engine and do not offer the verbs yet, -`network address` among them. `network prefix` shows how they get declared: hand-written against +Some resources the CLI will reach are writable in the Engine before the CLI offers the verbs. +`network prefix` and `network address` show how they get declared: hand-written against the legacy client, but with the same verbs, flags-as-payload, confirmation and error shape as the registry, so a user cannot tell which half of the CLI served the command. @@ -84,7 +84,8 @@ create a tag object, and `anexia core resource tag add` does not. Four operations have no honest CRUD spelling and are allowed as leaf verbs: `network address reserve`, `dns zone import`, `dns zone apply`, and -`storage bucket empty-and-delete`. The two DNS ones ship; the others are planned. +`storage bucket empty-and-delete`. The DNS operations and `network address reserve` ship; +`storage bucket empty-and-delete` is planned. `import` and `apply` take a document via `--file` rather than payload flags, because the Engine accepts a BIND zone file and a JSON changeset respectively, and spelling either out as repeated flags would mean inventing a small language diff --git a/internal/cli/conformance_test.go b/internal/cli/conformance_test.go index 29e3cb1..341afda 100644 --- a/internal/cli/conformance_test.go +++ b/internal/cli/conformance_test.go @@ -120,6 +120,10 @@ var engineActions = map[string]string{ "anexia network prefix delete": `deleting prefix "placeholder"`, "anexia network address list": "listing addresses", "anexia network address get": `reading address "placeholder"`, + "anexia network address create": "creating address", + "anexia network address update": `reading address "placeholder"`, + "anexia network address delete": `deleting address "placeholder"`, + "anexia network address reserve": "reserving addresses", "anexia dns zone list": "listing zones", "anexia dns zone get": `reading zone "placeholder"`, "anexia dns zone create": `creating zone "placeholder"`, @@ -257,6 +261,7 @@ func TestConformanceLeafAliasesUseKnownVerbs(t *testing.T) { "anexia core tag delete": {"destroy": true}, "anexia network vlan delete": {"destroy": true}, "anexia network prefix delete": {"destroy": true}, + "anexia network address delete": {"destroy": true}, "anexia dns zone delete": {"destroy": true}, "anexia dns record delete": {"destroy": true}, "anexia kubernetes cluster delete": {"destroy": true}, @@ -426,6 +431,8 @@ var invocationFlags = map[string]string{ "admin-email": "admin@example.com", "location": "placeholder", "description": "placeholder", + "prefix": "placeholder", + "address": "10.0.0.1", "version": "4", "netmask": "24", "vlan": "placeholder", diff --git a/internal/cli/network_address.go b/internal/cli/network_address.go index e2a9e46..6147871 100644 --- a/internal/cli/network_address.go +++ b/internal/cli/network_address.go @@ -2,14 +2,18 @@ package cli import ( "fmt" + "math" "strconv" + "time" "github.com/spf13/cobra" "github.com/spf13/pflag" "go.anx.io/go-anxcloud/pkg/ipam/address" "go.anx.io/go-anxcloud/pkg/utils/param" + "github.com/ProbstenHias/anexia-cli/internal/confirm" "github.com/ProbstenHias/anexia-cli/internal/errmap" + "github.com/ProbstenHias/anexia-cli/internal/output" "github.com/ProbstenHias/anexia-cli/internal/resource" ) @@ -17,16 +21,33 @@ import ( // in go-anxcloud's generic pkg/apis tree, so these commands drive the legacy // ipam/address client directly, sharing the paging and rendering helpers. // -// The library implements create, update, delete and ReserveRandom here as -// well. They are left for the change that gives the resource registry its -// write verbs, so the two halves of the CLI keep offering the same verbs. +// The write verbs drive the same legacy client, using its Create, Update, and +// ReserveRandom types as the Engine payload contract. func newNetworkAddressCommand(opts *globalOptions) *cobra.Command { return resource.Noun("address", "addresses", "Work with Anexia IP addresses", newNetworkAddressListCommand(opts), newNetworkAddressGetCommand(opts), + newNetworkAddressCreateCommand(opts), + newNetworkAddressUpdateCommand(opts), + newNetworkAddressDeleteCommand(opts), + newNetworkAddressReserveCommand(opts), ) } +var addressColumns = []string{"identifier", "name", "role", "description"} + +func addressRow(s *address.Summary) []string { + return []string{s.ID, s.Name, s.Role, s.DescriptionCustomer} +} + +func renderAddressSummary(w *output.Writer, s *address.Summary) error { + if w.Format().Structured() { + return w.Object(s) + } + + return w.Table(addressColumns, [][]string{addressRow(s)}) +} + // addressFilters registers the fields the Engine's filtered address endpoint // accepts and returns the ones the user set. Names follow the field rather // than the query parameter, so --role sets role_text and --organization sets @@ -163,14 +184,320 @@ func newNetworkAddressListCommand(opts *globalOptions) *cobra.Command { return opts.Fail(fmt.Errorf("listing addresses: %w", err)) } - return resource.RenderList(cmd, w, "addresses", found, - []string{"identifier", "name", "role", "description"}, - func(s *address.Summary) []string { - return []string{s.ID, s.Name, s.Role, s.DescriptionCustomer} - }, - ) + return resource.RenderList(cmd, w, "addresses", found, addressColumns, addressRow) + } + + return cmd +} + +type addressCreateFlags struct { + prefix string + address string + description string + role string + organization string + rdns string +} + +func (f *addressCreateFlags) register(flags *pflag.FlagSet) { + flags.StringVar(&f.prefix, "prefix", "", "prefix identifier the address belongs to") + flags.StringVar(&f.address, "address", "", "IP address to create") + flags.StringVar(&f.description, "description", "", "customer description") + flags.StringVar(&f.role, "role", "Default", "address role") + flags.StringVar(&f.organization, "organization", "", "organization identifier the address belongs to") + flags.StringVar(&f.rdns, "rdns", "", "reverse DNS name") +} + +func (f *addressCreateFlags) payload() (address.Create, error) { + if f.prefix == "" { + return address.Create{}, errmap.Usagef("--prefix is required") + } + + if err := resource.ValidateIdentifier("prefix", f.prefix); err != nil { + return address.Create{}, err + } + + if f.address == "" { + return address.Create{}, errmap.Usagef("--address is required") + } + + if f.organization != "" { + if err := resource.ValidateIdentifier("organization", f.organization); err != nil { + return address.Create{}, err + } + } + + return address.Create{ + PrefixID: f.prefix, + Address: f.address, + DescriptionCustomer: f.description, + Role: f.role, + Organization: f.organization, + RDNSName: f.rdns, + }, nil +} + +func newNetworkAddressCreateCommand(opts *globalOptions) *cobra.Command { + var f addressCreateFlags + + cmd := &cobra.Command{ + Use: "create", + Short: "Create an address", + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + body, err := f.payload() + if err != nil { + return err + } + + w, err := opts.Writer(cmd.OutOrStdout()) + if err != nil { + return err + } + + c, err := opts.Client(cmd.Flags()) + if err != nil { + return err + } + + ctx, cancel := opts.Context(cmd.Context()) + defer cancel() + + created, err := address.NewAPI(c).Create(ctx, body) + if err != nil { + return opts.Fail(fmt.Errorf("creating address: %w", err)) + } + + return renderAddressSummary(w, &created) + }, + } + + f.register(cmd.Flags()) + + return cmd +} + +func newNetworkAddressUpdateCommand(opts *globalOptions) *cobra.Command { + var description, role, rdns string + + cmd := &cobra.Command{ + Use: "update ", + Short: "Update an address", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + if err := resource.ValidateIdentifier("address", args[0]); err != nil { + return err + } + + flags := cmd.Flags() + if !flags.Changed("description") && !flags.Changed("role") && !flags.Changed("rdns") { + return errmap.Usagef("nothing to update: pass at least one field to change") + } + + if flags.Changed("description") && description == "" { + return errmap.Usagef("--description cannot be emptied: go-anxcloud drops an empty description from the request") + } + + if flags.Changed("role") && role == "" { + return errmap.Usagef("--role cannot be emptied: go-anxcloud drops an empty role from the request") + } + + w, err := opts.Writer(cmd.OutOrStdout()) + if err != nil { + return err + } + + c, err := opts.Client(cmd.Flags()) + if err != nil { + return err + } + + ctx, cancel := opts.Context(cmd.Context()) + defer cancel() + + a := address.NewAPI(c) + current, err := a.Get(ctx, pathValue(args[0])) + if err != nil { + return opts.Fail(fmt.Errorf("reading address %q: %w", args[0], err)) + } + + body := address.Update{RDNSName: current.RDNSName} + if flags.Changed("description") { + body.DescriptionCustomer = description + } + if flags.Changed("role") { + body.Role = role + } + if flags.Changed("rdns") { + body.RDNSName = rdns + } + + updated, err := a.Update(ctx, pathValue(args[0]), body) + if err != nil { + return opts.Fail(fmt.Errorf("updating address %q: %w", args[0], err)) + } + + return renderAddressSummary(w, &updated) + }, + } + + cmd.Flags().StringVar(&description, "description", "", "customer description") + cmd.Flags().StringVar(&role, "role", "", "address role") + cmd.Flags().StringVar(&rdns, "rdns", "", "reverse DNS name") + + return cmd +} + +func newNetworkAddressDeleteCommand(opts *globalOptions) *cobra.Command { + return &cobra.Command{ + Use: "delete ", + Aliases: []string{"destroy"}, + Short: "Delete an address", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + if err := resource.ValidateIdentifier("address", args[0]); err != nil { + return err + } + + if err := confirm.Prompt(cmd.InOrStdin(), cmd.ErrOrStderr(), fmt.Sprintf("delete address %q", args[0]), opts.AssumeYes()); err != nil { + return err + } + + c, err := opts.Client(cmd.Flags()) + if err != nil { + return err + } + + ctx, cancel := opts.Context(cmd.Context()) + defer cancel() + + if err := address.NewAPI(c).Delete(ctx, pathValue(args[0])); err != nil { + return opts.Fail(fmt.Errorf("deleting address %q: %w", args[0], err)) + } + + _, err = fmt.Fprintf(cmd.ErrOrStderr(), "deleted address %s\n", args[0]) + + return err + }, + } +} + +type addressReserveFlags struct { + location string + vlan string + count int + prefix string + version int + reservationPeriod time.Duration +} + +func (f *addressReserveFlags) register(flags *pflag.FlagSet) { + flags.StringVar(&f.location, "location", "", "location identifier to reserve addresses in") + flags.StringVar(&f.vlan, "vlan", "", "VLAN identifier to reserve addresses in") + flags.IntVar(&f.count, "count", 1, "number of addresses to reserve") + flags.StringVar(&f.prefix, "prefix", "", "prefix identifier to reserve addresses from") + flags.IntVar(&f.version, "version", 0, "IP version to reserve, 4 or 6") + flags.DurationVar(&f.reservationPeriod, "reservation-period", 0, "how long to reserve addresses") +} + +func (f *addressReserveFlags) payload(flags *pflag.FlagSet) (address.ReserveRandom, error) { + if f.location == "" { + return address.ReserveRandom{}, errmap.Usagef("--location is required") + } + + if err := resource.ValidateIdentifier("location", f.location); err != nil { + return address.ReserveRandom{}, err + } + + if f.vlan == "" { + return address.ReserveRandom{}, errmap.Usagef("--vlan is required") + } + + if err := resource.ValidateIdentifier("vlan", f.vlan); err != nil { + return address.ReserveRandom{}, err + } + + if f.count < 1 { + return address.ReserveRandom{}, errmap.Usagef("--count must be at least 1") + } + + if flags.Changed("version") && f.version != 4 && f.version != 6 { + return address.ReserveRandom{}, errmap.Usagef("--version %d must be 4 or 6", f.version) + } + + if f.prefix != "" { + if err := resource.ValidateIdentifier("prefix", f.prefix); err != nil { + return address.ReserveRandom{}, err + } + } + + seconds := f.reservationPeriod / time.Second + if flags.Changed("reservation-period") && seconds <= 0 { + return address.ReserveRandom{}, errmap.Usagef("--reservation-period must be positive") + } + if strconv.IntSize == 32 && seconds > math.MaxUint32 { + return address.ReserveRandom{}, errmap.Usagef("--reservation-period is too large") } + // #nosec G115 -- seconds is positive and bounded to MaxUint32 on 32-bit platforms. + period := uint(seconds) + + return address.ReserveRandom{ + LocationID: f.location, + VlanID: f.vlan, + Count: f.count, + PrefixID: f.prefix, + IPVersion: address.IPReserveVersionLimit(f.version), + ReservationPeriod: period, + }, nil +} + +func newNetworkAddressReserveCommand(opts *globalOptions) *cobra.Command { + var f addressReserveFlags + + cmd := &cobra.Command{ + Use: "reserve", + Short: "Reserve random addresses", + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + body, err := f.payload(cmd.Flags()) + if err != nil { + return err + } + + w, err := opts.Writer(cmd.OutOrStdout()) + if err != nil { + return err + } + + c, err := opts.Client(cmd.Flags()) + if err != nil { + return err + } + + ctx, cancel := opts.Context(cmd.Context()) + defer cancel() + + reserved, err := address.NewAPI(c).ReserveRandom(ctx, body) + if err != nil { + return opts.Fail(fmt.Errorf("reserving addresses: %w", err)) + } + + if w.Format().Structured() { + return w.Object(reserved) + } + + return resource.RenderList(cmd, w, "reserved addresses", reserved.Data, + []string{"identifier", "address", "prefix"}, + func(ip *address.ReservedIP) []string { + return []string{ip.ID, ip.Address, ip.Prefix} + }, + ) + }, + } + + f.register(cmd.Flags()) + return cmd } diff --git a/internal/cli/network_test.go b/internal/cli/network_test.go index 7ef7ef1..581d487 100644 --- a/internal/cli/network_test.go +++ b/internal/cli/network_test.go @@ -51,16 +51,13 @@ func TestNetworkWithoutSubcommandPrintsHelp(t *testing.T) { require.Contains(t, stdout, "address") } -func TestNetworkAddressOnlyHasReadVerbs(t *testing.T) { +func TestNetworkAddressHasEveryVerb(t *testing.T) { isolate(t) stdout, _, err := run(t, "network", "address") require.NoError(t, err) - require.Contains(t, stdout, "list") - require.Contains(t, stdout, "get") - for _, absent := range []string{"create", "update", "delete", "destroy", "reserve"} { - require.NotContains(t, stdout, absent, - "address write verbs are still to be declared") + for _, verb := range []string{"list", "get", "create", "update", "delete", "reserve"} { + require.Contains(t, stdout, verb) } } @@ -1232,6 +1229,192 @@ func TestNetworkAddressesPluralAlias(t *testing.T) { require.Contains(t, stdout, "10.0.0.1") } +const oneAddress = `{"identifier":"a-1","name":"10.0.0.1","role_text":"Default","description_customer":"gateway","rdns_name":"old.example.com"}` + +func TestNetworkAddressCreateSendsTheLegacyCreateBody(t *testing.T) { + isolate(t) + + var sent []request + srv := recordingServer(t, &sent, oneAddress) + + stdout, _, err := run(t, "network", "address", "create", + "--prefix", "p-1", "--address", "10.0.0.1", "--description", "gateway", + "--organization", "o-1", "--rdns", "host.example.com", + "--token", "tok", "--api-base-url", srv) + require.NoError(t, err) + require.Len(t, sent, 1) + require.Equal(t, http.MethodPost, sent[0].method) + require.Equal(t, "/api/ipam/v1/address.json", sent[0].path) + + var body map[string]any + require.NoError(t, json.Unmarshal([]byte(sent[0].body), &body)) + require.Equal(t, map[string]any{ + "prefix": "p-1", + "name": "10.0.0.1", + "description_customer": "gateway", + "role": "Default", + "organization": "o-1", + "rdns_name": "host.example.com", + }, body) + require.Contains(t, stdout, "10.0.0.1") +} + +func TestNetworkAddressCreateRejectsBadFlags(t *testing.T) { + tests := []struct { + name string + args []string + want string + }{ + {name: "missing prefix", args: []string{"--address", "10.0.0.1"}, want: "--prefix is required"}, + {name: "missing address", args: []string{"--prefix", "p-1"}, want: "--address is required"}, + {name: "invalid prefix", args: []string{"--prefix", "p/1", "--address", "10.0.0.1"}, want: `prefix "p/1" does not name a prefix`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + isolate(t) + var sent []request + srv := recordingServer(t, &sent, oneAddress) + + args := append([]string{"network", "address", "create"}, tt.args...) + args = append(args, "--token", "tok", "--api-base-url", srv) + _, _, err := run(t, args...) + require.Error(t, err) + require.Equal(t, errmap.ExitUsage, errmap.ExitCode(err)) + require.Contains(t, errmap.Message(err), tt.want) + require.Empty(t, sent) + }) + } +} + +func TestNetworkAddressUpdateKeepsRDNSAndRendersTheWrite(t *testing.T) { + isolate(t) + + var sent []request + updated := strings.Replace(oneAddress, `"gateway"`, `"lab"`, 1) + srv := recordingServer(t, &sent, oneAddress, updated) + + stdout, _, err := run(t, "network", "address", "update", "a-1", "--description", "lab", "-o", "json", + "--token", "tok", "--api-base-url", srv) + require.NoError(t, err) + require.Len(t, sent, 2) + require.Equal(t, http.MethodGet, sent[0].method) + require.Equal(t, http.MethodPut, sent[1].method) + + var body map[string]any + require.NoError(t, json.Unmarshal([]byte(sent[1].body), &body)) + require.Equal(t, map[string]any{ + "description_customer": "lab", + "rdns_name": "old.example.com", + }, body) + + var shown map[string]any + require.NoError(t, json.Unmarshal([]byte(stdout), &shown)) + require.Equal(t, "lab", shown["description_customer"]) +} + +func TestNetworkAddressUpdateRejectsInvalidChanges(t *testing.T) { + tests := []struct { + name string + args []string + want string + }{ + {name: "nothing changed", want: "nothing to update"}, + {name: "empty description", args: []string{"--description", ""}, want: "--description cannot be emptied"}, + {name: "empty role", args: []string{"--role", ""}, want: "--role cannot be emptied"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + isolate(t) + var sent []request + srv := recordingServer(t, &sent, oneAddress) + + args := append([]string{"network", "address", "update", "a-1"}, tt.args...) + args = append(args, "--token", "tok", "--api-base-url", srv) + _, _, err := run(t, args...) + require.Error(t, err) + require.Equal(t, errmap.ExitUsage, errmap.ExitCode(err)) + require.Contains(t, errmap.Message(err), tt.want) + require.Empty(t, sent) + }) + } +} + +func TestNetworkAddressDeleteConfirms(t *testing.T) { + isolate(t) + + var sent []request + srv := recordingServer(t, &sent, `{}`) + + stdout, stderr, err := runWithInput(t, "y\n", "network", "address", "delete", "a-1", + "--token", "tok", "--api-base-url", srv) + require.NoError(t, err) + require.Empty(t, stdout) + require.Len(t, sent, 1) + require.Equal(t, http.MethodDelete, sent[0].method) + require.Contains(t, stderr, `delete address "a-1"`) + require.Contains(t, stderr, "deleted address a-1") +} + +func TestNetworkAddressReserveSendsThePayloadAndRendersTheData(t *testing.T) { + isolate(t) + + var sent []request + srv := recordingServer(t, &sent, `{"limit":1,"page":1,"total_items":1,"total_pages":1,"data":[{"identifier":"a-9","text":"10.0.0.9","prefix":"10.0.0.0/24"}]}`) + + stdout, _, err := run(t, "network", "address", "reserve", "--location", "l-1", "--vlan", "v-1", + "--count", "2", "--prefix", "p-1", "--version", "6", "--reservation-period", "30m", + "--token", "tok", "--api-base-url", srv) + require.NoError(t, err) + require.Len(t, sent, 1) + require.Equal(t, http.MethodPost, sent[0].method) + require.Equal(t, "/api/ipam/v1/address/reserve/ip/count.json", sent[0].path) + + var body map[string]any + require.NoError(t, json.Unmarshal([]byte(sent[0].body), &body)) + require.Equal(t, map[string]any{ + "location_identifier": "l-1", + "vlan_identifier": "v-1", + "count": float64(2), + "prefix_identifier": "p-1", + "ip_version": float64(6), + "reservation_period": float64(1800), + }, body) + require.Contains(t, stdout, "a-9") + require.Contains(t, stdout, "10.0.0.9") +} + +func TestNetworkAddressReserveRejectsBadFlags(t *testing.T) { + tests := []struct { + name string + args []string + want string + }{ + {name: "missing location", args: []string{"--vlan", "v-1"}, want: "--location is required"}, + {name: "missing vlan", args: []string{"--location", "l-1"}, want: "--vlan is required"}, + {name: "zero count", args: []string{"--location", "l-1", "--vlan", "v-1", "--count", "0"}, want: "--count must be at least 1"}, + {name: "invalid version", args: []string{"--location", "l-1", "--vlan", "v-1", "--version", "5"}, want: "--version 5 must be 4 or 6"}, + {name: "zero reservation period", args: []string{"--location", "l-1", "--vlan", "v-1", "--reservation-period", "0s"}, want: "--reservation-period must be positive"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + isolate(t) + var sent []request + srv := recordingServer(t, &sent, `{}`) + + args := append([]string{"network", "address", "reserve"}, tt.args...) + args = append(args, "--token", "tok", "--api-base-url", srv) + _, _, err := run(t, args...) + require.Error(t, err) + require.Equal(t, errmap.ExitUsage, errmap.ExitCode(err)) + require.Contains(t, errmap.Message(err), tt.want) + require.Empty(t, sent) + }) + } +} + // TestNetworkListAllCarriesFiltersAcrossPages pins that the parameters that // pick what is listed survive a multi-page walk. Both nouns build their // request per page, so a filter left out of the second request would silently From 652a5554fa8520dc5b7fe79040f0bb3ae2cfc1e6 Mon Sep 17 00:00:00 2001 From: ProbstenHias Date: Sat, 12 Sep 2026 13:52:08 +0200 Subject: [PATCH 2/5] fix(network): tighten address reserve validation and document update semantics Reject an empty reservation result instead of rendering success, require --reservation-period to be at least 1s because the Engine takes whole seconds, and drop the unreachable 32-bit guard. Pin the rDNS carry-over, identifier guards, delete paths and minimal reserve payload in tests, and record the address update and summary behaviour in the design doc. --- docs/cli-design.md | 19 ++- internal/cli/network.go | 4 +- internal/cli/network_address.go | 24 ++-- internal/cli/network_test.go | 234 +++++++++++++++++++++++++++++++- 4 files changed, 258 insertions(+), 23 deletions(-) diff --git a/docs/cli-design.md b/docs/cli-design.md index aa034ca..35e9fd6 100644 --- a/docs/cli-design.md +++ b/docs/cli-design.md @@ -47,8 +47,8 @@ verb. | `list` | none | `List` | Paged. Always available if the Engine can enumerate the resource. | | `get` | `` | `Get` | One object by identifier. | | `create` | none, flags carry the payload | `Create` | | -| `update` | ``, flags carry the changes | Read then `Update` | The read is `Get`, or `List` where the Engine has no single-object read (`dns record`). `network prefix` sends a sparse `Update` without the read, see below. | -| `delete` | `` | `Destroy` | Confirms first. Aliased to `destroy`, which is never a command name. The legacy clients (`core tag`, `network prefix`) call it `Delete`. | +| `update` | ``, flags carry the changes | Read then `Update` | The read is `Get`, or `List` where the Engine has no single-object read (`dns record`). `network prefix` sends a sparse `Update` without the read; `network address` reads only to retain rDNS, see below. | +| `delete` | `` | `Destroy` | Confirms first. Aliased to `destroy`, which is never a command name. The legacy clients (`core tag`, `network prefix`, `network address`) call it `Delete`. | A resource only gets the verbs the Engine actually supports. `core location` is read-only in the Engine, so it exposes `list` and `get` and nothing else. This is deliberate: a `create` that @@ -86,6 +86,9 @@ Four operations have no honest CRUD spelling and are allowed as leaf verbs: `network address reserve`, `dns zone import`, `dns zone apply`, and `storage bucket empty-and-delete`. The DNS operations and `network address reserve` ship; `storage bucket empty-and-delete` is planned. +`network address reserve` takes `--reservation-period` as a duration, sends it to the Engine as +whole seconds, and requires at least 1s when passed. Without it, the Engine applies its 30m default. +An address reservation that returns no addresses is an error, not an empty table. `import` and `apply` take a document via `--file` rather than payload flags, because the Engine accepts a BIND zone file and a JSON changeset respectively, and spelling either out as repeated flags would mean inventing a small language @@ -187,6 +190,11 @@ field and the Engine keeps the rest. It offers `--description` only: the legacy `name`, but a prefix's name is its CIDR, assigned by the Engine, so it is not offered for the same reason `dns zone update` has no `--name`. `--description ""` is refused for the same reason `vlan` does. +`network address update` reads the address only to retain `rdns_name`, the one field in +go-anxcloud's `address.Update` without `omitempty`. `description_customer` and `role` are sparse, +so `--description ""` and `--role ""` are refused with the same "cannot be emptied" wording as +prefix; `--rdns ""` reaches the Engine and clears the reverse DNS name. + A field the Engine cannot change safely does not get a flag. `dns zone update` has no `--name`, because the Engine's zone update carries the name only in the request body with no old name anywhere in the request, so what a changed name does is undefined. Renaming is not offered rather @@ -219,9 +227,10 @@ Four formats, one flag. `table` is the default and is meant for humans: aligned columns, uppercase headers, no borders. Column sets are short on purpose, up to five fields, because a table wider than a terminal is -useless. Fewer when the Engine returns less: a prefix write is answered with the list summary, -so `network prefix create` and `update` show its three fields, and the full object is a -`network prefix get -o json` away. Everywhere else the full object is one `-o json` away. +useless. Fewer when the Engine returns less: prefix and address writes are answered with their list +summaries, so `network prefix create`, `network address create` and their `update` verbs show the +summary fields only, and the full object is a `get -o json` away. Everywhere else the full +object is one `-o json` away. `tsv` is `table` without the alignment: raw values, lowercase headers, tab-separated. This is the one to pipe into `cut` and `awk`. diff --git a/internal/cli/network.go b/internal/cli/network.go index e4d0737..f1299fa 100644 --- a/internal/cli/network.go +++ b/internal/cli/network.go @@ -23,8 +23,8 @@ func newNetworkCommand(opts *globalOptions) *cobra.Command { // newNetworkVlanCommand builds "network vlan". VLANs are the one object in // this group go-anxcloud models generically, so this is a Spec with every verb. -// prefix is hand-written against the legacy client with the same verbs, and -// address is read-only until its write verbs are declared. +// prefix and address are hand-written against legacy clients with their +// supported verbs, including address reservation. func newNetworkVlanCommand(opts *globalOptions) *cobra.Command { return resource.Command(opts, resource.Spec[vlanv1.VLAN, *vlanv1.VLAN]{ Noun: "vlan", diff --git a/internal/cli/network_address.go b/internal/cli/network_address.go index 6147871..479ff4e 100644 --- a/internal/cli/network_address.go +++ b/internal/cli/network_address.go @@ -1,8 +1,8 @@ package cli import ( + "errors" "fmt" - "math" "strconv" "time" @@ -277,6 +277,10 @@ func newNetworkAddressCreateCommand(opts *globalOptions) *cobra.Command { return cmd } +// newNetworkAddressUpdateCommand reads the address first only to carry +// rdns_name, the one address.Update field without omitempty, so an unset +// --rdns does not wipe reverse DNS. Description and role are sparse fields, so +// emptying them is refused while --rdns "" clears the reverse DNS name. func newNetworkAddressUpdateCommand(opts *globalOptions) *cobra.Command { var description, role, rdns string @@ -343,7 +347,7 @@ func newNetworkAddressUpdateCommand(opts *globalOptions) *cobra.Command { cmd.Flags().StringVar(&description, "description", "", "customer description") cmd.Flags().StringVar(&role, "role", "", "address role") - cmd.Flags().StringVar(&rdns, "rdns", "", "reverse DNS name") + cmd.Flags().StringVar(&rdns, "rdns", "", "reverse DNS name, empty clears it") return cmd } @@ -397,7 +401,7 @@ func (f *addressReserveFlags) register(flags *pflag.FlagSet) { flags.IntVar(&f.count, "count", 1, "number of addresses to reserve") flags.StringVar(&f.prefix, "prefix", "", "prefix identifier to reserve addresses from") flags.IntVar(&f.version, "version", 0, "IP version to reserve, 4 or 6") - flags.DurationVar(&f.reservationPeriod, "reservation-period", 0, "how long to reserve addresses") + flags.DurationVar(&f.reservationPeriod, "reservation-period", 0, "how long to reserve the addresses (the Engine defaults to 30m)") } func (f *addressReserveFlags) payload(flags *pflag.FlagSet) (address.ReserveRandom, error) { @@ -431,15 +435,12 @@ func (f *addressReserveFlags) payload(flags *pflag.FlagSet) (address.ReserveRand } } - seconds := f.reservationPeriod / time.Second - if flags.Changed("reservation-period") && seconds <= 0 { - return address.ReserveRandom{}, errmap.Usagef("--reservation-period must be positive") - } - if strconv.IntSize == 32 && seconds > math.MaxUint32 { - return address.ReserveRandom{}, errmap.Usagef("--reservation-period is too large") + if flags.Changed("reservation-period") && f.reservationPeriod < time.Second { + return address.ReserveRandom{}, errmap.Usagef("--reservation-period must be at least 1s") } - // #nosec G115 -- seconds is positive and bounded to MaxUint32 on 32-bit platforms. + seconds := f.reservationPeriod / time.Second + // #nosec G115 -- seconds is at least 1 and bounded by time.Duration's range. period := uint(seconds) return address.ReserveRandom{ @@ -482,6 +483,9 @@ func newNetworkAddressReserveCommand(opts *globalOptions) *cobra.Command { if err != nil { return opts.Fail(fmt.Errorf("reserving addresses: %w", err)) } + if len(reserved.Data) == 0 { + return opts.Fail(errors.New("reserving addresses: the Engine returned no addresses")) + } if w.Format().Structured() { return w.Object(reserved) diff --git a/internal/cli/network_test.go b/internal/cli/network_test.go index 581d487..7a50b85 100644 --- a/internal/cli/network_test.go +++ b/internal/cli/network_test.go @@ -1229,13 +1229,17 @@ func TestNetworkAddressesPluralAlias(t *testing.T) { require.Contains(t, stdout, "10.0.0.1") } -const oneAddress = `{"identifier":"a-1","name":"10.0.0.1","role_text":"Default","description_customer":"gateway","rdns_name":"old.example.com"}` +const ( + oneAddress = `{"identifier":"a-1","name":"10.0.0.1","role_text":"Default","description_customer":"gateway","rdns_name":"old.example.com"}` + createdAddress = `{"identifier":"a-1","name":"10.0.0.9","role_text":"Default","description_customer":"gateway","rdns_name":"host.example.com"}` +) +// TestNetworkAddressCreateSendsTheLegacyCreateBody pins the address creation payload and Engine response rendering. func TestNetworkAddressCreateSendsTheLegacyCreateBody(t *testing.T) { isolate(t) var sent []request - srv := recordingServer(t, &sent, oneAddress) + srv := recordingServer(t, &sent, createdAddress) stdout, _, err := run(t, "network", "address", "create", "--prefix", "p-1", "--address", "10.0.0.1", "--description", "gateway", @@ -1256,9 +1260,13 @@ func TestNetworkAddressCreateSendsTheLegacyCreateBody(t *testing.T) { "organization": "o-1", "rdns_name": "host.example.com", }, body) - require.Contains(t, stdout, "10.0.0.1") + require.Equal(t, + "IDENTIFIER NAME ROLE DESCRIPTION\n"+ + "a-1 10.0.0.9 Default gateway\n", + stdout) } +// TestNetworkAddressCreateRejectsBadFlags pins that invalid create input never reaches the Engine. func TestNetworkAddressCreateRejectsBadFlags(t *testing.T) { tests := []struct { name string @@ -1268,6 +1276,7 @@ func TestNetworkAddressCreateRejectsBadFlags(t *testing.T) { {name: "missing prefix", args: []string{"--address", "10.0.0.1"}, want: "--prefix is required"}, {name: "missing address", args: []string{"--prefix", "p-1"}, want: "--address is required"}, {name: "invalid prefix", args: []string{"--prefix", "p/1", "--address", "10.0.0.1"}, want: `prefix "p/1" does not name a prefix`}, + {name: "invalid organization", args: []string{"--prefix", "p-1", "--address", "10.0.0.1", "--organization", "o/1"}, want: `organization "o/1" does not name a organization`}, } for _, tt := range tests { @@ -1287,6 +1296,7 @@ func TestNetworkAddressCreateRejectsBadFlags(t *testing.T) { } } +// TestNetworkAddressUpdateKeepsRDNSAndRendersTheWrite pins rDNS preservation and rendering the PUT response. func TestNetworkAddressUpdateKeepsRDNSAndRendersTheWrite(t *testing.T) { isolate(t) @@ -1299,7 +1309,11 @@ func TestNetworkAddressUpdateKeepsRDNSAndRendersTheWrite(t *testing.T) { require.NoError(t, err) require.Len(t, sent, 2) require.Equal(t, http.MethodGet, sent[0].method) + require.Equal(t, "/api/ipam/v1/address.json/a-1", sent[0].path) + require.Empty(t, sent[0].query) require.Equal(t, http.MethodPut, sent[1].method) + require.Equal(t, "/api/ipam/v1/address.json/a-1", sent[1].path) + require.Empty(t, sent[1].query) var body map[string]any require.NoError(t, json.Unmarshal([]byte(sent[1].body), &body)) @@ -1313,6 +1327,63 @@ func TestNetworkAddressUpdateKeepsRDNSAndRendersTheWrite(t *testing.T) { require.Equal(t, "lab", shown["description_customer"]) } +// TestNetworkAddressUpdateHandlesRDNSAndRoleChanges pins sparse updates and explicit rDNS overrides. +func TestNetworkAddressUpdateHandlesRDNSAndRoleChanges(t *testing.T) { + tests := []struct { + name string + args []string + want map[string]any + }{ + {name: "clears rdns", args: []string{"--rdns", ""}, want: map[string]any{"rdns_name": ""}}, + {name: "changes role", args: []string{"--role", "Reserved"}, want: map[string]any{"role": "Reserved", "rdns_name": "old.example.com"}}, + {name: "overrides rdns", args: []string{"--rdns", "new.example.com"}, want: map[string]any{"rdns_name": "new.example.com"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + isolate(t) + var sent []request + srv := recordingServer(t, &sent, oneAddress, oneAddress) + + args := append([]string{"network", "address", "update", "a-1"}, tt.args...) + args = append(args, "--token", "tok", "--api-base-url", srv) + _, _, err := run(t, args...) + require.NoError(t, err) + require.Len(t, sent, 2) + + var body map[string]any + require.NoError(t, json.Unmarshal([]byte(sent[1].body), &body)) + require.Equal(t, tt.want, body) + }) + } +} + +// TestNetworkAddressUpdateReportsAFailedWrite pins that a failed PUT is named as an update. +func TestNetworkAddressUpdateReportsAFailedWrite(t *testing.T) { + isolate(t) + + var methods []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + methods = append(methods, r.Method) + w.Header().Set("Content-Type", "application/json") + if r.Method == http.MethodPut { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"error":{"code":500}}`)) + + return + } + _, _ = w.Write([]byte(oneAddress)) + })) + t.Cleanup(srv.Close) + + _, _, err := run(t, "network", "address", "update", "a-1", "--description", "lab", + "--token", "tok", "--api-base-url", srv.URL) + require.Error(t, err) + require.Equal(t, []string{http.MethodGet, http.MethodPut}, methods) + require.Contains(t, errmap.Message(err), `updating address "a-1"`) +} + +// TestNetworkAddressUpdateRejectsInvalidChanges pins refused sparse update values before a read or write. func TestNetworkAddressUpdateRejectsInvalidChanges(t *testing.T) { tests := []struct { name string @@ -1341,6 +1412,56 @@ func TestNetworkAddressUpdateRejectsInvalidChanges(t *testing.T) { } } +// TestNetworkAddressWriteVerbsGuardTheIdentifier pins URL path safety for address writes. +func TestNetworkAddressWriteVerbsGuardTheIdentifier(t *testing.T) { + verbs := []struct { + name string + args func(string) []string + method string + }{ + {name: "update", args: func(id string) []string { return []string{"network", "address", "update", id, "--description", "lab"} }, method: http.MethodPut}, + {name: "delete", args: func(id string) []string { return []string{"network", "address", "delete", id, "--yes"} }, method: http.MethodDelete}, + } + badIDs := map[string]string{ + "a slash": "p/1", "nothing": "", "whitespace": " ", "a dot": ".", "two dots": "..", "padded dots": " .. ", + } + + for _, verb := range verbs { + for label, id := range badIDs { + t.Run(verb.name+" refuses "+label, func(t *testing.T) { + isolate(t) + var sent []request + srv := recordingServer(t, &sent, oneAddress) + + args := append(verb.args(id), "--token", "tok", "--api-base-url", srv) + _, _, err := run(t, args...) + require.Error(t, err) + require.Equal(t, errmap.ExitUsage, errmap.ExitCode(err)) + require.Contains(t, errmap.Message(err), fmt.Sprintf("address %q does not name a address", id)) + require.Empty(t, sent) + }) + } + + t.Run(verb.name+" escapes the identifier", func(t *testing.T) { + isolate(t) + var sent []request + responses := []string{oneAddress} + if verb.name == "update" { + responses = append(responses, oneAddress) + } + srv := recordingServer(t, &sent, responses...) + + args := append(verb.args("a 1?x=y"), "--token", "tok", "--api-base-url", srv) + _, _, err := run(t, args...) + require.NoError(t, err) + require.Equal(t, verb.method, sent[len(sent)-1].method) + require.Equal(t, "/api/ipam/v1/address.json/a 1?x=y", sent[len(sent)-1].path) + require.Empty(t, sent[len(sent)-1].query) + }) + } +} + +// TestNetworkAddressDeleteConfirms pins the confirmation prompt and deletion note. func TestNetworkAddressDeleteConfirms(t *testing.T) { isolate(t) @@ -1353,10 +1474,51 @@ func TestNetworkAddressDeleteConfirms(t *testing.T) { require.Empty(t, stdout) require.Len(t, sent, 1) require.Equal(t, http.MethodDelete, sent[0].method) + require.Equal(t, "/api/ipam/v1/address.json/a-1", sent[0].path) + require.Empty(t, sent[0].body) require.Contains(t, stderr, `delete address "a-1"`) require.Contains(t, stderr, "deleted address a-1") } +// TestNetworkAddressDeleteStopsOnARefusal pins that a declined confirmation makes no request. +func TestNetworkAddressDeleteStopsOnARefusal(t *testing.T) { + isolate(t) + var sent []request + srv := recordingServer(t, &sent, `{}`) + + _, _, err := runWithInput(t, "n\n", "network", "address", "delete", "a-1", + "--token", "tok", "--api-base-url", srv) + require.Error(t, err) + require.Equal(t, errmap.ExitCanceled, errmap.ExitCode(err)) + require.Empty(t, sent) +} + +// TestNetworkAddressDeleteWithYesSkipsThePrompt pins the destroy alias and noninteractive delete. +func TestNetworkAddressDeleteWithYesSkipsThePrompt(t *testing.T) { + isolate(t) + srv, last := server(t, http.StatusOK, `{}`) + + _, stderr, err := run(t, "network", "address", "destroy", "a-1", "--yes", + "--token", "tok", "--api-base-url", srv.URL) + require.NoError(t, err) + require.Equal(t, http.MethodDelete, last.method) + require.NotContains(t, stderr, "[y/N]") + require.Contains(t, stderr, "deleted address a-1") +} + +// TestNetworkAddressDeleteReportsTheFailure pins the action wording and not-found exit code. +func TestNetworkAddressDeleteReportsTheFailure(t *testing.T) { + isolate(t) + srv, _ := server(t, http.StatusNotFound, `{"error":{"code":404}}`) + + _, _, err := run(t, "network", "address", "delete", "a-1", "--yes", + "--token", "tok", "--api-base-url", srv.URL) + require.Error(t, err) + require.Equal(t, errmap.ExitNotFound, errmap.ExitCode(err)) + require.Contains(t, errmap.Message(err), `deleting address "a-1"`) +} + +// TestNetworkAddressReserveSendsThePayloadAndRendersTheData pins an explicit reservation body and list rendering. func TestNetworkAddressReserveSendsThePayloadAndRendersTheData(t *testing.T) { isolate(t) @@ -1381,10 +1543,65 @@ func TestNetworkAddressReserveSendsThePayloadAndRendersTheData(t *testing.T) { "ip_version": float64(6), "reservation_period": float64(1800), }, body) - require.Contains(t, stdout, "a-9") - require.Contains(t, stdout, "10.0.0.9") + require.Equal(t, + "IDENTIFIER ADDRESS PREFIX\n"+ + "a-9 10.0.0.9 10.0.0.0/24\n", + stdout) +} + +// TestNetworkAddressReserveWithMinimalFlagsPins the Engine default reservation payload. +func TestNetworkAddressReserveWithMinimalFlagsPins(t *testing.T) { + isolate(t) + var sent []request + srv := recordingServer(t, &sent, `{"data":[{"identifier":"a-9","text":"10.0.0.9","prefix":"10.0.0.0/24"}]}`) + + _, _, err := run(t, "network", "address", "reserve", "--location", "l-1", "--vlan", "v-1", + "--token", "tok", "--api-base-url", srv) + require.NoError(t, err) + var body map[string]any + require.NoError(t, json.Unmarshal([]byte(sent[0].body), &body)) + require.Equal(t, map[string]any{ + "location_identifier": "l-1", + "vlan_identifier": "v-1", + "count": float64(1), + }, body) +} + +// TestNetworkAddressReserveJSONPins structured output of the full reservation summary. +func TestNetworkAddressReserveJSONPins(t *testing.T) { + isolate(t) + srv, _ := server(t, http.StatusOK, `{"total_items":1,"data":[{"identifier":"a-9","text":"10.0.0.9","prefix":"10.0.0.0/24"}]}`) + + stdout, _, err := run(t, "network", "address", "reserve", "--location", "l-1", "--vlan", "v-1", "-o", "json", + "--token", "tok", "--api-base-url", srv.URL) + require.NoError(t, err) + var summary struct { + TotalItems int `json:"total_items"` + Data []struct { + ID string `json:"identifier"` + } `json:"data"` + } + require.NoError(t, json.Unmarshal([]byte(stdout), &summary)) + require.Equal(t, 1, summary.TotalItems) + require.Len(t, summary.Data, 1) + require.Equal(t, "a-9", summary.Data[0].ID) +} + +// TestNetworkAddressReserveRejectsAnEmptyEngineResponse pins that an accepted reservation always contains addresses. +func TestNetworkAddressReserveRejectsAnEmptyEngineResponse(t *testing.T) { + isolate(t) + srv, _ := server(t, http.StatusOK, `{"data":[]}`) + + stdout, stderr, err := run(t, "network", "address", "reserve", "--location", "l-1", "--vlan", "v-1", + "--token", "tok", "--api-base-url", srv.URL) + require.Error(t, err) + require.NotEqual(t, 0, errmap.ExitCode(err)) + require.Empty(t, stdout) + require.Empty(t, stderr) + require.Contains(t, errmap.Message(err), "reserving addresses: the Engine returned no addresses") } +// TestNetworkAddressReserveRejectsBadFlags pins reserve validation before the Engine is contacted. func TestNetworkAddressReserveRejectsBadFlags(t *testing.T) { tests := []struct { name string @@ -1395,7 +1612,12 @@ func TestNetworkAddressReserveRejectsBadFlags(t *testing.T) { {name: "missing vlan", args: []string{"--location", "l-1"}, want: "--vlan is required"}, {name: "zero count", args: []string{"--location", "l-1", "--vlan", "v-1", "--count", "0"}, want: "--count must be at least 1"}, {name: "invalid version", args: []string{"--location", "l-1", "--vlan", "v-1", "--version", "5"}, want: "--version 5 must be 4 or 6"}, - {name: "zero reservation period", args: []string{"--location", "l-1", "--vlan", "v-1", "--reservation-period", "0s"}, want: "--reservation-period must be positive"}, + {name: "zero version", args: []string{"--location", "l-1", "--vlan", "v-1", "--version", "0"}, want: "--version 0 must be 4 or 6"}, + {name: "invalid location", args: []string{"--location", "l/1", "--vlan", "v-1"}, want: `location "l/1" does not name a location`}, + {name: "invalid vlan", args: []string{"--location", "l-1", "--vlan", "v/1"}, want: `vlan "v/1" does not name a vlan`}, + {name: "invalid prefix", args: []string{"--location", "l-1", "--vlan", "v-1", "--prefix", "p/1"}, want: `prefix "p/1" does not name a prefix`}, + {name: "sub-second reservation period", args: []string{"--location", "l-1", "--vlan", "v-1", "--reservation-period", "500ms"}, want: "--reservation-period must be at least 1s"}, + {name: "negative reservation period", args: []string{"--location", "l-1", "--vlan", "v-1", "--reservation-period", "-1s"}, want: "--reservation-period must be at least 1s"}, } for _, tt := range tests { From df2154fd3e6f1d4031868dd711d120a70f395dc5 Mon Sep 17 00:00:00 2001 From: ProbstenHias Date: Sat, 12 Sep 2026 14:04:48 +0200 Subject: [PATCH 3/5] test(network): pin address create defaults, update read failure and reserve exit code Also document the --address flag naming and list network address among the resources that report a provisioning state. --- README.md | 2 +- docs/cli-design.md | 8 ++- internal/cli/network_address.go | 4 +- internal/cli/network_test.go | 108 +++++++++++++++++++++++++++++--- 4 files changed, 109 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index 345eb72..b21abc5 100644 --- a/README.md +++ b/README.md @@ -82,7 +82,7 @@ anexia network address list --prefix --version 4 anexia network address create --prefix --address 192.0.2.10 --description "web" --role Default --rdns web.example.com anexia network address update --description "web (retired)" --role Default --rdns old-web.example.com anexia network address delete --yes -anexia network address reserve --location --vlan --count 2 --prefix --reservation-period 30m +anexia network address reserve --location --vlan --count 2 --prefix --reservation-period 1h ``` Boolean payload flags such as `--vm-provisioning` are switched off on `update` with an explicit diff --git a/docs/cli-design.md b/docs/cli-design.md index 35e9fd6..477bca5 100644 --- a/docs/cli-design.md +++ b/docs/cli-design.md @@ -153,6 +153,9 @@ is not past anything, so it stays a plain not-found and exits 4. Flag names are lowercase and use dashes, never underscores. Every flag has a usage string. A command never registers a local flag whose name collides with a global one. +`network address create` calls the Engine's `name` field `--address`, because the value is an IP +address and `--name` would read as a label. `list` and `get` still show it in the NAME column. + Filter flags on `list` are named after the field they filter, in the singular: `--tag`, `--name`, `--location`, `--status`, `--service`. Not after the Engine's query parameter, which is why `core tag list` takes `--name` even though the Engine calls it `query`. Repeatable filters would @@ -171,8 +174,9 @@ that accepts sorting. Write verbs on resources that report a provisioning state will get `--wait` and `--wait-timeout`. Resources without a state must not get the flags at all, so `--wait` is never accepted only to -fail later. `network vlan` (`Pending`, `Active`, `Marked for deletion`) and `network prefix` (`status`) -are the implemented resources that report one; their `--wait` is still to come. +fail later. `network vlan` (`Pending`, `Active`, `Marked for deletion`), `network prefix` (`status`) +and `network address` (`status`) are the implemented resources that report one; their `--wait` is +still to come. An `update` that names no field is refused before the write. The Engine would accept it, and on a resource that versions its contents that means a revision nobody asked for, reported as success. diff --git a/internal/cli/network_address.go b/internal/cli/network_address.go index 479ff4e..97aad44 100644 --- a/internal/cli/network_address.go +++ b/internal/cli/network_address.go @@ -34,6 +34,8 @@ func newNetworkAddressCommand(opts *globalOptions) *cobra.Command { ) } +// addressColumns is the projection list create and update share: the Engine +// answers both writes with the list summary rather than the full object. var addressColumns = []string{"identifier", "name", "role", "description"} func addressRow(s *address.Summary) []string { @@ -440,7 +442,7 @@ func (f *addressReserveFlags) payload(flags *pflag.FlagSet) (address.ReserveRand } seconds := f.reservationPeriod / time.Second - // #nosec G115 -- seconds is at least 1 and bounded by time.Duration's range. + // #nosec G115 -- the value is 0 when unset and otherwise at least 1, so it is never negative. period := uint(seconds) return address.ReserveRandom{ diff --git a/internal/cli/network_test.go b/internal/cli/network_test.go index 7a50b85..4a89e0f 100644 --- a/internal/cli/network_test.go +++ b/internal/cli/network_test.go @@ -1266,6 +1266,48 @@ func TestNetworkAddressCreateSendsTheLegacyCreateBody(t *testing.T) { stdout) } +// TestNetworkAddressCreateCarriesTheSelectedAndDefaultFields pins create payload values without omitempty. +func TestNetworkAddressCreateCarriesTheSelectedAndDefaultFields(t *testing.T) { + tests := []struct { + name string + args []string + want map[string]any + }{ + { + name: "selected role", + args: []string{"--prefix", "p-1", "--address", "10.0.0.1", "--role", "Reserved"}, + want: map[string]any{ + "prefix": "p-1", "name": "10.0.0.1", "description_customer": "", "role": "Reserved", "organization": "", "rdns_name": "", + }, + }, + { + name: "defaults", + args: []string{"--prefix", "p-1", "--address", "10.0.0.1"}, + want: map[string]any{ + "prefix": "p-1", "name": "10.0.0.1", "description_customer": "", "role": "Default", "organization": "", "rdns_name": "", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + isolate(t) + var sent []request + srv := recordingServer(t, &sent, createdAddress) + + args := append([]string{"network", "address", "create"}, tt.args...) + args = append(args, "--token", "tok", "--api-base-url", srv) + _, _, err := run(t, args...) + require.NoError(t, err) + require.Len(t, sent, 1) + + var body map[string]any + require.NoError(t, json.Unmarshal([]byte(sent[0].body), &body)) + require.Equal(t, tt.want, body) + }) + } +} + // TestNetworkAddressCreateRejectsBadFlags pins that invalid create input never reaches the Engine. func TestNetworkAddressCreateRejectsBadFlags(t *testing.T) { tests := []struct { @@ -1383,6 +1425,28 @@ func TestNetworkAddressUpdateReportsAFailedWrite(t *testing.T) { require.Contains(t, errmap.Message(err), `updating address "a-1"`) } +// TestNetworkAddressUpdateStopsWhenTheReadFails pins rDNS preservation by refusing to write after a failed read. +func TestNetworkAddressUpdateStopsWhenTheReadFails(t *testing.T) { + isolate(t) + + var sent []request + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + sent = append(sent, request{method: r.Method, path: r.URL.Path}) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"error":{"code":404}}`)) + })) + t.Cleanup(srv.Close) + + _, _, err := run(t, "network", "address", "update", "a-1", "--description", "lab", + "--token", "tok", "--api-base-url", srv.URL) + require.Error(t, err) + require.Len(t, sent, 1, "the read carries rdns_name, so ignoring its error would wipe reverse DNS") + require.Equal(t, http.MethodGet, sent[0].method) + require.Equal(t, errmap.ExitNotFound, errmap.ExitCode(err)) + require.Contains(t, errmap.Message(err), `reading address "a-1"`) +} + // TestNetworkAddressUpdateRejectsInvalidChanges pins refused sparse update values before a read or write. func TestNetworkAddressUpdateRejectsInvalidChanges(t *testing.T) { tests := []struct { @@ -1412,6 +1476,23 @@ func TestNetworkAddressUpdateRejectsInvalidChanges(t *testing.T) { } } +// TestNetworkAddressUpdateOffersTheWritableFields pins the complete update flag surface. +func TestNetworkAddressUpdateOffersTheWritableFields(t *testing.T) { + isolate(t) + + stdout, _, err := run(t, "network", "address", "update", "--help") + require.NoError(t, err) + + _, after, found := strings.Cut(stdout, "Flags:\n") + require.True(t, found) + local, _, _ := strings.Cut(after, "Global Flags:") + var names []string + for _, m := range regexp.MustCompile(`(?m)^\s+(?:-\w, )?--([\w-]+)`).FindAllStringSubmatch(local, -1) { + names = append(names, m[1]) + } + require.Equal(t, []string{"description", "help", "rdns", "role"}, names) +} + // TestNetworkAddressWriteVerbsGuardTheIdentifier pins URL path safety for address writes. func TestNetworkAddressWriteVerbsGuardTheIdentifier(t *testing.T) { verbs := []struct { @@ -1423,7 +1504,7 @@ func TestNetworkAddressWriteVerbsGuardTheIdentifier(t *testing.T) { {name: "delete", args: func(id string) []string { return []string{"network", "address", "delete", id, "--yes"} }, method: http.MethodDelete}, } badIDs := map[string]string{ - "a slash": "p/1", "nothing": "", "whitespace": " ", "a dot": ".", "two dots": "..", "padded dots": " .. ", + "a slash": "a/1", "nothing": "", "whitespace": " ", "a dot": ".", "two dots": "..", "padded dots": " .. ", } for _, verb := range verbs { @@ -1454,9 +1535,16 @@ func TestNetworkAddressWriteVerbsGuardTheIdentifier(t *testing.T) { args := append(verb.args("a 1?x=y"), "--token", "tok", "--api-base-url", srv) _, _, err := run(t, args...) require.NoError(t, err) + if verb.name == "update" { + require.Len(t, sent, 2) + } else { + require.Len(t, sent, 1) + } require.Equal(t, verb.method, sent[len(sent)-1].method) - require.Equal(t, "/api/ipam/v1/address.json/a 1?x=y", sent[len(sent)-1].path) - require.Empty(t, sent[len(sent)-1].query) + for _, request := range sent { + require.Equal(t, "/api/ipam/v1/address.json/a 1?x=y", request.path) + require.Empty(t, request.query) + } }) } } @@ -1518,7 +1606,7 @@ func TestNetworkAddressDeleteReportsTheFailure(t *testing.T) { require.Contains(t, errmap.Message(err), `deleting address "a-1"`) } -// TestNetworkAddressReserveSendsThePayloadAndRendersTheData pins an explicit reservation body and list rendering. +// TestNetworkAddressReserveSendsThePayloadAndRendersTheData pins an explicit reservation body and list rendering. ReserveRandom sleeps up to one second before each request, so reserve tests are slow by design. func TestNetworkAddressReserveSendsThePayloadAndRendersTheData(t *testing.T) { isolate(t) @@ -1549,8 +1637,8 @@ func TestNetworkAddressReserveSendsThePayloadAndRendersTheData(t *testing.T) { stdout) } -// TestNetworkAddressReserveWithMinimalFlagsPins the Engine default reservation payload. -func TestNetworkAddressReserveWithMinimalFlagsPins(t *testing.T) { +// TestNetworkAddressReserveSendsOnlyTheRequiredFieldsByDefault pins the Engine default reservation payload. +func TestNetworkAddressReserveSendsOnlyTheRequiredFieldsByDefault(t *testing.T) { isolate(t) var sent []request srv := recordingServer(t, &sent, `{"data":[{"identifier":"a-9","text":"10.0.0.9","prefix":"10.0.0.0/24"}]}`) @@ -1558,6 +1646,7 @@ func TestNetworkAddressReserveWithMinimalFlagsPins(t *testing.T) { _, _, err := run(t, "network", "address", "reserve", "--location", "l-1", "--vlan", "v-1", "--token", "tok", "--api-base-url", srv) require.NoError(t, err) + require.Len(t, sent, 1) var body map[string]any require.NoError(t, json.Unmarshal([]byte(sent[0].body), &body)) require.Equal(t, map[string]any{ @@ -1567,8 +1656,8 @@ func TestNetworkAddressReserveWithMinimalFlagsPins(t *testing.T) { }, body) } -// TestNetworkAddressReserveJSONPins structured output of the full reservation summary. -func TestNetworkAddressReserveJSONPins(t *testing.T) { +// TestNetworkAddressReserveKeepsTheSummaryInJSON pins structured output of the full reservation summary. +func TestNetworkAddressReserveKeepsTheSummaryInJSON(t *testing.T) { isolate(t) srv, _ := server(t, http.StatusOK, `{"total_items":1,"data":[{"identifier":"a-9","text":"10.0.0.9","prefix":"10.0.0.0/24"}]}`) @@ -1595,7 +1684,7 @@ func TestNetworkAddressReserveRejectsAnEmptyEngineResponse(t *testing.T) { stdout, stderr, err := run(t, "network", "address", "reserve", "--location", "l-1", "--vlan", "v-1", "--token", "tok", "--api-base-url", srv.URL) require.Error(t, err) - require.NotEqual(t, 0, errmap.ExitCode(err)) + require.Equal(t, errmap.ExitError, errmap.ExitCode(err)) require.Empty(t, stdout) require.Empty(t, stderr) require.Contains(t, errmap.Message(err), "reserving addresses: the Engine returned no addresses") @@ -1618,6 +1707,7 @@ func TestNetworkAddressReserveRejectsBadFlags(t *testing.T) { {name: "invalid prefix", args: []string{"--location", "l-1", "--vlan", "v-1", "--prefix", "p/1"}, want: `prefix "p/1" does not name a prefix`}, {name: "sub-second reservation period", args: []string{"--location", "l-1", "--vlan", "v-1", "--reservation-period", "500ms"}, want: "--reservation-period must be at least 1s"}, {name: "negative reservation period", args: []string{"--location", "l-1", "--vlan", "v-1", "--reservation-period", "-1s"}, want: "--reservation-period must be at least 1s"}, + {name: "zero reservation period", args: []string{"--location", "l-1", "--vlan", "v-1", "--reservation-period", "0s"}, want: "--reservation-period must be at least 1s"}, } for _, tt := range tests { From d4dd7fab663201176113ec7ad498c76cf85e880d Mon Sep 17 00:00:00 2001 From: ProbstenHias Date: Sat, 12 Sep 2026 14:18:32 +0200 Subject: [PATCH 4/5] test(network): pin address reserve period boundary and tidy update tests Order the address file by verb like prefix, note why update offers no --name flag, and pin that 1s is accepted while sub-second remainders are truncated to whole seconds. --- docs/cli-design.md | 3 +- internal/cli/network_address.go | 102 ++++++++++++++++---------------- internal/cli/network_test.go | 69 +++++++++++++-------- 3 files changed, 97 insertions(+), 77 deletions(-) diff --git a/docs/cli-design.md b/docs/cli-design.md index 477bca5..91f5a4e 100644 --- a/docs/cli-design.md +++ b/docs/cli-design.md @@ -197,7 +197,8 @@ reason `dns zone update` has no `--name`. `--description ""` is refused for the `network address update` reads the address only to retain `rdns_name`, the one field in go-anxcloud's `address.Update` without `omitempty`. `description_customer` and `role` are sparse, so `--description ""` and `--role ""` are refused with the same "cannot be emptied" wording as -prefix; `--rdns ""` reaches the Engine and clears the reverse DNS name. +prefix; `--rdns ""` reaches the Engine and clears the reverse DNS name. The legacy update also +carries `name`, but an address's name is the IP itself, so it is not offered, as with prefix. A field the Engine cannot change safely does not get a flag. `dns zone update` has no `--name`, because the Engine's zone update carries the name only in the request body with no old name diff --git a/internal/cli/network_address.go b/internal/cli/network_address.go index 97aad44..e7888a3 100644 --- a/internal/cli/network_address.go +++ b/internal/cli/network_address.go @@ -192,6 +192,54 @@ func newNetworkAddressListCommand(opts *globalOptions) *cobra.Command { return cmd } +func newNetworkAddressGetCommand(opts *globalOptions) *cobra.Command { + return &cobra.Command{ + Use: "get ", + Short: "Show one address", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + if err := resource.ValidateIdentifier("address", args[0]); err != nil { + return err + } + + w, err := opts.Writer(cmd.OutOrStdout()) + if err != nil { + return err + } + + c, err := opts.Client(cmd.Flags()) + if err != nil { + return err + } + + ctx, cancel := opts.Context(cmd.Context()) + defer cancel() + + info, err := address.NewAPI(c).Get(ctx, pathValue(args[0])) + if err != nil { + return opts.Fail(fmt.Errorf("reading address %q: %w", args[0], err)) + } + + if w.Format().Structured() { + return w.Object(info) + } + + // Four columns, per the column budget in docs/cli-design.md. + // The VLAN and prefix an address sits in are one "-o json" + // away, and are less use at a glance than what it is. + return w.Table( + []string{"identifier", "name", "version", "status"}, + [][]string{{ + info.ID, + info.Name, + versionValue(info.Version), + info.Status, + }}, + ) + }, + } +} + type addressCreateFlags struct { prefix string address string @@ -282,7 +330,9 @@ func newNetworkAddressCreateCommand(opts *globalOptions) *cobra.Command { // newNetworkAddressUpdateCommand reads the address first only to carry // rdns_name, the one address.Update field without omitempty, so an unset // --rdns does not wipe reverse DNS. Description and role are sparse fields, so -// emptying them is refused while --rdns "" clears the reverse DNS name. +// emptying them is refused while --rdns "" clears the reverse DNS name. The +// update body also carries name, but an address's name is the IP itself, so no +// flag is offered for it. func newNetworkAddressUpdateCommand(opts *globalOptions) *cobra.Command { var description, role, rdns string @@ -313,7 +363,7 @@ func newNetworkAddressUpdateCommand(opts *globalOptions) *cobra.Command { return err } - c, err := opts.Client(cmd.Flags()) + c, err := opts.Client(flags) if err != nil { return err } @@ -506,51 +556,3 @@ func newNetworkAddressReserveCommand(opts *globalOptions) *cobra.Command { return cmd } - -func newNetworkAddressGetCommand(opts *globalOptions) *cobra.Command { - return &cobra.Command{ - Use: "get ", - Short: "Show one address", - Args: cobra.ExactArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - if err := resource.ValidateIdentifier("address", args[0]); err != nil { - return err - } - - w, err := opts.Writer(cmd.OutOrStdout()) - if err != nil { - return err - } - - c, err := opts.Client(cmd.Flags()) - if err != nil { - return err - } - - ctx, cancel := opts.Context(cmd.Context()) - defer cancel() - - info, err := address.NewAPI(c).Get(ctx, pathValue(args[0])) - if err != nil { - return opts.Fail(fmt.Errorf("reading address %q: %w", args[0], err)) - } - - if w.Format().Structured() { - return w.Object(info) - } - - // Four columns, per the column budget in docs/cli-design.md. - // The VLAN and prefix an address sits in are one "-o json" - // away, and are less use at a glance than what it is. - return w.Table( - []string{"identifier", "name", "version", "status"}, - [][]string{{ - info.ID, - info.Name, - versionValue(info.Version), - info.Status, - }}, - ) - }, - } -} diff --git a/internal/cli/network_test.go b/internal/cli/network_test.go index 4a89e0f..1c7eabf 100644 --- a/internal/cli/network_test.go +++ b/internal/cli/network_test.go @@ -1429,20 +1429,12 @@ func TestNetworkAddressUpdateReportsAFailedWrite(t *testing.T) { func TestNetworkAddressUpdateStopsWhenTheReadFails(t *testing.T) { isolate(t) - var sent []request - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - sent = append(sent, request{method: r.Method, path: r.URL.Path}) - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(http.StatusNotFound) - _, _ = w.Write([]byte(`{"error":{"code":404}}`)) - })) - t.Cleanup(srv.Close) + srv, last := server(t, http.StatusNotFound, `{"error":{"code":404}}`) _, _, err := run(t, "network", "address", "update", "a-1", "--description", "lab", "--token", "tok", "--api-base-url", srv.URL) require.Error(t, err) - require.Len(t, sent, 1, "the read carries rdns_name, so ignoring its error would wipe reverse DNS") - require.Equal(t, http.MethodGet, sent[0].method) + require.Equal(t, http.MethodGet, last.method, "a PUT afterwards would overwrite the recorded GET") require.Equal(t, errmap.ExitNotFound, errmap.ExitCode(err)) require.Contains(t, errmap.Message(err), `reading address "a-1"`) } @@ -1496,12 +1488,13 @@ func TestNetworkAddressUpdateOffersTheWritableFields(t *testing.T) { // TestNetworkAddressWriteVerbsGuardTheIdentifier pins URL path safety for address writes. func TestNetworkAddressWriteVerbsGuardTheIdentifier(t *testing.T) { verbs := []struct { - name string - args func(string) []string - method string + name string + args func(string) []string + method string + requests int }{ - {name: "update", args: func(id string) []string { return []string{"network", "address", "update", id, "--description", "lab"} }, method: http.MethodPut}, - {name: "delete", args: func(id string) []string { return []string{"network", "address", "delete", id, "--yes"} }, method: http.MethodDelete}, + {name: "update", args: func(id string) []string { return []string{"network", "address", "update", id, "--description", "lab"} }, method: http.MethodPut, requests: 2}, + {name: "delete", args: func(id string) []string { return []string{"network", "address", "delete", id, "--yes"} }, method: http.MethodDelete, requests: 1}, } badIDs := map[string]string{ "a slash": "a/1", "nothing": "", "whitespace": " ", "a dot": ".", "two dots": "..", "padded dots": " .. ", @@ -1526,20 +1519,12 @@ func TestNetworkAddressWriteVerbsGuardTheIdentifier(t *testing.T) { t.Run(verb.name+" escapes the identifier", func(t *testing.T) { isolate(t) var sent []request - responses := []string{oneAddress} - if verb.name == "update" { - responses = append(responses, oneAddress) - } - srv := recordingServer(t, &sent, responses...) + srv := recordingServer(t, &sent, oneAddress) args := append(verb.args("a 1?x=y"), "--token", "tok", "--api-base-url", srv) _, _, err := run(t, args...) require.NoError(t, err) - if verb.name == "update" { - require.Len(t, sent, 2) - } else { - require.Len(t, sent, 1) - } + require.Len(t, sent, verb.requests) require.Equal(t, verb.method, sent[len(sent)-1].method) for _, request := range sent { require.Equal(t, "/api/ipam/v1/address.json/a 1?x=y", request.path) @@ -1606,7 +1591,9 @@ func TestNetworkAddressDeleteReportsTheFailure(t *testing.T) { require.Contains(t, errmap.Message(err), `deleting address "a-1"`) } -// TestNetworkAddressReserveSendsThePayloadAndRendersTheData pins an explicit reservation body and list rendering. ReserveRandom sleeps up to one second before each request, so reserve tests are slow by design. +// TestNetworkAddressReserveSendsThePayloadAndRendersTheData pins an explicit +// reservation body and list rendering. ReserveRandom sleeps up to one second +// before each request, so reserve tests are slow by design. func TestNetworkAddressReserveSendsThePayloadAndRendersTheData(t *testing.T) { isolate(t) @@ -1656,6 +1643,36 @@ func TestNetworkAddressReserveSendsOnlyTheRequiredFieldsByDefault(t *testing.T) }, body) } +// TestNetworkAddressReserveTruncatesAcceptedPeriods pins the one-second +// boundary and the whole-seconds payload conversion. +func TestNetworkAddressReserveTruncatesAcceptedPeriods(t *testing.T) { + tests := []struct { + period string + want float64 + }{ + {period: "1s", want: 1}, + {period: "1500ms", want: 1}, + {period: "1h", want: 3600}, + } + + for _, tt := range tests { + t.Run(tt.period, func(t *testing.T) { + isolate(t) + var sent []request + srv := recordingServer(t, &sent, `{"data":[{"identifier":"a-9","text":"10.0.0.9","prefix":"10.0.0.0/24"}]}`) + + _, _, err := run(t, "network", "address", "reserve", "--location", "l-1", "--vlan", "v-1", + "--reservation-period", tt.period, "--token", "tok", "--api-base-url", srv) + require.NoError(t, err) + require.Len(t, sent, 1) + + var body map[string]any + require.NoError(t, json.Unmarshal([]byte(sent[0].body), &body)) + require.Equal(t, tt.want, body["reservation_period"]) + }) + } +} + // TestNetworkAddressReserveKeepsTheSummaryInJSON pins structured output of the full reservation summary. func TestNetworkAddressReserveKeepsTheSummaryInJSON(t *testing.T) { isolate(t) From ac6da099521ab8c0a2ad58f6605c2acc455defff Mon Sep 17 00:00:00 2001 From: ProbstenHias Date: Sat, 12 Sep 2026 14:26:41 +0200 Subject: [PATCH 5/5] test(network): pin the full address update JSON and tidy doc comments --- docs/cli-design.md | 2 +- internal/cli/network_address.go | 4 +++- internal/cli/network_test.go | 14 ++++++++------ 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/docs/cli-design.md b/docs/cli-design.md index 91f5a4e..7c3b5b8 100644 --- a/docs/cli-design.md +++ b/docs/cli-design.md @@ -234,7 +234,7 @@ Four formats, one flag. Column sets are short on purpose, up to five fields, because a table wider than a terminal is useless. Fewer when the Engine returns less: prefix and address writes are answered with their list summaries, so `network prefix create`, `network address create` and their `update` verbs show the -summary fields only, and the full object is a `get -o json` away. Everywhere else the full +list columns only, and the full object is a `get -o json` away. Everywhere else the full object is one `-o json` away. `tsv` is `table` without the alignment: raw values, lowercase headers, tab-separated. This is the diff --git a/internal/cli/network_address.go b/internal/cli/network_address.go index e7888a3..a2ecead 100644 --- a/internal/cli/network_address.go +++ b/internal/cli/network_address.go @@ -34,7 +34,7 @@ func newNetworkAddressCommand(opts *globalOptions) *cobra.Command { ) } -// addressColumns is the projection list create and update share: the Engine +// addressColumns is the projection list, create and update share: the Engine // answers both writes with the list summary rather than the full object. var addressColumns = []string{"identifier", "name", "role", "description"} @@ -42,6 +42,7 @@ func addressRow(s *address.Summary) []string { return []string{s.ID, s.Name, s.Role, s.DescriptionCustomer} } +// renderAddressSummary prints a write's summary answer as a table or object. func renderAddressSummary(w *output.Writer, s *address.Summary) error { if w.Format().Structured() { return w.Object(s) @@ -240,6 +241,7 @@ func newNetworkAddressGetCommand(opts *globalOptions) *cobra.Command { } } +// addressCreateFlags holds the create payload before it becomes an address.Create. type addressCreateFlags struct { prefix string address string diff --git a/internal/cli/network_test.go b/internal/cli/network_test.go index 1c7eabf..ef8e9cd 100644 --- a/internal/cli/network_test.go +++ b/internal/cli/network_test.go @@ -1249,6 +1249,7 @@ func TestNetworkAddressCreateSendsTheLegacyCreateBody(t *testing.T) { require.Len(t, sent, 1) require.Equal(t, http.MethodPost, sent[0].method) require.Equal(t, "/api/ipam/v1/address.json", sent[0].path) + require.Empty(t, sent[0].query) var body map[string]any require.NoError(t, json.Unmarshal([]byte(sent[0].body), &body)) @@ -1364,9 +1365,9 @@ func TestNetworkAddressUpdateKeepsRDNSAndRendersTheWrite(t *testing.T) { "rdns_name": "old.example.com", }, body) - var shown map[string]any - require.NoError(t, json.Unmarshal([]byte(stdout), &shown)) - require.Equal(t, "lab", shown["description_customer"]) + // The PUT answer is rendered whole: every summary field, including the + // ones the table drops (rdns_name, role_text), must survive into -o json. + require.JSONEq(t, updated, stdout) } // TestNetworkAddressUpdateHandlesRDNSAndRoleChanges pins sparse updates and explicit rDNS overrides. @@ -1526,9 +1527,9 @@ func TestNetworkAddressWriteVerbsGuardTheIdentifier(t *testing.T) { require.NoError(t, err) require.Len(t, sent, verb.requests) require.Equal(t, verb.method, sent[len(sent)-1].method) - for _, request := range sent { - require.Equal(t, "/api/ipam/v1/address.json/a 1?x=y", request.path) - require.Empty(t, request.query) + for _, req := range sent { + require.Equal(t, "/api/ipam/v1/address.json/a 1?x=y", req.path) + require.Empty(t, req.query) } }) } @@ -1607,6 +1608,7 @@ func TestNetworkAddressReserveSendsThePayloadAndRendersTheData(t *testing.T) { require.Len(t, sent, 1) require.Equal(t, http.MethodPost, sent[0].method) require.Equal(t, "/api/ipam/v1/address/reserve/ip/count.json", sent[0].path) + require.Empty(t, sent[0].query) var body map[string]any require.NoError(t, json.Unmarshal([]byte(sent[0].body), &body))