Permit multiple patterns for the driver globs in match (LP: #1918421)

[waveform80]

Description

The networkd backend, which is the only backend currently supporting driver matches, permits multiple whitespace separated globs. Netplan will happily generate a .link file containing the full list of driver globs (which will subsequently work with networkctl) but fails to match the driver to the interface itself, meaning the initial apply during boot fails. This is observed with the current Raspberry Pi focal images.

Enhance the handling of match.driver to accept a single driver glob (as before) or a sequence of driver globs (new feature).

Example:

            match:
              driver: ["bcmgenet", "smsc*"]
              name: en*

LP: #1918421

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.

[codecov-io]

Codecov Report

Merging #202 (85ea965) into master (2f7d973) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #202   +/-   ##
=======================================
  Coverage   99.00%   99.00%           
=======================================
  Files          54       54           
  Lines        8316     8322    +6     
=======================================
+ Hits         8233     8239    +6     
  Misses         83       83           

Impacted Files	Coverage Δ
netplan/cli/utils.py	100.00% <100.00%> (ø)
tests/test_utils.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 2f7d973...85ea965. Read the comment docs.

[slyon]

Thank you very much @waveform80 for working on this!
Well, this is a though one... I think providing a whitespace separated list was never meant to be supported (at least it is not documented anywhere or part of any test case). But apparently it has been working (and been used; although not widely) in prior versions.
So it seems to be a regression at the same time as being "unsupported".

At very least I would not want to expose this whitespace separated list publicly in the documentation/examples, but maybe keep it as a hidden, unsupported but still working "workaround". As it does not fit the YAML schema.

I wonder, though, if we should rather adopt/fix parse.c to:

• Throw an yaml_error() in handle_generic_str() if a whitespace separated list is detected
• Change match_handlers[]->driver to YAML_NO_NODE
• Add a new handle_match_driver() function, similar to handle_tunnel_key_mapping() accepting YAML_SCALAR_NODE & YAML_SEQUENCE_NODE

This new function could then re-combine the sequence/list to a whitespace separated string for internal data storage (parse.h: match->driver) for now, to keep things simple and considering that it is the format of the only currently supported backend (networkd) for this feature.

Let me check back with the team in the next Netplan sync, to see how we want to move forward here.

[waveform80]

Sounds good - happy to handle that, if that's decided it's the right way to proceed.

[slyon]

Hey @waveform80 !

After some discussion we found the following consensus: The behavior is currently unspecified and neither a whitespace separated list nor a sequence/list OR scalar/string field feels correct with regard to the YAML schema.

What we want to support going forward is a (pseudo-) RegEx like syntax, which would be a normal string/scalar supporting simple shell-style glob matching (like * or ?) in addition to the OR operator (|). We want to support this pseudo regex syntax for driver and name matching.

Example:

match:
  driver: bcmgenet|smsc*
  name: en*

What do you think, does this make sense to you?
IMO in a first step we should update match_handlers->driver in parse.c to make use of a new handle_match_driver() function (instead of handle_netdef_str()), splitting the given string at | and storing it as a space separated string (consumable by systemd-networkd) for internal usage. Also, we would update utils.py – as you did – but splitting at | instead of space.
This "pseudo regex shell-style glob matching" should be specified appropiately in the documentation.

In a next step, we could extend the driver matching for the NetworkManager backend and consider applying the same logic to the match_handlers->name field, while potentially refactoring the internal data storage of the matching fields (GArray instead of string). But that should most probably be a separate task.

[waveform80]

After some discussion we found the following consensus: The behavior is currently unspecified and neither a whitespace separated list nor a sequence/list OR scalar/string field feels correct with regard to the YAML schema.

I'd certainly agree that the whitespace separated list isn't in keeping with the YAML schema (or the YAML language more generally). I'm slightly surprised the sequence/list-or-scalar/string field isn't. Whilst a pure sequence/list is obviously the "right" thing, it would be a backwards incompatible breaking change, which seems sufficient justification to me to permit the alternate singular case.

What we want to support going forward is a (pseudo-) RegEx like syntax, which would be a normal string/scalar supporting simple shell-style glob matching (like * or ?) in addition to the OR operator (|). We want to support this pseudo regex syntax for driver and name matching.

Example:

match:
  driver: bcmgenet|smsc*
  name: en*

What do you think, does this make sense to you?

Erm, not really. Obviously, it's not my place to dictate things, so please feel free to ignore the following, but here are my thoughts on this design:

Semantically it doesn't solve anything. The original objection to the space separated string points out (entirely correctly) this isn't in keeping with YAML. But neither is this. In fact, at its simplest this is just another "character-separated string" scheme that just happens to use "|" instead of " " as its separator.

Now let's look at it from the user's point of view. Imperfect as the space separated string scheme was, it was at least consistent with:

• networkd's format (from which it derives, and which some users might well be familiar with given the subject matter)
• shell usage where globbed patterns are typically space separated

I'm not attempting to argue that the above means the space separated string design is good or even acceptable, but now let's compare the bar-separated string design:

• inconsistent with shell usage, despite containing globs
• consistent with (or at least "looks like") regular expression usage ... but isn't a regular expression and would be wrongly interpreted as such (given the different meanings of the "*" and "?" glob operators to their regex counterparts)

For me, the bar-separated design fails the "obviousness" test which I generally consider rather important with configuration files. It's not obvious to write (users familiar with YAML won't expect it, users familiar with networkd won't expect it, and users generally familiar with globbing won't expect it), and although it might be semi-obvious to read, the bars the confuse the meaning of any globbing operators that appear in the components.

If it were up to me, I would reject my space separated string PR for the hack that it is, and go for the sequence-or-string option in the YAML (treating the string option as a special case of a one-item list). That seems to me to be in keeping with YAML's format (mostly) and, while different to networkd's underlying format, it should be reasonably obviousi to users given that the YAML list syntax is used plenty of other places in the schema, and trivial to transform to/from networkd's space separated syntax. It also doesn't confuse the operation of the globbing characters, or suggest a regular expression facility that doesn't exist.

Anyway, that's my thoughts -- but obviously I'm happy to accept whatever the netplan team decide is best.

[slyon]

Thank you very much for your constructive feedback!

Now let's look at it from the user's point of view. Imperfect as the space separated string scheme was, it was at least consistent with:

• networkd's format (from which it derives, and which some users might well be familiar with given the subject matter)
• shell usage where globbed patterns are typically space separated

True! The previous space-separated style was consistent with networkd and shell globbing (although not really specified). But we also have users expecting a more RegEx style format: https://askubuntu.com/questions/1326021/netplan-match-physical-ifs-only
Again, the root cause here is the missing specification & documentation.

I'm not attempting to argue that the above means the space separated string design is good or even acceptable, but now let's compare the bar-separated string design:

• inconsistent with shell usage, despite containing globs
• consistent with (or at least "looks like") regular expression usage ... but isn't a regular expression and would be wrongly interpreted as such (given the different meanings of the "*" and "?" glob operators to their regex counterparts)

You're making some very strong points here; and I agree. It isn't a regular expression and we should make it look like one.

If it were up to me, I would reject my space separated string PR for the hack that it is, and go for the sequence-or-string option in the YAML (treating the string option as a special case of a one-item list). That seems to me to be in keeping with YAML's format (mostly) and, while different to networkd's underlying format, it should be reasonably obvious to users given that the YAML list syntax is used plenty of other places in the schema, and trivial to transform to/from networkd's space separated syntax. It also doesn't confuse the operation of the globbing characters, or suggest a regular expression facility that doesn't exist.

Yeah, you swayed me and I'm back to my original idea of supporting the sequence-or-string format, where a sequence would be the officially documented style and (single word, single match) string would be a fallback option, to stay backwards compatible with existing configs, printing a deprecation warning when used.

Still, i'd like to get a 2nd opinion (or ACK) from @vorlonofportland here.

[slyon]

Sorry for taking a long time to find a definite answer to this design question!

Yeah, you swayed me and I'm back to my original idea of supporting the sequence-or-string format, where a sequence would be the officially documented style and (single word, single match) string would be a fallback option, to stay backwards compatible with existing configs, printing a deprecation warning when used.

After some more discussion with the netplan team, we're now all on the same page about implementing the "sequence-or-string" format, so that a single driver can be matched by passing a single, simple glob string (scalar) and multiple drivers can be matched by passing a list of glob strings (sequence). E.g.:

match:
  driver: [ "bcmgenet", "smsc*" ]
  name: en*

Sounds good - happy to handle that, if that's decided it's the right way to proceed.

@waveform80 would you still be interested to get that implemented?

[waveform80]

@waveform80 would you still be interested to get that implemented?

Yup, leave it with me and I'll see what I can do! Not sure whether you want a separate issue for this, or if it's preferable to just re-write the commits for this one, but if you want to close this and create a new issue covering it, feel free to stick my face on it!

[slyon]

Yup, leave it with me and I'll see what I can do! Not sure whether you want a separate issue for this, or if it's preferable to just re-write the commits for this one, but if you want to close this and create a new issue covering it, feel free to stick my face on it!

Thanks! Re-writing the commits for this one is fine, so we can keep the discussions and resolutions to a common place.

[codecov-commenter]

Codecov Report

Merging #202 (918a336) into main (331ca01) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #202   +/-   ##
=======================================
  Coverage   99.13%   99.14%           
=======================================
  Files          60       60           
  Lines       10432    10490   +58     
=======================================
+ Hits        10342    10400   +58     
  Misses         90       90           

Impacted Files	Coverage Δ
netplan/cli/utils.py	100.00% <100.00%> (ø)
src/netplan.c	100.00% <100.00%> (ø)
src/networkd.c	100.00% <100.00%> (ø)
src/parse.c	99.82% <100.00%> (+<0.01%)	⬆️
tests/generator/test_ethernets.py	100.00% <100.00%> (ø)
tests/test_utils.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 331ca01...918a336. Read the comment docs.

[slyon]

@waveform80 Hey Dave! I took this one over, as I'd like to get it included in the pending 0.104 release. Let me know what you think!

[schopin-pro]

Before I actually review the code, glancing at the commit list I'm guessing the PR description is out of date and we went to a list of patterns instead of using whitespace as a pattern separator?

[slyon]

Before I actually review the code, glancing at the commit list I'm guessing the PR description is out of date and we went to a list of patterns instead of using whitespace as a pattern separator?

Indeed, I forgot to update the PR description. Did that now.

[schopin-pro]

Overall, LGTM.

However, aside from nitpicks, I have a small question regarding the choice of separator in the C field.

[schopin-pro]

The nice thing here is that the space-separated list can be used as is in the generated systemd-networkd output.. But I can certainly re-work it to store it in a more explicit `\t`-separated string and convert back in `networkd.c`.

I have a slight preference for internal consistency vs ease of conversion to external formats, but I'm fine if you want it to remain as such.

[slyon]

Thanks for your comments @schopin-pro
I rebased on top of origin/main and added some additional commits to resolve your remarks.
I think I addressed everything and feel like this is ready for merging.

Feel free to (squash) merge, once you're +1 on this!

[schopin-pro]

autopkgtest [11:45:46]: @@@@@@@@@@@@@@@@@@@@ summary
ovs                  PASS
ethernets            PASS
bridges              PASS
bonds                PASS
routing              PASS
vlans                PASS
wifi                 PASS
tunnels              PASS
scenarios            PASS
regressions          PASS
autostart            PASS
cloud-init           PASS

Merging :)

[schopin-pro]

So, funny story: this is actually buggy. I don't exactly understand why it works, but the YAML_NO_NODE tag means that the union is supposed to contain a .variable callback, which has a different parameter count.

Honestly, it's my bad. Manually tagged unions are a footgun, I should have stuck to plain structs -_-'

{"driver", YAML_NO_NODE, {.variable=handle_match_driver}},

That would introduce a compilation error since the signature has changed

[slyon]

Thanks for providing a follow up in #257