From 75c2729b2b3c3239837d8422165188efb1667782 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20K=C3=A4stner?= Date: Wed, 27 May 2026 18:51:30 +0200 Subject: [PATCH 1/6] Implement NX-OS reprovisioning via NX-API MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the defunct GNMI-based reprovisioning with NXAPI JSON-RPC calls. The new implementation issues two requests: 1. boot poap enable + copy running-config startup-config (batched with stop-on-error rollback) 2. reload (separate request that tolerates transport errors because the device goes down before responding) Add nxapi.IsTransportError to distinguish network-level errors (EOF, timeout, connection reset) from logical errors (RPCError, HTTPError), enabling callers of disruptive commands to tolerate expected connection drops. Remove the now unused BootPOAP GNMI type. Signed-off-by: Felix Kästner --- internal/provider/cisco/nxos/provider.go | 43 +++--- .../provider/cisco/nxos/reprovision_test.go | 122 ++++++++++++++++++ internal/provider/cisco/nxos/system.go | 8 -- internal/transport/nxapi/nxapi.go | 17 +++ internal/transport/nxapi/nxapi_test.go | 81 ++++++++++++ 5 files changed, 246 insertions(+), 25 deletions(-) create mode 100644 internal/provider/cisco/nxos/reprovision_test.go diff --git a/internal/provider/cisco/nxos/provider.go b/internal/provider/cisco/nxos/provider.go index 29f91d86f..f4368bd37 100644 --- a/internal/provider/cisco/nxos/provider.go +++ b/internal/provider/cisco/nxos/provider.go @@ -76,13 +76,14 @@ type Provider struct { nxapi *nxapi.Client } +// timeout is the default timeout for all HTTP/gRPC requests made by the provider. +const timeout = 30 * time.Second + func NewProvider() provider.Provider { return &Provider{} } func (p *Provider) Connect(ctx context.Context, conn *deviceutil.Connection) (err error) { - // timeout is the default timeout for all HTTP/gRPC requests made by the provider. - const timeout = 30 * time.Second p.conn, err = grpcext.NewClient(conn, grpcext.WithDefaultTimeout(timeout)) if err != nil { return fmt.Errorf("failed to create grpc connection: %w", err) @@ -147,23 +148,31 @@ func (p *Provider) FactoryReset(ctx context.Context, conn *deviceutil.Connection return FactoryReset(ctx, p.conn) } -func (p *Provider) Reprovision(ctx context.Context, conn *deviceutil.Connection) (reterr error) { - if err := p.Connect(ctx, conn); err != nil { - return err +func (p *Provider) Reprovision(ctx context.Context, conn *deviceutil.Connection) error { + c := *conn + c.Address = netip.MustParseAddrPort(conn.Address).Addr().String() + client, err := nxapi.NewClient(&c, timeout) + if err != nil { + return fmt.Errorf("failed to create nxapi client: %w", err) } - defer func() { - if err := p.Disconnect(ctx, conn); err != nil { - reterr = errors.Join(reterr, err) - } - }() - // This is currently defunct on NX-OS, as enabling POAP requires a `copy running-config startup-config` which we - // cannot issue via GNMI - // TODO add once NXAPI client is available - poap := BootPOAP("enable") - if err := p.client.Update(ctx, &poap); err != nil { - return err + + _, err = client.Do(ctx, nxapi.NewRequest( + "boot poap enable", + "copy running-config startup-config", + ).WithRollback(nxapi.Stop)) + if err != nil { + return fmt.Errorf("failed to prepare device for reprovisioning: %w", err) } - return Reboot(ctx, p.conn) + + // Reboot is issued as a separate request because it actually restarts + // the device. The connection will drop before a response is received, + // so transport errors are expected and tolerated. + _, err = client.Do(ctx, nxapi.NewRequest("reload")) + if err != nil && !nxapi.IsTransportError(err) { + return fmt.Errorf("failed to reboot device: %w", err) + } + + return nil } func (p *Provider) ListPorts(ctx context.Context) ([]provider.DevicePort, error) { diff --git a/internal/provider/cisco/nxos/reprovision_test.go b/internal/provider/cisco/nxos/reprovision_test.go new file mode 100644 index 000000000..6c9dba26e --- /dev/null +++ b/internal/provider/cisco/nxos/reprovision_test.go @@ -0,0 +1,122 @@ +// SPDX-FileCopyrightText: 2026 SAP SE or an SAP affiliate company and IronCore contributors +// SPDX-License-Identifier: Apache-2.0 + +package nxos + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "slices" + "testing" + + "github.com/ironcore-dev/network-operator/internal/deviceutil" +) + +func TestReprovision(t *testing.T) { + t.Run("success with connection drop on reload", func(t *testing.T) { + var requests [][]string + called := 0 + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var cmds []struct { + Params struct { + Cmd string `json:"cmd"` + } `json:"params"` + } + if err := json.NewDecoder(r.Body).Decode(&cmds); err != nil { + t.Fatalf("failed to decode request: %v", err) + } + + batch := make([]string, len(cmds)) + for i, c := range cmds { + batch[i] = c.Params.Cmd + } + requests = append(requests, batch) + called++ + + if called == 1 { + w.Header().Set("Content-Type", "application/json-rpc") + fmt.Fprint(w, `[ + {"jsonrpc":"2.0","result":null,"id":1}, + {"jsonrpc":"2.0","result":null,"id":2} + ]`) + return + } + + // Simulate device going down by closing connection abruptly. + hijacker, ok := w.(http.Hijacker) + if !ok { + t.Fatal("server does not support hijacking") + } + conn, _, err := hijacker.Hijack() + if err != nil { + t.Fatalf("hijack failed: %v", err) + } + conn.Close() + })) + defer srv.Close() + + p := &Provider{} + conn := &deviceutil.Connection{Address: srv.Listener.Addr().String(), Username: "admin", Password: "secret"} + + err := p.Reprovision(t.Context(), conn) + if err != nil { + t.Fatalf("Reprovision returned unexpected error: %v", err) + } + + if len(requests) != 2 { + t.Fatalf("expected 2 NXAPI requests, got %d", len(requests)) + } + if !slices.Equal(requests[0], []string{"boot poap enable", "copy running-config startup-config"}) { + t.Errorf("prep batch = %v", requests[0]) + } + if !slices.Equal(requests[1], []string{"reload"}) { + t.Errorf("reload request = %v", requests[1]) + } + }) + + t.Run("prep batch RPC error fails", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json-rpc") + w.WriteHeader(http.StatusBadRequest) + fmt.Fprint(w, `[{"jsonrpc":"2.0","error":{"code":-32602,"message":"Invalid command"},"id":1}]`) + })) + defer srv.Close() + + p := &Provider{} + conn := &deviceutil.Connection{Address: srv.Listener.Addr().String(), Username: "admin", Password: "secret"} + + err := p.Reprovision(t.Context(), conn) + if err == nil { + t.Fatal("expected error from prep batch, got nil") + } + }) + + t.Run("reload RPC error fails", func(t *testing.T) { + called := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + called++ + w.Header().Set("Content-Type", "application/json-rpc") + if called == 1 { + fmt.Fprint(w, `[ + {"jsonrpc":"2.0","result":null,"id":1}, + {"jsonrpc":"2.0","result":null,"id":2} + ]`) + return + } + w.WriteHeader(http.StatusBadRequest) + fmt.Fprint(w, `[{"jsonrpc":"2.0","error":{"code":-32602,"message":"Permission denied"},"id":1}]`) + })) + defer srv.Close() + + p := &Provider{} + conn := &deviceutil.Connection{Address: srv.Listener.Addr().String(), Username: "admin", Password: "secret"} + + err := p.Reprovision(t.Context(), conn) + if err == nil { + t.Fatal("expected error from reload RPC failure, got nil") + } + }) +} diff --git a/internal/provider/cisco/nxos/system.go b/internal/provider/cisco/nxos/system.go index 009c22f1d..b1d8999a7 100644 --- a/internal/provider/cisco/nxos/system.go +++ b/internal/provider/cisco/nxos/system.go @@ -68,14 +68,6 @@ func (*FirmwareVersion) XPath() string { return "System/showversion-items/nxosVersion" } -var _ gnmiext.DataElement = (*BootPOAP)(nil) - -type BootPOAP string - -func (*BootPOAP) XPath() string { - return "/System/boot-items/poap" -} - type BootTime UnixTime func (*BootTime) XPath() string { diff --git a/internal/transport/nxapi/nxapi.go b/internal/transport/nxapi/nxapi.go index 6606820b9..cb5f88301 100644 --- a/internal/transport/nxapi/nxapi.go +++ b/internal/transport/nxapi/nxapi.go @@ -10,6 +10,7 @@ import ( "errors" "fmt" "io" + "net" "net/http" "net/url" "time" @@ -263,3 +264,19 @@ type HTTPError struct { func (e *HTTPError) Error() string { return fmt.Sprintf("nxapi: non-2xx status code: %d - %s", e.Code, string(e.Body)) } + +// IsTransportError reports whether err is a network-level transport error +// (connection reset, timeout, EOF) as opposed to a logical error returned +// by the NX-API endpoint (RPCError, HTTPError). This is useful for callers +// that issue disruptive commands (e.g. reboot) where the device going down +// mid-request is expected. +func IsTransportError(err error) bool { + if err == nil { + return false + } + if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) { + return true + } + var netErr net.Error + return errors.As(err, &netErr) +} diff --git a/internal/transport/nxapi/nxapi_test.go b/internal/transport/nxapi/nxapi_test.go index 0b6c73a32..53e17f25b 100644 --- a/internal/transport/nxapi/nxapi_test.go +++ b/internal/transport/nxapi/nxapi_test.go @@ -9,8 +9,10 @@ import ( "encoding/json" "errors" "fmt" + "io" "net/http" "net/http/httptest" + "net/url" "strings" "testing" @@ -295,3 +297,82 @@ func TestDo(t *testing.T) { }) } } + +func TestIsTransportError(t *testing.T) { + tests := []struct { + desc string + err error + want bool + }{ + { + desc: "nil is not a transport error", + err: nil, + want: false, + }, + { + desc: "RPCError is not a transport error", + err: &RPCError{Code: -32602, Message: "Invalid params"}, + want: false, + }, + { + desc: "RPCErrors is not a transport error", + err: RPCErrors{&RPCError{Code: -32602, Message: "Invalid params"}}, + want: false, + }, + { + desc: "HTTPError is not a transport error", + err: &HTTPError{Code: 401, Body: []byte("unauthorized")}, + want: false, + }, + { + desc: "generic error is not a transport error", + err: errors.New("some logic error"), + want: false, + }, + { + desc: "io.EOF is a transport error", + err: io.EOF, + want: true, + }, + { + desc: "io.ErrUnexpectedEOF is a transport error", + err: io.ErrUnexpectedEOF, + want: true, + }, + { + desc: "wrapped io.EOF is a transport error", + err: fmt.Errorf("request failed: %w", io.EOF), + want: true, + }, + { + desc: "net.Error is a transport error", + err: &netError{msg: "i/o timeout"}, + want: true, + }, + { + desc: "url.Error wrapping net.Error is a transport error", + err: &url.Error{Op: "Post", URL: "http://x/ins", Err: &netError{msg: "i/o timeout"}}, + want: true, + }, + { + desc: "wrapped net.Error is a transport error", + err: fmt.Errorf("read tcp: %w", &netError{msg: "connection reset by peer"}), + want: true, + }, + } + for _, test := range tests { + t.Run(test.desc, func(t *testing.T) { + got := IsTransportError(test.err) + if got != test.want { + t.Errorf("IsTransportError(%v) = %t, want %t", test.err, got, test.want) + } + }) + } +} + +// netError is a mock net.Error for testing. +type netError struct{ msg string } + +func (e *netError) Error() string { return e.msg } +func (e *netError) Timeout() bool { return false } +func (e *netError) Temporary() bool { return false } From 4477bd731f18b1e4061aa7593a3784695b676ec4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20K=C3=A4stner?= Date: Fri, 19 Jun 2026 14:13:12 +0200 Subject: [PATCH 2/6] Fix maintenance annotation removal on failed operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the maintenance annotation deletion to after the switch statement so it is only removed when the action succeeds. This allows the reconciler to retry failed maintenance operations on the next reconciliation instead of requiring the user to re-apply the annotation. Additionally, return a terminal error for unknown maintenance actions since retrying will never succeed. Signed-off-by: Felix Kästner --- internal/controller/core/device_controller.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/controller/core/device_controller.go b/internal/controller/core/device_controller.go index b3dfc8267..52940f348 100644 --- a/internal/controller/core/device_controller.go +++ b/internal/controller/core/device_controller.go @@ -427,8 +427,6 @@ func (r *DeviceReconciler) reconcileMaintenance(ctx context.Context, obj *v1alph if !ok { return nil } - delete(obj.Annotations, v1alpha1.DeviceMaintenanceAnnotation) - switch action { case v1alpha1.DeviceMaintenanceReboot: prov, ok := r.Provider().(provider.MaintenanceProvider) @@ -501,9 +499,12 @@ func (r *DeviceReconciler) reconcileMaintenance(ctx context.Context, obj *v1alph default: r.Recorder.Eventf(obj, nil, "Warning", "UnknownMaintenanceAction", "Maintenance", "Unknown maintenance action: %s", action) - return fmt.Errorf("unknown maintenance action: %s", action) + return reconcile.TerminalError(fmt.Errorf("unknown maintenance action: %s", action)) } + // Only remove the annotation after the operation succeeds so that + // failed actions are retried on the next reconciliation. + delete(obj.Annotations, v1alpha1.DeviceMaintenanceAnnotation) return nil } From c8babcd66c18172ebc2de657d93665b7f508f950 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20K=C3=A4stner?= Date: Tue, 23 Jun 2026 14:23:30 +0200 Subject: [PATCH 3/6] Add WithPort option to NX-API client for testability MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a functional option to nxapi.NewClient that overrides the port in the connection address. This allows tests to point the client at an httptest server on a random port. Reprovision now reuses p.nxapi if already set (e.g. from Connect or injected by a test), falling back to creating a standalone client with port stripping for production. Signed-off-by: Felix Kästner --- internal/provider/cisco/nxos/provider.go | 16 ++++-- .../provider/cisco/nxos/reprovision_test.go | 32 ++++++++--- internal/transport/nxapi/nxapi.go | 55 +++++++++++++------ internal/transport/nxapi/nxapi_test.go | 13 +++-- 4 files changed, 79 insertions(+), 37 deletions(-) diff --git a/internal/provider/cisco/nxos/provider.go b/internal/provider/cisco/nxos/provider.go index f4368bd37..fe9bd24fe 100644 --- a/internal/provider/cisco/nxos/provider.go +++ b/internal/provider/cisco/nxos/provider.go @@ -149,14 +149,18 @@ func (p *Provider) FactoryReset(ctx context.Context, conn *deviceutil.Connection } func (p *Provider) Reprovision(ctx context.Context, conn *deviceutil.Connection) error { - c := *conn - c.Address = netip.MustParseAddrPort(conn.Address).Addr().String() - client, err := nxapi.NewClient(&c, timeout) - if err != nil { - return fmt.Errorf("failed to create nxapi client: %w", err) + client := p.nxapi + if client == nil { + c := *conn + c.Address = netip.MustParseAddrPort(conn.Address).Addr().String() + var err error + client, err = nxapi.NewClient(&c, timeout) + if err != nil { + return fmt.Errorf("failed to create nxapi client: %w", err) + } } - _, err = client.Do(ctx, nxapi.NewRequest( + _, err := client.Do(ctx, nxapi.NewRequest( "boot poap enable", "copy running-config startup-config", ).WithRollback(nxapi.Stop)) diff --git a/internal/provider/cisco/nxos/reprovision_test.go b/internal/provider/cisco/nxos/reprovision_test.go index 6c9dba26e..510d4a728 100644 --- a/internal/provider/cisco/nxos/reprovision_test.go +++ b/internal/provider/cisco/nxos/reprovision_test.go @@ -6,12 +6,14 @@ package nxos import ( "encoding/json" "fmt" + "net" "net/http" "net/http/httptest" "slices" "testing" "github.com/ironcore-dev/network-operator/internal/deviceutil" + "github.com/ironcore-dev/network-operator/internal/transport/nxapi" ) func TestReprovision(t *testing.T) { @@ -58,11 +60,15 @@ func TestReprovision(t *testing.T) { })) defer srv.Close() - p := &Provider{} + _, port, _ := net.SplitHostPort(srv.Listener.Addr().String()) //nolint:errcheck // httptest address is always host:port conn := &deviceutil.Connection{Address: srv.Listener.Addr().String(), Username: "admin", Password: "secret"} - - err := p.Reprovision(t.Context(), conn) + client, err := nxapi.NewClient(conn, 0, nxapi.WithPort(port)) if err != nil { + t.Fatalf("failed to create nxapi client: %v", err) + } + + p := &Provider{nxapi: client} + if err := p.Reprovision(t.Context(), conn); err != nil { t.Fatalf("Reprovision returned unexpected error: %v", err) } @@ -85,11 +91,15 @@ func TestReprovision(t *testing.T) { })) defer srv.Close() - p := &Provider{} + _, port, _ := net.SplitHostPort(srv.Listener.Addr().String()) //nolint:errcheck // httptest address is always host:port conn := &deviceutil.Connection{Address: srv.Listener.Addr().String(), Username: "admin", Password: "secret"} + client, err := nxapi.NewClient(conn, 0, nxapi.WithPort(port)) + if err != nil { + t.Fatalf("failed to create nxapi client: %v", err) + } - err := p.Reprovision(t.Context(), conn) - if err == nil { + p := &Provider{nxapi: client} + if err := p.Reprovision(t.Context(), conn); err == nil { t.Fatal("expected error from prep batch, got nil") } }) @@ -111,11 +121,15 @@ func TestReprovision(t *testing.T) { })) defer srv.Close() - p := &Provider{} + _, port, _ := net.SplitHostPort(srv.Listener.Addr().String()) //nolint:errcheck // httptest address is always host:port conn := &deviceutil.Connection{Address: srv.Listener.Addr().String(), Username: "admin", Password: "secret"} + client, err := nxapi.NewClient(conn, 0, nxapi.WithPort(port)) + if err != nil { + t.Fatalf("failed to create nxapi client: %v", err) + } - err := p.Reprovision(t.Context(), conn) - if err == nil { + p := &Provider{nxapi: client} + if err := p.Reprovision(t.Context(), conn); err == nil { t.Fatal("expected error from reload RPC failure, got nil") } }) diff --git a/internal/transport/nxapi/nxapi.go b/internal/transport/nxapi/nxapi.go index cb5f88301..869f6faed 100644 --- a/internal/transport/nxapi/nxapi.go +++ b/internal/transport/nxapi/nxapi.go @@ -33,38 +33,61 @@ func (f RoundTripFunc) RoundTrip(r *http.Request) (*http.Response, error) { // Use [NewClient] to construct one. type Client struct { client *http.Client - uri string + url url.URL +} + +// Option configures a [Client]. +type Option func(*Client) error + +// WithPort overrides the port in the connection address. +// This is useful when NX-API is reachable on a different +// port (e.g. 8443) than the default (80/443). +func WithPort(port string) Option { + return func(c *Client) error { + host, _, err := net.SplitHostPort(c.url.Host) + if err != nil { + return fmt.Errorf("nxapi: failed to parse address: %w", err) + } + c.url.Host = net.JoinHostPort(host, port) + return nil + } +} + } // NewClient creates a new [Client] for the given connection. // If the connection has a TLS configuration set, HTTPS is used; otherwise HTTP. -// The underlying HTTP client uses timeout for all requests; a value of 0 -// means no timeout. -func NewClient(c *deviceutil.Connection, timeout time.Duration) (*Client, error) { +func NewClient(conn *deviceutil.Connection, timeout time.Duration, opts ...Option) (*Client, error) { proto := "http" - if c.TLS != nil { + if conn.TLS != nil { proto = "https" } - uri, err := url.JoinPath(proto+"://"+c.Address, "ins") - if err != nil { - return nil, fmt.Errorf("nxapi: failed to join path: %w", err) - } transport := http.DefaultTransport.(*http.Transport).Clone() - if c.TLS != nil { - transport.TLSClientConfig = c.TLS + if conn.TLS != nil { + transport.TLSClientConfig = conn.TLS } - return &Client{ + c := &Client{ client: &http.Client{ Transport: RoundTripFunc(func(r *http.Request) (*http.Response, error) { r.Header.Set("Content-Type", "application/json-rpc") r.Header.Set("Cache-Control", "no-cache") - r.SetBasicAuth(c.Username, c.Password) + r.SetBasicAuth(conn.Username, conn.Password) return transport.RoundTrip(r) }), Timeout: timeout, }, - uri: uri, - }, nil + url: url.URL{ + Scheme: proto, + Host: conn.Address, + Path: "/ins", + }, + } + for _, opt := range opts { + if err := opt(c); err != nil { + return nil, err + } + } + return c, nil } // Do sends a Request to the device and returns one [json.RawMessage] per @@ -77,7 +100,7 @@ func (c *Client) Do(ctx context.Context, r Request) ([]json.RawMessage, error) { return nil, fmt.Errorf("nxapi: failed to encode request: %w", err) } - req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.uri, bytes.NewReader(b)) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.url.String(), bytes.NewReader(b)) if err != nil { return nil, fmt.Errorf("nxapi: failed to create request: %w", err) } diff --git a/internal/transport/nxapi/nxapi_test.go b/internal/transport/nxapi/nxapi_test.go index 53e17f25b..d9d7a037a 100644 --- a/internal/transport/nxapi/nxapi_test.go +++ b/internal/transport/nxapi/nxapi_test.go @@ -13,7 +13,6 @@ import ( "net/http" "net/http/httptest" "net/url" - "strings" "testing" "github.com/ironcore-dev/network-operator/internal/deviceutil" @@ -42,12 +41,14 @@ func TestUri(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - wantPrefix := test.wantProto + "://" - if !strings.HasPrefix(c.uri, wantPrefix) { - t.Errorf("uri = %q, want prefix %q", c.uri, wantPrefix) + if c.url.Scheme != test.wantProto { + t.Errorf("scheme = %q, want %q", c.url.Scheme, test.wantProto) } - if !strings.HasSuffix(c.uri, "/ins") { - t.Errorf("uri = %q, want suffix %q", c.uri, "/ins") + if c.url.Host != test.conn.Address { + t.Errorf("host = %q, want %q", c.url.Host, test.conn.Address) + } + if c.url.Path != "/ins" { + t.Errorf("path = %q, want %q", c.url.Path, "/ins") } }) } From d008ee67fba3be9ca63604e432f6c6521577f4f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20K=C3=A4stner?= Date: Fri, 17 Jul 2026 16:51:17 +0200 Subject: [PATCH 4/6] Make timeout a functional option on NX-API client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the positional timeout parameter in NewClient with a WithTimeout option, consistent with WithPort. The default is 0 (no timeout). Signed-off-by: Felix Kästner --- hack/nxapi/main.go | 2 +- internal/provider/cisco/nxos/provider.go | 4 ++-- internal/provider/cisco/nxos/reprovision_test.go | 6 +++--- internal/transport/nxapi/nxapi.go | 10 ++++++++-- internal/transport/nxapi/nxapi_test.go | 4 ++-- 5 files changed, 16 insertions(+), 10 deletions(-) diff --git a/hack/nxapi/main.go b/hack/nxapi/main.go index 2171c3d27..b4336de96 100644 --- a/hack/nxapi/main.go +++ b/hack/nxapi/main.go @@ -66,7 +66,7 @@ func main() { ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt /* == syscall.SIGINT */, syscall.SIGTERM) defer cancel() - c, err := nxapi.NewClient(conn, 0) + c, err := nxapi.NewClient(conn) if err != nil { fmt.Fprintf(os.Stderr, "error: %v\n", err) return diff --git a/internal/provider/cisco/nxos/provider.go b/internal/provider/cisco/nxos/provider.go index fe9bd24fe..d3bd663de 100644 --- a/internal/provider/cisco/nxos/provider.go +++ b/internal/provider/cisco/nxos/provider.go @@ -99,7 +99,7 @@ func (p *Provider) Connect(ctx context.Context, conn *deviceutil.Connection) (er // NXAPI only uses the address for URI construction. c := *conn c.Address = netip.MustParseAddrPort(conn.Address).Addr().String() - p.nxapi, err = nxapi.NewClient(&c, timeout) + p.nxapi, err = nxapi.NewClient(&c, nxapi.WithTimeout(timeout)) if err != nil { return fmt.Errorf("failed to create nxapi client: %w", err) } @@ -154,7 +154,7 @@ func (p *Provider) Reprovision(ctx context.Context, conn *deviceutil.Connection) c := *conn c.Address = netip.MustParseAddrPort(conn.Address).Addr().String() var err error - client, err = nxapi.NewClient(&c, timeout) + client, err = nxapi.NewClient(&c, nxapi.WithTimeout(timeout)) if err != nil { return fmt.Errorf("failed to create nxapi client: %w", err) } diff --git a/internal/provider/cisco/nxos/reprovision_test.go b/internal/provider/cisco/nxos/reprovision_test.go index 510d4a728..57fe78bea 100644 --- a/internal/provider/cisco/nxos/reprovision_test.go +++ b/internal/provider/cisco/nxos/reprovision_test.go @@ -62,7 +62,7 @@ func TestReprovision(t *testing.T) { _, port, _ := net.SplitHostPort(srv.Listener.Addr().String()) //nolint:errcheck // httptest address is always host:port conn := &deviceutil.Connection{Address: srv.Listener.Addr().String(), Username: "admin", Password: "secret"} - client, err := nxapi.NewClient(conn, 0, nxapi.WithPort(port)) + client, err := nxapi.NewClient(conn, nxapi.WithPort(port)) if err != nil { t.Fatalf("failed to create nxapi client: %v", err) } @@ -93,7 +93,7 @@ func TestReprovision(t *testing.T) { _, port, _ := net.SplitHostPort(srv.Listener.Addr().String()) //nolint:errcheck // httptest address is always host:port conn := &deviceutil.Connection{Address: srv.Listener.Addr().String(), Username: "admin", Password: "secret"} - client, err := nxapi.NewClient(conn, 0, nxapi.WithPort(port)) + client, err := nxapi.NewClient(conn, nxapi.WithPort(port)) if err != nil { t.Fatalf("failed to create nxapi client: %v", err) } @@ -123,7 +123,7 @@ func TestReprovision(t *testing.T) { _, port, _ := net.SplitHostPort(srv.Listener.Addr().String()) //nolint:errcheck // httptest address is always host:port conn := &deviceutil.Connection{Address: srv.Listener.Addr().String(), Username: "admin", Password: "secret"} - client, err := nxapi.NewClient(conn, 0, nxapi.WithPort(port)) + client, err := nxapi.NewClient(conn, nxapi.WithPort(port)) if err != nil { t.Fatalf("failed to create nxapi client: %v", err) } diff --git a/internal/transport/nxapi/nxapi.go b/internal/transport/nxapi/nxapi.go index 869f6faed..f5be03fb6 100644 --- a/internal/transport/nxapi/nxapi.go +++ b/internal/transport/nxapi/nxapi.go @@ -53,11 +53,18 @@ func WithPort(port string) Option { } } +// WithTimeout sets the HTTP client timeout. +// The default is 0 (no timeout). +func WithTimeout(d time.Duration) Option { + return func(c *Client) error { + c.client.Timeout = d + return nil + } } // NewClient creates a new [Client] for the given connection. // If the connection has a TLS configuration set, HTTPS is used; otherwise HTTP. -func NewClient(conn *deviceutil.Connection, timeout time.Duration, opts ...Option) (*Client, error) { +func NewClient(conn *deviceutil.Connection, opts ...Option) (*Client, error) { proto := "http" if conn.TLS != nil { proto = "https" @@ -74,7 +81,6 @@ func NewClient(conn *deviceutil.Connection, timeout time.Duration, opts ...Optio r.SetBasicAuth(conn.Username, conn.Password) return transport.RoundTrip(r) }), - Timeout: timeout, }, url: url.URL{ Scheme: proto, diff --git a/internal/transport/nxapi/nxapi_test.go b/internal/transport/nxapi/nxapi_test.go index d9d7a037a..5deaf8f58 100644 --- a/internal/transport/nxapi/nxapi_test.go +++ b/internal/transport/nxapi/nxapi_test.go @@ -37,7 +37,7 @@ func TestUri(t *testing.T) { } for _, test := range tests { t.Run(test.desc, func(t *testing.T) { - c, err := NewClient(test.conn, 0) + c, err := NewClient(test.conn) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -258,7 +258,7 @@ func TestDo(t *testing.T) { Username: "admin", Password: "secret", } - c, err := NewClient(conn, 0) + c, err := NewClient(conn) if err != nil { t.Fatalf("unexpected error: %v", err) } From 4fe2bf1fac6cac977bf6f93e31dbccead9f5a54a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20K=C3=A4stner?= Date: Fri, 17 Jul 2026 19:31:10 +0200 Subject: [PATCH 5/6] Add Connect/Disconnect to reconcileMaintenance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Maintenance operations (reboot, factory reset, reprovision) require an established provider connection. Previously they were called without Connect, which meant Reboot and FactoryReset would use a nil gRPC connection and Reprovision had to create its own NX-API client as a workaround. Wrap device-targeting maintenance actions in the same Connect/Disconnect pattern used by reconcile and reconcileMinimal. Signed-off-by: Felix Kästner --- internal/controller/core/device_controller.go | 137 ++++++++++-------- 1 file changed, 75 insertions(+), 62 deletions(-) diff --git a/internal/controller/core/device_controller.go b/internal/controller/core/device_controller.go index 52940f348..9fa361d08 100644 --- a/internal/controller/core/device_controller.go +++ b/internal/controller/core/device_controller.go @@ -427,69 +427,8 @@ func (r *DeviceReconciler) reconcileMaintenance(ctx context.Context, obj *v1alph if !ok { return nil } - switch action { - case v1alpha1.DeviceMaintenanceReboot: - prov, ok := r.Provider().(provider.MaintenanceProvider) - if !ok { - r.Recorder.Eventf(obj, nil, "Warning", "MaintenanceUnsupported", "Maintenance", "Provider does not support maintenance operation: %s", action) - return nil - } - // Reboot triggers a device restart. The device remains in its current phase - // and will resume normal operation after the reboot completes. - r.Recorder.Eventf(obj, nil, "Normal", "RebootRequested", "Maintenance", "Device reboot has been requested") - if err := prov.Reboot(ctx, conn); err != nil { - conditions.Set(obj, metav1.Condition{ - Type: v1alpha1.ReadyCondition, - Status: metav1.ConditionFalse, - Reason: v1alpha1.MaintenanceFailedReason, - Message: fmt.Sprintf("Failed to reboot device: %v", err), - }) - r.Recorder.Eventf(obj, nil, "Warning", "RebootFailed", "Maintenance", "Device reboot has failed: %v", err) - return fmt.Errorf("failed to reboot device: %w", err) - } - - case v1alpha1.DeviceMaintenanceFactoryReset: - prov, ok := r.Provider().(provider.MaintenanceProvider) - if !ok { - r.Recorder.Eventf(obj, nil, "Warning", "MaintenanceUnsupported", "Maintenance", "Provider does not support maintenance operation: %s", action) - return nil - } - // FactoryReset erases all device configuration and returns it to its original state. - // After completion, the device phase is reset to Pending to restart the lifecycle. - r.Recorder.Eventf(obj, nil, "Normal", "FactoryResetRequested", "Maintenance", "Device factory reset has been requested") - if err := prov.FactoryReset(ctx, conn); err != nil { - conditions.Set(obj, metav1.Condition{ - Type: v1alpha1.ReadyCondition, - Status: metav1.ConditionFalse, - Reason: v1alpha1.MaintenanceFailedReason, - Message: fmt.Sprintf("Failed to factory reset device: %v", err), - }) - r.Recorder.Eventf(obj, nil, "Warning", "FactoryResetFailed", "Maintenance", "Device factory reset has failed: %v", err) - return fmt.Errorf("failed to reset device to factory defaults: %w", err) - } - obj.Status.Phase = v1alpha1.DevicePhasePending - - case v1alpha1.DeviceMaintenanceReprovision: - prov, ok := r.Provider().(provider.ProvisioningProvider) - if !ok { - r.Recorder.Eventf(obj, nil, "Warning", "MaintenanceUnsupported", "Maintenance", "Provider does not support provisioning operation: %s", action) - return nil - } - // Reprovision prepares the device for re-provisioning without a full factory reset. - // The provider initiates the provisioning process, then the phase is reset to Pending. - r.Recorder.Eventf(obj, nil, "Normal", "ReprovisionRequested", "Maintenance", "Device reprovisioning has been requested") - if err := prov.Reprovision(ctx, conn); err != nil { - conditions.Set(obj, metav1.Condition{ - Type: v1alpha1.ReadyCondition, - Status: metav1.ConditionFalse, - Reason: v1alpha1.MaintenanceFailedReason, - Message: fmt.Sprintf("Failed to prepare device for reprovisioning: %v", err), - }) - r.Recorder.Eventf(obj, nil, "Warning", "ReprovisionFailed", "Maintenance", "Device reprovisioning preparation has failed: %v", err) - return fmt.Errorf("failed to prepare device for reprovisioning: %w", err) - } - obj.Status.Phase = v1alpha1.DevicePhasePending + switch action { case v1alpha1.DeviceMaintenanceResetPhase: // Reset phase is a soft reset that only changes the device phase to Pending without // performing any device-side operations. This is useful for recovering from terminal @@ -497,6 +436,80 @@ func (r *DeviceReconciler) reconcileMaintenance(ctx context.Context, obj *v1alph r.Recorder.Eventf(obj, nil, "Normal", "PhaseReset", "Maintenance", "Device phase has been reset to Pending") obj.Status.Phase = v1alpha1.DevicePhasePending + case v1alpha1.DeviceMaintenanceReboot, + v1alpha1.DeviceMaintenanceFactoryReset, + v1alpha1.DeviceMaintenanceReprovision: + + prov := r.Provider() + if err := prov.Connect(ctx, conn); err != nil { + return fmt.Errorf("failed to connect to device: %w", err) + } + defer prov.Disconnect(ctx, conn) //nolint:errcheck + + switch action { + case v1alpha1.DeviceMaintenanceReboot: + mp, ok := prov.(provider.MaintenanceProvider) + if !ok { + r.Recorder.Eventf(obj, nil, "Warning", "MaintenanceUnsupported", "Maintenance", "Provider does not support maintenance operation: %s", action) + return nil + } + // Reboot triggers a device restart. The device remains in its current phase + // and will resume normal operation after the reboot completes. + r.Recorder.Eventf(obj, nil, "Normal", "RebootRequested", "Maintenance", "Device reboot has been requested") + if err := mp.Reboot(ctx, conn); err != nil { + conditions.Set(obj, metav1.Condition{ + Type: v1alpha1.ReadyCondition, + Status: metav1.ConditionFalse, + Reason: v1alpha1.MaintenanceFailedReason, + Message: fmt.Sprintf("Failed to reboot device: %v", err), + }) + r.Recorder.Eventf(obj, nil, "Warning", "RebootFailed", "Maintenance", "Device reboot has failed: %v", err) + return fmt.Errorf("failed to reboot device: %w", err) + } + + case v1alpha1.DeviceMaintenanceFactoryReset: + mp, ok := prov.(provider.MaintenanceProvider) + if !ok { + r.Recorder.Eventf(obj, nil, "Warning", "MaintenanceUnsupported", "Maintenance", "Provider does not support maintenance operation: %s", action) + return nil + } + // FactoryReset erases all device configuration and returns it to its original state. + // After completion, the device phase is reset to Pending to restart the lifecycle. + r.Recorder.Eventf(obj, nil, "Normal", "FactoryResetRequested", "Maintenance", "Device factory reset has been requested") + if err := mp.FactoryReset(ctx, conn); err != nil { + conditions.Set(obj, metav1.Condition{ + Type: v1alpha1.ReadyCondition, + Status: metav1.ConditionFalse, + Reason: v1alpha1.MaintenanceFailedReason, + Message: fmt.Sprintf("Failed to factory reset device: %v", err), + }) + r.Recorder.Eventf(obj, nil, "Warning", "FactoryResetFailed", "Maintenance", "Device factory reset has failed: %v", err) + return fmt.Errorf("failed to reset device to factory defaults: %w", err) + } + obj.Status.Phase = v1alpha1.DevicePhasePending + + case v1alpha1.DeviceMaintenanceReprovision: + pp, ok := prov.(provider.ProvisioningProvider) + if !ok { + r.Recorder.Eventf(obj, nil, "Warning", "MaintenanceUnsupported", "Maintenance", "Provider does not support provisioning operation: %s", action) + return nil + } + // Reprovision prepares the device for re-provisioning without a full factory reset. + // The provider initiates the provisioning process, then the phase is reset to Pending. + r.Recorder.Eventf(obj, nil, "Normal", "ReprovisionRequested", "Maintenance", "Device reprovisioning has been requested") + if err := pp.Reprovision(ctx, conn); err != nil { + conditions.Set(obj, metav1.Condition{ + Type: v1alpha1.ReadyCondition, + Status: metav1.ConditionFalse, + Reason: v1alpha1.MaintenanceFailedReason, + Message: fmt.Sprintf("Failed to prepare device for reprovisioning: %v", err), + }) + r.Recorder.Eventf(obj, nil, "Warning", "ReprovisionFailed", "Maintenance", "Device reprovisioning preparation has failed: %v", err) + return fmt.Errorf("failed to prepare device for reprovisioning: %w", err) + } + obj.Status.Phase = v1alpha1.DevicePhasePending + } + default: r.Recorder.Eventf(obj, nil, "Warning", "UnknownMaintenanceAction", "Maintenance", "Unknown maintenance action: %s", action) return reconcile.TerminalError(fmt.Errorf("unknown maintenance action: %s", action)) From decbea99aef5b116f21bc25190d0c538654e72d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Felix=20K=C3=A4stner?= Date: Fri, 17 Jul 2026 19:31:18 +0200 Subject: [PATCH 6/6] Simplify Reprovision to use the pre-connected NX-API client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Now that reconcileMaintenance calls Connect before invoking Reprovision, the fallback client creation is unnecessary. Reprovision can use p.nxapi directly, matching all other provider methods. Signed-off-by: Felix Kästner --- internal/provider/cisco/nxos/provider.go | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/internal/provider/cisco/nxos/provider.go b/internal/provider/cisco/nxos/provider.go index d3bd663de..3dc290002 100644 --- a/internal/provider/cisco/nxos/provider.go +++ b/internal/provider/cisco/nxos/provider.go @@ -149,18 +149,7 @@ func (p *Provider) FactoryReset(ctx context.Context, conn *deviceutil.Connection } func (p *Provider) Reprovision(ctx context.Context, conn *deviceutil.Connection) error { - client := p.nxapi - if client == nil { - c := *conn - c.Address = netip.MustParseAddrPort(conn.Address).Addr().String() - var err error - client, err = nxapi.NewClient(&c, nxapi.WithTimeout(timeout)) - if err != nil { - return fmt.Errorf("failed to create nxapi client: %w", err) - } - } - - _, err := client.Do(ctx, nxapi.NewRequest( + _, err := p.nxapi.Do(ctx, nxapi.NewRequest( "boot poap enable", "copy running-config startup-config", ).WithRollback(nxapi.Stop)) @@ -171,7 +160,7 @@ func (p *Provider) Reprovision(ctx context.Context, conn *deviceutil.Connection) // Reboot is issued as a separate request because it actually restarts // the device. The connection will drop before a response is received, // so transport errors are expected and tolerated. - _, err = client.Do(ctx, nxapi.NewRequest("reload")) + _, err = p.nxapi.Do(ctx, nxapi.NewRequest("reload")) if err != nil && !nxapi.IsTransportError(err) { return fmt.Errorf("failed to reboot device: %w", err) }