From 55e7a5106b487b14be4737eb5d62d6e75a526df2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Untersch=C3=BCtz?= Date: Sat, 14 Nov 2026 12:30:24 +0100 Subject: [PATCH] fix: interface_name strategy, instance hosts in sftp/scp, alias rewrites Found by the new Docker test environment against a real NetBox 4.7: - The interface_name strategy filtered IP addresses with interface_name and vminterface_name, which NetBox ignores, so it returned the first address of the host instead of the named interface's. Use the real filters interface and vminterface; a missing interface (HTTP 400) falls through to the next strategy. - sftp and scp took instance-qualified hosts (prod/web01:/path) for local paths, because a "/" comes before the ":". Such arguments are remote when the prefix is a configured NetBox instance. - Shell aliases of instance-qualified hosts ran the shortcut rules on "instance/host", so anchored rewrite rules never matched. Co-Authored-By: Claude Opus 5.5 --- cmd/netssh/main.go | 10 +++++ internal/netbox/client_test.go | 2 +- internal/netbox/sources_test.go | 2 +- internal/resolver/interface_name.go | 9 ++-- internal/resolver/interface_name_test.go | 56 ++++++++++++++++++++++++ internal/shortcuts/shortcuts.go | 7 ++- internal/shortcuts/shortcuts_test.go | 10 +++++ internal/ssh/args.go | 4 +- internal/ssh/client.go | 31 +++++++++++-- internal/ssh/client_test.go | 24 ++++++++++ 10 files changed, 144 insertions(+), 11 deletions(-) create mode 100644 internal/resolver/interface_name_test.go diff --git a/cmd/netssh/main.go b/cmd/netssh/main.go index b8b679d..cff6248 100644 --- a/cmd/netssh/main.go +++ b/cmd/netssh/main.go @@ -130,6 +130,7 @@ func findDestination(client internalssh.Client, args []string) (*destination, er // runClient resolves the NetBox host in args and execs ssh, sftp, or scp. func runClient(client internalssh.Client, flags internalssh.WrapperFlags, args []string) { cfg := loadConfigOrSetup() + client = client.WithInstances(instanceNames(cfg)) dest, err := findDestination(client, args) if err != nil { @@ -175,6 +176,15 @@ func hostDefaults(cfg *config.Config, host resolvedHost) (string, int) { return user, port } +// instanceNames lists the named NetBox instances, which may prefix hostnames. +func instanceNames(cfg *config.Config) []string { + names := make([]string, 0, len(cfg.NetBoxes)) + for _, source := range cfg.NetBoxes { + names = append(names, source.Name) + } + return names +} + // prependArgs puts extra before args and shifts the destination indices. func prependArgs(extra, args []string, dests []int) []string { for i := range dests { diff --git a/internal/netbox/client_test.go b/internal/netbox/client_test.go index a167107..373efe1 100644 --- a/internal/netbox/client_test.go +++ b/internal/netbox/client_test.go @@ -235,7 +235,7 @@ func TestGetIPsWithFilter(t *testing.T) { defer srv.Close() c := NewClient(srv.URL, "token", 0) - ips, err := c.GetIPsWithFilter(context.Background(), "device_id=1&interface_name=mgmt0") + ips, err := c.GetIPsWithFilter(context.Background(), "device_id=1&interface=mgmt0") if err != nil { t.Fatalf("GetIPsWithFilter: %v", err) } diff --git a/internal/netbox/sources_test.go b/internal/netbox/sources_test.go index a3686c6..0f56205 100644 --- a/internal/netbox/sources_test.go +++ b/internal/netbox/sources_test.go @@ -78,7 +78,7 @@ func TestMultiSourceSearchAndRouting(t *testing.T) { if err != nil || len(ips) != 1 || ips[0] != tc.ip { t.Fatalf("wrong IP source: %v, %v", ips, err) } - ips, err = c.GetIPsWithFilterForHost(ctx, host, "device_id=1&interface_name=mgmt0") + ips, err = c.GetIPsWithFilterForHost(ctx, host, "device_id=1&interface=mgmt0") if err != nil || len(ips) != 1 || ips[0] != tc.ip { t.Fatalf("wrong interface source: %v, %v", ips, err) } diff --git a/internal/resolver/interface_name.go b/internal/resolver/interface_name.go index 2b3483f..a39d6f3 100644 --- a/internal/resolver/interface_name.go +++ b/internal/resolver/interface_name.go @@ -16,13 +16,16 @@ type InterfaceNameStrategy struct { func (s *InterfaceNameStrategy) Name() string { return "interface_name" } func (s *InterfaceNameStrategy) Resolve(ctx context.Context, entry *netbox.HostEntry, client *netbox.Client) (string, error) { - // Build filter parameters for IP addresses attached to the named interface. + // NetBox filters IP addresses by interface name with interface= (devices) + // and vminterface= (VMs); unknown parameters would be ignored silently and + // return every address of the host. A missing interface is an HTTP 400, + // which lets the chain try the next strategy. var filterParam string switch entry.Kind { case "device": - filterParam = fmt.Sprintf("device_id=%d&interface_name=%s", entry.ID, url.QueryEscape(s.name)) + filterParam = fmt.Sprintf("device_id=%d&interface=%s", entry.ID, url.QueryEscape(s.name)) case "vm": - filterParam = fmt.Sprintf("virtual_machine_id=%d&vminterface_name=%s", entry.ID, url.QueryEscape(s.name)) + filterParam = fmt.Sprintf("virtual_machine_id=%d&vminterface=%s", entry.ID, url.QueryEscape(s.name)) default: return "", fmt.Errorf("unknown kind %q", entry.Kind) } diff --git a/internal/resolver/interface_name_test.go b/internal/resolver/interface_name_test.go new file mode 100644 index 0000000..3d06c7d --- /dev/null +++ b/internal/resolver/interface_name_test.go @@ -0,0 +1,56 @@ +package resolver + +import ( + "context" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "git.zb-server.de/Sebi/ssh-netbox-wrapper/internal/netbox" +) + +// NetBox ignores unknown filters, so the strategy must use the real filter +// names; otherwise it silently returns the host's first address. +func TestInterfaceNameStrategyFilters(t *testing.T) { + for kind, want := range map[string]string{ + "device": "device_id=7&interface=mgmt0", + "vm": "virtual_machine_id=7&vminterface=mgmt0", + } { + var got string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + q := r.URL.Query() + q.Del("limit") + q.Del("offset") + got = q.Encode() + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"count":1,"results":[{"address":"10.0.0.7/24"}]}`)) + })) + ip, err := (&InterfaceNameStrategy{name: "mgmt0"}).Resolve(context.Background(), &netbox.HostEntry{ID: 7, Kind: kind}, netbox.NewClient(srv.URL, "t", 1)) + srv.Close() + if err != nil || ip != "10.0.0.7" { + t.Fatalf("%s: %q %v", kind, ip, err) + } + if parsed, _ := urlQuery(want); got != parsed { + t.Errorf("%s: query %q, want %q", kind, got, parsed) + } + } +} + +func TestInterfaceNameStrategyMissingInterfaceFallsThrough(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, `{"vminterface":["Select a valid choice."]}`, http.StatusBadRequest) + })) + defer srv.Close() + client := netbox.NewClient(srv.URL, "t", 1) + chain := &Chain{strategies: []Strategy{&InterfaceNameStrategy{name: "mgmt0"}, &PrimaryIPStrategy{}}} + ip, err := chain.Resolve(context.Background(), &netbox.HostEntry{ID: 7, Kind: "vm", PrimaryIP4: "10.0.0.9"}, client) + if err != nil || ip != "10.0.0.9" { + t.Fatalf("got %q %v", ip, err) + } +} + +func urlQuery(s string) (string, error) { + q, err := url.ParseQuery(s) + return q.Encode(), err +} diff --git a/internal/shortcuts/shortcuts.go b/internal/shortcuts/shortcuts.go index f1d5ffb..e2f99ad 100644 --- a/internal/shortcuts/shortcuts.go +++ b/internal/shortcuts/shortcuts.go @@ -59,6 +59,11 @@ func MakeNormalizer(cfg config.ShortcutsConfig) func(string) string { // It normalizes the name (strips configured domains, applies rewrites, optionally strips hyphens) then // replaces any remaining dots with underscores so the result is a valid identifier. func AliasName(name string, cfg config.ShortcutsConfig) string { - s := Normalize(name, cfg) + // Shorten only the hostname: anchored rewrite rules must not see "instance/". + prefix := "" + if source, host, ok := strings.Cut(name, "/"); ok { + prefix, name = strings.ToLower(source)+"_", host + } + s := prefix + Normalize(name, cfg) return strings.NewReplacer(".", "_", "/", "_").Replace(s) } diff --git a/internal/shortcuts/shortcuts_test.go b/internal/shortcuts/shortcuts_test.go index 51e96c0..67b15b1 100644 --- a/internal/shortcuts/shortcuts_test.go +++ b/internal/shortcuts/shortcuts_test.go @@ -115,3 +115,13 @@ func TestNormalizeRewritesOrder(t *testing.T) { t.Fatalf("got %q", got) } } + +func TestAliasNameRewritesInstanceQualifiedHosts(t *testing.T) { + cfg := config.ShortcutsConfig{ + Domains: []string{".hoster.test"}, + Rewrites: []config.ShortcutRewrite{{Pattern: `^([a-z]+)[0-9]+-`, Replace: "$1"}}, + } + if got := shortcuts.AliasName("fsn/fsn1-pve1.hoster.test", cfg); got != "fsn_fsnpve1" { + t.Fatalf("got %q", got) + } +} diff --git a/internal/ssh/args.go b/internal/ssh/args.go index b36ea5a..6262568 100644 --- a/internal/ssh/args.go +++ b/internal/ssh/args.go @@ -59,7 +59,7 @@ func (c Client) Parse(args []string) *ParsedArgs { func (c Client) parsed(args []string, i int) *ParsedArgs { dest, path := args[i], "" if c.paths { - if d, p, remote := splitRemote(dest); remote { + if d, p, remote := c.splitRemote(dest); remote { dest, path = d, p } } @@ -88,7 +88,7 @@ func (c Client) replaceHost(args []string, dests []int, newHost string) []string prefix, original = original[:at+1], original[at+1:] } if c.paths { - if _, path, remote := splitRemote(original); remote { + if _, path, remote := c.splitRemote(original); remote { suffix = ":" + path } } diff --git a/internal/ssh/client.go b/internal/ssh/client.go index 3ffb926..1395f92 100644 --- a/internal/ssh/client.go +++ b/internal/ssh/client.go @@ -13,6 +13,18 @@ type Client struct { port byte // flag for the port: -p (ssh) or -P (sftp, scp) user byte // flag for the login user; 0 when only user@ or -o User work paths bool // destinations are host[:path] + // instances are the configured NetBox instance names; "inst/host:path" + // is remote only for these, so local paths like "dir/file:x" stay local. + instances map[string]bool +} + +// WithInstances returns c aware of the NetBox instance prefixes in host:path. +func (c Client) WithInstances(names []string) Client { + c.instances = make(map[string]bool, len(names)) + for _, name := range names { + c.instances[strings.ToLower(name)] = true + } + return c } var ( @@ -61,7 +73,7 @@ func (c Client) Operands(args []string) ([]Operand, error) { if strings.HasPrefix(arg, "scp://") || strings.HasPrefix(arg, "sftp://") { return nil, fmt.Errorf("%q: use host:path instead of a URI", arg) } - dest, path, remote := splitRemote(arg) + dest, path, remote := c.splitRemote(arg) if !remote { continue } @@ -105,9 +117,22 @@ func (c Client) insertionPoint(args []string) int { return len(args) } -// splitRemote mirrors scp's colon(): an argument is remote when a ':' comes +// splitRemote is splitHostPath with netssh's [user@]instance/host:path form. +func (c Client) splitRemote(arg string) (dest, path string, remote bool) { + if head, rest, ok := strings.Cut(arg, "/"); ok { + instance := head[strings.LastIndex(head, "@")+1:] + if c.instances[strings.ToLower(instance)] { + if d, p, r := splitHostPath(rest); r { + return head + "/" + d, p, true + } + } + } + return splitHostPath(arg) +} + +// splitHostPath mirrors scp's colon(): an argument is remote when a ':' comes // before any '/'. A leading "[" starts a bracketed IPv6 address. -func splitRemote(arg string) (dest, path string, remote bool) { +func splitHostPath(arg string) (dest, path string, remote bool) { if strings.HasPrefix(arg, "/") || strings.HasPrefix(arg, ".") { return "", "", false } diff --git a/internal/ssh/client_test.go b/internal/ssh/client_test.go index 3825a0a..2edf3d7 100644 --- a/internal/ssh/client_test.go +++ b/internal/ssh/client_test.go @@ -154,3 +154,27 @@ func TestApplyInsertsOptionsBeforeDoubleDash(t *testing.T) { t.Fatalf("got %v, want %v", got, want) } } + +// Instance-qualified hosts contain "/" before the ":" of host:path; only +// configured instances make such an argument remote. +func TestInstanceQualifiedHostPath(t *testing.T) { + scp := SCP.WithInstances([]string{"prod"}) + ops, err := scp.Operands([]string{"a.txt", "ops@prod/web01:/tmp/", "dir/file:x"}) + if err != nil { + t.Fatal(err) + } + if want := []Operand{{Idx: 1, User: "ops", Host: "prod/web01", Path: "/tmp/"}}; !reflect.DeepEqual(ops, want) { + t.Fatalf("got %+v", ops) + } + got := scp.Apply([]string{"a.txt", "ops@prod/web01:/tmp/"}, []int{1}, Host{Name: "web01", IP: "192.0.2.1"}, config.SSHOptions{}, WrapperFlags{}).Args + if want := []string{"a.txt", "ops@192.0.2.1:/tmp/"}; !reflect.DeepEqual(got, want) { + t.Fatalf("apply: %v", got) + } + sftp := SFTP.WithInstances([]string{"prod"}) + if p := sftp.Parse([]string{"prod/web01:/var/log"}); p == nil || p.Host != "prod/web01" || p.Path != "/var/log" { + t.Fatalf("sftp: %+v", p) + } + if ops, _ := SCP.Operands([]string{"prod/web01:/tmp/", "."}); len(ops) != 0 { + t.Fatalf("unknown instance must stay local: %+v", ops) + } +}