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

[BUG] [Any CNI ver] Validate/update pre-existing HCN network properties for mismatches with CNI config's expected properties. #98

Open
aznashwan opened this issue Nov 2, 2023 · 2 comments
Assignees

Comments

@aznashwan
Copy link
Contributor

Describe the bug

Currently, the CNI plugins simply attempt to look up the name of HCN networks as provided in the CNI config and use them unquestionably if they already exist, regardless of their properties lining up with what the CNI config expects.

To Reproduce

Steps to reproduce the behavior:

  1. Pre-create HCN network by either:
  • manually defining one through the PowerShell utility module using New-HNSNetwork
  • creating a CNI config and container using ctr, nerdctl, or any other means which calls ADD on the CNI plugins (in which case the plugins will automatically create the HNS network for us)
  1. Modify the CNI JSON config in any arbitrary way like subnet/gateway/routes/etc (except for its name ofc)
  2. Call ADD of the plugins with the updated JSON config and notice that the new properties are completely ignored (neither causing error, nor leading to the HNS network properties being synced to what the CNI config asks for)

Expected behavior
The plugins should notice that the CNI config properties mismatch with the HNS network properties and either:

  1. error out with a clear message requesting either a corrected CNI config or as for updates to the HNS network whose properties should the ones defined in the CNI config. While this would be relatively straightforward to implement, it might "soft-lock" users into needing to forcefully delete and redefine their HNS networks.
  2. attempt to update the properties of the HNS network (presuming it can be done safely without affecting any existing endpoints which might rely on the current properties of the HNS network)

CNI Version
CNI config version 0.2.0/0.3.0 (or any future ones we plan to support too)
Latest commit on CNI plugins as of time of writing: d502b1b

Additional context
FWIW, at least some of the reference CNI plugins for Linux do take steps to create OS-side resources
I will need to further check how/if they handle updates to CNI settings, however.

@TBBle
Copy link
Contributor

TBBle commented Nov 2, 2023

For visibility, at some point the opposite appeared to be the plan, per

// getOrCreateNetwork
// TODO: Require network to be created beforehand and make it an error of the network is not found.
// Once that is done, remove this function.
func getOrCreateNetwork(

@aznashwan
Copy link
Contributor Author

For visibility, at some point the opposite appeared to be the plan, per [this TODO]

Indeed, though note that the original commit's from 2018 and IMHO we should strive to replicate however the Linux plugins are handling things wherever possible (at least when approaching it from the containerd and general OCI interop angle, as there may be internal uses of these plugins I am unfortunately not aware of)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants