Allow forwarding of specified port ranges to all interfaces

[jandubois]

This allows to expose e.g. an ingress port outside of the local host while still forwarding other ports only to localhost.

Example:

allInterfaces:
- port: 443
- port: 4000
  until: 4999

I'm not quite sure how this will interact with #45, but I think/hope it will be independent (the filtering of incoming connections would be done via iptables inside the guest?).

I'm also not really happy about the allInterfaces key name, but couldn't come up with anything better.

[AkihiroSuda]

I'm also not really happy about the allInterfaces key name, but couldn't come up with anything better.

What about ports (or portForwards) ?

ports:
- guestPort: 443
  hostIP: "0.0.0.0" # overrides the default value "127.0.0.1"
# default: hostPort: 443
# default: guestIP: "127.0.0.1" (implementation can be done later, but we have to reserve the field name now)
# default: proto: "tcp" (same as above)

- guestPortRange: [4000, 4999]
  hostIP:  "0.0.0.0" # overrides the default value "127.0.0.1"
# default: hostPortRange: [4000, 4999]
# default: guestIP: "127.0.0.1"
# default: proto: "tcp"

- guestPort: 80
  hostPort: 8080 # overrides the default value 80
# default: hostIP: "127.0.0.1"
# default: guestIP: "127.0.0.1"
# default: proto: "tcp"

I'm not quite sure how this will interact with #45, but I think/hope it will be independent (the filtering of incoming connections would be done via iptables inside the guest?).

No, iptables inside the guest cannot be used, as the guest is unaware of the src IP.
So the filtering will be implemented in a usermode proxy process on the host.

I assume #45 will not need a new YAML field. "Try ssh -L first and then fall back to our own forwarder" would be fine. We can also consider completely removing ssh -L, depending on benchmarking result.

[jandubois]

What about ports (or portForwards) ?

Yes, much better and more flexible. I've updated the PR to match your samples.

[jandubois]

Sorry, did one more push to fix a trivial error in an error message:

        case port > 65535:
-               return errors.Errorf("field `%s` must be < 65535", field)
+               return errors.Errorf("field `%s` must be < 65536", field)

But now I'm really done...

[jandubois]

@AkihiroSuda I've been wondering how GuestIP is supposed to be used. Is given that we only forward ports from 0.0.0.0 or 127.0.0.1, is there a reason to every use anything else but 127.0.0.1 as the guest IP?

Even if the port is bound to ::1 it is still reachable via 127.0.0.1 as well, as both addresses are just aliases on the lo adapter.

I tried to make the logic deal with GuestIP as well, but it quickly got confusing, and I don't actually see any functionality it would give us, so I dropped it again. Maybe this will be different when we get an actual IP address via vde_vmnet?

[AkihiroSuda]

I'd like to see unit tests and/or integration tests, but tests can be added later

[AkihiroSuda]

I guess the "guestIP" isn't going to be used in most cases. The field is only useful when an application inside VM calls bind("192.168.X.X:Y").

[jandubois]

I'd like to see unit tests and/or integration tests, but tests can be added later

I've been trying to create integration tests today, but can't make them work reliably because there is no way to flush the hostagent logs to disk. While looking into this I noticed that logrus is in maintenance mode, but all the newer libraries don't seem to have a Flush API either (but maybe one can implement it at the app level, which doesn't seem to be possible with logrus).

Anyways, will change the tests to shut down the instance, and analyze the logs afterwards.

BTW, I noticed that virtually all the output from the hostagent except for status messages goes to stderr. If this is intentional, then I think it would make more sense to just have a single log stream for the hostagent.

[AkihiroSuda]

I've been trying to create integration tests today, but can't make them work reliably because there is no way to flush the hostagent logs to disk.

Implemented in commit hostagent: sync every (io.Writer).Write. Accidentally pushed to master without opening a PR. 😅
722b7d1

BTW, I noticed that virtually all the output from the hostagent except for status messages goes to stderr. If this is intentional, then I think it would make more sense to just have a single log stream for the hostagent.

Stdout is guaranteed to be unmarshallable as JSON lines, but stderr is no, because imported libraries may print something to stderr without using logrus. So we have to maintain the two streams.

[jandubois]

Implemented in commit hostagent: sync every (io.Writer).Write.

Thanks! It turns out that I also mis-remembered that the guest-agent checks for port changes every second; it only checks every 3 seconds; so my strategic sleep 2 pauses need to be sleep 5 to make sure forwards are removed before they are added back...

I see that the lima-guestagent has a --tick option to specify a different poll frequency, but there is no way to configure it via lima. I'm inclined to add an env variable to set it (LIMA_GUESTAGENT_TICK?), or do you want it to be part of the yaml file?

(Wild idea for some distant time in the future: make the polling adaptive: poll more frequently whenever a new process or container is being created, as ports are typically opened early in the lifetime of a server process. Back off later, and especially when running on battery. Not sure if it is worth it; maybe it is just related to me seeing a demo using eBPF/LSM hooks yesterday 😄 ).

Stdout is guaranteed to be unmarshallable as JSON lines, but stderr is not,

Yes, but stdout seems to contain only 2 lines:

{"time":"2021-07-21T00:09:03.305227-07:00","status":{"sshLocalPort":60020}}
{"time":"2021-07-21T00:09:25.48178-07:00","status":{"running":true,"sshLocalPort":60020}}

I don't think they are important enough to require a separate JSON-unmarshallable file. Is there anything else written to it?

Anyways, my argument was that all the tracing and logging should go into stdout as well, and only errors should be written to stderr. Or maybe I'm still confused about what each file is supposed to be used for.

[AkihiroSuda]

I see that the lima-guestagent has a --tick option to specify a different poll frequency, but there is no way to configure it via lima. I'm inclined to add an env variable to set it (LIMA_GUESTAGENT_TICK?), or do you want it to be part of the yaml file?

Either is fine to me, but I think we should clarify that the field is likely to be removed in future.

(Wild idea for some distant time in the future: make the polling adaptive: poll more frequently whenever a new process or container is being created, as ports are typically opened early in the lifetime of a server process. Back off later, and especially when running on battery. Not sure if it is worth it; maybe it is just related to me seeing a demo using eBPF/LSM hooks yesterday 😄 ).

Yes, actually I was planning to replace ticks with eBPF retprobes for bind, though it might be too much complicated.
We can also consider patching CNI portmap to inject a tick to the lima guestagent.

[AkihiroSuda]

Thank you!