finch icon indicating copy to clipboard operation
finch copied to clipboard

Finch cannot resolve special IP `host-gateway`

Open ningziwen opened this issue 2 years ago • 19 comments

Describe the bug Finch cannot resolve special IP host-gateway in finch run --add-host name:host-gateway IMAGE

Steps to reproduce

finch run --add-host name:host-gateway -it public.ecr.aws/amazonlinux/amazonlinux:2 bash
# cat /etc/hosts
...
host-gateway    name
...

Expected behavior

finch run --add-host name:host-gateway -it public.ecr.aws/amazonlinux/amazonlinux:2 bash
# cat /etc/hosts
...
<resolved-ip>    name
...

Screenshots or logs

Additional context In Docker,

docker run --add-host name:host-gateway -it public.ecr.aws/amazonlinux/amazonlinux:2 bash
# cat /etc/hosts
...
192.168.65.2	name
...

Can reproduce in nerdctl in Finch VM:

nerdctl run --add-host name:host-gateway -it public.ecr.aws/amazonlinux/amazonlinux:2 bash
# cat/etc/hosts
...
host-gateway    name
...

ningziwen avatar Feb 06 '23 09:02 ningziwen

In Finch, to make the feature parity with Docker, the ideal user stories will be:

As a Finch user, if I use host-gateway in my finch run with configuring host-gateway-ip in Finch config, when I try to access host with the alias in container, it will use the ip I configured in Finch config.

cat ~/.finch/finch.yaml
...
host-gateway-ip: "192.168.5.2"
...
finch run --add-host name:host-gateway -it public.ecr.aws/amazonlinux/amazonlinux:2 bash
# cat /etc/hosts
...
192.168.5.2    name
...

## curl name:<port> with a valid port will succeed if 192.168.5.2 is a valid host gateway ip

As a Finch user, if I use host-gateway in my finch run without configuring host-gateway-ip in Finch config, my container can still use the IP alias to access host.

finch run --add-host name:host-gateway -it public.ecr.aws/amazonlinux/amazonlinux:2 bash
# cat /etc/hosts
...
<default-ip>    name
...
## curl name:<port> with a valid port will succeed

ningziwen avatar Feb 13 '23 02:02 ningziwen

The solution could be split to multiple stages:

At the first stage, we can resolve host-gateway to a static IP without being configurable.

Lima has a static host IP 192.168.5.2 that can be accessed in containers. The value can be retrieved from code. We can directly resolve host-gateway to it before passing to nerdctl. The value retrieved by Finch will change if Lima changes the value. E2e tests will also be added to validate the ip equals to the value and it can be used to access host.

Making host-gateway-ip configurable will come later.

ningziwen avatar Feb 13 '23 02:02 ningziwen

The first stage is already completed. The next thing to do it to make it configurable in Finch config for feature parity. It requires the nerdctl change to be promoted to Finch.

ningziwen avatar Mar 21 '23 23:03 ningziwen

Hi @ningziwen, I want to work on this issue. Can you guide me around it?

TrungBui59 avatar Jun 07 '23 23:06 TrungBui59

@TrungBui59

Sure, thanks for the interest! Assigned the issue to you. Feel free to raise any questions directly in the issue. I will be actively monitoring it and helping you.

ningziwen avatar Jun 07 '23 23:06 ningziwen

Thank you very much, I am kind of clueless, so can you give me a rough idea on how I should tackle the issue?

TrungBui59 avatar Jun 07 '23 23:06 TrungBui59

@TrungBui59 Yes. You may follow these steps to start working on this.

  1. Understand what is Finch overall, and read the code to understand how Finch uses Cobra as CLI tool and how Finch interacts with Lima and Nerdctl.
  2. Understand what is this issue for from the description. (basically for docker feature parity)
  3. Read the existing converation and PRs to understand current workaround in Finch, and the existing fix in Nerdctl.
  4. Think how to use the existing Nerdctl fix to resolve the issue in Finch and remove the workaround.

ningziwen avatar Jun 07 '23 23:06 ningziwen

@ningziwen I see. Thank you

TrungBui59 avatar Jun 07 '23 23:06 TrungBui59

@ningziwen Just to make sure my solution is acceptable. I will add a new field called host-gateway-ip on the finch.yaml. I will then parse by changing the pkg/config.go file and then get the data and pass to the SlirpGateway. Then Im gonna use it when user specify the ip

TrungBui59 avatar Jun 08 '23 19:06 TrungBui59

My understanding is SlirpGateway IP is a constant value in Lima, which is not designed to be configurable.

This nerdctl change introduced a host-gateway-ip field in nerdctl config, which is designed to be configurable. https://github.com/containerd/nerdctl/pull/1978/files

So my suggestion is if host-gateway-ip exists in finch config, pass it to host-gateway-ip in nerdctl config. Otherwise, pass SlirpGatewayIP to host-gateway-ip in nerdctl config.

Let me know if you have different thoughts.

ningziwen avatar Jun 08 '23 19:06 ningziwen

@ningziwen I have some issue with the tests. Unit tests keep giving me race condition on file I dont even touch. and e2e test is output something that I dont quite understand

e2e: Screenshot 2023-06-12 at 5 00 07 PM

unittest: Screenshot 2023-06-12 at 5 02 49 PM

TrungBui59 avatar Jun 13 '23 00:06 TrungBui59

@TrungBui59 For unit tests, please ignore the race condition related log. The race condition is just a warning and won't cause it failing. If you scroll up you should be able to find the true error causing it failing.

Scroll up to here:

?       github.com/runfinch/finch/benchmark     [no test files]
ok      github.com/runfinch/finch/benchmark/all 0.627s [no tests to run]
ok      github.com/runfinch/finch/benchmark/container   0.555s [no tests to run]
ok      github.com/runfinch/finch/benchmark/vm  0.695s [no tests to run]
ok      github.com/runfinch/finch/cmd/finch     0.598s
ok      github.com/runfinch/finch/pkg/command   0.312s
ok      github.com/runfinch/finch/pkg/config    0.690s
ok      github.com/runfinch/finch/pkg/dependency        0.873s
ok      github.com/runfinch/finch/pkg/dependency/vmnet  0.756s
?       github.com/runfinch/finch/pkg/flog      [no test files]
?       github.com/runfinch/finch/pkg/fmemory   [no test files]
?       github.com/runfinch/finch/pkg/mocks     [no test files]
?       github.com/runfinch/finch/pkg/system    [no test files]
?       github.com/runfinch/finch/pkg/version   [no test files]
ok      github.com/runfinch/finch/pkg/disk      0.337s
ok      github.com/runfinch/finch/pkg/fssh      0.448s
ok      github.com/runfinch/finch/pkg/lima      0.578s
ok      github.com/runfinch/finch/pkg/path      0.619s
-test.shuffle 1686616423691009000

If all of them are ? or ok, then you are good. There is an issue tracking the race condition warning. https://github.com/runfinch/finch/issues/447

ningziwen avatar Jun 13 '23 00:06 ningziwen

@ningziwen so for the e2e test, what does that error mean?

TrungBui59 avatar Jun 13 '23 00:06 TrungBui59

@TrungBui59 For e2e tests, most of the Finch e2e tests are in common-tests repo. And here is the failing test scenario from your log. https://github.com/runfinch/common-tests/blob/6c7275007bf34fb6ddecc4013c16f1d79ff6d1d0/tests/run.go#L335 Finch e2e tests use a framework called Ginkgo.

The tests are failing because your change broke them. You also need to scroll up to find the failed scenario and get more useful logs from there.

ningziwen avatar Jun 13 '23 00:06 ningziwen

@ningziwen I have finished the task, but there is one question I want to make sure. If the user update the host_gateway_ip in the finch config file, do we only allow to update it when they init the VM or they can update when they start the VM?

TrungBui59 avatar Jun 17 '23 22:06 TrungBui59

@TrungBui59 We think we should try to allow them update when they start (restart) the VM. Why close the PR?

ningziwen avatar Jun 18 '23 06:06 ningziwen

@ningziwen Got it. For the PR, it was an accident, I type a command and somehow it closed the PR

TrungBui59 avatar Jun 18 '23 22:06 TrungBui59

@ningziwen sorry for not really finishing earlier, I was quite busy with my work. Is this issue still open? If it is, I will be able to finish this by the couple next days

TrungBui59 avatar Aug 22 '23 17:08 TrungBui59

@ningziwen sorry for not really finishing earlier, I was quite busy with my work. Is this issue still open? If it is, I will be able to finish this by the couple next days

@TrungBui59 No worries. It is still open. Feel free to complete it.

ningziwen avatar Aug 22 '23 17:08 ningziwen

Retested this, seems this has been implemented:

192.168.5.2     name

Shubhranshu153 avatar Aug 20 '24 00:08 Shubhranshu153