openconfig / ygot

A YANG-centric Go toolkit - Go/Protobuf Code Generation; Validation; Marshaling/Unmarshaling
Apache License 2.0
286 stars 107 forks source link

If a leaf-list has a relative path leafref, validation fails #623

Closed hansthienpondt closed 2 years ago

hansthienpondt commented 2 years ago

Hi,

Considering the following yang model, validation fails with following debugLibrary output:

DataNodeAtPath got leafref with path elem:{name:".."} elem:{name:".."} elem:{name:".."} elem:{name:"two"} elem:{name:"targetlist"} elem:{name:"targetname"} from node path /device/root/one/referencelist/target, field name Target
going up data tree from type string to []string, schema path from parent is [target], remaining path elem:{name:".."} elem:{name:".."} elem:{name:"two"} elem:{name:"targetlist"} elem:{name:"targetname"}
going up data tree from type []string to *hansygot.Uncompressed_Root_One_Referencelist, schema path from parent is [target], remaining path elem:{name:".."} elem:{name:"two"} elem:{name:"targetlist"} elem:{name:"targetname"}
going up data tree from type map[string]*hansygot.Uncompressed_Root_One_Referencelist to *hansygot.Uncompressed_Root_One, schema path from parent is [referencelist], remaining path elem:{name:"two"} elem:{name:"targetlist"} elem:{name:"targetname"}
root element type *hansygot.Uncompressed_Root_One with remaining path elem:{name:"two"} elem:{name:"targetlist"} elem:{name:"targetname"}
. GetNode next path name:"two", value { map[key:0xc000199160] (map ptr) }
. getNodesContainer: schema one, next path name:"two", value { map[key:0xc000199160] (map ptr) }
childSchema for schema one, field Referencelist, tag referencelist
RelativeSchemaPath yields [referencelist]
traversing schema Dirs.../referencelist - found
. check field name referencelist, paths [[referencelist]]
. ERR: could not find path in tree beyond schema node one, (type *hansygot.Uncompressed_Root_One), remaining path elem:{name:"two"} elem:{name:"targetlist"} elem:{name:"targetname"}
. Validate with value { { { map[key:0xc000199160] (map ptr ptr ptr) }, { map[mypolicy:0xc000010c38] (map ptr ptr ptr) } } }, type *hansygot.Device, schema name device
. validateContainer with value { { { map[key:0xc000199160] (map ptr ptr ptr) }, { map[mypolicy:0xc000010c38] (map ptr ptr ptr) } } }, type *hansygot.Device, schema name device
childSchema for schema device, field Root, tag root
RelativeSchemaPath yields [root]
traversing schema Dirs.../root - found
. Validate with value { { map[key:0xc000199160] (map ptr ptr) }, { map[mypolicy:0xc000010c38] (map ptr ptr) } }, type *hansygot.Uncompressed_Root, schema name root
. validateContainer with value { { map[key:0xc000199160] (map ptr ptr) }, { map[mypolicy:0xc000010c38] (map ptr ptr) } }, type *hansygot.Uncompressed_Root, schema name root
childSchema for schema root, field One, tag one
RelativeSchemaPath yields [one]
traversing schema Dirs.../one - found
. Validate with value { map[key:0xc000199160] (map ptr) }, type *hansygot.Uncompressed_Root_One, schema name one
. validateContainer with value { map[key:0xc000199160] (map ptr) }, type *hansygot.Uncompressed_Root_One, schema name one
childSchema for schema one, field Referencelist, tag referencelist
RelativeSchemaPath yields [referencelist]
traversing schema Dirs.../referencelist - found
. Validate with value map[key:0xc000199160] (map), type map[string]*hansygot.Uncompressed_Root_One_Referencelist, schema name referencelist
. validateList with value map[key:0xc000199160], type map[string]*hansygot.Uncompressed_Root_One_Referencelist, schema name referencelist
childSchema for schema referencelist, field Name, tag name
RelativeSchemaPath yields [name]
traversing schema Dirs.../name - found
. Validate with value key (string ptr), type *string, schema name name
. validateLeaf with value key (string ptr) (*string), schema name name (string)
childSchema for schema referencelist, field Target, tag target
RelativeSchemaPath yields [target]
traversing schema Dirs.../target - found
. Validate with value [ mypolicy (string) ], type []string, schema name target
. validateLeafList with value [ mypolicy (string) ], type []string, schema name target
. validateLeaf with value mypolicy (string ptr) (*interface {}), schema name target (leafref)
. follow schema leaf-ref from target to targetname, type string
childSchema for schema root, field Two, tag two
RelativeSchemaPath yields [two]
traversing schema Dirs.../two - found
. Validate with value { map[mypolicy:0xc000010c38] (map ptr) }, type *hansygot.Uncompressed_Root_Two, schema name two
. validateContainer with value { map[mypolicy:0xc000010c38] (map ptr) }, type *hansygot.Uncompressed_Root_Two, schema name two
childSchema for schema two, field Targetlist, tag targetlist
RelativeSchemaPath yields [targetlist]
traversing schema Dirs.../targetlist - found
. Validate with value map[mypolicy:0xc000010c38] (map), type map[string]*hansygot.Uncompressed_Root_Two_Targetlist, schema name targetlist
. validateList with value map[mypolicy:0xc000010c38], type map[string]*hansygot.Uncompressed_Root_Two_Targetlist, schema name targetlist
childSchema for schema targetlist, field Targetname, tag targetname
RelativeSchemaPath yields [targetname]
traversing schema Dirs.../targetname - found
. Validate with value mypolicy (string ptr), type *string, schema name targetname
. validateLeaf with value mypolicy (string ptr) (*string), schema name targetname (string)
panic: validation err: could not find path in tree beyond schema node one, (type *hansygot.Uncompressed_Root_One), remaining path elem:{name:"two"} elem:{name:"targetlist"} elem:{name:"targetname"}

I've used following generator args, ygot v0.15.1:

//go:generate bash -c "/home/hans/git/ygot/generator/generator -output_file=hansygot/ygot.go -package_name=hansygot -generate_fakeroot -fakeroot_name device -path=. -typedef_enum_with_defmod  -shorten_enum_leaf_names  -exclude_state -generate_simple_unions -generate_append -generate_delete -generate_getters bug.yang"

I suspect it's related to ytypes/leafref.go.

I've implemented a dirty workaround, but considering it's a breaking change, reaching out for help as I don't know the codebase well enough yet.

robshakir commented 2 years ago

Thanks for this - I suspect this is the confluence of uncompressed schema generation, and leafref validation. @wenovus - might you have a moment to look at this at some point?

wenovus commented 2 years ago

I think the fix is essentially correct: we handle the case of lists (a single level in YANG) being represented as two levels in Go (map -> element), but don't handle the case of leaf-lists also being represented as two levels in Go (slice -> element). The "two levels" is how ForEachField processes elements, which creates this corner case for DataNodeAtPath.

It turns out we already had a test case for this but it had an extra "../" so it was testing the wrong behaviour.

I made the fixes to the testcase and everything in ygot looks to be passing:

diff --git a/ytypes/leafref.go b/ytypes/leafref.go
index 1e0e67e..07b0c6b 100644
--- a/ytypes/leafref.go
+++ b/ytypes/leafref.go
@@ -189,7 +189,7 @@ func dataNodesAtPath(ni *util.NodeInfo, path *gpb.Path, pathQueryNode *util.Path
                        if root.Parent == nil {
                                return nil, fmt.Errorf("no parent for leafref path at %v, with remaining path %s", ni.Schema.Path(), path)
                        }
-                       if !util.IsCompressedSchema(root.Schema) && root.Parent.Schema.IsList() && util.IsValueMap(root.Parent.FieldValue) {
+                       if !util.IsCompressedSchema(root.Schema) && ((root.Parent.Schema.IsList() && util.IsValueMap(root.Parent.FieldValue)) || (root.Parent.Schema.IsLeafList() && util.IsValueSlice(root.Parent.FieldValue))) {
                                // If we are in an uncompressed schema, then we have one more level of the data tree than
                                // the YANG expects, since our data tree layout is:
                                // struct (parent container)
diff --git a/ytypes/leafref_test.go b/ytypes/leafref_test.go
index 6c342a4..23c3ebc 100644
--- a/ytypes/leafref_test.go
+++ b/ytypes/leafref_test.go
@@ -152,7 +152,7 @@ func TestValidateLeafRefData(t *testing.T) {
                                                Kind: yang.LeafEntry,
                                                Type: &yang.YangType{
                                                        Kind: yang.Yleafref,
-                                                       Path: "../../../leaf-list",
+                                                       Path: "../../leaf-list",
                                                },
                                                ListAttr: yang.NewDefaultListAttr(),
                                        },
@@ -336,7 +336,7 @@ func TestValidateLeafRefData(t *testing.T) {
                                LeafList:   []*int32{Int32(40), Int32(41), Int32(42)},
                                Container2: &Container2{LeafListRefToLeafList: []*int32{Int32(41), Int32(42), Int32(43)}},
                        },
-                       wantErr: `field name LeafListRefToLeafList value 43 (int32 ptr) schema path /leaf-list-ref-to-leaf-list has leafref path ../../../leaf-list not equal to any target nodes`,
+                       wantErr: `field name LeafListRefToLeafList value 43 (int32 ptr) schema path /leaf-list-ref-to-leaf-list has leafref path ../../leaf-list not equal to any target nodes`,
                },
                {
                        desc: "keyed list match",

@hansthienpondt I think we're ready for a PR for this fix.

hansthienpondt commented 2 years ago

Fix in https://github.com/openconfig/ygot/pull/626