Fix Network Namespace Bug For Ctr

[dcantah]

If you try and run a hypervisor isolated container through ctr (.\ctr.exe run --runtime io.containerd.runhcs.v1 --rm --isolated mcr.microsoft.com/windows/nanoserver:1809 hyp-test cmd /c "echo Hello World!") currently you'll get "ctr: failure while creating namespace for container: network namespace not found: unknown". The normal path through ctr is no network namespace is passed, so our shim will try and make one. The namespace was being created via hns.CreateNamespace which stores the ID of the namespace in all caps, however later on in the process when we go to add the namespace to the uvm we re-grab a namespace object via hcn.GetNamespaceByID which populates the Id field in all lowercase.

When we originally store the namespace in our map of known namespaces we use the hns packages casing, and when we go to add any endpoints to the vm then we'll fail to find the namespace due to the casing mismatch. We already create the namespace for cri interactions with the hcn package so this truthfully brings this fallback path in line.

[dcantah]

Did some RNG to assign two people and it landed on you @helsaawy @katiewasnothere 😆

[kevpar]

It seems righteous to move from hns to hcn, but I'm not sure this code path should even exist in the first place. If the orchestrator wanted networking, they should pass a network namespace. I assume this won't give them real networking anyways since there will be no endpoint wired up. What happens if we just don't set a network namespace at all?

[dcantah]

@kevpar I'd thought of this also but thought that might be a longer discussion for removing this path altogether. It seems like the path exists if the client decides to allocate their own endpoints and place them in https://github.com/opencontainers/runtime-spec/blob/8958f93039ab90be53d803cd7e231a775f644451/specs-go/config.go#L548, which we don't use that field anywhere else in this project besides for this fallback path.
I'd thought this path was specifically for the non orchestrator routes like ctr/nerdctl for the moment, but really it seems like the route is to invoke whatever cni is configured to handle networking for those routes too so maybe this fallback is truly irrelevant at this point.

I assume this won't give them real networking anyways since there will be no endpoint wired up. What happens if we just don't set a network namespace at all?

Right, if nothings in the EndpointList field then there will be no adapter in the uvm if we hit this code path. Nothing goes wrong if we just skip network setup if there's no namespace passed at all iirc, but let me make sure. I can shift this to being getting rid of the codepath entirely and just skipping net setup altogether if the namespace received is empty if that makes the most sense.

[dcantah]

@kevpar Just skipping networking completely turns out fine which isn't too much of a surprise. Would you prefer we swap to this approach and get rid of the code that makes a netns for the fallback path as well?

package hcsoci
@@ -216,14 +217,12 @@ func initializeCreateOptions(ctx context.Context, createOptions *CreateOptions)
 // configureSandboxNetwork creates a new network namespace for the pod (sandbox)
 // if required and then adds that namespace to the pod.
 func configureSandboxNetwork(ctx context.Context, coi *createOptionsInternal, r *resources.Resources, ct oci.KubernetesContainerType) error {
-       if coi.NetworkNamespace != "" {
-               r.SetNetNS(coi.NetworkNamespace)
-       } else {
-               err := createNetworkNamespace(ctx, coi, r)
-               if err != nil {
-                       return err
-               }
+       if coi.NetworkNamespace == "" {
+               // Nothing for us to do as the caller didn't supply a network namespace, the container(s) will not have a network adapter unless
+               // added through external means.
+               return nil
        }
+       r.SetNetNS(coi.NetworkNamespace)

[dcantah]

Ideally I'd want to get this into the new hcsshim tag I was planning so folks can actually launch hyper-v containers through ctr when Containerd 1.6 comes out. @helsaawy @kevpar Let me know what makes the most sense to y'all. To me it's probably easier to reason with there being no fallback "make a net namespace for them" route like we have currently.

[kevpar]

Sorry, not completely following. Sounds like you're trying to pick between two options. Can you define what option A and B are, and what pros and cons they have? What do you think is the right path forward?

[dcantah]

@kevpar Yep. The gist of the problem is by default no namespace (networking in general really) is setup through ctr so we end up taking a slightly different code path than you normally would through a cri interaction. The reason this is a problem currently is stated in the pr/commit description so I won't rehash there, but the two options are as follows:

Option A
Get rid of this fallback code path entirely. Right now the fallback path creates a network namespace and then loops through a separate field in the runtime spec that is supposed to contain IDs for HCN endpoints to add to the newly created namespace. This field isn't employed in Containerd and this fallback path is the only time the contents are checked and used in hcsshim itself (before reaching the OS is what I'm trying to get across). It looks like this was how networking was handled for the V1 HCS schema.

Pros:

1. Easier to reason about; the client doesn't supply a namespace with the endpoints you want assigned to the container then the container will just not have networking.
2. Saves another event like this where we somehow break this fallback code path.
3. Perf improvement as you skip setting up a namespace in the VM if there's not going to be an adapter added to it (likely not much overhead in the first place but no work will always be better than a small amount).

Cons:

1. Perhaps there's some history here that I'm missing for why this fallback existed; why I was hesitant to remove it altogether here.

Option B
What we have right now in this PR. Fix the bug in the code path that handles no network namespace being passed due to casing issues with the old HNS APIs.

Pros:

1. Smaller change, by addition/deletion metrics at least.

Cons:

1. If someone was relying on populating that endpointlist field for some containerd client they drummed up, and by extension of that relying on us creating a namespace for them here for us to add the endpoints to, then that will break if we remove this.

[katiewasnothere]

@dcantah I prefer option A personally unless there's some actual need for the fallback case.

[dcantah]

@katiewasnothere Agreed, only bit is if by some small chance someone was relying on this behavior. It's been here since 2018 5ab7622#diff-f2f83f5f5b4ef2d52a5b06695dc8393c3e26aae86610c4c50d1fa93809c20d5dR8

[dcantah]

@helsaawy What do you think about the above? #1270 (comment)

[helsaawy]

I prefer option B:
We can probably add a flag or annotation to disable this path if a reason arises, but I think changing behavior without a major version increment or a deprecation warning in advance is a bad idea if we have no idea who is relying on it

[dcantah]

@helsaawy I mean I'm fine with sticking with the fix in this PR currently (option B). If anything we can get this in, backport to the release/0.9 branch, and then do option A and have it be the 0.10 way of doing things. Hard to reason with changes like this and versioning when we don't follow semver to a t

[dcantah]

Talked internally. Our plan will be to go with the fix in this PR (Option B) for now, backport the fix to release/0.9 and get out a new 0.9.x tag, and then get rid of this fallback path for the 0.10.x line going forward.

[katiewasnothere]

LGTM