If previously the profile would track two addresses ("10.116.1.130/24",
"10.116.1.65/24"), and during an update the second address was removed
(leaving "10.116.1.130/24"), then the addresses of the profile were
wrongly not changed.
The effect is that removing a secondary IP address might not take
effect.
Fix that.
https://bugzilla.redhat.com/show_bug.cgi?id=1920838
Fixes: 69f048bf0c ('cloud-setup: add tool for automatic IP configuration in cloud')
We use the iface_idx for example to determine the routing table,
by using table 30400+iface_idx.
While the HTTP API for Azure has a index, it does not mean that we
should use that index as-is for our purpose.
Instead, treat those indexes separately and ensure that the
iface_idx that we return is numbering the interfaces starting
from zero.
While it's not clear whether we should be strict or forgiving
when fetching the HTTP meta data, we should be consistent.
On a parse error of the IP addresses we fail. Hence also
fail on a parse error for the subnet.
The API of mcs_provider_get_config() allows to explicitly request
for certain interfaces (MAC addresses), but it also allows to fetch
any.
That means, the result dictionary will be pre-populated with the
MAC addresses that were requested, but if we encounter an unknown
interface, then that is not a reason to fail.
Previously we would call
nmcs_utils_hwaddr_normalize(g_bytes_get_data(response, NULL), -1);
which treats the data in response as NUL terminated. That is not
entirely wrong, because the HTTP request's response is guaranteed
to have a NUL termination at the end. However, it doesn't seam to good
either.
For one, we already have the length. Use it. But also, if the response
contains any NUL bytes in the middle, then this would wrongly only
consider the first line. We should not accept "00:11:22:33:44:55\0bogus"
as valid.
While at it, reject NUL characters from nmcs_utils_hwaddr_normalize() --
except one NUL at the end.
First note that all three provider implementations are very similar.
That is why NMCSProvider's implementation does already some work that
is common to all implementations. For example, it provides the
NMCSProviderGetConfigTaskData structure to help tracking the data of
the request.
Also note that the GCP/Azure implementations didn't handle the
cancellation correctly. They always would pass
g_task_get_cancellable(get_config_data->task)
to the asynchronous requests. That is the GCancellable provider by the
caller. That is fine when there is only one async operation ongoing. But
that is not the case, we have parallel HTTP requests.
Then, when an error happened, the overall get_config() operations fails
and the still pending requests should all be aborted. However, we must
not cancel the GCancellable of the user (because that is not owned by us).
The correct solution is to use an internal cancellable in those cases.
Anyway. Since all of this is similar, we can extend the base class
to handle things for us. This also gets the cancellation right by having
a "get_config_data->intern_cancellable".
The code is not entirely straight forward. Consistent naming
is hence important.
In "nmcs-provider-ec2.c", variables of this kind are called
"get_config_data". That also matches to the type of the data
(NMCSProviderGetConfigTaskData).
Rename the variables to make naming consistent. Also, I find the
longer name to be clearer.
Currently libnm headers include <linux/if_{ether,infiniband,vlan}.h>.
These are public headers, that means we drag in the linux header to all
users of <NetworkManager.h>.
Often the linux headers work badly together with certain headers from libc.
Depending on the libc version, you have to order linux headers in the right
order with respect to libc headers.
We should do better about libnm headers. As a first step, assume that
the linux headers don't get included by libnm, and explicitly include
them where they are needed.
If we call g_cancellable_connect() on a GCancellable that is already
cancelled, then the callback is invoked synchronously. We need to
handle that.
However, we can slightly simplify the code. There is no change in
behavior, but we can always let the cancelled callback return the
result.
"iface_data->cancellable" is an internal cancellable for the parallel
HTTP requests. Once we encounter a failure, those requests are all
obsolete and must be cancelled.
There are two GCancellable at work: one is provided by the user
during nmcs_provider_get_config(), and one is used internally for the
individual HTTP GET requests.
In _get_config_fetch_done_cb(), if the error reason is "cancelled",
then it means that our internal iface_data->cancellable was cancelled.
Probably because an error happend (like a timeout or the user cancelled
the external GCancellable).
In that case, we must not report that the task completed with a
cancellation, because we need to preserve the error that was the
original cause.
Run:
./contrib/scripts/nm-code-format.sh -i
./contrib/scripts/nm-code-format.sh -i
Yes, it needs to run twice because the first run doesn't yet produce the
final result.
Signed-off-by: Antonio Cardace <acardace@redhat.com>
nm_utils_hexstr2bin_full() is our general hexstr to binary parsing
method. It uses (either mandatory or optional) delimiters. Before,
if delimiters are in use, it would accept individual hexdigits.
E.g. "a:b" would be accepted as "0a:0b:.
Add an argument that prevents accepting such single digits.
Seems with LTO the compiler can sometimes think that thes variables are
uninitialized. Usually those code paths are only after an assertion was
hit (g_return*()), but we still need to workaround the warning.
Make the error handling similar to the other provider implementations.
- only actually return once all callbacks completed.
- cache the first error and report it.
- drop AzureData.success field. It is redundant to have AzureData.error set.
Also it was actually unused.
- ensure that we keep the first error passed during
_get_config_maybe_task_return(). Once we set an error, that error gets
returned. There is a twist here, that we prefer cancellation errors
over other error reasons.
- drop GCPData.success field. It is redundant to have GCPData.error set.
Also, it's meaningless to indicate failure, if we don't have an error
at hand.
- ensure that we keep the first error passed during
_get_config_maybe_task_return(). Once we set an error, that error gets
returned. There is a twist here, that we prefer cancellation errors
over other error reasons.
- in _get_config_fip_cb(), ensure to call _get_config_maybe_task_return()
even if we are not yet ready. That is useful to record a potential
error.
If the list of addresses, routes and rules is empty, we still want to mangle
the applied connection, to also have an empty list.
nm-cloud-setup has certain expectations. For example, that the static addresses,
routes and rules of the active connection is entirely under the control of the
tool. For example, so it usually replaces the lists entirely. It also should do
that, if the new list is empty.
Maybe, one day there could be more complex merging strategies, where the user could
also add static addresses, routes, or rules to the profile, and nm-cloud-setup
would preserve them. However, that is not implemented, nor is it clear how exactly
that would work.
nm_http_client_poll_get_finish() can only either succeed (returning TRUE
and setting no GError), or failing (returning FALSE and setting GError).
Checking for both is redundant and unnecessary.
Since commit 3bd30f6064 ('nmcs: add error message when a HTTP request times
out'), the case where polling returns %FALSE without an error is no
longer possible. This is preferable, because it follows a consistent
API where a function clearly fails or succeeds.
So, checking for the error code and the returned boolean is redundant and
unnecessary.
nm_utils_parse_next_line() operates on the response buffer obtained
from NMHttpClient. We own this buffer, and we also can rely on the fact
that the buffer has a trailing NUL byte after the data.
There is no need to copy the string to a GString, just use it directly.
nm_utils_parse_next_line() operates on the response buffer obtained
from NMHttpClient. We own this buffer, and we also can rely on the fact
that the buffer has a trailing NUL byte after the data.
There is no need to clone the string to a GString, just use it directly.
