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

feat(networkd): add ipv6 ra overrides (LP: #1973222) #461

Merged
merged 16 commits into from
Jun 6, 2024

Conversation

KhooHaoYit
Copy link
Contributor

@KhooHaoYit KhooHaoYit commented Apr 21, 2024

Description

This PR implements all Ipv6AcceptRA Options in Ubuntu 18.04 LTS

I wasn't able to pass all test due to test_status_diff.py due to minuscule difference ranging from mac address to [hightlight] soo both make check and make check-coverage checklist are not checked, the rest of the test should be passing
Seems like the CI was able to pass all check and coverage, soo this issue is now resolved

I also have questions about that which version of networkd, NetworkManager, ubuntu, and other are we supporting, since it looks like networkd are using [DHCPv4] and [DHCPv6], while we are using [DHCP] which I can barely find any documentation about in the internet

Feedback is appreciated

Checklist

  • Runs make check successfully.
  • 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: LP#1973222

References

Ipv6AcceptRA Options for Ubuntu 18.04 LTS (Bionic Beaver) - manpages.ubuntu.com
Ipv6AcceptRA Options for Ubuntu 22.04 LTS (Jammy Jellyfish) - manpages.ubuntu.com
DHCPv4 and DHCPv6 Configuration - freedesktop.org
DHCPv4 and DHCPv6 Configuration - manpath.be
DHCP Configuration - manpath.be
First commit using DHCP - github.com

@slyon slyon added schema change This PR includes a change to netplan's YAML schema, which needs sign-off community This PR has been proposed by somebody outside of the Netplan team and roadmap commitments. labels Apr 22, 2024
Copy link
Collaborator

@daniloegea daniloegea left a comment

Choose a reason for hiding this comment

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

Hello, thank you so much for your contribution. I think it's an important addition to Netplan!

The current version of networkd supports many other options for [IPv6AcceptRA] but we can extend it later. We usually target the recent versions of the backends we support.

I left a few suggestions in your PR I'd like you to consider.

Thank you :)

src/abi.h Outdated Show resolved Hide resolved
src/abi.h Outdated Show resolved Hide resolved
src/netplan.c Show resolved Hide resolved
src/networkd.c Outdated Show resolved Hide resolved
@slyon slyon changed the title feat(networkd): add ipv6 ra overrides feat(networkd): add ipv6 ra overrides (LP: #1973222) Apr 22, 2024
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.

Thank you very much for your contribution to Netplan! You touched quite a few modules to get this change landed. Kudos for working through all of them!

Thanks for working through Danilo's recommendations already! I left plenty of additional inline comments. It's mostly small stuff that should be relatively easy to fix.

Regarding the YAML schema addition, I mostly agree with your proposal. It matches networkd's naming and I couldn't find corresponding NetworkManager settings. But I think should consider renaming the new setting to ra-overrides staying consistent with the accept-ra setting that it relates to (without the "ipv6" prefix). Furthermore, I think we should rename route-table to just table, as it is used in other places in Netplan's configuration. E.g.:

  accept-ra: true
  ra-overrides:
    use-routes: false
    use-domains: false
    table: 127

Alternatively, I was also pondering about implicitly setting accept-ra: true when using a mapping, as those settings only make sense in combination. But that would not be consistent with the existing dhcp4/6-overrides stanzas:

  accept-ra: # implicitly set to "true", as a mapping is defined below
    use-dns: false
    use-domains: false

I'd like to get an opinion from our architect about that.
@vorlonofportland What do you think about this new schema?

Comment on lines +408 to +409
> (networkd back end only) Overrides default IPv6 Router Advertisement (RA)
> behaviour; see the `IPv6 Router Advertisement Overrides` section below.
Copy link
Collaborator

Choose a reason for hiding this comment

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

question: Did you do any research if this could also be implemented in our NetworkManager backend? Can you link any relevant NM documentation?

Copy link
Contributor Author

@KhooHaoYit KhooHaoYit May 11, 2024

Choose a reason for hiding this comment

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

I was able to find ipv6.ignore-auto-dns but I am not sure if dhcp6-overrides.use-dns or ra-overrides.use-dns should be used to set that option

I am able to ignore the DNS coming from RA for my VM by setting ipv6.ignore-auto-dns to true with ipv6.method being auto

doc/netplan-yaml.md Outdated Show resolved Hide resolved
doc/netplan-yaml.md Outdated Show resolved Hide resolved
@@ -697,6 +702,40 @@ client processes as specified in the Netplan YAML.
>
> **Requires feature: `dhcp-use-domains`**

## IPv6 Router Advertisement Overrides
Copy link
Collaborator

Choose a reason for hiding this comment

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

note (non-blocking): There are plenty settings in networkd's [IPv6AcceptRA] section, but I think it's sensible to start implementing a small subset at first.

doc/netplan-yaml.md Outdated Show resolved Hide resolved
src/parse.c Outdated Show resolved Hide resolved
src/parse.c Outdated Show resolved Hide resolved
src/networkd.c Outdated Show resolved Hide resolved
tests/generator/base.py Outdated Show resolved Hide resolved
tests/generator/test_ipv6_ra_overrides.py Outdated Show resolved Hide resolved
@KhooHaoYit KhooHaoYit requested a review from slyon May 11, 2024 14:41
@vorlonofportland
Copy link

I'd like to get an opinion from our architect about that. @vorlonofportland What do you think about this new schema?

Thanks, this looks pretty good. Notes:

  • "ra" is used elsewhere in the schema to refer to IPV6 RA (e.g. accept-ra) so this is consistent and appropriate.
  • "overrides" is consistent with dhcp4-overrides and dhcp6-overrides, so should at least be considered for consistency
  • I believe that the default for use-domains (and current behavior) is False? this should be included in the documentation (I notice the systemd.network(5) manpage also does not document the default...)
  • referring to the table and use-domains settings as "overrides" is not necessarily correct; e.g. setting use-domains to a non-default value is not overriding the RA but instead honoring the option when sent, and there is no such thing as a table value from the RA. However, we also already have dhcp4-overrides: use-domains:, so should not treat that as a blocker.

@slyon slyon added the schema ok label Jun 6, 2024
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.

Thank you for addressing all of my remarks!

LGTM, and kudos, this is a really nice PR!

I left one tiny inline-comment, also we should probably address this comment from Steve before merging:

I believe that the default for use-domains (and current behavior) is False? this should be included in the documentation

I might be able to do that quickly myself..

src/abi.h Outdated Show resolved Hide resolved
@slyon slyon merged commit 44b2a5c into canonical:main Jun 6, 2024
15 of 17 checks passed
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. schema change This PR includes a change to netplan's YAML schema, which needs sign-off schema ok
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants