Add per-FIB gateway support to rc.d/routing
ClosedPublic
Actions

Authored by andrew.fengler_scaleengine.com on Dec 6 2019, 5:36 PM.

Details

Reviewers

olivier
rgrimes
kevans
bcr
allanjude

Group Reviewers

manpages

Commits

rGc6ec1b441ad3: Add support for adding default routes for other FIBs
rG30659d1dcbcc: Add support for adding default routes for other FIBs

Summary

Make rc.d/routing to read defaultrouter_fibN and ipv6_defaultrouter_fibN, and
set it as the default gateway for FIB N, where N is from 1 to (net.fibs - 1)
This allows adding gateways for multiple FIBs in the same format as the main
gateway. (FIB 0)

Test Plan

The patch has been tested on several 12.0 machines, some with net.fibs == 1,
and some with net.fibs == 8 and a default gateway set for FIB 2.
Note that the _fibN gateways will only be read if you set net.fibs to a
number > 1 in /boot/loader.conf

To test:
Apply the patch for rc.d/routing
Add some extra per-fib defaultrouter variables, or not
Run rc.d/routing restart, or reboot the machine

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 28043
Build 26194: arc lint + arc unit

Event Timeline

andrew.fengler_scaleengine.com created this revision.Dec 6 2019, 5:36 PM

Herald added a reviewer: manpages. · View Herald TranscriptDec 6 2019, 5:36 PM

Herald added subscribers: Contributor Reviews (src), imp. · View Herald Transcript

Harbormaster completed remote builds in B28008: Diff 65317.Dec 6 2019, 5:36 PM

andrew.fengler_scaleengine.com added reviewers: olivier, rgrimes, kevans.Dec 6 2019, 8:10 PM

andrew.fengler_scaleengine.com added a subscriber: allanjude.

rgrimes accepted this revision.Dec 7 2019, 6:26 AM

This revision is now accepted and ready to land.Dec 7 2019, 6:26 AM

Thanks for this change. Is there a sentence stop missing in the man page change?

share/man/man5/rc.conf.5
2813	Is there is a sentence stop missing here?

olivier accepted this revision.Dec 9 2019, 2:16 AM

Fix truncated line in man page

This revision now requires review to proceed.Dec 9 2019, 3:23 PM

Harbormaster completed remote builds in B28043: Diff 65426.Dec 9 2019, 3:23 PM

Thanks for fixing it. OK from manpages.
Can you commit it yourself or should I do that (giving proper credit, of course)?

This revision is now accepted and ready to land.Dec 9 2019, 3:41 PM

In D22706#497573, @bcr wrote:

Can you commit it yourself or should I do that (giving proper credit, of course)?

Please commit it, I don't think I can. Thanks.

share/man/man5/rc.conf.5
2813	Thanks, looks like I accidentally truncated the line at some point and didn't notice.

allanjude accepted this revision.Jan 13 2020, 11:52 PM

@allanjude Can you commit these changes? For the src part, I would require that bit, so I think it would be better if you did that.

Closed by commit rG30659d1dcbcc: Add support for adding default routes for other FIBs (authored by andrew.fengler_scaleengine.com, committed by allanjude). · Explain WhyMay 12 2021, 2:04 AM

This revision was automatically updated to reflect the committed changes.

allanjude added a commit: rG30659d1dcbcc: Add support for adding default routes for other FIBs.

allanjude added a commit: rGc6ec1b441ad3: Add support for adding default routes for other FIBs.Dec 2 2022, 9:28 PM

freebsd_oldach.net added a subscriber: freebsd_oldach.net.Dec 4 2022, 9:55 AM

freebsd_oldach.net added inline comments.

libexec/rc/rc.d/routing
174	This should be ${_fibnum} not ${fibnum}?
267	This should be ${_fibnum} not ${fibnum}?

Indeed. It appears that somehow this patch differs from my local version which does have those 2 places as ${_fibnum}, as well as line 250 should be:
ipv6_static_routes="${ipv6_static_routes} _default_fib${_fibnum}"

I think this needs a new patch to be made? I'm not very familiar with phabricator so i'm not sure if this needs to be re-opened or a new patch to fix the issue.

libexec/rc/rc.d/routing
266

In D22706#855072, @andrew.fengler_scaleengine.com wrote:

Indeed. It appears that somehow this patch differs from my local version which does have those 2 places as ${_fibnum}, as well as line 250 should be:
ipv6_static_routes="${ipv6_static_routes} _default_fib${_fibnum}"

I think this needs a new patch to be made? I'm not very familiar with phabricator so i'm not sure if this needs to be re-opened or a new patch to fix the issue.

The diff can be updated (should be an "Update Diff" button along the right side near the top of the review, or arc diff --update IIRC)

For updating the diff, should i upload the proper version of the original patch, or a new patch off of head? I assume the latter.