diff --git a/go.mod b/go.mod index 82891a09..edd2da8f 100644 --- a/go.mod +++ b/go.mod @@ -20,7 +20,7 @@ require ( github.com/openconfig/goyang v1.6.0 github.com/openconfig/ygot v0.34.0 github.com/prometheus/client_golang v1.23.2 - github.com/scrapli/scrapligo v1.4.0 + github.com/scrapli/scrapligo v1.4.1 github.com/sdcio/cache v0.0.38 github.com/sdcio/logger v0.0.3 github.com/sdcio/schema-server v0.0.34 diff --git a/go.sum b/go.sum index 4f8192a6..1a81efae 100644 --- a/go.sum +++ b/go.sum @@ -181,8 +181,8 @@ github.com/prometheus/procfs v0.16.1/go.mod h1:teAbpZRB1iIAJYREa1LsoWUXykVXA1KlT github.com/rogpeppe/go-internal v1.14.1 h1:UQB4HGPB6osV0SQTLymcB4TgvyWu6ZyliaW0tI/otEQ= github.com/rogpeppe/go-internal v1.14.1/go.mod h1:MaRKkUm5W0goXpeCfT7UZI6fk/L7L7so1lCWt35ZSgc= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= -github.com/scrapli/scrapligo v1.4.0 h1:gF7bIiRHT/aB1zTu9PYIK9R2A/gXr4fF/CKUyyWaNq4= -github.com/scrapli/scrapligo v1.4.0/go.mod h1:pOWxVyPsQRrWTrkoSSDg05tjOqtWfLffAZtAsCc0w3M= +github.com/scrapli/scrapligo v1.4.1 h1:D0lg2xusZKbKnzy9l0yT7ly/D2rgqab12yZvEXNim0U= +github.com/scrapli/scrapligo v1.4.1/go.mod h1:pOWxVyPsQRrWTrkoSSDg05tjOqtWfLffAZtAsCc0w3M= github.com/sdcio/cache v0.0.38 h1:8meykZC/DAgzKI+R3GtZvTs0/OXWRCeNN5mxVKG0Ba8= github.com/sdcio/cache v0.0.38/go.mod h1:PHJd4pTmfFZBLdPohtXJAVBNmAKhhKdZHTi3MejGrk0= github.com/sdcio/goyang v1.6.2-2 h1:qfeUKBmoKpiKAruuEc3+V8wgHKP/n1jRDEnTy23knV8= diff --git a/pkg/datastore/target/netconf/sync.go b/pkg/datastore/target/netconf/sync.go index e8d1fc57..075b7676 100644 --- a/pkg/datastore/target/netconf/sync.go +++ b/pkg/datastore/target/netconf/sync.go @@ -70,7 +70,12 @@ func (s *NetconfSyncImpl) Start() error { return nil } - go func() { _ = s.internalSync(req) }() + go func() { + err = s.internalSync(req) + if err != nil { + log.Error(err, "failed syncing") + } + }() go func() { ticker := time.NewTicker(s.config.Interval) diff --git a/pkg/tree/ops/utils.go b/pkg/tree/ops/utils.go index f3818d54..5f8cb6b9 100644 --- a/pkg/tree/ops/utils.go +++ b/pkg/tree/ops/utils.go @@ -22,6 +22,14 @@ func getListEntrySortFunc(parent api.Entry) func(a, b api.Entry) int { aLvSlice := GetHighestPrecedence(achild, false, true, true) bLvSlice := GetHighestPrecedence(bchild, false, true, true) + // A key leaf can legitimately end up without any LeafVariant (e.g. a + // structural/placeholder entry created while navigating the tree without + // a value ever being set for it). We cannot compare such entries, so treat + // them as equal on this key, just like the "doesn't exist" case above. + if len(aLvSlice) == 0 || len(bLvSlice) == 0 { + return 0 + } + aEntry := aLvSlice[0] bEntry := bLvSlice[0] diff --git a/pkg/tree/ops/utils_test.go b/pkg/tree/ops/utils_test.go new file mode 100644 index 00000000..45efacc7 --- /dev/null +++ b/pkg/tree/ops/utils_test.go @@ -0,0 +1,67 @@ +package ops_test + +import ( + "context" + "runtime" + "testing" + + "github.com/sdcio/data-server/pkg/pool" + "github.com/sdcio/data-server/pkg/tree" + "github.com/sdcio/data-server/pkg/tree/api" + "github.com/sdcio/data-server/pkg/tree/ops" + "github.com/sdcio/data-server/pkg/utils/testhelper" + sdcpb "github.com/sdcio/sdc-protos/sdcpb" + "go.uber.org/mock/gomock" +) + +// TestToJson_ListEntryKeyWithoutLeafVariant reproduces a panic scenario where a +// list entry's key leaf exists in the tree (e.g. as a structural/placeholder +// entry created while navigating the tree) but never got a LeafVariant assigned +// to it and has no active children either. Sorting such list entries for JSON +// rendering used to index into an empty slice returned by GetHighestPrecedence, +// causing an "index out of range [0] with length 0" panic. ToJson must instead +// treat such incomparable entries as equal and render without panicking. +func TestToJson_ListEntryKeyWithoutLeafVariant(t *testing.T) { + mockCtrl := gomock.NewController(t) + defer mockCtrl.Finish() + + scb, err := testhelper.GetSchemaClientBound(t, mockCtrl) + if err != nil { + t.Fatal(err) + } + + ctx := context.Background() + tc := tree.NewTreeContext(scb, pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0))) + root, err := tree.NewTreeRoot(ctx, tc) + if err != nil { + t.Fatal(err) + } + + // Create two list entries purely via structural navigation (no updates / no + // LeafVariants applied), leaving the "name" key entries valueless, similar to + // how a placeholder path can end up in the tree without ever carrying data. + for _, name := range []string{"ethernet-1/1", "ethernet-1/2"} { + keyNode, err := ops.GetOrCreateChilds(ctx, root.Entry, &sdcpb.Path{ + Elem: []*sdcpb.PathElem{ + sdcpb.NewPathElem("interface", map[string]string{"name": name}), + }, + }) + if err != nil { + t.Fatal(err) + } + // Attach the "name" key leaf entry structurally, without ever adding a + // LeafVariant to it. + if _, err := api.NewEntry(ctx, keyNode, "name", tc); err != nil { + t.Fatal(err) + } + } + + if err := root.FinishInsertionPhase(ctx); err != nil { + t.Fatal(err) + } + + // Must not panic. + if _, err := ops.ToJson(ctx, root.Entry, false); err != nil { + t.Fatalf("ToJson() returned unexpected error: %v", err) + } +}