GNOME Bugzilla – Bug 796645
Add support for gretap tunnels
Last modified: 2018-07-02 18:46:52 UTC
I'd like to request the addition of gre(6)tap to the ip-tunnel interface type. At least using "ip" it is configured exactly the same way as gre(6) tunnels are. I guess it must be rather simple to add it due to this, but I'm afraid I'm not well enough versed in C to actually do it. If there's anything I can do to help I'd be glad to.
Sounds reasonable to me, I'll prepare a patch.
Pushed branch bg/ip-tunnel-gretap-bgo796645 for review.
First of all I'd like to thank you for taking the time considering and implementing this feature for integration in NM. I really appreciate it! Made a quick and dirty build of this branch for easy testing with Fedora: https://copr.fedorainfracloud.org/coprs/heffer/NetworkManager-gretap/ I can only do some very limited testing right now, as I'm on the go, but I can successfully establish a gretap over IPv4 connection between two servers through a WAN link using the testing builds I have posted above and push data through it. I will do some testing on the ip6gretap tomorrow.
Oh, sorry. For completeness sake this is the command I used: nmcli connection add ifname gretap-test connection.id gretap-test connection.type ip-tunnel ip-tunnel.mode gretap ip-tunnel.remote X.X.X.X ip-tunnel.local X.X.X.X ip-tunnel.input-key 1013 ip-tunnel.output-key 1013 ipv4.method disabled ipv6.method ignore
> Made a quick and dirty build of this branch for easy testing with Fedora: > https://copr.fedorainfracloud.org/coprs/heffer/NetworkManager-gretap/ FYI, another convenient way to create SRPM/RPM from upstream NetworkManager are the scripts in contrib directory. Checkout the branch in git and run: $ ./contrib/fedora/rpm/build_clean.sh -g
> platform: add ip6gre/ip6gretap tunnels support the logging in link_ip6gre_add(), does it provide any useful information beyond what nm_platform_link_ip6gre_add() logs? Optimally, there is only one log-line (giving the most of useful information). +static int +link_ip6gre_add (NMPlatform *platform, should return gboolean. Btw, while other virtual functions do it this way, I wonder if these functions shouldn't instead return NMPlatformError enum. Instead of later doing: + if (!klass->link_ip6gre_add (self, name, props, out_link)) + return NM_PLATFORM_ERROR_UNSPECIFIED; + return NM_PLATFORM_ERROR_SUCCESS; as discussed, IFLA_GRE_IFLAGS seems to be be16 types, not 32 bit. wouldn't it make more sense to merge link_ip6gre_add() and link_ip6tnl_add()? Yes, the outer API has different functions nm_platform_link_ip6tnl_add() and nm_platform_link_ip6gre_add(), but the closer you go down, it seams at one point they become so similar, that it would be better to merge the implementations. Especially, since NMPlatformLnkIp6Tnl already can differenciate via is_gre. Regarding the tests, wouldn't it be nicer if you move: NMPlatformLnkIp6Tnl lnk_ip6tnl = { }; to the most-outer-scope, and then replace g_assert_cmpint (plnk->tclass, ==, 21); with g_assert (nm_platform_lnk_ip6tnl_cmp (&lnk_ip6tnl, plnk) == 0); ? (same for other places). > ip-tunnel: add support for gretap tunnel connections case NM_LINK_TYPE_GRE: - case NM_LINK_TYPE_GRETAP: /* Hardware address is the network-endian IPv4 address */ why this change? Maybe it's worth a separate commit to explain why? > ip-tunnel: add support for ip6gre and ip6gretap tunnel connections g_set_error (error, NM_DEVICE_ERROR, NM_DEVICE_ERROR_CREATION_FAILED, - "Failed to create IPIP interface '%s' for '%s': %s", + "Failed to create IPv6 tunnel interface '%s' for '%s': %s", indentation + val = _nm_utils_ascii_str_to_int64 (nm_setting_ip_tunnel_get_output_key (s_ip_tunnel), + 10, + 0, + G_MAXUINT32, + -1); + if (val != -1) { + lnk_ip6tnl.output_key = val; + lnk_ip6tnl.output_flags = NM_GRE_KEY; + } as we found out, IFLA_GRE_OFLAGS is 16bit. I think NMSettingIPTunnel:verify() should check that the flags are small enough (for IP6GRE/IP6GRETAP type), and it should convert them with G_MAXUINT16 Pushed two fixups. Rest lgtm.
(In reply to Thomas Haller from comment #6) > > platform: add ip6gre/ip6gretap tunnels support > > the logging in link_ip6gre_add(), does it provide any useful information > beyond what nm_platform_link_ip6gre_add() logs? Optimally, there is only one > log-line (giving the most of useful information). Fixed. > +static int > +link_ip6gre_add (NMPlatform *platform, > > should return gboolean. I've updated also existing functions. > Btw, while other virtual functions do it this way, I > wonder if these functions shouldn't instead return NMPlatformError enum. > Instead of later doing: > > + if (!klass->link_ip6gre_add (self, name, props, out_link)) > + return NM_PLATFORM_ERROR_UNSPECIFIED; > + return NM_PLATFORM_ERROR_SUCCESS; Yes, that's a possible improvement, but unless we change also do_add_link_with_lookup() to return a meaningful error code, a boolean seems enough for now. > as discussed, IFLA_GRE_IFLAGS seems to be be16 types, not 32 bit. Ok. > wouldn't it make more sense to merge link_ip6gre_add() and > link_ip6tnl_add()? Yes, the outer API has different functions > nm_platform_link_ip6tnl_add() and nm_platform_link_ip6gre_add(), but the > closer you go down, it seams at one point they become so similar, that it > would be better to merge the implementations. Especially, since > NMPlatformLnkIp6Tnl already can differenciate via is_gre. They use different netlink attributes, so keeping them separate is justified in my opinion. > Regarding the tests, wouldn't it be nicer if you move: > > NMPlatformLnkIp6Tnl lnk_ip6tnl = { }; > > to the most-outer-scope, and then replace > > g_assert_cmpint (plnk->tclass, ==, 21); > > with > > g_assert (nm_platform_lnk_ip6tnl_cmp (&lnk_ip6tnl, plnk) == 0); I've changed the GRE tests as you suggested. For IP6GRE(TAP) we can't test all attributes because our travis CI runs a Ubuntu 14.04 where iproute2 does not support 'encaplimit', so that wouldn't work. > > ip-tunnel: add support for gretap tunnel connections > > > case NM_LINK_TYPE_GRE: > - case NM_LINK_TYPE_GRETAP: > /* Hardware address is the network-endian IPv4 address */ > > why this change? Maybe it's worth a separate commit to explain why? Ok. > I think > NMSettingIPTunnel:verify() should check that the flags are small enough (for > IP6GRE/IP6GRETAP type), and it should convert them with G_MAXUINT16 The setting doesn't have input or output flags. > Pushed two fixups. Rest lgtm. Squashed, thanks.
LGTM
Merged to master: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=2ee76c88c10c109d7607501033097a0a762f9687