Page MenuHomeVyOS Platform

change the default for Interface creation to False
Open, Requires assessmentPublic

Description

It would be safer if the default for interface creation was False and interface call updated.

Details

Difficulty level
Unknown (require assessment)
Version
-
Why the issue appeared?
Will be filled on close
Is it a breaking change?
Unspecified (possibly destroys the router)

Event Timeline

thomas-mangin created this object in space S1 VyOS Public.
thomas-mangin updated the task description. (Show Details)
jjakob added a comment.EditedApr 23 2020, 1:09 PM

Actually, if an interface doesn't exist and if we try to get its properties (for example if mac != BridgeIf('br0', create=False).get_mac():) that raises an exception. I'm not sure what the best way to do here is. Use create=True anyway? This is fine if used in conf_mode scripts that'll create it after that anyway, but what if we're checking another interface that we don't know if it exists or not? First do an existence check? How do we do that?

Edit: if we create an interface that's disabled, the conf_mode script will first create it (physically) and then delete it immediately after that, I'm not sure that's the best behavior - if we use create=True for checking values. So either create=False should not return an exception if the interface doesn't exist, but return empty values for all get_ commands, or we need to check if the interface exists before any invocation of its *If class with create=False.

is if iface in Section.interfaces() the correct way to check if the interface already exists?

runar added a subscriber: runar.Apr 23 2020, 3:39 PM

"This is fine if used in conf_mode scripts that'll create it after that anyway" if the intention of the code was not to create the interdace this is not fine if you ask me. :)

In T2366#61424, @runar wrote:

"This is fine if used in conf_mode scripts that'll create it after that anyway" if the intention of the code was not to create the interdace this is not fine if you ask me. :)

Yeah, if the interface is disabled it would create and immediately delete it, so it's not suitable there either.

So every call to an interface's methods should be preceded by a check if it exists, like this (example where we know the interface's section and type):
if iface in Section.interfaces(section='bridge') and mac != BridgeIf('br0', create=False).get_mac(): ...

or if we don't know the interface's section or type:
if iface in Section.interfaces() and mac != Interface('br0', create=False).get_mac(): ...

If we don't first check if the interface is in Section.interfaces, the second call will throw an exception.

Or use try/except, which I guess is more pythonic, but requires more lines and can't be put into a one-line expression.

@jjakob yes, what you propose to check if an interface exists is good. If you know the type (as defined in the class which as the same name as the "set interface" section such as ethernet) you can use Section.interfaces('ethernet') to only get what you want.

thomas-mangin added a comment.EditedApr 23 2020, 8:24 PM

@jjakob There is no need to check if an interface exists before creation, the code has always tried to find the interface and use it if there, otherwise it will create it.

Nothing has changed in the way Interface code is working. The logic is still the same. For example in apply(), when the interface is deleted, if the interface is not there the code will create and delete the newly created interface (which is less likely to raise support query :-p).

It is just that the code is now also used by op_mode scripts and there is some cases we do not want to create things by accident there! For example when a user completes commands for "show" and mistype the name .. You do not want to create the interface in that case - I had to fix such a bug. That's the real use case for create=False. debug=False is important to make sure op command do not print weird stuff if something goes wrong. IF.

I was considering having the behaviour safe by default but NOT creating interface unless requested. This task was really a note to self to document what would be best and explain why a change should it be required. ATM, I am not sure what would be best to do yet. The jury is still out :-)

jjakob added a comment.EditedMay 9 2020, 9:56 AM

I again ran into a situation where it would be nice if create=False were the default, and if we could use the Interface class without creating the physical interface. The bridge member port configuration defaults were previously stored as a dict in interfaces-bridge.py, but as I added a method Interface.add_to_bridge() that all interfaces can call on themselves, I couldn't place the default port config into BridgeIf: when creating a bridge for the first time, the physical interface doesn't exist, but we need to get the member port default config in interfaces-bridge.py get_config() without creating the physical bridge interface (this should only happen in apply()). Thus I had to put it in util.get_bridge_member_config(). If I could call BridgeIf.get_member_config() when the physical interface doesn't exist without creating the physical interface, thet code could be moved from vyos.util into BridgeIf.

The same for util.ifname_from_config() - it could be moved to Interface if the above were realised.

I will look into the use case and see if I can think of something.