-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: describe the VPN #114
Conversation
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.
Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @dopiera)
arch-snapshot/arch.md
line 650 at r1 (raw file):
- WASM images ### VPN
I think this chapter lacks description how Requestor interacts with VPN.
And it is very cumbersome in current implementation, since he can't just use normal network interface.
SDK needs to make strange things like implementing proxy that talks with yagna through WebSocket. It looks like something to improve
arch-snapshot/arch.md
line 656 at r1 (raw file):
This component provides that functionality. The VPN operates as a typical layer 3 VPN and was designed to meet the following
I'm not sure about this.
From technical perspective VPN works on Ethernet frames levels. This is only due to smoltcp stack being not able to work on IP layer (and newer version can), and I would be glad to change that, but this makes this sentence problematic.
But we could say that this implementation and functionally our VPN behaves like layer 3 VPN.
But this would be problematic as well. From VM perspective it looks like this. But from Requestor perspective you can't use VPN in transparent way. For Requestor it would be rather application layer of OSI model or something like that.
Code quote:
typical layer 3 VPN
arch-snapshot/arch.md
line 677 at r1 (raw file):
2. The hypervisor forwards traffic to/from the virtualized Ethernet device to ExeUnit running on the host OS. 3. The ExeUnit tunnels traffic to/from other ExeUnits responsible for different
Somewhere should be mentioned that VPN packets are tunneled through GSB. This is potentially problematic, because of additional operations that needs to be performed and may potentially lead to worse performance.
arch-snapshot/arch.md
line 683 at r1 (raw file):
The guest VM is run using [qemu](https://www.qemu.org/). Qemu offers the `-netdev socket` option, which creates a virtual Ethernet device in the guest OS
It looks like very quemu specific thing. I would create separate chapters about VM examples
arch-snapshot/arch.md
line 704 at r1 (raw file):
fulfilled: - Every VM, along with the Requestor (which is always part of the VPN), must be
Requestor creates the network, but he doesn't have to assign IP address to himself, so doesn't have to be part of the VPN network. Providers could talk only between themselves.
arch-snapshot/arch.md
line 727 at r1 (raw file):
OS. This is achieved via qemu’s `-chardev` argument, which emulates a character device in the guest VM. Essentially, this device acts as a bidirectional pipe to a designated UNIX socket on the host OS. This mechanism facilitates an RPC
This looks like part of VM description. Here could be mentioning that there is a communication channel and link to description in ExeUnit Supervisor and VMs.
Controlling VPN is only one aspect of the interface for controlling ExeUnit Runtimes.
In general I would organize VPN description into part about generic mechanisms and examples or specifics about VM implementation
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.
Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @nieznanysprawiciel)
arch-snapshot/arch.md
line 650 at r1 (raw file):
Previously, nieznanysprawiciel wrote…
I think this chapter lacks description how Requestor interacts with VPN.
And it is very cumbersome in current implementation, since he can't just use normal network interface.
SDK needs to make strange things like implementing proxy that talks with yagna through WebSocket. It looks like something to improve
Added, PTAL.
arch-snapshot/arch.md
line 656 at r1 (raw file):
Previously, nieznanysprawiciel wrote…
I'm not sure about this.
From technical perspective VPN works on Ethernet frames levels. This is only due to smoltcp stack being not able to work on IP layer (and newer version can), and I would be glad to change that, but this makes this sentence problematic.But we could say that this implementation and functionally our VPN behaves like layer 3 VPN.
But this would be problematic as well. From VM perspective it looks like this. But from Requestor perspective you can't use VPN in transparent way. For Requestor it would be rather application layer of OSI model or something like that.
Here's the reasoning behind my conclusion:
- The VPN only tunnels IP packets and a only a narrow subset of ARP packets (only those necessary for IP communication). (https://github.com/golemfactory/yagna/blob/3d584097a0b2759d0184d9e89ea0756db1779bbb/exe-unit/src/network/vpn.rs#L285)
- This suggests it does not operate at Layer 2.
- Since it can tunnel any IP packet, this points to Layer 3 functionality.
Additionally, the VPN appears to use IP addresses for routing rather than MAC addresses for switching, which further supports the idea that it's working at Layer 3.
While I understand that we're forwarding entire Ethernet frames, this seems to be more of an implementation convenience rather than a core indication of Layer 2 functionality. For instance, in the ya-runtime-vm example, the MAC addresses are synthesized, indicating they aren't critical from the VPN's perspective.
Regarding the suggestion of a higher-level OSI model: I’m curious why a client implementation (i.e., the Requestor) would influence the classification of this VPN. The ya-vpn-connector exposes a network interface and uses TUN, which is Layer 3. BTW it also synthesizes MAC addresses.
arch-snapshot/arch.md
line 677 at r1 (raw file):
Previously, nieznanysprawiciel wrote…
Somewhere should be mentioned that VPN packets are tunneled through GSB. This is potentially problematic, because of additional operations that needs to be performed and may potentially lead to worse performance.
Added below
arch-snapshot/arch.md
line 683 at r1 (raw file):
Previously, nieznanysprawiciel wrote…
It looks like very quemu specific thing. I would create separate chapters about VM examples
It is indeed. I've moved the VM-specific stuff to the end of the section.
Given that the VPN makes no sense without the VMs, I'm not sure what meaningful generalization there if VMs are too specific.
arch-snapshot/arch.md
line 704 at r1 (raw file):
Previously, nieznanysprawiciel wrote…
Requestor creates the network, but he doesn't have to assign IP address to himself, so doesn't have to be part of the VPN network. Providers could talk only between themselves.
fixed
arch-snapshot/arch.md
line 727 at r1 (raw file):
Previously, nieznanysprawiciel wrote…
This looks like part of VM description. Here could be mentioning that there is a communication channel and link to description in ExeUnit Supervisor and VMs.
Controlling VPN is only one aspect of the interface for controlling ExeUnit Runtimes.In general I would organize VPN description into part about generic mechanisms and examples or specifics about VM implementation
This is a good point, let me rebase this on top of #116 in order to ensure that what I want to link to is available. EDIT - this has not yet been described - keeping this comment open until it is.
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.
Reviewable status: 0 of 1 files reviewed, 6 unresolved discussions (waiting on @dopiera and @nieznanysprawiciel)
arch-snapshot/arch.md
line 727 at r1 (raw file):
Previously, dopiera (Marek Dopiera) wrote…
This is a good point, let me rebase this on top of #116 in order to ensure that what I want to link to is available. EDIT - this has not yet been described - keeping this comment open until it is.
Fixed, PTAL
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.
Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @dopiera)
arch-snapshot/arch.md
line 683 at r1 (raw file):
Previously, dopiera (Marek Dopiera) wrote…
It is indeed. I've moved the VM-specific stuff to the end of the section.
Given that the VPN makes no sense without the VMs, I'm not sure what meaningful generalization there if VMs are too specific.
For example outbound-gateway: https://github.com/golemfactory/ya-runtime-outbound-gateway which was used in prototype of VPN routing traffic through golem nodes
arch-snapshot/arch.md
line 2374 at r3 (raw file):
The VM runtime allows Requestors to spin up virtual machines (VMs). To enable these VMs to act as a cluster, a VPN is essential for securely connecting them.
I forgot to comment earlier: securely
can be misleading. VPN doesn't encrypt traffic by itself. It relies on networking layer to do it. And currently there is no encryption.
arch-snapshot/arch.md
line 2450 at r3 (raw file):
unreliable channels
I changed terminology to transport type.
But I think you can leave it as is.
Maybe you could add link to the description?
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.
Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @dopiera)
arch-snapshot/arch.md
line 2422 at r3 (raw file):
The Requestor Agent acts as the source of truth for the IP configuration of every VM in the VPN. SDKs simplify this process for users—they specify the
We don't have interaction with artifacts described like in other parts of document. For example when describing Requestor interaction we don't say that SDKs provides user-friendly abstraction.
But in general I'm ok with omitting this.
This description is good enough
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.
Reviewable status: 0 of 1 files reviewed, 4 unresolved discussions (waiting on @dopiera and @nieznanysprawiciel)
arch-snapshot/arch.md
line 683 at r1 (raw file):
Previously, nieznanysprawiciel wrote…
For example outbound-gateway: https://github.com/golemfactory/ya-runtime-outbound-gateway which was used in prototype of VPN routing traffic through golem nodes
I didn't know about this one - added to the list of clients.
Anyway I think after reorganizing the content from this PR your comment has been addressed, no?
arch-snapshot/arch.md
line 2374 at r3 (raw file):
Previously, nieznanysprawiciel wrote…
I forgot to comment earlier:
securely
can be misleading. VPN doesn't encrypt traffic by itself. It relies on networking layer to do it. And currently there is no encryption.
I've added a sentence to the "Data Plane Internals" section to make it clear.
arch-snapshot/arch.md
line 2422 at r3 (raw file):
Previously, nieznanysprawiciel wrote…
We don't have interaction with artifacts described like in other parts of document. For example when describing Requestor interaction we don't say that SDKs provides user-friendly abstraction.
But in general I'm ok with omitting this.
This description is good enough
ACK
arch-snapshot/arch.md
line 2450 at r3 (raw file):
Previously, nieznanysprawiciel wrote…
unreliable channels
I changed terminology to transport type.
But I think you can leave it as is.
Maybe you could add link to the description?
Fixed, PTAL.
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.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dopiera)
This change is