-
Notifications
You must be signed in to change notification settings - Fork 208
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
Conversation
69b6ab0
to
9cb7136
Compare
There was a problem hiding this 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?
Thanks! Apologies for the super late reply; I haven't abandoned this.
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 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: |
There was a problem hiding this comment.
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
@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.
Yes and no... I'd suggest a 2-step approach (3 steps, actually):
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
9cb7136
to
fff4674
Compare
There was a problem hiding this 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!
Description
The relevant systemd pull request has long been merged, so add support for
IPv6Token=prefixstable
in the networkd generator.systemd/systemd#16618
Checklist
make check
successfully.make check-coverage
).