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.

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

doc/examples: remove unnecessary route for IPv6 on-link gateways #312

Merged
merged 2 commits into from
Jan 19, 2023

Conversation

sbraz
Copy link
Contributor

@sbraz sbraz commented Jan 17, 2023

Description

Hi,
When the on-link flag is enabled, the additional route has no effect. This extra route is only required when the on-link flag is not set.

Indeed, for netorkd, when "on-link" is true, the resulting systemd-networkd config file contains "GatewayOnLink=true", which translates to "flags: onlink" in its logs. The resulting route appears with this "onlink" flag in the output of "ip route".

Similarly, for NetworkManager, "route1_options=onlink=true" gets added by

netplan/src/nm.c

Lines 228 to 230 in e5ff9f6

if (cur_route->onlink) {
/* onlink for IPv6 addresses is only supported since nm-1.18.0. */
g_string_append_printf(tmp_val, "onlink=true,");

Checklist

  • Runs make check successfully → except for test_with_empty_config which also fails on the main branch on my Ubuntu 22.10 test server, so I'm assuming this is entirely unrelated.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

When the on-link flag is enabled, the additional route has no effect.
This extra route is only required when the on-link flag is *not* set.

Indeed, for netorkd, when "on-link" is true, the resulting
systemd-networkd config file contains "GatewayOnLink=true", which
translates to "flags: onlink" in its logs. The resulting route appears
with this "onlink" flag in the output of "ip route".

Similarly, for NetworkManager, "route1_options=onlink=true" gets added
by
https://github.com/canonical/netplan/blob/e5ff9f6dbeb20a8a3e6abc712efcf1b0008afa75/src/nm.c#L228-L230
@daniloegea
Copy link
Contributor

Hello and thank you for your PR.

That sounds correct.

We also should mention in the docs that scope only applies to IPv4 routes (I can check that later). We mention that in the code but we include it in the generated configuration anyway:

/* For IPv6 addresses, kernel and NetworkManager don't support a scope.

@daniloegea
Copy link
Contributor

Tagging @slyon for further thoughts.

@slyon slyon self-requested a review January 19, 2023 15:38
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. Yes, I agree with the understanding that an additional scope: link route is not needed for IPv6. Checking the origin of this example doesn't give any special reason for it to be included, neither: canonical/netplan.io@ed72e3a

And checking the relevant systemd-networkd docs for Scope= states the following:

For IPv4 route, defaults to "host" if Type= is "local" or "nat", and "link" if Type= is "broadcast", "multicast", "anycast", or "unicast". In other cases, defaults to "global". The value is not used for IPv6.

LGTM!

Also, I agree with @daniloegea's assessment that we should add a comment about scope being effective only for IPv4. I'll add such comment before merging.

@slyon slyon merged commit db608aa into canonical:main Jan 19, 2023
@slyon slyon added documentation Documentation improvements. community This PR has been proposed by somebody outside of the Netplan team and roadmap commitments. labels Oct 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community This PR has been proposed by somebody outside of the Netplan team and roadmap commitments. documentation Documentation improvements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants