ATTN: parser: validate lacp-rate properly (LP: #1745648)#324
ATTN: parser: validate lacp-rate properly (LP: #1745648)#324slyon merged 1 commit intocanonical:mainfrom
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Thanks, this is a simple change and lgtm overall!
BUT: We need to take some special attention, as this is making stricter input validation on parsing the input YAML. So if somebody has a broken configuration currently this change will stop the generate binary to produce any network configuration at bootup, leaving such systems with broken networking on update. That's not good!
I think we have two options to handle this case:
- SAFE: Downgrade the
yaml_errorto just a warning, and keep ignoring invalid configuration as we did up to now. Then after some more releases, make it a hard requirement. - CLEAN: Make it a hard requirement for the next release (as proposed here). But clearly mark the commit (e.g. using an "ATTN:" prefix), so that when doing backports/SRUs to stable series we can make sure to downgrade the
yaml_errorto just a warning for stable series.
=> I think I slightly prefer the CLEAN option. WDYT?
1/ We can ignore the ERROR: Program 'pytest-3 pytest3' not found or not executable CodeQL failure, it's already fixed in the main branch
2/ I think we should not implement special handling for NetworkManager's 0/1 fallback. "slow"/"fast" is what our documentation specifies and also what systemd-networkd and NetworkManager expect by default.
3/ A small nitpick is inline, to improve the error message a bit, to provide a solution to the user right away.
slyon
changed the title
slyon
added
the
breaking
This field should accept only the values "fast" and "slow". It's documented in the Netplan documentation. It addresses LP: #1745648 Note for backports/SRUs: this change will make invalid values that were being accepted before to break netplan generate. While we want this fixed for future releases, this shouldn't be adopted by stable releases of Ubuntu. The recommended option for backports is to demote the yaml_error to a simple warning so the user will know that the configuration needs to be fixed.
daniloegea
force-pushed
the
fix_lp1745648
branch
from
115e6a8 to
d24c04a
Compare
This field should accept only the values "fast" and "slow". It's documented in the Netplan documentation. It addresses LP: #1745648 Note for backports/SRUs: this change will make invalid values that were being accepted before to break netplan generate. While we want this fixed for future releases, this shouldn't be adopted by stable releases of Ubuntu. The recommended option for backports is to demote the yaml_error to a simple warning so the user will know that the configuration needs to be fixed.
2 participants
This field should accept only the values "fast" and "slow". It's documented in the Netplan documentation.
It addresses LP: #1745648
Note: although we have it documented that the expected values are "fast" and "slow", NetworkManager also accepts 1 and 0, respectively, besides the keywords. There is a chance that someone out there is using NetworkManager as renderer and the values as numbers and it is just working fine. Should we handle the case and also accept 1 and 0? In this case, we would simply forward the value to NM and translate it to text before generating the configuration for networkd.
Checklist
make checksuccessfully.make check-coverage).