openconfig / public

Repository for publishing OpenConfig models, documentation, and other material for the community.
Apache License 2.0
890 stars 644 forks source link

How to disable sending BGP communities at group and neighbor level #945

Closed dplore closed 5 days ago

dplore commented 1 year ago
          Sorry to comment after this was merged, but I am realizing that the NONE option was deprecated by this change. I don't believe that was a good decision.

BGP configuration is based on a model of inheritance where neighbor settings override group settings, which in turn override global settings. Suppose the group or global configuration specifies that standard, extended and large communities should all be transmittable. The configuration of a specific neighbor should be able to override this and make no communities transmittable to that peer. But without a 'NONE' option, how do you do this? A send-community-type leaf-list with no elements is equivalent to no configuration of send-community-type at all, which should mean that the configuration is inherited from a higher context -- but clearly that's not what we want.

TLDR - it should not be assumed that an empty leaf-list is equivalent to 'NONE'.

Originally posted by @nokia1adam in https://github.com/openconfig/public/issues/852#issuecomment-1684261880

dplore commented 1 year ago

The use case is to send communities at the global level and disable/not send communities for a specific peer-group or neighbor. #852 unintentionally deprecated this ability.

If we like the leaf-list of enum design, then we could add a 'send-community-enabled' boolean at the bgp global level and at the group/neighbor level. The global could have a default of false and the group/neighbor levels would not have a default (so inheritance is preserved).

earies commented 1 year ago

Taking a step back here, it seems this problem statement isn't unique to BGP communities (or even BGP) but rather configuration hierarchies, groupings and expected inheritance across years of implementation to template, reuse, override partial/all parent behavior, etc..

This seems like a wider problem statement that needs to be dug into vs. just a leaf-by-leaf behavior no?

jhaas-pfrc commented 1 year ago

Taking a step back here, it seems this problem statement isn't unique to BGP communities (or even BGP) but rather configuration hierarchies, groupings and expected inheritance across years of implementation to template, reuse, override partial/all parent behavior, etc..

This seems like a wider problem statement that needs to be dug into vs. just a leaf-by-leaf behavior no?

Dealing with inheritance, especially in the presence of defaults, has been a tricky thing. The IETF BFD work ran into such an issue and it was only caught by careful review.

jhaas-pfrc commented 1 year ago

If we like the leaf-list of enum design, then we could add a 'send-community-enabled' boolean at the bgp global level and at the group/neighbor level. The global could have a default of false and the group/neighbor levels would not have a default (so inheritance is preserved).

RFC 7950 §7.7.3 suggests that it's permissible to have an empty leaf-list.

It seems to me that the easiest way to model "NONE" without introducing a lot of other headaches is to simply document that none is achieved by an empty leaf-list. This would override a less specific scope that has entries within it.

nokia1adam commented 1 year ago

Taking a step back here, it seems this problem statement isn't unique to BGP communities (or even BGP) but rather configuration hierarchies, groupings and expected inheritance across years of implementation to template, reuse, override partial/all parent behavior, etc..

This seems like a wider problem statement that needs to be dug into vs. just a leaf-by-leaf behavior no?

I agree, this is a wider problem. There are many OC models where the same grouping, container, leaf etc. is repeated at multiple levels of the configuration hierarchy, but nothing is stated about how overrides or inheritance should work. I think implementers know 'roughly' what is expected from years of industry accepted best practice, but there is still a lot of opportunity for things not to work as expected.

RFC 7950 §7.7.3 suggests that it's permissible to have an empty leaf-list.

It seems to me that the easiest way to model "NONE" without introducing a lot of other headaches is to simply document that none is achieved by an empty leaf-list. This would override a less specific scope that has entries within it.

An empty leaf-list can be accepted but what does it mean in the context of an inheritance hierarchy? In this context it can either have the meaning "send nothing" or it can have the meaning "I don't care, inherit my send setting from a higher level of configuration", but it can't convey both of these meanings...and there are cases where we need both.

dplore commented 1 year ago

Taking a step back here, it seems this problem statement isn't unique to BGP communities (or even BGP) but rather configuration hierarchies, groupings and expected inheritance across years of implementation to template, reuse, override partial/all parent behavior, etc.. This seems like a wider problem statement that needs to be dug into vs. just a leaf-by-leaf behavior no?

I agree, this is a wider problem. There are many OC models where the same grouping, container, leaf etc. is repeated at multiple levels of the configuration hierarchy, but nothing is stated about how overrides or inheritance should work. I think implementers know 'roughly' what is expected from years of industry accepted best practice, but there is still a lot of opportunity for things not to work as expected.

As noted, due to well known expectations for the data which is being modeled, I don't think we need an explicit mechanism to model inheritance in yang. I think we can use description to describe the inheritance expectations on a per container/leaf basis.

The scope of this issue I think can be constrained to: we formerly had an ability to explictly negate a configuration which can be inherited from a parent. We need to choose a way to indicate negation the configuration inherited by the parent. We already have a way to accept (no list) or to override (specify a list) the configuraton from the parent.

RFC 7950 §7.7.3 suggests that it's permissible to have an empty leaf-list. It seems to me that the easiest way to model "NONE" without introducing a lot of other headaches is to simply document that none is achieved by an empty leaf-list. This would override a less specific scope that has entries within it.

An empty leaf-list can be accepted but what does it mean in the context of an inheritance hierarchy? In this context it can either have the meaning "send nothing" or it can have the meaning "I don't care, inherit my send setting from a higher level of configuration", but it can't convey both of these meanings...and there are cases where we need both.

I do see a use case for "negating" the configuration of a parent. I agree that an empty list can only convey 'inherit from parent' or 'negate parent', but not both.

So one suggestion is for an enabled leaf. I do think it's worth considering if this is a reusable pattern for this scenario where inheritance is involved.

earies commented 1 year ago

My read is that an empty leaf-list should not be permitted but I see it permitted by at least yanglint today

RFC 6020 (7950 conveys same) - Section 7.7.3/7.7.5 as subset of "leaf-list"

7.7.3.  The min-elements Statement

   The "min-elements" statement, which is optional, takes as an argument
   a non-negative integer that puts a constraint on valid list entries.
   A valid leaf-list or list MUST have at least min-elements entries.

   If no "min-elements" statement is present, it defaults to zero.

This would imply that min-elements=0 here since it's not defined on the leaf-list and thus should not be acceptable

earies commented 1 year ago

My read is that an empty leaf-list should not be permitted but I see it permitted by at least yanglint today

RFC 6020 (7950 conveys same) - Section 7.7.3/7.7.5 as subset of "leaf-list"

7.7.3.  The min-elements Statement

   The "min-elements" statement, which is optional, takes as an argument
   a non-negative integer that puts a constraint on valid list entries.
   A valid leaf-list or list MUST have at least min-elements entries.

   If no "min-elements" statement is present, it defaults to zero.

This would imply that min-elements=0 here since it's not defined on the leaf-list and thus should not be acceptable

scratch that - misread .... MUST have at least 0 entries is the gotcha here..... so yes a leaf-list w/ no entries is permissible

jhaas-pfrc commented 1 year ago

An empty leaf-list can be accepted but what does it mean in the context of an inheritance hierarchy? In this context it can either have the meaning "send nothing" or it can have the meaning "I don't care, inherit my send setting from a higher level of configuration", but it can't convey both of these meanings...and there are cases where we need both.

I do see a use case for "negating" the configuration of a parent. I agree that an empty list can only convey 'inherit from parent' or 'negate parent', but not both.

So one suggestion is for an enabled leaf. I do think it's worth considering if this is a reusable pattern for this scenario where inheritance is involved.

As I mentioned in my original reply, clarification of the pattern in the description will be necessary. Here's how I think we'd like this to work:

The absence of send-community across the entire inheritance hierarchy means "we're not sending anything". The presence of any non-empty send-community at a point in the hierarchy means "use this list for this scope and all children that don't explicitly override it". The presence of any emptysend-community at a point in the hierarchy means "send nothing for this scope and all children that don't explicitly override it".

Basically, the pattern becomes the total absence at any scope and its parents means default to send none, and anything configured means that scope and all children that don't override.

dplore commented 6 months ago

This was discussed in the OC Operators meeting today. It seems logical, programmatic/generic and legal for a configuration to contain a path to a leaf-list which is empty (with no values assigned). These are all good things.

But it also seems non-intuitive compared to an explicitly modeled way to negate configuration or inheritance of configuration. While not generic, an explicit "do not send communities" leaf is very clear on it's intent. For example, introducing a leaf such as send-community-enabled at each level in the hierarchy.
/network-instances/network-instance/protocols/protocol/bgp/global/afi-safis/afi-safi/config/send-community-enabled /network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/afi-safis/afi-safi/config/send-community-enabled /network-instances/network-instance/protocols/protocol/bgp/neighbors/neighbor/afi-safis/afi-safi/config/send-community-enabled

I'd like to hear a little more feedback from the community if there is a preference and any urgency on which way to model this.

rszarecki commented 1 month ago

I do not think we need new leaf.

First let me ask whta is an object of inherience? Is it leaf-leas and whole atomic object OR is is list elements? It is the former. Lets run example:

global/config/send-community-type: "[STANDERD, LARGE, EXTENDED]"
neighbor/config/send-community-type: "[STANDERD, EXTENDED]"

What shall be neighor behaviour? Whould be "LARGE" inherited? My understanding is NO. neighbor would send only STANDARD, EXTENDED. The ENTIERE list is inherited or entire list is NOT inherited. Note: This is the ONLY interpretation that allows express reduced list on child vs. broader in parent.

Now if we have :

global/config/send-community-type: "[STANDERD, LARGE, EXTENDED]"
neighbor/config/send-community-type: "[]"

We do have exlicitly configured list for neighbor. Hence list form global MUST NOT be inherited. And list at neighbor is empty = send nothing.

Let say neighbor has no configuration for send-community-type leaf-list

global/config/send-community-type: "[STANDERD, LARGE, EXTENDED]"

We do NOT have exlicitly configured list for neighbor nor peer-group. Hence list form global MUST be inherited.

If key exist with null value it is an entry. If key do not exist there is no entry.


adding new leas like "send-community-type-enabled" do not provide any value and only inytoducer confusion. What is expected behaviour in this case:

global/config/send-community-type: "[STANDERD, LARGE, EXTENDED]"
neighbor/config/send-community-type-enabled: TRUE
neighbor/config/send-community-type: "[]"

Shall e send all 3 types or none? It do not bringa any additional value.

dplore commented 5 days ago

Thanks for all the feedback. I read the rough consensus as an empty list (for example: send-community-type: []) in configuration is a valid and explicit value (empty). In the inheritance scenario causes any inheritance from higher levels to be negated.

So no change is needed.

LimeHat commented 5 days ago

In my view, this is a problematic conclusion. It might be acceptable to allow the configuration of empty lists; however, assigning different behaviors for empty and uninitialized lists is a huge challenge for the tooling ecosystem and interoperability.

This will create a lot of ambiguity for any tools/languages/protocols where this distinction is blurred or non-existent. For example, netconf and XML encoding.

I know the OpenConfig community is not always concerned about these things, but they do exist and are used by other customers.

dplore commented 5 days ago

Thanks @LimeHat , can you give some more details?

We are concerned and want to support netconf+xml. Although I will admit we don't have as much engagement from contributors with keen awareness of issues impacting netconf and xml encoding.

LimeHat commented 5 days ago

We can take a look at the sections 7.8.5-7.8.7 of RFC7950 for lists

A list is encoded as a series of XML elements, one for each entry in the list. Each element's local name is the list's identifier, and its namespace is the module's XML namespace (see Section 7.1.3). There is no XML element surrounding the list as a whole.

And sections 7.7.8-7.7.10 for leaf-lists says the same thing. Simply put, this distinction ("unconfigured/undefined" vs "empty") most likely will be lost in any tool that works with xml/netconf encoding.

Let's take as an example the following code snippet, which uses a simplified grpc-server structure from ygot. https://goplay.tools/snippet/XdYAaF_3fFB

xml.Marshal from encoding/xml will simply drop the empty leaf-list (services) according to the above definition.

dplore commented 5 days ago

The absence of send-community across the entire inheritance hierarchy means "we're not sending anything". The presence of any non-empty send-community at a point in the hierarchy means "use this list for this scope and all children that don't explicitly override it". The presence of any emptysend-community at a point in the hierarchy means "send nothing for this scope and all children that don't explicitly override it".

Any suggestion on how to solve for this? What do you think about the suggestion of adding a boolean in https://github.com/openconfig/public/issues/945#issuecomment-1967334831?

nokia1adam commented 5 days ago

I see only two options and neither is totally ideal: