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

Added initial prototype for rathole #8632

Merged
merged 113 commits into from
Jun 30, 2024
Merged

Conversation

madhavajay
Copy link
Collaborator

Description

Please include a summary of the change, the motivation, and any additional context that will help others understand your PR. If it closes one or more open issues, please tag them as described here.

Affected Dependencies

List any dependencies that are required for this change.

How has this been tested?

  • Describe the tests that you ran to verify your changes.
  • Provide instructions so we can reproduce.
  • List any relevant details for your test configuration.

Checklist

shubham3121 and others added 24 commits April 15, 2024 17:40
- added a tomlreaderwriter to read/write with locks
- added a RatholeClientToml Reader/Writer
- define a RatholeServerTole reader/writer
- integrate nginx builder with toml manager
- add requirements.txt
- update rathole dockerfile

Co-authored-by: Khoa Nguyen <[email protected]>
- fix imports in rathole utils
- add rathole loadbalancer to traefik
@shubham3121 shubham3121 self-assigned this Jun 27, 2024
@shubham3121 shubham3121 requested a review from yashgorana June 27, 2024 07:15
@yashgorana yashgorana changed the title WIP - Added initial prototype for rathole Added initial prototype for rathole Jun 27, 2024
RUN apt update && apt install -y git
RUN git clone -b v${RATHOLE_VERSION} https://github.com/rapiz1/rathole

WORKDIR /rathole
Copy link
Member

@shubham3121 shubham3121 Jun 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted to compiling rathole, as it current image on dockerhub doesn't have binaries for arm


# Copy configuration files
copy_config() {
cp -L -r -f /conf/* conf/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copies the mounted data from rathole configmap to the respective working directory.


url = self.url

if self.rathole_token:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If rathole token is part of connection, then all GET, POST, PUT calls made via the internal proxy url with Hostname param as Node name.

url.url_path, host=url.host_or_ip
)
else:
blob_url = api.connection.stream_via(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stream download data if connection is via proxy client.

yield chunk

@router.put("/stream/{peer_uid}/{url_path}/", name="stream")
async def stream_upload(peer_uid: str, url_path: str, request: Request) -> Response:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API endpoint to stream download Blob url from the domain node to the end user via the gateway node.

@@ -615,8 +664,7 @@ def delete_route_on_peer(
self,
context: AuthedServiceContext,
peer: NodePeer,
route: NodeRoute | None = None,
route_id: UID | None = None,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecated updation or Deletion of node routes using route id, since route id was not unique and consistent during creation of node routes.

Copy link
Contributor

@yashgorana yashgorana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just need to review packages/syft/src/syft/service/network/association_request.py

packages/grid/devspace.yaml Outdated Show resolved Hide resolved
packages/grid/devspace.yaml Outdated Show resolved Hide resolved
packages/syft/src/syft/util/util.py Outdated Show resolved Hide resolved
@@ -138,6 +138,10 @@ def base_url(self) -> str:
def base_url_no_port(self) -> str:
return f"{self.protocol}://{self.host_or_ip}"

@property
def url_no_protocol(self) -> str:
return f"{self.host_or_ip}:{self.port}{self.path}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will self.path always have a / in front of it? if we can't guarantee it then we must explicitly add / in between with a self.path.lstrip('/')

packages/syft/src/syft/client/client.py Outdated Show resolved Hide resolved
packages/grid/helm/syft/values.yaml Outdated Show resolved Hide resolved
packages/grid/rathole/start.sh Show resolved Hide resolved
@yashgorana
Copy link
Contributor

Also test_delete_route_on_peer seem flaky.

@yashgorana
Copy link
Contributor

I made some more changes to helm charts & devspace

  • rathole will be build only with profile gateway or domain-tunnel
  • Refactored some dev values files helm/examples/dev to make things a bit cleaner

Tests will always run with domain & gateway both having rathole.

@yashgorana yashgorana enabled auto-merge June 30, 2024 12:05
@yashgorana yashgorana merged commit 9939108 into OpenMined:dev Jun 30, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants