kubenet containerd: match upstream

[justinsb]

Configure kubenet in containerd/CNI mode to match upstream configuration.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [justinsb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[hakman]

The current approach is from the containerd project instructions and and results in 1 test failing:
https://github.com/containerd/containerd/blob/master/script/setup/install-cni
https://testgrid.k8s.io/kops-network-plugins#kops-aws-cni-kopeio

[sig-network] Services should preserve source pod IP for traffic thru service cluster IP [LinuxOnly]

The ptp approach from this PR results in 5 tests failing:
#10761
https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kops/10761/pull-kops-e2e-k8s-containerd/1358693640242728960

[sig-network] Services [It] should have session affinity timeout work for NodePort service [LinuxOnly] [Conformance]
[sig-network] Services [It] should be able to update service type to NodePort listening on same port number but different protocols
[sig-network] Services [It] should be able to switch session affinity for NodePort service [LinuxOnly] [Conformance]
[sig-network] Services [It] should be able to change the type from ExternalName to NodePort [Conformance]
[sig-network] Services [It] should have session affinity work for NodePort service [LinuxOnly] [Conformance]

If you prefer to use the ptp approach then we should skip the NodePort tests.

[justinsb]

Oh - thanks for confirming where it came from! Losing the pod source IP is ... not good. But I don't think we had this problem before, so I'm wondering if something else also changed.

But having the nodeport tests fail is also ... not good!

Thanks for pointing me in the right direction - I'll take a look and try to figure it out!

/hold

[hakman]

Oh - thanks for confirming where it came from! Losing the pod source IP is ... not good. But I don't think we had this problem before, so I'm wondering if something else also changed.

This was always a problem of the containerd kubenet integration.

[justinsb]

So after one or two iterations here this is working!

The thing I was missing was that if we don't pass networking=kubenet to kube-proxy it doesn't set up the default destination NAT rules. So all outbound traffic from the pods was broken; that was the NodePort failures AFAICT. (It seems we don't have any tests that check that directly, which is kinda surprising...)

upstream added a more sophisticated masquerade rule than kube-proxy was previously installing.

New way:
https://github.com/kubernetes/kubernetes/blob/e0bba105cca88c897c60ad80d4aa0a54c984dbb5/cluster/gce/gci/configure-helper.sh#L153-L170

Old logic:
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/dockershim/network/kubenet/kubenet_linux.go#L197-L203

I'm inclined to say that we take the CNI/containerd opportunity to sync up with what upstream is doing.

Along those lines, upstream has also switched to ptp (and the code passed with ptp). I'm pretty sure we can make it work with either. I don't know whether there's an advantage to one or the other. I'm inclined to switch to match upstream, open to arguments either way!

[hakman]

Nicely done! Looks good to me either way, but upstream would probably get better support in the future.

[justinsb]

OK so cleaned up and containerd is passing. I went with ptp to match upstream, and I went with a NAT configuration that more closely matched what we did before, as I'm not convinced that the upstream configuration is suitable for AWS / anywhere where the routing might not be symmetric. WDYT @hakman et al?

If we agree I can rebase to remove the force-kubenet trick!

[hakman]

Maybe we can discuss this more in detail during Office Hours, but I think it looks good as is now.
I vote to merge it as is and release 1.19.1, as it contains quite a lot of bug fixes.

@olemarkus Any thoughts?

[justinsb]

Yes, I think we can probably reach consensus tomorrow in office hours, and I think it's time for 1.19.1. The last issue I want to look at is 10719, though it looks like we have a fix available :-)

[hakman]

Storage driver failure, retesting.
/retest

[hakman]

Seems like it's time to merge :).
/lgtm

[justinsb]

/hold cancel