Preserve backwards compatibility of CreateNetworkStatus #72
Conversation
The CNI spec - [0] - 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.
[0] - https://github.com/containernetworking/cni/blob/main/SPEC.md#add-success
Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
| networkStatuses, err := CreateNetworkStatuses(cniResult, "test-default-net-without-sandbox", true, nil) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| Expect(networkStatuses).To(WithTransform( |
There was a problem hiding this comment.
we must clear the DNS information since we are using the convertDNS function within CreateNetworkStatuses ...
|
/cc @dougbtv |
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 |
| @@ -337,6 +337,48 @@ var _ = Describe("Netwok Attachment Definition manipulations", func() { | |||
| }) | |||
| }) | |||
|
|
|||
| Context("create network statuses for a single interface which omits the sandbox info", func() { | |||
There was a problem hiding this comment.
thanks for adding the test case for this one
|
Miguel also noted that it does read as a "should" in the spec, and a not a "must"
from: https://github.com/containernetworking/cni/blob/main/SPEC.md#add-success |
Which improves backwards compatibility for network-status in latest updates to the client library, especially related to Calico. See also: k8snetworkplumbingwg/network-attachment-definition-client#72
Which improves backwards compatibility for network-status in latest updates to the client library, especially related to Calico. See also: k8snetworkplumbingwg/network-attachment-definition-client#72
|
great thanks |
The CNI spec states that the
sandboxattribute in the CNI result should be used to represent: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
CreateNetworkStatusesis created with a single interface, thus ensuring the original behavior will be provided to user. Unit tests are also added.