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

Openconfig-bgp-policy ::The community-member leaf-list needs to be reordered. #1202

Open
Pramoda-mj opened this issue Oct 21, 2024 · 3 comments

Comments

@Pramoda-mj
Copy link

Pramoda-mj commented Oct 21, 2024

As per RFC 7950, Section 9.12, when defining a YANG node with a union data type that includes both string and identityref member types, it is crucial to order the identityref type before the string type. This ordering ensures that the parser correctly identifies and handles the identityref type before attempting to match the string type.

Example:
Consider the following YANG leaf-list definition:

          leaf-list community-member {
            type union {
              type oc-bgp-types:bgp-std-community-type;
              type oc-bgp-types:bgp-community-regexp-type;
              type oc-bgp-types:bgp-well-known-community-type;
            }
            description
              "Members of the community set.
              For an ADD operation these are the communities that will be
              added. The regexp type is not valid in this operation.
          
              For REMOVE or REPLACE operations then matching communities will
              be removed unless match-set-options is INVERT which will
              reverse this to mean that anything that does not match will be
              removed.
          
              For MATCH operations the posix-eregexp type should be evaluated
              against each community associated with a prefix one community
              at a time. Communities must be represented as strings in formats
              conforming to oc-bgp-types:bgp-std-community-type. For example:
              `1000:1000` for a standard community";
          }     

In this example, the community-member leaf-list uses a union data type that includes three types:
1. oc-bgp-types:bgp-std-community-type
2. oc-bgp-types:bgp-community-regexp-type
3. oc-bgp-types:bgp-well-known-community-type

Detailed Type Definitions:
1. bgp-std-community-type:

               typedef bgp-std-community-type {
                // TODO: further refine restrictions and allowed patterns
                // 4-octet value:
                //  <as number> 2 octets
                //  <community value> 2 octets
                type union {
                  type uint32 {
                  // per RFC 1997, 0x00000000 - 0x0000FFFF and 0xFFFF0000 -
                  // 0xFFFFFFFF are reserved
                  }
                  type string {
                    pattern '(6553[0-5]|655[0-2][0-9]|65[0-4][0-9]{2}'      +
                            '|6[0-4][0-9]{3}|[1-5][0-9]{4}|[1-9][0-9]{1,3}|[0-9]):'      +
                            '(6553[0-5]|655[0-2][0-9]|65[0-4][0-9]{2}'       +
                            '|6[0-4][0-9]{3}|[1-5][0-9]{4}|[1-9][0-9]{1,3}|[0-9])';
                    oc-ext:posix-pattern '^(6553[0-5]|655[0-2][0-9]|65[0-4][0-9]{2}'      +
                            '|6[0-4][0-9]{3}|[1-5][0-9]{4}|[1-9][0-9]{1,3}|[0-9]):'      +
                            '(6553[0-5]|655[0-2][0-9]|65[0-4][0-9]{2}'       +
                            '|6[0-4][0-9]{3}|[1-5][0-9]{4}|[1-9][0-9]{1,3}|[0-9])$';
                  }
                }
                description
                  "Type definition for standard commmunity attributes represented as
                  a integer value, or a string of the form N:M where N and M are
                  integers between 0 and 65535.";
                reference "RFC 1997 - BGP Communities Attribute";
              } 

2. bgp-community-regexp-type:

       typedef bgp-community-regexp-type {
        type oc-types:posix-eregexp; 
        description
          "Type definition for communities specified as regular
          expression patterns.  The regular expression must be a
          POSIX extended regular expression with some limitations
          which are commonly found in device implementations described
          in draft-ietf-idr-bgp-model.";
        reference
          "draft-ietf-idr-bgp-model";
      } 

3. bgp-well-known-community-type:

        typedef bgp-well-known-community-type {
          type identityref {
            base BGP_WELL_KNOWN_STD_COMMUNITY;
          }
          description
            "Type definition for well-known IETF community attribute
            values";
          reference
            "IANA Border Gateway Protocol (BGP) Well Known Communities";
        } 

Importance of Type Order:
In the union type, if identityref is not ordered before string, the parser may incorrectly match the input value as a string instead of recognizing it as an identityref. This can lead to errors in processing the data.

Proposed Solution:
To ensure correct parsing, it is recommended to:
Order the identityref type before the string type in the union. i.e) like below

            type oc-bgp-types:bgp-std-community-type; 
            type oc-bgp-types:bgp-well-known-community-type; 
            type oc-bgp-types:bgp-community-regexp-type;
@dplore
Copy link
Member

dplore commented Nov 1, 2024

Formally OC is yang v1.0 compliant, but this still looks like a change that should be implemented. Would you like to send a pull request to change the ordering?

@dplore dplore moved this to Waiting for author in OC Operator Review Nov 1, 2024
@dplore
Copy link
Member

dplore commented Nov 5, 2024

Also, to move the identity-ref to the first element in the list of union type, shouldn't the union order be:

type oc-bgp-types:bgp-well-known-community-type;   (identity-ref)
type oc-bgp-types:bgp-std-community-type;                 (union of uint32 and string)
type oc-bgp-types:bgp-community-regexp-type;           (regex string)

@dplore
Copy link
Member

dplore commented Nov 5, 2024

I discussed this offline with others and it seems a change should not be needed.

The order is currently

          leaf-list community-member {
            type union {
              type oc-bgp-types:bgp-std-community-type; (uint32, string)
              type oc-bgp-types:bgp-community-regexp-type; (regex string)
              type oc-bgp-types:bgp-well-known-community-type; (identity-ref)
            }

An implementation parsing json encoded string values like NO_EXPORT and all the other bgp-well-known-community-types must resolve that these do not pattern match a uint32 nor the string pattern for bgp-std-community-type nor the string pattern for bgp-community-regexp-type.

In other words, NO_EXPORT won't match the first two types but will match the intended bgp-well-known-community-type; (identity-ref)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Waiting for author
Development

No branches or pull requests

2 participants