Support config flag for disabling HostPortManager

[ppmathis]

Description
While setting up Cilium 1.8 according to their official documentation on a fresh Kubernetes 1.18 dual-stack cluster with cri-o 1.18.2, I've noticed that the provided connectivity test pods by Cilium 1.8 kept failing their liveness/readiness probes and ended up crashing over and over again.

The culprit turned out to be the pod for echo-b, which was stuck on ContainerCreating with the following error message taken from kubectl describe pod <echo-b-instance>:

Warning FailedCreatePodSandBox 3m (x515 over 140m) kubelet, k8s-worker1 (combined from similar events): Failed to create pod sandbox: rpc error: code = Unknown desc = failed to add hostport mapping for sandbox k8s_echo-b-659766fb56-q6krf_default_23315f9c-9555-4c0c-8388-f1f21d86df38_0(90b3292f8b0b8596cc737018a362aa6fc5daccbcec65fe5b80a2192613289ef5): HostPortManager IP family mismatch: <redacted>::7f76, isIPv6 - true

After a discussion with one of the Cilium maintainers on their Slack channel, I realized that cri-o was the culprit. When Cilium is being installed in kube-proxy-replacement mode (= no kube-proxy installed), it will also manage HostPort mappings itself using eBPF. I first assumed that cri-o would deploy a hostport CNI plugin by default which I would just have to disable, but then realized that cri-o always interacts with HostPortManager (k8s.io/kubernetes/pkg/kubelet/dockershim/network/hostport) in the sandbox code:

cri-o/server/sandbox_run.go

Lines 72 to 89 in 754d46b

	func convertPortMappings(in []*pb.PortMapping) []*hostport.PortMapping {
	out := make([]*hostport.PortMapping, 0, len(in))
	for _, v := range in {
	if v.HostPort <= 0 {
	continue
	}
	if v.Protocol != pb.Protocol_TCP && v.Protocol != pb.Protocol_UDP {
	continue
	}
	out = append(out, &hostport.PortMapping{
	HostPort: v.HostPort,
	ContainerPort: v.ContainerPort,
	Protocol: v1.Protocol(v.Protocol.String()),
	HostIP: v.HostIp,
	})
	}
	return out
	}

cri-o/server/sandbox_run_linux.go

Lines 350 to 355 in 754d46b

	portMappings := convertPortMappings(sbox.Config().GetPortMappings())
	portMappingsJSON, err := json.Marshal(portMappings)
	if err != nil {
	return nil, err
	}
	g.AddAnnotation(annotations.PortMappings, string(portMappingsJSON))

Expected results
I expected cri-o to not interact with HostPortManager by default as this will break Cilium (or any other CNI which desires to handle hostport mappings by itself). I am not able to understand this behavior as it seems like the job of the CNI to do such a thing, but I unfortunately was not able to find any documentation why this code has been introduced.

Actual results
cri-o unconditionally parses the port mappings of a pod and interacts with HostPortManager to setup HostPort mappings by itself. I continued by compiling my own static build of cri-o, where I've added this hacky snippet

	log.Infof(ctx, "runSandbox: resetting parsed hostport mappings: %s", string(portMappings))
	portMappings = []*hostport.PortMapping{}
	portMappingsJSON, err = json.Marshal(portMappings)
	if err != nil {
		return nil, err
	}

before this

cri-o/server/sandbox_run_linux.go

Line 355 in 754d46b

g.AddAnnotation(annotations.PortMappings, string(portMappingsJSON))

line from the cri-o sources.

I was able to confirm that disabling the automated mapping resolved the issues I've had with Cilium and that cri-o indeed attempted to create a mapping based on this log message I've seen: RunSandbox: resetting parsed hostport mappings: [(*hostport.PortMapping=&{ 40000 80 TCP })]

Output of crio --version:

crio version 1.18.2
Version:       1.18.2
GitCommit:     7f261aeebffed079b4475dde8b9d602b01973d33
GitTreeState:  clean
BuildDate:     2020-06-18T21:05:27Z
GoVersion:     go1.14
Compiler:      gc
Platform:      linux/amd64
Linkmode:      static

Suggestion
I assume that this automated mapping is being relied upon by a certain set of users, so patching it out completely would probably not be the best choice. I suggest to add a configuration flag to cri-o which would allow to disable this HostPortManager integration completely, as this would allow ensuring compatibility with other CNIs like Cilium.

[haircommander]

thanks for opening an issue @ppmathis . We'd certainly consider adding a CLI flag that would gate this behaviour. Considering you've already dived in the code a bit, would you have any interest spinning up a PR?

[ppmathis]

would you have any interest spinning up a PR?

Sure! I've started working on a PR and will get one submitted in the next couple days. Code should be done, but testing, updating docs and those things are pending.

[haircommander]

would you have any interest spinning up a PR?

Sure! I've started working on a PR and will get one submitted in the next couple days. Code should be done, but testing, updating docs and those things are pending.

excellent to hear! thanks

[Pranav2612000]

Has this issue been fixed? I'll like to work on this

[haircommander]

it has not been, and it'd be great if you could pick it up! thanks @Pranav2612000

[github-actions]

A friendly reminder that this issue had no activity for 30 days.

[haircommander]

@Pranav2612000 did you still want to work on this?

[github-actions]

A friendly reminder that this issue had no activity for 30 days.

[hasan4791]

I can take up this issue meanwhile. Any words @haircommander

[haircommander]

sounds great! it's yours :)

[github-actions]

A friendly reminder that this issue had no activity for 30 days.