search

microsoft / windows-container-networking

Container networking plugins for Windows containers
MIT License
77 stars 42 forks source link

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

Open aznashwan opened 10 months ago

aznashwan commented 10 months ago

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)
  2. Modify the CNI JSON config in any arbitrary way like subnet/gateway/routes/etc (except for its name ofc)
  3. 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 commented 10 months ago

For visibility, at some point the opposite appeared to be the plan, per https://github.com/microsoft/windows-container-networking/blob/b360521954ff8b432d3b5fcf76c4997d19a39b51/common/core/network.go#L396-L399

aznashwan commented 10 months ago

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)