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

Disable temporary address generation when renderer is NetworkManager (LP: #1948027) – change in behavior #244

Merged
merged 5 commits into from
Dec 10, 2021

Conversation

peleroux
Copy link
Contributor

@peleroux peleroux commented Nov 10, 2021

…and ipv6-privacy is false

Description

Fix for https://bugs.launchpad.net/bugs/1948027

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#1948027

@slyon
Copy link
Collaborator

slyon commented Nov 11, 2021

Thank you very much for the contribution @peleroux !
This change seems to interfere with some of the existing unit tests in make check (see https://github.com/canonical/netplan/runs/4180730044?check_suite_focus=true) Could you please check those failures and handle them accordingly?

FTR: Relevant documentation: https://developer-old.gnome.org/NetworkManager/stable/settings-ipv6.html

@slyon slyon self-requested a review November 11, 2021 17:01
@peleroux
Copy link
Contributor Author

Thank you for your answer.
Last commit should fix errors in make check.

@codecov-commenter
Copy link

codecov-commenter commented Dec 10, 2021

Codecov Report

Merging #244 (7ca04c2) into main (5361619) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #244   +/-   ##
=======================================
  Coverage   99.10%   99.10%           
=======================================
  Files          58       58           
  Lines       10035    10038    +3     
=======================================
+ Hits         9945     9948    +3     
  Misses         90       90           
Impacted Files Coverage Δ
tests/generator/base.py 100.00% <ø> (ø)
tests/generator/test_common.py 100.00% <ø> (ø)
tests/generator/test_dhcp_overrides.py 100.00% <ø> (ø)
tests/generator/test_routing.py 100.00% <ø> (ø)
tests/generator/test_tunnels.py 100.00% <ø> (ø)
tests/generator/test_vlans.py 100.00% <ø> (ø)
src/nm.c 100.00% <100.00%> (ø)
tests/parser/base.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5361619...7ca04c2. Read the comment docs.

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 Pierre-Emmanuel for this contribution and bug fix! (Sorry for me taking so long to get around reviewing this)

The fix is perfectly valid and brings the NM renderer implementation in line with the networkd backend and what is suggested by our netplan reference documentation:

Enable IPv6 Privacy Extensions (RFC 4941) for the specified interface, and
prefer temporary addresses. Defaults to false - no privacy extensions. There
is currently no way to have a private address but prefer the public address.

networkd's IPv6PrivacyExtensions= setting defaults to false and does not generate privacy addresses: https://www.freedesktop.org/software/systemd/man/systemd.network.html#IPv6PrivacyExtensions=

NM OTOH has been using the fallback mechanism to check the NM's global ipv6.ip6-privacy setting or fall back to the kernel's setting in /proc/sys/net/ipv6/conf/default/use_tempaddr (that would be networkd's "kernel" config value). => Therefore it would fall back to generate privacy IPs anyways, regardless of the ipv6-privacy: false setting.

So this is a change in default behaviour for the NM renderer backend, but at the same time fixes a bug to bring it in line with what is suggested by the documentation (and actually be able to disable privacy IP generation).

I'll merge this, but we should make sure to mention the change in behaviour in the release notes.

PS: I had a tiny remark about your adopted tests in test_keyfile.py, we should mark the ip6-privacy=0 setting as a default value instead of passing it through the keyfile parser. I fixed that up myself and pushed to your branch.

@slyon slyon changed the title Disable temporary address generation when renderer is NetworkManager … Disable temporary address generation when renderer is NetworkManager (LP: #1948027) – change in behavior Dec 10, 2021
@slyon slyon merged commit 64e381e into canonical:main Dec 10, 2021
slyon added a commit that referenced this pull request Feb 16, 2022
Commit 64e381e (#244) changed netplan's
behaviour to always write out the ipv6-privacy value (default to 0=off),
in order to comply with what is written in our documentation and how it is
implemented in the networkd backend.

This is a problem for our NetworkManager integration, as NM's default value
is -1=unknown. Netplan only supports values 0=off and 2=on while NM supports
-1, 0, 1 and 2. We need to handle unsupported cases (-1 and 1) via passthrough.
slyon added a commit that referenced this pull request Feb 16, 2022
Commit 64e381e (#244) changed netplan's
behaviour to always write out the ipv6-privacy value (default to 0=off),
in order to comply with what is written in our documentation and how it is
implemented in the networkd backend.

This is a problem for our NetworkManager integration, as NM's default value
is -1=unknown. Netplan only supports values 0=off and 2=on while NM supports
-1, 0, 1 and 2. We need to handle unsupported cases (-1 and 1) via passthrough.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants