From 3aa2434c12e7595357212317f17d772567bc6c25 Mon Sep 17 00:00:00 2001 From: wenovus Date: Mon, 21 Jun 2021 15:32:49 -0700 Subject: [PATCH] Support explicitly setting proto field tags. Credits to @jasonewang for posting the first implementation at #339. I've modified this a bit to read the extension values from the new openconfig-codegen-extensions.yang model that's now in openconfig/public, and also to address some of the corner cases that were discussed in the PR. Spec: https://github.com/openconfig/ygot/blob/master/docs/yang-to-protobuf-transformations-spec.md#field-numbering Corner cases not mentioned in the spec: 1. multiple `field-number` extensions. - Error out. 1. multiple `field-number-offset` extensions. - Add them up. 1. `field-number` or `field-number-offset` extensions on non-leaves. - Silently ignore. 1. `field-number-offset` on non-uses statements, including leafs. - Allow. 1. Conflicts. - Allow. The proto compilation will of course fail. Automated checking can be added here if we'd prefer. I made some simple choices for the above items. Erroring out for 3 and 4 would likely require changing `goyang` to specifically support checking these cases, which I'm not sure is worth it here. --- go.mod | 2 +- go.sum | 4 +- ygen/codegen_test.go | 7 +- ygen/protogen.go | 67 +++--- ygen/protogen_test.go | 194 +++++++++++++++++- .../proto/openconfig-codegen-extensions.yang | 80 ++++++++ ...test-a.compress.parent.child.formatted-txt | 3 +- ...proto-test-a.compress.parent.formatted-txt | 1 + ...st-a.nocompress.parent.child.formatted-txt | 2 +- ygen/testdata/proto/proto-test-a.yang | 6 +- 10 files changed, 323 insertions(+), 43 deletions(-) create mode 100644 ygen/testdata/proto/openconfig-codegen-extensions.yang diff --git a/go.mod b/go.mod index 71d7d6f38..98dbdf026 100644 --- a/go.mod +++ b/go.mod @@ -8,7 +8,7 @@ require ( github.com/google/go-cmp v0.5.5 github.com/kylelemons/godebug v1.1.0 github.com/openconfig/gnmi v0.0.0-20200508230933-d19cebf5e7be - github.com/openconfig/goyang v0.2.5 + github.com/openconfig/goyang v0.2.6 github.com/openconfig/gribi v0.1.1-0.20210423184541-ce37eb4ba92f github.com/pmezard/go-difflib v1.0.0 google.golang.org/genproto v0.0.0-20201214200347-8c77b98c765d diff --git a/go.sum b/go.sum index 423902501..705900b8a 100644 --- a/go.sum +++ b/go.sum @@ -44,8 +44,8 @@ github.com/openconfig/gnmi v0.0.0-20200508230933-d19cebf5e7be h1:VEK8utxoyZu/hkp github.com/openconfig/gnmi v0.0.0-20200508230933-d19cebf5e7be/go.mod h1:M/EcuapNQgvzxo1DDXHK4tx3QpYM/uG4l591v33jG2A= github.com/openconfig/goyang v0.0.0-20200115183954-d0a48929f0ea/go.mod h1:dhXaV0JgHJzdrHi2l+w0fZrwArtXL7jEFoiqLEdmkvU= github.com/openconfig/goyang v0.2.2/go.mod h1:vX61x01Q46AzbZUzG617vWqh/cB+aisc+RrNkXRd3W8= -github.com/openconfig/goyang v0.2.5 h1:ZvV+5cF5thPFun1H6/Itt/Jbwb/bKmjS0o8imN+7ddw= -github.com/openconfig/goyang v0.2.5/go.mod h1:vX61x01Q46AzbZUzG617vWqh/cB+aisc+RrNkXRd3W8= +github.com/openconfig/goyang v0.2.6 h1:uJlr2bOUe/9mTtnIXgWG9VBqZd/BeCKur4+wAU2jJPc= +github.com/openconfig/goyang v0.2.6/go.mod h1:vX61x01Q46AzbZUzG617vWqh/cB+aisc+RrNkXRd3W8= github.com/openconfig/gribi v0.1.1-0.20210423184541-ce37eb4ba92f h1:8vRtC+y0xh9BYPrEGf/jG/paYXiDUJ6P8iYt5rCVols= github.com/openconfig/gribi v0.1.1-0.20210423184541-ce37eb4ba92f/go.mod h1:OoH46A2kV42cIXGyviYmAlGmn6cHjGduyC2+I9d/iVs= github.com/openconfig/ygot v0.6.0/go.mod h1:o30svNf7O0xK+R35tlx95odkDmZWS9JyWWQSmIhqwAs= diff --git a/ygen/codegen_test.go b/ygen/codegen_test.go index 2cf694643..ccdabf549 100644 --- a/ygen/codegen_test.go +++ b/ygen/codegen_test.go @@ -1967,8 +1967,11 @@ func TestGenerateProto3(t *testing.T) { wantOutputFiles map[string]string wantErr bool }{{ - name: "simple protobuf test with compression", - inFiles: []string{filepath.Join(TestRoot, "testdata", "proto", "proto-test-a.yang")}, + name: "simple protobuf test with compression", + inFiles: []string{ + filepath.Join(TestRoot, "testdata", "proto", "proto-test-a.yang"), + filepath.Join(TestRoot, "testdata", "proto", "openconfig-codegen-extensions.yang"), + }, inConfig: GeneratorConfig{ TransformationOptions: TransformationOpts{ CompressBehaviour: genutil.PreferIntendedConfig, diff --git a/ygen/protogen.go b/ygen/protogen.go index deae832d6..e34351f79 100644 --- a/ygen/protogen.go +++ b/ygen/protogen.go @@ -93,6 +93,13 @@ const ( protoMatchingListNameKeySuffix = "key" ) +// Names from https://github.com/openconfig/public/blob/master/release/models/extensions/openconfig-codegen-extensions.yang +const ( + codegenExtModuleName = "openconfig-codegen-extensions" + fieldNumExtName = "field-number" + offsetExtName = "field-number-offset" +) + // protoMsgField describes a field of a protobuf message. // Note, throughout this package private structs that have public fields are used // in text/template which cannot refer to unexported fields. @@ -1048,40 +1055,44 @@ func safeProtoIdentifierName(name string) string { } // protoTagForEntry returns a protobuf tag value for the entry e. +// If the entry has user-specified field numberings, then use that instead: +// https://github.com/openconfig/ygot/blob/master/docs/yang-to-protobuf-transformations-spec.md#field-numbering func protoTagForEntry(e *yang.Entry) (uint32, error) { - var offset uint32 - for _, ext := range e.Exts { - if ext.Keyword == "occodegenext:field-number-offset" { - off, err := strconv.ParseUint(ext.Argument, 10, 32) - if err != nil { - return 0, fmt.Errorf("could not parse %s %s: %s", ext.Keyword, ext.Argument, err) - } - offset = uint32(off) - if offset == 0 { - return 0, fmt.Errorf("%s cannot be 0", ext.Keyword) - } - break + var fieldNum uint32 + exts, err := yang.MatchingEntryExtensions(e, codegenExtModuleName, fieldNumExtName) + if err != nil { + return 0, err + } + switch len(exts) { + case 0: + return fieldTag(e.Path()) + case 1: + num, err := strconv.ParseUint(exts[0].Argument, 10, 32) + if err != nil || num == 0 { + return 0, fmt.Errorf("could not parse %s:%s %q as a non-zero integer: %s", codegenExtModuleName, fieldNumExtName, exts[0].Argument, err) } + fieldNum = uint32(num) + default: + return 0, fmt.Errorf("more than one %s:%s defined", codegenExtModuleName, fieldNumExtName) } - for _, ext := range e.Exts { - if ext.Keyword == "occodegenext:field-number" { - fn, err := strconv.ParseUint(ext.Argument, 10, 32) - if err != nil { - return 0, fmt.Errorf("could not parse %s %s: %s", ext.Keyword, ext.Argument, err) - } - if fn == 0 { - return 0, fmt.Errorf("%s cannot be 0", ext.Keyword) - } - annotatedFieldNumber := uint32(fn) + offset - if annotatedFieldNumber > 1000 || annotatedFieldNumber < 1 { - return 0, fmt.Errorf("%s field number %d not in annotation reserved range of 1-1000", - e.Name, annotatedFieldNumber) - } - return annotatedFieldNumber, nil + + exts, err = yang.MatchingEntryExtensions(e, codegenExtModuleName, offsetExtName) + if err != nil { + return 0, err + } + for _, ext := range exts { + num, err := strconv.ParseUint(ext.Argument, 10, 32) + if err != nil { + return 0, fmt.Errorf("could not parse %s:%s %q as an uint: %s", codegenExtModuleName, offsetExtName, ext.Argument, err) } + // Allow for multiple offsets to accumulate. + fieldNum += uint32(num) } - return fieldTag(e.Path()) + if fieldNum > 1000 || fieldNum < 1 { + return 0, fmt.Errorf("%s: %d not in reserved range of 1-1000 for %s:%s", e.Path(), fieldNum, codegenExtModuleName, offsetExtName) + } + return fieldNum, nil } // fieldTag takes an input string and calculates a FNV hash for the value. If the diff --git a/ygen/protogen_test.go b/ygen/protogen_test.go index 31d6a1fec..f649cf250 100644 --- a/ygen/protogen_test.go +++ b/ygen/protogen_test.go @@ -20,6 +20,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/kylelemons/godebug/pretty" + "github.com/openconfig/gnmi/errdiff" "github.com/openconfig/goyang/pkg/yang" "github.com/openconfig/ygot/testutil" ) @@ -63,7 +64,7 @@ func TestGenProto3Msg(t *testing.T) { inParentPackage string inChildMsgs []*generatedProto3Message wantMsgs map[string]*protoMsg - wantErr bool + wantErrSubstring string }{{ name: "simple message with only scalar fields", inMsg: &Directory{ @@ -539,7 +540,7 @@ func TestGenProto3Msg(t *testing.T) { }, }, }, - wantErr: true, + wantErrSubstring: "could not resolve", }, { name: "message with an unimplemented mapping", inMsg: &Directory{ @@ -565,7 +566,7 @@ func TestGenProto3Msg(t *testing.T) { }, Path: []string{"", "messasge-with-invalid-contents", "unimplemented"}, }, - wantErr: true, + wantErrSubstring: "unimplemented type", }, { name: "message with any anydata field", inMsg: &Directory{ @@ -673,6 +674,26 @@ func TestGenProto3Msg(t *testing.T) { Argument: "100", }, }, + Node: &yang.Leaf{ + Name: "field-one", + Parent: &yang.Container{ + Name: "message-name", + Parent: &yang.Module{ + Name: "module", + Prefix: &yang.Value{ + Name: "foo", + }, + Import: []*yang.Import{{ + Prefix: &yang.Value{ + Name: "occodegenext", + }, + Module: &yang.Module{ + Name: codegenExtModuleName, + }, + }}, + }, + }, + }, }, "field-two": { Name: "field-two", @@ -683,6 +704,26 @@ func TestGenProto3Msg(t *testing.T) { Argument: "1", }, }, + Node: &yang.Leaf{ + Name: "field-two", + Parent: &yang.Container{ + Name: "message-name", + Parent: &yang.Module{ + Name: "module", + Prefix: &yang.Value{ + Name: "foo", + }, + Import: []*yang.Import{{ + Prefix: &yang.Value{ + Name: "occodegenext", + }, + Module: &yang.Module{ + Name: codegenExtModuleName, + }, + }}, + }, + }, + }, }, "field-three": { Name: "field-three", @@ -697,6 +738,64 @@ func TestGenProto3Msg(t *testing.T) { Argument: "100", }, }, + Node: &yang.Leaf{ + Name: "field-three", + Parent: &yang.Container{ + Name: "message-name", + Parent: &yang.Module{ + Name: "module", + Prefix: &yang.Value{ + Name: "foo", + }, + Import: []*yang.Import{{ + Prefix: &yang.Value{ + Name: "occodegenext", + }, + Module: &yang.Module{ + Name: codegenExtModuleName, + }, + }}, + }, + }, + }, + }, + "field-four": { + Name: "field-four", + Type: &yang.YangType{Kind: yang.Yint8}, + Exts: []*yang.Statement{ + { + Keyword: "occodegenext:field-number", + Argument: "1", + }, + { + Keyword: "occodegenext:field-number-offset", + Argument: "100", + }, + { + Keyword: "occodegenext:field-number-offset", + Argument: "200", + }, + }, + Node: &yang.Leaf{ + Name: "field-four", + Parent: &yang.Container{ + Name: "message-name", + Parent: &yang.Module{ + Name: "module", + Prefix: &yang.Value{ + Name: "foo", + }, + Import: []*yang.Import{{ + Prefix: &yang.Value{ + Name: "occodegenext", + }, + Module: &yang.Module{ + Name: codegenExtModuleName, + }, + }}, + }, + }, + }, }, }, Path: []string{"", "root", "message-name"}, @@ -719,6 +818,10 @@ func TestGenProto3Msg(t *testing.T) { Tag: 101, Name: "field_three", Type: "ywrapper.IntValue", + }, { + Tag: 301, + Name: "field_four", + Type: "ywrapper.IntValue", }}, }, }, @@ -745,11 +848,79 @@ func TestGenProto3Msg(t *testing.T) { Argument: "1000", }, }, + Node: &yang.Leaf{ + Name: "field-one", + Parent: &yang.Container{ + Name: "message-name", + Parent: &yang.Module{ + Name: "module", + Prefix: &yang.Value{ + Name: "foo", + }, + Import: []*yang.Import{{ + Prefix: &yang.Value{ + Name: "occodegenext", + }, + Module: &yang.Module{ + Name: codegenExtModuleName, + }, + }}, + }, + }, + }, }, }, Path: []string{"", "root", "message-name"}, }, - wantErr: true, + wantErrSubstring: "not in reserved range", + }, { + name: "message with duplicate field numbers", + inMsg: &Directory{ + Name: "MessageName", + Entry: &yang.Entry{ + Name: "message-name", + Dir: map[string]*yang.Entry{}, + Kind: yang.DirectoryEntry, + }, + Fields: map[string]*yang.Entry{ + "field-one": { + Name: "field-one", + Type: &yang.YangType{Kind: yang.Ystring}, + Exts: []*yang.Statement{ + { + Keyword: "occodegenext:field-number", + Argument: "1", + }, + { + Keyword: "occodegenext:field-number", + Argument: "1000", + }, + }, + Node: &yang.Leaf{ + Name: "field-one", + Parent: &yang.Container{ + Name: "message-name", + Parent: &yang.Module{ + Name: "module", + Prefix: &yang.Value{ + Name: "foo", + }, + Import: []*yang.Import{{ + Prefix: &yang.Value{ + Name: "occodegenext", + }, + Module: &yang.Module{ + Name: codegenExtModuleName, + }, + }}, + }, + }, + }, + }, + }, + Path: []string{"", "root", "message-name"}, + }, + wantErrSubstring: "more than one", }} for _, tt := range tests { @@ -771,11 +942,20 @@ func TestGenProto3Msg(t *testing.T) { annotateSchemaPaths: tt.inAnnotateSchemaPaths, }, tt.inParentPackage, tt.inChildMsgs, true, true) - if (errs != nil) != tt.wantErr { - t.Errorf("s: genProtoMsg(%#v, %#v, *genState, %v, %v, %s, %s): did not get expected error status, got: %v, wanted err: %v", tt.name, tt.inMsg, tt.inMsgs, tt.inCompressPaths, tt.inBasePackage, tt.inEnumPackage, errs, tt.wantErr) + var err error + switch len(errs) { + case 1: + err = errs[0] + fallthrough + case 0: + if diff := errdiff.Substring(err, tt.wantErrSubstring); diff != "" { + t.Fatalf("s: genProtoMsg(%#v, %#v, *genState, %v, %v, %s, %s): did not get expected error: %s", tt.name, tt.inMsg, tt.inMsgs, tt.inCompressPaths, tt.inBasePackage, tt.inEnumPackage, diff) + } + default: + t.Fatalf("got more than 1 error: %v", errs) } - if tt.wantErr { + if tt.wantErrSubstring != "" { return } diff --git a/ygen/testdata/proto/openconfig-codegen-extensions.yang b/ygen/testdata/proto/openconfig-codegen-extensions.yang new file mode 100644 index 000000000..da7631e6c --- /dev/null +++ b/ygen/testdata/proto/openconfig-codegen-extensions.yang @@ -0,0 +1,80 @@ +module openconfig-codegen-extensions { + + yang-version "1"; + + // namespace + namespace "http://openconfig.net/yang/openconfig-codegen-ext"; + + prefix "oc-codegen-ext"; + + // import some basic types + import openconfig-extensions { prefix oc-ext; } + + // meta + organization "OpenConfig working group"; + + contact + "OpenConfig working group + www.openconfig.net"; + + description + "This module provides OpenConfig-specific code generation extensions to the + YANG language."; + + oc-ext:openconfig-version "0.1.0"; + + revision "2020-10-05" { + description + "Initial commit of code generation extensions."; + reference "0.1.0"; + } + + // extension statements + + extension camelcase-name { + argument "name" { + yin-element false; + } + description + "When specified, this extension indicates a specific CamelCase name that + is to be used for the field, for example, this can allow for more natural + capitalisation than can be achieved algorithmically (e.g., IPv4Address rather + than Ipv4Address)."; + } + + extension field-number { + argument "number" { + yin-element false; + } + description + "field-number is used to explicitly specify the field number used in the + protocol buffer generated code instead of auto-generating them. + + Only 1-1000 are valid numbers. The rest is either reserved for Protobuf + internal usage or for use by the generated code when generating field + numbers automatically. + + Specification at + https://github.com/openconfig/ygot/blob/master/docs/yang-to-protobuf-transformations-spec.md#field-numbering"; + } + + extension field-number-offset { + argument "offset" { + yin-element false; + } + description + "field-number-offset is used within a uses statement to specify the + offset that is added to every explicit field-number for fields directly + within the grouping. + + When field-number is used to explicitly specify Protobuf field numbers + used in the protocol buffer generated code, it's possible that different + fields having the same field number could collide when multiple logical + groupings are imported into the same schema tree location. + field-number-offset helps resolve this by adding a different offset to + each grouping. + + Specification at + https://github.com/openconfig/ygot/blob/master/docs/yang-to-protobuf-transformations-spec.md#field-numbering"; + } +} diff --git a/ygen/testdata/proto/proto-test-a.compress.parent.child.formatted-txt b/ygen/testdata/proto/proto-test-a.compress.parent.child.formatted-txt index 656aa627d..a49c5387d 100644 --- a/ygen/testdata/proto/proto-test-a.compress.parent.child.formatted-txt +++ b/ygen/testdata/proto/proto-test-a.compress.parent.child.formatted-txt @@ -3,6 +3,7 @@ // // Input schema modules: // - testdata/proto/proto-test-a.yang +// - testdata/proto/openconfig-codegen-extensions.yang syntax = "proto3"; package openconfig.parent; @@ -12,7 +13,7 @@ import "github.com/openconfig/ygot/proto/yext/yext.proto"; // Child represents the /proto-test-a/parent/child YANG schema element. message Child { - ywrapper.BoolValue boolean = 101; + ywrapper.BoolValue boolean = 201; ywrapper.IntValue integer = 367917455; repeated ywrapper.StringValue leaf_list = 370551192; ywrapper.StringValue leaf_with_dashes = 503746721; diff --git a/ygen/testdata/proto/proto-test-a.compress.parent.formatted-txt b/ygen/testdata/proto/proto-test-a.compress.parent.formatted-txt index bcb9c5e86..56c64ccbb 100644 --- a/ygen/testdata/proto/proto-test-a.compress.parent.formatted-txt +++ b/ygen/testdata/proto/proto-test-a.compress.parent.formatted-txt @@ -3,6 +3,7 @@ // // Input schema modules: // - testdata/proto/proto-test-a.yang +// - testdata/proto/openconfig-codegen-extensions.yang syntax = "proto3"; package openconfig; diff --git a/ygen/testdata/proto/proto-test-a.nocompress.parent.child.formatted-txt b/ygen/testdata/proto/proto-test-a.nocompress.parent.child.formatted-txt index f44df7f48..9a9912f03 100644 --- a/ygen/testdata/proto/proto-test-a.nocompress.parent.child.formatted-txt +++ b/ygen/testdata/proto/proto-test-a.nocompress.parent.child.formatted-txt @@ -25,7 +25,7 @@ message Config { // State represents the /proto-test-a/parent/child/state YANG schema element. message State { - ywrapper.BoolValue boolean = 101; + ywrapper.BoolValue boolean = 201; ywrapper.IntValue integer = 486380674; repeated ywrapper.StringValue leaf_list = 256667601; ywrapper.StringValue leaf_with_dashes = 475722830; diff --git a/ygen/testdata/proto/proto-test-a.yang b/ygen/testdata/proto/proto-test-a.yang index 6dfc43ee4..b6b93780e 100644 --- a/ygen/testdata/proto/proto-test-a.yang +++ b/ygen/testdata/proto/proto-test-a.yang @@ -6,6 +6,8 @@ module proto-test-a { "Test YANG simple scheam for protobuf translation."; + import openconfig-codegen-extensions { prefix occodegenext; } + grouping child-cfg { leaf string { type string; } leaf integer { type int64; } @@ -38,7 +40,9 @@ module proto-test-a { container state { config false; uses child-cfg; - uses child-state; + uses child-state { + occodegenext:field-number-offset 100; + } } } }