-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Gateways, allow for configuring a gatewaygroup as the default gateway. #3781
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
Gateways, allow for configuring a gatewaygroup as the default gateway. #3781
Conversation
2102ef1 to
2bd758d
Compare
|
I spoke with @PiBa-NL on IRC about this, there are a couple more tweaks he's going to make before we try testing it out. |
|
Good to know @jim-p. So I'll wait for it too. BTW, it's too big for 2.4 so I marked to be merged post 2.4.0 release |
|
@PiBa-NL I haven't looked at your code yet, but does this patch somehow fix https://redmine.pfsense.org/issues/5476 ? Enable traffic from the firewall itself to route using PBR? |
|
@luckman212 I don't touch the pf and its pbr functionality, though my patch is aimed at addressing the issues of pfSense not having dns and other functionality when its 'default' gateway is not available. It will pick a new default from the chosen gatewaygroup that is still 'up'. That doesnt make pbr work, as that would be configureable to behave different for different firewallrules matching traffic.. My patch only juggles with the default route.. |
|
@PiBa-NL Is this complete now, or is there still something else you planned to add/change? |
|
not complete yet.. ive got several more changes to add.. but need to clean them up a bit.. ill try and do that in next few days. |
|
In an effort to improve the tracking of changes and bug/feature requests, we have decided to require an entry on the pfSense Redmine issue tracker associated with every pull request, and likewise the Redmine entry should also have a link back to the pull request. If you could, please associate this PR to a Redmine issue either by locating an existing issue at https://redmine.pfsense.org or by creating a new issue. Add a link to the Redmine issue that points to this PR, and also add a link on the PR to the Redmine issue. For more information, see https://doc.pfsense.org/index.php/Submitting_a_Pull_Request_via_Github Thanks! |
|
Redmine issue: https://redmine.pfsense.org/issues/8187 |
2bd758d to
68beacb
Compare
jim-p
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.
Looks pretty good, we need to test it yet though.
The config upgrade code needs a small bump, since this was done originally something else already increased to config rev to 17.4, so this needs to move up to 17.5.
| } | ||
| } | ||
|
|
||
| function upgrade_173_to_174() { |
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 will need to change, it's already at 17.4 now, so this would be upgrade_174_to_175().
src/etc/inc/globals.inc
Outdated
| "crashreporterurl" => "https://crashreporter.pfsense.org/crash_reporter.php", | ||
| "debug" => false, | ||
| "latest_config" => "17.3", | ||
| "latest_config" => "17.4", |
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 will need to be 17.5
25f31e8 to
42f57e9
Compare
|
Okay bumped to 17.6 . |
rbgarga
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.
Code looks good. Still need to be tested
|
anyone interested in pulling this? yes i know the upgrade_config version number has a conflict.. but its kinda pointless to change that every time another commit upgrades that in master.. |
|
Yeah we want to bring it in, a bit backlogged at the moment. We were hoping to have someone here give it a good run-through before merging. If you could fix the conflict so it applies cleanly again that would speed things up, thanks! |
|
I will do what I can to test and merge this week. |
-Avoid changing routes by just visiting a webgui page. -Avoid change some unneeded events when nothing changed.
42f57e9 to
43a9b03
Compare
|
Okay thanks, rebased it. |
|
Hello @sbeaver-netgate, Have you test PR ? Do you think you can merge it for 2.4.4 ? |
Gateways, allow for configuring a gatewaygroup as the default gateway.
-Avoid changing routes by just visiting a webgui page.
-Avoid change some unneeded events when nothing changed.