-
Notifications
You must be signed in to change notification settings - Fork 699
Subscribe to container engine API for published ports #4021
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
2792bfd
to
6e5e4a6
Compare
Seems overengineering. |
go.mod
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too many new dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, there are many indirect dependencies being used for just a few direct ones. However, I went ahead and compared the guest agent sizes from the master build and my PR, and I’m happy to share the results below:
total 231384
-rw-r--r-- 1 ninok wheel 55M Sep 11 10:27 guestagent_master
-rw-r--r-- 1 ninok wheel 58M Sep 11 10:26 guestagent_pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is mostly about the maintenance burdens rather than the binary footprint; it is hard to handle dependabot PRs and detect potential supply chain attacks.
Also, the inflation of the Go deps is also challenging for getting Lima in Debian, as Debian makes dpkg for each of those deps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is mostly about the maintenance burdens rather than the binary footprint; it is hard to handle dependabot PRs and detect potential supply chain attacks.
True, but these are all dependencies of the containerd
and dockerd
client libraries, so should be subject to some scrutiny from upstream already.
Also, independent of this issue, we should maybe consider setting a dependabot cooldown period, e.g. 7 days. It will tell dependabot not to make a PR until there have been no new releases of the dependency for those 7 days. This also helps with accidental breakage in dependencies, that is normally fixed quickly.
I've found the cooldown more important for node dependencies, but it can be setup for each eco system.
Also, the inflation of the Go deps is also challenging for getting Lima in Debian, as Debian makes dpkg for each of those deps.
I just looked at https://go-team.pages.debian.net/packaging.html but did not find any rationale for why they would go to all that work. What is the benefit?
Anyways, I assume that Debian has packaged all the prerequisites for docker and containerd clients already, so this should not block anything.
return ipPorts, nil | ||
} | ||
// If the label is not present, we check the network config in the following path: | ||
// <DATAROOT>/<ADDRHASH>/containers/<NAMESPACE>/<CID>/network-config.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file isn't expected to be parsed externally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the issue is that since containerd/nerdctl#4290 the information is no longer available via labels. How are you expected to get the port mapping information?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expectation is not to depend on container engine implementations; audit events or eBPF should work (see my comments below)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expectation is not to depend on container engine implementations; audit events or eBPF should work (see my comments below)
I think this is a poor excuse for nerdctl
breaking backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What backwards compatibility is broken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The backwards compatibility issue is that you used to be able to query container labels for the list of exposed ports. That functionality has been removed in nerdctl 2.1.3 because it broke in an edge case when a large number of ports were exposed.
There is no accommodation for exposing ranges as ranges, they were unrolled to create a label for each port, hitting the containerd limitation when using large ranges. So the labels were removed completely, making it impossible to determine exposed ports via inspection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The labels are private implementation details too.
The correct way to inspect the port is to:
$ nerdctl container inspect foo | jq .[0].HostConfig.PortBindings
{
"80/tcp": [
{
"HostIp": "0.0.0.0",
"HostPort": "8080"
}
]
}
This works for Docker too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you do it over the API? You can do this via the API with docker (and you used to be able to do it with nerdctl containers too).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No containerd API, as the port forwarding is not handled by the daemon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really want to argue this anymore, and maybe it is Hyrum's law, but in the end the fact remains that information about the container was available via an API and now is only available by running a subprocess, with the inherent brittleness of that approach (error handling).
There are no event watchers for docker or containerd; we rely on polling See e.g. abiosoft/colima#71 This PR is basically a port of the event watchers from Rancher Desktop on Windows, which does not suffer from this issue. |
We also monitor audit events: lima/pkg/guestagent/guestagent_linux.go Line 33 in 69538a7
Maybe the audit monitor has a bug ? |
There is also a proposal to use eBPF to monitor ports cc @balajiv113 |
In addition to what @jandubois also pointed out, the current implementation |
This PR is in response to #2536 I thought I read a note from @balajiv113 that the audit approach didn't work out because most cloud images did not have the required kernel modules for it installed, but can't find the reference right now. And #3067 also sounds like it doesn't work anymore for Kubernetes. |
#1855 may explain why audit monitoring doesn't seem to work. |
Monitor container creation and deletion events by subscribing to the container engine's API. Upon receiving a container creation or deletion event, the system immediately forwards the port mappings through the aggregated channel. This ensures that the ports are opened on the host without any latency. Signed-off-by: Nino Kodabande <[email protected]>
6e5e4a6
to
2a0d45d
Compare
Probably we can revisit this TODO to improve the ticking lag lima/cmd/lima-guestagent/daemon_linux.go Lines 57 to 63 in 7459f45
( |
On the eBPF PR this is the current state,
|
Thanks, that sounds promising! So what I remember about some distros not having the prerequisite kernel modules, or missing permissions, was not, or is no longer correct? |
@@ -17,10 +17,13 @@ require ( | |||
github.com/cpuguy83/go-md2man/v2 v2.0.7 | |||
github.com/digitalocean/go-qemu v0.0.0-20221209210016-f035778c97f7 | |||
github.com/diskfs/go-diskfs v1.7.0 // gomodjail:unconfined | |||
github.com/docker/docker v28.3.3+incompatible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it help with respect to code maintenance if we can create a go mod to handle all these.
I did create trackport to handle multiple various implementation of port identification
https://github.com/balajiv113/trackport/blob/main/pkg/trackapi/api.go
https://github.com/balajiv113/trackport/tree/main/pkg/internal
I was able to solve it using kprobe. This is available in almost all distro I checked. Even our FT were passing except docker that's when it hit the kubernetes issue |
So you are saying we should not merge this PR, but instead wait for you to finish yours?
We already have a Kubernetes port monitor in the current guest agent; we will just need to keep it. |
The combination of @balajiv113 's eBPF PR (#3067) w/ the existing kubernetesservice watcher seems the most robust and compatible solution for now? I wish we could remove the dependency on Kubernetes client libraries, but it can be discussed separately in future. |
Yes, if it works and is indeed robust, then a generic solution is obviously preferable. I was under the impression that this experiment had failed. I'll be happy to learn if I was wrong. |
This PR aims to capture the published ports within the VM for various container engines. It directly subscribes to the corresponding container engine APIs (Docker, containerd and Kubernetes) to detect published ports immediately as a container is created.
I'm also planning to move the iptables and procnet settings under the portMonitor property; however, that will be addressed in a follow-up PR.