network: do not automatically add default* networks when custom ones are specified - #3685
#3685network: do not automatically add default* networks when custom ones are specified#3685
default* networks when custom ones are specified#3685Conversation
|
Can one of the admins verify this patch? |
7259af2 to
c6b803b
Compare
|
ping @squeed, I was told you are usually interested on CNI-related changes |
| return nil, err | ||
| } | ||
| netSlice := make([]activeNet, 0, len(nets)) | ||
| for _, net := range nets { |
There was a problem hiding this comment.
Iterating over this map may discard the original order. This is a behavioral change compared to existing code and I don't think this is really what you want (even though you probably won't notice this often at runtime). Should we just directly receive an array here without post-processing?
There was a problem hiding this comment.
ah, you are right!
The logic here can become very complex. We either use the order defined by .conf file names on disk, with some other logic to merge contents from /etc/rkt/net.d and what's built into stage1, or we use the order from the command line, but it is unclear what to do for --net=all.
Thoughts?
|
I went through a bit of code and tickets history, and I now tend to agree with @fabiokung opinion that the current logic is more like an unwanted artifact of several iterations (especially #1216 (comment)). I'm fine with taking this PR to change the current behavior, and I left some comments inline. I still think this is a breaking change though which should be called out in the release notes. |
default* networks when custom ones are specified
default* networks when custom ones are specifieddefault* networks when custom ones are specified
c6b803b to
eb8db86
Compare
|
@lucab I added some bits to preserve ordering from filenames. This should look more like the previous behavior. Adding tests for this is non-trivial ATM ( |
**BREAKING** Let builtin network configurations (e.g.: default and default-restricted) in the default stage1 image be overriden by custom images. This allows users to specify --net=custom1,default and have the default network come from builtin configurations. Users can also provide a custom network named default to override what's builtin, as needed. Fixes rkt#3598 and rkt#3605, a default network is only added to containers when --net is empty (no custom networks), or when default or default-restricted were explicitly specified. For backwards compatibility, the order in which networks will be applied is define by their config file names. This allows some control on what is eth0, eth1, etc. inside pods, with filenames like Z01-custom.conf, N15-default.conf, ... Signed-off-by: Fabio Kung <fabio.kung@gmail.com>
Now that the network loader respects the filename order, --net=all was causing the default networks to be added as eth0, instead of the desired custom network Signed-off-by: Fabio Kung <fabio.kung@gmail.com>
f6250c4 to
d8ea9a0
Compare
|
Thanks, LGTM! |
BREAKING: do not automatically add
default*networks when custom ones are specifiedCurrently, a network named
"default"(or"default-restricted") is always added to Pods, even when--net=custom1,custom2,...was passed in. E.g.:The container in that case should only have
loandeth0, noteth1.The current behavior (a default network is always added) is not what is specified on docs (
Documentation/networking/overview.md):Signed-off-by: Fabio Kung fabio.kung@gmail.com