Introduce support for networkd address options#89
Conversation
Codecov Report
@@ Coverage Diff @@
## master #89 +/- ##
======================================
Coverage 100% 100%
======================================
Files 34 36 +2
Lines 4066 4912 +846
======================================
+ Hits 4066 4912 +846
Continue to review full report at Codecov.
|
|
From a design perspective, what is the rationale for supporting either of these in netplan? 'Label' would appear to only be useful for tools which are managing networkd state directly and bypassing netplan. Do you have a practical example of this, and of why you would want the addresses to be initially configured via netplan but then reconfigured via networkd? PreferredLifetime takes only two values: "forever", and "0", where 0 means the address will not be used by default and only used if an application explicitly requests it. That's an interesting idea, can you speak to the practical need for it? In terms of the schema, I'm not in love with the idea of having addresses as both scalars and mappings, but I don't immediately see a cleaner way to do this. |
One of the main points in favor of supporting these in netplan is to have
Setting the lifetime to "0" allows one to have multiple IP addresses where each I've added the label key with this kind of scenario in mind, as it ties in
I agree that mixing types is not the cleanest solution, but I'm also having a |
|
It has been a while since any activity on this PR, so I've updated it to be aligned with the current master branch. Is there anything else that I can do to help with the review? I would be happy to implement any changes or suggestions if there are any problems with the current schema changes. I think the |
|
I for one could use something like this. I have a need for multiple incoming addresses, but wish to control which single outbound address is selected. Particularly since some addresses I have network controls in place. |
Codecov Report
@@ Coverage Diff @@
## master #89 +/- ##
===========================================
- Coverage 100.00% 99.74% -0.26%
===========================================
Files 41 41
Lines 5082 5157 +75
===========================================
+ Hits 5082 5144 +62
- Misses 0 13 +13
Continue to review full report at Codecov.
|
|
This would also resolve LP #1743200 (https://bugs.launchpad.net/ubuntu/+source/nplan/+bug/1743200) Did you do any research of how this could be implemented for the NetworkManager backend? Ideally, we should implement this feature for both backends equally. If it's not possible, we should at least throw an error from |
|
@slyon As far as I've seen from the nm-settings documentation [0], it doesn't I do agree that we should try to handle this gracefully in both backends |
|
Yes, I couldn't find anything related to lifetimes/labels in NM documentation either. So it would be fine if you implement a simple safeguard in |
There was a problem hiding this comment.
Thank you @hrasiq for your continued effort of getting this PR ready for merging.
Your work is very much appreciated!
The safeguard you implemented for the NetworkManager backend is looking good and (unfortunately) seems the best we can do at the moment, as NM does not support this feature.
So here comes your full technical code review!
I like the overall design and structure of your PR, but requested some smaller cosmetic or technical improvements inline. Especially the renaming of addr_options -> NetplanAddressOptions, as this is now exported via libnetplan.
The only bigger issue I could spot is that the address options data might get appended multiple times to the cur_netdef->address_options array in handle_addresses. This is a similar problem to what I fixed here recently: cc013bd Please have a look there for reference and lets discuss the solution here, if need be.
I'll try to get the other team members to sign-off on the schema changes for this PR!
Thanks!
Lukas
| if (!process_mapping(doc, value, address_option_handlers, error)) | ||
| return FALSE; | ||
|
|
||
| g_array_append_val(cur_netdef->address_options, cur_addr_option); |
There was a problem hiding this comment.
This is a bit tricky... But you should make sure not to add the same options data multiple times into the array. This can happen as this function might be called multiple times, if the YAML parser needs multiple passes to resolve all netplan interfaces.
I implemented this similarly in this commit recently: cc013bd
Please have a look there for reference and let's discuss in this PR how to best solve it for the NetplanAddressOptions data.
There was a problem hiding this comment.
I've thought a bit about this, and wanted to get more input before "tainting" this PR with more complex code (maybe I'll push a separate commit on top of this one?).
One "easy" way to do this would be to check if the first element on the cur_netdef->address_options array is equal to the struct we're currently pushing there (meaning we're past the first YAML parsing round). This would probably need an additional comparison helper, and we would also need to take into account addresses that are set without specifying any of the options (these would skip this code block, and would need to be handled further down in their IPv4/IPv6 sections).
How is this being handled currently for addresses without any options? How viable would it be to try and detect when the parser needs multiple passes, so that we can just verify if the addresses were set correctly and completely skip the helpers here?
There was a problem hiding this comment.
Pushing a separate commit on top is a good idea. Keep it in small logical chunks/commits. We will squash all commits from this PR into a single "merge commit" anyways, once it is accepted into master.
It might be even easier to compare the length cur_netdef->address_options->len. It will be 0 on first pass, while it will be equal to the length of the YAML sequence in following passes. This is what I did in the other commit (cc013bd):
/* Avoid adding the same peers in a 2nd parsing pass by comparing
the array size to the YAML sequence size. Skip if they are equal. */
guint item_count = node->data.sequence.items.top - node->data.sequence.items.start;
if (cur_netdef->wireguard_peers->len == item_count) {
g_debug("%s: all wireguard peers have already been added", cur_netdef->id);
return TRUE;
}
For IP4/IP6 addresses this is currently handled by comparing all IP address strings of the array, which might be a bit overkill... as each parsing pass will contain the very same IP address sequence. And in fact this fix did not yet make it to the master branch, but can be seen here: 2f2971b (It was introduced as part of a OVS pull request which are currently still tracked in the openvswitch-support branch.
There was a problem hiding this comment.
I'm not sure the length comparison will work out in this case. If we take the
example below, for instance:
network:
version: 2
ethernets:
engreen:
addresses:
- 192.168.14.2/24:
label: out
lifetime: 0
- 192.168.14.3/24
- 2001:FFfe::1/64:
label: ipv6For this example, node->data.sequence.items.top - node->data.sequence.items.start will result in 3 (two IPv4 keys and one IPv6 key), but we'll never reach that amount in cur_netdef->address_options as only two of the addresses set options. And I might be missing something here, but I don't think we can know how many addresses set options in the middle of parsing. This seems to be an issue for the IPv4/IPv6 addresses as well, right? (i.e. we can't know how many of the keys will be IPv4 and how many will be IPv6 until we're done parsing all of them).
I've added a draft of how we could do this for address options in a separate commit, but it's not a very elegant solution imho (it also breaks check-coverage atm :). If we have no way of knowing that we've done a first pass already, I don't think we'll be able to implement something much different from that, unfortunately. What do you think? Any other ideas?
There was a problem hiding this comment.
You are absolutely correct! Length comparison won't work out here, due to having MAPPING and SCALAR nodes handled differently.
You draft is looking pretty good already.
Well, I guess we could introduce a new global variable, which increments with each new parsing pass, to know number of the current pass. But this would be very abstract and might confuse people... I think I'd like to keep the multi-pass handling local to this function and array, so it can be easily understood.
Allocating a new cur_addr_option, processing all it's mappings, comparing each field inside via addr_options_equals and free'ing it up afterwards seems like lots of overhead, though. I think we should move the check prior to allocating new memory for cur_addr_option and just compare the opts->address VS scalar(key), which we already have at that point. This way we know if there are any options set for this address and can skip it's handling completely.
I prepared a test-case, adding a bridge to trigger multi-pass parsing. Also, I drafted my thoughts from above in this commit: slyon@1734de6
Feel free to cherry-pick and/or adopt.
vorlonofportland
left a comment
vorlonofportland
left a comment
There was a problem hiding this comment.
General +1 on the schema change. Some comments inline.
|
@slyon @vorlonofportland Thanks a lot for the reviews! I've pushed a new version, fixing most of your comments. I left minor comments on some of the previous reviews, but please let me know if I've missed anything! |
hrasiq
force-pushed
the
master
branch
2 times, most recently
from
4f7974c to
bafa8ce
Compare
slyon
left a comment
slyon
left a comment
There was a problem hiding this comment.
@hrasiq Thanks, this PR is shaping up nicely!
I've added two tiny inline formatting suggestions. And drafted an improvement for your multi-pass handling here:
slyon@1734de6
Let me know what you think!
| if (!process_mapping(doc, value, address_option_handlers, error)) | ||
| return FALSE; | ||
|
|
||
| g_array_append_val(cur_netdef->address_options, cur_addr_option); |
There was a problem hiding this comment.
You are absolutely correct! Length comparison won't work out here, due to having MAPPING and SCALAR nodes handled differently.
You draft is looking pretty good already.
Well, I guess we could introduce a new global variable, which increments with each new parsing pass, to know number of the current pass. But this would be very abstract and might confuse people... I think I'd like to keep the multi-pass handling local to this function and array, so it can be easily understood.
Allocating a new cur_addr_option, processing all it's mappings, comparing each field inside via addr_options_equals and free'ing it up afterwards seems like lots of overhead, though. I think we should move the check prior to allocating new memory for cur_addr_option and just compare the opts->address VS scalar(key), which we already have at that point. This way we know if there are any options set for this address and can skip it's handling completely.
I prepared a test-case, adding a bridge to trigger multi-pass parsing. Also, I drafted my thoughts from above in this commit: slyon@1734de6
Feel free to cherry-pick and/or adopt.
This patch introduces a way to set options for the "[Address]" section in the networkd backend. It changes the addresses YAML key to a sequence of scalars and mappings, keeping backward compatibility with the previous address notation. Signed-off-by: Heitor Alves de Siqueira <[email protected]>
|
@slyon Thanks for the feedback! You're correct, doing it before allocating and I did a minor tweak to your draft I would like to get your opinion on: instead |
slyon
left a comment
slyon
left a comment
There was a problem hiding this comment.
Thank you @hrasiq !
I cannot think of any case where the IP addresses (or their options) would not be parsed in the same pass and all our test cases pass with your shortcut applied. So I think we should be fine to keep the optimization.
I do not have any other concerns regarding your PR.
LGTM!
I will check back with the team and will probably merge it soon. Thank you very much for your work, it is highly appreciated!
|
Passes all integration- and unit-tests. |
6 participants
Description
This patch introduces a way to set options for the [Address] section in the networkd backend. It changes the addresses YAML key to a sequence of scalars and mappings, keeping backward compatibility with the previous address notation.
We can extend the current address notation either flattened or expanded, e.g. below:
Due to the nature of the networkd backend, addresses which have these options set need to be placed in their separate [Address] section in the resulting
.networkfile.Two keys are supported for now:
lifetimeandlabel, which correspond to their networkd counterpartsPreferredLifetimeandLabel. I've tried to write this in a way that makes it easy to add other keys in the future, but I'm open to other different approaches in changing the code.Checklist
make checksuccessfully.make check-coverage).