The current approach is flawed. During a commit of the L3
configuration we do a RTM_GETROUTE to find the next-hop to the DNS
server on the current interface, in order to create the DNS route to
inject into the l3cd. However, we haven't added routes to kernel yet
and so the result of the RTM_GETROUTE is going to be wrong.
In some cases, for example when IPv4 DAD is enabled, the bug can't be
easily noticed because we perform multiple commits for the interface,
and the regular routes are already set in kernel from the 2nd commit
on.
To fix the problem, do the following: during a commit we first add
addresses and routes to platform. Then, we create a list of DNS routes
to configure, we collect the old DNS routes, and do a comparison. If
they changed, we need to add the DNS routes to platform in a 2nd step.
Note that in the previous approach we tracked the routes in the
committed-l3cd object of the l3cfg, and so they were applied to kernel
automatically. Because of the 2-step requirement, that no longer works
and we must apply the DNS routes manually.
Fixes: 5449b18a94 ('core: support automatically adding DNS routes')
This function would be useful when performing operations related to the
IPv4 addresses configured on the l3cfg. E.g this function will be used
for getting the IPv4 to announce on a GARP on bonding-slb when one of
the ports failover.
When the "ipvX.routed-dns" property is set to true, add a route for
each DNS server via the current interface. The feature works in the
following way.
A new routing rule is created ("priority $PRIO not fwmark $MARK lookup
$TABLE") where $PRIO, $MARK and $TABLE are fixed values and are the
same for all interfaces. This rule is evaluated before standard rules
and tries to look up routes in table $TABLE, where NM adds the routes
to DNS servers.
To determine the next-hop to the name server, NM issues a RTM_GETROUTE
netlink request to kernel, specifying to return the route via the
current interface. In order to avoid results from $TABLE, NM also sets
the fwmark as $MARK in the request.
During a commit of layer-3 configuration, multiple signals are
emitted:
- if the combined l3cd configuration changes, we first emit a
L3CD_CHANGED signal, with flag `commited` FALSE;
- if the previously committed configuration is different from the one
we want to commit, we emit again the same signal with `commited`
TRUE;
- a PRE_COMMIT signal
- a POST_COMMIT signal
The usefulness of the first and third signals is questionable: there
is no need to signal that the configuration changes if we are not
going to commit it. Also, PRE_COMMIT is redundant as we just emitted
L3CD_CHANGED. Nobody is using those 2 signals.
Simplify this by leaving only PRE_COMMIT and POST_COMMIT, which are
always emitted during a commit and provide information on the l3cd
changes.
This commit doesn't change behavior.
When a collision is detected by the Address Conflict Detection
mechanism, store the conflicting MAC address in NML3AcdAddrInfo, so
that it is available to listeners of NML3Cfg for events of type
NM_L3_CONFIG_NOTIFY_TYPE_ACD_EVENT.
Previously, there was "temporary-not-available" mechanism in NML3Cfg,
which aimed to handle IPv6 routes with prefsrc. Theoretically, that
mechanism may have been extended to other use-cases, like IPv4 routes
with prefsrc. What it attempted to handle, is the inability to configure
such routes, unless the respective prefsrc address is configured and
non-tentative. However, the address that we are waiting for, could also
be on another interface, so that mechanism wasn't applicable. This is
now replaced by _routes_watch_ip_addrs(). It seems there isn't anything
useful left for the "temporary-not-available" mechanism and it can go,
except...
We want to log a warning when we are unable to configure a route. Also,
in the future we might want to know when the IP configuration is
degradated due to inability to configure the desired routes (a condition
that we might want to expose to the user, not only via logging; or we
may want to react on that).
However, with prefsrc routes we don't know right away whether the
inability to configure the route right away indicates an actual problem,
or whether that will resolve itself (e.g. after the address passes
DAD/ACD, after we received an DHCP lease or after the address was
configured on another interface). Consequently, to know whether the
current inability to configure such a route is a problem, we need to
know the larger context. nm_platform_ip_route_sync() does not have that
context.
Instead, nm_platform_ip_route_sync() needs only do debug log about
failure to configure routes. It will now also return all the failed
routes to NML3Cfg, which can decide whether that is a problem.
This reworks the previous "temporary-not-available" mechanism to track
the state of the failed routes, to eventually decide whether there is an
actual problem (and log about it).
Another problem this solves is that since commit ('platform: always
reconfigure IP routes even if removed externally'), we will eagerly
re-try to configure the same route over and over. We cannot just spam
the log with warnings about the same failure on every commit. We need to
remember that we already logged about the problem and rate limit
warnings otherwise. This is what the new mechanism also achieves.
Indeed, all this is mostly for the sole benefit of logging better
warnings (and not duplicated).
NML3Cfg and NMNetns are already strongly related and cooperate.
An NML3Cfg instance is created via NMNetns, which is necessary
because NMNetns ensures that there is only one NML3Cfg instance per
ifindex and it won't ever make sense to have multiple NML3Cfg instances
per namespace.
Note that NMNetns tracks additional information for each NML3Cfg.
Previously, in a pointless attempt to separate code, it did so
by putting that information in another struct (L3CfgData).
But as the classes are strongly related, there really is no
reason why we cannot just attach this information to NML3Cfg
directly. Sure, we want that code has low coupling, high cohesion
but that doesn't mean we gain anything by putting data that is
strongly related to the NML3Cfg to another struct L3CfgData.
The advantage is we save some redundant data and an additional
L3CfgData. But the bigger reason is that with this change, it
will be possible to access the NMNetns specific data directly from
an NML3Cfg instance, without another dictionary lookup. Currently
such a lookup is never used, but it will be.
Basically, NML3Cfg and NMNetns shares some state. It is now in the
"internal_netns" field of the NML3Cfg instead of L3CfgData.
G_TYPE_CHECK_INSTANCE_CAST() can trigger a "-Wcast-align":
src/core/devices/nm-device-macvlan.c: In function 'parent_changed_notify':
/usr/include/glib-2.0/gobject/gtype.h:2421:42: error: cast increases required alignment of target type [-Werror=cast-align]
2421 | # define _G_TYPE_CIC(ip, gt, ct) ((ct*) ip)
| ^
/usr/include/glib-2.0/gobject/gtype.h:501:66: note: in expansion of macro '_G_TYPE_CIC'
501 | #define G_TYPE_CHECK_INSTANCE_CAST(instance, g_type, c_type) (_G_TYPE_CIC ((instance), (g_type), c_type))
| ^~~~~~~~~~~
src/core/devices/nm-device-macvlan.h:13:6: note: in expansion of macro 'G_TYPE_CHECK_INSTANCE_CAST'
13 | (G_TYPE_CHECK_INSTANCE_CAST((obj), NM_TYPE_DEVICE_MACVLAN, NMDeviceMacvlan))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
Avoid that by using _NM_G_TYPE_CHECK_INSTANCE_CAST().
This can only be done for our internal usages. The public headers
of libnm are not changed.
NetworkManager primarily manages interfaces in an independent fashion.
That means, whenever possible, we want to have a interface specific
view. In many cases, the underlying kernel API also supports that view.
For example, when configuring IP addresses or unicast routes, we do so
per interfaces and don't need a holistic view.
However, that is not always sufficient. For routing rules and certain
route types (blackhole, unreachable, etc), we need a system wide view
of all the objects in the network namespace.
Originally, NMPRulesManager was added to track routing rules. Then, it
was extended to also track certain route types, and the API was renamed to
NMPRouteManager.
This will also be used to track MPTCP addresses.
So rename again, to give it a general name that is suitable for what it
does. Still, the name is not great (suggestion welcome), but it should
cover the purpose of the API well enough. And it's the best I came
up with.
Rename.
ASSUME is causing more troubles than benefits it provides. This patch is
dropping NM_L3_CFG_COMMIT_TYPE_ASSUME and assume_config_once. NM3LCfg
will commit as if the sys-iface-state is MANAGED.
This patch is part of the effort to remove ASSUME from NetworkManager.
After ASSUME is dropped when starting NetworkManager it will take full
control of the interface, re-configuring it. The interface will be
managed from the start instead of assumed and then managed.
This will solve the situations where an interface is half-up and then a
restart happens. When NetworkManager is back it won't add the missing
addresses (which is what assume does) so the interface will fail during
the activation and will require a full activation.
https://bugzilla.redhat.com/show_bug.cgi?id=2050216https://bugzilla.redhat.com/show_bug.cgi?id=2077605https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1196
(cherry picked from commit bf5927b978)
l3cfg has a "temp_not_available" list of objects that couldn't be
added to platform, but can be added once some preconditions become
true (for example, a IPv6 route with a "src" attribute requires a
non-tentative src address to be present).
Retry to commit those objects once all addresses have completed
ACD/DAD.
(cherry picked from commit 9a090fdf7b)
Certain route types (blackhole, unreachable, prohibit) are not tied to
an interface. They are thus global and we need to track them system wide
(or better: per network namespace). That is done by NMPRouteManager.
For the routing rules, it's NMDevice itself to track/untrack the rules.
That is done for historical reasons, at the time, NML3Cfg did not exit.
Now with NML3Cfg, it seems that also NML3Cfg should be the part that
handles nodev routes. One reason is that we want to move IP
functionality out of NMDevice. So callers (NMDevice) would just add
blackhole routes to the NML3ConfigData and let NML3Cfg handle them.
Still, to handle these routes is rather different from regular routes.
Normally, NML3Cfg tracks an object state (ObjStateData) for each address/route,
and it hooks into platform signals to update the os_plobj field. Those signals
are dispatched by NMNetns and are only per-ifindex. Hence, NML3Cfg
wouldn't be notified about those nodev routes. Consequently, there
os_plobj could not be (efficiently) maintained and there is no
ObjStateData for such routes.
Instead, all that NML3Cfg does is have the routes in the NML3ConfigData and
tell NMPRouteManager about them. Seems simple enough. The only question
is when should NMPRouteManager sync? For now, we sync when the
track/untracking brings any changes and during reapply. Which is
probably fine.
(cherry picked from commit 9ab53e561a)
Now that higher priorities numbers really mean more important,
it seems that the VPN configuration should be rather important.
Bump the number, also in relation to NMDevice's L3ConfigDataType.
It might not matter too much, because usually the VPN tunnel device does
not have NDevice to add other l3cds and those from VPN might be alone.
Except, maybe with routing VPN (libreswan) that is different. Dunno.
NM_L3CFG_CONFIG_PRIORITY_IPV6LL and L3_CONFIG_DATA_TYPE_LL_6 are
somewhat related. They probably should be numerically identical.
Now, L3_CONFIG_DATA_TYPE_LL_6 cannot be zero (because
L3_CONFIG_DATA_TYPE_LL_4 is zero and L3ConfigDataType values must be
distinct). Therefore, also bump NM_L3CFG_CONFIG_PRIORITY_IPV6LL to 1.
Of course, NM_L3CFG_CONFIG_PRIORITY_IPV6LL is not obviously more important
than NM_L3CFG_CONFIG_PRIORITY_IPV4LL. But to be consistent with
L3_CONFIG_DATA_TYPE_LL_6 is probably more important here.
This commit does not seem correct. The enum was moved with the declared
intent to make manual IP configuration preferred. But the code comment
in L3ConfigDataType states that higher numbers are more important.
Also, all the other values are intentionally ordered so that more
important method have higher numbers. For example, LL_4 < DHCP_4 < SHARED_4 <
DEV_4 (in increasing priority). While it's often not clear whether to
prefer one method over the other, or what the actual effect of (LL_4 < DHCP_4)
is, the numbers were chosen intentionally.
Only moving MANUALIP first, counters the relative order of the other values.
If there is the problem, that the code comment is wrong (and lower
numbers mean more important), then we would have to reverse all
enum values.
The real problem is that NML3Cfg's _l3_config_datas_get_sorted_cmp()
has the wrong order/understanding. So the real fix is there and will
be done next. That is, unless we would agree that _l3_config_datas_get_sorted_cmp()
is in the right, and prefer lower numbers -- in which case all values had to be
reversed.
This reverts commit af1903fe3f.
This bumps L3_CONFIG_DATA_TYPE_MANUALIP to be the most important address
source; which is what had been the case before NetworkManager/next and
is presumably what the user expects.
It also comes into play for iBFT-booted machines, where iBFT contains a
permanent address (no lifetime data), while DHCP might lease out the
same one. In that case, expiry of the latter could potentially disrupt
connectivity to a vital storage volume.
Fixes: 14962cb414 ('merge: branch 'next''):
https://bugzilla.redhat.com/show_bug.cgi?id=2013921https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1011
We use clang-format for automatic formatting of our source files.
Since clang-format is actively maintained software, the actual
formatting depends on the used version of clang-format. That is
unfortunate and painful, but really unavoidable unless clang-format
would be strictly bug-compatible.
So the version that we must use is from the current Fedora release, which
is also tested by our gitlab-ci. Previously, we were using Fedora 34 with
clang-tools-extra-12.0.1-1.fc34.x86_64.
As Fedora 35 comes along, we need to update our formatting as Fedora 35
comes with version "13.0.0~rc1-1.fc35".
An alternative would be to freeze on version 12, but that has different
problems (like, it's cumbersome to rebuild clang 12 on Fedora 35 and it
would be cumbersome for our developers which are on Fedora 35 to use a
clang that they cannot easily install).
The (differently painful) solution is to reformat from time to time, as we
switch to a new Fedora (and thus clang) version.
Usually we would expect that such a reformatting brings minor changes.
But this time, the changes are huge. That is mentioned in the release
notes [1] as
Makes PointerAligment: Right working with AlignConsecutiveDeclarations. (Fixes https://llvm.org/PR27353)
[1] https://releases.llvm.org/13.0.0/tools/clang/docs/ReleaseNotes.html#clang-format
Problem: if l3cfg commits an address and routes from DHCP, when the
address expires those objects are removed automatically. NM tracks the
objects as missing as if the user removed them. This is to prevent
l3cfg to committing them again. If the lease if renewed, l3cfg should
be allowed to commit those objects again.
Introduce a l3cd flag to indicate that it should be force-committed
once, and propagate this flag to platform objects. In this way, l3cfg
can avoid committing again objects that are removed externally, but it
can commit them when the l3cd changes.
Fixes-test: @bridge_down_to_l2_only
Sticky update flag forces a commit at UPDATE level after unmanaging
a device. As a result, all the link local addresses will be removed.
To prevent the commit after unmanaging a device, clear sticky update
flag.
Signed-off-by: Wen Liang <liangwen12year@gmail.com>
Completely rework IP configuration in the daemon. Use NML3Cfg as layer 3
manager for the IP configuration of an interface. Use NML3ConfigData as
pieces of configuration that the various components collect and
configure. NMDevice is managing most of the IP configuration at a higher
level, that is, it starts DHCP and other IP methods. Rework the state
handling there.
This is a huge rework of how NetworkManager daemon handles IP
configuration. Some fallout is to be expected.
It appears the patch deletes many lines of code. That is not accurate, because
you also have to count the files `src/core/nm-l3*`, which were unused previously.
Co-authored-by: Beniamino Galvani <bgalvani@redhat.com>
We have "ipv[46].may-fail", which are per-address family. This works
together with nm_l3cfg_check_ready(), where we check whether an
NML3ConfigData is ready. We need to have that check also per-address
family.
- add "pre-commit" signal.
- fix assertion in nm_l3_config_data_get_ip6_privacy().
- set IPv6 privacy in _init_from_connection_ip() from profile.
- fix leaking "os_zombie_lst" in _obj_state_data_free().
- remove wrong assertion about VRF.
- fix _routes_temporary_not_available_update() to honor only the
requested object type. Otherwise, we always prune unrelated objects
too.
The idea was that NMIPConfig would register itself with the property (like "address-data")
and then NML3Cfg would emit the property changed notification.
However, we can already achive that via the regular notification, in particular
by listening to NM_L3_CONFIG_NOTIFY_TYPE_PLATFORM_CHANGE_ON_IDLE notification.
Also, NML3Cfg does not really understand the details when the property should
be emitted. For example, many routes not not exposed via "route-data" property,
and changes to those should not trigger a notification.
Drop the unused API.
We have nm_l3cfg_commit(), however that is synchronous and triggers an
avalanche of side effects. So it should be avoided if a component is
not aware of the current circumstances in which it gets called (most of them).
The alternative is nm_l3cfg_commit_on_idle_schedule(), but previously
that only supported the auto type.
Two changes:
- add a commit_type parameter to nm_l3cfg_commit_on_idle_schedule().
This allows to explicitly select a type for the next commit.
Previously, if the caller wanted for example to trigger a reapply
once, they had to register a handle, trigger the commit and unregister
the handle again. This basically allows to specify an ad-hoc commit
type that is only used once.
- if an explicit commit type is requested, then still always combine
it with auto. That means, we always use the "maximum" of what is
requested and what is registered.
NML3ConfigData is supposed to be immutable. It can be initialized from a
NMConnection, and its DNS priority property might be zero.
For the DNS priority, the value can be overwritten by global defaults.
We thus need to inject the default value at the right place.
This helper class is supposed to encapsulate most logic about
configuring IPv6 link local addresses and exposes a simpler API in order
to simplify NMDevice. Currently this logic is spread out in NMDevice.
Also, NML3IPv6LL directly uses NML3Cfg, thereby freeing NMDevice to care
about that too much.
For several reasons, NML3IPv6LL works different than NML3IPv4LL.
For one, with IPv6 we need to configure the address in kernel, which does
DAD for us. So, NML3IPv6LL will tell NML3Cfg to configure those
addresses that it wants to probe. For IPv4, it only tells NML3Cfg to do
ACD, without configuring anything yet. That is left to the caller.
It's a bit tricky how this flag works. It's needed for IPv6
link local addresses, which commits changes in %NM_L3_CFG_COMMIT_TYPE_ASSUME
mode. See the code comments how it works.
This commit only adds the flags and let's the NMPlatformIP{Address,Route}
properly track it. What is still needed is to actually implement any
meaning to that during the sync.
The "only_dirty" parameter to a remove-all() function is odd.
For one, the function is called remove-all, but depending on a parameter
it does not remove all.
Also, setting remove-all(only_dirty=TRUE) means it will remove not
everything, so passing TRUE will remove only parts. That logic seems
confusing.
Avoid that, by removing the parameter from nm_l3cfg_remove_config_all()
and add nm_l3cfg_remove_config_all_dirty().
