From 4dff5bb7f12664ed9101c1d931b58c7d698b78d0 Mon Sep 17 00:00:00 2001 From: steiler Date: Mon, 1 Jun 2026 16:22:35 +0200 Subject: [PATCH 1/4] fix: normalize identityref values in JSON importer and leafref validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GetKeyValue in the JSON importer now routes through GetTVValue/ToString so identityref keys are emitted as module-qualified strings (e.g. "sdcio-model-identity:ETHERNET") rather than a raw proto-struct dump. resolveLeafrefKeyPath switches from tv.GetStringVal() to tv.ToString() for the same reason — GetStringVal returns an empty string for TypedValue_IdentityRef, causing leafref lookups to silently fail. Navigate in yangParserEntryAdapter now calls StripPathElemPrefixPath before NavigateSdcpbPath so xpath evaluation against identityref-keyed list entries succeeds when path elements carry a module prefix. Adds regression tests: TestLeafref_IdentityrefKey, TestMust_IdentityrefKey (validation), and JSON-importer round-trip test with identityref list key. Extends sdcio_model_identity.yang with matching fixtures. Co-authored-by: Cursor --- pkg/tree/importer/import_config_adapter.go | 3 + pkg/tree/importer/json/json_tree_importer.go | 9 +- .../importer/json/json_tree_importer_test.go | 31 ++++ .../validation/validation_entry_leafref.go | 2 +- .../validation_entry_leafref_test.go | 141 ++++++++++++++++++ .../ops/validation/yang-parser-adapter.go | 2 + tests/schema/sdcio_model_identity.yang | 26 ++++ 7 files changed, 212 insertions(+), 2 deletions(-) diff --git a/pkg/tree/importer/import_config_adapter.go b/pkg/tree/importer/import_config_adapter.go index 6096178f..480462ca 100644 --- a/pkg/tree/importer/import_config_adapter.go +++ b/pkg/tree/importer/import_config_adapter.go @@ -29,6 +29,9 @@ type ImportConfigAdapterElement interface { // GetKeyValue can be called on Leafs or LeafList elements to retrieve the underlaying value // When and were to expect a Leafs or LeafList is defined by the yang schema. // The String value is typically used for the keys. + // Contract: for identityref leaf types, GetKeyValue must return the bare identity name + // (e.g. "BGP"), stripping any module prefix (e.g. "openconfig-policy-types:BGP"). + // This ensures JSON_IETF and plain JSON inputs produce identical tree keys. GetKeyValue(ctx context.Context, slt *sdcpb.SchemaLeafType) (string, error) // GetTVValue returns the TypedValue based value defined via the SchemaLeafType. Can also only be called on Leafs or LeafLists GetTVValue(ctx context.Context, slt *sdcpb.SchemaLeafType) (*sdcpb.TypedValue, error) diff --git a/pkg/tree/importer/json/json_tree_importer.go b/pkg/tree/importer/json/json_tree_importer.go index 8a7237c7..250537b5 100644 --- a/pkg/tree/importer/json/json_tree_importer.go +++ b/pkg/tree/importer/json/json_tree_importer.go @@ -107,7 +107,14 @@ func (j *JsonTreeImporterElement) GetElements() []importer.ImportConfigAdapterEl } func (j *JsonTreeImporterElement) GetKeyValue(ctx context.Context, slt *sdcpb.SchemaLeafType) (string, error) { - return fmt.Sprintf("%v", j.data), nil + if slt == nil { + return fmt.Sprintf("%v", j.data), nil + } + tv, err := j.GetTVValue(ctx, slt) + if err != nil { + return "", err + } + return tv.ToString(), nil } func (j *JsonTreeImporterElement) GetTVValue(ctx context.Context, slt *sdcpb.SchemaLeafType) (*sdcpb.TypedValue, error) { diff --git a/pkg/tree/importer/json/json_tree_importer_test.go b/pkg/tree/importer/json/json_tree_importer_test.go index 5b695a6e..38bf099a 100644 --- a/pkg/tree/importer/json/json_tree_importer_test.go +++ b/pkg/tree/importer/json/json_tree_importer_test.go @@ -332,3 +332,34 @@ func TestJsonTreeImporter_UnknownContainer(t *testing.T) { t.Error("GetElement(\"non-existent-container\") should return nil") } } + +// TestJsonTreeImporter_GetKeyValue_Identityref verifies that GetKeyValue strips the module +// prefix from a JSON_IETF identityref value and returns the bare identity name, and that +// plain JSON (no prefix) and JSON_IETF (module-prefixed) inputs produce identical keys. +func TestJsonTreeImporter_GetKeyValue_Identityref(t *testing.T) { + slt := &sdcpb.SchemaLeafType{ + Type: "identityref", + IdentityPrefixesMap: map[string]string{"BGP": "oc-policy-types"}, + ModulePrefixMap: map[string]string{"BGP": "openconfig-policy-types"}, + } + + tests := []struct { + name string + data any + }{ + {"plain_json", "BGP"}, + {"json_ietf_prefixed", "openconfig-policy-types:BGP"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + elem := newJsonTreeImporterElement("afi-safi-type", tt.data) + got, err := elem.GetKeyValue(context.Background(), slt) + if err != nil { + t.Fatalf("GetKeyValue() error: %v", err) + } + if got != "BGP" { + t.Errorf("GetKeyValue() = %q, want %q", got, "BGP") + } + }) + } +} diff --git a/pkg/tree/ops/validation/validation_entry_leafref.go b/pkg/tree/ops/validation/validation_entry_leafref.go index fa37b0ca..937c0030 100644 --- a/pkg/tree/ops/validation/validation_entry_leafref.go +++ b/pkg/tree/ops/validation/validation_entry_leafref.go @@ -195,7 +195,7 @@ func resolveLeafrefKeyPath(ctx context.Context, e api.Entry, keys map[string]*ty return fmt.Errorf("no leafentry found") } tv := lvs[0].Value() - keys[k].Value = tv.GetStringVal() + keys[k].Value = tv.ToString() keys[k].DoNotResolve = true } return nil diff --git a/pkg/tree/ops/validation/validation_entry_leafref_test.go b/pkg/tree/ops/validation/validation_entry_leafref_test.go index ce6ccbf9..e688ebc3 100644 --- a/pkg/tree/ops/validation/validation_entry_leafref_test.go +++ b/pkg/tree/ops/validation/validation_entry_leafref_test.go @@ -23,6 +23,147 @@ import ( "go.uber.org/mock/gomock" ) +// buildTree creates a tree populated with config supplied as a plain-JSON map. +func buildTree(t *testing.T, jsonConf map[string]any) *tree.RootEntry { + t.Helper() + ctx := context.Background() + + sc, schema, err := testhelper.InitSDCIOSchema() + if err != nil { + t.Fatal(err) + } + scb := schemaClient.NewSchemaClientBound(schema, sc) + tc := tree.NewTreeContext(scb, pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0))) + + root, err := tree.NewTreeRoot(ctx, tc) + if err != nil { + t.Fatal(err) + } + + vpf := pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)) + _, err = root.ImportConfig(ctx, &sdcpb.Path{}, jsonImporter.NewJsonTreeImporter(jsonConf, "owner1", 500, false), types.NewUpdateInsertFlags(), vpf) + if err != nil { + t.Fatal(err) + } + + if err := root.FinishInsertionPhase(ctx); err != nil { + t.Fatal(err) + } + return root +} + +// TestLeafref_IdentityrefKey verifies that leafref validation resolves +// current()-relative key predicates whose values are identityref leaves. +// Without the tv.ToString() fix in resolveLeafrefKeyPath the key is "" and +// the reference cannot be found. +func TestLeafref_IdentityrefKey(t *testing.T) { + tests := []struct { + name string + conf map[string]any + wantErrLen int + }{ + { + name: "identityref key resolves - pass", + conf: map[string]any{ + "identityref": map[string]any{"cryptoA": "otherAlgo"}, + "intentityrefkey": []any{map[string]any{"crypto": "otherAlgo", "description": "my-desc"}}, + "intentityrefkey-ref": "my-desc", + }, + wantErrLen: 0, + }, + { + name: "identityref key mismatch - fail", + conf: map[string]any{ + "identityref": map[string]any{"cryptoA": "rsa"}, + "intentityrefkey": []any{map[string]any{"crypto": "otherAlgo", "description": "my-desc"}}, + "intentityrefkey-ref": "my-desc", + }, + wantErrLen: 1, + }, + } + + lrefPath := &sdcpb.Path{ + Elem: []*sdcpb.PathElem{sdcpb.NewPathElem("intentityrefkey-ref", nil)}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + root := buildTree(t, tt.conf) + + e, err := ops.NavigateSdcpbPath(ctx, root.Entry, lrefPath) + if err != nil { + t.Fatalf("NavigateSdcpbPath: %v", err) + } + + valConf := config.NewValidationConfig() + valConf.DisabledValidators.DisableAll() + valConf.DisabledValidators.Leafref = false + valConf.DisableConcurrency = true + + result, _ := validation.Validate(ctx, e, valConf, pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0))) + if len(result) != tt.wantErrLen { + t.Errorf("Validate() returned %d errors, want %d: %v", len(result), tt.wantErrLen, result) + } + }) + } +} + +// TestMust_IdentityrefKey verifies that must-statement validation navigates +// correctly when the predicate key value originates from an identityref leaf. +// Without the p.StripPathElemPrefixPath() fix in Navigate the YangString() +// prefix ("sdcio_identity:otherAlgo") prevents the list entry from being found. +func TestMust_IdentityrefKey(t *testing.T) { + mustPath := &sdcpb.Path{ + Elem: []*sdcpb.PathElem{sdcpb.NewPathElem("identityref-must-test", nil)}, + } + + tests := []struct { + name string + conf map[string]any + wantErrLen int + }{ + { + name: "must satisfied - pass", + conf: map[string]any{ + "identityref-must-test": map[string]any{"crypto-ref": "otherAlgo"}, + "intentityrefkey": []any{map[string]any{"crypto": "otherAlgo", "description": "present"}}, + }, + wantErrLen: 0, + }, + { + name: "must not satisfied - fail", + conf: map[string]any{ + "identityref-must-test": map[string]any{"crypto-ref": "otherAlgo"}, + // no intentityrefkey entry → description path resolves to "" → must false + }, + wantErrLen: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + root := buildTree(t, tt.conf) + + e, err := ops.NavigateSdcpbPath(ctx, root.Entry, mustPath) + if err != nil { + t.Fatalf("NavigateSdcpbPath: %v", err) + } + + valConf := config.NewValidationConfig() + valConf.DisabledValidators.DisableAll() + valConf.DisabledValidators.MustStatement = false + valConf.DisableConcurrency = true + + result, _ := validation.Validate(ctx, e, valConf, pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0))) + if len(result) != tt.wantErrLen { + t.Errorf("Validate() returned %d errors, want %d: %v", len(result), tt.wantErrLen, result) + } + }) + } +} + func Test_sharedEntryAttributes_validateLeafRefs(t *testing.T) { owner1 := "owner1" diff --git a/pkg/tree/ops/validation/yang-parser-adapter.go b/pkg/tree/ops/validation/yang-parser-adapter.go index 40152643..a55ba3d7 100644 --- a/pkg/tree/ops/validation/yang-parser-adapter.go +++ b/pkg/tree/ops/validation/yang-parser-adapter.go @@ -120,6 +120,8 @@ func (y *yangParserEntryAdapter) Navigate(p *sdcpb.Path) (xpath.Entry, error) { return y, nil } + p.StripPathElemPrefixPath() + entry, err := ops.NavigateSdcpbPath(y.ctx, y.e, p) if err != nil { return newYangParserValueEntry(xpath.NewNodesetDatum([]xutils.XpathNode{}), err), nil diff --git a/tests/schema/sdcio_model_identity.yang b/tests/schema/sdcio_model_identity.yang index bd01904f..b3256563 100644 --- a/tests/schema/sdcio_model_identity.yang +++ b/tests/schema/sdcio_model_identity.yang @@ -38,5 +38,31 @@ module sdcio_model_identity { type string; } } + + // must exercises Navigate prefix stripping: the key predicate value is + // derived from an identityref leaf via current(), which YangString() + // returns as "prefix:value". Navigate must strip that prefix before + // calling NavigateSdcpbPath, otherwise the list entry cannot be found. + // The sdcio_identity: self-prefix on path steps is a non-standard alias + // (the importing module uses a different alias), exercising the strip. + container identityref-must-test { + leaf crypto-ref { + type identityref { + base identity_base:crypto-alg; + } + } + must "../sdcio_identity:intentityrefkey[sdcio_identity:crypto=current()/sdcio_identity:crypto-ref]/sdcio_identity:description != ''" { + error-message "intentityrefkey entry for crypto-ref must exist with non-empty description"; + } + } + + // leafref exercises resolveLeafrefKeyPath identityref key normalisation: + // the key predicate current()/../identityref/cryptoA resolves to an + // IdentityrefVal; tv.ToString() must be used (not tv.GetStringVal()). + leaf intentityrefkey-ref { + type leafref { + path "../intentityrefkey[crypto=current()/../identityref/cryptoA]/description"; + } + } } } From 3165586eea623bb2860fc2f776f2542b7ff18000 Mon Sep 17 00:00:00 2001 From: steiler Date: Tue, 2 Jun 2026 14:25:21 +0200 Subject: [PATCH 2/4] fix: serialize performRevert with in-flight transactions via dmutex Periodic GET sync revert raced with concurrent intent delete transactions: LoadAllButRunningIntents read stale cache (deleted intent still present), then applyIntent pushed deleted config back to device, undoing the deletion. Acquiring dmutex in performRevert forces it to wait for any active transaction to fully complete (device write + IntentDelete) before snapshotting the intent store. Co-authored-by: Cursor --- pkg/datastore/sync.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/datastore/sync.go b/pkg/datastore/sync.go index 42101ec5..10f990f0 100644 --- a/pkg/datastore/sync.go +++ b/pkg/datastore/sync.go @@ -134,6 +134,14 @@ func (d *Datastore) NewEmptyTree(ctx context.Context) (*tree.RootEntry, error) { func (d *Datastore) performRevert(ctx context.Context, t *tree.RootEntry) error { log := logger.FromContext(ctx) + + // Serialize with in-flight transactions: a concurrent delete transaction must + // fully complete (device write + cache delete) before we snapshot the intent + // store. Without this, LoadAllButRunningIntents can race with IntentDelete and + // push stale intent config back to the device, undoing the deletion. + d.dmutex.Lock() + defer d.dmutex.Unlock() + _, err := d.LoadAllButRunningIntents(ctx, t) if err != nil { return err From f6e4d40f615c1da62a9c5db649e91046aea7c2ee Mon Sep 17 00:00:00 2001 From: steiler Date: Tue, 2 Jun 2026 14:29:11 +0200 Subject: [PATCH 3/4] fix: initialize dmutex in sync_test.go Datastore fixture Co-authored-by: Cursor --- pkg/datastore/sync_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/datastore/sync_test.go b/pkg/datastore/sync_test.go index 1ae172bc..55f55056 100644 --- a/pkg/datastore/sync_test.go +++ b/pkg/datastore/sync_test.go @@ -374,6 +374,7 @@ func TestApplyToRunning(t *testing.T) { datastore := &Datastore{ syncTreeMutex: &sync.RWMutex{}, + dmutex: &sync.Mutex{}, syncTree: syncTree, taskPool: pool.NewSharedTaskPool(ctx, runtime.GOMAXPROCS(0)), cacheClient: tt.cacheClientFunc(ctrl), From 603901dfadc5468326cb57a90d90e1997131309a Mon Sep 17 00:00:00 2001 From: steiler Date: Tue, 2 Jun 2026 16:07:13 +0200 Subject: [PATCH 4/4] fix: map ErrTransactionOngoing to codes.Aborted and narrow dmutex scope ErrTransactionOngoing was falling through translateInternalToGrpcError and being wrapped by gRPC as codes.Unknown. config-server treats codes.Unknown as non-recoverable, causing a cascade of permanent failures instead of a backoff-and-retry when a transaction collision occurs. Map it to codes.Aborted alongside ErrDatastoreLocked so config-server correctly retries. Additionally, split the single dmutex acquisition in performRevert into two narrow critical sections: one covering LoadAllButRunningIntents (the cache snapshot that must be atomic with IntentDelete) and one covering applyIntent (the gNMI device write). FinishInsertionPhase, GetDeletes, and ToProtoUpdates operate only on the local tree copy and no longer hold the mutex, reducing contention on concurrent transactions. Co-authored-by: Cursor --- pkg/datastore/sync.go | 20 ++++++++++++++------ pkg/server/transaction.go | 3 +++ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/pkg/datastore/sync.go b/pkg/datastore/sync.go index 10f990f0..55422520 100644 --- a/pkg/datastore/sync.go +++ b/pkg/datastore/sync.go @@ -135,18 +135,20 @@ func (d *Datastore) NewEmptyTree(ctx context.Context) (*tree.RootEntry, error) { func (d *Datastore) performRevert(ctx context.Context, t *tree.RootEntry) error { log := logger.FromContext(ctx) - // Serialize with in-flight transactions: a concurrent delete transaction must - // fully complete (device write + cache delete) before we snapshot the intent - // store. Without this, LoadAllButRunningIntents can race with IntentDelete and - // push stale intent config back to the device, undoing the deletion. + // Critical section 1: snapshot the intent store. + // Hold dmutex so that any concurrent delete transaction (device write + + // cache delete) fully completes before we read the cache. Without this, + // LoadAllButRunningIntents can race with IntentDelete and push stale intent + // config back to the device, undoing the deletion. d.dmutex.Lock() - defer d.dmutex.Unlock() - _, err := d.LoadAllButRunningIntents(ctx, t) + d.dmutex.Unlock() if err != nil { return err } + // Tree processing operates only on the local copy t — no shared state is + // accessed, so no lock is required here. err = t.FinishInsertionPhase(ctx) if err != nil { return err @@ -172,6 +174,12 @@ func (d *Datastore) performRevert(ctx context.Context, t *tree.RootEntry) error } if performApply { + // Critical section 2: device write. + // Re-acquire dmutex to serialize the gNMI SET with in-flight + // transactions; a concurrent delete must not interleave its device write + // with ours. + d.dmutex.Lock() + defer d.dmutex.Unlock() log.Info("reverting after sync") resp, err := d.applyIntent(ctx, adapter.NewEntryOutputAdapter(t.Entry)) if err != nil { diff --git a/pkg/server/transaction.go b/pkg/server/transaction.go index 4e8be390..0cade8ae 100644 --- a/pkg/server/transaction.go +++ b/pkg/server/transaction.go @@ -156,5 +156,8 @@ func translateInternalToGrpcError(err error) error { if errors.Is(err, datastore.ErrDatastoreLocked) { return status.Error(codes.Aborted, err.Error()) } + if errors.Is(err, types.ErrTransactionOngoing) { + return status.Error(codes.Aborted, err.Error()) + } return err }