Inconsistencies in sets - openconfig-bgp-policy.yang

[suhgopin]

This is regarding an issue we observed in the latest openconfig revision for openconfig-bgp-policy.yang https://github.com/openconfig/public/blob/master/release/models/bgp/openconfig-bgp-policy.yang

To give a brief overview, sets are treated as independent entities which do not define the execution of the policy. Sets were used in the conditional statements which define the policy execution. But in the latest OC model, a part of the execution logic is moved into the sets(openconfig-bgp-policy.yang) - community-set and ext-community-set are such that they carry match options like ANY, ALL, INVERT along with the set rather than the conditional statement in the policy. (See the tree diff attached between the old OC model and the latest on the right)

This change is not consistent with all the sets in the same file. community-set and ext-community-set follow the new convention(highlighted in green) whereas as-path-set follows older convention(in the red box) which does not included match-set-options. Few sets are also part of openconfig-routing-policy.yang which still follows the older convention like as-path-sets.

Request to reconsider/review this change in openconfig-bgp-policy.yang sets and revert back to the older convention.

Also, in the newer revision policy execution data is coupled with sets. Earlier a set could be used in different policies with different match-set-options, but because of this new change, a set is bound to be used with a single match-set-option only. This restricts the way a set could be used with different match-set-options. For example, consider a community-set XYZ, in the older convention, XYZ can be used in various statements with any of the match-set-option. But in the new convention, XYZ will store the match-set-option within, so the user must always limit the set to be used with that match-set-option. If we must use XYZ with a different match-set-option, we will have to clone a new set from XYZ and change the match-set-option(losing out on reusability). This will lead to duplicate sets with different match-set-option.

[oscargdd]

Hi! I agree that is easier to understand and more efficient having the options like invert in the conditions rather than in the set. Now, it is true that you require to maintain several sets with the same values if you require to match vs the set and vs NOT the set.

I'd like to hear more views from implementors on the willingness to change the place of the options.

In any case, as you mention, it is true that the solution should be the same for all the sets.

[suhgopin]

Hi!

Appreciate your prompt response. Looking forward to hearing back from you on a resolution.

[robshakir]

@aashaikh PTAL here -- would appreciate your comments on the history here.

[suhgopin]

Hi Team, any update on this?

[oscargdd]

HI @robshakir @aashaikh ping on this topic. I still think having the match options outside the set is much more consistent behaviour. Besides... having the logic of the match inside the set is really weird... Most vendors in their implementations have the community sets just.. community sets ....

[dplore]

Here is a reference to a post that led to this change: https://groups.google.com/g/openconfig-ops/c/XsV_TSkpD1A/m/2PNrHFfgAwAJ

[suhgopin]

This inconsistency leads to a lot of churn in policy usage and admin scripts, request to you revert to the earlier behavior.

[dplore]

FWIW, I updated the email group thread with a link to this comment thread.

Barring any strong evidence of a blocking problem, I'll submit a pull request to move community-set matching back to the same pattern as used for as-path-set and prefix-set matching. That is:

Existing, related paths:

/routing-policy/policy-definitions/policy-definition/statements/statement/conditions/bgp-conditions/match-as-path-set/config/match-set-options
/routing-policy/policy-definitions/policy-definition/statements/statement/conditions/match-prefix-set/config/match-set-options
/routing-policy/policy-definitions/policy-definition/statements/statement/conditions/match-neighbor-set/config/match-set-options
/routing-policy/policy-definitions/policy-definition/statements/statement/conditions/match-tag-set/config/match-set-options

New path to be added (re-introduced): /routing-policy/policy-definitions/policy-definition/statements/statement/conditions/match-community-set/config/match-set-options

This path would be deprecated: /routing-policy/defined-sets/bgp-defined-sets/community-sets/community-set/config/match-set-options

[dplore]

Reviewed in Oct 3, 2023 OC Operators meeting without objection. I'll open a PR to make this change