diff --git a/pkg/datastore/sync.go b/pkg/datastore/sync.go index 42101ec5..55422520 100644 --- a/pkg/datastore/sync.go +++ b/pkg/datastore/sync.go @@ -134,11 +134,21 @@ 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) + + // 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() _, 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 @@ -164,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/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), 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 } 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"; + } + } } }