Skip to content
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.

Sign up for GitHub

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

Jump to bottom

set up routes correctly (pull request for #94) #95

Merged
merged 1 commit into from
Feb 7, 2017
Merged

set up routes correctly (pull request for #94) #95

merged 1 commit into from
Feb 7, 2017

Conversation

mrbaseman
Copy link
Collaborator

@mrbaseman mrbaseman commented Feb 1, 2017 •
edited
Loading

This is a larger rework of the routing code, here a summary of the changes (for a detailed discussion see in issue #94 ):

  • in ipv4_set_split_routes: add a host route to the gateway (basically copy&paste code from ipv4_set_default_routes)
  • also in ipv4_set_split_routes: merge in the two fixes for Routes not correctly set #25, namely add the gateway flag to these routes and corrected the test of the gateway ip (I have merged in pull request fix split routes route definition (issue #25) #88 here, for details see there). @adrienverge: if you want to merge this as a separate pull, please let me know and I can undo the changes in my branch.
  • in ipv4_set_default_routes: change the route flag to host instead of gateway for the route to the Fortigate. This is important when the existing default route is over a ppp device.
  • in ipv4_restore_routes: remove the host route to the gateway which is now added in gateway mode, and free resources (just work that has to be done now that the above changes have been implemented).
  • in ppp_interfaces_is_up: here are the changes from fix detecting new connection bug when ppp connections are existed. #90 which has already been merged. This actually was the starting point for the whole work. Since this is already upstream, these changes can be removed by rebase and squash of my whole pull request. I just did not do this yet, because I first thought that comments on the individual commits would stay intact, which was not the case.

@mrbaseman
Copy link
Collaborator Author

mrbaseman commented Feb 1, 2017 •
edited
Loading

I just noticed that the comments which I have placed on the individual commits all showed up in a pile above, which didn't make much sense. So I have removed them. So maybe I should just rebase and squash?

@adrienverge
Copy link
Owner

Thanks @mrbaseman, this looks good!

Yes it's better to squash and have one commit for one logical change, but I would do it anyway before merging.

Most importantly, you need to explain the change in the commit message itself, so it is possible to understand why code changed by looking at the git history. This allows busting bugs more efficiently, avoid mistakes, enable useful git blame, etc. Also if this project moves from GitHub some day, we'll still have this information in git itself. (If you do a git log on this repo, you'll see that all non-trivial commits do this.)

About #88: do as you prefer, either include it as a separated commit here or wait for #88 to be merged.

@mrbaseman
This is a larger rework of the routing code
88226bd 
This issue has been discussed in adrienverge#94 (set up routes correctly when ppp0 interface is already there) as a follow-up for adrienverge#90 which implemented that a new ppp device is used when ppp0 is already there.
- in ipv4_set_split_routes: add a host route to the gateway (basically copy&paste code from ipv4_set_default_routes)
- in ipv4_set_split_routes: merge in the two fixes for issue adrienverge#25 from adrienverge#88:
   1) add the gateway flag to the routes received from the remote side. Without the gateway flag the destination is directly connected (over the vpn tunnel), which is not always the case. There might be more hops behind the remote gateway to reach the network to which this route points.
   2) correct the test if the route has already a gateway set or not: -1 casted to an unsigned int would result in UINT_MAX which is 255.255.255.255 in IP notation. Routes without a gateway however carry the gateway IP 0.0.0.0 which is 0 written as uint.
- in ipv4_set_default_routes: change the route flag to host instead of gateway for the route to the Fortigate. This is important when the existing default route is over a ppp device, because in that case the gateway flag can not be set on linux. Moreover, the gateway flag is not needed here at all, because the vpn traffic is not routed via this ip as a gateway in the sense of a routing instance. The traffic is sent through the vpn channel and the encrypted packages are sent to the remote IP which can be seen as a single host that unpacks the encrypted traffic afterwards.
- in ipv4_restore_routes: remove the host route to the gateway which is now added in gateway mode, and properly free resources
@mrbaseman mrbaseman changed the title set up routs correctly (pull request for #94) set up routes correctly (pull request for #94) Feb 7, 2017
@mrbaseman
Copy link
Collaborator Author

Hi @adrienverge,

I have rebased and squashed the pull request and added the above explanations to the commit message. I have also inserted a few comments from #90. I have kept the changes of #88 in this pull request because it is also part of the fix for the routing issues. I have also included my comments from there in the commit message.

@adrienverge
Copy link
Owner

Thanks!

@adrienverge adrienverge merged commit 80745fe into adrienverge:master Feb 7, 2017
This was referenced Feb 7, 2017
Closed
Closed
Closed
Closed
@mrbaseman mrbaseman deleted the ppp-routes branch February 7, 2017 13:29
This was referenced May 4, 2017
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mrbaseman @adrienverge