search

containernetworking / plugins

Some reference and example networking plugins, maintained by the CNI team.
Apache License 2.0
2.17k stars 778 forks source link

Add "activateInterface" option to bridge plugin CNI #951

Open AlonaKaplan opened 11 months ago

AlonaKaplan commented 11 months ago

Add "activateInterface" option to bridge plugin CNI. The option will control whether the defined interface will be brought up by the plugin. The default will be true. This is required in case some other CNI in the plugin chain wants to modify the interface before it is brought up and made visible to the network.

For example, in case I want to prevent my interface to send any IPv6 traffic. I can use a another CNI to achieve this by setting net.ipv6.conf.all.disable_ipv6 to 1. But since the bridge CNI will first activate the interface, it may send IPv6 data (NDP) on activation, before the next CNI disables it. Having activateInterface=false will solve this issue. The bridge CNI will create and configure the interface and the next CNI in the chain will apply the sysctl and activate the interface,

maiqueb commented 11 months ago

Without going into if the ask makes sense or not, I'd say the flag should be the opposite - i.e. by default have the interface enabled, thus having the existing current behavior as "the" default.

AlonaKaplan commented 11 months ago

Without going into if the ask makes sense or not, I'd say the flag should be the opposite - i.e. by default have the interface enabled, thus having the existing current behavior as "the" default.

The ticket says the default will be true.

maiqueb commented 11 months ago

Without going into if the ask makes sense or not, I'd say the flag should be the opposite - i.e. by default have the interface enabled, thus having the existing current behavior as "the" default.

The ticket says the default will be true.

Oh right !

FWIW, you don't say what's the name of the attribute, and thus: "The option will control whether the defined interface will be brought up by the plugin. The default will be true." can pretty much mean anything - i.e. a disableInterface knob controls whether the defined interface will be brought up by the plugin, and if it defaults to true, you'd get a disabled interface.

I'm happy we share the default should be the "current behavior", but I still think the issue could be a bit more explicit; Near the end of the description you do propose a name for the knob; could you maybe introduce it it the beginning ? IMO that's more explicit / less confusing.