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

New set-as-path BGP action, and support last AS for for setting AS path. #1148

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

xuqma
Copy link
Contributor

@xuqma xuqma commented Jul 15, 2024

Change Scope

  • Add set-as-path BGP action to replacing the entire AS path attribute with the following new path:
    • /routing-policy/policy-definitions/policy-definition/statements/statement/actions/bgp-actions/set-as-path (a list of ASNs or last-as)
  • Add oc-bgp-pol:bgp-as-mode-type to specify last AS in set-as-path.
    • This type is only used for BGP policies, so I decided to put it in oc-bgp-pol instead of oc-bgp-types.
  • This change is backwards compatible.

New paths after the model update:

To be updated.

Platform Implementations

@xuqma xuqma requested a review from a team as a code owner July 15, 2024 05:05
@dplore
Copy link
Member

dplore commented Jul 16, 2024

/gcbrun

@OpenConfigBot
Copy link

OpenConfigBot commented Jul 16, 2024

No major YANG version changes in commit fd28c04

@xuqma
Copy link
Contributor Author

xuqma commented Jul 16, 2024

Removed the extra comma and rebased against master.

@dplore
Copy link
Member

dplore commented Aug 7, 2024

/gcbrun

@dplore dplore self-assigned this Aug 7, 2024
release/models/bgp/openconfig-bgp-policy.yang Outdated Show resolved Hide resolved
release/models/bgp/openconfig-bgp-policy.yang Outdated Show resolved Hide resolved
release/models/bgp/openconfig-bgp-policy.yang Outdated Show resolved Hide resolved
release/models/bgp/openconfig-bgp-policy.yang Outdated Show resolved Hide resolved
release/models/bgp/openconfig-bgp-policy.yang Outdated Show resolved Hide resolved
@xuqma xuqma force-pushed the master branch 3 times, most recently from 68a38ff to bffc29b Compare August 9, 2024 22:28
@dplore
Copy link
Member

dplore commented Nov 12, 2024

/gcbrun

set-as-path action";

leaf-list as-paths {
type union {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this need to be ordered-by-user .

Is there precedence for this in existing implementations or is this intended to be a new feature request?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. Updated type with ordered-by user.

IIUC, only Arista support replacing the entire AS path with [set as-path match all replacement] (https://www.arista.com/en/um-eos/eos-acls-and-route-maps#xx1313923). This is the recommended Arista implementation for the existing as-path-expand in Juniper, which is also vendor specific unefortunately. I am not sure which one fits the model better (neither is ideal due to single-vendor support). set-as-path might be slightly more generic based on our past discussions. Kindly share your thoughts please.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, we can declare OC which achieves an operational use case that multiple vendors only deliver by different syntax/schema and semantics.

In this case I think the guiding principle is intact, in that the use case can be delivered by multiple vendors. It is ok for OC to adopt a syntax that looks more like one implementation than the others, when they are all different. The alternative is for OC to contrive some schema that that no existing implementation has, just to be different. I'd rather go with what operators have some rough consensus is "easy enough" to use.

release/models/bgp/openconfig-bgp-policy.yang Outdated Show resolved Hide resolved
@xuqma xuqma force-pushed the master branch 3 times, most recently from b8f43f3 to 61d8291 Compare November 14, 2024 06:13
@xuqma xuqma closed this Nov 14, 2024
@xuqma xuqma reopened this Nov 14, 2024
@xuqma xuqma force-pushed the master branch 2 times, most recently from fb94dc8 to b9068be Compare November 14, 2024 06:41
@xuqma xuqma changed the title New set-as-path BGP action, and support last AS for for setting and updating AS path. New set-as-path BGP action, and support last AS for for setting AS path. Nov 14, 2024
@dplore
Copy link
Member

dplore commented Jan 3, 2025

/gcbrun

2 similar comments
@dplore
Copy link
Member

dplore commented Jan 3, 2025

/gcbrun

@dplore
Copy link
Member

dplore commented Jan 3, 2025

/gcbrun

@dplore
Copy link
Member

dplore commented Jan 3, 2025

LGTM, moving to last call for Jan 16, 2025

@xuqma xuqma force-pushed the master branch 2 times, most recently from 30ffc7e to bdeb512 Compare January 3, 2025 23:43
@xuqma
Copy link
Contributor Author

xuqma commented Jan 3, 2025

Thanks Darren! I have fixed the revisions based on the previous check result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: last-call
Development

Successfully merging this pull request may close these issues.

3 participants