search

k8snetworkplumbingwg / network-attachment-definition-client

A Golang Kubernetes client
Apache License 2.0
12 stars 37 forks source link

Preserve backwards compatibility of CreateNetworkStatus #72

Closed maiqueb closed 2 weeks ago

maiqueb commented 2 weeks ago

The CNI spec states that the sandbox attribute in the CNI result should be used to represent:

The isolation domain reference (e.g. path to network namespace) for the interface, or empty if on the host. For interfaces created inside the container, this should be the value passed via CNI_NETNS

Some CNIs (like calico) do not set the sandbox for their interfaces (below you'll find an example of a cached CNI result created by calico CNI):

{
  "result": {
    "cniVersion": "0.3.1",
    "dns": {},
    "interfaces": [
      {
        "name": "cali05f4a1849c5"
      }
    ],
    "ips": [
      {
        "address": "10.244.196.152/32",
        "version": "4"
      },
      {
        "address": "fd10:244::c497/128",
        "version": "6"
      }
    ]
  }
}

Thus, when multus started to use CreateNetworkStatuses (plural) we broke calico users relying on the network-status annotations (since that function relies on the sandbox attribute being defined to identify container interfaces).

This commit changes the code to use the original status creation whenever CreateNetworkStatuses is created with a single interface, thus ensuring the original behavior will be provided to user. Unit tests are also added.

maiqueb commented 2 weeks ago

/cc @dougbtv

dougbtv commented 2 weeks ago

the original status creation whenever CreateNetworkStatuses is created with a single interface

Ok, I think I get it... so if there's only a single interface, we can rely on it, and we don't have to care about if it has a sandbox reference or not

dougbtv commented 2 weeks ago

Miguel also noted that it does read as a "should" in the spec, and a not a "must"

sandbox (string): The isolation domain reference (e.g. path to network namespace) for the interface, or empty if on the host. For interfaces created inside the container, this should be the value passed via CNI_NETNS.

from: https://github.com/containernetworking/cni/blob/main/SPEC.md#add-success

oshoval commented 1 week ago

great thanks please consider adding the issue you opened https://github.com/projectcalico/calico/issues/9294 on the PR desc for additional context