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

networkd: Implement ipv6-address-generation: stable-privacy #480

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

tatokis
Copy link
Contributor

@tatokis tatokis commented Jun 16, 2024

Description

The relevant systemd pull request has long been merged, so add support for IPv6Token=prefixstable in the networkd generator.

systemd/systemd#16618

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.

@tatokis tatokis force-pushed the networkd-ipv6-stable-privacy branch from 69b6ab0 to 9cb7136 Compare June 16, 2024 06:30
@slyon slyon added the community This PR has been proposed by somebody outside of the Netplan team and roadmap commitments. label Jun 18, 2024
@slyon slyon self-requested a review June 18, 2024 09:16
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!

I very much like this PR and the fact that you're fixing an existing TODO. Kudos!

When checking the context of this PR, I found that the [Network].IPv6Prefix= setting isn't listed anymore in the most recent man-page. It seems like the logic was shuffled a little to make use of a Token= setting instead, see: https://www.freedesktop.org/software/systemd/man/latest/systemd.network.html (and systemd/systemd#20778)

There might still be some legacy-fallback, which might keep this PR totally valid!
Are we certain that it actually works, though? Would you mind creating an integration test around it, e.g. similar to tests/integration/ethernets.py:test_dhcp6, but then checking for the stable prefix?

src/networkd.c Show resolved Hide resolved
tests/generator/test_ovs.py Outdated Show resolved Hide resolved
@tatokis
Copy link
Contributor Author

tatokis commented Jul 8, 2024

Thank you very much for your contribution to Netplan!

I very much like this PR and the fact that you're fixing an existing TODO. Kudos!

Thanks! Apologies for the super late reply; I haven't abandoned this.

When checking the context of this PR, I found that the [Network].IPv6Prefix= setting isn't listed anymore in the most recent man-page. It seems like the logic was shuffled a little to make use of a Token= setting instead, see: https://www.freedesktop.org/software/systemd/man/latest/systemd.network.html (and systemd/systemd#20778)

There might still be some legacy-fallback, which might keep this PR totally valid! Are we certain that it actually works, though? Would you mind creating an integration test around it, e.g. similar to tests/integration/ethernets.py:test_dhcp6, but then checking for the stable prefix?

Indeed. I did not notice that it was removed from the documentation. I am 100% sure it still works though since I am actively using it myself on noble.

That said, I think I should re-do this with the Token setting under [IPv6AcceptRA] instead. It was added in systemd v250 systemd/systemd@a73628e which I believe to be okay. (Unless you explicitly want jammy (systemd v249) support. Focal and earlier was always out of the question.)

Please let me know what you'd prefer.

I'll look into adding an integration test either way, although it might be difficult as I can not for the life of me get the tests and the coverage checks to run correctly locally. I'll let you know how it goes.

} else {
switch (def->ip6_addr_gen_mode) {
case NETPLAN_ADDRGEN_DEFAULT:
case NETPLAN_ADDRGEN_EUI64:
Copy link
Contributor Author

@tatokis tatokis Jul 8, 2024

Choose a reason for hiding this comment

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

Note: IPv6AcceptRA.Token now accepts eui64 explicitly since systemd 250, so that should be added too. systemd/systemd@140bf8d

@slyon
Copy link
Collaborator

slyon commented Jul 10, 2024

When checking the context of this PR, I found that the [Network].IPv6Prefix= setting isn't listed anymore in the most recent man-page. It seems like the logic was shuffled a little to make use of a Token= setting instead, see: https://www.freedesktop.org/software/systemd/man/latest/systemd.network.html (and systemd/systemd#20778)
There might still be some legacy-fallback, which might keep this PR totally valid! Are we certain that it actually works, though? Would you mind creating an integration test around it, e.g. similar to tests/integration/ethernets.py:test_dhcp6, but then checking for the stable prefix?

Indeed. I did not notice that it was removed from the documentation. I am 100% sure it still works though since I am actively using it myself on noble.

@tatokis Great! That's good to know. systemd usually has pretty good backwards compatibility, so I was expecting it to work, even though it's not document for the most recent version.

That said, I think I should re-do this with the Token setting under [IPv6AcceptRA] instead. It was added in systemd v250 systemd/systemd@a73628e which I believe to be okay. (Unless you explicitly want jammy (systemd v249) support. Focal and earlier was always out of the question.)

Please let me know what you'd prefer.

Yes and no... I'd suggest a 2-step approach (3 steps, actually):

  1. Let's get the minor comments fixed and the unit- & coverage-test green on this PR as-is. So we can get it landed soon and potentially integrate for the upcoming Netplan 1.1 release (planned for August). Those commits would also allow for easier backporting to Jammy (if needed), as you mentioned above (no need to worry about Focal or earlier).
  2. Optionally: Provide an integration test (see below), as this would make refactoring easier afterwards
  3. Prepare a new PR, building on top of this to refactor stuff, using the new Token= setting. If we don't have an integration test, yet, get one ready for this new PR.

I'll look into adding an integration test either way, although it might be difficult as I can not for the life of me get the tests and the coverage checks to run correctly locally. I'll let you know how it goes.

Yeah, it needs some setup to run it locally, but the GitHub actions workflow from our CI, can be used as a template for step-by-step instructions of how to run the autopkgtests inside a local LXD container, see: https://github.com/canonical/netplan/blob/main/.github/workflows/autopkgtest.yml#L34

Furthermore, you should be able to confirm the commands & resulting IP addresses on your local machine natively and then the CI will run the new tests on this PR for you. It's OK if you need to push multiple fixups to make the test work ;-)

The relevant systemd pull request has long been merged, so add support
for IPv6Token=prefixstable in the networkd generator.

systemd/systemd#16618
@tatokis tatokis force-pushed the networkd-ipv6-stable-privacy branch from 9cb7136 to fff4674 Compare July 16, 2024 12:06
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 addressing my remarks!

This is ready to be merged, as in step (1) is done.
I'm looking forward to seeing step (2) & (3) addressed in follow-up PR(s). Keep up the good work, thanks!

@slyon slyon merged commit b879213 into canonical:main Jul 17, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants