Project

General

Profile

Actions

Feature #12687

open

Option to disable auto-addition of static routes for ``dpinger``

Added by → luckman212 4 months ago. Updated 2 days ago.

Status:
Feedback
Priority:
Normal
Assignee:
Category:
Gateway Monitoring
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
Plus Target Version:
22.05
Release Notes:
Default

Description

Summary
  • Currently, static routes are added for each gateway monitor IP, to force dpinger ICMP to leave via the given interface. In some (I would argue most) cases, it's preferable that these static routes not be created. Mainly because it can create a situation where DNS is completely broken due to a common configuration e.g. WAN1 + WAN2, with 8.8.8.8 being used as a monitor IP on WAN1 as well as a client-side DNS server. In such a setup, if WAN1 goes down, all DNS resolution is broken.
Previous redmine New Pull Request Discussions

Files

Actions #1

Updated by Jim Pingle 4 months ago

  • Status changed from New to Pull Request Review
Actions #2

Updated by Jim Pingle 3 months ago

  • Target version set to 2.7.0
  • Plus Target Version set to 22.05
Actions #3

Updated by Jim Pingle 3 months ago

  • Status changed from Pull Request Review to Feedback
  • Assignee set to Jim Pingle

PR merged, thanks!

Actions #4

Updated by Viktor Gurov 3 months ago

after this merge, the "Gateway Edit Page" has double content

fix:
https://gitlab.netgate.com/pfSense/pfSense/-/merge_requests/656

Actions #5

Updated by → luckman212 3 months ago

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.

Actions #6

Updated by Viktor Gurov 3 months ago

→ 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

Actions #7

Updated by Flole Systems 3 months ago

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.

Actions #8

Updated by Flole Systems 3 months ago

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

Actions #9

Updated by → luckman212 3 months ago

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.

Actions #10

Updated by Flole Systems 3 months ago

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?

Actions #11

Updated by Jim Pingle 3 months ago

  • Status changed from New to Pull Request Review

Adding cleanup for routes when activating the option should probably get filed under a separate request, since this is working as originally intended by stopping the routes from being created on future attempts.

Actions #12

Updated by → luckman212 3 months ago

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

Actions #13

Updated by Jim Pingle 3 months ago

#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.

Actions #14

Updated by Flole Systems 3 months ago

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.

Actions #15

Updated by Jim Pingle 3 months ago

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.

Actions #16

Updated by Flole Systems 3 months ago

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.

Actions #17

Updated by Jim Pingle 3 months ago

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.

Actions #18

Updated by Kristopher Kolpin 26 days ago

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.

Actions #19

Updated by Jim Pingle 5 days ago

  • Status changed from Pull Request Review to Feedback

PR was merged two months ago.

Actions #20

Updated by Jim Pingle 2 days ago

  • Subject changed from Add option to disable auto-addition of static routes for dpinger to Option to disable auto-addition of static routes for ``dpinger``

Updating subject for release notes.

Actions

Also available in: Atom PDF