-
Notifications
You must be signed in to change notification settings - Fork 53
Port forwarding test between apps connected to different network instances #576
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
Conversation
giggsoff
left a comment
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, it does not work for me, probably due to hardcoded overlapping subnets on interfaces.
> exec -t 1m bash test_connectivity.sh 2234 2223
[stdout]
192.168.0.10
ssh -o ConnectTimeout=10 -o StrictHostKeyChecking=no -i /Users/petrfedchenkov/GolandProjects/eden/dist/tests/eclient/image/cert/id_rsa root@127.0.0.1 -p 2234 sh /root/portmap_test.sh 2223
ssh -o ConnectTimeout=10 -o StrictHostKeyChecking=no -i /root/.ssh/id_rsa root@192.168.0.10 -p 2223 grep Ubuntu /etc/issue
[stderr]
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@ WARNING: REMOTE HOST IDENTIFICATION HAS CHANGED! @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
IT IS POSSIBLE THAT SOMEONE IS DOING SOMETHING NASTY!
Someone could be eavesdropping on you right now (man-in-the-middle attack)!
It is also possible that a host key has just been changed.
The fingerprint for the ECDSA key sent by the remote host is
SHA256:hYSnInDfR3z6drvLPzQCcF3Zq3Q1d75Z73ZCCN5SWrs.
Please contact your system administrator.
Add correct host key in /Users/petrfedchenkov/.ssh/known_hosts to get rid of this message.
Offending ECDSA key in /Users/petrfedchenkov/.ssh/known_hosts:4
Password authentication is disabled to avoid man-in-the-middle attacks.
Keyboard-interactive authentication is disabled to avoid man-in-the-middle attacks.
ssh: connect to host 192.168.0.10 port 2223: Connection timed out
[exit status 255]
FAIL: ../eclient/testdata/port_forward.txt:87: command failure
I pushed new version of eclient, can you please also try?
pkg/eden/eden.go
Outdated
| offset := 0 | ||
| firstNet := nets[0].Subnet | ||
| firstAddr := nets[0].FirstAddress | ||
| secondAddr := net.ParseIP("192.168.0.11") |
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.
Do we really need to hardcode it? Why not to re-use existing logic with novel offsets?
In my case I have 192.168.0.10/24 and 192.168.0.11/24 on interfaces.
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.
Existing logic created two different subnets and packets do not get routed between these subnets. Intention is to create a single subnet and keep both interfaces in that subnet. I did not understand the novel offsets part. Can you please elaborate?
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 mean, that before your changes we had nics on different subnets provided by utils.GetSubnetsNotUsed function. With your change we have one subnet (firstNet) which depends on environment (192.168.0.1/24 in my case) and another one is hardcoded (net.ParseIP("192.168.0.11")). Are we really expecting this? What is the point of putting a harcode in this line?
I did not understand the novel offsets part. Can you please elaborate?
If I correctly understood, your add port forwarding on the second nic with predefined offset from ports in the config. In this case why not to add this logic into existing code
Line 447 in 125938a
| qemuOptions += fmt.Sprintf(",hostfwd=tcp::%s-:%s", k, v) |
Intention is to create a single subnet and keep both interfaces in that subnet.
Could you elaborate why this is necessary?
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.
utils.GetSubnetsNotUsed creates subnet and that is 192.169.0.0/24 and it also allocates an IP i.e. 192.168.0.10 for the first NIC. The 192.168.0.11 that is see is not a subnet, but an IP used for the second NIC.
Both NICs have to be in the same subnet, because if they are in different subnets, qemu networking is not routing packets between them. Instead packets are going somewhere out into the internet. Probably that is because of the separate NAT instances created for these subnets. Also port forwarding has the case where port forwarded packets pass between two different interfaces in the same subnet. Basically apps connected to different network instances, but with uplink ports connected to the same network should be able to talk to each other using port forwarding.
| if len(qemuPorts) > 0 { //not empty forwarding rules, need to check for existing | ||
| for _, qv := range qemuPorts { | ||
| if qv == strconv.Itoa(extPort) { | ||
| ni.ports[extPort] = intPort | ||
| continue exit | ||
| } | ||
| } | ||
| log.Fatalf("Cannot use external port %d. Not in Qemu %s", extPort, qemuPorts) | ||
| } |
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.
Probably you should add check for ports with offset (and add offset to default.go) instead of deleting the check?
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 do
| echo {{template "ssh"}}$HOST -p $1 sh /root/portmap_test.sh $2 | ||
| {{template "ssh"}}$HOST -p $1 sh /root/portmap_test.sh $EVE_IP $2 |
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.
Please, keep lines in sync.
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 fix
pkg/eden/eden.go
Outdated
| commandArgsString = fmt.Sprintf("natnetwork add --netname %s --network %s --enable --dhcp on", | ||
| "natnet1", "10.0.2.0/24") |
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 see hardcoded subnet here, but not for the second nic. Why do we have different behavior here?
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.
Both NICs use the same subnet. This is the same subnet that vboxmanage is hardcoded to create. We have to provide a subnet to create network.
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 also make this a constant in pkg/defaults package.
It's a bit odd. The same test with qemu passed on Linux and fails on Mac. |
For some reason networking with Qemu on mac does not work the same as Qemu on Linux. For now, the new test has been restricted to run only if the host OS is LInux. |
| }, | ||
| } | ||
|
|
||
| func getUplinkPortIPMap() map[string]net.IP { |
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.
Does this function belong here?
It looks it should be wrapped somewhere around set-ports.sh ?
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 am lost. Please help me here.
This function makes API call to controller for the lastest info message that the device sent.
We read the IP addresses that the edge-node got from DHCP from this reponse. These IP addresses are used for setting up port map rules during the test.
Should this be done using shell script and pass the information to go program somehow?
| return "container doesn't exist", nil | ||
| } | ||
| return state, nil | ||
| } |
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.
Same large comment - let's separate the functions and implementation-specific things.
I'd like to see
Vbox.go
Parallels.go
Qemu.go
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 do that
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.
made separate files
| if err != nil { | ||
| log.Fatalf("Port map port %s could not be converted to Integer", qv) | ||
| } | ||
| if portNum == extPort || (portNum + defaults.DefaultPortMapOffset) == extPort { |
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.
is it super clear to do such arithmetic calculations?
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'll see if I can avoid this.
| } | ||
| return IP, nil | ||
| ips := []net.IP{IP} | ||
| IP2 := net.ParseIP(fmt.Sprintf("192.168.%d.11", ind)) |
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.
Magic number?
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. It is an IP address. Needed to allocate another static IP address for a different ethernet interface on the emulated machine. Copied from what is there 4 lines above.
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.
got it.
| @@ -0,0 +1,4 @@ | |||
| #! /bin/sh | |||
|
|
|||
| EVE_IP=$(curl -s "http://169.254.169.254/eve/v1/network.json" | jq -r '."external-ipv4"') | |||
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.
Looks it's a strange magic number. Maybe even wrong?
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.
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.
EVE is the meta-data distributor in this case. Please let me know if it still looks wrong.
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.
@mydatascience The eve-specific URL is documented in https://github.com/lf-edge/eve/blob/master/docs/ECO-METADATA.md The generic use of http://169.254.169.254/ is part of cloud-init.
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.
got it
fd64a24 to
c691932
Compare
… - Qemu Signed-off-by: gkodali-zededa <gkodali@zededa.com>
…VBox Signed-off-by: GopiKrishna Kodali <gkodali@zededa.com>
Signed-off-by: GopiKrishna Kodali <gkodali@zededa.com>
Signed-off-by: GopiKrishna Kodali <gkodali@zededa.com>
Signed-off-by: GopiKrishna Kodali <gkodali@zededa.com>
Signed-off-by: GopiKrishna Kodali <gkodali@zededa.com>
|
Please resolve the conflicts and let's merge. But I am still confused with the calculation for the shifts + ports. |
I resolved the conflicts and the PR build is currently running. The port maps list is right now considered global and the same port maps are added on all ethernet interfaces of the emulated machine. With this behavior there will be port map conflicts. For my test I didn't want that to happen and so decided to add an offset for each interface (keeping the port map list unchanged). |
giggsoff
left a comment
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.
Please take a look onto ipMap for VirtualBox, seems, we will have no port map without restart of EVE after onboard.
| } | ||
| } | ||
|
|
||
| eth0IP := ipMap["eth0"] |
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.
Can we have fallback variant here(with empty ipMap)? In your variant, we expect ipMap filled, but it is true only after EVE onboard.
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.
It's not clear to me. Can you please elaborate?
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.
You add getUplinkPortIPMap() function, which will return non-empty map only in case of onboarded EVE. But in the beginning (not onboarded, just started) it will be empty. So, we should care about it.
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.
Yes, you are right. We'll need reboot for the port maps to take affect. Before device onboard, the rules will have the IP as empty string. This should not adversely affect us because we do not run tests before the device in onboarded. After the device is onboarded and rebooted, the port map rules will have valid IP addresses. We already have a reboot in the beginning of tests/workflow that helps setup the port maps properly in this case.
With virtual box, unfortunately we cannot just tell the destination port for packet while configuring host port map rules. It will require the destination IP address for configuring port maps. It's easier with qemu but a bit of inconvenient with vbox
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.
Sure, but Eden is not only about the workflow. Ok, if you think, it is difficult to workaround empty ipMap here, we can think about it in next PRs.
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 point was that it did not make any difference if we handled the empty ip map or not. It would still require a reboot before the port maps can take effect. Do you have something that proves contrary?
| log.Errorf("Parsing %s to Integer value failed", v) | ||
| break | ||
| } | ||
| guestPort += 10 |
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.
We have default for this. And the same point for another parts.
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 discussion above applies to this also.
|
|
||
| {{$osruntime := EdenOSRuntime "nan"}} | ||
|
|
||
| {{if (eq $osruntime "linux")}} |
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.
If I correctly understood, we rely here on network environment generated with qemu/VirualBox. But we have tests on GCP for now.
Can you please add here checks similar with checks in the workflow
| {{if or (eq $devmodel "ZedVirtual-4G") (eq $devmodel "VBox") (eq $devmodel "parallels") }} |
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.
Done. Pushed the changes.
|
@gkodali-zededa Please consider this ticket https://www.virtualbox.org/ticket/20175. We still have problems running on macOS with VirtualBox. Fix will reach the release version soon, but not there yet. |
Signed-off-by: GopiKrishna Kodali <gkodali@zededa.com>
Petr gave me the URL for fixed build and I currently use that build on mac. Thanks for letting me know. |
giggsoff
left a comment
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.
Thanks for the edits and your patience!
Can we merge this now? |

Needs to be seen how parallels can be made to do the same.