Skip to content

Commit de2c33d

Browse files
tengyiGtengyiG
authored
Fix a bug in protoFromPathsInternal function in proto.go causing enca… (#1028)
* Fix a bug in protoFromPathsInternal function in proto.go causing encapsulation header details to be lost when mapping gNMI paths to protobuf fiedls. Root cause: the issue stems from incorrect path construction when recursing into nested messages. Something like this will happen: "DEBUG: recursing into nested message gribi_aft.Afts.NextHop.EncapHeader.mpls, path: elem:{name:"afts"} elem:{name:"next-hops"} elem:{name:"next-hop"} elem:{name:"encap-headers"} elem:{name:"encap-header" key:{key:"index" value:"11"}} elem:{name:"afts"} elem:{name:"next-hops"} elem:{name:"next-hop"} elem:{name:"encap-headers"} elem:{name:"encap-header"} elem:{name:"mpls"}. When findChilren uses these malformed paths to search for nested fields, it returns 0 children because no paths in the input map match this duplicated structure. * Fix a bug in protoFromPathsInternal function in proto.go causing encapsulation header details to be lost when mapping gNMI paths to protobuf fiedls. Root cause: the issue stems from incorrect path construction when recursing into nested messages. Something like this will happen: "DEBUG: recursing into nested message gribi_aft.Afts.NextHop.EncapHeader.mpls, path: elem:{name:"afts"} elem:{name:"next-hops"} elem:{name:"next-hop"} elem:{name:"encap-headers"} elem:{name:"encap-header" key:{key:"index" value:"11"}} elem:{name:"afts"} elem:{name:"next-hops"} elem:{name:"next-hop"} elem:{name:"encap-headers"} elem:{name:"encap-header"} elem:{name:"mpls"}. When findChilren uses these malformed paths to search for nested fields, it returns 0 children because no paths in the input map match this duplicated structure.
1 parent b49bc64 commit de2c33d

File tree

4 files changed

+251
-13
lines changed

4 files changed

+251
-13
lines changed

‎protomap/proto.go‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -540,7 +540,7 @@ func ProtoFromPaths(p proto.Message, vals map[*gpb.Path]interface{}, opt ...Unma
540540
if err != nil {
541541
return fmt.Errorf("invalid value prefix supplied, %v", err)
542542
}
543-
valPrefix = schemaPath(valPrefix)
543+
valPrefix = util.GNMIPathToSchemaPath(valPrefix)
544544

545545
protoPrefix, err := hasProtoMsgPrefix(opt)
546546
if err != nil {
@@ -550,15 +550,6 @@ func ProtoFromPaths(p proto.Message, vals map[*gpb.Path]interface{}, opt ...Unma
550550
return protoFromPathsInternal(p, vals, valPrefix, protoPrefix, hasIgnoreExtraPaths(opt))
551551
}
552552

553-
// schemaPath converts the path p into a schema path by removing all of the keys within the path.
554-
func schemaPath(p *gpb.Path) *gpb.Path {
555-
np := proto.Clone(p).(*gpb.Path)
556-
for _, e := range np.Elem {
557-
e.Key = nil
558-
}
559-
return np
560-
}
561-
562553
// findChildren returns the entries from the vals map that correspond to children of the specified protoPrefix path.
563554
// The valPrefix path is prepended to the paths within the vals map to make these values absolute. If the directOnly bool
564555
// is set to true, then only direct children (not subsequent descendents) are returned. If mustBeChildren is set to true
@@ -633,7 +624,7 @@ func protoFromPathsInternal(p proto.Message, vals map[*gpb.Path]any, valPrefix,
633624

634625
if len(directCh) != 0 {
635626
for _, ap := range annotatedPath {
636-
trimmedPrefix := schemaPath(protoPrefix)
627+
trimmedPrefix := util.GNMIPathToSchemaPath(protoPrefix)
637628
if !util.PathMatchesPathElemPrefix(ap, trimmedPrefix) {
638629
rangeErr = fmt.Errorf("annotation %s does not match the supplied prefix %s", ap, protoPrefix)
639630
return false
@@ -722,7 +713,10 @@ func protoFromPathsInternal(p proto.Message, vals map[*gpb.Path]any, valPrefix,
722713
default:
723714
childMsg := m.NewField(fd).Message()
724715
np := proto.Clone(valPrefix).(*gpb.Path)
725-
np.Elem = append(np.Elem, util.TrimGNMIPathElemPrefix(annotatedPath[0], protoPrefix).Elem...)
716+
ap := annotatedPath[0]
717+
718+
trimmed := util.TrimGNMIPathElemPrefixKeyAware(ap, protoPrefix)
719+
np.Elem = append(np.Elem, trimmed.Elem...)
726720

727721
// There may be paths that are not direct descendents, so do not error. Return indirect children too.
728722
children, err := findChildren(vals, valPrefix, np, false, false)
@@ -777,7 +771,7 @@ func createListField(m proto.Message, fd protoreflect.FieldDescriptor, fieldPath
777771
}
778772
// Since the fieldPath is a schema path, then we need to compare just schema paths
779773
// to avoid comparing the keys.
780-
if !util.PathMatchesPathElemPrefix(schemaPath(absPath), schemaPath(fieldPath)) {
774+
if !util.PathMatchesPathElemPrefix(util.GNMIPathToSchemaPath(absPath), util.GNMIPathToSchemaPath(fieldPath)) {
781775
continue
782776
}
783777
// The key of the list is in the last element of the absolute path in the values map (the values

‎protomap/proto_test.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1378,6 +1378,19 @@ func TestProtoFromPaths(t *testing.T) {
13781378
mustPath("/union"): "fish",
13791379
},
13801380
wantErrSubstring: `did not map path elem:{name:"union"}`,
1381+
}, {
1382+
desc: "nested list with complex keys - preserve keys when trimming path",
1383+
inProto: &epb.Subinterface{},
1384+
inVals: map[*gpb.Path]any{
1385+
mustPath("config/description"): "subint-description",
1386+
},
1387+
inOpt: []UnmapOpt{
1388+
ProtobufMessagePrefix(mustPath("interfaces/interface/subinterfaces/subinterface")),
1389+
ValuePathPrefix(mustPath("interfaces/interface[name=eth0]/subinterfaces/subinterface[index=42]")),
1390+
},
1391+
wantProto: &epb.Subinterface{
1392+
Description: &wpb.StringValue{Value: "subint-description"},
1393+
},
13811394
}}
13821395

13831396
for _, tt := range tests {

‎util/gnmi.go‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,34 @@ func TrimGNMIPathElemPrefix(path, prefix *gpb.Path) *gpb.Path {
178178
return out
179179
}
180180

181+
// GNMIPathToSchemaPath converts the path p into a schema path by removing all of the keys within the path.
182+
func GNMIPathToSchemaPath(p *gpb.Path) *gpb.Path {
183+
if p == nil {
184+
return nil
185+
}
186+
np := proto.Clone(p).(*gpb.Path)
187+
for _, e := range np.Elem {
188+
e.Key = nil
189+
}
190+
return np
191+
}
192+
193+
// TrimGNMIPathElemPrefixKeyAware trims the prefix from a path while properly handling keys.
194+
// Unlike TrimGNMIPathElemPrefix, this function compares paths as schema paths (without keys) to
195+
// determine the prefix relationship, which ensures correct handling when keys are present.
196+
func TrimGNMIPathElemPrefixKeyAware(path, prefix *gpb.Path) *gpb.Path {
197+
if path == nil || prefix == nil || len(prefix.Elem) == 0 || len(path.Elem) < len(prefix.Elem) {
198+
return path
199+
}
200+
201+
pathPrefix := &gpb.Path{Elem: path.Elem[:len(prefix.Elem)]}
202+
if !proto.Equal(GNMIPathToSchemaPath(pathPrefix), GNMIPathToSchemaPath(prefix)) {
203+
return path
204+
}
205+
206+
return &gpb.Path{Elem: path.Elem[len(prefix.Elem):]}
207+
}
208+
181209
// FindPathElemPrefix finds the longest common prefix of the paths specified.
182210
func FindPathElemPrefix(paths []*gpb.Path) *gpb.Path {
183211
var prefix *gpb.Path

‎util/gnmi_test.go‎

Lines changed: 203 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -929,6 +929,209 @@ func TestTrimGNMIPathElemPrefix(t *testing.T) {
929929
}
930930
}
931931

932+
func TestGNMIPathToSchemaPath(t *testing.T) {
933+
tests := []struct {
934+
name string
935+
in *gpb.Path
936+
want *gpb.Path
937+
}{{
938+
name: "nil path",
939+
in: nil,
940+
want: nil,
941+
}, {
942+
name: "empty path",
943+
in: &gpb.Path{},
944+
want: &gpb.Path{},
945+
}, {
946+
name: "path with no keys",
947+
in: &gpb.Path{
948+
Elem: []*gpb.PathElem{
949+
{Name: "one"},
950+
{Name: "two"},
951+
},
952+
},
953+
want: &gpb.Path{
954+
Elem: []*gpb.PathElem{
955+
{Name: "one"},
956+
{Name: "two"},
957+
},
958+
},
959+
}, {
960+
name: "path with keys",
961+
in: &gpb.Path{
962+
Elem: []*gpb.PathElem{
963+
{Name: "interfaces", Key: map[string]string{"name": "eth0"}},
964+
{Name: "config"},
965+
},
966+
},
967+
want: &gpb.Path{
968+
Elem: []*gpb.PathElem{
969+
{Name: "interfaces"},
970+
{Name: "config"},
971+
},
972+
},
973+
}, {
974+
name: "path with multiple keys",
975+
in: &gpb.Path{
976+
Elem: []*gpb.PathElem{
977+
{Name: "interfaces", Key: map[string]string{"name": "eth0"}},
978+
{Name: "subinterfaces", Key: map[string]string{"index": "0"}},
979+
{Name: "config"},
980+
},
981+
},
982+
want: &gpb.Path{
983+
Elem: []*gpb.PathElem{
984+
{Name: "interfaces"},
985+
{Name: "subinterfaces"},
986+
{Name: "config"},
987+
},
988+
},
989+
}}
990+
991+
for _, tt := range tests {
992+
t.Run(tt.name, func(t *testing.T) {
993+
got := util.GNMIPathToSchemaPath(tt.in)
994+
if diff := cmp.Diff(tt.want, got, protocmp.Transform()); diff != "" {
995+
t.Errorf("GNMIPathToSchemaPath(%v) returned unexpected diff (-want, +got):\n%s", tt.in, diff)
996+
}
997+
})
998+
}
999+
}
1000+
1001+
func TestTrimGNMIPathElemPrefixKeyAware(t *testing.T) {
1002+
tests := []struct {
1003+
desc string
1004+
inPath *gpb.Path
1005+
inPrefix *gpb.Path
1006+
want *gpb.Path
1007+
}{{
1008+
desc: "nil prefix",
1009+
inPath: &gpb.Path{Elem: []*gpb.PathElem{{Name: "a"}, {Name: "b"}}},
1010+
want: &gpb.Path{Elem: []*gpb.PathElem{{Name: "a"}, {Name: "b"}}},
1011+
}, {
1012+
desc: "empty prefix",
1013+
inPath: &gpb.Path{Elem: []*gpb.PathElem{{Name: "a"}, {Name: "b"}}},
1014+
inPrefix: &gpb.Path{},
1015+
want: &gpb.Path{Elem: []*gpb.PathElem{{Name: "a"}, {Name: "b"}}},
1016+
}, {
1017+
desc: "prefix longer than path",
1018+
inPath: &gpb.Path{Elem: []*gpb.PathElem{{Name: "a"}}},
1019+
inPrefix: &gpb.Path{Elem: []*gpb.PathElem{{Name: "a"}, {Name: "b"}}},
1020+
want: &gpb.Path{Elem: []*gpb.PathElem{{Name: "a"}}},
1021+
}, {
1022+
desc: "normal prefix without keys",
1023+
inPath: &gpb.Path{Elem: []*gpb.PathElem{{Name: "a"}, {Name: "b"}, {Name: "c"}}},
1024+
inPrefix: &gpb.Path{Elem: []*gpb.PathElem{{Name: "a"}, {Name: "b"}}},
1025+
want: &gpb.Path{Elem: []*gpb.PathElem{{Name: "c"}}},
1026+
}, {
1027+
desc: "prefix with keys matches path with keys",
1028+
inPath: &gpb.Path{Elem: []*gpb.PathElem{
1029+
{Name: "interfaces", Key: map[string]string{"name": "eth0"}},
1030+
{Name: "config"},
1031+
{Name: "description"},
1032+
}},
1033+
inPrefix: &gpb.Path{Elem: []*gpb.PathElem{
1034+
{Name: "interfaces", Key: map[string]string{"name": "eth0"}},
1035+
{Name: "config"},
1036+
}},
1037+
want: &gpb.Path{Elem: []*gpb.PathElem{{Name: "description"}}},
1038+
}, {
1039+
desc: "prefix with diff key values still matches as schema path",
1040+
inPath: &gpb.Path{Elem: []*gpb.PathElem{
1041+
{Name: "interfaces", Key: map[string]string{"name": "eth0"}},
1042+
{Name: "config"},
1043+
{Name: "description"},
1044+
}},
1045+
inPrefix: &gpb.Path{Elem: []*gpb.PathElem{
1046+
{Name: "interfaces", Key: map[string]string{"name": "eth1"}},
1047+
{Name: "config"},
1048+
}},
1049+
want: &gpb.Path{Elem: []*gpb.PathElem{{Name: "description"}}},
1050+
}, {
1051+
desc: "prefix with no keys matches path with keys",
1052+
inPath: &gpb.Path{Elem: []*gpb.PathElem{
1053+
{Name: "interfaces", Key: map[string]string{"name": "eth0"}},
1054+
{Name: "config"},
1055+
{Name: "description"},
1056+
}},
1057+
inPrefix: &gpb.Path{Elem: []*gpb.PathElem{
1058+
{Name: "interfaces"},
1059+
{Name: "config"},
1060+
}},
1061+
want: &gpb.Path{Elem: []*gpb.PathElem{{Name: "description"}}},
1062+
}, {
1063+
desc: "demonstrates bug fix (outer structure match even with keys)",
1064+
inPath: &gpb.Path{Elem: []*gpb.PathElem{
1065+
{Name: "interfaces", Key: map[string]string{"name": "eth0"}},
1066+
{Name: "encap-headers", Key: map[string]string{"index": "11"}},
1067+
{Name: "mpls"},
1068+
{Name: "label-stack"},
1069+
}},
1070+
inPrefix: &gpb.Path{Elem: []*gpb.PathElem{
1071+
{Name: "interfaces"},
1072+
{Name: "encap-headers"},
1073+
}},
1074+
want: &gpb.Path{Elem: []*gpb.PathElem{
1075+
{Name: "mpls"},
1076+
{Name: "label-stack"},
1077+
}},
1078+
}, {
1079+
desc: "non-matching prefix returns original path",
1080+
inPath: &gpb.Path{Elem: []*gpb.PathElem{
1081+
{Name: "interfaces", Key: map[string]string{"name": "eth0"}},
1082+
{Name: "config"},
1083+
}},
1084+
inPrefix: &gpb.Path{Elem: []*gpb.PathElem{
1085+
{Name: "different-prefix"},
1086+
}},
1087+
want: &gpb.Path{Elem: []*gpb.PathElem{
1088+
{Name: "interfaces", Key: map[string]string{"name": "eth0"}},
1089+
{Name: "config"},
1090+
}},
1091+
}}
1092+
1093+
for _, tt := range tests {
1094+
t.Run(tt.desc, func(t *testing.T) {
1095+
got := util.TrimGNMIPathElemPrefixKeyAware(tt.inPath, tt.inPrefix)
1096+
if diff := cmp.Diff(tt.want, got, protocmp.Transform()); diff != "" {
1097+
t.Errorf("TrimGNMIPathElemPrefixKeyAware(%v, %v) returned unexpected diff (-want, +got):\n%s",
1098+
tt.inPath, tt.inPrefix, diff)
1099+
}
1100+
})
1101+
}
1102+
}
1103+
1104+
func TestCompareTrimFunctions(t *testing.T) {
1105+
// This test demonstrates the difference between the original TrimGNMIPathElemPrefix
1106+
// and the new key-aware version by showing a case where they behave differently
1107+
path := &gpb.Path{Elem: []*gpb.PathElem{
1108+
{Name: "interfaces", Key: map[string]string{"name": "eth0"}},
1109+
{Name: "config"},
1110+
{Name: "description"},
1111+
}}
1112+
1113+
prefix := &gpb.Path{Elem: []*gpb.PathElem{
1114+
{Name: "interfaces", Key: map[string]string{"name": "eth1"}}, // Note different key value
1115+
{Name: "config"},
1116+
}}
1117+
1118+
trimResultWithoutKeyAware := util.TrimGNMIPathElemPrefix(path, prefix)
1119+
keyAwareTrimResult := util.TrimGNMIPathElemPrefixKeyAware(path, prefix)
1120+
1121+
// Original trim should not trim because keys don't match exactly
1122+
if !proto.Equal(trimResultWithoutKeyAware, path) {
1123+
t.Errorf("Original trim did not preserve path with different keys: got %v, want %v",
1124+
trimResultWithoutKeyAware, path)
1125+
}
1126+
1127+
// Key-aware trim should trim because schema paths match after removing keys
1128+
wantTrimmed := &gpb.Path{Elem: []*gpb.PathElem{{Name: "description"}}}
1129+
if !proto.Equal(keyAwareTrimResult, wantTrimmed) {
1130+
t.Errorf("Key-aware trim did not correctly trim path: got %v, want %v",
1131+
keyAwareTrimResult, wantTrimmed)
1132+
}
1133+
}
1134+
9321135
func TestFindPathElemPrefix(t *testing.T) {
9331136
tests := []struct {
9341137
name string

0 commit comments

Comments
 (0)