Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
dplore opened this issue Aug 21, 2023 · 18 comments · Fixed by #1177
Closed

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

dplore opened this issue Aug 21, 2023 · 18 comments · Fixed by #1177
Assignees

Comments

@dplore
Copy link
Member

dplore commented Aug 21, 2023

          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 #852 (comment)

@dplore
Copy link
Member Author

dplore commented Aug 21, 2023

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).

@dplore dplore self-assigned this Aug 21, 2023
@earies
Copy link
Contributor

earies commented Aug 22, 2023

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
Copy link

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
Copy link

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
Copy link
Contributor

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
Copy link
Member Author

dplore commented Aug 22, 2023

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
Copy link
Contributor

earies commented Aug 22, 2023

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
Copy link
Contributor

earies commented Aug 22, 2023

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
Copy link

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 dplore moved this to Todo in OC Operator Review Feb 6, 2024
@dplore dplore moved this from Todo to Needs Reviewer in OC Operator Review Feb 6, 2024
@dplore dplore moved this from Needs Reviewer to Todo in OC Operator Review Feb 6, 2024
@dplore
Copy link
Member Author

dplore commented Feb 27, 2024

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.

@dplore dplore moved this from Todo to In Progress in OC Operator Review Feb 27, 2024
@rszarecki
Copy link
Contributor

rszarecki commented Jul 8, 2024

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
Copy link
Member Author

dplore commented Aug 26, 2024

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.

@dplore dplore closed this as completed Aug 26, 2024
@LimeHat
Copy link

LimeHat commented Aug 26, 2024

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
Copy link
Member Author

dplore commented Aug 27, 2024

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
Copy link

LimeHat commented Aug 27, 2024

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
Copy link
Member Author

dplore commented Aug 27, 2024

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 #945 (comment)?

@nokia1adam
Copy link
Contributor

I see only two options and neither is totally ideal:

  • Adding a 'none' option to oc-bgp-types:community-type
  • Add a new leaf in the same locations as send-community-type. The leaf would be called something like disable-all-communities, type boolean

@dplore
Copy link
Member Author

dplore commented Sep 6, 2024

  • Adding a 'none' option to oc-bgp-types:community-type

There is a none enum value today, but it is deprecated. We could "un-deprecate" this enum.

I will propose this in a PR for review at the Sep 10 OC Operators meeting and Sep 12 OC Community meeting for feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

6 participants