Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/check.yml
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ jobs:
libibverbs-dev libasan8 libcmocka-dev libedit-dev libarchive-dev \
libevent-dev libsmartcols-dev libmnl-dev libnuma-dev python3-pyelftools \
socat tcpdump traceroute graphviz iproute2 iputils-ping ndisc6 jq \
dnsmasq \
dnsmasq scapy \
"linux-modules-extra-$(uname -r)"
if echo $MESON_EXTRA_OPTS | grep -q frr=enabled ; then
sudo apt-get install -qy --no-install-recommends \
Expand Down
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -281,13 +281,13 @@ In order to run the `smoke-tests`, `lint`, `check-patches` and `update-graph`
targets, you'll need additional packages:

```sh
dnf install gawk gdb clang-tools-extra jq codespell curl traceroute graphviz ndisc6
dnf install gawk gdb clang-tools-extra jq codespell curl traceroute graphviz ndisc6 scapy
```

or

```sh
apt install gawk gdb clang-format jq codespell curl traceroute graphviz ndisc6
apt install gawk gdb clang-format jq codespell curl traceroute graphviz ndisc6 scapy
```

### Build
Expand Down
776 changes: 403 additions & 373 deletions docs/graph.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
8 changes: 8 additions & 0 deletions frr/rt_grout.c
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,9 @@ static int grout_gr_nexthop_to_frr_nexthop(
case SR_BEHAVIOR_END_DT46:
action = ZEBRA_SEG6_LOCAL_ACTION_END_DT46;
break;
case SR_BEHAVIOR_END_DX2:
action = ZEBRA_SEG6_LOCAL_ACTION_END_DX2;
break;
}

ctx.table = ifindex_grout_to_frr(sr6->out_vrf_id);
Expand Down Expand Up @@ -726,6 +729,11 @@ grout_add_nexthop(uint32_t nh_id, gr_nh_origin_t origin, const struct nexthop *n
nh->nh_srv6->seg6local_ctx.table
);
break;
case ZEBRA_SEG6_LOCAL_ACTION_END_DX2:
sr6_local->behavior = SR_BEHAVIOR_END_DX2;
// FIXME
sr6_local->out_vrf_id = ifindex_frr_to_grout(GR_VRF_ID_ALL);
break;
Comment on lines +732 to +736
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: Incorrect function usage for DX2 interface mapping.

The code passes GR_VRF_ID_ALL to ifindex_frr_to_grout(), which expects an FRR interface index (ifindex_t), not a VRF ID constant. This will fail the mapping lookup and return GR_IFACE_ID_UNDEF.

For END.DX2 behavior, you need to extract the output interface from the seg6local context (similar to how other behaviors use nh->nh_srv6->seg6local_ctx.table). The FIXME acknowledges this is incomplete, but the current implementation will not work correctly.

Expected pattern based on other behaviors

Other local behaviors extract the interface/table from the nexthop context:

case ZEBRA_SEG6_LOCAL_ACTION_END_DT6:
    sr6_local->behavior = SR_BEHAVIOR_END_DT6;
    sr6_local->out_vrf_id = ifindex_frr_to_grout(
        nh->nh_srv6->seg6local_ctx.table
    );
    break;

For DX2, you should extract the output interface from the appropriate field in seg6local_ctx rather than using GR_VRF_ID_ALL.

🤖 Prompt for AI Agents
In @frr/rt_grout.c around lines 732 - 736, Replace the incorrect GR_VRF_ID_ALL
usage: for ZEBRA_SEG6_LOCAL_ACTION_END_DX2 set sr6_local->out_vrf_id by mapping
the FRR interface index taken from the seg6local context on the nexthop (i.e.,
use ifindex_frr_to_grout(nh->nh_srv6->seg6local_ctx.ifindex) or the actual
seg6local_ctx field that holds the output interface), so the code mirrors the
pattern used by other behaviors (e.g., END_DT6 uses
nh->nh_srv6->seg6local_ctx.table) and does not pass a VRF constant to
ifindex_frr_to_grout.

default:
gr_log_err(
"not supported srv6 local behaviour action=%u",
Expand Down
4 changes: 4 additions & 0 deletions modules/infra/api/gr_infra.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ typedef enum : uint16_t {
typedef enum : uint8_t {
GR_IFACE_MODE_L3 = 0,
GR_IFACE_MODE_L1_XC,
GR_IFACE_MODE_SRV6_XC,
GR_IFACE_MODE_COUNT
} gr_iface_mode_t;

Expand All @@ -74,6 +75,7 @@ struct __gr_iface_base {
uint16_t vrf_id; // L3 addressing and routing domain
uint16_t domain_id; // L2 xconnect peer interface id
};
uint32_t mode_data; // Abstract mode data
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels a bit hacky. Could you explain the reasoning behind this opaque parameter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Similar to the type specific info, we need a mode specific info.
Yet, we can't have 2 VLA at then end of the structure. And yes, that's a bit hacky :(

uint32_t speed; // Link speed in Megabit/sec.
};

Expand Down Expand Up @@ -452,6 +454,8 @@ static inline const char *gr_iface_mode_name(gr_iface_mode_t mode) {
return "l3";
case GR_IFACE_MODE_L1_XC:
return "l1-xc";
case GR_IFACE_MODE_SRV6_XC:
return "srv6-evpn";
case GR_IFACE_MODE_COUNT:
break;
}
Expand Down
26 changes: 25 additions & 1 deletion modules/infra/cli/gr_cli_iface.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,21 @@ struct cli_iface_type {
void (*list_info)(struct gr_api_client *c, const struct gr_iface *, char *, size_t);
};

struct cli_iface_mode {
STAILQ_ENTRY(cli_iface_mode) next;
gr_iface_mode_t mode_id;
const char *str;
int (*init)(struct ec_node *);
void (*show)(struct gr_api_client *, const struct gr_iface *);
void (*list_info)(struct gr_api_client *, const struct gr_iface *, char *, size_t);
};

void register_iface_type(struct cli_iface_type *);
void register_iface_mode(struct cli_iface_mode *);

const struct cli_iface_type *type_from_name(const char *name);
const struct cli_iface_type *type_from_id(gr_iface_type_t type_id);
const struct cli_iface_mode *mode_from_id(gr_iface_mode_t mode_id);
struct gr_iface *iface_from_name(struct gr_api_client *c, const char *name);
struct gr_iface *iface_from_id(struct gr_api_client *c, uint16_t ifid);

Expand Down Expand Up @@ -51,7 +62,7 @@ int complete_iface_names(
#define INTERFACE_SET_CTX(root) \
CLI_CONTEXT(root, INTERFACE_ARG, CTX_ARG("set", "Modify an existing interface."))

#define IFACE_ATTRS_CMD "(up|down),(promisc PROMISC),(mtu MTU),(vrf VRF)"
#define IFACE_ATTRS_CMD "(up|down),(promisc PROMISC),(mtu MTU),(vrf VRF),(mode IFACE_MODE)"

#define IFACE_ATTRS_ARGS \
with_help("Set the interface UP.", ec_node_str("up", "up")), \
Expand All @@ -67,6 +78,10 @@ int complete_iface_names(
with_help( \
"L3 addressing/routing domain ID.", \
ec_node_uint("VRF", 0, UINT16_MAX - 1, 10) \
), \
with_help( \
"interface mode.", \
EC_NODE_OR("IFACE_MODE", with_help("L3 routing.", ec_node_str("", "l3"))) \
)

uint64_t parse_iface_args(
Expand All @@ -76,3 +91,12 @@ uint64_t parse_iface_args(
size_t info_size,
bool update
);

#define CALLBACK_ATTR_IFACE_MODE "callback_iface_mode"
typedef cmd_status_t (*cmd_iface_set_cb_t)(
struct gr_api_client *,
struct gr_iface *,
const struct ec_pnode *,
uint64_t *
);
struct ec_node *with_iface_set_callback(cmd_iface_set_cb_t cb, struct ec_node *node);
106 changes: 95 additions & 11 deletions modules/infra/cli/iface.c
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,40 @@
#include <unistd.h>

static STAILQ_HEAD(, cli_iface_type) types = STAILQ_HEAD_INITIALIZER(types);
static STAILQ_HEAD(, cli_iface_mode) modes = STAILQ_HEAD_INITIALIZER(modes);

void register_iface_type(struct cli_iface_type *type) {
STAILQ_INSERT_TAIL(&types, type, next);
}

void register_iface_mode(struct cli_iface_mode *mode) {
STAILQ_INSERT_TAIL(&modes, mode, next);
}

struct ec_node *with_iface_set_callback(cmd_iface_set_cb_t cb, struct ec_node *node) {
if (node == NULL)
return NULL;
struct ec_dict *attrs = ec_node_attrs(node);
if (attrs == NULL || ec_dict_set(attrs, CALLBACK_ATTR_IFACE_MODE, cb, NULL) < 0) {
ec_node_free(node);
node = NULL;
}
return node;
}

static cmd_iface_set_cb_t find_cmd_iface_mode_callback(const struct ec_pnode *parsed) {
const struct ec_pnode *p;

for (p = parsed; p != NULL; p = EC_PNODE_ITER_NEXT(parsed, p, true)) {
const struct ec_node *node = ec_pnode_get_node(p);
cmd_iface_set_cb_t cb = ec_dict_get(ec_node_attrs(node), CALLBACK_ATTR_IFACE_MODE);
if (cb != NULL)
return cb;
}

return errno_set_null(EOPNOTSUPP);
}

const struct cli_iface_type *type_from_name(const char *name) {
const struct cli_iface_type *type;

Expand Down Expand Up @@ -57,6 +86,7 @@ int complete_iface_types(
}
return 0;
}

const struct cli_iface_type *type_from_id(gr_iface_type_t type_id) {
const struct cli_iface_type *type;

Expand All @@ -68,6 +98,17 @@ const struct cli_iface_type *type_from_id(gr_iface_type_t type_id) {
return NULL;
}

const struct cli_iface_mode *mode_from_id(gr_iface_mode_t mode_id) {
const struct cli_iface_mode *mode;

STAILQ_FOREACH (mode, &modes, next) {
if (mode->mode_id == mode_id)
return mode;
}
errno = ENODEV;
return NULL;
}

int complete_iface_names(
struct gr_api_client *c,
const struct ec_node *node,
Expand Down Expand Up @@ -147,6 +188,7 @@ uint64_t parse_iface_args(
size_t info_size,
bool update
) {
const struct ec_pnode *node;
const char *name, *promisc;
uint64_t set_attrs = 0;

Expand Down Expand Up @@ -191,6 +233,19 @@ uint64_t parse_iface_args(
if (arg_u16(p, "VRF", &iface->vrf_id) == 0)
set_attrs |= GR_IFACE_SET_VRF;

if ((node = ec_pnode_find(p, "IFACE_MODE")) != NULL) {
if (ec_pnode_find(node, "l3")) {
set_attrs |= GR_IFACE_SET_MODE;
set_attrs |= GR_IFACE_SET_VRF;
iface->mode = GR_IFACE_MODE_L3;
iface->vrf_id = 0;
} else {
cmd_iface_set_cb_t cb = find_cmd_iface_mode_callback(node);
if (cb)
cb(c, iface, node, &set_attrs);
}
}
Comment on lines +236 to +247
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Mode configuration failure is silently ignored.

Lines 243-245: when find_cmd_iface_mode_callback() returns NULL (setting errno = EOPNOTSUPP), the code silently skips the callback without reporting an error. The function continues and may return success even though the requested mode was not configured.

This differs from other parse errors in this function, which jump to the err: label and return 0. A user invoking interface set <name> mode <unsupported> would see the command succeed even though the mode was not applied.

🔧 Propagate the error
 	} else {
 		cmd_iface_set_cb_t cb = find_cmd_iface_mode_callback(node);
-		if (cb)
-			cb(c, iface, node, &set_attrs);
+		if (cb == NULL)
+			goto err;
+		cb(c, iface, node, &set_attrs);
 	}
 }
🤖 Prompt for AI Agents
In @modules/infra/cli/iface.c around lines 236 - 247, When ec_pnode_find(node,
"l3") is false and find_cmd_iface_mode_callback(node) returns NULL (with
errno=EOPNOTSUPP), the code currently ignores this and proceeds as if
successful; update the branch handling around find_cmd_iface_mode_callback so
that if cb is NULL you jump to the existing err: cleanup path (same behavior as
other parse errors) instead of continuing, ensuring the function returns failure
rather than reporting success when an unsupported mode is requested; adjust
references around find_cmd_iface_mode_callback, cb, set_attrs and the err: label
to mirror the error propagation used elsewhere in this function.


return set_attrs;
err:
return 0;
Expand Down Expand Up @@ -232,10 +287,12 @@ static cmd_status_t iface_list(struct gr_api_client *c, const struct ec_pnode *p
scols_table_new_column(table, "DOMAIN", 0, 0);
scols_table_new_column(table, "TYPE", 0, 0);
scols_table_new_column(table, "INFO", 0, 0);
scols_table_new_column(table, "", 0, 0);
scols_table_set_column_separator(table, " ");

gr_api_client_stream_foreach (iface, ret, c, GR_INFRA_IFACE_LIST, sizeof(req), &req) {
const struct cli_iface_type *type = type_from_id(iface->type);
const struct cli_iface_mode *mode = mode_from_id(iface->mode);
struct libscols_line *line = scols_table_new_line(table, NULL);
char buf[128];

Expand All @@ -253,17 +310,8 @@ static cmd_status_t iface_list(struct gr_api_client *c, const struct ec_pnode *p
scols_line_set_data(line, 2, buf);

// mode
switch (iface->mode) {
case GR_IFACE_MODE_L1_XC:
scols_line_set_data(line, 3, "XC");
break;
case GR_IFACE_MODE_L3:
scols_line_set_data(line, 3, "L3");
break;
default:
scols_line_sprintf(line, 3, "%u", iface->mode);
break;
}
assert(mode != NULL);
scols_line_sprintf(line, 3, "%s", mode->str);

// vrf
scols_line_sprintf(line, 4, "%u", iface->vrf_id);
Expand All @@ -276,6 +324,11 @@ static cmd_status_t iface_list(struct gr_api_client *c, const struct ec_pnode *p
buf[0] = 0;
type->list_info(c, iface, buf, sizeof(buf));
scols_line_set_data(line, 6, buf);
if (mode->list_info) {
buf[0] = 0;
mode->list_info(c, iface, buf, sizeof(buf));
scols_line_set_data(line, 7, buf);
}
}

scols_print_table(table);
Expand Down Expand Up @@ -436,6 +489,7 @@ static cmd_status_t iface_rates(struct gr_api_client *c, const struct ec_pnode *

static cmd_status_t iface_show(struct gr_api_client *c, const struct ec_pnode *p) {
const struct cli_iface_type *type;
const struct cli_iface_mode *mode;
char buf[128];

if (arg_str(p, "stats") != NULL) {
Expand Down Expand Up @@ -473,12 +527,19 @@ static cmd_status_t iface_show(struct gr_api_client *c, const struct ec_pnode *p
assert(type != NULL);
type->show(c, iface);

mode = mode_from_id(iface->mode);
assert(mode);
if (mode->show)
mode->show(c, iface);

free(iface);

return CMD_SUCCESS;
}

static int ctx_init(struct ec_node *root) {
struct ec_node *iface_mode = NULL;
struct cli_iface_mode *mode;
int ret;

if (INTERFACE_ADD_CTX(root) == NULL)
Expand All @@ -487,6 +548,17 @@ static int ctx_init(struct ec_node *root) {
if (INTERFACE_SET_CTX(root) == NULL)
return -1;

// IFACE_MODE is present many times in the ecoli cli tree:
// It can be configured with "interface add" and "interface set".
// Also, each interface type (port, vlan, ... ) will need to be attached
// all interface modes.
while ((iface_mode = ec_node_find_next(root, iface_mode, "IFACE_MODE"))) {
STAILQ_FOREACH (mode, &modes, next) {
if ((ret = mode->init(iface_mode)) < 0)
return ret;
}
Comment on lines +555 to +559
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a comment explaining that there are as many IFACE_MODE nodes as number of interfaces and that we need to call mode->init() on all of them?

}

ret = CLI_COMMAND(
INTERFACE_CTX(root),
"del NAME",
Expand Down Expand Up @@ -584,7 +656,19 @@ static struct cli_event_printer printer = {
},
};

static int l3_mode_init(struct ec_node *) {
// This default l3 mode is handled statically
return 0;
}

static struct cli_iface_mode iface_mode_l3 = {
.mode_id = GR_IFACE_MODE_L3,
.str = "L3",
.init = l3_mode_init,
};

static void __attribute__((constructor, used)) init(void) {
cli_context_register(&ctx);
cli_event_printer_register(&printer);
register_iface_mode(&iface_mode_l3);
}
Loading
Loading