ygot icon indicating copy to clipboard operation
ygot copied to clipboard

Is the current generated proto3 for YANG enums with a default value flawed?

Open wenovus opened this issue 3 years ago • 4 comments

Currently, the generated proto code uses the default value as the first/default enum value for all "leaf" (not leaf-list) enumerated values.

Because in proto the enum definition alone can specify the default value (i.e. 0), this behaviour only works if the default value is guaranteed to be the same across all usage instances. However, this is not true in YANG:

A typedef enumeration's default value can be overridden by a leaf statement's default value (ref). When this happens, that place will have the wrong default value.

An inlined enumeration's default value can be overridden by a deviation statement, such that the deviated leaf will have the wrong default value in the generated code compared to the other non-deviated usage instances.

The question I want to ask is whether this is actually what we want, because if this is not what we want I'd rather delete it from the codebase since it's adding complexity to ygen.

wenovus avatar May 11 '22 16:05 wenovus

So currently if there is an override whichever one the code generation found as the default value first is used.

wenovus avatar May 18 '22 00:05 wenovus

This is a good observation that was overlooked in the initial implementation, and yes, might be incorrectly setting a default that is not the actual schema default for some leaves. I am good with removing this functionality.

robshakir avatar May 19 '22 22:05 robshakir

TLDR: I will keep this feature for inline enums and remove it for typedef enums (option 2 below)


Actually I wasn't precise with inline enums: since grouping instances are always replicated; therefore, the default values can be specific for each instance, such that deviations don't matter. A conflict is possible only for compressed enum leaves between config vs. state, but we don't expect defaults between applied vs. intended configuration to be different, right?

I think we have a few options here:

  1. Just remove this feature.
  2. Keep it for inlined enumerations, including compressed code because we don't expect defaults between applied vs. intended configuration to be different.
  3. Keep this feature, but change proto3's enum generation again, such that we replicate every enumeration, including the global enumerations. Now there wouldn't be any global enumerations.

I like option 2 right now because it's the least disruptive to current users.

wenovus avatar May 24 '22 23:05 wenovus

Yes, the defaults between intended and applied can never differ -- we probably don't verify this today, but it's a requirement since they are the same type. I'm OK with number 2.

robshakir avatar May 24 '22 23:05 robshakir