netconf: classify mid-flight transport drops as ErrNotConnected

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.

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

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)
+			}
+		})
+	}
+}

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
 		}