-
Notifications
You must be signed in to change notification settings - Fork 1.4k
staticd: in route config, reject keywords as ifname #20311
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
base: master
Are you sure you want to change the base?
Conversation
|
I think we need to add this to the staticd documentation. |
| "segments", | ||
| "nexthop-vrf", | ||
| NULL /*End sentinel*/ | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit fragile given these are all coming from inside DEFPY's defined elsewhere in this file. I'm curious if @eqvinox can take a look and see if there's some way we can leverage the cli parsing logic to disallow these automatically from the CLI command definition.
If I understand correctly this is coming from defs that look like:
...
<INTERFACE|Null0>
[{tag (1-4294967295)
|(1-255)$distance
|vrf NAME
...
I'm wondering if the CLI parser can automatically handle this.
BTW, is vrf foobar actually accepted as well? From my read here it's allowing tag NUM b/c tag is consumed by INTERFACE and then the number is consumed by (1-255)$distance, but there's no generic WORD in that follow on selection so I would expect vrf foobar to fail to parse. If that logic holds I think the only keywords that will trigger this bug are ones followed by a number from 1 to 255, as it's the (1-255)$distance atom that's causing us all the pain. If distance had had a keyword like distance i don't think we'd have this issue. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, the 'vrf XXX' tokens work - the matching is willing to use the more-specific match on the keyword 'vrf' followed by another word. but the lower-level match that would accept 'vrf' as an interface name will be rejected.
as you say, it's brittle to have to encode the ambiguous keywords, and it would be better to have an explicit "ifname YYY" instead of the positional matching. let's all try to squash examples like this as we review cli change proposals going forward...?
choppsv1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eqvinox any way to have the CLI parser deal with this?
906852f to
a5bd6b7
Compare
|
rebased to newer master |
|
yes, sure, I've pushed a small doc update too.
|
Reject cli keywords from the various 'ip route' configs if the vty code interprets them as interface names. Signed-off-by: Mark Stapp <[email protected]>
Add a note to the docs that config keywords are not valid ifnames. Signed-off-by: Mark Stapp <[email protected]>
4026428 to
8852762
Compare
|
rebased to newer master |
Reject cli keywords from the various 'ip route' configs if the vty code interprets them as interface names; some examples are "tag", "label", "color". Here's an example: