diff --git a/pkg/datastore/target/netconf/CONTEXT.md b/pkg/datastore/target/netconf/CONTEXT.md
new file mode 100644
index 00000000..057c1470
--- /dev/null
+++ b/pkg/datastore/target/netconf/CONTEXT.md
@@ -0,0 +1,11 @@
+# NETCONF target
+
+Implements the `Target`/`Driver` interfaces (see `pkg/datastore/target/`) for southbound NETCONF devices, translating `sdcpb` get/set requests into NETCONF RPCs (`get-config`, `edit-config`, `commit`, `discard-changes`) and back.
+
+## Language
+
+**Transport error**:
+A failure of the underlying connection to the device *during* an in-flight RPC (e.g. the device restarts mid-`edit-config`) — detected by `isTransportError` — as opposed to a semantic/validation error the device itself returned (e.g. a rejected `edit-config`). Only transport errors trigger `Close`+`reconnect` and get wrapped in `ErrNotConnected`.
+
+**ErrNotConnected** (defined in `pkg/datastore/target/types`, not this package):
+Within this context, `ErrNotConnected` is deliberately reused for two distinct scenarios under one sentinel: a **pre-flight** check (the target was never connected when a request started, via `Status().Err()`) and a **mid-flight** transport error (the target *was* connected but the transport dropped during an in-progress RPC, via `isTransportError`). Both are wrapped the same way so `translateInternalToGrpcError` maps either to `codes.Unavailable` and config-server's retry logic treats them identically — this is an intentional unification, not an accidental conflation of two different states.
diff --git a/pkg/datastore/target/netconf/driver/scrapligo/scrapligo.go b/pkg/datastore/target/netconf/driver/scrapligo/scrapligo.go
index 424a8651..fd0b1578 100644
--- a/pkg/datastore/target/netconf/driver/scrapligo/scrapligo.go
+++ b/pkg/datastore/target/netconf/driver/scrapligo/scrapligo.go
@@ -15,7 +15,9 @@
package scrapligo
import (
+ "errors"
"fmt"
+ "io"
"github.com/beevik/etree"
scraplinetconf "github.com/scrapli/scrapligo/driver/netconf"
@@ -26,6 +28,27 @@ import (
"github.com/sdcio/data-server/pkg/datastore/target/netconf/types"
)
+// normalizeTransportError translates scrapligo's own opaque connection-drop
+// sentinel (util.ErrConnectionError) into io.EOF.
+//
+// scrapligo's channel read loop swallows a real transport io.EOF/connection
+// drop and re-surfaces it as util.ErrConnectionError (see
+// scrapligo/channel/read.go and driver/netconf/rpc.go), so callers that only
+// understand the standard io/net error vocabulary would otherwise never
+// recognize a mid-flight transport drop. This keeps that scrapligo-specific
+// knowledge contained to this driver adapter, per the Driver interface
+// abstraction: everything on the other side of the Driver interface (nc.go)
+// stays driver-agnostic and only needs to know about io.EOF/net.Error.
+func normalizeTransportError(err error) error {
+ if err == nil {
+ return nil
+ }
+ if errors.Is(err, util.ErrConnectionError) {
+ return fmt.Errorf("%w: %w", io.EOF, err)
+ }
+ return err
+}
+
type ScrapligoNetconfTarget struct {
driver *scraplinetconf.Driver
}
@@ -95,7 +118,7 @@ func (snt *ScrapligoNetconfTarget) EditConfig(target string, config string) (*ty
// send the edit config rpc
resp, err := snt.driver.EditConfig(target, xdoc)
if err != nil {
- return nil, err
+ return nil, normalizeTransportError(err)
}
if len(resp.ErrorMessages) > 0 {
return nil, resp.Failed
@@ -119,7 +142,7 @@ func (snt *ScrapligoNetconfTarget) GetConfig(source string, filter string) (*typ
// execute the GetConfig rpc
resp, err := snt.driver.GetConfig(source, filterDoc, options.WithNetconfForceSelfClosingTags())
if err != nil {
- return nil, err
+ return nil, normalizeTransportError(err)
}
if resp.Failed != nil {
return nil, resp.Failed
@@ -151,7 +174,7 @@ func (snt *ScrapligoNetconfTarget) Commit() error {
// execute the Commit rpc
resp, err := snt.driver.Commit()
if err != nil {
- return err
+ return normalizeTransportError(err)
}
if resp.Failed != nil {
return resp.Failed
diff --git a/pkg/datastore/target/netconf/driver/scrapligo/scrapligo_test.go b/pkg/datastore/target/netconf/driver/scrapligo/scrapligo_test.go
new file mode 100644
index 00000000..6fe7b118
--- /dev/null
+++ b/pkg/datastore/target/netconf/driver/scrapligo/scrapligo_test.go
@@ -0,0 +1,99 @@
+// Copyright 2024 Nokia
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package scrapligo
+
+import (
+ "errors"
+ "fmt"
+ "io"
+ "testing"
+
+ "github.com/scrapli/scrapligo/util"
+)
+
+func Test_normalizeTransportError(t *testing.T) {
+ tests := []struct {
+ name string
+ err error
+ wantNil bool
+ wantEOF bool
+ wantSameAs error
+ }{
+ {
+ name: "nil error",
+ err: nil,
+ wantNil: true,
+ },
+ {
+ name: "scrapligo connection error is normalized to io.EOF",
+ err: util.ErrConnectionError,
+ wantEOF: true,
+ },
+ {
+ name: "wrapped scrapligo connection error is still normalized to io.EOF",
+ err: fmt.Errorf("channel read loop failed: %w", util.ErrConnectionError),
+ wantEOF: true,
+ },
+ {
+ name: "unrelated scrapligo error is returned unchanged",
+ err: util.ErrTimeoutError,
+ wantSameAs: util.ErrTimeoutError,
+ },
+ {
+ name: "generic error is returned unchanged",
+ err: errors.New("some other failure"),
+ wantSameAs: nil, // checked by message equality below instead
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ got := normalizeTransportError(tt.err)
+
+ if tt.wantNil {
+ if got != nil {
+ t.Fatalf("expected nil, got %v", got)
+ }
+ return
+ }
+
+ if tt.wantEOF {
+ if !errors.Is(got, io.EOF) {
+ t.Errorf("expected errors.Is(got, io.EOF), got %v", got)
+ }
+ // the original scrapligo error must still be discoverable so
+ // logs/debugging don't lose the underlying detail.
+ if !errors.Is(got, util.ErrConnectionError) {
+ t.Errorf("expected the original util.ErrConnectionError to still be discoverable via errors.Is, got %v", got)
+ }
+ return
+ }
+
+ if tt.wantSameAs != nil {
+ if !errors.Is(got, tt.wantSameAs) {
+ t.Errorf("expected error to be returned unchanged, got %v want %v", got, tt.wantSameAs)
+ }
+ if errors.Is(got, io.EOF) {
+ t.Errorf("did not expect a non-connection error to be normalized to io.EOF, got %v", got)
+ }
+ return
+ }
+
+ // generic error case: returned unchanged (same error value).
+ if got != tt.err {
+ t.Errorf("expected the original error to be returned unchanged, got %v want %v", got, tt.err)
+ }
+ })
+ }
+}
diff --git a/pkg/datastore/target/netconf/nc.go b/pkg/datastore/target/netconf/nc.go
index c2ce441f..d46ba632 100644
--- a/pkg/datastore/target/netconf/nc.go
+++ b/pkg/datastore/target/netconf/nc.go
@@ -17,7 +17,6 @@ package netconf
import (
"context"
"fmt"
- "strings"
"sync"
"time"
@@ -130,9 +129,8 @@ func (t *ncTarget) internalGet(ctx context.Context, req *sdcpb.GetDataRequest) (
// execute the GetConfig rpc
ncResponse, err := t.driver.GetConfig(source, filterDoc)
if err != nil {
- if strings.Contains(err.Error(), "EOF") {
- _ = t.Close(ctx)
- go t.reconnect(ctx)
+ if isTransportError(err) {
+ return nil, t.handleTransportError(ctx, err)
}
return nil, err
}
@@ -186,6 +184,16 @@ func (t *ncTarget) Status() *types.TargetStatus {
return result
}
+// handleTransportError closes the current connection and kicks off a
+// background reconnect, then wraps err in types.ErrNotConnected so the
+// caller's caller (ultimately the TransactionSet gRPC handler, via
+// translateInternalToGrpcError) can classify it as recoverable.
+func (t *ncTarget) handleTransportError(ctx context.Context, err error) error {
+ _ = t.Close(ctx)
+ go t.reconnect(ctx)
+ return fmt.Errorf("%s: %w: %w", t.name, types.ErrNotConnected, err)
+}
+
func (t *ncTarget) Close(ctx context.Context) error {
if t == nil {
return nil
@@ -261,10 +269,8 @@ func (t *ncTarget) setToDevice(ctx context.Context, commitDatastore string, sour
resp, err := t.driver.EditConfig(commitDatastore, xdoc)
if err != nil {
log.Error(err, "failed during edit-config")
- if strings.Contains(err.Error(), "EOF") {
- _ = t.Close(ctx)
- go t.reconnect(ctx)
- return nil, err
+ if isTransportError(err) {
+ return nil, t.handleTransportError(ctx, err)
}
// candidate should discard on error
@@ -288,9 +294,8 @@ func (t *ncTarget) setToDevice(ctx context.Context, commitDatastore string, sour
// commit the config
err = t.driver.Commit()
if err != nil {
- if strings.Contains(err.Error(), "EOF") {
- _ = t.Close(ctx)
- go t.reconnect(ctx)
+ if isTransportError(err) {
+ return nil, t.handleTransportError(ctx, err)
}
return nil, err
}
diff --git a/pkg/datastore/target/netconf/nc_transport_error_test.go b/pkg/datastore/target/netconf/nc_transport_error_test.go
new file mode 100644
index 00000000..5bae8d93
--- /dev/null
+++ b/pkg/datastore/target/netconf/nc_transport_error_test.go
@@ -0,0 +1,214 @@
+// Copyright 2024 Nokia
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package netconf
+
+import (
+ "context"
+ "errors"
+ "io"
+ "sync"
+ "testing"
+ "time"
+
+ "github.com/beevik/etree"
+ "github.com/sdcio/data-server/mocks/mocknetconf"
+ "github.com/sdcio/data-server/pkg/config"
+ nctypes "github.com/sdcio/data-server/pkg/datastore/target/netconf/types"
+ "github.com/sdcio/data-server/pkg/datastore/target/types"
+ sdcpb "github.com/sdcio/sdc-protos/sdcpb"
+ "go.uber.org/mock/gomock"
+)
+
+// fakeTargetSource is a minimal types.TargetSource used to drive setToDevice
+// without needing a real datastore tree.
+type fakeTargetSource struct {
+ doc *etree.Document
+}
+
+func newFakeTargetSource() *fakeTargetSource {
+ doc := etree.NewDocument()
+ if err := doc.ReadFromString("eth0"); err != nil {
+ panic(err)
+ }
+ return &fakeTargetSource{doc: doc}
+}
+
+func (f *fakeTargetSource) ToJson(_ context.Context, _ bool) (any, error) { return nil, nil }
+func (f *fakeTargetSource) ToJsonIETF(_ context.Context, _ bool) (any, error) {
+ return nil, nil
+}
+
+func (f *fakeTargetSource) ToXML(_ context.Context, _, _, _, _ bool) (*etree.Document, error) {
+ return f.doc, nil
+}
+
+func (f *fakeTargetSource) ToProtoUpdates(_ context.Context, _ bool) ([]*sdcpb.Update, error) {
+ return nil, nil
+}
+
+func (f *fakeTargetSource) ToProtoDeletes(_ context.Context) ([]*sdcpb.Path, error) {
+ return nil, nil
+}
+
+func (f *fakeTargetSource) ContainsChanges(_ context.Context) (bool, error) {
+ return true, nil
+}
+
+// testSBIConfig returns a minimal SBI config good enough to let a background
+// reconnect() attempt (and quickly fail/retry against a bogus address)
+// without blocking the test itself, since reconnect is always launched via
+// `go t.reconnect(ctx)`.
+func testSBIConfig() *config.SBI {
+ return &config.SBI{
+ Address: "127.0.0.1:0",
+ ConnectRetry: time.Millisecond,
+ NetconfOptions: &config.SBINetconfOptions{
+ CommitDatastore: "candidate",
+ },
+ }
+}
+
+func rpcReplyDoc(t *testing.T) *etree.Document {
+ t.Helper()
+ doc := etree.NewDocument()
+ if err := doc.ReadFromString(""); err != nil {
+ t.Fatalf("building rpc-reply doc: %v", err)
+ }
+ return doc
+}
+
+func Test_ncTarget_internalGet_TransportError(t *testing.T) {
+ mockCtrl := gomock.NewController(t)
+ d := mocknetconf.NewMockDriver(mockCtrl)
+ d.EXPECT().IsAlive().AnyTimes().Return(true)
+ d.EXPECT().GetConfig(gomock.Any(), gomock.Any()).Return(nil, io.EOF)
+ d.EXPECT().Close().Return(nil)
+
+ tr := &ncTarget{
+ name: "dev1",
+ m: new(sync.Mutex),
+ driver: d,
+ sbiConfig: testSBIConfig(),
+ }
+
+ _, err := tr.internalGet(context.Background(), &sdcpb.GetDataRequest{})
+ if err == nil {
+ t.Fatal("expected an error, got nil")
+ }
+ if !errors.Is(err, types.ErrNotConnected) {
+ t.Errorf("expected errors.Is(err, types.ErrNotConnected), got %v", err)
+ }
+ if !errors.Is(err, io.EOF) {
+ t.Errorf("expected the original io.EOF to still be discoverable via errors.Is, got %v", err)
+ }
+}
+
+func Test_ncTarget_internalGet_NonTransportError(t *testing.T) {
+ mockCtrl := gomock.NewController(t)
+ d := mocknetconf.NewMockDriver(mockCtrl)
+ d.EXPECT().IsAlive().AnyTimes().Return(true)
+ nonTransportErr := errors.New("invalid filter")
+ d.EXPECT().GetConfig(gomock.Any(), gomock.Any()).Return(nil, nonTransportErr)
+ // Close must NOT be called for a non-transport error; the strict mock
+ // controller will fail the test if it is.
+
+ tr := &ncTarget{
+ name: "dev1",
+ m: new(sync.Mutex),
+ driver: d,
+ sbiConfig: testSBIConfig(),
+ }
+
+ _, err := tr.internalGet(context.Background(), &sdcpb.GetDataRequest{})
+ if !errors.Is(err, nonTransportErr) {
+ t.Errorf("expected the original error to be returned unwrapped, got %v", err)
+ }
+ if errors.Is(err, types.ErrNotConnected) {
+ t.Errorf("did not expect errors.Is(err, types.ErrNotConnected), got %v", err)
+ }
+}
+
+func Test_ncTarget_setToDevice_EditConfig_TransportError(t *testing.T) {
+ mockCtrl := gomock.NewController(t)
+ d := mocknetconf.NewMockDriver(mockCtrl)
+ d.EXPECT().IsAlive().AnyTimes().Return(true)
+ d.EXPECT().EditConfig(gomock.Any(), gomock.Any()).Return(nil, io.EOF)
+ d.EXPECT().Close().Return(nil)
+ // Discard must NOT be called for a transport error.
+
+ tr := &ncTarget{
+ name: "dev1",
+ m: new(sync.Mutex),
+ driver: d,
+ sbiConfig: testSBIConfig(),
+ }
+
+ _, err := tr.setToDevice(context.Background(), "candidate", newFakeTargetSource())
+ if !errors.Is(err, types.ErrNotConnected) {
+ t.Errorf("expected errors.Is(err, types.ErrNotConnected), got %v", err)
+ }
+ if !errors.Is(err, io.EOF) {
+ t.Errorf("expected the original io.EOF to still be discoverable via errors.Is, got %v", err)
+ }
+}
+
+func Test_ncTarget_setToDevice_EditConfig_NonTransportError_DiscardsCandidate(t *testing.T) {
+ mockCtrl := gomock.NewController(t)
+ d := mocknetconf.NewMockDriver(mockCtrl)
+ d.EXPECT().IsAlive().AnyTimes().Return(true)
+ nonTransportErr := errors.New("rpc-error: data-exists")
+ d.EXPECT().EditConfig(gomock.Any(), gomock.Any()).Return(nil, nonTransportErr)
+ d.EXPECT().Discard().Return(nil)
+ // Close must NOT be called for a non-transport error.
+
+ tr := &ncTarget{
+ name: "dev1",
+ m: new(sync.Mutex),
+ driver: d,
+ sbiConfig: testSBIConfig(),
+ }
+
+ _, err := tr.setToDevice(context.Background(), "candidate", newFakeTargetSource())
+ if !errors.Is(err, nonTransportErr) {
+ t.Errorf("expected the original error to be returned unwrapped, got %v", err)
+ }
+ if errors.Is(err, types.ErrNotConnected) {
+ t.Errorf("did not expect errors.Is(err, types.ErrNotConnected), got %v", err)
+ }
+}
+
+func Test_ncTarget_setToDevice_Commit_TransportError(t *testing.T) {
+ mockCtrl := gomock.NewController(t)
+ d := mocknetconf.NewMockDriver(mockCtrl)
+ d.EXPECT().IsAlive().AnyTimes().Return(true)
+ d.EXPECT().EditConfig(gomock.Any(), gomock.Any()).Return(&nctypes.NetconfResponse{Doc: rpcReplyDoc(t)}, nil)
+ d.EXPECT().Commit().Return(io.EOF)
+ d.EXPECT().Close().Return(nil)
+
+ tr := &ncTarget{
+ name: "dev1",
+ m: new(sync.Mutex),
+ driver: d,
+ sbiConfig: testSBIConfig(),
+ }
+
+ _, err := tr.setToDevice(context.Background(), "candidate", newFakeTargetSource())
+ if !errors.Is(err, types.ErrNotConnected) {
+ t.Errorf("expected errors.Is(err, types.ErrNotConnected), got %v", err)
+ }
+ if !errors.Is(err, io.EOF) {
+ t.Errorf("expected the original io.EOF to still be discoverable via errors.Is, got %v", err)
+ }
+}
\ No newline at end of file
diff --git a/pkg/datastore/target/netconf/transport_error.go b/pkg/datastore/target/netconf/transport_error.go
new file mode 100644
index 00000000..5d16e45f
--- /dev/null
+++ b/pkg/datastore/target/netconf/transport_error.go
@@ -0,0 +1,57 @@
+// Copyright 2024 Nokia
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package netconf
+
+import (
+ "errors"
+ "io"
+ "net"
+ "strings"
+)
+
+// isTransportError reports whether err represents a transport-level connection
+// drop (e.g. the southbound device restarting mid-RPC) rather than a
+// semantic/validation failure returned by the device itself.
+//
+// It relies only on the standard io/net error vocabulary (io.EOF, net.Error)
+// plus a substring fallback for drivers that don't preserve that wrapping.
+// Driver implementations are responsible for normalizing their own
+// library-specific transport sentinels into io.EOF (or a net.Error) at the
+// Driver interface boundary, so this function stays driver-agnostic.
+func isTransportError(err error) bool {
+ if err == nil {
+ return false
+ }
+ if errors.Is(err, io.EOF) {
+ return true
+ }
+ var netErr net.Error
+ if errors.As(err, &netErr) {
+ return true
+ }
+ // Substring fallback: a safety net for drivers other than scrapligo (or a
+ // future scrapligo version) that don't preserve io.EOF/net.Error wrapping
+ // through their own error paths. Driver implementations should still
+ // prefer normalizing at their own boundary (see driver/scrapligo's
+ // normalizeTransportError) so this fallback ideally never fires in
+ // practice.
+ msg := err.Error()
+ for _, sub := range []string{"EOF", "broken pipe", "connection reset", "i/o timeout"} {
+ if strings.Contains(msg, sub) {
+ return true
+ }
+ }
+ return false
+}
diff --git a/pkg/datastore/target/netconf/transport_error_test.go b/pkg/datastore/target/netconf/transport_error_test.go
new file mode 100644
index 00000000..75785805
--- /dev/null
+++ b/pkg/datastore/target/netconf/transport_error_test.go
@@ -0,0 +1,84 @@
+// Copyright 2024 Nokia
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package netconf
+
+import (
+ "errors"
+ "fmt"
+ "io"
+ "net"
+ "testing"
+)
+
+func Test_isTransportError(t *testing.T) {
+ tests := []struct {
+ name string
+ err error
+ want bool
+ }{
+ {
+ name: "nil error",
+ err: nil,
+ want: false,
+ },
+ {
+ name: "raw io.EOF",
+ err: io.EOF,
+ want: true,
+ },
+ {
+ name: "wrapped io.EOF",
+ err: fmt.Errorf("reading response: %w", io.EOF),
+ want: true,
+ },
+ {
+ name: "net.Error",
+ err: &net.OpError{Op: "read", Err: errors.New("connection reset by peer")},
+ want: true,
+ },
+ {
+ name: "broken pipe substring",
+ err: errors.New("write: broken pipe"),
+ want: true,
+ },
+ {
+ name: "connection reset substring",
+ err: errors.New("read: connection reset by peer"),
+ want: true,
+ },
+ {
+ name: "i/o timeout substring",
+ err: errors.New("read: i/o timeout"),
+ want: true,
+ },
+ {
+ name: "literal EOF substring",
+ err: errors.New("unexpected EOF while parsing"),
+ want: true,
+ },
+ {
+ name: "unrelated error",
+ err: errors.New("invalid value"),
+ want: false,
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ if got := isTransportError(tt.err); got != tt.want {
+ t.Errorf("isTransportError(%v) = %v, want %v", tt.err, got, tt.want)
+ }
+ })
+ }
+}