netplan status --diff fixes and improvements #466
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
daniloegea
force-pushed
the
status_diff_fixes
branch
3 times, most recently
from
2f63fca
to
d1210f2
Compare
slyon
reviewed
Jun 5, 2024
There was a problem hiding this comment.
Thank you very much for those enhancements and quality of life improvements!
Looks mostly good to me. Please consider using named constants for the ip-address family instead of plain numbers.
Also, we should avoid introducing new public API for deprecated symbols (gateay4/6), still handling them as you did via internal/private API is fine!
The (deprecated but still available) properties gateway4 and gateway6 were not being taken into account when comparing routes. Because of that, the diff report will show the default route as missing in netplan. There is a complication when considering gateway4 and gateway6 though: both networkd and NM will set a metric for them. As these properties cannot set metrics, these routes would still be shown in the diff report. To avoid this noise, when gateway4 and/or gateway6 are used AND the system contains one, and only one, default route per protocol family, we adopt the same metric and route table from the existing routes. By doing this, the comparison will see that both routes, from the system and from netplan, are the same and not show them as a diff.
When available, use the flag "dynamic" from addr_info to check if the IP addresses was dynamically assigned. The flag will be displayed in the status output along other flags: Addresses: 10.86.126.222/24 (dynamic, dhcp)
There are cases where IPv6 addresses received via RA are not set as 'dynamic'. In these cases, use the information about routes to find these IPs and add the tag 'ra' to them. It's done by iterating through IPv6 addresses and checking if there is any addresses in the same network as routes received via RA.
Add the Python binding and unit test. NOTE accept_ra is not perfectly mapped from the netdef structure to the YAML setting and back to the renderer. It's a boolean value in the YAML but it maps to three different values inside libnetplan: kernel, enabled and disabled. Not setting it in the YAML means it will not be set in networkd. Although, networkd will disable it unconditionally in the kernel and will itself handle RAs, meaning that when unset, it's enabled by default (with a few exceptions, see systemd.network(5)). Because of that, the low level C API will return the actual internal value of the property and not only TRUE or FALSE. The Python code will return True, False or None (when it's unset). This will help the consumers to differentiate the case where it's disabled or unset.
When IPv6 link-local and RA (which depends on link-local) are enabled, the system will receive IP addresses, nameservers and routes dynamically via RA. These addresses are not obviously identified as dynamic in the system. networkd probably does have this information but it's not fully exposed apparently. Here I try to improve the comparison of these addresses by checking if they are 1) a link-local address and 2) part of the same network as the route received via RA. Without this, status --diff will show the RA IP addresses and link-local DNS nameserver as a diff.
The logic to remove tags from the output wasn't working for the interface header in diff mode.
daniloegea
force-pushed
the
status_diff_fixes
branch
from
a24921b
to
762b73e
Compare
When networkd is used, the ConfigSource property can identify the source of the configuration so let's use it to be more precise and reduce the guessing. We consume the same information when Network Manager is the renderer but in this case networkd will not know the sources so this property can't be relied on. For now, we collect the sources for IP addresses and DNS. Routes can be included in the future. We also should create abstractions for each configuration so we don't need to keep information such as the data source in separate tables. Network Manager seems to expose the source through its DBus API. This also could be considered in the future.
|
Thank you, Lukas. I addressed your comments and tried to improve a little the analysis of dynamic configuration with networkd. |
slyon
reviewed
Jun 26, 2024
slyon
approved these changes
Jun 26, 2024
There was a problem hiding this comment.
Thanks a lot for addressing my remarks! I like the idea of using networkd's ConfigSource. Handling similar cases through NetworkManager's DBus API in the future is a good approach!
I pushed a tiny commit to clean up some unnecessary newlines/whitespace in include/netdef.h. Otherwise, LGTM. Let's get this merged!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains a bunch of small improvements to
netplan status --diffand some additions to the libnetplan API:gateway4andgateway6properties. This is required during the analysis of routes.gateway4andgateway6are added to the list of routes being analyzed.dynamicfromaddr_infoto help determining if the address was received dynamically.raflag from routes to help finding IPs that were received via RA.accept-raproperty.--diffmode whenpython3-richis not present.I recommend testing it inside a LXD VM where IPV6 addresses are assigned dynamically. The currently netplan.io will show the IPv6 address and DNS as diffs. The heuristics added to the analysis help to reduce this noise.
Description
Checklist
make checksuccessfully.make check-coverage).