containerd / nerdctl

contaiNERD CTL - Docker-compatible CLI for containerd, with support for Compose, Rootless, eStargz, OCIcrypt, IPFS, ...
Apache License 2.0
7.98k stars 594 forks source link

[Proposal] Implement namespacing for networks #3096

Closed apostasie closed 3 months ago

apostasie commented 3 months ago

Hey folks.

From the conversation at #3095 I appreciate that plugins networks cannot be fully "isolated" from one another, and as such that there is no concept of namespacing in the spec.

Nevertheless, not having namespace for networks commands is very surprising for the user - especially since we do not complain at all about the use of the global --namespace flag alongside the network command. Furthermore, I certainly see use cases where it would make a lot of sense as a feature.

This (relatively simple) proof of concept does implement "namespace-ing", effectively preventing one namespace from listing or manipulating directly networks of another.

Let me know what you are thinking about this - and if this would be a welcome feature or not.

apostasie commented 3 months ago

Note that the current situation has weird UX because of the inconsistency here. Specifically, trying to remove a network used by containers in another namespace will return the ids of said containers, which of course cannot be inspected nor listed.

apostasie commented 3 months ago

Just a note that I will wait for feedback on the idea and design before engaging with CI / test fixing.

AkihiroSuda commented 3 months ago

Thanks, the design looks good if it can pass the CI.

We may want to have something like nerdctl network create --global-namespace to keep supporting unnamespaced networks

apostasie commented 3 months ago

Thanks, the design looks good if it can pass the CI.

Ok.

I'll fix whatever is needed on CI/test to make this ready.

We may want to have something like nerdctl network create --global-namespace to keep supporting unnamespaced networks

With the current design, unnamespaced networks will show up in all namespaces. That should guarantee no disruption / migration required for users, and also acts as an interesting feature.

But we can talk about the details here when the PR is fully ready and with adequate tests.

Will ping here when done.

Thanks!

Zheaoli commented 3 months ago

I think maybe we should make this feature optional or experimental. Users can config it in the global config. I think this is a huge change for user

apostasie commented 3 months ago

I think maybe we should make this feature optional or experimental. Users can config it in the global config. I think this is a huge change for user

That works for me too if you prefer to minimize impact for now.

That being said, this change is transparent for the user and in most cases they would not notice it.

apostasie commented 3 months ago

Clarification on how this is working:

nerdctl --namespace=XXXX network ls

Will lookup in:

When the default network is not found and needs to be created, it will be created in the same usual location with NO namespace subfolder. This is also the exact same behavior we currently have.

When any other network is being created, it will be created under the namespaced folder instead of the global folder. This is the one difference in term of user experience.

Finally, when the user is deleting a network, it will first try to find it in the namespaced folder. If it cannot be found there, it will search and possibly delete in the global folder. Here as well, this aligns with the current behavior.

The CI is now green - I think this is ready for formal review now (I am re-pushing right now but this is just md / documentation).

Note that I am not yet adding specific tests for the new behaviors - I am currently working on cleanup of all network tests, and would like to send a PR for that soon + new network namespace tests built on top of that cleaned test suite if you don't mind.

Kindly let me know your thoughts on all this, and if you want it experimental or mainline.

@AkihiroSuda @Zheaoli

apostasie commented 3 months ago

@AkihiroSuda last push fixed your two comments (tests+doc).

Pending CI (green locally).

PTAL at your convenience.

Thanks!

AkihiroSuda commented 3 months ago

nerdctl namespace ls will have to show the number of the networks, but it can be done later