Initial merge of Solo out-of-tree inpod CNI

[bleggett]

CNI node agent changes that go with @yuval-k's istio/ztunnel#747

For context see: https://docs.google.com/document/d/1dynKlnNgIOywv3cwuw_2RCk_SKFRs7IrESaC8_r-sm0/edit

Basically, this reworks the ambient parts of the current Istio CNI agent, and fully replaces the old ambient CNI components.

Reviewers

Key code to review is:

• cni/nodeagent
• cni/plugin
• cni/zdsapi

the rest is tests and integrating with existing code, and deletion of now-unused code.

Internally, we actually had built this as a pure-ambient agent and had stripped out all of the sidecar-related code from the plugin/installer/agent, running it as a separate container in the istio-cni daemonset and chaining with the Istio plugin.

For this PR I've re-combined them since that's not likely to be acceptable in-tree, so note that the old sidecar iptables stuff and tests (and related messiness, TODOs, and non-mockability) has not been touched for the most part (other than renaming it to more clearly segregate it from the ambient stuff.

TL;DR

Old code:

• If ambient was enabled, the CNI plugin installed on the node by the node-agent install server would no-op.
• If ambient was enabled, the CNI server would start an ambient cluster watch server, which would configure host-netns networking for newly observed pods.

New code:

• If ambient is enabled (for CNI agent itself, and the observed pod), the CNI plugin installed on the node by the node-agent install server will forward an Add event to the ambient watch server over UDS (but will otherwise do nothing).
• If ambient is enabled for the CNI agent itself, the CNI server will start an ambient cluster watch server, which will listen for plugin events over UDS and/or K8S control plane events for relevant pods. The CNI agent will obtain the netns of the relevant pods, jump into the pods, and set up iptables redirection rules. It will then push a message to the node ztunnel over another UDS socket with the pod netns file descriptor, so that ztunnel can establish listeners inside the pod's netns.

Goals:

• Redirection/enrollment happens at the earliest possible time - when the CNI is informed of the pod creation, before K8S itself is aware or the pod is ready.
• CNI agent on the node is not in the pod datapath, and can safely be restarted (though ambient-enrolled pods will not be schedulable if the CNI agent on the node they are scheduled on is non-responsible, as the CNI plugin will error out - this is A Good Thing)
• ztunnel on the node can also be safely restarted - on restart it will re-announce itself to the node's CNI agent and get a snapshot of current workloads.

TODOs:

• As part of this we added a more mockable/testable/robust iptables impl than the one that was previously used by the plugin for sidecar redirection - Ideally we could replace the sidecar iptables impl with this new one, rather than have two. The sidecar one makes use of several not-very-flexible plugin-level global var overrides to support mocking and testing and it would be good to remove it in favor of the new one, but I have not done that.

• This replaces the old ambient CNI server, which renders the existing ambient eBPF impl (which assumes a host-netns redirection model among other things) non-working.

• The re-merge might have broken some of the test suites, I'll fix those shortly - however note that integ tests will not pass until a new ztunnel build is cut from inpod redirection mode ztunnel#747 and the SHA is bumped.

[PlatformLC]

A general question: will we plan to remove/cleanup the old redirection implementation eventually or support both mode?

[hzxuzhonghu]

can you split it into several prs or commits, i think it is difficult to review

[bleggett]

A general question: will we plan to remove/cleanup the old redirection implementation eventually or support both mode?

This PR fully replaces the old impl, dropping it, and I would suggest we continue with that tack.

The CNI agent is IMO (with ambient + sidecar + alt/optional flows for each, 2-3 listening servers spawned) already too complex and multipurpose. Keeping the old impl around in what is already a nonstable mode just adds more spaghetti, so I don't think we should.

That's partially why in the internal impl we split the ambient CNI out into a separate container that was optionally enabled if ambient was enabled and disabled ambient mode in the existing container, such that it only handled sidecar CNI and the additional container handled only ambient CNI, effectively shipping 2 separate containers inside the Istio CNI daemonset.

That is very clean operationally and testing-wise but it introduces quite a bit of code duplication (not a bad thing in a private fork as it nicely buffers you from upstream) that I don't think would be acceptable to the Istio maintainers.

If I'm wrong about that I'd be happy to go back to the two-container model, where we have one sidecar CNI impl and one ambient CNI impl and they live in different trees and are built as 2 separate containers managing different sets of pods, deployed in the same daemonset - but keeping everything as 1 container and adding yet-another-flag on top of the ones we have seems like a needless maintenance mess when we have no back compat requirements for ambient.

[bleggett]

can you split it into several prs or commits, i think it is difficult to review

I agree and can, but not in a way that would be functional, or would be mergable/pass CI.

I can do what we had done internally and PR a net-new standalone container that only does ambient CNI, and disables the existing ambient handling in the current container so that it only manages sidecars. That would still be a lot of (duplicated) code but it would be all net-new - that might be easier to review, it's hard for me to judge.

[bleggett]

@howardjohn and @keithmattix I think I addressed/resolved most of the simple stuff, thanks - there are a few unresolved comments where I opened an issue to track as a followup, or there's an open question.

[bleggett]

@howardjohn when you're ready to remove the DNM I'll cherry-pick to 1.21 - thanks!

[bleggett]

Reverting 316bd69 - I can reproduce test flakes due to missed pod events with the mock kube client the tests use without this.

This is jogging my memory - this is why @yuval-k did the queueing in this manner - to work around the fact that the mock client in pkg/kube/client.go has slightly different behavior than the real thing, and otherwise in tests will miss events.

With the revert, the flake is gone (after ~500 iterations anyway) and this tracks with why we didn't see it internally for the past 6 months.

Digging into why the existing test rig kube client drops events without this manual requeue, but the above revert should be fine either way.

This is (we think) related to kubernetes/kubernetes#95372

Threw up #49027 - the double-queue we have here will work, but we might be able to arrive at a solution for tests that doesn't require that by either forcing more cache syncs, or fixing bugs in the fake kube client.

[istio-testing]

In response to a cherrypick label: new pull request created: #49028