Skip to content
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

Finch cannot resolve special IP host-gateway #209

Closed
ningziwen opened this issue Feb 6, 2023 · 20 comments
Closed

Finch cannot resolve special IP host-gateway #209

ningziwen opened this issue Feb 6, 2023 · 20 comments
Assignees
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@ningziwen
Copy link
Member

ningziwen commented Feb 6, 2023

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
Copy link
Member Author

ningziwen commented Feb 13, 2023

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
Copy link
Member Author

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 added a commit to runfinch/common-tests that referenced this issue Feb 13, 2023
Signed-off-by: Ziwen Ning <ningziwe@amazon.com>

Issue #, if available:
runfinch/finch#209

*Description of changes:*
Start a server in host and try to curl it from container with
`--add-host test-host:host-gateway`

*Testing done:*
`make run` with the change runfinch/finch#216



- [ X ] I've reviewed the guidance in CONTRIBUTING.md


#### License Acceptance

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.

Signed-off-by: Ziwen Ning <ningziwe@amazon.com>
ningziwen added a commit that referenced this issue Feb 13, 2023
Issue #, if available:
#209

*Description of changes:*
Support special IP `host-gateway` in `--add-host` flag. Finch will
resolve to the IP that can be used to access host from containers. It is
not configurable for now. See
[issue](#209) for more details.

*Testing done:*
unit tests and e2e tests
runfinch/common-tests#29

- [ X ] I've reviewed the guidance in CONTRIBUTING.md


#### License Acceptance

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.

---------

Signed-off-by: Ziwen Ning <ningziwe@amazon.com>
ningziwen added a commit that referenced this issue Feb 20, 2023
#209

*Description of changes:*

parse --add-host special ip with equal sign, like
`--add-host=name:host-gateway`

*Testing done:*

unit tests and e2e tests


- [ X ] I've reviewed the guidance in CONTRIBUTING.md


#### License Acceptance

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.

Signed-off-by: Ziwen Ning <ningziwe@amazon.com>
@ningziwen
Copy link
Member Author

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 ningziwen added the good first issue Good for newcomers label Jun 6, 2023
@ningziwen ningziwen removed their assignment Jun 6, 2023
@TrungBui59
Copy link

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

@ningziwen
Copy link
Member Author

@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.

@TrungBui59
Copy link

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

@ningziwen
Copy link
Member Author

@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.

@TrungBui59
Copy link

@ningziwen I see. Thank you

@TrungBui59
Copy link

@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

@ningziwen
Copy link
Member Author

ningziwen commented Jun 8, 2023

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.

@TrungBui59
Copy link

@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

@ningziwen
Copy link
Member Author

ningziwen commented Jun 13, 2023

@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. #447

@TrungBui59
Copy link

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

@ningziwen
Copy link
Member Author

@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.

@TrungBui59
Copy link

@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?

@ningziwen
Copy link
Member Author

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

@TrungBui59
Copy link

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

@TrungBui59
Copy link

@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

@ningziwen
Copy link
Member Author

@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.

@Shubhranshu153
Copy link
Contributor

Retested this, seems this has been implemented:

192.168.5.2     name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants