Remove listener only mode guest agent #7446
Conversation
There was a problem hiding this comment.
I think we need to refactor how we add/remove port mappings, so that if one of the three things we do (add listener, expose, and sending to wsl-proxy) fails, we have a unified approach to dealing with errors. That can either be keep going (to get partial forwarding), or rollback all three. Currently it's a… weird mish-mash?
| *ListenerTracker | ||
| } | ||
|
|
||
| // NewAPITracker creates a new instance of a API Tracker. | ||
| func NewAPITracker(forwarder forwarder.Forwarder, baseURL, tapIfaceIP string, isAdmin bool) *APITracker { | ||
| func NewAPITracker(ctx context.Context, wslProxyForwarder forwarder.Forwarder, baseURL, tapIfaceIP string, isAdmin, enableListeners bool) *APITracker { |
There was a problem hiding this comment.
There's so many args here that it needs to be documented. When is enableListeners relevant?
We may want to consider e.g. functional options instead.
There was a problem hiding this comment.
I added documentation for the args. Also, I like the use of functional options, however, I can't see how it could benefit us in this case.
| hostPort, err := strconv.Atoi(portBinding.HostPort) | ||
| if err != nil { | ||
| log.Errorf("error converting hostPort: %s", err) | ||
| continue |
There was a problem hiding this comment.
This also stops trying to call gVisor; is that the desired behaviour? (Same with the other error in the enableListeners block, and in Remove.)
|
|
Signed-off-by: Nino Kodabande <[email protected]>
| const isAdminInstall = await this.getIsAdminInstall(); | ||
|
|
||
| const guestAgentConfig: Record<string, string> = { | ||
| LOG_DIR: await this.wslify(paths.logs), | ||
| GUESTAGENT_ADMIN_INSTALL: isAdminInstall ? 'true' : 'false', | ||
| GUESTAGENT_KUBERNETES: enableKubernetes ? 'true' : 'false', | ||
| GUESTAGENT_IPTABLES: iptables.toString(), // only enable IPTABLES for older K8s |
There was a problem hiding this comment.
Shouldn't we edit /etc/init.d/guestagent or whatever it's called to drop the arg, then?
Nino-K
force-pushed
the
remove-listener-only-mode-guest-agent
branch
from
25ffb9a to
a00aa9d
Compare
Move all the Host Switch API calling code into its own package so that it can be shared across. Signed-off-by: Nino Kodabande <[email protected]>
When enableListeners flag is enabled we would also call the host-switch service API in addition to adding and removing the net listeners. Signed-off-by: Nino Kodabande <[email protected]>
Signed-off-by: Nino Kodabande <[email protected]>
The original rationale for including the `enableListeners` flag in the `APITracker` was to address specific Kubernetes versions that required a port forwarding fix. The need for this fix was determined based on the Kubernetes version as follows: - **Versions < 1.21**: No fix required. - **Version 1.21**: Fix needed if the patch version is 12 or higher. - **Version 1.22**: Fix needed if the patch version is 10 or higher. - **Version 1.23**: Fix needed if the patch version is 7 or higher. - **Versions >= 1.24**: Fix required. - **Versions with a major version number other than 1**: Fix required. Initially, the approach was to create local network listeners for these specific Kubernetes versions to leverage the WSL listener forwarding feature. However, since we no longer depend on WSL’s listener forwarding, we can now directly establish connections to the container using the NodePort entries in iptables. As a result, the `enableListeners` flag is no longer necessary for handling port forwarding fixes, and connections can be managed more directly through iptables configurations. Therefore, calling the host switch API should be adequate for all k8s services that either require the fix or not. Signed-off-by: Nino Kodabande <[email protected]>
Nino-K
force-pushed
the
remove-listener-only-mode-guest-agent
branch
from
a00aa9d to
0a4e92b
Compare
Signed-off-by: Nino Kodabande <[email protected]>
Signed-off-by: Nino Kodabande <[email protected]>
Nino-K
force-pushed
the
remove-listener-only-mode-guest-agent
branch
from
0a4e92b to
0cd6df8
Compare
This PR removes the iptable's listener creation functionality since we no longer rely on the WSL listener forwarding functionality in the virtual network. In addition, previously we did not call the host-switch API for the entries in iptables. This PR addresses that issue. In addition, it removes iptables package which was redundant. The original idea behind scraping the iptables was to pickup the portmaps in /proc/net/tcp that were placed in there from CNI portmap plugin. However, we subscribe directly to the k8s API, and we pick up all the port mapping directly from the API in kube/watcher package.
Fixes: #7447
and
Fixes: #7129
and
Fixes: #6944
Related: #6944