pfSense

Thanks Viktor! Ouch, I don't know how I missed that.

I can't see the private gitlab but I assume you just removed the extra $form->add($section); from L236 right?

e.g.
https://github.com/luckman212/pfsense/compare/dd965531e98962545f6cc4cf461f5089f27da283..316f83da8d1794abfeb809c17e73ba0cc333d3cd

I posted a large warning note on the original PR#4551 explaining to back out that patch and use the new commit instead, in case anyone had manually added it.

→ luckman212 wrote in #note-5:

Thanks Viktor! Ouch, I don't know how I missed that.

I can't see the private gitlab but I assume you just removed the extra $form->add($section); from L236 right?

Right
You'll see this change after merging

With this change it should be possible to set the same monitor IP on multiple different gateways, right? The GUI isn't allowing that though.

Also I tried to enable this option for all my Gateways now but the static routes are still there. So it looks like that applying the gateway changes doesn't remove previously created routes properly? I can't seem to find a codepath for that either

Flole Systems Systems you're right that in theory you should be able to use the same monitor IP for multiple gateways after applying this change. I didn't consider that but I will test a patch and see if it works properly.

As far as removing the static routes, you're also right about that - after applying this change, currently a reboot is required. It should probably be stated in the GUI somewhere. I'm going to experiment with a better version of this patch that removes the static route without requiring a reboot.

Wow thanks, that was a fast response! I think you simply need to check if the option is set for the current gateway or for all gateways and then just omit that monitor address check for this gateway, I think it's as simple as that.

If a reboot can somehow be avoided that should be done, rebooting a firewall is really no fun ;) I think you can do something similar to https://github.com/luckman212/pfsense/blob/dd965531e98962545f6cc4cf461f5089f27da283/src/etc/inc/gwlb.inc#L2115, so if it was disabled and is now enabled remove the routes. Don't forget that this needs to happen for the global option aswell. If that is changed there doesn't seem to be the necessary "reload all gateways and create/delete the necessary routes" either?

Jim Pingle I was going to open a new PR for the additional 2 changes:

1) allow same monitor IP to be used across multiple gateways
2) add/delete routes without requiring a reboot, if setting change requires it

#1 should definitely be in its own separate PR with its own feature request. I'm not sure that's viable even without static routes as the traffic will likely flow out the default gateway without the routes in place. Even if the outbound floating rules nudge it with route-to out another interface it still hits the default gw interface first, so it may not work as intended. It will need heavy testing to ensure it's doing the right thing in each case. And that said, using the same monitor IP address on multiple interfaces is of dubious use anyhow given there are so many valid choices to use for monitor addresses in the world.

I'd still put #2 in a separate redmine issue as well so this can be tested separately.

dpinger binds itself to an interface, the routing table is never used since dpinger makes that decision. I am sometimes using dpinger with the monitoring section (under status, the one with the graphs) to compare the latency and packet loss for multiple gateways, so ideally they all have the same monitoring IP so that the results are actually comparable. Especially when there's packet loss on a route.

Also we just got rid of the routing rules (that's what this is all about), so if it causes issues (like traffic taking the default route) then they should have been tested and detected before this was merged.

Flole Systems wrote in #note-14:

dpinger binds itself to an interface, the routing table is never used since dpinger makes that decision.

Just because it binds to an interface does not mean a packet exits that interface. It still follows the routing table.

I am sometimes using dpinger with the monitoring section (under status, the one with the graphs) to compare the latency and packet loss for multiple gateways, so ideally they all have the same monitoring IP so that the results are actually comparable. Especially when there's packet loss on a route.

It makes more sense to use multiple addresses at the same destination network with static routes than the exact same address and rely on binding. In theory your method may sound good but in practice I don't see it having the intended result. It may appear to work short term but when tested thoroughly it's not likely to withstand real-world scrutiny.

Also we just got rid of the routing rules (that's what this is all about), so if it causes issues (like traffic taking the default route) then they should have been tested and detected before this was merged.

That is unrelated to this request as it's just one of several possible use cases for the option.

Uhm, this PR gets rid of the entries in the routing table. If that's a problem then this shouldn't have been merged.

Binding to an interfaces ignores the interface preferences of the routing table, so even if the routing table would route the packets through a different interface then binding to another interface bypasses that.

Flole Systems wrote in #note-16:

Uhm, this PR gets rid of the entries in the routing table. If that's a problem then this shouldn't have been merged.

As I said there are multiple use cases which may benefit. Someone may manage the routes another way (manually via static routes, a dynamic routing protocol, etc), or they may have a use case that doesn't require the routes and they conflict somehow.

Binding to an interfaces ignores the interface preferences of the routing table, so even if the routing table would route the packets through a different interface then binding to another interface bypasses that.

Binding to an interface does not change routing. It only changes the source address of the packets. If you bind a service to LAN and it tries to send out a packet to an Internet host, it will exit the default gateway.

We have automatic route-to rules in place outbound on the WANs which attempt to nudge traffic out the expected path so in most cases this will appear to work as intended but with some side effects. For example the packet will hit outbound floating rules on the default gateway WAN even if it's supposed to exit a different WAN. If it doesn't match floating rules, it will hit the automatic rules with route-to and egress as expected and the user doesn't typically have to care about that -- but it still happens. If there is no default gateway the packets may not exit any interface no matter the binding.

The OP's original concern also pops up when using a single physical WAN with multiple PPPoE sessions. Some ISPs allow multiple PPPoE sessions (up to 5 in some cases) as a way to grab additional dynamic "public" IPs. In this situation other problems arise such as the default gateway being the same for all additional public IPs. This in an of itself is fine. However, it breaks dpinger. An error message is returned saying that the dpinger IP is already in use. So, it would seem the automatic route-out rules break in this situation. One might then think, "I'll just use 8.8.8.8 to ping gateway status." Well, that then breaks any other device on the LAN pinging 8.8.8.8. Which leads us back to the OP's original concern.

This tested OK to me. Note that I only tested the checkbox in on the gateway, since it looks like the other subjects were not included in this ticket.

Tested the setting persisted across reboots.

The route is added when the checkbox is checked and saved but it is not removed when it is unchecked and saved. The route is removed if there is a quick edit/save on WAN after unchecking/saving or a reboot. It looks like this is the expected behavior.

I'd call it good.

I have a new PR almost ready that dynamically adds/removes the static routes when the checkbox is changed without requiring a reboot (as requested by Flole Systems)

I know 22.05 (.06?) is imminent so I wasn't sure whether to try to push that now or wait until after.

Needed one more fix: https://github.com/pfsense/pfsense/pull/4590

That may not make it into 22.05 at this point. If not we can re-target this at 22.09.

Flole Systems please test with the updated version of this patch if you have the time: https://github.com/pfsense/pfsense/pull/4591

What's in now will have to be considered on its own -- any refinements should be done on a separate Redmine issue.

follow-up issue: https://redmine.pfsense.org/issues/13242