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

Add CLI about locator and related funcs #16894

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

Conversation

Yubin-Li
Copy link

@Yubin-Li Yubin-Li commented Sep 23, 2024

zebra: Add support for srv6 locator-opcode
In order to support configuring static SRv6 decapsulation functionality on the system, we need to support configuring a purely static mode local SID. This local SID does not rely on BGP to randomly generate a specific mySID address. Instead, it generates a unique mySID table entry by manually specifying the opcode.
example:
'''
segment-routing
srv6
locators
locator a
prefix fd00::/80 block-len 32 node-len 48 func-bits 16
opcode ::1:2:3:4 end
opcode ::1:2:3:5 end-dt46 vrf Vrf1
exit
!
exit
'''
Signed-off-by: hanyu.zly [email protected]

Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the rebase PR needs rebase label Sep 23, 2024
@donaldsharp
Copy link
Member

This should be broken up in to a bunch of smaller commits, with detailed explanations of what each commit is doing and why. Additionally I see white space changes that make it even harder to figure out what is going on. They should be removed.

@donaldsharp
Copy link
Member

Additionally this is not going to be even looked at until we have a topology test as well as documentation for the new functionality

@zice312963205 zice312963205 force-pushed the project-phoenixwing-ysj branch from 113f49b to 9d52e4a Compare November 23, 2024 14:18
@frrbot frrbot bot added the zebra label Nov 23, 2024
@Yubin-Li Yubin-Li changed the title add CLI about locator, related funcs and KERNELBYPASS flag on dplane_ctx Add CLI about locator and related funcs Nov 25, 2024
@frrbot frrbot bot added documentation libfrr tests Topotests, make check, etc labels Nov 27, 2024
@github-actions github-actions bot added size/XL and removed size/L labels Nov 27, 2024
@GaladrielZhao GaladrielZhao force-pushed the project-phoenixwing-ysj branch 3 times, most recently from 2c51a1d to 64739c5 Compare November 27, 2024 09:32
.parent_node = SRV6_LOC_NODE,
.prompt = "%s(config-srv6-locator-prefix)# ",
};

Copy link
Member

Choose a reason for hiding this comment

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

why defining a new srv6 prefix node ?
on a locator, we only have one prefix supported.
so I think it is not necessary to add a nesting level for sid commands.

many code change in this commit would be simplified. only the sid commands would be kept.

install_element(SRV6_LOC_NODE, &exit_srv6_loc_config_cmd);
install_element(SRV6_LOC_NODE, &vtysh_end_all_cmd);

install_element(SRV6_PREFIX_NODE, &exit_srv6_prefix_config_cmd);
install_element(SRV6_PREFIX_NODE, &vtysh_end_all_cmd);

Copy link
Member

Choose a reason for hiding this comment

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

ditto

.node = SRV6_PREFIX_NODE,
.parent_node = SRV6_LOC_NODE,
.prompt = "%s(config-srv6-locator-prefix)# " };

Copy link
Member

Choose a reason for hiding this comment

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

ditto: can be removed as per first comment.

@@ -334,6 +367,7 @@ DEFUN_NOSH (srv6_locator,
if (locator) {
VTY_PUSH_CONTEXT(SRV6_LOC_NODE, locator);
locator->status_up = true;
vty->node = SRV6_LOC_NODE;
return CMD_SUCCESS;
Copy link
Member

Choose a reason for hiding this comment

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

ditto.

@@ -253,6 +279,13 @@ DEFUN (show_srv6_locator_detail,
vty_out(vty, "- prefix: %s, owner: %s\n", str,
zebra_route_string(chunk->proto));
}
vty_out(vty, " sids:\n");
Copy link
Member

Choose a reason for hiding this comment

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

is it possible to display "sids:" only if there are sids configured ?

if (argv_find(argv, argc, "node-len", &idx))
node_bit_len = strtoul(argv[idx + 1]->arg, NULL, 10);
if (argv_find(argv, argc, "func-bits", &idx))
func_bit_len = strtoul(argv[idx + 1]->arg, NULL, 10);

Copy link
Member

Choose a reason for hiding this comment

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

ditto. the code would be the same.

struct vrf *vrf = NULL;

if (!locator->status_up) {
vty_out(vty, "Missing valid prefix.\n");
Copy link
Member

@pguibert6WIND pguibert6WIND Jan 9, 2025

Choose a reason for hiding this comment

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

The configuration of sids should always be possible.
Adding to this, modifying the prefix should trigger an update on the sid list.
Each SID should have a status that tells if it is available or not.

some cases:

  • I create a locator without prefix, and configure the sid list
  • I modify the prefix list after having configured the sid list: I don't expect sids to be reconfigured
  • I configured the locator in uncompressed mode (non microsid) : I expect mentioned sids will be inactive.
  • vrf information should be refreshed (do not prevent from configuring if vrf not yet present at config time).
  • a dynamic VRF SID is already allocated, then the configured sid should be inactive, though config should be kept.

char vrfName[VRF_NAMSIZ + 1];
struct prefix_ipv6 ipv6Addr;
char sidstr[PREFIX_STRLEN];
};
Copy link
Member

Choose a reason for hiding this comment

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

  • add state to tell if it is active for usage or inactive.

for (ALL_LIST_ELEMENTS(locator->sids, node, next, srv6_sid)) {
if (IPV6_ADDR_SAME(&srv6_sid->ipv6Addr.prefix, &ipv6prefix.prefix)) {
for (ALL_LIST_ELEMENTS_RO(zrouter.client_list, client_node, client))
zsend_srv6_manager_del_sid(client, VRF_DEFAULT, locator, srv6_sid);
Copy link
Member

Choose a reason for hiding this comment

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

  • A registration scheme would avoid sending unnecessary messages to every daemon.
    For instance, the sending could be triggered by some config in BGP, ISIS, PATHD. Eventually, this should be only sent to those daemons.

  • Why sending all the sids while only one is configured ?

@@ -211,6 +211,9 @@ typedef enum {
ZEBRA_SRV6_MANAGER_GET_LOCATOR,
ZEBRA_SRV6_MANAGER_GET_SRV6_SID,
ZEBRA_SRV6_MANAGER_RELEASE_SRV6_SID,
ZEBRA_SRV6_MANAGER_GET_LOCATOR_SID,
ZEBRA_SRV6_MANAGER_RELEASE_LOCATOR_SID,
ZEBRA_SRV6_MANAGER_GET_LOCATOR_ALL,
ZEBRA_ERROR,
Copy link
Member

Choose a reason for hiding this comment

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

GET_LOCATOR_ALL is unused. to be removed.

jo_sid = srv6_locator_sid_detailed_json(loc, sid);
json_object_array_add(jo_sids, jo_sid);
}

Copy link
Member

Choose a reason for hiding this comment

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

  • separate lib change from test change (separate commit).
  • this part of code should be made in the same commit as the changes done in show_srv6_locator_detail_cmd()

locators
locator loc1
prefix fcbb:bbbb:1::/48 block-len 32 node-len 16 func-bits 16
sid fcbb:bbbb:1:fe01:: behavior uDT6 vrf Vrf1
Copy link
Member

Choose a reason for hiding this comment

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

we usually use lower cases for vrf names.

Choose a reason for hiding this comment

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

SONiC requires Capital V as vrf name.

VRF name is just a string. There is no need to restrict it to lower cases.

@pguibert6WIND
Copy link
Member

Where is the BGP and IS-IS , and others implementation ?
Is there a roadmap for that ?

In order to support configuring
static SRv6 decapsulation functionality on the system,
we need to support configuring a purely static mode local SID.
This local SID does not rely on BGP
to randomly generate a specific mySID address.
Instead, it generates a unique mySID table entry
by manually specifying the sid value.

Signed-off-by: hanyu.zly <[email protected]>
Signed-off-by: Yuqing Zhao <[email protected]>
Add topotest zebra_static_locator_sid to check whether
static locator sids can be configured manually properly.
Besides, supplement JSON display for these sids info.
The corresponding command is
"show segment-routing srv6 locator NAME detail json".

Signed-off-by: Yuqing Zhao <[email protected]>
Update the srv6 locator configuration and
srv6 locator detail [json] display sections in zebra.rst.

Signed-off-by: Yuqing Zhao <[email protected]>
@GaladrielZhao GaladrielZhao force-pushed the project-phoenixwing-ysj branch from 0fbd628 to c2f7728 Compare January 10, 2025 03:18
@eddieruan-alibaba
Copy link

Where is the BGP and IS-IS , and others implementation ? Is there a roadmap for that ?

This is for my locator configuration.

BGP configuration would use locator as its vpn sid.
ISIS is not in our rador. But you could add them.

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

Successfully merging this pull request may close these issues.

6 participants