Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Proposal] Implement namespacing for networks #3096

Merged
merged 1 commit into from
Jun 19, 2024

Conversation

apostasie
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@AkihiroSuda
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

Zheaoli commented Jun 17, 2024

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
Copy link
Contributor Author

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 apostasie force-pushed the dev-3095 branch 2 times, most recently from ee1458f to b7c821a Compare June 19, 2024 01:23
@apostasie
Copy link
Contributor Author

Clarification on how this is working:

nerdctl --namespace=XXXX network ls

Will lookup in:

  • prefix/cni/net.d/: any network found there will show in ALL namespaces - this means this will give users the exact same behavior they currently have
  • prefix/cni/net.d/XXXX: this is the new, namespaced location - any network in there is only available to that exact namespace

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 apostasie force-pushed the dev-3095 branch 2 times, most recently from e910f8a to fb297f4 Compare June 19, 2024 03:22
@apostasie
Copy link
Contributor Author

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

Pending CI (green locally).

PTAL at your convenience.

Thanks!

Signed-off-by: apostasie <spam_blackhole@farcloser.world>
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@AkihiroSuda
Copy link
Member

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

@AkihiroSuda AkihiroSuda merged commit e9d1691 into containerd:main Jun 19, 2024
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants