Supporting YANG 1.1 changes to sub-statement cardinalities

[andaru]

In evaluating what changes are necessary to support YANG 1.1 completely in the lexer, I've found one interesting type of breaking change - sub-statement cardinality changes (from 0..1 to 0..n). Example:

In YANG 1.0, a uses statement has a child statement augment, with a cardinality of 0..1 (ref: RFC6020 s7.12.1).

In YANG 1.1, a uses statement has a child statement augment, with a cardinality of 0..n (ref: RFC7951 s7.13.1).

Due to the use of reflection in the YANG lexer, to support this change we could change one field in the Uses type;

   Augment *Augment `yang:"augment"`

to:

   Augment []*Augment `yang:"augment"`

However, this causes a breaking change for library users. The Augment field of Uses is not used by the goyang library itself at this point.

Multiple augment children of a uses is found in both [email protected] and [email protected], e.g.,

https://github.com/YangModels/yang/blob/master/standard/ietf/RFC/ietf-ipv4-unicast-routing%402016-11-04.yang#L222-L244

Currently this leads to an error in goyang: augment: already set (from pkg/yang/ast.go, the error returned has no statement location attached at present).

Thoughts on the most appropriate way to make this change?

It should be noted there's a few other cases of this sort of change in YANG 1.1; the refine substatement of uses also changes similarly.

Changing from a single pointer to a slice with no other changes would remove the cardinality 0..1 syntax check for YANG 1.0 modules. This could be moved to the ToEntry function since the whole schema has been lexed (and thus the yang-version statement is available for determining which syntax and semantic rules to apply to the derived schema). As YANG 1.1 makes other semantic changes (such as its changes to scoping rules for submodules), we will need to make a switch during ToEntry based on the YANG version.

[pborman]

I think it would be appropriate to have a 1.0 and 1.1 version of goyang. Probably create pkg/yang/v1.1, clone the source code there, and keep it with the package name of yang. The import line would then be:

import "github.com/openconfig/goyang/pkg/yang/v1.1"

And then mark that package as in flux for now (i.e., we will make breaking changes to migrate from 1.0 to 1.1)

What do you think?

[andaru]

Paul, thanks for the response. Sounds like a fine approach, I'll start a new branch with those changes.

While I guess we haven't reached a "stable" version yet, have you had any thoughts about use of gopkg.in for publicly versioning the package?

[LeonGWang]

Hi, I would like to help resolve this issue. @robshakir do you think we should version goyang? If so, how do you envision such change?

[robshakir]

I think there are a couple of options.

1. We can fork the package like @pborman suggested above. Downside for this is that we need to backport fixes across the board.
2. We could create a tag at this current revision of HEAD, such that folks that are using gopkg.in etc. can bind to that particular version. We can then make backwards incompatible changes to the goyang API. In this case, we'd just maintain HEAD going forward, and ask users to move. If there were significant push-back from users, then this would mean we could reconsider whether we wanted to do the first option.

In your codebase, how much of an issue is this change likely to cause? In ygot, I think we have minimal impact of adopting these cardinality changes.

@andaru -- do you have thoughts here?

[andaru]

Hi,

I side with Rob's "option 2", but to be honest I don't have a great argument as to why. My concerns about the effect upon users (existing as well as potential) comes down to these thoughts:

• go get to the repo with no trailing fragements/query params should return the "old API" tag until cutover.
• directing developers interested in YANG 1.1 features to HEAD (README changes)
• perhaps setting the repo's default tag (on the web) to the "old API" tag (I think I've seen this on other projects for similar reasons).

Any thoughts or things I'm missing?

Regarding validation, I agree that the goyang core shouldn't deal with much more than correct YANG (ABNF) syntax (there's a YANG 1.1 change for this, to be done in the lexer), as well as checking the things it does care about as much as it needs to, and no more.

As for strict or custom validation of the YANG schema, as well as linters/etc, we can offer a yang.Validator interface type roughly like:

type Validator interface {
  Validate(*Entry) error
}

Composition of validators is trivial this way and validation is completely optional.

[robshakir]

These sound reasonable to me. We can develop Y1.1 on a branch, and simply merge there until we're happy that this development feels in reasonable shape. At that point, we'd merge that branch into master. I don't think there's a much cleaner way of doing this, as go get is quite opinionated about returning HEAD@master -- unless I'm missing something?

I like the idea of providing the Validator interface.

@LeonGWang -- do you guys have a timeline for wanting to start this work? Seems like we have a general direction here.

[pborman]

Btw, you may want to consider having an augment field and an augments field, if clients use the augment field directly. The augment field would be backwards compatible with 1.0 and be a copy of the first element in the augments list.

You would still have to special case the filling in, however, and you need a check than len(augments) <= 1 when in 1.0 mode.

On Mar 16, 2019, at 6:36 AM, Rob Shakir [email protected] wrote:

These sound reasonable to me. We can develop Y1.1 on a branch, and simply merge there until we're happy that this development feels in reasonable shape. At that point, we'd merge that branch into master. I don't think there's a much cleaner way of doing this, as go get is quite opinionated about returning HEAD@master -- unless I'm missing something?

I like the idea of providing the Validator interface.

@LeonGWang -- do you guys have a timeline for wanting to start this work? Seems like we have a general direction here.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[LeonGWang]

Arista's OpenConfig codebase is not affected by the cardinality changes. I've only seen IETF YANG models take advantage of YANG 1.1 cardinality changes.

I can begin right away. The main features I am concerned with is the cardinality changes and allow Uses's Augment statements to take effect (although that's a separate issue).

[andaru]

@pborman Thanks, I'd considered and forgotten about that. I'd also considered adding an accessor method to the concrete type to retrieve the appropriate value from the singular or plural field within that type, so that folks could switch over to using that accessor as they wish to implement 1.1 support.

[andaru]

@LeonGWang So if I understand correctly, the choices are to:

1. change the 1-ary field to a slice (though we don't have a full agreement yet on HEAD versus new branch, the new branch is most practical for no changes to go get use); or
2. have a Uses and a Usess (for want of a better name) field being the existing 1-ary field along with a slice for the 2nd to Nth such statement. This can be deployed incrementally as folks need the fields, allowing us to keep developing as is. We could provide another accessor (e.g., func GetUses(n int) *Uses) if people think it's worth the binary size increases (documentation can also suffice).

I believe the other YANG 1.1 changes (identifier namespace scope changes, lexer handling of un-defined escape characters) can be introduced incrementally, so IMO option 2 as @pborman describes gives us the only backwards-compatible way forward.

The issue with using the existing field name in the concrete statement types, for example, the Augment field in the Module struct type, is that changing the field from a singular *Augment pointer to a slice of them causes user code referencing them to no-longer compile. Ideally we would avoid that so existing code continues to compile (but can only extract YANG 1.0 statements) while allowing YANG 1.1 modules to be loaded by goyang.

Any other arguments for or against, or different?

[LeonGWang]

@andaru Thanks for the summary. I agree with option 2. I will begin work on it.

[wenovus]

Looks like this issue is starting to pop up (#139) @LeonGWang do you have a status update on the work that might help us here?