hook: accept interface names in ifindex= option#508
hook: accept interface names in ifindex= option#508Bodlux wants to merge 1 commit intofacebook:mainfrom
Conversation
|
Hi @Bodlux! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Hi, thanks for your first contribution! |
qdeslandes
left a comment
qdeslandes
left a comment
There was a problem hiding this comment.
Repeating my comment:
Hi, thanks for your first contribution! ifindex is more suited for an interface index (number). I would suggest introducing iface= which would allow either an interface index or an interface name.
Also, could you get rid of the comment to trigger the CLA check.
|
Thanks for the suggestion. I've renamed the option from ifindex= to iface= so it accepts either an interface name or index. |
qdeslandes
left a comment
qdeslandes
left a comment
There was a problem hiding this comment.
This change is incomplete, as it's only modifying the code and not the doc. Additionally, renaming an existing option will break use cases. An alternative would be to create a new iface option which handles interface index and interface name, then make ifindex as deprecated in the doc.
Add a new hook option `iface=` that accepts either an interface name (resolved via `if_nametoindex`) or a numeric interface index. The existing `ifindex=` option is kept for backward compatibility but is now marked as deprecated: it continues to accept numeric indices only, and emits a runtime warning pointing users to `iface=`. `iface=` and `ifindex=` share the same `BF_HOOKOPTS_IFINDEX` slot, so either one satisfies the XDP/TC requirement and specifying both in the same chain is rejected as a duplicate. The on-wire (msgpack) serialization key remains `ifindex`, so existing serialized chains continue to load unchanged. Docs, unit tests and e2e duplicate-detection tests are updated. Relates to facebook#409.
Bodlux
force-pushed
the
feat/ifindex-accept-interface-names
branch
from
8d8081d to
55647eb
Compare
|
Thanks for the detailed feedback, @qdeslandes. I've reworked the PR along the lines you suggested and force-pushed a single, clean commit. Summary of the new approach:
Let me know if you'd like any further adjustments. |
qdeslandes
left a comment
qdeslandes
left a comment
There was a problem hiding this comment.
Small changes, otherwise LGTM.
| /* Alias table: alternative names that map to an existing bf_hookopts_type but | ||
| * use a different parser. The `type` field determines which bit in `used_opts` | ||
| * is set, so aliases share duplicate-detection and flavor | ||
| * requirement/support checks with their primary entry. */ | ||
| static struct bf_hookopts_ops _bf_hookopts_alias_ops[] = { | ||
| {.name = "iface", | ||
| .type = BF_HOOKOPTS_IFINDEX, | ||
| .required_by = 0, | ||
| .supported_by = BF_FLAGS(BF_FLAVOR_XDP, BF_FLAVOR_TC), | ||
| .parse = _bf_hookopts_iface_parse, | ||
| .dump = _bf_hookopts_ifindex_dump}, | ||
| }; |
There was a problem hiding this comment.
No need for a dedicated table here, this entry should be in _bf_hookopts_ops, and you should add a check for incompatibility in their respective parsers:
_bf_hookopts_iface_parse: error ififindexis defined, and error ififaceis already defined_bf_hookopts_ifindex_parse: error ififaceis defined, and error ififindexis already defined
2 participants
Use bf_if_index_from_str() to parse the ifindex= hook option value, which accepts both numeric indices and interface names (e.g. eth0). This is consistent with how the REDIRECT verdict already resolves interface references.
Closes #409